fix(stack): consume protect-ffi 0.31.0 - #860
Merged
Merged
Conversation
Bumps the exact pin from 0.30.0 to 0.31.0 across `@cipherstash/stack` and the two adapters that carry it as a devDependency. 0.31.0 is a release with a `Breaking` heading, and four incompatibilities land with it. **1. The `ProtectError` class is gone**, replaced by an `isProtectErrorCode` guard. Both bindings now throw an ordinary `Error` with `code` set by Rust, so there is no class to match — and `instanceof` was unreliable regardless, being false across duplicate copies of a package. Every type-only import is unaffected; the two value sites move to a value check. That check is on the code's VALUE, not the presence of a `code` property, which fixes a pre-existing bug in `dynamodb/helpers.ts`. Its fallback branch accepted any string-valued `code` and asserted it into `ProtectErrorCode`, so a Node error — `ECONNRESET` from the DynamoDB client, say — was reported as an encryption error code. The two branches collapse into one correct one. **2. The wasm `newClient` moved credentials into `clientOpts`** and renamed `strategy` to `authStrategy`. Credentials left at the top level are now rejected outright, so that half fails loudly; a `keyset` left there would be silently ignored and bind the client to the DEFAULT keyset, encrypting under the wrong keys. This config forwards no keyset, and the test now asserts `clientOpts` as a whole so one landing elsewhere is caught. The `as never` is deleted. 0.30 typed the wasm options as `any`, so the cast was load-bearing; 0.31 types them properly. Removing it immediately surfaced `encryptConfig`, below — which is the argument for removing it. **3. `encryptConfig` no longer needs normalising.** 0.30's wasm binding accepted EQL-native `cast_as` only, so the factory ran `normalizeCastAs` first. 0.31 normalizes at the Rust deserialization boundary on both bindings and types the result as `CanonicalEncryptConfig`, documented as a shape nothing asks you to build — so it is not assignable to the `EncryptConfig` `newClient` declares, and keeping the call would need an assertion that misdescribes the value. `normalizeCastAs` is deprecated rather than deleted — it is the only exhaustive consumer of `toEqlCastAs`, and removing both is a deliberate cleanup, not a side effect of a dependency bump. **4. Unknown payload keys now reach Rust and are rejected.** Stack attaches a correlation `id` to every bulk encrypt/decrypt payload, and protect-ffi's `EncryptPayload` / `BulkDecryptPayload` have never declared one — 0.30 dropped it silently, 0.31 fails the whole call with ``unknown field `id` ``. Nothing was using it: results are correlated positionally, by `keyMap` index in the model helpers and against the original array in `mapEncryptedDataToResult` / `mapDecryptedDataToResult`. The id is stripped at the FFI boundary and stays on stack's own side of it. This is the first of four stacked PRs splitting the protect-ffi monorepo absorption. It consumes the PUBLISHED 0.31.0 from npm and is independent of the vendoring that follows: `packages/stack` uses nothing added to protect-ffi after the 0.31.0 tag, and `eql-v3.ts`'s export surface is identical between the published release and the later in-tree copy.
protect-ffi 0.31.0 narrowed an explicit `clientKey` to hex only. It used to be decoded by `SecretKey::from_hex`, which falls back to standard padded base64 — the encoding `~/.cipherstash/secretkey.json` stores on disk — so a base64 value pasted into the secret worked. The Neon entry forwards `CS_CLIENT_KEY` straight through as `clientKey`, so this is the exact value that now has to be hex. Without a check, a base64 secret presents as all six credentialed workflows failing simultaneously at client construction, with `invalid clientKey: expected a hex-encoded key` and nothing else — protect-ffi discards the decode error on purpose, because hex's own message names the offending character and its offset, which would put part of a live key into logs and error trackers. Six unrelated-looking red jobs and a message that does not mention encoding is a bad afternoon. It goes in `require-cs-secrets` because every workflow that sets CS_CLIENT_KEY already calls that action — verified across all six — so this is one edit rather than six, and it sits next to the existing presence check it naturally follows. The key is never echoed: the check is a charset and even-length test, and both the success and failure messages report only the length.
🦋 Changeset detectedLatest commit: 4932503 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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughThe PR upgrades Changesprotect-ffi 0.31 compatibility
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StackCaller
participant wasmNewClient
participant protect_ffi
StackCaller->>wasmNewClient: provide credentials and authentication strategy
wasmNewClient->>protect_ffi: construct client with authStrategy and clientOpts
wasmNewClient->>protect_ffi: forward encryptConfig unchanged
protect_ffi-->>wasmNewClient: return initialized client
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Aug 6, 2026
freshtonic
approved these changes
Aug 6, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Review: consume protect-ffi 0.31.0
Reviewed at the actual PR head (66ba41a7) against main. Built, checked, and ran the tests locally; verified every changed call against the installed @cipherstash/protect-ffi@0.31.0 type surface.
Verdict: Approve. Clean adoption, no blocking issues. Two optional test-coverage nits below.
Verification
pnpm --filter @cipherstash/stack build— clean (DTS type-check passes, which is what validates dropping theas neverinwasm-inline.ts).pnpm --filter @cipherstash/stack test— 1026 passed, 44 skipped (skips are live lock-context tests withoutUSER_JWT), 0 failed.pnpm run code:check— 0 errors (the only diagnostics on a raw run come from the gitignored.turbo/cache, which CI never sees); changed source carries only 2 pre-existing, allowedas neverwarnings it doesn't touch.error-codes.test.ts— 18 pass with allCS_*unset: the newisProtectErrorCodeassertions and the FFI validation-error cases resolve client-side, so they're a real executable guard (the PR note that the suite "was not run locally" understates it — it runs).
Confirmed against the published 0.31.0 types:
isProtectErrorCode/ProtectErrorCodeexported,ProtectErrorclass gone — all imports match; no stale references remain anywhere inpackages/.NewClientOptions/ClientOpts/CredentialOpts— the newwasmNewClient({ authStrategy, encryptConfig, clientOpts: { clientId, clientKey }, eqlVersion })matches exactly, and the comment's claim that a futurekeysetbelongs inclientOptsis correct.EncryptPayload/BulkDecryptPayload— traced every model-helper and bulk payload builder; each is{ id, ...FFI-fields }, so stripping onlyidleaves precisely the accepted shape. No other undeclared key survives.
Non-blocking suggestions
- No direct test for the
handleErrorbug fix. The changeset headlines that a foreigncode(ECONNRESET) is no longer surfaced as an encryption code, but that's only exercised at the predicate level.packages/stack/src/dynamodb/helpers.ts:44-58— add a credential-free unit test callinghandleErrorwith{ code: 'ECONNRESET' }(→DYNAMODB_ENCRYPTION_ERROR) and{ code: 'UNKNOWN_COLUMN' }(→ preserved). Cheap, pins the promised behaviour. - No direct test for
getErrorCode.packages/stack/src/encryption/helpers/error-code.ts:19-22— a one-liner assertinggetErrorCode({ code: 'ECONNRESET' })isundefinedand{ code: 'UNKNOWN_COLUMN' }returns the code. Optional; the wrapper is trivial.
Highlights
- The DynamoDB fix is folded into the bump rather than merely ported — the collapsed branches remove a pre-existing fail-open, and the comment explains why the value-check matters (Node sets
codeon its own errors). withoutIdis a single well-named boundary with a rationale proving the id was inert (results correlate by array index), and the public bulk API still returnsidbecause result mapping reads the original array.- Dropping
as neverand letting the compiler check the option shape is exactly what would catch a misplaced credential/keyset— and thekeyset-silently-ignored footgun is called out in both the comment and the test. - The CI hex pre-flight uses
[[ =~ ]](whole-string anchor) overgrep -q(per-line) specifically to rejectdeadbeef\n<junk>, and never echoes the key. - Compliance is complete:
major+stash: patch(skills-only) changesets, and thestash-authskill updated per the AGENTS.md package→skill map.
freshtonic
approved these changes
Aug 6, 2026
freshtonic
left a comment
freshtonic
left a comment
Contributor
There was a problem hiding this comment.
Re-review: consume protect-ffi 0.31.0 — Approve
Re-reviewed at PR head 66ba41a7, independently of the earlier approval. The head is unchanged since the prior review (both commits predate it), so freshtonic's two non-blocking test nits remain open — they stay non-blocking. I re-ran the full verification myself in an isolated worktree against npm @cipherstash/protect-ffi@0.31.0 rather than trust the prior run.
Verdict: Approve. Clean adoption, no blocking issues.
Verification
pnpm --filter @cipherstash/stack build— clean; DTS type-check passes (this is what validates dropping theas neveron thewasmNewClientcall).- Credential-free unit suites (
wasm-inline-new-client,wasm-inline-v3,encrypt-lock-context-guards,encrypt-query-match-preflight) — 33 passed, 18 skipped, 0 failed. error-codes.test.tsfails only in itsbeforeAll(constructs a live client →Invalid CRNwhenCS_*are unset) — not a code defect; see Nit 2.biome checkon all changed source — 0 errors; the only 2 warnings are pre-existingas neverinisEncrypted(lines 166/851) that this PR doesn't touch.- Checked every changed call against the installed 0.31.0
.d.ts:isProtectErrorCode/PROTECT_ERROR_CODES(hasUNKNOWN_COLUMN),NewClientOptions { encryptConfig, clientOpts?, authStrategy?, eqlVersion? },ClientOpts = CredentialOpts & { keyset? }withclientId/clientKey. All match; no staleProtectErrorclass references remain inpackages/*/src.
Non-blocking
1. Promised bug-fixes still lack a direct test (unchanged from the prior review). The changeset headlines the DynamoDB handleError fix (a foreign ECONNRESET no longer surfaced as an encryption code) and the getErrorCode value-check, but the new tests only exercise isProtectErrorCode — the library predicate, not the two stack wrappers that consume it (packages/stack/src/dynamodb/helpers.ts:55-58, packages/stack/src/encryption/helpers/error-code.ts:19-22). A credential-free unit test asserting handleError({ code: 'ECONNRESET' }) → DYNAMODB_ENCRYPTION_ERROR, handleError({ code: 'UNKNOWN_COLUMN' }) preserved, and getErrorCode({ code: 'ECONNRESET' }) → undefined would pin the promised behaviour cheaply.
2. The new predicate tests only run in the credentialed CI job (new observation). The isProtectErrorCode block added at error-codes.test.ts:42-64 is pure and credential-independent, but its parent beforeAll (error-codes.test.ts:39) constructs a real Encryption client. With CS_* unset the whole suite fails at beforeAll (verified locally), so these tests never run without live credentials — not the always-on client-side guard one might assume. Moving them to a describe outside the credential-gated beforeAll (or a standalone file) would let them run everywhere and would resolve Nit 1 for free.
Highlights
- The DynamoDB fix is genuinely folded into the bump: collapsing the two branches removes a pre-existing fail-open, and the
isProtectErrorCodetype guard narrowserrorObj.codetoProtectErrorCodewith no assertion — type-safe and null-safe via optional chaining. withoutIdis a single well-named FFI boundary with a rationale proving theidwas inert (results correlate by array index), and the public bulk API still returnsidbecause result mapping reads the original array.- Removing
as neverand letting the compiler check the option shape is the point — it's what would catch a misplaced credential or akeysetin the wrong place; the silently-ignored-keysetfootgun is called out in both the comment and the test. - CI hex pre-flight: presence is asserted first so the hex step never sees an empty value; whole-string
[[ =~ ]]anchor rejectsdeadbeef\n<junk>; never echoes the key. - Compliance complete:
major+stash: patch(skills-only) changesets, andstash-authupdated per the package→skill map. No CLI surface change, sostash-clicorrectly untouched.
…odes protect-ffi 0.31.0 removed the `ProtectError` class both helpers matched with `instanceof`, and the collapse of that branch fixed a bug: the old fallback accepted ANY string-valued `code` and asserted it into `ProtectErrorCode`, so a Node error (`ECONNRESET` off a dropped socket) was handed back as an encryption error code. A caller keying retry-vs-fail off `error.code` read a transport fault as a crypto fault. That fix only had predicate-level coverage — `isProtectErrorCode` was tested directly, but its two call sites were reachable only through live ZeroKMS. Both new blocks fail against the pre-0.31 guard and pass against the current one, credential-free. - `handleError` joins `throwPreservingCode` in the DynamoDB pure-helper suite; they are the two ends of one seam (the latter exists so the code survives `withResult`'s wrapping for the former to read back). Also covers the message-extraction ladder and the errorHandler/logger fan-out. - `getErrorCode` joins `getErrorMessage` in the error-helper suite, including the null/undefined inputs its optional chain exists for.
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.
Stack 1 of 4 — splitting #858 (the protect-ffi monorepo absorption) into reviewable pieces.
packages/protect-ffi(subtree, upstream history preserved)What
Bumps the exact pin
0.30.0→0.31.0in@cipherstash/stackand the two adapters that carry it as a devDependency, and adapts the source to four breaking changes:ProtectErroris gone, replaced by anisProtectErrorCodeguard. The check is on the code's value, which fixes a pre-existing bug indynamodb/helpers.ts— its fallback accepted any string-valuedcode, soECONNRESETfrom the DynamoDB client surfaced as an encryption error code.newClientmoved credentials intoclientOptsand renamedstrategy→authStrategy. Credentials left at the top level are now rejected outright; akeysetleft there would be silently ignored and bind the client to the default keyset.encryptConfigno longer needs normalising — 0.31 normalizes at the Rust deserialization boundary on both bindings.idto every bulk payload that protect-ffi never declared; 0.30 dropped it silently, 0.31 fails the call. It is now stripped at the FFI boundary.Carries the
majorchangeset for the adoption, and arequire-cs-secretspre-flight for the new hex-onlyclientKey— without it a base64 secret presents as all six credentialed workflows failing at once with a message that never mentions encoding.Why this is independent of the vendoring
It consumes the published 0.31.0 from npm. Verified:
packages/stackuses nothing added to protect-ffi after the 0.31.0 tag, andeql-v3.ts's export surface is identical between the published release and the later in-tree copy.Verification
pnpm --filter @cipherstash/stack buildclean; scripts suite 178 passing; biome 0 errors; lockfile in sync under--frozen-lockfile. Theerror-codessuite needs live CS_* credentials and was not run locally.Summary by CodeRabbit
CS_CLIENT_KEYandclientKeyvalues must now be even-length hexadecimal strings; base64 values are rejected.