fix(vc): fail closed when credential revocation cannot be verified - #135
Open
venables wants to merge 3 commits into
Open
fix(vc): fail closed when credential revocation cannot be verified#135venables wants to merge 3 commits into
venables wants to merge 3 commits into
Conversation
did-jwt-vc builds the credential issuer as `{ id: iss, ...payload.issuer }`,
so an `issuer` object in the JWT payload replaces the `id` taken from `iss`.
The signature binds `iss` only, and nothing compared the two.
Anyone could sign a credential with their own key, name another DID as the
issuer, and pass every issuer check downstream: `trustedIssuers` in
`verifyParsedCredential`, `trustedReceiptIssuers` in `verifyPaymentReceipt`,
and the status list issuer check in `isRevoked`.
`parseJwtCredential` now rejects a credential whose `issuer.id` differs from
the verified signer. The presentation path has the same weakness through the
re-exported `verifyPresentation`; that is documented at the export and left
for a separate change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`isRevoked` returned `false` for every failure. A network error, DNS failure, timeout, HTTP error, non-JSON body, or any body that did not match the expected shape read as "not revoked", so `verifyParsedCredential` accepted a revoked credential. Anyone able to disrupt the status list endpoint could use a revoked credential indefinitely. The fetched list was also trusted on shape alone: its proof was never verified, its issuer never checked, and it was never bound to the URL the credential named. `statusListIndex` went through `parseInt`, so a non-numeric value became `NaN` and read as an unset bit. An index past the end of the list read as unset too. The entry's `statusPurpose` was ignored, and an unrecognized `credentialStatus` was skipped silently. `isRevoked(credential, options)` now takes a resolver and throws `RevocationCheckError`, or `UnsupportedCredentialStatusError` for a status it does not implement. It verifies the list's proof, requires a trusted issuer and a matching `id`, rejects an expired, pre-dated or multi-bit list, checks the purpose, validates the index and its bounds, restricts the URL to http(s), refuses redirects, and bounds the body and the decode. `verifyParsedCredential` checks `trustedIssuers` before the revocation fetch, so an untrusted issuer cannot make the verifier dereference a URL it chose. Status lists carry a 24 hour expiry by default, because every earlier version of a list stays validly signed and can be replayed to clear a later revocation. Pass `null` to opt out, or bound age with `maxStatusListAgeMs`. Both errors carry one fixed message, with the URL and the response in `detail` and `cause`, because API handlers return the message to the caller. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The status endpoint returned the credential inside this API's `{ ok, data }`
envelope. A verifier dereferences that URL expecting the credential itself, so
the body never parsed as one and every revocation check against this issuer
failed open. Revocation did not work at all.
The endpoint now returns the signed credential directly, as the W3C Bitstring
Status List specification requires. It also parses `listId` before the value
reaches either the query or the credential `id`, so `/status/01` cannot sign
caller-supplied text into the id.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
WalkthroughThe VC package now binds JWT credential issuers to verified signers and performs authenticated, fail-closed revocation checks. Credential verification validates trusted issuers first. The issuer example returns signed status-list credentials directly. ChangesVC verification hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/vc/src/verification/is-revoked.test.ts (1)
481-513: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for a future-dated status list.
assertStatusListIsFreshinpackages/vc/src/verification/is-revoked.tsrejects anissuanceDatemore thanCLOCK_SKEW_MSahead of the local clock, and it accepts one inside that window. No test covers either branch. ThesignedStatusListhelper already acceptsissuedAt, so both cases are cheap to add.💚 Proposed tests for the clock-skew branch
it("throws when the status list is dated beyond the clock skew allowance", async () => { mockFetch.mockResolvedValueOnce( Response.json( await signedStatusList({ revokedIndex: 5, issuedAt: new Date(Date.now() + 10 * 60 * 1000), }), ), ) const error = await captureRevocationError( isRevoked(buildCredential(statusEntry()), { resolver }), ) expect(error.detail).toMatch(/in the future/) }) it("accepts a status list dated inside the clock skew allowance", async () => { mockFetch.mockResolvedValueOnce( Response.json( await signedStatusList({ revokedIndex: 5, issuedAt: new Date(Date.now() + 60 * 1000), }), ), ) await expect( isRevoked(buildCredential(statusEntry()), { resolver }), ).resolves.toBe(true) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vc/src/verification/is-revoked.test.ts` around lines 481 - 513, Add two tests alongside the existing freshness tests in is-revoked.test.ts: one using signedStatusList with issuedAt beyond CLOCK_SKEW_MS that captures the revocation error and asserts the detail mentions “in the future,” and another using a timestamp within the skew allowance that verifies isRevoked resolves successfully. Reuse the existing resolver, statusEntry, buildCredential, and mockFetch setup.packages/vc/src/verification/is-revoked.ts (2)
108-125: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoffConsider a host policy for status list URLs.
toHttpUrlaccepts any host onhttp:orhttps:.redirect: "error"stops a redirect to an internal host, but the first request still goes to whatever host the credential names. A credential from a trusted issuer can therefore point this process at169.254.169.254,localhost, or an RFC1918 address.The residual impact is bounded:
verifyParsedCredentialcheckstrustedIssuersbefore callingisRevoked, the response must carry a proof from a trusted issuer, and the error message is sanitized. The remaining signal is timing and reachability.Two options are worth considering:
- Add an optional
allowedStatusListHosts(or a blocked-range predicate) toRevocationCheckOptions, mirroring theallowedHttpHostspolicy thatgetDidResolveralready uses inpackages/did/src/did-resolvers/get-did-resolver.ts.- Deploy verifiers behind an egress proxy that denies link-local and private ranges.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vc/src/verification/is-revoked.ts` around lines 108 - 125, The status-list URL validation in toHttpUrl permits requests to arbitrary HTTP(S) hosts. Add an optional allowedStatusListHosts policy to RevocationCheckOptions, consistent with the existing allowedHttpHosts behavior in getDidResolver, and enforce it before returning the normalized URL; reject hosts outside the configured policy, including private, loopback, and link-local targets when applicable. Preserve current behavior when no policy is configured.
525-531: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that
bits.lengthis the status list bit count.
The range check usesstatusListIndexas a bit position beforestatusSizescaling; state this requirement nearif (index >= bits.length)so the length invariant is explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vc/src/verification/is-revoked.ts` around lines 525 - 531, Clarify the comment immediately above the index boundary check in the status-list verification flow that bits.length represents the total number of status-list bits, before any statusSize scaling is applied. Keep the existing index validation and undetermined error behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/vc/README.md`:
- Around line 118-121: Update the fetch-limits documentation in the README to
mention the RevocationCheckOptions.maxEncodedListBytes option, including its
default of 65536 bytes and that consumers can increase it when larger encoded
status lists are valid. Keep the existing statusListTimeoutMs and redirect
behavior documentation unchanged.
In `@packages/vc/src/verification/verify-proof.test.ts`:
- Around line 40-56: Update decodedIssuerId() to recognize a string-valued
issuer and return that DID before attempting to access issuer.id; preserve the
existing object-form handling and undefined result for invalid issuer values.
---
Nitpick comments:
In `@packages/vc/src/verification/is-revoked.test.ts`:
- Around line 481-513: Add two tests alongside the existing freshness tests in
is-revoked.test.ts: one using signedStatusList with issuedAt beyond
CLOCK_SKEW_MS that captures the revocation error and asserts the detail mentions
“in the future,” and another using a timestamp within the skew allowance that
verifies isRevoked resolves successfully. Reuse the existing resolver,
statusEntry, buildCredential, and mockFetch setup.
In `@packages/vc/src/verification/is-revoked.ts`:
- Around line 108-125: The status-list URL validation in toHttpUrl permits
requests to arbitrary HTTP(S) hosts. Add an optional allowedStatusListHosts
policy to RevocationCheckOptions, consistent with the existing allowedHttpHosts
behavior in getDidResolver, and enforce it before returning the normalized URL;
reject hosts outside the configured policy, including private, loopback, and
link-local targets when applicable. Preserve current behavior when no policy is
configured.
- Around line 525-531: Clarify the comment immediately above the index boundary
check in the status-list verification flow that bits.length represents the total
number of status-list bits, before any statusSize scaling is applied. Keep the
existing index validation and undetermined error behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8601c627-3fc2-46d3-998c-92e815295cb8
📒 Files selected for processing (23)
.changeset/vc-issuer-signer-binding.md.changeset/vc-revocation-fail-closed.mdexamples/issuer/README.mdexamples/issuer/src/routes/status.test.tsexamples/issuer/src/routes/status.tspackages/vc/AGENTS.mdpackages/vc/README.mdpackages/vc/src/index.tspackages/vc/src/revocation/status-list-credential.tspackages/vc/src/revocation/types.tspackages/vc/src/schemas/valibot.tspackages/vc/src/schemas/zod.tspackages/vc/src/verification/errors.tspackages/vc/src/verification/is-revoked.test.tspackages/vc/src/verification/is-revoked.tspackages/vc/src/verification/is-verifiable.tspackages/vc/src/verification/parse-jwt-credential.test.tspackages/vc/src/verification/parse-jwt-credential.tspackages/vc/src/verification/verify-parsed-credential.test.tspackages/vc/src/verification/verify-parsed-credential.tspackages/vc/src/verification/verify-proof.test.tspackages/vc/src/verification/verify-revoked-credential.test.tstools/api-utils/src/middleware/error-handler.ts
Comment on lines
+118
to
+121
| Two limits on the fetch are worth knowing. `statusListTimeoutMs` bounds the | ||
| status list request only, not the DID resolution that verifies its proof. And | ||
| the request does not follow redirects, so serve the credential at the URL the | ||
| `statusListCredential` names. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document maxEncodedListBytes.
RevocationCheckOptions exposes maxEncodedListBytes, and the default of 65536 rejects a large dense status list with encodedList over the 65536 byte limit. The README does not mention the option, so a consumer that hits the limit has no documented way to raise it.
📝 Proposed documentation addition
-Two limits on the fetch are worth knowing. `statusListTimeoutMs` bounds the
+Three limits are worth knowing. `statusListTimeoutMs` bounds the
status list request only, not the DID resolution that verifies its proof. And
the request does not follow redirects, so serve the credential at the URL the
-`statusListCredential` names.
+`statusListCredential` names. `maxEncodedListBytes` caps the `encodedList`
+string at 65536 by default, because the decode inflates it with no output
+limit; raise it to consume a large list with many bits set. A fixed 5 MB cap on
+the whole response body applies first.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Two limits on the fetch are worth knowing. `statusListTimeoutMs` bounds the | |
| status list request only, not the DID resolution that verifies its proof. And | |
| the request does not follow redirects, so serve the credential at the URL the | |
| `statusListCredential` names. | |
| Three limits are worth knowing. `statusListTimeoutMs` bounds the | |
| status list request only, not the DID resolution that verifies its proof. And | |
| the request does not follow redirects, so serve the credential at the URL the | |
| `statusListCredential` names. `maxEncodedListBytes` caps the `encodedList` | |
| string at 65536 by default, because the decode inflates it with no output | |
| limit; raise it to consume a large list with many bits set. A fixed 5 MB cap on | |
| the whole response body applies first. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vc/README.md` around lines 118 - 121, Update the fetch-limits
documentation in the README to mention the
RevocationCheckOptions.maxEncodedListBytes option, including its default of
65536 bytes and that consumers can increase it when larger encoded status lists
are valid. Keep the existing statusListTimeoutMs and redirect behavior
documentation unchanged.
Comment on lines
+40
to
+56
| /** | ||
| * Read `issuer.id` off a decoded-credential fixture, which may be any shape. | ||
| */ | ||
| function decodedIssuerId(credential: unknown): string | undefined { | ||
| if (typeof credential !== "object" || credential === null) { | ||
| return undefined | ||
| } | ||
|
|
||
| const { issuer } = credential as { issuer?: unknown } | ||
|
|
||
| if (typeof issuer !== "object" || issuer === null) { | ||
| return undefined | ||
| } | ||
|
|
||
| const { id } = issuer as { id?: unknown } | ||
|
|
||
| return typeof id === "string" ? id : undefined |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return the signer DID for string-form issuers.
decodedIssuerId() returns undefined when a fixture uses issuer: "did:example:issuer". The mock then does not report the signer DID that Line 32 says it must report. Handle the W3C string form before reading issuer.id.
Proposed fix
const { issuer } = credential as { issuer?: unknown }
+ if (typeof issuer === "string") {
+ return issuer
+ }
+
if (typeof issuer !== "object" || issuer === null) {
return undefined
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /** | |
| * Read `issuer.id` off a decoded-credential fixture, which may be any shape. | |
| */ | |
| function decodedIssuerId(credential: unknown): string | undefined { | |
| if (typeof credential !== "object" || credential === null) { | |
| return undefined | |
| } | |
| const { issuer } = credential as { issuer?: unknown } | |
| if (typeof issuer !== "object" || issuer === null) { | |
| return undefined | |
| } | |
| const { id } = issuer as { id?: unknown } | |
| return typeof id === "string" ? id : undefined | |
| /** | |
| * Read `issuer.id` off a decoded-credential fixture, which may be any shape. | |
| */ | |
| function decodedIssuerId(credential: unknown): string | undefined { | |
| if (typeof credential !== "object" || credential === null) { | |
| return undefined | |
| } | |
| const { issuer } = credential as { issuer?: unknown } | |
| if (typeof issuer === "string") { | |
| return issuer | |
| } | |
| if (typeof issuer !== "object" || issuer === null) { | |
| return undefined | |
| } | |
| const { id } = issuer as { id?: unknown } | |
| return typeof id === "string" ? id : undefined |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vc/src/verification/verify-proof.test.ts` around lines 40 - 56,
Update decodedIssuerId() to recognize a string-valued issuer and return that DID
before attempting to access issuer.id; preserve the existing object-form
handling and undefined result for invalid issuer values.
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.
Changes
isRevokedfails closed: it throws when revocation status cannot be established, instead of returningfalse.idbound to the dereferenced URL, expiry, purpose,statusSize, and index bounds.http(s)only, no redirects, 5s timeout, streamed body cap, and a cap on the encoded list before it is decoded.verifyParsedCredentialcheckstrustedIssuersbefore the revocation fetch, so an untrusted issuer cannot choose a URL for the verifier to request.issuer.idmust match the DID that signed the JWT.examples/issuerserves the status list credential unwrapped, and parseslistIdbefore it reaches the query or the credentialid.maxStatusListAgeMsbounds lists from issuers that publish no expiry.Problem
isRevokedtreated every failure as "not revoked":false— acceptedfalsefalse— trusted on shape alonestatusListIndex: "abc"false—parseIntgaveNaNfalseAnyone able to disrupt the status list endpoint could keep using a revoked credential.
The reference issuer made this worse. It served the credential inside this API's
{ ok, data }envelope, which is not a credential, so the body never parsed and every revocation check against it failed open. Revocation did not work at all for anyone following the example.While fixing the status list issuer check, a second and more serious problem surfaced. did-jwt-vc builds the credential issuer as
{ id: iss, ...payload.issuer }, so anissuerobject in the JWT payload replaces theidtaken fromiss. The signature bindsissonly, and nothing compared the two. A proof of concept confirmed it: a JWT signed bydid:web:attacker.example.comdecoded withissuer.idofdid:web:issuer.example.com. That defeatedtrustedIssuers,trustedReceiptIssuers, and the status list issuer binding.Both defects date to the initial commit (
2deaf04, 2025-05-19) and are present in every published version through 0.10.1.Solution
Every branch in
isRevokednow either returns a verified answer or throws.RevocationCheckErrorandUnsupportedCredentialStatusErroreach carry one fixed message, with the URL and the response indetailandcause, because API handlers return the message to the caller and it would otherwise report which hosts the verifier can reach.parseJwtCredentialrejects a credential whoseissuer.iddiffers from the verified signer.Status list replay is bounded by an expiry, because every earlier version of a list stays validly signed and can be served back to clear a later revocation.
Testing
pnpm run checkpasses: 29/29 tasks, 525 tests.New coverage includes each attack path as an explicit test: unreachable list, HTTP error, envelope-wrapped body, unsigned list, invalid proof, untrusted issuer, substituted list, purpose mismatch, multi-bit list,
NaNand out-of-range index, non-http scheme, oversized body and encoded list, replayed list, and the issuer spoof.verify-revoked-credential.test.tscovers the chain end to end, andexamples/issuer/src/routes/status.test.tsasserts a verifier can actually read a revocation from the served list.Notes
Deliberately out of scope, listed so they are not lost:
verifyPresentationis re-exported from did-jwt-vc unchanged;normalizeJwtPresentationPayloadtakesholderfromissonly when the payload has noholder, and it does not verify embedded credential proofs. Nothing in this repo calls it. Documented at the export; needs its own change.StatusList2021EntryandstatusPurpose: "suspension"now fail closed rather than being ignored. Supporting them needs type and dual-schema work.credentialStatus.typeis still rejected: widening the type breaks assignability to did-jwt-vc'sCredentialPayload(TS2345).maxStatusListAgeMsis off by default, so a third-party list with no expiry is not age-bounded unless the caller opts in. Changing that default is a policy call.Two related fixes are on
fix/verification-fail-closed-followupsand depend on this branch: theisExpiredfail-open for an unparseableexpirationDate, and arevocationoption passthrough forverifyPaymentReceipt.Marked
minor:isRevokedtakes a required second argument,isRevocableaccepts fewer shapes, and both throw where they returnedfalse.AI usage
Written with Claude Code (Claude Opus 5). It investigated the report, wrote the fixes and tests, and ran an eight-round multi-agent review loop (Claude, Codex, and two opencode models) that found the issuer-spoofing bug and a regression this change had introduced. All findings were verified against the code, and several were rejected as incorrect. Reviewed by the author before opening.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation