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/.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/] } }, + }, +}) 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