fix(oauth): validate discovered endpoints before merge/use (#511)#739
Conversation
Discovery metadata was merged without the https/loopback endpoint rule that configured endpoints already pass, letting a malicious issuer downgrade the authorization/token/device/registration endpoint after config validation. - Add exported ValidateEndpointURL as the single endpoint-safety rule; validateTokenEndpoint now delegates to it. - Backstop in BuildAuthorizationURL (the choke point both the provider and MCP flows build their browser URL through). - Validate each discovered endpoint before merge in the provider resolveEndpoints and validate resolved MCP metadata in both branches; fail closed on insecure.
e5be0f1 to
d6a3ff6
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughOAuth endpoint validation is centralized in ChangesOAuth endpoint safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/oauth/flow.go (1)
93-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
ErrInsecureTokenEndpointto a general sentinel.
ValidateEndpointURLis now the shared rule for authorization, token, device-authorization, and registration endpoints, but the wrapped sentinel is still namedErrInsecureTokenEndpoint. When an authorization or registration endpoint fails, the error chain reads"…insecure token endpoint: http://evil.example", which is confusing for debugging.Renaming to
ErrInsecureEndpoint(or similar) would align the sentinel with its broader usage. This is a low-priority naming improvement since theerrors.Ischecks work correctly regardless.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/oauth/flow.go` around lines 93 - 110, Rename the shared sentinel ErrInsecureTokenEndpoint to a general ErrInsecureEndpoint and update ValidateEndpointURL plus all references and errors.Is checks to use the new name. Preserve the existing validation behavior and error wrapping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/oauth/flow.go`:
- Around line 93-110: Rename the shared sentinel ErrInsecureTokenEndpoint to a
general ErrInsecureEndpoint and update ValidateEndpointURL plus all references
and errors.Is checks to use the new name. Preserve the existing validation
behavior and error wrapping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c2003f85-6a1c-4bb4-9722-1812c981c959
📒 Files selected for processing (6)
internal/mcp/oauth.gointernal/mcp/oauth_test.gointernal/oauth/flow.gointernal/oauth/flow_test.gointernal/oauth/manager.gointernal/oauth/manager_test.go
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Reviewed at d6a3ff6. This is the right shape for the fix: one exported rule, validation at every merge site, and a backstop at the choke point both flows funnel through. Approving.
What I checked:
- Traced every credential-bearing path to a validation: device start and poll (device.go:49/157), token exchange (flow.go:176), the browser URL choke point (flow.go:46), provider config (providers.go:112), both discovered merges (manager.go:226-238), and both MCP branches (mcp/oauth.go:149). I could not find an endpoint that reaches the wire unvalidated; PostToken redirect replay is scoped out to your #729 follow-up, which is fine.
- Threw some hostile URLs at ValidateEndpointURL: http://localhost@evil.com resolves Hostname to evil.com and is rejected, scheme-relative //evil.com/x has no scheme and is rejected, http://127.0.0.1.evil.com fails ParseIP and is rejected, http://[::1]:8080 is correctly allowed. Everything ambiguous fails closed.
- Independently re-verified your fail-without-fix claim: kept the three new tests, reverted flow.go/manager.go/mcp/oauth.go to main, and all three fail; restored, they pass. Race run on both packages is green here too.
One behavior change worth stating plainly for the changelog: the MCP config branch now validates CONFIGURED endpoints as well, so a working (if ill-advised) http non-loopback MCP OAuth setup goes from failing at token time with a confusing error to failing up front with a clear one. That is the right call, just worth remembering if someone reports it as a regression.
Non-blocking nit: the backstop surfaces ErrInsecureTokenEndpoint for an authorization endpoint, so the message says "token endpoint" about a URL that is not one (your own manual-test transcript shows it). Renaming the exported error would break errors.Is callers, so leaving it is fine; a message tweak can ride along some other day.
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
Reviewed head d6a3ff6. The fix is sound and defense-in-depth is placed correctly: one shared HTTPS-or-loopback rule, validation before provider/MCP discovery merges, validation of configured MCP endpoints in both resolver branches, and a BuildAuthorizationURL backstop before either flow opens the browser. I traced authorization, token, device-authorization, and registration endpoints to validation before credential-bearing use; spoofed loopback host forms fail through URL Hostname plus net.ParseIP handling. Redirect replay remains correctly scoped to the separate #729 follow-up. OAuth/MCP race tests, vet, and diff hygiene pass. Approving.
3 participants
Summary
Closes #511.
Discovery-learned OAuth endpoints were merged and used without the https/loopback endpoint-safety rule that configured endpoints already pass. A malicious issuer's discovery document could therefore downgrade the authorization, token, device, or MCP registration endpoint after the provider config had already validated — silently redirecting the login to an attacker-controlled
http://origin. This affects both the provider OAuth flow and the MCP OAuth flow (MCP delegates discovery tointernal/oauth).Root cause
internal/oauth/providers.govalidates configured endpoints viavalidateTokenEndpoint, butinternal/oauth/manager.goresolveEndpointsmerged discovered endpoints with no revalidation.internal/mcp/oauth.goresolveAuthorizationServerlikewise merged discovered MCP metadata without validation.internal/oauth/flow.goBuildAuthorizationURLparsed the authorization endpoint but never checked its scheme, so a downgraded endpoint reached the browser.Fix — defense in depth (3 layers)
oauth.ValidateEndpointURL(https, or http on a loopback host);validateTokenEndpointnow delegates to it so configured and discovered endpoints share a single rule.BuildAuthorizationURLvalidates the authorization endpoint. Both the provider loopback flow and the MCP flow build their browser URL through this function, so a downgraded endpoint can never open in the browser even if a merge site is missed.resolveEndpointsvalidates each discovered authorization / token / device endpoint before merging.validateResolvedEndpointsapplied in both return branches ofresolveAuthorizationServer(authorization / token / registration, configured and discovered alike).Scope
internal/oauth+internal/mcponly. No config schema change. No behavior change for valid endpoints — https and http-loopback logins are unaffected. No performance change expected.Testing
resolveEndpoints, MCPresolveAuthorizationServer, and theBuildAuthorizationURLbackstop. Each was confirmed to fail without the fix (reverted the source, kept the tests) and pass with it; the pre-existing configured-endpoint test is unchanged.go test -race ./internal/oauth/... ./internal/mcp/...green.go build ./...,go vet ./...,gofmt,govulncheck,git diff HEAD --checkall clean.authorization_endpoint: http://evil.example/authorize,zero auth login <provider>now exits 1 withrefusing to send credential to a non-https token endpoint: http://evil.exampleand never opens the browser — whereas unpatchedmainbuilds and opens that attacker URL. A legitimate https discovery login still succeeds (happy path intact).Follow-up
The paired hardening for #729 (token-endpoint redirect replay in
PostToken) is a separate PR on the same files.Summary by CodeRabbit
Security
Bug Fixes
Tests