Skip to content

Fix: streamable-http server: a supplied Mcp-Method contradicting an initialize body is silently accepted - #1275

Merged
DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
SIDDARTHAREDDY8:oss-bhai-2026-09-15
Sep 24, 2026
Merged

DaleSeo merged 1 commit into
modelcontextprotocol:mainfrom
SIDDARTHAREDDY8:oss-bhai-2026-09-15

Conversation

@SIDDARTHAREDDY8

Copy link
Copy Markdown
Contributor

Fixes #1271.

Implemented the fix for modelcontextprotocol/rust-sdk issue #1271 in crates/rmcp/src/transport/streamable_http_server/tower.rs. In validate_standard_headers, the InitializeRequest arm no longer returns Ok(()) blindly: a supplied Mcp-Method header is now checked against the body method (initialize), tolerating absence but rejecting contradiction with a header_mismatch JSON-RPC error (HTTP 400), mirroring the existing validate_header_matches_init_body pattern used for MCP-Protocol-Version. Added HEADER_MCP_METHOD to the http_header imports, updated the function docstring, and added 3 unit tests in a new standard_header_init_tests module: header-contradicting (rejected), header-absent (accepted), header-matching (accepted). Pushed to SIDDARTHAREDDY8/rust-sdk branch oss-bhai-2026-09-15 (commit 508d989) against upstream base main, matching the repo CONTRIBUTING-required base. No Rust toolchain in this environment, so tests were not executed locally and must run in CI. PR-body note for the publisher: this change was developed with AI assistance; disclose that honestly (e.g. an Assisted-by trailer or PR-body statement) since maintainers here actively check AI-use disclosure.

Assisted-by: AI agent (Bhai), reviewed by maintainer of the contributing account.

Tests could not be run in this environment: No Rust toolchain exists in this environment, so cargo test could not be run; the 3 new unit tests must run in CI. Static verification was done instead (API signatures confirmed against upstream source, HEADER_MCP_METHOD import confirmed, brace/paren balance checked, pushed fork blob re-fetched and confirmed to contain the new initialize arm and all 3 tests).

@SIDDARTHAREDDY8
SIDDARTHAREDDY8 requested a review from a team as a code owner September 15, 2026 15:55
@github-actions github-actions Bot added T-core Core library changes T-transport Transport layer changes labels Sep 15, 2026
@SIDDARTHAREDDY8

Copy link
Copy Markdown
Contributor Author

Fixed the Code Formatting check failure: ran nightly cargo fmt on crates/rmcp/src/transport/streamable_http_server/tower.rs — the two multiline assert!(validate_standard_headers(...)) calls fit within max_width = 100, so they were collapsed to single lines. No logic changed (commit 13c3fd5 stacked on 508d989).

Copy link
Copy Markdown

AI-assisted source review of 13c3fd544b0855062a65f525bb6c4bc9d0ec38dd; the regression cases below have not been compiled or run locally.

The new initialize check still has two edge cases in headers.get(HEADER_MCP_METHOD).and_then(|value| value.to_str().ok()):

  1. HeaderMap::get returns only the first value. Appending tools/list after initialize therefore leaves a contradictory second value unchecked; reversing the order changes the result. The duplicate-aware get_all(...).iter() workaround in @DaleSeo's original streamable-http server: a supplied Mcp-Method contradicting an initialize body is silently accepted #1271 already avoids this, so this is a gap between that workaround and this PR, not a new issue discovery.
  2. A present opaque-byte value, e.g. HeaderValue::from_bytes(&[0xff]), is representable by http::HeaderValue, but to_str() fails. .ok() erases that failure, causing the initialize branch to treat the supplied invalid method value like an absent header.

Suggested additions inside this PR's standard_header_init_tests, reusing its existing helpers (source-derived expected failures, not executed results):

#[test]
fn initialize_rejects_conflicting_duplicates_in_both_orders() {
    for (first, second) in [("initialize", "tools/list"), ("tools/list", "initialize")] {
        let mut headers = headers_with(Some(first));
        headers.append(HEADER_MCP_METHOD, http::HeaderValue::from_str(second).unwrap());
        assert!(validate_standard_headers(&headers, &initialize_message(), no_tool_schema).is_err());
    }
}

#[test]
fn initialize_rejects_present_non_text_method() {
    let mut headers = headers_with(None);
    let value = http::HeaderValue::from_bytes(&[0xff]).unwrap();
    assert!(value.to_str().is_err());
    headers.insert(HEADER_MCP_METHOD, value);
    assert!(validate_standard_headers(&headers, &initialize_message(), no_tool_schema).is_err());
}

The original issue's approach provides a small fix: inspect get_all(HEADER_MCP_METHOD), compare the supplied value's bytes with b"initialize", and reject an additional value before returning success. Keep the existing version gate, header-mismatch response, and missing/matching-header acceptance tests. Rejecting even identical duplicates would preserve the reporter's workaround; that duplicate policy should be explicit rather than accidental.

