fix: use SSRF-safe HTTP client for MCP OAuth authorization server metadata fetch#3035
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🔴 CRITICAL
The PR introduces an SSRF-safe HTTP client for MCP OAuth metadata fetches, but the fix is incomplete. The newly created safe client (o.metadataClient) is only wired into the second fetch (getAuthorizationServerMetadata), while the first outbound request — GET /.well-known/oauth-protected-resource — still uses the unguarded http.DefaultClient. A serverURL pointing at an internal host will bypass SSRF protection on that initial call.
| tokenStore := NewKeyringTokenStore() | ||
|
|
||
| o := &oauth{metadataClient: &http.Client{Timeout: 5 * time.Second}} | ||
| o := &oauth{metadataClient: httpclient.NewSafeClient(5*time.Second, false)} |
There was a problem hiding this comment.
[HIGH] Incomplete SSRF fix — first metadata fetch still uses http.DefaultClient
The safe client created here (o.metadataClient) is only used by the subsequent o.getAuthorizationServerMetadata call. The first outbound request in this function — the GET /.well-known/oauth-protected-resource fetch 16 lines below — still uses the bare http.DefaultClient:
resp, err := http.DefaultClient.Do(resourceReq) // line 40 — unguardedThis means the PR's SSRF protection is incomplete. Any serverURL whose hostname resolves to a private/internal address (e.g. http://192.168.1.1/, http://169.254.169.254/) will still trigger an unguarded outbound request during the first fetch. The fix should use o.metadataClient.Do(resourceReq) instead of http.DefaultClient.Do(resourceReq).
There was a problem hiding this comment.
Fixed -- both fetches now use o.metadataClient. The agent config can be loaded from a remote URL or OCI reference, making serverURL attacker-influenced in those cases.
There was a problem hiding this comment.
Confirmed fixed — both metadata fetches now use the safe client. Thanks for the additional context about remote config loading; that makes the attack surface much clearer and confirms why guarding both requests was essential.
What
Replaces the unguarded
&http.Client{Timeout: 5s}used asoauth.metadataClientinPerformOAuthLoginwithhttpclient.NewSafeClient(5s, false), preserving the original timeoutwhile adding the SSRF-safe transport and redirect cap.
Why
PerformOAuthLoginfetches/.well-known/oauth-protected-resourcefromthe configured MCP server, then follows the
authorization_serversURLreturned in that response. That second URL is attacker-controlled -- a
malicious MCP server can point it at an internal host (e.g.
http://169.254.169.254/). The unguardedmetadataClientwould fetchit unconditionally.
Closes #3034.
References
IsPublicIPfor IPv6 / CGNAT gaps (same audit)