fix(oauth): block redirects for credential posts#5980
Conversation
There was a problem hiding this comment.
Clean, contained security fix. Correct principle: fail-closed beats fail-open — credential-bearing POSTs never follow a redirect, because an OAuth token or dynamic-registration endpoint has no legitimate reason to 3xx, and forwarding an Authorization header or PKCE-bearing body to a Location target is a credential leak.
Things I checked
oauth-safe-fetch.ts:48—...(method === 'POST' && { maxRedirects: 0 })is valid JS: for GET the expression isfalseand spreads nothing (falls through to the default), for POST it injectsmaxRedirects: 0. Right shape.url-security.ts:465—options?.maxRedirects ?? 5uses nullish coalescing, so0survives. This is the load-bearing detail; a|| 5here would have silently reverted the fix to 5 hops. It's correct.url-security.ts:519,525— initial fetch is hardcodedredirect: 'manual'and the only follow mechanism is thefor (i < maxRedirects && ...)loop. WithmaxRedirects: 0,0 < 0is false, the loop never runs, undici never dialsLocation, and the raw 3xx is returned. No credential ever reaches the redirect target.- GET discovery unchanged:
maxRedirectsdefaults to 5, per-hopvalidateRedirectTargetSSRF validation intact. Verified by the discovery test aturl-security-redirects.test.ts:62-95. - Caller handling:
oauth-client-credentials-exchange.ts:116takes a 3xx asresponse.ok === falseand returns{ ok: false }without re-issuing — no re-POST that would re-leak. - No changeset required:
server/**app code is outside the protocol-package changeset scope (.agents/playbook.md). Correct absence. - Test plan in the PR body (vitest 20 passed, typecheck,
git diff --check) is checked, and the primary path — credentials not forwarded across 302/307/308 — is exactly what the new parametrized test asserts. Ships validated against the path it claims to fix.
code-reviewer: ready to push, correctness confirmed. security-reviewer: fix is correct and effective, no ship-blockers — credential leak on POST 3xx confirmed closed, GET discovery preserved, no residual auto-follow path.
Follow-ups (non-blocking — file as issues)
- Pin
redirect: 'manual'as an explicit invariant. The whole guarantee rests on the hardcodedredirect: 'manual'insafeFetch. The new tests mock undici, so a future refactor toredirect: 'follow'would neutermaxRedirects: 0while the tests stayed green. Worth an assertion (or a comment aturl-security.ts:519) that names this as load-bearing. (security-reviewer, Low.) - SDK-delegated flows unverified.
agent-oauth.tspassesoauthSafeFetchinto@adcp/sdk(not in this checkout). Structurally safe — any SDK re-follow would re-entersafeFetch's SSRF validation — but a test driving a token exchange through the SDK against a 3xx mock would close the gap. (security-reviewer, Low.)
Minor nits (non-blocking)
- 301/303 not in the parametrized set.
url-security-redirects.test.ts:37covers 302/307/308.maxRedirects: 0short-circuits before the status is inspected, so these are equivalent, but adding 301/303 would make the "no follow regardless of status class" intent explicit.
One behavior change worth noting for adopters: a provider that legitimately 307s a token request to a regional endpoint now fails as a 3xx rather than following. That is fail-closed and correct — flagging only so it isn't chased as a regression later.
LGTM. Follow-ups noted below.
* fix(oauth): route agent flow requests through safe fetch * test(oauth): inject callback state without cookie middleware * fix(oauth): secure client credential token exchange * fix(oauth): block redirects for credential posts (#5980)
Summary
Dependency
This is intentionally stacked on #5961 because the scoped OAuth fetch adapter is not on main yet. Do not merge this PR before #5961. After #5961 merges, retarget this PR to main; auto-merge is intentionally disabled.
Testing
Closes #5963.