References: HeaderMap::get / append, HeaderValue::from_bytes / to_str. These support the source analysis, not end-to-end transport validation. No live MCP service was probed, and no exploitable deployment or native-suite pass is claimed.

Prepared and posted by an authorized AI assistant for the contributing account; not a claim of the account owner's unaided Rust implementation or testing.

@SIDDARTHAREDDY8

Copy link
Copy Markdown
Contributor Author

Addressed both edge cases in 2efb0b5:

  • The initialize arm now inspects headers.get_all(HEADER_MCP_METHOD) instead of get, so a contradictory appended duplicate can no longer hide behind a matching first value. Duplicates (even identical ones) are now rejected explicitly rather than passing by accident.
  • Comparison is against the value bytes, so a present-but-non-UTF8 value is rejected instead of being silently treated as an absent header.
  • Added both suggested regression tests (initialize_rejects_conflicting_duplicates_in_both_orders, initialize_rejects_present_non_text_method).

No Rust toolchain in this environment, so the new tests were not executed locally; they need to run in CI.

@jarrettdustinqq jarrettdustinqq left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native validation follow-up to #issuecomment-5733844405 and the author's correction in 2efb0b5a004ba9360df682432bd14ba9b1c3211c.

I compiled the unmodified pinned head on x86_64 Linux using the project's Rust 1.96.0 toolchain. No source fix or competing implementation was needed.

  • cargo +1.96.0 test -p rmcp --lib --features transport-streamable-http-server standard_header_init_tests -- --nocapture completed successfully: 5 passed, 0 failed, 0 ignored, 218 filtered out. This includes both suggested duplicate-order and non-text-header tests, plus the existing missing/matching/contradicting-header controls.
  • I then executed that exact Cargo-built native test binary without the filter (--nocapture --test-threads=1): 223 passed, 0 failed, 0 ignored, 0 filtered out. The five targeted tests are included in those 223, not additional unique tests.

Build settings: CARGO_BUILD_JOBS=1, CARGO_PROFILE_DEV_DEBUG=0, CARGO_PROFILE_TEST_DEBUG=0, CARGO_INCREMENTAL=0; rustc 1.96.0 (ac68faa20 2026-05-25), cargo 1.96.0 (30a34c682 2026-05-25). The build emitted three dead_code warnings in existing feature-gated code.

Scope and preserved failures: these are the library tests enabled by default features plus transport-streamable-http-server, not all-feature, integration, conformance, or upstream CI results. A subsequent offline Cargo invocation failed after I removed temporary downloaded crate archives to recover disk space; the already-built native binary remained intact and supplied the full 223-test result above. A separate attempt to execute the pre-review function as a negative control was rejected by the remote command runner before execution, so no red/green replay is claimed. The checked-out source remained byte-for-byte unchanged. No deployed service was probed.

This verifies that the current native tests execute and pass on the corrected head; it is not a maintainer approval, merge recommendation based on full CI, or claim that the account owner independently implemented/tested Rust. Work was performed by an authorized AI assistant; original reporting and the patch remain credited to their respective contributors.

Comment on lines +850 to +853
let mut method_values = headers.get_all(HEADER_MCP_METHOD).iter();
let Some(first) = method_values.next() else {
return Ok(());
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inside validate_standard_headers, handle_post bypasses the new check when legacy sessions are enabled and no session ID is provided. For example, an initialize body and protocol header that both specify 2026-07-28, along with Mcp-Method: tools/list, still return HTTP 200 and create a session. We should run the validator before that path creates the session. Can you also add an HTTP-level regression test?

@github-actions github-actions Bot added the T-test Testing related changes label Sep 24, 2026
@SIDDARTHAREDDY8

Copy link
Copy Markdown
Contributor Author

Addressed your review feedback: validate_standard_headers now runs on the no-session-id initialize path in handle_post before the session is created, so a supplied Mcp-Method header contradicting the initialize body is rejected with HTTP 400 (-32020) instead of returning 200 with a fresh session. Also added HTTP-level regression tests in crates/rmcp/tests/test_streamable_http_standard_headers.rs covering the legacy-session-mode scenario (contradicting header -> 400, matching header -> 200). CI is running on the updated head.

Signed-off-by: SIDDARTHA REDDY <75976672+SIDDARTHAREDDY8@users.noreply.github.com>

Signed-off-by: SIDDARTHA REDDY <75976672+SIDDARTHAREDDY8@users.noreply.github.com>
@SIDDARTHAREDDY8

Copy link
Copy Markdown
Contributor Author

Applied cargo +nightly fmt fixes (collapsed the closure in the validate_standard_headers call and the long test fn signature) per the repo's rustfmt.toml; CI should be green now.

@DaleSeo
DaleSeo merged commit 6677eee into modelcontextprotocol:main Sep 24, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T-core Core library changes T-test Testing related changes T-transport Transport layer changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

streamable-http server: a supplied Mcp-Method contradicting an initialize body is silently accepted

3 participants