Fall back to access_token query param when token/info rejects the Bearer header - #640
Fall back to access_token query param when token/info rejects the Bearer header#640zaporylie wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📜 Recent review details🔇 Additional comments (3)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesOAuth token verification
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@oauth-proxy.ts`:
- Around line 672-681: Add fallback-specific regression tests for the token
validation flow around the 401 retry in the relevant OAuth proxy test suite.
Mock the first request to return 401 when sent with the Bearer Authorization
header, then verify the retry uses a correctly URL-encoded access_token query
parameter and succeeds; also verify non-401 responses perform no retry.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc588d91-b9e9-4bae-9652-f9cc7d78ad23
📒 Files selected for processing (1)
oauth-proxy.ts
📜 Review details
🔇 Additional comments (2)
oauth-proxy.ts (2)
679-681: 🔒 Security & PrivacySecurity Misconfiguration (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: External
Require HTTPS for the fallback request.
Line 680 sends the token in the request target. This class does not validate that
_gitlabBaseUrluseshttps:. Verify that provider creation rejectshttp:URLs before this fallback can run. If it does not, enforce an HTTPS base URL.
679-681: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSensitive Data Exposure (CWE-598)
Reachability: External
Protect the token when the fallback uses a query parameter.
Line 680 puts a replayable bearer token in the request URL.
encodeURIComponentprevents syntax injection. It does not prevent retention in access logs, traces, or cache keys.Make this fallback an explicit deployment opt-in for instances with verified query-token redaction. Confirm that GitLab and the edge cache do not persist
access_tokenvalues. Otherwise, a user with access to those records can replay the token.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/mcp-oauth-tests.ts`:
- Line 519: Update the request tracking and assertions around the OAuth retry
test to validate the full authentication contract: the initial request must use
the Authorization header with Bearer ${TOKEN}, while the query-parameter retry
must omit the authorization header. Replace the boolean-only hasAuthHeader check
in the request records and assert both the header value and retry behavior for
the relevant requests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 94e6fd26-3ff6-4e5c-bb1b-a18e09f7765b
📒 Files selected for processing (1)
test/mcp-oauth-tests.ts
📜 Review details
🔇 Additional comments (3)
test/mcp-oauth-tests.ts (3)
566-597: LGTM!
599-626: LGTM!
515-515: 🎯 Functional CorrectnessNo duplicate
requestsdeclaration to fix.
1 participant
Problem
Some GitLab instances sit behind an edge cache that strips the
Authorizationheader on/oauth/*paths while passing it through on/api/v4/*. Observed on git.drupalcode.org (Drupal's GitLab, fronted by Varnish):GET /oauth/token/infowithAuthorization: Bearer <valid token>→ 401{"error":"invalid_token","error_description":"The access token is invalid"}GET /oauth/token/info?access_token=<same token>→ 200GET /api/v4/userwith the same Bearer header → 200Since
verifyAccessToken()in the MCP OAuth proxy only uses the Bearer-header form, every/mcprequest fails bearer validation on such instances — clients (e.g. claude.ai's connector) complete the full OAuth flow successfully, then loop refresh → 401 → refresh and give up. Nothing is logged server-side, which makes this painful to diagnose (found it via tcpdump inside the container's network namespace).Fix
On a 401 from the Bearer-header call, retry
/oauth/token/infoonce with the RFC 6750access_tokenquery parameter, which Doorkeeper also accepts. All other responses keep the existing behavior.Verified against git.drupalcode.org: with this change the claude.ai connector completes OAuth, initializes an MCP session, and lists tools normally.