Repository navigation
fix(transport): match explicit HTTP media types and Accept quality values - #1382
passionworkeer wants to merge 1 commit into
Conversation
|
@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
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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_headersSupplemental 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"));
+}
Streamable HTTP currently checks
Acceptby substring andContent-Typeby prefix. This rejects valid case variants and splitAcceptfields, while acceptingapplication/jsonx,application/json-seq, and a required media type withq=0.Match complete media-type essences case-insensitively, inspect all
Acceptfield 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:
-D warningsandcargo +nightly fmt --all -- --checkpass.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.