fix(samples): fail closed in x402 PSP when agent-provider key is missing

[chopmob-cloud]

Summary

The x402 PSP settle_payment skipped SD-JWT mandate verification when the agent-provider public key could not be loaded, and then continued to binding, ecrecover and settlement. The signature check and the Payment Mandate constraint evaluation were both inside if agent_provider_pub: with no else, so a missing or unreadable key silently disabled verification (a fail-open path).

Two sibling roles already fail closed on exactly this condition:

• merchant_agent_mcp/server.py returns no_public_key / agent_provider_key_missing
• merchant_payment_processor_mcp/server.py returns agent_provider_key_missing

This change makes the settlement role consistent: it returns agent_provider_key_missing when the key cannot be loaded, before any binding or settlement step. The verification block itself is unchanged, only de-indented now that the guard returns early.

Verification

Reproduced the branch directly against settle_payment (it is a plain function under @mcp.tool()):

• key present, malformed mandate: returns mandate_verification_failed (verification runs).
• key absent, before this change: verification is skipped and execution reaches Step 1 binding.
• key absent, after this change: returns agent_provider_key_missing, binding is never reached.

Tests

Adds code/samples/python/tests/:

• settle_payment_failclosed_tests.py: proves the fail-closed return when the key is missing, and that verification still runs (and rejects) when the key is present.
• conftest.py: puts the samples src on sys.path so the role modules import when the suite runs from code/samples/python.

Both pass locally on Python 3.12. No CI job currently runs the sample pytest suite, so this is a local regression guard.

Notes

• Also adds domain terms already present in x402_psp_mcp/server.py (fastmcp, sdjwt, keccak, toggleable, usdc, gwei, levelname, sepolia) to .cspell/custom-words.txt so the touched file passes the spellcheck job, which checks the whole changed file.
• Filed in response to the observation in [Bug]: x402 PSP continues without mandate verification when the agent-provider key is unavailable, while two other roles refuse #309. Whether the sample intended to tolerate a missing verification key for local demonstration is a maintainer call; either way this keeps the three verifying roles consistent and avoids a silent skip of signature verification.

Refs #309

[gemini-code-assist]

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

[giorgioroth]

Thanks for picking this up. Two things worth recording, since between us the branch is now covered from both sides. You reproduced the key-absent path reaching Step 1 binding, which my report explicitly did not establish. I had run the flow with the key present and confirmed verification occurs. It is no longer a source observation.

The diff reads correctly to me. Both loss paths leave agent_provider_pub as None — the missing file and the read error — and the early return keys on the variable rather than on the failure mode, so it catches both. The verification block is unchanged, only de-indented. The second test is the one that matters: it proves verification still runs and rejects when the key is present, which is what stops a fail-closed fix from quietly becoming a fail-always one.

One thing you fixed that was not in my report: the except branch previously logged "key not found — skipping" for a corrupt or unreadable file, which was inaccurate, and used warning rather than exception, which lost the traceback. _logger.exception("Failed to load agent-provider public key") is the right message for that path.

On the failing check — BIOME_LINT is the only one red, and this PR touches three Python files. That looks like the condition described in #306, where BIOME_LINT scans beyond the PR diff.

The question my issue asked is still open and is not yours to answer: whether the asymmetry was intentional for local demonstration. The fix is right either way.

[Silentpartnercoding]

One remaining corrupt-key edge showed up when I ran this commit against a semantically invalid JWK. With AGENT_PROVIDER_PUB_PATH containing {}, JWK.from_json() raises jwcrypto.jwk.InvalidJWKType; that is a JWException, not a ValueError or JSONDecodeError, so it escapes the new guard before agent_provider_key_missing is returned.

I verified that importing JWException from jwcrypto.common and including it in the key-load exception tuple makes this case fail closed. A regression case using {} (rather than non-JSON text) would cover the library-level key-validation branch. The missing-key and valid-key tests otherwise look right.

[chopmob-cloud]

Thanks @Silentpartnercoding, confirmed and fixed. JWK.from_json("{}") does raise jwcrypto.jwk.InvalidJWKType (a JWException), which escaped the (OSError, ValueError, JSONDecodeError) guard.

While adding that, an adversarial pass over the key-load path turned up a related family: a key file that is valid JSON but not an object ([], null, 123) makes JWK.from_json raise TypeError, which also escaped and propagated. Rather than enumerate library exception types (which this shows is leaky), I changed the key-load except to catch broadly, so any failure to load or validate the key (missing, unreadable, malformed JSON, wrong JSON type, or invalid JWK) fails closed via the existing agent_provider_key_missing guard.

The regression test is now parametrized over invalid-JWK, non-object-JSON and empty-file inputs. Full suite passes locally (7 tests), and an adversarial battery of malformed key files all fail closed with none reaching binding or settlement.

Thanks also @giorgioroth for the review. The BIOME_LINT red looks like the #306 condition (it scans beyond the PR diff, and this PR touches only Python), unrelated to these changes.

[arjun2075]

I noticed the note here that the new sample regression tests currently don't run in CI. I checked the workflows on main as well and don't see a Python test job for the SDK/sample suites.

Given that #300, #301, and this PR are adding regression coverage around payment authorization and fail-closed behavior, would maintainers be open to a small follow-up PR that adds a scoped Python CI job for the SDK and security-relevant sample tests?

I'd keep it separate from this fix and limited to test execution — no protocol or sample behavior changes. Happy to put together a small version using the repo's preferred dependency/setup conventions if that would be useful.

[chopmob-cloud]

Good check, and you are right that nothing runs the Python suites today. Please do open the scoped CI job PR you proposed, that is the right home for it and better coming from you.

