From 7c36ea5c4ed99867b3e677593c4569da99d52184 Mon Sep 17 00:00:00 2001 From: naveen-kurra Date: Wed, 15 Jul 2026 11:26:15 -0400 Subject: [PATCH 1/2] fix(oauth): open OAuth URL on Windows without cmd's `&` truncation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 ` 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= &code_challenge= &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. --- forge-core/llm/oauth/flow.go | 11 +++- forge-core/llm/oauth/flow_test.go | 83 +++++++++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) create mode 100644 forge-core/llm/oauth/flow_test.go diff --git a/forge-core/llm/oauth/flow.go b/forge-core/llm/oauth/flow.go index edf16c30..41b988e7 100644 --- a/forge-core/llm/oauth/flow.go +++ b/forge-core/llm/oauth/flow.go @@ -145,6 +145,15 @@ func (f *Flow) buildAuthURL(pkce *PKCEParams, state string) string { } // openBrowser opens the given URL in the default browser. +// +// Windows note: `cmd /c start ` treats `&` as the shell "AND" +// separator, truncating any URL that has more than one query +// parameter — the OpenAI OAuth authorize URL has eight, so the +// browser opens with only `?response_type=code` and OpenAI's auth +// server returns a generic `unknown_error`. `rundll32 +// url.dll,FileProtocolHandler` opens URLs through the Windows shell +// API without invoking cmd's parser, so `&` in query strings stays +// intact across Windows Terminal / PowerShell / cmd.exe. func openBrowser(url string) error { var cmd *exec.Cmd switch runtime.GOOS { @@ -153,7 +162,7 @@ func openBrowser(url string) error { case "linux": cmd = exec.Command("xdg-open", url) case "windows": - cmd = exec.Command("cmd", "/c", "start", url) + cmd = exec.Command("rundll32", "url.dll,FileProtocolHandler", url) default: return fmt.Errorf("unsupported platform: %s", runtime.GOOS) } diff --git a/forge-core/llm/oauth/flow_test.go b/forge-core/llm/oauth/flow_test.go new file mode 100644 index 00000000..68f12602 --- /dev/null +++ b/forge-core/llm/oauth/flow_test.go @@ -0,0 +1,83 @@ +package oauth + +import ( + "net/url" + "strings" + "testing" +) + +// TestBuildAuthURL_MultipleParamsAreIntact pins the invariant that +// the built authorize URL carries every required OAuth 2.0 param +// (client_id, redirect_uri, scope, state, code_challenge, +// code_challenge_method) plus any provider-declared extras. The +// value is that a Windows regression where the URL was truncated at +// the first `&` (see openBrowser docs) presents to the user as a +// generic OpenAI "authentication error" with no obvious server-side +// pointer. Pinning the URL shape here catches URL-builder changes +// that would strip params; pairing with the openBrowser fix protects +// the launcher path. +func TestBuildAuthURL_MultipleParamsAreIntact(t *testing.T) { + cfg := OpenAIConfig() + f := NewFlow(cfg) + authURL := f.buildAuthURL(&PKCEParams{ + Verifier: "verifier-fixture", + Challenge: "challenge-fixture", + Method: "S256", + }, "state-fixture") + + u, err := url.Parse(authURL) + if err != nil { + t.Fatalf("parse authURL: %v", err) + } + if u.Scheme+"://"+u.Host+u.Path != cfg.AuthURL { + t.Errorf("scheme/host/path mismatch: got %q, want %q", + u.Scheme+"://"+u.Host+u.Path, cfg.AuthURL) + } + q := u.Query() + // Required OAuth 2.0 + PKCE fields. + for _, key := range []string{ + "response_type", "client_id", "redirect_uri", + "scope", "state", "code_challenge", "code_challenge_method", + } { + if q.Get(key) == "" { + t.Errorf("required OAuth param %q missing from authorize URL", key) + } + } + // The provider's extra params (OpenAI's Codex flow flags) must + // also be present; losing them silently switches OpenAI to a + // different consent variant. + for k, v := range cfg.ExtraParams { + if got := q.Get(k); got != v { + t.Errorf("extra param %q: got %q, want %q", k, got, v) + } + } + // The URL must contain at least seven `&` separators — the + // count OpenAI needs to render the consent screen. If it drops + // to zero (as it does when a Windows launcher's shell truncates + // at the first `&`), the auth server returns "unknown_error". + if amps := strings.Count(authURL, "&"); amps < 7 { + t.Errorf("expected ≥7 `&` separators (multi-param URL); got %d — URL: %s", + amps, authURL) + } +} + +// TestOpenAIConfig_ClientIDAndScopes pins the exact values Forge +// registers with OpenAI's OAuth. Rotating the ClientID or dropping +// `offline_access` from the scopes is a silent behavior change — +// tokens stop refreshing, sessions die after ~1h, and the failure +// mode is subtle. Test guards both. +func TestOpenAIConfig_ClientIDAndScopes(t *testing.T) { + c := OpenAIConfig() + if c.ClientID == "" { + t.Fatal("ClientID must be set") + } + if !strings.Contains(c.Scopes, "offline_access") { + t.Error("Scopes should include `offline_access` for refresh-token support") + } + if !strings.HasPrefix(c.AuthURL, "https://") || !strings.HasPrefix(c.TokenURL, "https://") { + t.Error("Auth/Token URLs must be https") + } + if c.RedirectURI == "" || !strings.Contains(c.RedirectURI, "1455") { + t.Errorf("RedirectURI should bind to the callback server's port 1455; got %q", c.RedirectURI) + } +} From 2fc379a3b590a4369be9c635ad91f55a191a565f Mon Sep 17 00:00:00 2001 From: MK Date: Mon, 20 Jul 2026 14:18:32 -0400 Subject: [PATCH 2/2] fix(ui): retire cmd /c start; table-test the browser launcher (#312 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the two actionable findings on #312: - Finding 1 (completeness): forge-ui/server.go still launched the dashboard via `cmd /c start ` — 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). --- forge-core/llm/oauth/flow.go | 39 +++++++++++++++++---------- forge-core/llm/oauth/flow_test.go | 43 +++++++++++++++++++++++++++++ forge-ui/server.go | 28 ++++++++++++++----- forge-ui/server_browser_test.go | 45 +++++++++++++++++++++++++++++++ 4 files changed, 134 insertions(+), 21 deletions(-) create mode 100644 forge-ui/server_browser_test.go diff --git a/forge-core/llm/oauth/flow.go b/forge-core/llm/oauth/flow.go index 41b988e7..c002fbc0 100644 --- a/forge-core/llm/oauth/flow.go +++ b/forge-core/llm/oauth/flow.go @@ -145,26 +145,37 @@ func (f *Flow) buildAuthURL(pkce *PKCEParams, state string) string { } // openBrowser opens the given URL in the default browser. +func openBrowser(url string) error { + cmd := browserCommand(runtime.GOOS, url) + if cmd == nil { + return fmt.Errorf("unsupported platform: %s", runtime.GOOS) + } + return cmd.Start() +} + +// browserCommand builds (but does not start) the platform launcher for +// url, so the selection table is unit-testable — a shell-truncation +// regression is invisible to a URL-shape test. Returns nil for an +// unsupported GOOS. The url is always passed as a single argument, +// never through a shell, so `&` in query strings survives. // // Windows note: `cmd /c start ` treats `&` as the shell "AND" // separator, truncating any URL that has more than one query -// parameter — the OpenAI OAuth authorize URL has eight, so the -// browser opens with only `?response_type=code` and OpenAI's auth -// server returns a generic `unknown_error`. `rundll32 -// url.dll,FileProtocolHandler` opens URLs through the Windows shell -// API without invoking cmd's parser, so `&` in query strings stays -// intact across Windows Terminal / PowerShell / cmd.exe. -func openBrowser(url string) error { - var cmd *exec.Cmd - switch runtime.GOOS { +// parameter — the OpenAI OAuth authorize URL has eight, so the browser +// opens with only `?response_type=code` and OpenAI's auth server +// returns a generic `unknown_error`. `rundll32 +// url.dll,FileProtocolHandler` opens URLs through the Windows shell API +// without invoking cmd's parser, so `&` stays intact across Windows +// Terminal / PowerShell / cmd.exe. +func browserCommand(goos, url string) *exec.Cmd { + switch goos { case "darwin": - cmd = exec.Command("open", url) + return exec.Command("open", url) case "linux": - cmd = exec.Command("xdg-open", url) + return exec.Command("xdg-open", url) case "windows": - cmd = exec.Command("rundll32", "url.dll,FileProtocolHandler", url) + return exec.Command("rundll32", "url.dll,FileProtocolHandler", url) default: - return fmt.Errorf("unsupported platform: %s", runtime.GOOS) + return nil } - return cmd.Start() } diff --git a/forge-core/llm/oauth/flow_test.go b/forge-core/llm/oauth/flow_test.go index 68f12602..34d42fad 100644 --- a/forge-core/llm/oauth/flow_test.go +++ b/forge-core/llm/oauth/flow_test.go @@ -2,6 +2,7 @@ package oauth import ( "net/url" + "reflect" "strings" "testing" ) @@ -61,6 +62,48 @@ func TestBuildAuthURL_MultipleParamsAreIntact(t *testing.T) { } } +// TestBrowserCommand pins the platform→launcher selection — the exact +// line the Windows fix changes, which no URL-shape test can see (a +// shell truncation is invisible until the browser opens). It asserts +// each GOOS builds the expected argv AND that the multi-`&` URL rides +// as a single, un-split argument — the invariant `cmd /c start` +// violated on Windows by letting cmd's parser eat everything after the +// first `&`. +func TestBrowserCommand(t *testing.T) { + const multiParam = "https://auth.openai.com/authorize?response_type=code&client_id=x&scope=a+b&state=s" + cases := []struct { + goos string + args []string // full argv incl. arg0; nil = unsupported → nil cmd + }{ + {"darwin", []string{"open", multiParam}}, + {"linux", []string{"xdg-open", multiParam}}, + {"windows", []string{"rundll32", "url.dll,FileProtocolHandler", multiParam}}, + {"plan9", nil}, + } + for _, tc := range cases { + t.Run(tc.goos, func(t *testing.T) { + cmd := browserCommand(tc.goos, multiParam) + if tc.args == nil { + if cmd != nil { + t.Fatalf("unsupported %s must yield nil, got %v", tc.goos, cmd.Args) + } + return + } + if cmd == nil { + t.Fatalf("%s yielded nil command", tc.goos) + } + if !reflect.DeepEqual(cmd.Args, tc.args) { + t.Fatalf("%s argv = %v, want %v", tc.goos, cmd.Args, tc.args) + } + // The URL must survive as exactly one trailing argument — + // no shell, no splitting on `&`. + if last := cmd.Args[len(cmd.Args)-1]; last != multiParam { + t.Errorf("%s: URL arg mutated/split: got %q, want %q", tc.goos, last, multiParam) + } + }) + } +} + // TestOpenAIConfig_ClientIDAndScopes pins the exact values Forge // registers with OpenAI's OAuth. Rotating the ClientID or dropping // `offline_access` from the scopes is a silent behavior change — diff --git a/forge-ui/server.go b/forge-ui/server.go index 73a8b251..9c43684e 100644 --- a/forge-ui/server.go +++ b/forge-ui/server.go @@ -206,16 +206,30 @@ func corsMiddleware(next http.Handler) http.Handler { // openBrowser opens the default browser to the given URL. func openBrowser(url string) { - var cmd *exec.Cmd - switch runtime.GOOS { + if cmd := browserCommand(runtime.GOOS, url); cmd != nil { + _ = cmd.Start() + } +} + +// browserCommand builds (but does not start) the platform launcher for +// url, so the selection table is unit-testable. The url is always a +// single argument, never parsed by a shell. Returns nil for an +// unsupported GOOS. +// +// Windows uses `rundll32 url.dll,FileProtocolHandler` rather than +// `cmd /c start`: cmd treats `&` as a command separator and truncates +// 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 { + switch goos { case "darwin": - cmd = exec.Command("open", url) + return exec.Command("open", url) case "linux": - cmd = exec.Command("xdg-open", url) + return exec.Command("xdg-open", url) case "windows": - cmd = exec.Command("cmd", "/c", "start", url) + return exec.Command("rundll32", "url.dll,FileProtocolHandler", url) default: - return + return nil } - _ = cmd.Start() } diff --git a/forge-ui/server_browser_test.go b/forge-ui/server_browser_test.go new file mode 100644 index 00000000..48d115c0 --- /dev/null +++ b/forge-ui/server_browser_test.go @@ -0,0 +1,45 @@ +package forgeui + +import ( + "reflect" + "testing" +) + +// TestBrowserCommand pins the dashboard's platform→launcher selection. +// The Windows branch must use `rundll32 url.dll,FileProtocolHandler`, +// not `cmd /c start` — the latter lets cmd's parser split the URL on +// `&`, silently breaking the moment the dashboard URL gains a query +// param. Asserts the argv per GOOS and that the URL rides as a single, +// un-split trailing argument. +func TestBrowserCommand(t *testing.T) { + const multiParam = "http://localhost:8080/?agent=x&tab=logs" + cases := []struct { + goos string + args []string // full argv incl. arg0; nil = unsupported → nil cmd + }{ + {"darwin", []string{"open", multiParam}}, + {"linux", []string{"xdg-open", multiParam}}, + {"windows", []string{"rundll32", "url.dll,FileProtocolHandler", multiParam}}, + {"plan9", nil}, + } + for _, tc := range cases { + t.Run(tc.goos, func(t *testing.T) { + cmd := browserCommand(tc.goos, multiParam) + if tc.args == nil { + if cmd != nil { + t.Fatalf("unsupported %s must yield nil, got %v", tc.goos, cmd.Args) + } + return + } + if cmd == nil { + t.Fatalf("%s yielded nil command", tc.goos) + } + if !reflect.DeepEqual(cmd.Args, tc.args) { + t.Fatalf("%s argv = %v, want %v", tc.goos, cmd.Args, tc.args) + } + if last := cmd.Args[len(cmd.Args)-1]; last != multiParam { + t.Errorf("%s: URL arg mutated/split: got %q, want %q", tc.goos, last, multiParam) + } + }) + } +}