Skip to content

Commit 54b712b

Browse files
robert-inkeepmiles-kt-inkeep
robert-inkeep
and
miles-kt-inkeep
authored
Prevent forwardedHeaders from overriding MCP auth headers (#2893)
* fix: prevent forwarded headers from overriding MCP auth * feedback * tests * remove casting --------- Co-authored-by: miles-kt-inkeep <miles.kamingthanassi@inkeep.com> Co-authored-by: miles-kt-inkeep <135626743+miles-kt-inkeep@users.noreply.github.com>
1 parent 9ee6ab7 commit 54b712b

File tree

6 files changed

+316
-16
lines changed

6 files changed

+316
-16
lines changed

‎agents-api/src/__tests__/run/agents/AgentMcpManager.test.ts‎

Lines changed: 93 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,9 @@ function createMcpTool(overrides: Record<string, any> = {}): any {
7373
};
7474
}
7575

76-
function createManager(options: { credentialStuffer?: any } = {}): AgentMcpManager {
76+
function createManager(
77+
options: { credentialStuffer?: any; forwardedHeaders?: Record<string, string> } = {}
78+
): AgentMcpManager {
7779
const config = {
7880
id: 'sub-agent-1',
7981
agentId: 'parent-agent-1',
@@ -82,7 +84,7 @@ function createManager(options: { credentialStuffer?: any } = {}): AgentMcpManag
8284
name: 'Test Sub-Agent',
8385
userId: undefined,
8486
contextConfigId: undefined,
85-
forwardedHeaders: undefined,
87+
forwardedHeaders: options.forwardedHeaders,
8688
} as any;
8789

8890
const executionContext = {
@@ -205,6 +207,64 @@ describe('AgentMcpManager', () => {
205207
undefined
206208
);
207209
});
210+
211+
test('does not allow forwarded headers to override trusted Slack auth headers', async () => {
212+
const { isSlackWorkAppTool } = await import('@inkeep/agents-core');
213+
vi.mocked(isSlackWorkAppTool).mockReturnValue(true);
214+
215+
const trustedSlackTool = createMcpTool({
216+
config: {
217+
type: 'mcp',
218+
mcp: {
219+
server: { url: 'https://api.inkeep.example/work-apps/slack/mcp' },
220+
},
221+
},
222+
});
223+
224+
await createManager({
225+
forwardedHeaders: {
226+
Authorization: 'Bearer attacker-token',
227+
'x-inkeep-tenant-id': 'attacker-tenant',
228+
'x-inkeep-project-id': 'attacker-project',
229+
'x-inkeep-tool-id': 'attacker-tool',
230+
'x-custom-forwarded': 'allowed',
231+
},
232+
}).getToolSet(trustedSlackTool);
233+
234+
const headers = vi.mocked(McpClient).mock.calls[0]?.[0]?.server?.headers;
235+
expect(headers?.Authorization).toBe('Bearer test-slack-key');
236+
expect(headers?.['x-inkeep-tenant-id']).toBe('tenant-1');
237+
expect(headers?.['x-inkeep-project-id']).toBe('project-1');
238+
expect(headers?.['x-inkeep-tool-id']).toBe('test-tool-id');
239+
expect(headers?.['x-custom-forwarded']).toBe('allowed');
240+
});
241+
242+
test('blocks case-insensitive header override attempts on Slack headers', async () => {
243+
const { isSlackWorkAppTool } = await import('@inkeep/agents-core');
244+
vi.mocked(isSlackWorkAppTool).mockReturnValue(true);
245+
246+
const trustedSlackTool = createMcpTool({
247+
config: {
248+
type: 'mcp',
249+
mcp: {
250+
server: { url: 'https://api.inkeep.example/work-apps/slack/mcp' },
251+
},
252+
},
253+
});
254+
255+
await createManager({
256+
forwardedHeaders: {
257+
authorization: 'Bearer attacker-token',
258+
'X-INKEEP-TENANT-ID': 'attacker-tenant',
259+
'X-Inkeep-Project-Id': 'attacker-project',
260+
},
261+
}).getToolSet(trustedSlackTool);
262+
263+
const headers = vi.mocked(McpClient).mock.calls[0]?.[0]?.server?.headers;
264+
expect(headers?.Authorization).toBe('Bearer test-slack-key');
265+
expect(headers?.['x-inkeep-tenant-id']).toBe('tenant-1');
266+
expect(headers?.['x-inkeep-project-id']).toBe('project-1');
267+
});
208268
});
209269

210270
describe('GitHub MCP API key forwarding', () => {
@@ -240,6 +300,37 @@ describe('AgentMcpManager', () => {
240300
undefined
241301
);
242302
});
303+
304+
test('does not allow forwarded headers to override trusted GitHub auth headers', async () => {
305+
const { isGithubWorkAppTool } = await import('@inkeep/agents-core');
306+
vi.mocked(isGithubWorkAppTool).mockReturnValue(true);
307+
308+
const trustedGithubTool = createMcpTool({
309+
config: {
310+
type: 'mcp',
311+
mcp: {
312+
server: { url: 'https://api.inkeep.example/work-apps/github/mcp' },
313+
},
314+
},
315+
});
316+
317+
await createManager({
318+
forwardedHeaders: {
319+
Authorization: 'Bearer attacker-token',
320+
'x-inkeep-tenant-id': 'attacker-tenant',
321+
'x-inkeep-project-id': 'attacker-project',
322+
'x-inkeep-tool-id': 'attacker-tool',
323+
'x-custom-forwarded': 'allowed',
324+
},
325+
}).getToolSet(trustedGithubTool);
326+
327+
const headers = vi.mocked(McpClient).mock.calls[0]?.[0]?.server?.headers;
328+
expect(headers?.Authorization).toBe('Bearer test-github-key');
329+
expect(headers?.['x-inkeep-tenant-id']).toBe('tenant-1');
330+
expect(headers?.['x-inkeep-project-id']).toBe('project-1');
331+
expect(headers?.['x-inkeep-tool-id']).toBe('test-tool-id');
332+
expect(headers?.['x-custom-forwarded']).toBe('allowed');
333+
});
243334
});
244335

