Skip to content

fix(rest/nodejs): read signing_keys[] where the schema places it and drop the ucp.keys[] mirror - #226

Open
vishkaty wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/samples-keys-capabilities
Open

fix(rest/nodejs): read signing_keys[] where the schema places it and drop the ucp.keys[] mirror#226
vishkaty wants to merge 1 commit into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/samples-keys-capabilities

Conversation

@vishkaty

Copy link
Copy Markdown
Contributor

Fixes #225

Observed vs expected

This server declares UCP_VERSION 2026-04-08 (config.ts:15). At that pin, source/discovery/profile_schema.json $defs/base requires ucp and separately declares signing_keys as a top level sibling of ucp; that schema defines no keys field anywhere. discovery.ts published signing_keys[] correctly at the top level but also mirrored it into a nested ucp.keys[] with no schema basis at any pin, and extractKeys() in signature.ts read only that nested field. Since ucp is required on every real profile document, a reader that looks inside ucp whenever it is present can never see a top level sibling field on any real document, so the verifier could not resolve the schema correct field the same server publishes. Full evidence and the trace to our own prior #162 and #179 are in the linked issue.

Two related, smaller corrections to the same file are folded in, since fixing keys touched discovery.ts anyway:

  • DiscoveryCapability.extends was typed string, but capability.json allows oneOf [reverse_domain_name, array (minItems 1)] at both 2026-04-08 and 2026-08-25 ("Use array for multi-parent extensions"). Widened to string | string[].
  • dev.ucp.shopping.refund, .return, and .dispute were declared in the capability catalog with no backing schema file at either pin, no route, and no precedent in either Python reference server. Removed.

Fix

  • signature.ts: extractKeys() now reads the top level signing_keys field directly instead of descending into ucp first. Comment states the schema citation and flags that when this server moves UCP_VERSION to 2026-08-25, the field name must move to keys in the same change, per ucp#566.
  • discovery.ts: the ucp object no longer carries a keys member; UcpDiscoveryMetadata drops the field from its type. Top level signing_keys[] is unchanged. DiscoveryCapability.extends widened to string | string[]. The three non-spec capability entries removed from the catalog.
  • test/discovery.test.ts: capability list assertion drops refund/return/dispute; new test asserts none of the three are declared; new test asserts signing_keys[] is published at the top level and ucp.keys is absent; new test proves DiscoveryCapability.extends accepts a multi-parent array (this one fails tsc if extends is narrowed back to a bare string).
  • test/signing.test.ts: extractKeys and fetchSigningKeys tests rewritten around the schema correct top level shape; added tests proving nested ucp.keys/ucp.signing_keys are not read, and that a top level keys[] (the 2026-08-25 name) is not read yet either, since this server has not moved its declared version.
  • test/signature.test.ts: the shared "good" profile fixture used by the request-signature verification tests moved from { ucp: { keys: [...] } } to { ucp: {}, signing_keys: [...] }, matching the schema correct shape (this fixture models the profile of an inbound counterparty and was missed by an initial grep for .keys since it is a bare object key, not a property access).
  • test/webhook_signing.test.ts: the key-discovery test now asserts ucp.keys is absent instead of asserting it is present.

Verification

  • TDD throughout: every assertion above was written first against the unfixed code and confirmed red for the stated reason, then made green by the source changes.
  • Baseline before this change: 154/154 passing (npm ci && npm run build && npm test in rest/nodejs, mirroring .github/workflows/nodejs.yml, against upstream/main 00333a8). After: 160/160 passing, tsc clean.
  • Four kill tests on the final branch, each reverting one piece of the fix in isolation with the new tests kept, confirming red, then restoring:
    • Reverting extractKeys to the old nested-ucp.keys read: 19 of 160 tests fail.
    • Reintroducing the keys field on UcpDiscoveryMetadata and the ucp object: fails tsc (Object literal may only specify known properties); with the type widened back to accept it too, 2 of 20 targeted tests fail at the assertion level, independent of the type system.
    • Reintroducing the refund/return/dispute capability entries: 2 of 6 targeted tests fail.
    • Narrowing extends back to a bare string: fails tsc.
  • Cross-repo schema validation: booted the server, fetched the live /.well-known/ucp document, and validated it with Python jsonschema 4.26 (Draft202012Validator, real $ref resolution across the full vendored release/2026-04-08 schema tree, not the ucp-schema CLI) against source/discovery/profile_schema.json. Two pre-existing schema deviations remain, both of one kind: the google.pay and dev.ucp.mock_payment payment handler entries each declare version: "1.0", which fails the date pattern the entity schema requires. Both are present identically before and after this change, confirmed by validating the unmodified upstream document too. No new schema violation is introduced or removed by this PR; schema validation alone cannot see the keys defect either way, since additionalProperties: true at the profile base permits the nonstandard nested field. The round-trip tests above are what actually prove and guard the fix.
  • pre-commit (pinned .pre-commit-config.yaml) run on every touched file: clean (prettier reformatted two of the new test blocks; those reformats are folded into the single commit).

