fix(oauth): route agent flow requests through safe fetch#5961
Conversation
There was a problem hiding this comment.
Clean, contained SSRF hardening — routes every server-side Agent OAuth request through the existing safeFetch, closing the DNS-rebind and redirect-to-private-literal window that validateExternalUrl's synchronous preflight cannot. Fail-closed beats fail-open: the adapter throws on any request shape outside the OAuth surface rather than degrading to bare global fetch.
Things I checked
- Completeness: all five outbound OAuth entry points in
agent-oauth.tsnow carry the scoped fetcher — PRM discovery (L75), AS-metadata discovery (L97), authorize+DCR viastartWebOAuthFlow(L309→L312), token exchange viacompleteWebOAuthFlow(L414→L417), status discovery viadiscoverOAuthMetadata(L529).security-reviewerconfirmed zero remaining barefetch/axios/undicicall sites in the file;web-oauth-stores.tsdoes no outbound HTTP. - Per-hop guard preserved:
oauthSafeFetchdelegates tosafeFetch, which validates hop 0 (url-security.ts:455), re-validates every redirect Location (:522), and dials every hop throughbuildSsrfSafeDispatcher()'s connect-timessrfSafeLookup(:364-399). Rebind protection holds across discovery → registration → token exchange. - Adapter normalization (
oauth-safe-fetch.ts:19-40): string/URL input allowlist, GET/POST method allowlist, header flattening vianew Headers(...).entries(), string/Uint8Array/URLSearchParams body handling, and null-body/null-signal short-circuits are all correct.typeof fetchconformance is sound — the wider input param is contravariantly assignable to the SDKFetchLike. - Two different SDKs, two different injection keys, each used correctly: positional
fetchFnfordiscoverOAuthProtectedResourceMetadata,{ fetchFn }fordiscoverAuthorizationServerMetadata(MCP SDK),{ fetch }for the three@adcp/sdkentry points. The@adcp/sdkcalls pass object literals, so a wrong key name would fail the typecheck the PR body reports as passing — not silently fail-open. - No wire/schema changes → no changeset required (changesets are the protocol-package surface; server-side hardening carries none). Title is valid conventional-commits.
security-reviewer: SOUND, no High.code-reviewer: no blockers.
Follow-ups (non-blocking — file as issues)
- SDK key correctness has no integration coverage. The unit tests mock the SDK, so they assert
fetch:/fetchFn:is passed, never that the SDK reads it. Typecheck covers the key name today, but an integration assertion that globalfetchis never dialed during a real DCR + token-exchange flow would catch a future SDK signature drift that the mocks can't. (security-reviewerMedium — downgraded to follow-up on the strength of the passing typecheck.) - Cross-origin credential forwarding on redirect.
safeFetchforwardsAuthorizationacross redirect hops and re-sends the body on 307/308 (url-security.ts:543-564); the OAuth adapter sets neithersameSiteRedirectsOnlynormaxRedirects: 0. RFC 6749 token endpoints don't redirect, so a malicious/misconfigured AS answering the token POST with a 307 to a public host could receive the authorization code, PKCE verifier, and client secret. Impact is bounded — single-use, bound to that AS's registered client, org-scoped, no cross-tenant reach — so hardening, not a live break. ConsidermaxRedirects: 0on the token-exchange hop. - Token refresh path untouched. OAuth refresh during later MCP/A2A calls is a separate code path hitting the same attacker-influenced token endpoint; out of scope for #5960 but the same treatment applies.
Minor nits (non-blocking)
- URLSearchParams content-type.
oauth-safe-fetch.ts:28—.toString()on aURLSearchParamsbody drops theapplication/x-www-form-urlencodedthat native fetch auto-derives, defaulting the string body totext/plain. Safe today because the MCP-based token/registration paths setContent-Typeexplicitly; worth a comment noting the dependency (code-reviewer). - Redundant guards.
oauth-safe-fetch.ts:34-39duplicates the GET-body / POST-no-body checks already insafeFetch(url-security.ts:484-490). Harmless, clearer local errors — a one-line "intentional, clearer diagnostics" comment would help the next reader. - Dropped init fields. The adapter intentionally ignores
redirect/cache/credentialssosafeFetchowns redirect-following; a one-line comment saying so prevents a future reader reading it as an oversight.
Lineage: continues the SSRF work from #3609/#3620 (proxy-env detection) and #5958 (fail-closed for private external URLs). Fixes #5960.
Approving on the strength of the completeness check plus security-reviewer's SOUND verdict. Follow-ups noted.
|
Issue #5963 proposes redirect restriction for credential-bearing OAuth POSTs — same surface as this PR; consider folding before merge or confirm follow-up. Generated by Claude Code |
There was a problem hiding this comment.
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.
This is an automated message from the Argus AI review workflow.
There was a problem hiding this comment.
The automated review encountered an issue (possibly reached max turns, timed out, or failed to post the final gh pr review). A human reviewer should take this PR.
This is an automated message from the Argus AI review workflow.
Summary
Why
Synchronous URL validation cannot prevent DNS rebinding between preflight and connection, and bare global fetch can follow redirects to private literal addresses. The scoped adapter applies the existing per-hop validation and connect-time DNS guard to every server-side Agent OAuth request.
Validation
git diff --checkNo changeset or Addie version bump: this is server-side security hardening.
Fixes #5960.