fix(oauth): open OAuth URL on Windows without cmd's & truncation#312
Conversation
## Problem
Naveen hit "Authentication Error / error_code: unknown_error" on
OpenAI's OAuth page when running `forge init` → "Login with
ChatGPT" on Windows. macOS worked fine.
Root cause is the Windows browser launcher:
case "windows":
cmd = exec.Command("cmd", "/c", "start", url)
`cmd /c start <url>` interprets every unquoted `&` in the URL as
the shell "AND" separator. The OAuth authorize URL Forge builds
carries eight query parameters glued with `&`:
?response_type=code
&client_id=app_EMoamEEZ73f0CkXaXp7hrann
&redirect_uri=http%3A%2F%2Flocalhost%3A1455%2Fauth%2Fcallback
&scope=openid+profile+email+offline_access
&state=<random>
&code_challenge=<base64url>
&code_challenge_method=S256
&id_token_add_organizations=true
&codex_cli_simplified_flow=true
The browser only receives everything up to the first `&`:
https://auth.openai.com/oauth/authorize?response_type=code
Everything after (client_id / redirect_uri / PKCE / state / scope)
is stripped and interpreted by cmd as separate commands that
silently error. OpenAI's auth server rejects the incomplete request
with the generic `unknown_error` code — no clue what actually
happened. macOS `open` and Linux `xdg-open` treat the URL as one
argument and are unaffected.
## Fix
Switch the Windows branch to `rundll32 url.dll,FileProtocolHandler`
— the Windows shell API that opens URLs via the registered protocol
handler without going through cmd's parser. Robust across cmd.exe,
PowerShell, and Windows Terminal; used by many Go projects for
exactly this reason (Docker Desktop, GitHub CLI, browser-launcher
libraries).
## Tests
New `forge-core/llm/oauth/flow_test.go` with two pins that would
have caught this class of bug:
- `TestBuildAuthURL_MultipleParamsAreIntact` — asserts every
required OAuth 2.0 + PKCE param is present in the built authorize
URL, all `ExtraParams` from the provider config land verbatim,
and the URL carries ≥7 `&` separators. Any future URL-builder
refactor that drops a param (or a launcher that truncates at
the first `&`) is caught here.
- `TestOpenAIConfig_ClientIDAndScopes` — pins that OpenAI's Codex
client_id is set, that `offline_access` is in the scope list
(silent removal would kill refresh tokens ≈1h after login), and
that AuthURL/TokenURL are https + RedirectURI targets the 1455
callback port.
Both pass. Full `forge-core/llm/oauth` suite green; gofmt +
golangci-lint clean.
## Manual verification path
Since I can't test Windows locally, the code change is small enough
(one line + a doc comment) that a reviewer can validate the
behavior with:
# On Windows PowerShell / cmd, after checking out this branch:
forge init
# Choose Provider: OpenAI → Sign in with ChatGPT → browser opens
# to auth.openai.com with FULL URL (all 8 params visible) →
# consent screen appears → click Continue → redirects to
# localhost:1455/auth/callback → Forge saves token → done.
The `rundll32` invocation is standard on Windows since Windows XP;
no compatibility concern.
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review: open OAuth URL on Windows without cmd's & truncation
A precise, well-diagnosed external contribution. The root cause is correct — cmd /c start <url> treats & as the shell command separator, so the 9-param OpenAI authorize URL reached the browser as just ?response_type=code and OpenAI returned a generic unknown_error. The rundll32 url.dll,FileProtocolHandler fix opens the URL through the Windows shell API without cmd's parser. Root-cause writeup and doc comment are excellent. CI fully green, including both Windows builds.
Two things I verified that de-risk this entirely:
- The fix matches existing in-repo precedent.
forge-cli/cmd/mcp_browser.goalready uses the identicalrundll32 url.dll,FileProtocolHandleron its Windows branch — so this isn't just aligned with the de-factopkg/browserstandard, it matches a launcher the project already ships. Any "is rundll32 an acceptable LOLBin?" concern is already settled by the maintainers' own code. - Security posture improves. The URL is now a single
exec.Commandargument with no shell in the path — the cmd-parser surface is gone, strictly better thancmd /c start.
Finding 1 (Medium, completeness): the same broken pattern still lives in forge-ui/server.go:216
There are three near-identical platform-switch browser launchers: this OAuth flow (now fixed), mcp_browser.go (already correct), and forge-ui/server.go:216 — still exec.Command("cmd", "/c", "start", url) for the forge ui dashboard. Latent today (the dashboard URL has no & params), but the identical footgun: the moment that URL gains a param (?agent=x&tab=y), forge ui breaks on Windows exactly as OAuth did. Since this PR is the one establishing "we don't use cmd /c start," it's the natural place to either fold in the one-line forge-ui fix or file a follow-up. Best outcome: extract the three copies into one shared openBrowser(url) helper — the divergence is exactly how one copy stayed buggy while another was already fixed.
Finding 2 (Minor, test): the changed line has no direct test
The two new tests pin the URL builder and config, not the launcher — the actual line this PR changes (rundll32 vs cmd) is untested. The ≥7 & assertion is a reasonable proxy (proves the URL has the params that were being truncated), but nothing pins that the Windows branch preserves them through openBrowser. Extracting browserCommand(goos, url string) *exec.Cmd would make the selection table-testable (assert the windows case is rundll32 with the URL as a single un-split arg) — directly locking the fix against regression. Not blocking; just noting the fix itself rides on manual verification.
Finding 3 (Minor, validation): the Windows manual-verification box is unchecked — and it's the fix's target
The test plan's post-fix Windows and macOS-no-regression boxes are both unticked. CI's Windows builds only prove the tests compile/pass there, not that rundll32 launches the browser with the full URL (can't be CI-tested). Low-risk given the mcp_browser.go precedent already ships rundll32 in production, but a maintainer with a Windows box should still run forge init → ChatGPT login end-to-end (and confirm darwin is untouched) before merge.
What's good
- The
offline_accessscope pin is a genuinely valuable test (verified the scope is real atflow.go:32): silently dropping it would kill refresh tokens ~1h after login, a subtle delayed failure. Sharp instinct to guard, especially from a first-time contributor. - Root-cause analysis is exemplary — reproduced, traced to the exact line, explained why macOS/Linux are unaffected.
Verdict
Correct, low-risk, merge-ready. The one thing I'd want addressed is finding 1: the sibling cmd /c start in forge-ui/server.go should be fixed here or explicitly deferred to a follow-up (ideally consolidating the three launchers), since this PR is precisely the moment that pattern is being retired. Findings 2–3 are a testability nicety and a manual tick a maintainer owns. Nice contribution.
| cmd = exec.Command("xdg-open", url) | ||
| case "windows": | ||
| cmd = exec.Command("cmd", "/c", "start", url) | ||
| cmd = exec.Command("rundll32", "url.dll,FileProtocolHandler", url) |
There was a problem hiding this comment.
The fix is correct — and worth noting it matches forge-cli/cmd/mcp_browser.go, which already uses this exact rundll32 url.dll,FileProtocolHandler on its Windows branch. So this aligns with shipped in-repo precedent, not just external libraries.
One testability nicety (non-blocking): the changed line here is the actual fix, but neither new test exercises openBrowser — they pin the URL builder + config instead. Extracting browserCommand(goos, url string) *exec.Cmd and asserting the windows case is rundll32 with the URL as a single un-split argument would lock this line against a future regression, since a shell-truncation bug is invisible to a URL-shape test. See finding 2 in the review body.
…eview) Address the two actionable findings on #312: - Finding 1 (completeness): forge-ui/server.go still launched the dashboard via `cmd /c start <url>` — the same &-truncation footgun the OAuth flow just fixed, latent only because the dashboard URL has no query params yet. Switch it to `rundll32 url.dll,FileProtocolHandler`. - Finding 2 (test): extract a pure browserCommand(goos, url) in both flow.go and server.go and table-test the selection, asserting the windows case is rundll32 with the URL as a single un-split argument — locking the actual changed line, which a URL-shape test can't see. The third copy (forge-cli/cmd/mcp_browser.go) was already correct. Full 3-way consolidation into one cross-module helper is deferred (it spans forge-core/forge-cli/forge-ui and the os/exec-linking constraint noted in mcp_browser.go).
|
Addressed the review findings in 2fc379a: Finding 1 (completeness) — the third Finding 2 (test) — the changed line was untested. Extracted a pure Finding 3 (manual Windows validation). Still owned by a maintainer with a Windows box — CI’s Windows builds prove compile/pass, not that
|
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Re-review: fix commit 2fc379a — both actionable findings resolved
Verified the fix commit against the two findings I raised. Both addressed with the stronger option; CI fully green (all 6 builds incl. both Windows targets, Lint, Test, Integration).
Finding 1 (Medium, completeness) — ✅ resolved
forge-ui/server.go no longer uses cmd /c start; the Windows branch is now rundll32 url.dll,FileProtocolHandler, identical to the OAuth fix and the already-correct mcp_browser.go. The latent footgun (dashboard breaks the moment its URL gains a & param) is closed before it could bite — the right call, since this PR is the one retiring the pattern.
Finding 2 (Minor, test) — ✅ resolved, exactly as suggested
browserCommand(goos, url) *exec.Cmd is extracted in both flow.go and server.go (pure, no side effects; Start() split into openBrowser) and table-tested across darwin/linux/windows/plan9. The key assertion is the one that was missing: the URL rides as a single un-split trailing argument — precisely the invariant cmd /c start violated. The changed line is now locked against regression rather than riding on manual verification.
Finding 3 (Minor, manual Windows tick) — correctly untouched
Not code-addressable; the end-to-end forge init → ChatGPT check on a Windows box is a maintainer's to run before merge. mcp_browser.go already ships rundll32 in production, so the runtime behavior is de-risked.
Deferral is honest
Full 3-way consolidation into one cross-module helper is deferred with a stated reason (spans forge-core/forge-cli/forge-ui + the os/exec-linking constraint in mcp_browser.go). Reasonable — the two copies that mattered for this bug are now identical and both tested; a shared helper is a refactor, not part of this fix.
Verdict: does what it claims, nothing dangling, merge-ready.
| // any URL with multiple query params (the OAuth launcher in | ||
| // forge-core/llm/oauth was fixed the same way). Latent here today — the | ||
| // dashboard URL has no params — but a footgun the moment one is added. | ||
| func browserCommand(goos, url string) *exec.Cmd { |
There was a problem hiding this comment.
Nice — this browserCommand is now byte-identical to the copy in forge-core/llm/oauth/flow.go, and both are table-tested the same way. That makes the deferred follow-up concrete: extracting these two (plus the already-correct mcp_browser.go) into one shared openBrowser/browserCommand helper would remove exactly the divergence that let one copy stay on cmd /c start while another was already fixed. Non-blocking; good candidate for the consolidation follow-up.
|
Filed the deferred consolidation as #347 — extract the three platform browser-launchers ( |
2 participants
Problem
forge init→ "Login with ChatGPT" on Windows lands on OpenAI's auth page with:macOS works fine. Repro'd by Naveen on a fresh Windows install.
Root cause
forge-core/llm/oauth/flow.go:155-156:cmd /c start <url>interprets every unquoted&in the URL as the shell "AND" separator. The OAuth authorize URL Forge builds glues eight query parameters with&:What actually reaches the browser on Windows: only
?response_type=code. Every param after the first&is stripped and interpreted by cmd as separate commands that silently error. OpenAI's auth server rejects the incomplete request with the genericunknown_error— no server-side hint what went wrong. macOSopenand Linuxxdg-opentreat the URL as one atomic argument and are unaffected.Fix
Switch the Windows branch to
rundll32 url.dll,FileProtocolHandler— the Windows shell API that opens URLs via the registered protocol handler without going through cmd's parser. Robust across cmd.exe, PowerShell, and Windows Terminal. Standard Windows API since XP; used by Docker Desktop, GitHub CLI, and every mature Go browser-launcher library for exactly this reason.Tests
New
forge-core/llm/oauth/flow_test.gowith two pins that would have caught this class of bug:TestBuildAuthURL_MultipleParamsAreIntact— asserts every required OAuth 2.0 + PKCE param is present in the built authorize URL,ExtraParamsland verbatim, and the URL carries ≥7&separators. Catches any URL-builder or launcher regression that drops params.TestOpenAIConfig_ClientIDAndScopes— pins OpenAI's Codex client_id, verifiesoffline_accessis in the scope list (silent removal would kill refresh tokens ≈1h after login), and that the endpoints are https + the redirect URI targets the 1455 callback port.Both pass. Full
forge-core/llm/oauthsuite green;gofmt+golangci-lintclean.Manual verification path (Windows)
Reviewer with a Windows box, after checking out this branch:
Pre-fix: browser lands on the auth error page. Post-fix: consent flow completes.
Test plan
go test ./forge-core/llm/oauth/...— green (macOS)golangci-lint run ./forge-core/...— 0 issuesforge initon a Windows machine completes the ChatGPT OAuth flow end-to-endforge initon macOS still works (no regression on the darwin branch)