Merge pull request #493 from cipherstash/fix/restore-null-guards-in-encryption-ops

fix(stack): restore runtime null guards in encryption operations

Author: coderdan

.changeset/restore-null-guards-in-encryption-ops.md

@@ -0,0 +1,22 @@
+---
+"@cipherstash/stack": minor
+---
+
+Fix: restore runtime null short-circuits in the encryption operation classes.
+
+A prior refactor (`feat(stack): remove null from Encrypted type`) tightened the type signatures to disallow `null` and, alongside that, deleted the `if (value === null) return null` guards from every operation in `packages/stack/src/encryption/operations/`. The type guard does not survive runtime: callers reaching the operation through a cast (e.g. `null as any`), dynamic model walking, or JS interop would then have their null silently encrypted by protect-ffi into a real SteVec ciphertext (`{ k: 'sv', v: 2, ... }`) — which is observable, surprising, and breaks symmetry with the model-helpers layer that does still treat null as "absent" at the field level.
+
+Restored, mirroring the pattern in `@cipherstash/protect`:
+
+- `encrypt` / `encryptWithLockContext`: `if (plaintext === null) return null`.
+- `bulkEncrypt` / `bulkEncryptWithLockContext`: per-element null filter; nulls are preserved in position in the output.
+- `decrypt` / `decryptWithLockContext`: `if (encryptedData === null) return null`.
+- `bulkDecrypt` / `bulkDecryptWithLockContext`: per-element null filter, position-preserving merge.
+- `encryptQuery` / `encryptQueryWithLockContext`: `if (plaintext === null || plaintext === undefined) return { data: null }`.
+- `batchEncryptQuery` / `batchEncryptQueryWithLockContext`: per-element null/undefined filter; null slots in the input array stay null in the result array.
+
+Type adjustments to support the runtime behavior honestly:
+
+- `BulkEncryptPayload['plaintext']`, `BulkEncryptedData['data']`, `BulkDecryptPayload['data']`, and the `T` of `BulkDecryptedData` all widen to `... | null`. Bulk APIs now accept and return mixed nullable arrays without filtering ahead of time.
+- `EncryptedQueryResult` widens to include `null` so the batch query path can return position-stable arrays with null slots.
+- `Encryption.encrypt()` and `Encryption.decrypt()` public signatures are unchanged — still narrow (`JsPlaintext` / `Encrypted` input, `Encrypted` / `JsPlaintext` non-nullable output). The runtime null short-circuit in `EncryptOperation` / `DecryptOperation` is defense in depth for callers reaching the operation classes through casts, dynamic field walking, or JS interop. The narrow-return contract holds for any caller that respects the input contract.

packages/stack/__tests__/null-guards.test.ts

