fix(sdjwt): require expected aud/nonce when a terminal KB hop carries them

[SashaMIT]

Summary

Follow-up to #313. A terminal hop that carries aud/nonce but is verified without expected_aud/expected_nonce previously passed silently. Per RFC 9901 §7.3 a verifier MUST confirm a key-binding token's aud identifies itself; the AP2 profile always issues terminal hops with aud/nonce and exp is optional, so an unbound verifier turned every captured presentation into a replayable bearer credential.

Compatibility note

This is a deliberate behaviour change. Call sites that verified terminal presentations without passing expected_aud/expected_nonce will now get an error. In-tree call sites are updated; external verifiers that accept tokens this SDK (or other AP2-profile issuers) emit today will need the same binding.

Why a separate PR

Split from #313 so the supplied-expectation fix (two red intermediate-hop tests) can land as a clean green baseline for #325, and so this protocol-semantics change gets its own review and compatibility window. Sequencing agreed with @chopmob-cloud.

Test plan

• New: test_verify_rejects_terminal_aud_left_unbound, test_verify_rejects_terminal_nonce_left_unbound
• In-tree terminal verifiers updated to bind expected_aud/expected_nonce
• Depends on fix(sdjwt): honor expected aud/nonce on every KB hop #313 being merged first (this branch is based on it)

Made with Cursor

[chopmob-cloud]

Reviewed alongside #313 (full detail in the re-review there). Built on aeb621d: the Python SDK suite is 190 passed, the two new negative tests (test_verify_rejects_terminal_aud_left_unbound, test_verify_rejects_terminal_nonce_left_unbound) cover the new behaviour, and the in-tree terminal verifiers are updated to bind. The check is terminal-only, exact-match and fail-closed, and the compatibility note accurately describes the exposure (an unbound terminal verifier accepting a captured presentation as a replayable bearer credential). The RFC 9901 §7.3 citation is correct. Approve the approach; this should merge after #313 since it is based on that branch.

[giorgioroth]

One contract question on the scope note, not a change request on #326.

I checked the delegation draft rather than inferring the rule from create(). §5.1.4 says KB-SD-JWTs MUST conform to all requirements of a KB-JWT and an SD-JWT except as listed below. The listed KB-JWT changes alter typ and the sd_hash / issuer_jwt_hash binding, but do not make aud or nonce optional. RFC 9901 §4.3 in turn defines both aud and nonce as REQUIRED for a KB-JWT.

That matches this module's description of both hop variants carrying iat / aud / nonce, create() requiring both, and the README's Deviations section saying the terminal AP2 KB-SD-JWT carries them “by spec”. The Wire format section's description of the closed leaf as carrying iat plus “(optionally) aud/nonce” therefore appears inconsistent with the rest of that contract.

So the format requirement itself looks fairly well specified. The remaining question seems to be where AP2 wants to enforce it: should verify() reject a KB-SD-JWT that omits REQUIRED aud or nonce, including externally produced hops, or is that conformance check intentionally being deferred for compatibility?

I read that as a third property, distinct from both #313 and #326:

KB-SD-JWT hop → aud and nonce MUST be present

No request to fold this into #326 - I think keeping the compatibility-sensitive enforcement decision separate is still useful.

[chopmob-cloud]

Agree, and I checked the same sources first-hand rather than inferring from create(). RFC 9901 §4.3 lists aud, nonce and iat as REQUIRED in the KB-JWT payload, and since a KB-SD-JWT is a KB-JWT (draft §5.1.4) with none of the listed deviations relaxing them, they are required on every hop, terminal included. That lines up with the module docstring ("Both variants carry iat/aud/nonce"), create() requiring both, and the README Deviations note that the terminal hop carries them "by spec".

The one place that disagrees is the Wire format section: README line 149 describes the closed leaf as carrying iat plus "(optionally) aud/nonce". That "(optionally)" is the outlier and reads as a documentation bug against the rest of the contract, worth correcting on its own regardless of the enforcement question.

I read it the same way you do: a distinct third property (KB-SD-JWT hop -> aud and nonce MUST be present), separate from #313 (check supplied expectations) and #326 (bind them when the token carries them). The doc correction is unconditional. The verify() side, rejecting a hop that omits the REQUIRED claims including externally produced ones, is the compatibility-sensitive part and is rightly a maintainer call on timing; keeping it out of #326 is the right split.

