diff --git a/CHANGELOG.md b/CHANGELOG.md index 76881fd..e6a2a8c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,23 @@ under the pre-1.0 conventions in [VERSIONING.md](docs/governance/VERSIONING.md): ### Fixed +- **A GTIN check on the create path could not fail, and read as though it were + the only thing checking.** `POST /api/v1/dpp` re-validated the GS1 check digit + of `ProductGroupData::Battery`'s GTIN — a value that had already been through + `Gtin::parse`, so the second check could only ever succeed. + + The branch matched on the battery variant alone, which made it look like ten + other product groups carrying a `gtin` were going unchecked. They are not. + Every typed payload declares `gtin: Gtin`, `Gtin`'s `Deserialize` calls + `Gtin::parse`, and `Gtin`'s inner field is private with `parse` as its only + constructor — so an invalid GTIN cannot be deserialised, cannot be constructed, + and never reaches a handler, for all eleven product groups at once. + + The dead branch is removed and three tests (`gtin_boundary`) pin where the + rejection actually happens, so the next reader does not have to re-derive it. + **No behaviour change**: a malformed GTIN was refused before this change and is + refused after it, at the same point in the request. + - **A restricted product-group field could have reached the public view.** The resolver filters a passport in two passes — the envelope, then `productGroupData` handed on as its own root document. The core filter now diff --git a/CLAUDE.md b/CLAUDE.md index 89dad5c..a48a8fe 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -437,6 +437,35 @@ The internal endpoints are mTLS-gated (`CN=odal-vault`). `just test` (unit, no Docker) and `just test-integration` (the Docker tiers). See Build and Development above for the full recipe set. +### Answer "what actually happens here?" with a test you keep + +When you need to establish a fact about behaviour — does this reject that input, +which layer enforces this rule, is this branch reachable — **write a `#[test]` +and commit it.** Not a scratch binary, not a `python - < Response { /// a naive walk — taking a real field out of the contract an SDK validates /// against. No schema declares one today; this costs nothing and stops that /// being a latent trap for whoever adds the first. -fn strip_descriptions(node: &mut Value) { - /// Keywords whose object values are keyed by author-chosen names, not by - /// schema keywords — descend into the values, never treat the keys as - /// keywords. - const NAME_KEYED: [&str; 4] = ["properties", "$defs", "definitions", "patternProperties"]; +/// Keywords whose object values are keyed by author-chosen names, not by +/// schema keywords — descend into the values, never treat the keys as +/// keywords. +/// +/// Module-level rather than local so the test that checks the result walks by +/// the same list. Two copies of this would drift, and the direction they drift +/// is a check that quietly stops looking. +const NAME_KEYED: [&str; 4] = ["properties", "$defs", "definitions", "patternProperties"]; +fn strip_descriptions(node: &mut Value) { match node { Value::Object(map) => { map.remove("description"); @@ -268,6 +272,45 @@ mod tests { ); } + /// Collect the paths of any surviving `description` **keyword**, walking the + /// tree the way `strip_descriptions` does. + /// + /// A substring search over the serialised schema cannot answer this, because + /// a property may legitimately be *named* `description` — unsold-goods + /// v2.0.0 has one, the line description of Impl. Reg. (EU) 2026/2 Annex I + /// note (e). Its name has to survive stripping; its own description keyword + /// must not. Only a structural walk can tell those apart. + fn surviving_description_keywords(node: &Value, path: &str, out: &mut Vec) { + match node { + Value::Object(map) => { + if map.contains_key("description") { + out.push(path.to_owned()); + } + for (key, value) in map { + if NAME_KEYED.contains(&key.as_str()) { + if let Value::Object(named) = value { + for (name, schema) in named { + surviving_description_keywords( + schema, + &format!("{path}/{key}/{name}"), + out, + ); + } + } + } else { + surviving_description_keywords(value, &format!("{path}/{key}"), out); + } + } + } + Value::Array(items) => { + for (i, item) in items.iter().enumerate() { + surviving_description_keywords(item, &format!("{path}/{i}"), out); + } + } + _ => {} + } + } + #[test] fn no_embedded_schema_keeps_a_description_after_stripping() { let registry = VersionedSchemaRegistry::new(); @@ -275,12 +318,41 @@ mod tests { let raw = registry.get(product_group, version).expect("just listed"); let mut schema: Value = serde_json::from_str(raw).unwrap(); strip_descriptions(&mut schema); + + let mut surviving = Vec::new(); + surviving_description_keywords(&schema, "", &mut surviving); assert!( - !serde_json::to_string(&schema) - .unwrap() - .contains("\"description\""), - "{product_group} v{version} still carries a description keyword after stripping" + surviving.is_empty(), + "{product_group} v{version} still carries a description keyword at: {}", + surviving.join(", ") ); } } + + #[test] + fn the_detector_reports_keywords_and_ignores_a_property_of_that_name() { + // A green check proves nothing until it has been seen to fail, and this + // one replaced an assertion that could not distinguish these two cases + // at all. So: one schema, un-stripped, holding both. + let schema: Value = json!({ + "description": "root prose", + "properties": { + // A field named `description` — a contract, not prose. Its own + // description keyword *is* prose and must be reported. + "description": { "type": "string", "description": "Note (e)" }, + // No description keyword anywhere: must not be reported. + "unitsDiscarded": { "type": "integer" } + } + }); + + let mut found = Vec::new(); + surviving_description_keywords(&schema, "", &mut found); + found.sort(); + + assert_eq!( + found, + vec!["".to_owned(), "/properties/description".to_owned()], + "the root keyword and the one on the `description` field, and nothing else" + ); + } } diff --git a/crates/dpp-vault/src/handlers/create.rs b/crates/dpp-vault/src/handlers/create.rs index f88906d..947717c 100644 --- a/crates/dpp-vault/src/handlers/create.rs +++ b/crates/dpp-vault/src/handlers/create.rs @@ -8,7 +8,6 @@ use axum::{ use chrono::Utc; use dpp_common::url_guard::validate_public_https_url; -use dpp_digital_link::validate_gtin; use dpp_domain::{ ProductGroupCatalog, passport::{Passport, PassportId, PassportRef}, @@ -34,8 +33,12 @@ pub use dpp_types::CreatePassportRequest as CreateRequest; /// `POST /api/v1/dpp` — validate fields and create a new passport in `Draft` status. /// /// Rejects blank required fields, unsafe Unicode characters (null bytes, bidi -/// overrides), out-of-range numeric values, invalid product group data, and malformed -/// GTINs before touching the database. +/// overrides), out-of-range numeric values and invalid product group data before +/// touching the database. +/// +/// A malformed GTIN never reaches here: every typed payload declares +/// `gtin: Gtin`, whose `Deserialize` validates the GS1 check digit, so the body +/// fails to parse. See the `gtin_boundary` tests. pub async fn create_handler( State(state): State, Extension(auth): Extension, @@ -399,6 +402,65 @@ mod schema_validation { } } +#[cfg(test)] +mod gtin_boundary { + //! Where a malformed GTIN is actually refused. + //! + //! Every typed payload declares `gtin: Gtin`, and `Gtin`'s `Deserialize` + //! calls `Gtin::parse`. So a bad check digit is rejected while the request + //! body is being parsed, for every product group at once, before any handler + //! validation runs. These tests pin that, because the handler's own GTIN + //! check reads as if it were the thing enforcing it. + use super::*; + + fn tyre_body(gtin: &str) -> serde_json::Value { + serde_json::json!({ + "productName": "All-season 205/55R16", + "manufacturer": { "name": "M", "address": "A" }, + "productGroupData": { + "productGroup": "tyre", + "gtin": gtin, + "tyreClass": "C1", + "fuelEfficiencyClass": "A", + "wetGripClass": "A", + "externalRollingNoiseDb": 70.0 + } + }) + } + + #[test] + fn a_bad_check_digit_is_refused_while_the_body_is_parsed() { + // Valid 14-digit shape, wrong GS1 mod-10 check digit. + let err = serde_json::from_value::(tyre_body("09506000134353")) + .expect_err("a bad check digit must not deserialize"); + assert!( + err.to_string().to_lowercase().contains("check digit"), + "the rejection should name the check digit, got: {err}" + ); + } + + #[test] + fn a_valid_gtin_parses_and_the_request_is_accepted() { + let body = serde_json::from_value::(tyre_body("09506000134352")) + .expect("a valid GTIN must deserialize"); + assert!( + validate_create_request(&body).is_none(), + "a well-formed tyre body must pass every create validation" + ); + } + + #[test] + fn the_handler_reads_the_gtin_generically_not_battery_only() { + // The handler asks the payload rather than matching on the variant. If + // this ever returns `None` for a product group that declares a `gtin`, + // the check silently stops covering it — which is how it came to cover + // battery alone. + let body = serde_json::from_value::(tyre_body("09506000134352")).unwrap(); + let data = body.product_group_data.expect("payload present"); + assert_eq!(data.gtin(), Some("09506000134352")); + } +} + /// Every validation `POST /api/v1/dpp` applies to a request body, with no side /// effects. Returns the rejection response, or `None` when the body would be /// accepted. @@ -473,16 +535,17 @@ pub fn validate_create_request(body: &CreateRequest) -> Option