Prevent forwardedHeaders from overriding MCP auth headers#2893
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Fixes a header-injection vulnerability where caller-supplied Key changes
Summary ｜ 6 files ｜ 5 commits ｜ base: Case-insensitive header merge utility
A2A client header protection in
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Severity: medium-high — the MCP fix is correct but the same vulnerability exists in another path.
The mergeHeadersWithoutOverrides helper and its application in AgentMcpManager are well-implemented. However, the same class of bug — untrusted forwardedHeaders overriding trusted auth headers via object spread — also exists in executionHandler.ts:318, where forwardedHeaders is spread after Authorization and x-inkeep-* headers in the A2A client construction. Since forwardedHeaders can originate from the unfiltered body.forwardedHeaders in the /executions route, this is an equivalent spoofing vector that should be addressed in the same PR.
Claude Opus ｜ 𝕏
| mergedHeaders[headerName] = headerValue; | ||
| } | ||
|
|
||
| return mergedHeaders; |
There was a problem hiding this comment.
The helper is clean and correct. One minor observation: the existingHeaders parameter type is Record<string, string> | undefined but forwardedHeaders is non-optional. This is fine since the call site guards with Object.keys(...).length > 0, but consider whether a forwardedHeaders: undefined guard is ever needed at the call site vs. inside the function for defensive robustness.
| const headers = vi.mocked(McpClient).mock.calls[0]?.[0]?.server?.headers; | ||
| expect(headers?.Authorization).toBe('Bearer test-slack-key'); | ||
| expect(headers?.['x-inkeep-tenant-id']).toBe('tenant-1'); | ||
| expect(headers?.['x-custom-forwarded']).toBe('allowed'); |
There was a problem hiding this comment.
Good coverage for the Slack path. Consider also adding a test for the GitHub work-app tool path (isGithubWorkAppTool) — it uses the same header injection pattern at lines 156-162 and the same mergeHeadersWithoutOverrides call protects it, but an explicit test would prevent future regressions if those code paths diverge.
| ...this.config.forwardedHeaders, | ||
| }; | ||
| serverConfig.headers = mergeHeadersWithoutOverrides( | ||
| serverConfig.headers as Record<string, string> | undefined, |
There was a problem hiding this comment.
The as Record<string, string> | undefined cast is safe here since serverConfig.headers is typed as Record<string, string> in practice, but it papers over a type mismatch. If the upstream McpServerConfig.headers type changes, this cast will silently suppress errors. Not blocking, but worth a follow-up to tighten the type.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(4) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
🔴 1) executionHandler.ts:309-321 Same header override vulnerability exists in A2A client construction
Issue: The exact same vulnerability pattern this PR fixes in AgentMcpManager.ts exists in executionHandler.ts. On lines 309-321, forwardedHeaders is spread AFTER trusted auth headers when constructing the A2A client:
const a2aClient = new A2AClient(agentBaseUrl, {
headers: {
Authorization: `Bearer ${authToken}`,
'x-inkeep-tenant-id': tenantId,
'x-inkeep-project-id': projectId,
'x-inkeep-agent-id': agentId,
'x-inkeep-sub-agent-id': currentAgentId,
...(runAsUserId ? { 'x-inkeep-run-as-user-id': runAsUserId } : {}),
...(appPrompt ? { 'x-inkeep-app-prompt': appPrompt } : {}),
...(forwardedHeaders || {}), // ← Can override ALL headers above!
},
...
});In JavaScript object spread, later properties override earlier ones with the same key. Since forwardedHeaders comes from the API request body without sanitization, an attacker with API key access could override Authorization, x-inkeep-tenant-id, x-inkeep-project-id, etc.
Why: This could allow cross-tenant requests or privilege escalation within internal A2A communication. The attack surface is identical to what this PR fixes in MCP server requests.
Fix: Apply the same mergeHeadersWithoutOverrides() pattern:
import { mergeHeadersWithoutOverrides } from '../agents/services/AgentMcpManager';
// Then in the A2AClient construction:
const trustedHeaders = {
Authorization: `Bearer ${authToken}`,
'x-inkeep-tenant-id': tenantId,
'x-inkeep-project-id': projectId,
'x-inkeep-agent-id': agentId,
'x-inkeep-sub-agent-id': currentAgentId,
...(runAsUserId ? { 'x-inkeep-run-as-user-id': runAsUserId } : {}),
...(appPrompt ? { 'x-inkeep-app-prompt': appPrompt } : {}),
};
const a2aClient = new A2AClient(agentBaseUrl, {
headers: mergeHeadersWithoutOverrides(trustedHeaders, forwardedHeaders || {}),
fetchFn: getInProcessFetch(),
});Alternatively, consider exporting mergeHeadersWithoutOverrides from a shared utility module since it's now needed in multiple places.
Refs:
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
AgentMcpManager.test.ts:236Missing test for GitHub MCP header override protection
🟡 Minor (2) 🟡
Inline Comments:
- 🟡 Minor:
AgentMcpManager.test.ts:225Test doesn't verify case-insensitive header blocking
🟡 1) AgentMcpManager.test.ts:225-228 Test assertions don't cover all protected headers
Issue: The test verifies Authorization and x-inkeep-tenant-id cannot be overridden, but the implementation injects four trusted headers: Authorization, x-inkeep-tenant-id, x-inkeep-project-id, and x-inkeep-tool-id.
Why: If the merge function fails to protect project-id or tool-id, an attacker could access resources from other projects or tools.
Fix: Extend forwardedHeaders and assertions:
forwardedHeaders: {
Authorization: 'Bearer attacker-token',
'x-inkeep-tenant-id': 'attacker-tenant',
'x-inkeep-project-id': 'attacker-project',
'x-inkeep-tool-id': 'attacker-tool',
'x-custom-forwarded': 'allowed',
},
// ...
expect(headers?.['x-inkeep-project-id']).toBe('project-1');
expect(headers?.['x-inkeep-tool-id']).toBe('test-tool-id');Refs:
🚫 REQUEST CHANGES
Summary: The fix to AgentMcpManager.ts is well-implemented with proper case-insensitive header protection. However, the same vulnerability exists in executionHandler.ts (lines 309-321) where forwardedHeaders can override trusted A2A auth headers. This is a critical security issue that should be fixed in the same PR since it's the identical attack pattern. Additionally, test coverage should be expanded to cover GitHub MCP endpoints and case-insensitive blocking.
Discarded (0)
No findings were discarded.
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
2 | 1 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
4 | 0 | 0 | 0 | 2 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-appsec |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 6 | 1 | 0 | 0 | 2 | 0 | 1 |
Note: The security-iam reviewer correctly identified the executionHandler.ts vulnerability as a related but pre-existing issue. This was elevated to Critical because it's the exact same attack pattern being fixed and should be addressed together.
| expect(headers?.Authorization).toBe('Bearer test-slack-key'); | ||
| expect(headers?.['x-inkeep-tenant-id']).toBe('tenant-1'); | ||
| expect(headers?.['x-custom-forwarded']).toBe('allowed'); | ||
| }); |
There was a problem hiding this comment.
🟠 MAJOR: Missing test for GitHub MCP header override protection
Issue: This test validates Slack MCP header protection, but no equivalent test exists for GitHub MCP endpoints which inject the same sensitive headers (Authorization, x-inkeep-tenant-id, x-inkeep-project-id, x-inkeep-tool-id).
Why: Both Slack and GitHub code paths (lines 148-163 and 165-176 in AgentMcpManager.ts) inject trusted headers before the mergeHeadersWithoutOverrides call. If someone accidentally modifies the GitHub path differently, there would be no regression test to catch it.
Fix: Add an equivalent test in the 'GitHub MCP API key forwarding' describe block:
test('does not allow forwarded headers to override trusted GitHub auth headers', async () => {
const { isGithubWorkAppTool } = await import('@inkeep/agents-core');
vi.mocked(isGithubWorkAppTool).mockReturnValue(true);
const trustedGithubTool = createMcpTool({
config: {
type: 'mcp',
mcp: {
server: { url: 'https://api.inkeep.example/work-apps/github/mcp' },
},
},
});
await createManager({
forwardedHeaders: {
Authorization: 'Bearer attacker-token',
'x-inkeep-tenant-id': 'attacker-tenant',
'x-custom-forwarded': 'allowed',
},
}).getToolSet(trustedGithubTool);
const headers = vi.mocked(McpClient).mock.calls[0]?.[0]?.server?.headers;
expect(headers?.Authorization).toBe('Bearer test-github-key');
expect(headers?.['x-inkeep-tenant-id']).toBe('tenant-1');
expect(headers?.['x-custom-forwarded']).toBe('allowed');
});Refs:
| }); | ||
|
|
||
| await createManager({ | ||
| forwardedHeaders: { |
There was a problem hiding this comment.
🟡 Minor: Test doesn't verify case-insensitive header blocking
Issue: The mergeHeadersWithoutOverrides() function uses case-insensitive comparison (toLowerCase()), but this test only uses exact-case header names. HTTP headers are case-insensitive per RFC 7230, so an attacker could attempt authorization or AUTHORIZATION to bypass protection.
Why: If the case-insensitive logic is accidentally removed during refactoring, this test wouldn't catch the regression.
Fix: Add a test case for case variations:
test('blocks forwarded headers regardless of casing (case-insensitive)', async () => {
const { isSlackWorkAppTool } = await import('@inkeep/agents-core');
vi.mocked(isSlackWorkAppTool).mockReturnValue(true);
const trustedSlackTool = createMcpTool({
config: {
type: 'mcp',
mcp: {
server: { url: 'https://api.inkeep.example/work-apps/slack/mcp' },
},
},
});
await createManager({
forwardedHeaders: {
authorization: 'Bearer attacker-token', // lowercase
'X-INKEEP-TENANT-ID': 'attacker-tenant', // uppercase
},
}).getToolSet(trustedSlackTool);
const headers = vi.mocked(McpClient).mock.calls[0]?.[0]?.server?.headers;
expect(headers?.Authorization).toBe('Bearer test-slack-key');
expect(headers?.['x-inkeep-tenant-id']).toBe('tenant-1');
});Refs:
Ito Test Report ✅10 test cases ran. 10 passed. The unified local verification run passed all 10 of 10 executed test cases with zero failures, and no confirmed production-code defects were found; a separate durable-execution lifecycle case was investigated but excluded because evidence indicated environment credential dependency rather than an application bug. Key findings were that streaming and non-stream chat paths remained stable across turns, forwarded-header hardening correctly blocked attempts to override trusted MCP authorization/tool identity while still allowing additive headers, and resilience held under large/malicious headers, reconnect and rapid double-submit scenarios, and mobile back/forward resend flows without crashes, cross-run contamination, or auth/context leakage (with noted local fallback/bypass setup in some non-production checks). ✅ Passed (10)
Commit: Tell us how we did: Give Ito Feedback |
miles-kt-inkeep
force-pushed
the
codex/fix-durable-execution-untrusted-header-vulnerability
branch
from
0d387be to
278b53f
Compare
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
This PR successfully addresses the header-injection vulnerability from the previous review. The implementation is correct and secure. One test coverage gap remains.
Previous Feedback Status
All 4 issues from the prior review have been addressed:
- ✅ Critical:
executionHandler.tsvulnerability → Fixed withmergeHeadersWithoutOverrides() - ✅ Major: Missing GitHub MCP test → Added (lines 304-333)
- ✅ Minor: Case-insensitive test missing → Added (lines 242-267)
- ✅ Minor: Test assertions incomplete → Now covers all 4 protected headers
🟠⚠️ Major (1) 🟠⚠️
🟠 1) executionHandler.ts:321 Missing test coverage for A2A header override protection
Issue: The PR fixes the header-injection vulnerability in executionHandler.ts using mergeHeadersWithoutOverrides(), but there are no regression tests verifying this fix in the A2A client path. The MCP path has comprehensive tests (Slack + GitHub + case-insensitive), but the A2A path does not.
Why: Without a regression test, a future refactor could reintroduce the vulnerability by changing the merge order back to {...trustedHeaders, ...forwardedHeaders}. The A2A path handles internal agent-to-agent communication with service tokens, making header spoofing a cross-tenant and privilege escalation risk. This creates an asymmetric coverage gap where the same class of vulnerability is tested in one location but not the other.
Fix: Add tests in a new file or extend executionHandler-run-as-user.test.ts (which already has infrastructure for capturing A2A client headers via getA2AClientHeaders()):
describe('ExecutionHandler - forwardedHeaders security', () => {
it('does not allow forwardedHeaders to override trusted headers', async () => {
await handler.execute({
// ... existing params
forwardedHeaders: {
Authorization: 'Bearer attacker-token',
'x-inkeep-tenant-id': 'attacker-tenant',
'x-inkeep-project-id': 'attacker-project',
},
});
const headers = getA2AClientHeaders();
expect(headers?.Authorization).toBe('Bearer mock-service-token');
expect(headers?.['x-inkeep-tenant-id']).toBe('test-tenant');
expect(headers?.['x-inkeep-project-id']).toBe('test-project');
});
it('allows non-conflicting forwardedHeaders through', async () => {
await handler.execute({
// ... existing params
forwardedHeaders: { 'x-custom-header': 'allowed-value' },
});
expect(getA2AClientHeaders()?.['x-custom-header']).toBe('allowed-value');
});
it('blocks case-insensitive header override attempts', async () => {
await handler.execute({
// ... existing params
forwardedHeaders: {
authorization: 'Bearer attacker',
'X-INKEEP-TENANT-ID': 'attacker',
},
});
const headers = getA2AClientHeaders();
expect(headers?.Authorization).toBe('Bearer mock-service-token');
expect(headers?.['x-inkeep-tenant-id']).toBe('test-tenant');
});
});Refs:
- executionHandler.ts:310-321 — fix location
- executionHandler-run-as-user.test.ts — existing test infrastructure
💭 Consider (1) 💭
💭 1) merge-headers.ts Consider adding JSDoc for the utility function
Issue: The mergeHeadersWithoutOverrides() function is a security-critical utility but lacks documentation explaining its purpose and behavior.
Why: Future maintainers may not understand why this function exists or accidentally modify it incorrectly.
Fix: Add a brief JSDoc comment explaining the security rationale.
💡 APPROVE WITH SUGGESTIONS
Summary: Excellent security fix! The mergeHeadersWithoutOverrides() implementation is correct and secure — case-insensitive comparison, no prototype pollution vectors, and proper handling of undefined inputs. All feedback from the previous review has been addressed. The only remaining item is adding regression tests for the executionHandler.ts A2A path to match the comprehensive test coverage already in place for the MCP path. This is a non-blocking suggestion given the security fix is correct.
Discarded (0)
No findings were discarded.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-appsec |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
| Total | 1 | 1 | 0 | 0 | 0 | 0 | 0 |
Note: AppSec reviewer confirmed the fix is complete with no bypass vectors. Tests reviewer identified the A2A coverage gap.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This re-review covers the delta since the last automated review (commit 278b53f12ca2 → b9957f696945).
Previous Feedback Status
The single Major finding from the prior review has been addressed:
- ✅ Major: Missing test coverage for A2A header override protection → Added (lines 204-315 in
executionHandler-run-as-user.test.ts)
Delta Analysis
The new ExecutionHandler - A2A client header override protection test suite (113 lines) provides comprehensive coverage:
| Test | Protection Verified |
|---|---|
| Individual header tests | Authorization, x-inkeep-tenant-id, x-inkeep-project-id, x-inkeep-agent-id, x-inkeep-sub-agent-id |
| Case-insensitive test | Mixed case variants (authorization, X-INKEEP-TENANT-ID, X-Inkeep-Project-Id) |
| Pass-through test | Non-conflicting headers (x-forwarded-cookie, x-inkeep-client-timezone, x-custom-header) |
| Combined scenario | All trusted headers + legitimate headers simultaneously |
The tests correctly verify:
- Trusted headers cannot be overridden by
forwardedHeaders(security) - Non-conflicting headers pass through (functionality)
- Case-insensitive blocking works per RFC 7230 (completeness)
✅ APPROVE
Summary: All previous feedback has been addressed. The A2A header override protection now has comprehensive regression test coverage matching the MCP path tests. The security fix is complete and well-tested. 🎉
Note: Unable to submit formal APPROVE verdict due to GitHub API permissions. This is a recommendation to approve.
Discarded (0)
No findings were discarded.
Reviewers (1)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator-direct |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta scope (test-only additions) was small enough for direct orchestrator analysis without dispatching sub-reviewers.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This re-review covers the delta since the last automated review (commit b9957f696945 → fa23e2ccb5a9).
Delta Analysis
The changes since the last APPROVE review are minimal:
| Change | File | Impact |
|---|---|---|
| Removed type cast | AgentMcpManager.ts:182 |
Code quality improvement — serverConfig.headers is now properly typed, eliminating the need for the as Record<string, string> | undefined cast |
| Merge from main | Multiple files | Brings in unrelated CSP/runtime config changes already reviewed in their respective PRs (#2896, #2898, #2899) |
Security Fix Status
The header-injection vulnerability fix is complete and unchanged:
- ✅
mergeHeadersWithoutOverrides()correctly implements case-insensitive header blocking - ✅ Applied in
AgentMcpManager.ts(MCP path) - ✅ Applied in
executionHandler.ts(A2A path) - ✅ Comprehensive test coverage for all protected headers
- ✅ Case-insensitive blocking tests per RFC 7230
✅ APPROVE
Summary: The delta since the last review is trivial — a type cast removal that improves code quality without changing behavior. The security fix remains complete and well-tested. Ship it! 🚀
Note: Unable to submit formal APPROVE verdict due to GitHub App permissions. This is a recommendation to approve.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
orchestrator-direct |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Delta scope (1 line type cast removal + main merge) was trivial enough for direct orchestrator analysis without dispatching sub-reviewers.
miles-kt-inkeep
deleted the
codex/fix-durable-execution-untrusted-header-vulnerability
branch
|
No documentation updates needed for this PR. This is an internal security fix that hardens header merging behavior without changing any public APIs or developer-facing behavior. |
Ito Test Report ❌14 test cases ran. 1 failed, 13 passed. Across 14 test cases, 13 passed and 1 failed, showing broad stability of run/chat routes, SSE and non-streaming behavior, mobile flow, MCP execution, and security hardening (trusted auth/scope headers resisted override and spoof attempts while additive forwarded metadata remained accepted, with no scope leakage after interrupted streaming). The key finding is one confirmed high-severity production reliability bug in /run/api/executions: parallel requests reusing the same conversationId can intermittently return HTTP 500 due to a pre-existing non-atomic read-then-insert conversation creation path lacking an atomic upsert/ON CONFLICT safeguard. ❌ Failed (1)
|
| Category | Summary | Screenshot |
|---|---|---|
| Adversarial | Confirmed Authorization override attempts cannot poison internal A2A auth; trusted token/header precedence is preserved. |  |
| Adversarial | Baseline and spoofed forwarded project header requests both stayed stable at HTTP 200 SSE, with no cross-scope reroute observed. |  |
| Adversarial | Refresh/back-forward interruption plus spoofed-scope follow-up showed no scope leakage; spoofed project was rejected and valid scope continued. |  |
| Edge | Confirmed no product defect in header hardening; case-insensitive trusted-header collisions are blocked and targeted coverage passed. |  |
| Edge | Confirmed selective merge behavior is correct; trusted collisions are ignored while benign additive headers are preserved. |  |
| Edge | Omitted and empty forwardedHeaders variants behaved as no-op equivalents to baseline. |
 |
| Edge | Cookie precedence remained deterministic across cookie-only, forwarded-only, and dual-cookie requests. |  |
| Edge | Mobile viewport run/chat flow and metadata-bearing request path remained functional after environment remediation. |  |
| Happy-path | Durable baseline execution succeeded with SSE output and no forwarded header overrides. |  |
| Happy-path | Additive forwarded headers were preserved without trusted-header collision or auth/scope regression. |  |
| Happy-path | OpenAI-compatible SSE chat completed with HTTP 200 and streaming chunks after valid request payload. |  |
| Happy-path | Vercel chat JSON mode returned HTTP 200 with valid choices content and stop finish reason. |  |
| Happy-path | Source and tests confirm additive MCP header merge without trusted-header override regression. |  |
Commit: fa23e2c
Tell us how we did: Give Ito Feedback
2 participants
Motivation
forwardedHeadersand merged them into MCP server requests after injecting trusted auth and tenant/project headers, allowing API-key holders to spoof or override sensitive headers.Description
mergeHeadersWithoutOverrides(existingHeaders, forwardedHeaders)to merge forwarded headers only when the header name does not already exist (case-insensitive), preserving injected trusted headers.agents-api/src/domains/run/agents/services/AgentMcpManager.tswith the new merge helper to prevent overrides ofAuthorization,x-inkeep-tenant-id,x-inkeep-project-id,x-inkeep-tool-id, etc.agents-api/src/__tests__/run/agents/AgentMcpManager.test.tsthat verifies trusted Slack MCPAuthorizationand tenant headers cannot be replaced byforwardedHeaderswhile allowing additive custom headers.createManager()to acceptforwardedHeadersto exercise the path under test.Testing
pnpm --filter agents-api exec biome check --write src/domains/run/agents/services/AgentMcpManager.ts src/__tests__/run/agents/AgentMcpManager.test.ts, which completed successfully.pnpm --filter agents-api exec vitest --run --config vitest.config.ts src/__tests__/run/agents/AgentMcpManager.test.ts, but the test run failed in this environment due to unresolved package entry for@inkeep/agents-core(monorepo build/setup required), so the regression could not be executed end-to-end here.Codex Task