fix: enforce execution window for immediate payments - #318
Open
Silentpartnercoding wants to merge 2 commits into
Open
Conversation
Author
|
CI follow-up: the incremental spellcheck scanned the complete modified Python files and surfaced six existing AP2 payment/security terms that were absent from the repository dictionary. Commit The remaining |
vishkaty
added a commit
to vishkaty/AP2
that referenced
this pull request
Aug 10, 2026
…is unverifiable (google-agentic-commerce#328) Observed vs expected -------------------- `PaymentMandateChain.verify()` treats `expected_transaction_id` as optional and checks the closed mandate's `transaction_id` only when it is supplied. A verifier that passes only `expected_open_checkout_hash` therefore accepts a closed Payment Mandate whose `transaction_id` binds a DIFFERENT Checkout JWT than the one being processed. Expected: verification cannot succeed unless the closed checkout binding is confirmed. Runtime repro (AP2 main @ e1ea56d, code/sdk/python) --------------------------------------------------- Mismatched-but-accepted (the bug): chain.verify(expected_open_checkout_hash=OPEN_HASH) # closed tx binds a # different JWT -> [] # ACCEPTED Supplied-correctly control (proves the check works when given the value): chain.verify(expected_transaction_id=REAL_JWT_HASH, expected_open_checkout_hash=OPEN_HASH) -> ['Payment transaction_id mismatch: expected ..., got ...'] So the vulnerability is the optional default silently skipping the closed-binding check, not the comparison itself. Exact location -------------- code/sdk/python/ap2/sdk/payment_mandate_chain.py:38-93 (verify): the `transaction_id` comparison was gated on `expected_transaction_id is not None`. code/sdk/python/ap2/sdk/constraints.py:140-169 (PaymentReferenceEvaluator) enforces only the OPEN-side `payment.reference`; it never inspects the closed `transaction_id`. Spec grounding (both) --------------------- docs/ap2/security_and_privacy_considerations.md L19-21 ("Manipulated Checkout"): "The Payment Mandate MUST contain a reference to its associated Checkout. This is via `transaction_id` for closed Payment Mandates and the `mandate.payment.reference` constraint for open ones." BOTH bindings are mandatory; the SDK enforced only the latter by default. `transaction_id` is a REQUIRED field whose schema description is "base64url-encoded hash of the checkout_jwt ... uniquely identifying the checkout" (generated/payment_mandate.py:25). UCP cross-check: the closed `transaction_id` hashes the per-session Checkout JWT that UCP already carries; enforcing the closed binding strengthens and does not contradict UCP. Design rationale (fail closed without trapping legit callers) ------------------------------------------------------------- The authoritative value for the closed binding is the hash of the Checkout JWT the verifier is fulfilling, which is EXTERNAL to the chain (the chain holds only open+closed mandates; the open `conditional_transaction_id` hashes a different artifact, as the MPP passing two distinct values confirms). A chain cannot self-certify which external checkout it is bound to, so deriving the expectation internally (comparing `transaction_id` to itself) is a tautology that closes nothing. The fix therefore REQUIRES the caller to assert the checkout it is processing: `verify()` fails closed when `expected_transaction_id` is absent OR blank (mirroring the falsy guard the open-side evaluator already uses), reporting a violation instead of silently passing. Legitimate callers are never trapped: any holder of a full chain is downstream of a Checkout JWT (the required `transaction_id` IS that JWT's hash), and a caller doing constraints-only analysis that is not a settlement decision has a bounded, honest exit -- the existing public `check_payment_constraints()` -- a distinct, self-describing entry point that cannot be laundered to look like a full verify(). No boolean opt-out is introduced, keeping the surface minimal and un-relaxable. Dedup ----- Reporter mh-yu, no assignee, OPEN. No open PR addresses google-agentic-commerce#328 semantically. Two open PRs touch the same files on orthogonal concerns and rebase trivially: google-agentic-commerce#318 adds a `current_time` param to the same `verify()` (execution window), and google-agentic-commerce#326 edits the same test file (terminal KB aud/nonce). The x402_psp one-line caller change here textually overlaps google-agentic-commerce#310, which rewrites that settle block but keeps the vulnerable verify call unchanged (so google-agentic-commerce#310 does not fix the caller); the added keyword carries onto google-agentic-commerce#310 on rebase. Distinct from google-agentic-commerce#315/google-agentic-commerce#317/google-agentic-commerce#319/google-agentic-commerce#320/google-agentic-commerce#268. Class sweep (optional param that skips a required binding when omitted) ---------------------------------------------------------------------- CONVERTED PaymentMandateChain.verify / expected_transaction_id -- this fix. NOT VULN CheckoutMandateChain.verify / checkout_jwt -- already fails closed (returns a violation when absent). NOT VULN PaymentReferenceEvaluator / open_checkout_hash -- already fails closed when a payment.reference constraint is present. NOT VULN AgentRecurrence/Budget / mandate_context -- already fail closed when the constraint needing context is present. SIBLING CheckoutMandateChain.verify / expected_checkout_hash -- same class, deferred: distinct spec clause (L46-53 checkout_hash<->checkout_jwt) and its fail-closed form also requires updating merchant_agent_mcp:881 and 3 SDK tests that omit it; warrants its own issue. DEFERRED Open-side vacuity when no payment.reference constraint exists -- an unconditional requirement would trap recurring/budget open mandates, which legitimately span multiple checkouts; the closed-side fix already backstops each individual settlement. Warrants separate design. OUT (filed) MandateClient.verify / expected_aud,expected_nonce (google-agentic-commerce#319, PRs google-agentic-commerce#313/google-agentic-commerce#326); ExecutionDateEvaluator / execution_date (google-agentic-commerce#317, PR google-agentic-commerce#318).
Silentpartnercoding
marked this pull request as ready for review
August 18, 2026 19:31
Author
|
Marking this ready for review — it was left in draft by oversight; the work described above is complete and unchanged since 7 Aug. Status for a reviewer, so the failing check doesn't cost you time:
No rush from my side — flagging only because a red check on an otherwise-quiet PR tends to read as "author has work to do," and in this case I don't think it is. |
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.
What changed
Treat an omitted closed-mandate
execution_dateas immediate execution and evaluate that effective time against the open mandate's authorized execution window.The verifier clock is injectable through
PaymentMandateChain.verify()andcheck_payment_constraints()for deterministic verification and tests. When no clock is supplied, the verifier uses the current UTC time and normalizes the derived ISO 8601 value toZ.Why
The generated payment-mandate schema describes an omitted
execution_dateas immediate execution. Previously,ExecutionDateEvaluatorreturned no violations whenever that value was omitted, so a payment could pass without enforcing the open mandate'snot_before/not_afterwindow.Fixes #317.
Validation
Regression coverage proves that an immediate payment:
not_before;not_after; andLocal results: