fix(mcp): header safety validation and Authorization conflict bug from #704#752
Conversation
github-actions
bot
added
scope: tool/mcp
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the security and robustness of the MCP client's HTTP request handling. It addresses potential vulnerabilities related to header injection and resolves a critical issue where custom authentication headers were being inadvertently overwritten. The changes are backed by new, thorough integration tests that validate the behavior at the network level, ensuring reliable and secure communication. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces important security enhancements and bug fixes for the MCP client. It adds validation against CRLF injection in custom headers, which is a crucial step to prevent HTTP header smuggling. The fix to prevent overwriting user-provided Authorization headers is also a critical correction that ensures API-key based authentication is not silently broken. The addition of wire-level integration tests using a real echo server is an excellent practice and greatly increases confidence in the correctness of the header handling logic. My review includes a suggestion to further strengthen the header validation to check for all control characters, not just CRLF, for even better security.
| if name.bytes().any(|b| b == b'\r' || b == b'\n') { | ||
| return Err(ConfigError::InvalidConfig { | ||
| reason: format!("Header name '{}' contains invalid CRLF characters", name), | ||
| }); | ||
| } | ||
| if value.bytes().any(|b| b == b'\r' || b == b'\n') { | ||
| return Err(ConfigError::InvalidConfig { | ||
| reason: format!( | ||
| "Header value for '{}' contains invalid CRLF characters", | ||
| name | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
The current validation only checks for \r and \n. To make this validation more robust against header injection and align better with HTTP specifications (RFC 7230), I suggest checking for all ASCII control characters. This provides a stronger guarantee of header safety.
| if name.bytes().any(|b| b == b'\r' || b == b'\n') { | |
| return Err(ConfigError::InvalidConfig { | |
| reason: format!("Header name '{}' contains invalid CRLF characters", name), | |
| }); | |
| } | |
| if value.bytes().any(|b| b == b'\r' || b == b'\n') { | |
| return Err(ConfigError::InvalidConfig { | |
| reason: format!( | |
| "Header value for '{}' contains invalid CRLF characters", | |
| name | |
| ), | |
| }); | |
| } | |
| if name.bytes().any(|b| b.is_ascii_control()) { | |
| return Err(ConfigError::InvalidConfig { | |
| reason: format!("Header name '{}' contains invalid control characters", name), | |
| }); | |
| } | |
| if value.bytes().any(|b| b.is_ascii_control()) { | |
| return Err(ConfigError::InvalidConfig { | |
| reason: format!( | |
| "Header value for '{}' contains invalid control characters", | |
| name | |
| ), | |
| }); | |
| } |
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Good security fix porting CRLF validation and Authorization conflict resolution from #704. Strong test coverage (unit + wire-level). Two blocking issues:
-
Validation not enforced on load path --
load_mcp_servers()andload_mcp_servers_from_db()bypass CRLF checks. A corrupted config file with CRLF in headers passes through to McpClient. Validate on load or at McpClient construction. -
Header name validation incomplete -- Only checks for
\rand\n. HTTP header names (RFC 9110) must be valid tokens -- spaces, colons, null bytes would pass validation but cause undefined behavior. Usehttp::HeaderName::from_bytes()which does the full RFC check and is already a transitive dependency via reqwest.
Non-blocking:
has_custom_auth_header()checksserver_configbut actual headers come fromself.custom_headers-- works today but indirect- Wire test comment about "last one wins" isn't guaranteed reqwest behavior for all headers
Replace hand-written CRLF checks with reqwest::header::HeaderName::from_bytes() and HeaderValue::from_str(), catching spaces, colons, null bytes, and all non-token characters that the previous validation missed. Add validation to load_mcp_servers_from() and load_mcp_servers_from_db() so corrupted configs from disk or DB are rejected at load time instead of silently flowing through to McpClient. Improve app.rs error handling to distinguish "no config" from "corrupted config" (including malformed JSON). Also fix build_request_headers() to check self.custom_headers directly instead of indirectly via server_config, and clarify the wire test comment about HeaderMap::insert replacement semantics.
reidliu41
force-pushed
the
fix/639-mcp-header-safety
branch
from
60f6a87 to
93ea7f0
Compare
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
Both blocking issues from the previous review are addressed:
-
Load-path validation --
load_mcp_servers_from()andload_mcp_servers_from_db()now callserver.validate()on every server entry. Theapp.rserror handling distinguishes validation failures (warn) from missing config (debug). Regression test covers the file-based path with a corrupted header name written directly to disk. -
RFC 9110 header validation -- Replaced the manual CRLF check with
reqwest::header::HeaderName::from_bytes()andHeaderValue::from_str(), which enforce the full RFC token grammar. Test coverage is thorough: CRLF in names and values, spaces, colons, null bytes, empty names.
Non-blocking items also resolved: build_request_headers() now checks self.custom_headers directly for Authorization (no more indirection through server_config), and the wire test comment accurately describes HeaderMap::insert replacement semantics.
CI fix commit (93ea7f0) is trivial -- id: 1 to id: Some(1) matching the McpRequest type.
No new issues found.
|
I see it's already approved — is this good to merge |
…nearai#704 (nearai#752) * fix(mcp): header safety validation and Authorization conflict bug from nearai#704 * fix(mcp): enforce RFC 9110 header validation on all config load paths Replace hand-written CRLF checks with reqwest::header::HeaderName::from_bytes() and HeaderValue::from_str(), catching spaces, colons, null bytes, and all non-token characters that the previous validation missed. Add validation to load_mcp_servers_from() and load_mcp_servers_from_db() so corrupted configs from disk or DB are rejected at load time instead of silently flowing through to McpClient. Improve app.rs error handling to distinguish "no config" from "corrupted config" (including malformed JSON). Also fix build_request_headers() to check self.custom_headers directly instead of indirectly via server_config, and clarify the wire test comment about HeaderMap::insert replacement semantics. * fix ci issue
…nearai#704 (nearai#752) * fix(mcp): header safety validation and Authorization conflict bug from nearai#704 * fix(mcp): enforce RFC 9110 header validation on all config load paths Replace hand-written CRLF checks with reqwest::header::HeaderName::from_bytes() and HeaderValue::from_str(), catching spaces, colons, null bytes, and all non-token characters that the previous validation missed. Add validation to load_mcp_servers_from() and load_mcp_servers_from_db() so corrupted configs from disk or DB are rejected at load time instead of silently flowing through to McpClient. Improve app.rs error handling to distinguish "no config" from "corrupted config" (including malformed JSON). Also fix build_request_headers() to check self.custom_headers directly instead of indirectly via server_config, and clarify the wire test comment about HeaderMap::insert replacement semantics. * fix ci issue
2 participants
fix(mcp): add CRLF header validation and fix Authorization overwrite bug
Port the unique security contributions from #704 that #721's transport
refactor did not cover.
injection sequences that could smuggle extra headers into HTTP requests.
Reject empty header names for the same reason.
overwrote any user-configured Authorization header with the OAuth
Bearer token unconditionally. Servers using API-key auth via a custom
Authorization header would silently lose their credentials. Now skip
OAuth injection when has_custom_auth_header() detects user-provided
auth (case-insensitive match).
server to verify custom headers arrive on the wire, per-request headers
override custom headers correctly, and custom Authorization is preserved
when no OAuth token is injected.