fix(vc): fail closed when credential revocation cannot be verified

.changeset/vc-issuer-signer-binding.md

@@ -0,0 +1,29 @@
+---
+"@agentcommercekit/vc": patch
+---
+
+Security: bind a credential's issuer to the DID that signed it (CWE-290).
+
+`parseJwtCredential` returned the credential `normalizeCredential` builds from
+the JWT. That function derives the issuer as `{ id: iss, ...payload.issuer }`,
+so an `issuer` object in the payload replaces the `id` taken from `iss`. The
+signature binds `iss` only, and nothing compared the two.
+
+Anyone could therefore sign a credential with their own key, put
+`issuer: { id: "<any DID>" }` in the payload, and produce a credential that
+verifies and reports that DID as its issuer. Every issuer check downstream
+accepted it: the `trustedIssuers` list in `verifyParsedCredential`,
+`trustedReceiptIssuers` in `verifyPaymentReceipt`, and the status list issuer
+check in `isRevoked`. With `jti` set to the status list URL, the same forgery
+also passed the URL binding on a status list credential and cleared a
+revocation.
+
+`parseJwtCredential` now rejects a credential whose `issuer.id` differs from the
+verified signer, with `InvalidCredentialError`.
+
+This covers the credential path only. `verifyPresentation` is re-exported from
+did-jwt-vc unchanged, and `normalizeJwtPresentationPayload` sets `holder` from
+`iss` only when the payload carries no `holder`, so a presentation can still
+name a holder that did not sign it. That function also does not verify the
+proofs of the credentials it embeds. Nothing in this repository calls it. Treat
+its result as unverified until a bound wrapper replaces it.

.changeset/vc-revocation-fail-closed.md

@@ -0,0 +1,60 @@
+---
+"@agentcommercekit/vc": minor
+"@agentcommercekit/ack-pay": minor
+---
+
+Security: fail closed when a credential's revocation status cannot be verified
+(CWE-299).
+
+`isRevoked` treated every failure as "not revoked". A network error, DNS
+failure, timeout, HTTP 4xx/5xx, non-JSON body, or any body that did not match
+the expected shape resolved to `false`, so `verifyParsedCredential` accepted a
+revoked credential. Anyone able to disrupt reachability of the status list — or
+simply presenting a credential while the status endpoint was down — could use a
+revoked credential indefinitely.
+
+The fetched status 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 pointed at. A tampered or substituted list therefore cleared
+revocation for every credential it covered. `statusListIndex` went through
+`parseInt`, so a non-numeric value produced `NaN` and read as an unset bit, and
+an index past the end of the list also read as unset. The entry's
+`statusPurpose` was ignored, and a `credentialStatus` of an unrecognized type
+was silently skipped.
+
+`isRevoked(credential, options)` now takes a resolver and throws
+`RevocationCheckError` when the status cannot be established, or
+`UnsupportedCredentialStatusError` for a status it does not implement. It
+verifies the status list credential's proof, requires it to be issued by the
+credential's issuer (override with `revocation.trustedStatusListIssuers`),
+requires its `id` to match the dereferenced URL, rejects an expired list,
+requires the entry and list `statusPurpose` to match, validates
+`statusListIndex` and its bounds, restricts the status list URL to `http(s)`,
+and applies a 5s timeout (override with `revocation.statusListTimeoutMs`).
+
+`verifyParsedCredential` accepts a `revocation` option and now rejects
+credentials whose revocation status is undeterminable, so `verifyPaymentReceipt`
+rejects them too. It also checks `trustedIssuers` before the revocation check,
+so an untrusted issuer can no longer make the verifier dereference a URL of the
+issuer's choosing.
+
+A status list credential stays validly signed after the issuer publishes a newer
+version, so anyone holding an older copy could serve it back and clear a later
+revocation. `createStatusListCredential` now sets an `expirationDate`, 24 hours
+out by default, and issuers must republish the list before it lapses. For
+issuers that publish no expiry, `revocation.maxStatusListAgeMs` bounds how old
+an accepted list may be.
+
+`RevocationCheckError` carries one fixed message and puts the URL and the
+response in `detail` and `cause`. API error handlers return the message of a
+`CredentialVerificationError` to the caller, so the detail must not travel with
+it.
+
+This release is `minor`, not `patch`: `isRevoked` takes a required second
+argument, `isRevocable` accepts fewer shapes, and both throw where they returned
+`false`.
+
+The `examples/issuer` status endpoint served the credential wrapped in this
+API's `{ ok, data }` envelope, which is not a credential. Every revocation check
+against it failed open. It now serves the signed credential directly, as the
+W3C Bitstring Status List spec requires.

