From df49936b961d4c19cfb4c30448bf93aab11d03ed Mon Sep 17 00:00:00 2001 From: arjun2075 Date: Tue, 22 Sep 2026 06:12:40 -0700 Subject: [PATCH] fix(sdk): verify checkout mandate binding from presented checkout CheckoutMandateChain.verify() evaluated the supplied checkout_jwt against the open mandate's constraints but never derived its hash. The closed mandate's checkout_hash was only compared against an optional caller-provided expected_checkout_hash, so omitting that argument skipped the closed-mandate binding check entirely: a different but otherwise valid Checkout JWT that satisfied the open constraints verified cleanly. Derive the hash from the checkout_jwt being verified and compare it with the closed Checkout Mandate's checkout_hash, making the spec-required binding part of normal chain verification. expected_checkout_hash is retained as a secondary consistency assertion only. Tests cover a mismatched presented checkout with no caller-supplied hash, a caller-supplied hash that cannot mask a mismatch, and a closed mandate whose own checkout_jwt and checkout_hash refer to different checkouts. Per docs/ap2/specification.md (Verification -> Merchant), the Merchant MUST verify that the hash of the Checkout JWT sent for approval matches the checkout_hash claim. PR #330 addresses the analogous Payment Mandate binding and identifies this as the same class of issue. SECP is added to the cspell word list because cspell rescans the whole of checkout_mandate_chain_tests.py once the file is in the changed set, which surfaces the pre-existing ec.SECP256R1() curve name. --- .cspell/custom-words.txt | 1 + .../python/ap2/sdk/checkout_mandate_chain.py | 33 ++- .../ap2/tests/checkout_mandate_chain_tests.py | 200 +++++++++++++++++- 3 files changed, 225 insertions(+), 9 deletions(-) diff --git a/.cspell/custom-words.txt b/.cspell/custom-words.txt index ce73c361..9d7c0a57 100644 --- a/.cspell/custom-words.txt +++ b/.cspell/custom-words.txt @@ -149,6 +149,7 @@ ropeproject RPCURL Rulebook screenreaders +SECP setlocal sharedpref Shopcider diff --git a/code/sdk/python/ap2/sdk/checkout_mandate_chain.py b/code/sdk/python/ap2/sdk/checkout_mandate_chain.py index 2fb4b0e0..df2dd47c 100644 --- a/code/sdk/python/ap2/sdk/checkout_mandate_chain.py +++ b/code/sdk/python/ap2/sdk/checkout_mandate_chain.py @@ -13,7 +13,7 @@ from ap2.sdk.generated.open_checkout_mandate import OpenCheckoutMandate from ap2.sdk.generated.types.checkout import Checkout from ap2.sdk.mandate import _log_event -from ap2.sdk.utils import b64url_decode +from ap2.sdk.utils import b64url_decode, compute_sha256_b64url from pydantic import ValidationError @@ -48,11 +48,20 @@ def verify( ) -> list[str]: """Verifies the constraints of the checkout mandate chain. + The closed Checkout Mandate is bound to one specific Checkout JWT + through its ``checkout_hash`` claim. This method derives the hash from + the ``checkout_jwt`` it is asked to verify and compares it with that + claim, so the binding is always checked and cannot be skipped by + omitting an argument. + Args: - expected_checkout_hash: An optional hash to check against the closed - mandate's checkout_hash. - checkout_jwt: The JWT containing the checkout details, required for - verifying open mandate constraints. + expected_checkout_hash: An optional hash the caller already expects + the closed mandate to be bound to. It is an additional consistency + assertion only; it never substitutes for the hash derived from + ``checkout_jwt``. + checkout_jwt: The serialized JWT containing the checkout details. + Required, both to bind the closed mandate and to verify open + mandate constraints. Returns: A list of strings describing any violations found. @@ -82,6 +91,18 @@ def verify( violations.append(str(e)) return violations + # The closed mandate is bound to the exact serialized Checkout JWT + # whose hash is recorded in checkout_hash. Derive that hash from the + # checkout actually presented for verification rather than trusting a + # value supplied by the caller. + presented_checkout_hash = compute_sha256_b64url(checkout_jwt) + if presented_checkout_hash != self.closed_mandate.checkout_hash: + violations.append( + 'Checkout checkout_hash mismatch: computed' + f' {presented_checkout_hash}, got' + f' {self.closed_mandate.checkout_hash}' + ) + violations.extend( check_checkout_constraints( self.open_mandate, @@ -89,6 +110,8 @@ def verify( ) ) + # Secondary, caller-driven consistency assertion. Retained for + # backwards compatibility; the binding above does not depend on it. if ( expected_checkout_hash is not None and expected_checkout_hash != self.closed_mandate.checkout_hash diff --git a/code/sdk/python/ap2/tests/checkout_mandate_chain_tests.py b/code/sdk/python/ap2/tests/checkout_mandate_chain_tests.py index 6e77ec25..a9f8977f 100644 --- a/code/sdk/python/ap2/tests/checkout_mandate_chain_tests.py +++ b/code/sdk/python/ap2/tests/checkout_mandate_chain_tests.py @@ -14,6 +14,7 @@ Item as MandateItem, ) from ap2.sdk.generated.types.merchant import Merchant +from ap2.sdk.utils import compute_sha256_b64url from ap2.tests.conftest import make_checkout_jwt, make_cnf, make_line_item from cryptography.hazmat.primitives.asymmetric import ec @@ -43,12 +44,13 @@ def test_checkout_chain_verify_constraint_violation(): ), CheckoutMandate( checkout_jwt=checkout_jwt, - checkout_hash='hash', + checkout_hash=compute_sha256_b64url(checkout_jwt), ), ] chain = CheckoutMandateChain.parse(payloads) violations = chain.verify(checkout_jwt=checkout_jwt) assert any('not in allowed list' in v for v in violations) + assert not any('checkout_hash mismatch' in v for v in violations) def test_checkout_chain_hash_mismatch(): @@ -64,7 +66,7 @@ def test_checkout_chain_hash_mismatch(): ), CheckoutMandate( checkout_jwt=checkout_jwt, - checkout_hash='actual_hash', + checkout_hash=compute_sha256_b64url(checkout_jwt), ), ] chain = CheckoutMandateChain.parse(payloads) @@ -73,6 +75,7 @@ def test_checkout_chain_hash_mismatch(): ) assert len(violations) == 1 assert 'Checkout checkout_hash mismatch' in violations[0] + assert 'expected expected_hash' in violations[0] def test_full_checkout_end_to_end( @@ -101,7 +104,10 @@ def test_full_checkout_end_to_end( holder_key=agent_key, mandate_token=open_tok, payloads=[ - CheckoutMandate(checkout_jwt=checkout_jwt, checkout_hash='hash') + CheckoutMandate( + checkout_jwt=checkout_jwt, + checkout_hash=compute_sha256_b64url(checkout_jwt), + ) ], aud='merchant', nonce='merchant-nonce', @@ -141,7 +147,10 @@ def test_checkout_line_items_constraint(): ], cnf=make_cnf(_DUMMY_KEY.public_key()), ), - CheckoutMandate(checkout_jwt=checkout_jwt, checkout_hash='hash'), + CheckoutMandate( + checkout_jwt=checkout_jwt, + checkout_hash=compute_sha256_b64url(checkout_jwt), + ), ] chain = CheckoutMandateChain.parse(payloads) violations = chain.verify(checkout_jwt=checkout_jwt) @@ -172,3 +181,186 @@ def test_checkout_fields_parsed(): assert len(checkout.line_items) == 1 assert checkout.line_items[0].item.id == 'item-1' assert checkout.line_items[0].quantity == 2 + + +def _bound_chain(checkout_jwt: str) -> CheckoutMandateChain: + """Build a chain whose closed mandate is bound to ``checkout_jwt``.""" + return CheckoutMandateChain( + open_mandate=OpenCheckoutMandate( + constraints=[], cnf=make_cnf(_DUMMY_KEY.public_key()) + ), + closed_mandate=CheckoutMandate( + checkout_jwt=checkout_jwt, + checkout_hash=compute_sha256_b64url(checkout_jwt), + ), + ) + + +def test_checkout_binding_matching_checkout_passes(): + """The Checkout JWT the closed mandate is bound to verifies cleanly.""" + checkout_a_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_a', + ) + chain = _bound_chain(checkout_a_jwt) + assert chain.verify(checkout_jwt=checkout_a_jwt) == [] + + +def test_checkout_binding_rejects_other_checkout_without_expected_hash(): + """A different Checkout JWT is rejected with no caller-supplied hash. + + This is the regression anchor: the closed mandate is bound to checkout A, + checkout B is presented for verification, and no + ``expected_checkout_hash`` is supplied. Before the binding fix verify() + never derived the hash itself, so this returned no violations. + """ + checkout_a_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_a', + ) + checkout_b_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_b', + ) + assert checkout_a_jwt != checkout_b_jwt + + chain = _bound_chain(checkout_a_jwt) + + violations = chain.verify(checkout_jwt=checkout_b_jwt) + + assert any('checkout_hash mismatch' in v for v in violations) + + +def test_checkout_chain_rejects_internally_inconsistent_closed_mandate(): + """A closed mandate whose own jwt and hash disagree is rejected. + + Here the presented checkout is the closed mandate's own ``checkout_jwt``, + so the two execution contexts agree. The mandate is nonetheless invalid + because its ``checkout_hash`` records a different checkout, meaning it was + never bound to the checkout it carries. + """ + checkout_a_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_a', + ) + checkout_b_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_b', + ) + chain = CheckoutMandateChain( + open_mandate=OpenCheckoutMandate( + constraints=[], cnf=make_cnf(_DUMMY_KEY.public_key()) + ), + closed_mandate=CheckoutMandate( + checkout_jwt=checkout_a_jwt, + checkout_hash=compute_sha256_b64url(checkout_b_jwt), + ), + ) + # The presented checkout matches the mandate's embedded checkout exactly. + assert chain.closed_mandate.checkout_jwt == checkout_a_jwt + + violations = chain.verify(checkout_jwt=checkout_a_jwt) + + assert any('checkout_hash mismatch' in v for v in violations) + + +def test_checkout_binding_not_skipped_when_expected_hash_agrees(): + """A caller-supplied hash cannot mask a mismatched presented checkout. + + The caller passes the closed mandate's own checkout_hash, so the optional + consistency assertion is satisfied. The derived binding check must still + reject the mismatched checkout. + """ + checkout_a_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_a', + ) + checkout_b_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_b', + ) + chain = _bound_chain(checkout_a_jwt) + + violations = chain.verify( + expected_checkout_hash=chain.closed_mandate.checkout_hash, + checkout_jwt=checkout_b_jwt, + ) + + assert any('checkout_hash mismatch' in v for v in violations) + + +def test_checkout_binding_reported_with_constraints_satisfied(): + """Binding failure is reported even when open constraints all pass.""" + checkout_a_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Good Store'), + checkout_id='chk_a', + ) + checkout_b_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Good Store'), + checkout_id='chk_b', + ) + chain = CheckoutMandateChain( + open_mandate=OpenCheckoutMandate( + constraints=[ + AllowedMerchants( + allowed=[Merchant(id='m-1', name='Good Store')], + ), + ], + cnf=make_cnf(_DUMMY_KEY.public_key()), + ), + closed_mandate=CheckoutMandate( + checkout_jwt=checkout_a_jwt, + checkout_hash=compute_sha256_b64url(checkout_a_jwt), + ), + ) + + violations = chain.verify(checkout_jwt=checkout_b_jwt) + + assert not any('not in allowed list' in v for v in violations) + assert any('checkout_hash mismatch' in v for v in violations) + + +def test_checkout_binding_does_not_leak_checkout_jwt(): + """The mismatch violation does not embed the Checkout JWT contents.""" + checkout_a_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_a', + ) + checkout_b_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_b', + ) + chain = _bound_chain(checkout_a_jwt) + + violations = chain.verify(checkout_jwt=checkout_b_jwt) + + assert violations + for violation in violations: + assert checkout_b_jwt not in violation + assert checkout_a_jwt not in violation + + +def test_checkout_binding_missing_checkout_jwt_still_fails(): + """A missing Checkout JWT remains fail-closed.""" + checkout_a_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_a', + ) + chain = _bound_chain(checkout_a_jwt) + + violations = chain.verify(checkout_jwt=None) + + assert any('checkout_jwt is required' in v for v in violations) + + +def test_checkout_binding_malformed_checkout_jwt_still_fails(): + """A malformed Checkout JWT remains fail-closed.""" + checkout_a_jwt = make_checkout_jwt( + merchant=Merchant(id='m-1', name='Store'), + checkout_id='chk_a', + ) + chain = _bound_chain(checkout_a_jwt) + + violations = chain.verify(checkout_jwt='not-a-jwt') + + assert any('Malformed checkout_jwt' in v for v in violations)