@@ -0,0 +1,108 @@
+// Defense-in-depth tests for the runtime null short-circuits restored
+// across the encryption operation classes. These hit the operation
+// constructors directly rather than going through `Encryption()` so they
+// don't need credentials or a network — the guard short-circuits before
+// any FFI call. See `fix(stack): restore runtime null guards in
+// encryption operations` for context.
+
+import { BatchEncryptQueryOperation } from '@/encryption/operations/batch-encrypt-query'
+import { BulkDecryptOperation } from '@/encryption/operations/bulk-decrypt'
+import { BulkEncryptOperation } from '@/encryption/operations/bulk-encrypt'
+import { DecryptOperation } from '@/encryption/operations/decrypt'
+import { EncryptQueryOperation } from '@/encryption/operations/encrypt-query'
+import { EncryptOperation } from '@/encryption/operations/encrypt'
+import { encryptedColumn, encryptedTable } from '@/schema'
+import { describe, expect, it } from 'vitest'
+
+const table = encryptedTable('null-guards-test', {
+  metadata: encryptedColumn('metadata').searchableJson(),
+})
+
+// Any truthy stand-in — the guard returns before the client is touched.
+const stubClient = {} as any
+
+describe('runtime null guards (defense in depth)', () => {
+  it('encrypt(null) short-circuits without an FFI call', async () => {
+    const op = new EncryptOperation(stubClient, null, {
+      column: table.metadata,
+      table,
+    })
+    const result = await op.execute()
+    if (result.failure) throw new Error(result.failure.message)
+    expect(result.data).toBeNull()
+  })
+
+  it('decrypt(null) short-circuits without an FFI call', async () => {
+    const op = new DecryptOperation(stubClient, null)
+    const result = await op.execute()
+    if (result.failure) throw new Error(result.failure.message)
+    expect(result.data).toBeNull()
+  })
+
+  it('bulkEncrypt preserves null positions in mixed arrays', async () => {
+    // Single null-only input — no FFI call should happen, the all-null
+    // fast path returns a {data: null} placeholder per element.
+    const op = new BulkEncryptOperation(
+      stubClient,
+      [{ id: 'a', plaintext: null }],
+      { column: table.metadata, table },
+    )
+    const result = await op.execute()
+    if (result.failure) throw new Error(result.failure.message)
+    expect(result.data).toEqual([{ id: 'a', data: null }])
+  })
+
+  it('bulkDecrypt preserves null positions in mixed arrays', async () => {
+    const op = new BulkDecryptOperation(stubClient, [{ id: 'a', data: null }])
+    const result = await op.execute()
+    if (result.failure) throw new Error(result.failure.message)
+    expect(result.data).toEqual([{ id: 'a', data: null }])
+  })
+
+  it('encryptQuery(null) short-circuits to { data: null }', async () => {
+    const op = new EncryptQueryOperation(stubClient, null, {
+      column: table.metadata,
+      table,
+      queryType: 'steVecSelector',
+      returnType: 'composite-literal',
+    } as any)
+    const result = await op.execute()
+    if (result.failure) throw new Error(result.failure.message)
+    expect(result.data).toBeNull()
+  })
+
+  it('encryptQuery(undefined) short-circuits to { data: null }', async () => {
+    const op = new EncryptQueryOperation(stubClient, undefined, {
+      column: table.metadata,
+      table,
+      queryType: 'steVecSelector',
+      returnType: 'composite-literal',
+    } as any)
+    const result = await op.execute()
+    if (result.failure) throw new Error(result.failure.message)
+    expect(result.data).toBeNull()
+  })
+
+  it('batchEncryptQuery preserves null/undefined slots position-stably', async () => {
+    // All-null/undefined batch — no FFI call needed.
+    const op = new BatchEncryptQueryOperation(stubClient, [
+      {
+        column: table.metadata,
+        table,
+        queryType: 'steVecSelector',
+        returnType: 'composite-literal',
+        value: null,
+      } as any,
+      {
+        column: table.metadata,
+        table,
+        queryType: 'steVecSelector',
+        returnType: 'composite-literal',
+        value: undefined,
+      } as any,
+    ])
+    const result = await op.execute()
+    if (result.failure) throw new Error(result.failure.message)
+    expect(result.data).toEqual([null, null])
+  })
+})

packages/stack/src/encryption/index.ts

