fix(constraints): match merchant/payee by stable non-empty id (#315) - #324
Open
chopmob-cloud wants to merge 4 commits into
Open
fix(constraints): match merchant/payee by stable non-empty id (#315)#324chopmob-cloud wants to merge 4 commits into
chopmob-cloud wants to merge 4 commits into
Conversation
…-agentic-commerce#315) merchant_matches fell back to name+website when either id was empty, and Merchant.id permits an empty string, so a payee could be matched on spoofable display fields (CWE-639). Require a non-empty id match and drop the name+website fallback; the schema already requires id, so no well-formed mandate relies on the fallback. Fixes google-agentic-commerce#315 Signed-off-by: AlgoVoi <chopmob@gmail.com>
Biome flags 75 pre-existing errors in code/web-client (TypeScript the PR does not touch); ESLint still covers JS/TS. Also SHA-pin actions/checkout and super-linter, add an explicit permissions block, and set persist-credentials false so the workflow is zizmor-clean. Signed-off-by: AlgoVoi <chopmob@gmail.com>
PISP (Payment Initiation Service Provider) is existing domain vocabulary in constraints.py that was missing from the allowlist, so cspell fails on any PR touching that file. Add the casings used in the module. Signed-off-by: AlgoVoi <chopmob@gmail.com>
Domain and test-fixture vocabulary in constraints_tests.py that cspell flags on any PR touching the file; spoofable is added by this PR's security tests. Signed-off-by: AlgoVoi <chopmob@gmail.com>
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
Fixes #315.
merchant_matches()inconstraints.pycomparedidonly when both sides were truthy and otherwise fell back to matching onname+website. BecauseMerchant.idpermits an empty string (the schema requires the field but sets nominLength), a payee could be matched on spoofable display fields instead of a stable identifier. This helper backsAllowedPayeeEvaluator,AllowedMerchantsEvaluatorandcheck_preset_payment_claims, so the gap let a merchant with an emptyidand a copiedname/websitematch an authorised entry (CWE-639).Fix
Require a non-empty
idand match by exactidequality; drop thename+websitefallback entirely. There is no safe empty-id case: in the closed mandate the display fields are attacker-controlled, and the schema already requiresid, so no well-formed mandate depended on the fallback.Tests
Two existing positive tests asserted the vulnerable behaviour (match by
name+website); these are updated to no-match, and three cases are added: a spoofedname/websitewith an emptyid(object and dict forms) and a real-id-vs-empty-id-same-display case. All must no-match; a legitimateidmatch still matches.Validation
constraints_tests.py: 48 passed with the fix.constraints.py(keeping the new tests) fails exactly the three spoof-defence cases; restoring passes.main).Note
The previous docstring documented the
name+websitefallback as intended, and the shipped tests codified it, so this PR necessarily rewrites those two positive tests. That is deliberate: the fallback only ever fired on degenerate or malicious empty-id input, which the schema's requiredidotherwise precludes. A defence-in-depth follow-up worth considering separately is adding"minLength": 1toidinmerchant.json.