Repository navigation
fix(transport): match explicit HTTP media types and Accept quality values #1382
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -45,11 +45,21 @@ async fn spawn_server() -> (reqwest::Client, String, CancellationToken) { | |||||
| async fn spawn_server_with<S>( | ||||||
| service_factory: impl Fn() -> Result<S, std::io::Error> + Send + Sync + 'static, | ||||||
| ) -> (reqwest::Client, String, CancellationToken) | ||||||
| where | ||||||
| S: ServerHandler + Send + 'static, | ||||||
| { | ||||||
| spawn_server_with_mode(service_factory, false).await | ||||||
| } | ||||||
|
|
||||||
| async fn spawn_server_with_mode<S>( | ||||||
| service_factory: impl Fn() -> Result<S, std::io::Error> + Send + Sync + 'static, | ||||||
| legacy_session_mode: bool, | ||||||
| ) -> (reqwest::Client, String, CancellationToken) | ||||||
| where | ||||||
| S: ServerHandler + Send + 'static, | ||||||
| { | ||||||
| let config = StreamableHttpServerConfig::default() | ||||||
| .with_legacy_session_mode(legacy_session_mode) | ||||||
| .with_json_response(true) | ||||||
| .with_sse_keep_alive(None) | ||||||
| .with_cancellation_token(CancellationToken::new()); | ||||||
|
|
@@ -495,3 +505,219 @@ async fn accepts_initialize_with_matching_mcp_method() -> anyhow::Result<()> { | |||||
| ct.cancel(); | ||||||
| Ok(()) | ||||||
| } | ||||||
|
|
||||||
| #[tokio::test] | ||||||
| async fn media_type_headers_follow_http_semantics() { | ||||||
|
Comment on lines
+509
to
+510
Member
There was a problem hiding this comment.
|
||||||
| let (client, url, ct) = spawn_server().await; | ||||||
| let cases: &[(&str, &[&str], &str, u16)] = &[ | ||||||
| ( | ||||||
| "ordinary", | ||||||
| &["application/json, text/event-stream"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "content type case", | ||||||
| &["application/json, text/event-stream"], | ||||||
| "Application/JSON", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "accept case", | ||||||
| &["APPLICATION/JSON, TEXT/EVENT-STREAM"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "split accept", | ||||||
| &["application/json", "text/event-stream"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "content type suffix", | ||||||
| &["application/json, text/event-stream"], | ||||||
| "application/jsonx", | ||||||
| 415, | ||||||
| ), | ||||||
| ( | ||||||
| "accept suffix", | ||||||
| &["application/json-seq, text/event-stream"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "zero quality", | ||||||
| &["application/json;q=0, text/event-stream"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "zero quality decimal", | ||||||
| &["application/json;Q=0.000, text/event-stream"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "positive quality", | ||||||
| &["application/json;q=0.001, text/event-stream"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
|
Comment on lines
+562
to
+566
Member
There was a problem hiding this comment. The table covers the zero and positive boundary, but not the upper bound or malformed weights. So the
Author
There was a problem hiding this comment. Agreed—the current matrix misses the Thanks also to theamodhshetty for the supplemental matrix and independent macOS run. That covers this gap well; it has not been folded into the branch yet, and I’ll rerun the tests on the resulting commit.
Author
There was a problem hiding this comment. Added in e0eb843. The matrix now covers the upper bound and the rejection paths in To be explicit about the behaviour you asked to settle on: a qvalue that is not The GET-side cases got the same treatment: Validation on this revision: all 67 tests across the six HTTP integration targets and 378 library tests passed, plus targeted Clippy with warnings denied and nightly formatting. Those are local checks; the upstream workflows on this head are waiting on maintainer approval. |
||||||
| ( | ||||||
| "parameters", | ||||||
| &["application/json;profile=\"a,b;c\", text/event-stream"], | ||||||
| "application/json; charset=utf-8", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "parameter is not media", | ||||||
| &["text/plain;profile=\"application/json, text/event-stream\""], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "escaped quoted parameter", | ||||||
| &[r#"application/json;profile="a\",b;c", text/event-stream"#], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "quoted quality is not a weight", | ||||||
| &[r#"application/json;profile="a;q=0", text/event-stream"#], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "escaped parameter is not media", | ||||||
| &[r#"text/plain;profile="a\",application/json, text/event-stream""#], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ("wildcard alone", &["*/*"], "application/json", 406), | ||||||
| // Invalid weights follow this implementation's admission policy. | ||||||
|
Member
There was a problem hiding this comment.
Suggested change
|
||||||
| ( | ||||||
| "one", | ||||||
| &["application/json;q=1, text/event-stream"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "one decimal", | ||||||
| &["application/json;q=1., text/event-stream"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "one three digits", | ||||||
| &["application/json;q=1.000, text/event-stream"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "over precision", | ||||||
| &["application/json;q=0.0001, text/event-stream"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "above one", | ||||||
| &["application/json;q=1.001, text/event-stream"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "missing leading zero", | ||||||
| &["application/json;q=.5, text/event-stream"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "negative quality", | ||||||
| &["application/json;q=-0.1, text/event-stream"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "non digit quality", | ||||||
| &["application/json;q=abc, text/event-stream"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "zero SSE", | ||||||
| &["application/json, text/event-stream;q=0"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ( | ||||||
| "positive SSE", | ||||||
| &["application/json, text/event-stream;q=0.01"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "weighted split reversed", | ||||||
| &["text/event-stream;q=0.5", "application/json;q=0.5"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "same field reversed", | ||||||
| &["text/event-stream, application/json"], | ||||||
| "application/json", | ||||||
| 202, | ||||||
| ), | ||||||
| ( | ||||||
| "SSE near match", | ||||||
| &["application/json, text/event-streaming"], | ||||||
| "application/json", | ||||||
| 406, | ||||||
| ), | ||||||
| ]; | ||||||
| let mut failures = Vec::new(); | ||||||
| for (name, accept, content_type, expected) in cases { | ||||||
| let mut request = client | ||||||
| .post(&url) | ||||||
| .header("Content-Type", *content_type) | ||||||
| .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(); | ||||||
| if actual != *expected { | ||||||
| failures.push(format!("{name}: expected {expected}, got {actual}")); | ||||||
| } | ||||||
| } | ||||||
| ct.cancel(); | ||||||
| // Legacy mode dispatches GET through media validation before requiring a session ID. | ||||||
| let (client, url, ct) = spawn_server_with_mode(|| Ok(HeaderValidationServer), true).await; | ||||||
| for (accept, expected) in [ | ||||||
| ("text/event-stream", 400), | ||||||
| ("TEXT/EVENT-STREAM", 400), | ||||||
| ("text/event-streamx", 406), | ||||||
| ("text/event-stream;q=0.0", 406), | ||||||
| ("text/event-stream;q=1", 400), | ||||||
| ("text/event-stream;q=1.", 400), | ||||||
| ("text/event-stream;q=1.000", 400), | ||||||
| ("text/event-stream;q=1.001", 406), | ||||||
| ("text/event-stream;q=0.0001", 406), | ||||||
| ("text/event-stream;q=.5", 406), | ||||||
| ("text/event-stream;q=-0.1", 406), | ||||||
| ("text/event-stream;q=abc", 406), | ||||||
| ] { | ||||||
| let actual = client | ||||||
| .get(&url) | ||||||
| .header("Accept", accept) | ||||||
| .send() | ||||||
| .await | ||||||
| .unwrap() | ||||||
| .status() | ||||||
| .as_u16(); | ||||||
| if actual != expected { | ||||||
| failures.push(format!("GET {accept}: expected {expected}, got {actual}")); | ||||||
| } | ||||||
| } | ||||||
| ct.cancel(); | ||||||
| assert!(failures.is_empty(), "{}", failures.join("\n")); | ||||||
| } | ||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rejecting malformed weights like
q=.5orq=abcturns requests that work today into 406 responses, even though the client clearly listed the media type. Should we only exclude a media range when the weight is explicitly zero?