245336
describe('createMcpConnection error handling', () => {

‎agents-api/src/__tests__/run/handlers/executionHandler-run-as-user.test.ts‎

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,3 +200,116 @@ describe('ExecutionHandler - x-inkeep-run-as-user-id forwarding', () => {
200200
expect(headers?.['x-inkeep-run-as-user-id']).toBeUndefined();
201201
});
202202
});
203+
204+
describe('ExecutionHandler - A2A client header override protection', () => {
205+
let handler: ExecutionHandler;
206+
207+
beforeEach(() => {
208+
vi.clearAllMocks();
209+
handler = new ExecutionHandler();
210+
mockSendMessage.mockResolvedValue({
211+
result: {
212+
id: 'task-123',
213+
status: { state: 'completed' },
214+
contextId: 'test-context',
215+
artifacts: [{ parts: [{ kind: 'text', text: 'response' }] }],
216+
},
217+
});
218+
});
219+
220+
async function executeWithForwardedHeaders(forwardedHeaders: Record<string, string>) {
221+
await handler.execute({
222+
executionContext: createExecutionContext({ type: 'user', id: 'user_abc123' }) as any,
223+
conversationId: 'conv-123',
224+
userMessage: 'hello',
225+
initialAgentId: 'sub-1',
226+
requestId: 'req-123',
227+
sseHelper: createMockStreamHelper() as any,
228+
forwardedHeaders,
229+
});
230+
}
231+
232+
it('does not allow forwardedHeaders to override Authorization', async () => {
233+
await executeWithForwardedHeaders({
234+
Authorization: 'Bearer attacker-token',
235+
});
236+
const headers = getA2AClientHeaders();
237+
expect(headers?.Authorization).toBe('Bearer mock-service-token');
238+
});
239+
240+
it('does not allow forwardedHeaders to override x-inkeep-tenant-id', async () => {
241+
await executeWithForwardedHeaders({
242+
'x-inkeep-tenant-id': 'attacker-tenant',
243+
});
244+
const headers = getA2AClientHeaders();
245+
expect(headers?.['x-inkeep-tenant-id']).toBe('test-tenant');
246+
});
247+
248+
it('does not allow forwardedHeaders to override x-inkeep-project-id', async () => {
249+
await executeWithForwardedHeaders({
250+
'x-inkeep-project-id': 'attacker-project',
251+
});
252+
const headers = getA2AClientHeaders();
253+
expect(headers?.['x-inkeep-project-id']).toBe('test-project');
254+
});
255+
256+
it('does not allow forwardedHeaders to override x-inkeep-agent-id', async () => {
257+
await executeWithForwardedHeaders({
258+
'x-inkeep-agent-id': 'attacker-agent',
259+
});
260+
const headers = getA2AClientHeaders();
261+
expect(headers?.['x-inkeep-agent-id']).toBe('test-agent');
262+
});
263+
264+
it('does not allow forwardedHeaders to override x-inkeep-sub-agent-id', async () => {
265+
await executeWithForwardedHeaders({
266+
'x-inkeep-sub-agent-id': 'attacker-sub-agent',
267+
});
268+
const headers = getA2AClientHeaders();
269+
expect(headers?.['x-inkeep-sub-agent-id']).toBe('sub-1');
270+
});
271+
272+
it('blocks case-insensitive override attempts', async () => {
273+
await executeWithForwardedHeaders({
274+
authorization: 'Bearer attacker-token',
275+
'X-INKEEP-TENANT-ID': 'attacker-tenant',
276+
'X-Inkeep-Project-Id': 'attacker-project',
277+
});
278+
const headers = getA2AClientHeaders();
279+
expect(headers?.Authorization).toBe('Bearer mock-service-token');
280+
expect(headers?.['x-inkeep-tenant-id']).toBe('test-tenant');
281+
expect(headers?.['x-inkeep-project-id']).toBe('test-project');
282+
});
283+
284+
it('passes through non-conflicting forwarded headers', async () => {
285+
await executeWithForwardedHeaders({
286+
'x-forwarded-cookie': 'session=abc123',
287+
'x-inkeep-client-timezone': 'America/New_York',
288+
'x-custom-header': 'allowed-value',
289+
});
290+
const headers = getA2AClientHeaders();
291+
expect(headers?.['x-forwarded-cookie']).toBe('session=abc123');
292+
expect(headers?.['x-inkeep-client-timezone']).toBe('America/New_York');
293+
expect(headers?.['x-custom-header']).toBe('allowed-value');
294+
});
295+
296+
it('blocks all trusted headers simultaneously while allowing legitimate ones', async () => {
297+
await executeWithForwardedHeaders({
298+
Authorization: 'Bearer attacker-token',
299+
'x-inkeep-tenant-id': 'attacker-tenant',
300+
'x-inkeep-project-id': 'attacker-project',
301+
'x-inkeep-agent-id': 'attacker-agent',
302+
'x-inkeep-sub-agent-id': 'attacker-sub-agent',
303+
'x-inkeep-run-as-user-id': 'attacker-user',
304+
'x-forwarded-cookie': 'session=legit',
305+
});
306+
const headers = getA2AClientHeaders();
307+
expect(headers?.Authorization).toBe('Bearer mock-service-token');
308+
expect(headers?.['x-inkeep-tenant-id']).toBe('test-tenant');
309+
expect(headers?.['x-inkeep-project-id']).toBe('test-project');
310+
expect(headers?.['x-inkeep-agent-id']).toBe('test-agent');
311+
expect(headers?.['x-inkeep-sub-agent-id']).toBe('sub-1');
312+
expect(headers?.['x-inkeep-run-as-user-id']).toBe('user_abc123');
313+
expect(headers?.['x-forwarded-cookie']).toBe('session=legit');
314+
});
315+
});

‎agents-api/src/__tests__/run/utils/merge-headers.test.ts‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
import { describe, expect, it } from 'vitest';
2+
import { mergeHeadersWithoutOverrides } from '../../../domains/run/utils/merge-headers';
3+
4+
describe('mergeHeadersWithoutOverrides', () => {
5+
it('preserves all existing headers', () => {
6+
const result = mergeHeadersWithoutOverrides(
7+
{ Authorization: 'Bearer trusted', 'x-inkeep-tenant-id': 'tenant-1' },
8+
{}
9+
);
10+
expect(result).toEqual({
11+
Authorization: 'Bearer trusted',
12+
'x-inkeep-tenant-id': 'tenant-1',
13+
});
14+
});
15+
16+
it('adds non-conflicting forwarded headers', () => {
17+
const result = mergeHeadersWithoutOverrides(
18+
{ Authorization: 'Bearer trusted' },
19+
{ 'x-custom': 'value' }
20+
);
21+
expect(result.Authorization).toBe('Bearer trusted');
22+
expect(result['x-custom']).toBe('value');
23+
});
24+
25+
it('blocks forwarded headers that conflict with existing headers (exact case)', () => {
26+
const result = mergeHeadersWithoutOverrides(
27+
{ Authorization: 'Bearer trusted', 'x-inkeep-tenant-id': 'tenant-1' },
28+
{ Authorization: 'Bearer attacker', 'x-inkeep-tenant-id': 'attacker-tenant' }
29+
);
30+
expect(result.Authorization).toBe('Bearer trusted');
31+
expect(result['x-inkeep-tenant-id']).toBe('tenant-1');
32+
});
33+
34+
it('blocks forwarded headers with different casing (case-insensitive)', () => {
35+
const result = mergeHeadersWithoutOverrides(
36+
{ Authorization: 'Bearer trusted', 'x-inkeep-tenant-id': 'tenant-1' },
37+
{ authorization: 'Bearer attacker', 'X-INKEEP-TENANT-ID': 'attacker-tenant' }
38+
);
39+
expect(result.Authorization).toBe('Bearer trusted');
40+
expect(result['x-inkeep-tenant-id']).toBe('tenant-1');
41+
expect(result.authorization).toBeUndefined();
42+
expect(result['X-INKEEP-TENANT-ID']).toBeUndefined();
43+
});
44+
45+
it('handles undefined existing headers', () => {
46+
const result = mergeHeadersWithoutOverrides(undefined, { 'x-custom': 'value' });
47+
expect(result).toEqual({ 'x-custom': 'value' });
48+
});
49+
50+
it('handles empty forwarded headers', () => {
51+
const result = mergeHeadersWithoutOverrides({ Authorization: 'Bearer trusted' }, {});
52+
expect(result).toEqual({ Authorization: 'Bearer trusted' });
53+
});
54+
55+
it('allows non-overlapping headers while blocking overlapping ones', () => {
56+
const result = mergeHeadersWithoutOverrides(
57+
{
58+
Authorization: 'Bearer trusted',
59+
'x-inkeep-tenant-id': 'tenant-1',
60+
'x-inkeep-project-id': 'project-1',
61+
},
62+
{
63+
Authorization: 'Bearer attacker',
64+
'x-inkeep-tenant-id': 'attacker-tenant',
65+
'x-custom-forwarded': 'allowed',
66+
'x-another-custom': 'also-allowed',
67+
}
68+
);
69+
expect(result.Authorization).toBe('Bearer trusted');
70+
expect(result['x-inkeep-tenant-id']).toBe('tenant-1');
71+
expect(result['x-inkeep-project-id']).toBe('project-1');
72+
expect(result['x-custom-forwarded']).toBe('allowed');
73+
expect(result['x-another-custom']).toBe('also-allowed');
74+
});
75+
});

