Add status list support for SD-JWT - #561
Conversation
# Conflicts: # eudi/credentials/sdjwtvc/verifier_helpers.go # eudi/credentials/sdjwtvc/verify.go # go.mod # go.sum
# Conflicts: # go.mod
# Conflicts: # eudi/openid4vci/client_test.go
…ign/irmago into sd-jwt-status-lists
![dobby-coder[bot]](https://avatars.githubusercontent.com/in/3365457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for implementing Token Status List (draft-15) support — the package is well-structured and thoroughly tested. A few things to address before merge:
Blocking
statuslist/checker.goreturns an error when the cache write fails, which contradicts its own comment and rejects otherwise-valid credentials on a transient cache failure (fail-closed path). See inline comment.
Non-blocking
openid4vci_statuslist_test.gois self-labeled "UNVERIFIED SCAFFOLDING" yet wired into the main session suite — please validate it against the stack or gate it behind a skip until then.- Status-list checks use
context.Background()instead of a caller-supplied context, so they can't be cancelled by the surrounding session/request.
PR metadata (rule check)
- The body's
Fixes https://github.com/privacybydesign/irmamobile/issues/516is a bare cross-repo URL, which does not auto-close on merge. To have the issue close automatically, use the fully-qualified form on its own line:Fixes privacybydesign/irmamobile#516. (Cross-repo auto-close is best-effort on GitHub; if it still doesn't fire, the issue will need to be closed manually.)
Note: the storage/db/services tests couldn't be exercised in the review sandbox (sqlcipher native dep unavailable — pre-existing infra limitation, not this PR); statuslist + sdjwtvc suites pass.
|
/dobby please review again |
There was a problem hiding this comment.
🧦 Consolidated review — status list support for SD-JWT
Went through the diff plus a per-rule compliance pass (memory rules + irmago repo notes). No blocking correctness/security/rule issues found: concurrency is safe (singleflight + RWMutex cache), status checks are fail-closed with sane TTL bounds [60s, 24h], DB access is parameterized, and unit tests cover the new package. Nice work.
Verdict is a non-blocking COMMENT — three nits below, none merge-blocking.
CI note (non-blocking): integration-test-clientside is red on this run — TestClientIntegration/KeyshareSessions failed with UNEXPECTED_REQUEST (Session not yet started or already finished). That test doesn't touch the status-list code and master is currently green, so it reads as a flake / session-start timing race rather than a regression from this PR — but since master passes, please re-run the job and confirm it's not fallout from the docker-compose restructuring (shared veramo_postgres / new statuslist_agent startup ordering) before merge. The [no tests to run] line for the statuslist package in the same log is also worth a glance (build-tag/_test.go package mismatch?).
|
/dobby review again |
There was a problem hiding this comment.
🧦 Dobby consolidated review — status list support for SD-JWT
Ran the per-rule compliance sweep (irmago repo rules + general PR/code rules) alongside the diff review. The PR is well-structured and thoroughly tested; unit tests pass for all changed packages (integration needs docker, skipped). CHANGELOG claims match the delivered code. One doc/behaviour inconsistency worth a look (inline).
Non-blocking heads-up (CodeQL): the new statuslist/fetcher.go error strings embed remote-server-controlled values (resp.Status, the Content-Type header) which then reach a logger via eudi.Logger.Warnf. irmago's CodeQL +security-and-quality suite has historically flagged log-injection/clear-text on changed log lines (see PR #597). If the CodeQL check goes red on this PR, that flow is the likely cause — sanitize the value before logging or drop it from the message.
Not blocking; posting as COMMENT.
|
/dobby i changed the architecture a bit, please review it one more time |
|
Got it — I'll take another pass over PR #561 now that the architecture's changed and post a fresh review shortly. |
There was a problem hiding this comment.
One note from re-review, non-blocking. The architecture change reads cleanly: the x5c-or-kid+DID dispatch is now shared via eudi/jwt.JwtKeyProvider, status checks are wired at issuance / post-verification / background sweep, and the batch invariant checks in validateStatusReferences look correct (Reference is a value struct of uint64+string, so the != / map-key comparisons are safe). One stale line in the CHANGELOG — see inline.
|
Re-reviewed the updated architecture and posted a review — non-blocking. It reads cleanly overall: the shared One thing to fix: the CHANGELOG still mentions the |
|
/dobby fix the changelog |
|
Got it — I'm on it. I'll take a look at the changelog in this PR and push a fix. |
The flag was removed in the status-list simplification refactor and no longer exists in the tree; a delegated Status Issuer is accepted as long as the signature is trusted and sub matches the referenced uri. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The method's doc claimed the result is joined with its batch's IssuerURL, but CredentialStatusInstance has no such field and the query selects/joins none. Drop the misleading clause. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
🧦 Dobby consolidated review — Token Status List support for SD-JWT.
Changelog fix (your request): done. The stale RequireStatusListIssuerMatch clause was dropped in 42281d2f, and I verified the rest of the Unreleased status-list entry now matches the tree — RefreshStatuses exists, and StatusRefreshService / RefreshAll / status_refresh_service.go are gone from both code and changelog. The TTL wording (prefers the token's own ttl/exp, falls back to HTTP max-age, clamped [60s, 24h]) matches the code — no leftover min(...) claim.
Review + rule sweep: the only finding this cycle was one doc nit — ListInstancesWithStatusReference's comment claimed the result was "joined with its batch's IssuerURL", but CredentialStatusInstance has no such field and the query selects/joins none. I fixed it directly in 0afaf71d (doc-only, builds green). No bug- or rule-level issues remain; PR-metadata (fully-qualified Fixes privacybydesign/irmamobile#516) is correct.
Approving. Nice, well-tested feature.
|
🧦 Done — the changelog is fixed and PR #561 is now approved. I dropped the stale Submitted a formal APPROVE review — nothing left blocking on my side, merge whenever you're ready. |
4 participants
Implementing Token Status List (TSL) draft 15. We don't implement the latest version of the spec yet, because DIIP v5 is specifically talking about this draft.
Fixes privacybydesign/irmamobile#516