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

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

.changeset/dynamodb-wasm-v2-read.md

@@ -0,0 +1,39 @@
+---
+'@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. 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
+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

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

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

@@ -6,6 +6,7 @@
   "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",

packages/bench/src/drizzle/setup.ts

@@ -28,10 +28,30 @@ 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).
+ *
+ * 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 = {
-  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,17 @@
+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'],
+  },
+})

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<string>()
   })
 })
+
+/**
+ * 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<WasmResult>` 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<CallableEncryptionClient['encryptModel']>
+    >().toBeUnknown()
+    expectTypeOf<
+      ReturnType<CallableEncryptionClient['bulkEncryptModels']>
+    >().toBeUnknown()
+    expectTypeOf<
+      ReturnType<CallableEncryptionClient['decryptModel']>
+    >().toBeUnknown()
+    expectTypeOf<
+      ReturnType<CallableEncryptionClient['bulkDecryptModels']>
+    >().toBeUnknown()
+  })
+})