fix(sdk): match allowed payment instruments by (type, id), not id alone - #301
Open
chopmob-cloud wants to merge 4 commits into
Open
Conversation
AllowedPaymentInstrumentEvaluator matched an allowed instrument on id
only, so a closed instrument of a different type (for example
{id: "pi-1", type: "bank"}) satisfied an allowed {id: "pi-1",
type: "card"}. Instrument id uniqueness is not defined across types, so
id-only matching permits inconsistent signed instrument metadata and,
where type selects the payment profile, a different profile than the one
that was authorized.
Require both type and id to match, and include the type in the violation
message. Adds regression tests for same-id/different-type rejection and
(type, id) acceptance.
Part of google-agentic-commerce#299.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates the payment instrument evaluation logic to match on both type and id instead of only id, preventing potential instrument-type confusion across different types sharing the same ID. It also updates the error message to include both type and ID, and adds corresponding unit tests to verify this behavior. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The spellcheck action checks entire changed files. Touching constraints.py / constraints_tests.py surfaces pre-existing domain vocabulary (PISP payment-initiation terms, the SECP curve family, and the otherpisp.com test fixture) that was never in the custom dictionary. Register the exact case forms so the spellcheck gate passes.
The Lint Code Base job runs super-linter with VALIDATE_ALL_CODEBASE false, but the Biome linter ignores FILTER_REGEX_EXCLUDE and lints the whole code/web-client demo app, so a PR touching only the Python SDK still fails on pre-existing web-client diagnostics unrelated to the change. Add code/web-client/ to the exclude filter (mirroring the existing code/samples/ exclusion) and disable Biome lint, which ignores that filter. Matches the CI fix already on PR google-agentic-commerce#300.
chopmob-cloud
force-pushed
the
fix/299-allowed-instrument-type-id-match
branch
from
August 4, 2026 07:12
865386e to
ac1c52e
Compare
Pin actions/checkout and super-linter to release hashes, add a least privilege permissions block, set persist-credentials false, and disable Biome lint (ESLint still covers JS/TS). Matches the configuration proven green on PR 310.
vishkaty
added a commit
to vishkaty/AP2
that referenced
this pull request
Aug 10, 2026
…hrough signing ## Observed vs expected The AP2 specification permits a Payment Instrument `type` to define additional properties, but the generated `PaymentInstrument` model silently discards every property beyond `id`, `type`, and `description`. Because signed claims are built with `model_dump()`, those extension fields are absent from the signed Payment Mandate, and a downstream verifier that reads them observes a missing value. Expected: fields a `type` defines (for x402: `payee_address`, `facilitator`) survive parse -> model_dump -> sign -> verify, so a verifier acts on the values the user actually signed. ## Runtime reproduction (main @ e1ea56d) PaymentInstrument( id="x402-usdc-1", type="x402", payee_address="0xAbCd...0001", facilitator="https://facilitator.example", ).model_dump() # -> {'id': 'x402-usdc-1', 'type': 'x402', 'description': None} # payee_address and facilitator are dropped before signing. Driving the actual x402 Credential Provider sample end to end with a genuinely signed mandate chain (destination 0xAbCd...0001, amount 199c), the CP authorizes an EIP-3009 transfer to 0x7099...79C8 (DEFAULT_MERCHANT_ADDRESS) for 12500000 USDC units (the hard-coded 1250c fallback) rather than the verified destination and amount. This is a fail-open on a payment path: a valid, signed mandate is replaced by fabricated fallback values. ## Root cause and exact sites - Model drops the fields: code/sdk/python/ap2/sdk/generated/types/payment_instrument.py#L10-L22 - Signed via model_dump: code/sdk/python/ap2/sdk/sdjwt/common.py#L225-L229 - CP fail-open on the missing field: code/samples/python/src/roles/x402_credentials_provider_mcp/server.py#L148-L170 The generated model carries no `extra` policy, so Pydantic's default silently ignores unknown properties. `code/sdk/schemas/ap2/types/payment_instrument.json` declares no `additionalProperties`, and `generate.py` runs datamodel-codegen which, given no `additionalProperties`, emits a model with the default (ignore) posture. ## Fix Declare the open extension surface in the schema: `payment_instrument.json` gains `"additionalProperties": true`. datamodel-codegen already maps a schema's `additionalProperties` to a Pydantic `extra` policy (`jwk.json` -> `extra='forbid'`; `ucp/types/buyer.json` and `ucp/types/checkout.json` -> `extra='allow'`), so regenerating emits `model_config = ConfigDict(extra='allow')` on `PaymentInstrument`. Pydantic v2 then preserves the extra properties through `model_dump` (hence through signing, parsing, and verification) and exposes them for attribute access. This is schema-driven, not per-type code: the durable invariant lives in the schema (data), it opens the extension surface rather than closing it (so it does not constrain what a `type` may define), and it reuses the same convention the repo already applies to buyer/checkout. It is the AP2 analogue of the extension-preservation fixes made in UCP python-sdk#66 and js-sdk#40. ## Spec grounding - AP2 specification.md, Payment Instrument: "additional properties MAY be defined for that specific `type`." - UCP models preserve extension (`extra`) data through their round trip; this keeps AP2 consistent with that. ## Dedup google-agentic-commerce#299 item 1 is unaddressed by any open PR. google-agentic-commerce#301 (item 2, allowed-instrument matching) explicitly left item 1 to maintainers: "Preserving type-specific instrument fields through parsing and signing (google-agentic-commerce#299 item 1) is a schema and generated-model design decision ... which I have left for maintainers." google-agentic-commerce#300 hardens the sample amount fallback (fail closed) and is complementary: it does not restore the dropped destination field, which this change does at the source. ## Class sweep (schema drops a field that is passed and must travel) | Model | Schema additionalProperties | Passed non-schema fields? | Disposition | |-------|-----------------------------|---------------------------|-------------| | PaymentInstrument | absent -> now `true` | yes: payee_address, facilitator (x402) | CONVERTED | | types/buyer, types/checkout | already `true` | n/a | already `extra='allow'`, no change | | types/jwk | `false` (intentionally closed) | no | out of scope, must stay closed | | PaymentReceipt, CheckoutReceipt | oneOf variants | fields are schema-declared, preserved | out of scope, no drop | | all other generated models | absent (default ignore) | no construction passes non-schema fields | out of scope, no observed extension use | ## Tests Adds code/sdk/python/ap2/tests/payment_instrument_extension_tests.py: - model_dump / delegate-claims preserve x402 extension fields - fields survive the full sign -> verify -> typed-parse round trip - kill-test mirroring the CP extraction: verified destination + amount are sourced, never the default address or the 1250 fallback Full SDK suite: 190 passed. The two failing kb_sd_jwt aud/nonce tests are pre-existing on main and unrelated to this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses the allowed-instrument matching defect reported in #299 (item 2): the Python evaluator matches an allowed
PaymentInstrumentonidalone.AllowedPaymentInstrumentEvaluator.evaluatecompared onlyallowed.id == instrument.id(constraints.py#L246-L251). BecausePaymentInstrument.iduniqueness is not defined acrosstypes, a closed instrument of a different type satisfied the constraint.Failure scenario
Allowed set:
[{id: "pi-1", type: "card"}]. A closed mandate carrying{id: "pi-1", type: "bank"}passed the constraint under id-only matching. Wheretypeselects the payment profile (or ids are scoped by type / credential provider), a different profile than the one the user authorized could satisfy theallowed_payment_instrumentconstraint, and at minimum inconsistent signed instrument metadata is accepted.Fix
Require both
typeandidto match, and includetypein the violation message so a mismatch is diagnosable:Tests
code/sdk/python/ap2/tests/constraints_tests.py:test_payment_allowed_payment_instrument_same_id_different_type_rejected: sameid, differenttypeis a violation.test_payment_allowed_payment_instrument_type_and_id_match: an allowed(type, id)still passes.The two existing allowed-instrument tests are unchanged and still pass.
Validated with
pip install .in a cleanpython:3.12-slimcontainer:constraints_tests.py49 passed.Scope
This PR is intentionally limited to the matching semantics. The other two items in #299 are handled separately:
1250).payment_instrument.jsonplus the datamodel-codegen output), which I have left for maintainers rather than editing a generated file. Happy to follow up if you want a direction on it.