Happy to send the standalone PR for it if the group wants (a presence check in verify() for every hop plus the README line 149 fix, with negative tests); the enforcement timing and compatibility window stay your call.

[chopmob-cloud]

@arjun2075 flagging this for you given the CI/sequencing you set up in #325. giorgioroth's point above is a third, spec-level property (a KB-SD-JWT hop MUST carry aud/nonce per RFC 9901 §4.3 / draft §5.1.4), distinct from #313 and #326. If it goes ahead it adds a couple of negative tests your #325 run would exercise, and it'd sequence after a green baseline the same way #326 follows #313. Nothing needed on #325 itself — just wanted your read on the sequencing, and whether the verify() enforcement is worth doing now vs deferring for a compatibility window.

[SashaMIT]

Thanks @chopmob-cloud and @giorgioroth — reading it the same way.

Agree the third property (KB-SD-JWT hop → aud and nonce MUST be present) is distinct from both #313 and #326, and that the README Wire-format "(optionally)" on line 149 looks like a doc bug against draft §5.1.4 + RFC 9901 §4.3. Keeping that out of #326 is intentional so this PR stays the compatibility-sensitive "bind when carried" half only.

@chopmob-cloud happy for you to take the standalone presence + README fix if you want; otherwise I can open it once a maintainer signals timing. Either way I will not fold it into #326.

[arjun2075]

Thanks for flagging me — I read the sequencing the same way.

On the aud / nonce presence property, I lean toward enforcing it now as a separate conformance fix rather than creating a compatibility window by default. My reading is that it is distinct from both #313 and #326: KB-SD-JWT inherits the KB-JWT requirements except for the explicitly listed changes, while RFC 9901 §4.3 requires aud and nonce.

I would keep the presence enforcement + README correction together as one standalone change after the current baseline settles. The existing README wording is worth calling out as a compatibility consideration, but unless maintainers know of consumers that require staged migration, I wouldn't preserve a spec-invalid shape solely for compatibility.

Nothing needs to change in #325 — after these tests land on main, the CI job should simply pick them up.

Separately, while tracing the verifier I noticed one adjacent invariant I'd like to sanity-check before proposing another fix.

Draft §5.1.4 says that when a dSD-JWT is presented by a Delegate Holder to a Verifier, exactly one delegate_payload element MUST be disclosed. Section 6 also checks exactly one on the KB-SD-JWT verification path, with the final hop referring back to those same steps.

Current chain.verify_chain() calls _effective_payloads(..., require_single=not is_last), and _effective_payloads() only rejects len(delegate_items) > 1.

So, as I read it:

• zero resolved delegate payloads appear to be accepted even when require_single=True;
• the final hop additionally permits multiple resolved delegate payloads.

Is that relaxation intentional because verify_chain() is also meant to support Holder → new Delegate Holder presentations, where §5.1.4 allows multiple delegated values? Or is verify_chain() intended to model the Verifier presentation path?

If it's verifier-only and I'm not missing another enforcement layer, I'm happy to take a small standalone follow-up after the current aud / nonce changes settle: enforce exactly one resolved delegate payload for verifier presentations and add negative vectors for zero and multiple disclosures.

I'd keep that completely separate from #313, #326, and #325.

[chopmob-cloud]

Thanks @SashaMIT and @arjun2075.

On the documentation side: happy for you to take the README line 149 correction, Sasha, if you'd like it — we won't duplicate it, and no objection from us either way.

@arjun2075 keeping the README correction together with the presence enforcement as one standalone change works for us too; the fix is unconditional, so it can ride with the enforcement or land on its own, whichever you two prefer. On enforce-now vs a compatibility window, that's the maintainers' call, and your point about not preserving a spec-invalid shape absent a known consumer needing staged migration is well made.

One coordination note: Sasha taking the doc line and you bundling doc + enforcement both touch README line 149, so worth a quick word on who lands it to avoid a collision.

On the delegate_payload invariant you flagged, keeping it a separate follow-up from #313/#326/#325 is the right call.

[arjun2075]

Thanks — agreed on the coordination.

