fix(oauth): refuse redirects on credential POSTs (#729)

[gnanam1990]

Summary

Closes #729.

OAuth token requests validated only the initial endpoint, then let the supplied or default http.Client follow a 307/308 redirect that replays the POST body — authorization codes, PKCE verifiers, refresh tokens, client secrets, client IDs — to another origin (or an HTTPS→HTTP downgrade) that never passed endpoint validation.

Affected paths

• internal/oauth/flow.go PostToken — token exchange + refresh (and MCP OAuth login/refresh, which delegate to oauth.ExchangeCode/oauth.Refresh).
• internal/oauth/device.go RequestDeviceCode — the device authorization POST (client_id + client_secret).
• internal/oauth/device.go pollDeviceOnce — the device-token poll (device_code + client_secret).

Fix

New withoutRedirects(client) returns a shallow copy of the client whose CheckRedirect refuses every redirect (ErrUnsafeRedirect). Applied at all three credential POSTs before client.Do. Because the copy is per-request, the caller's client is never mutated, and a legitimate token endpoint that returns 200 is unaffected (real token endpoints don't redirect).

Scope

internal/oauth only. No config or API change. No behavior change for non-redirecting endpoints. No performance change expected.

Testing

• 5 regression tests (PostToken, ExchangeCode, Refresh, RequestDeviceCode, pollDeviceOnce) using a redirect trap: a credential endpoint that 307/308-redirects to an attacker server which counts hits. Each asserts the call returns ErrUnsafeRedirect and the attacker receives zero requests (the body was never replayed). All five were confirmed to fail without the fix (reverted the source, kept the tests).
• go test -race ./internal/oauth/... ./internal/mcp/... green. go build ./..., go vet ./..., gofmt, govulncheck, git diff HEAD --check all clean.
• Manual end-to-end through the real CLI. With a device endpoint that 307-redirects to an attacker, zero auth login <provider> --device now fails with refusing to follow a redirect from a credential endpoint (exit 1) and the attacker server receives 0 requests — whereas unpatched main replays the client_secret-bearing POST (attacker hit) and proceeds.

Related

Pairs with #511 (validate discovered endpoints) — the other half of the OAuth endpoint-safety hardening. Independent branch; no stacking.

Summary by CodeRabbit

• Security Improvements
  • OAuth credential-bearing requests now refuse temporary/permanent redirects to prevent sensitive form data from being replayed to unvalidated destinations.
  • Added a clear ErrUnsafeRedirect error surfaced when redirects are blocked for token exchange, refresh, device authorization, and device polling.
• Tests
  • Added coverage ensuring redirects are rejected and that no additional requests occur to confirm sensitive payloads are not replayed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 69a5cfe1-2d6b-4ede-ad0f-b063f29df62a

📥 Commits

Reviewing files that changed from the base of the PR and between a5c2e28 and 0919a84.

📒 Files selected for processing (5)

• internal/oauth/device.go
• internal/oauth/device_test.go
• internal/oauth/flow.go
• internal/oauth/flow_test.go
• internal/oauth/oauth.go

🚧 Files skipped from review as they are similar to previous changes (5)

• internal/oauth/oauth.go
• internal/oauth/device_test.go
• internal/oauth/flow_test.go
• internal/oauth/flow.go
• internal/oauth/device.go

---

Walkthrough

OAuth credential-bearing requests now reject redirects through a shared client wrapper. Token exchange, refresh, device authorization, and device polling return ErrUnsafeRedirect, with tests confirming redirect targets receive no replayed requests.

Changes

OAuth redirect safety

Layer / File(s)	Summary
Redirect policy contract internal/oauth/oauth.go, internal/oauth/flow.go	Adds ErrUnsafeRedirect and a withoutRedirects helper that clones clients and rejects redirects.
Credential endpoint wiring internal/oauth/flow.go, internal/oauth/device.go	Applies redirect refusal to token, device-authorization, and device-token polling requests.
Redirect replay validation internal/oauth/flow_test.go, internal/oauth/device_test.go	Tests 307/308 responses, ErrUnsafeRedirect, and zero requests reaching redirect targets.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OAuthClient
  participant OAuthEndpoint
  participant RedirectTarget
  OAuthClient->>OAuthEndpoint: Send credential-bearing POST
  OAuthEndpoint-->>OAuthClient: Return 307 or 308 redirect
  OAuthClient-->>RedirectTarget: Do not replay request
  OAuthClient-->>OAuthClient: Return ErrUnsafeRedirect

Loading

Possibly related PRs

• Gitlawb/zero#738: Also changes token redirect handling and redirect-safety tests.

Suggested reviewers: vasanthdev2004

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main security fix: refusing redirects on OAuth credential POSTs.
Linked Issues check	✅ Passed	The PR implements token-client redirect rejection and adds redirect tests for exchange, refresh, and device flows as requested.
Out of Scope Changes check	✅ Passed	The changes stay focused on OAuth redirect handling and test coverage with no obvious unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/oauth-token-redirect

---

_{Comment @coderabbitai help to get the list of available commands.}

[github-actions]

Zero automated PR review

Verdict: No blockers found

Blockers

• None found.

Validation

• [pass] Diff hygiene: git diff --check
• [pass] Tests: go test ./...
• [pass] Build: go run ./cmd/zero-release build
• [pass] Smoke build: go run ./cmd/zero-release smoke

Scope

Head: 0919a848d18d
Changed files (5): internal/oauth/device.go, internal/oauth/device_test.go, internal/oauth/flow.go, internal/oauth/flow_test.go, internal/oauth/oauth.go

This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality.

[anandh8x]

Reviewed the OAuth redirect hardening at 0919a84. The credential-bearing token exchange, refresh, device authorization, and device polling paths reject redirects without mutating the caller's HTTP client. Focused race tests for internal/oauth and internal/mcp pass, focused vet is clean, and CI is green. No blocking findings.

[Vasanthdev2004]

Approving.

withoutRedirects shallow-copies the client and sets CheckRedirect to return ErrUnsafeRedirect, applied at all three credential POSTs: PostToken (which backs ExchangeCode/Refresh and the MCP delegations), RequestDeviceCode, and pollDeviceOnce. Go treats a CheckRedirect error as fatal and returns before issuing the follow-up request, so a 307/308 never replays the form body (codes, PKCE verifiers, refresh tokens, secrets), and it also blocks an HTTPS to HTTP downgrade. The caller's client is left unmutated, so a caller-supplied client can't smuggle the redirect back in. The regression tests are convincing: each asserts errors.Is(err, ErrUnsafeRedirect) AND that the attacker server got zero hits, which is exactly the replay threat.

Two non-blocking notes:

• registerClient in internal/mcp/oauth.go:303 POSTs without the same guard. It's outside #729's scope (the request body carries no pre-existing credential, just client metadata), but the response carries a client_secret, so applying withoutRedirects there too would be good defense in depth. Fine as a follow-up.
• This overlaps #738, which patches only the token endpoint. #741 is the broader fix (adds device-code + poll and 5 tests), so I'd land this one and close #738 in its favor. Merge order is your / kevin's call.

CI green, oauth and mcp packages green locally.