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 2 commits 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: e6e2e38 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 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 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
| 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.
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
| `@cipherstash/stack/wasm-inline` now has the model helpers: `encryptModel` / `decryptModel` and `bulkEncryptModels` / `bulkDecryptModels` (#742). They run the same schema traversal as the native entry (shared code, so the two entries cannot drift on which fields get encrypted): declared columns are encrypted — matched by JS property name, nested fields via the column's dotted path — everything else passes through, and `null`/`undefined` fields are preserved without reaching ZeroKMS. Each call is one ZeroKMS round trip regardless of how many fields or models it covers, `types.Date`/`types.Timestamp` columns round-trip `Date` → `Date` (ISO strings on the wire), and failures follow this entry's `{ data } | { failure }` Result contract, with decrypt failures naming every failing field by its model path. Edge code no longer needs the hand-written `bulkEncrypt` field mapping whose failure mode was a schema column silently persisted in plaintext. | ||
|
|
||
| The shared model traversal is also hardened: it no longer mutates the caller's model (previously a nested-column decrypt wrote decrypted plaintext back into the caller's input, and encrypt overwrote it with ciphertext); a literal flat dotted key, a `__proto__`-shaped key, or a non-object model element is handled safely instead of crashing, leaking plaintext, or reaching `Object.prototype`; an already-encrypted field is passed through rather than re-encrypted; and an invalid `Date` is rejected per field. On the WASM entry, model ops now validate the table against the client's schemas, `Date` values are normalized at every encrypt/query crossing (not just the model path), and a `null`/empty model batch returns `{ data: [] }`. The skills update ships in the `stash` tarball, hence the `stash` patch. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the ZeroKMS round-trip claim.
Line [6] says every call performs one ZeroKMS round trip, but Line [8] documents that null/empty batches return without contacting ZeroKMS. The implementation also skips FFI when no operation fields are present. Document that non-empty batches containing encrypted fields use one round trip, while empty/no-op batches short-circuit.
🤖 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 @.changeset/wasm-inline-model-helpers.md around lines 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.
| `null` / `undefined` entries yield `null` at the same index without reaching ZeroKMS. Because each entry names its own column, one call can cover several columns across many rows. When items fail to decrypt, `failure.message` names every failing index. The model helpers (`encryptModel` / `bulkEncryptModels` / …) are **not** available on the WASM entry. | ||
| `null` / `undefined` entries yield `null` at the same index without reaching ZeroKMS. Because each entry names its own column, one call can cover several columns across many rows. When items fail to decrypt, `failure.message` names every failing index. | ||
|
|
||
| **The model helpers are available on the WASM entry too**: `encryptModel(model, table)`, `decryptModel(model, table)`, `bulkEncryptModels(models, table)`, `bulkDecryptModels(models, table)`. They run the same schema walk as the native client — declared columns are encrypted (matched by **JS property name**; nested fields via the column's dotted path), everything else passes through, `null`/`undefined` fields are preserved without reaching ZeroKMS, and the caller's model is never mutated — and each call is **one ZeroKMS round trip** no matter how many fields or models it covers. `table` must be one the client was built with (`Encryption({ schemas })`), else the call fails, as on the native client. `types.Date` / `types.Timestamp` columns round-trip `Date` → `Date` (on the wire they travel as ISO strings); because matching is by JS property name, a row keyed by raw DB column names (e.g. a raw `SELECT` returning `created_on`) still decrypts, but its date fields come back as ISO strings — key your models by the schema's property names. Differences from the native typed client: every method returns a plain `Promise` of the `{ data } | { failure }` Result (no `.audit()` chaining), and there is **no lock-context argument** — identity-bound encryption on the edge is configured at client construction via `config.authStrategy`. Decrypt failures name every failing field: `bulkDecryptModels` prefixes the model index (`[model 1] profile.ssn`), `decryptModel` names the field alone (`profile.ssn`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Clarify the empty-batch exception in the round-trip claim.
This says every model-helper call makes one ZeroKMS round trip, but empty batches are short-circuited and make zero calls. Please qualify this as “one round trip for non-empty batches.”
🧰 Tools
🪛 SkillSpector (2.3.11)
[warning] 53: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 103: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 105: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 108: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 110: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 114: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 132: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 620: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
[warning] 620: [RP1] null: npx commands without a version suffix (e.g. @1.0.0) create a rug-pull risk if the upstream server is compromised and publishes a malicious update.
Remediation: Pin the version: npx @scope/server@1.2.3
(MCP Rug Pull (RP1))
🤖 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 `@skills/stash-encryption/SKILL.md` at 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.
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 |
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.