feat(stack): port the model helpers to wasm-inline — encryptModel/decryptModel + bulk forms#767
feat(stack): port the model helpers to wasm-inline — encryptModel/decryptModel + bulk forms#767coderdan wants to merge 1 commit into
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: 6fdb7f6 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 |
📝 WalkthroughWalkthroughAdds single and bulk model encryption/decryption to ChangesWASM inline model helpers
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
| expect(ffi.encryptBulk).not.toHaveBeenCalled() | ||
| expect(expectData(out)).toEqual({ | ||
| email: null, | ||
| createdOn: undefined, | ||
| id: 1, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does Vitest toEqual ignore undefined object properties while toStrictEqual checks them?
💡 Result:
Yes, that is correct. In Vitest,.toEqual ignores properties that are set to undefined, whereas.toStrictEqual includes them in its check [1][2]. When you use.toEqual, Vitest asserts that the objects have the same structure and values, but it treats missing keys and keys explicitly set to undefined as equivalent [1][2]. Conversely,.toStrictEqual is more rigorous: it requires that the objects not only have the same structure and values but also the same types and that all properties—including those explicitly set to undefined—are present [1][2]. For example: expect({ a: 1 }).toEqual({ a: 1, b: undefined }) // Passes expect({ a: 1 }).not.toStrictEqual({ a: 1, b: undefined }) // Passes [2]
Citations:
Use toStrictEqual for this undefined-preservation assertion
toEqual treats undefined properties as absent, so this still passes if createdOn is dropped instead of preserved.
💚 Assert undefined preservation strictly
- expect(ffi.encryptBulk).not.toHaveBeenCalled()
- expect(expectData(out)).toEqual({
- email: null,
- createdOn: undefined,
- id: 1,
- })
+ expect(ffi.encryptBulk).not.toHaveBeenCalled()
+ expect(expectData(out)).toStrictEqual({
+ email: null,
+ createdOn: undefined,
+ id: 1,
+ })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(ffi.encryptBulk).not.toHaveBeenCalled() | |
| expect(expectData(out)).toEqual({ | |
| email: null, | |
| createdOn: undefined, | |
| id: 1, | |
| }) | |
| }) | |
| expect(ffi.encryptBulk).not.toHaveBeenCalled() | |
| expect(expectData(out)).toStrictEqual({ | |
| email: null, | |
| createdOn: undefined, | |
| id: 1, | |
| }) | |
| }) |
🤖 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/__tests__/wasm-inline-models.test.ts` around lines 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.
1 participant
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-#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
New Features
null/undefined.Documentation