fix: MS Teams OAuth on Windows and browser.cdpUrl security redaction#68077
fix: MS Teams OAuth on Windows and browser.cdpUrl security redaction#68077nightq wants to merge 1 commit intoopenclaw:mainfrom
Conversation
Fixes openclaw#67738 Root cause: explorer.exe <url> on Windows delegates to the running Explorer shell and exits with code 1, causing OAuth promise rejection even though the browser opened correctly. Fix: Use cmd /c start "" URL instead, which exits 0 on success. Also updates test assertions to handle all three platforms (darwin, win32, linux). Additionally adds .cdpUrl to sensitive URL config paths for security redaction.
Greptile SummaryThis PR fixes two issues: it adds a proper Windows code path to Both fixes are correct and well-scoped. The Confidence Score: 5/5Safe to merge — both fixes are correct, minimal, and well-tested for non-Windows platforms. No P0 or P1 issues. The single P2 comment flags that the updated test mirrors production logic rather than independently verifying each platform branch, so the new Windows path is untested on Linux/macOS CI — a test-coverage concern, not a correctness issue. The code changes themselves are sound. extensions/msteams/src/setup-surface.test.ts — the platform-conditional test approach means the win32 branch is never exercised on Linux/macOS CI. Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/setup-surface.test.ts
Line: 70-81
Comment:
**Test mirrors production logic — Windows branch untested on Linux/macOS CI**
The test derives `expectedCmd`/`expectedArgs` using the same `process.platform` conditionals as the production code, so the test is a near-tautology: it will pass on Linux (only verifying the `xdg-open` path) without ever executing the `cmd /c start "" url` branch. If the Windows args were typo'd in production, the test would still green on Linux CI.
Consider stubbing `process.platform` with `vi.spyOn(process, 'platform', 'get')` to drive each platform branch independently, e.g.:
```typescript
for (const [platform, expectedCmd, expectedArgs] of [
["darwin", "open", [url]],
["win32", "cmd", ["/c", "start", '""', url]],
["linux", "xdg-open", [url]],
] as const) {
vi.spyOn(process, "platform", "get").mockReturnValue(platform);
// ... spawn and assert
}
```
This would give real coverage of the new Windows code path regardless of where CI runs.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix: MS Teams OAuth on Windows uses cmd ..." | Re-trigger Greptile |
| const expectedCmd = | ||
| process.platform === "darwin" ? "open" : process.platform === "win32" ? "cmd" : "xdg-open"; | ||
| const expectedArgs = | ||
| process.platform === "darwin" | ||
| ? [url] | ||
| : process.platform === "win32" | ||
| ? ["/c", "start", '""', url] | ||
| : [url]; | ||
| expect(spawn).toHaveBeenCalledWith(expectedCmd, expectedArgs, { | ||
| stdio: "ignore", | ||
| shell: false, | ||
| }); |
There was a problem hiding this comment.
The test derives expectedCmd/expectedArgs using the same process.platform conditionals as the production code, so the test is a near-tautology: it will pass on Linux (only verifying the xdg-open path) without ever executing the cmd /c start "" url branch. If the Windows args were typo'd in production, the test would still green on Linux CI.
Consider stubbing process.platform with vi.spyOn(process, 'platform', 'get') to drive each platform branch independently, e.g.:
for (const [platform, expectedCmd, expectedArgs] of [
["darwin", "open", [url]],
["win32", "cmd", ["/c", "start", '""', url]],
["linux", "xdg-open", [url]],
] as const) {
vi.spyOn(process, "platform", "get").mockReturnValue(platform);
// ... spawn and assert
}This would give real coverage of the new Windows code path regardless of where CI runs.
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/setup-surface.test.ts
Line: 70-81
Comment:
**Test mirrors production logic — Windows branch untested on Linux/macOS CI**
The test derives `expectedCmd`/`expectedArgs` using the same `process.platform` conditionals as the production code, so the test is a near-tautology: it will pass on Linux (only verifying the `xdg-open` path) without ever executing the `cmd /c start "" url` branch. If the Windows args were typo'd in production, the test would still green on Linux CI.
Consider stubbing `process.platform` with `vi.spyOn(process, 'platform', 'get')` to drive each platform branch independently, e.g.:
```typescript
for (const [platform, expectedCmd, expectedArgs] of [
["darwin", "open", [url]],
["win32", "cmd", ["/c", "start", '""', url]],
["linux", "xdg-open", [url]],
] as const) {
vi.spyOn(process, "platform", "get").mockReturnValue(platform);
// ... spawn and assert
}
```
This would give real coverage of the new Windows code path regardless of where CI runs.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8d68107e8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| process.platform === "darwin" | ||
| ? ["open", [url]] | ||
| : process.platform === "win32" | ||
| ? ["cmd", ["/c", "start", '""', url]] |
There was a problem hiding this comment.
Quote URL when calling
cmd /c start on Windows
On Windows this passes the OAuth URL to cmd as an unquoted command argument (["/c", "start", '""', url]), but MS Teams auth URLs are built with multiple query params (...?...&...) in buildMSTeamsAuthUrl (extensions/msteams/src/oauth.flow.ts:47). In cmd, unquoted metacharacters like & are command separators, so the URL can be truncated and extra tokens are parsed as separate commands, which breaks the browser-open flow and can execute unintended command fragments from the URL text. The URL should be safely quoted/escaped before handing it to cmd start.
Useful? React with 👍 / 👎.
|
Codex deep review: this should be narrowed before merge. Current state on
The Windows launcher fix here is the right direction: Best mergeable shape:
I would not close #67659 until that narrow current patch lands. |
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Close as superseded. The CDP redaction half of this PR is already on current main, while the remaining MS Teams Windows OAuth launcher bug is tracked by the focused open issue/PR pair #67659/#67660; this branch also introduces a concrete Windows So I’m closing this here and keeping the remaining discussion on the canonical linked item. Review detailsBest possible solution: Close this PR and keep the remaining work centered on #67659/#67660 or an equivalent narrow replacement. The accepted fix should be MS Teams-only, avoid routing OAuth URLs through Do we have a high-confidence way to reproduce the issue? Yes. The current-main bug is reproducible by stubbing Is this the best way to solve the issue? No. This PR mixes an already-shipped redaction fix with a Windows launcher change that conflicts with prior OpenClaw hardening against Security review: Security review needs attention: The PR introduces a concrete Windows command-parsing concern by sending OAuth URLs through
What I checked:
Likely related people:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 7969f1f07ccc. |
Summary
This PR fixes two issues reported in #67738:
Root Cause
explorer.exe <url>delegates to the running Explorer shell and exits with code 1, causing the OAuth promise to reject even though the browser opened correctly..cdpUrlconfig paths (Chrome DevTools Protocol URLs) can contain sensitive credentials but were not being redacted.Fix
cmd /c start "" URLinstead ofexplorer.exe, which exits 0 on success..cdpUrlto the list of sensitive URL config paths..cdpUrlpath.Test Plan
Closes #67738