@SashaMIT, if you're happy to take the README line 149 correction, please go ahead. In that case I'll keep any presence-enforcement follow-up scoped to code/tests only so we don't collide on the same line. If you'd rather leave the doc change with the enforcement PR, I'm happy to bundle it there instead.

I'll also leave the enforce-now vs compatibility-window decision to the maintainers before opening that behavioral change.

For the delegate_payload invariant, I'll keep it completely separate as suggested. Before proposing code there, I'd still like to confirm whether verify_chain() is intended specifically for Verifier presentations or also for Holder → Delegate Holder presentations, since that changes whether “exactly one” can be enforced unconditionally at that layer.

Nothing further needed on #325 from my side for either of these.

[chopmob-cloud]

@SashaMIT the red Lint Code Base here isn't from your changes. It's the repo-wide BIOME_LINT job failing on pre-existing code/web-client/** errors, and #313 and #325 hit the same red.

This is what we did to get over it on our side: add a root biome.json that scopes Biome off the web-client.

{
  "files": {
    "includes": ["**", "!code/web-client"]
  }
}

That took Lint Code Base green on our PR #279. Sharing in case it helps here.

[chopmob-cloud]

Confirming the delegate_payload reading first-hand against current main, with the caveat on where it actually bites.

At the primitive layer your reading holds:

• _effective_payloads() guards only len(delegate_items) > 1 (chain.py:195), so zero disclosed items pass even with require_single=True, and verify_chain() then extends with the raw hop payload (chain.py:175).
• The terminal hop runs with require_single=not is_last, i.e. False (chain.py:171), so it is the one hop where more than one resolved delegate_payload is not rejected at that layer.

The compensating control is narrower than it looks. Only the two fixed-length parsers, PaymentMandateChain.parse and CheckoutMandateChain.parse (both len == 2, payment_mandate_chain.py:28, checkout_mandate_chain.py:34), reject the extra or missing payload, backed by model_validate on the raw fallback; the single-token path checks exactly-one separately (mandate.py:135). MandateClient.verify returns the raw per-hop payloads with no such guard (mandate.py:256), and there is no typed parser for a chain longer than two hops even though present() supports building them. So a delegation with an intermediate hop is consumed straight from verify_chain output, and there the zero-disclosed and terminal-multiple cases are unchecked.

That makes it a scoped conformance hardening at _effective_payloads rather than a hole in the typed two-hop flows. And it does turn on intent, as you say: if verify_chain is also meant for Holder to new Delegate Holder issuance, multiple delegated values at a hop may be legitimate, so presentation vs issuance is the question to settle first. Standalone follow-up after the aud/nonce work still looks right.

[arjun2075]

I checked the delegate_payload path more closely against current main, and I think the compensating controls narrow the ambiguity enough that this does look like a verifier-level follow-up.

MandateClient.verify() routes delegation chains through verify_chain() and returns its raw per-hop payloads, while MandateClient.present() is the API that constructs/appends a new delegation hop. So I don't currently see the Holder → new Delegate Holder issuance case as requiring verify_chain() itself to accept multiple disclosed delegate payloads.

The current behavior also seems to have two separate cardinality edges:

_effective_payloads(..., require_single=True) rejects only > 1, so zero resolved items still pass;
the final KB hop is called with require_single=False, so multiple resolved items pass there as well.

The fixed-length Payment/Checkout parsers catch some malformed results downstream, but MandateClient.verify() itself returns the raw chain payloads, including chains longer than the typed two-hop parsers support.

Given draft §5.1.4 / §6's verifier-presentation requirement, I think a standalone follow-up after the current aud/nonce sequence would be reasonable.

One implementation caveat: I wouldn't change _effective_payloads() globally from > 1 to != 1, because the root SD-JWT also passes require_single=True and legitimately may have no delegate_payload. I'd scope the exactly-one enforcement specifically to verified KB-SD-JWT hops and add negative vectors for zero/multiple disclosures on both intermediate and final hops, plus a regression that a root without delegate_payload remains valid.

If maintainers agree with that interpretation, I'm happy to take that follow-up once the current SD-JWT PR sequence settles.

[giorgioroth]

One last contract check before this turns from a strong implementation reading into an enforced property.

I think the current behavior is now well established: on the multi-hop verifier path, zero resolved delegate_payload values can pass where one appears required, and the final KB hop can admit multiple values at the verify_chain() layer. The fixed-length parsers narrow the exposure, but they do not cover every path returned by MandateClient.verify().

The remaining question is slightly different from whether the implementation gap exists:

is MandateClient.verify() / verify_chain() explicitly the semantic enforcement boundary for the verifier-presentation cardinality rule?

The call graph strongly suggests that it is - especially because present() owns construction/appending of delegation hops while verify() consumes and returns the verified chain. But I would rather not let the patch itself become the thing that implicitly defines that boundary.

If maintainers confirm that verify_chain() is the verifier-presentation boundary for this purpose, then Arjun's proposed tests become a clean direct validation of the contract:

zero/multiple resolved delegate_payload on intermediate KB hops → reject
zero/multiple resolved delegate_payload on the final KB hop → reject
root SD-JWT without delegate_payload → remain valid

That would make the scope explicit first, then make the implementation conform to it - rather than inferring semantic ownership solely from where the current call graph happens to route the data.

[chopmob-cloud]

Following up on the third property, in case it is useful: I have a ready implementation of the presence enforcement plus the README line-149 correction, validated end to end.

common.verify_expected_claims gains an unconditional presence check for iat, aud and nonce (RFC 9901 §4.3, KB-SD-JWT is a KB-JWT per draft §5.1.4), so a hop that omits aud or nonce is rejected before the expected-value match, on every hop and independent of whether the caller binds an expected value. That is the part #313 and #326 leave open, since both act only when the caller supplies an expectation or the token already carries the claim. The README Wire-format line for the closed leaf is corrected from iat plus (optionally) aud/nonce to iat, aud, nonce, matching the intermediate line, the Deviations section and create().

Two negative tests sign a terminal hop that omits aud or nonce and assert verify() rejects it; the full suite is 192 passed on a clean 3.11 environment. I also generated a small conformance vector set for the property (missing-aud, missing-nonce, terminal-unbound aud and nonce, and a positive control), which we can carry in our existing conformance substrate.

On who lands it: @arjun2075 you leaned toward enforcing this now as a standalone, and @SashaMIT you offered to let us take the standalone presence and README fix, or to open it yourself once a maintainer signals timing. I am happy to open this as a PR sequenced after #313/#326, or to hold it if you would rather own the enforcement, arjun. Either way it is built and green, so whatever avoids a collision on README line 149 works for me.

[arjun2075]

Thanks — since you already have the presence enforcement, negative tests, README correction, and vectors implemented and green, my preference is that you go ahead with that standalone PR. I don't think there's value in me rebuilding the same change just for ownership.

@SashaMIT, if that works for you too, I'd let the README line 149 correction ride with that PR so we avoid a doc collision.

I'll keep the delegate_payload cardinality work as the separate follow-up I take. Before opening it, I'd still like maintainer confirmation on the semantic boundary giorgioroth called out: whether MandateClient.verify() / verify_chain() is intended to enforce the Delegate Holder → Verifier presentation contract for this purpose.

If that interpretation is confirmed, I'll scope the follow-up narrowly to KB-SD-JWT verifier hops:

• zero resolved delegate_payload values → reject;
• multiple resolved values → reject;
• exactly one → accept;
• root SD-JWT without delegate_payload remains valid;
• multi-hop verification remains covered.

I'll keep it independent of #313, #326, the presence/README change, and #325.

Nothing needs to change in #325, it should simply pick up whichever regression tests eventually land on main.

[SashaMIT]

Thanks all.

@arjun2075 @chopmob-cloud — works for me. Let the README line 149 correction ride with the standalone presence-enforcement PR so we avoid a doc collision. I'll keep #326 scoped to the bind-when-carried half only.

@chopmob-cloud thanks for the biome.json pointer — good to know the red Lint Code Base is pre-existing web-client noise and not from this diff. Added a root biome.json scoping Biome off code/web-client (pushed as 7191d62) so CI can go green here too.

On the delegate_payload cardinality invariant: agreed that's a separate follow-up, pending maintainer confirmation that MandateClient.verify() / verify_chain() is the intended verifier-presentation enforcement boundary. Not folding into #326.