fix: Cookie header forwarding for MCP server authentication #1394
Conversation
- Add cookie header validation to auth middleware (x-forwarded-cookie and cookie) - Forward user session headers through A2A task metadata - Transform browser cookie header to x-forwarded-cookie for downstream forwarding - Include forwarded headers in MCP client cache key to prevent stale connections - Add header redaction for cookie and x-forwarded-cookie in loggers - Add security comment about not using debugLogger with sensitive headers
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
🦋 Changeset detectedLatest commit: c2fb299 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Claude finished @amikofalvy's task —— View job 🔍 Comprehensive Code Review CompleteI've conducted a thorough architectural and code quality review of this cookie header forwarding implementation. Here's my analysis: Overview AssessmentThis PR implements a comprehensive cookie header forwarding system to fix authentication issues with MCP tool calls from copilot chat. The implementation is well-structured and addresses a legitimate authentication gap. Key Strengths ✅1. Complete Flow Implementation
2. Security Considerations
3. Caching Strategy
Areas for Improvement 🔍1. Type Safety & Consistency (Medium Priority)// Current implementation uses generic Record<string, any>
metadata?: Record<string, any>Recommendation: Consider defining a more specific type for A2A task metadata: interface A2ATaskMetadata {
blocking?: boolean;
custom?: Record<string, string>;
conversationId?: string;
forwardedHeaders?: Record<string, string>;
// ... other known properties
}This would provide better type safety and make the forwarded headers pattern more explicit. 2. Error Handling & Edge Cases (Medium Priority)The code doesn't handle several edge cases: a) Header Size Limits: // Consider adding validation
if (Object.keys(forwardedHeaders).length > 0) {
const headerSize = JSON.stringify(forwardedHeaders).length;
if (headerSize > MAX_HEADER_SIZE) {
logger.warn('Forwarded headers exceed size limit, truncating');
// Handle appropriately
}
}b) Malformed Headers: No validation for header content format or potential injection attacks. c) Connection Failures: The auth middleware fallback logic could be more robust when session validation fails with forwarded cookies. 3. Documentation & Comments (Low Priority)While the security comment in
4. Code Duplication (Low Priority)The header extraction logic is duplicated across multiple files:
Recommendation: Extract into a shared utility: // utils/headerExtraction.ts
export function extractForwardedHeaders(request: HonoRequest): Record<string, string> {
const forwardedHeaders: Record<string, string> = {};
const xForwardedCookie = request.header('x-forwarded-cookie');
const authorization = request.header('authorization');
const cookie = request.header('cookie');
// Priority: x-forwarded-cookie (explicit) > cookie (browser-sent)
if (xForwardedCookie) {
forwardedHeaders['x-forwarded-cookie'] = xForwardedCookie;
} else if (cookie) {
forwardedHeaders['x-forwarded-cookie'] = cookie;
}
if (authorization) forwardedHeaders.authorization = authorization;
return forwardedHeaders;
}5. Testing Considerations (Medium Priority)This implementation would benefit from:
Architectural Assessment 🏗️Excellent Design Patterns:
Future Considerations:
Security Review 🔒Strong security practices:
Minor security note: Consider adding header sanitization to prevent potential header injection attacks, especially if these headers might be used in downstream HTTP requests. Final AssessmentThis is a well-implemented solution that addresses a legitimate authentication gap. The code follows good architectural patterns and demonstrates security awareness. The main improvements are around code organization (reducing duplication), type safety, and comprehensive error handling. Recommendation: Approve with minor improvements - The implementation is solid and production-ready, with suggested improvements being optimization opportunities rather than blocking issues. |
Summary
Fixes the authentication issue where MCP tool calls from the copilot chat were failing with "Invalid Token" errors. The root cause was that the auth middleware wasn't validating sessions via cookie headers, only via the Authorization header.
Changes
Auth Fix (Key Change)
agents-manage-api/src/middleware/auth.ts- Added cookie header validation to the auth middleware. Now checksx-forwarded-cookieandcookieheaders for session validation alongside theAuthorizationheader.Header Forwarding Infrastructure
agents-run-api/src/routes/chatDataStream.ts- Extract and transformcookie→x-forwarded-cookiefor the/api/chatrouteagents-run-api/src/routes/chat.ts- Same transformation for/v1/chat/completionsrouteagents-run-api/src/a2a/handlers.ts- Extract headers from incoming A2A requests and pass to task metadataagents-run-api/src/a2a/types.ts- AddedforwardedHeaderstoA2ATaskMetadatatypeagents-run-api/src/handlers/executionHandler.ts- AddedforwardedHeadersparam to pass headers to A2A clientagents-run-api/src/agents/generateTaskHandler.ts- Extract headers from task metadata and pass to Agentagents-run-api/src/agents/Agent.ts- Merge forwarded headers into MCP config + include in cache key to prevent stale connectionsMCP/SDK
agents-manage-api/src/routes/mcp.ts- Mapx-forwarded-cookie→cookiefor downstream API calls + security commentpackages/agents-manage-mcp/src/hooks/header-forwarding-hook.ts- Simplified header forwarding hookSecurity
packages/agents-core/src/utils/logger.ts- Added redaction forcookieandx-forwarded-cookieheadersagents-manage-ui/src/lib/logger.ts- Same header redactionFlow
cookieheader to Run APIcookie→x-forwarded-cookieand passes through A2A task metadata/mcpendpoint/mcpmapsx-forwarded-cookie→cookiefor SDK hookcookieheader to downstream API requestscookieheaderTesting
Tested end-to-end with copilot chat - MCP tool calls now authenticate successfully using the user's browser session cookie.