Guard OAuth token and DCR endpoints against SSRF

[jhrozek]

Summary

An authorization-server metadata document served by a remote MCP server is untrusted input, but two outbound paths dialed endpoints out of it without an SSRF guard (GHSA-3768-rwj3-38p2, CWE-918):

• The code-for-token exchange ran on http.DefaultClient. Flow.handleCallback set no oauth2.HTTPClient on the context, so golang.org/x/oauth2 fell back to the default client — no dial guard, no redirect policy. A token_endpoint pointing at a loopback address received a POST containing the authorization code and PKCE verifier, and a 302 from there was followed off-host. The same poisoned TokenURL persisted into the run config and was reused on every later refresh.
• The host-scoped guard waived itself for loopback. networking.NewHostScopedClientBuilder OR'd IsLocalhost(host) and INSECURE_DISABLE_URL_VALIDATION into the private-IP gate, so a registration_endpoint of http://127.0.0.1:PORT/x passed validation and then got an unguarded DCR POST carrying the initial access token — server-side, with no user in the loop.

The distinction that fixes both is who supplied the URL, not what it resolves to. An operator who configures dex at 127.0.0.1 has decided to trust it; a host that arrived inside a document a remote server served us has not been trusted by anyone. TargetIsPrivate-style gating cannot tell those apart, which is why the loopback waiver was correct on the operator path and a defect on the DCR path.

What changed:

• Route every token request through a guarded client. oauth.NewTokenHTTPClient builds it once in NewFlow and it is injected at the exchange and all four refresh sinks. Untrusted endpoints get the private-IP dial guard; every token client gets SameHostRedirectPolicy and DisableKeepAlives so the check re-runs per request and cannot be walked off-host by a redirect.
• Derive trust from the endpoint that is actually dialed. networking.AuthorityMatchesAny compares an endpoint's authority against the URLs the operator configured. An authorization server naming its own authority in its own metadata stays covered by the operator's decision to configure it; naming any other authority does not. discovery.go can only ever clear the flag when it overwrites TokenURL, never set it, so a trust decision can't outlive the URL it was made about.
• Keep the private-IP restriction for server-supplied hosts. networking.NewServerSuppliedHostClientBuilder applies the same policy minus the loopback and env-var waivers on the private-IP gate. In pkg/auth/dcr the provenance decision lives in the resolver, so a registration_endpoint that names an unconfigured authority is strict even when the operator chose the DiscoveryURL — no caller can forget to say so.
• Delete the vulnerable default. NewNonCachingRefresher and NewResourceTokenSource now require an explicit *http.Client instead of falling back to an unguarded one.

Reported by kta1kri.

Type of change

• Bug fix
• New feature
• Refactoring (no behavior change)
• Dependency update
• Documentation
• Other (describe):

Test plan

• Unit tests (task test)
• E2E tests (task test-e2e)
• Linting (task lint-fix)
• Manual testing (describe below)

Every new assertion is about whether a listener received a request, not about the value of a trust flag — flag-value assertions are what let the first cut of this fix pass while the behavior was wrong.

• pkg/auth/oauth — an untrusted loopback token endpoint is refused at the private-IP guard with zero hits on the listener; a trusted one completes the exchange (the dex / Keycloak-in-Docker non-regression case); a trusted endpoint's cross-host 302 leaves the redirect target at zero hits with errors.Is(err, ErrRedirectRefused).
• pkg/auth/discovery — an operator-configured issuer whose metadata advertises token_endpoint on a different authority loses the trust; one naming its own authority keeps it.
• pkg/auth/dcr — a metadata-named registration_endpoint on an unconfigured authority is refused and never receives the initial access token; an upstream naming its own authority still registers. Verified the first test fails when the guard is neutered (foreignHits = 1).
• pkg/networking — AuthorityMatchesAny table; two real dials proving the server-supplied builder refuses loopback by default and permits it under allowPrivateIPs; a case pinning that INSECURE_DISABLE_URL_VALIDATION no longer widens the private-IP gate.
• cmd/thv/app — the flow-config builder is now extracted and asserted directly, because the thv proxy trust wiring was previously unreachable from tests.

Not verified: no kind/e2e run. thv proxy --remote-auth-issuer http://localhost:PORT/... <public-mcp-url> against a real dex is the one shape that resists unit testing, and it is worth a manual check before merge.

Changes

File	Change
pkg/networking/utilities.go	New AuthorityMatchesAny — the operator-provenance test
pkg/networking/http_client.go	New NewServerSuppliedHostClientBuilder; corrected the stale "single source of truth" claim on the host-scoped builder
pkg/auth/oauth/flow.go	New NewTokenHTTPClient; guarded client built in NewFlow and injected at the exchange and refresh
pkg/auth/oauth/non_caching_refresher.go	*http.Client is now required; unguarded fallback removed
pkg/auth/oauth/resource_token_source.go	Threads the required client through
pkg/auth/discovery/discovery.go	TokenEndpointTrusted / IssuerTrusted; clears trust when a discovered endpoint changes authority
pkg/auth/remote/handler.go	Derives trust after the discovered-endpoint override, via one shared method
pkg/auth/remote/persisting_token_source.go	Guarded client on the cached-refresh path; returns an error
pkg/auth/tokensource/tokensource.go	Guarded client on both the cache-restore and browser-flow refreshers
pkg/auth/dcr/request.go	ServerSuppliedEndpoints, documenting what it does and does not cover
pkg/auth/dcr/resolver.go	Provenance decided in the resolver for metadata-derived registration endpoints
cmd/thv/app/proxy.go	Sets the trust flags; flow-config construction extracted so it is testable

Does this introduce a user-facing change?

Yes, one narrowing: INSECURE_DISABLE_URL_VALIDATION no longer widens the private-IP gate for endpoints supplied by a remote server or its metadata. It still relaxes the HTTPS-scheme requirement as before. Operators who relied on it to reach a private IdP should configure that IdP's issuer or token URL explicitly, or set the existing allowPrivateIPs option on the upstream.

Operator-configured localhost and in-cluster IdPs (dex, Keycloak-in-Docker, 10.x/*.internal) keep working, including on the DCR path — there are non-regression tests for each.

Special notes for reviewers

Scope exception. This is 12 non-test files against the 10-file guidance in CLAUDE.md. The two defects were triaged as separable and the original plan was two stacked PRs, but the fix for the second one uses AuthorityMatchesAny, introduced by the first — splitting now would mean either duplicating that helper or landing a DCR fix that cannot express its own trust test. Happy to split if you would rather review them apart.

Where to look hardest:

• discovery.go — the "clear but never set" rule on TokenEndpointTrusted is what stops a trust decision outliving the URL it described. An earlier cut of this fix computed the flag before the discovered-endpoint override and reopened the vulnerability on the configured-issuer + DCR path.
• resolver.go — provenance is per-endpoint, not per-request: the DiscoveryURL can be operator-supplied while the registration_endpoint it yields is not. An earlier cut treated every metadata-derived endpoint as strict, which broke operator-configured loopback IdPs.
• The trust model is deliberately delegated: an operator who configures an issuer trusts that issuer's metadata to name its own endpoints. It does not extend to a third authority, and it does not extend through a redirect.

Follow-up, not in scope: canonicalAuthority retains a bare-authority (host:port) parsing branch that no current caller reaches; pkg/vmcp/auth/strategies performs client_credentials grants on http.DefaultClient with operator-supplied token URLs — not this vulnerability's class, but the non-interactive server-side path worth hardening next.

🤖 Generated with Claude Code