MCP: prefer static Authorization header over stored OAuth tokens#609
MCP: prefer static Authorization header over stored OAuth tokens#609romzes5000 wants to merge 1 commit intomattermost:masterfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
romzes5000
force-pushed
the
fix/mcp-static-auth-ignores-oauth-kv
branch
from
8e283f9 to
56a1164
Compare
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 6 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes introduce a helper function to detect custom authorization headers and update the HTTP client to conditionally control OAuth token loading behavior. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
When an MCP server is configured with a custom Authorization header (e.g. API key), do not wrap requests with oauth2.Transport using a previously stored per-user OAuth token. That stale KV entry still caused createOAuthConfig and token refresh after switching from OAuth to token-only auth. Adds a unit test for header detection.
romzes5000
force-pushed
the
fix/mcp-static-auth-ignores-oauth-kv
branch
from
56a1164 to
2e8dbbd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mcp/http_client_test.go (1)
12-31: Add one transport-level regression test for the actual auth bypass path.This validates the helper well, but it would be valuable to also assert that when Authorization is present,
authenticationTransport.RoundTripskips token load / OAuth wrapping (not just key detection).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcp/http_client_test.go` around lines 12 - 31, Add a transport-level regression test that verifies authenticationTransport.RoundTrip skips token loading/OAuth wrapping when an Authorization header is present: create a new test (e.g. TestAuthenticationTransportSkipsTokenLoadWhenAuthorizationPresent) that constructs an authenticationTransport with a fake token provider whose LoadToken method toggles a flag and a fake base RoundTripper that records the request it receives; build an *http.Request containing the Authorization header via the custom headers path used by authorizationHeaderInCustomHeaders, call authenticationTransport.RoundTrip(req), and assert that the fake token provider was not called and the outgoing request still contains the original Authorization header (i.e. no OAuth wrapping occurred).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@mcp/http_client_test.go`:
- Around line 12-31: Add a transport-level regression test that verifies
authenticationTransport.RoundTrip skips token loading/OAuth wrapping when an
Authorization header is present: create a new test (e.g.
TestAuthenticationTransportSkipsTokenLoadWhenAuthorizationPresent) that
constructs an authenticationTransport with a fake token provider whose LoadToken
method toggles a flag and a fake base RoundTripper that records the request it
receives; build an *http.Request containing the Authorization header via the
custom headers path used by authorizationHeaderInCustomHeaders, call
authenticationTransport.RoundTrip(req), and assert that the fake token provider
was not called and the outgoing request still contains the original
Authorization header (i.e. no OAuth wrapping occurred).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b8e65933-4f08-4522-96af-72d718a00ad5
📒 Files selected for processing (3)
mcp/http_client.gomcp/http_client_test.gomcp/oauth_transport.go
edgarbellot
left a comment
edgarbellot
left a comment
There was a problem hiding this comment.
@romzes5000 Thanks for the contribution! The approach of skipping oauth2.Transport when a static Authorization header is present makes sense and the tests look clean.
I left a couple of comments on areas where the new flag could be checked more broadly to keep the static-auth path fully isolated from the OAuth machinery. We might need to update the test to cover the edge cases mentioned in the comments. Happy to provide more context if needed :)
| } | ||
|
|
||
| // If we get a 401, force an actual error so we can handle it. Include the header info in the error | ||
| if resp.StatusCode == http.StatusUnauthorized { |
There was a problem hiding this comment.
Nice work on skipping the OAuth token load and oauth2.Transport when a static Authorization header is present - the outgoing request path (67-86) looks correct.
However, should we also guard the 401 response handling block (109-131) with the same preferStaticAuthorizationHeader check? Right now, if a static-auth MCP server responds with a 401 (e.g. expired API key), the transport still returns an mcpUnauthorized error. The caller in createSession (293-302) then catches it via errors.As and calls InitiateOAuthFlow, which triggers outbound HTTP requests for metadata discovery (discoverProtectedResourceMetadata, discoverAuthorizationServerMetadata) and potentially DCR (loadOrCreateClientCredentials).
This means a compromised MCP server could craft its 401 WWW-Authenticate header to point the plugin at an attacker-controlled URL, causing the Mattermost server to make outbound requests to that URL during discovery. The MattermostTransport does block reserved IPs, which limits internal SSRF, but external endpoints are still reachable.
Would it make sense to short-circuit the 401 path when preferStaticAuthorizationHeader is true - something like returning a plain error instead of mcpUnauthorized? For example:
if resp.StatusCode == http.StatusUnauthorized {
if t.preferStaticAuthorizationHeader {
drainAndCloseResponseBody(resp)
return nil, fmt.Errorf("static authorization rejected by server (HTTP 401)")
}
// ... existing OAuth 401 handling
}| headers map[string]string | ||
| } | ||
|
|
||
| func (t *headerTransport) RoundTrip(req *http.Request) (*http.Response, error) { |
There was a problem hiding this comment.
This is a pre-existing issue in headerTransport, but I think this PR makes it worth flagging because the credential at stake changes significantly.
Go's http.Client strips the Authorization header on cross-domain redirects (since Go 1.12) before calling Transport.RoundTrip(). However, headerTransport.RoundTrip unconditionally re-adds all configured headers - including Authorization - on every call. Since http.Client.Do() calls RoundTrip again for each redirect hop, this effectively bypasses Go's built-in protection.
Before this PR, the Authorization header would typically be overwritten by oauth2.Transport with a short-lived OAuth token, so a leaked credential would expire quickly. With preferStaticAuthorizationHeader set to true, oauth2.Transport is skipped and the static API key flows through unchanged. That means a compromised MCP server responding with a 302 Location: https://attacker.com/steal would receive the long-lived API key in the redirected request - silently, with no error or log.
As mentioned in the other comment, the MattermostTransport blocks reserved/internal IPs, which mitigates internal SSRF, but external hosts are fully reachable. There's also no CheckRedirect policy on the http.Client built by httpClientForMCP, so Go's default 10-redirect limit applies with no domain restrictions.
Would it make sense to add a CheckRedirect policy that strips credentials on cross-domain redirects? Something like:
httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
if len(via) >= 10 {
return errors.New("stopped after 10 redirects")
}
if len(via) > 0 && req.URL.Host != via[0].URL.Host {
req.Header.Del("Authorization")
}
return nil
}2 participants
Summary
When an MCP server is configured with a custom
Authorizationheader (e.g. Outline API keyol_api_*), the HTTP client must not wrap requests withoauth2.Transportusing a previously stored per-user OAuth token from the plugin KV store.Without this, switching from OAuth to token-only auth leaves stale
mcp_oauth_token_v1_*rows; the plugin still callscreateOAuthConfig/ token refresh on every request, which can fail (e.g. DCR rate limits) even though the admin intends to use only the static header.Changes
Authorization(case-insensitive), skip loading the OAuth token and do not wrap withoauth2.Transport.TestAuthorizationHeaderInCustomHeaders.Test steps
Authorization: Bearer <api key>in custom headers and remove OAuth client fields if any.Made with Cursor
Summary by CodeRabbit