fix(stack): restore runtime null guards in encryption operations#493
Merged
Conversation
Commit 5b7288b (feat(stack): remove null from Encrypted type) tightened the public types to disallow null in single-value encrypt/decrypt, and deleted the matching runtime `if (value === null) return null` guards across every operation in packages/stack/src/encryption/operations/. The type narrowing does not survive runtime. Callers that reach an operation through a cast (e.g. `null as any`), dynamic model field walking, or JS interop will silently have their null encrypted by protect-ffi into a real SteVec ciphertext (`{ k: 'sv', v: 2, ... }`), breaking symmetry with the model-helpers layer where null is treated as "absent" at the field level. The newly-ported searchable-json `round- trips null values` test in PR #328 exercises this path directly and surfaced the regression. Restored guards mirror the @cipherstash/protect pattern: - encrypt / EncryptOperationWithLockContext: early return null - bulkEncrypt / *WithLockContext: filter-null + position-preserving merge - decrypt / *WithLockContext: early return null - bulkDecrypt / *WithLockContext: filter-null + position-preserving merge - encryptQuery / *WithLockContext: return { data: null } for null/undefined - batchEncryptQuery / *WithLockContext: per-element filter, position-stable Internal operation field/return types widen to `T | null` so the restored guards compile. Public bulk types (`BulkEncryptPayload`, `BulkEncryptedData`, `BulkDecryptPayload`, `BulkDecryptedData`) and `EncryptedQueryResult` widen to admit null in element positions — these now honestly reflect runtime behavior and let callers process mixed nullable arrays without filtering ahead of time. `Encryption.decrypt()` accepts `Encrypted | null`. `Encryption.encrypt()`'s public signature stays narrow (`JsPlaintext`); the runtime guard is defense in depth.
🦋 Changeset detectedLatest commit: 0e778c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRestores runtime null short-circuiting across encrypt/decrypt, bulk, and batch query operations; widens relevant bulk/query types to accept null; bulk/batch flows filter null inputs before FFI calls and reconstruct outputs to preserve original input positions. ChangesNull-Safe Encryption Operations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
coderdan
added a commit
that referenced
this pull request
May 26, 2026
Contributor
Author
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/stack/src/encryption/operations/bulk-encrypt.ts (1)
189-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the all-null short-circuit before lock-context retrieval.
Line 189 fetches lock context before the null-only check at Line 201. For all-null payloads, this can fail or add latency even though no encryption call is needed.
Suggested fix
- const context = await this.lockContext.getLockContext() - if (context.failure) { - throw new Error(`[encryption]: ${context.failure.message}`) - } - - const nonNullPayloads = createEncryptPayloads( + const nonNullPayloads = createEncryptPayloads( plaintexts, column, table, - context.data.context, ) if (nonNullPayloads.length === 0) { return createNullResult(plaintexts) } + + const context = await this.lockContext.getLockContext() + if (context.failure) { + throw new Error(`[encryption]: ${context.failure.message}`) + } + + const payloadsWithContext = nonNullPayloads.map((p) => ({ + ...p, + lockContext: context.data.context, + })) const { metadata } = this.getAuditData() const encryptedData = await encryptBulk(client, { - plaintexts: nonNullPayloads, + plaintexts: payloadsWithContext, serviceToken: context.data.ctsToken, unverifiedContext: metadata, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/src/encryption/operations/bulk-encrypt.ts` around lines 189 - 203, The code calls this.lockContext.getLockContext() before checking for an all-null short-circuit; move the null-only check so we avoid acquiring the lock/context when unnecessary. Specifically, call createEncryptPayloads(plaintexts, column, table, ...) or otherwise determine nonNullPayloads from plaintexts first (using the same logic that createEncryptPayloads uses) and if nonNullPayloads.length === 0 return createNullResult(plaintexts) before invoking lockContext.getLockContext(); only after that short-circuit acquire the lock via this.lockContext.getLockContext() and then proceed to use context.data.context for encryption.packages/stack/src/encryption/operations/bulk-decrypt.ts (1)
155-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShort-circuit null-only decrypt payloads before lock-context fetch.
Line 155 retrieves lock context before the all-null guard at Line 165. Null-only inputs should return immediately and not depend on lock-context availability.
Suggested fix
- const context = await this.lockContext.getLockContext() - if (context.failure) { - throw new Error(`[encryption]: ${context.failure.message}`) - } - const nonNullPayloads = createDecryptPayloads( - encryptedPayloads, - context.data.context, + encryptedPayloads, ) if (nonNullPayloads.length === 0) { return createNullResult(encryptedPayloads) } + + const context = await this.lockContext.getLockContext() + if (context.failure) { + throw new Error(`[encryption]: ${context.failure.message}`) + } + + const payloadsWithContext = nonNullPayloads.map((p) => ({ + ...p, + lockContext: context.data.context, + })) const { metadata } = this.getAuditData() const decryptedData = await decryptBulkFallible(client, { - ciphertexts: nonNullPayloads, + ciphertexts: payloadsWithContext, serviceToken: context.data.ctsToken, unverifiedContext: metadata, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/src/encryption/operations/bulk-decrypt.ts` around lines 155 - 167, The code fetches lock context before short-circuiting null-only inputs; move the early-return so null-only decrypts do not call lockContext.getLockContext(): first detect if the incoming encryptedPayloads are all "null" decrypts (use a simple predicate over encryptedPayloads to determine null-only), if so immediately return createNullResult(encryptedPayloads), and only after that call this.lockContext.getLockContext() and then invoke createDecryptPayloads(encryptedPayloads, context.data.context) for non-null work; reference symbols: lockContext.getLockContext, createDecryptPayloads, createNullResult.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/stack/src/encryption/operations/bulk-decrypt.ts`:
- Around line 155-167: The code fetches lock context before short-circuiting
null-only inputs; move the early-return so null-only decrypts do not call
lockContext.getLockContext(): first detect if the incoming encryptedPayloads are
all "null" decrypts (use a simple predicate over encryptedPayloads to determine
null-only), if so immediately return createNullResult(encryptedPayloads), and
only after that call this.lockContext.getLockContext() and then invoke
createDecryptPayloads(encryptedPayloads, context.data.context) for non-null
work; reference symbols: lockContext.getLockContext, createDecryptPayloads,
createNullResult.
In `@packages/stack/src/encryption/operations/bulk-encrypt.ts`:
- Around line 189-203: The code calls this.lockContext.getLockContext() before
checking for an all-null short-circuit; move the null-only check so we avoid
acquiring the lock/context when unnecessary. Specifically, call
createEncryptPayloads(plaintexts, column, table, ...) or otherwise determine
nonNullPayloads from plaintexts first (using the same logic that
createEncryptPayloads uses) and if nonNullPayloads.length === 0 return
createNullResult(plaintexts) before invoking lockContext.getLockContext(); only
after that short-circuit acquire the lock via this.lockContext.getLockContext()
and then proceed to use context.data.context for encryption.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b74b156c-8649-4fcb-96dd-3456d39ce5c5
📒 Files selected for processing (9)
.changeset/restore-null-guards-in-encryption-ops.mdpackages/stack/src/encryption/index.tspackages/stack/src/encryption/operations/batch-encrypt-query.tspackages/stack/src/encryption/operations/bulk-decrypt.tspackages/stack/src/encryption/operations/bulk-encrypt.tspackages/stack/src/encryption/operations/decrypt.tspackages/stack/src/encryption/operations/encrypt-query.tspackages/stack/src/encryption/operations/encrypt.tspackages/stack/src/types.ts
7 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restores runtime null short-circuits across @cipherstash/stack encryption operations so null inputs do not reach @cipherstash/protect-ffi (which would otherwise encrypt JSON null into a real ciphertext), and updates types to reflect the null-preserving behavior.
Changes:
- Reintroduced runtime
null/undefinedguards for single encrypt/decrypt and query-encrypt operations. - Updated bulk and batch operations to filter nullish inputs, call the FFI only for non-null elements, and then reassemble position-stable outputs with
nullslots preserved. - Widened relevant public types (bulk payload/results, query result) and added a changeset entry.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/stack/src/types.ts | Widens bulk/query result types to include null for position-stable null preservation. |
| packages/stack/src/encryption/operations/encrypt.ts | Adds runtime guard to short-circuit null plaintexts before FFI encryption. |
| packages/stack/src/encryption/operations/decrypt.ts | Adds runtime guard to short-circuit null ciphertexts before FFI decryption. |
| packages/stack/src/encryption/operations/bulk-encrypt.ts | Filters null plaintexts out of the FFI call and re-inserts null in the output at original indices. |
| packages/stack/src/encryption/operations/bulk-decrypt.ts | Filters null ciphertexts out of the FFI call and re-inserts null in the output at original indices. |
| packages/stack/src/encryption/operations/encrypt-query.ts | Returns { data: null } for nullish plaintexts and adjusts typings to allow nullish input. |
| packages/stack/src/encryption/operations/batch-encrypt-query.ts | Filters nullish query terms and reconstructs a position-stable results array with null slots. |
| packages/stack/src/encryption/index.ts | Widens decrypt() to accept `Encrypted |
| .changeset/restore-null-guards-in-encryption-ops.md | Adds a patch changeset describing the null-guard restoration and type changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Keep public encrypt() / decrypt() return types narrow. The runtime guards now cast `null as unknown as Encrypted` / `JsPlaintext` at the short-circuit site so callers who respect the input contract get a non-nullable `data` field as before. The cast acknowledges that the null branch is unreachable by the public type — defense in depth only trips on direct construction with a cast plaintext / ciphertext. - Public `Encryption.decrypt()` reverts to `decrypt(encryptedData: Encrypted)` (was widened to `Encrypted | null`). JSDoc gets a `@remarks` block documenting the runtime null short-circuit so the defense-in-depth behavior is discoverable. - encrypt-query operations no longer carry a redundant `| null` in their generic / execute return — `EncryptedQueryResult` already includes null, so the union was double-nullable. - encrypt-query: capture a local `const plaintext: JsPlaintext` after the null/undefined guard so the three `as JsPlaintext` casts inside withResult drop. Same in the WithLockContext variant. - bulk-encrypt: consolidate the two `@/types` import statements. - Add `__tests__/null-guards.test.ts` covering encrypt(null), decrypt(null), bulkEncrypt all-null, bulkDecrypt all-null, encryptQuery(null|undefined), and batchEncryptQuery all-null/undefined. Constructs operation classes directly so the test runs without credentials — the short-circuits return before any FFI call.
coderdan
added a commit
that referenced
this pull request
May 26, 2026
Contributor
Author
|
Addressed Copilot's review in
PR #328 rebased on the updated fix branch. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/stack/src/encryption/operations/bulk-encrypt.ts (1)
189-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAll-null lock-context path still performs context retrieval before short-circuit.
At Line 189,
getLockContext()is called before the all-null check at Line 201. For all-null payloads, this can fail on context lookup instead of returning{ id, data: null }placeholders.💡 Proposed fix
- const context = await this.lockContext.getLockContext() - if (context.failure) { - throw new Error(`[encryption]: ${context.failure.message}`) - } - - const nonNullPayloads = createEncryptPayloads( - plaintexts, - column, - table, - context.data.context, - ) - - if (nonNullPayloads.length === 0) { + const hasNonNullPlaintext = plaintexts.some( + ({ plaintext }) => plaintext !== null, + ) + if (!hasNonNullPlaintext) { return createNullResult(plaintexts) } + + const context = await this.lockContext.getLockContext() + if (context.failure) { + throw new Error(`[encryption]: ${context.failure.message}`) + } + + const nonNullPayloads = createEncryptPayloads( + plaintexts, + column, + table, + context.data.context, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/src/encryption/operations/bulk-encrypt.ts` around lines 189 - 203, The code calls this.lockContext.getLockContext() before checking for an all-null case, which can cause unnecessary context lookup failures; change the order so you first call createEncryptPayloads(plaintexts, column, table, ...) to compute nonNullPayloads and if nonNullPayloads.length === 0 immediately return createNullResult(plaintexts) without calling this.lockContext.getLockContext(); only when nonNullPayloads is non-empty call this.lockContext.getLockContext(), check context.failure and proceed using context.data.context for the encryption flow.packages/stack/src/encryption/operations/encrypt.ts (1)
138-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep numeric validation consistent in the lock-context path.
EncryptOperation.execute()still rejectsNaN/Infinityon Lines 73-85, but this branch only short-circuitsnulland then goes straight toffiEncrypt. That makesencrypt(x)andencrypt(x).withLockContext(...)behave differently for the same invalid numeric input.Suggested fix
async () => { if (!client) { throw noClientError() } if (plaintext === null) { return null as unknown as Encrypted } + + if (typeof plaintext === 'number' && Number.isNaN(plaintext)) { + throw new Error('[encryption]: Cannot encrypt NaN value') + } + + if (typeof plaintext === 'number' && !Number.isFinite(plaintext)) { + throw new Error('[encryption]: Cannot encrypt Infinity value') + } const { metadata } = this.getAuditData() const context = await this.lockContext.getLockContext()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/src/encryption/operations/encrypt.ts` around lines 138 - 173, EncryptOperation.execute() is missing the numeric validation for NaN/Infinity in the lock-context branch, causing different behavior between the short-circuit path and the path that calls lockContext and ffiEncrypt; before calling ffiEncrypt (after obtaining context.data and metadata) validate the plaintext the same way the non-lock path does (reject NaN and Infinity) and throw the same error/Result as the other branch so encrypt(x) and encrypt(x).withLockContext(...) behave identically; update the logic in EncryptOperation.execute() just prior to calling ffiEncrypt to perform that numeric check on plaintext and surface the same error.
🧹 Nitpick comments (1)
packages/stack/__tests__/null-guards.test.ts (1)
42-53: ⚡ Quick winAdd a
withLockContext()all-null short-circuit test to guard this regression.This suite validates non-lock bulk null handling, but it doesn’t cover the lock-context variant. A dedicated all-null
withLockContext()case would catch the Line 189 ordering issue inpackages/stack/src/encryption/operations/bulk-encrypt.ts.✅ Suggested test shape
+ it('bulkEncrypt.withLockContext all-null short-circuits before context lookup', async () => { + const op = new BulkEncryptOperation( + stubClient, + [{ id: 'a', plaintext: null }], + { column: table.metadata, table }, + ).withLockContext({ + getLockContext: async () => { + throw new Error('should not be called') + }, + } as any) + + const result = await op.execute() + if (result.failure) throw new Error(result.failure.message) + expect(result.data).toEqual([{ id: 'a', data: null }]) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/__tests__/null-guards.test.ts` around lines 42 - 53, Add a new unit test that mirrors the existing all-null fast-path case but invokes the lock-context variant by calling withLockContext() on the BulkEncryptOperation so the all-null short-circuit is exercised under lock; specifically, construct a BulkEncryptOperation with [{id: 'a', plaintext: null}], call operation.withLockContext() before execute(), assert no FFI call is made and the result.data equals [{id: 'a', data: null}], and ensure the test targets the behavior in bulk-encrypt.ts (BulkEncryptOperation.execute / withLockContext) to catch the ordering/regression described.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/stack/src/encryption/operations/bulk-encrypt.ts`:
- Around line 189-203: The code calls this.lockContext.getLockContext() before
checking for an all-null case, which can cause unnecessary context lookup
failures; change the order so you first call createEncryptPayloads(plaintexts,
column, table, ...) to compute nonNullPayloads and if nonNullPayloads.length ===
0 immediately return createNullResult(plaintexts) without calling
this.lockContext.getLockContext(); only when nonNullPayloads is non-empty call
this.lockContext.getLockContext(), check context.failure and proceed using
context.data.context for the encryption flow.
In `@packages/stack/src/encryption/operations/encrypt.ts`:
- Around line 138-173: EncryptOperation.execute() is missing the numeric
validation for NaN/Infinity in the lock-context branch, causing different
behavior between the short-circuit path and the path that calls lockContext and
ffiEncrypt; before calling ffiEncrypt (after obtaining context.data and
metadata) validate the plaintext the same way the non-lock path does (reject NaN
and Infinity) and throw the same error/Result as the other branch so encrypt(x)
and encrypt(x).withLockContext(...) behave identically; update the logic in
EncryptOperation.execute() just prior to calling ffiEncrypt to perform that
numeric check on plaintext and surface the same error.
---
Nitpick comments:
In `@packages/stack/__tests__/null-guards.test.ts`:
- Around line 42-53: Add a new unit test that mirrors the existing all-null
fast-path case but invokes the lock-context variant by calling withLockContext()
on the BulkEncryptOperation so the all-null short-circuit is exercised under
lock; specifically, construct a BulkEncryptOperation with [{id: 'a', plaintext:
null}], call operation.withLockContext() before execute(), assert no FFI call is
made and the result.data equals [{id: 'a', data: null}], and ensure the test
targets the behavior in bulk-encrypt.ts (BulkEncryptOperation.execute /
withLockContext) to catch the ordering/regression described.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a639a3a-f122-4358-8594-0647b36447e1
📒 Files selected for processing (7)
.changeset/restore-null-guards-in-encryption-ops.mdpackages/stack/__tests__/null-guards.test.tspackages/stack/src/encryption/index.tspackages/stack/src/encryption/operations/bulk-encrypt.tspackages/stack/src/encryption/operations/decrypt.tspackages/stack/src/encryption/operations/encrypt-query.tspackages/stack/src/encryption/operations/encrypt.ts
✅ Files skipped from review due to trivial changes (2)
- packages/stack/src/encryption/index.ts
- .changeset/restore-null-guards-in-encryption-ops.md
auxesis
approved these changes
May 26, 2026
Per Lindsay's review on #493: the public type signatures shifted (BulkEncryptPayload, BulkEncryptedData, BulkDecryptPayload, BulkDecryptedData, and EncryptedQueryResult all admit null in element positions, where they used to be narrow). Patch undersells that for SDK consumers — bumping to minor.
coderdan
added a commit
that referenced
this pull request
May 26, 2026
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
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.
Fixes #494.
Summary
Restores the
if (value === null) return nullguards that were stripped from every operation inpackages/stack/src/encryption/operations/by commit5b7288b feat(stack): remove null from Encrypted type. That commit was meant as a type-level tightening but also deleted the matching runtime guards — so callers reaching an operation through a cast, dynamic model walking, or JS interop would silently get their null encrypted into a real SteVec ciphertext ({ k: 'sv', v: 2, ... }).Discovered via the newly-ported
round-trips null valuesintegration test in #328, which callsencrypt(null)directly and asserts the round-trip preserves null. Without these guards, that test fails with:What changed
Mirrors the
@cipherstash/protectpattern across all six operations:encrypt/*WithLockContextreturn nullon null plaintextbulkEncrypt/*WithLockContextdecrypt/*WithLockContextreturn nullon null ciphertextbulkDecrypt/*WithLockContextencryptQuery/*WithLockContext{ data: null }for null/undefinedbatchEncryptQuery/*WithLockContextType adjustments so the restored guards compile and honestly reflect runtime:
BulkEncryptPayload['plaintext'],BulkEncryptedData['data'],BulkDecryptPayload['data'], and theTofBulkDecryptedDatawiden to... | null. Bulk APIs now accept and return mixed nullable arrays without ahead-of-time filtering.EncryptedQueryResultwidens to includenullfor batch-path position-stability.Encryption.decrypt()acceptsEncrypted | null.Encryption.encrypt()public signature is unchanged (stillJsPlaintext). The runtime guard is defense in depth.Test plan
round-trips null valuestest passes__tests__/nested-models.test.tsand__tests__/bulk-protect.test.ts(these exercise null handling through the model layer)Summary by CodeRabbit
Bug Fixes
Tests