docs(stack): stop advertising the WASM entry as browser-capable, and pin the reason against the core - #953
Draft
tobyhede wants to merge 3 commits into
Draft
docs(stack): stop advertising the WASM entry as browser-capable, and pin the reason against the core#953tobyhede wants to merge 3 commits into
tobyhede wants to merge 3 commits into
Conversation
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". (cherry picked from commit a438b0a)
…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. (cherry picked from commit 2f992d6)
… shape The two commits recovered from the #804 branch were written on 2026-07-28, against protect-ffi 0.30. Cherry-picking them onto main compiled but every credential assertion failed: 0.31 moved `clientId` / `clientKey` under `clientOpts` and denies unknown keys at the top level, so the calls were rejected with `unknown field 'clientId'` before reaching anything the test meant to observe. Nesting the credentials fixed three of six. The rest were asserting error strings the core no longer emits: missing field `clientKey` -> clientOpts.clientId and clientOpts.clientKey are required (one message names both; the core does not say which is absent) invalid clientKey: invalid hex -> invalid clientKey: expected a hex-encoded key opts.strategy is required -> Not authenticated The last one carried the file's ordering claim, so it was re-derived rather than re-spelled: probing all three combinations against the real core shows `Not authenticated` wins whenever `strategy` is absent, while omitting the credentials WITH a strategy gives the credential error. The strategy is still read first, which is what makes "even when an auth strategy is supplied" mean anything in the tests above. The finding the file exists to pin is unchanged and now pinned against the shipping core: federation does not remove the `clientKey` requirement. Claude-Session: https://claude.ai/code/session_01E1J2nVGJWVkqvLepDfinRf
🦋 Changeset detectedLatest commit: 26bafc8 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 |
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.
Summary
@cipherstash/stack/wasm-inlineis the build of our encryption client that carries the engine as a WebAssembly blob instead of a native module, so it can run where native modules cannot: Deno, Supabase Edge Functions, Cloudflare Workers. Several places in this repo also advertised it as working in a browser. It does not, and cannot today.The reason is
clientKey. It is a workspace-wide secret, the client requires it on every authentication path — including the OIDC federation path, where a per-user token might look like it should replace it — and the core loads it as encryption key material during construction. Putting that entry in a browser bundle ships the workspace secret to every visitor.This PR removes the browser claim from the four places that made it, and adds a test suite that pins the reason against the real WASM core rather than a mock, so the claim cannot quietly stop being true.
Changes
Documentation that said the wrong thing
examples/supabase-worker/README.mdand its edge function both listed "modern browsers" among the runtimes this entry supports. The example readsCS_CLIENT_KEYfrom the environment, so it demonstrated the exact thing it called browser-safe.packages/stack/tsup.config.tslisted browsers among "the only runtimes that need wasm-inline" — in the same file cited elsewhere as evidence that nobrowserexport condition exists.skills/stash-supabase/SKILL.mdgave Postgres introspection as the reason the factory cannot run in a browser. True but incomplete, and misleading next to Add thebrowserexport condition to @cipherstash/stack-supabase, with a live browser smoke test #805 ("add thebrowserexport condition"): dropping thepgdependency unblocks Workers, not browsers, becauseclientKeyis required either way.packages/stack/src/wasm-inline.tsnow states the constraint where callers meet it, onWasmClientConfigand the auth-strategy re-export.A contract test that runs against the real core —
packages/stack/__tests__/wasm-inline-core-credential-contract.test.ts, 291 lines, 7 tests.expect(calls.getToken).toBe(0)proved nothing on its own: a counter that is never incremented reads as zero whether the claim is true or the core simply never callsgetTokenduring construction. It does call it — structurally complete key material clears the key provider, after which the core callsgetTokenexactly once. So authentication is reached during construction, only after the key is loaded, and the counter is live. Still fully offline: the token never parses, so nothing leaves the process.browserexport condition on any subpath. Adding one to quiet a bundler would ship a workspace secret to the browser.A drift guard, and three corrections to overstated comments — the previous version claimed a rename of
opts.strategywould silently hollow out the file. That is false, and is now pinned rather than assumed: the core reads the strategy by name before deserialising credentials, so the other tests would fail loudly rather than pass vacuously.A changeset —
@cipherstash/stackpatch andstashpatch (the skills ship in thestashtarball).Verification
pnpm --filter @cipherstash/stack exec vitest run __tests__/wasm-inline-core-credential-contract.test.ts— 7 passed, against the real@cipherstash/protect-ffiWASM build, no mocks.pnpm --filter @cipherstash/stack exec vitest run __tests__/wasm-inline-normalize.test.ts __tests__/wasm-inline-bundle-isolation.test.ts— 8 passed. These are the two adjacent wasm suites the test file's comments now describe correctly.The test was ported, not cherry-picked clean, and that is the part worth reviewing. It was written on 2026-07-28 against
@cipherstash/protect-ffi0.30. On currentmainit compiled and every credential assertion failed: 0.31 movedclientId/clientKeyunderclientOptsand rejects unknown top-level keys, so the calls were refused withunknown field 'clientId'before reaching anything the test meant to observe. Nesting the credentials fixed three of six. The other three asserted error strings the core no longer emits:missing field `clientKey`clientOpts.clientId and clientOpts.clientKey are required(one message names both; the core does not say which is absent)invalid clientKey: invalid hexinvalid clientKey: expected a hex-encoded keyopts.strategy is requiredNot authenticatedThe last one carried the file's ordering claim, so it was re-derived by probing the real core, not re-spelled:
Not authenticatedwins wheneverstrategyis absent, while omitting the credentials with a strategy gives the credential error. The strategy is still read first, which is what makes "even when an auth strategy is supplied" mean anything in the tests above it.The finding the file exists to pin is unchanged, and is now pinned against the shipping core: federation does not remove the
clientKeyrequirement.Related
Refs #804, #805.
This supersedes #810 — do not merge that one. #810 is a 100-commit draft whose base is 2,803 commits behind
main. The two documentation commits worth keeping were recovered onto currentmainhere, plus the port commit above. #810 can be closed once this lands.Review notes
Start with the port commit (
26bafc8e) — it is the only one whose content is new rather than recovered, and the error-string table above is the whole of what changed.This branch conflicts with #951 in
skills/stash-supabase/SKILL.md. Both edit the same paragraph: this one inserts a paragraph explaining why the browser is ruled out, immediately below the sentence that PR replaces to say the Worker is not. The two are complementary; whichever merges second should rebase and keep both.e2e/wasm/roundtrip.test.tscovers theaccessKeyarm, so the federation arm reasoned about here has no live end-to-end coverage anywhere. That is stated in the test file rather than fixed, and is not something this PR sets out to close.