feat(stack): port the model helpers to wasm-inline — encryptModel/decryptModel + bulk forms#767
Conversation
…ryptModel + bulk forms `@cipherstash/stack/wasm-inline` exposed no model operations, so edge code hand-rolled the per-field bulkEncrypt mapping for every read and write. The failure mode of that mapping is the quiet one #742 describes: add a column to the schema, forget to extend the mapping, and the field silently persists in PLAINTEXT. The model helpers close that gap by construction. The traversal is SHARED, not ported: the pure walk (field matching by JS property name, dotted-path nesting, null bookkeeping, nested placement, numeric validation, the property→DB-name column map) moves out of packages/stack's model-helpers.ts into a new bundle-safe encryption/helpers/model-traversal.ts with no runtime protect-ffi imports, and both entries consume it — the native model helpers pair it with the NAPI batch calls exactly as before, the WASM entry pairs it with the /wasm-inline FFI batch calls. The two entries can no longer drift on which fields get encrypted. The new surface follows this entry's local conventions (#741 precedent): - Every method returns the `{ data } | { failure }` WasmResult; arguments are plain models plus a v3 table — no `{ id, … }` envelopes. - One ZeroKMS round trip per call, however many fields/models it covers, with assertBatchLength guarding the positional pairing. - decryptModel/bulkDecryptModels ride decryptBulkFallible: one bad field doesn't mask the rest, and the single failure names EVERY failed field by its model path (`[model 1] profile.ssn`), with per-item codes. - Date columns round-trip Date → Date: encrypt serializes to ISO-8601 (WasmPlaintext carries no Date across the wasm serde — a raw Date would arrive as `{}`), decrypt rebuilds via the table's cast_as, mirroring the native v3 client's rowReconstructor. - No .withLockContext(): identity-bound encryption on the edge is configured at client construction via config.authStrategy. 13 new mocked unit tests pin the FFI payload shapes, single-crossing batching, passthrough/null preservation, the property→DB-name mapping, dotted-path nesting, Date round-trip, per-field failure labelling, and the short-response refusal; the gated Deno e2e gains live round-trip coverage for all four methods. skills/stash-encryption drops its "not available on the WASM entry" caveat in favour of the real contract. Closes #742
🦋 Changeset detectedLatest commit: 7251719 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 |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds single and bulk model encryption/decryption to ChangesWASM inline model helpers
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant WasmEncryptionClient
participant ZeroKMS
Caller->>WasmEncryptionClient: encryptModel or bulkEncryptModels
WasmEncryptionClient->>ZeroKMS: one bulk encryption request
ZeroKMS-->>WasmEncryptionClient: encrypted field payloads
WasmEncryptionClient-->>Caller: rebuilt model result
Caller->>WasmEncryptionClient: decryptModel or bulkDecryptModels
WasmEncryptionClient->>ZeroKMS: one fallible bulk decryption request
ZeroKMS-->>WasmEncryptionClient: decrypted values or field failures
WasmEncryptionClient-->>Caller: rebuilt models or failure result
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stack/src/encryption/helpers/model-traversal.ts (1)
162-174: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueOptional: anchor the nested-recursion prefix match to a path boundary. The gate
columnPaths.some((path) => path.startsWith(fullKey))matches sibling keys that merely share a textual prefix (e.g.'ab.c'.startsWith('a')), so the walk recurses into objects that have no schema column beneath them. This is benign today — it only over-recurses and never skips a real nested column, so there is no plaintext-leak path — but tightening it to a path boundary keeps this sensitive traversal precise as the schema shapes grow.
packages/stack/src/encryption/helpers/model-traversal.ts#L162-L174: change the recurse guard tocolumnPaths.some((path) => path.startsWith(\${fullKey}.`))`.packages/stack/src/encryption/helpers/model-traversal.ts#L244-L256: apply the same\${fullKey}.`` boundary in the bulk walker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/src/encryption/helpers/model-traversal.ts` around lines 162 - 174, Anchor both nested-recursion guards to a path boundary by changing the prefix checks in the model traversal logic at packages/stack/src/encryption/helpers/model-traversal.ts lines 162-174 and 244-256 to match `${fullKey}.`; update the guards in both the single-object walker and bulk walker, preserving recursion only when a schema column exists beneath the current key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/stack/__tests__/wasm-inline-models.test.ts`:
- Around line 124-130: Update the assertion in the test around expectData(out)
to use strict equality, ensuring the createdOn property is explicitly preserved
as undefined rather than omitted. Keep the expected object and surrounding
encryption assertions unchanged.
---
Nitpick comments:
In `@packages/stack/src/encryption/helpers/model-traversal.ts`:
- Around line 162-174: Anchor both nested-recursion guards to a path boundary by
changing the prefix checks in the model traversal logic at
packages/stack/src/encryption/helpers/model-traversal.ts lines 162-174 and
244-256 to match `${fullKey}.`; update the guards in both the single-object
walker and bulk walker, preserving recursion only when a schema column exists
beneath the current key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2797e1e6-3876-4d2a-a54e-003fc15f5cde
📒 Files selected for processing (7)
.changeset/wasm-inline-model-helpers.mde2e/wasm/roundtrip.test.tspackages/stack/__tests__/wasm-inline-models.test.tspackages/stack/src/encryption/helpers/model-helpers.tspackages/stack/src/encryption/helpers/model-traversal.tspackages/stack/src/wasm-inline.tsskills/stash-encryption/SKILL.md
Code review —
|
…pers Recall-mode review (#767) surfaced a cluster of defects in the SHARED model traversal that this PR newly ships on the wasm surface, plus wasm-specific gaps. Fixed at the right depth — mostly by rewriting the traversal to be non-mutating and unambiguous, so the native entry benefits too. Shared traversal (packages/stack/src/encryption/helpers/model-traversal.ts): The walk used to shallow-copy the model and `delete` matched fields out of the copy, which mutated the caller's nested objects (a nested-column decrypt wrote decrypted PLAINTEXT back into the caller's "encrypted" input), and re-`split` the dotted key to find the delete target (crash / plaintext leak on a literal flat dotted key, and `Object.prototype` reach via `__proto__.x`). Rewritten to build a FRESH passthrough tree that omits operation fields and keeps nulls in place: - caller model is never mutated (encrypt and decrypt); - a literal flat dotted key is normalized to its column path — no crash, no surviving plaintext; - a `__proto__`-shaped key is refused by the setNestedValue guard (no prototype pollution) instead of an unguarded delete-walk; - a dotted null key no longer materializes a phantom nested object (nulls ride in the tree; the rebuild stopped re-applying a separate null map); - an already-encrypted field is passed through, not re-encrypted (was silent double-encryption for a types.Json column); - a non-object / null model element is rejected loudly (was silent char-explosion / {}); - prefix recursion is `.`-boundary correct; - Date/class instances pass through by reference (a plain-object clone would rebuild a Date as {}), preserving the exact behaviour the 108 live native model tests assert. WASM entry (packages/stack/src/wasm-inline.ts): - model ops validate the table against the client's schemas and precompute per-table date paths once at construction (was table.build() per call, and a foreign table silently accepted) — mirrors the native typed client; - a null / empty models batch returns { data: [] } like the native helpers; - Date is normalized to RFC 3339 at EVERY encrypt/query crossing via one shared toWasmFfiPlaintext (was only the model path; a Date to bulkEncrypt crossed the serde as {}), and an invalid Date is rejected per field; - decrypt guards new Date(): an unparseable date value is returned as-is rather than a silent Invalid Date; - docblocks corrected (native-parity overclaims, the fabricated "no { id } envelopes" difference). Tests / docs: - 11 new hardening unit tests (no mutation, literal dotted key, double-encrypt passthrough, non-object element, null argument, unknown table, invalid Date, __proto__ refusal, value-level Date normalization); - the bulk-model e2e now asserts the payloads are actually encrypted (a passthrough no-op would have passed before); - skill: DB-named-row Date caveat + accurate per-field failure labels; - changeset: adds the `stash` patch for the skill change. Deferred: the V3EncryptedModel/V3DecryptedModel type-level lie for dotted-path columns (they type the nested column as passthrough) — a recursive mapped-type change to the shared v3 types affecting the native client too; runtime is correct. Tracked for a follow-up. Full suite: 1045 pass; test:types clean; code:check clean.
Review feedback addressed —
|
| # | Finding | Resolution |
|---|---|---|
| 1 | Caller model mutated; plaintext written back | Fixed — traversal builds a fresh tree; caller is never written to. New tests assert the input object and returned object are independent on encrypt and decrypt. |
| 2 | __proto__ delete-walk → prototype pollution |
Fixed — no delete-walk exists anymore; a __proto__-shaped key is refused by the existing setNestedValue FORBIDDEN_KEYS guard. Test asserts {}.toString survives and the call fails cleanly. |
| 3 | Non-object array element silently corrupted | Fixed — assertModelObject rejects a non-object/null element with a clear failure before any FFI call. |
| 4 | Literal flat dotted key → crash / plaintext leak | Fixed — the field is omitted (not re-split), so it's normalized to its column path: no crash, no surviving plaintext. |
| 5 | Already-encrypted field re-encrypted (silent for types.Json) |
Fixed — an isEncryptedPayload value at a schema path is passed through, not collected. |
| 6 | Invalid Date fails whole batch, no coordinate | Fixed — assertEncryptableValue rejects new Date(NaN) per field, naming the column. |
| 7 | Accepts uninitialized tables; build() per call |
Fixed — the client now stores its schemas, precomputes per-table date paths once at construction, and requireTable() returns the cached set or fails with the "not initialized with" error (matching the native typed client). The empty-batch short-circuit runs before the table check. |
| 8 | new Date(item.data) → silent Invalid Date |
Fixed — reconstructDate returns the raw value when the parse is NaN instead of an Invalid Date object. |
| 9 | null models argument crashes vs native [] |
Fixed — both batch engines short-circuit `if (!models |
| 10 | Dotted null key → phantom nested object | Fixed — nulls ride in the tree in place; the rebuild no longer re-applies a separate null map (which is what split the dotted key into a phantom). |
| 11 | V3EncryptedModel/V3DecryptedModel type dotted columns as passthrough |
Deferred (see below). |
| 12 | Prefix recursion lacks . boundary |
Fixed — `path === fullKey |
| 13 | Vacuous bulk-model e2e | Fixed — the e2e now asserts isEncrypted(row.email) and passthrough integrity, so a passthrough no-op regression fails. |
| 14 | Date normalization only on the model path | Fixed — one shared toWasmFfiPlaintext() (Date → RFC 3339 + invalid-Date reject) is applied at encrypt, bulkEncrypt, encryptModelsBatch, and toFfiQueryTerm. |
| 15 | Changeset missing stash: patch |
Fixed — added. |
| — | Docblock: fabricated "no { id } envelopes" parity; datePropertyPaths native-parity overclaim; skill [model N] on all failures |
Fixed — docblocks corrected; skill now distinguishes bulkDecryptModels ([model 1] …) from decryptModel (bare field) and adds the DB-named-row Date caveat. |
One regression caught during the fix
The first cut of the non-mutating builder recursed into any typeof === 'object', which rebuilt a passthrough Date as {} (a Date has no enumerable keys) — 24 native model tests failed. Fixed by only recursing into plain objects/arrays (isPlainContainer) and passing Dates / class instances through by reference. Good catch by the existing live suite; it's now also covered by the "keeps a valid Date" assertions.
Deferred: finding 11 (type-level dotted-column lie)
V3EncryptedModel/V3DecryptedModel type a dotted-path column's field as untouched passthrough (data.profile.ssn is typed as its plaintext type though it holds an envelope). Fixing this correctly needs a recursive mapped type that maps dotted column paths onto the nested model shape, and it lives in the shared v3 types consumed by the native typed client too — a larger, riskier type-system change than this review's runtime scope. The runtime is correct; I've left it for a focused follow-up rather than rush a mapped-type change here. Happy to spin that out as its own issue/PR.
🤖 Generated with Claude Code
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/wasm-inline-model-helpers.md:
- Around line 6-8: The changeset’s ZeroKMS round-trip description must
distinguish normal work from short-circuit cases. Update the wording around the
model helper operations to state that non-empty batches containing encrypted
fields use one round trip, while null/empty batches and batches with no
operation fields return without contacting ZeroKMS.
In `@skills/stash-encryption/SKILL.md`:
- Line 463: Update the WASM model-helper documentation’s round-trip statement to
specify that non-empty batches make one ZeroKMS round trip, while empty batches
are short-circuited and make zero calls; keep the behavior description for
non-empty calls unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 951ee1af-f5e5-4d1a-8cf1-147252d1bd30
📒 Files selected for processing (6)
.changeset/wasm-inline-model-helpers.mde2e/wasm/roundtrip.test.tspackages/stack/__tests__/wasm-inline-models.test.tspackages/stack/src/encryption/helpers/model-traversal.tspackages/stack/src/wasm-inline.tsskills/stash-encryption/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/wasm/roundtrip.test.ts
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Reviewed by Claude Code on behalf of James Sadler.
Verdict: Approve. This is a careful, well-tested port. The central safety property — schema-declared columns cannot silently slip through unencrypted — is preserved by sharing the traversal (model-traversal.ts) between the native and WASM entries rather than re-implementing it, backed by the wasm-inline-bundle-isolation test to keep protect-ffi out of the WASM bundle. I traced the correctness-critical paths and found no blocking defects.
What I verified
- Batch alignment / ordering. In
encryptModelsBatch/decryptModelsBatch,fieldsis a global flatMap overoperationFieldsin visit order, the FFI payload isfields.map(...), results come back positionally aligned, andassertBatchLengthgates the count before any rebuild. The rebuild uses the same global index (fields.forEach((field, i) => … results[i])), so field↔result pairing is internally consistent.Object.entriesinsertion order is preserved on both the build and read side, so no drift. Not routing throughrunBatchis justified — model batches are never sparse (nulls stay in the passthrough tree, never become operation fields). - No plaintext leakage. Matching is
columnSet.has(fullKey)keyed by JS property name viaresolveEncryptColumnMap, addressed to the DB name viatoColumnName— same mechanism as native. A schema column holding a non-envelope object (e.g.types.Json) is still collected as an operation field.null/undefinedschema fields are preserved in place and never reach ZeroKMS. - Non-mutation.
buildPassthroughTreereturns a fresh tree; nested containers with a schema column beneath them are recursed and cloned, so the caller's input is untouched on both encrypt and decrypt. The.not.toBe(input.profile)and "does not write decrypted plaintext back" tests pin this. This fixes a genuine prior bug where a decrypt wrote plaintext back into the caller's object. - Date handling.
toWasmFfiPlaintextis now applied at every encrypt/query crossing (single, bulk, query, model), fixing silent{}corruption ofDateacross the wasm serde; an invalidDateis rejected pre-FFI per field;reconstructDatereturns the raw value on an unparseable stored date rather than handing back anInvalid Date. Round-trip isDate → ISO → Date, keyed bydatePropertyPaths(JS property names). - Failure reporting.
decryptBulkFalliblemeans one bad field doesn't mask the rest; the aggregate{ failure }names every failing field by model path with per-item code. Message formats match the tests. - Table validation & prototype safety.
requireTablerejects an unregistered table on both model paths;setNestedValueblocks__proto__/prototype/constructorin operation-field placement (the__proto__.toStringtest confirms it surfaces as a failure, and globalObject.prototypeis intact).
Non-blocking notes (optional)
buildPassthroughTreepassthrough assignment vs a literal__proto__own key. Passthrough (non-schema, non-envelope) fields are copied without[key] = value. For a field literally named__proto__(e.g. fromJSON.parse), bracket assignment invokes the prototype setter rather than storing data, unlike the old{ ...model }spread (which used define semantics). This does not pollute globalObject.prototype(it only sets the freshout's own prototype) and only affects a non-schema passthrough field, so it is not a security or plaintext issue — but it silently drops that field and is a fidelity gap given the changeset advertises__proto__-key safety. ConsiderObject.defineProperty/ a forbidden-key guard in the passthrough copy for symmetry withsetNestedValue. Low priority.- Array-index columns in dotted paths.
setNestedValuematerialises{}intermediates, not arrays, so a hypothetical array-indexed column path would rebuild as an object. Pre-existing limitation, unlikely in practice; the array-preservation arm inbuildPassthroughTreecovers passthrough arrays. Worth a comment at most. - Test gap.
requireTableis exercised on the decrypt path against a foreign table but there is no equivalent test forencryptModel/bulkEncryptModelsagainst an unregistered table (both callrequireTable). Cheap to add.
None of these block. Nice work on the shared-traversal approach and the hardening pass.
|
Sharing the traversal instead of porting it is the right call, and the batching/ 1. The new Encrypted before this PR. Same failure class the PR exists to close, and it now applies to the native entry too. Require the envelope to be this column's: 2. A literal
3. 4. Rebuild is O(models × total fields) — Smaller:
PR body needs correcting: "pure move — no behaviour change" and "native model behaviour out of scope" are no longer true — the changeset is honest about it, the body isn't. Native Conventions all fine: changeset present with the |
Review feedback on #767: - Test: assert undefined-preservation with toStrictEqual, not toEqual — toEqual treats an undefined property as absent, so the createdOn case would pass even if the field were dropped (CodeRabbit). - Changeset + skill: qualify the one-round-trip claim — non-empty batches make one ZeroKMS round trip; null/empty or no-schema-field batches short-circuit and make zero calls (CodeRabbit). - Traversal: store passthrough fields via defineProperty so a field literally named __proto__ is kept verbatim instead of being swallowed by the prototype setter; mirrors the setNestedValue guard. Fidelity only — never reaches global Object.prototype (review note). - Tests: cover encryptModel/bulkEncryptModels against an unregistered table (both call requireTable), and the __proto__ passthrough case (review note). - Document setNestedValue's object-intermediate limitation for array-index paths (review note).
3 participants
Closes #742.
Problem
@cipherstash/stack/wasm-inlineexposed no model operations, so edge code hand-rolled the per-fieldbulkEncryptmapping for every read and write. The failure mode is the quiet one the issue describes: add a column to the schema, forget to extend the hand-written mapping, and the field silently persists in plaintext. #741 deliberately shipped only the value-level bulk halves; this is the remaining, larger half.Approach: share the traversal, don't port it
The pure model walk — field matching by JS property name, dotted-path nesting, null bookkeeping, nested placement, per-field numeric validation, and the property→DB-name column map (
resolveEncryptColumnMap) — moves out ofpackages/stack/src/encryption/helpers/model-helpers.tsinto a new bundle-safemodel-traversal.ts(no runtime protect-ffi imports; the existingwasm-inline-bundle-isolationtest enforces that at the artifact level). Both entries now consume it:encryptBulk/decryptBulkexactly as before (pure move — no behaviour change,resolveEncryptColumnMapre-exported);/wasm-inlineFFI batch calls.The traversal is where the plaintext-leak class of bug lives, so the two entries can no longer drift on which fields get encrypted.
The new surface
Follows this module's local conventions, per the issue's guidance and the #741 precedent (note: the issue's "this surface throws" is pre-cipherstash/stack#741 — the entry returns
{ data } | { failure }Results now, and the new methods do too):encryptModel(model, table)null/undefinedpreserved without reaching ZeroKMSbulkEncryptModels(models, table)[]→{ data: [] }with no calldecryptModel(model, table)tabledrivesDatereconstruction fromcast_asbulkDecryptModels(models, table)[model <i>] <field.path>Details:
assertBatchLengthguarding the positional pairing (the model batches don't route throughrunBatch— they're never sparse and carry{ modelIndex, fieldKey }structure; its docblock is updated accordingly).decryptBulkFallible: one bad field doesn't mask the rest; the single{ failure }names every failed field by its model path with its per-item code.Datecolumns round-tripDate→Date: encrypt serializes to ISO-8601 (WasmPlaintextdeliberately carries noDate— a rawDateacross the wasm serde would arrive as{}, silent corruption), decrypt rebuilds via the table'scast_as, mirroring the native v3 client'srowReconstructor..withLockContext()— identity-bound encryption on the edge is configured at client construction viaconfig.authStrategy(wasm-inline has no auto/dev-profile auth strategy — credential-free local dev impossible on WASM #663 context), noted in the class docblock.V3ModelInput/V3EncryptedModel/V3DecryptedModel), so date columns demandDatein and yieldDateout at the type level too.Changes
packages/stack/src/encryption/helpers/model-traversal.ts— new shared pure-traversal module (moved code, not new logic)packages/stack/src/encryption/helpers/model-helpers.ts— slimmed to the native FFI pairing; imports the traversalpackages/stack/src/wasm-inline.ts— the four methods + two private batch engines,FallibleDecryptItemhoisted for sharing, docblocks updated (the class doc recorded this exact gap)packages/stack/__tests__/wasm-inline-models.test.ts— 13 mocked tests: FFI payload shapes, single-crossing batching, property→DB-name mapping (createdOn→created_on), dotted-path nesting, null/passthrough preservation, Date round-trip, NaN pre-FFI rejection, short-response refusal, per-field failure labels, empty-batch short-circuitse2e/wasm/roundtrip.test.ts— live Deno coverage for all four methods (gated on realCS_*creds, as the rest of that suite)skills/stash-encryption/SKILL.md— replaces "model helpers are not available on the WASM entry" with the actual contract + example.changeset/wasm-inline-model-helpers.md—@cipherstash/stackminorTesting
pnpm --filter @cipherstash/stack test— 1034 tests pass (includes the 13 new ones and the bundle-isolation suite against a freshdist/)pnpm --filter @cipherstash/stack build— clean, including.d.tsemissionpnpm run test:types— no type errors;tsc --noEmitreports zero errors in every touched file (remaining repo-wide errors are pre-existing in integration files)pnpm -w run code:check— clean (exit 0)Out of scope
Summary by CodeRabbit
Datehandling, and preservesnull/undefinedvalues.Dateinputs and normalizing them consistently.Summary by CodeRabbit
New Features
null/undefinedvalues, passthrough fields, andDatevalues during round trips.Bug Fixes
Documentation