From 901f1e2db5b171919fbdd22404614366ed033cf1 Mon Sep 17 00:00:00 2001 From: Vishal Katyal Date: Mon, 10 Aug 2026 17:47:36 -0400 Subject: [PATCH] fix(sdk): preserve type-specific PaymentInstrument extension fields through 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 #299 item 1 is unaddressed by any open PR. #301 (item 2, allowed-instrument matching) explicitly left item 1 to maintainers: "Preserving type-specific instrument fields through parsing and signing (#299 item 1) is a schema and generated-model design decision ... which I have left for maintainers." #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. --- .cspell/custom-words.txt | 4 + .../sdk/generated/types/payment_instrument.py | 5 +- .../payment_instrument_extension_tests.py | 143 ++++++++++++++++++ .../schemas/ap2/types/payment_instrument.json | 3 +- 4 files changed, 153 insertions(+), 2 deletions(-) create mode 100644 code/sdk/python/ap2/tests/payment_instrument_extension_tests.py diff --git a/.cspell/custom-words.txt b/.cspell/custom-words.txt index ce73c361..09a7a9da 100644 --- a/.cspell/custom-words.txt +++ b/.cspell/custom-words.txt @@ -22,6 +22,7 @@ CMSPI cmwallet cncf Cobo +codegen Connor contentnegotiation credman @@ -29,6 +30,7 @@ Crossmint cryptographical CYGPATTERN Dafiti +datamodel disclosable Disclosable davecgh @@ -149,6 +151,8 @@ ropeproject RPCURL Rulebook screenreaders +sdjwt +SECP setlocal sharedpref Shopcider diff --git a/code/sdk/python/ap2/sdk/generated/types/payment_instrument.py b/code/sdk/python/ap2/sdk/generated/types/payment_instrument.py index 81ff9568..b201ec08 100644 --- a/code/sdk/python/ap2/sdk/generated/types/payment_instrument.py +++ b/code/sdk/python/ap2/sdk/generated/types/payment_instrument.py @@ -4,7 +4,7 @@ from __future__ import annotations -from pydantic import BaseModel, Field +from pydantic import BaseModel, ConfigDict, Field class PaymentInstrument(BaseModel): @@ -12,6 +12,9 @@ class PaymentInstrument(BaseModel): Instrument used for payment. """ + model_config = ConfigDict( + extra='allow', + ) id: str = Field(..., description='unique identifier for this instrument') type: str = Field( ..., description='unique string identifying this category of instrument' diff --git a/code/sdk/python/ap2/tests/payment_instrument_extension_tests.py b/code/sdk/python/ap2/tests/payment_instrument_extension_tests.py new file mode 100644 index 00000000..e5621d00 --- /dev/null +++ b/code/sdk/python/ap2/tests/payment_instrument_extension_tests.py @@ -0,0 +1,143 @@ +"""Round-trip tests for type-specific PaymentInstrument extension fields. + +The AP2 specification states that additional properties MAY be defined for a +specific Payment Instrument ``type`` (specification.md, Payment Instrument). +The x402 instrument type carries ``payee_address`` and ``facilitator``. These +fields must survive the full sign -> parse -> verify round trip, because the +signed Payment Mandate is the sole authorization the x402 Credential Provider +acts on. If they are dropped before signing, the Credential Provider reads a +missing ``payee_address`` and falls back to a default payout address and a +hard-coded amount -- a fail-open on a payment path (issue #299 item 1). +""" + +from __future__ import annotations + +from ap2.sdk.generated.open_payment_mandate import OpenPaymentMandate +from ap2.sdk.generated.payment_mandate import PaymentMandate +from ap2.sdk.generated.types.amount import Amount +from ap2.sdk.generated.types.merchant import Merchant +from ap2.sdk.generated.types.payment_instrument import PaymentInstrument +from ap2.sdk.mandate import SdJwtMandate +from ap2.sdk.payment_mandate_chain import PaymentMandateChain +from ap2.sdk.sdjwt import sd_jwt +from ap2.sdk.sdjwt.common import delegate_claims_from_model +from ap2.tests.conftest import make_cnf +from cryptography.hazmat.primitives.asymmetric import ec + + +_X402_PAYEE = '0xAbCd000000000000000000000000000000000001' +_X402_FACILITATOR = 'https://facilitator.example' +_SIGNED_AMOUNT = 199 + +# Sentinels standing in for the x402 Credential Provider fallbacks +# (server.py L148-160): a mismatch means the CP authorized fabricated values. +_DEFAULT_ADDR = '0x000000000000000000000000000000000000dead' +_FALLBACK_AMOUNT = 1250 + + +def _x402_instrument() -> PaymentInstrument: + return PaymentInstrument( + id='x402-instrument-1', + type='x402', + payee_address=_X402_PAYEE, + facilitator=_X402_FACILITATOR, + ) + + +def _closed_payment_mandate() -> PaymentMandate: + return PaymentMandate( + transaction_id='tx_x402', + payee=Merchant(name='Shop', id='s-1'), + payment_amount=Amount(amount=_SIGNED_AMOUNT, currency='USD'), + payment_instrument=_x402_instrument(), + ) + + +def _extract_like_x402_cp(chain: PaymentMandateChain): + """Verbatim mirror of x402_credentials_provider_mcp/server.py L148-160. + + Any deviation from the signed values here is the fail-open being exercised. + """ + try: + payee_address = chain.closed_mandate.payment_instrument.payee_address + amount_cents = chain.closed_mandate.payment_amount.amount + if not payee_address: + payee_address = _DEFAULT_ADDR + except AttributeError: + payee_address = _DEFAULT_ADDR + amount_cents = _FALLBACK_AMOUNT + return payee_address, amount_cents + + +# ── Root cause: the pre-sign serialization drops the extension fields ────── + + +def test_model_dump_preserves_type_specific_extension_fields(): + """``model_dump`` (the pre-sign step) must keep x402 extension fields.""" + dumped = _x402_instrument().model_dump() + assert dumped.get('payee_address') == _X402_PAYEE + assert dumped.get('facilitator') == _X402_FACILITATOR + + +def test_delegate_claims_preserve_type_specific_extension_fields(): + """The delegate claims that actually get signed must carry the fields.""" + claims = delegate_claims_from_model(_x402_instrument()) + assert claims.get('payee_address') == _X402_PAYEE + assert claims.get('facilitator') == _X402_FACILITATOR + + +# ── Full sign -> parse -> verify round trip ──────────────────────────────── + + +def test_x402_extensions_survive_sign_verify_roundtrip( + issuer_key, issuer_public_key +): + """x402 extension fields survive create -> verify -> typed parse.""" + issuer = sd_jwt.create( + payload=_closed_payment_mandate(), issuer_key=issuer_key + ) + parsed = SdJwtMandate.from_sd_jwt( + issuer.sd_jwt_issuance, issuer_public_key, PaymentMandate + ) + instrument = parsed.mandate_payload.payment_instrument + assert instrument.payee_address == _X402_PAYEE + assert instrument.facilitator == _X402_FACILITATOR + + +# ── Kill-test: the x402 Credential Provider fail-open ────────────────────── + + +def test_x402_cp_sources_verified_destination_and_amount_not_fallback( + issuer_key, issuer_public_key +): + """After the round trip, the CP must read verified values, never fallbacks. + + Kill-test for the payment-path fail-open (issue #299 item 1): mirrors the + Credential Provider's extraction. Without preserved extension fields the + ``payee_address`` access raises ``AttributeError`` and the CP authorizes a + default destination plus the hard-coded ``1250`` amount. + """ + dummy = ec.generate_private_key(ec.SECP256R1()) + open_mandate = OpenPaymentMandate( + constraints=[], cnf=make_cnf(dummy.public_key()) + ) + issuer = sd_jwt.create( + payload=_closed_payment_mandate(), issuer_key=issuer_key + ) + parsed = SdJwtMandate.from_sd_jwt( + issuer.sd_jwt_issuance, issuer_public_key, PaymentMandate + ) + chain = PaymentMandateChain( + open_mandate=open_mandate, closed_mandate=parsed.mandate_payload + ) + + payee_address, amount_cents = _extract_like_x402_cp(chain) + + assert payee_address == _X402_PAYEE, ( + 'x402 CP fell back to the default payout address instead of the ' + 'verified, signed destination' + ) + assert amount_cents == _SIGNED_AMOUNT + assert amount_cents != _FALLBACK_AMOUNT, ( + 'x402 CP authorized the hard-coded 1250 fallback amount' + ) diff --git a/code/sdk/schemas/ap2/types/payment_instrument.json b/code/sdk/schemas/ap2/types/payment_instrument.json index 99857b37..89b3bd52 100644 --- a/code/sdk/schemas/ap2/types/payment_instrument.json +++ b/code/sdk/schemas/ap2/types/payment_instrument.json @@ -21,5 +21,6 @@ "required": [ "id", "type" - ] + ], + "additionalProperties": true } \ No newline at end of file