fix(mcp): forward HTTP auth to internal tool fetches - #5218
diegosouzapw merged 2 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an HTTP authentication context for the Model Context Protocol (MCP) server using AsyncLocalStorage to forward credentials (such as bearer tokens, cookies, and API keys) to internal fetches. Feedback on the changes highlights a critical header merging order issue in both server.ts and advancedTools.ts where system-wide API keys can overwrite forwarded user credentials. Additionally, the new test file violates the repository style guide regarding file placement and should be moved to the tests/ directory.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const headers: Record<string, string> = { | ||
| "Content-Type": "application/json", | ||
| ...getMcpHttpAuthHeadersForInternalFetch(), | ||
| ...(apiKey ? { Authorization: `Bearer ${apiKey}` } : {}), | ||
| ...((options.headers as Record<string, string>) || {}), | ||
| }; |
There was a problem hiding this comment.
The current header merging order causes the forwarded Authorization header from getMcpHttpAuthHeadersForInternalFetch() to be overwritten by the default system-wide apiKey (if configured). To ensure forwarded user credentials take precedence over the system-wide API key, place the default Authorization header before spreading the forwarded headers.
| const headers: Record<string, string> = { | |
| "Content-Type": "application/json", | |
| ...getMcpHttpAuthHeadersForInternalFetch(), | |
| ...(apiKey ? { Authorization: `Bearer ${apiKey}` } : {}), | |
| ...((options.headers as Record<string, string>) || {}), | |
| }; | |
| const headers: Record<string, string> = { | |
| "Content-Type": "application/json", | |
| ...(apiKey ? { Authorization: `Bearer ${apiKey}` } : {}), | |
| ...getMcpHttpAuthHeadersForInternalFetch(), | |
| ...((options.headers as Record<string, string>) || {}), | |
| }; |
| const headers: Record<string, string> = { | ||
| "Content-Type": "application/json", | ||
| ...getMcpHttpAuthHeadersForInternalFetch(), | ||
| ...(OMNIROUTE_API_KEY ? { Authorization: `Bearer ${OMNIROUTE_API_KEY}` } : {}), | ||
| ...((options.headers as Record<string, string>) || {}), | ||
| }; |
There was a problem hiding this comment.
The current header merging order causes the forwarded Authorization header from getMcpHttpAuthHeadersForInternalFetch() to be overwritten by the default system-wide OMNIROUTE_API_KEY (if configured). To ensure forwarded user credentials take precedence over the system-wide API key, place the default Authorization header before spreading the forwarded headers.
| const headers: Record<string, string> = { | |
| "Content-Type": "application/json", | |
| ...getMcpHttpAuthHeadersForInternalFetch(), | |
| ...(OMNIROUTE_API_KEY ? { Authorization: `Bearer ${OMNIROUTE_API_KEY}` } : {}), | |
| ...((options.headers as Record<string, string>) || {}), | |
| }; | |
| const headers: Record<string, string> = { | |
| "Content-Type": "application/json", | |
| ...(OMNIROUTE_API_KEY ? { Authorization: `Bearer ${OMNIROUTE_API_KEY}` } : {}), | |
| ...getMcpHttpAuthHeadersForInternalFetch(), | |
| ...((options.headers as Record<string, string>) || {}), | |
| }; |
| @@ -0,0 +1,143 @@ | |||
| import { describe, expect, it, vi } from "vitest"; | |||
There was a problem hiding this comment.
According to the Repository Style Guide (Rule 1, File Placement & Organization), all unit tests, integration tests, ecosystem tests, or Vitest files must strictly be placed within the tests/ directory (e.g., tests/unit/, tests/integration/). Please move this test file to the tests/ directory (e.g., tests/unit/mcp-server/httpAuthContext.test.ts).
References
- All unit tests, integration tests, ecosystem tests, or Vitest files must strictly be placed within the
tests/directory. (link)
1a6308f to
a59f05f
Compare
|
Thanks @KooshaPari! Rebased your two commits cleanly onto the current release tip (the PR was base-stale, inflating the diff). The Note: precedence is |
3ee53cd
into
diegosouzapw:release/v3.8.40
Forward MCP HTTP auth to internal tool fetches via AsyncLocalStorage (diegosouzapw#5211). Rebased onto release tip. Integrated into release/v3.8.40.
No description provided.