One finding to save you a red first run: two existing SDK tests already fail on current main, test_verify_rejects_aud_mismatch and test_verify_rejects_nonce_mismatch in code/sdk/python/ap2/tests/kb_sd_jwt_intermediate_tests.py. kb_sd_jwt.verify only enforces expected_aud and expected_nonce on terminal hops, so the checks are skipped on intermediate hops and a Python CI job would go red on main until that is fixed. Happy to send a small standalone PR for just those two so your CI job can land green, or you are welcome to fold it into yours. Let me know which you prefer.

[arjun2075]

Thanks — that sounds good.

I'd prefer to keep the existing kb_sd_jwt verification fix separate so the CI PR stays purely infrastructure/test execution and doesn't mix in a behavioral change. If you're happy to send the small standalone PR for those two failing tests, please go ahead.

Once that lands (or is otherwise resolved), I'll open the scoped Python CI PR against the green baseline. I'll use the repo's existing uv setup and keep the workflow limited to the SDK/sample regression suites we discussed.

Appreciate the heads-up — that saved exactly the kind of confusing first red run CI is supposed to help us avoid.

[giorgioroth]

One small suggestion before kb_sd_jwt.verify moves: worth confirming the intended contract for expected_aud and expected_nonce on intermediate hops first, since the two tests in code/sdk/python/ap2/tests/kb_sd_jwt_intermediate_tests.py already read more like a description of that contract than of current behaviour - both name their values correct against a wrong expectation, match on an aud mismatch / nonce mismatch message, and build a hop carrying cnf for the next one.

The spec side points the same way: aud and nonce are the values that bind a presentation to a verifier and a request. Whether that binding is intended to hold at every hop or only at the terminal one is the question. If it holds at every hop, then the tests are expressing the contract and the implementation is what is out of step.

Either way, the ordering you two settled on is the important part: a small behavioral PR for those two, then the CI-only PR against a green baseline. That keeps the test runner from accidentally deciding protocol semantics merely because an existing test is red.

[chopmob-cloud]

Agreed, and I think the repo settles the contract question on its own. create requires aud and nonce on every KB-SD-JWT hop, intermediate included (if not aud or not nonce: raises), and test_create_requires_aud_and_nonce sits in the intermediate suite and pins that. So both claims are always issued at every hop, and verify's docstring lists the expected match as a general check, "If expected_aud / expected_nonce are provided, they match", not a terminal only one.

The implementation is the part out of step: verify only calls verify_expected_claims under if typ in TYP_TERMINAL, so a caller pinning expected_aud or expected_nonce on an intermediate hop gets no check at all. The tests express the documented and issued contract; current behaviour does not.

So the change stays small: enforce the expected values verify is handed on intermediate hops too, as the docstring already states, with no new presence rule and no protocol change. @giorgioroth, you have mapped that contract more cleanly than the tests do, so I would rather build it with you than solo, whether that is reviewing or co-authoring the branch. @arjun2075, your catch that nothing runs the Python suites yet is what set the sensible ordering here, and I would like to keep this a joint effort across the three of us: the behavioural PR for those two first, co-authored with whoever wants in, then your CI only PR against the green baseline. I will coordinate so both land clean.

[arjun2075]

Thanks again for the encouragement — I opened the scoped CI work as Draft PR #325:

#325

I kept it limited to CI/test execution only. The draft currently treats #310 and #313 as upstream prerequisites, so I expect it to remain blocked until those land.

Once both are on main, I'll rebase #325, rerun the SDK and sample suites on the merged baseline, and mark it ready for review only once everything is green.

[giorgioroth]

Thanks @chopmob-cloud, happy to join on the contract and adversarial-review side.

The property looks settled now: if verify is given an expected_aud or expected_nonce, that expectation should participate in verification regardless of whether the hop is intermediate or terminal. Otherwise the caller supplies a constraint that is silently ignored.

I’ll take a look at #313 from the contract and adversarial side - tests, boundary, and failure cases.

[chopmob-cloud]

Quick coordination note so we do not double up. The behavioural fix for those two red tests (test_verify_rejects_aud_mismatch and test_verify_rejects_nonce_mismatch) already exists as #313, open since 5 August. Rather than open a second branch I reviewed #313 against current main and left notes there: it fixes both tests by honouring expected_aud and expected_nonce on every hop, which is the minimal change the verify() docstring and create() already imply.

The ordering we settled still holds. #313 lands the behavioural fix and gives the green baseline, then @arjun2075's CI-only #325 runs against it. @giorgioroth, glad to have you on the contract and adversarial side of #313; that is where the remaining judgement call sits, whether the stronger terminal-binding change rides along or splits out.

[giorgioroth]

I think these are two separate properties and should be split.

The minimal fix says a supplied constraint cannot be silently ignored. verify() documents that supplied expected_aud / expected_nonce are matched, with no restriction to terminal hops - and the very next line of the same docstring does condition on hop type, for cnf. So the terminal-only guard around verify_expected_claims(...) reads as a discrepancy with the documented behaviour rather than an unwritten rule. create() requiring both on every hop means the claims are there to be checked; it is evidence about what this implementation emits, not yet about what verify() must accept.

The stronger change is a different property: it says the binding must be present even when the caller supplied no expectation. That moves a rule from the issuer to the verifier, changes what counts as an acceptable hop, and can refuse tokens that verify today - including ones this SDK did not produce.

I'd keep #313 to the documented supplied-expectation fix that restores the green baseline #325 depends on, and take mandatory presence separately, with its own tests and a compatibility rationale.