-
Notifications
You must be signed in to change notification settings - Fork 59
Warn when sensitive credentials are forwarded to upstream API #632
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 2 changed, 0 removedBuild ID: 6fd70c556b3081b49e797c30 URL: https://www.apollographql.com/docs/deploy-preview/6fd70c556b3081b49e797c30 |
|
✅ Changeset file added - thank you! |
| } | ||
| outgoing.insert(header_name, value.clone()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Consider] The sensitive header check is case-sensitive, but HTTP header names are case-insensitive per RFC 7230. While this works correctly because reqwest normalizes header names to lowercase internally, consider using header_name.as_str().to_lowercase() to make the case-insensitivity explicit (similar to the hop-by-hop header check at line 48). This improves clarity about the behavior.
Reference: Chapter 1 (clear code beats comments)
|
|
||
| let mut incoming = HeaderMap::new(); | ||
| incoming.insert("authorization", HeaderValue::from_static("Bearer token")); | ||
| incoming.insert("cookie", HeaderValue::from_static("session=abc")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Consider] For consistency with Chapter 5.1 recommendations, consider organizing this test within a mod forward_headers block alongside the other forward_headers tests (lines 244-315). This would make the test output read as forward_headers::warns_on_sensitive_headers and group related tests together.
Reference: Chapter 5.1 - Use modules for organization
Review SummaryThis PR adds runtime warnings when sensitive credential headers (authorization, cookie, proxy-authorization, x-api-key) are forwarded to the upstream GraphQL API, helping operators identify potential security risks per MCP security best practices. The implementation is clean, focused, and well-tested. The warning is emitted only when the sensitive header is actually present in an incoming request, which is the right approach to avoid noise. Findings
Test Coverage Assessment✅ Excellent test coverage:
Final RecommendationApprove with suggestions - The implementation is solid and ready to merge. The suggestions above are minor improvements for clarity and organization, not blocking issues. Reviewed by Claude Code Sonnet 4.5 |
pragl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Docs changes LGTM!
When users configure
forward_headerswith sensitive credential headers likeAuthorization,Cookie,Proxy-Authorization, orX-Api-Key, the server now logs a warning at runtime when those headers are actually forwarded to the upstream GraphQL API. This helps operators catch unintentional credential exposure without blocking the request.Per MCP security best practices, forwarding credentials can introduce audience confusion and privilege escalation risks.
Testing
forward_headers:Authorizationheader.2026-02-10T15:16:21.994684Z WARN mcp_server{method=POST uri=/mcp status_code=200 OK}:call_tool{apollo.mcp.tool_name="GetAstronautsCurrentlyInSpace" apollo.mcp.request_id=2}: Forwarding sensitive header to upstream GraphQL API header=authorizationx-tenant-iddoes not produce a warning.