Pin the WASM clientKey contract; document /wasm-inline as server-side only (#804) - #810
Draft
tobyhede wants to merge 5 commits into
Draft
Pin the WASM clientKey contract; document /wasm-inline as server-side only (#804)#810tobyhede wants to merge 5 commits into
tobyhede wants to merge 5 commits into
Conversation
🦋 Changeset detectedLatest commit: 44c82d0 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 skippedAuto reviews are disabled on base/target branches other than the default branch. 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 |
tobyhede
force-pushed
the
fix/804-wasm-clientkey-browser-contract
branch
from
July 28, 2026 00:50
f83bb6d to
fc04e28
Compare
…erver-side only (#804) `WasmClientConfig` requires `clientId` / `clientKey` on every auth arm, including the `authStrategy` (OIDC federation) arm — the arm that exists so a caller never handles a workspace secret. That looked like it might be a leftover from the access-key path that the type over-declares, which would have meant a browser could hold a client authorised by a federated JWT alone. It is not. Verified against the real core (protect-ffi 0.29.0 and 0.30.0): - Omitting either field rejects during deserialisation of `NewClientOpts` (``missing field `clientKey` ``) regardless of the strategy supplied. - `clientKey` is decoded in two stages, both before the strategy is consulted: hex, then a key provider that deserialises it into cryptographic key material (the core names `p1` as a `Permutation` — a keyed permutation for the searchable-index schemes). Nothing that merely validated a credential's format would decode a permutation out of it, and `getToken` is never called on any failure path, so this is not an auth input federation could displace. - It is a workspace secret (`stash env` writes it under "these are secrets, do not commit them"), so no configuration of this entry belongs in a browser bundle. Hence no `browser` export condition. No behaviour change. What changes is that the constraint is now stated where callers meet it and enforced instead of assumed: - A contract test that runs against the REAL WASM core. Every other wasm suite mocks `newClient`, and `vitest.shared.ts` aliases the whole `@cipherstash/protect-ffi/wasm-inline` specifier to a throwing stub — which is why this was never caught. The test resolves the module through Node's resolver, which the Vite alias does not intercept. It runs offline: every failure it asserts occurs before any ZeroKMS / CTS call. If it ever fails, the core changed and browser support is worth revisiting. - `WasmClientConfig` gains a NOT BROWSER-SAFE note and per-field docs saying `clientKey` is secret key material consumed independently of the strategy. - The `OidcFederationStrategy` re-export notes that "edge" means a server-side runtime, and that federation changes who a request authenticates AS without removing the `clientKey` requirement. - `skills/stash-encryption` marks the entry server-side only in the entry table. Still blocked on the core, now for a verified reason rather than a suspicion: the `browser` export condition (#805), a live browser smoke test, and browser guidance in `skills/stash-supabase`. A browser-safe shape is a Rust-side design job, not a JS one.
#777 added `skills/stash-edge`, and AGENTS.md now routes any change to `packages/stack/src/wasm-inline.ts` through it. The skill's federation section is exactly where the #804 mistake gets made: it teaches that an auth strategy authenticates the client as the end user, which invites the inference that the entry is therefore browser-safe. It is not. `clientKey` is required on every auth path including `authStrategy`, and it is a workspace secret the core decodes into cryptographic key material. Add a warning alongside the existing auth-vs-key-binding callout, which already draws the neighbouring distinction.
tobyhede
force-pushed
the
fix/804-wasm-clientkey-browser-contract
branch
from
July 28, 2026 01:08
fc04e28 to
d9e90a7
Compare
…contract (#804) Review of #810 found the contract test proved less than it claimed, and that two of its stated premises were wrong. Both are fixed by testing them. `expect(calls.getToken).toBe(0)` had no positive control. Nothing in the file ever reached a state where `getToken` was called, so a count of zero was equally consistent with "key loading precedes auth" (the claim) and with "the core never calls `getToken` during `newClient` at all" — a counter that is never incremented reads as zero either way. It does call it. Structurally complete key material — derived from the core's own error messages, not from any credential: `{ p1, p2_from, p2_to, p3 }`, each a `Permutation` — clears the key provider, after which the core calls `getToken` exactly once and fails on the deliberately malformed token the stand-in returns. So auth IS reached during construction, only after the key is loaded, and the counter is live. Still offline: the token never parses, so nothing leaves the process. The claim that a rename of `opts.strategy` would silently hollow out the file turned out to be false, and is now pinned rather than assumed: the core reads the strategy by name BEFORE deserialising the credentials, so omitting it yields `opts.strategy is required`, which matches none of the other tests' regexes. They would fail loudly, not pass vacuously. Also pins the consequence the docs assert but nothing enforced: no `browser` export condition on any subpath. Adding one to quiet a bundler would ship a workspace secret to the browser and leave `src/wasm-inline.ts` silently wrong. Three corrections to comments that overstated the evidence: - "the strategy was never consulted" → never INVOKED. The core does read `opts.strategy` and typecheck its `getToken` ahead of serde; what never happens is the call. - "every other wasm test mocks `newClient`" → every wasm test that constructs a client. `wasm-inline-normalize.test.ts` relies on the alias stub, and `wasm-inline-bundle-isolation.test.ts` never loads protect-ffi at all. - "a stub that throws" → a stub whose `newClient` throws. Its `isEncrypted` returns `false`. The `e2e/wasm/roundtrip.test.ts` pointer now notes that suite covers the `accessKey` arm, so the federation arm reasoned about here has no live coverage anywhere. Fixes the stub's own docblock while adjacent: it claimed protect-ffi exports no `/wasm-inline` subpath, which 0.30.0 does, and which the contract test resolves directly.
#810 documented `@cipherstash/stack/wasm-inline` as server-side only, but left three places on this branch saying the opposite — including the example a reader is most likely to copy. - `examples/supabase-worker/README.md` and its edge function both listed "modern browsers" among the runtimes the entry works in. The example reads `CS_CLIENT_KEY` from the environment, so it demonstrated the exact thing it claimed was browser-safe. - `packages/stack/tsup.config.ts` listed browsers among "the only runtimes that need wasm-inline", in the same file #810 cites as evidence there is no `browser` export condition. - `skills/stash-supabase` gave `pg` introspection as the reason the factory cannot run in a browser. True but incomplete, and misleading next to #805 ("add the `browser` export condition"): removing `pg` unblocks Workers, not browsers, because `clientKey` is required on every auth path regardless. Also corrects two claims in #810's own prose. `clientKey` is loaded before the core ever CALLS the strategy — it reads `opts.strategy` earlier than that — and the contract test displaces mocks in eight suites plus a stub in one, not "the stub every other wasm suite uses".
Contributor
Author
|
Work paused pending |
…shes mid-walk `lint-no-hardcoded-runners.mjs` walks the packages tree with a recursive `readdir`. The repo's own script self-tests create and delete probe package directories in that same tree while other suites run, and vitest ran those suites in parallel — so the walk could enumerate a probe directory and find it gone before recursing into it. Unhandled, that rejects with ENOENT and Node exits 1: the same code the linter uses for "found a hardcoded npx". CI then failed naming nothing, on a tree the identical script had just passed one step earlier, and a re-run went green. The script already draws this distinction for a missing target — "Exit 2 means the linter could not run" — but only at the top level, not one frame down. Observed on three branches before this one (30316404472, 30317589094), which is why it reads as a flake rather than a defect. Two changes: - `walk` treats ENOENT as "nothing here to lint" and skips. A directory that no longer exists cannot contain a violation, and crashing with the violation exit code is the actual bug. - `scripts/vitest.config.mjs` sets `fileParallelism: false`. Several of these suites mutate the real repo tree, so cross-file parallelism is unsound here regardless of this particular pair. Eleven small files; it costs a second. Verified by reproducing the race locally — churning a probe directory while running the linter failed 6/30 before, 0/40 after. One wrinkle worth recording: the first draft of these comments named the probe directories with their `packages/` prefix, which the sibling `lint-no-dead-package-paths` linter correctly flagged as references to directories that do not exist. The names are now written bare.
tobyhede
marked this pull request as draft
July 28, 2026 01:45
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.
Closes #804.
The question
WasmClientConfigrequiresclientId/clientKeyon every auth arm — including theauthStrategy(OIDC federation) arm, the one that exists so a caller never handles a workspace secret. #804 asked whether that was a leftover from the access-key path that the type over-declares. If it were, a browser could hold a client authorised by a federated JWT alone, and the whole portable-runtime browser story would be unblocked.It is not a leftover.
What I verified
Against the real core (protect-ffi 0.29.0 and 0.30.0 — a stable contract, not a version quirk):
NewClientOptswithmissing field `clientKey`, regardless of the strategy supplied.clientKeydecodes in two stages, both ahead of the strategy: hex, then a key provider.getTokenreports zero calls on every failure path — it is not an auth input that federation could displace.CS_CLIENT_KEYis hex of a CBOR-serialised key struct. Reaching into it, the core namesp1as aPermutation— a keyed permutation for the searchable-index schemes. Nothing that merely validated a credential's format decodes a permutation out of it.packages/cli/src/commands/env/index.tswrites it under the literal line# CS_CLIENT_KEY and CS_CLIENT_ACCESS_KEY are secrets. Do not commit them.The repo already asserted this, in
packages/stack/src/types.tsonClientConfig.authStrategy: "theclientKeyis still required for encryption". Node types the fields optional only because protect-ffi backfills them from env vars; the WASM entry has no environment to read, so the type demands them. Same constraint, different defaulting — which is why it reads as a WASM-specific quirk but isn't.What this PR changes
No behaviour change. Types and runtime are untouched. What changes is that the constraint is now stated where callers meet it, and enforced instead of assumed.
packages/stack/__tests__/wasm-inline-core-credential-contract.test.ts(new) — the point of the PR. Every other wasm suite mocksnewClient, andvitest.shared.tsaliases the whole@cipherstash/protect-ffi/wasm-inlinespecifier to a throwing stub, which is exactly why this was never caught. This file resolves the module through Node's resolver, which the Vite alias does not intercept, and asserts against the actual core. Runs offline — every failure it asserts happens before any ZeroKMS / CTS call.packages/stack/src/wasm-inline.ts— aNOT BROWSER-SAFE (#804)note onWasmClientConfig, per-field docs explaining thatclientKeyis secret key material consumed independently of the strategy, and a note on theOidcFederationStrategyre-export that "edge" means a server-side runtime. Verified the note ships in the built.d.ts.skills/stash-encryption/SKILL.md— the entry-point table now marks/wasm-inlineserver-side only and says why. Every other skill already described it as the "edge" entry and never claimed browser, so nothing else needed changing.@cipherstash/stack+stash, both patch (skills/ships in thestashtarball).Consequences
Still blocked, now for a verified reason rather than a suspicion: the
browserexport condition (#805), a live browser smoke test, and browser guidance inskills/stash-supabase. A browser-safe auth shape is a Rust-side design job, not a JS one.If the new test ever fails, that is good news — it means the core relaxed the requirement and browser support should be re-examined. The header says so, and points at #804.
Verification
The test was confirmed live, not vacuous, by inverting its assertions and checking it fails reporting the genuine core error.
The wider
@cipherstash/stacksuite has 18 pre-existing failures across 16 files on this machine, allClient key not configured: Profile not foundfrom an absent local dev credential profile. None is a wasm suite; none is a file this PR touches.Not in scope
docs/superpowers/plans/2026-07-24-supabase-portable-runtime.mdstill instructs the impossible — "In a browser, useOidcFederationStrategy— never shipconfig.clientKey", which no caller can follow since the client will not construct without it. That file does not exist onremove-v2; it is onmain. It needs a separate change on whichever branch carries that work.