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) + } +}