‎agents-api/src/domains/run/agents/services/AgentMcpManager.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import runDbClient from '../../../../data/db/runDbClient';
2121
import { env } from '../../../../env';
2222
import { getLogger } from '../../../../logger';
2323
import { agentSessionManager } from '../../session/AgentSession';
24+
import { mergeHeadersWithoutOverrides } from '../../utils/merge-headers';
2425
import { setSpanWithError, tracer } from '../../utils/tracer';
2526
import type { AgentConfig } from '../Agent';
2627
import type { AiSdkToolDefinition } from '../agent-types';
@@ -177,10 +178,10 @@ export class AgentMcpManager {
177178
}
178179

179180
if (this.config.forwardedHeaders && Object.keys(this.config.forwardedHeaders).length > 0) {
180-
serverConfig.headers = {
181-
...serverConfig.headers,
182-
...this.config.forwardedHeaders,
183-
};
181+
serverConfig.headers = mergeHeadersWithoutOverrides(
182+
serverConfig.headers,
183+
this.config.forwardedHeaders
184+
);
184185
}
185186

186187
logger.info(

‎agents-api/src/domains/run/handlers/executionHandler.ts‎

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import type { StreamHelper } from '../stream/stream-helpers.js';
2929
import { BufferingStreamHelper } from '../stream/stream-helpers.js';
3030
import { registerStreamHelper, unregisterStreamHelper } from '../stream/stream-registry.js';
3131
import { agentInitializingOp, completionOp, errorOp } from '../utils/agent-operations.js';
32+
import { mergeHeadersWithoutOverrides } from '../utils/merge-headers.js';
3233
import { resolveModelConfig } from '../utils/model-resolver.js';
3334
import { tracer } from '../utils/tracer.js';
3435

@@ -306,17 +307,18 @@ export class ExecutionHandler {
306307

307308
const appPrompt = executionContext.metadata?.appPrompt;
308309

310+
const trustedHeaders: Record<string, string> = {
311+
Authorization: `Bearer ${authToken}`,
312+
'x-inkeep-tenant-id': tenantId,
313+
'x-inkeep-project-id': projectId,
314+
'x-inkeep-agent-id': agentId,
315+
'x-inkeep-sub-agent-id': currentAgentId,
316+
...(runAsUserId ? { 'x-inkeep-run-as-user-id': runAsUserId } : {}),
317+
...(appPrompt ? { 'x-inkeep-app-prompt': appPrompt } : {}),
318+
};
319+
309320
const a2aClient = new A2AClient(agentBaseUrl, {
310-
headers: {
311-
Authorization: `Bearer ${authToken}`,
312-
'x-inkeep-tenant-id': tenantId,
313-
'x-inkeep-project-id': projectId,
314-
'x-inkeep-agent-id': agentId,
315-
'x-inkeep-sub-agent-id': currentAgentId,
316-
...(runAsUserId ? { 'x-inkeep-run-as-user-id': runAsUserId } : {}),
317-
...(appPrompt ? { 'x-inkeep-app-prompt': appPrompt } : {}),
318-
...(forwardedHeaders || {}),
319-
},
321+
headers: mergeHeadersWithoutOverrides(trustedHeaders, forwardedHeaders || {}),
320322
fetchFn: getInProcessFetch(),
321323
});
322324

‎agents-api/src/domains/run/utils/merge-headers.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
export function mergeHeadersWithoutOverrides(
2+
existingHeaders: Record<string, string> | undefined,
3+
forwardedHeaders: Record<string, string>
4+
): Record<string, string> {
5+
const mergedHeaders = { ...(existingHeaders || {}) };
6+
const existingHeaderNames = new Set(
7+
Object.keys(mergedHeaders).map((header) => header.toLowerCase())
8+
);
9+
10+
for (const [headerName, headerValue] of Object.entries(forwardedHeaders)) {
11+
if (existingHeaderNames.has(headerName.toLowerCase())) {
12+
continue;
13+
}
14+
mergedHeaders[headerName] = headerValue;
15+
}
16+
17+
return mergedHeaders;
18+
}

0 commit comments

Comments
 (0)