From a3997b7996639c498913696658b9744b92da0314 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Sat, 25 Jul 2026 14:37:17 +1000 Subject: [PATCH 1/4] 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 5b8da9735..71ff378a6 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 000000000..82a356e11 --- /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 4134c57ce..550087366 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 16ba56d2e..d8bf48d0b 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 e9fc828b6..a145c6f6e 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 000000000..fa01c8c55 --- /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/4] 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 000000000..1e52ae725 --- /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 d8f1bc872..c6ab542ec 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 38c62d099..186e36490 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 a950c8f7c..927d5a6ac 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 cfe37bb68..d521f1405 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 0b7e89663..05a3a97b7 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 From b953364c2bfeaa98deba614d63ef935cc46321a0 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Mon, 27 Jul 2026 13:27:34 +1000 Subject: [PATCH 3/4] fix(stack): stop the wasm-inline client failing every DynamoDB v3 write (#788 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the three review findings on #788, plus a real defect found while verifying the second one. The wasm+v2 guard message is now operation-neutral. `assertClientTableVersionMatch` runs on all four operations, so a plain-JS caller reaching `encryptModel` with a v2 table got "cannot read legacy EQL v2 items ... would fail at the first read" — naming an operation that never ran. The pairing is wrong in both directions anyway (`Encryption()` on that entry rejects a v2 schema), so the message now says so instead of describing a read. Verifying that finding surfaced the larger one: `encryptModel` / `bulkEncryptModels` chained `.audit()` onto the client's result unconditionally. The native clients return a thenable operation carrying it; the wasm-inline client's encrypt returns a bare `Promise` and has no `.audit()` anywhere. So EVERY EQL v3 write through this adapter on that entry failed with `client.encryptModel(...).audit is not a function` — surfaced as a `DYNAMODB_ENCRYPTION_ERROR`, indistinguishable from a genuine encryption fault. This PR's own changeset claimed the opposite ("EQL v3 tables are unaffected ... the wasm-inline client keeps working there"); it was true of decrypt only. `resolveEncryptResult` mirrors the existing `resolveDecryptResult`: tolerate the bare promise, drop audit metadata observably rather than crashing, and fail closed on a malformed result so an unencrypted item can never pass through as a success. The changeset and the `types.ts` docblock are corrected to match. Regression tests are credential-free. The chainable half matters most — the native clients' encrypt audit trail had no coverage outside a live-ZeroKMS suite, so nothing would have caught breaking it. skills/stash-dynamodb documented v2 decrypt as unconditionally supported and never mentioned that wasm-inline refuses v2 tables, on the documented entry for the runtimes most often paired with DynamoDB. It also claimed audit metadata forwards "regardless of client shape" — the exact phrase this PR removed from the source as untrue. Both corrected, plus the wasm-inline row in skills/stash-encryption, which never said the entry is v3-only. packages/bench/package.json is back to 2-space indent, keeping only the substantive `test:unit` addition: 53 changed lines down to 1. The repo's Biome config is `indentStyle: "space"` and the file was spaces on main, so the churn was neither required nor conformant — Biome ignores `**/package.json` entirely, verified by direct invocation. --- .changeset/dynamodb-skill-wasm-caveat.md | 17 +++ .changeset/dynamodb-wasm-v2-read.md | 16 ++- packages/bench/package.json | 54 ++++---- .../dynamodb/resolve-decrypt.test.ts | 120 ++++++++++++++++-- .../dynamodb/v2-table-forwarding.test.ts | 109 +++++++++++++++- packages/stack/src/dynamodb/helpers.ts | 58 +++++++++ packages/stack/src/dynamodb/index.ts | 10 +- .../operations/bulk-encrypt-models.ts | 13 +- .../src/dynamodb/operations/encrypt-model.ts | 11 +- packages/stack/src/dynamodb/types.ts | 19 ++- skills/stash-dynamodb/SKILL.md | 18 ++- skills/stash-encryption/SKILL.md | 4 +- 12 files changed, 391 insertions(+), 58 deletions(-) create mode 100644 .changeset/dynamodb-skill-wasm-caveat.md diff --git a/.changeset/dynamodb-skill-wasm-caveat.md b/.changeset/dynamodb-skill-wasm-caveat.md new file mode 100644 index 000000000..21d3f80ba --- /dev/null +++ b/.changeset/dynamodb-skill-wasm-caveat.md @@ -0,0 +1,17 @@ +--- +'stash': patch +--- + +The `stash-dynamodb` and `stash-encryption` skills documented EQL v2 decrypt as +unconditionally supported, without noting that `@cipherstash/stack/wasm-inline` +is EQL v3 only. Since that is the documented entry for Deno, Bun, Cloudflare +Workers and Supabase Edge Functions — runtimes commonly paired with DynamoDB — a +reader following the skill hit a runtime refusal with no forewarning. Both skills +now state that legacy v2 items are readable on the native `@cipherstash/stack` +entry only. + +The `stash-dynamodb` API reference also claimed audit metadata forwards to +ZeroKMS "regardless of client shape". It does not: the wasm-inline client's +operations return a plain promise with no `.audit()`, so its audit metadata is +dropped (logged at debug). The reference now says so, and says the operation +still succeeds. diff --git a/.changeset/dynamodb-wasm-v2-read.md b/.changeset/dynamodb-wasm-v2-read.md index 1e52ae725..b75673051 100644 --- a/.changeset/dynamodb-wasm-v2-read.md +++ b/.changeset/dynamodb-wasm-v2-read.md @@ -17,8 +17,20 @@ 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. +combination and the fix. The message is operation-neutral: the guard runs on all +four operations, so a plain-JS caller reaching the write path with a v2 table +gets a message that does not claim a read it never attempted. + +`encryptModel` / `bulkEncryptModels` now also tolerate a client whose encrypt +returns a plain promise. They chained `.audit()` onto the result +unconditionally, which the wasm-inline client does not carry — so **every EQL v3 +write through this adapter on that entry** failed with +`client.encryptModel(...).audit is not a function`, surfaced as a +`DYNAMODB_ENCRYPTION_ERROR` and so indistinguishable from a real encryption +fault. The read path already handled this; the write path now matches, via the +same fail-closed check that rejects a malformed result rather than passing an +unencrypted item through as a success. Audit metadata still has nowhere to go on +that client, so it is dropped and logged at debug — the write itself succeeds. Three comments in this package claimed audit metadata was forwarded "regardless of client shape" and that "every client this package ships carries `.audit()` on diff --git a/packages/bench/package.json b/packages/bench/package.json index 550087366..46ef39c61 100644 --- a/packages/bench/package.json +++ b/packages/bench/package.json @@ -1,29 +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", - "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" - } + "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/stack/__tests__/dynamodb/resolve-decrypt.test.ts b/packages/stack/__tests__/dynamodb/resolve-decrypt.test.ts index 92044eb78..f3ce936ae 100644 --- a/packages/stack/__tests__/dynamodb/resolve-decrypt.test.ts +++ b/packages/stack/__tests__/dynamodb/resolve-decrypt.test.ts @@ -1,15 +1,28 @@ /** - * Pure unit tests for the two client-shape helpers behind the DynamoDB adapter's - * decrypt path. Both shipped clients — nominal `EncryptionClient` and the typed - * EQL v3 client (whose decrypt returns a `MappedDecryptOperation`) — are - * chainable and carry `.audit()`; the bare-promise branch remains only for a - * non-conforming custom client. Every branch was previously reachable only - * through a live ZeroKMS decrypt; these move that assurance onto the pure CI - * lane. No credentials, no network. + * Pure unit tests for the client-shape helpers behind the DynamoDB adapter's + * decrypt AND encrypt paths. + * + * On decrypt, both native clients — nominal `EncryptionClient` and the typed EQL + * v3 client (whose decrypt returns a `MappedDecryptOperation`) — are chainable + * and carry `.audit()`; the bare-promise branch is taken by the wasm-inline + * client and by a non-conforming custom one. + * + * The same split exists on encrypt, and only the decrypt half handled it: the + * encrypt operations chained `.audit()` unconditionally, so the wasm-inline + * client failed every write (#788 review follow-up). `resolveEncryptResult` is + * the mirror, and the chainable half of its coverage matters most — the native + * clients' encrypt audit trail has no other credential-free test. + * + * Every branch was previously reachable only through live ZeroKMS; these move + * that assurance onto the pure CI lane. No credentials, no network. */ import type { Result } from '@byteslice/result' import { afterEach, describe, expect, it, vi } from 'vitest' -import { resolveDecryptResult, throwPreservingCode } from '@/dynamodb/helpers' +import { + resolveDecryptResult, + resolveEncryptResult, + throwPreservingCode, +} from '@/dynamodb/helpers' import { EncryptionOperation } from '@/encryption/operations/base-operation' import { MappedDecryptOperation } from '@/encryption/operations/mapped-decrypt' import { type EncryptionError, EncryptionErrorTypes } from '@/errors' @@ -178,6 +191,97 @@ describe('resolveDecryptResult', () => { }) }) +/** + * The write-path mirror (#788 review follow-up). The encrypt operations used + * to chain `.audit()` unconditionally, so a client returning a bare promise + * (the wasm-inline entry) failed every encrypt with + * `.audit is not a function`. These pin BOTH directions: the chainable path + * must still forward metadata — the native clients' audit trail depends on it, + * and its only other coverage is a live-credential suite — and the bare path + * must resolve instead of throwing. + */ +describe('resolveEncryptResult', () => { + it('chains .audit and forwards metadata when present (native clients)', async () => { + let seen: unknown + let calls = 0 + const operation = { + audit(config: { metadata?: Record }) { + calls += 1 + seen = config.metadata + return Promise.resolve({ data: { encrypted: true } }) + }, + } + + const result = await resolveEncryptResult( + operation, + { metadata: { sub: 'u1' } }, + 'encryptModel', + ) + + expect(result).toEqual({ data: { encrypted: true } }) + expect(seen).toEqual({ sub: 'u1' }) + expect(calls).toBe(1) + }) + + it('awaits a bare promise instead of throwing (wasm-inline encrypt)', async () => { + const result = await resolveEncryptResult( + Promise.resolve({ data: { encrypted: true } }), + { metadata: { dropped: true } }, + 'encryptModel', + ) + + expect(result).toEqual({ data: { encrypted: true } }) + }) + + it('propagates a failure result unchanged', async () => { + const failure = { failure: { message: 'boom', code: 'X' } } + + await expect( + resolveEncryptResult(Promise.resolve(failure), {}, 'bulkEncryptModels'), + ).resolves.toEqual(failure) + }) + + it('returns a failure — not a fake success — for a malformed result', async () => { + // Fail closed. A bare value cast straight through would hand the caller an + // "encrypted" item that was never encrypted. + for (const malformed of [{}, 42, undefined]) { + const result = await resolveEncryptResult( + Promise.resolve(malformed), + {}, + 'encryptModel', + ) + + expect(result.failure).toBeDefined() + expect(result.data).toBeUndefined() + } + }) + + it('names the operation in the dropped-metadata log, and stays silent without metadata', async () => { + const spy = vi.spyOn(logger, 'debug').mockImplementation(() => {}) + + try { + await resolveEncryptResult( + Promise.resolve({ data: {} }), + { metadata: { m: 1 } }, + 'bulkEncryptModels', + ) + expect(spy).toHaveBeenCalledWith( + expect.stringContaining('bulkEncryptModels audit metadata ignored'), + ) + + spy.mockClear() + await resolveEncryptResult( + Promise.resolve({ data: {} }), + {}, + 'encryptModel', + ) + expect(spy).not.toHaveBeenCalled() + } finally { + spy.mockRestore() + } + }) +}) + describe('throwPreservingCode', () => { it('rethrows as an Error carrying the FFI code', () => { try { diff --git a/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts b/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts index c6ab542ec..ce9e87cdd 100644 --- a/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts +++ b/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts @@ -133,7 +133,13 @@ describe('a client whose decrypt requires the table', () => { "Cannot read properties of undefined (reading 'tableName')", ) } - return Promise.resolve({ data: {} }) + // A bare promise — NOT a thenable operation with `.audit()`. That is + // the whole point of this stub: it is the shape `WasmEncryptionClient` + // returns from `wasmResult`. The bulk methods resolve to an array, + // index-aligned with their input, as the real client does. + return Promise.resolve( + method.startsWith('bulk') ? { data: [{}] } : { data: {} }, + ) } const client = { requiresTableForDecrypt: true, @@ -156,7 +162,7 @@ describe('a client whose decrypt requires the table', () => { // 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/, + /wasm-inline client cannot be paired with the legacy EQL v2 table/, ) // Refused before the client is touched, so the user never sees the // TypeError about `tableName`. @@ -168,10 +174,41 @@ describe('a client whose decrypt requires the table', () => { const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) expect(() => dynamo.bulkDecryptModels([{ pk: 'a' }], usersV2)).toThrow( - /wasm-inline client cannot read legacy EQL v2 items/, + /wasm-inline client cannot be paired with the legacy EQL v2 table/, ) }) + /** + * #788 review, minor finding. + * + * The guard runs on all four operations, not just the two read ones, so a + * plain-JS caller reaching the write path with a v2 table hits the SAME + * message. It must therefore not be phrased for reads only — "would fail at + * the first read" names an operation that never ran. + * + * Typed callers cannot get here (the write overloads are `AnyV3Table`-only, + * pinned by `client-compat.test-d.ts`), so this is about the message a JS + * caller or a cast lands on, not about reachable behaviour changing. + */ + it('is refused on the v2 WRITE path, with a message that does not claim a read', () => { + const { calls, client } = wasmShapedClient([]) + const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) + + for (const call of [ + () => dynamo.encryptModel({ pk: 'a' } as never, usersV2 as never), + () => dynamo.bulkEncryptModels([{ pk: 'a' }] as never, usersV2 as never), + ]) { + expect(call).toThrow( + /wasm-inline client cannot be paired with the legacy EQL v2 table/, + ) + // The read-path phrasing must not survive on a write. + expect(call).not.toThrow(/would fail at the first read/) + expect(call).not.toThrow(/cannot read legacy EQL v2 items/) + } + + expect(calls).toHaveLength(0) + }) + // 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 () => { @@ -183,4 +220,70 @@ describe('a client whose decrypt requires the table', () => { expect(calls).toHaveLength(1) expect(calls[0]?.argCount).toBe(2) }) + + /** + * #788 review follow-up: the same "v3 tables are unaffected" promise, on the + * WRITE path. + * + * The encrypt operations chained `.audit()` onto the client's result + * unconditionally. The native clients return a thenable operation carrying + * it; `WasmEncryptionClient.encryptModel` returns a plain + * `Promise` from `wasmResult` (it has no `.audit()` anywhere), + * so every v3 encrypt through this adapter died with + * `client.encryptModel(...).audit is not a function` — surfaced as a + * `DYNAMODB_ENCRYPTION_ERROR` failure, not a crash, so it read as a genuine + * encryption fault. The decrypt path already tolerates the bare promise via + * `resolveDecryptResult`; the write path must match. + */ + it('encrypts an EQL v3 table even though its encrypt returns a bare promise', async () => { + const { calls, client } = wasmShapedClient(['users_v3']) + const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) + + const single = await dynamo.encryptModel({ pk: 'a' } as never, usersV3) + expect(single.failure).toBeUndefined() + + const bulk = await dynamo.bulkEncryptModels([{ pk: 'a' }] as never, usersV3) + expect(bulk.failure).toBeUndefined() + + expect(calls.map((c) => c.method)).toEqual([ + 'encryptModel', + 'bulkEncryptModels', + ]) + }) + + /** + * Audit metadata has nowhere to go on this client shape, so it is dropped — + * but the encrypt must still succeed rather than failing the whole write. + * Mirrors the decrypt path's documented behaviour. + */ + it('drops audit metadata on that client rather than failing the encrypt', async () => { + const { client } = wasmShapedClient(['users_v3']) + const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) + + const result = await dynamo + .encryptModel({ pk: 'a' } as never, usersV3) + .audit({ metadata: { requestId: 'r-1' } }) + + expect(result.failure).toBeUndefined() + }) + + /** + * The tolerance must not swallow a malformed result into a fake success — + * the same guard `resolveDecryptResult` applies on read. + */ + it('rejects a bare encrypt result that is not { data } or { failure }', async () => { + const client = { + requiresTableForDecrypt: true, + getEncryptConfig: () => ({ v: 1, tables: { users_v3: {} } }), + encryptModel: () => Promise.resolve('not-a-result'), + bulkEncryptModels: () => Promise.resolve('not-a-result'), + decryptModel: () => Promise.resolve({ data: {} }), + bulkDecryptModels: () => Promise.resolve({ data: {} }), + } + const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) + + const result = await dynamo.encryptModel({ pk: 'a' } as never, usersV3) + + expect(result.failure?.message).toMatch(/malformed result/) + }) }) diff --git a/packages/stack/src/dynamodb/helpers.ts b/packages/stack/src/dynamodb/helpers.ts index 186e36490..538091d31 100644 --- a/packages/stack/src/dynamodb/helpers.ts +++ b/packages/stack/src/dynamodb/helpers.ts @@ -144,6 +144,64 @@ export async function resolveDecryptResult( type DecryptFailure = { message: string; code?: string } +/** + * Resolve an encrypt call against either client shape — the write-path mirror + * of {@link resolveDecryptResult}. + * + * Both paths face the same split, and only the read one handled it. The native + * clients return a thenable operation carrying `.audit()`; the WASM client's + * `encryptModel` / `bulkEncryptModels` return a plain `Promise` + * from `wasmResult`, with no `.audit()` on this entry at all. Chaining it + * unconditionally threw `client.encryptModel(...).audit is not a function`, + * which `withResult` caught and reported as a `DYNAMODB_ENCRYPTION_ERROR` — + * so every v3 write through this adapter on the wasm entry looked like a + * genuine encryption fault (#788 review follow-up). + * + * Audit metadata still has nowhere to go on that shape, so it is dropped, and + * the drop is logged rather than silent — exactly as on decrypt. + */ +export async function resolveEncryptResult( + operation: unknown, + auditData: { metadata?: Record }, + context: 'encryptModel' | 'bulkEncryptModels', +): Promise< + { data: T; failure?: never } | { data?: never; failure: DecryptFailure } +> { + const chainable = operation as { + audit?: (data: { metadata?: Record }) => unknown + } + + if (typeof chainable?.audit !== 'function' && auditData.metadata) { + logger.debug( + `DynamoDB: ${context} audit metadata ignored — this client's encrypt does not return a chainable operation with .audit(). Audited encrypts need a client from the default @cipherstash/stack entry; the wasm-inline client's encrypt returns a plain promise.`, + ) + } + + const resolved = + typeof chainable?.audit === 'function' + ? await chainable.audit(auditData) + : await operation + + // Same fail-closed check the read path applies: a bare value has neither + // `data` nor `failure`, and casting it through would surface a fake success + // carrying `undefined` — here, an "encrypted" item that was never encrypted. + if ( + resolved === null || + typeof resolved !== 'object' || + (!('data' in resolved) && !('failure' in resolved)) + ) { + return { + failure: { + message: `DynamoDB: ${context} returned a malformed result — expected { data } or { failure }.`, + }, + } + } + + return resolved as + | { data: T; failure?: never } + | { data?: never; failure: DecryptFailure } +} + /** * Rethrow a Result failure as an `Error` that preserves the FFI error code. * `withResult`'s `ensureError` wraps non-Error objects, which would otherwise diff --git a/packages/stack/src/dynamodb/index.ts b/packages/stack/src/dynamodb/index.ts index 927d5a6ac..cfe39096f 100644 --- a/packages/stack/src/dynamodb/index.ts +++ b/packages/stack/src/dynamodb/index.ts @@ -49,12 +49,20 @@ function assertClientTableVersionMatch( // otherwise throws a TypeError about `tableName` from deep inside // `requireTable`. Refuse the pairing here, where the message can name it // (#772 review, finding 10). + // + // The message is deliberately operation-NEUTRAL. This guard runs on all + // four operations, so a plain-JS caller (the write overloads are + // `AnyV3Table`-only, so TypeScript never reaches this) hits it on encrypt + // too — where "would fail at the first read" names an operation that never + // ran. The pairing is wrong in both directions anyway: `Encryption()` on + // that entry rejects a v2 schema outright, so the client can never hold + // this table (#788 review). 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.`, + `encryptedDynamoDB: the @cipherstash/stack/wasm-inline client cannot be paired with the legacy EQL v2 table "${table.tableName}". That entry is EQL v3 only — Encryption() there rejects a v2 schema, and its model operations require a table it was initialized with — so both encrypt and decrypt would fail on this table. 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 diff --git a/packages/stack/src/dynamodb/operations/bulk-encrypt-models.ts b/packages/stack/src/dynamodb/operations/bulk-encrypt-models.ts index eafee15aa..063d83545 100644 --- a/packages/stack/src/dynamodb/operations/bulk-encrypt-models.ts +++ b/packages/stack/src/dynamodb/operations/bulk-encrypt-models.ts @@ -5,6 +5,7 @@ import { deepClone, handleError, isV3Table, + resolveEncryptResult, throwPreservingCode, toEncryptedDynamoItem, } from '../helpers' @@ -43,12 +44,16 @@ export class BulkEncryptModelsOperation< return await withResult( async () => { const client = this.encryptionClient as CallableEncryptionClient - const encryptResult = await client - .bulkEncryptModels( + const encryptResult = await resolveEncryptResult< + Record[] + >( + client.bulkEncryptModels( this.items.map((item) => deepClone(item)), this.table, - ) - .audit(this.getAuditData()) + ), + this.getAuditData(), + 'bulkEncryptModels', + ) if (encryptResult.failure) { throwPreservingCode(encryptResult.failure) diff --git a/packages/stack/src/dynamodb/operations/encrypt-model.ts b/packages/stack/src/dynamodb/operations/encrypt-model.ts index 56a56ea47..18c5840a5 100644 --- a/packages/stack/src/dynamodb/operations/encrypt-model.ts +++ b/packages/stack/src/dynamodb/operations/encrypt-model.ts @@ -5,6 +5,7 @@ import { deepClone, handleError, isV3Table, + resolveEncryptResult, throwPreservingCode, toEncryptedDynamoItem, } from '../helpers' @@ -43,9 +44,13 @@ export class EncryptModelOperation< return await withResult( async () => { const client = this.encryptionClient as CallableEncryptionClient - const encryptResult = await client - .encryptModel(deepClone(this.item), this.table) - .audit(this.getAuditData()) + const encryptResult = await resolveEncryptResult< + Record + >( + client.encryptModel(deepClone(this.item), this.table), + this.getAuditData(), + 'encryptModel', + ) if (encryptResult.failure) { throwPreservingCode(encryptResult.failure) diff --git a/packages/stack/src/dynamodb/types.ts b/packages/stack/src/dynamodb/types.ts index d521f1405..df3533f9e 100644 --- a/packages/stack/src/dynamodb/types.ts +++ b/packages/stack/src/dynamodb/types.ts @@ -37,16 +37,23 @@ export type AnyEncryptedTable = * nominal `TypedEncryptionClient` parameter would reject a client built for * a narrower schema tuple. * - * Both NATIVE clients return a chainable operation on the decrypt paths — the - * nominal client's `DecryptModelOperation` and the typed wrapper's + * Both NATIVE clients return a chainable operation on every path — 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`. * - * 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. + * The wasm-inline client does not, on EITHER path: its encrypt and decrypt are + * plain `async` methods returning a bare `Promise`, so audit + * metadata is dropped (observably — `resolveDecryptResult` and + * `resolveEncryptResult` log it). Chaining `.audit()` unconditionally is + * therefore a bug, not just a lost audit record; the encrypt path made exactly + * that mistake and failed every v3 write on this entry (#788 review follow-up). + * + * Its EQL v2 path is refused outright by `assertClientTableVersionMatch` — the + * v2 read relies on calling decrypt WITHOUT a table, and that entry's + * `Encryption()` rejects a v2 schema anyway, so the pairing is wrong in both + * directions. */ export type DynamoDBEncryptionClient = { encryptModel(input: never, table: never): unknown diff --git a/skills/stash-dynamodb/SKILL.md b/skills/stash-dynamodb/SKILL.md index af53d7fca..d6ba5ed19 100644 --- a/skills/stash-dynamodb/SKILL.md +++ b/skills/stash-dynamodb/SKILL.md @@ -52,9 +52,15 @@ Non-encrypted attributes pass through unchanged. On decryption, the `__source` a | Schema | `encryptedTable` + `types.*` | `encryptedTable` + `encryptedColumn` | | Client | `Encryption({ schemas })` (or the deprecated `EncryptionV3`) | `Encryption({ schemas })` | | encrypt | ✅ | ❌ removed — write is v3 only | -| decrypt | ✅ | ✅ reads stored v2 items | +| decrypt | ✅ | ✅ reads stored v2 items — **native entry only** | | Nested fields | Flat dotted path (`"profile.ssn"`) | Nested group + `encryptedField` | +> **v2 reads need the native entry.** A client from `@cipherstash/stack/wasm-inline` +> (the documented entry for Deno, Bun, Cloudflare Workers and Supabase Edge Functions) +> is EQL v3 only: `encryptedDynamoDB` throws at the call site if you pass it a v2 +> table. Read legacy v2 items with a client from the default `@cipherstash/stack` +> entry. EQL v3 tables work on both entries. + There is no infrastructure migration between the versions — DynamoDB has no EQL extension to install and no schema to alter — and there is no automatic *data* migration either. The two use **different wire formats**; a stored v2 item still decrypts through a v2 table, but new writes are v3. To fully move a table to v3, re-encrypt every item with a v3 schema. The client must be built for the table. Build the client with the same v3 table you hand to `encryptedDynamoDB` — `Encryption({ schemas: [users] })` returns the typed v3 client for a concrete v3 schema set. Passing a v3 table to a client that never registered it (a client built for a different schema set) throws a clear error naming the table on the first operation, instead of failing later with an opaque FFI deserialization error. @@ -181,6 +187,14 @@ A v2 table is **read-only** through this adapter now: pass it to `decryptModel` `bulkEncryptModels` no longer accept a v2 table — author new writes with an EQL v3 table. +**Not on the WASM entry.** This whole section requires a client from the default +`@cipherstash/stack` entry. `@cipherstash/stack/wasm-inline` is EQL v3 only — +`Encryption()` there rejects a v2 schema outright, and `encryptedDynamoDB` refuses +the pairing at the call site with a message naming the fix. So on Deno, Bun, +Workers and Supabase Edge Functions, legacy v2 items are not readable through this +adapter; re-encrypt them to a v3 schema, or read them from a Node process using the +native entry. + ```typescript import { encryptedTable, encryptedColumn, encryptedField } from "@cipherstash/stack/schema" import { Encryption } from "@cipherstash/stack" @@ -497,7 +511,7 @@ Let `T` be inferred from the argument; do not pass explicit type arguments on th | `decryptModel` | `(item: Record, v2Table)` | `T` | | `bulkDecryptModels` | `(items: Record[], v2Table)` | `T[]` | -All operations are thenable (awaitable) and support `.audit({ metadata })` chaining — including the decrypt methods, whose metadata now forwards to ZeroKMS regardless of client shape (see the Setup note). +All operations are thenable (awaitable) and support `.audit({ metadata })` chaining. On the default `@cipherstash/stack` entry the metadata forwards to ZeroKMS on every operation, encrypt and decrypt alike (see the Setup note). The `@cipherstash/stack/wasm-inline` client has no `.audit()` — its operations return a plain promise — so audit metadata is **dropped** there (logged at debug level). The operation itself still succeeds; only the audit record is lost. Use the native entry when audit trails matter. Types exported from `@cipherstash/stack/dynamodb`: `EncryptedDynamoDBInstance`, `EncryptedDynamoDBConfig`, `EncryptedDynamoDBError`, `AnyEncryptedTable`, `DynamoDBEncryptionClient`, `EncryptedAttributes`, `DecryptedAttributes`, `AuditConfig`. diff --git a/skills/stash-encryption/SKILL.md b/skills/stash-encryption/SKILL.md index 3c780c1cb..11c3d8093 100644 --- a/skills/stash-encryption/SKILL.md +++ b/skills/stash-encryption/SKILL.md @@ -190,8 +190,8 @@ The SDK never logs plaintext data. | `@cipherstash/stack/types` | All TypeScript types | | `@cipherstash/stack-drizzle` | Drizzle ORM integration for EQL v3 schemas — the package root, EQL v3 only (see the `stash-drizzle` skill) | | `@cipherstash/stack-supabase` | `encryptedSupabase` wrapper for Supabase — EQL v3 only (see the `stash-supabase` skill) | -| `@cipherstash/stack/wasm-inline` | The **edge** entry — Deno, Bun, Cloudflare Workers, Supabase Edge Functions. Its own `Encryption` factory plus the v3 authoring surface, `EncryptionErrorTypes`, and the WASM build of protect-ffi inlined into the bundle. No native binding, so no bundler externalisation needed. | -| `@cipherstash/stack/dynamodb` | `encryptedDynamoDB` — encrypt/write is **EQL v3 only** (`types.*`); decrypt still reads existing v2 items. See the `stash-dynamodb` skill | +| `@cipherstash/stack/wasm-inline` | The **edge** entry — Deno, Bun, Cloudflare Workers, Supabase Edge Functions. Its own `Encryption` factory plus the v3 authoring surface, `EncryptionErrorTypes`, and the WASM build of protect-ffi inlined into the bundle. No native binding, so no bundler externalisation needed. **EQL v3 only** — `Encryption()` here rejects a v2 schema, and its operations return plain Results with no `.audit()` or `.withLockContext()` chaining. | +| `@cipherstash/stack/dynamodb` | `encryptedDynamoDB` — encrypt/write is **EQL v3 only** (`types.*`); decrypt still reads existing v2 items, on the native entry only. See the `stash-dynamodb` skill | | `@cipherstash/stack/schema`, `@cipherstash/stack/client`, `@cipherstash/stack/encryption` | Legacy v2 schema builders and client surface — see "Legacy: EQL v2" below | ## Schema Definition From 687173a730c101a319a608fd32b20c2ac6585be6 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Mon, 27 Jul 2026 14:25:02 +1000 Subject: [PATCH 4/4] fix(stack,bench): stop the client type asserting the .audit() it just disproved (#788 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the #788 review of this branch. The largest item is that the fix landed at runtime but the TYPE still declared the invariant it disproves. `CallableEncryptionClient.encryptModel` / `bulkEncryptModels` returned `ChainableEncryptOperation`, declaring an `.audit()` that the wasm-inline entry does not have — precisely the assumption that let the write path chain it unconditionally and fail every EQL v3 write there. The decrypt members already returned `unknown` with a docblock explaining why; encrypt never got the same treatment. Both are now `unknown`, `ChainableEncryptOperation` is deleted (it had no other reference), and the docblock covers all four members. This is not cosmetic: re-introducing `client.encryptModel(...).audit(...)` now fails to compile with TS2571, verified by reverting the call site under tsc. The regression is statically unreachable rather than merely fixed. The type is internal — absent from every emitted .d.ts — so there is no API change and no changeset. The test I had described as load-bearing was the weakest one. It stubbed `audit()` returning a promise; the native contract is `audit(): this` on a thenable whose `then()` calls `execute()`, so metadata is read back off the operation at execution time rather than passed forward. That stub passes even if `audit()` stops recording entirely. It now subclasses the real `EncryptionOperation`, mirroring what the decrypt half of the file already did, and asserts the metadata reaches `execute()` and that the operation runs exactly once. A native-failure case covers the other arm. Also strengthened, all verified by mutation (each kills exactly 2 tests): - the audit-drop test asserted only that the result succeeded — it now asserts the drop is logged and names the entry to switch to, plus a mirror proving a chainable client does NOT trip the drop path - both malformed-result loops gained `null` and `[]`. `null` makes the `resolved === null` clause load-bearing (without it `'data' in null` throws); `[]` is `typeof 'object'` so it must be rejected on the key checks `DecryptFailure` is renamed `ResultFailure` — it was already the encrypt helper's failure type, structurally identical, wrong name. bench: the `BenchPlaintextRow` docblock claimed it was "derived from the table so the two cannot drift again". It is hand-written. Deriving it was investigated and is actively worse: `InferInsertModel` describes the ENCRYPTED column and degrades to optional `any`, and `extractEncryptionSchema` returns the widened `AnyV3Table` whose index-signature column map admits both `encTxt: 'x'` and `encText: 12345`, which the literal rejects. The comment now says so and names the unit test as the real guard. The credential-free bench step moved ahead of `docker compose up`: leaving it last meant a database failure would skip the one check whose entire rationale is that it cannot be skipped for want of a database. Its `server.deps.inline` for test-kit was inert — nothing on that path imports it — and is gone; verified still passing with DATABASE_URL and all four CS_* vars unset. --- .github/workflows/tests-bench.yml | 22 ++--- packages/bench/src/drizzle/setup.ts | 12 ++- packages/bench/vitest.unit.config.ts | 1 - .../dynamodb/client-compat.test-d.ts | 35 ++++++++ .../dynamodb/resolve-decrypt.test.ts | 84 ++++++++++++++----- .../dynamodb/v2-table-forwarding.test.ts | 81 +++++++++++++++--- packages/stack/src/dynamodb/helpers.ts | 16 ++-- packages/stack/src/dynamodb/types.ts | 30 ++++--- 8 files changed, 218 insertions(+), 63 deletions(-) diff --git a/.github/workflows/tests-bench.yml b/.github/workflows/tests-bench.yml index 71ff378a6..488b03ba4 100644 --- a/.github/workflows/tests-bench.yml +++ b/.github/workflows/tests-bench.yml @@ -68,6 +68,18 @@ jobs: - name: Build stack + adapter packages run: pnpm exec turbo run build --filter @cipherstash/stack --filter @cipherstash/stack-drizzle + # Deliberately BEFORE Postgres. Separate config, no globalSetup: these + # need neither a database nor credentials, so they must not be reachable + # only after the steps that do — a docker failure below would otherwise + # skip the one check whose whole rationale is that it cannot be skipped. + # The seed keying they check was wrong for the entire 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 + # Service-scoped `postgres`: local/docker-compose.yml also defines a # PostgREST that bench never talks to. # @@ -88,13 +100,3 @@ 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/src/drizzle/setup.ts b/packages/bench/src/drizzle/setup.ts index d8bf48d0b..df9697eed 100644 --- a/packages/bench/src/drizzle/setup.ts +++ b/packages/bench/src/drizzle/setup.ts @@ -37,8 +37,16 @@ export const encryptionBenchTable = extractEncryptionSchema(benchTable) * 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. + * HAND-WRITTEN — it cannot be derived from `benchTable` without getting weaker, + * so `__unit__/seed-keys.test.ts` is the real drift guard, not the type. + * Drizzle's `InferInsertModel` describes the ENCRYPTED column (the custom type's + * `data` is the EQL envelope, not the plaintext) and degrades these three to + * optional `any`. Deriving from `encryptionBenchTable` is no better: + * `extractEncryptionSchema` returns the widened `AnyV3Table`, whose column map + * is an index signature — a type that admits `encTxt: 'x'` and `encText: 12345` + * alike, both of which the literal below rejects. + * + * Update it by hand when `benchTable` changes; the unit test will tell you. */ export type BenchPlaintextRow = { encText: string diff --git a/packages/bench/vitest.unit.config.ts b/packages/bench/vitest.unit.config.ts index fa01c8c55..6e8695ad8 100644 --- a/packages/bench/vitest.unit.config.ts +++ b/packages/bench/vitest.unit.config.ts @@ -13,6 +13,5 @@ import { defineConfig } from 'vitest/config' export default defineConfig({ test: { include: ['__unit__/**/*.test.ts'], - server: { deps: { inline: [/packages\/test-kit/] } }, }, }) diff --git a/packages/stack/__tests__/dynamodb/client-compat.test-d.ts b/packages/stack/__tests__/dynamodb/client-compat.test-d.ts index 971b641a2..1f57c0bd9 100644 --- a/packages/stack/__tests__/dynamodb/client-compat.test-d.ts +++ b/packages/stack/__tests__/dynamodb/client-compat.test-d.ts @@ -14,6 +14,7 @@ import { describe, expectTypeOf, it } from 'vitest' import type { EncryptedAttributes, EncryptedDynamoDBInstance } from '@/dynamodb' import { encryptedDynamoDB } from '@/dynamodb' +import type { CallableEncryptionClient } from '@/dynamodb/types' import type { EncryptionClient } from '@/encryption' import type { EncryptionV3 } from '@/encryption/v3' import { encryptedTable as encryptedTableV3, types } from '@/eql/v3' @@ -383,3 +384,37 @@ describe('operations chain and resolve', () => { expectTypeOf(result.data.email__source).toEqualTypeOf() }) }) + +/** + * The INTERNAL client view, `CallableEncryptionClient` — the shape the operation + * classes cast to. Distinct from everything above, which is the PUBLIC instance. + * + * All four members must return `unknown`. The shipped clients disagree about + * what an operation is: the nominal client returns a chainable + * `EncryptModelOperation`, the typed client a `MappedDecryptOperation`, and the + * wasm-inline client a bare `Promise` carrying no `.audit()` at all. + * Declaring a chainable shape here asserts an `.audit()` that entry does not + * have — and that assertion is not academic: it is what let the write path chain + * `.audit()` unconditionally and fail EVERY EQL v3 write on the wasm entry + * (#788 review follow-up). + * + * `unknown` forces every call site through `resolveEncryptResult` / + * `resolveDecryptResult`, which normalise all three shapes and fail closed. The + * encrypt members were the last two still declaring the lie. + */ +describe('the internal callable client view is shape-agnostic', () => { + it('returns unknown from all four members, so no shape is presumed', () => { + expectTypeOf< + ReturnType + >().toBeUnknown() + expectTypeOf< + ReturnType + >().toBeUnknown() + expectTypeOf< + ReturnType + >().toBeUnknown() + expectTypeOf< + ReturnType + >().toBeUnknown() + }) +}) diff --git a/packages/stack/__tests__/dynamodb/resolve-decrypt.test.ts b/packages/stack/__tests__/dynamodb/resolve-decrypt.test.ts index f3ce936ae..77f31bf4e 100644 --- a/packages/stack/__tests__/dynamodb/resolve-decrypt.test.ts +++ b/packages/stack/__tests__/dynamodb/resolve-decrypt.test.ts @@ -75,11 +75,13 @@ describe('resolveDecryptResult', () => { // A non-conforming client that resolves to a bare value (or `{}`) has // neither `data` nor `failure`. Casting it straight through would surface a // fake success carrying `undefined`; the shape must be rejected instead. - for (const malformed of [{}, 42, undefined]) { + // `null` and `[]` pin the two clauses that are otherwise untested: without + // the `resolved === null` guard, `'data' in null` throws a TypeError, and an + // array passes `typeof === 'object'` so it must be rejected on the key checks. + for (const malformed of [{}, 42, undefined, null, []]) { const result = await resolveDecryptResult(Promise.resolve(malformed), {}) - expect(result.failure).toBeDefined() - expect(typeof result.failure?.message).toBe('string') + expect(result.failure?.message).toMatch(/malformed result/) expect(result.data).toBeUndefined() } }) @@ -201,26 +203,67 @@ describe('resolveDecryptResult', () => { * must resolve instead of throwing. */ describe('resolveEncryptResult', () => { - it('chains .audit and forwards metadata when present (native clients)', async () => { - let seen: unknown - let calls = 0 - const operation = { - audit(config: { metadata?: Record }) { - calls += 1 - seen = config.metadata - return Promise.resolve({ data: { encrypted: true } }) - }, + /** + * The NATIVE shape, not a hand-rolled lookalike. + * + * `EncryptionOperation.audit()` returns `this` and the operation is a thenable + * whose `then()` calls `execute()` — so the metadata is read back out of the + * operation at execution time, not passed forward as an argument. A stub whose + * `audit()` returns a promise satisfies the helper while testing none of that: + * break `audit()` so it stops recording, or break `then()` so it never + * executes, and such a stub still passes while every native encrypt loses its + * audit trail. Subclass the real base class instead, exactly as the decrypt + * half of this file does with `MappedDecryptOperation`. + */ + it('forwards metadata into a real native operation and executes it once', async () => { + let seenMetadata: Record | undefined + let executions = 0 + + class NativeEncrypt extends EncryptionOperation<{ encrypted: boolean }> { + override async execute(): Promise< + Result<{ encrypted: boolean }, EncryptionError> + > { + executions += 1 + seenMetadata = this.getAuditData().metadata + return { data: { encrypted: true } } + } } const result = await resolveEncryptResult( - operation, + new NativeEncrypt(), { metadata: { sub: 'u1' } }, 'encryptModel', ) expect(result).toEqual({ data: { encrypted: true } }) - expect(seen).toEqual({ sub: 'u1' }) - expect(calls).toBe(1) + // The metadata reached `execute()` — the only place it can reach ZeroKMS. + expect(seenMetadata).toEqual({ sub: 'u1' }) + // Awaiting a thenable that `.audit()` returned as `this` must not run it twice. + expect(executions).toBe(1) + }) + + it('propagates a native operation failure without unwrapping it', async () => { + class FailingEncrypt extends EncryptionOperation<{ encrypted: boolean }> { + override async execute(): Promise< + Result<{ encrypted: boolean }, EncryptionError> + > { + return { + failure: { + type: EncryptionErrorTypes.EncryptionError, + message: 'ffi exploded', + }, + } + } + } + + const result = await resolveEncryptResult( + new FailingEncrypt(), + { metadata: { sub: 'u1' } }, + 'encryptModel', + ) + + expect(result.failure?.message).toBe('ffi exploded') + expect(result.data).toBeUndefined() }) it('awaits a bare promise instead of throwing (wasm-inline encrypt)', async () => { @@ -242,16 +285,19 @@ describe('resolveEncryptResult', () => { }) it('returns a failure — not a fake success — for a malformed result', async () => { - // Fail closed. A bare value cast straight through would hand the caller an - // "encrypted" item that was never encrypted. - for (const malformed of [{}, 42, undefined]) { + // Fail closed on every non-Result shape. `null` and `[]` are here + // deliberately: `null` is what makes the `resolved === null` clause + // load-bearing (without it, `'data' in null` throws a TypeError rather than + // returning the intended message), and `typeof [] === 'object'` means an + // array reaches the property checks and must still be rejected. + for (const malformed of [{}, 42, undefined, null, []]) { const result = await resolveEncryptResult( Promise.resolve(malformed), {}, 'encryptModel', ) - expect(result.failure).toBeDefined() + expect(result.failure?.message).toMatch(/malformed result/) expect(result.data).toBeUndefined() } }) diff --git a/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts b/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts index ce9e87cdd..d13ad10b7 100644 --- a/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts +++ b/packages/stack/__tests__/dynamodb/v2-table-forwarding.test.ts @@ -18,10 +18,18 @@ * the adapter makes rather than on a live decrypt. That matters because the only * other coverage of this path is an integration suite requiring live ZeroKMS. */ -import { describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { encryptedDynamoDB } from '@/dynamodb' import { encryptedTable as encryptedTableV3, types } from '@/eql/v3' import { encryptedColumn, encryptedTable as encryptedTableV2 } from '@/schema' +import { logger } from '@/utils/logger' + +// The audit-drop tests spy on the shared `logger` singleton, which other suites +// in this directory also patch. Each restores its own spy in a `finally`; this +// is the safety net so a patched method can never survive a test boundary. +afterEach(() => { + vi.restoreAllMocks() +}) const usersV2 = encryptedTableV2('users_v2', { email: encryptedColumn('email').equality(), @@ -253,18 +261,71 @@ describe('a client whose decrypt requires the table', () => { /** * Audit metadata has nowhere to go on this client shape, so it is dropped — - * but the encrypt must still succeed rather than failing the whole write. - * Mirrors the decrypt path's documented behaviour. + * but the encrypt must still succeed rather than failing the whole write, and + * the drop must be OBSERVABLE rather than silent. Asserting only that the + * result succeeded would pass even with the debug log deleted, and that log is + * the half that makes a missing audit record diagnosable. */ - it('drops audit metadata on that client rather than failing the encrypt', async () => { - const { client } = wasmShapedClient(['users_v3']) - const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) + it('drops audit metadata observably, without failing the encrypt', async () => { + const spy = vi.spyOn(logger, 'debug').mockImplementation(() => {}) + + try { + const { client } = wasmShapedClient(['users_v3']) + const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) + + const result = await dynamo + .encryptModel({ pk: 'a' } as never, usersV3) + .audit({ metadata: { requestId: 'r-1' } }) + + expect(result.failure).toBeUndefined() + expect(spy).toHaveBeenCalledWith( + expect.stringContaining('encryptModel audit metadata ignored'), + ) + // It must name the entry to switch to, not merely report a loss. + expect(spy.mock.calls.at(-1)?.[0]).toMatch(/@cipherstash\/stack/) + } finally { + spy.mockRestore() + } + }) + + /** + * The mirror: a client that DOES carry `.audit()` must not trip the drop path. + * Without this, a "fix" that logged unconditionally — or that stopped chaining + * `.audit()` at all — would leave every test green while silently discarding + * the native clients' audit trail. + */ + it('does not report a drop when the client can carry the metadata', async () => { + const spy = vi.spyOn(logger, 'debug').mockImplementation(() => {}) + + try { + let seen: unknown + const chainable = { + audit(config: { metadata?: Record }) { + seen = config.metadata + return Promise.resolve({ data: {} }) + }, + } + const client = { + getEncryptConfig: () => ({ v: 1, tables: { users_v3: {} } }), + encryptModel: () => chainable, + bulkEncryptModels: () => chainable, + decryptModel: () => Promise.resolve({ data: {} }), + bulkDecryptModels: () => Promise.resolve({ data: {} }), + } + const dynamo = encryptedDynamoDB({ encryptionClient: client as never }) - const result = await dynamo - .encryptModel({ pk: 'a' } as never, usersV3) - .audit({ metadata: { requestId: 'r-1' } }) + const result = await dynamo + .encryptModel({ pk: 'a' } as never, usersV3) + .audit({ metadata: { requestId: 'r-1' } }) - expect(result.failure).toBeUndefined() + expect(result.failure).toBeUndefined() + expect(seen).toEqual({ requestId: 'r-1' }) + expect(spy).not.toHaveBeenCalledWith( + expect.stringContaining('audit metadata ignored'), + ) + } finally { + spy.mockRestore() + } }) /** diff --git a/packages/stack/src/dynamodb/helpers.ts b/packages/stack/src/dynamodb/helpers.ts index 538091d31..4ee8efa59 100644 --- a/packages/stack/src/dynamodb/helpers.ts +++ b/packages/stack/src/dynamodb/helpers.ts @@ -101,7 +101,7 @@ export async function resolveDecryptResult( operation: unknown, auditData: { metadata?: Record }, ): Promise< - { data: T; failure?: never } | { data?: never; failure: DecryptFailure } + { data: T; failure?: never } | { data?: never; failure: ResultFailure } > { const chainable = operation as { audit?: (data: { metadata?: Record }) => unknown @@ -139,10 +139,16 @@ export async function resolveDecryptResult( return resolved as | { data: T; failure?: never } - | { data?: never; failure: DecryptFailure } + | { data?: never; failure: ResultFailure } } -type DecryptFailure = { message: string; code?: string } +/** + * The failure member both resolvers hand back — shared, because the encrypt and + * decrypt paths return structurally identical failures and `throwPreservingCode` + * consumes either. `code` is the FFI error code, preserved so `handleError` can + * read it back off the rethrown Error (the error-code contract in AGENTS.md). + */ +type ResultFailure = { message: string; code?: string } /** * Resolve an encrypt call against either client shape — the write-path mirror @@ -165,7 +171,7 @@ export async function resolveEncryptResult( auditData: { metadata?: Record }, context: 'encryptModel' | 'bulkEncryptModels', ): Promise< - { data: T; failure?: never } | { data?: never; failure: DecryptFailure } + { data: T; failure?: never } | { data?: never; failure: ResultFailure } > { const chainable = operation as { audit?: (data: { metadata?: Record }) => unknown @@ -199,7 +205,7 @@ export async function resolveEncryptResult( return resolved as | { data: T; failure?: never } - | { data?: never; failure: DecryptFailure } + | { data?: never; failure: ResultFailure } } /** diff --git a/packages/stack/src/dynamodb/types.ts b/packages/stack/src/dynamodb/types.ts index df3533f9e..83aac0414 100644 --- a/packages/stack/src/dynamodb/types.ts +++ b/packages/stack/src/dynamodb/types.ts @@ -62,15 +62,6 @@ export type DynamoDBEncryptionClient = { bulkDecryptModels(input: never, table: never): unknown } -type ChainableEncryptOperation = { - audit(data: { - metadata?: Record - }): PromiseLike< - | { data: T; failure?: never } - | { data?: never; failure: { message: string; code?: string } } - > -} - /** * @internal Callable view of {@link DynamoDBEncryptionClient}. * @@ -80,21 +71,28 @@ type ChainableEncryptOperation = { * satisfy. The operation classes therefore cast to this shape at the call site * — the same split the Drizzle v3 operators use. * - * `decryptModel` is intentionally untyped in its return: both shipped clients - * return a chainable operation, but different classes of one (the nominal - * client's `DecryptModelOperation`, the typed client's `MappedDecryptOperation`), - * and a custom client may return something else entirely. See - * `resolveDecryptResult`, which normalises all three. + * The returns are intentionally untyped on ALL FOUR members. The clients + * disagree about what an operation even is: the nominal client returns a + * chainable `EncryptModelOperation` / `DecryptModelOperation`, the typed client + * a `MappedDecryptOperation`, and the wasm-inline client a bare + * `Promise` with no `.audit()` anywhere on it. + * + * Declaring a chainable shape here asserts an `.audit()` that the wasm entry + * does not have, and that assertion was not academic — it is exactly what let + * the write path chain `.audit()` unconditionally and fail EVERY EQL v3 write on + * that entry (#788 review follow-up). `unknown` forces each call site through + * `resolveEncryptResult` / `resolveDecryptResult`, which normalise all three + * shapes and fail closed on anything else. */ export type CallableEncryptionClient = { encryptModel( input: Record, table: AnyEncryptedTable, - ): ChainableEncryptOperation> + ): unknown bulkEncryptModels( input: Record[], table: AnyEncryptedTable, - ): ChainableEncryptOperation[]> + ): unknown decryptModel( input: Record, table?: AnyEncryptedTable,