@@ -308,6 +308,13 @@ export class EncryptionClient {
    *      .withLockContext(lockContext)
    * ```
    *
+   * @remarks
+   * The public input type rejects null, but at runtime `decrypt` will
+   * short-circuit and return null when given a null ciphertext
+   * (defense in depth for legacy / manually-NULLed DB rows reached via
+   * casts or dynamic field walking). The narrow return type holds for
+   * any caller that respects the input contract.
+   *
    * @see {@link LockContext}
    * @see {@link DecryptOperation}
    */

packages/stack/src/encryption/operations/batch-encrypt-query.ts

@@ -6,6 +6,7 @@ import type { Client, EncryptedQueryResult, ScalarQueryTerm } from '@/types'
 import { createRequestLogger } from '@/utils/logger'
 import { type Result, withResult } from '@byteslice/result'
 import {
+  type JsPlaintext,
   type QueryPayload,
   encryptQueryBulk as ffiEncryptQueryBulk,
 } from '@cipherstash/protect-ffi'
@@ -21,6 +22,21 @@ import {
 import { noClientError } from '../index'
 import { EncryptionOperation } from './base-operation'
 
+// Separates null/undefined values from non-null terms in the input array
+// so they bypass the FFI call. Original indices are tracked so the
+// reassembled result preserves position.
+function filterNullTerms(terms: readonly ScalarQueryTerm[]): {
+  nonNullTerms: { term: ScalarQueryTerm; originalIndex: number }[]
+} {
+  const nonNullTerms: { term: ScalarQueryTerm; originalIndex: number }[] = []
+  terms.forEach((term, index) => {
+    if (term.value !== null && term.value !== undefined) {
+      nonNullTerms.push({ term, originalIndex: index })
+    }
+  })
+  return { nonNullTerms }
+}
+
 /**
  * Validates and transforms a single term into a QueryPayload.
  * Throws an error if the value is NaN or Infinity.
@@ -42,7 +58,7 @@ function buildQueryPayload(
   assertValueIndexCompatibility(term.value, indexType, term.column.getName())
 
   const payload: QueryPayload = {
-    plaintext: term.value,
+    plaintext: term.value as JsPlaintext,
     column: term.column.getName(),
     table: term.table.tableName,
     indexType,
@@ -57,15 +73,22 @@ function buildQueryPayload(
 }
 
 /**
- * Maps encrypted values to formatted results based on term.returnType.
+ * Reassembles the result array, slotting null at the original indices of
+ * filtered-out terms and formatting encrypted values for non-null terms.
  */
 function assembleResults(
-  terms: readonly ScalarQueryTerm[],
+  totalLength: number,
   encryptedValues: (CipherStashEncrypted | CipherStashEncryptedQuery)[],
+  nonNullTerms: { term: ScalarQueryTerm; originalIndex: number }[],
 ): EncryptedQueryResult[] {
-  return terms.map((term, i) =>
-    formatEncryptedResult(encryptedValues[i], term.returnType),
-  )
+  const results: EncryptedQueryResult[] = new Array(totalLength).fill(null)
+  nonNullTerms.forEach(({ term, originalIndex }, i) => {
+    results[originalIndex] = formatEncryptedResult(
+      encryptedValues[i],
+      term.returnType,
+    )
+  })
+  return results
 }
 
 /**
@@ -107,13 +130,20 @@ export class BatchEncryptQueryOperation extends EncryptionOperation<
       return { data: [] }
     }
 
+    const { nonNullTerms } = filterNullTerms(this.terms)
+
+    if (nonNullTerms.length === 0) {
+      log.emit()
+      return { data: this.terms.map(() => null) }
+    }
+
     const result = await withResult(
       async () => {
         if (!this.client) throw noClientError()
 
         const { metadata } = this.getAuditData()
 
-        const queries: QueryPayload[] = this.terms.map((term) =>
+        const queries: QueryPayload[] = nonNullTerms.map(({ term }) =>
           buildQueryPayload(term),
         )
 
@@ -122,7 +152,7 @@ export class BatchEncryptQueryOperation extends EncryptionOperation<
           unverifiedContext: metadata,
         })
 
-        return assembleResults(this.terms, encrypted)
+        return assembleResults(this.terms.length, encrypted, nonNullTerms)
       },
       (error: unknown) => {
         log.set({ errorCode: getErrorCode(error) ?? 'unknown' })
@@ -169,6 +199,15 @@ export class BatchEncryptQueryOperationWithLockContext extends EncryptionOperati
       return { data: [] }
     }
 
+    // Check for all-null terms BEFORE fetching lockContext to avoid an
+    // unnecessary network call.
+    const { nonNullTerms } = filterNullTerms(this.terms)
+
+    if (nonNullTerms.length === 0) {
+      log.emit()
+      return { data: this.terms.map(() => null) }
+    }
+
     const lockContextResult = await this.lockContext.getLockContext()
     if (lockContextResult.failure) {
       log.emit()
@@ -183,7 +222,7 @@ export class BatchEncryptQueryOperationWithLockContext extends EncryptionOperati
 
         const { metadata } = this.getAuditData()
 
-        const queries: QueryPayload[] = this.terms.map((term) =>
+        const queries: QueryPayload[] = nonNullTerms.map(({ term }) =>
           buildQueryPayload(term, context),
         )
 
@@ -193,7 +232,7 @@ export class BatchEncryptQueryOperationWithLockContext extends EncryptionOperati
           unverifiedContext: metadata,
         })
 
-        return assembleResults(this.terms, encrypted)
+        return assembleResults(this.terms.length, encrypted, nonNullTerms)
       },
       (error: unknown) => {
         log.set({ errorCode: getErrorCode(error) ?? 'unknown' })

packages/stack/src/encryption/operations/bulk-decrypt.ts

@@ -5,40 +5,53 @@ import type { BulkDecryptPayload, BulkDecryptedData, Client } from '@/types'
 import { createRequestLogger } from '@/utils/logger'
 import { type Result, withResult } from '@byteslice/result'
 import {
+  type Encrypted as CipherStashEncrypted,
   type DecryptResult,
   decryptBulkFallible,
 } from '@cipherstash/protect-ffi'
 import { noClientError } from '../index'
 import { EncryptionOperation } from './base-operation'
 
-// Helper functions for better composability
+// Drops nulls so they don't reach protect-ffi's bulk decrypt. The
+// dropped positions are re-inserted as null in `mapDecryptedDataToResult`.
 const createDecryptPayloads = (
   encryptedPayloads: BulkDecryptPayload,
   lockContext?: Context,
 ) => {
-  return encryptedPayloads.map(({ id, data }) => ({
-    id,
-    ciphertext: data,
-    ...(lockContext && { lockContext }),
-  }))
+  return encryptedPayloads
+    .filter(({ data }) => data !== null)
+    .map(({ id, data }) => ({
+      id,
+      ciphertext: data as CipherStashEncrypted,
+      ...(lockContext && { lockContext }),
+    }))
 }
 
+const createNullResult = (
+  encryptedPayloads: BulkDecryptPayload,
+): BulkDecryptedData =>
+  encryptedPayloads.map(({ id }) => ({ id, data: null }))
+
 const mapDecryptedDataToResult = (
   encryptedPayloads: BulkDecryptPayload,
   decryptedData: DecryptResult[],
 ): BulkDecryptedData => {
-  return decryptedData.map((decryptResult, i) => {
-    if ('error' in decryptResult) {
-      return {
-        id: encryptedPayloads[i].id,
-        error: decryptResult.error,
+  const result: BulkDecryptedData = new Array(encryptedPayloads.length)
+  let decryptedIndex = 0
+  for (let i = 0; i < encryptedPayloads.length; i++) {
+    if (encryptedPayloads[i].data === null) {
+      result[i] = { id: encryptedPayloads[i].id, data: null }
+    } else {
+      const decryptResult = decryptedData[decryptedIndex]
+      if ('error' in decryptResult) {
+        result[i] = { id: encryptedPayloads[i].id, error: decryptResult.error }
+      } else {
+        result[i] = { id: encryptedPayloads[i].id, data: decryptResult.data }
       }
+      decryptedIndex++
     }
-    return {
-      id: encryptedPayloads[i].id,
-      data: decryptResult.data,
-    }
-  })
+  }
+  return result
 }
 
 export class BulkDecryptOperation extends EncryptionOperation<BulkDecryptedData> {
@@ -71,12 +84,16 @@ export class BulkDecryptOperation extends EncryptionOperation<BulkDecryptedData>
         if (!this.encryptedPayloads || this.encryptedPayloads.length === 0)
           return []
 
-        const payloads = createDecryptPayloads(this.encryptedPayloads)
+        const nonNullPayloads = createDecryptPayloads(this.encryptedPayloads)
+
+        if (nonNullPayloads.length === 0) {
+          return createNullResult(this.encryptedPayloads)
+        }
 
         const { metadata } = this.getAuditData()
 
         const decryptedData = await decryptBulkFallible(this.client, {
-          ciphertexts: payloads,
+          ciphertexts: nonNullPayloads,
           unverifiedContext: metadata,
         })
 
@@ -140,15 +157,19 @@ export class BulkDecryptOperationWithLockContext extends EncryptionOperation<Bul
           throw new Error(`[encryption]: ${context.failure.message}`)
         }
 
-        const payloads = createDecryptPayloads(
+        const nonNullPayloads = createDecryptPayloads(
           encryptedPayloads,
           context.data.context,
         )
 
+        if (nonNullPayloads.length === 0) {
+          return createNullResult(encryptedPayloads)
+        }
+
         const { metadata } = this.getAuditData()
 
         const decryptedData = await decryptBulkFallible(client, {
-          ciphertexts: payloads,
+          ciphertexts: nonNullPayloads,
           serviceToken: context.data.ctsToken,
           unverifiedContext: metadata,
         })