fix(stack,bench): wasm-inline DynamoDB v2 reads, and a bench seed that never encrypted (#772 review findings 10, 12)#788
Conversation
…base
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.
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 detectedLatest commit: bcf247a The changes in this PR will be included in the next version bump. This PR includes changesets to release 11 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approve.
Both fixes are correct and contract-preserving. Finding 10 rejects the wasm-inline + EQL-v2 pairing at the call site keyed on the declared requiresTableForDecrypt capability rather than sniffing arity/constructor, and the native/typed path is provably unaffected (they never set that flag — the still-green forwarding tests confirm it). Finding 12 is a real "moving plaintext" bench bug; the fix is right and is now pinned by a credential-free structural test (derived from the table's own buildColumnKeyMap()) wired into tests-bench.yml, which closes the "too expensive to run, so it rotted" gap that caused it. Result/payload shapes, ESM/CJS exports, and the v3 read path are untouched. Changeset present and correctly scoped (bench is private, needs none).
Non-blocking:
dynamodb/index.tsguard message is read-flavored ("would fail at the first read") but also fires onencryptModel/bulkEncryptModels. Outcome is correct (wasm-inline is v3-only), but consider softening to "cannot handle legacy EQL v2 tables" so it reads correctly on encrypt calls too.skills/stash-dynamodb/SKILL.mdstates a blanket "decrypt still reads existing v2 items" — a one-line caveat that the wasm-inline edge client is now the exception would preempt a customer hitting the new throw.
2 participants
Review remediation for #772 — findings 10 and 12.
Finding 10 — the wasm-inline client on the DynamoDB v2 read path
decrypt-model.tscallsclient.decryptModel(item)with no table for a v2 table, deliberately: a v2 table means nothing to a v3 client's reconstructor map, and the native clients derive it from the payloads.WasmEncryptionClientcannot — its decrypt requires the table and resolves date fields from a per-table map, so the omitted argument reachedrequireTable(undefined)and threwTypeError: 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 / Supabase Edge Functions, and satisfies
DynamoDBEncryptionClientstructurally — so it was accepted with no cast and only failed at the first read.Now rejected where the message can name the combination, keyed on a declared capability (
requiresTableForDecrypt) rather than sniffing arity or constructor name. EQL v3 tables are always passed the table, so they still work — pinned by a test, so this cannot become a blanket rejection of the wasm entry.Three comments claimed audit metadata is forwarded "regardless of client shape" and that "every client this package ships carries
.audit()on decrypt". Neither is true of the wasm client. Corrected, along with a debug message that told the user to build a client withEncryption({ schemas })— which is exactly what the wasm entry's own factory is.Finding 12 — the bench seed never encrypted anything
extractEncryptionSchemakeys the column map by the Drizzle table's JS property (encText), not the DB column name (enc_text). The v2→v3 port left the seed keyed by DB name, so no field matched:bulkEncryptModelsreturned every row untouched with no failure, and the remap atseed.ts:66was moving plaintext.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 for anyone with credentials.Nothing caught it because nothing could:
tsc --noEmitpasses (the row type was hand-written and agreed with itself),harness.test.tsasserts a row count and never that a column is encrypted, and CI runs only thedb-onlyfilter, which never seeds.So the new check is deliberately cheap —
__unit__/seed-keys.test.tscompares the row's keys againstbuildColumnKeyMap()'s under a second vitest config with noglobalSetup, so it needs neither a database nor credentials and cannot be skipped for want of either.Not in this PR
Cut-list items 3 (nested dotted date columns never
Date-reconstructed — pre-existing onmain) and 8 (dead v2-onlyPgCompositere-exports onadapter-kitand the top-level entry). Both are real; 8 removes public surface and wants its own review.Verification
pnpm --filter @cipherstash/stack test→ 140 passed, 51 skipped; the 2 failing files are the pre-existing credential-gated live suites (identical count on the base).test:types99 passed.pnpm --filter @cipherstash/bench build(tsc) clean,test:unit3 passed. Biome: 0 errors.