Uh oh!
There was an error while loading. Please reload this page.
feat: transparent session re-init on HTTP 404 for streamable HTTP - #743
Conversation
| .expect_initialized::<C::Error>() | ||
| .await?; | ||
| let new_session_id: Option<Arc<str>> = new_session_id_str.map(|s| Arc::from(s.as_str())); |
Check failure
Code scanning / CodeQL
Cleartext logging of sensitive information High
Show autofix suggestionHide autofix suggestion
Copilot Autofix
AI 6 months ago
In general, to fix cleartext logging of sensitive information, you either (a) remove the sensitive data from the log output entirely, or (b) mask/replace it with a redacted or hashed version that cannot be used to compromise security. You only log high‑level status (e.g., “session reinitialized”) rather than the exact session identifier or token.
For this specific code, the data in question is new_session_id_str / new_session_id, which likely contains a session identifier. The safe approach is to ensure that, when we log reinitialization, we do not ever include the actual session id. Since the only logging machinery visible in this file is tracing::debug, and CodeQL says that the mapping operation leads to a logging sink, the least‑intrusive fix is to add a dedicated debug message that explicitly avoids printing the session id, and not log new_session_id_str directly at all. To make the intent clear and to satisfy the analyzer, we can log only whether a session id was obtained (e.g., Some vs None), without including its content. This preserves existing functionality (session handling logic is unchanged) while removing any potential for accidentally logging the raw session identifier.
Concretely, within perform_reinitialization in crates/rmcp/src/transport/streamable_http_client.rs, right after we convert new_session_id_str into new_session_id, we’ll add a debug! call that reports only the presence/absence of the ID, not the ID value. We do not need any new imports (the file already imports tracing::debug). We avoid any change to how new_session_id is used elsewhere.
| @@ -343,6 +343,10 @@ | ||
| .await?; | ||
| let new_session_id:Option<Arc<str>> = new_session_id_str.map(|s| Arc::from(s.as_str())); | ||
| debug!( | ||
| "Reinitialization completed; new session id obtained: {}", | ||
| if new_session_id.is_some(){"yes"}else{"no"} | ||
| ); | ||
| // Start from custom_headers, then inject the negotiated MCP-Protocol-Version | ||
| // so all subsequent requests carry the right version (MCP 2025-06-18 spec). |
There was a problem hiding this comment.
This appears to be a false positive. We're not even logging the session ID in the first place. new_session_id_str is only mapped into an Arc<str> and returned.
Fixes#733
Motivation and Context
The MCP spec on Session Management states that when a client gets an HTTP 404 response to a request with
Mcp-Session-Id, it must start a new session by sending a freshInitializeRequestwithout a session ID. Before, the streamable HTTP client would show a genericUnexpectedServerResponseerror and get stuck with the old session, which goes against the spec.This PR allows the client to detect 404 responses on session-aware requests and show them as a new
SessionExpirederror. It will then automatically re-initialize by replaying the original initialize handshake, setting up a new SSE stream, and retrying the original message, all without the caller noticing. It will attempt a single retry, and if the re-initialization fails, the error will propagate normally to avoid infinite loops.How Has This Been Tested?
Added new integration tests to verify both the low-level error detection and the end-to-end transparent recovery.
Breaking Changes
StreamableHttpErrorgains a newSessionExpiredvariant. Because the enum is#[non_exhaustive], downstream code that matches on it will not break.Types of changes
Checklist
Additional context