fix(vc): bind credential verification to the signed proof (#105, #108)

.changeset/bind-parsed-credential-to-proof.md

@@ -0,0 +1,5 @@
+---
+"@agentcommercekit/vc": patch
+---
+
+Bind credential verification to the signed proof. `verifyProof()` now returns the credential decoded from `proof.jwt`, and `verifyParsedCredential()` runs every downstream check (expiry, revocation, trusted issuer, and claim verifiers) against that verified credential rather than the caller-supplied object. It also now returns the verified credential, so consumers can read signed fields from the return value instead of the object they passed in. On the parsed-credential input path a caller could previously attach a valid `proof.jwt` while mutating the outer `credentialSubject`, `issuer`, etc.; those fields were trusted directly. Now they are ignored in favor of the signed payload. Fixes #105 and #108.

packages/vc/src/verification/verify-parsed-credential.test.ts

@@ -1,12 +1,10 @@
-import {
-  createDidDocumentFromKeypair,
-  createDidWebUri,
-  getDidResolver,
-} from "@agentcommercekit/did"
+import { createDidKeyUri, getDidResolver } from "@agentcommercekit/did"
+import { createJwtSigner } from "@agentcommercekit/jwt"
 import { generateKeypair } from "@agentcommercekit/keys"
 import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"
 
 import { createCredential } from "../create-credential"
+import { signCredential } from "../signing/sign-credential"
 import type { Verifiable, W3CCredential } from "../types"
 import {
   CredentialExpiredError,
@@ -17,9 +15,11 @@ import {
 } from "./errors"
 import { isExpired } from "./is-expired"
 import { isRevoked } from "./is-revoked"
+import { parseJwtCredential } from "./parse-jwt-credential"
 import { verifyParsedCredential } from "./verify-parsed-credential"
-import { verifyProof } from "./verify-proof"
 
+// Expiry and revocation are checked elsewhere; mock them so each test can
+// drive those branches independently of the (real) signed credential.
 vi.mock("./is-expired", () => ({
   isExpired: vi.fn(),
 }))
@@ -28,26 +28,14 @@ vi.mock("./is-revoked", () => ({
   isRevoked: vi.fn(),
 }))
 
-vi.mock("./verify-proof", () => ({
-  verifyProof: vi.fn(),
-}))
-
 async function setup() {
   const resolver = getDidResolver()
-  const subjectDid = createDidWebUri("https://subject.example.com")
 
   const issuerKeypair = await generateKeypair("secp256k1")
-  const issuerDid = createDidWebUri("https://issuer.example.com")
-  resolver.addToCache(
-    issuerDid,
-    createDidDocumentFromKeypair({
-      did: issuerDid,
-      keypair: issuerKeypair,
-    }),
-  )
-
-  // Generate an unsigned attestation
-  const credential = createCredential({
+  const issuerDid = createDidKeyUri(issuerKeypair)
+  const subjectDid = createDidKeyUri(await generateKeypair("secp256k1"))
+
+  const unsigned = createCredential({
     id: "test-credential",
     type: "TestCredential",
     subject: subjectDid,
@@ -57,18 +45,13 @@ async function setup() {
     },
   })
 
-  credential.issuer = {
-    id: issuerDid,
-  }
+  const jwt = await signCredential(unsigned, {
+    did: issuerDid,
+    signer: createJwtSigner(issuerKeypair),
+  })
 
-  const vc = {
-    ...credential,
-    // just dummy fields, we mock the actual proof verification
-    proof: {
-      type: "JwtProof2020",
-      jwt: "test.jwt.token",
-    },
-  } as unknown as Verifiable<W3CCredential>
+  // A real parsed credential, with a signed `proof.jwt`.
+  const vc = await parseJwtCredential(jwt, resolver)
 
   return { vc, issuerDid, resolver }
 }
@@ -77,23 +60,36 @@ describe("verifyParsedCredential", () => {
   beforeEach(() => {
     vi.mocked(isExpired).mockReturnValue(false)
     vi.mocked(isRevoked).mockResolvedValue(false)
-    vi.mocked(verifyProof).mockResolvedValue(undefined)
   })
 
   afterEach(() => {
     vi.clearAllMocks()
   })
 
   it("throws when no proof is present", async () => {
-    const { vc: baseVc, issuerDid, resolver } = await setup()
+    const { vc, issuerDid, resolver } = await setup()
 
-    const vc = {
-      ...baseVc,
-      proof: undefined,
-    }
+    await expect(
+      verifyParsedCredential(
+        { ...vc, proof: undefined } as unknown as W3CCredential,
+        {
+          trustedIssuers: [issuerDid],
+          resolver,
+        },
+      ),
+    ).rejects.toThrow(InvalidProofError)
+  })
+
+  it("throws for an invalid proof", async () => {
+    const { vc, issuerDid, resolver } = await setup()
+
+    const tampered = {
+      ...vc,
+      proof: { type: "JwtProof2020", jwt: "invalid.jwt.token" },
+    } as unknown as Verifiable<W3CCredential>
 
     await expect(
-      verifyParsedCredential(vc, {
+      verifyParsedCredential(tampered, {
         trustedIssuers: [issuerDid],
         resolver,
       }),
@@ -137,19 +133,6 @@ describe("verifyParsedCredential", () => {
     ).rejects.toThrow(UntrustedIssuerError)
   })
 
-  it("throws for an invalid proof", async () => {
-    const { vc, issuerDid, resolver } = await setup()
-
-    vi.mocked(verifyProof).mockRejectedValueOnce(new InvalidProofError())
-
-    await expect(
-      verifyParsedCredential(vc, {
-        trustedIssuers: [issuerDid],
-        resolver,
-      }),
-    ).rejects.toThrow(InvalidProofError)
-  })
-
   it("throws if any claim verifier fails", async () => {
     const { vc, issuerDid, resolver } = await setup()
 
@@ -232,4 +215,108 @@ describe("verifyParsedCredential", () => {
       }),
     ).resolves.not.toThrow()
   })
+
+  // Regression for #105 / #108: on the parsed-credential input path, the
+  // top-level fields are caller-supplied and not bound to the signed proof.
+  // Verification must read the signed payload (`proof.jwt`), so mutating the
+  // outer object cannot bypass the issuer or claim-verifier checks.
+  describe("binds checks to the signed proof, not caller-supplied fields", () => {
+    it("rejects a spoofed issuer even when the outer object names a trusted DID", async () => {
+      const { vc, resolver } = await setup()
+
+      const spoofedTrustedDid = "did:web:trusted.example.com"
+      const spoofed = {
+        ...vc,
+        issuer: { id: spoofedTrustedDid },
+      } as Verifiable<W3CCredential>
+
+      // The outer issuer claims the trusted DID, but the signed payload was
+      // issued by the real (untrusted-here) issuer, so this must be rejected.
+      await expect(
+        verifyParsedCredential(spoofed, {
+          trustedIssuers: [spoofedTrustedDid],
+          resolver,
+        }),
+      ).rejects.toThrow(UntrustedIssuerError)
+    })
+
+    it("runs claim verifiers against the signed subject, not a mutated outer subject", async () => {
+      const { vc, issuerDid, resolver } = await setup()
+
+      const spoofed = {
+        ...vc,
+        credentialSubject: { ...vc.credentialSubject, test: "spoofed" },
+      } as Verifiable<W3CCredential>
+
+      // The verifier accepts only the signed value ("test"). If verification
+      // read the mutated outer subject ("spoofed") this would reject; binding
+      // to the signed payload means it sees "test" and passes.
+      await expect(
+        verifyParsedCredential(spoofed, {
+          trustedIssuers: [issuerDid],
+          resolver,
+          verifiers: [
+            {
+              accepts: () => true,
+              verify: (subject) =>
+                (subject as { test?: string }).test === "test"
+                  ? Promise.resolve()
+                  : Promise.reject(
+                      new Error("subject was not the signed value"),
+                    ),
+            },
+          ],
+        }),
+      ).resolves.not.toThrow()
+    })
+
+    it("returns the credential decoded from the signed proof, not the caller-supplied object", async () => {
+      const { vc, issuerDid, resolver } = await setup()
+
+      const spoofed = {
+        ...vc,
+        issuer: { id: "did:web:attacker.example.com" },
+        credentialSubject: { ...vc.credentialSubject, test: "spoofed" },
+      } as Verifiable<W3CCredential>
+
+      const verified = await verifyParsedCredential(spoofed, { resolver })
+
+      // The returned credential is the signed one, regardless of the mutated
+      // outer fields the caller supplied.
+      expect(verified.issuer.id).toBe(issuerDid)
+      expect((verified.credentialSubject as { test?: string }).test).toBe(
+        "test",
+      )
+    })
+
+    it("selects claim verifiers by the signed type, not a mutated outer type", async () => {
+      const { vc, issuerDid, resolver } = await setup()
+
+      const spoofed = {
+        ...vc,
+        type: ["VerifiableCredential", "SpoofedType"],
+      } as Verifiable<W3CCredential>
+
+      let receivedTypes: string[] | undefined
+
+      await expect(
+        verifyParsedCredential(spoofed, {
+          trustedIssuers: [issuerDid],
+          resolver,
+          verifiers: [
+            {
+              accepts: (type) => {
+                receivedTypes = type
+                return type.includes("TestCredential")
+              },
+              verify: () => Promise.resolve(),
+            },
+          ],
+        }),
+      ).resolves.toBeDefined()
+
+      expect(receivedTypes).toContain("TestCredential")
+      expect(receivedTypes).not.toContain("SpoofedType")
+    })
+  })
 })

packages/vc/src/verification/verify-parsed-credential.ts

@@ -48,51 +48,68 @@ function isVerifiable(
  *
  * @param credential - The credential to verify.
  * @param options - The {@link VerifyCredentialOptions} to use
+ * @returns The verified credential decoded from the signed proof. Callers
+ *   should use this returned value rather than the object they passed in, whose
+ *   top-level fields are not bound to the signature.
  * @throws on error
  */
 export async function verifyParsedCredential(
   credential: W3CCredential,
   options: VerifyCredentialOptions,
-): Promise<void> {
+): Promise<Verifiable<W3CCredential>> {
   if (!isVerifiable(credential)) {
     throw new InvalidProofError("Credential does not contain a proof")
   }
 
-  await verifyProof(credential.proof, options.resolver)
+  // verifyProof returns the credential decoded from the signed proof. The
+  // top-level fields of a caller-supplied parsed credential are NOT bound to
+  // that signed payload, so every check below (expiry, revocation, trusted
+  // issuer, claim verifiers) runs against the verified credential rather than
+  // the caller-supplied object, which could otherwise be mutated to diverge
+  // from what was actually signed. (#105, #108)
+  const verifiedCredential = await verifyProof(
+    credential.proof,
+    options.resolver,
+  )
 
-  if (isExpired(credential)) {
+  if (isExpired(verifiedCredential)) {
     throw new CredentialExpiredError()
   }
 
-  if (await isRevoked(credential)) {
+  if (await isRevoked(verifiedCredential)) {
     throw new CredentialRevokedError()
   }
 
   // If trustedIssuers is defined, we require the issuer is in the array (even
   // if the array is empty). If it is not defined, we skip the check.
   if (
     Array.isArray(options.trustedIssuers) &&
-    !options.trustedIssuers.includes(credential.issuer.id)
+    !options.trustedIssuers.includes(verifiedCredential.issuer.id)
   ) {
     throw new UntrustedIssuerError(
-      `Issuer is not trusted '${credential.issuer.id}'`,
+      `Issuer is not trusted '${verifiedCredential.issuer.id}'`,
     )
   }
 
   // If verifiers are provided, we verify the credential against them.
   if (options.verifiers?.length) {
     const verifiers = options.verifiers.filter((v) =>
-      v.accepts(credential.type),
+      v.accepts(verifiedCredential.type),
     )
 
     if (!verifiers.length) {
       throw new UnsupportedCredentialTypeError(
-        `Unsupported credential type: ${credential.type.join(", ")}`,
+        `Unsupported credential type: ${verifiedCredential.type.join(", ")}`,
       )
     }
 
     for (const verifier of verifiers) {
-      await verifier.verify(credential.credentialSubject, options.resolver)
+      await verifier.verify(
+        verifiedCredential.credentialSubject,
+        options.resolver,
+      )
     }
   }
+
+  return verifiedCredential
 }

packages/vc/src/verification/verify-proof.test.ts

@@ -56,13 +56,22 @@ describe("verifyProof", () => {
     ).rejects.toThrow(InvalidProofError)
   })
 
-  it("successfully verifies a valid JwtProof2020", async () => {
+  it("successfully verifies a valid JwtProof2020 and returns the decoded credential", async () => {
     const validProof = {
       type: "JwtProof2020",
       jwt: "valid.jwt.token",
     }
 
-    vi.mocked(verifyCredential).mockResolvedValueOnce({} as VerifiedCredential)
-    await expect(verifyProof(validProof, mockResolver)).resolves.not.toThrow()
+    const decoded = {
+      issuer: { id: "did:example:signed-issuer" },
+    }
+
+    vi.mocked(verifyCredential).mockResolvedValueOnce({
+      verifiableCredential: decoded,
+    } as unknown as VerifiedCredential)
+
+    // The returned credential must come from the decoded proof payload, not the
+    // caller-supplied proof object.
+    await expect(verifyProof(validProof, mockResolver)).resolves.toBe(decoded)
   })
 })