feat(protect-ffi): vendor the package and run its own checks - #862
feat(protect-ffi): vendor the package and run its own checks#862tobyhede wants to merge 563 commits into
Conversation
On AWS, we had issues where GLIBC_2.36 was not supported. As the currently supported Node version is 20+, and it requires 2.28, we set our GLIBC version to 2.28 here too
Adds end-to-end handling for JS Date values. Previously, `cast_as: 'date'` was advertised in the config but the plaintext conversion layer had no path to or from date types — encrypt and decrypt both failed. - New `cast_as: 'timestamp'` -> `ColumnType::Timestamp` alongside the existing 'date' -> `ColumnType::Date` (day precision). - `JsPlaintext::Date(DateTime<Utc>)` variant maps to `Plaintext::Timestamp`; string arms in `to_plaintext_with_type` parse ISO 8601 / YYYY-MM-DD for callers who pass a JS Date via `d.toISOString()`. - Decrypt returns a plain RFC 3339 string; callers wrap with `new Date(...)` if they want a Date object (JSON has no native Date type, so this keeps the wire format honest). - Error messages no longer echo the user's input string, preventing a secret that gets mistakenly routed to a date column from appearing in the error that propagates to the caller.
feat: support date and timestamp plaintexts
Bumps aws-lc-rs 1.16.1 -> 1.16.3, which in turn bumps aws-lc-sys 0.38.0 -> 0.40.0 (>= 0.39.0). Addresses GHSA-9f94-5g5w-gf6r and GHSA-394x-vwmw-crm3.
Addresses GHSA-82j2-j2ch-gfr8 (out-of-bounds panic in bit_string_flags via the issuingDistributionPoint CRL extension).
fix(deps): patch aws-lc-sys to 0.40.0
fix(deps): patch vite to 8.0.10
fix(deps): patch rustls-webpki to 0.103.13
Set GLIBC version using cargo-zigbuild
Bump cipherstash-client, cts-common, and stack-profile from 0.34.1-alpha.2 to 0.34.1-alpha.4 and adapt protect-ffi to the breaking API changes: - ColumnType::Utf8Str -> ColumnType::Text, ColumnType::JsonB -> ColumnType::Json - Plaintext::Utf8Str -> Plaintext::Text, Plaintext::JsonB -> Plaintext::Json - EqlEncryptOpts gained a required decryption_policy field - IndexType gained an Ope variant; handle it alongside Ore in the index-name helpers
…ent-0.34.1-alpha.4 chore(deps): upgrade cipherstash-client to 0.34.1-alpha.4
Bumps cipherstash-client, cts-common, and stack-profile from 0.34.1-alpha.4 to 0.34.1-alpha.5 to pick up shared schema and config types ahead of the CanonicalEncryptionConfig migration. `IndexType::SteVec` gained a required `mode: SteVecMode` field in this release; the FFI sets it to `SteVecMode::default()` (`Compat` at alpha.5) so existing behaviour is preserved. No public TypeScript API change.
Surfaces the `mode` option on SteVec indexes through the config API, letting callers choose between `compat` and `standard` encoding. Previously the FFI hard-coded `SteVecMode::default()`, giving callers no way to opt in to the newer encoding. How: - Add the `SteVecMode` TypeScript type (`'compat' | 'standard'`). - Add a `mode` field to the Rust `SteVecIndexOpts` struct with `#[serde(default)]` so configs that omit it continue to parse. - Thread the parsed value through to `IndexType::SteVec` instead of always falling back to `SteVecMode::default()`. - Document the option in the JSONB API reference. Backwards compatibility: configs that omit `mode` keep using the upstream library default, which at this commit (cipherstash-client alpha.5) is still `Compat`. The follow-up alpha.7 bump flips the upstream default to `Standard` — see the CanonicalEncryptionConfig refactor commit for the user-visible breaking change that introduces.
Adds `.worktrees/` to .gitignore so scratch worktrees created under the repo root don't show up as untracked. Personal workflow convention; not used by CI or other contributors.
Bumps cipherstash-client, cipherstash-config, cipherstash-core, cts-common, stack-auth, stack-profile, and zerokms-protocol from alpha.5 to alpha.7. Required so the FFI can deserialize encrypt configs directly into `CanonicalEncryptionConfig` and reuse upstream validation (config version check, ste_vec/match plaintext-type rules). BREAKING (latent at this commit, becomes user-visible once the follow-up CanonicalEncryptionConfig refactor swaps the FFI types over): - `SteVecMode::default()` flips from `Compat` to `Standard` at the library level. Because the FFI currently parses `mode` through its own struct that falls back to `SteVecMode::default()` when the field is omitted, any ste_vec index that omits `mode` will now resolve to `Standard` instead of `Compat`. The two encodings are NOT cross-compatible — stored data indexed under `Compat` cannot be queried under `Standard`. Callers that need to preserve the previous behaviour must pin `mode: 'compat'` explicitly. The migration docs commit later in this branch records the full set of breaking changes and the caller-facing migration recipes.
Adds a TypeScript translation layer that converts the public, JS-friendly EncryptConfig vocabulary into the canonical vocabulary that cipherstash-config's CanonicalEncryptionConfig expects. Not yet wired into `newClient`; the wiring lands in the follow-up "normalize encrypt config vocabulary at the FFI boundary" commit. Why translate in TypeScript rather than rename in Rust: - Keeps the public TypeScript API stable for existing callers (`cast_as: 'string' | 'number' | 'bigint' | ...`). - Lets the native config adopt upstream's canonical names (`text`, `float`, `big_int`) without leaking them into the JS interface. - Future vocabulary tweaks can ship as TS-only changes without another Rust release. How: - Remap `cast_as` values that have no canonical equivalent: `string` → `text`, `number` → `float`, `bigint` → `big_int`. All other values pass through unchanged. - Inject `array_index_mode: 'none'` on any `ste_vec` index that omits the field. The upstream library defaults to `'all'`, so without this we would silently change array-indexing behaviour for existing configs (see the migration design doc earlier in this branch). - Leave `mode` untouched. Omitted `mode` follows the upstream default (`Standard` at alpha.7) — this is a documented breaking change, surfaced once the wiring lands. - Never mutate the caller's config object; build a fresh `NativeEncryptConfig` and return it. Includes unit tests covering each remapped value, the `array_index_mode` default injection, pass-through of canonical values, immutability of the input, and the no-op cases.
…Config
Removes the 688-line `encrypt_config.rs` module and deserializes
the encrypt config directly into cipherstash-config's
`CanonicalEncryptionConfig`. Eliminates duplicate type definitions
across the FFI and the shared schema crate, picks up upstream
validation (version check, ste_vec/match plaintext-type rules)
for free, and lets future schema changes propagate without an FFI
patch.
How:
- Drop `mod encrypt_config` and import `CanonicalEncryptionConfig`
and `Identifier` from `cipherstash_client::schema` directly.
- Change `NewClientOptions::encrypt_config` to
`CanonicalEncryptionConfig`.
- Replace the bespoke `Error::SteVecRequiresJsonCastAs` and
`Error::Config(String)` variants with
`Error::Config(#[from] ConfigError)`, surfacing the upstream
error verbatim.
- Rename the old `Error::Config(String)` to
`Error::Credentials(String)` to reflect its actual usage
(only emitted from SecretKey hex parsing). Note: the
`#[error("Configuration error: {0}")]` display template is
left unchanged for this variant — a follow-up to tighten the
wording is captured in the migration doc.
BREAKING CHANGES (visible to TS callers — see
docs/canonical-encryption-config-migration.md for migration
recipes):
1. SteVec `mode` default: `Compat` → `Standard`. Any ste_vec
config that omits `mode` now indexes new writes under
`Standard` encoding. The two encodings are NOT
cross-compatible: data indexed under `Compat` cannot be
queried under `Standard`, and vice versa. Pin `mode: 'compat'`
explicitly to preserve the pre-migration behaviour for stored
data, or plan a re-encryption of affected columns.
2. `match` index now requires a text-family `cast_as` (`'text'`
or `'string'`). Previously unvalidated; now fails at
`newClient` (mapped to `MATCH_REQUIRES_TEXT` by the follow-up
FFI-boundary wiring commit).
3. Config `v` must equal `1`. Previously unchecked; other values
now fail at `newClient` (mapped to `UNSUPPORTED_CONFIG_VERSION`
by the follow-up FFI-boundary wiring commit).
4. Config-validation error message text now comes from upstream
`ConfigError` and is worded differently. `ProtectError.code`
values are preserved, so consumers branching on `code` are
unaffected; consumers string-matching on `err.message` for
config-validation errors must update.
BREAKING CHANGE: ste_vec indexes that omit `mode` now use
`Standard` encoding instead of `Compat`. Pin `mode: 'compat'` or
plan re-encryption of stored data.
BREAKING CHANGE: `match` indexes now require a text-family
`cast_as` (`'text'` or `'string'`); previously unvalidated configs
will fail at `newClient`.
BREAKING CHANGE: encrypt config `v` must equal `1`; other values
fail at `newClient` instead of being silently accepted.
BREAKING CHANGE: config-validation error message wording changed
(error codes preserved); consumers string-matching on
`err.message` must update.
Wires the `normalizeEncryptConfig` helper (added earlier in this
branch) into the public `newClient` entry point so callers keep
using the JS-friendly `EncryptConfig` vocabulary while the native
side receives the canonical `CanonicalEncryptionConfig` shape
produced by the preceding refactor.
How:
- Pipe `opts.encryptConfig` through `normalizeEncryptConfig`
before passing it to `native.newClient`.
- Introduce an internal `NativeNewClientOptions` type so the
native module declaration reflects the post-normalization shape
(`NativeEncryptConfig`) without leaking it from the public API.
Error-code mapping:
- `inferErrorCode` now recognises three message fragments produced
by upstream `ConfigError`:
- `'requires plaintext_type: json'` → `STE_VEC_REQUIRES_JSON_CAST_AS`
(existing code; substring updated to match the new wording).
- `'requires plaintext_type: text'` → `MATCH_REQUIRES_TEXT`
(new code).
- `'unsupported config version'` → `UNSUPPORTED_CONFIG_VERSION`
(new code).
- Adds `MATCH_REQUIRES_TEXT` and `UNSUPPORTED_CONFIG_VERSION` to
the exported `ProtectErrorCode` union.
BREAKING:
- `ProtectError` messages for config-validation errors are now
worded as `ConfigError` emits them. Consumers branching on
`ProtectError.code` are unaffected; consumers string-matching
on `err.message` must update their match strings.
- The exported `ProtectErrorCode` union gains two new values
(`MATCH_REQUIRES_TEXT`, `UNSUPPORTED_CONFIG_VERSION`).
Exhaustive switches over `ProtectErrorCode` will need
additional cases to stay exhaustive (TS will flag missing
cases when `--strict` is on).
BREAKING CHANGE: two new `ProtectErrorCode` values exist
(`MATCH_REQUIRES_TEXT`, `UNSUPPORTED_CONFIG_VERSION`); exhaustive
switches over the union need additional cases.
BREAKING CHANGE: config-validation error message text is now
sourced from upstream `ConfigError`; consumers string-matching on
`err.message` must update.
Adds `docs/canonical-encryption-config-migration.md` describing the four breaking changes that ship with the CanonicalEncryptionConfig migration, with migration recipes for each: 1. SteVec `mode` default: `Compat` → `Standard` (most impactful; existing data must be re-encrypted or `mode: 'compat'` pinned). Calls out explicitly that the two encodings are not cross-compatible. 2. `match` index now requires text-family `cast_as`. 3. Config `v` must equal `1`. 4. ConfigError message text differs from the old hand-rolled error wording. `ProtectError.code` values are preserved. Also expands `docs/jsonb-api-reference.md` with: - The full `cast_as` vocabulary table showing the public ↔ canonical mapping (so callers can debug error messages that reference canonical names). - Validation rules for `v`, `ste_vec`, and `match` together with the error codes they emit. - A SteVec `mode` section warning that re-encryption is required when changing modes. - The two new `ProtectErrorCode` values (`MATCH_REQUIRES_TEXT`, `UNSUPPORTED_CONFIG_VERSION`). Includes a "explicitly not changed" section calling out that `array_index_mode` still defaults to `'none'` at the FFI boundary (the TS helper injects it), and a "follow-ups" section recording two non-blocking polish items (unit tests for `inferErrorCode` and the `Error::Credentials` display template). Documentation only.
Fixes two doc-drift issues caught after the migration docs landed: - The JSDoc on `SteVecMode` (src/index.cts) still claimed `compat` was the default. After the alpha.7 bump the runtime default is `standard`. Update the comment to match runtime behaviour and add a hint that callers should pin `compat` explicitly to preserve pre-alpha.7 encoding for stored data. - The `cast_as` union snippet in docs/jsonb-api-reference.md was missing `'text'` and `'timestamp'` (added by the migration). Update the snippet to mirror the public `CastAs` union exported from src/index.cts. Documentation only.
Adds an integration test exercising newClient with legacy cast_as values plus an ste_vec config without mode, and three negative cases asserting the ProtectError codes MATCH_REQUIRES_TEXT, UNSUPPORTED_CONFIG_VERSION, and STE_VEC_REQUIRES_JSON_CAST_AS — replacing the manual verification steps from the migration PR.
The cipherstash-client 0.34.1-alpha.7 default flipped SteVec encoding from `Compat` to `Standard`, which collapses the old `b3`/`ocf`/`ocv` SteVec entry fields into two: - Scalar strings and numbers share a single orderable field `oc` (CLLW ORE with tagged-plaintext domain separation). - Booleans, null, arrays, and objects produce an `hm` HMAC-SHA256. Update the integration tests to assert against `oc`/`hm` instead of the retired `b3`/`ocf`/`ocv` names. Restructure the `unique index field (b3)` block as `HMAC index field (hm)` and exercise the non-orderable types (root object, booleans) that actually produce HMAC entries under Standard mode.
The cipherstash-client 0.34.1-alpha.7 default flipped SteVec encoding from `Compat` to `Standard`, collapsing the old `b3`/`ocf`/`ocv` fields. Update the JSONB API reference to match the runtime: - Replace `b3`/`ocf`/`ocv` in the EqlCiphertext / EqlCiphertextBody type snippets with the current fields (`oc` for Standard SteVec, `op` for Compat SteVec, `opf`/`opv` for non-SteVec OPE indexes). - Note that `hm` now also covers SteVec MAC entries (objects, arrays, booleans, null), not just the standalone `unique` index. - Add a small table summarising which entry field each JSON value type produces, plus a mention of the Compat-mode `op` variant. - Update the storage and term-query example outputs accordingly. Documentation only.
Root-level `hm` is HMAC-SHA256 for unique (exact) indexes; SteVec MAC entries live under `sv`, not at the root.
Collapses three near-identical remap tests and the canonical-values loop into table-driven `it.each` blocks.
`queryOp` travelled as a bare `String` from the options struct all the way to `prepare_query_plaintext`, where a `match` turned it into cipherstash-client's `QueryOp` and an unrecognised value became `Error::UnknownQueryOp`. So the field was validated late, after the column and index had already been resolved, and every function between the boundary and that `match` took a `&str` that might be anything. `QueryOpName` is now the type on the wire, in `query_op.rs` with the tests that define it. A value that is not one of the four spellings cannot be constructed, so `parse_query_op`, `default_query_op` and the `UnknownQueryOp` variant are all gone — `to_query_op` is infallible because the value was already checked. The mapping to `QueryOp` stays explicit rather than deriving or re-exporting the library's enum: that type is cipherstash-client's internal vocabulary and can grow variants this binding has no wire spelling for. Two things that constrained the shape: - `inferErrorCode` in `src/errors.ts` matches `Unknown query operation:` to produce the public `UNKNOWN_QUERY_OP` code. A derived `Deserialize` would emit serde's own "unknown variant" wording and that code would quietly become unreachable, so `Deserialize` is hand-written around a shared `UNKNOWN_QUERY_OP_PREFIX`, and the coupling is pinned from both ends. Verified the message survives the trip: neon's `Json` extractor Displays the serde error verbatim into the thrown `JsError`, and the wasm entry does the same. The message now also lists what IS accepted, which it did not before. - `QueryOpKind` is renamed `SteVecQueryOpKind`, variants losing the `SteVec` prefix. All 17 uses are on `ste_vec` paths — the other index types have no shape for the input to be wrong in — so the prefix was redundant, which is what `clippy::enum_variant_names` was pointing at. Its `Display` now goes through `QueryOpName::as_str`, so an error cannot name an operation with a spelling the caller could not pass back. `Default` keeps its parenthetical, being inferred rather than written. `lib.rs` is 70 lines shorter. `cargo clippy --no-deps --tests --all-features --all-targets -- -D warnings` clean, 279 Rust tests (8 new), 50 vitest, wasm32 checks, wasm type tests green. Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
`prune_nulls` treats a null-valued key as an absent field, which is what
makes `{cast_as: cfg.castAs}` with an undefined `castAs` work on wasm.
It also, unavoidably, does the same to NaN and ±Infinity:
`serde_json::Value` cannot hold a non-finite float and
`JSON.stringify(Infinity)` is literally `null`, so by the time a config
reaches this function the two are the same value on both bindings.
So `{match: {m: Infinity}}` used to fail with `invalid type: null,
expected usize` and now silently takes `m`'s default. That is worse for
that input, and it is not fixable here — there is nothing left to
distinguish. Accepted because the frequencies are not comparable: an
undefined property is ordinary JavaScript that hard-errored on wasm, a
non-finite bloom filter parameter is a typo nobody has written.
Documented on `prune_nulls` and pinned by
`a_non_finite_number_is_indistinguishable_from_undefined`, so it reads
as a known trade rather than something nobody noticed. Catching it would
have to happen on the JS side, before the `Value` hop.
Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
Every options struct now carries `deny_unknown_fields`, so a misspelling, a stale key, or a value in the wrong place fails by name rather than being discarded on the way in. The issue proposed the serde attribute and flagged two obstacles. Both turned out differently: - `deny_unknown_fields` alongside `#[serde(flatten)]` works. serde_derive generates a leftover check for exactly that combination, so `ClientOpts` and `EnsureKeysetOpts` needed nothing beyond the attribute — no custom Deserialize, no unflattening `CredentialOpts`. - The attribute alone is a no-op on wasm, which the issue did not anticipate. serde-wasm-bindgen's `deserialize_struct` looks up each expected field with `Reflect::get` and never enumerates the object, so an undeclared key is invisible to serde and there is nothing to reject. The boundary that silently dropped the credentials was the one the fix would have missed. `DenyUnknown` closes that: an empty flattened marker puts every options struct on serde's `deserialize_map` path, which goes through `Object::entries` and does enumerate. The two structs that already flatten are on that path and don't carry it. Also: - The wasm `newClient` strips `authStrategy` / `strategy` from a shallow copy before deserializing. They're read with `Reflect` beforehand — a JS function can't survive serde — and a struct that denies unknown fields would otherwise reject them. The copy is so a config reused across calls keeps its strategy. - The Neon `newClient` rebuilt the native options object field by field, dropping unrecognised top-level keys before the Rust could see them. It now forwards the rest verbatim. Breaking: input that was previously accepted and ignored is now rejected. That is the point — `lockContext` at the top level of a bulk call used to encrypt every value unbound while the caller believed it was identity-bound, with nothing in the output to tell the two apart.
…m a doc The `lockContext` narrative belongs in the PR, not on the struct field. Removed; the field documents what it does. The comment on the strip made two claims. One was wrong and the other was missing: - "Non-object input passes through untouched" described a path nothing takes. A primitive never reaches the guard — the `Reflect::get` above it has already failed with "called on non-object". Arrays and functions DO reach it, get flattened by `Object::assign`, and are rejected by serde for their missing fields. Checked against the built wasm rather than reasoned about. - Nothing said why the deletes can't fail. They can't because `Object::assign` writes through [[Set]] onto a fresh object, so every copied property is a plain configurable data property regardless of the descriptor it had on the caller's object. That is the sort of guarantee a later refactor breaks silently, so it is written down now. Both descriptor cases that could plausibly defeat the strip now have tests: a frozen options object, and a non-enumerable strategy (which `Object::assign` never copies — it is read off the original beforehand, so it is used rather than lost).
`js_sys::Object::assign` is declared without `catch` — `pub fn assign<T>(target, source) -> Object<T>`, no `Result`. Every clone in `wasm.rs` copies an object the caller built, and a copy reads each own enumerable property, so any of them can meet a getter that throws. That throw would travel straight out of wasm, skipping the destructors of the zeroizing values these paths carry. Every other JS call in the module is `Reflect::*` with `map_err`; js-sys points at `try_assign` for exactly this. All four clone sites now go through one `shallow_clone` helper, which names what it was copying — the failure is the caller's object misbehaving and they have to be able to find it. `new_client`'s strip was the site under review, but `encode_plaintext` and `encode_plaintext_list` read caller objects on every encrypt path and had the same hole. Also records the other direction of the [[Set]] semantics the strip already relies on: a setter for a copied key on `Object.prototype` swallows the value and the key never arrives. A silent drop inside the function that exists to stop silent drops, but reaching it means a poisoned `Object.prototype`, which breaks far more than this. Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
Seven of thirteen structs carrying `deny_unknown_fields` were asserted; six were not — `PlaintextPayload`, `QueryPayload`, `EncryptQueryOptions`, `EncryptQueryBulkOptions`, `DecryptOptions`, `BulkDecryptPayload`. All six reject correctly, so this is what the tests were missing, not the code. The bulk-payload case matters most: a misspelled `lockContext` on a payload item is the per-item form of the top-level one this change exists for, and the likelier of the two. Both spellings encrypt UNBOUND with nothing in the output to tell them apart. Its mirror image is covered too — `unverifiedContext` lives on the container and `lockContext` on the item, and each is now rejected in the other's place rather than dropped, which is the trap for anyone copying the scalar call shape. The `queryOpp` assertion also pins what the marker costs: the message is `unknown field \`queryOpp\`` and nothing else, because serde's flatten path buffers the map and reports at its closing brace, dropping the `expected one of ...` list. Left unasserted that would go quietly. On the wasm side, none of the existing cases actually depended on the marker: `ClientOpts` reaches the map path through its own flattened credentials, so only the `clientId` test needed `DenyUnknown` at all — the other ten marker-carrying structs had no wasm coverage and dropping the marker from all of them would have failed one test. A misspelled `eqlVersion` on `newClient` covers it without credentials; the encrypt / encryptBulk / decrypt cases cover the structs that actually carry a lock context. Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
Forwarding what the JS layer doesn't handle is what lets Rust reject an unknown
key by name — but it also means an unknown key now reaches neon's `Json`
extractor, which is `JSON.stringify`. A circular value or a `bigint` throws
there, before serde runs:
newClient({...appConfig, logger})
-> TypeError: Converting circular structure to JSON
`normalizeError` / `inferErrorCode` match neither, so the caller gets a raw
`TypeError` — not a `ProtectError`, and not the name of the key they got wrong.
On a change whose whole point is naming the offender, that is the wrong failure.
Only the forwarded keys are checked, so this covers exactly what forwarding
newly exposed: `clientOpts` and the auth strategy never reach it, and a circular
`encryptConfig` fails the way it always has. The cost is one `JSON.stringify` of
the config, once, at client construction.
A function- or symbol-valued key is deliberately not caught — `JSON.stringify`
drops those without throwing, so they stay a silent drop on this binding while
wasm reports them. That asymmetry is in the CHANGELOG rather than fixed here.
Interim: #150 replaces this error layer with codes derived in Rust.
Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
Three things the note got wrong or left out, all checked against the pinned serde-wasm-bindgen 0.6.5 rather than reasoned about. **The lookup is not `Reflect::get`.** `deserialize_struct` reads each expected field with `ObjectExt::get_with_ref_key`, a wasm-bindgen `indexing_getter` — plain `obj[key]`. Same prototype semantics, wrong name. **`deserialize_map` tries `js_sys::try_iter` FIRST**; `Object::entries` is only the fallback, with no `Map`-only guard on the iterator arm — unlike `deserialize_any`. So an options object carrying `Symbol.iterator` is read through the iterator and its own properties are ignored entirely, which is the silent-drop class this marker closes, reopened on a shape almost nobody passes. `Object::assign` does not close it: an own enumerable `[Symbol.iterator]` survives the copy. An array of `[k, v]` pairs and a JS `Map` also become accepted where `deserialize_struct` rejected them, and the array form bypasses `encode_plaintext`, so a `bigint` plaintext loses precision above 2^53. **Two diagnostics regressions were undocumented.** A misspelled REQUIRED field now reports `missing field \`indexType\`` and never names `indexTyp`; the `expected one of ...` list is gone from every rejection. Neon-only — the wasm path had no error to lose. Both are now asserted, not just described. Also corrects the scope of the narrowing: the clones in `wasm.rs` are shallow, so a nested `LockContext` is read from the caller's own object on every entry point, and `encode_plaintext_list` returns `opts` untouched when nothing needed encoding — which includes a legitimate empty `plaintexts` — so the bulk entries' top-level bag is narrowed too. Own non-enumerable properties are dropped as well, not only inherited ones. And the per-key allocation cost the map path adds for valid input is written down, roughly 5N `String`s on an N-payload `encryptBulk` where there were none. The CHANGELOG gains the two boundary asymmetries it was short: function- and symbol-valued unknown keys, and the `JSON.stringify` throw on a circular or `bigint` value. Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
feat: converge the wasm and Neon interfaces, and declare real types on the wasm build (#142)
fix!: reject unknown option keys instead of dropping them (#144)
Clippy errors on wasm32, all pre-existing, none of them a bug — the point is that nothing was telling us. `--all-targets` in the lint task means all target *kinds* (lib, bins, tests, benches), not all platform targets, so wasm32 has never been linted. Unused imports: Neon-only pieces of `cipherstash_client` that `lib.rs` imported ungated. `wasm.rs` imports its own copies of the ones it needs straight from the crate, so gating them costs the wasm build nothing. `once_cell::OnceCell` holds the Tokio runtime the Neon exports block on, and `BTreeMap` is used only by gated code. One duplicated attribute: `mod wasm;` in lib.rs already carries `#[cfg(target_arch = "wasm32")]`, so the inner `#![cfg(...)]` in wasm.rs restated it. Dropped, with the reason recorded in the module docs so it does not come back. The dead-code errors this originally also fixed — `EnsureKeysetOpts` / `EnsureKeysetResult` — arrived on main with #147, which moved them into `client_options.rs` already gated. What that comment did not carry is why: the gate keeps the lint honest, it is not an endorsement of the split. `ensureKeyset` is missing from the wasm surface by oversight — the module docs used to call it a deliberate boundary (provisioning belongs on your server), and that reads well but does not hold up: wasm ships to servers too. Corrected in both places, because the reason it went unnoticed generalises: `ensureKeyset`'s only caller in this repo is an integration test, and one of the eighteen integration files loads the wasm build. A missing export is invisible when no test on that target would have called it. Filed as #149; taking those gates off is what marks it done. Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
The two gaps this closes are the same gap: a check nothing invokes reads exactly like a check that passes. `mise run lint:rust` is now an aggregate over three arms — clippy for the host, clippy for wasm32, and `cargo fmt --check`. It keeps the name CI already called, so the step gets strictly more coverage without a rename. The wasm arm lints the lib only; the unit tests are host-run. `npm test` now reaches `test:format:rust`, which has sat in package.json with no caller. That also makes the README's claim about `npm test` true again — it said it formatted and linted Rust, and it did neither. Drops the `cargo check --target wasm32-unknown-unknown` step from test.yml: clippy checks as it lints, so it was doing that work twice. `src/lintWiring.test.ts` guards the call graph rather than the checks. Its general form — no `test:*` script unreachable from `npm test`, no `lint:rust:*` task the aggregate skips — is what catches the next orphan, not just this one. Exemptions have to name a reason. Verified it fails on each regression it claims to catch: re-orphaning `test:format:rust`, dropping the wasm arm from `depends`, and CI calling clippy directly instead of the entry point. No changelog entry: nothing here changes the published surface. Claude-Session: https://claude.ai/code/session_01YJekcEBAsUg8qJoBcqyzBx
`Error` is a 14-variant enum, several carrying structured fields. All of it was discarded at the FFI boundary: Neon exports returned `extract::Error`, whose `TryIntoJs` is `cx.error(cause.to_string())`, and `wasm.rs` did the same via `js_error(&e.to_string())`. Only the message crossed. Each variant that JS can act on now carries `#[diagnostic(code(..))]`, and both boundaries read it onto `err.code`. Values are unchanged, so this half is additive on its own — the JS side that stops inferring them is the next commit. Notes on the shape, since two parts of the issue's proposal did not survive contact: - `#[diagnostic(transparent)]` on the `#[error(transparent)]` variants buys nothing. cipherstash-client, stack-auth, cipherstash-config and eql-bindings contain zero `#[diagnostic(code(..))]` and no manual `code` impls between them — they use `Diagnostic` for `help()` text only — so inheriting would inherit `None`. Six of the eleven wrapped types do not implement `Diagnostic` at all. The codes are therefore ours, which also settles the issue's worry about coupling to upstream naming: there is nothing to couple to. - `Error::Config` is split into four variants. Three published codes (`STE_VEC_REQUIRES_JSON_CAST_AS`, `MATCH_REQUIRES_TEXT`, `UNSUPPORTED_CONFIG_VERSION`) are sub-variants of one upstream `ConfigError`, and the derive cannot compute a code from inner state. `From<ConfigError> for Error` routes them by variant, so an upstream rename is a compile error where the substring match it replaces would have silently degraded to `UNKNOWN`. `#[error(transparent)]` on all four keeps the message identical. The Neon exports had to move their bodies into `do_*` helpers returning `Result<_, Error>`, mirroring what `wasm.rs` already does. `TryIntoJs` is sealed behind a private module, so no type declared here can implement it, and `extract::with` — which defers conversion until the JS thread and hands it a `Cx` — is the only hook for setting a property on the thrown error. That opaque return type is not something `?` can convert into, hence the split. Two things the wasm entry gains beyond the code itself: - `newClient` routed `into_config_map`, `ZeroKMSBuilder::build` and `ScopedCipher::init` through `js_error` rather than `error_to_js`. The divergence was invisible while no code was being carried; it meant the three config codes arrived bare on this entry. - `WasmDecryptResult` is gone. It existed only to describe the missing `code` — the field was synthesised by the Neon JS wrapper, which this build has no equivalent of. Both entries now name one `DecryptResult`. `UNKNOWN_QUERY_OP` is the one code that could not be derived from the variant the error was built as. #143 moved `queryOp` parsing into `query_op.rs`, where an unknown value is rejected inside `Deserialize` — which is what makes the failure name the field rather than surfacing later from query preparation — and serde's `de::Error::custom` takes a `Display`, so nothing typed reaches the boundary. `Error::unknown_query_op` recovers it from the message prefix, and `From<serde_json::Error>` / `wasm::from_js_value` route both entries through it. That is the same prefix match `src/errors.ts` was doing, moved rather than removed, and worth being explicit about in a commit whose point is that codes stop coming from prose. What moving it buys: it sits beside `UNKNOWN_QUERY_OP_PREFIX`, the constant that defines the message, in the same crate and the same review diff, and the prefix is pinned from both sides — `query_op`'s `an_unknown_value_keeps_the_prefix_the_error_routing_matches` and `error_codes::an_unknown_query_op_is_routed_off_the_serde_message`. A change that breaks the mapping fails `cargo test` instead of silently degrading a caller's `code` to `UNKNOWN`. `other_deserialization_failures_stay_uncoded` pins the other side, since a prefix match that over-captured would be worse than none. Rebase note: this series was written against a tree where `UnknownQueryOp` was still a plain `Error` variant, and #143 landed on main in between. `integration-tests/tests/wasm-error-codes.test.ts` covers the wasm entry. It needs no credentials, unlike the round-trip suite (#149) — every case is config validation, which fails before any network I/O. Claude-Session: https://claude.ai/code/session_01BYfRbVEWtYXMG5SPBNdX6m
`inferErrorCode` is deleted. It matched the message against fourteen
prefixes and substrings to recover what Rust had just thrown away — the
same process serialising structure to prose and then parsing the prose
back.
It worked, and it was fragile in a way nothing tested. Three of those
patterns matched wording owned by cipherstash-config, not this repo:
if (message.includes('requires plaintext_type: json'))
return 'STE_VEC_REQUIRES_JSON_CAST_AS'
if (message.includes('unsupported config version'))
return 'UNSUPPORTED_CONFIG_VERSION'
An upstream reword would silently downgrade a caller's error to
`UNKNOWN` — the call still fails, just less usefully, and nothing here
would have failed to say so. Three of fourteen understates it, because
the table gave no way to tell which three: `' index configured'` reads
exactly like an upstream phrase and is this repo's own `MissingIndex`.
`docs/canonical-encryption-config-migration.md` had already flagged the
gap as a follow-up, proposing tests for the substrings. This closes it by
removing them instead.
`normalizeError` reads `err.code` and validates it against the declared
set. Validation is the point: Node puts a `code` on its own errors, so a
bare structural read would let an `ECONNRESET` through as a
`ProtectErrorCode`. `isProtectErrorCode` exports that check, for callers
who cannot rely on `instanceof ProtectError` — the wasm entry has no JS
wrapper to construct one.
`PROTECT_ERROR_CODES` is now the single declaration, with the union
derived from it, and `errorCodes.test.ts` reads the Rust attributes and
proves the two sets agree. That is the one remaining way for this to go
wrong, and it is silent: a code TypeScript does not declare still arrives
at runtime and still fails the predicate.
BREAKING CHANGE: a failed `decryptBulkFallible` item with no code omits
`code` rather than setting `'UNKNOWN'`. The declared type has always been
`code?: ProtectErrorCode`, but the field was in practice always present on
the Neon entry, because the wrapper stored whatever the inference returned.
Test for absence instead.
The api reference stopped restating the union — its copy was already
missing `SHORT_MATCH_NEEDLE`, which is the argument.
Claude-Session: https://claude.ai/code/session_01YJekcEBAsUg8qJoBcqyzBx
The previous commit stopped `src/errors.ts` inferring a code from the message, but left the layer that existed to carry it: every export ran through `wrapAsync`/`wrapSync`, which caught each failure and re-threw it as a `ProtectError`. Once Rust sets `code` on the error it builds, that layer adds nothing. It was not free. It made the two bindings throw different things — wasm has no JS wrapper, so its errors stayed plain. It re-based the stack trace onto the wrapper, demoting the real one to `cause`. And the check it existed to provide, `instanceof`, is false across duplicate copies of a package, which is the failure the issue's payoff section already called out. So the exports return what the binding threw. `newClient` on a bad config now produces a byte-identical error object on Neon and wasm, which is what this PR series has been converging on since #142. No replacement guard is shipped, and the `isProtectError` I first reached for is not there. Narrowing is not a neon limitation — TypeScript types every `catch` variable as `unknown` (TS18046), so a caller narrows once no matter what Rust throws, and neon has no class API, so Rust could not throw an `instanceof`-able class unless JS handed it one. Branching needs nothing from this package: if (err instanceof Error && 'code' in err && err.code === 'MISSING_INDEX') and `isProtectErrorCode`, already exported for validating a code value, narrows `err.code` to a typed `ProtectErrorCode` for callers that want to store it. A second predicate would have been API replacing API. Every export that returns a promise is now `async`, and that keyword is load-bearing rather than stylistic: neon extracts arguments SYNCHRONOUSLY. A bad client handle, an options object serde rejects (every unknown-key rejection from #144), or an out-of-range bigint threw from the call itself, and `wrapAsync` was quietly converting those into rejections. Verified against the built addon that all four still reject rather than throw, and that an out-of-range bigint is still a `RangeError` — which the README and the `JsPlaintext` JSDoc promise, and which previously survived only because the inference table happened not to match its message. BREAKING CHANGE: `ProtectError` is no longer exported and nothing throws it. Both entries throw an ordinary `Error` with a `code` property. Replace `err instanceof ProtectError && err.code === X` with `err instanceof Error && 'code' in err && err.code === X`, or use `isProtectErrorCode` where the code is wanted as a typed value. Claude-Session: https://claude.ai/code/session_01YJekcEBAsUg8qJoBcqyzBx
ci: lint the wasm32 target, and give the Rust checks one entry point
feat!: derive error codes in Rust instead of string-matching Display output (#146)
The commit before this one is a `git subtree add` of `cipherstash/protectjs-ffi` at its v0.31.0 tag, tree-identical to upstream and carrying its full history. This makes it a member of the monorepo. **Scripts are split so the repo stays Rust-free by default.** Root `pnpm test` runs `turbo test --filter './packages/*'`, which now reaches this package — so a cargo process on that path is a Rust toolchain on every contributor's machine. `test` is the JS chain and `build` is `tsc`; `cargo test` and `cargo fmt --check` live behind `test:cargo`, clippy behind `mise run lint:rust`, and `cargo build --release` behind `build:native`. `src/lintWiring.test.ts` enforces the split: no `test:*` script may be unreachable from both entry points, nothing cargo may be reachable from `test`, and every cargo check must be reachable from `test:cargo`. `test:typecheck:wasm` is deliberately NOT here. It needs `dist/wasm`, so it cannot hang off the default `test`, and the exemption list that would carve it out requires a root workflow to name it — the job that does arrives with the workspace link, in the last PR of this stack. Adding the script now would be exactly the laundering the exemption list exists to prevent: a carve-out whose "some other job runs it" reason is prose, and prose does not fail. **Six per-platform binary packages** under `platforms/*` are linked with `workspace:*` and globbed in `pnpm-workspace.yaml` — `packages/*` only reaches one level, so they need their own entry. `.changeset/config.json` gains the matching fixed group. **Publishing has not moved.** All seven packages are still published from `cipherstash/protectjs-ffi` until npm trusted publishing is repointed, so `scripts/lint-no-ffi-changeset.mjs` fails CI on a changeset naming any of them. The package can be changed freely; its changeset waits for the cutover. **`lib/` is the package `main` and is generated**, so a workspace consumer resolves an empty package until `build` has run. `turbo.json` carries a `@cipherstash/protect-ffi#build` override declaring `outputs: ["lib/**"]`; without it Turbo caches the repo-wide `dist/**` and a cache hit restores nothing while reporting success. **Three WASM declaration files are tracked** (`dist/wasm/*.d.ts`) so stack's declaration build resolves `@cipherstash/protect-ffi/wasm-inline` without Rust. Everything else under `dist/` stays ignored; the re-inclusion chain spans the root `.gitignore`, the package's own, and one wasm-pack generates. Consumers are untouched: `@cipherstash/stack` and the two adapters still resolve the published 0.31.0 from npm. Vendoring and switching to the workspace copy are separate steps, and this is the first.
The absorption deposited upstream's workflows under `packages/protect-ffi/.github/`, a directory GitHub never reads — it takes workflows from the repository root alone. So from the day the package landed, its Rust checks and its 19-file live integration suite ran NOWHERE, and a suite that never starts reads exactly like a suite that passes. **`tests-rust.yml`** runs `test:cargo` (cargo test + rustfmt) and `mise run lint:rust` (clippy, host and wasm32), path-filtered to the package. It runs `lint:rust` by name rather than its arms, because an arm reachable only by name is an arm nobody runs; `lintWiring.test.ts` asserts every `lint:rust:*` task is in its `depends` list. **`integration-protect-ffi.yml`** runs the integration suite against live ZeroKMS and a real Postgres. Two things there are deliberately not copies of upstream. It builds the binding with `.github/actions/build-ffi-binding` rather than `mise run build:debug`, because a RELEASE `index.node` at the package root satisfies `src/load.cts`'s `debug:` fallback and that action caches it on a content hash of the Rust inputs; and it invokes vitest directly, since the mise task would recompile in the debug profile over the artifact CI already paid for. It does not use `.github/actions/integration-db` — the EQL installs pipe SQL through `docker exec -i protect-ffi-postgres`, which only this suite's own compose file produces, and that action provides no EQL at all. Nothing else in the repo installs EQL v2, which half the suite needs. `src/integrationSuiteCi.test.ts` asserts a ROOT workflow still invokes the suite, and deliberately scans only the repo-root workflow directory. That is what stops it going quiet again. **Three guards land with the workflows they guard**, each written against a defect that was live rather than hypothetical: - `workflow-dispatch-job-conditions.test.mjs` evaluates every job-level `if:` against a synthetic context per event. Six workflows gated on `github.event_name == 'push' || <same-repo check>` — an ALLOWLIST of events, which fails shut on the one nobody enumerated. On a manual dispatch the event name is neither, and the payload carries no `pull_request` object, so GitHub coerces null to 0 against a string that is NaN and both operands are false: the run is created, the only job is skipped, and it reports success having executed nothing. All six now say "not a fork pull request" instead. The evaluator THROWS on an expression outside its grammar, so a condition it cannot reason about fails loudly rather than being waved through. - `workflow-node-gyp.test.mjs` asserts every job running `pnpm install` puts node-gyp on PATH first, flattening local composite actions so the four jobs that reach their install through `integration-setup` are checked where they actually install. node-pty is the repo's one `onlyBuiltDependencies` entry and ships no linux prebuild, so its `node-gyp rebuild` fallback is unconditional on a Linux runner. - `ffi-binding-action.test.mjs` pins the mise-action SHA and the cache-key inputs of `build-ffi-binding`. Dependabot gains the `cargo` ecosystem for the in-tree workspace — monthly, and ignoring the exact-pinned CipherStash crates that share a release train with the `@cipherstash/auth` catalog. The supply-chain e2e suite now derives ecosystem coverage from the filesystem, so a lockfile for a new language fails the suite until `dependabot.yml` covers it, and checks each entry's `directory` actually contains the manifest its ecosystem reads — load-bearing for cargo, whose workspace root is `packages/protect-ffi`, not the repo root.
🦋 Changeset detectedLatest commit: 9ad87f2 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:
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 |
Vendoring the package puts it inside `turbo test --filter './packages/*'`, so root `pnpm test` now runs protect-ffi's own suite — and `src/nativeLoading.test.ts` asserts the platform binary loads. Nothing in the `run-tests` job produced `index.node`, so it failed with `Cannot find module '.../protect-ffi-linux-x64-gnu/index.node'` on Node 22 and 24. This is a consequence of vendoring alone, not of linking consumers to the workspace copy, which is why the step belongs in this PR rather than the next one. It reproduces only in CI: a local checkout that has ever built the binding keeps `index.node` on disk, gitignored, and the test passes on the stale artifact.
freshtonic
left a comment
There was a problem hiding this comment.
Review — APPROVE
Reviewed all four first-parent commits. The subtree add (aa69b3ea) is byte-identical to upstream protectjs-ffi v0.31.0 and not reviewed beyond "right commit"; the reconcile + CI-wiring commits are the actual work.
Verified locally
pnpm run code:check(biome): 0 errors (warnings/infos pre-existing, CI gates on errors only)pnpm run test:scripts: 285/285 pass- supply-chain e2e: 21/21 pass
- protect-ffi JS unit: 83 pass; the one failure (
assertNativeBindingAvailable > succeeds when the binding is present) needs a compiledindex.nodefrom the Rust build — exactly as the PR's verification note documents. All emit-shape guards (lazy load, no__importStar, body reaches native) pass. - Rust checks + live ZeroKMS/Postgres integration not runnable here (no toolchain/creds), consistent with the PR.
Blocking
None.
Should-fix (non-blocking)
assertNativeBindingAvailablenegative path is untested (packages/protect-ffi/src/index.cts,nativeLoading.test.ts). The function exists to detect a missing binary, and its doc comment promises the error propagates asMODULE_NOT_FOUNDwith the samecode/message— but nothing asserts that. The test defers the negative case to "the CLI's missing-binary fixture," which doesn't exist yet, so the promised contract ships unverified. PointingcreateRequireat a fixture entry whoseload.cjsproxy has no platform match would cover it. Low urgency (new, not yet consumed), but it's the important half of the contract.
Nits
- The new public export + lazy-load behavior change land with no changeset — deliberately, since
scripts/lint-no-ffi-changeset.mjsblocks a protect-ffi changeset until trusted publishing is repointed. The script says the "phase-2 laziness changeset lands too" in the cutover PR; nothing enforces that it isn't forgotten there, so worth a checklist item on phase 4. skills/stash-supply-chain-securitydescribes the cargo/npm cooldown as "7 days minor/patch" and drops the 14-day semver-major window — consistent with majors being ignored entirely, which makessemver-major-days: 14effectively dead config for npm and cargo. Harmless.
Highlights
- Every CI guard is written against a real reproduced defect and asserts the property not a string proxy — the
workflow_dispatchevaluator models GitHub's coercion (null→0, string→NaN, case-insensitive compare) and throws on unknown grammar so "didn't understand it" ≠ "job runs". Guards defend their own vacuity (matched > 0,EXPECTED_*floors). - The lazy native load is a genuine correctness win (pure-JS consumers like
@cipherstash/migratecan import without a binary), verified against emitted JS since the difference is invisible on a machine that has a binary. turbo.json/pnpm-workspace.yaml/dependabot.ymlcomments thoroughly explain the why (Turbo empty-dist/cache trap,platforms/*nesting glob, cargodirectory: /packages/protect-ffinot root).- The filesystem-derived ecosystem-coverage e2e test turns "someone adds a new-language lockfile" into a CI failure instead of a silent monitoring gap.
- Vendored-source reconciliation is pure Biome reformatting — no behavior change.
AGENTS.md compliance
Skill updated in-PR with a matching stash patch changeset; protect-ffi changeset correctly deferred; only the root run-tests job (which now builds the binding) invokes protect-ffi's native suite — tests-bench is scoped to packages/bench. All good.
Stack 3 of 4 — splitting #858. Base: #861.
packages/protect-ffiand wire its own CIReviewing this one
The first commit is a
git subtree addofcipherstash/protectjs-ffiat its v0.31.0 tag — the vendored tree is byte-identical to upstream (verified by tree comparison) and the full upstream history is preserved and reachable. There is nothing to review in it beyond "is this the right commit". The two commits after it are the actual work.Consumers are untouched.
@cipherstash/stackand the adapters still resolve the published 0.31.0 from npm. Vendoring and switching are separate steps, and this is only the first.Commit 2 — reconcile with the monorepo
Scripts are split so the repo stays Rust-free by default. Root
pnpm testreaches this package, so a cargo process on that path would be a Rust toolchain on every contributor's machine.testis the JS chain,buildistsc; cargo lives behindtest:cargo,mise run lint:rustandbuild:native.src/lintWiring.test.tsenforces the split from the manifest side.Six per-platform binary packages linked with
workspace:*and globbed explicitly (packages/*only reaches one level).turbo.jsongains a build override declaringoutputs: ["lib/**"]— without it Turbo caches the repo-widedist/**and a cache hit restores nothing while reporting success. Three WASM.d.tsare tracked so stack's declaration build resolves without Rust.Publishing has not moved —
scripts/lint-no-ffi-changeset.mjsfails CI on a changeset naming any of the seven packages until trusted publishing is repointed.Commit 3 — run its checks from root workflows
The absorption deposited upstream's workflows under
packages/protect-ffi/.github/, a directory GitHub never reads. So from the day the package landed, its Rust checks and its 19-file live integration suite ran nowhere — and a suite that never starts reads exactly like a suite that passes.tests-rust.ymlandintegration-protect-ffi.ymlfix that;src/integrationSuiteCi.test.tsstops it going quiet again.Three guards land with the workflows they guard, each written against a live defect:
workflow-dispatch-job-conditions— six workflows gated on an allowlist of event names, so a manual dispatch created a run, skipped its only job, and reported success having executed nothing. All six now say "not a fork pull request". The evaluator throws on expressions outside its grammar rather than waving them through.workflow-node-gyp— every job runningpnpm installmust have node-gyp on PATH first, flattening composites so the four jobs that install viaintegration-setupare checked where they actually install.ffi-binding-action— pins the mise-action SHA and the cache-key inputs.Dependabot gains the
cargoecosystem; the supply-chain e2e suite now derives ecosystem coverage from the filesystem, so a lockfile for a new language fails the suite untildependabot.ymlcovers it.Verification
protect-ffi's JS chain passing; scripts suite 285 passing; supply-chain e2e 21 passing; biome 0 errors; lockfile in sync.
assertNativeBindingAvailableneeds a compiledindex.node(Rust build) — verified passing locally with the binary present, which CI produces.