feat(stack): add WASM-inline subpath + Deno verification#496
Open
coderdan wants to merge 7 commits into
Open
Conversation
Add `@cipherstash/stack/wasm-inline` so Protect runs in Deno, Bun, Cloudflare Workers, Supabase Edge, and browsers — anywhere the native protect-ffi NAPI bindings don't work. Why now: protect-ffi 0.24.0 ships a WASM build; auth 0.37.0-alpha.8 ships the matching `AccessKeyStrategy` for the WASM path. The stack package is the natural place to expose this combination as a single import target. What changed: - Bump @cipherstash/protect-ffi 0.23.0 → 0.24.0 (Node API unchanged — strategy-based newClient only lives on /wasm-inline). Bump @cipherstash/auth catalog → 0.37.0-alpha.8 for the WASM strategy. Both packages added to pnpm-workspace.yaml's release-age exclude. - New packages/stack/src/wasm-inline.ts exporting `Encryption()` + `WasmEncryptionClient` (encrypt / decrypt round-trip), plus raw re-exports of `newClientRaw`/`encryptRaw`/`decryptRaw`/ `AccessKeyStrategy` for consumers who want the spike-style API. - ESM-only bundle (the WASM-inline runtime is ESM, CJS require() would ERR_REQUIRE_ESM); tsup drops dist/wasm-inline.cjs in onSuccess, package.json exports omits the require branch. - tsconfig customConditions: ["node"] so bundler resolution picks Node protect-ffi types for the main entries; /wasm-inline subpath has unconditional types so it's unaffected. Verification: - e2e/wasm/ Deno smoke test runs encrypt → decrypt round-trip against ZeroKMS/CTS with no --allow-ffi, proving the WASM path is the only path that could have completed. - New wasm-e2e-tests CI job (Deno 2.x on blacksmith runner) exercises this on every PR. - examples/supabase-worker/ demonstrates the same code path inside a Supabase Edge Function for documentation / runbook use. Draft: depends on @cipherstash/auth shipping a non-alpha release with /wasm-inline (planned for 0.38.0); bump catalog when that lands.
📝 WalkthroughWalkthroughAdds an ESM-only ChangesWASM-inline entry point and E2E infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
CI on the first push exposed two issues: 1. `Failed to load native binding for linux-x64` in @cipherstash/wizard and the e2e/ suite. Cause: auth 0.37.0-alpha migrated the per-platform napi binaries from `optionalDependencies` to `peerDependencies` (optional), and pnpm does NOT install platform-matched optional peer deps. Revert the catalog to 0.36.0 (which still uses optionalDependencies and works for NAPI consumers) and pin @cipherstash/stack to 0.37.0-alpha.8 directly — stack only uses the `/wasm-inline` subpath, never touches the NAPI bindings. 2. `Import "zod" not a dependency` when Deno loaded dist/wasm-inline.js. Cause: tsup left zod as an external import, and Deno's nodeModulesDir: auto doesn't resolve bare specifiers without an explicit map. Bundle zod and @byteslice/result via noExternal — both are small and dependency-free. Also add --no-check to the Deno test task: stack's dist .d.ts strips the encryptedTable column-intersection brand when loaded via file URL, so TypeScript can't see `users.email`. The runtime round-trip is the only thing this smoke test asserts; column-on-table typing is covered by the package's own vitest suite.
|
…lient
The Node entry of protect-ffi normalizes SDK-facing cast_as values
(`string`, `number`, …) to EQL-native (`text`, `double`, …) via
`normalizeEncryptConfig.js` before handing them to the Rust core. The
WASM bindings don't ship that normalization layer, so an
`encryptedColumn('email')` (defaults to `cast_as: 'string'`) was
rejected with `unknown variant 'string', expected one of 'big_int',
'boolean', 'date', 'decimal', 'double', 'float', 'real', 'int', 'json',
'jsonb', 'small_int', 'text', 'timestamp'`.
Walk the EncryptConfig in the WASM Encryption() factory and apply
`toEqlCastAs` to each column's cast_as field. Same mapping the Node
side does internally.
0.38.0 promotes the WASM-inline work that landed across the 0.37.0-alpha series to a stable release. Bump the catalog so all consumers (wizard, cli, stack) track the same major; drop the direct 0.37.0-alpha.8 pin from packages/stack and the Deno test's import map.
…lDeps @cipherstash/auth 0.37.0+ ships per-platform native bindings as optional peerDependencies. pnpm doesn't auto-install platform-matched optional peer deps, so on fresh Linux CI no binary lands in node_modules and the NAPI loader throws: Failed to load native binding for linux-x64. Ensure the optional dependency "@cipherstash/auth-linux-x64-gnu" is installed. Declare the six platform sub-packages as `optionalDependencies` on the two workspace packages that actually `import auth from '@cipherstash/auth'` (wizard, cli). `optionalDependencies` IS platform-aware in pnpm — each sub-package's own `os`/`cpu` filter picks one binary for the host and silently skips the rest. @cipherstash/stack itself doesn't need this — it only uses the `/wasm-inline` subpath, which never loads the NAPI shim.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 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 @.github/workflows/tests.yml:
- Around line 164-176: The new GitHub Actions job "wasm-e2e-tests" exposes
credentials by omitting a permissions block and using checkout with default
persisted credentials; add a minimal permissions map (e.g., permissions:
contents: read and only other actions-required permissions) at the job level for
least privilege and update the "Checkout Repo" step (actions/checkout@v6) to
include persist-credentials: false so the runner does not leave the token in the
workspace or pass it to subsequent steps.
In `@examples/supabase-worker/.env.example`:
- Around line 4-7: The .env.example lists CS_WORKSPACE_CRN but the code does not
consume it; either remove the CS_WORKSPACE_CRN line from the example to keep
envs minimal and accurate, or if it is required, wire it into the app by reading
process.env.CS_WORKSPACE_CRN in the initialization/config loader (where other
vars like CS_CLIENT_ID/CS_CLIENT_KEY/CS_CLIENT_ACCESS_KEY are consumed) and
ensure the app uses that value; update the .env.example to match whichever
choice you make.
In `@examples/supabase-worker/package.json`:
- Around line 1-11: Add Node and pnpm enforcement to this example package
manifest by adding an "engines" entry with "node": ">=22" and a "packageManager"
field set to "pnpm@9" in the package.json; locate the top-level manifest object
in examples/supabase-worker/package.json (the file containing name, version,
description and scripts) and insert the keys "engines": {"node": ">=22"} and
"packageManager": "pnpm@9" so the example follows the repo policy.
In `@examples/supabase-worker/README.md`:
- Around line 12-50: The README's "Run locally", "Deploy" and "What this proves"
sections are missing required install and test instructions; update the README
to include an explicit "Install" step (e.g., "pnpm install" and any prerequisite
Node/pnpm versions), expand the "Run locally" section to show how to populate
.env.local and run the app locally, add a "Tests" section with the exact
command(s) to run unit/integration tests (e.g., "pnpm test" or "pnpm -w test"
and any setup needed before running tests), and append a short "Native module
externalization" note stating that native modules are externalized in the
Supabase Edge Functions build and no native dependencies are required (or list
any required externals); keep the headings consistent with "Run locally",
"Deploy" and "What this proves" so reviewers can find the changes easily.
In `@examples/supabase-worker/supabase/functions/cipherstash-roundtrip/index.ts`:
- Around line 69-77: Wrap the current response payload in the required success
contract by returning an object with a top-level data property; replace the
current Response.json body that returns ok, plaintext, decrypted,
isEncrypted(...), and ciphertextIdentifier with { data: { ok: decrypted ===
plaintext, plaintext, decrypted, isEncrypted: isEncrypted(encrypted),
ciphertextIdentifier: (encrypted as { i?: unknown }).i } } and keep the same
headers — update the Response.json call in the function returning this response
(where isEncrypted is used) so the example conforms to the { data } contract.
- Around line 79-83: The catch block currently returns raw error fields
including stack and does not follow the { failure } contract; update the catch
to map errors to a stable failure shape (e.g., return Response.json({ failure: {
type: 'internal_error', code: err.code ?? 'unknown_error', message: 'Internal
server error' } }, { status: 500 })) and remove any stack or internal detail;
locate the catch that casts to const err and replace the payload with the stable
{ failure } object (keep err.code normalized if useful but never expose
err.stack or raw messages).
In `@packages/cli/package.json`:
- Around line 53-61: The optionalDependencies block in packages/cli/package.json
pins six `@cipherstash/auth-`<platform> packages to 0.38.0 which drifts from the
main `@cipherstash/auth` catalog; fix by adding those six package names
(`@cipherstash/auth-darwin-arm64`, `@cipherstash/auth-darwin-x64`,
`@cipherstash/auth-linux-arm64-gnu`, `@cipherstash/auth-linux-x64-gnu`,
`@cipherstash/auth-linux-x64-musl`, `@cipherstash/auth-win32-x64-msvc`) to the
workspace catalog (e.g., pnpm-workspace.yaml or a dedicated catalog used by the
repo) so you can replace the hardcoded versions with catalog:repo references in
the optionalDependencies block, or alternatively keep the hardcoded pins but
document and centralize their bumping process if you cannot add them to the
catalog now.
In `@packages/stack/package.json`:
- Around line 182-187: The ./wasm-inline subpath export currently only has an
"import" branch, breaking CommonJS consumers; add a "require" branch under the
"./wasm-inline" export in package.json that points to the CJS bundle (e.g.,
"./dist/wasm-inline.cjs.js" or the existing CJS artifact), and ensure the
corresponding artifact exists in dist/publish output (or regenerate/build it via
the wasm-inline build target); update the export entry for "./wasm-inline" to
include both "import" and "require" keys so
require("`@cipherstash/stack/wasm-inline`") works.
In `@packages/stack/src/wasm-inline.ts`:
- Around line 136-142: The public constructor on WasmEncryptionClient
(constructor(client: unknown)) exposes the raw client and allows invalid
instances; make the constructor non-public (private or protected) and provide an
internal/static factory method or export a top-level Encryption() factory that
accepts the raw client and returns a WasmEncryptionClient, updating any usages
to call the factory instead of new WasmEncryptionClient; apply the same change
for the duplicate constructor occurrence referenced in the diff so construction
is hidden until the public API is stable.
- Around line 144-163: The public wrapper narrows JSON to objects only; update
the `encrypt` and `decrypt` signatures and any related typing so JSON payloads
can be null or arrays too—e.g., broaden the plaintext/return union in
`encrypt(plaintext: ...)` and `decrypt(...): Promise<...>` to include null and
array types (for example add `null` and `unknown[]` or replace with a shared
JsonValue type) and keep calls to `wasmEncrypt`/`wasmDecrypt` and the use of
`EncryptOptions`, `Encrypted`, `wasmEncrypt`, and `wasmDecrypt` unchanged.
- Around line 239-251: resolveStrategy currently silently prefers cfg.strategy
when both cfg.strategy and cfg.accessKey are provided; change it to explicitly
reject conflicting inputs by checking if both are set and throwing a clear Error
stating that accessKey and strategy are mutually exclusive. Specifically, in the
resolveStrategy function add an initial guard like "if (cfg.strategy &&
cfg.accessKey) throw new Error('[encryption]: WASM entry requires either
`config.strategy` or `config.accessKey`, not both')" otherwise keep the existing
behavior of returning cfg.strategy or creating AccessKeyStrategy via
AccessKeyStrategy.create(cfg.region ?? DEFAULT_REGION, cfg.accessKey). Ensure
the thrown message matches the existing error style and references
WasmClientConfig/conflicting inputs.
🪄 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: d4d3ba2a-662b-411f-b440-d1855a852fff
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
.github/workflows/tests.ymle2e/package.jsone2e/wasm/deno.jsone2e/wasm/roundtrip.test.tsexamples/supabase-worker/.env.exampleexamples/supabase-worker/README.mdexamples/supabase-worker/package.jsonexamples/supabase-worker/supabase/functions/cipherstash-roundtrip/index.tspackages/cli/package.jsonpackages/stack/package.jsonpackages/stack/src/wasm-inline.tspackages/stack/tsconfig.jsonpackages/stack/tsup.config.tspackages/wizard/package.jsonpnpm-workspace.yaml
Comment on lines
+1
to
+11
| { | ||
| "name": "@cipherstash/supabase-worker-example", | ||
| "private": true, | ||
| "version": "0.0.0", | ||
| "description": "CipherStash Protect inside a Supabase Edge Function, via @cipherstash/stack/wasm-inline.", | ||
| "type": "module", | ||
| "scripts": { | ||
| "//": "Run via the Supabase CLI; the function imports stack/wasm-inline (and its transitive npm: deps) at runtime, so there is no build step here.", | ||
| "serve": "supabase functions serve --env-file .env.local cipherstash-roundtrip" | ||
| } | ||
| } |
There was a problem hiding this comment.
Declare required Node and pnpm versions in the example manifest.
This manifest should explicitly enforce Node.js >= 22 and pnpm 9.x per repo policy.
Suggested update
{
"name": "`@cipherstash/supabase-worker-example`",
"private": true,
"version": "0.0.0",
"description": "CipherStash Protect inside a Supabase Edge Function, via `@cipherstash/stack/wasm-inline`.",
"type": "module",
+ "packageManager": "pnpm@9",
+ "engines": {
+ "node": ">=22"
+ },
"scripts": {
"//": "Run via the Supabase CLI; the function imports stack/wasm-inline (and its transitive npm: deps) at runtime, so there is no build step here.",
"serve": "supabase functions serve --env-file .env.local cipherstash-roundtrip"
}
}As per coding guidelines, examples/**/package.json must use Node.js >= 22 and pnpm 9.x.
📝 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.
Suggested change
| { | |
| "name": "@cipherstash/supabase-worker-example", | |
| "private": true, | |
| "version": "0.0.0", | |
| "description": "CipherStash Protect inside a Supabase Edge Function, via @cipherstash/stack/wasm-inline.", | |
| "type": "module", | |
| "scripts": { | |
| "//": "Run via the Supabase CLI; the function imports stack/wasm-inline (and its transitive npm: deps) at runtime, so there is no build step here.", | |
| "serve": "supabase functions serve --env-file .env.local cipherstash-roundtrip" | |
| } | |
| } | |
| { | |
| "name": "`@cipherstash/supabase-worker-example`", | |
| "private": true, | |
| "version": "0.0.0", | |
| "description": "CipherStash Protect inside a Supabase Edge Function, via `@cipherstash/stack/wasm-inline`.", | |
| "type": "module", | |
| "packageManager": "pnpm@9", | |
| "engines": { | |
| "node": ">=22" | |
| }, | |
| "scripts": { | |
| "//": "Run via the Supabase CLI; the function imports stack/wasm-inline (and its transitive npm: deps) at runtime, so there is no build step here.", | |
| "serve": "supabase functions serve --env-file .env.local cipherstash-roundtrip" | |
| } | |
| } |
🤖 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 `@examples/supabase-worker/package.json` around lines 1 - 11, Add Node and pnpm
enforcement to this example package manifest by adding an "engines" entry with
"node": ">=22" and a "packageManager" field set to "pnpm@9" in the package.json;
locate the top-level manifest object in examples/supabase-worker/package.json
(the file containing name, version, description and scripts) and insert the keys
"engines": {"node": ">=22"} and "packageManager": "pnpm@9" so the example
follows the repo policy.
Comment on lines
+12
to
+50
| ## Run locally | ||
|
|
||
| ```sh | ||
| cp .env.example .env.local | ||
| # fill in CS_WORKSPACE_CRN, CS_CLIENT_ID, CS_CLIENT_KEY, CS_CLIENT_ACCESS_KEY | ||
|
|
||
| supabase functions serve --env-file .env.local cipherstash-roundtrip | ||
| ``` | ||
|
|
||
| In another shell: | ||
|
|
||
| ```sh | ||
| curl -s http://localhost:54321/functions/v1/cipherstash-roundtrip | jq | ||
| ``` | ||
|
|
||
| Expected response: | ||
|
|
||
| ```json | ||
| { | ||
| "ok": true, | ||
| "plaintext": "alice@example.com", | ||
| "decrypted": "alice@example.com", | ||
| "isEncrypted": true, | ||
| "ciphertextIdentifier": { "t": "users", "c": "email" } | ||
| } | ||
| ``` | ||
|
|
||
| ## Deploy | ||
|
|
||
| ```sh | ||
| supabase functions deploy cipherstash-roundtrip | ||
| supabase secrets set --env-file .env.local | ||
| ``` | ||
|
|
||
| ## What this proves | ||
|
|
||
| - Protect's WASM build works inside Supabase Edge Functions. | ||
| - The full `@cipherstash/stack/wasm-inline` developer surface (`Encryption`, `encryptedTable`, `encryptedColumn`, …) is usable from an Edge Function with no native dependencies. | ||
| - A CipherStash service-to-service `AccessKeyStrategy` is the right credential shape for serverless / edge environments. |
There was a problem hiding this comment.
README is missing required install and test instructions.
Please add explicit install steps (e.g., pnpm install) and a “how to run tests” section to satisfy the example README contract.
As per coding guidelines, each examples/**/README.md must cover setup (env vars, install, run commands), native module externalization notes, and how to run tests.
🤖 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 `@examples/supabase-worker/README.md` around lines 12 - 50, The README's "Run
locally", "Deploy" and "What this proves" sections are missing required install
and test instructions; update the README to include an explicit "Install" step
(e.g., "pnpm install" and any prerequisite Node/pnpm versions), expand the "Run
locally" section to show how to populate .env.local and run the app locally, add
a "Tests" section with the exact command(s) to run unit/integration tests (e.g.,
"pnpm test" or "pnpm -w test" and any setup needed before running tests), and
append a short "Native module externalization" note stating that native modules
are externalized in the Supabase Edge Functions build and no native dependencies
are required (or list any required externals); keep the headings consistent with
"Run locally", "Deploy" and "What this proves" so reviewers can find the changes
easily.
Comment on lines
+69
to
+77
| return Response.json( | ||
| { | ||
| ok: decrypted === plaintext, | ||
| plaintext, | ||
| decrypted, | ||
| isEncrypted: isEncrypted(encrypted), | ||
| ciphertextIdentifier: (encrypted as { i?: unknown }).i, | ||
| }, | ||
| { headers: { 'content-type': 'application/json' } }, |
There was a problem hiding this comment.
Use the required { data } success contract shape.
The success payload currently returns ad-hoc top-level fields (ok, plaintext, decrypted, etc.) instead of { data: ... }, which breaks the example-app contract.
Suggested shape
- return Response.json(
- {
- ok: decrypted === plaintext,
- plaintext,
- decrypted,
- isEncrypted: isEncrypted(encrypted),
- ciphertextIdentifier: (encrypted as { i?: unknown }).i,
- },
- { headers: { 'content-type': 'application/json' } },
- )
+ return Response.json(
+ {
+ data: {
+ ok: decrypted === plaintext,
+ isEncrypted: isEncrypted(encrypted),
+ ciphertextIdentifier: (encrypted as { i?: unknown }).i,
+ },
+ },
+ { headers: { 'content-type': 'application/json' } },
+ )As per coding guidelines, examples operations must return { data } or { failure } with stable error type strings.
🤖 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 `@examples/supabase-worker/supabase/functions/cipherstash-roundtrip/index.ts`
around lines 69 - 77, Wrap the current response payload in the required success
contract by returning an object with a top-level data property; replace the
current Response.json body that returns ok, plaintext, decrypted,
isEncrypted(...), and ciphertextIdentifier with { data: { ok: decrypted ===
plaintext, plaintext, decrypted, isEncrypted: isEncrypted(encrypted),
ciphertextIdentifier: (encrypted as { i?: unknown }).i } } and keep the same
headers — update the Response.json call in the function returning this response
(where isEncrypted is used) so the example conforms to the { data } contract.
Comment on lines
+182
to
+187
| "./wasm-inline": { | ||
| "import": { | ||
| "types": "./dist/wasm-inline.d.ts", | ||
| "default": "./dist/wasm-inline.js" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== wasm-inline export entry ==="
jq '.exports["./wasm-inline"]' packages/stack/package.json
echo "=== require branch present? ==="
if jq -e '.exports["./wasm-inline"].require' packages/stack/package.json >/dev/null; then
echo "OK: require branch exists"
else
echo "FAIL: require branch missing"
fi
echo "=== build config references to wasm-inline CJS cleanup ==="
rg -n 'wasm-inline\.cjs|onSuccess|format:\s*\[' packages/stack/tsup.config.tsRepository: cipherstash/stack
Length of output: 511
Restore CJS subpath export for ./wasm-inline
packages/stack/package.json exports ./wasm-inline with only an "import" branch; there is no "require" entry, so require("@cipherstash/stack/wasm-inline") isn’t supported. Keep a CJS ("require") export for this subpath (and ensure the corresponding artifact is present in dist/publish output) or move this to an explicit breaking-change release boundary.
Suggested export fix
"./wasm-inline": {
"import": {
"types": "./dist/wasm-inline.d.ts",
"default": "./dist/wasm-inline.js"
+ },
+ "require": {
+ "types": "./dist/wasm-inline.d.cts",
+ "default": "./dist/wasm-inline.cjs"
}
},📝 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.
Suggested change
| "./wasm-inline": { | |
| "import": { | |
| "types": "./dist/wasm-inline.d.ts", | |
| "default": "./dist/wasm-inline.js" | |
| } | |
| }, | |
| "./wasm-inline": { | |
| "import": { | |
| "types": "./dist/wasm-inline.d.ts", | |
| "default": "./dist/wasm-inline.js" | |
| }, | |
| "require": { | |
| "types": "./dist/wasm-inline.d.cts", | |
| "default": "./dist/wasm-inline.cjs" | |
| } | |
| }, |
🤖 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/package.json` around lines 182 - 187, The ./wasm-inline
subpath export currently only has an "import" branch, breaking CommonJS
consumers; add a "require" branch under the "./wasm-inline" export in
package.json that points to the CJS bundle (e.g., "./dist/wasm-inline.cjs.js" or
the existing CJS artifact), and ensure the corresponding artifact exists in
dist/publish output (or regenerate/build it via the wasm-inline build target);
update the export entry for "./wasm-inline" to include both "import" and
"require" keys so require("`@cipherstash/stack/wasm-inline`") works.
Comment on lines
+144
to
+163
| async encrypt( | ||
| plaintext: string | number | boolean | Record<string, unknown>, | ||
| opts: EncryptOptions, | ||
| ): Promise<Encrypted> { | ||
| const ffiOpts = { | ||
| plaintext, | ||
| table: opts.table.tableName, | ||
| column: getColumnName(opts.column), | ||
| } | ||
| return (await wasmEncrypt( | ||
| this.client as never, | ||
| ffiOpts as never, | ||
| )) as Encrypted | ||
| } | ||
|
|
||
| async decrypt(encrypted: Encrypted): Promise<string | number | boolean | Record<string, unknown>> { | ||
| return (await wasmDecrypt(this.client as never, { | ||
| ciphertext: encrypted, | ||
| } as never)) as string | number | boolean | Record<string, unknown> | ||
| } |
There was a problem hiding this comment.
Broaden the JSON value types on the public wrapper.
The schema layer already supports cast_as: 'json', but this wrapper only types JSON payloads as objects. That blocks valid null and array payloads for TS callers on the new wasm-inline API, and the same restriction leaks into decrypt().
💡 Proposed fix
async encrypt(
- plaintext: string | number | boolean | Record<string, unknown>,
+ plaintext: string | number | boolean | null | unknown[] | Record<string, unknown>,
opts: EncryptOptions,
): Promise<Encrypted> {
@@
- async decrypt(encrypted: Encrypted): Promise<string | number | boolean | Record<string, unknown>> {
+ async decrypt(encrypted: Encrypted): Promise<string | number | boolean | null | unknown[] | Record<string, unknown>> {
return (await wasmDecrypt(this.client as never, {
ciphertext: encrypted,
- } as never)) as string | number | boolean | Record<string, unknown>
+ } as never)) as string | number | boolean | null | unknown[] | Record<string, unknown>
}🤖 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/wasm-inline.ts` around lines 144 - 163, The public wrapper
narrows JSON to objects only; update the `encrypt` and `decrypt` signatures and
any related typing so JSON payloads can be null or arrays too—e.g., broaden the
plaintext/return union in `encrypt(plaintext: ...)` and `decrypt(...):
Promise<...>` to include null and array types (for example add `null` and
`unknown[]` or replace with a shared JsonValue type) and keep calls to
`wasmEncrypt`/`wasmDecrypt` and the use of `EncryptOptions`, `Encrypted`,
`wasmEncrypt`, and `wasmDecrypt` unchanged.
Six fixes from a recall-biased high-effort review:
1. Bump @cipherstash/stack 0.17.0 → 0.18.0 so the Supabase example's
`npm:@cipherstash/stack@^0.18.0/wasm-inline` import resolves once
this lands on npm. The minor bump matches the new public subpath.
2. Make `WasmClientConfig.clientId` and `clientKey` required (was `?`),
and use a discriminated union for `accessKey` vs `strategy` so the
compiler enforces one-or-the-other. Previously a `{region, accessKey}`
config typechecked but forwarded `{clientId: undefined, clientKey:
undefined}` to the WASM client and failed authentication at first
encrypt.
3. `normalizeCastAs` now throws synchronously if `toEqlCastAs` returns
`undefined` (i.e. someone added a new SDK-facing cast_as variant
without extending the switch). The previous behaviour silently
handed `undefined` to the WASM serde, which surfaced as an opaque
`unknown variant 'null'` startup crash.
4. Drop the `decryptRaw` / `encryptRaw` / `newClientRaw` /
`AccessKeyStrategy` re-exports. They were a footgun — a consumer
reaching `newClientRaw` bypasses `normalizeCastAs` and hits the
exact same `unknown variant 'string'` failure the high-level
`Encryption()` was patched to avoid. Anyone who needs raw access
can import from `@cipherstash/protect-ffi/wasm-inline` or
`@cipherstash/auth/wasm-inline` directly.
5. Move the six `@cipherstash/auth-<platform>` packages into the
`pnpm-workspace.yaml` catalog and reference them via `catalog:repo`
from `wizard` and `cli`'s `optionalDependencies`. Previously each
sub-package was hard-pinned to the literal `0.38.0`, so a catalog
bump of `@cipherstash/auth` alone would drift the platform binary
versions and break the NAPI loader.
6. The `wasm-e2e-tests` job now asserts the four required `CS_*`
secrets are present BEFORE running `deno task test`. Without this,
a rotated / cleared CI secret made the test silently `ignore` and
the job reported green, hiding any real WASM regression.
tobyhede
reviewed
May 27, 2026
|
|
||
| function resolveStrategy( | ||
| cfg: WasmClientConfig, | ||
| ): { getToken(): Promise<unknown> } { |
Contributor
There was a problem hiding this comment.
Return shape is very broad, can the AccessKeyStrategy.create type be used directly?
Contributor
Author
There was a problem hiding this comment.
Probably should be. Let me investigate.
| } catch (e) { | ||
| const err = e as { code?: string; message?: string; stack?: string } | ||
| return Response.json( | ||
| { code: err.code, message: err.message, stack: err.stack }, |
Contributor
There was a problem hiding this comment.
Is a demo, but error handler leaks stack traces.
Consider dropping the stack or adding a comment for users that it's debug-only.
| node-version: 22 | ||
| cache: 'pnpm' | ||
|
|
||
| - name: Install node-gyp |
Contributor
There was a problem hiding this comment.
Installing node-gyp is in conflict with the goal of "no native bindings"
Confirm and add note to explain if required.
Contributor
Author
There was a problem hiding this comment.
Hmm yeah not sure. Good catch. Will investigate.
tobyhede
approved these changes
May 27, 2026
| // ----------------------------------------------------------------------- | ||
|
|
||
| /** Default region used when `WasmClientConfig.region` is unset. */ | ||
| const DEFAULT_REGION = 'ap-southeast-2.aws' |
Contributor
There was a problem hiding this comment.
Consider making this a US region.
| // `require` branch for ./wasm-inline so npm consumers never reach | ||
| // this path. The dual ESM + CJS d.ts pair stays so type-only CJS | ||
| // imports of stack's public surface still resolve. | ||
| onSuccess: async () => { |
Contributor
There was a problem hiding this comment.
Source: Claude
- Format-strip via onSuccess is brittle (packages/stack/tsup.config.ts:32-41)
You build CJS for every entry then rm dist/wasm-inline.cjs{,.map}. If a future tsup version emits
additional artifacts (.d.cts for instance — which you currently want to keep), the deny-list will
silently drift. A more declarative approach is to use two defineConfig entries (one ESM-only for
wasm-inline, one dual for the rest) — which is what was there pre-PR. The current consolidation
trades clarity for a single entry array; the prior shape was easier to reason about.
| * than handing `undefined` to the WASM serde (which surfaces as an | ||
| * opaque `unknown variant 'null'` error). | ||
| */ | ||
| function normalizeCastAs(config: EncryptConfig): unknown { |
Contributor
There was a problem hiding this comment.
Function tested end-to-end, but unit test would catch a schema/toEqlCastAs drift before the e2e step.
Twelve fixes from a high-effort review pass:
CI hardening:
- Add least-privilege permissions: contents: read to the new
wasm-e2e-tests job; disable persist-credentials on checkout.
- Document node-gyp install — it's for the workspace-wide pnpm
install (node-pty), not the WASM path itself.
Supabase worker example:
- Drop CS_WORKSPACE_CRN from .env.example — the worker doesn't
read it (parity gap with the Node entry tracked separately).
- Sanitise the 500 response: log err.stack to the operator, never
return it to the caller. Mark the response shape debug-only.
- Add engines.node + packageManager to follow repo policy.
- Expand README: install step, native-modules note, and a brief
"what this verifies" / "automated coverage" pointer to the
Deno smoke test in e2e/wasm/.
@cipherstash/stack/wasm-inline:
- Hide WasmEncryptionClient constructor behind an internal
symbol-gated factory so callers can't wrap arbitrary objects.
- Broaden plaintext type to include arrays (WasmPlaintext is
recursive, matching protect-ffi's JsPlaintext).
- Tighten the strategy type to the actual AccessKeyStrategy class
from @cipherstash/auth/wasm-inline instead of a structural
duck-type. Custom token stores still flow via
AccessKeyStrategy.create({ store }).
- Mark region @deprecated — a follow-up will replace it with a
workspaceCrn that the auth strategy derives region from
(tracked in a separate bug).
- Export normalizeCastAs for unit-test coverage.
Build pipeline:
- Split tsup back into two configs: main entries dual ESM+CJS,
wasm-inline ESM-only. Move dist cleanup to a prebuild npm
script so both configs run with clean: false and don't race.
The previous onSuccess deny-list was brittle (Toby/Claude).
- Side-effect: dist/wasm-inline.d.cts no longer emitted, closing
the orphan-types-file concern from the same review.
Tests:
- New __tests__/wasm-inline-normalize.test.ts: exercises every
CastAs variant, asserts toEqlCastAs is exhaustive, and verifies
normalizeCastAs throws on drift instead of handing undefined
to WASM serde. Catches future enum additions before the
e2e step.
CodeRabbit findings deferred:
- The "{ data } success contract" and "{ failure } error shape"
are skipped — no such convention in this repo's other examples.
- Restoring CJS subpath for wasm-inline skipped — protect-ffi's
wasm-inline runtime is ESM-only; a require branch would
re-introduce ERR_REQUIRE_ESM for downstream consumers.
There was a problem hiding this comment.
♻️ Duplicate comments (3)
packages/stack/src/wasm-inline.ts (3)
314-317:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a runtime guard for conflicting
strategy+accessKeyinputs.Line 315 relies on TS narrowing only; JS callers can still supply both fields and get silent precedence behavior.
Suggested patch
function resolveStrategy(cfg: WasmClientConfig): AccessKeyStrategy { + if ((cfg as { strategy?: unknown }).strategy && (cfg as { accessKey?: unknown }).accessKey) { + throw new Error( + '[encryption]: `config.strategy` and `config.accessKey` are mutually exclusive', + ) + } if (cfg.strategy) return cfg.strategy // Discriminated union guarantees this branch implies `accessKey` is set. return AccessKeyStrategy.create(cfg.region, cfg.accessKey as string) }🤖 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/wasm-inline.ts` around lines 314 - 317, In resolveStrategy, add a runtime guard that throws a clear error when both cfg.strategy and cfg.accessKey are provided (since TS narrowing isn't enforced at runtime); specifically, at the top of the function check if cfg.strategy !== undefined && cfg.accessKey !== undefined and throw an Error explaining the conflicting inputs, otherwise preserve the existing behavior of returning cfg.strategy if present or calling AccessKeyStrategy.create(cfg.region, cfg.accessKey as string).
198-201:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
WasmEncryptionClient.__constructis publicly callable and re-opens arbitrary client wrapping.Line 199 exposes a public static constructor path, so callers can still instantiate
WasmEncryptionClientwith invalid raw objects.Suggested patch
export class WasmEncryptionClient { @@ - /** `@internal` */ - static __construct(client: unknown): WasmEncryptionClient { - return new WasmEncryptionClient(INTERNAL_CONSTRUCT, client) + /** `@internal` */ + static __construct(token: symbol, client: unknown): WasmEncryptionClient { + if (token !== INTERNAL_CONSTRUCT) { + throw new Error( + '[encryption]: WasmEncryptionClient cannot be constructed directly — use the Encryption() factory.', + ) + } + return new WasmEncryptionClient(INTERNAL_CONSTRUCT, client) }🤖 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/wasm-inline.ts` around lines 198 - 201, The static method WasmEncryptionClient.__construct is currently publicly callable despite the `@internal` tag, allowing arbitrary objects to be wrapped; make this factory inaccessible by either (a) marking __construct as a private static method (so external callers cannot call WasmEncryptionClient.__construct) or (b) hardening the constructor guard: change the constructor to require a unique, module-scoped Symbol and assert the passed token equals INTERNAL_CONSTRUCT (throw if not), and update __construct to pass that Symbol—refer to WasmEncryptionClient.__construct, the class constructor, and INTERNAL_CONSTRUCT when making this change.
108-113:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
WasmPlaintextstill omitsnull, which narrows JSON payload support.The public wasm-inline wrapper accepts arrays now, but Line 108-113 / Line 218-221 still type-exclude
null, which can block valid JSON plaintext/decrypt flows for TS consumers.Suggested patch
export type WasmPlaintext = | string | number | boolean + | null | Record<string, unknown> | WasmPlaintext[]In `@cipherstash/protect-ffi` v0.24.0 (wasm-inline), what is the exact plaintext type accepted by encrypt/decrypt (e.g., JsPlaintext)? Does it include null?Also applies to: 218-221
🤖 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/wasm-inline.ts` around lines 108 - 113, WasmPlaintext currently omits null which prevents valid JSON/null values from passing through encrypt/decrypt; update the WasmPlaintext type declaration (export type WasmPlaintext) to include null in the union and ensure any other plaintext aliases (e.g., the corresponding JsPlaintext or other exported plaintext types referenced later around the second occurrence) are updated the same way so arrays can contain null elements too; locate the WasmPlaintext type and the repeated definition near the other mention (lines around the second occurrence) and add null to the union in both places.
🤖 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.
Duplicate comments:
In `@packages/stack/src/wasm-inline.ts`:
- Around line 314-317: In resolveStrategy, add a runtime guard that throws a
clear error when both cfg.strategy and cfg.accessKey are provided (since TS
narrowing isn't enforced at runtime); specifically, at the top of the function
check if cfg.strategy !== undefined && cfg.accessKey !== undefined and throw an
Error explaining the conflicting inputs, otherwise preserve the existing
behavior of returning cfg.strategy if present or calling
AccessKeyStrategy.create(cfg.region, cfg.accessKey as string).
- Around line 198-201: The static method WasmEncryptionClient.__construct is
currently publicly callable despite the `@internal` tag, allowing arbitrary
objects to be wrapped; make this factory inaccessible by either (a) marking
__construct as a private static method (so external callers cannot call
WasmEncryptionClient.__construct) or (b) hardening the constructor guard: change
the constructor to require a unique, module-scoped Symbol and assert the passed
token equals INTERNAL_CONSTRUCT (throw if not), and update __construct to pass
that Symbol—refer to WasmEncryptionClient.__construct, the class constructor,
and INTERNAL_CONSTRUCT when making this change.
- Around line 108-113: WasmPlaintext currently omits null which prevents valid
JSON/null values from passing through encrypt/decrypt; update the WasmPlaintext
type declaration (export type WasmPlaintext) to include null in the union and
ensure any other plaintext aliases (e.g., the corresponding JsPlaintext or other
exported plaintext types referenced later around the second occurrence) are
updated the same way so arrays can contain null elements too; locate the
WasmPlaintext type and the repeated definition near the other mention (lines
around the second occurrence) and add null to the union in both places.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b9b4008-4b93-4358-8b4d-0139d1a78b55
📒 Files selected for processing (10)
.github/workflows/tests.ymle2e/wasm/roundtrip.test.tsexamples/supabase-worker/.env.exampleexamples/supabase-worker/README.mdexamples/supabase-worker/package.jsonexamples/supabase-worker/supabase/functions/cipherstash-roundtrip/index.tspackages/stack/__tests__/wasm-inline-normalize.test.tspackages/stack/package.jsonpackages/stack/src/wasm-inline.tspackages/stack/tsup.config.ts
✅ Files skipped from review due to trivial changes (2)
- examples/supabase-worker/package.json
- examples/supabase-worker/README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/wasm/roundtrip.test.ts
- packages/stack/package.json
- examples/supabase-worker/supabase/functions/cipherstash-roundtrip/index.ts
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
2 participants
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.
Summary
@cipherstash/stack/wasm-inlinesubpath so Protect runs in Deno, Bun, Cloudflare Workers, Supabase Edge, browsers — anywhere the native protect-ffi NAPI bindings don't work.--allow-ffi) verifies the WASM path is genuinely WASM, not a silent native fallback.Dependency bumps
@cipherstash/protect-ffi0.23.0 → 0.24.0 (Node API unchanged — strategy-basednewClientonly lives on/wasm-inline).@cipherstash/authcatalog → 0.37.0-alpha.8 for the WASMAccessKeyStrategy. Both packages added topnpm-workspace.yaml's release-age exclude.Draft status
Blocking on a non-alpha
@cipherstash/authrelease with/wasm-inline— planned for0.38.0. Bump the catalog when that ships.What changed
packages/stack/src/wasm-inline.tsexportsEncryption()+WasmEncryptionClient(encrypt / decrypt round-trip) wired to WASM protect-ffi +AccessKeyStrategyfrom auth/wasm-inline, plus raw re-exports (newClientRaw,encryptRaw,decryptRaw,isEncrypted,AccessKeyStrategy) for consumers who want the lower-level API.require()(ERR_REQUIRE_ESM). tsup dropsdist/wasm-inline.cjsviaonSuccess;package.jsonexports omits therequirebranch for./wasm-inline.tsconfig.jsonaddscustomConditions: ["node"]so bundler resolution picks Node protect-ffi types for the main entries (the/wasm-inlinesubpath has unconditional types and is unaffected).e2e/wasm/— Deno smoke test that round-trips an encryption against ZeroKMS/CTS. No--allow-ffi— if anything ever silently fell back to a native binding, the test would fail loudly.examples/supabase-worker/— port of a working spike, refactored to import from@cipherstash/stack/wasm-inlineinstead of going direct to@cipherstash/protect-ffi/wasm-inline+@cipherstash/auth/wasm-inline.wasm-e2e-testsCI job (Deno 2.x on blacksmith runner) exercises the smoke test on every PR. Exposes only the fourCS_*secrets — no debug-only host overrides.Test plan
pnpm exec turbo run build --filter @cipherstash/stackproducesdist/wasm-inline.{js,d.ts,d.cts}(no.cjs).pnpm test --exclude searchable-json-pg --exclude supabase).cjs-require.test.tspasses — confirms thedist/wasm-inline.cjsdrop is doing its job.wasm-e2e-testsCI job runs green on this PR.cd examples/supabase-worker && supabase functions serve --env-file .env.local cipherstash-roundtripreturns{ ok: true }.@cipherstash/protect-ffi-{platform}native binary and rerun the Deno test — must still pass.Notes
packages/protectstill pinsprotect-ffi@0.23.0— out of scope for this PR.eql-2.3.1intests.yml. 0.24.0 is WASM-focused (not an EQL format bump), so it should be fine, but ifrun-testsfails post-bump, the image needs to move too.Summary by CodeRabbit
New Features
Tests
Documentation
Chores