diff --git a/.changeset/vc-ackpay-proof-binding.md b/.changeset/vc-ackpay-proof-binding.md new file mode 100644 index 0000000..f02104a --- /dev/null +++ b/.changeset/vc-ackpay-proof-binding.md @@ -0,0 +1,18 @@ +--- +"@agentcommercekit/vc": patch +"@agentcommercekit/ack-pay": patch +--- + +Security: bind credential trust decisions to the verified proof. + +`verifyParsedCredential` previously verified the JWT in `proof.jwt` but then made +every trust decision (expiry, revocation, trusted-issuer, claim verifiers) from +the caller-supplied credential object, which is not bound to the proof. An +attacker could wrap a valid `proof.jwt` in an object with tampered `issuer` / +`credentialSubject` fields and pass verification. `verifyPaymentReceipt` had the +same gap on its object-input path, returning the tampered `paymentRequestToken` +and `receipt`. + +`verifyProof` and `verifyParsedCredential` now return the credential decoded from +the verified proof, and all trust decisions and returned values flow from that +credential. The JWT-string input paths were already safe. diff --git a/packages/ack-pay/src/verify-payment-receipt.test.ts b/packages/ack-pay/src/verify-payment-receipt.test.ts index 100e45d..249f9b9 100644 --- a/packages/ack-pay/src/verify-payment-receipt.test.ts +++ b/packages/ack-pay/src/verify-payment-receipt.test.ts @@ -102,6 +102,31 @@ describe("verifyPaymentReceipt()", () => { expect(result.paymentRequest).toBeDefined() }) + it("returns verified values when outer object fields are tampered", async () => { + // Reuse the valid proof from a legitimately signed receipt, but tamper the + // outer object's credentialSubject. The forgery must be ignored: all + // returned values must come from the credential decoded from the proof. + const tampered = { + ...signedReceipt, + credentialSubject: { + ...signedReceipt.credentialSubject, + paymentRequestToken: "forged.jwt.token", + }, + } as Verifiable + + const result = await verifyPaymentReceipt(tampered, { + resolver, + trustedReceiptIssuers: [receiptIssuerDid], + verifyPaymentRequestTokenJwt: false, + }) + + expect(result.paymentRequestToken).toBe(paymentRequestToken) + expect( + (result.receipt.credentialSubject as { paymentRequestToken: string }) + .paymentRequestToken, + ).toBe(paymentRequestToken) + }) + it("preserves receipt metadata through JWT verification", async () => { const evidenceMetadata = { policyRef: "policy://merchant-spend-v3", diff --git a/packages/ack-pay/src/verify-payment-receipt.ts b/packages/ack-pay/src/verify-payment-receipt.ts index 3359ed8..9351fd1 100644 --- a/packages/ack-pay/src/verify-payment-receipt.ts +++ b/packages/ack-pay/src/verify-payment-receipt.ts @@ -73,25 +73,37 @@ export async function verifyPaymentReceipt( throw new InvalidCredentialError("Receipt is not a JWT or Credential") } + // Cheap structural fast-reject only — NOT a trust boundary. The authoritative + // receipt check runs on the proof-verified credential below. if (!isPaymentReceiptCredential(parsedCredential)) { throw new InvalidCredentialError( "Credential is not a PaymentReceiptCredential", ) } - await verifyParsedCredential(parsedCredential, { + // `verifyParsedCredential` returns the credential decoded from the verified + // proof. All reads below use that verified credential, never the + // caller-supplied `parsedCredential` object, whose fields are not bound to + // the proof and may have been tampered with on the object-input path. + const verifiedReceipt = await verifyParsedCredential(parsedCredential, { resolver, trustedIssuers: trustedReceiptIssuers, verifiers: [getReceiptClaimVerifier()], }) + if (!isPaymentReceiptCredential(verifiedReceipt)) { + throw new InvalidCredentialError( + "Verified credential is not a PaymentReceiptCredential", + ) + } + // Verify the paymentRequestToken is a valid JWT const paymentRequestToken = - parsedCredential.credentialSubject.paymentRequestToken + verifiedReceipt.credentialSubject.paymentRequestToken if (!verifyPaymentRequestTokenJwt) { return { - receipt: parsedCredential, + receipt: verifiedReceipt, paymentRequestToken, paymentRequest: null, } @@ -117,7 +129,7 @@ export async function verifyPaymentReceipt( ) return { - receipt: parsedCredential, + receipt: verifiedReceipt, paymentRequestToken, paymentRequest, } diff --git a/packages/vc/src/verification/verify-parsed-credential.test.ts b/packages/vc/src/verification/verify-parsed-credential.test.ts index b189d0f..0f5277c 100644 --- a/packages/vc/src/verification/verify-parsed-credential.test.ts +++ b/packages/vc/src/verification/verify-parsed-credential.test.ts @@ -70,6 +70,11 @@ async function setup() { }, } as unknown as Verifiable + // `verifyProof` returns the credential decoded from the verified proof. In + // the happy path that matches the object under test, so default the mock to + // return `vc` itself. + vi.mocked(verifyProof).mockResolvedValue(vc) + return { vc, issuerDid, resolver } } @@ -77,7 +82,6 @@ describe("verifyParsedCredential", () => { beforeEach(() => { vi.mocked(isExpired).mockReturnValue(false) vi.mocked(isRevoked).mockResolvedValue(false) - vi.mocked(verifyProof).mockResolvedValue(undefined) }) afterEach(() => { @@ -217,6 +221,50 @@ describe("verifyParsedCredential", () => { ).resolves.not.toThrow() }) + it("returns the proof-decoded credential and ignores tampered outer fields", async () => { + const { vc, issuerDid, resolver } = await setup() + + // The authoritative credential decoded from the verified proof + const verifiedSubject = { id: "did:example:subject", role: "user" } + const verifiedCredential = { + ...vc, + issuer: { id: issuerDid }, + credentialSubject: verifiedSubject, + } as unknown as Verifiable + vi.mocked(verifyProof).mockResolvedValue(verifiedCredential) + + // The caller-supplied object carries tampered fields (untrusted issuer, + // escalated subject) while reusing the same valid proof. + const tampered = { + ...vc, + issuer: { id: "did:example:attacker" }, + credentialSubject: { id: "did:example:subject", role: "admin" }, + } as unknown as Verifiable + + const received: unknown[] = [] + + const result = await verifyParsedCredential(tampered, { + // Only the real issuer is trusted; the tampered "attacker" issuer is not + trustedIssuers: [issuerDid], + resolver, + verifiers: [ + { + accepts: () => true, + verify: (subject) => { + received.push(subject) + return Promise.resolve() + }, + }, + ], + }) + + // Returns the verified credential (the contract callers like + // verifyPaymentReceipt rely on), not the caller-supplied object + expect(result).toBe(verifiedCredential) + // Trust decisions used the verified payload, not the tampered outer object + expect(received).toEqual([verifiedSubject]) + }) + it("verifies a valid credential without a list of trusted issuers", async () => { const { vc, resolver } = await setup() diff --git a/packages/vc/src/verification/verify-parsed-credential.ts b/packages/vc/src/verification/verify-parsed-credential.ts index 4ca8bd9..676dde2 100644 --- a/packages/vc/src/verification/verify-parsed-credential.ts +++ b/packages/vc/src/verification/verify-parsed-credential.ts @@ -48,23 +48,33 @@ function isVerifiable( * * @param credential - The credential to verify. * @param options - The {@link VerifyCredentialOptions} to use + * @returns The credential decoded from the verified proof. Callers MUST use + * this returned value for any post-verification reads — the `credential` + * argument is caller-supplied and is not bound to the proof. * @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) + // The proof (a JWT) is the only authoritative source. `verifyProof` returns + // the credential decoded from the verified proof; all trust decisions below + // run against that, never against the caller-supplied `credential` object, + // whose fields are not bound to the proof and may have been tampered with. + 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 +82,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..50d87e6 100644 --- a/packages/vc/src/verification/verify-proof.test.ts +++ b/packages/vc/src/verification/verify-proof.test.ts @@ -56,13 +56,23 @@ describe("verifyProof", () => { ).rejects.toThrow(InvalidProofError) }) - it("successfully verifies a valid JwtProof2020", async () => { + it("returns the credential decoded from the verified proof", async () => { const validProof = { type: "JwtProof2020", jwt: "valid.jwt.token", } - vi.mocked(verifyCredential).mockResolvedValueOnce({} as VerifiedCredential) - await expect(verifyProof(validProof, mockResolver)).resolves.not.toThrow() + const verifiableCredential = { + issuer: { id: "did:example:issuer" }, + credentialSubject: { id: "did:example:subject" }, + } + + vi.mocked(verifyCredential).mockResolvedValueOnce({ + verifiableCredential, + } as unknown as VerifiedCredential) + + await expect(verifyProof(validProof, mockResolver)).resolves.toBe( + verifiableCredential, + ) }) }) diff --git a/packages/vc/src/verification/verify-proof.ts b/packages/vc/src/verification/verify-proof.ts index 5f96094..1221ada 100644 --- a/packages/vc/src/verification/verify-proof.ts +++ b/packages/vc/src/verification/verify-proof.ts @@ -1,8 +1,8 @@ import type { Resolvable } from "@agentcommercekit/did" -import { verifyCredential } from "did-jwt-vc" import type { Verifiable, W3CCredential } from "../types" import { InvalidProofError, UnsupportedProofTypeError } from "./errors" +import { parseJwtCredential } from "./parse-jwt-credential" interface JwtProof { type: "JwtProof2020" @@ -29,28 +29,35 @@ export function isJwtProof(proof: unknown): proof is JwtProof { async function verifyJwtProof( proof: Verifiable["proof"], resolver: Resolvable, -): Promise { +): Promise> { if (!isJwtProof(proof)) { throw new InvalidProofError() } try { - await verifyCredential(proof.jwt, resolver) + return await parseJwtCredential(proof.jwt, resolver) } catch (_error) { throw new InvalidProofError() } } /** - * Verify a proof + * Verify a proof and return the credential decoded from it. + * + * The returned credential is derived from the verified proof (the JWT payload), + * not from any caller-supplied object. Callers MUST treat the returned value as + * the authoritative credential: a `JwtProof2020` proof is only bound to the + * claims inside its own JWT, so any outer object wrapping the proof can carry + * tampered fields that the proof does not attest to. * * @param proof - The credential proof to verify * @param resolver - The resolver to use for did resolution + * @returns The {@link Verifiable} decoded from the verified proof */ export async function verifyProof( proof: Verifiable["proof"], resolver: Resolvable, -): Promise { +): Promise> { switch (proof.type) { case "JwtProof2020": return verifyJwtProof(proof, resolver)