Skip to content

fix(oauth): route agent flow requests through safe fetch#5961

Open
bokelley wants to merge 4 commits into
mainfrom
codex/oauth-safe-fetch
Open

fix(oauth): route agent flow requests through safe fetch#5961
bokelley wants to merge 4 commits into
mainfrom
codex/oauth-safe-fetch

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

  • adapt the existing SSRF-safe fetcher to the OAuth SDK request surface
  • inject it into protected-resource and authorization-server discovery, dynamic registration, token exchange, and status discovery
  • preserve headers, URL-encoded/JSON request bodies, and abort signals while rejecting unsupported request shapes
  • add route-level injection and adapter normalization coverage

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

  • focused Agent OAuth and URL-security suites: 108 passed
  • TypeScript typecheck
  • Prettier check
  • git diff --check

No changeset or Addie version bump: this is server-side security hardening.

Fixes #5960.

@bokelley
bokelley marked this pull request as ready for review July 21, 2026 20:22
@bokelley
bokelley enabled auto-merge (squash) July 21, 2026 20:22
Comment thread server/tests/unit/agent-oauth-membership-guard.test.ts Fixed
aao-secretariat[bot]
aao-secretariat Bot previously approved these changes Jul 21, 2026

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts now carry the scoped fetcher — PRM discovery (L75), AS-metadata discovery (L97), authorize+DCR via startWebOAuthFlow (L309→L312), token exchange via completeWebOAuthFlow (L414→L417), status discovery via discoverOAuthMetadata (L529). security-reviewer confirmed zero remaining bare fetch/axios/undici call sites in the file; web-oauth-stores.ts does no outbound HTTP.
  • Per-hop guard preserved: oauthSafeFetch delegates to safeFetch, which validates hop 0 (url-security.ts:455), re-validates every redirect Location (:522), and dials every hop through buildSsrfSafeDispatcher()'s connect-time ssrfSafeLookup (: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 via new Headers(...).entries(), string/Uint8Array/URLSearchParams body handling, and null-body/null-signal short-circuits are all correct. typeof fetch conformance is sound — the wider input param is contravariantly assignable to the SDK FetchLike.
  • Two different SDKs, two different injection keys, each used correctly: positional fetchFn for discoverOAuthProtectedResourceMetadata, { fetchFn } for discoverAuthorizationServerMetadata (MCP SDK), { fetch } for the three @adcp/sdk entry points. The @adcp/sdk calls 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 global fetch is never dialed during a real DCR + token-exchange flow would catch a future SDK signature drift that the mocks can't. (security-reviewer Medium — downgraded to follow-up on the strength of the passing typecheck.)
  • Cross-origin credential forwarding on redirect. safeFetch forwards Authorization across redirect hops and re-sends the body on 307/308 (url-security.ts:543-564); the OAuth adapter sets neither sameSiteRedirectsOnly nor maxRedirects: 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. Consider maxRedirects: 0 on 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)

  1. URLSearchParams content-type. oauth-safe-fetch.ts:28.toString() on a URLSearchParams body drops the application/x-www-form-urlencoded that native fetch auto-derives, defaulting the string body to text/plain. Safe today because the MCP-based token/registration paths set Content-Type explicitly; worth a comment noting the dependency (code-reviewer).
  2. Redundant guards. oauth-safe-fetch.ts:34-39 duplicates the GET-body / POST-no-body checks already in safeFetch (url-security.ts:484-490). Harmless, clearer local errors — a one-line "intentional, clearer diagnostics" comment would help the next reader.
  3. Dropped init fields. The adapter intentionally ignores redirect/cache/credentials so safeFetch owns 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.

@bokelley

Copy link
Copy Markdown
Contributor Author

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

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Argus review could not complete

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.

View workflow run

This is an automated message from the Argus AI review workflow.

@aao-secretariat aao-secretariat Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Argus review could not complete

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.

View workflow run

This is an automated message from the Argus AI review workflow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

agent-oauth: use SSRF-safe fetch across discovery and token flows

2 participants