diff --git a/.changeset/bind-parsed-credential-to-proof.md b/.changeset/bind-parsed-credential-to-proof.md new file mode 100644 index 0000000..c913b8b --- /dev/null +++ b/.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. diff --git a/packages/vc/src/verification/verify-parsed-credential.test.ts b/packages/vc/src/verification/verify-parsed-credential.test.ts index b189d0f..ecf030d 100644 --- a/packages/vc/src/verification/verify-parsed-credential.test.ts +++ b/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 + // A real parsed credential, with a signed `proof.jwt`. + const vc = await parseJwtCredential(jwt, resolver) return { vc, issuerDid, resolver } } @@ -77,7 +60,6 @@ describe("verifyParsedCredential", () => { beforeEach(() => { vi.mocked(isExpired).mockReturnValue(false) vi.mocked(isRevoked).mockResolvedValue(false) - vi.mocked(verifyProof).mockResolvedValue(undefined) }) afterEach(() => { @@ -85,15 +67,29 @@ describe("verifyParsedCredential", () => { }) 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 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 + + // 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 + + // 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 + + 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 + + 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") + }) + }) }) diff --git a/packages/vc/src/verification/verify-parsed-credential.ts b/packages/vc/src/verification/verify-parsed-credential.ts index 4ca8bd9..4314433 100644 --- a/packages/vc/src/verification/verify-parsed-credential.ts +++ b/packages/vc/src/verification/verify-parsed-credential.ts @@ -48,23 +48,35 @@ 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 { +): Promise> { 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() } @@ -72,27 +84,32 @@ export async function verifyParsedCredential( // 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 } diff --git a/packages/vc/src/verification/verify-proof.test.ts b/packages/vc/src/verification/verify-proof.test.ts index 4615ba6..d9b8dc2 100644 --- a/packages/vc/src/verification/verify-proof.test.ts +++ b/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) }) }) diff --git a/packages/vc/src/verification/verify-proof.ts b/packages/vc/src/verification/verify-proof.ts index 5f96094..3ad916c 100644 --- a/packages/vc/src/verification/verify-proof.ts +++ b/packages/vc/src/verification/verify-proof.ts @@ -26,16 +26,24 @@ export function isJwtProof(proof: unknown): proof is JwtProof { ) } +/** + * Verify a JWT proof and return the credential decoded from the signed payload. + * + * The returned credential is reconstructed from `proof.jwt`, so its fields are + * exactly what was signed, rather than whatever a caller may have placed on the + * surrounding object. + */ async function verifyJwtProof( proof: Verifiable["proof"], resolver: Resolvable, -): Promise { +): Promise> { if (!isJwtProof(proof)) { throw new InvalidProofError() } try { - await verifyCredential(proof.jwt, resolver) + const { verifiableCredential } = await verifyCredential(proof.jwt, resolver) + return verifiableCredential as Verifiable } catch (_error) { throw new InvalidProofError() } @@ -46,11 +54,14 @@ async function verifyJwtProof( * * @param proof - The credential proof to verify * @param resolver - The resolver to use for did resolution + * @returns The credential decoded from the signed proof. For JWT proofs this is + * the payload recovered from `proof.jwt`, so callers can rely on it instead of + * on caller-supplied top-level fields that are not bound to the signature. */ export async function verifyProof( proof: Verifiable["proof"], resolver: Resolvable, -): Promise { +): Promise> { switch (proof.type) { case "JwtProof2020": return verifyJwtProof(proof, resolver)