feat(mcp): support custom HTTP headers for non-OAuth MCP servers (#639)#704
feat(mcp): support custom HTTP headers for non-OAuth MCP servers (#639)#704reidliu41 wants to merge 1 commit intonearai:mainfrom
Conversation
github-actions
bot
added
scope: channel/cli
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 enhances IronClaw's compatibility by introducing the capability to configure and utilize custom HTTP headers for MCP server connections. This change broadens the range of supported authentication mechanisms beyond OAuth, enabling seamless interaction with various internal services and browser-use cases that rely on static header-based authentication. The implementation includes robust handling of header parsing, validation, and intelligent conflict resolution for Authorization headers, ensuring both functionality and security. Highlights
Changelog
Activity
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 support for custom HTTP headers for MCP servers. While the implementation includes robust validation and masking of sensitive header values in CLI output, a critical security concern was identified regarding the plaintext storage of these headers in configuration files and the database. It is strongly recommended to encrypt these headers using the existing SecretsStore or ensure restricted filesystem permissions for configuration files, aligning with best practices for handling sensitive tokens. Additionally, a minor suggestion is provided to improve code readability and maintainability in the header processing logic.
| /// Used for MCP servers that require non-OAuth authentication | ||
| /// (e.g., `X-API-Key`, `Authorization: Bearer <static-token>`). | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub headers: Option<HashMap<String, String>>, |
There was a problem hiding this comment.
Insecure Storage of Sensitive Custom Headers
Severity: Medium
Sub-category: Insecure Data Handling
Description:
Custom HTTP headers provided via the --header flag are stored in plaintext in the mcp-servers.json configuration file and the database settings table. These headers are intended for authentication (e.g., Authorization: Bearer <token>) and frequently contain sensitive API keys or session tokens. Storing them in plaintext increases the risk of credential theft if the configuration file or database is accessed by unauthorized users or compromised. While the application correctly masks these values when displaying them in the terminal, the underlying storage remains unencrypted.
Impact:
An attacker with local access to the user's filesystem or access to the database can retrieve sensitive API keys and tokens, potentially gaining unauthorized access to the associated MCP servers and services.
Remediation:
Sensitive headers should be stored in the encrypted SecretsStore instead of the plaintext configuration. Alternatively, the application should ensure that the mcp-servers.json file is created with restricted file permissions (e.g., 0600 on Unix-like systems) to prevent other users on the same machine from reading its contents.
References
- This rule highlights the necessity of supporting custom headers for service-specific requirements, which often involve sensitive authentication tokens. Consequently, the secure storage and handling of these headers are paramount to prevent credential leakage.
| let has_custom_auth = if let Some(ref config) = self.server_config | ||
| && config.has_custom_headers() | ||
| { | ||
| let has_auth = config.has_custom_auth_header(); | ||
| if let Some(ref headers) = config.headers { | ||
| for (name, value) in headers { | ||
| // Headers were validated at config time (CRLF-safe) | ||
| req_builder = req_builder.header(name.as_str(), value.as_str()); | ||
| } | ||
| } | ||
| has_auth | ||
| } else { | ||
| false | ||
| }; |
There was a problem hiding this comment.
This block for adding custom headers and checking for a custom Authorization header is a bit complex, as it mixes a side effect (modifying req_builder) inside an expression. This can be refactored for better readability and maintainability by separating the action of adding headers from the check for the authorization header.
let has_custom_auth = if let Some(config) = self.server_config.as_ref() {
if let Some(headers) = &config.headers {
for (name, value) in headers {
// Headers were validated at config time (CRLF-safe)
req_builder = req_builder.header(name.as_str(), value.as_str());
}
config.has_custom_auth_header()
} else {
false
}
} else {
false
};
zmanian
left a comment
zmanian
left a comment
There was a problem hiding this comment.
The feature is well-implemented -- CRLF injection prevention, case-insensitive Authorization conflict detection, header value masking, and wire-level integration tests are all solid work.
However, this PR has heavy conflicts with #721 (MCP transport abstraction by ilblackdragon). Both PRs touch the same 5 files and add custom headers support with different designs:
- #704:
Option<HashMap<String, String>> - #721:
HashMap<String, String>with#[serde(default, skip_serializing_if)]
Recommendation: Merge #721 first, then port #704's unique contributions as a follow-up PR:
- CRLF header validation -- #721 doesn't validate header names/values at config time
has_custom_auth_header()-- #721'sbuild_request_headers()unconditionally overwrites customAuthorizationheaders with OAuth tokens. This is a bug that #704's approach fixes correctly.- CLI
--header "Name:Value"argument and parsing - Header value masking in
mcp list --verbose - Wire-level echo server tests
The Authorization conflict bug in #721 should be filed separately -- your has_custom_auth_header() logic is the correct approach.
Also: The masking code in src/cli/mcp.rs uses byte-index slicing (&value[..2]), which can panic on multi-byte characters per the project's UTF-8 safety rules. Use char_indices() or is_char_boundary().
|
Closing in favor of a focused follow-up PR. #721 (MCP transport abstraction) has been merged and already covers custom headers support The unique contributions from #704 that #721 doesn't cover will be ported in a new PR based on
Now let me implement the new PR #752. |
…#704 (#752) * fix(mcp): header safety validation and Authorization conflict bug from #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
…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
Many MCP servers (Browser-Use, internal services) authenticate via
static HTTP headers rather than OAuth. Previously these servers could
not be connected because IronClaw only supported the OAuth flow.
Add a
headersfield to McpServerConfig that lets users declarearbitrary HTTP headers injected into every MCP request. Key design
decisions:
has_custom_headers()is independent ofrequires_auth()soexisting OAuth semantics are untouched
auth header injection
injection
--header "Name:Value"arg,mcp testheader-only passthrough,and
mcp list --verbosevalue maskingUsage:
ironclaw mcp add browser-use https://api.browser-use.com
--header "Authorization:Bearer sk-xxx"
Fixes #639