examples/issuer/README.md

@@ -269,14 +269,28 @@ curl --request DELETE \
 
 #### GET /status/:listId
 
-Retrieve a Bitstring Status List credential for checking revocation status
+Retrieve a Bitstring Status List credential for checking revocation status.
+
+Unlike the other endpoints, this one returns the signed credential directly
+rather than in the `{ ok, data }` envelope. Verifiers dereference this URL as
+the credential's `statusListCredential` and expect the credential itself; a
+wrapped body cannot be verified, so revocation checks would fail.
 
 **Response Body**
 
 ```json
 {
-  "ok": true,
-  "data": "jwt-string"
+  "@context": ["https://www.w3.org/2018/credentials/v1"],
+  "id": "http://localhost:3456/status/1",
+  "type": ["VerifiableCredential", "BitstringStatusListCredential"],
+  "issuer": { "id": "did:web:..." },
+  "credentialSubject": {
+    "id": "http://localhost:3456/status/1#list",
+    "type": "BitstringStatusList",
+    "statusPurpose": "revocation",
+    "encodedList": "..."
+  },
+  "proof": { "type": "JwtProof2020", "jwt": "jwt-string" }
 }
 ```
 

examples/issuer/src/routes/status.test.ts

@@ -0,0 +1,200 @@
+import {
+  bytesToHexString,
+  DidResolver,
+  getDidResolver,
+  isRevoked,
+  type Revocable,
+  type W3CCredential,
+} from "agentcommercekit"
+import {
+  afterEach,
+  beforeAll,
+  beforeEach,
+  describe,
+  expect,
+  it,
+  vi,
+} from "vitest"
+
+import type { DatabaseClient } from "@/db/get-db"
+import { getStatusList } from "@/db/queries/status-lists"
+import type { DatabaseStatusList } from "@/db/schema"
+import { STATUS_LIST_MAX_SIZE } from "@/db/schema"
+import {
+  createDidWebWithSigner,
+  type DidWithSigner,
+} from "@/test-helpers/did-web-with-signer"
+
+import app from ".."
+
+vi.mock("agentcommercekit", async () => {
+  const actual = await vi.importActual("agentcommercekit")
+  return {
+    ...actual,
+    getDidResolver: vi.fn<() => DidResolver>(),
+  }
+})
+
+vi.mock("@/db/queries/status-lists", async () => {
+  const actual = await vi.importActual("@/db/queries/status-lists")
+  return {
+    ...actual,
+    getStatusList:
+      vi.fn<
+        (
+          db: DatabaseClient,
+          listId: number,
+        ) => Promise<DatabaseStatusList | undefined>
+      >(),
+  }
+})
+
+const baseUrl = "https://issuer.example.com"
+const statusListUrl = `${baseUrl}/status/1`
+const revokedIndex = 7
+
+function statusListData(revoked: number[]) {
+  return Array.from({ length: STATUS_LIST_MAX_SIZE }, (_, index) =>
+    revoked.includes(index) ? "1" : "0",
+  ).join("")
+}
+
+function mockStatusList(revoked: number[]) {
+  vi.mocked(getStatusList).mockResolvedValue({
+    id: 1,
+    credentialType: "ControllerCredential",
+    data: statusListData(revoked),
+    createdAt: new Date(),
+    updatedAt: new Date(),
+  })
+}
+
+function revocableCredential(issuerDid: string): Revocable<W3CCredential> {
+  return {
+    "@context": ["https://www.w3.org/2018/credentials/v1"],
+    type: ["VerifiableCredential"],
+    issuer: { id: issuerDid },
+    issuanceDate: "2024-01-01T00:00:00.000Z",
+    credentialSubject: { id: "did:web:subject.example.com" },
+    credentialStatus: {
+      id: `${statusListUrl}#${revokedIndex}`,
+      type: "BitstringStatusListEntry",
+      statusPurpose: "revocation",
+      statusListIndex: String(revokedIndex),
+      statusListCredential: statusListUrl,
+    },
+  }
+}
+
+describe("GET /status/:listId", () => {
+  let issuer: DidWithSigner
+
+  beforeAll(async () => {
+    issuer = await createDidWebWithSigner(baseUrl)
+
+    process.env.ISSUER_PRIVATE_KEY = bytesToHexString(issuer.keypair.privateKey)
+    process.env.BASE_URL = baseUrl
+  })
+
+  beforeEach(() => {
+    const resolver = new DidResolver()
+    resolver.addToCache(issuer.did, issuer.didDocument)
+    vi.mocked(getDidResolver).mockReturnValue(resolver)
+  })
+
+  // Unstub here, not at the end of a test body: a failed assertion would
+  // otherwise leave the `fetch` stub in place for the rest of the file.
+  afterEach(() => {
+    vi.unstubAllGlobals()
+    vi.clearAllMocks()
+  })
+
+  it("returns the status list credential unwrapped, so verifiers can read it", async () => {
+    mockStatusList([])
+
+    const res = await app.request("/status/1")
+    const body: unknown = await res.json()
+
+    expect(res.status).toBe(200)
+    // A verifier dereferences this URL expecting the credential itself. An
+    // `{ ok, data }` envelope silently defeats every revocation check.
+    expect(body).toMatchObject({
+      id: statusListUrl,
+      proof: { type: "JwtProof2020" },
+      credentialSubject: { statusPurpose: "revocation" },
+    })
+    expect(body).not.toHaveProperty("data")
+  })
+
+  it("serves a status list a verifier reads a revoked credential from", async () => {
+    mockStatusList([revokedIndex])
+
+    const res = await app.request("/status/1")
+    const statusListCredential = await res.json()
+
+    const resolver = new DidResolver()
+    resolver.addToCache(issuer.did, issuer.didDocument)
+    vi.stubGlobal(
+      "fetch",
+      vi.fn(() => Promise.resolve(Response.json(statusListCredential))),
+    )
+
+    await expect(
+      isRevoked(revocableCredential(issuer.did), { resolver }),
+    ).resolves.toBe(true)
+  })
+
+  it("serves a status list a verifier reads an unrevoked credential from", async () => {
+    mockStatusList([])
+
+    const res = await app.request("/status/1")
+    const statusListCredential = await res.json()
+
+    const resolver = new DidResolver()
+    resolver.addToCache(issuer.did, issuer.didDocument)
+    vi.stubGlobal(
+      "fetch",
+      vi.fn(() => Promise.resolve(Response.json(statusListCredential))),
+    )
+
+    await expect(
+      isRevoked(revocableCredential(issuer.did), { resolver }),
+    ).resolves.toBe(false)
+  })
+
+  it("returns a 404 for an unknown status list", async () => {
+    vi.mocked(getStatusList).mockResolvedValue(undefined)
+
+    const res = await app.request("/status/999")
+
+    expect(res.status).toBe(404)
+  })
+
+  it("serves status list 0, which holds the first issued credentials", async () => {
+    mockStatusList([])
+
+    const res = await app.request("/status/0")
+
+    expect(res.status).toBe(200)
+    expect(getStatusList).toHaveBeenCalledWith(expect.anything(), 0)
+  })
+
+  it("returns a 404 for a non-numeric status list id", async () => {
+    mockStatusList([])
+
+    const res = await app.request("/status/1abc")
+
+    expect(res.status).toBe(404)
+    expect(getStatusList).not.toHaveBeenCalled()
+  })
+
+  it("signs the parsed id into the credential, not the raw path text", async () => {
+    mockStatusList([])
+
+    const res = await app.request("/status/0001")
+    const body: unknown = await res.json()
+
+    expect(res.status).toBe(200)
+    expect(body).toMatchObject({ id: statusListUrl })
+  })
+})