From 5dfc50f293a4f0e920a22b621396f7e949fd85ad Mon Sep 17 00:00:00 2001 From: Hare Date: Sun, 1 Nov 2026 06:15:36 +0900 Subject: [PATCH] fix: validate RFC 9457 URI references --- Cargo.lock | 1 + crates/error-contract/Cargo.toml | 1 + crates/error-contract/src/problem.rs | 23 ++-- crates/error-contract/tests/contracts.rs | 168 +++++++++++++++++++++++ openapi/problem-details.schema.json | 6 +- 5 files changed, 182 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9a4cb76f8..30a5d87ca 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1518,6 +1518,7 @@ dependencies = [ name = "error-contract" version = "0.1.0" dependencies = [ + "iri-string", "schemars", "serde", "serde_json", diff --git a/crates/error-contract/Cargo.toml b/crates/error-contract/Cargo.toml index ecbcef511..99f7ca082 100644 --- a/crates/error-contract/Cargo.toml +++ b/crates/error-contract/Cargo.toml @@ -11,6 +11,7 @@ typescript = ["dep:ts-rs", "json-schema"] test-support = [] [dependencies] +iri-string = { version = "0.7.12", default-features = false } serde = { workspace = true, features = ["derive"] } serde_json.workspace = true thiserror.workspace = true diff --git a/crates/error-contract/src/problem.rs b/crates/error-contract/src/problem.rs index be15d43e5..37f3675bf 100644 --- a/crates/error-contract/src/problem.rs +++ b/crates/error-contract/src/problem.rs @@ -86,7 +86,7 @@ pub trait ApiBoundary { #[cfg_attr(feature = "typescript", derive(ts_rs::TS))] pub struct ProblemDetails { #[serde(rename = "type")] - #[cfg_attr(feature = "json-schema", schemars(length(min = 1, max = 2048)))] + #[cfg_attr(feature = "json-schema", schemars(length(max = 2048)))] pub type_uri: String, #[cfg_attr(feature = "json-schema", schemars(length(min = 1, max = 2048)))] pub title: String, @@ -94,7 +94,7 @@ pub struct ProblemDetails { pub status: u16, #[cfg_attr(feature = "json-schema", schemars(length(min = 1, max = 2048)))] pub detail: String, - #[cfg_attr(feature = "json-schema", schemars(length(min = 1, max = 2048)))] + #[cfg_attr(feature = "json-schema", schemars(length(max = 2048)))] pub instance: String, pub yoi_failure: FailureEnvelope, } @@ -122,19 +122,16 @@ impl ProblemResponse { } fn validate_uri(value: &str) -> Result<(), ContractError> { - validate_explanation(value)?; - if !value.is_ascii() - || value.bytes().any(|byte| { - byte <= b' ' || byte == b'"' || byte == b'<' || byte == b'>' || byte == b'\\' - }) - { + if value.len() > MAX_EXPLANATION_BYTES { return Err(ContractError::Invalid); } - // Accept absolute URI and RFC URI-reference instances, not arbitrary prose. - if !(value.contains(':') || value.starts_with('/') || value.starts_with('#')) { - return Err(ContractError::Invalid); - } - Ok(()) + // Reuse the RFC 3986 validator already in the transport dependency graph, without its + // optional features. Unlike WHATWG URL parsing, borrowed UriReferenceStr validation + // neither resolves a base URI nor normalizes malformed input into different text. + // Empty references, relative paths, queries and fragments are valid URI-references. + iri_string::types::UriReferenceStr::new(value) + .map(|_| ()) + .map_err(|_| ContractError::Invalid) } pub fn to_problem_details( diff --git a/crates/error-contract/tests/contracts.rs b/crates/error-contract/tests/contracts.rs index da87fab5c..7a4a28a74 100644 --- a/crates/error-contract/tests/contracts.rs +++ b/crates/error-contract/tests/contracts.rs @@ -1395,3 +1395,171 @@ fn schemas_derive_typed_details_and_shared_context_bounds_from_the_same_wire_typ ); assert!(workdir["$defs"]["WorkdirInUse"]["properties"]["operation"].is_object()); } + +struct UriBoundary(&'static str); +impl problem::ApiBoundary for UriBoundary { + fn classify(&self, _: &SubjectDenied) -> problem::ProblemClass { + problem::ProblemClass { + status: problem::HttpFailureStatus::Conflict, + type_uri: self.0, + title: "Operation conflict", + } + } +} + +#[test] +fn rfc9457_uri_references_accept_relative_absolute_and_component_syntax_in_both_directions() { + let projected = browser_subject(); + // RFC 3986 section 5.4 references, plus authorities and escaped delimiters. Syntax only: + // no base resolution, normalization, DNS, HTTP scheme restriction or implicit execution. + for reference in [ + "../requests/1", + "?occurrence=1", + "", + "g:h", + "g", + "./g", + "g/", + "/g", + "//g", + "?y", + "g?y", + "#s", + "g#s", + "g?y#s", + ";x", + "g;x", + "g;x?y#s", + ".", + "./", + "..", + "../", + "../g", + "../..", + "../../", + "../../g", + "a/b:c", + "a//b", + "///path", + "?", + "#", + "urn:yoi:occurrence:one", + "mailto:user@example.com", + "https://user:pass@example.test:443/a?b#c", + "//[2001:db8::1]:443/a", + "//[::ffff:192.0.2.128]/", + "//[v1.a:b]/x", + "//host:/", + "a%20b/%5B?x=%25&y=?/#%2f", + "http://host/path??query#fragment?with/slash", + ] { + let boundary = UriBoundary(reference); + let response = problem::to_problem_details(&projected, &boundary, reference, budget()) + .unwrap_or_else(|error| panic!("valid reference {reference:?}: {error}")); + let wire: Value = serde_json::from_slice(response.body()).unwrap(); + assert_eq!(wire["type"], reference); + assert_eq!( + wire["instance"], reference, + "syntax validation must not normalize the reference" + ); + assert!( + matches!( + problem::decode_problem_details::( + response.body(), + response.status, + response.content_type, + &boundary, + budget() + ), + DecodeFailure::Known(_) + ), + "{reference:?}" + ); + } + let exact = format!("/{}", "a".repeat(MAX_EXPLANATION_BYTES - 1)); + assert!(problem::to_problem_details(&projected, &WorkerApi, &exact, budget()).is_ok()); + assert!(problem::to_problem_details(&projected, &WorkerApi, &(exact + "a"), budget()).is_err()); +} + +#[test] +fn rfc9457_uri_references_reject_malformed_components_in_both_directions() { + let projected = browser_subject(); + let response = + problem::to_problem_details(&projected, &WorkerApi, "urn:yoi:occurrence:test", budget()) + .unwrap(); + let original: Value = serde_json::from_slice(response.body()).unwrap(); + for reference in [ + "/requests/%GG", + "/requests/[invalid", + "foo%", + "foo%2", + "?%GG", + "#%GG", + "//user@@host/x", + "//host:wrongport/x", + "//[::not-ipv6]/", + "//[v.a]/", + "//[vG.foo]/", + "//[v1.]/", + "//[::1]tail/", + "//[::1", + "//host]/", + "bad space", + "1invalid:rest", + ":rest", + "a{b", + "a#b#c", + "a\\b", + "日本語", + "/a\n", + "?x=\0", + ] { + let invalid_class = UriBoundary(reference); + assert!( + problem::to_problem_details( + &projected, + &invalid_class, + "urn:yoi:occurrence:test", + budget() + ) + .is_err(), + "type {reference:?}" + ); + assert!( + problem::to_problem_details(&projected, &WorkerApi, reference, budget()).is_err(), + "instance {reference:?}" + ); + let mut instance_wire = original.clone(); + instance_wire["instance"] = json!(reference); + assert!( + matches!( + problem::decode_problem_details::( + &serde_json::to_vec(&instance_wire).unwrap(), + 409, + problem::PROBLEM_CONTENT_TYPE, + &WorkerApi, + budget() + ), + DecodeFailure::Corrupt(_) + ), + "received instance {reference:?}" + ); + let mut type_wire = original.clone(); + type_wire["type"] = json!(reference); + // Use a matching invalid mapping too, so a classification mismatch cannot mask a + // syntax-validation bypass on receive. + assert!( + matches!( + problem::decode_problem_details::( + &serde_json::to_vec(&type_wire).unwrap(), + 409, + problem::PROBLEM_CONTENT_TYPE, + &invalid_class, + budget() + ), + DecodeFailure::Corrupt(_) + ), + "received type {reference:?}" + ); + } +} diff --git a/openapi/problem-details.schema.json b/openapi/problem-details.schema.json index 5432ae9a5..91788cbfb 100644 --- a/openapi/problem-details.schema.json +++ b/openapi/problem-details.schema.json @@ -11,8 +11,7 @@ }, "instance": { "type": "string", - "maxLength": 2048, - "minLength": 1 + "maxLength": 2048 }, "status": { "type": "integer", @@ -27,8 +26,7 @@ }, "type": { "type": "string", - "maxLength": 2048, - "minLength": 1 + "maxLength": 2048 }, "yoi_failure": { "$ref": "#/$defs/FailureEnvelope"