diff --git a/.changeset/raw-decrypt-date-boundary.md b/.changeset/raw-decrypt-date-boundary.md new file mode 100644 index 000000000..9ed37c620 --- /dev/null +++ b/.changeset/raw-decrypt-date-boundary.md @@ -0,0 +1,18 @@ +--- +'@cipherstash/stack': patch +'stash': patch +--- + +Document the `Date` reconstruction boundary on `decrypt` / `bulkDecrypt`, and correct the reason given for it. + +A `types.Date` / `types.Timestamp` column comes back as a `Date` from `decryptModel(row, table)` and as the string it was stored as from `decrypt(payload)` / `bulkDecrypt(payloads)`. Reconstruction is driven by the table's `cast_as`, and only the model path is handed a table. That split is intentional — the raw methods resolve to the FFI plaintext union, which excludes `Date`, so reconstructing without widening the return type would make the declared type wrong — but it was undocumented, and the JSDoc explained it with a reason that does not hold: that a lone ciphertext "carries no column identity". Every stored payload carries `i: { t, c }`, so the identity is present and simply unused. The real constraint is static typing (TypeScript cannot know which column a runtime payload came from), not a missing capability at runtime. + +No behaviour change. What changed: + +- `decrypt` and the new `bulkDecrypt` JSDoc state the boundary, its consequence, and the actual reason, on both the native and `wasm-inline` entries. +- The one-arg `decryptModel(row)` / `bulkDecryptModels(rows)` overloads had the same wrong justification ("there is no `cast_as` to reconstruct from"); corrected to name the real one — the `table` argument is what selects reconstruction, and `Decrypted` types those fields `string` to match. +- `skills/stash-encryption` and the `@cipherstash/stack` README now call out the `Date`-vs-string consequence and point at the model helpers. Both also stop calling `bulkEncrypt` untyped: its plaintexts are pinned to the column's domain via `{ table, column }`, exactly like `encrypt` — it is `bulkDecrypt`, which takes the payloads alone, that resolves to the untyped plaintext union. +- `bulkDecrypt` was the one path with no test either way; it is now pinned, using a payload whose `i: { t, c }` names a registered date-like column, so a future change of heart is a deliberate decision rather than a silent drift. +- The boundary is pinned at the type layer too, which is the layer it is argued from: the raw paths resolve to the FFI plaintext union with no `Date` arm, the table-taking model path resolves to `Date`, and the table-less one to `string`. If `JsPlaintext` ever gains a `Date` arm upstream, the justification for the split expires — and the type tests fail rather than the docs quietly going stale. + +Resolves #779. diff --git a/packages/stack/README.md b/packages/stack/README.md index bef0cdd50..0e310b0d2 100644 --- a/packages/stack/README.md +++ b/packages/stack/README.md @@ -231,7 +231,7 @@ const decrypted = await client.bulkDecryptModels(encrypted.data, users) #### Bulk Encrypt / Decrypt (raw values) -`bulkEncrypt` / `bulkDecrypt` are untyped passthroughs for raw value arrays: +`bulkEncrypt` / `bulkDecrypt` work on raw value arrays rather than models. `bulkEncrypt` is typed like `encrypt` — `{ table, column }` pins every `plaintext` to that column's domain — while `bulkDecrypt` takes the payloads alone, so it resolves to the plaintext union and does **no `Date` reconstruction**: a `types.Date` / `types.Timestamp` column read this way is the string it was stored as, where `bulkDecryptModels(rows, table)` gives you a `Date`. See [`EncryptionClient` Methods](#encryptionclient-methods) for why. ```typescript const plaintexts = [ @@ -736,7 +736,9 @@ Method signatures are derived from your schemas: plaintext arguments are pinned | `bulkDecrypt` | `(encryptedPayloads)` | `BulkDecryptOperation` (thenable) | | `getEncryptConfig` | `()` | The resolved encrypt config | -The thenable operations support `.withLockContext(lockContext)` for identity-aware encryption, and `decryptModel` / `bulkDecryptModels` also support `.audit({ metadata })`. Those two additionally accept the lock context as an optional third argument — use one form or the other. `decrypt` of a single value cannot be strongly typed (a lone ciphertext carries no column identity), and `encryptQuery` rejects storage-only columns at compile time. +The thenable operations support `.withLockContext(lockContext)` for identity-aware encryption, and `decryptModel` / `bulkDecryptModels` also support `.audit({ metadata })`. Those two additionally accept the lock context as an optional third argument — use one form or the other. `decrypt` of a single value cannot be strongly typed (TypeScript cannot know which column a runtime payload came from), and `encryptQuery` rejects storage-only columns at compile time. + +**`decrypt` / `bulkDecrypt` do not reconstruct `Date` values.** A `types.Date` / `types.Timestamp` column read through the raw path comes back as the string it was stored as; read through `decryptModel` / `bulkDecryptModels` with the table, it comes back as a `Date`. Reconstruction is driven by the table's `cast_as`, which only the model path is handed. Use the model helpers when you want the column's declared plaintext type, or rebuild at the call site with `new Date(value)`. ### `LockContext` (legacy) diff --git a/packages/stack/__tests__/typed-client-v3.test-d.ts b/packages/stack/__tests__/typed-client-v3.test-d.ts index 17ca69f92..05ac218ae 100644 --- a/packages/stack/__tests__/typed-client-v3.test-d.ts +++ b/packages/stack/__tests__/typed-client-v3.test-d.ts @@ -1,3 +1,4 @@ +import type { JsPlaintext } from '@cipherstash/protect-ffi' import { describe, expectTypeOf, it } from 'vitest' import type { EncryptionClient } from '@/encryption' // Everything comes from the single `@cipherstash/stack/v3` surface (re-exported @@ -208,6 +209,58 @@ describe('typed v3 client — model decrypt yields precise plaintext', () => { }) }) +/** + * The raw-vs-model `Date` boundary (#779), pinned at the layer it is argued + * from. `typed-client-v3.test.ts` pins the runtime — that the wrapper hands the + * stored string back untouched — but the reason it is allowed to is a type-level + * claim: the raw methods resolve to the FFI plaintext union, which has no `Date` + * arm, so reconstructing would make the declared type wrong. A runtime test + * cannot see that claim expire. If `JsPlaintext` ever gains `Date` upstream, the + * justification dissolves while every runtime assertion still passes; these are + * what notice. + */ +describe('typed v3 client — the raw decrypt paths exclude Date', () => { + /** The `data` of an awaited operation's success arm. */ + type SuccessData = Extract, { data: unknown }>['data'] + + type RawDecrypted = SuccessData> + type RawBulkDecrypted = SuccessData> + + it('resolves decrypt to the FFI plaintext union, unwidened', () => { + expectTypeOf().toEqualTypeOf() + // The whole argument for the split in one assertion: no `Date` arm to + // return one through. Widen `JsPlaintext` upstream and this fails. + expectTypeOf().not.toExtend() + }) + + it('resolves bulkDecrypt items to the same union, per position', () => { + expectTypeOf().toEqualTypeOf< + JsPlaintext | null | undefined + >() + expectTypeOf().not.toExtend() + }) + + it('reconstructs Date on the model path, which is handed the table', () => { + // The contrast the boundary consists of, asserted on the type a caller + // actually awaits (not just the `V3DecryptedModel` mapping above). + type ModelDecrypted = SuccessData< + ReturnType< + typeof client.decryptModel + > + > + expectTypeOf().toEqualTypeOf() + }) + + it('types the table-less model path string, matching its unreconstructed runtime', () => { + // `Decrypted` — no table, no reconstruction, and the declared type says + // so. Reconstructing here would be the lie the JSDoc describes. + type LooseDecrypted = SuccessData< + ReturnType> + > + expectTypeOf().toEqualTypeOf() + }) +}) + describe('typed v3 client — soundness', () => { it('rejects a hand-rolled structural table (no brand / private field)', () => { const fakeTable = { diff --git a/packages/stack/__tests__/typed-client-v3.test.ts b/packages/stack/__tests__/typed-client-v3.test.ts index 563e49c96..ca32583c4 100644 --- a/packages/stack/__tests__/typed-client-v3.test.ts +++ b/packages/stack/__tests__/typed-client-v3.test.ts @@ -1,7 +1,19 @@ import { describe, expect, it } from 'vitest' -import type { EncryptionClient } from '@/encryption' import { createEncryptionClient } from '@/encryption/client-v3' import { encryptedTable, types } from '@/eql/v3' +import type { Encrypted } from '@/types' + +/** + * What `createEncryptionClient` wraps. Derived from the factory rather than + * imported, because `UnderlyingNativeClient` is deliberately module-private — + * naming it here is the only thing a test needs from it, and `Parameters` gets + * that without widening the module's export surface. + * + * The stubs below are cast to this, not to `EncryptionClient`: the wrapper takes + * the NATIVE client, and casting to the public client type instead was a type + * error in every call (invisible, since `__tests__` is not typechecked in CI). + */ +type NativeClientStub = Parameters[0] const table = encryptedTable('t', { when: types.Timestamp('when'), @@ -34,11 +46,11 @@ function fakeOp(result: R) { * A minimal client stub whose model-decrypt methods return an operation * resolving to a fixed `Result` payload. */ -function fakeClient(data: Record): EncryptionClient { +function fakeClient(data: Record): NativeClientStub { return { decryptModel: () => fakeOp({ data }), bulkDecryptModels: () => fakeOp({ data: [data] }), - } as unknown as EncryptionClient + } as unknown as NativeClientStub } describe('createEncryptionClient — decrypt reconstruction', () => { @@ -141,7 +153,7 @@ describe('createEncryptionClient — decrypt reconstruction', () => { fakeOp({ failure: { type: 'DecryptionError', message: 'boom' }, }), - } as unknown as EncryptionClient + } as unknown as NativeClientStub const client = createEncryptionClient(failing, table) const result = await client.decryptModel({}, table) @@ -232,3 +244,90 @@ describe('createEncryptionClient — decrypt reconstruction', () => { expect(rows[0].when).toBe('2021-06-01T00:00:00.000Z') }) }) + +/** + * The raw-path boundary, pinned rather than asserted away (#779). + * + * `decrypt` / `bulkDecrypt` are bare delegations — the typed wrapper adds no + * `Date` reconstruction — so a `timestamp` column read this way is the string it + * was stored as, where `decryptModel(row, table)` above turns the same value + * into a `Date`. That split is deliberate: the raw methods resolve to the FFI + * plaintext union, which excludes `Date`, and reconstructing without widening + * it would make the declared type a lie. + * + * What it is NOT is a missing capability, which is the claim the JSDoc used to + * make. The payloads below carry `i: { t, c }` naming a REGISTERED date-like + * column — the identity needed to resolve `cast_as` is right there and + * deliberately unused. Pinning it here means a future change of heart has to + * delete this test, which is the point: the integration suite covers the + * single-value half end-to-end (`integration/shared/v2-decrypt-compat`), but + * nothing covered the bulk half, and that gap is what let the split read as an + * accident. + */ +describe('createEncryptionClient — raw decrypt paths are unmapped', () => { + const storedIso = '2020-01-02T03:04:05.000Z' + // Shaped like a real storage payload: `i` names `t`'s `when` column, which + // the table above declares `types.Timestamp` (`cast_as: 'timestamp'`). Typed + // as `Encrypted` so both calls below go through the PUBLIC signature — the + // arity a caller actually reaches, unlike the one-arg model tests above. + const payload: Encrypted = { + k: 'ct', + v: 2, + i: { t: 't', c: 'when' }, + c: 'ciphertext', + } + + /** + * Stubs only the two raw methods. They are returned by the wrapper unchanged, + * so — unlike the model paths, which get wrapped in a `MappedDecryptOperation` + * that calls `.execute()` — the stub is awaited directly and can simply be a + * promise of the `Result`. + */ + function rawFakeClient() { + const forwarded: unknown[] = [] + const client = { + decrypt: (encrypted: unknown) => { + forwarded.push(encrypted) + return Promise.resolve({ data: storedIso }) + }, + bulkDecrypt: (payloads: unknown) => { + forwarded.push(payloads) + return Promise.resolve({ + data: [{ id: 'u1', data: storedIso }], + }) + }, + } as unknown as NativeClientStub + return { client, forwarded } + } + + it('returns a date-like column as its stored string from decrypt', async () => { + const { client: underlying, forwarded } = rawFakeClient() + const client = createEncryptionClient(underlying, table) + + const result = await client.decrypt(payload) + expect(result.failure).toBeFalsy() + if (result.failure) return + + expect(result.data).toBe(storedIso) + expect(result.data).not.toBeInstanceOf(Date) + // Bare delegation: the payload reaches the underlying client untouched. + expect(forwarded).toEqual([payload]) + }) + + it('returns date-like columns as stored strings from bulkDecrypt', async () => { + const { client: underlying, forwarded } = rawFakeClient() + const client = createEncryptionClient(underlying, table) + + const result = await client.bulkDecrypt([{ id: 'u1', data: payload }]) + expect(result.failure).toBeFalsy() + if (result.failure) return + + expect(result.data).toHaveLength(1) + const [first] = result.data + expect(first.data).toBe(storedIso) + expect(first.data).not.toBeInstanceOf(Date) + // Position-stable identifiers survive the (absent) mapping too. + expect(first.id).toBe('u1') + expect(forwarded).toEqual([[{ id: 'u1', data: payload }]]) + }) +}) diff --git a/packages/stack/src/encryption/client-v3.ts b/packages/stack/src/encryption/client-v3.ts index 68dd046ea..2d1740857 100644 --- a/packages/stack/src/encryption/client-v3.ts +++ b/packages/stack/src/encryption/client-v3.ts @@ -190,8 +190,32 @@ export interface EncryptionClient< ): BulkEncryptModelsOperation> /** - * Decrypt a single value. Cannot be strongly typed — a lone ciphertext carries - * no column identity — so it resolves to the FFI plaintext union unchanged. + * Decrypt a single stored payload, resolving to the FFI plaintext union + * (`JsPlaintext`) unchanged. + * + * **A `date` / `timestamp` column comes back as the string it was stored as, + * not a `Date`** — where `decryptModel(row, table)` reconstructs it. Same + * value, two JavaScript types, depending on which method read it (#779). + * + * The scalar form cannot be strongly *typed*: TypeScript has no way to know + * which column a runtime `Encrypted` came from, so the static type has to be + * the whole union. That is a statement about the type layer only. At runtime + * every payload carries its own `i: { t, c }` table/column identity, so for + * one of the tables this client was initialized with, the `cast_as` is + * reachable from here — skipping reconstruction is a deliberate contract + * line, not a missing capability. `JsPlaintext` excludes `Date` by + * construction, and that is the type callers have annotated against; + * reconstructing without widening it would make the type a lie. + * + * (A payload from a table NOT in the client's schemas — the legacy EQL v2 + * read path — is the one case where the capability genuinely is absent: the + * `i` names a table there is no encrypt config to resolve. The contract is + * the same either way, which is why the boundary is drawn at the method and + * not per payload.) + * + * For `Date` values, read through {@link decryptModel} / + * {@link bulkDecryptModels} with the table, or rebuild at the call site + * (`new Date(value)`). */ decrypt(encrypted: Encrypted): DecryptOperation @@ -224,8 +248,12 @@ export interface EncryptionClient< /** * Table-less form: decrypt whatever encrypted fields the model carries, with - * no `Date` reconstruction (there is no `cast_as` to reconstruct from) and no - * precise plaintext shape. + * no `Date` reconstruction and no precise plaintext shape. The `table` + * argument is what selects the reconstruction map, and `Decrypted` types + * every decrypted field as `string` to match. (For a table that IS + * registered, the payload's own `i: { t, c }` would resolve the `cast_as` — + * declining to use it is what keeps this arity's runtime agreeing with its + * declared type. #779.) * * This is the read path for rows that predate this client's schemas — legacy * **EQL v2** models above all, whose table is not, and cannot be, a member of @@ -258,6 +286,16 @@ export interface EncryptionClient< plaintexts: BulkEncryptPayloadFor, opts: { table: Table; column: Col }, ): BulkEncryptOperation + + /** + * Decrypt many stored payloads in one ZeroKMS round trip, position-stable + * with a per-item `{ data } | { error }` entry. + * + * Draws the same boundary as {@link decrypt}, for the same reason: **no + * `Date` reconstruction** — date-like columns arrive as their stored strings. + * Prefer {@link bulkDecryptModels} with the table when you want the column's + * declared plaintext type back (#779). + */ bulkDecrypt(payloads: BulkDecryptPayload): BulkDecryptOperation getEncryptConfig(): ReturnType } @@ -342,12 +380,16 @@ export function createEncryptionClient( } // Pass-through maps for the table-less one-arg decrypt call, where `table` is - // absent: decrypt WITHOUT date reconstruction, because with no table there is - // no `cast_as` to reconstruct from. This client is what `Encryption` returns - // for every v3 schema set, so generic consumers — and the legacy EQL v2 read - // path, whose table is not in `S` — can call `decryptModel(x)` / - // `bulkDecryptModels(xs)` with no table. Degrade gracefully instead of - // dereferencing `undefined.tableName`. + // absent: decrypt WITHOUT date reconstruction. The `table` argument is what + // selects a reconstructor, and the one-arg overload's `Decrypted` return + // type declares every decrypted field `string` to match — reconstructing + // under that type is the drift #779 rules out. (The payload's own `i: { t, c }` + // could resolve a `cast_as` for a registered table; the boundary is drawn at + // the arity so the runtime and the declared type never disagree.) + // This client is what `Encryption` returns for every v3 schema set, so + // generic consumers — and the legacy EQL v2 read path, whose table is not in + // `S` — can call `decryptModel(x)` / `bulkDecryptModels(xs)` with no table. + // Degrade gracefully instead of dereferencing `undefined.tableName`. const passthroughRow = (row: Record) => row const passthroughRows = (rows: Array>) => rows diff --git a/packages/stack/src/wasm-inline.ts b/packages/stack/src/wasm-inline.ts index c1ad98a1a..d5a49259d 100644 --- a/packages/stack/src/wasm-inline.ts +++ b/packages/stack/src/wasm-inline.ts @@ -819,6 +819,18 @@ export class WasmEncryptionClient { }, EncryptionErrorTypes.EncryptionError) } + /** + * Decrypt a single stored payload, resolving to the plaintext union unchanged. + * + * **A `date` / `timestamp` column comes back as the string it was stored as, + * not a `Date`** — where `decryptModel(row, table)` reconstructs it via + * {@link datePropertyPaths}. Same boundary the native client draws, and for + * the same reason: reconstruction is a property of the MODEL path, which is + * handed a table to read `cast_as` from. Not because the identity is missing + * — every payload carries `i: { t, c }` — but because this method's declared + * plaintext union excludes `Date`, and returning one would make the type a + * lie (#779). Read through the model helpers, or `new Date(value)` yourself. + */ async decrypt(encrypted: Encrypted): Promise> { return wasmResult( async () => @@ -1074,6 +1086,12 @@ export class WasmEncryptionClient { * worth considering if callers ask for it — but it is a different return * type from every other method here, so it is not the default.) * + * ## No date reconstruction + * + * Same boundary as {@link decrypt}: date-like columns arrive as their stored + * strings, not `Date` values. Prefer {@link bulkDecryptModels} with the table + * when you want the column's declared plaintext type back (#779). + * * @example * ```ts * const rows = await sql`SELECT email FROM users LIMIT 50` diff --git a/skills/stash-encryption/SKILL.md b/skills/stash-encryption/SKILL.md index 39b1392a3..1e7bdafe2 100644 --- a/skills/stash-encryption/SKILL.md +++ b/skills/stash-encryption/SKILL.md @@ -364,7 +364,9 @@ if (!decrypted.failure) { } ``` -`decrypt` of a single value cannot be strongly typed — a lone ciphertext carries no column identity. All plaintext values passed to `encrypt` must be non-null; null handling is managed at the model level by `encryptModel` and `decryptModel`. +`decrypt` of a single value cannot be strongly typed — TypeScript cannot know which column a runtime payload came from, so the result is the whole plaintext union. All plaintext values passed to `encrypt` must be non-null; null handling is managed at the model level by `encryptModel` and `decryptModel`. + +> **`decrypt` / `bulkDecrypt` hand back date columns as strings, not `Date`.** Reconstruction is driven by the table's `cast_as`, and only the model path is given a table — so the same stored value comes back as a `Date` from `decryptModel(row, table)` and as its stored ISO string from `decrypt(payload)`. Nothing warns: the raw path's declared type is the plaintext union, which includes `string`. Comparing two ISO strings orders correctly, so this survives review and breaks later on `.getTime()` or date arithmetic. Use the model helpers when you want the column's declared plaintext type, or rebuild at the call site (`new Date(value)`). Same on the WASM entry, and same for the one-arg `decryptModel(row)` / `bulkDecryptModels(rows)` forms, which take no table. ## Model Operations @@ -391,7 +393,7 @@ if (!enc.failure) { Typed-client model notes: - `decryptModel` / `bulkDecryptModels` take the **table as a second argument** and return a chainable `AuditableDecryptModelOperation` — await it for the `Result`, or chain `.audit({ metadata })` / `.withLockContext(lockContext)` first. A lock context may instead be passed as the optional third argument; use one form or the other, not both (chaining `.withLockContext()` onto a decrypt that already took a positional lock context throws). -- `Date` columns are reconstructed to real `Date` instances on decrypt; `bigint` columns round-trip as native `bigint`. A stored value that does **not** parse as a date (a legacy non-ISO string, say) is handed back unchanged rather than as an `Invalid Date`, even though the declared type is `Date` — guard with `instanceof Date` before calling `Date` methods on a column whose stored values you don't control. +- `Date` columns are reconstructed to real `Date` instances by `decryptModel(row, table)` / `bulkDecryptModels(rows, table)` — the table-taking forms, and only those (see the raw-path note above); `bigint` columns round-trip as native `bigint` on every path. A stored value that does **not** parse as a date (a legacy non-ISO string, say) is handed back unchanged rather than as an `Invalid Date`, even though the declared type is `Date` — guard with `instanceof Date` before calling `Date` methods on a column whose stored values you don't control. - Nullable schema fields stay nullable through the round trip. ## Bulk Operations @@ -414,7 +416,7 @@ const decrypted = await client.bulkDecryptModels(encrypted.data, users) ### Bulk Encrypt / Decrypt (Raw Values) -`bulkEncrypt` / `bulkDecrypt` are parity passthroughs (not v3-strengthened): +These two work on raw value arrays rather than models. `bulkEncrypt` is typed like `encrypt` — `{ table, column }` pins every `plaintext` to that column's domain — while `bulkDecrypt` takes the payloads alone, so it resolves to the plaintext union and does **no `Date` reconstruction**: a `types.Timestamp` column read this way is the stored ISO string, where `bulkDecryptModels(rows, table)` gives you a `Date`: ```typescript const plaintexts = [ @@ -986,15 +988,15 @@ Useful when the backfill needs to run in a worker, on a schedule, or alongside a | Method | Signature | Returns | |---|---|---| | `encrypt` | `(plaintext, { table, column })` — plaintext pinned to the column's domain type | `EncryptOperation` | -| `decrypt` | `(encryptedData)` — untyped (no column identity) | `DecryptOperation` | +| `decrypt` | `(encryptedData)` — untyped (the column is not known statically); no `Date` reconstruction | `DecryptOperation` | | `encryptQuery` | `(plaintext, { table, column, queryType?, returnType? })` — queryable columns only; `queryType` constrained to the column's capabilities | `EncryptQueryOperation` | | `encryptQuery` | `(terms: readonly ScalarQueryTerm[])` — batch form | `BatchEncryptQueryOperation` | | `encryptModel` | `(model, table)` — schema fields validated against inferred plaintext types | `EncryptModelOperation>` | | `decryptModel` | `(model, table, lockContext?)` | `AuditableDecryptModelOperation>` | | `bulkEncryptModels` | `(models, table)` | `BulkEncryptModelsOperation>` | | `bulkDecryptModels` | `(models, table, lockContext?)` | `AuditableDecryptModelOperation[]>` | -| `bulkEncrypt` | `(plaintexts, { column, table })` — parity passthrough | `BulkEncryptOperation` | -| `bulkDecrypt` | `(encryptedPayloads)` — parity passthrough | `BulkDecryptOperation` | +| `bulkEncrypt` | `(plaintexts, { column, table })` — raw values, each `plaintext` pinned to the column's domain type | `BulkEncryptOperation` | +| `bulkDecrypt` | `(encryptedPayloads)` — parity passthrough; no `Date` reconstruction | `BulkDecryptOperation` | | `getEncryptConfig` | `()` | The client's encrypt config | All of these operations are thenable (awaitable) and support `.withLockContext()` and `.audit()` chaining — including `decryptModel`/`bulkDecryptModels`, which also accept the lock context as a third argument. Use one or the other: chaining `.withLockContext()` onto a decrypt that already took a positional lock context throws.