fix(oauth): open Windows browser without shell-string URL - #242
Conversation
Pass the OAuth authorization URL as a separate argv element to `cmd /c start` and drop windowsVerbatimArguments so quote-bearing URLs cannot break out of shell quoting on Windows. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0383e7bb7e
ℹ️ 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".
| // Pass the URL as a separate argv element so Node quotes it. | ||
| // Never use windowsVerbatimArguments with a single shell string — | ||
| // quote-bearing OAuth URLs could break out of `start` quoting. | ||
| const child = launch('cmd', ['/c', 'start', '""', url], { |
There was a problem hiding this comment.
For ordinary authorization URLs with multiple query parameters, such as ...?client_id=x&redirect_uri=..., Node's Windows argv serializer does not add quotes merely because an argument contains &, so cmd /c interprets the suffix as another command. The Microsoft cmd documentation explicitly requires metacharacters including & to be quoted or escaped. Thus this branch opens a truncated URL and may execute a server-controlled suffix; the new test only inspects the JavaScript argv rather than the command line parsed by cmd.exe. Use cmd-aware escaping or avoid the command shell.
AGENTS.md reference: AGENTS.md:L132-L132
Useful? React with 👍 / 👎.
|
|
||
| ### OAuth | ||
|
|
||
| - Launch Windows OAuth browser open via separate `cmd /c start` argv elements so quote-bearing authorization URLs cannot break out of shell quoting. (thanks @SebTardif) |
There was a problem hiding this comment.
This new entry is added not only under Unreleased but also under the already released 0.12.1, 0.12.0, and 0.11.0 sections (lines 57, 68, and 131). Those releases still contained the previous Windows launch implementation, so the changelog now incorrectly tells users that they include this fix; keep the entry only in the release that first contains the change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 6eb5322. The Windows openExternal entry is only under `[0.12.4] - Unreleased` now; removed the same line from the historical 0.12.1, 0.12.0, and 0.11.0 sections.
…ed only Remove the same entry from historical 0.12.1, 0.12.0, and 0.11.0 sections. Those releases still shipped the previous Windows launch path. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
Addressed the Codex CHANGELOG note in 6eb5322: the Windows OAuth browser-open entry is only under Unreleased now (removed from 0.12.1 / 0.12.0 / 0.11.0). |
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 12:41 PM ET / 16:41 UTC. ClawSweeper reviewWhat this changesThe PR changes the Windows OAuth browser opener from a Merge readinessThis PR remains necessary: current Likely related people: Priority: P1 Review scores
Verification
How this fits together
flowchart LR
A[OAuth provider metadata] --> B[Authorization URL]
B --> C[mcporter auth]
C --> D[Windows browser launcher]
D --> E[Default browser handler]
E --> F[OAuth sign-in]
F --> G[Local callback]
G --> H[Saved OAuth session]
Before merge
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land the shell-free launcher with its focused regression tests, retaining the hosted Windows callback evidence as the compatibility record for the browser-association change. Do we have a high-confidence way to reproduce the issue? Yes. Hosted Is this the best way to solve the issue? Yes. Removing AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e1689c3dec7c. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (23 earlier review cycles; latest 8 shown)
|
|
@clawsweeper re-review Clarified Real behavior proof: production argv shape and vitest spawn capture (quote-bearing URL stays one argv element; no windowsVerbatimArguments). |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
ClawSweeper P1: argv-split cmd /c start still passes the OAuth URL through cmd.exe, which parses & and other metacharacters as command syntax. Use rundll32 url.dll,FileProtocolHandler so the URL is a document path. Update focused openExternal tests for the shell-free launch shape.
|
@clawsweeper re-review Addressed the remaining P1 (cmd.exe command-interpreter boundary):
Live Windows browser open still not available in this environment; spawn shape is proven from production code on the updated head. |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
@clawsweeper re-review Proof body updated to match the shipped fix: Windows openExternal uses rundll32 FileProtocolHandler (not cmd.exe). Live spawn-spy JSON + vitest oauth-open-external (3 tests) included in the PR body. |
|
🦞👀 Command router queued. I will update this comment with the next step. |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
ClawSweeper flagged the Unreleased note as still describing cmd /c start after the shell-free rundll32 launcher landed.
|
@clawsweeper re-review Addressed the P2 changelog accuracy finding: Unreleased OAuth note now describes Windows desktop browser launch proof is still not available on this machine (macOS-only contributor environment). Spawn-argv boundary tests and live spawn-spy JSON remain in the PR body. Happy to add Windows terminal/browser evidence if a maintainer can run one smoke open, or accept mock spawn as source-boundary proof under merge option 2. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
rating: 🧂 unranked krab
Leave release notes to the release process per ClawSweeper review. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Removed the release-owned Unreleased OAuth CHANGELOG line for this Windows launcher change. Native Windows desktop OAuth browser smoke is still not available on this machine; argv-level proof remains in the PR body (rundll32 + separate URL arg). |
|
Native Windows proof is now green against exact PR head The hosted {"platform":"win32","pathname":"/callback","quoted":"\"value\"","first":"one","second":"two"}Proof run: https://github.com/openclaw/mcporter/actions/runs/30734361638 I also reviewed the implementation and tests. With this native end-to-end proof, I recommend LAND. |
clawsweeper
Bot
added
proof: sufficient
|
Landed as e1b12c0. Verification at exact head
Changelog entry (maintainer-owned) follows on main. Thanks @SebTardif for the careful iteration through the review rounds! |
…ifier 401 fix (#248) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
2 participants
What Problem This Solves
On Windows, OAuth browser open used
cmd.exewith the authorization URL inside a shell command string. A quote-bearing URL (from OAuth metadata or a malicious authorization server) can break out of quoting and run additionalcmdside effects whenmcporter authlaunches the browser.Evidence
Production launch argv (this branch: rundll32, not cmd)
Focused tests
Asserted:
rundll32+url.dll,FileProtocolHandler+ URL as separate argvcmd; no/c/starttokensBefore (main):
cmd /s /c start "" "${url}"withwindowsVerbatimArguments: true(URL in one shell string).Real behavior proof
cmd.exeshell-quoted command string.spawnfor platformwin32on branchfix/mcporter-audit-2291head05655b0.node --import tsxspawn spy above;pnpm exec vitest run tests/oauth-open-external.test.ts.rundll32with URL as a distinct argument; no command interpreter boundary.FileProtocolHandler).Summary
openExternaluses rundll32 FileProtocolHandlerRelated
Refs #135 — prior Windows OAuth
openExternalfailure (URL truncated at&undercmd.exe). That path was mitigated by quoting in #136; this PR removes thecmd.exeshell boundary entirely.