Notes for reviewers

  • The Python reference has the identical defect (routes/discovery.py mirrors the JWK into ucp["keys"]; _extract_keys in ucp_signing.py reads only ucp.get("keys")), the reader introduced by fix(rest/python): verify request signatures per RFC 9421 instead of accepting everything #122 and the mirror by our matching fix(rest/python): sign order-event webhooks (RFC 9421) and retry failed deliveries #169. This PR is Node only, matching the scope this was requested under; the Python twin is called out in the issue as a companion follow-up, not bundled here.
  • The refund/return/dispute removal shrinks a declared capability surface. No route implements them today, so no client-visible behavior changes, but flagging since it is a different risk class than the keys fix.
  • Not in this PR: the UCP_VERSION literal and the SDK bump to 2026-08-25 (and the accompanying keys[] rename the comments in this PR point at), since @ucp-js/sdk 0.5.0 is not yet published and that flip needs to land in the same change as the version literal, not ahead of it.

…drop the ucp.keys[] mirror

This server declares UCP_VERSION 2026-04-08 (config.ts:15). At that pin,
source/discovery/profile_schema.json $defs/base requires ucp and separately
declares signing_keys as a top level sibling of ucp, not a field nested
inside it. That schema defines no keys field anywhere. discovery.ts
published signing_keys[] correctly at the top level but also mirrored it
into a nonstandard nested ucp.keys[], and its own verifier, extractKeys()
in signature.ts, read only that nested field. Since ucp is required on
every real profile document, a reader that looks inside ucp whenever it is
present can never see a top level sibling field on any real document, so
the verifier could not resolve the schema correct field the same server
publishes.

Traced to two of our own merged PRs: Universal-Commerce-Protocol#162 introduced extractKeys reading
only a nested keys[], pre-adopting ucp#566 both too early (before the
version literal moved) and in the wrong location (nested rather than top
level); Universal-Commerce-Protocol#179 then added the top level signing_keys[] publication and, to
satisfy the Universal-Commerce-Protocol#162 reader, the nested mirror. extractKeys() now reads the
top level signing_keys field directly; discovery.ts no longer publishes
the nested field; the fetchSigningKeys() doc comment is corrected to match.

Also widens DiscoveryCapability.extends to string | string[] per
capability.json (oneOf reverse_domain_name, array minItems 1, both pins),
and removes the dev.ucp.shopping.refund/.return/.dispute capability
declarations: no schema file exists for them under source/schemas/shopping/
at either pin, no route implements them, and neither reference python
samples server declares them.

Baseline 154/154 (upstream/main 00333a8, npm ci && npm run build && npm
test). After: 160/160, tsc clean. Four kill tests (revert extractKeys,
reintroduce the keys field, reintroduce refund/return/dispute, narrow
extends back) each confirmed red before restoring green. pre-commit clean
on every touched file (prettier reformatted two test files' import wraps,
folded in here).
@damaz91 damaz91 added the status:needs-triage Signal that the PR is ready for human triage label Sep 1, 2026
@carolinerg1 carolinerg1 added status:under-review and removed status:needs-triage Signal that the PR is ready for human triage labels Sep 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node reference verifier cannot discover the signing keys of a schema correct peer at its own declared version

4 participants