From a3997b7996639c498913696658b9744b92da0314 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:37:17 +1000 Subject: [PATCH 1/2] fix(bench): key seed rows by JS property, and check it without a database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extractEncryptionSchema keys the encrypted-table column map by the Drizzle table's JS PROPERTY (encText), not the DB column name (enc_text) — so buildColumnKeyMap() is {encText: 'enc_text', ...} and resolveEncryptColumnMap matches models on the property names. The v2 -> v3 port left the seed keyed by DB name, so no field matched: bulkEncryptModels returned every row untouched, with no failure, and the insert then carried PLAINTEXT into eql_v3_* columns. The remap at seed.ts:66 was moving plaintext, not ciphertext, and its comment had become false. Not a data-safety bug — verified against a real Postgres with the pinned EQL 3.0.2 bundle, all three domains reject the insert with 23514. It is a bench that cannot run at all for anyone with credentials. Nothing caught it because nothing could. `tsc --noEmit` passes: BenchPlaintextRow was hand-written and agreed with itself. harness.test.ts asserts a row count and never that a column is encrypted. And CI runs only `test:local db-only`, which never seeds — every suite that touches the seed needs credentials. So the check that fails on this is deliberately cheap: __unit__/seed-keys.test.ts compares the row's keys against buildColumnKeyMap()'s, under a second vitest config with no globalSetup, so it needs neither a database nor credentials and cannot be skipped for want of either. BenchPlaintextRow is now the property space, and the identity remap is gone. --- .github/workflows/tests-bench.yml | 10 +++++ packages/bench/__unit__/seed-keys.test.ts | 50 +++++++++++++++++++++ packages/bench/package.json | 53 ++++++++++++----------- packages/bench/src/drizzle/setup.ts | 18 ++++++-- packages/bench/src/harness/seed.ts | 23 +++++----- packages/bench/vitest.unit.config.ts | 18 ++++++++ 6 files changed, 131 insertions(+), 41 deletions(-) create mode 100644 packages/bench/__unit__/seed-keys.test.ts create mode 100644 packages/bench/vitest.unit.config.ts diff --git a/.github/workflows/tests-bench.yml b/.github/workflows/tests-bench.yml index 5b8da973..71ff378a 100644 --- a/.github/workflows/tests-bench.yml +++ b/.github/workflows/tests-bench.yml @@ -88,3 +88,13 @@ jobs: - name: Run bench smoke tests working-directory: packages/bench run: pnpm test:local db-only + + # Separate config, no globalSetup: these need neither a database nor + # credentials, so they cannot be skipped for want of either. The seed + # keying they check was wrong for the whole v2 -> v3 port precisely + # because every suite that would have caught it needs credentials, and + # `tsc --noEmit` passes on a hand-written row type that agrees with + # itself (#772 review, finding 12). + - name: Run bench unit checks (no database, no credentials) + working-directory: packages/bench + run: pnpm test:unit diff --git a/packages/bench/__unit__/seed-keys.test.ts b/packages/bench/__unit__/seed-keys.test.ts new file mode 100644 index 00000000..82a356e1 --- /dev/null +++ b/packages/bench/__unit__/seed-keys.test.ts @@ -0,0 +1,50 @@ +/** + * The seed's row keys must be the keys model encryption MATCHES on. + * + * `extractEncryptionSchema` keys the encrypted-table column map by the Drizzle + * table's **JS property** (`encText`), while the column's DB name (`enc_text`) + * is what the builder carries. So `resolveEncryptColumnMap().columnPaths` — the + * list every model operation matches a row's fields against — is the property + * names. A row keyed by DB name matches nothing: `bulkEncryptModels` returns it + * untouched, with no failure, and the insert then puts PLAINTEXT into columns + * typed `eql_v3_*`. + * + * That is exactly what the v2 -> v3 port left behind, and nothing caught it: + * `tsc --noEmit` passes because `BenchPlaintextRow` was hand-written and agreed + * with itself, and CI only runs the `db-only` filter, which never seeds + * (#772 review, finding 12). + * + * Credential-free by construction — this compares two key sets and never + * reaches ZeroKMS. + */ +import { describe, expect, it } from 'vitest' +import { encryptionBenchTable } from '../src/drizzle/setup.js' +import { makePlaintextRow } from '../src/harness/seed.js' + +describe('bench seed rows are keyed for model encryption', () => { + // `resolveEncryptColumnMap` is internal, but it derives `columnPaths` as + // exactly `Object.keys(table.buildColumnKeyMap())` — read the same source. + const columnPaths = Object.keys(encryptionBenchTable.buildColumnKeyMap()) + + it('finds the encrypted columns (guards against an empty comparison)', () => { + expect(columnPaths.length).toBeGreaterThan(0) + }) + + it('emits a field for every encrypted column, under the matched key', () => { + const rowKeys = Object.keys(makePlaintextRow(0)) + + // Every encrypted column must be present in the row, or that column is + // silently never encrypted. + expect(rowKeys).toEqual(expect.arrayContaining([...columnPaths])) + }) + + it('emits no field the matcher would pass through as plaintext', () => { + const rowKeys = Object.keys(makePlaintextRow(0)) + const unmatched = rowKeys.filter((key) => !columnPaths.includes(key)) + + expect( + unmatched, + `these seed fields match no encrypted column, so they would be inserted as plaintext into an eql_v3_* column: ${unmatched.join(', ')}`, + ).toEqual([]) + }) +}) diff --git a/packages/bench/package.json b/packages/bench/package.json index 4134c57c..55008736 100644 --- a/packages/bench/package.json +++ b/packages/bench/package.json @@ -1,28 +1,29 @@ { - "name": "@cipherstash/bench", - "version": "0.0.5-rc.4", - "private": true, - "description": "Performance / index-engagement benchmarks for stack integrations (Drizzle, encryptedSupabase, Prisma).", - "type": "module", - "scripts": { - "build": "tsc --noEmit", - "db:setup": "tsx src/cli/setup.ts", - "db:reset": "tsx src/cli/reset.ts", - "test:local": "vitest run", - "bench:local": "vitest bench --run" - }, - "dependencies": { - "@cipherstash/stack": "workspace:*", - "@cipherstash/stack-drizzle": "workspace:*", - "drizzle-orm": "0.45.2", - "pg": "^8.22.0" - }, - "devDependencies": { - "@cipherstash/test-kit": "workspace:*", - "@types/node": "^22.20.1", - "@types/pg": "^8.20.0", - "tsx": "catalog:repo", - "typescript": "catalog:repo", - "vitest": "catalog:repo" - } + "name": "@cipherstash/bench", + "version": "0.0.5-rc.4", + "private": true, + "description": "Performance / index-engagement benchmarks for stack integrations (Drizzle, encryptedSupabase, Prisma).", + "type": "module", + "scripts": { + "build": "tsc --noEmit", + "test:unit": "vitest run --config vitest.unit.config.ts", + "db:setup": "tsx src/cli/setup.ts", + "db:reset": "tsx src/cli/reset.ts", + "test:local": "vitest run", + "bench:local": "vitest bench --run" + }, + "dependencies": { + "@cipherstash/stack": "workspace:*", + "@cipherstash/stack-drizzle": "workspace:*", + "drizzle-orm": "0.45.2", + "pg": "^8.22.0" + }, + "devDependencies": { + "@cipherstash/test-kit": "workspace:*", + "@types/node": "^22.20.1", + "@types/pg": "^8.20.0", + "tsx": "catalog:repo", + "typescript": "catalog:repo", + "vitest": "catalog:repo" + } } diff --git a/packages/bench/src/drizzle/setup.ts b/packages/bench/src/drizzle/setup.ts index 16ba56d2..d8bf48d0 100644 --- a/packages/bench/src/drizzle/setup.ts +++ b/packages/bench/src/drizzle/setup.ts @@ -28,10 +28,22 @@ export const benchTable = pgTable('bench', { */ export const encryptionBenchTable = extractEncryptionSchema(benchTable) +/** + * A seed row, keyed by the Drizzle table's **JS property** names. + * + * That is what model encryption matches on: `extractEncryptionSchema` keys the + * encrypted-table column map by property (`encText`), not by DB column name + * (`enc_text`). A row keyed by DB name matches nothing — `bulkEncryptModels` + * returns it untouched, with no failure, and the plaintext then goes into an + * `eql_v3_*` column (#772 review, finding 12). + * + * Derived from the table so the two cannot drift again; + * `__unit__/seed-keys.test.ts` checks the row actually fills it. + */ export type BenchPlaintextRow = { - enc_text: string - enc_int: number - enc_jsonb: { idx: number; group: number } + encText: string + encInt: number + encJsonb: { idx: number; group: number } } /** diff --git a/packages/bench/src/harness/seed.ts b/packages/bench/src/harness/seed.ts index e9fc828b..a145c6f6 100644 --- a/packages/bench/src/harness/seed.ts +++ b/packages/bench/src/harness/seed.ts @@ -24,11 +24,12 @@ export function getTargetRows(): number { return n } -function makePlaintextRow(idx: number): BenchPlaintextRow { +/** Exported so `__unit__/seed-keys.test.ts` can check the row keys. */ +export function makePlaintextRow(idx: number): BenchPlaintextRow { return { - enc_text: `value-${String(idx).padStart(7, '0')}`, - enc_int: idx, - enc_jsonb: { idx, group: idx % 100 }, + encText: `value-${String(idx).padStart(7, '0')}`, + encInt: idx, + encJsonb: { idx, group: idx % 100 }, } } @@ -63,14 +64,12 @@ export async function seed( ) } - // bulkEncryptModels returns rows keyed by the encryptedTable column names - // (snake_case here) with encrypted EQL v3 envelopes as values. Drizzle's - // `benchTable` uses camelCase TS field names — remap before insert. - const encRows = encResult.data.map((r) => ({ - encText: r.enc_text, - encInt: r.enc_int, - encJsonb: r.enc_jsonb, - })) + // bulkEncryptModels returns rows under the SAME keys it matched on — the + // Drizzle table's JS property names — with EQL v3 envelopes as values. Those + // are the keys `db.insert()` wants, so there is nothing to remap. (The + // remap that used to sit here rewrote enc_text -> encText, which only + // appeared to work: nothing was ever encrypted, so it was moving plaintext.) + const encRows = encResult.data for (let i = 0; i < encRows.length; i += INSERT_BATCH) { const batch = encRows.slice(i, i + INSERT_BATCH) diff --git a/packages/bench/vitest.unit.config.ts b/packages/bench/vitest.unit.config.ts new file mode 100644 index 00000000..fa01c8c5 --- /dev/null +++ b/packages/bench/vitest.unit.config.ts @@ -0,0 +1,18 @@ +import { defineConfig } from 'vitest/config' + +/** + * Bench checks that need neither a database nor credentials. + * + * The main config's `globalSetup` installs EQL v3 through the built CLI, so + * every suite under it requires `turbo run build --filter stash` and a live + * Postgres. That is right for the benchmarks, and wrong for a check that only + * compares two key sets — and "it was too expensive to run in CI" is exactly + * how the seed came to insert plaintext into `eql_v3_*` columns unnoticed + * (#772 review, finding 12). + */ +export default defineConfig({ + test: { + include: ['__unit__/**/*.test.ts'], + server: { deps: { inline: [/packages\/test-kit/] } }, + }, +}) From bcf247aa12bad2f107c670ec76e4e1a089fe5c2f Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:40:16 +1000 Subject: [PATCH 2/2] fix(stack): refuse the wasm-inline client for DynamoDB EQL v2 reads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The adapter's v2 read path calls decryptModel(item) with NO table, on purpose: a v2 table means nothing to a v3 client's reconstructor map, and the native clients derive the table from the payloads. WasmEncryptionClient cannot — its decrypt requires the table and resolves date fields from a per-table map, so the omitted argument reached requireTable(undefined) and threw `TypeError: Cannot read properties of undefined (reading 'tableName')` from deep inside the client. That client ships in this package, is the documented entry for Deno, Workers and Supabase Edge Functions, and satisfies DynamoDBEncryptionClient structurally — so encryptedDynamoDB accepted it with no cast and the pairing only failed at the first read, with a message pointing nowhere near the cause. Reject it where the message can name the combination. The signal is a declared capability on the client (`requiresTableForDecrypt`) rather than sniffing arity or constructor name. v3 tables are always passed the table, so they still work. Also corrects three comments this package carried claiming audit metadata is forwarded "regardless of client shape" and that "every client this package ships carries .audit() on decrypt". The wasm-inline client's decrypt is a plain async method; its audit metadata is dropped, and the debug message that fires told the user to build a client with Encryption({ schemas }) — which is exactly what the wasm entry's own factory is. --- .changeset/dynamodb-wasm-v2-read.md | 27 +++++++ .../dynamodb/v2-table-forwarding.test.ts | 78 +++++++++++++++++++ packages/stack/src/dynamodb/helpers.ts | 22 +++--- packages/stack/src/dynamodb/index.ts | 19 ++++- packages/stack/src/dynamodb/types.ts | 10 ++- packages/stack/src/wasm-inline.ts | 15 ++++ 6 files changed, 158 insertions(+), 13 deletions(-) create mode 100644 .changeset/dynamodb-wasm-v2-read.md diff --git a/.changeset/dynamodb-wasm-v2-read.md b/.changeset/dynamodb-wasm-v2-read.md new file mode 100644 index 00000000..1e52ae72 --- /dev/null +++ b/.changeset/dynamodb-wasm-v2-read.md @@ -0,0 +1,27 @@ +--- +'@cipherstash/stack': patch +--- + +`encryptedDynamoDB` now refuses a `@cipherstash/stack/wasm-inline` client paired +with a legacy EQL v2 table, instead of failing at the first read with a +misleading error. + +The adapter's v2 read path deliberately calls `decryptModel(item)` with **no** +table — a v2 table means nothing to a v3 client's reconstructor map, and the +native clients derive the table from the payloads anyway. `WasmEncryptionClient` +cannot do that: its decrypt requires the table and resolves date fields from a +per-table map, so the omitted argument surfaced as +`TypeError: Cannot read properties of undefined (reading 'tableName')` thrown +from deep inside the client — on the documented entry for Deno, Cloudflare +Workers and Supabase Edge Functions, which satisfies the adapter's client type +structurally and so was accepted with no cast. + +The pairing is now rejected at the call site, with a message naming both the +combination and the fix. EQL v3 tables are unaffected: they are always passed +the table, so the wasm-inline client keeps working there. + +Three comments in this package claimed audit metadata was forwarded "regardless +of client shape" and that "every client this package ships carries `.audit()` on +decrypt". Neither was true of the wasm-inline client, whose decrypt returns a +plain promise — the metadata is dropped, observably (it is logged at debug), and +the comments and the debug message now say so. diff --git a/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts b/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts index d8f1bc87..c6ab542e 100644 --- a/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts +++ b/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts @@ -106,3 +106,81 @@ describe('bulkDecryptModels table forwarding', () => { expect(calls[0]?.table).toBe(usersV3) }) }) + +/** + * #772 review, finding 10. + * + * The table-less v2 decrypt above is correct for the native clients, which + * derive the table from the payloads. `WasmEncryptionClient` cannot: its + * decrypt requires the table and resolves date fields from a per-table map, so + * the omitted argument reached `requireTable(undefined)` and threw a TypeError + * about reading `tableName` — a message pointing nowhere near the cause, on the + * documented entry for Deno / Workers / Supabase Edge Functions, which + * satisfies `DynamoDBEncryptionClient` structurally and so is accepted with no + * cast. + */ +describe('a client whose decrypt requires the table', () => { + /** The shape `WasmEncryptionClient` presents: declared capability, no `.audit()`. */ + function wasmShapedClient(knownTables: string[]) { + const calls: { method: string; argCount: number }[] = [] + const record = + (method: string) => + (...args: unknown[]) => { + calls.push({ method, argCount: args.length }) + // Mirrors requireTable: throws rather than returning a Result. + if (args[1] === undefined) { + throw new TypeError( + "Cannot read properties of undefined (reading 'tableName')", + ) + } + return Promise.resolve({ data: {} }) + } + const client = { + requiresTableForDecrypt: true, + getEncryptConfig: () => ({ + v: 1, + tables: Object.fromEntries(knownTables.map((t) => [t, {}])), + }), + encryptModel: record('encryptModel'), + bulkEncryptModels: record('bulkEncryptModels'), + decryptModel: record('decryptModel'), + bulkDecryptModels: record('bulkDecryptModels'), + } + return { calls, client } + } + + it('is refused for an EQL v2 table, naming the entry to use instead', () => { + const { calls, client } = wasmShapedClient([]) + const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) + + // Synchronous: the guard runs when the operation is built, so the failure + // lands at the call site rather than as a rejected promise later. + expect(() => dynamo.decryptModel({ pk: 'a' }, usersV2)).toThrow( + /wasm-inline client cannot read legacy EQL v2 items/, + ) + // Refused before the client is touched, so the user never sees the + // TypeError about `tableName`. + expect(calls).toHaveLength(0) + }) + + it('is refused on the bulk v2 path too', () => { + const { client } = wasmShapedClient([]) + const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) + + expect(() => dynamo.bulkDecryptModels([{ pk: 'a' }], usersV2)).toThrow( + /wasm-inline client cannot read legacy EQL v2 items/, + ) + }) + + // v3 tables ARE forwarded the table, so this client works there — the guard + // must not turn into a blanket rejection of the wasm entry. + it('is accepted for an EQL v3 table, which is always given the table', async () => { + const { calls, client } = wasmShapedClient(['users_v3']) + const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) + + await dynamo.decryptModel({ pk: 'a' }, usersV3) + + expect(calls).toHaveLength(1) + expect(calls[0]?.argCount).toBe(2) + }) +}) diff --git a/packages/stack/src/dynamodb/helpers.ts b/packages/stack/src/dynamodb/helpers.ts index 38c62d09..186e3649 100644 --- a/packages/stack/src/dynamodb/helpers.ts +++ b/packages/stack/src/dynamodb/helpers.ts @@ -86,11 +86,16 @@ export function handleError( /** * Resolve a decrypt call against either client shape. * - * Both the nominal `EncryptionClient` and the typed client return a chainable + * The nominal `EncryptionClient` and the typed client both return a chainable * operation carrying `.audit()` on decrypt (the typed client's is a - * `MappedDecryptOperation`). Chain the audit metadata onto it; the branch that - * awaits a bare promise remains only for a non-conforming custom client that - * exposes no `.audit()`. Audit metadata is forwarded regardless of client shape. + * `MappedDecryptOperation`). Chain the audit metadata onto it. + * + * NOT every client this package accepts does that. `WasmEncryptionClient` + * (`@cipherstash/stack/wasm-inline` — the documented entry for Deno, Workers + * and Supabase Edge Functions) returns a bare promise from decrypt, so it takes + * the branch below and its audit metadata is dropped. It ships in this package + * and satisfies `DynamoDBEncryptionClient` structurally, so it is accepted + * without a cast (#772 review, finding 10). */ export async function resolveDecryptResult( operation: unknown, @@ -103,12 +108,11 @@ export async function resolveDecryptResult( } if (typeof chainable?.audit !== 'function' && auditData.metadata) { - // Every client this package ships carries `.audit()` on decrypt, so this - // only fires for a custom client whose decrypt returns something else — - // there is then nowhere to put the metadata. Make the drop observable - // rather than silent. + // Reached by the wasm-inline client (bare promise, no `.audit()`) and by + // any custom client whose decrypt returns something else. There is nowhere + // to put the metadata, so make the drop observable rather than silent. logger.debug( - "DynamoDB: decrypt audit metadata ignored — this client's decrypt does not return a chainable operation with .audit(). Audited decrypts need a client built with Encryption({ schemas }).", + "DynamoDB: decrypt audit metadata ignored — this client's decrypt does not return a chainable operation with .audit(). Audited decrypts need a client from the default @cipherstash/stack entry; the wasm-inline client's decrypt returns a plain promise.", ) } diff --git a/packages/stack/src/dynamodb/index.ts b/packages/stack/src/dynamodb/index.ts index a950c8f7..927d5a6a 100644 --- a/packages/stack/src/dynamodb/index.ts +++ b/packages/stack/src/dynamodb/index.ts @@ -41,7 +41,24 @@ function assertClientTableVersionMatch( table: AnyEncryptedTable, ): void { // Only v3 tables carry the strict wire-format requirement this guards. - if (!isV3Table(table)) return + if (!isV3Table(table)) { + // The v2 read path calls `decryptModel(item)` with NO table on purpose — + // a v2 table means nothing to a v3 client's reconstructor map. That is + // fine for the native clients, which derive the table from the payloads, + // and impossible for the WASM client, whose decrypt requires the table and + // otherwise throws a TypeError about `tableName` from deep inside + // `requireTable`. Refuse the pairing here, where the message can name it + // (#772 review, finding 10). + if ( + (encryptionClient as { requiresTableForDecrypt?: boolean }) + .requiresTableForDecrypt + ) { + throw new Error( + `encryptedDynamoDB: the @cipherstash/stack/wasm-inline client cannot read legacy EQL v2 items. Its decrypt requires the table, and a v2 table carries none of the information it needs — so "${table.tableName}" would fail at the first read. Use the default @cipherstash/stack entry for tables that still hold EQL v2 items, or migrate the table to an EQL v3 schema (types.* domains) and pass that.`, + ) + } + return + } const getEncryptConfig = ( encryptionClient as { diff --git a/packages/stack/src/dynamodb/types.ts b/packages/stack/src/dynamodb/types.ts index cfe37bb6..d521f140 100644 --- a/packages/stack/src/dynamodb/types.ts +++ b/packages/stack/src/dynamodb/types.ts @@ -37,12 +37,16 @@ export type AnyEncryptedTable = * nominal `TypedEncryptionClient` parameter would reject a client built for * a narrower schema tuple. * - * Both clients now return a chainable operation on the decrypt paths — the + * Both NATIVE clients return a chainable operation on the decrypt paths — the * nominal client's `DecryptModelOperation` and the typed wrapper's * `MappedDecryptOperation` each carry `.audit()` (the typed wrapper also takes * the table as a second argument). The operation classes handle both; see - * `DecryptModelOperation` and `resolveDecryptResult`. Audit metadata on decrypt - * is therefore forwarded regardless of which client shape is supplied. + * `DecryptModelOperation` and `resolveDecryptResult`. + * + * The wasm-inline client does not: its decrypt is a plain `async` method, so + * audit metadata is dropped (observably — `resolveDecryptResult` logs it) and + * its EQL v2 read path is refused outright by `assertClientTableVersionMatch`, + * because that path relies on calling decrypt WITHOUT a table. */ export type DynamoDBEncryptionClient = { encryptModel(input: never, table: never): unknown diff --git a/packages/stack/src/wasm-inline.ts b/packages/stack/src/wasm-inline.ts index 0b7e8966..05a3a97b 100644 --- a/packages/stack/src/wasm-inline.ts +++ b/packages/stack/src/wasm-inline.ts @@ -663,6 +663,21 @@ const INTERNAL_CONSTRUCT = Symbol('cs-wasm-client') * prevent callers from wrapping arbitrary objects in this type. */ export class WasmEncryptionClient { + /** + * This client's `decryptModel` / `bulkDecryptModels` REQUIRE the table — they + * resolve date fields from a per-table map and throw without it. The native + * clients derive the table from the payloads instead, so callers that hold a + * client structurally cannot tell the two apart. + * + * `encryptedDynamoDB` is the caller that cares: its legacy EQL v2 read path + * deliberately omits the table (a v2 table means nothing to a v3 + * reconstructor map), which reached `requireTable` with `undefined` and threw + * a TypeError about `tableName` pointing nowhere near the cause. Declared + * rather than sniffed so the check is a stated capability, not a guess about + * arity or constructor name (#772 review, finding 10). + */ + readonly requiresTableForDecrypt = true + /** @internal */ private readonly client: unknown