From 6078a99644d22e1ce2bcd1fc4d4894b55ec3881e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 18 Oct 2026 05:07:06 +0000 Subject: [PATCH] fix(documents): answer malformed input to the public verify endpoints with 400, not 500 verifyProof and verifyAttestationChain need no account, but malformed JSON reached the verifiers and threw: a null attestation payload, a numeric version.contentHash, null reviews, or a signed payload of "null" all came back as INTERNAL_SERVER_ERROR. verifyAudit already had a shape check, caps and a catch; these two now match it. - proofPackageProblem checks every field verifyProofPackage reads (shared with the audit check as proofShapeProblem) and caps reviews at 200, the per-version cap the audit package already applied. - verifyReview treats a non-object signed payload, or a non-string signed contentHash, as an invalid review instead of throwing. - verifyAttestationChain requires object payloads (checked, not parsed, so an extra field still breaks the signature) and caps the chain at AUDIT_LIMITS.chain (500); key ids are looked up as own properties only. - Both mutations catch what the shape check cannot see and return 400, logging the cause. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_011icbSAD1vGxmDnJQ4fByEk --- src/__tests__/documentAttestation.test.ts | 19 ++ src/__tests__/documentSignoff.test.ts | 81 ++++++++ .../trpc/documentVerifyEndpoints.test.ts | 179 ++++++++++++++++++ src/lib/documents/attestation.ts | 9 +- src/lib/documents/audit.ts | 52 +---- src/lib/documents/proof.ts | 85 ++++++++- src/server/api/routers/documents.ts | 74 ++++++-- 7 files changed, 433 insertions(+), 66 deletions(-) create mode 100644 src/__tests__/trpc/documentVerifyEndpoints.test.ts diff --git a/src/__tests__/documentAttestation.test.ts b/src/__tests__/documentAttestation.test.ts index 01cd6330..cbdf3b87 100644 --- a/src/__tests__/documentAttestation.test.ts +++ b/src/__tests__/documentAttestation.test.ts @@ -278,3 +278,22 @@ describe("the chain", () => { expect(result.ok).toBe(true); }); }); + +describe("an untrusted key id", () => { + it("is unknown when it names a property every object inherits", () => { + // `publicKeysById` is a plain object built from request JSON, so a lookup + // by "constructor" must not find Object.prototype's function and report a + // bad signature — the key is simply not one the caller recognises. + for (const keyId of ["constructor", "toString", "__proto__"]) { + const [record] = chain(1); + const result = verifyAttestationChain( + [{ ...record!, publicKeyId: keyId }], + KEYS, + ); + expect(result.ok).toBe(false); + expect(result.errors).toEqual([ + `sequence 1: signed by unknown key "${keyId}".`, + ]); + } + }); +}); diff --git a/src/__tests__/documentSignoff.test.ts b/src/__tests__/documentSignoff.test.ts index 32bf6389..290c0a81 100644 --- a/src/__tests__/documentSignoff.test.ts +++ b/src/__tests__/documentSignoff.test.ts @@ -20,6 +20,7 @@ import { } from "@/lib/documents/payload"; import { PROOF_FORMAT, + PROOF_LIMITS, VERIFICATION_INSTRUCTIONS, parseProofPackage, proofPackageProblem, @@ -444,3 +445,83 @@ describe("parseProofPackage", () => { expect(proofPackageProblem([])).toMatch(/no version or policy/i); }); }); + +/** + * `verifyProof` is a public endpoint, so the gate has to stop every package the + * verifier would otherwise throw on — a wrong type in any field it reads must + * be named as a problem, never surface as a TypeError and a 500. + */ +describe("proofPackageProblem on fields of the wrong type", () => { + const valid = () => makeProof([makeReview(SIGNER_A)]); + + it("passes a well-formed package, with or without the optional review fields", () => { + expect(proofPackageProblem(valid())).toBeNull(); + const withParty = valid(); + withParty.reviews[0] = { ...withParty.reviews[0]!, partyId: "p1", comment: "ok" }; + expect(proofPackageProblem(withParty)).toBeNull(); + }); + + it("says in a sentence what is wrong, for the paste box on /verify", () => { + const pkg = valid() as unknown as Record; + pkg.reviews = null; + expect(proofPackageProblem(pkg)).toBe( + "That proof package is malformed: proof.reviews is missing or too long.", + ); + }); + + it.each<[string, (p: Record) => void, RegExp]>([ + ["version is a string", (p) => (p.version = "v1"), /proof\.version/], + ["contentHash is a number", (p) => (p.version.contentHash = 5), /proof\.version/], + ["versionNumber is a string", (p) => (p.version.versionNumber = "1"), /proof\.version/], + ["document is missing", (p) => delete p.document, /proof\.document/], + ["policy is a string", (p) => (p.policy = "2-of-3"), /proof\.policy/], + ["requiredSigners is a string", (p) => (p.policy.requiredSigners = "2"), /proof\.policy/], + ["a signer address is a number", (p) => (p.policy.signersAddresses = [1]), /proof\.policy/], + ["reviews is null", (p) => (p.reviews = null), /proof\.reviews/], + ["a review is null", (p) => (p.reviews = [null]), /review is malformed/], + ["a review payload is an object", (p) => (p.reviews[0].payload = {}), /review is malformed/], + ["a review comment is an object", (p) => (p.reviews[0].comment = { toString: 1 }), /review is malformed/], + ])("names the problem when %s", (_, mutate, problem) => { + const pkg = valid() as unknown as Record; + mutate(pkg); + expect(proofPackageProblem(pkg)).toMatch(problem); + }); + + it("refuses more reviews than any wallet could produce", () => { + // Each review costs a CIP-8 check on the server; the cap bounds that work. + const review = makeReview(SIGNER_A); + const padded = makeProof( + Array.from({ length: PROOF_LIMITS.reviews + 1 }, () => review), + ); + expect(proofPackageProblem(padded)).toMatch(/too long/); + }); +}); + +describe("verifyProofPackage on a signed payload of the wrong shape", () => { + // The payload string is what the signer signed, so it is attacker-chosen + // too. Valid, canonical JSON that is not a sign-off payload has to be an + // invalid review, not an exception. + it.each(["null", "42", "[]", '"approve"'])( + "reports %s as not a JSON object", + async (payload) => { + const review = { ...makeReview(SIGNER_A), payload }; + const result = await verifyProofPackage(makeProof([review], 1), { + checkSignature: acceptAll, + }); + expect(result.valid).toBe(false); + expect(result.reviews[0]!.errors).toContain("payload is not a JSON object"); + }, + ); + + it("fails the binding when the signed contentHash is not a string", async () => { + const good = makeReview(SIGNER_A); + const tampered = { ...JSON.parse(good.payload), contentHash: 5 }; + const review = { ...good, payload: canonicalize(tampered) }; + const result = await verifyProofPackage(makeProof([review], 1), { + checkSignature: acceptAll, + }); + expect(result.valid).toBe(false); + expect(result.reviews[0]!.payloadBindsToVersion).toBe(false); + expect(result.reviews[0]!.errors.join(" ")).toMatch(/payload\.contentHash/); + }); +}); diff --git a/src/__tests__/trpc/documentVerifyEndpoints.test.ts b/src/__tests__/trpc/documentVerifyEndpoints.test.ts new file mode 100644 index 00000000..e5832834 --- /dev/null +++ b/src/__tests__/trpc/documentVerifyEndpoints.test.ts @@ -0,0 +1,179 @@ +import { generateKeyPairSync } from "crypto"; +import { beforeAll, describe, expect, it, jest } from "@jest/globals"; +import { TRPCError } from "@trpc/server"; + +import { makeAnonymousCtx } from "./helpers"; + +/** + * The public verify endpoints — `verifyProof` and `verifyAttestationChain` — + * need no account and read no database, so anyone can send them anything. The + * contract under test: input the verifier cannot read is a 400 that says so, + * never a TypeError surfacing as a 500, and an honest chain still verifies. + * + * No database is touched, so these run with or without DATABASE_URL. + */ + +jest.mock("superjson", () => ({ + __esModule: true, + default: { + serialize: (value: unknown) => value, + deserialize: (value: unknown) => value, + }, +})); + +jest.mock("@/server/auth", () => ({ + __esModule: true, + getServerAuthSession: jest.fn(), +})); + +let createCaller: typeof import("@/server/api/root").createCaller; +let attestation: typeof import("@/lib/documents/attestation"); +let AUDIT_LIMITS: typeof import("@/lib/documents/audit").AUDIT_LIMITS; +let canonicalize: typeof import("@/lib/documents/payload").canonicalize; + +beforeAll(async () => { + ({ createCaller } = await import("@/server/api/root")); + attestation = await import("@/lib/documents/attestation"); + ({ AUDIT_LIMITS } = await import("@/lib/documents/audit")); + ({ canonicalize } = await import("@/lib/documents/payload")); +}); + +// A fresh IP per caller keeps the public rate limiter out of the picture. +const documents = () => createCaller(makeAnonymousCtx() as never).document; + +async function codeOf(call: Promise): Promise { + try { + await call; + return "OK"; + } catch (error) { + return error instanceof TRPCError ? error.code : "THREW"; + } +} + +function signedChain(n: number) { + const { publicKey, privateKey } = generateKeyPairSync("ed25519"); + const spkiHex = publicKey + .export({ type: "spki", format: "der" }) + .toString("hex"); + const keyId = attestation.attestationKeyId(spkiHex); + const chain = []; + let prev = attestation.GENESIS_PREV; + for (let i = 1; i <= n; i++) { + const payload = attestation.buildAttestationPayload({ + attestedAt: new Date(Date.UTC(2026, 0, i, 12)), + contentHash: "a".repeat(64), + documentId: "doc_1", + prevAttestationHash: prev, + sequence: i, + versionId: `ver_${i}`, + versionNumber: i, + walletId: "wallet_1", + }); + chain.push({ + payload, + signature: attestation.signAttestation(payload, privateKey), + publicKeyId: keyId, + }); + prev = attestation.attestationHash(payload); + } + return { chain, publicKeys: { [keyId]: spkiHex } }; +} + +describe("verifyAttestationChain", () => { + it("verifies an honest chain", async () => { + const { chain, publicKeys } = signedChain(2); + const result = await documents().verifyAttestationChain({ + chain, + publicKeys, + }); + expect(result.errors).toEqual([]); + expect(result.ok).toBe(true); + }); + + it("does not trim an extra payload field back into a valid signature", async () => { + // The payload reaches the verifier exactly as sent: a field the platform + // never signed must break the signature, not be stripped by validation. + const { chain, publicKeys } = signedChain(1); + const padded = [{ ...chain[0]!, payload: { ...chain[0]!.payload, note: "x" } }]; + const result = await documents().verifyAttestationChain({ + chain: padded, + publicKeys, + }); + expect(result.ok).toBe(false); + expect(result.errors).toContain("sequence 1: signature does not verify."); + }); + + it.each<[string, unknown]>([ + ["a null payload", null], + ["a string payload", "payload"], + ["an array payload", []], + ["a missing payload", undefined], + ])("answers %s with BAD_REQUEST", async (_, payload) => { + const record = { payload, signature: "", publicKeyId: "" }; + expect( + await codeOf( + documents().verifyAttestationChain({ chain: [record] } as never), + ), + ).toBe("BAD_REQUEST"); + }); + + it("refuses a chain longer than an audit package may carry", async () => { + // Every record costs an Ed25519 verify on a route with no account behind it. + const { chain } = signedChain(1); + const long = Array.from({ length: AUDIT_LIMITS.chain + 1 }, () => chain[0]!); + expect( + await codeOf(documents().verifyAttestationChain({ chain: long })), + ).toBe("BAD_REQUEST"); + }); +}); + +describe("verifyProof", () => { + it.each<[string, unknown]>([ + ["a string version", { version: "v1", policy: "p" }], + ["null reviews", { version: {}, policy: {}, reviews: null }], + [ + "a numeric content hash", + { + document: { id: "d", walletId: "w" }, + version: { id: "v", versionNumber: 1, contentHash: 5 }, + policy: { requiredSigners: 1, walletPolicyHash: "h", signersAddresses: [] }, + reviews: [], + }, + ], + ])("answers %s with BAD_REQUEST", async (_, proof) => { + expect(await codeOf(documents().verifyProof({ proof }))).toBe( + "BAD_REQUEST", + ); + }); + + it("answers BAD_REQUEST when the verifier itself throws", async () => { + // The shape check cannot see inside the signed payload string. A field + // there that cannot even be printed in an error message makes the + // verifier throw; that must still come back as a 400. + const proof = { + document: { id: "d", walletId: "w" }, + version: { id: "v", versionNumber: 1, contentHash: "a".repeat(64) }, + policy: { + requiredSigners: 1, + walletPolicyHash: "h", + signersAddresses: ["addr_a"], + }, + reviews: [ + { + signerAddress: "addr_a", + action: "approve", + payload: canonicalize({ documentId: { toString: 1 } }), + signature: "s", + signatureKey: "k", + signedAt: "2026-01-01T00:00:00.000Z", + }, + ], + }; + const warn = jest.spyOn(console, "warn").mockImplementation(() => {}); + expect(await codeOf(documents().verifyProof({ proof }))).toBe( + "BAD_REQUEST", + ); + expect(warn).toHaveBeenCalled(); + warn.mockRestore(); + }); +}); diff --git a/src/lib/documents/attestation.ts b/src/lib/documents/attestation.ts index 78248ed2..a73c46d2 100644 --- a/src/lib/documents/attestation.ts +++ b/src/lib/documents/attestation.ts @@ -248,7 +248,14 @@ export function verifyAttestationChain( previousAt = attestedAt; } - const publicKey = publicKeysById[record.publicKeyId]; + // Own keys only: an id of "constructor" must read as unknown, not as + // Object.prototype's function. + const publicKey = Object.prototype.hasOwnProperty.call( + publicKeysById, + record.publicKeyId, + ) + ? publicKeysById[record.publicKeyId] + : undefined; if (!publicKey) { errors.push(`${at}: signed by unknown key "${record.publicKeyId}".`); } else if (!verifyAttestation(payload, record.signature, publicKey)) { diff --git a/src/lib/documents/audit.ts b/src/lib/documents/audit.ts index 38b3a974..85a301e6 100644 --- a/src/lib/documents/audit.ts +++ b/src/lib/documents/audit.ts @@ -33,7 +33,9 @@ import { } from "./attestation"; import { normalizeContentHash } from "./payload"; import { + PROOF_LIMITS, buildProofPackage, + proofShapeProblem, verifyProofPackage, type CheckSignatureFn, type ProofPackage, @@ -584,7 +586,7 @@ export async function verifyAuditPackage( /** Caps for a package arriving at a public endpoint. Real documents sit far below. */ export const AUDIT_LIMITS = { versions: 500, - reviewsPerVersion: 200, + reviewsPerVersion: PROOF_LIMITS.reviews, chain: 500, events: 20_000, } as const; @@ -596,52 +598,8 @@ const isInt = (v: unknown): v is number => Number.isInteger(v); const isNullableString = (v: unknown) => v === null || isString(v); function proofProblem(proof: unknown, where: string): string | null { - if (!isObject(proof)) return `${where}: proof is not an object`; - const { document, version, policy, reviews } = proof; - if ( - !isObject(document) || - !isString(document.id) || - !isString(document.walletId) - ) { - return `${where}: proof.document is malformed`; - } - if ( - !isObject(version) || - !isString(version.id) || - !isInt(version.versionNumber) || - !isString(version.contentHash) - ) { - return `${where}: proof.version is malformed`; - } - if ( - !isObject(policy) || - !isInt(policy.requiredSigners) || - !isString(policy.walletPolicyHash) || - !Array.isArray(policy.signersAddresses) || - !policy.signersAddresses.every(isString) - ) { - return `${where}: proof.policy is malformed`; - } - if ( - !Array.isArray(reviews) || - reviews.length > AUDIT_LIMITS.reviewsPerVersion - ) { - return `${where}: proof.reviews is missing or too long`; - } - for (const r of reviews) { - if ( - !isObject(r) || - !isString(r.signerAddress) || - !isString(r.payload) || - !isString(r.signature) || - !isString(r.signatureKey) || - !isString(r.signedAt) || - !isString(r.action) - ) { - return `${where}: a review is malformed`; - } - } - return null; + const problem = proofShapeProblem(proof); + return problem === null ? null : `${where}: ${problem}`; } /** diff --git a/src/lib/documents/proof.ts b/src/lib/documents/proof.ts index bd125b3f..1cb20f46 100644 --- a/src/lib/documents/proof.ts +++ b/src/lib/documents/proof.ts @@ -107,6 +107,10 @@ export const VERIFICATION_INSTRUCTIONS = [ * far more useful answer to a counterparty than a flat rejection — and * {@link verifyProofPackage} already records an unknown format as a * verification error. + * + * It does judge every field the verifier reads. `verifyProof` is public, so a + * package whose `version.contentHash` is a number or whose `reviews` is null + * has to come back as a 400 that says so, not as a TypeError and a 500. */ export function proofPackageProblem(value: unknown): string | null { if (!value || typeof value !== "object") { @@ -116,6 +120,75 @@ export function proofPackageProblem(value: unknown): string | null { if (!pkg.version || !pkg.policy) { return "That JSON is not a Document Sign-Off proof package — it carries no version or policy."; } + const problem = proofShapeProblem(value); + return problem === null + ? null + : `That proof package is malformed: ${problem}.`; +} + +/** + * Caps for a proof arriving at a public endpoint. Each review costs a CIP-8 + * check, so the count is bounded; real wallets sit far below it. + */ +export const PROOF_LIMITS = { reviews: 200 } as const; + +const isObject = (v: unknown): v is Record => + !!v && typeof v === "object" && !Array.isArray(v); +const isString = (v: unknown): v is string => typeof v === "string"; +const isInt = (v: unknown): v is number => Number.isInteger(v); +/** Absent and null both mean "none" for the optional review fields. */ +const isOptionalString = (v: unknown) => v == null || isString(v); + +/** + * The structural half of {@link proofPackageProblem}: every field + * {@link verifyProofPackage} dereferences, with the type it assumes. Shared + * with the audit-package check, which embeds one proof per version. + */ +export function proofShapeProblem(proof: unknown): string | null { + if (!isObject(proof)) return "proof is not an object"; + const { document, version, policy, reviews } = proof; + if ( + !isObject(document) || + !isString(document.id) || + !isString(document.walletId) + ) { + return "proof.document is malformed"; + } + if ( + !isObject(version) || + !isString(version.id) || + !isInt(version.versionNumber) || + !isString(version.contentHash) + ) { + return "proof.version is malformed"; + } + if ( + !isObject(policy) || + !isInt(policy.requiredSigners) || + !isString(policy.walletPolicyHash) || + !Array.isArray(policy.signersAddresses) || + !policy.signersAddresses.every(isString) + ) { + return "proof.policy is malformed"; + } + if (!Array.isArray(reviews) || reviews.length > PROOF_LIMITS.reviews) { + return "proof.reviews is missing or too long"; + } + for (const r of reviews) { + if ( + !isObject(r) || + !isString(r.signerAddress) || + !isString(r.payload) || + !isString(r.signature) || + !isString(r.signatureKey) || + !isString(r.signedAt) || + !isString(r.action) || + !isOptionalString(r.partyId) || + !isOptionalString(r.comment) + ) { + return "a review is malformed"; + } + } return null; } @@ -295,6 +368,12 @@ async function verifyReview( verdict.errors.push("payload is not valid JSON"); return verdict; } + // `"null"` and `"42"` are valid, canonical JSON too; everything below reads + // fields off the payload, so anything but an object stops here. + if (!isObject(payload)) { + verdict.errors.push("payload is not a JSON object"); + return verdict; + } // The stored string must itself be canonical — otherwise two different // strings could carry the same JSON and only one of them is what was signed. @@ -312,7 +391,11 @@ async function verifyReview( ["versionNumber", payload.versionNumber, pkg.version.versionNumber], [ "contentHash", - normalizeContentHash(payload.contentHash), + // The signed bytes are attacker-supplied: a numeric contentHash must + // fail the binding, not throw out of the verifier. + typeof payload.contentHash === "string" + ? normalizeContentHash(payload.contentHash) + : payload.contentHash, normalizeContentHash(pkg.version.contentHash), ], ["walletId", payload.walletId, pkg.document.walletId], diff --git a/src/server/api/routers/documents.ts b/src/server/api/routers/documents.ts index 788dd09e..bdc5fdd3 100644 --- a/src/server/api/routers/documents.ts +++ b/src/server/api/routers/documents.ts @@ -70,6 +70,7 @@ import { } from "@/lib/documents/proof"; import { diffVersions } from "@/lib/documents/diff"; import { + AUDIT_LIMITS, auditPackageProblem, buildAuditPackage, verifyAuditPackage, @@ -883,10 +884,25 @@ export const documentRouter = createTRPCRouter({ throw new TRPCError({ code: "BAD_REQUEST", message: problem }); } const pkg = input.proof as ProofPackage; - return verifyProofPackage(pkg, { - expectedContentHash: input.expectedContentHash, - checkSignature, - }); + // The shape check covers every field the verifier reads; this catches + // what it cannot, such as a signed payload too deeply nested to + // canonicalize, so the answer is still a 400 and never a 500. + try { + return await verifyProofPackage(pkg, { + expectedContentHash: input.expectedContentHash, + checkSignature, + }); + } catch (error) { + // Logged so a real verifier bug does not hide behind the 400. + console.warn( + "[document.verifyProof] verifier threw:", + error instanceof Error ? error.message : String(error), + ); + throw new TRPCError({ + code: "BAD_REQUEST", + message: "That proof package could not be verified — it is malformed.", + }); + } }), /** @@ -930,23 +946,47 @@ export const documentRouter = createTRPCRouter({ verifyAttestationChain: publicProcedure .input( z.object({ - chain: z.array( - z.object({ - payload: z.unknown(), - signature: z.string(), - publicKeyId: z.string(), - }), - ), + // Bounded like the chain inside an audit package: every record costs + // an Ed25519 verify, and this endpoint needs no account. + chain: z + .array( + z.object({ + // An object, not z.unknown(): a null payload used to reach the + // verifier and throw. Checked rather than parsed, so the object + // reaches the verifier exactly as sent — a payload carrying an + // extra field must fail its signature, not be trimmed back into + // one that verifies. + payload: z.custom( + (v) => !!v && typeof v === "object" && !Array.isArray(v), + "payload must be an object", + ), + signature: z.string(), + publicKeyId: z.string(), + }), + ) + .max(AUDIT_LIMITS.chain), /** Omit to check against the keys this deployment publishes. */ publicKeys: z.record(z.string(), z.string()).optional(), }), ) - .mutation(({ input }) => - verifyAttestationChain( - input.chain as AttestationRecord[], - input.publicKeys ?? getAttestationPublicKeys(), - ), - ), + .mutation(({ input }) => { + try { + return verifyAttestationChain( + input.chain as unknown as AttestationRecord[], + input.publicKeys ?? getAttestationPublicKeys(), + ); + } catch (error) { + console.warn( + "[document.verifyAttestationChain] verifier threw:", + error instanceof Error ? error.message : String(error), + ); + throw new TRPCError({ + code: "BAD_REQUEST", + message: + "That attestation chain could not be verified — it is malformed.", + }); + } + }), /** * The draft body, if there is one. Returns null rather than throwing when a