Strip authorization requests before they reach authorization server#1543
Strip authorization requests before they reach authorization server#1543ignaciojimenezr merged 9 commits intoMCPJam:mainfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
@ignaciojimenezr : feel free to just iterate on this with claude/codex
[do] Header stripping is not case-insensitive — this will still leak on AUTHORIZATION, authORIZATION, etc.
HTTP headers are case-insensitive per RFC 7230 §3.2. You only delete Authorization and authorization, but a user could configure AUTHORIZATION or any other casing in their custom headers.
Fix this with a case-insensitive filter:
if (isAuthServer) {
for (const key of Object.keys(headers)) {
if (key.toLowerCase() === "authorization") {
delete headers[key];
}
}
}[do] Boolean trap — isAuthServer = false as a positional arg is fragile and invites regressions
Every future mergeHeaders call site requires the author to remember "oh right, pass true for auth server requests." If they forget, the header silently leaks again. This is the exact class of bug that regresses.
Suggestion: make it explicit with two named functions, or at minimum use an options object:
mergeHeaders(requestHeaders, { stripAuth: true })
// or better:
mergeHeadersForAuthServer(requestHeaders)
mergeHeadersForMcpServer(requestHeaders)This makes intent self-documenting and grep-able.
[do] Tests only cover the 2025-11-25 state machine, but the fix was applied to all three
You modified debug-oauth-2025-03-26.ts, debug-oauth-2025-06-18.ts, and debug-oauth-2025-11-25.ts — but the test file only imports and tests debug-oauth-2025-11-25. If one of the other two files has a subtle difference or a missed call site, you won't catch it. Add at least a smoke test for the other two machines, or extract mergeHeaders into the shared helpers.ts module so there's a single implementation to test.
[do] Verify: does loggingFetch (used by the SDK's discoverOAuthProtectedResourceMetadata) also need stripping?
In debug-oauth-2025-06-18.ts:685 and debug-oauth-2025-11-25.ts:830, you have a loggingFetch wrapper that calls mergeHeaders(normalizeHeaders(...)) without isAuthServer = true. This wrapper is passed to the MCP SDK's discoverOAuthProtectedResourceMetadata. The resource metadata endpoint (/.well-known/oauth-protected-resource) lives on the MCP server's domain, so it's probably fine — but please confirm the SDK doesn't follow any redirects or links that could send this to the auth server. Add a comment explaining why true is not needed here.
ignaciojimenezr
force-pushed
the
debugging-oauth
branch
from
d0a5e1a to
6cb8f9f
Compare
mergeHeaders(customHeaders, requestHeaders) — for MCP server requests (keeps all headers)
|
dosubot
bot
added
size:XL
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds three exported helpers: 📝 Coding Plan
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.ts (1)
909-916:⚠️ Potential issue | 🟠 MajorDon't copy
customHeadersinto synthetic response headers.These placeholders are persisted as
lastResponse/httpHistory.response, so this re-exposes user-suppliedAuthorizationvalues in logs even when the actual auth-server request stripped them on the wire.lastRequestalready holds the attempted request headers. The later token/authenticated catches repeat the same shape.🛡️ Safer placeholder response
const errorResponse = { status: 0, statusText: "Network Error", - headers: mergeHeaders(customHeaders, {}), + headers: {}, body: { error: error instanceof Error ? error.message : String(error), }, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.ts` around lines 909 - 916, The synthetic errorResponse currently copies user-supplied customHeaders via mergeHeaders(customHeaders, {}), which can persist sensitive Authorization values; update the construction of errorResponse (variable errorResponse) to avoid copying customHeaders — replace headers: mergeHeaders(customHeaders, {}) with either an empty headers object or a sanitized/redacted headers object (e.g., {}) so only non-sensitive metadata is stored (the original request headers remain available in lastRequest); ensure references to mergeHeaders and customHeaders are removed from this synthetic response.
🧹 Nitpick comments (1)
mcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-unit.test.ts (1)
15-35: Add a casing-mismatch override test.The suite only checks exact-case collisions.
authorizationvsAuthorizationis the HTTP-relevant failure mode, and it is the one that would let duplicate auth headers slip through here.🧪 Example coverage to add
describe("mergeHeaders", () => { + it("treats header names case-insensitively when request headers override custom headers", () => { + const result = mergeHeaders( + { authorization: "Bearer old-token" }, + { Authorization: "Bearer new-token" }, + ); + + expect( + Object.keys(result).filter((key) => key.toLowerCase() === "authorization"), + ).toHaveLength(1); + expect(result.Authorization).toBe("Bearer new-token"); + }); + it("returns request headers overriding custom headers", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-unit.test.ts` around lines 15 - 35, Add a unit test to cover case-insensitive header collisions for mergeHeaders: call mergeHeaders with a custom headers object containing a lowercased "authorization" and a request headers object containing "Authorization" (or vice versa) and assert that the request header value wins (the resulting headers include the request's Authorization value and key). Reference the mergeHeaders function and add this new it(...) test alongside the existing tests to ensure casing-mismatch overrides behave like exact-case collisions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts`:
- Around line 432-434: Wrap the "idle" case body in braces to confine
declarations to the case block; specifically, change the switch case for "idle"
so that const initialRequestHeaders and const initialRequest (and any other
declarations in that case) are declared inside a braced block (case "idle": {
... }) to satisfy Biome's noSwitchDeclarations rule and avoid TDZ/lexical
scoping issues.
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.ts`:
- Around line 578-580: The switch case that declares const initialRequestHeaders
(using mergeHeaders and "Content-Type") must be wrapped in a block to avoid
lexical-scope leakage; open a brace immediately after the case label and close
it before the break so the const is scoped to that case (i.e., surround the
entire case body that contains initialRequestHeaders with { ... } and keep the
existing break inside the braces).
In `@mcpjam-inspector/client/src/lib/oauth/state-machines/shared/helpers.ts`:
- Around line 232-239: mergeHeaders currently merges by exact key which treats
header names as case-sensitive; modify mergeHeaders to normalize header names
(e.g., lowercase) so requestHeaders override customHeaders case-insensitively:
iterate customHeaders and requestHeaders, insert keys into a new map using
key.toLowerCase() so later entries (requestHeaders) replace earlier ones, and
return that normalized map; refer to the mergeHeaders function to implement this
normalization when building the result.
- Around line 250-260: The current mergeHeadersForAuthServer removes every
Authorization header after merging; change it to only strip a connection-level
Bearer token that originated from customHeaders so request-specific auth (e.g.
Basic or registration tokens) remains. Concretely: after computing merged =
mergeHeaders(customHeaders, requestHeaders), check customHeaders
(case-insensitive for the header name) for an Authorization value and if that
value begins with "Bearer " (case-insensitive), then remove the corresponding
key from merged; otherwise leave merged.Authorization intact. Use the function
name mergeHeadersForAuthServer and the parameters customHeaders and
requestHeaders to locate and implement this conditional removal.
---
Outside diff comments:
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.ts`:
- Around line 909-916: The synthetic errorResponse currently copies
user-supplied customHeaders via mergeHeaders(customHeaders, {}), which can
persist sensitive Authorization values; update the construction of errorResponse
(variable errorResponse) to avoid copying customHeaders — replace headers:
mergeHeaders(customHeaders, {}) with either an empty headers object or a
sanitized/redacted headers object (e.g., {}) so only non-sensitive metadata is
stored (the original request headers remain available in lastRequest); ensure
references to mergeHeaders and customHeaders are removed from this synthetic
response.
---
Nitpick comments:
In `@mcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-unit.test.ts`:
- Around line 15-35: Add a unit test to cover case-insensitive header collisions
for mergeHeaders: call mergeHeaders with a custom headers object containing a
lowercased "authorization" and a request headers object containing
"Authorization" (or vice versa) and assert that the request header value wins
(the resulting headers include the request's Authorization value and key).
Reference the mergeHeaders function and add this new it(...) test alongside the
existing tests to ensure casing-mismatch overrides behave like exact-case
collisions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3b0bc3d-ce2d-4b2e-a1fe-1165d80cf8bd
📒 Files selected for processing (6)
mcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-auth-server.test.tsmcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-unit.test.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.tsmcpjam-inspector/client/src/lib/oauth/state-machines/shared/helpers.ts
| const initialRequestHeaders = mergeHeaders(customHeaders, { | ||
| "Content-Type": "application/json", | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file
find . -name "debug-oauth-2025-06-18.ts" -type fRepository: MCPJam/inspector
Length of output: 140
🏁 Script executed:
# Check if the file exists and view the relevant lines
if [ -f "mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts" ]; then
echo "File found. Checking lines 430-440 for context:"
sed -n '430,440p' "mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts" | cat -n
else
echo "File not found at expected path"
fiRepository: MCPJam/inspector
Length of output: 573
🏁 Script executed:
# Get broader context to see the switch structure
if [ -f "mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts" ]; then
echo "Checking lines 420-450 for full switch context:"
sed -n '420,450p' "mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts" | cat -n
fiRepository: MCPJam/inspector
Length of output: 1314
Wrap this case body in braces.
The const initialRequestHeaders and const initialRequest declarations inside the "idle" case bypass proper lexical scoping and trigger Biome's noSwitchDeclarations rule. Adding braces constrains them to the case scope and resolves the TDZ hazard:
case "idle": {
const initialRequestHeaders = mergeHeaders(customHeaders, {
"Content-Type": "application/json",
});
// ... rest of case body
}
🧰 Tools
🪛 Biome (2.4.6)
[error] 432-434: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts`
around lines 432 - 434, Wrap the "idle" case body in braces to confine
declarations to the case block; specifically, change the switch case for "idle"
so that const initialRequestHeaders and const initialRequest (and any other
declarations in that case) are declared inside a braced block (case "idle": {
... }) to satisfy Biome's noSwitchDeclarations rule and avoid TDZ/lexical
scoping issues.
| const initialRequestHeaders = mergeHeaders(customHeaders, { | ||
| "Content-Type": "application/json", | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.ts" -type fRepository: MCPJam/inspector
Length of output: 140
🏁 Script executed:
sed -n '560,600p' ./mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.tsRepository: MCPJam/inspector
Length of output: 1384
🏁 Script executed:
sed -n '575,620p' ./mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.tsRepository: MCPJam/inspector
Length of output: 1573
Wrap this case body in braces.
Lexical declarations in unbraced switch cases create scope pollution across all subsequent cases and temporal dead zone hazards. Biome's noSwitchDeclarations rule flags this; wrap the entire case body in braces to resolve.
🧰 Tools
🪛 Biome (2.4.6)
[error] 578-580: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
(lint/correctness/noSwitchDeclarations)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.ts`
around lines 578 - 580, The switch case that declares const
initialRequestHeaders (using mergeHeaders and "Content-Type") must be wrapped in
a block to avoid lexical-scope leakage; open a brace immediately after the case
label and close it before the break so the const is scoped to that case (i.e.,
surround the entire case body that contains initialRequestHeaders with { ... }
and keep the existing break inside the braces).
| export function mergeHeadersForAuthServer( | ||
| customHeaders: Record<string, string> | undefined, | ||
| requestHeaders: Record<string, string> = {}, | ||
| ): Record<string, string> { | ||
| const merged = mergeHeaders(customHeaders, requestHeaders); | ||
| for (const key of Object.keys(merged)) { | ||
| if (key.toLowerCase() === "authorization") { | ||
| delete merged[key]; | ||
| } | ||
| } | ||
| return merged; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Strip the leaked MCP bearer token, not every auth-server credential.
This helper deletes any Authorization header after the merge, so an explicit Basic ... client-auth header or registration access token would disappear too. The scrub should target connection-level Bearer headers from customHeaders, then leave request-specific auth intact.
🎯 Narrow the scrub to the problematic header
export function mergeHeadersForAuthServer(
customHeaders: Record<string, string> | undefined,
requestHeaders: Record<string, string> = {},
): Record<string, string> {
- const merged = mergeHeaders(customHeaders, requestHeaders);
- for (const key of Object.keys(merged)) {
- if (key.toLowerCase() === "authorization") {
- delete merged[key];
- }
- }
- return merged;
+ const safeCustomHeaders: Record<string, string> = { ...(customHeaders ?? {}) };
+
+ for (const key of Object.keys(safeCustomHeaders)) {
+ if (
+ key.toLowerCase() === "authorization" &&
+ /^Bearer\s/i.test(safeCustomHeaders[key] ?? "")
+ ) {
+ delete safeCustomHeaders[key];
+ }
+ }
+
+ return mergeHeaders(safeCustomHeaders, requestHeaders);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/client/src/lib/oauth/state-machines/shared/helpers.ts`
around lines 250 - 260, The current mergeHeadersForAuthServer removes every
Authorization header after merging; change it to only strip a connection-level
Bearer token that originated from customHeaders so request-specific auth (e.g.
Basic or registration tokens) remains. Concretely: after computing merged =
mergeHeaders(customHeaders, requestHeaders), check customHeaders
(case-insensitive for the header name) for an Authorization value and if that
value begins with "Bearer " (case-insensitive), then remove the corresponding
key from merged; otherwise leave merged.Authorization intact. Use the function
name mergeHeadersForAuthServer and the parameters customHeaders and
requestHeaders to locate and implement this conditional removal.
ignaciojimenezr
force-pushed
the
debugging-oauth
branch
from
b4f3074 to
da70951
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.ts (1)
526-556:⚠️ Potential issue | 🟡 MinorKeep recorded request headers identical to the actual wire headers.
Line 526 builds
requestHeaders, but Line 553 sends a different header object (MCP-Protocol-Versionis added only there). This makes HTTP history inaccurate during retries.Proposed fix
- const requestHeaders = mergeHeadersForAuthServer( - customHeaders, - {}, - ); + const requestHeaders = mergeHeadersForAuthServer(customHeaders, { + "MCP-Protocol-Version": "2025-03-26", + }); @@ - const response = await proxyFetch(url, { - method: "GET", - headers: mergeHeadersForAuthServer(customHeaders, { - "MCP-Protocol-Version": "2025-03-26", - }), - }); + const response = await proxyFetch(url, { + method: "GET", + headers: requestHeaders, + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.ts` around lines 526 - 556, The recorded request headers differ from the actual sent headers because you build requestHeaders with mergeHeadersForAuthServer(customHeaders, {}) but call proxyFetch with mergeHeadersForAuthServer(customHeaders, { "MCP-Protocol-Version": "2025-03-26" }); fix by creating a single headers object (e.g., call mergeHeadersForAuthServer(customHeaders, { "MCP-Protocol-Version": "2025-03-26" }) once), use that same object when setting lastRequest and updating updatedHistoryForRetry (references: mergeHeadersForAuthServer, updatedHistoryForRetry, updateState, proxyFetch) so the stored httpHistory exactly matches the wire headers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-auth-server.test.ts`:
- Around line 12-162: Replace the bespoke test mocks with the shared client mock
presets: import and apply mcpApiPresets and storePresets from
client/src/test/mocks at the top of the test (before importing
createDebugOAuthStateMachine) instead of manually mocking
"../state-machines/shared/helpers" and
"@modelcontextprotocol/sdk/client/auth.js"; remove the custom proxyFetch/helpers
and SDK mocks and rely on the preset mocks to provide proxyFetch,
generateRandomString, generateCodeChallenge, loadPreregisteredCredentials and
discoverOAuthProtectedResourceMetadata behavior so createTestMachine and
createDebugOAuthStateMachine continue to work with
serverUrl/serverName/redirectUrl/customHeaders/registrationStrategy as before.
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.ts`:
- Around line 909-913: The synthetic network-error response construction is
copying request-side customHeaders into errorResponse.headers (via
mergeHeaders(customHeaders, {})), which can leak secrets; update the error
path(s) where errorResponse is built (the instances referencing errorResponse
and mergeHeaders(customHeaders, {})) to not include request headers—use an empty
response header set or a safe/set of server-origin headers instead (e.g.,
replace mergeHeaders(customHeaders, {}) with {} or mergeHeaders({}, {}) and/or
explicitly omit sensitive keys like Authorization/Api-Key), and apply the same
change to the other occurrences that build errorResponse in this file.
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts`:
- Around line 1244-1248: The synthetic network-error response currently copies
customHeaders into errorResponse.headers (via mergeHeaders(customHeaders, {})),
which can leak sensitive values like Authorization or cookies; update the code
that builds the errorResponse object (the variable named errorResponse) to
sanitize headers first by removing sensitive keys ("authorization", "cookie",
"set-cookie", etc.) before calling mergeHeaders, and apply the same sanitization
logic to the other synthetic responses at the same pattern (the similar
constructions around the other occurrences). Ensure you centralize the filter (a
small helper like sanitizeHeaders or reuse an existing header util) and use that
helper in each place instead of passing customHeaders directly.
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.ts`:
- Around line 1411-1415: The synthetic error responses are incorrectly seeding
request custom headers into response metadata by setting errorResponse.headers =
mergeHeaders(customHeaders, {}); update each errorResponse construction (the
variable named errorResponse where headers are assigned) to avoid including
request credentials: replace mergeHeaders(customHeaders, {}) with a safe
response-only headers object (e.g., an empty/explicitly constructed headers map
or mergeHeaders({}, responseSafeHeaders)) so request-specific customHeaders are
not copied into the synthetic response; do this for every occurrence that uses
mergeHeaders(customHeaders, {}) (the three errorResponse blocks flagged).
---
Outside diff comments:
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.ts`:
- Around line 526-556: The recorded request headers differ from the actual sent
headers because you build requestHeaders with
mergeHeadersForAuthServer(customHeaders, {}) but call proxyFetch with
mergeHeadersForAuthServer(customHeaders, { "MCP-Protocol-Version": "2025-03-26"
}); fix by creating a single headers object (e.g., call
mergeHeadersForAuthServer(customHeaders, { "MCP-Protocol-Version": "2025-03-26"
}) once), use that same object when setting lastRequest and updating
updatedHistoryForRetry (references: mergeHeadersForAuthServer,
updatedHistoryForRetry, updateState, proxyFetch) so the stored httpHistory
exactly matches the wire headers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de5c9b71-3166-4200-b4cf-70a2779ea333
📒 Files selected for processing (6)
mcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-auth-server.test.tsmcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-unit.test.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.tsmcpjam-inspector/client/src/lib/oauth/state-machines/shared/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mcpjam-inspector/client/src/lib/oauth/tests/merge-headers-unit.test.ts
- mcpjam-inspector/client/src/lib/oauth/state-machines/shared/helpers.ts
| import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; | ||
| import type { OAuthFlowState } from "../state-machines/types"; | ||
| import { EMPTY_OAUTH_FLOW_STATE } from "../state-machines/types"; | ||
|
|
||
| // Track all proxyFetch calls so we can inspect headers | ||
| let proxyFetchCalls: Array<{ url: string; options: RequestInit }> = []; | ||
|
|
||
| // Mock the helpers module to intercept proxyFetch | ||
| vi.mock("../state-machines/shared/helpers", async (importOriginal) => { | ||
| const actual = | ||
| await importOriginal<typeof import("../state-machines/shared/helpers")>(); | ||
| return { | ||
| ...actual, | ||
| proxyFetch: vi.fn(async (url: string, options: RequestInit = {}) => { | ||
| proxyFetchCalls.push({ url, options }); | ||
|
|
||
| // Return 401 with WWW-Authenticate for MCP server requests | ||
| if ( | ||
| url.includes("mcp-server.example.com") && | ||
| !url.includes(".well-known") | ||
| ) { | ||
| return { | ||
| status: 401, | ||
| statusText: "Unauthorized", | ||
| headers: { | ||
| "www-authenticate": | ||
| 'Bearer resource_metadata="https://mcp-server.example.com/.well-known/oauth-protected-resource"', | ||
| }, | ||
| body: null, | ||
| ok: false, | ||
| }; | ||
| } | ||
|
|
||
| // Return resource metadata for /.well-known/oauth-protected-resource | ||
| if (url.includes("oauth-protected-resource")) { | ||
| return { | ||
| status: 200, | ||
| statusText: "OK", | ||
| headers: { "content-type": "application/json" }, | ||
| body: { | ||
| resource: "https://mcp-server.example.com", | ||
| authorization_servers: ["https://auth-server.example.com"], | ||
| }, | ||
| ok: true, | ||
| }; | ||
| } | ||
|
|
||
| // Return auth server metadata | ||
| if ( | ||
| url.includes("oauth-authorization-server") || | ||
| url.includes("openid-configuration") | ||
| ) { | ||
| return { | ||
| status: 200, | ||
| statusText: "OK", | ||
| headers: { "content-type": "application/json" }, | ||
| body: { | ||
| issuer: "https://auth-server.example.com", | ||
| authorization_endpoint: "https://auth-server.example.com/authorize", | ||
| token_endpoint: "https://auth-server.example.com/token", | ||
| registration_endpoint: "https://auth-server.example.com/register", | ||
| response_types_supported: ["code"], | ||
| grant_types_supported: ["authorization_code"], | ||
| code_challenge_methods_supported: ["S256"], | ||
| token_endpoint_auth_methods_supported: ["none"], | ||
| }, | ||
| ok: true, | ||
| }; | ||
| } | ||
|
|
||
| // Return success for client registration | ||
| if (url.includes("/register")) { | ||
| return { | ||
| status: 200, | ||
| statusText: "OK", | ||
| headers: { "content-type": "application/json" }, | ||
| body: { | ||
| client_id: "test-client-id", | ||
| client_secret: "test-client-secret", | ||
| token_endpoint_auth_method: "client_secret_post", | ||
| }, | ||
| ok: true, | ||
| }; | ||
| } | ||
|
|
||
| // Return success for token exchange | ||
| if (url.includes("/token")) { | ||
| return { | ||
| status: 200, | ||
| statusText: "OK", | ||
| headers: { "content-type": "application/json" }, | ||
| body: { | ||
| access_token: "new-access-token", | ||
| token_type: "Bearer", | ||
| expires_in: 3600, | ||
| }, | ||
| ok: true, | ||
| }; | ||
| } | ||
|
|
||
| // Default 404 | ||
| return { | ||
| status: 404, | ||
| statusText: "Not Found", | ||
| headers: {}, | ||
| body: null, | ||
| ok: false, | ||
| }; | ||
| }), | ||
| generateRandomString: vi.fn(() => "mock-random-string"), | ||
| generateCodeChallenge: vi.fn(async () => "mock-code-challenge"), | ||
| loadPreregisteredCredentials: vi.fn(() => ({ | ||
| clientId: undefined, | ||
| clientSecret: undefined, | ||
| })), | ||
| }; | ||
| }); | ||
|
|
||
| // Mock the MCP SDK's discoverOAuthProtectedResourceMetadata | ||
| // This SDK function is called during resource metadata discovery. | ||
| // It receives a loggingFetch wrapper, but we mock the whole function | ||
| // to return valid metadata directly. | ||
| vi.mock("@modelcontextprotocol/sdk/client/auth.js", () => ({ | ||
| discoverOAuthProtectedResourceMetadata: vi.fn(async () => ({ | ||
| resource: "https://mcp-server.example.com", | ||
| authorization_servers: ["https://auth-server.example.com"], | ||
| })), | ||
| })); | ||
|
|
||
| // Import after mocks are set up | ||
| import { createDebugOAuthStateMachine } from "../state-machines/debug-oauth-2025-11-25"; | ||
|
|
||
| // Helper: create a state machine with custom headers and track state updates | ||
| function createTestMachine(customHeaders: Record<string, string>) { | ||
| let state: OAuthFlowState = { ...EMPTY_OAUTH_FLOW_STATE }; | ||
|
|
||
| const machine = createDebugOAuthStateMachine({ | ||
| state, | ||
| getState: () => state, | ||
| updateState: (updates) => { | ||
| state = { ...state, ...updates }; | ||
| }, | ||
| serverUrl: "https://mcp-server.example.com", | ||
| serverName: "test-server", | ||
| redirectUrl: "http://localhost:3000/oauth/callback/debug", | ||
| customHeaders, | ||
| registrationStrategy: "dcr", | ||
| }); | ||
|
|
||
| return { machine, getState: () => state }; | ||
| } |
There was a problem hiding this comment.
Align client test setup with repository mock preset policy.
This test builds bespoke mocks but does not use the shared client mock presets (mcpApiPresets, storePresets). Please switch setup to the standard preset fixtures for consistency and maintainability.
As per coding guidelines mcpjam-inspector/client/**/__tests__/*.test.{ts,tsx}: Use mock presets from client/src/test/mocks/ including mcpApiPresets and storePresets in client tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@mcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-auth-server.test.ts`
around lines 12 - 162, Replace the bespoke test mocks with the shared client
mock presets: import and apply mcpApiPresets and storePresets from
client/src/test/mocks at the top of the test (before importing
createDebugOAuthStateMachine) instead of manually mocking
"../state-machines/shared/helpers" and
"@modelcontextprotocol/sdk/client/auth.js"; remove the custom proxyFetch/helpers
and SDK mocks and rely on the preset mocks to provide proxyFetch,
generateRandomString, generateCodeChallenge, loadPreregisteredCredentials and
discoverOAuthProtectedResourceMetadata behavior so createTestMachine and
createDebugOAuthStateMachine continue to work with
serverUrl/serverName/redirectUrl/customHeaders/registrationStrategy as before.
mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.ts
Show resolved
Hide resolved
mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts
Show resolved
Hide resolved
mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.ts
Show resolved
Hide resolved
ignaciojimenezr
changed the title
ignaciojimenezr
changed the title
|
Let's fix the 4 major comments from CodeRabbit and then you're good |
ignaciojimenezr
force-pushed
the
debugging-oauth
branch
from
77c10c0 to
0c57cf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts`:
- Around line 680-687: The fetch wrapper loggingFetch used when calling
discoverOAuthProtectedResourceMetadata must avoid leaking Authorization across
cross-origin metadata hops: when resolving metadataOptions.resourceMetadataUrl
(or the URL passed into loggingFetch) compare its origin to the MCP server
origin used by your code and, if the origins differ, use
mergeHeadersForAuthServer instead of mergeHeaders to build mergedHeaders
(replace the mergeHeaders call that takes customHeaders and
normalizeHeaders(init.headers) inside loggingFetch); this origin check should
also be applied at the other occurrence around lines 748-751 that currently uses
mergeHeaders so that cross-origin requests do not carry Authorization headers.
In
`@mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.ts`:
- Around line 825-833: The loggingFetch used by
discoverOAuthProtectedResourceMetadata currently builds mergedHeaders via
mergeHeaders(customHeaders, normalizeHeaders(init.headers)) which assumes
same-origin, risking Authorization header leakage to cross-origin
resourceMetadataUrl; update loggingFetch to apply origin-aware header stripping
like mergeHeadersForAuthServer (or reuse its logic) so that Authorization and
other sensitive headers are removed when the request target origin differs from
the MCP server origin (i.e., compare resourceMetadataUrl origin to the server
origin used by loggingFetch), and ensure the same change is applied to the other
occurrence at the code referenced around lines 892–895.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f04d0052-6737-4690-82fb-53469d498f94
📒 Files selected for processing (4)
mcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-auth-server.test.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-03-26.ts
- mcpjam-inspector/client/src/lib/oauth/tests/merge-headers-auth-server.test.ts
mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts
Outdated
Show resolved
Hide resolved
mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.ts
Outdated
Show resolved
Hide resolved
ignaciojimenezr
force-pushed
the
debugging-oauth
branch
from
5a1a239 to
3a18b19
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mcpjam-inspector/client/src/lib/oauth/state-machines/shared/helpers.ts (1)
269-279:⚠️ Potential issue | 🟠 MajorDo not drop request-specific Authorization headers for auth-server calls.
At Line 273–Line 277, removing every
Authorizationheader after merge also removes intentional per-request credentials (for example,Basicauth) when they are explicitly passed inrequestHeaders. Sanitize leaked connection-level auth fromcustomHeadersbefore merging, then let request headers win.🔧 Focused fix
export function mergeHeadersForAuthServer( customHeaders: Record<string, string> | undefined, requestHeaders: Record<string, string> = {}, ): Record<string, string> { - const merged = mergeHeaders(customHeaders, requestHeaders); - for (const key of Object.keys(merged)) { - if (key.toLowerCase() === "authorization") { - delete merged[key]; - } - } - return merged; + const safeCustomHeaders: Record<string, string> = { ...(customHeaders ?? {}) }; + + for (const [key, value] of Object.entries(safeCustomHeaders)) { + if ( + key.toLowerCase() === "authorization" && + /^Bearer\s/i.test(value ?? "") + ) { + delete safeCustomHeaders[key]; + } + } + + return mergeHeaders(safeCustomHeaders, requestHeaders); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mcpjam-inspector/client/src/lib/oauth/state-machines/shared/helpers.ts` around lines 269 - 279, The current mergeHeadersForAuthServer unconditionally deletes any Authorization header from the merged result, which removes request-specific credentials; instead, remove/sanitize Authorization from the customHeaders input before merging so requestHeaders can override it: create a shallow copy of customHeaders, strip any header whose key lowercased equals "authorization" from that copy, then call mergeHeaders(copyOfCustomHeaders, requestHeaders) so any Authorization provided in requestHeaders is preserved; refer to mergeHeadersForAuthServer, mergeHeaders, customHeaders, and requestHeaders when implementing this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mcpjam-inspector/client/src/lib/oauth/state-machines/shared/helpers.ts`:
- Around line 269-279: The current mergeHeadersForAuthServer unconditionally
deletes any Authorization header from the merged result, which removes
request-specific credentials; instead, remove/sanitize Authorization from the
customHeaders input before merging so requestHeaders can override it: create a
shallow copy of customHeaders, strip any header whose key lowercased equals
"authorization" from that copy, then call mergeHeaders(copyOfCustomHeaders,
requestHeaders) so any Authorization provided in requestHeaders is preserved;
refer to mergeHeadersForAuthServer, mergeHeaders, customHeaders, and
requestHeaders when implementing this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 81ee5444-15c8-421e-9a42-663c4cd4557f
📒 Files selected for processing (4)
mcpjam-inspector/client/src/lib/oauth/__tests__/merge-headers-unit.test.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.tsmcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-11-25.tsmcpjam-inspector/client/src/lib/oauth/state-machines/shared/helpers.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- mcpjam-inspector/client/src/lib/oauth/state-machines/debug-oauth-2025-06-18.ts
3 participants
Addresses #1535
The OAuth Debugger's mergeHeaders function merges connection-level custom headers into all requests — including those going to the Authorization Server. If a user sets a custom Authorization: Bearer header (intended for the MCP server), it leaks into Authorization Server requests where Bearer tokens have no defined meaning — RFC 6750 defines them exclusively for accessing "protected resources hosted by the resource server."
In the case of SuperTokens (#1535), the token endpoint receives the unexpected Authorization: Bearer eyJhbG... header and fails with IllegalArgumentException: Illegal base64 character 20, which likely means SuperTokens is trying to base64-decode the Bearer value as client credentials.
Fix: Strip Authorization: Bearer headers from requests before they reach the Authorization Server.