From 25b092ef07fb2a1e8355fef4fb7f4923502acbe6 Mon Sep 17 00:00:00 2001 From: Vishal Katyal Date: Mon, 10 Aug 2026 11:23:47 -0400 Subject: [PATCH] fix: enforce propertyNames key pattern on the generated Signals schema MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit quicktype's typescript-zod target emits only an object's shape, so the `propertyNames` key constraint and `additionalProperties: true` on `shopping/types/signals.json` are both dropped from the generated `CheckoutCreateRequestSignalsSchema` (src/spec_generated.ts). Observed (current main, committed models): CheckoutCreateRequestSignalsSchema.parse({ "dev.ucp.buyer_ip": "1.2.3.4", "bogus KEY!": "x", }); // succeeds; "bogus KEY!" is silently stripped rather than rejected CheckoutCreateRequestSignalsSchema.parse({ "com.example.device_id": "a" }); // succeeds but returns {}; the reverse-domain extra is lost Expected: the malformed key is rejected and the reverse-domain extra is preserved. signals.json requires every property name to match `^[a-z][a-z0-9]*(?:\.[a-z][a-z0-9_]*)+$` (signals.json propertyNames.pattern, the same pattern as shopping/types/reverse_domain_name.json), and sets `additionalProperties: true`. Fix (source-driven, in scripts/inject-schema-constraints.mjs): scan the source schemas for objects that declare propertyNames alongside named properties, read the key pattern from source (inline pattern or a $ref, never hand-copied), and splice `.catchall(z.any()).superRefine(...)` onto the matching z.object so extras are retained (additionalProperties: true) and every key is checked. RegExp.test matches JSON Schema's unanchored pattern semantics and, for the `^...$`-anchored source pattern, rejects a trailing-newline key in ECMA-262 (no `m` flag) — the JS analogue of the python-sdk#66 sibling's re.match -> re.fullmatch fix; pinned by a test. Scope: Signals is the only propertyNames + named-properties object in the 2026-04-08 spec. The pure dict-map propertyNames registries (ucp.json services/capabilities/payment_handlers) render as z.record(z.string(), V), which does not enforce the key pattern; unlike python-sdk#66's dict[ReverseDomainName] they are a genuine residual, left as a scoped follow-up needing a distinct z.record-key mechanism. Other propertyNames sites (requires.capabilities, supported_versions, identity_linking scopes) are not emitted. Sibling of python-sdk#66. Regeneration from the pinned spec is byte-identical (model-drift job stays green); npm test, npm run build, and pre-commit are clean. --- scripts/inject-schema-constraints.mjs | 167 +++++++++++++++++++++++++- src/spec_generated.ts | 21 +++- tests/signals-property-names.test.js | 128 ++++++++++++++++++++ 3 files changed, 310 insertions(+), 6 deletions(-) create mode 100644 tests/signals-property-names.test.js diff --git a/scripts/inject-schema-constraints.mjs b/scripts/inject-schema-constraints.mjs index ed62ec6..777e1c4 100644 --- a/scripts/inject-schema-constraints.mjs +++ b/scripts/inject-schema-constraints.mjs @@ -199,6 +199,34 @@ function resolveObject(node, file, seen = new Set(), depth = 0) { return Object.keys(properties).length ? { properties, file } : null; } +/** + * Resolve a `propertyNames` subschema to its literal regex `pattern`, following + * a `$ref` (e.g. `signals.json` inlines the pattern; `ucp.json` maps `$ref` + * `reverse_domain_name.json`). Returns the pattern string exactly as authored in + * the source schema (never a hand-copied literal), or null when the constraint + * is not a plain string pattern we can express as a zod key check. + */ +function resolvePropertyNamesPattern(node, file, seen = new Set(), depth = 0) { + if (!node || typeof node !== "object" || depth > 32) { + return null; + } + if (typeof node.$ref === "string") { + const key = `${file}|${node.$ref}`; + if (seen.has(key)) { + return null; + } + seen.add(key); + const resolved = resolveRef(node.$ref, file); + return resolvePropertyNamesPattern( + resolved.node, + resolved.file, + seen, + depth + 1 + ); + } + return typeof node.pattern === "string" ? node.pattern : null; +} + /** * A single `contains` cardinality clause: the array MUST hold between `min` * and `max` items whose `property` equals `value`. We only recover the @@ -289,6 +317,16 @@ function describeConstraint(propertyNode, file) { // setKey -> Map(propertyName -> Map(signature -> descriptor)) const constraintIndex = new Map(); +// Object-level `propertyNames` (a key-name constraint, not a per-field one) for +// objects that declare it alongside named `properties` -- the extra-allow + +// named-field shape (`signals.json`: reverse-domain key pattern + named +// dev.ucp.* fields + `additionalProperties: true`) that quicktype renders as a +// plain `z.object`, dropping both the key pattern and the extra-key retention. +// Pure dict-map `propertyNames` (no named `properties`, e.g. ucp.json's +// registries) render as `z.record` and are intentionally not handled here; they +// need a distinct record-key mechanism. setKey -> Map(signature -> descriptor). +const propertyNamesIndex = new Map(); + function recordObject(properties, file) { const setKey = Object.keys(properties).sort().join(","); if (!constraintIndex.has(setKey)) { @@ -307,6 +345,45 @@ function recordObject(properties, file) { } } +/** + * Record an object-level `propertyNames` key constraint, keyed like the scalar + * index by the object's sorted named-property set. Only the extra-allow + + * named-field shape is recorded: named `properties` are present (so the + * generated schema is a `z.object`, not a `z.record`), `additionalProperties` is + * `true` (extras allowed, so they must be retained AND key-checked), and the + * key constraint resolves to a literal pattern. `propertyNames` on an `allOf` + * branch is followed like the scalar merge (first branch wins). + */ +function recordPropertyNames(node, properties, file) { + let propertyNames = node.propertyNames; + let additionalProperties = node.additionalProperties; + if (propertyNames === undefined && Array.isArray(node.allOf)) { + for (const sub of node.allOf) { + if (sub && typeof sub === "object" && sub.propertyNames !== undefined) { + propertyNames = sub.propertyNames; + if (additionalProperties === undefined) { + additionalProperties = sub.additionalProperties; + } + break; + } + } + } + if (propertyNames === undefined || additionalProperties !== true) { + return; + } + const pattern = resolvePropertyNamesPattern(propertyNames, file); + if (pattern === null) { + return; + } + const setKey = Object.keys(properties).sort().join(","); + const descriptor = { pattern }; + const signature = JSON.stringify(descriptor); + if (!propertyNamesIndex.has(setKey)) { + propertyNamesIndex.set(setKey, new Map()); + } + propertyNamesIndex.get(setKey).set(signature, descriptor); +} + function walkSchema(node, file, seen = new Set(), depth = 0) { if (!node || typeof node !== "object" || depth > 64) { return; @@ -324,6 +401,7 @@ function walkSchema(node, file, seen = new Set(), depth = 0) { const resolvedObject = resolveObject(node, file); if (resolvedObject) { recordObject(resolvedObject.properties, resolvedObject.file); + recordPropertyNames(node, resolvedObject.properties, file); } if (node.properties && typeof node.properties === "object") { for (const child of Object.values(node.properties)) { @@ -388,6 +466,19 @@ for (const [setKey, byProperty] of constraintIndex) { } } +// Resolve the object-level propertyNames index to one descriptor per set, +// dropping any set that carried conflicting patterns (mirrors the scalar +// ambiguity guard so a coincidental property-set clash never over-restricts). +// setKey -> descriptor +const resolvedPropertyNames = new Map(); +for (const [setKey, bySignature] of propertyNamesIndex) { + if (bySignature.size === 1) { + resolvedPropertyNames.set(setKey, [...bySignature.values()][0]); + } else { + ambiguous.push({ setKey, name: "", count: bySignature.size }); + } +} + // --- Zod method rendering -------------------------------------------------- function toRegexLiteral(pattern) { @@ -446,6 +537,40 @@ function renderContainsRefine(groups) { ); } +/** + * Object-level `propertyNames` enforcement for an extra-allow object. + * + * `.catchall(z.any())` retains extra keys (a bare `z.object` strips them, which + * would silently drop the `additionalProperties: true` reverse-domain extras the + * schema means to keep), and the `.superRefine` matches every property name -- + * named fields and retained extras alike -- against the source key pattern. + * + * `RegExp.test` is used deliberately: it implements JSON Schema's unanchored + * `pattern` semantics, and for the source's `^...$`-anchored pattern it matches + * only end-of-input in ECMA-262 (no `m` flag), so a trailing-newline key is + * rejected -- the JS analogue of python-sdk#66's `re.fullmatch` fix (Python's + * `re.match` admits `"...\n"`). See scripts test for the pinned newline case. + * + * Out of scope for this per-object key check: zod-core strips an own `__proto__` + * key from every `z.object` (a prototype-pollution safeguard) before `.catchall` + * / `.superRefine` run, so such a key is silently dropped (safe direction: not + * preserved, no pollution) rather than surfaced as a rejection. That is an + * SDK-wide zod trait, not a per-schema property, and it is pinned by a test. + */ +function renderPropertyNamesRefine(pattern) { + const regex = toRegexLiteral(pattern); + return ( + `.catchall(z.any())` + + `.superRefine((value, ctx) => {` + + `for (const key of Object.keys(value)) {` + + `if (!${regex}.test(key)) {` + + `ctx.addIssue({ code: z.ZodIssueCode.custom, path: [key], message: ` + + "`Property name ${JSON.stringify(key)} does not match the required " + + "pattern (propertyNames)` });" + + `}}})` + ); +} + /** * Zod methods for a descriptor given the generated field's base kind. * Returns null when the base kind is incompatible with the constraint @@ -579,6 +704,22 @@ function alreadyConstrained(baseCall) { return false; } +/** + * Idempotency for the object-level propertyNames splice: is the whole + * `z.object({...})` call already wrapped by a key-check method chain + * (`.catchall(...)` / `.superRefine(...)`)? + */ +function objectAlreadyConstrained(objectCall) { + const parent = objectCall.parent; + if (parent && ts.isPropertyAccessExpression(parent)) { + const method = parent.name.text; + if (method === "catchall" || method === "superRefine") { + return true; + } + } + return false; +} + // --- Parse the generated file and compute edits ---------------------------- const sourceText = fs.readFileSync(targetPath, "utf8"); @@ -594,6 +735,7 @@ const edits = []; // { pos, text } const report = { objectsMatched: 0, fieldsInjected: 0, + propertyNamesInjected: 0, fieldsSkippedType: 0, fieldsAlreadyDone: 0, injections: [], @@ -620,12 +762,13 @@ function handleObjectLiteral(objectLiteral) { } const setKey = [...names].sort().join(","); const resolvedProperties = resolvedIndex.get(setKey); - if (!resolvedProperties) { + const propertyNamesDescriptor = resolvedPropertyNames.get(setKey); + if (!resolvedProperties && !propertyNamesDescriptor) { return; } let matchedAny = false; for (const prop of objectLiteral.properties) { - if (!ts.isPropertyAssignment(prop)) { + if (!resolvedProperties || !ts.isPropertyAssignment(prop)) { continue; } const name = @@ -659,6 +802,25 @@ function handleObjectLiteral(objectLiteral) { report.injections.push(`${setKey} :: ${name} ${methods.join("")}`); matchedAny = true; } + // Object-level propertyNames: splice a key-pattern check onto the whole + // `z.object({...})` call (the property-set here is the constrained object). + if (propertyNamesDescriptor) { + const objectCall = objectLiteral.parent; + if ( + objectCall && + ts.isCallExpression(objectCall) && + !objectAlreadyConstrained(objectCall) + ) { + const text = renderPropertyNamesRefine(propertyNamesDescriptor.pattern); + edits.push({ pos: objectCall.getEnd(), text }); + report.propertyNamesInjected += 1; + report.injections.push(`${setKey} :: ${text}`); + matchedAny = true; + } else if (objectCall && objectAlreadyConstrained(objectCall)) { + report.fieldsAlreadyDone += 1; + matchedAny = true; + } + } if (matchedAny) { report.objectsMatched += 1; } @@ -695,6 +857,7 @@ fs.writeFileSync(targetPath, output); process.stdout.write( `inject-schema-constraints: ${report.fieldsInjected} field(s) constrained ` + `across ${report.objectsMatched} object schema(s); ` + + `${report.propertyNamesInjected} propertyNames key-check(s); ` + `${report.fieldsAlreadyDone} already constrained; ` + `${report.fieldsSkippedType} skipped (base-type mismatch).\n` ); diff --git a/src/spec_generated.ts b/src/spec_generated.ts index d163731..71ca98c 100644 --- a/src/spec_generated.ts +++ b/src/spec_generated.ts @@ -196,10 +196,23 @@ export const PaymentCredentialSchema = z.object({ }); export type PaymentCredential = z.infer; -export const CheckoutCreateRequestSignalsSchema = z.object({ - "dev.ucp.buyer_ip": z.string().optional(), - "dev.ucp.user_agent": z.string().optional(), -}); +export const CheckoutCreateRequestSignalsSchema = z + .object({ + "dev.ucp.buyer_ip": z.string().optional(), + "dev.ucp.user_agent": z.string().optional(), + }) + .catchall(z.any()) + .superRefine((value, ctx) => { + for (const key of Object.keys(value)) { + if (!/^[a-z][a-z0-9]*(?:\.[a-z][a-z0-9_]*)+$/.test(key)) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + path: [key], + message: `Property name ${JSON.stringify(key)} does not match the required pattern (propertyNames)`, + }); + } + } + }); export type CheckoutCreateRequestSignals = z.infer< typeof CheckoutCreateRequestSignalsSchema >; diff --git a/tests/signals-property-names.test.js b/tests/signals-property-names.test.js new file mode 100644 index 0000000..2952667 --- /dev/null +++ b/tests/signals-property-names.test.js @@ -0,0 +1,128 @@ +// Copyright 2026 UCP Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +// propertyNames enforcement for the Signals object. +// +// signals.json declares named `properties` (dev.ucp.buyer_ip, dev.ucp.user_agent) +// alongside `propertyNames.pattern` (reverse-domain) and +// `additionalProperties: true`. quicktype's typescript-zod target emits only the +// object shape, dropping both `propertyNames` (so a malformed key is not +// rejected) and `additionalProperties: true` (so a well-formed reverse-domain +// extra is silently stripped rather than preserved). This is the js-sdk sibling +// of python-sdk#66; the enforcement is re-attached in +// scripts/inject-schema-constraints.mjs. +// +// The schemas are compiled from src/spec_generated.ts by the "pretest" step so +// the test exercises the generated zod schemas directly. + +const { test } = require("node:test"); +const assert = require("node:assert/strict"); + +const { + CheckoutCreateRequestSignalsSchema, + LookupRequestSignalsSchema, +} = require("./.dist/spec_generated.js"); + +const accepts = (schema, value) => schema.safeParse(value).success === true; +const rejects = (schema, value) => schema.safeParse(value).success === false; + +// The reverse-domain key pattern signals.json/propertyNames requires, identical +// to shopping/types/reverse_domain_name.json. +const PATTERN = /^[a-z][a-z0-9]*(?:\.[a-z][a-z0-9_]*)+$/; + +test("Signals rejects a malformed (non reverse-domain) property name", () => { + assert.ok( + rejects(CheckoutCreateRequestSignalsSchema, { + "dev.ucp.buyer_ip": "1.2.3.4", + "bogus KEY!": "x", + }), + "a key that violates propertyNames.pattern must be rejected" + ); +}); + +test("Signals accepts and preserves a well-formed reverse-domain extra key", () => { + const result = CheckoutCreateRequestSignalsSchema.safeParse({ + "dev.ucp.buyer_ip": "1.2.3.4", + "com.example.device_id": "abc123", + }); + assert.ok(result.success, "a reverse-domain extra key must be accepted"); + assert.equal( + result.data["com.example.device_id"], + "abc123", + "additionalProperties: true means the reverse-domain extra must be preserved" + ); +}); + +test("Signals still populates its named fields", () => { + const result = CheckoutCreateRequestSignalsSchema.safeParse({ + "dev.ucp.buyer_ip": "1.2.3.4", + "dev.ucp.user_agent": "curl/8", + }); + assert.ok(result.success); + assert.equal(result.data["dev.ucp.buyer_ip"], "1.2.3.4"); + assert.equal(result.data["dev.ucp.user_agent"], "curl/8"); +}); + +// python-sdk#66 anchor lesson, pinned for the JS/ECMA-262 dialect. Python's +// re.match admits a trailing newline against a `$`-anchored pattern; in +// JavaScript `$` (no `m` flag) matches only end-of-input, so RegExp.test — the +// operator that matches JSON Schema's unanchored `pattern` semantics — already +// rejects "com.example.k\n". This test pins that behavior so a future +// refactor cannot silently regress to a newline-admitting check. +test("Signals rejects a reverse-domain key with a trailing newline", () => { + assert.equal( + PATTERN.test("com.example.k\n"), + false, + "sanity: pattern rejects trailing newline" + ); + assert.ok( + rejects(CheckoutCreateRequestSignalsSchema, { + "com.example.k\n": "x", + }), + "a trailing-newline key must be rejected (anchor safety)" + ); +}); + +test("LookupRequestSignalsSchema enforces the same propertyNames rule", () => { + assert.ok( + rejects(LookupRequestSignalsSchema, { "bogus KEY!": "x" }), + "the aliased Signals schema must enforce propertyNames too" + ); + assert.ok(accepts(LookupRequestSignalsSchema, { "com.example.ok": "y" })); +}); + +// Known boundary, pinned. zod-core drops an own `__proto__` key from every +// z.object (a prototype-pollution safeguard) BEFORE .catchall/.superRefine run, +// so the key never reaches this per-object propertyNames check. The direction is +// safe -- the key is dropped, not preserved, and the prototype is not polluted -- +// but it is silently dropped rather than rejected. This is an SDK-wide zod trait, +// not specific to Signals, so it is documented here rather than special-cased. +// Every OTHER pattern-violating own key, including "constructor" and "prototype", +// is rejected as normal. +test("Signals: an own __proto__ key is dropped by zod-core (not preserved, no pollution)", () => { + const result = CheckoutCreateRequestSignalsSchema.safeParse( + JSON.parse('{"__proto__":{"polluted":true},"dev.ucp.buyer_ip":"1.2.3.4"}') + ); + assert.ok(result.success, "zod-core strips own __proto__ before validation"); + assert.ok( + !Object.prototype.hasOwnProperty.call(result.data, "__proto__"), + "the __proto__ key must not survive into the parsed output" + ); + assert.equal({}.polluted, undefined, "Object.prototype must not be polluted"); +}); + +test("Signals rejects own constructor / prototype keys (only literal __proto__ is dropped)", () => { + assert.ok(rejects(CheckoutCreateRequestSignalsSchema, { constructor: "x" })); + assert.ok(rejects(CheckoutCreateRequestSignalsSchema, { prototype: "x" })); +});