Skip to content

fix(transport): match explicit HTTP media types and Accept quality values - #1382

Open
passionworkeer wants to merge 1 commit into
modelcontextprotocol:mainfrom
passionworkeer:fix/http-media-type-matching
Open

passionworkeer wants to merge 1 commit into
modelcontextprotocol:mainfrom
passionworkeer:fix/http-media-type-matching

Conversation

@passionworkeer

Copy link
Copy Markdown

Streamable HTTP currently checks Accept by substring and Content-Type by prefix. This rejects valid case variants and split Accept fields, while accepting application/jsonx, application/json-seq, and a required media type with q=0.

Match complete media-type essences case-insensitively, inspect all Accept field values, and honor quality weights. List splitting preserves quoted strings and escaped quotes, so media-type names or delimiters inside parameter values do not affect admission. The existing explicit JSON/SSE requirement and Content-Type parameter tolerance are preserved; no dependencies are added.

Fixes #1381.

The implementation follows RFC 9110 sections 8.3.1, 5.3, and 12.4.2. It does not implement wildcard negotiation or full representation selection.

Validation

On Linux with the project's Rust 1.96 toolchain:

  • The same regression test exercises 19 real HTTP scenarios against an axum listener and reqwest client: the base implementation fails 12 scenarios, while the fix passes all 19.
  • All 67 tests across the six related HTTP integration targets pass, including that regression test.
  • All 378 rmcp library tests pass.
  • Targeted Clippy with -D warnings and cargo +nightly fmt --all -- --check pass.

The regression includes legacy GET dispatch, stateless POST, case variants, multiple field values, suffixes, zero/positive quality, and quoted/escaped parameters. Full feature-matrix and cross-platform CI were not run locally.

AI assistance: Codex assisted the investigation, implementation, tests, and an independent agent review of the patch.

@passionworkeer
passionworkeer requested a review from a team as a code owner October 21, 2026 11:58
Copilot AI balanced review requested due to automatic review settings October 21, 2026 11:58

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added T-test Testing related changes T-core Core library changes T-transport Transport layer changes labels Oct 21, 2026
@github-actions
github-actions Bot requested a review from DaleSeo October 21, 2026 11:59
@github-actions

Copy link
Copy Markdown
Contributor

@DaleSeo (assignment failed) is on triage rotation this week and is the reviewer for this one.

Labels come from the PR labeler. Please review the change.

Automated triage. Labels are a first pass and the assigned maintainer has the final say.

@connor4312 connor4312 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.

Non-blocking suggestion: could we expose an accepted_media_types(&HeaderMap) iterator that yields media-type essences after quote-aware splitting and quality filtering, then have handle_post scan it once to track whether both JSON and SSE are accepted? handle_get could use the same iterator with .any(...).

This would separate HTTP parsing from the MCP-specific admission policy and avoid parsing all Accept fields twice in the POST handler. The iterator should filter out zero/invalid quality values, preserve case-insensitive matching at the comparison step, and retain the current quoted/escaped-parameter behavior. POST should use a single loop rather than two .any(...) calls on the same iterator, since the first call can consume entries needed by the second.

I do not consider this a correctness blocker; the current two independent scans are correct.

@theamodhshetty theamodhshetty 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.

I independently ran the PR's 19 HTTP admission scenarios on macOS arm64 with Rust 1.96, then added 14 quality-boundary/split-header cases. All 33 pass on 61a5373; the baseline transport fails 20 (12 original cases and eight added cases). All 15 tests in the header integration target pass with the supplemental fixture.

The added cases cover quality limits (1., 1.000, zero SSE weight, over-precision/out-of-range values), weighted split fields and an SSE near-match. I found no actionable defect in this scope. This corroborates admission behavior; it does not establish full representation negotiation or every feature/platform combination.

The test-only patch below applies cleanly to 61a5373ac387259bb1e867aeeb85a070bb42b42e. I am offering it here so you can fold it into this PR if useful; I have not opened a competing PR. The minimum-positive-quality case overlaps the existing matrix as a control. Rejection of malformed weights is checked as this implementation's policy, not a claim that every invalid input must receive this exact status under RFC 9110.

Codex assisted the local review and supplemental tests.

Reproduction:

cargo +1.96 test -p rmcp \
  --features "server,client,transport-streamable-http-server,reqwest" \
  --test test_streamable_http_standard_headers
Supplemental test patch (apply on the PR head)
diff --git a/crates/rmcp/tests/test_streamable_http_standard_headers.rs b/crates/rmcp/tests/test_streamable_http_standard_headers.rs
index f6f1e4d..5cdfad8 100644
--- a/crates/rmcp/tests/test_streamable_http_standard_headers.rs
+++ b/crates/rmcp/tests/test_streamable_http_standard_headers.rs
@@ -636,3 +636,94 @@ async fn media_type_headers_follow_http_semantics() {
     ct.cancel();
     assert!(failures.is_empty(), "{}", failures.join("\n"));
 }
+
+// Independent local review matrix for PR #1382; not a competing implementation.
+#[tokio::test]
+async fn review_media_type_quality_boundaries() {
+    let (client, url, ct) = spawn_server().await;
+    let cases: &[(&str, &[&str], u16)] = &[
+        (
+            "minimum positive",
+            &["application/json;q=0.001, text/event-stream"],
+            202,
+        ),
+        ("one", &["application/json;q=1, text/event-stream"], 202),
+        (
+            "one decimal",
+            &["application/json;q=1., text/event-stream"],
+            202,
+        ),
+        (
+            "one three digits",
+            &["application/json;q=1.000, text/event-stream"],
+            202,
+        ),
+        (
+            "positive SSE",
+            &["application/json, text/event-stream;q=0.01"],
+            202,
+        ),
+        (
+            "zero SSE",
+            &["application/json, text/event-stream;q=0"],
+            406,
+        ),
+        (
+            "zero dot",
+            &["application/json;q=0., text/event-stream"],
+            406,
+        ),
+        (
+            "too precise",
+            &["application/json;q=0.0001, text/event-stream"],
+            406,
+        ),
+        (
+            "greater than one",
+            &["application/json;q=1.001, text/event-stream"],
+            406,
+        ),
+        (
+            "missing leading zero",
+            &["application/json;q=.5, text/event-stream"],
+            406,
+        ),
+        (
+            "negative quality",
+            &["application/json;q=-0.1, text/event-stream"],
+            406,
+        ),
+        (
+            "weight split across fields",
+            &["application/json;q=0.5", "text/event-stream;q=0.5"],
+            202,
+        ),
+        (
+            "near-match event type",
+            &["application/json, text/event-streaming"],
+            406,
+        ),
+        (
+            "wildcard cannot replace explicit JSON",
+            &["application/*, text/event-stream"],
+            406,
+        ),
+    ];
+    let mut failures = Vec::new();
+    for (name, accept, expected) in cases {
+        let mut request = client
+            .post(&url)
+            .header("Content-Type", "application/json")
+            .body(r#"{"jsonrpc":"2.0","method":"notifications/initialized"}"#);
+        for value in *accept {
+            request = request.header("Accept", *value);
+        }
+        let actual = request.send().await.unwrap().status().as_u16();
+        eprintln!("REVIEW {name}: expected {expected}, got {actual}");
+        if actual != *expected {
+            failures.push(format!("{name}: expected {expected}, got {actual}"));
+        }
+    }
+    ct.cancel();
+    assert!(failures.is_empty(), "{}", failures.join("\n"));
+}

This branch has not been deployed

No deployments
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: Accept and Content-Type are matched case-sensitively and by substring/prefix

4 participants