From e22ea1387736e1da7bdfacf1de2f5215fd8bcbbd Mon Sep 17 00:00:00 2001 From: ygd58 Date: Fri, 4 Sep 2026 20:55:42 +0200 Subject: [PATCH] fix(rpc): parse Accept header as a list of media ranges (#341) ApiVersion::from_accept_header() matched the entire trimmed header value as a single media type, so any Accept header carrying more than one media range (comma-separated) or any parameters (e.g. a q value) failed to match and fell through to a 406, even when a supported version was present in the header. Per RFC 9110 SS12.5.1, Accept is a comma-separated list of media ranges, each optionally followed by ;-separated parameters including q. Rewrite the parser to split on comma, strip parameters per range, and pick the supported version with the highest q among ranges that are not explicitly marked unacceptable (q=0). Media types and the q parameter name are matched case-insensitively (RFC 9110 SS8.3.1/SS5.6.6/SS12.4.2). A q value that fails to parse, or parses outside the valid [0,1] range -- including nan/inf/-inf, all accepted by f32::from_str's grammar, and any finite out-of-range value such as -1 or 1.5 -- is treated uniformly as q=1, i.e. maximally preferred rather than merely acceptable. RFC 9110 SS12.4.1 permits disregarding an unsatisfiable Accept header entirely, so this is within spec; documented as a known interaction with the tie-break rule to revisit once a second ApiVersion exists (a malformed q could then out-rank a client's genuinely preferred version). Also adds status_for_accept()-based middleware tests exercising the same header values through the actual extract_version layer end to end, and a best_match_with_quality() test-only helper so the quality-based tie-breaking rule is observable (and falsifiable) even though only one ApiVersion variant exists today. Fixes #341 --- crates/malachite-app/src/rpc/middleware.rs | 56 ++++ crates/malachite-app/src/rpc/version.rs | 304 ++++++++++++++++++++- 2 files changed, 349 insertions(+), 11 deletions(-) diff --git a/crates/malachite-app/src/rpc/middleware.rs b/crates/malachite-app/src/rpc/middleware.rs index d7b62c15..5fbf4c29 100644 --- a/crates/malachite-app/src/rpc/middleware.rs +++ b/crates/malachite-app/src/rpc/middleware.rs @@ -114,4 +114,60 @@ mod tests { None ); } + + /// Builds a minimal router with only the `extract_version` middleware + /// attached, and returns the response status for a given `Accept` header + /// value. Isolated from the rest of the app's routes/state so these tests + /// exercise exactly the middleware's negotiation behavior end to end, + /// the same way a real request would reach it. + async fn status_for_accept(accept: &str) -> StatusCode { + use axum::body::Body; + use axum::routing::get; + use axum::Router; + use tower::ServiceExt; + + let app = Router::new() + .route("/test", get(|| async { StatusCode::OK })) + .layer(axum::middleware::from_fn(extract_version)); + + let req = Request::builder() + .uri("/test") + .header(header::ACCEPT, accept) + .body(Body::empty()) + .unwrap(); + + app.oneshot(req).await.unwrap().status() + } + + #[tokio::test] + async fn test_accept_header_with_parameters() { + assert_eq!( + status_for_accept("application/vnd.arc.v1+json; q=0.9").await, + StatusCode::OK + ); + } + + #[tokio::test] + async fn test_accept_header_with_multiple_ranges() { + assert_eq!( + status_for_accept("text/html, application/vnd.arc.v1+json").await, + StatusCode::OK + ); + } + + #[tokio::test] + async fn test_accept_header_zero_quality_supported_version_is_not_acceptable() { + assert_eq!( + status_for_accept("application/vnd.arc.v1+json; q=0").await, + StatusCode::NOT_ACCEPTABLE + ); + } + + #[tokio::test] + async fn test_accept_header_only_unsupported_versions_is_not_acceptable() { + assert_eq!( + status_for_accept("application/vnd.arc.v2+json, application/vnd.arc.v99+json").await, + StatusCode::NOT_ACCEPTABLE + ); + } } diff --git a/crates/malachite-app/src/rpc/version.rs b/crates/malachite-app/src/rpc/version.rs index d00d916a..a20a62db 100644 --- a/crates/malachite-app/src/rpc/version.rs +++ b/crates/malachite-app/src/rpc/version.rs @@ -56,25 +56,116 @@ impl ApiVersion { /// - `application/json` -> default (V1) /// - Missing/empty -> default (V1) /// - /// Returns `None` if the header specifies an unsupported version or the - /// format is unrecognized/malformed (e.g. "text/html") + /// Per RFC 9110 §12.5.1, the `Accept` field value is a comma-separated list + /// of media ranges, each optionally carrying parameters including a `q` + /// (quality) value. This parses each range individually rather than + /// matching the whole header as one media type, so headers such as + /// `application/vnd.arc.v1+json; q=0.9` or + /// `text/html, application/vnd.arc.v1+json` correctly negotiate V1. + /// + /// Among all acceptable ranges (`q` != 0) that map to a supported version, + /// the one with the highest `q` value is selected (ties keep the + /// first-encountered range, matching the header's order -- per RFC 9110 + /// §12.5.1 this should really be the *most specific* range on a tie, but + /// with only one version defined today there is nothing for specificity + /// to distinguish; this will need revisiting once a second version + /// exists). A `q` value that fails to parse, or parses outside the valid + /// `[0, 1]` range -- including `nan`/`inf`/`-inf` (all accepted by + /// `f32::from_str`'s grammar) and any finite out-of-range value such as + /// `-1` or `1.5` -- is treated as `q=1`, i.e. *maximally* preferred, not + /// merely acceptable (RFC 9110 §12.4.1 permits disregarding an + /// unsatisfiable Accept header entirely, which this is squarely inside + /// of). With only one version defined today a malformed range can only + /// ever tie with a conforming one, so this is invisible; once a second + /// version exists, a malformed q on an old-version range would + /// out-rank a client's genuinely preferred new-version range -- revisit + /// this alongside the tie-break-by-specificity gap above when that + /// happens. Media types and parameter names are matched + /// case-insensitively per RFC 9110 §8.3.1/§5.6.6/§12.4.2. + /// + /// Returns `None` if no range in the header specifies a supported version + /// (e.g. "text/html" alone, or every versioned range has `q=0`). + /// + /// Known deviations from full RFC 9110 conformance, none of which affect + /// any header this API actually needs to accept: quoted parameter values + /// containing a comma are not parsed (`;`-params are split naively on + /// `,`), and `type/*` partial-wildcard ranges are not recognized (only + /// the literal `*/*`). pub fn from_accept_header(value: &str) -> Option { + Self::best_match_with_quality(value).map(|(version, _q)| version) + } + + /// Same as [`Self::from_accept_header`], but also returns the `q` value + /// of the winning range. Exists mainly so tests can observe the + /// tie-breaking rule (highest `q` wins) directly, since with only one + /// [`ApiVersion`] variant defined today, the returned version alone + /// can't distinguish "highest q" from "first" or "last" range winning. + fn best_match_with_quality(value: &str) -> Option<(Self, f32)> { let trimmed = value.trim(); - // Empty or generic JSON defaults to V1 - if trimmed.is_empty() || trimmed == MEDIA_TYPE_JSON || trimmed == MEDIA_TYPE_ANY { - return Some(Self::default()); + // Empty defaults to V1 + if trimmed.is_empty() { + return Some((Self::default(), 1.0)); } - // Parse versioned media type - if let Some(version_part) = trimmed.strip_prefix(MEDIA_TYPE_PREFIX) { - if let Some(version_str) = version_part.strip_suffix("+json") { - return ApiVersion::from_str(version_str).ok(); + let mut best: Option<(Self, f32)> = None; + let mut best_q = -1.0f32; + + for media_range in trimmed.split(',') { + let mut parts = media_range.split(';'); + let media_type = parts.next().unwrap_or("").trim(); + if media_type.is_empty() { + continue; + } + + let mut q = 1.0f32; + for param in parts { + let param = param.trim(); + if param.get(..2).is_some_and(|p| p.eq_ignore_ascii_case("q=")) { + // Per RFC 9110 SS12.4.2, a qvalue is at most 1, with no + // negative values, so anything outside [0, 1] -- finite + // or not -- cannot come from a conforming sender. + // `nan`/`inf`/`-inf` all parse successfully + // (f32::from_str's grammar accepts them) rather than + // failing to parse, so they must be filtered out + // explicitly alongside any finite out-of-range value + // (e.g. -1 or 1.5): all of it is equally malformed, + // and is treated uniformly as fully acceptable (q=1) + // rather than rejecting the range outright. + q = param[2..] + .trim() + .parse::() + .ok() + .filter(|v| (0.0..=1.0).contains(v)) + .unwrap_or(1.0); + } + } + + // q=0 explicitly marks this range as not acceptable. + if q <= 0.0 { + continue; + } + + let media_type = media_type.to_ascii_lowercase(); + let version = if media_type == MEDIA_TYPE_JSON || media_type == MEDIA_TYPE_ANY { + Some(Self::default()) + } else if let Some(version_part) = media_type.strip_prefix(MEDIA_TYPE_PREFIX) { + version_part + .strip_suffix("+json") + .and_then(|v| ApiVersion::from_str(v).ok()) + } else { + None + }; + + if let Some(version) = version { + if q > best_q { + best = Some((version, q)); + best_q = q; + } } } - // Unrecognized/malformed format defaults to None - None + best } } @@ -177,6 +268,197 @@ mod tests { assert_eq!(ApiVersion::from_accept_header("something/random"), None); } + #[test] + fn test_from_accept_header_with_quality_parameter() { + assert_eq!( + ApiVersion::from_accept_header("application/vnd.arc.v1+json; q=0.9"), + Some(ApiVersion::V1) + ); + } + + #[test] + fn test_from_accept_header_multiple_media_ranges() { + assert_eq!( + ApiVersion::from_accept_header("text/html, application/vnd.arc.v1+json"), + Some(ApiVersion::V1) + ); + } + + #[test] + fn test_from_accept_header_generic_json_in_range_list() { + assert_eq!( + ApiVersion::from_accept_header("text/html, application/json"), + Some(ApiVersion::V1) + ); + } + + #[test] + fn test_from_accept_header_wildcard_in_range_list() { + assert_eq!( + ApiVersion::from_accept_header("text/plain; q=0.5, */*; q=0.1"), + Some(ApiVersion::V1) + ); + } + + #[test] + fn test_from_accept_header_zero_quality_is_not_acceptable() { + // A supported version with q=0 is explicitly marked unacceptable + assert_eq!( + ApiVersion::from_accept_header("application/vnd.arc.v1+json; q=0"), + None + ); + // ...even when a generic-JSON range with q=0 is also present + assert_eq!( + ApiVersion::from_accept_header("application/json; q=0, application/vnd.arc.v1+json; q=0"), + None + ); + } + + #[test] + fn test_from_accept_header_only_unsupported_versions_in_list() { + assert_eq!( + ApiVersion::from_accept_header( + "application/vnd.arc.v2+json, application/vnd.arc.v99+json" + ), + None + ); + } + + #[test] + fn test_from_accept_header_supported_range_after_unsupported() { + assert_eq!( + ApiVersion::from_accept_header( + "application/vnd.arc.v99+json, application/vnd.arc.v1+json" + ), + Some(ApiVersion::V1) + ); + } + + #[test] + fn test_from_accept_header_whitespace_around_commas_and_parameters() { + assert_eq!( + ApiVersion::from_accept_header( + " text/html , application/vnd.arc.v1+json ; q=0.8 " + ), + Some(ApiVersion::V1) + ); + } + + #[test] + fn test_from_accept_header_malformed_quality_value_is_treated_as_acceptable() { + // An unparsable q value falls back to fully acceptable (q=1) rather + // than rejecting the range outright. + assert_eq!( + ApiVersion::from_accept_header("application/vnd.arc.v1+json; q=not-a-number"), + Some(ApiVersion::V1) + ); + } + + #[test] + fn test_from_accept_header_nan_quality_is_treated_as_acceptable() { + // f32::from_str's grammar accepts "nan" (case-insensitively), so it + // does not fall into the parse-failure path -- it must be filtered + // out explicitly (it fails the [0,1] range check), or a NaN q + // compares false against everything (q<=0.0 is false, so not + // skipped; q>best_q is also false, so never selected), making the + // range silently unselectable rather than "fully acceptable" as + // documented. + assert_eq!( + ApiVersion::from_accept_header("application/vnd.arc.v1+json; q=nan"), + Some(ApiVersion::V1) + ); + assert_eq!( + ApiVersion::from_accept_header("application/vnd.arc.v1+json; q=NaN"), + Some(ApiVersion::V1) + ); + } + + #[test] + fn test_from_accept_header_infinite_quality_falls_back_to_acceptable() { + // f32::from_str also accepts "inf"/"-inf"/"infinity", none of which + // are producible by a conforming sender (RFC 9110 SS12.4.2 caps a + // qvalue at 1, with no negative values). Like `nan` and an + // unparsable string, both fall outside the valid [0,1] range and + // fall back to the documented q=1 default (see the dedicated + // out-of-range test below for the finite case, e.g. q=1.5). + assert_eq!( + ApiVersion::best_match_with_quality("application/vnd.arc.v1+json; q=inf"), + Some((ApiVersion::V1, 1.0)) + ); + assert_eq!( + ApiVersion::best_match_with_quality("application/vnd.arc.v1+json; q=-infinity"), + Some((ApiVersion::V1, 1.0)) + ); + } + + #[test] + fn test_from_accept_header_out_of_range_finite_quality_falls_back_to_acceptable() { + assert_eq!( + ApiVersion::best_match_with_quality("application/vnd.arc.v1+json; q=-1"), + Some((ApiVersion::V1, 1.0)) + ); + assert_eq!( + ApiVersion::best_match_with_quality("application/vnd.arc.v1+json; q=-0.5"), + Some((ApiVersion::V1, 1.0)) + ); + assert_eq!( + ApiVersion::best_match_with_quality("application/vnd.arc.v1+json; q=1.5"), + Some((ApiVersion::V1, 1.0)) + ); + } + + + #[test] + fn test_from_accept_header_media_type_case_insensitive() { + // RFC 9110 SS8.3.1: "The type and subtype tokens are case-insensitive." + assert_eq!( + ApiVersion::from_accept_header("Application/JSON"), + Some(ApiVersion::V1) + ); + assert_eq!( + ApiVersion::from_accept_header("APPLICATION/VND.ARC.V1+JSON"), + Some(ApiVersion::V1) + ); + } + + #[test] + fn test_from_accept_header_quality_parameter_name_case_insensitive() { + // RFC 9110 SS12.4.2: "a common parameter, named q (case-insensitive)". + // A capitalized `Q=0` must still mark the range unacceptable -- prior + // to case-insensitive matching this silently fell back to the q=1 + // default and served a representation the client explicitly + // rejected. + assert_eq!( + ApiVersion::from_accept_header("application/vnd.arc.v1+json; Q=0"), + None + ); + } + + #[test] + fn test_from_accept_header_prefers_highest_quality_supported_range() { + // Only V1 exists today, so the *version* returned by + // `from_accept_header` can't distinguish "highest q wins" from + // "first wins" or "last wins" -- both ranges map to V1 either way. + // Assert on the winning q via `best_match_with_quality` instead, so + // this test actually fails if the selection rule regresses (e.g. to + // first- or last-wins) rather than only failing once a second + // version exists to tell the outcomes apart. + assert_eq!( + ApiVersion::best_match_with_quality( + "application/vnd.arc.v1+json; q=0.3, application/json; q=0.9" + ), + Some((ApiVersion::V1, 0.9)) + ); + // Same ranges, reversed order: still picks q=0.9, confirming this + // is quality-based selection and not header order. + assert_eq!( + ApiVersion::best_match_with_quality( + "application/json; q=0.9, application/vnd.arc.v1+json; q=0.3" + ), + Some((ApiVersion::V1, 0.9)) + ); + } + #[test] fn test_from_str() { assert_eq!("v1".parse::().unwrap(), ApiVersion::V1);