fix(stack,bench): wasm-inline DynamoDB v2 reads, and a bench seed that never encrypted (#772 review findings 10, 12)

.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.

.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

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([])
+  })
+})

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"
+	}
 }

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 }
 }
 
 /**

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)

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

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)
+  })
+})

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<T>(
   operation: unknown,
@@ -103,12 +108,11 @@ export async function resolveDecryptResult<T>(
   }
 
   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.",
     )
   }
 

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 {

packages/stack/src/dynamodb/types.ts

@@ -37,12 +37,16 @@ export type AnyEncryptedTable =
  * nominal `TypedEncryptionClient<S>` 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