fix(mcp): fix auth header handling and bump rmcp to 1.1 for stateless HTTP support#330
Open
ciaranashton wants to merge 12 commits intospacedriveapp:mainfrom
Open
fix(mcp): fix auth header handling and bump rmcp to 1.1 for stateless HTTP support#330ciaranashton wants to merge 12 commits intospacedriveapp:mainfrom
ciaranashton wants to merge 12 commits intospacedriveapp:mainfrom
Conversation
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request introduces builder-style construction patterns for MCP protocol models, adds per-server Authorization header handling for HTTP transport, exposes available MCP tool names through a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ciaranashton
changed the title
…header Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Upgrade rmcp from 0.17 to 1.1.0 for stateless Streamable HTTP server compatibility (servers with sessionIdGenerator: undefined) - Migrate to builder APIs for non_exhaustive structs (ClientInfo, Implementation, CallToolRequestParams) - Strip "Bearer " prefix before passing to auth_header() since rmcp adds it automatically via reqwest's bearer_auth() - Add system Chromium to Dockerfile for cross-arch browser support Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ciaranashton
force-pushed
the
fix/mcp-http-auth-header
branch
from
0e7ecc7 to
e598a9a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/mcp.rs (1)
366-375: Consider case-insensitive prefix stripping for robustness.The current logic only strips
"Bearer "and"bearer ", but HTTP authentication schemes are case-insensitive. A user providing"BEARER token"or"BeArEr token"would result in a malformed header like"Bearer BEARER token".♻️ Suggested fix for case-insensitive handling
if let Some(auth_value) = auth_header_value { // rmcp uses reqwest's .bearer_auth() which adds // "Bearer " automatically, so strip the prefix if // the user included it in their config. - let token = auth_value - .strip_prefix("Bearer ") - .or_else(|| auth_value.strip_prefix("bearer ")) - .unwrap_or(&auth_value); + let token = if auth_value.len() > 7 + && auth_value[..7].eq_ignore_ascii_case("bearer ") + { + &auth_value[7..] + } else { + &auth_value + }; transport_config = transport_config.auth_header(token); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp.rs` around lines 366 - 375, The auth prefix stripping is currently only handling "Bearer " and "bearer " and should be made case-insensitive; update the logic around auth_header_value where you call strip_prefix so that you detect and remove any case variation of the "Bearer " prefix (e.g. by checking the first 7 chars of auth_header_value, comparing them in a case-insensitive way such as to_ascii_lowercase() == "bearer ", and then slicing off that prefix) before calling transport_config.auth_header(token) so tokens like "BEARER token" or "BeArEr token" are normalized correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/mcp.rs`:
- Around line 366-375: The auth prefix stripping is currently only handling
"Bearer " and "bearer " and should be made case-insensitive; update the logic
around auth_header_value where you call strip_prefix so that you detect and
remove any case variation of the "Bearer " prefix (e.g. by checking the first 7
chars of auth_header_value, comparing them in a case-insensitive way such as
to_ascii_lowercase() == "bearer ", and then slicing off that prefix) before
calling transport_config.auth_header(token) so tokens like "BEARER token" or
"BeArEr token" are normalized correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf0174ce-8875-465c-8366-e0cfd2b5b3e9
⛔ Files ignored due to path filters (1)
Cargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (2)
Dockerfilesrc/mcp.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile (1)
52-53:⚠️ Potential issue | 🟡 MinorUpdate stale runtime stage comment.
Lines 52-53 still reference "Chrome runtime libraries for fetcher-downloaded Chromium" and imply Chrome is "downloaded on first browser tool use." With the system
chromiumpackage now installed, Chromium is pre-installed and the fetcher should not be triggered.📝 Suggested comment update
# ---- Runtime stage ---- -# Minimal runtime with Chrome runtime libraries for fetcher-downloaded Chromium. -# Chrome itself is downloaded on first browser tool use and cached on the volume. +# Minimal runtime with system Chromium for browser tools. +# System package works on amd64/arm64 and avoids the x86_64-only BrowserFetcher. FROM debian:bookworm-slim🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile` around lines 52 - 53, The runtime-stage comment that currently reads "Minimal runtime with Chrome runtime libraries for fetcher-downloaded Chromium" is stale; update that comment (the Dockerfile comment that references the Chrome runtime and the fetcher behavior) to state that the system "chromium" package is now installed and Chromium is pre-installed on the image, and remove any text implying Chromium is downloaded on first browser tool use or that a fetcher will be triggered; keep the comment concise and accurate about pre-installed Chromium and cached browser state.
🧹 Nitpick comments (1)
src/mcp.rs (1)
261-263: Use a non-abbreviated local name instead ofargs.Please rename
argsto a full name to match repo Rust naming conventions.♻️ Suggested rename
- if let Some(args) = arguments { - params = params.with_arguments(args); + if let Some(arguments_map) = arguments { + params = params.with_arguments(arguments_map); }As per coding guidelines "Use non-abbreviated variable names in Rust code:
queuenotq,messagenotmsg,channelnotch; common abbreviations likeconfigare acceptable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp.rs` around lines 261 - 263, The local pattern binding `args` should be renamed to a non-abbreviated name to match Rust conventions; change the `if let Some(args) = arguments { ... }` to e.g. `if let Some(arguments_value) = arguments { params = params.with_arguments(arguments_value); }`, updating the binding reference used in the call to `params.with_arguments` (symbols to locate: `arguments`, `params`, `with_arguments`).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp.rs`:
- Around line 366-375: The current logic around auth_header_value can pass
non-Bearer schemes into transport_config.auth_header(), producing malformed
headers; update the block handling auth_header_value to validate the scheme: if
auth_header_value contains no space treat it as a raw token and pass to
transport_config.auth_header(token); if it contains a space, split into scheme
and token and only accept scheme equals "Bearer" (case-insensitive) then pass
token; otherwise return/fail fast with a clear error (propagate a Result/Error)
indicating unsupported auth scheme. Ensure you reference and update the existing
auth_header_value handling and calls to transport_config.auth_header to
implement this validation and proper early error return.
---
Outside diff comments:
In `@Dockerfile`:
- Around line 52-53: The runtime-stage comment that currently reads "Minimal
runtime with Chrome runtime libraries for fetcher-downloaded Chromium" is stale;
update that comment (the Dockerfile comment that references the Chrome runtime
and the fetcher behavior) to state that the system "chromium" package is now
installed and Chromium is pre-installed on the image, and remove any text
implying Chromium is downloaded on first browser tool use or that a fetcher will
be triggered; keep the comment concise and accurate about pre-installed Chromium
and cached browser state.
---
Nitpick comments:
In `@src/mcp.rs`:
- Around line 261-263: The local pattern binding `args` should be renamed to a
non-abbreviated name to match Rust conventions; change the `if let Some(args) =
arguments { ... }` to e.g. `if let Some(arguments_value) = arguments { params =
params.with_arguments(arguments_value); }`, updating the binding reference used
in the call to `params.with_arguments` (symbols to locate: `arguments`,
`params`, `with_arguments`).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c5af2878-a90b-4d56-97d9-8bf23ae031d3
⛔ Files ignored due to path filters (1)
Cargo.tomlis excluded by!**/*.toml
📒 Files selected for processing (2)
Dockerfilesrc/mcp.rs
Add get_tool_names() to McpManager and pass connected MCP tool names into the worker_capabilities prompt template so the channel agent knows which external tools are available to workers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/mcp.rs (1)
366-375:⚠️ Potential issue | 🟠 MajorValidate and normalize auth scheme before calling
.auth_header().This still accepts values like
Basic ...(or malformed bearer values), which then becomeAuthorization: Bearer Basic ...downstream. Reject unsupported schemes and empty bearer tokens early.🔧 Suggested fix
if let Some(auth_value) = auth_header_value { - // rmcp uses reqwest's .bearer_auth() which adds - // "Bearer " automatically, so strip the prefix if - // the user included it in their config. - let token = auth_value - .strip_prefix("Bearer ") - .or_else(|| auth_value.strip_prefix("bearer ")) - .unwrap_or(&auth_value); - transport_config = transport_config.auth_header(token); + // rmcp/reqwest add the "Bearer " scheme automatically. + // Accept either a raw token or "Bearer <token>". + let token = match auth_value.split_once(char::is_whitespace) { + Some((scheme, value)) => { + if !scheme.eq_ignore_ascii_case("bearer") { + return Err(anyhow!( + "unsupported authorization scheme '{}' for server '{}'; expected Bearer token", + scheme, + self.name + )); + } + let value = value.trim(); + if value.is_empty() { + return Err(anyhow!( + "missing bearer token in authorization header for server '{}'", + self.name + )); + } + value + } + None => { + if auth_value.is_empty() { + return Err(anyhow!( + "missing bearer token in authorization header for server '{}'", + self.name + )); + } + auth_value.as_str() + } + }; + transport_config = transport_config.auth_header(token); }In rmcp 1.1.0, what format does StreamableHttpClientTransportConfig::auth_header() expect, and does the reqwest transport prepend "Bearer " automatically?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/mcp.rs` around lines 366 - 375, The code currently accepts any auth_header_value and naively strips "Bearer " prefixes, causing values like "Basic ..." to become "Authorization: Bearer Basic ..."; update the block that handles auth_header_value to validate and normalize the scheme: parse auth_header_value to split scheme and token (case-insensitive), accept only "bearer" and ensure token is non-empty, then call transport_config.auth_header(token); for unsupported schemes or empty tokens do not set auth_header (or return an error) so transport_config.auth_header() is only called with a raw bearer token; refer to auth_header_value, transport_config, and StreamableHttpClientTransportConfig::auth_header in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp.rs`:
- Around line 465-505: get_tool_names currently injects raw tool.description
into prompt-visible names and can hide the server namespace; update
get_tool_names to always include a stable server namespace (use
connection.name()/server_name combined with tool.name, e.g.
"{server_name}:{tool.name}") and stop inserting raw descriptions directly. If
you must include a description, sanitize it by
stripping/control-character/newline removal, limit to a short max length (e.g.
100 chars) and escape or remove special characters before appending in
parentheses; perform this sanitization where tool.description is read (in
get_tool_names before formatting) and use the sanitized string in the names.push
call so list_tools/tool.description cannot steer prompts or obscure server
identity.
---
Duplicate comments:
In `@src/mcp.rs`:
- Around line 366-375: The code currently accepts any auth_header_value and
naively strips "Bearer " prefixes, causing values like "Basic ..." to become
"Authorization: Bearer Basic ..."; update the block that handles
auth_header_value to validate and normalize the scheme: parse auth_header_value
to split scheme and token (case-insensitive), accept only "bearer" and ensure
token is non-empty, then call transport_config.auth_header(token); for
unsupported schemes or empty tokens do not set auth_header (or return an error)
so transport_config.auth_header() is only called with a raw bearer token; refer
to auth_header_value, transport_config, and
StreamableHttpClientTransportConfig::auth_header in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1dde19a-c504-4014-82cc-03eb6b068770
📒 Files selected for processing (5)
prompts/en/fragments/worker_capabilities.md.j2src/agent/channel.rssrc/agent/cortex_chat.rssrc/mcp.rssrc/prompts/engine.rs
Comment on lines
+465
to
+505
| /// Return namespaced tool names for all connected MCP servers. | ||
| /// | ||
| /// Used to inform the channel prompt about available MCP tools so the | ||
| /// agent knows it can delegate work that uses them. | ||
| pub async fn get_tool_names(&self) -> Vec<String> { | ||
| let connections = self | ||
| .connections | ||
| .read() | ||
| .await | ||
| .values() | ||
| .cloned() | ||
| .collect::<Vec<_>>(); | ||
|
|
||
| let mut names = Vec::new(); | ||
| for connection in connections { | ||
| if !connection.is_connected().await { | ||
| continue; | ||
| } | ||
|
|
||
| let server_name = connection.name().to_string(); | ||
| let tools = connection.list_tools().await; | ||
| for tool in tools { | ||
| let description = tool | ||
| .description | ||
| .as_ref() | ||
| .map(|d| d.as_ref().to_string()) | ||
| .unwrap_or_default(); | ||
| names.push(format!( | ||
| "{} — {}", | ||
| tool.name, | ||
| if description.is_empty() { | ||
| format!("from {}", server_name) | ||
| } else { | ||
| description | ||
| } | ||
| )); | ||
| } | ||
| } | ||
|
|
||
| names | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid injecting raw external tool descriptions into prompt-visible “tool names.”
get_tool_names() currently includes unsanitized tool.description text from external MCP servers and may omit stable server namespacing when descriptions are present. This can pollute/steer prompt behavior and makes same-named tools ambiguous across servers.
🔧 Suggested hardening
let mut names = Vec::new();
for connection in connections {
@@
let server_name = connection.name().to_string();
let tools = connection.list_tools().await;
for tool in tools {
- let description = tool
+ let namespaced_name = format!("{}::{}", server_name, tool.name);
+ let description = tool
.description
.as_ref()
- .map(|d| d.as_ref().to_string())
+ .map(|d| d.as_ref().replace(['\r', '\n'], " ").trim().to_string())
.unwrap_or_default();
- names.push(format!(
- "{} — {}",
- tool.name,
- if description.is_empty() {
- format!("from {}", server_name)
- } else {
- description
- }
- ));
+ if description.is_empty() {
+ names.push(namespaced_name);
+ } else {
+ names.push(format!("{namespaced_name} — {description}"));
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp.rs` around lines 465 - 505, get_tool_names currently injects raw
tool.description into prompt-visible names and can hide the server namespace;
update get_tool_names to always include a stable server namespace (use
connection.name()/server_name combined with tool.name, e.g.
"{server_name}:{tool.name}") and stop inserting raw descriptions directly. If
you must include a description, sanitize it by
stripping/control-character/newline removal, limit to a short max length (e.g.
100 chars) and escape or remove special characters before appending in
parentheses; perform this sanitization where tool.description is read (in
get_tool_names before formatting) and use the sanitized string in the names.push
call so list_tools/tool.description cannot steer prompts or obscure server
identity.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/mcp.rs`:
- Around line 366-388: The Authorization parsing in the block handling
auth_header_value should be made case-insensitive for the scheme and must reject
empty or missing bearer tokens; update the logic around auth_header_value /
has_scheme / token so you: 1) split the header at the first space (or treat the
whole header as token if no space) and compare the scheme case-insensitively
(e.g., lowercase) rather than only matching "Bearer " or "bearer "; 2) if a
scheme is present and it is not "bearer" bail with an error including self.name;
3) if the scheme is "bearer" ensure the following token is non-empty (bail if
empty or only whitespace); and 4) pass the validated non-empty token to
transport_config.auth_header(token).
…lation can fall back to the secrets store
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ontext_dump test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes two issues with MCP HTTP transport connections:
Auth header not sent on all requests — The
Authorizationheader was passed viacustom_headers(), which rmcp only includes on POST requests. The initial GET/SSE request went out unauthenticated, causing 401 errors. Now extracted and passed via.auth_header()which is included on all request types.Stateless HTTP servers fail to connect — rmcp 0.17 expected persistent sessions, so stateless MCP servers (those with
sessionIdGenerator: undefined) failed with "unexpected server response: expect initialized, accepted". Bumping to rmcp 1.1.0 adds proper stateless Streamable HTTP support.Changes
src/mcp.rs: ExtractAuthorizationfrom resolved headers and pass via.auth_header()instead of.custom_headers()src/mcp.rs: Validate Authorization header value before usesrc/mcp.rs: StripBearerprefix before passing to.auth_header()(rmcp adds it automatically via reqwest'sbearer_auth())src/mcp.rs: Migrate to builder APIs forClientInfo,Implementation,CallToolRequestParams(now#[non_exhaustive]in rmcp 1.1)Cargo.toml: Bump rmcp from 0.17 to 1.1Dockerfile: Add system Chromium for cross-arch browser supportTest plan