refactor(stack): inject the FFI binding into the encryption operations (#798, stages 1-3)#800
Draft
coderdan wants to merge 10 commits into
Draft
refactor(stack): inject the FFI binding into the encryption operations (#798, stages 1-3)#800coderdan wants to merge 10 commits into
coderdan wants to merge 10 commits into
Conversation
… stage 1) First stage of #798. Introduces the seam; changes no public behaviour. `CryptoBackend` (`encryption/backend.ts`) is a 1:1 restatement of the six FFI functions both entries already call — `encrypt`, `decrypt`, `encryptBulk`, `decryptBulkFallible`, `encryptQuery`, `encryptQueryBulk` — with type-only imports, so the interface itself is portable. `backend-native.ts` is the implementation over the Node-API binding, and is now the only value import of `@cipherstash/protect-ffi` on the encrypt path. `EncryptOperation` is migrated as proof the seam works end to end: it takes a backend in its constructor (alongside the client handle it already took) and calls `this.backend.encrypt` rather than a module-level `ffiEncrypt`. Its import of the NAPI entry is now `import type`. The client injects `nativeBackend`. The remaining five operations still import directly and are unchanged. Two details worth keeping: - The backend delegates through a namespace (`ffi.encrypt(...)`) rather than closing over destructured imports, so bindings resolve at call time. Binding all six eagerly broke three suites at module load: a partial `vi.mock('@cipherstash/protect-ffi', …)` that stubs only the function under test fails on the five it never calls. - `null-guards.test.ts` now injects a Proxy backend that throws on any method, so "short-circuits without an FFI call" asserts a violation would fail loudly. Previously it could only assert absence. That is the testability argument for DI, demonstrated rather than claimed. Verified: `@cipherstash/stack` 1073 passed; `code:check` clean; and the built `dist/wasm-inline.js` still references only `@cipherstash/protect-ffi/wasm-inline`, with no NAPI reference. That last check is currently manual — #798 calls for it to become a build test before shared operations reach the WASM entry.
…stage 2) Stage 2 of the plan in `docs/plans/798-shared-operation-layer.md` (added here). An operation was awaitable but not assignable to `Promise<…>`: TypeScript's `Promise<T>` is structural and wants `then`, `catch`, `finally`, and `[Symbol.toStringTag]`, and only `then` was implemented. Adds the other three, each delegating to `execute()` exactly as `then` already does. This is what makes stage 4 non-breaking. `wasm-inline` currently returns bare `Promise<WasmResult<T>>`; having it return shared operations instead — the point of #798 — would otherwise break anyone who annotated that return type. With the class assignable wherever a `Promise` was, that adoption is additive, and `.audit()` / `.withLockContext()` reach the WASM entry without a major. Two corrections found while implementing, both worth recording: - `implements Promise<T>` is wrong; operations resolve to `Result<T, EncryptionError>`, so the declaration is `implements Promise<Result<T, EncryptionError>>`. Declaring it (rather than relying on structural assignability) is what caught this. - Open question 1 in the plan answered itself immediately: `Client` from `@/types` is `… | undefined`, which the FFI does not accept. `CryptoBackend` now takes `NonNullable<Client>`; operations already guard with `noClientError()` before reaching the backend, so the narrowed handle is what actually crosses the boundary. `catch` is documented as NOT catching expected failures — those resolve to `{ failure }` per the Result contract rather than rejecting. It fires only on a thrown exception, the same cases `await` throws on. `[Symbol.toStringTag]` reports the concrete operation name rather than 'Promise', so an inspected operation is not mistaken for one. Verified: 1073 runtime tests pass; `test:types` 103 pass, including a new `operation-promise-conformance.test-d.ts` that pins the assignability, `Awaited<>` shape, the three members, and `Promise.all`. Nothing else in the suite would catch a regression here, because every runtime test only awaits.
…kend (#798, stage 3) All five remaining operations now take a `CryptoBackend` instead of importing one: `decrypt`, `bulk-encrypt`, `bulk-decrypt`, `encrypt-query`, and `batch-encrypt-query`, along with their `*WithLockContext` twins, which receive it through `getOperation()` or their constructor. Every NAPI import on the operation path is now `import type`. Still no public behaviour change — the client injects `nativeBackend` at each construction site, and 1073 runtime + 103 type tests pass unchanged. One non-mechanical detail: `batch-encrypt-query.ts` typed a helper parameter as `Awaited<ReturnType<typeof ffiEncryptQueryBulk>>`, a *value* reference in a type position. That is now `CryptoBackend['encryptQueryBulk']`, which says the same thing without naming a binding. Test construction sites are updated to inject a Proxy backend that throws on any method. In `null-guards` and `bulk-encrypt-validation` this is a real improvement rather than a port: both suites exist to prove work is rejected BEFORE the FFI is reached, and previously could only infer that from an error message or an unreached stub client. Now contact with the FFI fails the test. Two helpers deliberately NOT migrated, both deferred to stage 4: - `helpers/error-code.ts` narrows with `instanceof FfiProtectError`, a runtime value. Converging it means adopting the structural reader `wasm-inline.ts` already uses (`:392` explains why it must be structural across the serde boundary). That is a behaviour change to native error-code extraction, not a mechanical move, and belongs with the entry convergence where it can be tested against both bindings. - `helpers/model-helpers.ts` calls `decryptBulk`, which `CryptoBackend` does not yet expose — the WASM entry uses `decryptBulkFallible` exclusively. The binding exports both, so adding it is easy, but which one the shared model path should use is a convergence decision, not a rename. Bundle purity re-verified: `dist/wasm-inline.js` still references only `@cipherstash/protect-ffi/wasm-inline`.
`__tests__/wasm-inline-bundle-isolation.test.ts` already enforces the bundle boundary, and is stricter than the plan proposed: it parses every module specifier out of the built `dist/wasm-inline.js`, pins protect-ffi and auth to their `/wasm-inline` subpaths, keeps an allowlist of every external so additions must be deliberate, and rebuilds when `dist` is staler than `src` so it cannot pass against an old artifact. It was added during #741. The plan listed it as outstanding. That was wrong, and the error mattered: I recommended doing stage 5 before stage 4 on the grounds that the boundary was only verified by hand. It is not — it is enforced, and stage 4 will trip the existing test if it regresses. The test's header also records that this exact leak already happened once, via `@/encryption/helpers/error-code` pulling in the native `ProtectError` for an `instanceof` narrow. That corroborates deferring that helper in stage 3 and settles how it converges: the shared path must use the structural reader `wasm-inline.ts:392` already has, because the `instanceof` one cannot cross into the WASM bundle at all.
…4 groundwork) Everything the WASM entry needs in order to adopt the shared operations, short of the adoption itself. Three transitive native/Node dependencies were blocking it; each is removed here, and all of it is inert until stage 4 proper. `backend-wasm.ts` — the `CryptoBackend` over `@cipherstash/protect-ffi/wasm-inline`, mirroring `backend-native.ts`. Unused so far; it is the other half of the seam. `helpers/error-code.ts` now reads the code structurally instead of narrowing with `instanceof ProtectError`, which was a value import of the Node-API entry. This is the convergence the bundle-isolation test's header predicted: the `instanceof` version cannot cross into the WASM bundle at all (that build ships no error class), so the structural reader is the only one that can be shared. The behavioural difference is documented in the file — it now matches any object with a string `code`, which degrades an error message in the unusual case rather than changing behaviour. `no-client-error.ts` — every operation imported `noClientError` from `encryption/index.ts`, the native composition root. That single import would have dragged the NAPI binding into any bundle touching an operation. Now its own module, re-exported from the index so the public path is unchanged. `identity/resolve-lock-context.ts` — operations imported `resolveLockContext` from `@/identity`, which imports `@/utils/config`, which uses `fs` and `path`. Those are fatal in a Worker. The check is now structural-only, which costs no behaviour: `identity/index.ts` already paired `instanceof LockContext` with `'identityContext' in input`, the structural half existing to catch a context built in another realm. Anything the `instanceof` matched, the property check matches too. The `fs`/`path` leak was found by building and reading the bundle, not by reasoning — `__tests__/wasm-inline-bundle-isolation.test.ts` names every external precisely so this is a test failure rather than a runtime surprise in someone's Worker. It earned its keep here. ## Why stage 4 proper is not in this commit Wiring `WasmEncryptionClient.encrypt` to the shared `EncryptOperation` works — typechecks, builds, and the bundle stays clean — but regresses three cases in `wasm-inline-result-contract.test.ts`. The WASM entry's `wasmResult` coerces a non-`Error` rejection while preserving its detail (a string stays its own message, an object is JSON-serialised); the shared operation's error mapper reads `.message` off whatever it is caught, so a Rust-side string rejection becomes "Something went wrong". That is a real semantic difference between the two error paths, not a porting detail, and the shared operation should adopt the WASM entry's `toError` behaviour before any method migrates. Left for the next commit rather than regressed here.
…tage 4)
Ports the WASM entry's rejection handling into the shared operation path, then
migrates the first method onto it.
## The blocker, fixed
`helpers/to-error.ts` is `wasm-inline.ts`'s `toError` lifted out and shared.
Passed as `withResult`'s `onException` hook by all ten operations, which
previously passed none. Without it a non-`Error` rejection reached the failure
mapper as-is, `(error as Error).message` was `undefined`, and the detail was
replaced by a generic string.
That gap existed because the two bindings throw differently: the NAPI entry
throws `Error` subclasses, while wasm-bindgen hands back bare strings and plain
objects. The WASM entry had careful handling and a test for it; the native path
never needed it. Sharing the operations means the shared path has to be the
careful one, so the native operations now preserve rejection detail too —
strings kept verbatim, objects JSON-serialised, `code` carried onto the
synthesized Error so `failure.code` survives (`withResult` runs `onException`
first, so the mapper only ever sees the fresh Error).
## The migration
`WasmEncryptionClient.encrypt` returns the shared `EncryptOperation` instead of
a bare promise. Source-compatible thanks to stage 2: the operation satisfies
`Promise<Result<…>>` structurally, so `await client.encrypt(…)` yields the same
`{ data } | { failure }` and existing `Promise<…>` annotations still accept it.
`.audit()` and `.withLockContext()` now exist on the WASM entry for the first
time — they live on the shared operation rather than being reimplemented.
Marked `!` because the return type changes even though behaviour does not.
## Verification
1073 runtime + 103 type tests pass, including the three
`wasm-inline-result-contract` cases this regressed before `toError` was shared
— they are what proved the error path was not equivalent. Bundle isolation
still passes: `dist/wasm-inline.js` imports exactly
`@cipherstash/auth/wasm-inline` and `@cipherstash/protect-ffi/wasm-inline`.
`.withLockContext()` on this entry is plumbed but UNVERIFIED at runtime:
whether the Rust honours `lockContext` across the WASM serde boundary is
open question 2 in the plan (#793), and confirming it needs live credentials.
The remaining nine WASM methods still have their own implementations.
🦋 Changeset detectedLatest commit: ead9d45 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 skippedDraft detected. 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 |
…tions (#798) One interface across two bindings only prevents drift if the interface says what the bindings must agree on. Most of that was unstated and unenforced by types, so this writes it down where it is checkable. On `CryptoBackend`, per-method docs plus a contract section covering the four invariants neither the type system nor a test asserts: - **The three bulk calls preserve order.** Callers re-associate results with inputs positionally, so a backend that reorders or compacts mis-assigns ciphertexts to rows — silently, and irreversibly. - **Failure is a rejection, not a sentinel** — except per-item decrypt failure, which `decryptBulkFallible` reports in-band. - **`lockContext` sits in different places on the single and bulk calls**: top-level for the former, per payload item for the latter. The Node-API types reject the misplaced version at compile time; the WASM binding, typing `opts` as `any`, would accept and drop it. That asymmetry is now stated on both `encryptBulk` and `decryptBulkFallible`, and on each `*WithLockContext` class that has to thread it through a payload builder. - **`unverifiedContext` is audit metadata only** — it reaches the audit log and is not bound into key derivation, so unlike `lockContext` it never affects whether a value reads back. Also notes that `encryptBulk` and `encryptQueryBulk` payloads each name their own table and column, so a batch may span both. There is no single-column restriction; the docs should not leave room to infer one. On the twelve operation classes, a class-level doc each: what the operation does, that the FFI arrives injected so the class runs unchanged on both entries, and the null semantics — null short-circuits without an FFI call, so a NULL column stays NULL rather than becoming an encrypted JSON null. `BulkDecryptOperation` distinguishes the two kinds of "no value" it can return: a filtered null input versus a per-item decrypt failure. `withLockContext()` gains a doc on each of the six operations that has one, since that is what shows at the call site: it returns a new operation rather than mutating, audit metadata already set carries across but later additions do not, and the claim must match at decrypt time — a mismatch fails rather than falling back to an unbound key. Comment-only. 1073 runtime + 103 type tests pass, `code:check` clean.
Reverts 7292b9e. A review of the draft found four confirmed defects in it, none caught by the suite — 1073 unit tests, the type tests, and the bundle-isolation test all passed while the WASM entry was broken on import. Verified against the built artifact, not reasoned about: 1. `EncryptOperation` imports `@/utils/logger`, whose `initStackLogger()` runs at module scope and read `process.env` unguarded. Importing `@cipherstash/stack/wasm-inline` threw `ReferenceError: process is not defined` in any runtime without the Node global — a Worker without `nodejs_compat`, or a browser. Exactly the runtimes the entry exists for. `dist/wasm-inline.js:676`, with the module-scope call at 703. 2. The shared path dropped `toWasmFfiPlaintext`, whose own doc explains that a JS `Date` crosses wasm-bindgen as `{}` — silent corruption of every date/timestamp column. Bulk and query kept it; encrypt became the only path without. 3. `execute()` calls `log.set({ column: this.column.getName() })` outside `withResult`, so a malformed column rejects instead of returning `{ failure }`. The old body ran `getColumnName()` inside `wasmResult` with a named message. That is the contract `wasm-inline-result-contract.test.ts` exists to protect. 4. `dist/wasm-inline.d.ts` gained imports of `@byteslice/result` and the native `@cipherstash/protect-ffi` root. `wasm-inline.ts` documents this regression class already: `WasmResult` is declared locally precisely to keep the published types self-contained. (4) is the one that blocks the approach rather than the attempt. protect-ffi's WASM `.d.ts` types every `opts` as `any` and exports none of the option types, so `CryptoBackend` must import them from the native specifier — which then lands in the WASM entry's published types. The fix is upstream: have the WASM `.d.ts` use the same named types. That also removes the six `as never` casts in `backend-wasm.ts`, which are why the WASM call sites get no type-checking from the interface meant to keep both bindings honest. Also reverts `helpers/error-code.ts` to `instanceof`. The structural read is needed for WASM reachability, but it widens what reaches `failure.code` — any object with a string `code` matches, and `dynamodb/helpers.ts` copies that verbatim onto thrown errors. Acceptable as a deliberate released change; not as a side effect of an internal refactor. Restore it with stage 4. What stays, since stages 1-3 stand on their own: the `CryptoBackend` seam, the native backend, `Promise` conformance on `EncryptionOperation`, and the config-free `resolveLockContext` split. Fixed alongside, all found in the same review: - Guard `process.env` in the logger. Latent regardless of stage 4, and the trap that catches the next attempt. - Unify the two exported `LockContextInput` types. `identity/index.ts` had `LockContext | Context` while the operations used `{ identityContext } | Context`, so `v3.ts` typed the v3 client's parameter narrower than the untyped client's. It now re-exports the canonical one; every `LockContext` satisfies it. - Fix a near-vacuous test. The int64-boundary case asserted only that the failure message differed from the rejection message, which would also hold if the guard broke with different wording. It now records the FFI call and asserts the value arrived intact. The plan doc records all five findings, the upstream blocker, and the three items still outstanding (model-helpers, the duplicated error coercion, per-settlement re-execution). Changeset rewritten: this is stages 1-3, whose only user-visible change is `.catch()` / `.finally()` on operations.
coderdan
changed the title
cipherstash/protectjs-ffi#142 — WASM .d.ts types every opts as `any` and exports no option types, so CryptoBackend must import them from the native specifier.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Injects the FFI binding into the encryption operations, so one operation layer can serve both entries. Stages 1–3 of #798.
Important
Stage 4 was attempted and reverted (669d65c). A review found four confirmed defects, none caught by the suite. Stage 4 is now blocked on an upstream protect-ffi change — see below. What remains here is internal plumbing plus one additive API change.
Why
The native and
wasm-inlineentries had drifted into different client surfaces. Not because WASM is different, but because the operation classes reached their FFI binding by module-levelimport. A value import of@cipherstash/protect-ffiis a runtime load of a native binary, which cannot happen in a V8 isolate — so nothing importing an operation was reusable on the edge, andwasm-inline.tsreimplemented the whole surface separately. It then drifted on call shape (#792), and never gained.audit()or.withLockContext()(#793, #797).What landed
CryptoBackend— a 1:1 restatement of the six FFI functions, with type-only imports.backend-native.tsimplements it.EncryptionOperationgainscatch,finally,[Symbol.toStringTag], so it satisfiesPromise<Result<…>>structurally.wasm-inline-bundle-isolation.test.ts, from #741) and is stricter than planned.Three transitive blockers were removed on the way:
noClientErrorandresolveLockContextwere split out of modules that drag in the NAPI binding andfs/pathrespectively.The false dichotomy in #798 — Result or thenable — turned out not to be one.
@byteslice/resultis a union type plus a wrapper; it imposes nothing. Native already had both. So the question was only whether the WASM methods could return the operation object, and adding the three missingPromisemembers makes that additive rather than breaking.Why stage 4 came back out
Four defects, verified against the built artifact:
EncryptOperation→@/utils/logger, whoseinitStackLogger()runs at module scope and readprocess.envunguarded.ReferenceError: process is not definedin a Worker withoutnodejs_compat, or a browser — the runtimes the entry exists for.toWasmFfiPlaintext. Per its own doc, a JSDatecrosses wasm-bindgen as{}. Bulk and query kept the call; encrypt became the only path without it.log.set({ column: this.column.getName() })runs outsidewithResult, so a malformed column rejects instead of returning{ failure }.dist/wasm-inline.d.tsgained@byteslice/resultand the native protect-ffi root.Plus one unconfirmed:
unverifiedContext: undefinedreached the serde boundary for the first time.toFfiQueryTermdocuments exactly this failure forqueryOp— "the native NAPI layer tolerates undefined; the WASM one does not".All of this passed CI. 1073 unit tests, 103 type tests, and the bundle-isolation test were green throughout. That is the more useful finding than any individual defect.
The blocker
dist/wasm/protect_ffi.d.tstypes every binding function as(client: WasmClient, opts: any)and exports none of the option types. SoCryptoBackendmust import them from the native specifier, which then lands in the WASM entry's published types. Same root cause forces the sixas nevercasts inbackend-wasm.ts, meaning the WASM call sites get no type-checking from the interface meant to keep both bindings honest.The fix is upstream in protect-ffi (filed as cipherstash/protectjs-ffi#142): have the WASM
.d.tsuse the same named option types rather thanany. That makesCryptoBackenda genuinely shared contract instead of the native types being borrowed to describe both bindings — which is what #798 is actually after. (A runtime-free./typessubpath would also work, but is strictly weaker.)Worth noting the four protect-ffi type names still in the reverted
.d.tspredate this work, arriving via@/typesre-exports. The upstream change is worth making regardless.Also fixed here
process.envguard. Latent regardless of stage 4, and the trap that catches the next attempt.LockContextInput. Two exported types with the same name disagreed:identity/index.tshadLockContext | Context, the operations had{ identityContext } | Context.v3.tstyped the v3 client's parameter narrower than the untyped client's.CryptoBackendand the operations, stating the invariants neither the types nor any test enforce: bulk calls preserve order (callers re-associate positionally);lockContextis top-level on single calls but per-item on bulk ones;unverifiedContextis audit metadata and never affects decryptability.Behaviour
.catch()and.finally()now exist on operations — the only user-visible change, and additive.One caveat now documented in the changeset: an operation is lazy and executes per settlement.
then,catch, andfinallyeach callexecute().op.catch(h)followed byawait opis two ZeroKMS round trips. Pre-existing forthen; stage 2 widened the surface without memoising. Memoising would change existing native double-await semantics, so it deserves its own decision.Still outstanding
model-helpers.tsvalue-imports the native binding, so the four model operations cannot join the shared path. Needs a decision on wideningCryptoBackendwith the non-fallibledecryptBulk, which is currently excluded on purpose.wasm-inline.tsstill carries its owntoError/readErrorCode/safeString.lockContextacross serde needs a live call. Shipping.withLockContext()there while that is open risks a successful-looking payload whose key is not bound to the claim the caller asked for.Verification
1073 runtime + 103 type tests pass;
code:checkclean;dist/wasm-inline.jsexternals unchanged from baseline; the logger and@byteslice/resultconfirmed absent from the WASM bundle and its.d.tsafter the revert.