From 7baf0bf5dbdc3cf652241bab8a5fdc0150810e35 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 4 Aug 2026 12:40:56 +1000 Subject: [PATCH 1/7] docs(plans): record the CIP-3366 eql validate rewrite plan The approved plan for moving `db validate` to `eql validate` and rebuilding its rule set around the EQL v3 domain-type vocabulary. --- docs/plans/cip-3366-eql-validate-v3.md | 196 +++++++++++++++++++++++++ 1 file changed, 196 insertions(+) create mode 100644 docs/plans/cip-3366-eql-validate-v3.md diff --git a/docs/plans/cip-3366-eql-validate-v3.md b/docs/plans/cip-3366-eql-validate-v3.md new file mode 100644 index 000000000..171b5f6c1 --- /dev/null +++ b/docs/plans/cip-3366-eql-validate-v3.md @@ -0,0 +1,196 @@ +# CIP-3366 — Rewrite `eql validate` for the EQL v3 domain-type vocabulary + +## Step 0 — Branch & worktree + +```bash +git worktree add .claude/worktrees/toby+cip-3366-eql-validate-v3 \ + -b toby/cip-3366-rewrite-eql-validate-for-the-eql-v3-domain-type-vocabulary +cd .claude/worktrees/toby+cip-3366-eql-validate-v3 +pnpm install +pnpm --filter @cipherstash/stack build # cli resolves stack through dist/ +``` + +## Two decisions that shape everything else + +### 1. `EncryptConfig` is lossy — validate must read domains, not indexes + +`EncryptedV3Column.build()` emits only `{ cast_as, indexes }`; the comment on +`getEqlType()` says outright "Metadata only; not emitted by `build()`". So +`cast_as: 'number'` + `{ ope: {} }` is ambiguous across `eql_v3_integer_ord`, +`smallint_ord`, `real_ord`, `double_ord` and `numeric_ord`. Every new rule in the +issue — `_ord_ore` steering, bool-not-searchable, text-only match, declared-vs-observed +drift — needs the domain name. + +The tables are usually imported into the client file rather than re-exported from +it (see the scaffold's `import { users, orders } from './db/schema'`), so +duck-typing the module namespace is unreliable. Expose them on the client instead. + +**`packages/stack/src/encryption/client-v3.ts`** + +- Add `getSchemas(): S` to the `EncryptionClient` interface. +- Add `getSchemas: () => schemas` to the `typed` object returned by + `createEncryptionClient` (`schemas` is already a parameter — ~2 lines). +- Test in `packages/stack/__tests__/`: `getSchemas()` returns the tuple and each + column's `getEqlType()` round-trips. +- Changeset: `@cipherstash/stack` minor. Update `skills/stash-encryption`. + +**`packages/cli/src/config/index.ts`** — add `loadEncryptSchemas(path)` beside +`loadEncryptConfig`, reusing the same jiti load and the same +`requireUsableEncryptConfig` placeholder guard, returning +`{ config, schemas }`. Fall back to config-only with a warning if the client +predates `getSchemas` (older `@cipherstash/stack` in a customer repo). + +### 2. Empty-string ordering is not statically checkable — scope it out + +The issue lists "Ordered domains reject empty strings (CHECK requires non-empty +`ob`)". That is a value-level CHECK enforced at encrypt time; nothing in the +schema or in `information_schema` predicts it. Leave it out and file a +follow-up to improve the *error message* on that path instead. Say so in the PR. + +## Step 1 — Validation core + +New: `packages/cli/src/commands/eql/validate.ts`. Keep the pure core separate +from the command shell so it is unit-testable (the current file has no tests at +all). + +```ts +interface DeclaredColumn { + table: string + column: string // DB name, from getName() + eqlType: string // 'public.eql_v3_integer_ord' + castAs: PlaintextKind + queryable: boolean + indexes: ColumnSchema['indexes'] +} + +collectDeclaredColumns(schemas): DeclaredColumn[] +validateSchemas(cols, observed?: ObservedState): ValidationIssue[] +``` + +Keep the existing `ValidationIssue` / `reportIssues` shape and the +error-exits-1 contract. + +### Static rules (no database) + +| Rule | Severity | Detection | +|---|---|---| +| `_ord_ore` domain declared | Warning | `eqlType.endsWith('_ord_ore')` — the ORE opclass is superuser-only; steer to the `_ord` (OPE) twin, which indexes on managed Postgres | +| Column is not queryable | Info | `!column.isQueryable()` — successor to the v2 "no indexes" Info, now correct for `types.IntegerOrd` (see the bug below) | +| Searchable `bool` | Error | `castAs === 'boolean' && queryable` | +| `match` index on a non-text domain | Error | `indexes.match && castAs !== 'string'` | +| `ste_vec` without a json domain | Error | keep from v2; retarget the message at `types.Json` | + +Retired: the operator-family warning, `NON_STRING_CAST_TYPES`, and the +`--exclude-operator-family` flag (`validateInstallFlags` already hard-rejects it +with "v3 self-adapts", and `v2-retirement.test.ts:23` asserts it is gone from +`eql install`/`upgrade` — validate is the last consumer). + +The bool and text-only rules are unconstructible through `types.*` today; they +are guards for hand-authored configs and for the drift rules below, not the +value of this step. Note that honestly in the PR. + +### Database rules (when a URL resolves) + +Skip with a `p.log.info` notice when no URL resolves — do not fail. Reuse +`fetchPhysicalColumns` from `packages/cli/src/commands/encrypt/lib/db-readers.ts` +(already returns `table → column → domain_name`). + +| Rule | Severity | +|---|---| +| Declared column absent from the database | Error | +| Observed `domain_name` ≠ declared `eqlType` (minus the `public.` prefix) | Error | +| Observed column has no domain (plain `jsonb`/`text`) | Error | +| `_ord_ore` declared while the ORE opclass is absent | Error (upgrades the static Warning) | +| Queryable column with no functional index over its extractor | Info | + +ORE availability probe — mirrors the shipped bundle's own fallback test +(`@cipherstash/eql@3.0.4` `dist/sql/cipherstash-encrypt.sql`, the +`ore_fallback.sql` `DO` block): + +```sql +SELECT EXISTS ( + SELECT 1 + FROM pg_catalog.pg_opclass c + JOIN pg_catalog.pg_am am ON am.oid = c.opcmethod + WHERE am.amname = 'btree' + AND c.opcdefault + AND c.opcintype = to_regtype('eql_v3_internal.ore_block_256') +) AS ore_available; +``` + +`to_regtype` returns NULL instead of throwing when EQL is not installed, so the +probe degrades to `false`. Detect not-installed separately and report that +first, or the user gets "ORE unavailable" when the real answer is "run +`stash eql install`". + +Functional-index rule: read `pg_index` + `pg_get_indexdef` and look for +`eql_v3.eq_term` / `eql_v3.ord_term` / `eql_v3.ord_term_ore` / +`eql_v3.match_term` over the column. This is the finding +`skills/stash-indexing` already promises to resolve, so it keeps that skill's +cross-reference honest. + +## Step 2 — Command wiring + +- `runEqlCommand` (`packages/cli/src/bin/main.ts`): add `case 'validate'`. +- `runDbCommand`: `case 'validate'` warns via `messages.db.aliasDeprecated(STASH, 'validate')` + and forwards — same shape as `install` / `upgrade` / `status`. (The issue + assumes this alias already exists; it does not.) +- `packages/cli/src/cli/registry.ts`: move the entry from the Database group to + the EQL group as `eql validate`; flags `SUPABASE_COMPAT_FLAG` + + `DATABASE_URL_FLAG` only. +- `main.ts` HELP text (line 117): `db validate` → `eql validate`. +- Delete `packages/cli/src/commands/db/validate.ts`. + +## Step 3 — Tests + +New `packages/cli/src/commands/eql/__tests__/validate.test.ts`, table-driven over +real `encryptedTable` / `types.*` schemas rather than hand-built configs, with a +fake observed-state map for the DB rules. + +Pin the regression this issue exists for: + +```ts +// v2 validate reported both of these as "encrypted but has no indexes — +// it will not be searchable": hasAnyIndex never learned about `ope`. +const t = encryptedTable('users', { + age: types.IntegerOrd('age'), + createdAt: types.TimestampOrd('created_at'), +}) +expect(validateSchemas(collectDeclaredColumns([t]))).toEqual([]) +``` + +Extend `packages/cli/src/__tests__/v2-retirement.test.ts`: `eql validate` carries +no `--exclude-operator-family`, and `db validate` is absent from the registry. + +## Step 4 — Docs, skills, changesets + +Skills ship in the `stash` tarball, so these are part of the change, not follow-up: + +- `skills/stash-cli/SKILL.md:430` — rewrite the rule table, move the section from + Database to EQL, drop the operator-family row. +- `skills/stash-indexing/SKILL.md:17,272` — rename the "No indexes on an + encrypted column" finding to whatever Step 1 emits. +- `skills/stash-postgres/SKILL.md:468` — cross-reference. +- `skills/stash-encryption` — the new `getSchemas()` accessor. +- `packages/cli/README.md:69,217,222`. +- Scaffold comments naming `stash db validate`: + `packages/cli/src/commands/init/utils.ts:388,437,455,500` **and** the checked-in + fixtures `packages/cli/__fixtures__/scaffold/{generic,drizzle}.generated.ts` — + `placeholder-client-fixture.test.ts` compares them, so both move together. +- Changesets: `@cipherstash/stack` minor (accessor), `stash` minor (command move, + new rules, flag removal). + +No wizard change needed: `ALLOWED_DLX_TOOLS` / `ALLOWED_BASH_COMMANDS` already +allow the `stash eql` prefix. + +## Verification + +```bash +pnpm run code:fix +pnpm --filter @cipherstash/stack build && pnpm --filter @cipherstash/stack test +pnpm --filter stash build && pnpm --filter stash test +node packages/cli/dist/bin/stash.js manifest --json # diff against skills/stash-cli +``` + +Manual against a live database: one `_ord_ore` column on a non-superuser role +(Error), one drifted domain, one queryable column with no functional index. From 93d62c2914c0c6b6ff62b1b34c40138b627c7db4 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 4 Aug 2026 12:41:07 +1000 Subject: [PATCH 2/7] feat(stack): expose EncryptionClient.getSchemas() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `getEncryptConfig()` returns the protect-ffi view — each column builds to { cast_as, indexes } and the concrete EQL v3 domain name is dropped. That makes cast_as 'number' with an `ope` index ambiguous across eql_v3_integer_ord, smallint_ord, real_ord, double_ord and numeric_ord, so tooling that must reason about the DECLARED domain could not recover it from a client alone. The tables are usually imported into the client file rather than re-exported from it, so duck-typing the module namespace is not a reliable substitute. `getSchemas()` hands back the tuple passed to Encryption({ schemas }), by reference, from which each column yields getEqlType() / getName() / getQueryCapabilities() / isQueryable(). `stash eql validate` is the first consumer. The exact-member-set gate in v3-only-public-surface.test.ts is updated deliberately: the accessor returns the same tuple the reconstructor map and the unknown-table guard were derived from, and can neither replace nor extend them, so it is not a re-initialization path. --- .changeset/olive-poems-guess.md | 29 +++++ .../__tests__/client-get-schemas.test.ts | 102 ++++++++++++++++++ .../__tests__/v3-only-public-surface.test.ts | 6 ++ packages/stack/src/encryption/client-v3.ts | 37 +++++++ skills/stash-encryption/SKILL.md | 16 ++- 5 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 .changeset/olive-poems-guess.md create mode 100644 packages/stack/__tests__/client-get-schemas.test.ts diff --git a/.changeset/olive-poems-guess.md b/.changeset/olive-poems-guess.md new file mode 100644 index 000000000..669e33754 --- /dev/null +++ b/.changeset/olive-poems-guess.md @@ -0,0 +1,29 @@ +--- +'@cipherstash/stack': minor +--- + +Add `EncryptionClient.getSchemas()` — the tables passed to +`Encryption({ schemas })`, returned by reference. + +This is the domain-bearing view of your schema. `getEncryptConfig()` returns +what the FFI consumes: each column builds to `{ cast_as, indexes }`, and the +concrete EQL v3 domain name is dropped. That makes `cast_as: 'number'` with an +`ope` index ambiguous across `eql_v3_integer_ord`, `smallint_ord`, `real_ord`, +`double_ord` and `numeric_ord` — so tooling that has to reason about the +*declared* domain (schema linting, drift-checking a live database's +`information_schema.columns.domain_name`) could not recover it from a client +alone. + +`getSchemas()` closes that gap. Read a column's domain with +`column.getEqlType()`, its capabilities with `column.getQueryCapabilities()`, +and its DB name with `column.getName()`: + +```typescript +for (const table of client.getSchemas()) { + for (const column of Object.values(table.columnBuilders)) { + console.log(table.tableName, column.getName(), column.getEqlType()) + } +} +``` + +`stash eql validate` is the first consumer. diff --git a/packages/stack/__tests__/client-get-schemas.test.ts b/packages/stack/__tests__/client-get-schemas.test.ts new file mode 100644 index 000000000..6122bae46 --- /dev/null +++ b/packages/stack/__tests__/client-get-schemas.test.ts @@ -0,0 +1,102 @@ +import { describe, expect, it } from 'vitest' +import { createEncryptionClient } from '@/encryption/client-v3' +import { encryptedTable, types } from '@/eql/v3' + +/** + * `getSchemas()` exists so that a tool holding only the client can recover the + * DECLARED domain of every column. `getEncryptConfig()` cannot answer that: + * `EncryptedV3Column.build()` emits `{ cast_as, indexes }` and drops the domain + * name, so `cast_as: 'number'` + `{ ope: {} }` is ambiguous across five numeric + * ordering domains. `stash eql validate` reads the domain to steer `_ord_ore` + * columns and to drift-check against `information_schema.columns.domain_name`. + * + * The stub below is the same shape `typed-client-v3.test.ts` uses — the wrapper + * takes the NATIVE client, and `getSchemas` is pure pass-through, so no FFI, + * credentials or network are involved. + */ +type NativeClientStub = Parameters[0] + +const users = encryptedTable('users', { + email: types.TextSearch('email'), + age: types.IntegerOrd('age'), + createdOn: types.Date('created_on'), +}) + +const orders = encryptedTable('orders', { + total: types.NumericOrdOre('total'), +}) + +const nativeStub = {} as unknown as NativeClientStub + +describe('EncryptionClient.getSchemas', () => { + it('returns the registered tables, in order, by reference', () => { + const client = createEncryptionClient(nativeStub, users, orders) + + expect(client.getSchemas()).toEqual([users, orders]) + // By reference, not a copy: consumers pass these straight back into + // `encryptModel(model, table)` / `decryptModel(row, table)`. + expect(client.getSchemas()[0]).toBe(users) + expect(client.getSchemas()[1]).toBe(orders) + }) + + it('round-trips each column to its concrete EQL v3 domain', () => { + const client = createEncryptionClient(nativeStub, users, orders) + + const domains = client.getSchemas().flatMap((table) => + Object.values(table.columnBuilders).map((column) => ({ + table: table.tableName, + column: column.getName(), + eqlType: column.getEqlType(), + queryable: column.isQueryable(), + })), + ) + + expect(domains).toEqual([ + { + table: 'users', + column: 'email', + eqlType: 'public.eql_v3_text_search', + queryable: true, + }, + { + table: 'users', + column: 'age', + eqlType: 'public.eql_v3_integer_ord', + queryable: true, + }, + { + table: 'users', + // The DB name (`getName()`), not the JS property `createdOn` — the + // whole point of reading through the builder rather than the key. + column: 'created_on', + eqlType: 'public.eql_v3_date', + queryable: false, + }, + { + table: 'orders', + column: 'total', + eqlType: 'public.eql_v3_numeric_ord_ore', + queryable: true, + }, + ]) + }) + + it('recovers a domain the encrypt config cannot distinguish', () => { + // `IntegerOrd` and `DoubleOrd` build to the SAME encrypt-config column — + // this is the ambiguity `getSchemas()` exists to resolve. + const ambiguous = encryptedTable('t', { + a: types.IntegerOrd('a'), + b: types.DoubleOrd('b'), + }) + const client = createEncryptionClient(nativeStub, ambiguous) + + const built = ambiguous.build().columns + expect(built.a).toEqual(built.b) + + const [first, second] = Object.values( + client.getSchemas()[0].columnBuilders, + ).map((column) => column.getEqlType()) + expect(first).toBe('public.eql_v3_integer_ord') + expect(second).toBe('public.eql_v3_double_ord') + }) +}) diff --git a/packages/stack/__tests__/v3-only-public-surface.test.ts b/packages/stack/__tests__/v3-only-public-surface.test.ts index 21e5cf37d..28ac2af0a 100644 --- a/packages/stack/__tests__/v3-only-public-surface.test.ts +++ b/packages/stack/__tests__/v3-only-public-surface.test.ts @@ -76,6 +76,12 @@ describe('v3-only public surface', () => { 'encryptModel', 'encryptQuery', 'getEncryptConfig', + // Read-only accessor over the SAME tuple the reconstructor map and the + // unknown-table guard were derived from — it hands the captured + // `schemas` back and can neither replace nor extend them, so it is not a + // re-initialization path. Added for `stash eql validate`, which needs the + // concrete domain names that `getEncryptConfig()` drops. + 'getSchemas', ]) }) }) diff --git a/packages/stack/src/encryption/client-v3.ts b/packages/stack/src/encryption/client-v3.ts index 2d1740857..74c8367c6 100644 --- a/packages/stack/src/encryption/client-v3.ts +++ b/packages/stack/src/encryption/client-v3.ts @@ -298,6 +298,38 @@ export interface EncryptionClient< */ bulkDecrypt(payloads: BulkDecryptPayload): BulkDecryptOperation getEncryptConfig(): ReturnType + + /** + * The v3 tables this client was initialized with, in the order they were + * passed to `Encryption({ schemas })`. + * + * This is the DOMAIN-BEARING view of the schema, and the reason it exists + * alongside {@link getEncryptConfig}. `getEncryptConfig()` is what the FFI + * consumes, and `EncryptedV3Column.build()` emits only `{ cast_as, indexes }` + * — the concrete domain name is metadata and is deliberately not in there. + * So `cast_as: 'number'` with an `ope` index is ambiguous across + * `eql_v3_integer_ord`, `smallint_ord`, `real_ord`, `double_ord` and + * `numeric_ord`, and any tool that has to reason about the DECLARED domain + * (schema linting, drift-checking a live database's `information_schema` + * domains) cannot recover it from the encrypt config. + * + * The tables are usually *imported* into the client file rather than + * re-exported from it, so duck-typing the module namespace is unreliable — + * hence exposing them here, on the one export every such tool already finds. + * + * Read a column's domain with `column.getEqlType()` + * (`'public.eql_v3_integer_ord'`), its capabilities with + * `column.getQueryCapabilities()`, and its DB name with `column.getName()`. + * + * ```typescript + * for (const table of client.getSchemas()) { + * for (const column of Object.values(table.columnBuilders)) { + * console.log(table.tableName, column.getName(), column.getEqlType()) + * } + * } + * ``` + */ + getSchemas(): S } /** @@ -527,6 +559,11 @@ export function createEncryptionClient( client.bulkEncrypt(plaintexts as BulkEncryptPayload, opts), bulkDecrypt: (payloads) => client.bulkDecrypt(payloads), getEncryptConfig: () => client.getEncryptConfig(), + // The same tuple, by reference — not a copy. `S` is a `const` type + // parameter, so the caller's array literal keeps its per-table inference, + // and `table` arguments read back off `getSchemas()` stay assignable to + // `S[number]` (identity-keyed consumers therefore also keep working). + getSchemas: () => schemas, } return typed diff --git a/skills/stash-encryption/SKILL.md b/skills/stash-encryption/SKILL.md index 2e841d472..fa006decd 100644 --- a/skills/stash-encryption/SKILL.md +++ b/skills/stash-encryption/SKILL.md @@ -1002,10 +1002,24 @@ Useful when the backfill needs to run in a worker, on a schedule, or alongside a | `bulkDecryptModels` | `(models, table, lockContext?)` | `AuditableDecryptModelOperation[]>` | | `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 | +| `getEncryptConfig` | `()` | The client's encrypt config (the protect-ffi view: `cast_as` + index kinds, **no domain names**) | +| `getSchemas` | `()` | The tables passed to `Encryption({ schemas })`, by reference | 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. +`getSchemas()` is the domain-bearing view of the schema, and the reason it exists alongside `getEncryptConfig()`. The encrypt config is what the FFI consumes: a column builds to `{ cast_as, indexes }`, and the concrete domain name is dropped — so `cast_as: 'number'` with an `ope` index is ambiguous across `eql_v3_integer_ord`, `smallint_ord`, `real_ord`, `double_ord` and `numeric_ord`. Anything reasoning about the *declared* domain reads the tables instead. `stash eql validate` is the built-in consumer; the same accessor lets your own tooling do it: + +```typescript +for (const table of client.getSchemas()) { + for (const column of Object.values(table.columnBuilders)) { + console.log(table.tableName, column.getName(), column.getEqlType()) + // users email public.eql_v3_text_search + } +} +``` + +Per column: `getName()` is the **DB** column name (not the JS property), `getEqlType()` the concrete domain, `getQueryCapabilities()` the `{ equality, orderAndRange, freeTextSearch, searchableJson? }` flags, and `isQueryable()` whether it carries any query term at all. + ### Schema Builders ```typescript From d4737452624d0735358ba5d081c43045162988f8 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 4 Aug 2026 12:41:23 +1000 Subject: [PATCH 3/7] feat(cli): rewrite `db validate` as `eql validate` for EQL v3 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v2 rule set checked for ore/unique/match/ste_vec indexes and never learned about `ope`. EQL v3's default ordering domains emit `ope`, so types.IntegerOrd('age') and types.TimestampOrd('created_at') were both reported as "Column is encrypted but has no indexes — it will not be searchable". Two of the most ordinary columns anyone writes, told they were unsearchable. They are now silent. The command reads the user's tables through the new EncryptionClient.getSchemas(), so every rule can key off the concrete domain rather than the lossy encrypt config. New `loadEncryptSchemas` sits beside `loadEncryptConfig` and shares its jiti load and placeholder refusal (both now go through one `loadEncryptionClient`); it degrades to config-only, with a warning, when the project's @cipherstash/stack predates getSchemas(). Schema rules: an `_ord_ore` domain (Warning — its ORE operator class needs superuser), storage-only columns (Info), and three guards for hand-authored configs (searchable boolean, match on a non-text domain, ste_vec without json — all Error). Database rules, when a connection resolves: EQL not installed (reported once, and the remaining database rules are skipped, so the user is not told "ORE unavailable" when the answer is `stash eql install`), missing tables/columns, domain drift against information_schema, a still-plain column, an `_ord_ore` domain where the opclass is genuinely absent (upgrading the static Warning), and queryable columns with no functional index over their term extractor. An unreachable database is a notice, not a failure. Every database fact enters through an injected ObservedState, so the drift rules are unit-tested without a database. `--exclude-operator-family` is removed: `eql install`/`eql upgrade` already rejected it because the pinned v3 bundle self-adapts, and validate was its last consumer. `stash db validate` keeps working as a deprecated alias, like db install / db upgrade / db status. Not implemented, deliberately: "ordered domains reject empty strings" is a value-level CHECK enforced at encrypt time and is not statically checkable. --- .changeset/proud-ravens-repeat.md | 47 ++ packages/cli/README.md | 33 +- .../scaffold/drizzle.generated.ts | 4 +- .../scaffold/generic.generated.ts | 4 +- .../cli/src/__tests__/v2-retirement.test.ts | 22 + packages/cli/src/bin/main.ts | 31 +- packages/cli/src/cli/registry.ts | 38 +- .../cli/src/commands/db/config-scaffold.ts | 4 +- packages/cli/src/commands/db/validate.ts | 192 ------ packages/cli/src/commands/encrypt/context.ts | 2 +- .../commands/eql/__tests__/validate.test.ts | 599 +++++++++++++++++ packages/cli/src/commands/eql/validate.ts | 613 ++++++++++++++++++ .../src/commands/init/steps/install-eql.ts | 2 +- packages/cli/src/commands/init/utils.ts | 8 +- packages/cli/src/config/index.ts | 99 ++- .../cli/tests/e2e/command-help.e2e.test.ts | 13 + packages/cli/tests/e2e/smoke.e2e.test.ts | 31 + skills/stash-cli/SKILL.md | 46 +- skills/stash-indexing/SKILL.md | 4 +- skills/stash-postgres/SKILL.md | 2 +- 20 files changed, 1544 insertions(+), 250 deletions(-) create mode 100644 .changeset/proud-ravens-repeat.md delete mode 100644 packages/cli/src/commands/db/validate.ts create mode 100644 packages/cli/src/commands/eql/__tests__/validate.test.ts create mode 100644 packages/cli/src/commands/eql/validate.ts diff --git a/.changeset/proud-ravens-repeat.md b/.changeset/proud-ravens-repeat.md new file mode 100644 index 000000000..c666d5748 --- /dev/null +++ b/.changeset/proud-ravens-repeat.md @@ -0,0 +1,47 @@ +--- +'stash': minor +--- + +Rewrite `db validate` as `eql validate`, for the EQL v3 domain-type vocabulary. + +**Fixes a false finding on the most ordinary v3 columns.** The old rule set +checked for `ore` / `unique` / `match` / `ste_vec` indexes and never learned +about `ope`. EQL v3's default ordering domains emit `ope`, so +`types.IntegerOrd('age')` and `types.TimestampOrd('created_at')` were both +reported as "Column is encrypted but has no indexes — it will not be +searchable". They are now silent. + +The command reads your tables through the new +`EncryptionClient.getSchemas()`, so it sees each column's **concrete domain** +rather than the lossy encrypt config, and gains a database pass when one is +reachable. + +Schema checks (no database needed): + +| Rule | Severity | +|---|---| +| An `_ord_ore` domain is declared — its ORE operator class needs superuser | Warning | +| Storage-only column: encrypts and decrypts, carries no query terms | Info | +| Searchable `boolean` column | Error | +| Free-text `match` index on a non-text domain | Error | +| Encrypted-JSONB (`ste_vec`) index without `types.Json` | Error | + +Database checks (skipped with a notice, not a failure, when no database is +reachable): + +| Rule | Severity | +|---|---| +| EQL v3 is not installed — reported once, remaining database checks skipped | Error | +| A declared table or column does not exist in the database | Error | +| The database column's domain differs from the declared one | Error | +| The database column is still plain (no EQL domain) | Error | +| An `_ord_ore` domain where the EQL install could not create the ORE operator class | Error | +| A queryable column with no functional index over its term extractor | Info | + +`--exclude-operator-family` is removed: it warned that an `ore` index would not +support `ORDER BY` without operator families, and the pinned EQL v3 bundle +self-adapts. `eql install` / `eql upgrade` had already rejected the flag; +`validate` was its last consumer. + +`stash db validate` keeps working as a deprecated alias, like `db install` / +`db upgrade` / `db status`. Exits 1 on errors only. diff --git a/packages/cli/README.md b/packages/cli/README.md index a34d07111..f1808ccd4 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -66,7 +66,7 @@ export default defineConfig({ The CLI loads `.env` files automatically before reading the config, so `process.env` references work without extra setup. The config file is resolved by walking up from the current working directory. -Commands that consume `stash.config.ts`: `eql install`, `eql upgrade`, `db validate`, `eql status`, `db test-connection`, `schema build`, and `encrypt *`. +Commands that consume `stash.config.ts`: `eql install`, `eql upgrade`, `eql validate`, `eql status`, `db test-connection`, `schema build`, and `encrypt *`. --- @@ -214,23 +214,40 @@ The install SQL is idempotent and safe to re-run. If EQL is not installed, the c --- -### `npx stash db validate` +### `npx stash eql validate` -Validate your encryption schema for common misconfigurations. +Validate your encryption schema against the EQL v3 domain vocabulary, and — when +a database is reachable — against what that database actually has. ```bash -npx stash db validate [--supabase] [--exclude-operator-family] +npx stash eql validate [--supabase] [--database-url ] ``` +Schema checks (no database needed): + +| Rule | Severity | +|------|----------| +| An `_ord_ore` domain, whose ORE operator class only a superuser can create | Warning | +| Storage-only column — encrypts and decrypts, carries no query terms | Info | +| Searchable `boolean` column | Error | +| Free-text `match` on a non-text domain | Error | +| Encrypted-JSONB search without `types.Json` | Error | + +Database checks (skipped with a notice when no database is reachable): + | Rule | Severity | |------|----------| -| `freeTextSearch` on a non-string column | Warning | -| `orderAndRange` without operator families | Warning | -| No indexes on an encrypted column | Info | -| `searchableJson` without `dataType("json")` | Error | +| EQL v3 is not installed (reported once; the other database checks are skipped) | Error | +| Declared column or table absent from the database | Error | +| The database column's domain has drifted from the declaration | Error | +| The column is still plain (no EQL domain) | Error | +| An `_ord_ore` domain on a database whose EQL install could not create the ORE operator class | Error | +| Queryable column with no functional index over its term extractor | Info | The command exits with code 1 on errors (not on warnings or info). +`stash db validate` still works as a deprecated alias. + --- ### `npx stash db migrate` diff --git a/packages/cli/__fixtures__/scaffold/drizzle.generated.ts b/packages/cli/__fixtures__/scaffold/drizzle.generated.ts index 51571571b..c423ccb83 100644 --- a/packages/cli/__fixtures__/scaffold/drizzle.generated.ts +++ b/packages/cli/__fixtures__/scaffold/drizzle.generated.ts @@ -8,7 +8,7 @@ * to reference the encrypted tables you declared there. * * Until that happens, the encryption client is initialised with a single - * placeholder table so that this file compiles, and `stash db validate` and + * placeholder table so that this file compiles, and `stash eql validate` and * `stash encrypt backfill` refuse to run and point back here. (`stash * encrypt drop` resolves against the database and never reads this file.) * @@ -57,7 +57,7 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' // REPLACE THIS. It exists only so this file compiles before you have declared // any encrypted tables — `Encryption` requires at least one. Swap it for your -// real tables (see the patterns above); `stash db validate` and `stash +// real tables (see the patterns above); `stash eql validate` and `stash // encrypt backfill` refuse to run while the placeholder is still here. export const placeholderTable = encryptedTable('__stash_placeholder__', { replace_me: types.Text('replace_me'), diff --git a/packages/cli/__fixtures__/scaffold/generic.generated.ts b/packages/cli/__fixtures__/scaffold/generic.generated.ts index 97b7c4c22..80981180c 100644 --- a/packages/cli/__fixtures__/scaffold/generic.generated.ts +++ b/packages/cli/__fixtures__/scaffold/generic.generated.ts @@ -7,7 +7,7 @@ * `Encryption({ schemas: [...] })` call below to reference them. * * Until that happens, the encryption client is initialised with a single - * placeholder table so that this file compiles, and `stash db validate` and + * placeholder table so that this file compiles, and `stash eql validate` and * `stash encrypt backfill` refuse to run and point back here. (`stash * encrypt drop` resolves against the database and never reads this file.) * @@ -52,7 +52,7 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' // REPLACE THIS. It exists only so this file compiles before you have declared // any encrypted tables — `Encryption` requires at least one. Swap it for your -// real tables (see the patterns above); `stash db validate` and `stash +// real tables (see the patterns above); `stash eql validate` and `stash // encrypt backfill` refuse to run while the placeholder is still here. export const placeholderTable = encryptedTable('__stash_placeholder__', { replace_me: types.Text('replace_me'), diff --git a/packages/cli/src/__tests__/v2-retirement.test.ts b/packages/cli/src/__tests__/v2-retirement.test.ts index 32140668b..69369e063 100644 --- a/packages/cli/src/__tests__/v2-retirement.test.ts +++ b/packages/cli/src/__tests__/v2-retirement.test.ts @@ -24,6 +24,28 @@ describe('EQL v2 CLI retirement', () => { } }) + it('leaves no consumer of the operator-family flag anywhere in the manifest', () => { + // `db validate` was the last one: its v2 rule warned that an `ore` index + // would not support ORDER BY without operator families. EQL v3's install + // self-adapts, and `eql validate` reasons about the ORE domain instead — + // so the flag has no remaining meaning on any command. + for (const command of commands) { + const flags = command.flags?.map((flag) => flag.name) ?? [] + expect(flags).not.toContain('--exclude-operator-family') + } + }) + + it('moves validate into the EQL group, leaving `db validate` a hidden alias', () => { + const names = commands.map((command) => command.name) + + expect(names).toContain('eql validate') + // The `db` spelling still dispatches (with a deprecation warning), exactly + // like `db install` / `db upgrade` / `db status` — but it is deliberately + // absent from the registry, so help and `stash manifest --json` advertise + // one name. + expect(names).not.toContain('db validate') + }) + it('removes the Proxy choice from init', () => { const init = commands.find((command) => command.name === 'init') const flags = init?.flags?.map((flag) => flag.name) ?? [] diff --git a/packages/cli/src/bin/main.ts b/packages/cli/src/bin/main.ts index 16f26f639..26e460cf5 100644 --- a/packages/cli/src/bin/main.ts +++ b/packages/cli/src/bin/main.ts @@ -113,8 +113,8 @@ Commands: eql repair Repair migrations with an un-runnable ALTER COLUMN to an encrypted type eql upgrade Upgrade EQL extensions to the latest version eql status Show EQL installation status + eql validate Validate your encryption schema against EQL v3 - db validate Validate encryption schema db migrate Run pending encrypt config migrations db test-connection Test database connectivity @@ -219,6 +219,19 @@ async function runUpgrade( }) } +async function runValidate( + flags: Record, + values: Record, +) { + const { validateCommand } = await requireStack( + () => import('../commands/eql/validate.js'), + ) + await validateCommand({ + supabase: flags.supabase, + databaseUrl: values['database-url'], + }) +} + function rejectRetiredEqlFlags( flags: Record, values: Record, @@ -282,6 +295,9 @@ async function runEqlCommand( case 'status': await dbStatusCommand({ databaseUrl: values['database-url'] }) break + case 'validate': + await runValidate(flags, values) + break default: p.log.error(`${messages.eql.unknownSubcommand}: ${sub ?? '(none)'}`) console.log() @@ -318,17 +334,10 @@ async function runDbCommand( ) throw new CliExit(1) } - case 'validate': { - const { validateCommand } = await requireStack( - () => import('../commands/db/validate.js'), - ) - await validateCommand({ - supabase: flags.supabase, - excludeOperatorFamily: flags['exclude-operator-family'], - databaseUrl, - }) + case 'validate': + p.log.warn(messages.db.aliasDeprecated(STASH, 'validate')) + await runValidate(flags, values) break - } case 'status': p.log.warn(messages.db.aliasDeprecated(STASH, 'status')) await dbStatusCommand({ databaseUrl }) diff --git a/packages/cli/src/cli/registry.ts b/packages/cli/src/cli/registry.ts index 4496cda3e..cb482340c 100644 --- a/packages/cli/src/cli/registry.ts +++ b/packages/cli/src/cli/registry.ts @@ -73,10 +73,6 @@ const DRY_RUN_FLAG: Flag = { name: '--dry-run', description: 'Show what would happen without making changes.', } -const EXCLUDE_OPERATOR_FAMILY_FLAG: Flag = { - name: '--exclude-operator-family', - description: 'Skip operator family creation.', -} const SUPABASE_COMPAT_FLAG: Flag = { name: '--supabase', description: 'Use Supabase-compatible mode.', @@ -419,20 +415,36 @@ export const registry: CommandGroup[] = [ summary: 'Show EQL installation status', flags: [DATABASE_URL_FLAG], }, + { + name: 'eql validate', + summary: 'Validate your encryption schema against EQL v3', + long: [ + 'Read the tables passed to `Encryption({ schemas })` and check each encrypted', + 'column against the EQL v3 domain vocabulary — then, if a database is', + 'reachable, against what that database actually has.', + '', + 'Schema checks (no database needed): an `_ord_ore` domain, whose ORE operator', + 'class only a superuser can create; storage-only columns, reported so an', + 'unsearchable column is a decision rather than a surprise; and hand-authored', + 'configs that ask for free-text match on a non-text domain, encrypted-JSONB', + 'search without `types.Json`, or a searchable boolean.', + '', + 'Database checks (skipped with a notice when no database is reachable):', + 'declared columns missing from the database, a column whose domain has drifted', + 'from the schema, an `_ord_ore` column on a database whose EQL install could', + 'not create the ORE operator class, and queryable columns with no functional', + 'index over their term extractor.', + '', + 'Exits 1 on errors only — warnings and info do not fail the command.', + ].join('\n'), + examples: ['eql validate', 'eql validate --database-url postgres://…'], + flags: [SUPABASE_COMPAT_FLAG, DATABASE_URL_FLAG], + }, ], }, { title: 'Database', commands: [ - { - name: 'db validate', - summary: 'Validate encryption schema', - flags: [ - SUPABASE_COMPAT_FLAG, - EXCLUDE_OPERATOR_FAMILY_FLAG, - DATABASE_URL_FLAG, - ], - }, { // Dispatch currently only prints a "not yet implemented" warning and // reads no flags — describe that rather than advertising a working diff --git a/packages/cli/src/commands/db/config-scaffold.ts b/packages/cli/src/commands/db/config-scaffold.ts index b8b09f6d1..c8ee68340 100644 --- a/packages/cli/src/commands/db/config-scaffold.ts +++ b/packages/cli/src/commands/db/config-scaffold.ts @@ -96,7 +96,7 @@ function writeStashConfig(configPath: string, clientPath: string): string { } /** - * Create a `stash.config.ts` for the rest of the workflow (`db validate` and + * Create a `stash.config.ts` for the rest of the workflow (`eql validate` and * `encrypt *` load the encryption client through it). * `eql install` itself doesn't need one — it resolves the database URL * directly — so this is a setup convenience, never a blocker. @@ -136,7 +136,7 @@ export async function offerStashConfig( if (!isInteractive()) return null const create = await p.confirm({ - message: `Create a ${CONFIG_FILENAME}? (needed later for db validate / encrypt)`, + message: `Create a ${CONFIG_FILENAME}? (needed later for eql validate / encrypt)`, initialValue: true, }) if (p.isCancel(create) || !create) { diff --git a/packages/cli/src/commands/db/validate.ts b/packages/cli/src/commands/db/validate.ts deleted file mode 100644 index f4fc75c41..000000000 --- a/packages/cli/src/commands/db/validate.ts +++ /dev/null @@ -1,192 +0,0 @@ -import type { EncryptConfig } from '@cipherstash/stack/schema' -import * as p from '@clack/prompts' -import { detectPackageManager, runnerCommand } from '@/commands/init/utils.js' -import { loadEncryptConfig, loadStashConfig } from '@/config/index.js' - -type Severity = 'error' | 'warning' | 'info' - -interface ValidationIssue { - severity: Severity - table: string - column: string - message: string -} - -/** Cast-as types that are not string-like — free-text search is meaningless for these. */ -const NON_STRING_CAST_TYPES = new Set([ - 'int', - 'small_int', - 'big_int', - 'real', - 'double', - 'boolean', - 'date', - 'number', - 'bigint', -]) - -/** - * Validate an EncryptConfig against common misconfiguration rules. - * - * This is a pure function so it can be tested and reused (e.g. in `push`). - */ -export function validateEncryptConfig( - config: EncryptConfig, - options: { supabase?: boolean; excludeOperatorFamily?: boolean }, -): ValidationIssue[] { - const issues: ValidationIssue[] = [] - - for (const [tableName, columns] of Object.entries(config.tables)) { - for (const [columnName, column] of Object.entries(columns)) { - const { cast_as, indexes } = column - - // Rule 1: freeTextSearch (match index) on a non-string column - if (indexes.match && NON_STRING_CAST_TYPES.has(cast_as)) { - issues.push({ - severity: 'warning', - table: tableName, - column: columnName, - message: `freeTextSearch on a "${cast_as}" column has no effect — free-text search only works with string data`, - }) - } - - // Rule 2: orderAndRange (ore index) without operator families - if (indexes.ore && (options.supabase || options.excludeOperatorFamily)) { - issues.push({ - severity: 'warning', - table: tableName, - column: columnName, - message: - 'orderAndRange index will not support ORDER BY without operator families (Supabase limitation)', - }) - } - - // Rule 3: No indexes defined — column is encrypted but not searchable - const hasAnyIndex = - indexes.ore !== undefined || - indexes.unique !== undefined || - indexes.match !== undefined || - indexes.ste_vec !== undefined - if (!hasAnyIndex) { - issues.push({ - severity: 'info', - table: tableName, - column: columnName, - message: - 'Column is encrypted but has no indexes — it will not be searchable', - }) - } - - // Rule 4: ste_vec index without json data type - if (indexes.ste_vec && cast_as !== 'json') { - issues.push({ - severity: 'error', - table: tableName, - column: columnName, - message: `searchableJson requires dataType("json") but found "${cast_as}"`, - }) - } - } - } - - return issues -} - -function countTables(config: EncryptConfig): number { - return Object.keys(config.tables).length -} - -function countColumns(config: EncryptConfig): number { - let count = 0 - for (const columns of Object.values(config.tables)) { - count += Object.keys(columns).length - } - return count -} - -/** - * Print validation issues using `@clack/prompts` log methods. - * - * @returns `true` if there are any errors (severity === 'error'). - */ -export function reportIssues(issues: ValidationIssue[]): boolean { - for (const issue of issues) { - const line = `${issue.table}.${issue.column}: ${issue.message}` - - switch (issue.severity) { - case 'error': - p.log.error(line) - break - case 'warning': - p.log.warn(line) - break - case 'info': - p.log.info(line) - break - } - } - - const errors = issues.filter((i) => i.severity === 'error').length - const warnings = issues.filter((i) => i.severity === 'warning').length - const infos = issues.filter((i) => i.severity === 'info').length - - if (errors > 0) { - p.outro( - `${errors} error${errors !== 1 ? 's' : ''}, ${warnings} warning${warnings !== 1 ? 's' : ''}.`, - ) - } else if (warnings > 0) { - p.outro(`No errors found. ${warnings} warning${warnings !== 1 ? 's' : ''}.`) - } else if (infos > 0) { - p.outro(`No errors or warnings. ${infos} info${infos !== 1 ? 's' : ''}.`) - } else { - p.outro('No issues found.') - } - - return errors > 0 -} - -export async function validateCommand(options: { - supabase?: boolean - excludeOperatorFamily?: boolean - databaseUrl?: string -}) { - p.intro(runnerCommand(detectPackageManager(), 'stash db validate')) - - const s = p.spinner() - - s.start('Loading stash.config.ts...') - const config = await loadStashConfig({ - databaseUrlFlag: options.databaseUrl, - supabase: options.supabase, - }) - s.stop('Configuration loaded.') - - s.start(`Loading encrypt client from ${config.client}...`) - const encryptConfig = await loadEncryptConfig(config.client) - s.stop('Encrypt client loaded.') - - if (!encryptConfig) { - p.log.error('No encryption config found.') - process.exit(1) - } - - const tableCount = countTables(encryptConfig) - const columnCount = countColumns(encryptConfig) - p.log.success( - `Schema loaded: ${tableCount} table${tableCount !== 1 ? 's' : ''}, ${columnCount} encrypted column${columnCount !== 1 ? 's' : ''}`, - ) - - const issues = validateEncryptConfig(encryptConfig, options) - - if (issues.length === 0) { - p.outro('No issues found.') - return - } - - console.log() // blank line before issues - const hasErrors = reportIssues(issues) - - if (hasErrors) { - process.exit(1) - } -} diff --git a/packages/cli/src/commands/encrypt/context.ts b/packages/cli/src/commands/encrypt/context.ts index af22df0f2..a45acf01b 100644 --- a/packages/cli/src/commands/encrypt/context.ts +++ b/packages/cli/src/commands/encrypt/context.ts @@ -150,7 +150,7 @@ export async function loadEncryptionContext(): Promise { process.exit(1) } - // The same refusal `stash db validate` gets from + // The same refusal `stash eql validate` gets from // `loadEncryptConfig`, applied here because `stash encrypt` does not go // through that loader. Called, not re-implemented: it guards one file, so the // two commands must say one thing about it. Without this, `requireTable` diff --git a/packages/cli/src/commands/eql/__tests__/validate.test.ts b/packages/cli/src/commands/eql/__tests__/validate.test.ts new file mode 100644 index 000000000..c74cd3b82 --- /dev/null +++ b/packages/cli/src/commands/eql/__tests__/validate.test.ts @@ -0,0 +1,599 @@ +import { encryptedTable, types } from '@cipherstash/stack/eql/v3' +import type { EncryptConfig } from '@cipherstash/stack/schema' +import { describe, expect, it } from 'vitest' +import { + collectDeclaredColumns, + collectDeclaredColumnsFromConfig, + expectedExtractors, + type ObservedState, + parseIndexedExtractors, + validateSchemas, +} from '../validate.js' + +/** + * Built from real `encryptedTable` / `types.*` schemas, not hand-written + * encrypt configs. The rules key off the concrete domain, which only the real + * factories produce — a hand-built fixture would let a factory change drift + * away from what validate is told to expect. + * + * The database rules take their facts through an injected {@link ObservedState} + * rather than a live connection, so drift, ORE availability and missing + * functional indexes all have coverage with no database. + */ + +/** An `ObservedState` in which everything the schema declares is present and correct. */ +function observing( + columns: Record>, + overrides: Partial = {}, +): ObservedState { + return { + eqlInstalled: true, + oreAvailable: true, + columns: new Map( + Object.entries(columns).map(([table, cols]) => [ + table, + new Map(Object.entries(cols)), + ]), + ), + indexedExtractors: new Map(), + ...overrides, + } +} + +/** Every functional index a set of declared columns could want. */ +function fullyIndexed( + columns: ReturnType, +): Map> { + return new Map( + columns.map((column) => [ + `${column.table}.${column.column}`, + new Set(expectedExtractors(column.indexes)), + ]), + ) +} + +describe('the regression this command was rewritten for', () => { + it('reports nothing for the default ordering domains', () => { + // EQL v2's `hasAnyIndex` checked `ore` / `unique` / `match` / `ste_vec` and + // never learned about `ope`. EQL v3's `_ord` domains emit `ope`, so both of + // these were reported as "Column is encrypted but has no indexes — it will + // not be searchable" — for two of the most ordinary columns anyone writes. + const users = encryptedTable('users', { + age: types.IntegerOrd('age'), + createdAt: types.TimestampOrd('created_at'), + }) + + expect(validateSchemas(collectDeclaredColumns([users]))).toEqual([]) + }) + + it('still reports a genuinely storage-only column', () => { + const users = encryptedTable('users', { + notes: types.Text('notes'), + }) + + expect(validateSchemas(collectDeclaredColumns([users]))).toEqual([ + { + severity: 'info', + table: 'users', + column: 'notes', + message: expect.stringContaining('Storage-only column'), + }, + ]) + }) +}) + +describe('collectDeclaredColumns', () => { + it('flattens the tuple, keyed by DB column name and carrying the domain', () => { + const users = encryptedTable('users', { + email: types.TextSearch('email'), + // camelCase property, snake_case column: the DB name is what the + // database reports back, so it is what must be collected. + createdAt: types.TimestampOrd('created_at'), + }) + const orders = encryptedTable('orders', { + total: types.NumericOrd('total'), + }) + + expect(collectDeclaredColumns([users, orders])).toEqual([ + { + table: 'users', + column: 'email', + eqlType: 'public.eql_v3_text_search', + cast_as: 'string', + queryable: true, + indexes: expect.objectContaining({ unique: expect.anything() }), + }, + { + table: 'users', + column: 'created_at', + eqlType: 'public.eql_v3_timestamp_ord', + cast_as: 'timestamp', + queryable: true, + indexes: { ope: {} }, + }, + { + table: 'orders', + column: 'total', + eqlType: 'public.eql_v3_numeric_ord', + cast_as: 'number', + queryable: true, + indexes: { ope: {} }, + }, + ]) + }) +}) + +describe('schema rules (no database)', () => { + const cases: Array<{ + name: string + columns: ReturnType + expected: Array<{ severity: string; column?: string; match: RegExp }> + }> = [ + { + name: 'a fully searchable text column is clean', + columns: collectDeclaredColumns([ + encryptedTable('users', { email: types.TextSearch('email') }), + ]), + expected: [], + }, + { + name: 'equality, match and json domains are clean', + columns: collectDeclaredColumns([ + encryptedTable('users', { + email: types.TextEq('email'), + bio: types.TextMatch('bio'), + profile: types.Json('profile'), + }), + ]), + expected: [], + }, + { + name: 'an _ord_ore domain warns about the superuser-only operator class', + columns: collectDeclaredColumns([ + encryptedTable('users', { age: types.IntegerOrdOre('age') }), + ]), + expected: [ + { + severity: 'warning', + column: 'age', + match: /eql_v3_integer_ord_ore needs the ORE btree operator class/, + }, + ], + }, + { + name: 'the _ord_ore warning names the OPE twin to switch to', + columns: collectDeclaredColumns([ + encryptedTable('t', { at: types.TimestampOrdOre('at') }), + ]), + expected: [ + { + severity: 'warning', + column: 'at', + match: /Use eql_v3_timestamp_ord unless/, + }, + ], + }, + { + name: 'storage-only domains are reported as Info, one per column', + columns: collectDeclaredColumns([ + encryptedTable('users', { + notes: types.Text('notes'), + verified: types.Boolean('verified'), + }), + ]), + expected: [ + { severity: 'info', column: 'notes', match: /Storage-only column/ }, + { severity: 'info', column: 'verified', match: /Storage-only column/ }, + ], + }, + ] + + it.each(cases)('$name', ({ columns, expected }) => { + const issues = validateSchemas(columns) + + expect(issues).toHaveLength(expected.length) + for (const [i, want] of expected.entries()) { + expect(issues[i].severity).toBe(want.severity) + if (want.column) expect(issues[i].column).toBe(want.column) + expect(issues[i].message).toMatch(want.match) + } + }) + + it('reports every domain in the catalog without crashing', () => { + // A blunt sweep: whatever the rules decide, no factory may throw and every + // issue must name a real column. Guards against a rule that dereferences a + // field only some domains carry. + const table = encryptedTable('everything', { + a: types.Integer('a'), + b: types.IntegerEq('b'), + c: types.IntegerOrd('c'), + d: types.IntegerOrdOre('d'), + e: types.Text('e'), + f: types.TextEq('f'), + g: types.TextMatch('g'), + h: types.TextOrd('h'), + i: types.TextOrdOre('i'), + j: types.TextSearch('j'), + k: types.Boolean('k'), + l: types.Json('l'), + m: types.BigintOrd('m'), + n: types.DateOrd('n'), + o: types.RealOrd('o'), + p: types.DoubleOrd('p'), + q: types.SmallintOrd('q'), + r: types.NumericOrd('r'), + }) + const columns = collectDeclaredColumns([table]) + const issues = validateSchemas(columns) + + const declared = new Set(columns.map((column) => column.column)) + for (const issue of issues) { + expect(declared.has(issue.column ?? '')).toBe(true) + } + // No error is constructible through `types.*` — every finding here is a + // Warning (the two `_ord_ore` domains) or an Info (the three storage-only + // ones). + expect(issues.filter((issue) => issue.severity === 'error')).toEqual([]) + expect(issues.filter((issue) => issue.severity === 'warning')).toHaveLength( + 2, + ) + expect(issues.filter((issue) => issue.severity === 'info')).toHaveLength(3) + }) +}) + +describe('schema rules that guard hand-authored configs', () => { + /** + * Neither of these is constructible through `types.*` — `types.Boolean` is + * storage-only by construction and `match` is emitted only by text domains. + * They exist for an encrypt config written by hand or emitted by an older + * generator, so they are exercised through the config-only collection path, + * which is the one such a config actually arrives on. + */ + const configWith = ( + columns: EncryptConfig['tables'][string], + ): EncryptConfig => ({ v: 1, tables: { t: columns } }) as EncryptConfig + + it('rejects a searchable boolean', () => { + const columns = collectDeclaredColumnsFromConfig( + configWith({ flag: { cast_as: 'boolean', indexes: { unique: {} } } }), + ) + + expect(validateSchemas(columns)).toEqual([ + { + severity: 'error', + table: 't', + column: 'flag', + message: expect.stringContaining('searchable boolean column leaks'), + }, + ]) + }) + + it('rejects a match index on a non-text domain', () => { + const columns = collectDeclaredColumnsFromConfig( + configWith({ n: { cast_as: 'number', indexes: { match: {} } } }), + ) + + expect(validateSchemas(columns)).toEqual([ + { + severity: 'error', + table: 't', + column: 'n', + message: expect.stringContaining('Free-text match needs a text domain'), + }, + ]) + }) + + it('rejects ste_vec without a json cast', () => { + const columns = collectDeclaredColumnsFromConfig( + configWith({ doc: { cast_as: 'string', indexes: { ste_vec: {} } } }), + ) + + expect(validateSchemas(columns)).toEqual([ + { + severity: 'error', + table: 't', + column: 'doc', + message: expect.stringContaining('Encrypted-JSONB search needs'), + }, + ]) + }) + + it('cannot report domain rules without a domain', () => { + // The degraded path drops `eqlType`, so the ORE steer must not fire on a + // config-only load — it would have nothing to base the claim on. + const columns = collectDeclaredColumnsFromConfig( + configWith({ age: { cast_as: 'number', indexes: { ore: {} } } }), + ) + + expect(validateSchemas(columns)).toEqual([]) + }) +}) + +describe('database rules', () => { + const users = encryptedTable('users', { + email: types.TextSearch('email'), + age: types.IntegerOrd('age'), + }) + const columns = collectDeclaredColumns([users]) + const healthy = { + users: { + email: 'eql_v3_text_search', + age: 'eql_v3_integer_ord', + }, + } + + it('is silent when the database matches the schema and is fully indexed', () => { + const observed = observing(healthy, { + indexedExtractors: fullyIndexed(columns), + }) + + expect(validateSchemas(columns, observed)).toEqual([]) + }) + + it('reports a missing table once, and does not then look for its columns', () => { + const observed = observing({}, { indexedExtractors: fullyIndexed(columns) }) + const issues = validateSchemas(columns, observed) + + expect(issues).toHaveLength(2) + for (const issue of issues) { + expect(issue.severity).toBe('error') + expect(issue.message).toMatch(/Table "users" .* does not exist/) + } + }) + + it('reports a declared column the table does not have', () => { + const observed = observing( + { users: { email: 'eql_v3_text_search' } }, + { indexedExtractors: fullyIndexed(columns) }, + ) + + expect(validateSchemas(columns, observed)).toEqual([ + { + severity: 'error', + table: 'users', + column: 'age', + message: expect.stringContaining( + 'does not exist on table "users". Add it in a migration with the `public.eql_v3_integer_ord` type', + ), + }, + ]) + }) + + it('reports a column that is still plain', () => { + const observed = observing( + { users: { email: 'eql_v3_text_search', age: null } }, + { indexedExtractors: fullyIndexed(columns) }, + ) + + expect(validateSchemas(columns, observed)).toEqual([ + { + severity: 'error', + table: 'users', + column: 'age', + message: expect.stringContaining('is a plain (non-EQL) column'), + }, + ]) + }) + + it('reports a domain that has drifted from the declaration', () => { + const observed = observing( + { + users: { + email: 'eql_v3_text_search', + // A migration wrote the ORE twin; the schema says OPE. + age: 'eql_v3_integer_ord_ore', + }, + }, + { indexedExtractors: fullyIndexed(columns) }, + ) + + expect(validateSchemas(columns, observed)).toEqual([ + { + severity: 'error', + table: 'users', + column: 'age', + message: expect.stringContaining( + 'Declared `public.eql_v3_integer_ord` but the database column is `eql_v3_integer_ord_ore`', + ), + }, + ]) + }) + + it('upgrades the _ord_ore warning to an error when the operator class is absent', () => { + const ore = encryptedTable('t', { age: types.IntegerOrdOre('age') }) + const oreColumns = collectDeclaredColumns([ore]) + const observed = observing( + { t: { age: 'eql_v3_integer_ord_ore' } }, + { + oreAvailable: false, + indexedExtractors: fullyIndexed(oreColumns), + }, + ) + + const issues = validateSchemas(oreColumns, observed) + + expect(issues).toEqual([ + { + severity: 'error', + table: 't', + column: 'age', + message: expect.stringContaining('is unusable in this database'), + }, + ]) + // Not both: the static Warning is superseded, not stacked on top of. + expect(issues.filter((issue) => issue.severity === 'warning')).toEqual([]) + }) + + it('says nothing about ORE when the operator class is present', () => { + const ore = encryptedTable('t', { age: types.IntegerOrdOre('age') }) + const oreColumns = collectDeclaredColumns([ore]) + const observed = observing( + { t: { age: 'eql_v3_integer_ord_ore' } }, + { indexedExtractors: fullyIndexed(oreColumns) }, + ) + + expect(validateSchemas(oreColumns, observed)).toEqual([]) + }) + + it('reports each missing functional index with a runnable CREATE INDEX', () => { + const observed = observing(healthy, { + indexedExtractors: new Map([['users.email', new Set(['eq_term'])]]), + }) + + const issues = validateSchemas(columns, observed) + + expect(issues).toHaveLength(2) + expect(issues[0]).toMatchObject({ + severity: 'info', + table: 'users', + column: 'email', + }) + // `eq_term` is indexed; `ord_term` and `match_term` are not. + expect(issues[0].message).toContain( + '`eql_v3.ord_term` / `eql_v3.match_term`', + ) + expect(issues[0].message).not.toContain('eql_v3.eq_term') + expect(issues[1].message).toContain( + 'CREATE INDEX ON "users" (eql_v3.ord_term("age"));', + ) + }) + + it('asks for no index on a storage-only column', () => { + const table = encryptedTable('t', { notes: types.Text('notes') }) + const storageOnly = collectDeclaredColumns([table]) + const observed = observing({ t: { notes: 'eql_v3_text' } }) + + const issues = validateSchemas(storageOnly, observed) + + expect(issues).toHaveLength(1) + expect(issues[0].message).toMatch(/Storage-only column/) + }) + + it('asks for no scalar index on an encrypted-JSONB column', () => { + // `types.Json` is served by a GIN index over the column, not by a scalar + // extractor, so the missing-extractor finding must not fire on it. + const table = encryptedTable('t', { profile: types.Json('profile') }) + const json = collectDeclaredColumns([table]) + const observed = observing({ t: { profile: 'eql_v3_json_search' } }) + + expect(validateSchemas(json, observed)).toEqual([]) + }) + + it('reports a missing EQL install once and drops the per-column checks', () => { + const observed = observing({}, { eqlInstalled: false, oreAvailable: false }) + const issues = validateSchemas(columns, observed) + + expect(issues).toEqual([ + { + severity: 'error', + message: expect.stringContaining('EQL v3 is not installed'), + }, + ]) + // Not "table users does not exist" ×2 and "ORE unavailable" on top: one + // fact explains all of them, and it names the fix. + expect(issues.every((issue) => issue.table === undefined)).toBe(true) + }) +}) + +describe('expectedExtractors', () => { + it.each([ + ['TextSearch', types.TextSearch, ['eq_term', 'ord_term', 'match_term']], + ['TextOrd', types.TextOrd, ['eq_term', 'ord_term']], + ['TextOrdOre', types.TextOrdOre, ['eq_term', 'ord_term_ore']], + ['TextEq', types.TextEq, ['eq_term']], + ['TextMatch', types.TextMatch, ['match_term']], + ['IntegerOrd', types.IntegerOrd, ['ord_term']], + ['IntegerOrdOre', types.IntegerOrdOre, ['ord_term_ore']], + ['Text', types.Text, []], + ['Json', types.Json, []], + ] as const)('%s', (_name, factory, expected) => { + expect(expectedExtractors(factory('c').build().indexes)).toEqual(expected) + }) +}) + +describe('parseIndexedExtractors', () => { + it('reads a plain expression index', () => { + const parsed = parseIndexedExtractors([ + { + table: 'users', + indexdef: + 'CREATE INDEX users_email_eq ON public.users USING btree (eql_v3.eq_term(email))', + }, + ]) + + expect(parsed.get('users.email')).toEqual(new Set(['eq_term'])) + }) + + it('does not confuse ord_term_ore with ord_term', () => { + const parsed = parseIndexedExtractors([ + { + table: 't', + indexdef: + 'CREATE INDEX i ON public.t USING btree (eql_v3.ord_term_ore(a))', + }, + ]) + + expect(parsed.get('t.a')).toEqual(new Set(['ord_term_ore'])) + }) + + it('sees through a cast, whose nested parens a first-)-wins scan would truncate', () => { + const parsed = parseIndexedExtractors([ + { + table: 'users', + indexdef: + 'CREATE INDEX i ON public.users USING btree (eql_v3.ord_term((email)::public.eql_v3_text_ord))', + }, + ]) + + expect(parsed.get('users.email')).toEqual(new Set(['ord_term'])) + // The cast target must not be recorded as a column of its own. + expect(parsed.has('users.eql_v3_text_ord')).toBe(false) + }) + + it('unwraps a quoted identifier', () => { + const parsed = parseIndexedExtractors([ + { + table: 'users', + indexdef: + 'CREATE INDEX i ON public.users USING btree (eql_v3.match_term("Email Address"))', + }, + ]) + + expect(parsed.get('users.Email Address')).toEqual(new Set(['match_term'])) + }) + + it('collects several extractors across several indexes on one table', () => { + const parsed = parseIndexedExtractors([ + { + table: 'users', + indexdef: + 'CREATE INDEX a ON public.users USING btree (eql_v3.eq_term(email))', + }, + { + table: 'users', + indexdef: + 'CREATE INDEX b ON public.users USING btree (eql_v3.ord_term(email))', + }, + { + table: 'users', + indexdef: + 'CREATE INDEX c ON public.users USING btree (eql_v3.ord_term(age))', + }, + ]) + + expect(parsed.get('users.email')).toEqual(new Set(['eq_term', 'ord_term'])) + expect(parsed.get('users.age')).toEqual(new Set(['ord_term'])) + }) + + it('ignores an index that engages no extractor', () => { + const parsed = parseIndexedExtractors([ + { + table: 'users', + indexdef: + 'CREATE UNIQUE INDEX users_pkey ON public.users USING btree (id)', + }, + ]) + + expect(parsed.size).toBe(0) + }) +}) diff --git a/packages/cli/src/commands/eql/validate.ts b/packages/cli/src/commands/eql/validate.ts new file mode 100644 index 000000000..3a6571917 --- /dev/null +++ b/packages/cli/src/commands/eql/validate.ts @@ -0,0 +1,613 @@ +import type { AnyV3Table } from '@cipherstash/stack/eql/v3' +import type { ColumnSchema, EncryptConfig } from '@cipherstash/stack/schema' +import * as p from '@clack/prompts' +import pg from 'pg' +import { fetchPhysicalColumns } from '@/commands/encrypt/lib/db-readers.js' +import { detectPackageManager, runnerCommand } from '@/commands/init/utils.js' +import { loadEncryptSchemas, loadStashConfig } from '@/config/index.js' + +// --------------------------------------------------------------------------- +// The vocabulary +// --------------------------------------------------------------------------- + +export type Severity = 'error' | 'warning' | 'info' + +export interface ValidationIssue { + severity: Severity + /** Omitted for schema-wide findings (e.g. "EQL is not installed"). */ + table?: string + /** Omitted for schema-wide findings. */ + column?: string + message: string +} + +/** + * One declared encrypted column, flattened out of the schema tuple. + * + * `eqlType` is the load-bearing field and the reason this type exists rather + * than the raw `EncryptConfig`: the concrete domain (`public.eql_v3_integer_ord`) + * is what distinguishes an OPE ordering column from its ORE twin, and what a + * live database reports back as `information_schema.columns.domain_name`. It is + * `undefined` only on the degraded config-only path (see + * {@link collectDeclaredColumnsFromConfig}). + */ +export interface DeclaredColumn { + table: string + /** The DB column name (`column.getName()`), not the JS property name. */ + column: string + /** e.g. `'public.eql_v3_integer_ord'`. */ + eqlType?: string + cast_as: ColumnSchema['cast_as'] + queryable: boolean + indexes: ColumnSchema['indexes'] +} + +/** + * What a live database says about the declared schema. Every field is supplied + * by the caller, so the rules below are testable without a database — see + * {@link readObservedState} for the queries that populate it. + */ +export interface ObservedState { + /** + * Whether the EQL v3 bundle is installed at all. When `false` nothing else + * here is meaningful: an absent ORE opclass would otherwise be reported as + * "ORE unavailable on this platform" when the real answer is "run + * `stash eql install`". + */ + eqlInstalled: boolean + /** + * Whether the default btree opclass over `eql_v3_internal.ore_block_256` + * exists. `CREATE OPERATOR CLASS` requires superuser, so managed Postgres + * (Supabase and most hosted providers) installs without it and the EQL + * bundle poisons every `_ord_ore` domain with an always-raising CHECK. + */ + oreAvailable: boolean + /** `table → column → domain_name` (null for a plain, non-domain type). */ + columns: Map> + /** + * `table.column → extractor names` found inside functional index + * definitions, e.g. `users.email → Set{'eq_term', 'match_term'}`. + */ + indexedExtractors: Map> +} + +// --------------------------------------------------------------------------- +// Collecting the declared schema +// --------------------------------------------------------------------------- + +/** + * Flatten the tuple passed to `Encryption({ schemas })` into one row per + * encrypted column, carrying the concrete domain of each. + */ +export function collectDeclaredColumns( + schemas: readonly AnyV3Table[], +): DeclaredColumn[] { + const out: DeclaredColumn[] = [] + + for (const table of schemas) { + for (const builder of Object.values(table.columnBuilders)) { + const built = builder.build() + out.push({ + table: table.tableName, + // Key by the DB name, exactly as `EncryptedTable.build()` does — a + // camelCase property mapping to a snake_case column must be reported + // (and drift-checked) under the name the database knows. + column: builder.getName(), + eqlType: builder.getEqlType(), + cast_as: built.cast_as, + queryable: builder.isQueryable(), + indexes: built.indexes, + }) + } + } + + return out +} + +/** + * The degraded collection path, for a project whose installed + * `@cipherstash/stack` predates `getSchemas()`. + * + * `eqlType` is left `undefined` — it genuinely is not recoverable from the + * encrypt config — and `queryable` is inferred from the emitted index block + * instead of from the domain's capability flags. Every rule that needs a domain + * skips these columns rather than guessing. + */ +export function collectDeclaredColumnsFromConfig( + config: EncryptConfig, +): DeclaredColumn[] { + const out: DeclaredColumn[] = [] + + for (const [tableName, columns] of Object.entries(config.tables)) { + for (const [columnName, column] of Object.entries(columns)) { + out.push({ + table: tableName, + column: columnName, + eqlType: undefined, + cast_as: column.cast_as, + queryable: Object.keys(column.indexes ?? {}).length > 0, + indexes: column.indexes, + }) + } + } + + return out +} + +// --------------------------------------------------------------------------- +// The rules +// --------------------------------------------------------------------------- + +/** Strip the schema qualifier: `'public.eql_v3_integer_ord'` → `'eql_v3_integer_ord'`. */ +export function bareDomainName(eqlType: string): string { + const dot = eqlType.lastIndexOf('.') + return dot === -1 ? eqlType : eqlType.slice(dot + 1) +} + +/** + * The EQL v3 term extractors a column's indexes imply. These are the functions + * a functional index must be built over — `CREATE INDEX ON users + * (eql_v3.eq_term(email))` — because EQL forbids an operator class on the + * domain itself. + * + * `ste_vec` is deliberately absent: an encrypted-JSONB column is served by a + * GIN index over the column, not by a scalar extractor, so it is excluded from + * the missing-index finding rather than reported against a recipe that does not + * apply to it. + */ +export function expectedExtractors(indexes: ColumnSchema['indexes']): string[] { + const out: string[] = [] + if (indexes.unique) out.push('eq_term') + if (indexes.ope) out.push('ord_term') + if (indexes.ore) out.push('ord_term_ore') + if (indexes.match) out.push('match_term') + return out +} + +/** + * Validate the declared schema, optionally against what a live database + * reports. + * + * Pure: every database fact arrives through `observed`, so the drift rules have + * real coverage without a database. Order of findings is stable (schema-wide + * first, then declaration order) so callers and tests can compare lists. + */ +export function validateSchemas( + columns: DeclaredColumn[], + observed?: ObservedState, +): ValidationIssue[] { + const issues: ValidationIssue[] = [] + + // Report a missing EQL install once, up front, and drop the per-column + // database rules: every one of them would otherwise fire on every column and + // bury the single fact that explains all of them. + const eqlMissing = observed !== undefined && !observed.eqlInstalled + if (eqlMissing) { + issues.push({ + severity: 'error', + message: + 'EQL v3 is not installed in this database — run `stash eql install`. Skipping the database checks (declared domains, drift, indexes); the schema checks below still ran.', + }) + } + + const dbState = eqlMissing ? undefined : observed + + for (const column of columns) { + const at = { table: column.table, column: column.column } + const domain = column.eqlType + + // -- Static rules ------------------------------------------------------ + + // An `_ord_ore` domain needs the ORE btree operator class, which + // `CREATE OPERATOR CLASS` reserves to superusers. The `_ord` twin is OPE + // and indexes everywhere, so this is a portability warning even before a + // database has been consulted. With a database it is upgraded to an error + // (below) once the opclass is confirmed absent. + if (domain?.endsWith('_ord_ore') && dbState === undefined) { + issues.push({ + ...at, + severity: 'warning', + message: `${bareDomainName(domain)} needs the ORE btree operator class, which only a superuser can create — managed Postgres (Supabase and most hosted providers) installs EQL without it, and every value written to this column then fails a CHECK. Use ${bareDomainName(domain).replace(/_ord_ore$/, '_ord')} unless you control the database role.`, + }) + } + + // Successor to the v2 "no indexes" Info. Read from the domain's capability + // flags rather than from the emitted index keys: v2 checked + // `ore`/`unique`/`match`/`ste_vec` and never learned about `ope`, so every + // EQL v3 `_ord` column — `types.IntegerOrd`, `types.TimestampOrd`, … — was + // reported as unsearchable. That bug is why this command was rewritten. + if (!column.queryable) { + issues.push({ + ...at, + severity: 'info', + message: + 'Storage-only column: it encrypts and decrypts but carries no query terms, so it cannot be searched, ordered or matched server-side. Pick a term-carrying domain (`types.TextEq`, `types.IntegerOrd`, `types.TextSearch`, …) if you need to query it.', + }) + } + + // Guards for a hand-authored encrypt config: no `types.*` factory can + // build either of the next two. `types.Boolean` is storage-only by + // construction, and `match` is emitted only by the text domains. + if (column.cast_as === 'boolean' && column.queryable) { + issues.push({ + ...at, + severity: 'error', + message: + 'A searchable boolean column leaks its plaintext: with two possible values, an equality or ordering term is a direct read of the value. EQL v3 offers `types.Boolean` (storage-only) only, by design.', + }) + } + + if (column.indexes.match && column.cast_as !== 'string') { + issues.push({ + ...at, + severity: 'error', + message: `Free-text match needs a text domain — this column casts to "${column.cast_as}". Use \`types.TextMatch\` or \`types.TextSearch\`.`, + }) + } + + if (column.indexes.ste_vec && column.cast_as !== 'json') { + issues.push({ + ...at, + severity: 'error', + message: `Encrypted-JSONB search needs \`types.Json\` (cast_as "json") — this column casts to "${column.cast_as}".`, + }) + } + + // -- Database rules ---------------------------------------------------- + + if (dbState === undefined) continue + + const observedColumns = dbState.columns.get(column.table) + + if (observedColumns === undefined) { + issues.push({ + ...at, + severity: 'error', + message: `Table "${column.table}" is declared in your encryption schema but does not exist in the database.`, + }) + continue + } + + if (!observedColumns.has(column.column)) { + issues.push({ + ...at, + severity: 'error', + message: `Column "${column.column}" is declared in your encryption schema but does not exist on table "${column.table}". Add it in a migration with the ${domain ? `\`${domain}\`` : 'declared EQL'} type.`, + }) + continue + } + + const observedDomain = observedColumns.get(column.column) ?? null + + if (observedDomain === null) { + issues.push({ + ...at, + severity: 'error', + message: `Column "${column.column}" is a plain (non-EQL) column in the database, but your schema declares it encrypted${domain ? ` as \`${domain}\`` : ''}. Encrypted payloads written to it are unconstrained and unqueryable.`, + }) + } else if (domain && observedDomain !== bareDomainName(domain)) { + issues.push({ + ...at, + severity: 'error', + message: `Declared \`${domain}\` but the database column is \`${observedDomain}\`. The domains carry different terms, so writes fail the column's CHECK or queries silently return nothing. Align the migration with the schema.`, + }) + } + + // The static `_ord_ore` warning becomes an error once the opclass is + // confirmed absent: on that database every write to this column raises. + if (domain?.endsWith('_ord_ore') && !dbState.oreAvailable) { + issues.push({ + ...at, + severity: 'error', + message: `\`${domain}\` is unusable in this database: the EQL installer could not create the ORE operator class (it requires superuser), so the domain carries an always-raising CHECK. Switch to \`${domain.replace(/_ord_ore$/, '_ord')}\`.`, + }) + } + + const missing = expectedExtractors(column.indexes).filter( + (extractor) => + !dbState.indexedExtractors + .get(`${column.table}.${column.column}`) + ?.has(extractor), + ) + + if (missing.length > 0) { + issues.push({ + ...at, + severity: 'info', + message: `No functional index over ${missing.map((e) => `\`eql_v3.${e}\``).join(' / ')} — queries against this column sequential-scan the table. e.g. CREATE INDEX ON "${column.table}" (eql_v3.${missing[0]}("${column.column}"));`, + }) + } + } + + return issues +} + +// --------------------------------------------------------------------------- +// Reading the database +// --------------------------------------------------------------------------- + +/** + * Matches the head of an extractor call. `ord_term_ore` is listed before + * `ord_term` because alternation is left-biased and one is a prefix of the + * other; `\b` alone would not separate them. + */ +const EXTRACTOR_HEAD = + /eql_v3\.(ord_term_ore|ord_term|eq_term|match_term)\s*\(/gi + +/** + * Pull `table.column → extractor` pairs out of `pg_get_indexdef()` output. + * + * Pure and exported so the parse has coverage without a database. The SQL + * feeding it is one catalogue read, but the shapes it has to survive are all in + * the string: quoted identifiers, `::` casts, a table qualifier, several + * extractors in one index, and nested parentheses inside the argument list. + * + * It errs towards over-matching — a stray identifier in the argument list is + * recorded as indexed. The finding this feeds is an Info ("no functional index + * over …"), so a false negative is a missed hint while a false positive would + * be a wrong instruction to create an index that already exists. + */ +export function parseIndexedExtractors( + defs: ReadonlyArray<{ table: string; indexdef: string }>, +): Map> { + const out = new Map>() + + for (const { table, indexdef } of defs) { + EXTRACTOR_HEAD.lastIndex = 0 + let head = EXTRACTOR_HEAD.exec(indexdef) + while (head !== null) { + const extractor = head[1].toLowerCase() + const args = balancedArgs(indexdef, head.index + head[0].length) + for (const column of identifiersIn(args)) { + const key = `${table}.${column}` + const set = out.get(key) ?? new Set() + set.add(extractor) + out.set(key, set) + } + head = EXTRACTOR_HEAD.exec(indexdef) + } + } + + return out +} + +/** + * The argument text of a call whose opening `(` has already been consumed: + * everything up to the matching `)`. Parenthesis-balanced because a cast + * renders as `eql_v3.ord_term((col)::public.eql_v3_text_ord)`, where a + * first-`)`-wins scan would stop inside the arguments. + */ +function balancedArgs(source: string, start: number): string { + let depth = 1 + for (let i = start; i < source.length; i++) { + const ch = source[i] + if (ch === '(') depth++ + else if (ch === ')') { + depth-- + if (depth === 0) return source.slice(start, i) + } + } + return source.slice(start) +} + +/** + * Identifiers inside an extractor's argument list, with `::` cast targets + * removed and quoting unwrapped. `pg_get_indexdef` renders an expression index + * as e.g. `eql_v3.eq_term(email)`, `eql_v3.eq_term("user email")`, or + * `eql_v3.ord_term((users.col)::public.eql_v3_text_ord)`. + */ +function identifiersIn(args: string): string[] { + const out: string[] = [] + // Quoted identifiers first — they may contain characters the bare-identifier + // pattern would split on. + const quoted = /"((?:[^"]|"")*)"/g + for (const match of args.matchAll(quoted)) { + out.push(match[1].replace(/""/g, '"')) + } + // Then bare identifiers, with quoted runs and cast targets blanked out so + // neither contributes a spurious name. + const rest = args + .replace(quoted, ' ') + .replace(/::\s*[A-Za-z_][\w.]*/g, ' ') + // A schema/table qualifier: keep only the final component. + .replace(/[A-Za-z_]\w*\s*\./g, ' ') + for (const match of rest.matchAll(/[A-Za-z_]\w*/g)) { + out.push(match[0]) + } + return out +} + +/** + * Whether the ORE btree operator class exists. Mirrors the EQL bundle's own + * fallback test (`ore_fallback.sql`), with one deliberate difference: + * `to_regtype` returns NULL where the bundle's `::regtype` cast raises, so this + * degrades to `false` on a database with no EQL installed instead of throwing. + * That is why the not-installed case is detected and reported separately. + */ +const ORE_AVAILABLE_SQL = ` + SELECT EXISTS ( + SELECT 1 + FROM pg_catalog.pg_opclass c + JOIN pg_catalog.pg_am am ON am.oid = c.opcmethod + WHERE am.amname = 'btree' + AND c.opcdefault + AND c.opcintype = to_regtype('eql_v3_internal.ore_block_256') + ) AS ore_available` + +const EQL_INSTALLED_SQL = ` + SELECT EXISTS ( + SELECT 1 FROM pg_catalog.pg_namespace WHERE nspname = 'eql_v3' + ) AS eql_installed` + +const INDEX_DEFS_SQL = ` + SELECT c.relname AS table_name, + pg_catalog.pg_get_indexdef(i.indexrelid) AS indexdef + FROM pg_catalog.pg_index i + JOIN pg_catalog.pg_class c ON c.oid = i.indrelid + JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = current_schema()` + +/** Read everything the database rules need, in four catalogue queries. */ +export async function readObservedState( + client: pg.ClientBase, + tables: ReadonlyArray, +): Promise { + const [installed, ore, columns, indexes] = await Promise.all([ + client.query<{ eql_installed: boolean }>(EQL_INSTALLED_SQL), + client.query<{ ore_available: boolean }>(ORE_AVAILABLE_SQL), + fetchPhysicalColumns(client, tables), + client.query<{ table_name: string; indexdef: string }>(INDEX_DEFS_SQL), + ]) + + return { + eqlInstalled: installed.rows[0]?.eql_installed === true, + oreAvailable: ore.rows[0]?.ore_available === true, + columns, + indexedExtractors: parseIndexedExtractors( + indexes.rows.map((row) => ({ + table: row.table_name, + indexdef: row.indexdef, + })), + ), + } +} + +// --------------------------------------------------------------------------- +// Reporting +// --------------------------------------------------------------------------- + +/** + * Print validation issues using `@clack/prompts` log methods. + * + * @returns `true` if there are any errors (severity === 'error'). + */ +export function reportIssues(issues: ValidationIssue[]): boolean { + for (const issue of issues) { + const line = + issue.table && issue.column + ? `${issue.table}.${issue.column}: ${issue.message}` + : issue.message + + switch (issue.severity) { + case 'error': + p.log.error(line) + break + case 'warning': + p.log.warn(line) + break + case 'info': + p.log.info(line) + break + } + } + + const errors = issues.filter((i) => i.severity === 'error').length + const warnings = issues.filter((i) => i.severity === 'warning').length + const infos = issues.filter((i) => i.severity === 'info').length + + if (errors > 0) { + p.outro( + `${errors} error${errors !== 1 ? 's' : ''}, ${warnings} warning${warnings !== 1 ? 's' : ''}.`, + ) + } else if (warnings > 0) { + p.outro(`No errors found. ${warnings} warning${warnings !== 1 ? 's' : ''}.`) + } else if (infos > 0) { + p.outro(`No errors or warnings. ${infos} info${infos !== 1 ? 's' : ''}.`) + } else { + p.outro('No issues found.') + } + + return errors > 0 +} + +// --------------------------------------------------------------------------- +// The command +// --------------------------------------------------------------------------- + +export async function validateCommand(options: { + supabase?: boolean + databaseUrl?: string +}) { + p.intro(runnerCommand(detectPackageManager(), 'stash eql validate')) + + const s = p.spinner() + + s.start('Loading stash.config.ts...') + const config = await loadStashConfig({ + databaseUrlFlag: options.databaseUrl, + supabase: options.supabase, + }) + s.stop('Configuration loaded.') + + s.start(`Loading encrypt client from ${config.client}...`) + const { config: encryptConfig, schemas } = await loadEncryptSchemas( + config.client, + ) + s.stop('Encrypt client loaded.') + + const columns = schemas + ? collectDeclaredColumns(schemas) + : collectDeclaredColumnsFromConfig(encryptConfig) + + if (!schemas) { + p.log.warn( + 'Your installed @cipherstash/stack does not expose `getSchemas()`, so the concrete EQL domain of each column is unavailable. Domain checks (ORE portability, database drift) were skipped — upgrade @cipherstash/stack to run them.', + ) + } + + const tableCount = new Set(columns.map((column) => column.table)).size + p.log.success( + `Schema loaded: ${tableCount} table${tableCount !== 1 ? 's' : ''}, ${columns.length} encrypted column${columns.length !== 1 ? 's' : ''}`, + ) + + const observed = await tryReadObservedState(config.databaseUrl, columns) + + const issues = validateSchemas(columns, observed) + + if (issues.length === 0) { + p.outro('No issues found.') + return + } + + console.log() // blank line before issues + const hasErrors = reportIssues(issues) + + if (hasErrors) { + process.exit(1) + } +} + +/** + * Connect and read the observed state, or return `undefined` and say why. + * + * A database that cannot be reached is not a validation failure: the schema + * rules are worth running on a laptop with no database up, and failing here + * would make the command unusable in exactly that setting. + */ +async function tryReadObservedState( + databaseUrl: string | undefined, + columns: DeclaredColumn[], +): Promise { + if (!databaseUrl) { + p.log.info( + 'No database URL resolved — skipping the database checks (drift, ORE availability, functional indexes). Pass --database-url or set DATABASE_URL to run them.', + ) + return undefined + } + + const tables = [...new Set(columns.map((column) => column.table))] + const client = new pg.Client({ connectionString: databaseUrl }) + + try { + await client.connect() + return await readObservedState(client, tables) + } catch (error) { + const message = error instanceof Error ? error.message : String(error) + p.log.info( + `Could not read the database (${message}) — skipping the database checks (drift, ORE availability, functional indexes). The schema checks below still ran.`, + ) + return undefined + } finally { + await client.end().catch(() => {}) + } +} diff --git a/packages/cli/src/commands/init/steps/install-eql.ts b/packages/cli/src/commands/init/steps/install-eql.ts index 0bf8bd796..e2783ebd8 100644 --- a/packages/cli/src/commands/init/steps/install-eql.ts +++ b/packages/cli/src/commands/init/steps/install-eql.ts @@ -83,7 +83,7 @@ export const installEqlStep: InitStep = { // installCommand scaffolds stash.config.ts (which `import`s from `stash`) // for the rest of the workflow. `stash` must be installed or the config the - // user relies on next (db validate / encrypt) can't load. Detect + // user relies on next (eql validate / encrypt) can't load. Detect // the precondition and bail with a clear message instead. install-deps is // what installs the package, so a "no" there leaves us here. if (!isPackageInstalled('stash')) { diff --git a/packages/cli/src/commands/init/utils.ts b/packages/cli/src/commands/init/utils.ts index a1b134fe2..95458c123 100644 --- a/packages/cli/src/commands/init/utils.ts +++ b/packages/cli/src/commands/init/utils.ts @@ -385,7 +385,7 @@ const DRIZZLE_PLACEHOLDER = `/** * to reference the encrypted tables you declared there. * * Until that happens, the encryption client is initialised with a single - * placeholder table so that this file compiles, and \`stash db validate\` and + * placeholder table so that this file compiles, and \`stash eql validate\` and * \`stash encrypt backfill\` refuse to run and point back here. (\`stash * encrypt drop\` resolves against the database and never reads this file.) * @@ -434,7 +434,7 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' // REPLACE THIS. It exists only so this file compiles before you have declared // any encrypted tables — \`Encryption\` requires at least one. Swap it for your -// real tables (see the patterns above); \`stash db validate\` and \`stash +// real tables (see the patterns above); \`stash eql validate\` and \`stash // encrypt backfill\` refuse to run while the placeholder is still here. export const placeholderTable = encryptedTable('__stash_placeholder__', { replace_me: types.Text('replace_me'), @@ -452,7 +452,7 @@ const GENERIC_PLACEHOLDER = `/** * \`Encryption({ schemas: [...] })\` call below to reference them. * * Until that happens, the encryption client is initialised with a single - * placeholder table so that this file compiles, and \`stash db validate\` and + * placeholder table so that this file compiles, and \`stash eql validate\` and * \`stash encrypt backfill\` refuse to run and point back here. (\`stash * encrypt drop\` resolves against the database and never reads this file.) * @@ -497,7 +497,7 @@ import { Encryption, encryptedTable, types } from '@cipherstash/stack/v3' // REPLACE THIS. It exists only so this file compiles before you have declared // any encrypted tables — \`Encryption\` requires at least one. Swap it for your -// real tables (see the patterns above); \`stash db validate\` and \`stash +// real tables (see the patterns above); \`stash eql validate\` and \`stash // encrypt backfill\` refuse to run while the placeholder is still here. export const placeholderTable = encryptedTable('__stash_placeholder__', { replace_me: types.Text('replace_me'), diff --git a/packages/cli/src/config/index.ts b/packages/cli/src/config/index.ts index 267d5c9a6..969744a2e 100644 --- a/packages/cli/src/config/index.ts +++ b/packages/cli/src/config/index.ts @@ -1,6 +1,7 @@ import fs from 'node:fs' import path from 'node:path' import type { EncryptionClient } from '@cipherstash/stack/encryption' +import type { AnyV3Table } from '@cipherstash/stack/eql/v3' import type { EncryptConfig } from '@cipherstash/stack/schema' import { z } from 'zod' import { detectPackageManager, runnerCommand } from '../commands/init/utils.js' @@ -184,6 +185,28 @@ To create it by hand, add ${CONFIG_FILENAME} to your project root: export async function loadEncryptConfig( encryptClientPath: string, ): Promise { + const encryptClient = await loadEncryptionClient(encryptClientPath) + + return requireUsableEncryptConfig( + encryptClient.getEncryptConfig(), + encryptClientPath, + ) +} + +/** + * Find the user's `EncryptionClient` in their encryption-client file. + * + * Extracted from {@link loadEncryptConfig} so that {@link loadEncryptSchemas} + * reaches the same export through the same jiti load and the same refusals — + * two loaders that hand-copied this is exactly how the placeholder guard + * drifted before (see {@link requireUsableEncryptConfig}). + * + * Exits with code 1 if the file is missing, fails to load, or exports no + * client. + */ +async function loadEncryptionClient( + encryptClientPath: string, +): Promise { const resolvedPath = path.resolve(process.cwd(), encryptClientPath) if (!fs.existsSync(resolvedPath)) { @@ -231,10 +254,82 @@ export async function loadEncryptConfig( process.exit(1) } - return requireUsableEncryptConfig( + return encryptClient +} + +/** What {@link loadEncryptSchemas} recovers from the user's client file. */ +export interface LoadedEncryptSchemas { + /** The built encrypt config — always present, same value `loadEncryptConfig` returns. */ + config: EncryptConfig + /** + * The declared v3 tables, when the installed `@cipherstash/stack` exposes + * `getSchemas()`. `undefined` on an older release, which is why every caller + * has to degrade rather than assume. + */ + schemas: readonly AnyV3Table[] | undefined +} + +/** + * Load the user's encryption client and recover BOTH views of its schema: the + * built `EncryptConfig` and — when available — the declared v3 tables. + * + * The two are not interchangeable. `EncryptedV3Column.build()` emits only + * `{ cast_as, indexes }`, so the concrete domain name never reaches the encrypt + * config: `cast_as: 'number'` with an `ope` index is ambiguous across + * `eql_v3_integer_ord`, `smallint_ord`, `real_ord`, `double_ord` and + * `numeric_ord`. Any rule that reasons about the DECLARED domain — steering + * `_ord_ore` columns, or drift-checking against a live database's + * `information_schema.columns.domain_name` — needs the tables themselves. + * + * `schemas` is `undefined` when the project's installed `@cipherstash/stack` + * predates `getSchemas()`. That is a real customer state (the CLI and the + * library version independently), so it degrades to config-only rather than + * failing: the caller runs the subset of rules the encrypt config can answer + * and says which ones it skipped. + * + * Exits with code 1 through the same refusals as {@link loadEncryptConfig}. + */ +export async function loadEncryptSchemas( + encryptClientPath: string, +): Promise { + const encryptClient = await loadEncryptionClient(encryptClientPath) + + const config = requireUsableEncryptConfig( encryptClient.getEncryptConfig(), encryptClientPath, ) + + // Duck-typed, not a version check: the client comes from the USER's + // node_modules via jiti, so its shape is the only reliable signal of what it + // supports. + const getSchemas = ( + encryptClient as { getSchemas?: () => readonly AnyV3Table[] } + ).getSchemas + + if (typeof getSchemas !== 'function') { + return { config, schemas: undefined } + } + + const schemas = getSchemas.call(encryptClient) + + // A client built by an adapter (or a hand-rolled stub) could return + // something other than an array of tables. Verify the shape rather than + // trusting it — the caller's rules dereference `columnBuilders`. + if (!Array.isArray(schemas) || !schemas.every(isV3TableLike)) { + return { config, schemas: undefined } + } + + return { config, schemas } +} + +/** Structural check for the parts of an `EncryptedTable` the schema rules read. */ +function isV3TableLike(value: unknown): value is AnyV3Table { + return ( + !!value && + typeof value === 'object' && + typeof (value as { tableName?: unknown }).tableName === 'string' && + typeof (value as { columnBuilders?: unknown }).columnBuilders === 'object' + ) } /** @@ -242,7 +337,7 @@ export async function loadEncryptConfig( * cause. * * Shared rather than duplicated because it guards ONE file reached by two - * loaders — `loadEncryptConfig` for `stash db validate`, and + * loaders — `loadEncryptConfig` for `stash eql validate`, and * `loadEncryptionContext` for `stash encrypt backfill`. When the copies were * separate they had already drifted on the nullish-config case, so one command * named the cause while the other fell through to `requireTable`'s `Table diff --git a/packages/cli/tests/e2e/command-help.e2e.test.ts b/packages/cli/tests/e2e/command-help.e2e.test.ts index 1dc33110a..47f8633e5 100644 --- a/packages/cli/tests/e2e/command-help.e2e.test.ts +++ b/packages/cli/tests/e2e/command-help.e2e.test.ts @@ -21,10 +21,23 @@ describe('per-command --help', () => { expect(r.output).toContain('eql repair') expect(r.output).toContain('eql upgrade') expect(r.output).toContain('eql status') + expect(r.output).toContain('eql validate') // A group listing must NOT be the global banner. expect(r.output).not.toContain('CipherStash CLI v') }) + it('renders full command help for `eql validate --help`', async () => { + const r = await run(['eql', 'validate', '--help'], { + env: { npm_config_user_agent: '' }, + }) + expect(r.exitCode).toBe(0) + expect(r.output).toContain('Usage: npx stash eql validate [options]') + expect(r.output).toContain('--supabase') + expect(r.output).toContain('--database-url') + // Retired with the v2 rule set — the pinned EQL v3 bundle self-adapts. + expect(r.output).not.toContain('--exclude-operator-family') + }) + it('renders full command help for `eql migration --help`', async () => { const r = await run(['eql', 'migration', '--help'], { env: { npm_config_user_agent: '' }, diff --git a/packages/cli/tests/e2e/smoke.e2e.test.ts b/packages/cli/tests/e2e/smoke.e2e.test.ts index 72a007d4d..f0944152f 100644 --- a/packages/cli/tests/e2e/smoke.e2e.test.ts +++ b/packages/cli/tests/e2e/smoke.e2e.test.ts @@ -27,6 +27,7 @@ describe('stash CLI — non-interactive smoke', () => { expect(r.output).toContain('eql repair') expect(r.output).toContain('eql upgrade') expect(r.output).toContain('eql status') + expect(r.output).toContain('eql validate') // The dotenv "injected env" banner regression guard lives in the // dedicated test below — this cwd has no .env file, so a bare // `not.toContain('injected env')` here would pass vacuously. @@ -132,6 +133,36 @@ describe('stash CLI — non-interactive smoke', () => { expect(r.output).toContain('eql migration --drizzle') }) + // `validate` moved from the `db` group to the `eql` group. Both spellings + // still route; only the old one warns. Run from a directory with no + // `stash.config.ts` so the command stops at the config load — deterministic, + // and enough to prove the routing. + it('db validate still works as a deprecated alias and warns', async () => { + const tmpDir = mkdtempSync(join(tmpdir(), 'stash-validate-alias-')) + try { + const r = await run(['db', 'validate'], { cwd: tmpDir }) + expect(r.exitCode).toBe(1) + expect(r.output).toContain('stash db validate" is deprecated') + expect(r.output).toContain('eql validate" instead') + // It reached the real command (which needs a config it cannot find). + expect(r.output).toContain('Could not find stash.config.ts') + } finally { + rmSync(tmpDir, { recursive: true, force: true }) + } + }) + + it('eql validate routes without a deprecation warning', async () => { + const tmpDir = mkdtempSync(join(tmpdir(), 'stash-validate-')) + try { + const r = await run(['eql', 'validate'], { cwd: tmpDir }) + expect(r.exitCode).toBe(1) + expect(r.output).not.toContain('is deprecated') + expect(r.output).toContain('Could not find stash.config.ts') + } finally { + rmSync(tmpDir, { recursive: true, force: true }) + } + }) + it('eql install routes to the install command without a deprecation warning', async () => { const r = render(['eql', 'install', '--migration']) const { exitCode } = await r.exit diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index cc87a2e9d..4aa3c1de3 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -1,6 +1,6 @@ --- name: stash-cli -description: Drive CipherStash setup and encryption migrations through the `stash` CLI — `init`, `plan`, `impl`, `status`, `auth login`, `eql install/migration/repair/upgrade/status`, `db validate`, `encrypt backfill/drop`, `schema build`, and `manifest --json`. Covers the agent / non-interactive interface, credential rules, and the staged EQL v3 rollout lifecycle. +description: Drive CipherStash setup and encryption migrations through the `stash` CLI — `init`, `plan`, `impl`, `status`, `auth login`, `eql install/migration/repair/upgrade/status/validate`, `encrypt backfill/drop`, `schema build`, and `manifest --json`. Covers the agent / non-interactive interface, credential rules, and the staged EQL v3 rollout lifecycle. --- # CipherStash CLI (`stash`) @@ -199,7 +199,7 @@ export default defineConfig({ | Option | Required | Default | Purpose | |---|---|---|---| | `databaseUrl` | yes | — | PostgreSQL connection string | -| `client` | no | `./src/encryption/index.ts` | Encryption client, loaded by `db validate` and `encrypt backfill` (`schema build` only writes here; `encrypt drop` resolves against the database). **Not required in Prisma Next projects** — see below. | +| `client` | no | `./src/encryption/index.ts` | Encryption client, loaded by `eql validate` and `encrypt backfill` (`schema build` only writes here; `encrypt drop` resolves against the database). **Not required in Prisma Next projects** — see below. | In a **Prisma Next** project there is deliberately no client file: encrypted columns are declared in the PSL contract. When the configured `client` path is missing, `encrypt backfill` — the only command that loads it — detects Prisma Next, reads the emitted `contract.json` (searched at `src/prisma/`, `prisma/`, then the project root), and derives the schemas with the adapter's own `deriveStackSchemasV3`, so the CLI's schema view matches the application's. (`encrypt status` and `encrypt drop` never read the client file; they resolve against the database.) Both `@cipherstash/stack-prisma` and `@cipherstash/stack` are resolved from *your* project, not the CLI's dependency tree. Run `prisma-next contract emit` first; if the contract declares no `cipherstash.*()` column, the command says so rather than reporting a missing client file. @@ -425,18 +425,46 @@ The install SQL is safe to re-run — columns and data survive — but it cascad Whether EQL is installed and at which version, plus database permission status. It retains read-only EQL v2/config-table diagnostics for existing deployments. -### Database +#### `eql validate` — validate the encryption schema + +```bash +stash eql validate [--supabase] [--database-url ] +``` -#### `db validate` — validate the encryption schema +Reads the tables passed to `Encryption({ schemas })` — through the client's `getSchemas()` accessor, so it sees the **concrete domain** of every column, which the built encrypt config does not carry — and checks them against the EQL v3 vocabulary. If a database is reachable it then checks the declaration against what that database actually has. + +Schema checks (no database needed): | Rule | Severity | |---|---| -| `freeTextSearch` on a non-string column | Warning | -| `orderAndRange` without operator families | Warning | -| No indexes on an encrypted column | Info | -| `searchableJson` without `dataType("json")` | Error | +| An `_ord_ore` domain is declared (`types.NOrdOre`, `types.TextOrdOre`) | Warning | +| Storage-only column — encrypts and decrypts, carries no query terms | Info | +| Searchable `boolean` column | Error | +| Free-text `match` index on a non-text domain | Error | +| Encrypted-JSONB (`ste_vec`) index without `types.Json` | Error | + +Database checks (skipped with a notice, not a failure, when no database is reachable): -Exits 1 on errors only. The "No indexes" Info finding applies to term-carrying (queryable) columns — resolve it with the functional-index recipes in the `stash-indexing` skill. Storage-only columns (bare `types.T`, `types.Boolean`) have no index option by design; for them the finding needs no action. +| Rule | Severity | +|---|---| +| EQL v3 is not installed — reported once, and the remaining database checks are skipped | Error | +| A declared table or column does not exist in the database | Error | +| The database column's domain differs from the declared one | Error | +| The database column is still plain (no EQL domain) | Error | +| An `_ord_ore` domain on a database whose EQL install could not create the ORE operator class | Error | +| A queryable column with no functional index over its term extractor | Info | + +Exits 1 on errors only; warnings and info do not fail the command. + +**The `_ord_ore` finding is about portability.** `CREATE OPERATOR CLASS` requires superuser, so managed Postgres (Supabase and most hosted providers) installs EQL without the ORE btree operator class, and the bundle then poisons every `_ord_ore` domain with an always-raising CHECK. Prefer the `_ord` (OPE) twin unless you control the database role. With a database reachable, validate confirms which case you are in and upgrades the Warning to an Error when the operator class is genuinely absent. + +The "no functional index" Info applies only to term-carrying (queryable) columns — resolve it with the recipes in the `stash-indexing` skill. Storage-only columns (bare `types.T`, `types.Boolean`) have no index option by design, and `types.Json` is served by a GIN index over the column rather than a scalar extractor; neither is reported. + +**Not checked:** the ordered domains reject empty strings through a value-level CHECK enforced at encrypt time. Nothing in the schema or in `information_schema` predicts it, so validate cannot catch it statically. + +`stash db validate` still routes here, with a deprecation warning. + +### Database #### `db test-connection` diff --git a/skills/stash-indexing/SKILL.md b/skills/stash-indexing/SKILL.md index 4dc3de8cd..e64782418 100644 --- a/skills/stash-indexing/SKILL.md +++ b/skills/stash-indexing/SKILL.md @@ -14,7 +14,7 @@ This covers EQL v3 — the bundle `stash eql install` applies (`@cipherstash/eql - Writing or reviewing a schema migration that adds or changes an encrypted (`eql_v3_*`) column. - Deciding which indexes an encrypted column supports — or explaining why a column has none. - An encrypted query is slow, or `EXPLAIN` shows a `Seq Scan` where you expected an index. -- `stash db validate` reports "No indexes on an encrypted column". +- `stash eql validate` reports "No functional index over `eql_v3.…`" for a queryable column. - Answering whether encrypted columns can be indexed on Supabase or managed PostgreSQL (yes — see the superuser section). ## Which Columns Support Which Index @@ -269,7 +269,7 @@ Index not being used: ## Reference - `stash-encryption` — the `types.*` domain catalog, wire-format operators and ordering, and the staged rollout lifecycle. -- `stash-cli` — `stash eql install`, `stash db validate` (its "No indexes on an encrypted column" Info finding is resolved by this skill), and `stash encrypt backfill` / `drop`. +- `stash-cli` — `stash eql install`, `stash eql validate` (its "No functional index over `eql_v3.…`" Info finding is resolved by this skill), and `stash encrypt backfill` / `drop`. - `stash-drizzle`, `stash-supabase`, `stash-prisma` — per-integration query patterns; index DDL placement per the section above. - `stash-postgres` — the hand-written predicate forms these indexes serve (`pg` / `postgres-js`, no ORM). - `stash-edge` — the WASM entry, for apps whose queries run on Deno / Workers / Supabase Edge Functions. diff --git a/skills/stash-postgres/SKILL.md b/skills/stash-postgres/SKILL.md index 4f01be7b7..de8eb0352 100644 --- a/skills/stash-postgres/SKILL.md +++ b/skills/stash-postgres/SKILL.md @@ -465,7 +465,7 @@ SELECT column_name, domain_schema, domain_name - `stash-edge` — the WASM entry and running encryption from edge runtimes. - `stash-zerokms` — keysets, clients, and grants (canonical for keyset scoping). - `stash-auth` — credentials, auth strategies, and lock context (canonical). -- `stash-cli` — `stash eql install`, `stash db validate`, `stash encrypt backfill`. +- `stash-cli` — `stash eql install`, `stash eql validate` (schema-vs-database domain drift, and the `eql_v3.*` functional indexes this skill's predicates need), `stash encrypt backfill`. Upstream: From 3bbc7ec08d31a1e39e8f9dd1ce7af32d6eb3dd2f Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 4 Aug 2026 13:28:31 +1000 Subject: [PATCH 4/7] fix(cli): harden eql validate against review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six defects found reviewing #857, each pinned by a test written to fail first. A table absent from `current_schema()` was an Error, and both catalogue reads are scoped to that one schema. So a project whose tables live anywhere else — Prisma `multiSchema`, a tenant schema, a `schema.table` name that the reader compares whole against a bare `table_name` and never matches — failed validate on a completely healthy database. Validate cannot tell that apart from a migration that never ran, so it is now a Warning naming the schema it searched. The missing-COLUMN rule stays an Error: there the table resolved, so the column really is absent. `isV3TableLike` accepted `columnBuilders: null` (`typeof null === 'object'`) and never checked the builders themselves, so a malformed client reached `Object.values(null)` in `collectDeclaredColumns` and crashed with a stack trace — in the one guard written so that such a client degrades instead. It now rejects null and verifies every builder implements the four methods the rules call. The index read fetched and regex-parsed every `pg_get_indexdef()` in the schema to answer a question about a handful of declared columns; it is now constrained to those tables. `collectDeclaredColumnsFromConfig` guarded `column.indexes` with `?? {}` for the queryable check and then assigned it bare. That config is user code loaded through jiti — the zod types that make `indexes` non-optional never run against it — so a column missing the key threw on `column.indexes.match` partway down the rule list. Guarded on both. The CREATE INDEX suggestion is meant to be pasted, but quoted identifiers without doubling an embedded `"`; `identifiersIn` already un-doubles on the read side. Added `quoteIdent` as the write side of that rule. `EXTRACTOR_HEAD` was a module-level `/g` regex whose `lastIndex` an otherwise-pure function depended on. Now built per call. Also: `getSchemas()` returns a frozen tuple, since the reconstructor map is derived from it once at construction and the CLI reaches the client untyped; and the skill and changeset record the schema-scoping limit and the older-@cipherstash/stack fallback, neither of which was documented. --- .changeset/proud-ravens-repeat.md | 13 +- .../commands/eql/__tests__/validate.test.ts | 125 +++++++++++++++- packages/cli/src/commands/eql/validate.ts | 74 ++++++++-- .../__tests__/load-encrypt-schemas.test.ts | 133 ++++++++++++++++++ packages/cli/src/config/index.ts | 37 ++++- .../__tests__/client-get-schemas.test.ts | 23 +++ packages/stack/src/encryption/client-v3.ts | 14 +- skills/stash-cli/SKILL.md | 7 +- 8 files changed, 402 insertions(+), 24 deletions(-) create mode 100644 packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts diff --git a/.changeset/proud-ravens-repeat.md b/.changeset/proud-ravens-repeat.md index c666d5748..6b18f7d71 100644 --- a/.changeset/proud-ravens-repeat.md +++ b/.changeset/proud-ravens-repeat.md @@ -32,7 +32,8 @@ reachable): | Rule | Severity | |---|---| | EQL v3 is not installed — reported once, remaining database checks skipped | Error | -| A declared table or column does not exist in the database | Error | +| A declared table was not found in the searched schema | Warning | +| A declared column is missing from a table that was found | Error | | The database column's domain differs from the declared one | Error | | The database column is still plain (no EQL domain) | Error | | An `_ord_ore` domain where the EQL install could not create the ORE operator class | Error | @@ -43,5 +44,15 @@ support `ORDER BY` without operator families, and the pinned EQL v3 bundle self-adapts. `eql install` / `eql upgrade` had already rejected the flag; `validate` was its last consumer. +The database pass inspects `current_schema()` only, so a declared table that +lives in another schema (Prisma `multiSchema`, a tenant schema, a +`schema.table` name) cannot be told apart from one whose migration never ran. +That finding is therefore a Warning naming the schema searched, not an Error — +a healthy project in a non-default schema does not fail the command. + +Against a project whose `@cipherstash/stack` predates `getSchemas()`, validate +says so and falls back to the encrypt config, running the index-derived rules +and skipping the domain ones. + `stash db validate` keeps working as a deprecated alias, like `db install` / `db upgrade` / `db status`. Exits 1 on errors only. diff --git a/packages/cli/src/commands/eql/__tests__/validate.test.ts b/packages/cli/src/commands/eql/__tests__/validate.test.ts index c74cd3b82..8f0372490 100644 --- a/packages/cli/src/commands/eql/__tests__/validate.test.ts +++ b/packages/cli/src/commands/eql/__tests__/validate.test.ts @@ -7,6 +7,7 @@ import { expectedExtractors, type ObservedState, parseIndexedExtractors, + readObservedState, validateSchemas, } from '../validate.js' @@ -29,6 +30,7 @@ function observing( return { eqlInstalled: true, oreAvailable: true, + searchedSchema: 'public', columns: new Map( Object.entries(columns).map(([table, cols]) => [ table, @@ -330,17 +332,44 @@ describe('database rules', () => { expect(validateSchemas(columns, observed)).toEqual([]) }) - it('reports a missing table once, and does not then look for its columns', () => { + /** + * A table validate cannot see is NOT proof the table is missing. Both + * catalogue reads are scoped to one schema — `current_schema()`, the head of + * `search_path` — so a project whose tables live anywhere else (Prisma + * `multiSchema`, a tenant schema, a `schema.table` name in `encryptedTable`) + * looks identical to a project that never ran its migration. + * + * Validate cannot tell those apart, and one of them is a perfectly healthy + * database, so this must not exit 1. It reports the schema it actually + * searched instead, which is the fact the user needs to tell which case they + * are in. The missing-COLUMN rule below stays an Error: there the table + * resolved, so the schema was right and the column really is absent. + */ + it('warns rather than errors when a table is absent, naming the schema searched', () => { const observed = observing({}, { indexedExtractors: fullyIndexed(columns) }) const issues = validateSchemas(columns, observed) expect(issues).toHaveLength(2) for (const issue of issues) { - expect(issue.severity).toBe('error') - expect(issue.message).toMatch(/Table "users" .* does not exist/) + expect(issue.severity).toBe('warning') + expect(issue.message).toMatch( + /Table "users" was not found in schema "public"/, + ) + expect(issue.message).toMatch(/search_path/) } }) + it('names whichever schema was actually searched', () => { + const observed = observing( + {}, + { searchedSchema: 'tenant_7', indexedExtractors: fullyIndexed(columns) }, + ) + + expect(validateSchemas(columns, observed)[0]?.message).toContain( + 'schema "tenant_7"', + ) + }) + it('reports a declared column the table does not have', () => { const observed = observing( { users: { email: 'eql_v3_text_search' } }, @@ -597,3 +626,93 @@ describe('parseIndexedExtractors', () => { expect(parsed.size).toBe(0) }) }) + +describe('the encrypt config is user code, not a typed value', () => { + /** + * `getEncryptConfig()` is read out of the USER's node_modules through jiti, + * so its runtime shape is whatever their client hands back — the zod types + * that make `indexes` non-optional never run here. A column missing the key + * entirely must degrade to "no indexes", not throw on `column.indexes.match` + * partway through the rule list. + */ + it('survives a config column with no indexes key at all', () => { + const config = { + v: 1, + tables: { users: { email: { cast_as: 'string' } } }, + } as unknown as EncryptConfig + + const columns = collectDeclaredColumnsFromConfig(config) + + expect(() => validateSchemas(columns)).not.toThrow() + expect(validateSchemas(columns)).toEqual([ + { + severity: 'info', + table: 'users', + column: 'email', + message: expect.stringContaining('Storage-only column'), + }, + ]) + }) +}) + +describe('the CREATE INDEX suggestion is meant to be pasted', () => { + /** + * The Info finding hands the user runnable SQL. A column name containing a + * double quote has to be doubled inside the quoted identifier, the same way + * `identifiersIn` un-doubles it on the read side — otherwise the suggestion + * pastes as a syntax error at best. + */ + it('doubles a quote embedded in an identifier', () => { + const table = encryptedTable('we"ird', { 'a"b': types.TextEq('a"b') }) + const columns = collectDeclaredColumns([table]) + const observed = observing({ 'we"ird': { 'a"b': 'eql_v3_text_eq' } }) + + const [issue] = validateSchemas(columns, observed) + + expect(issue.severity).toBe('info') + expect(issue.message).toContain( + 'CREATE INDEX ON "we""ird" (eql_v3.eq_term("a""b"));', + ) + }) +}) + +describe('readObservedState', () => { + /** + * The index read used to scan every index in the schema and hand each + * `pg_get_indexdef()` to the parser, to answer a question about a handful of + * declared columns. On a large schema that is thousands of definitions + * fetched and regex-parsed for nothing. + */ + it('constrains the index scan to the declared tables', async () => { + const queries: Array<{ text: string; values?: unknown[] }> = [] + const client = { + query: (text: string, values?: unknown[]) => { + queries.push({ text, values }) + return Promise.resolve({ rows: [] }) + }, + } as unknown as Parameters[0] + + await readObservedState(client, ['users', 'orders']) + + const indexQuery = queries.find((q) => q.text.includes('pg_get_indexdef')) + + expect(indexQuery).toBeDefined() + expect(indexQuery?.text).toMatch(/relname\s*=\s*ANY/) + expect(indexQuery?.values).toEqual([['users', 'orders']]) + }) + + it('reports the schema it searched', async () => { + const client = { + query: (text: string) => + Promise.resolve({ + rows: text.includes('current_schema') + ? [{ searched_schema: 'tenant_7' }] + : [], + }), + } as unknown as Parameters[0] + + const observed = await readObservedState(client, ['users']) + + expect(observed.searchedSchema).toBe('tenant_7') + }) +}) diff --git a/packages/cli/src/commands/eql/validate.ts b/packages/cli/src/commands/eql/validate.ts index 3a6571917..3b6460b0c 100644 --- a/packages/cli/src/commands/eql/validate.ts +++ b/packages/cli/src/commands/eql/validate.ts @@ -62,6 +62,13 @@ export interface ObservedState { * bundle poisons every `_ord_ore` domain with an always-raising CHECK. */ oreAvailable: boolean + /** + * The schema both catalogue reads were scoped to — `current_schema()`, the + * head of `search_path`. Carried so the not-found findings can say WHERE + * they looked: a table absent from this schema may simply live in another + * one, and validate has no way to tell that from a missing migration. + */ + searchedSchema: string /** `table → column → domain_name` (null for a plain, non-domain type). */ columns: Map> /** @@ -125,8 +132,14 @@ export function collectDeclaredColumnsFromConfig( column: columnName, eqlType: undefined, cast_as: column.cast_as, + // `?? {}` on BOTH, deliberately. The zod schema makes `indexes` + // non-optional, but that zod never runs here: this config came out of + // the user's own client through jiti, so the static type is a + // description of what it should be, not a guarantee. A column missing + // the key must degrade to "no indexes", not throw partway down the + // rule list on `column.indexes.match`. queryable: Object.keys(column.indexes ?? {}).length > 0, - indexes: column.indexes, + indexes: column.indexes ?? {}, }) } } @@ -138,6 +151,17 @@ export function collectDeclaredColumnsFromConfig( // The rules // --------------------------------------------------------------------------- +/** + * Wrap an identifier as a quoted SQL name, doubling any embedded quote. + * + * The missing-index finding hands the user runnable SQL, so a name carrying a + * `"` has to survive the paste — `identifiersIn` already un-doubles it on the + * read side, and this is the write side of the same rule. + */ +export function quoteIdent(name: string): string { + return `"${name.replace(/"/g, '""')}"` +} + /** Strip the schema qualifier: `'public.eql_v3_integer_ord'` → `'eql_v3_integer_ord'`. */ export function bareDomainName(eqlType: string): string { const dot = eqlType.lastIndexOf('.') @@ -259,11 +283,19 @@ export function validateSchemas( const observedColumns = dbState.columns.get(column.table) + // NOT an error. Both catalogue reads are scoped to one schema, so "absent + // from `current_schema()`" covers two very different situations: a + // migration that was never applied, and a perfectly healthy project whose + // tables live in another schema (Prisma `multiSchema`, a tenant schema, or + // a `schema.table` name in `encryptedTable`, which the reader compares + // whole against a bare `table_name` and never matches). Validate cannot + // distinguish them, so it reports where it looked and leaves the exit code + // alone rather than failing a healthy database's CI. if (observedColumns === undefined) { issues.push({ ...at, - severity: 'error', - message: `Table "${column.table}" is declared in your encryption schema but does not exist in the database.`, + severity: 'warning', + message: `Table "${column.table}" was not found in schema "${dbState.searchedSchema}", so nothing about this column could be checked against the database. Validate only inspects current_schema() (the head of search_path) — if this table lives in another schema, point the connection at it (e.g. ?options=-csearch_path%3D); otherwise the migration that creates it has not been applied.`, }) continue } @@ -314,7 +346,7 @@ export function validateSchemas( issues.push({ ...at, severity: 'info', - message: `No functional index over ${missing.map((e) => `\`eql_v3.${e}\``).join(' / ')} — queries against this column sequential-scan the table. e.g. CREATE INDEX ON "${column.table}" (eql_v3.${missing[0]}("${column.column}"));`, + message: `No functional index over ${missing.map((e) => `\`eql_v3.${e}\``).join(' / ')} — queries against this column sequential-scan the table. e.g. CREATE INDEX ON ${quoteIdent(column.table)} (eql_v3.${missing[0]}(${quoteIdent(column.column)}));`, }) } } @@ -330,8 +362,12 @@ export function validateSchemas( * Matches the head of an extractor call. `ord_term_ore` is listed before * `ord_term` because alternation is left-biased and one is a prefix of the * other; `\b` alone would not separate them. + * + * A factory, not a shared constant: a `/g` regex carries `lastIndex`, and a + * module-level one would make an otherwise-pure function depend on where the + * previous call happened to stop. */ -const EXTRACTOR_HEAD = +const extractorHead = () => /eql_v3\.(ord_term_ore|ord_term|eq_term|match_term)\s*\(/gi /** @@ -353,8 +389,8 @@ export function parseIndexedExtractors( const out = new Map>() for (const { table, indexdef } of defs) { - EXTRACTOR_HEAD.lastIndex = 0 - let head = EXTRACTOR_HEAD.exec(indexdef) + const pattern = extractorHead() + let head = pattern.exec(indexdef) while (head !== null) { const extractor = head[1].toLowerCase() const args = balancedArgs(indexdef, head.index + head[0].length) @@ -364,7 +400,7 @@ export function parseIndexedExtractors( set.add(extractor) out.set(key, set) } - head = EXTRACTOR_HEAD.exec(indexdef) + head = pattern.exec(indexdef) } } @@ -439,29 +475,43 @@ const EQL_INSTALLED_SQL = ` SELECT 1 FROM pg_catalog.pg_namespace WHERE nspname = 'eql_v3' ) AS eql_installed` +/** + * Constrained to the declared tables, not the whole schema. Unfiltered this + * fetches and regex-parses every `pg_get_indexdef()` in the database to answer + * a question about a handful of columns — on a large schema, thousands of + * definitions for nothing. + */ const INDEX_DEFS_SQL = ` SELECT c.relname AS table_name, pg_catalog.pg_get_indexdef(i.indexrelid) AS indexdef FROM pg_catalog.pg_index i JOIN pg_catalog.pg_class c ON c.oid = i.indrelid JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace - WHERE n.nspname = current_schema()` + WHERE n.nspname = current_schema() + AND c.relname = ANY($1::text[])` + +/** The schema both catalogue reads are scoped to. Reported in not-found findings. */ +const SEARCHED_SCHEMA_SQL = `SELECT current_schema() AS searched_schema` -/** Read everything the database rules need, in four catalogue queries. */ +/** Read everything the database rules need, in five catalogue queries. */ export async function readObservedState( client: pg.ClientBase, tables: ReadonlyArray, ): Promise { - const [installed, ore, columns, indexes] = await Promise.all([ + const [installed, ore, schema, columns, indexes] = await Promise.all([ client.query<{ eql_installed: boolean }>(EQL_INSTALLED_SQL), client.query<{ ore_available: boolean }>(ORE_AVAILABLE_SQL), + client.query<{ searched_schema: string }>(SEARCHED_SCHEMA_SQL), fetchPhysicalColumns(client, tables), - client.query<{ table_name: string; indexdef: string }>(INDEX_DEFS_SQL), + client.query<{ table_name: string; indexdef: string }>(INDEX_DEFS_SQL, [ + tables, + ]), ]) return { eqlInstalled: installed.rows[0]?.eql_installed === true, oreAvailable: ore.rows[0]?.ore_available === true, + searchedSchema: schema.rows[0]?.searched_schema ?? 'public', columns, indexedExtractors: parseIndexedExtractors( indexes.rows.map((row) => ({ diff --git a/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts b/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts new file mode 100644 index 000000000..117b17760 --- /dev/null +++ b/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts @@ -0,0 +1,133 @@ +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { loadEncryptSchemas } from '@/config/index.js' + +/** + * `loadEncryptSchemas` reads a client out of the USER's `node_modules` through + * jiti. Nothing about that object is checked by TypeScript at runtime: the + * project may be on an older `@cipherstash/stack`, on an adapter-built client, + * or on a hand-rolled stub. So the loader duck-types `getSchemas()` and then + * verifies the shape of what it hands back. + * + * These tests drive that guard through the public seam, with a real temp + * project and real jiti — the client bodies below are plain object literals so + * no package resolution is involved. + */ +describe('loadEncryptSchemas against an untrusted client', () => { + let tmpDir: string + let originalCwd: () => string + + /** A minimal encrypt config that clears `requireUsableEncryptConfig`. */ + const CONFIG = `{ + v: 1, + tables: { users: { email: { cast_as: 'string', indexes: {} } } }, + }` + + const writeProject = (clientBody: string) => { + fs.writeFileSync( + path.join(tmpDir, 'stash.config.ts'), + `export default { + databaseUrl: 'postgresql://u:p@127.0.0.1:5432/db', + client: './client.ts', + }`, + ) + fs.writeFileSync(path.join(tmpDir, 'client.ts'), clientBody) + process.cwd = () => tmpDir + } + + /** A client exporting `getEncryptConfig` plus whatever `getSchemas` returns. */ + const clientReturning = (getSchemasBody: string) => + `export const encryptionClient = { + getEncryptConfig: () => (${CONFIG}), + getSchemas: () => (${getSchemasBody}), + }` + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'stash-load-schemas-')) + originalCwd = process.cwd + }) + + afterEach(() => { + process.cwd = originalCwd + vi.restoreAllMocks() + if (tmpDir && fs.existsSync(tmpDir)) { + fs.rmSync(tmpDir, { recursive: true, force: true }) + } + }) + + it('accepts a well-formed table', async () => { + writeProject( + clientReturning(`[{ + tableName: 'users', + columnBuilders: { + email: { + getName: () => 'email', + getEqlType: () => 'public.eql_v3_text_eq', + isQueryable: () => true, + build: () => ({ cast_as: 'string', indexes: { unique: {} } }), + }, + }, + }]`), + ) + + const { schemas } = await loadEncryptSchemas('./client.ts') + + expect(schemas).toHaveLength(1) + expect(schemas?.[0].tableName).toBe('users') + }) + + it('degrades to config-only when the client predates getSchemas()', async () => { + writeProject( + `export const encryptionClient = { getEncryptConfig: () => (${CONFIG}) }`, + ) + + const { config, schemas } = await loadEncryptSchemas('./client.ts') + + expect(schemas).toBeUndefined() + expect(config.tables.users).toBeDefined() + }) + + /** + * `typeof null === 'object'`, so a `columnBuilders: null` slipped through the + * shape check and reached `Object.values(null)` in `collectDeclaredColumns`, + * which throws. The guard exists precisely so a malformed client degrades + * instead of crashing the command with a stack trace. + */ + it('rejects a table whose columnBuilders is null rather than crashing later', async () => { + writeProject( + clientReturning(`[{ tableName: 'users', columnBuilders: null }]`), + ) + + const { schemas } = await loadEncryptSchemas('./client.ts') + + expect(schemas).toBeUndefined() + }) + + /** + * The shape check has to cover the builders too: every one of `getName`, + * `getEqlType`, `isQueryable` and `build` is called while collecting columns, + * so a table carrying inert objects is as unusable as a null map. + */ + it('rejects a table whose builders do not implement the column API', async () => { + writeProject( + clientReturning(`[{ + tableName: 'users', + columnBuilders: { email: { getName: () => 'email' } }, + }]`), + ) + + const { schemas } = await loadEncryptSchemas('./client.ts') + + expect(schemas).toBeUndefined() + }) + + it('rejects a getSchemas() that does not return an array', async () => { + writeProject(clientReturning(`{ users: { tableName: 'users' } }`)) + + const { schemas } = await loadEncryptSchemas('./client.ts') + + expect(schemas).toBeUndefined() + }) +}) diff --git a/packages/cli/src/config/index.ts b/packages/cli/src/config/index.ts index 969744a2e..6c13a6d79 100644 --- a/packages/cli/src/config/index.ts +++ b/packages/cli/src/config/index.ts @@ -322,13 +322,38 @@ export async function loadEncryptSchemas( return { config, schemas } } -/** Structural check for the parts of an `EncryptedTable` the schema rules read. */ +/** + * Structural check for the parts of an `EncryptedTable` the schema rules read. + * + * Checks the BUILDERS too, not just the map that holds them. The rules call + * `build()`, `getName()`, `getEqlType()` and `isQueryable()` on every one, so a + * table carrying inert objects is as unusable as a missing map — and + * `typeof null === 'object'`, so a null `columnBuilders` would otherwise pass + * here and reach `Object.values(null)`, turning a degradable client into a + * stack trace. + */ function isV3TableLike(value: unknown): value is AnyV3Table { - return ( - !!value && - typeof value === 'object' && - typeof (value as { tableName?: unknown }).tableName === 'string' && - typeof (value as { columnBuilders?: unknown }).columnBuilders === 'object' + if (!value || typeof value !== 'object') return false + + const { tableName, columnBuilders } = value as { + tableName?: unknown + columnBuilders?: unknown + } + + if (typeof tableName !== 'string') return false + if (!columnBuilders || typeof columnBuilders !== 'object') return false + + return Object.values(columnBuilders).every(isV3ColumnLike) +} + +/** The column-builder methods `collectDeclaredColumns` calls on every column. */ +function isV3ColumnLike(value: unknown): boolean { + if (!value || typeof value !== 'object') return false + + const builder = value as Record + + return (['build', 'getName', 'getEqlType', 'isQueryable'] as const).every( + (method) => typeof builder[method] === 'function', ) } diff --git a/packages/stack/__tests__/client-get-schemas.test.ts b/packages/stack/__tests__/client-get-schemas.test.ts index 6122bae46..cdcfcc113 100644 --- a/packages/stack/__tests__/client-get-schemas.test.ts +++ b/packages/stack/__tests__/client-get-schemas.test.ts @@ -100,3 +100,26 @@ describe('EncryptionClient.getSchemas', () => { expect(second).toBe('public.eql_v3_double_ord') }) }) + +describe('the tuple getSchemas() hands back', () => { + /** + * The client derives its per-table reconstructor map ONCE, at construction, + * from this tuple. A consumer that mutated the array afterwards would leave + * `getSchemas()` describing a schema set the client does not actually + * reconstruct for. The readonly tuple type blocks that from TypeScript; + * freezing closes it for untyped callers too, at no runtime cost. + */ + it('is frozen, so it cannot drift from the client it describes', () => { + const users = encryptedTable('users', { email: types.TextEq('email') }) + const client = createEncryptionClient(nativeStub, users) + + expect(Object.isFrozen(client.getSchemas())).toBe(true) + }) + + it('is the same tuple on every call', () => { + const users = encryptedTable('users', { email: types.TextEq('email') }) + const client = createEncryptionClient(nativeStub, users) + + expect(client.getSchemas()).toBe(client.getSchemas()) + }) +}) diff --git a/packages/stack/src/encryption/client-v3.ts b/packages/stack/src/encryption/client-v3.ts index 74c8367c6..bfcc393b5 100644 --- a/packages/stack/src/encryption/client-v3.ts +++ b/packages/stack/src/encryption/client-v3.ts @@ -400,6 +400,11 @@ export function createEncryptionClient( reconstructors.set(table.tableName, rowReconstructor(table)) } + // Frozen once, here, rather than on every `getSchemas()` call. `schemas` is + // the rest parameter's own array, so this never freezes an array the caller + // still owns. + const frozenSchemas = Object.freeze(schemas) as S + // A table not among the schemas this client was initialized with has no // precomputed reconstructor. Return a Result failure rather than building one // inline, which could throw and reject the Result-shaped decrypt promise. @@ -563,7 +568,14 @@ export function createEncryptionClient( // parameter, so the caller's array literal keeps its per-table inference, // and `table` arguments read back off `getSchemas()` stay assignable to // `S[number]` (identity-keyed consumers therefore also keep working). - getSchemas: () => schemas, + // + // Frozen because `reconstructors` above was derived from this tuple once, + // at construction: a caller that pushed to or spliced the returned array + // would leave `getSchemas()` advertising a schema set the client does not + // reconstruct for. The readonly tuple type already blocks that from + // TypeScript; this closes it for callers that reach the client untyped — + // which is exactly how the CLI loads it, out of the user's node_modules. + getSchemas: () => frozenSchemas, } return typed diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index 4aa3c1de3..b3b3b7309 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -433,6 +433,8 @@ stash eql validate [--supabase] [--database-url ] Reads the tables passed to `Encryption({ schemas })` — through the client's `getSchemas()` accessor, so it sees the **concrete domain** of every column, which the built encrypt config does not carry — and checks them against the EQL v3 vocabulary. If a database is reachable it then checks the declaration against what that database actually has. +`getSchemas()` is a recent addition. Against a project on an older `@cipherstash/stack` validate says so and falls back to the built encrypt config: the index-derived rules still run, but every rule that needs a domain (ORE portability, drift, plain-column detection) is skipped. Upgrading `@cipherstash/stack` restores them. + Schema checks (no database needed): | Rule | Severity | @@ -448,7 +450,8 @@ Database checks (skipped with a notice, not a failure, when no database is reach | Rule | Severity | |---|---| | EQL v3 is not installed — reported once, and the remaining database checks are skipped | Error | -| A declared table or column does not exist in the database | Error | +| A declared table was not found in the searched schema | Warning | +| A declared column is missing from a table that was found | Error | | The database column's domain differs from the declared one | Error | | The database column is still plain (no EQL domain) | Error | | An `_ord_ore` domain on a database whose EQL install could not create the ORE operator class | Error | @@ -456,6 +459,8 @@ Database checks (skipped with a notice, not a failure, when no database is reach Exits 1 on errors only; warnings and info do not fail the command. +**Validate only inspects one schema** — `current_schema()`, the head of `search_path`. A table declared in your encryption schema but living elsewhere (Prisma `multiSchema`, a tenant schema, or a `schema.table` name passed to `encryptedTable`) is indistinguishable from a table whose migration never ran, so that finding is a Warning naming the schema searched rather than an Error. Point the connection at the right schema (`?options=-csearch_path%3D`) to check those tables. + **The `_ord_ore` finding is about portability.** `CREATE OPERATOR CLASS` requires superuser, so managed Postgres (Supabase and most hosted providers) installs EQL without the ORE btree operator class, and the bundle then poisons every `_ord_ore` domain with an always-raising CHECK. Prefer the `_ord` (OPE) twin unless you control the database role. With a database reachable, validate confirms which case you are in and upgrades the Warning to an Error when the operator class is genuinely absent. The "no functional index" Info applies only to term-carrying (queryable) columns — resolve it with the recipes in the `stash-indexing` skill. Storage-only columns (bare `types.T`, `types.Boolean`) have no index option by design, and `types.Json` is served by a GIN index over the column rather than a scalar extractor; neither is reported. From 4a8c385e30ef92070906a1732fed4cc32770d525 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 4 Aug 2026 13:43:18 +1000 Subject: [PATCH 5/7] fix(cli): tell a wrong search_path apart from an unapplied migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up review of 3bbc7ec0, which downgraded the absent-table finding to a Warning because validate could not distinguish the two situations behind it. It can — it just was not asking. `OTHER_SCHEMAS_SQL` looks each declared table name up across every schema, deliberately unscoped where the other reads are scoped to `current_schema()`. Found under another schema, the project is healthy and merely pointed at the wrong `search_path`, so the finding stays a Warning and now names the schema that has the table and the connection option to reach it. Found under none, the migration genuinely has not run, and that is an Error again — the previous commit had traded a false failure for a missed one, and CI stopped catching a forgotten migration. The finding is also reported once per table rather than once per column, which is the rule `validateSchemas` already applies to the not-installed case for the same reason: one fact explains every column, and repeating it per column buries it. A twenty-column table produced twenty identical paragraphs. The columns still run their schema rules; only their database rules are skipped. Two smaller things from the same pass. The `readObservedState` fake routed on `text.includes('current_schema')`, which also matches the index read, so that query was fed schema rows and the test passed only because `RegExp.exec(undefined)` matches nothing; it now routes on the result alias each query selects. And `Object.freeze(schemas) as S` asserted nothing — removing it produces no type error — while its comment implied a depth the freeze does not have, so the assertion is gone and the comment now says shallow, with a test pinning that the tables inside stay mutable. --- .changeset/proud-ravens-repeat.md | 15 +- .../commands/eql/__tests__/validate.test.ts | 168 ++++++++++++++---- packages/cli/src/commands/eql/validate.ts | 95 ++++++++-- .../__tests__/client-get-schemas.test.ts | 18 ++ packages/stack/src/encryption/client-v3.ts | 6 +- skills/stash-cli/SKILL.md | 5 +- 6 files changed, 245 insertions(+), 62 deletions(-) diff --git a/.changeset/proud-ravens-repeat.md b/.changeset/proud-ravens-repeat.md index 6b18f7d71..a106658eb 100644 --- a/.changeset/proud-ravens-repeat.md +++ b/.changeset/proud-ravens-repeat.md @@ -32,7 +32,8 @@ reachable): | Rule | Severity | |---|---| | EQL v3 is not installed — reported once, remaining database checks skipped | Error | -| A declared table was not found in the searched schema | Warning | +| A declared table lives in a different schema than the one searched | Warning | +| A declared table exists in no schema at all | Error | | A declared column is missing from a table that was found | Error | | The database column's domain differs from the declared one | Error | | The database column is still plain (no EQL domain) | Error | @@ -44,11 +45,13 @@ support `ORDER BY` without operator families, and the pinned EQL v3 bundle self-adapts. `eql install` / `eql upgrade` had already rejected the flag; `validate` was its last consumer. -The database pass inspects `current_schema()` only, so a declared table that -lives in another schema (Prisma `multiSchema`, a tenant schema, a -`schema.table` name) cannot be told apart from one whose migration never ran. -That finding is therefore a Warning naming the schema searched, not an Error — -a healthy project in a non-default schema does not fail the command. +The database pass inspects `current_schema()` only, but looks each declared +table name up across every schema before deciding what a miss means. Found +elsewhere — Prisma `multiSchema`, a tenant schema, a `schema.table` name — the +project is healthy and merely pointed at the wrong `search_path`, so that is a +Warning naming the schema that has it. Found nowhere, the migration has not +run, and that is an Error. Either way it is reported once per table rather +than once per column. Against a project whose `@cipherstash/stack` predates `getSchemas()`, validate says so and falls back to the encrypt config, running the index-derived rules diff --git a/packages/cli/src/commands/eql/__tests__/validate.test.ts b/packages/cli/src/commands/eql/__tests__/validate.test.ts index 8f0372490..f34ef1160 100644 --- a/packages/cli/src/commands/eql/__tests__/validate.test.ts +++ b/packages/cli/src/commands/eql/__tests__/validate.test.ts @@ -31,6 +31,7 @@ function observing( eqlInstalled: true, oreAvailable: true, searchedSchema: 'public', + elsewhere: new Map(), columns: new Map( Object.entries(columns).map(([table, cols]) => [ table, @@ -333,41 +334,94 @@ describe('database rules', () => { }) /** - * A table validate cannot see is NOT proof the table is missing. Both - * catalogue reads are scoped to one schema — `current_schema()`, the head of - * `search_path` — so a project whose tables live anywhere else (Prisma - * `multiSchema`, a tenant schema, a `schema.table` name in `encryptedTable`) - * looks identical to a project that never ran its migration. + * A table absent from `current_schema()` is TWO different situations, and + * `elsewhere` is what separates them. * - * Validate cannot tell those apart, and one of them is a perfectly healthy - * database, so this must not exit 1. It reports the schema it actually - * searched instead, which is the fact the user needs to tell which case they - * are in. The missing-COLUMN rule below stays an Error: there the table - * resolved, so the schema was right and the column really is absent. + * Found under another schema, the project is healthy and merely pointed at + * the wrong `search_path` (Prisma `multiSchema`, a tenant schema, a + * `schema.table` name in `encryptedTable` that the reader compares whole + * against a bare `table_name`). Found under none, the migration genuinely + * has not run. The first must not fail the command; the second must. */ - it('warns rather than errors when a table is absent, naming the schema searched', () => { + it('warns, naming the schema that has the table, when it is merely elsewhere', () => { + const observed = observing( + {}, + { + elsewhere: new Map([['users', ['app']]]), + indexedExtractors: fullyIndexed(columns), + }, + ) + + expect(validateSchemas(columns, observed)).toEqual([ + { + severity: 'warning', + table: 'users', + message: expect.stringContaining( + 'exists in schema "app", not in "public"', + ), + }, + ]) + }) + + it('errors when the table exists in no schema at all', () => { const observed = observing({}, { indexedExtractors: fullyIndexed(columns) }) - const issues = validateSchemas(columns, observed) - expect(issues).toHaveLength(2) - for (const issue of issues) { - expect(issue.severity).toBe('warning') - expect(issue.message).toMatch( - /Table "users" was not found in schema "public"/, - ) - expect(issue.message).toMatch(/search_path/) - } + expect(validateSchemas(columns, observed)).toEqual([ + { + severity: 'error', + table: 'users', + message: expect.stringContaining('does not exist in any schema'), + }, + ]) + }) + + /** + * The same reason `validateSchemas` collapses the EQL-not-installed finding: + * one fact explains every column, and repeating it per column buries it. A + * twenty-column table produced twenty identical paragraphs. + */ + it('reports an absent table once, not once per column', () => { + const wide = encryptedTable('wide', { + a: types.TextEq('a'), + b: types.TextEq('b'), + c: types.TextEq('c'), + }) + const observed = observing({}) + + const issues = validateSchemas(collectDeclaredColumns([wide]), observed) + + expect(issues).toHaveLength(1) + expect(issues[0].column).toBeUndefined() + }) + + it('still reports each absent table separately', () => { + const a = encryptedTable('a', { x: types.TextEq('x') }) + const b = encryptedTable('b', { y: types.TextEq('y') }) + + const issues = validateSchemas( + collectDeclaredColumns([a, b]), + observing({}), + ) + + expect(issues.map((issue) => issue.table)).toEqual(['a', 'b']) }) - it('names whichever schema was actually searched', () => { + it('names both schemas — the one searched and the one that has the table', () => { const observed = observing( {}, - { searchedSchema: 'tenant_7', indexedExtractors: fullyIndexed(columns) }, + { + searchedSchema: 'tenant_7', + elsewhere: new Map([['users', ['app']]]), + indexedExtractors: fullyIndexed(columns), + }, ) - expect(validateSchemas(columns, observed)[0]?.message).toContain( - 'schema "tenant_7"', - ) + const [issue] = validateSchemas(columns, observed) + + expect(issue.message).toContain('exists in schema "app"') + expect(issue.message).toContain('not in "tenant_7"') + // The remedy has to name the schema to point at, not a placeholder. + expect(issue.message).toContain('search_path%3Dapp') }) it('reports a declared column the table does not have', () => { @@ -701,18 +755,66 @@ describe('readObservedState', () => { expect(indexQuery?.values).toEqual([['users', 'orders']]) }) + /** + * Routed on the result ALIAS each query selects, not on a substring that + * several of them share: `current_schema()` also appears in the index read, + * so an `includes('current_schema')` fake fed schema rows to the index + * parser and only survived because `RegExp.exec(undefined)` matches nothing. + */ + const fakeClient = (rowsFor: Record) => + ({ + query: (text: string) => { + const alias = Object.keys(rowsFor).find((key) => text.includes(key)) + return Promise.resolve({ rows: alias ? rowsFor[alias] : [] }) + }, + }) as unknown as Parameters[0] + it('reports the schema it searched', async () => { + const observed = await readObservedState( + fakeClient({ 'AS searched_schema': [{ searched_schema: 'tenant_7' }] }), + ['users'], + ) + + expect(observed.searchedSchema).toBe('tenant_7') + }) + + it('falls back to public when current_schema() answers nothing', async () => { + const observed = await readObservedState(fakeClient({}), ['users']) + + expect(observed.searchedSchema).toBe('public') + }) + + /** + * The lookup that separates "your search_path is wrong" from "you never ran + * the migration". Without it both look identical and validate has to hedge. + */ + it('collects the other schemas a declared table lives in', async () => { + const observed = await readObservedState( + fakeClient({ + 'AS other_schema': [ + { table_name: 'users', other_schema: 'app' }, + { table_name: 'users', other_schema: 'archive' }, + ], + }), + ['users'], + ) + + expect(observed.elsewhere.get('users')).toEqual(['app', 'archive']) + }) + + it('asks for the other schemas of exactly the declared tables', async () => { + const queries: Array<{ text: string; values?: unknown[] }> = [] const client = { - query: (text: string) => - Promise.resolve({ - rows: text.includes('current_schema') - ? [{ searched_schema: 'tenant_7' }] - : [], - }), + query: (text: string, values?: unknown[]) => { + queries.push({ text, values }) + return Promise.resolve({ rows: [] }) + }, } as unknown as Parameters[0] - const observed = await readObservedState(client, ['users']) + await readObservedState(client, ['users', 'orders']) + + const lookup = queries.find((q) => q.text.includes('AS other_schema')) - expect(observed.searchedSchema).toBe('tenant_7') + expect(lookup?.values).toEqual([['users', 'orders']]) }) }) diff --git a/packages/cli/src/commands/eql/validate.ts b/packages/cli/src/commands/eql/validate.ts index 3b6460b0c..2f769397a 100644 --- a/packages/cli/src/commands/eql/validate.ts +++ b/packages/cli/src/commands/eql/validate.ts @@ -69,6 +69,14 @@ export interface ObservedState { * one, and validate has no way to tell that from a missing migration. */ searchedSchema: string + /** + * For each declared table NOT in {@link searchedSchema}, the other schemas a + * relation of that name does live in. This is what separates "your + * `search_path` points at the wrong schema" (recoverable, the table is right + * there) from "the migration that creates this table has not run" (a real + * failure). Empty for a table that exists nowhere. + */ + elsewhere: Map /** `table → column → domain_name` (null for a plain, non-domain type). */ columns: Map> /** @@ -216,6 +224,34 @@ export function validateSchemas( const dbState = eqlMissing ? undefined : observed + // Reported per TABLE, ahead of the column loop, for the same reason the + // not-installed finding is reported once: one fact explains every column on + // the table, and repeating it per column buries it under itself. The columns + // still run their schema rules below; only their database rules are skipped. + const unreachable = new Set() + if (dbState !== undefined) { + for (const table of new Set(columns.map((column) => column.table))) { + if (dbState.columns.has(table)) continue + + unreachable.add(table) + const others = dbState.elsewhere.get(table) ?? [] + + issues.push( + others.length > 0 + ? { + table, + severity: 'warning', + message: `Table "${table}" exists in schema ${others.map((schema) => `"${schema}"`).join(' / ')}, not in "${dbState.searchedSchema}" — validate only inspects current_schema() (the head of search_path), so nothing about this table could be checked. Point the connection at that schema to check it (e.g. ?options=-csearch_path%3D${others[0]}).`, + } + : { + table, + severity: 'error', + message: `Table "${table}" is declared in your encryption schema but does not exist in any schema of this database. The migration that creates it has not been applied.`, + }, + ) + } + } + for (const column of columns) { const at = { table: column.table, column: column.column } const domain = column.eqlType @@ -281,24 +317,11 @@ export function validateSchemas( if (dbState === undefined) continue - const observedColumns = dbState.columns.get(column.table) + // Already reported once, above, against the table rather than this column. + if (unreachable.has(column.table)) continue - // NOT an error. Both catalogue reads are scoped to one schema, so "absent - // from `current_schema()`" covers two very different situations: a - // migration that was never applied, and a perfectly healthy project whose - // tables live in another schema (Prisma `multiSchema`, a tenant schema, or - // a `schema.table` name in `encryptedTable`, which the reader compares - // whole against a bare `table_name` and never matches). Validate cannot - // distinguish them, so it reports where it looked and leaves the exit code - // alone rather than failing a healthy database's CI. - if (observedColumns === undefined) { - issues.push({ - ...at, - severity: 'warning', - message: `Table "${column.table}" was not found in schema "${dbState.searchedSchema}", so nothing about this column could be checked against the database. Validate only inspects current_schema() (the head of search_path) — if this table lives in another schema, point the connection at it (e.g. ?options=-csearch_path%3D); otherwise the migration that creates it has not been applied.`, - }) - continue - } + const observedColumns = dbState.columns.get(column.table) + if (observedColumns === undefined) continue if (!observedColumns.has(column.column)) { issues.push({ @@ -493,25 +516,59 @@ const INDEX_DEFS_SQL = ` /** The schema both catalogue reads are scoped to. Reported in not-found findings. */ const SEARCHED_SCHEMA_SQL = `SELECT current_schema() AS searched_schema` -/** Read everything the database rules need, in five catalogue queries. */ +/** + * Where else a declared table name lives. Deliberately NOT scoped to + * `current_schema()` — this is the query that tells a misconfigured + * `search_path` apart from an unapplied migration, and it can only do that by + * looking everywhere the other reads do not. + * + * `relkind IN ('r','p','v','m','f')` covers ordinary, partitioned, view, + * materialized-view and foreign relations: any of them can carry the encrypted + * columns, and a partitioned table in particular is an ordinary choice for the + * large tables this command is pointed at. + */ +const OTHER_SCHEMAS_SQL = ` + SELECT c.relname AS table_name, + n.nspname AS other_schema + FROM pg_catalog.pg_class c + JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace + WHERE c.relname = ANY($1::text[]) + AND c.relkind = ANY(ARRAY['r','p','v','m','f']) + AND n.nspname <> current_schema() + ORDER BY n.nspname` + +/** Read everything the database rules need, in six catalogue queries. */ export async function readObservedState( client: pg.ClientBase, tables: ReadonlyArray, ): Promise { - const [installed, ore, schema, columns, indexes] = await Promise.all([ + const [installed, ore, schema, others, columns, indexes] = await Promise.all([ client.query<{ eql_installed: boolean }>(EQL_INSTALLED_SQL), client.query<{ ore_available: boolean }>(ORE_AVAILABLE_SQL), client.query<{ searched_schema: string }>(SEARCHED_SCHEMA_SQL), + client.query<{ table_name: string; other_schema: string }>( + OTHER_SCHEMAS_SQL, + [tables], + ), fetchPhysicalColumns(client, tables), client.query<{ table_name: string; indexdef: string }>(INDEX_DEFS_SQL, [ tables, ]), ]) + const elsewhere = new Map() + for (const row of others.rows) { + elsewhere.set(row.table_name, [ + ...(elsewhere.get(row.table_name) ?? []), + row.other_schema, + ]) + } + return { eqlInstalled: installed.rows[0]?.eql_installed === true, oreAvailable: ore.rows[0]?.ore_available === true, searchedSchema: schema.rows[0]?.searched_schema ?? 'public', + elsewhere, columns, indexedExtractors: parseIndexedExtractors( indexes.rows.map((row) => ({ diff --git a/packages/stack/__tests__/client-get-schemas.test.ts b/packages/stack/__tests__/client-get-schemas.test.ts index cdcfcc113..b75e74fc1 100644 --- a/packages/stack/__tests__/client-get-schemas.test.ts +++ b/packages/stack/__tests__/client-get-schemas.test.ts @@ -123,3 +123,21 @@ describe('the tuple getSchemas() hands back', () => { expect(client.getSchemas()).toBe(client.getSchemas()) }) }) + +describe('what freezing the tuple does and does not cover', () => { + /** + * The freeze is shallow, and the comment on it should not imply otherwise: it + * stops the ARRAY changing shape, which is what would leave `getSchemas()` + * listing tables the reconstructor map was never built for. A table object + * inside it is the user's own and stays mutable — pinned here so the claim + * and the mechanism cannot drift apart. + */ + it('freezes the array without freezing the tables inside it', () => { + const users = encryptedTable('users', { email: types.TextEq('email') }) + const client = createEncryptionClient(nativeStub, users) + const [table] = client.getSchemas() + + expect(Object.isFrozen(client.getSchemas())).toBe(true) + expect(Object.isFrozen(table)).toBe(false) + }) +}) diff --git a/packages/stack/src/encryption/client-v3.ts b/packages/stack/src/encryption/client-v3.ts index bfcc393b5..9bbcaeb80 100644 --- a/packages/stack/src/encryption/client-v3.ts +++ b/packages/stack/src/encryption/client-v3.ts @@ -402,8 +402,10 @@ export function createEncryptionClient( // Frozen once, here, rather than on every `getSchemas()` call. `schemas` is // the rest parameter's own array, so this never freezes an array the caller - // still owns. - const frozenSchemas = Object.freeze(schemas) as S + // still owns. SHALLOW by design: it pins the tuple's SHAPE, which is what + // `reconstructors` above was derived from. The table objects inside are the + // caller's own and stay mutable. + const frozenSchemas = Object.freeze(schemas) // A table not among the schemas this client was initialized with has no // precomputed reconstructor. Return a Result failure rather than building one diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index b3b3b7309..ab27de790 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -450,7 +450,8 @@ Database checks (skipped with a notice, not a failure, when no database is reach | Rule | Severity | |---|---| | EQL v3 is not installed — reported once, and the remaining database checks are skipped | Error | -| A declared table was not found in the searched schema | Warning | +| A declared table lives in a different schema than the one searched | Warning | +| A declared table exists in no schema at all | Error | | A declared column is missing from a table that was found | Error | | The database column's domain differs from the declared one | Error | | The database column is still plain (no EQL domain) | Error | @@ -459,7 +460,7 @@ Database checks (skipped with a notice, not a failure, when no database is reach Exits 1 on errors only; warnings and info do not fail the command. -**Validate only inspects one schema** — `current_schema()`, the head of `search_path`. A table declared in your encryption schema but living elsewhere (Prisma `multiSchema`, a tenant schema, or a `schema.table` name passed to `encryptedTable`) is indistinguishable from a table whose migration never ran, so that finding is a Warning naming the schema searched rather than an Error. Point the connection at the right schema (`?options=-csearch_path%3D`) to check those tables. +**Validate inspects one schema** — `current_schema()`, the head of `search_path` — but looks the table name up across all of them before deciding what a miss means. Found under another schema (Prisma `multiSchema`, a tenant schema, or a `schema.table` name passed to `encryptedTable`), it is a Warning naming that schema and the connection option to reach it: the project is healthy, just pointed elsewhere. Found under none, the migration has not run, and that is an Error. Findings are reported once per table, not once per column. **The `_ord_ore` finding is about portability.** `CREATE OPERATOR CLASS` requires superuser, so managed Postgres (Supabase and most hosted providers) installs EQL without the ORE btree operator class, and the bundle then poisons every `_ord_ore` domain with an always-raising CHECK. Prefer the `_ord` (OPE) twin unless you control the database role. With a database reachable, validate confirms which case you are in and upgrades the Warning to an Error when the operator class is genuinely absent. From 427877184f0d86eaaf37df35baa59ab89605b95b Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Tue, 4 Aug 2026 16:49:02 +1000 Subject: [PATCH 6/7] fix: unbreak CI and resolve audited review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI has been red since this branch's first push, on every commit, and the PR's own verification section did not catch it: `packages/stack` splits `test` (`vitest run`) from `test:types` (`vitest --run --typecheck.only`), CI gates on the latter, and only the former was ever run. `getSchemas(): S` was the first member to put `S` in an output position, which made a readonly-vs-mutable tuple difference observable for the first time and broke `encryption-v3-only.test-d.ts`. The fix is `getSchemas(): Readonly` — the type the implementation already produces, since it returns `Object.freeze(schemas)`. Note `readonly [...S]` does NOT work: a tuple spread gives `S` a measurable variance, which lets the identity relation short-circuit to comparing type arguments, and `[T]` is not identical to `readonly [T]`. `Readonly` is homomorphic, leaves variance unmeasurable, and falls back to structural comparison. Both files carry a comment saying so, because the failure is invisible from the source. A privilege-invisible table was reported as an unapplied migration. `information_schema.columns` shows only what the connected role holds a privilege on; `pg_catalog.pg_class` is not privilege-filtered. The `pg_class` lookup added two commits ago excluded `current_schema()`, so a table present-but-invisible landed in neither map and hit the hard "does not exist in any schema" Error — telling someone to re-run a migration that had already run. The lookup now covers every schema and the miss resolves four ways: elsewhere, invisible-to-role (carrying the GRANT to run), schema -qualified, or genuinely absent. Only the last exits 1. Verified against a real Postgres cluster, including that the emitted GRANT fixes it. A `schema.table` name gets its own finding rather than a silent false "missing". Splitting the name was rejected deliberately: the only column reader is scoped to `current_schema()`, so `app.users` would have validated against `public.users` and reported an unrelated table's drift as this one's. An explicit "not checked" beats a confident wrong answer. A `getSchemas()` that throws now degrades to config-only like every other malformed-client path, instead of escaping as "Fatal error". The `tableName` guard gained the test two reviews graded low-value: dropping it does not merely put `undefined` in the output, it feeds `table: undefined` into the unreachable-table rule, which raises an error and exits 1 — the same contract violation by another route. Characterization coverage for the two seams that had none: `readObservedState`'s six-query positional mapping (proved non-vacuous by mutation — swapping two entries, and simulating the reader's swallowing catch) and `reportIssues`, whose prefix logic changed in this PR precisely because the rewrite introduced schema-wide and table-level issue shapes; reverting it to v2's form reproduces `undefined.undefined:`. Skill and changeset record the four-way split, the two findings that no longer exit 1, and the widened fallback trigger. --- .changeset/proud-ravens-repeat.md | 22 +- .../commands/eql/__tests__/validate.test.ts | 370 +++++++++++++++++- packages/cli/src/commands/eql/validate.ts | 187 +++++++-- .../__tests__/load-encrypt-schemas.test.ts | 40 ++ packages/cli/src/config/index.ts | 22 +- .../__tests__/encryption-v3-only.test-d.ts | 73 ++++ packages/stack/src/encryption/client-v3.ts | 38 +- skills/stash-cli/SKILL.md | 15 +- 8 files changed, 699 insertions(+), 68 deletions(-) diff --git a/.changeset/proud-ravens-repeat.md b/.changeset/proud-ravens-repeat.md index a106658eb..7ad249270 100644 --- a/.changeset/proud-ravens-repeat.md +++ b/.changeset/proud-ravens-repeat.md @@ -33,6 +33,8 @@ reachable): |---|---| | EQL v3 is not installed — reported once, remaining database checks skipped | Error | | A declared table lives in a different schema than the one searched | Warning | +| A declared table is in the searched schema but invisible to the connected role | Warning | +| A declared table name carries a schema qualifier (`schema.table`) — not checked | Warning | | A declared table exists in no schema at all | Error | | A declared column is missing from a table that was found | Error | | The database column's domain differs from the declared one | Error | @@ -45,13 +47,19 @@ support `ORDER BY` without operator families, and the pinned EQL v3 bundle self-adapts. `eql install` / `eql upgrade` had already rejected the flag; `validate` was its last consumer. -The database pass inspects `current_schema()` only, but looks each declared -table name up across every schema before deciding what a miss means. Found -elsewhere — Prisma `multiSchema`, a tenant schema, a `schema.table` name — the -project is healthy and merely pointed at the wrong `search_path`, so that is a -Warning naming the schema that has it. Found nowhere, the migration has not -run, and that is an Error. Either way it is reported once per table rather -than once per column. +The database pass inspects `current_schema()` only, and distinguishes four +reasons a declared table can be missing from it, so that only the last fails +the command. In another schema (Prisma `multiSchema`, a tenant schema): a +Warning naming that schema. Present but invisible to the connected role: a +Warning carrying the `GRANT SELECT` to run — `information_schema` reports only +what the role holds a privilege on, so a missing grant is not a missing +migration. Declared as `schema.table`: a Warning saying it was not checked, +because validate matches table names unqualified. Absent everywhere: an Error. +Reported once per table rather than once per column. + +Two of those used to exit 1 and no longer do: a privilege-invisible table and +a schema-qualified declaration were both reported as "does not exist in any +schema", which sent people to re-run a migration that had already run. Against a project whose `@cipherstash/stack` predates `getSchemas()`, validate says so and falls back to the encrypt config, running the index-derived rules diff --git a/packages/cli/src/commands/eql/__tests__/validate.test.ts b/packages/cli/src/commands/eql/__tests__/validate.test.ts index f34ef1160..780924a39 100644 --- a/packages/cli/src/commands/eql/__tests__/validate.test.ts +++ b/packages/cli/src/commands/eql/__tests__/validate.test.ts @@ -1,6 +1,6 @@ import { encryptedTable, types } from '@cipherstash/stack/eql/v3' import type { EncryptConfig } from '@cipherstash/stack/schema' -import { describe, expect, it } from 'vitest' +import { beforeEach, describe, expect, it, vi } from 'vitest' import { collectDeclaredColumns, collectDeclaredColumnsFromConfig, @@ -8,9 +8,35 @@ import { type ObservedState, parseIndexedExtractors, readObservedState, + reportIssues, + type ValidationIssue, validateSchemas, } from '../validate.js' +// clack is chrome — silence it and spy on the channels `reportIssues` prints +// through. Same shape as the mock in `repair.test.ts`; `spinner` and `intro` +// are here because the module under test imports the whole namespace. +const clack = vi.hoisted(() => ({ + spinnerInstance: { start: vi.fn(), stop: vi.fn() }, + log: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + step: vi.fn(), + }, + intro: vi.fn(), + note: vi.fn(), + outro: vi.fn(), +})) +vi.mock('@clack/prompts', () => ({ + spinner: vi.fn(() => clack.spinnerInstance), + log: clack.log, + intro: clack.intro, + note: clack.note, + outro: clack.outro, +})) + /** * Built from real `encryptedTable` / `types.*` schemas, not hand-written * encrypt configs. The rules key off the concrete domain, which only the real @@ -31,7 +57,12 @@ function observing( eqlInstalled: true, oreAvailable: true, searchedSchema: 'public', + connectedRole: 'app_rw', elsewhere: new Map(), + // A table whose columns `information_schema` reports is by definition one + // `pg_class` has too, so the default tracks the visible set. Overriding + // just this one is what produces the privilege-invisible case. + searchedSchemaRelations: new Set(Object.keys(columns)), columns: new Map( Object.entries(columns).map(([table, cols]) => [ table, @@ -334,14 +365,16 @@ describe('database rules', () => { }) /** - * A table absent from `current_schema()` is TWO different situations, and - * `elsewhere` is what separates them. + * A table absent from the privilege-filtered `columns` read is FOUR different + * situations, and only the last is a failure of the project's migrations: + * + * - declared `schema.table`, which neither read can match (its own finding); + * - present in the searched schema but invisible to the role (a grant); + * - present in another schema — Prisma `multiSchema`, a tenant schema — so + * the project is healthy and merely pointed at the wrong `search_path`; + * - present nowhere, so the migration genuinely has not run. * - * Found under another schema, the project is healthy and merely pointed at - * the wrong `search_path` (Prisma `multiSchema`, a tenant schema, a - * `schema.table` name in `encryptedTable` that the reader compares whole - * against a bare `table_name`). Found under none, the migration genuinely - * has not run. The first must not fail the command; the second must. + * Only the last may fail the command. This case is the third. */ it('warns, naming the schema that has the table, when it is merely elsewhere', () => { const observed = observing( @@ -375,6 +408,118 @@ describe('database rules', () => { ]) }) + /** + * The third state, and the one that produced a confidently-wrong Error. + * + * `columns` comes from `information_schema`, which PostgreSQL filters to + * objects the connected role has privileges on; `searchedSchemaRelations` + * comes from `pg_class`, which it does not filter. A table in the second and + * not the first exists, in the schema being searched, and the role simply + * cannot see it — so "the migration that creates it has not been applied" is + * a false statement that sends someone to re-run a migration that already + * ran. It is a grant, and this is not a failure of the schema. + */ + it('warns about privileges — not a missing migration — for a table the role cannot see', () => { + const observed = observing( + {}, + { + searchedSchemaRelations: new Set(['users']), + connectedRole: 'app_readonly', + }, + ) + + const issues = validateSchemas(columns, observed) + + expect(issues).toEqual([ + { + severity: 'warning', + table: 'users', + message: expect.stringContaining('not visible to the connected role'), + }, + ]) + expect(issues[0].message).toContain('"app_readonly"') + // The remedy is runnable, and names the role to grant to. + expect(issues[0].message).toContain( + 'GRANT SELECT ON "users" TO "app_readonly"', + ) + expect(issues[0].message).not.toContain('does not exist in any schema') + }) + + it('still names the schema it searched when the role is unknown', () => { + const observed = observing( + {}, + { + searchedSchema: 'tenant_7', + searchedSchemaRelations: new Set(['users']), + connectedRole: undefined, + }, + ) + + const [issue] = validateSchemas(columns, observed) + + expect(issue.severity).toBe('warning') + expect(issue.message).toContain('exists in schema "tenant_7"') + expect(issue.message).not.toContain('undefined') + }) + + /** + * A name can be both invisible here and visible in another schema. The + * privilege reading wins: the table the declaration means is the one in the + * searched schema, and telling someone to re-point `search_path` at an + * unrelated same-named relation is the wrong instruction. + */ + it('prefers the privilege reading over the wrong-search_path one', () => { + const observed = observing( + {}, + { + searchedSchemaRelations: new Set(['users']), + elsewhere: new Map([['users', ['archive']]]), + }, + ) + + const [issue] = validateSchemas(columns, observed) + + expect(issue.message).toContain('not visible to the connected role') + expect(issue.message).not.toContain('search_path%3D') + }) + + /** + * `@cipherstash/migrate` accepts `schema.table` (`splitTableName` in + * `packages/migrate/src/version.ts`), so the spelling reaches validate — but + * the literal `'app.users'` is compared whole against a bare + * `information_schema.columns.table_name`, matches nothing, and was reported + * as a missing table. Saying nothing could be checked is honest; saying the + * migration never ran is not. + */ + it('says a schema-qualified table name cannot be checked, rather than reporting it missing', () => { + const qualified = collectDeclaredColumns([ + encryptedTable('app.users', { email: types.TextEq('email') }), + ]) + + const issues = validateSchemas(qualified, observing({})) + + expect(issues).toEqual([ + { + severity: 'warning', + table: 'app.users', + message: expect.stringContaining('schema-qualified'), + }, + ]) + expect(issues[0].message).not.toContain('does not exist in any schema') + // The way out, named: drop the qualifier and point the connection there. + expect(issues[0].message).toContain('search_path%3Dapp') + }) + + it('says nothing about a qualified name when there is no database to check against', () => { + const qualified = collectDeclaredColumns([ + encryptedTable('app.users', { email: types.TextEq('email') }), + ]) + + // The finding describes what the DATABASE pass could not do, so with no + // database pass there is nothing to report. + expect(validateSchemas(qualified)).toEqual([]) + }) + /** * The same reason `validateSchemas` collapses the EQL-not-installed finding: * one fact explains every column, and repeating it per column buries it. A @@ -791,9 +936,17 @@ describe('readObservedState', () => { it('collects the other schemas a declared table lives in', async () => { const observed = await readObservedState( fakeClient({ - 'AS other_schema': [ - { table_name: 'users', other_schema: 'app' }, - { table_name: 'users', other_schema: 'archive' }, + 'AS relation_schema': [ + { + table_name: 'users', + relation_schema: 'app', + is_searched_schema: false, + }, + { + table_name: 'users', + relation_schema: 'archive', + is_searched_schema: false, + }, ], }), ['users'], @@ -802,7 +955,49 @@ describe('readObservedState', () => { expect(observed.elsewhere.get('users')).toEqual(['app', 'archive']) }) - it('asks for the other schemas of exactly the declared tables', async () => { + /** + * The relation lookup used to exclude `current_schema()` in SQL, so a table + * present there but invisible to the role appeared in NEITHER read and got + * "does not exist in any schema". It now returns the searched schema too, and + * the two destinations are what tell the three states apart. + */ + it('separates a relation in the searched schema from one merely elsewhere', async () => { + const observed = await readObservedState( + fakeClient({ + 'AS relation_schema': [ + { + table_name: 'users', + relation_schema: 'public', + is_searched_schema: true, + }, + { + table_name: 'orders', + relation_schema: 'archive', + is_searched_schema: false, + }, + ], + }), + ['users', 'orders'], + ) + + expect(observed.searchedSchemaRelations).toEqual(new Set(['users'])) + expect(observed.elsewhere).toEqual(new Map([['orders', ['archive']]])) + }) + + it('reads the role the privilege finding has to name', async () => { + const observed = await readObservedState( + fakeClient({ + 'AS searched_schema': [ + { searched_schema: 'public', connected_role: 'app_readonly' }, + ], + }), + ['users'], + ) + + expect(observed.connectedRole).toBe('app_readonly') + }) + + it('asks for the schemas of exactly the declared tables', async () => { const queries: Array<{ text: string; values?: unknown[] }> = [] const client = { query: (text: string, values?: unknown[]) => { @@ -813,8 +1008,157 @@ describe('readObservedState', () => { await readObservedState(client, ['users', 'orders']) - const lookup = queries.find((q) => q.text.includes('AS other_schema')) + const lookup = queries.find((q) => q.text.includes('AS relation_schema')) expect(lookup?.values).toEqual([['users', 'orders']]) }) + + /** + * The six queries go out through one `Promise.all` and come back positionally + * destructured, so a reordered array or a renamed result alias moves a field + * onto the wrong read and it silently takes its falsy default. Two of those + * defaults are loud-wrong on a healthy database: `eqlInstalled: false` prints + * "run `stash eql install`" and skips every database check, and an empty + * `columns` reports every declared table as never-migrated and exits 1. + * + * Nothing else covers it — `fetchPhysicalColumns` has no test of its own and + * swallows every exception into an empty map, so `columns` is asserted by + * CONTENT here. A presence-only assertion passes vacuously against that catch. + */ + it('maps each query onto the field it feeds', async () => { + const observed = await readObservedState( + fakeClient({ + 'AS eql_installed': [{ eql_installed: true }], + 'AS ore_available': [{ ore_available: true }], + 'AS searched_schema': [ + { searched_schema: 'tenant_7', connected_role: 'app_rw' }, + ], + 'AS relation_schema': [ + { + table_name: 'users', + relation_schema: 'tenant_7', + is_searched_schema: true, + }, + { + table_name: 'users', + relation_schema: 'archive', + is_searched_schema: false, + }, + ], + 'information_schema.columns': [ + { + table_name: 'users', + column_name: 'email', + domain_name: 'eql_v3_text_search', + }, + { table_name: 'users', column_name: 'id', domain_name: null }, + ], + 'AS indexdef': [ + { + table_name: 'users', + indexdef: + 'CREATE INDEX i ON tenant_7.users USING btree (eql_v3.eq_term(email))', + }, + ], + }), + ['users'], + ) + + expect(observed).toEqual({ + eqlInstalled: true, + oreAvailable: true, + searchedSchema: 'tenant_7', + connectedRole: 'app_rw', + elsewhere: new Map([['users', ['archive']]]), + searchedSchemaRelations: new Set(['users']), + columns: new Map([ + [ + 'users', + new Map([ + ['email', 'eql_v3_text_search'], + ['id', null], + ]), + ], + ]), + indexedExtractors: new Map([['users.email', new Set(['eq_term'])]]), + }) + }) +}) + +describe('reportIssues', () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + /** + * The three shapes this command produces. v2 only ever produced the third, + * and its formatter prefixed every line unconditionally — under that + * formatter the first two render as `undefined.undefined: EQL v3 is not + * installed` and `users.undefined: Table "users" exists in schema "app"`. + * The rules tests assert those two shapes exist; this is what asserts they + * reach a user intact. + */ + const issues: ValidationIssue[] = [ + { severity: 'error', message: 'EQL v3 is not installed' }, + { + severity: 'warning', + table: 'users', + message: 'Table "users" exists in schema "app"', + }, + { + severity: 'info', + table: 'users', + column: 'email', + message: 'Storage-only column', + }, + ] + + const printed = () => [ + ...clack.log.error.mock.calls, + ...clack.log.warn.mock.calls, + ...clack.log.info.mock.calls, + ] + + it('prints each severity through its own channel, prefixing only the column-level line', () => { + reportIssues(issues) + + expect(clack.log.error.mock.calls).toEqual([['EQL v3 is not installed']]) + expect(clack.log.warn.mock.calls).toEqual([ + ['Table "users" exists in schema "app"'], + ]) + expect(clack.log.info.mock.calls).toEqual([ + ['users.email: Storage-only column'], + ]) + }) + + it('never renders an absent table or column as "undefined"', () => { + reportIssues(issues) + + expect(printed()).not.toHaveLength(0) + for (const [line] of printed()) { + expect(line).not.toContain('undefined') + } + }) + + it('returns true — the exit-1 gate — and counts the outro when an error is present', () => { + expect(reportIssues(issues)).toBe(true) + expect(clack.outro).toHaveBeenCalledWith('1 error, 1 warning.') + }) + + it('returns false when nothing is an error', () => { + const noErrors = issues.filter((issue) => issue.severity !== 'error') + + expect(reportIssues(noErrors)).toBe(false) + expect(clack.outro).toHaveBeenCalledWith('No errors found. 1 warning.') + }) + + it('still says something when every finding is Info', () => { + expect(reportIssues(issues.slice(2))).toBe(false) + expect(clack.outro).toHaveBeenCalledWith('No errors or warnings. 1 info.') + }) + + it('says so when there is nothing to report', () => { + expect(reportIssues([])).toBe(false) + expect(clack.outro).toHaveBeenCalledWith('No issues found.') + }) }) diff --git a/packages/cli/src/commands/eql/validate.ts b/packages/cli/src/commands/eql/validate.ts index 2f769397a..e7e742c20 100644 --- a/packages/cli/src/commands/eql/validate.ts +++ b/packages/cli/src/commands/eql/validate.ts @@ -69,6 +69,12 @@ export interface ObservedState { * one, and validate has no way to tell that from a missing migration. */ searchedSchema: string + /** + * `current_user` — the role the connection authenticated as. Carried for the + * privilege finding, which is unactionable without naming the role to grant + * to. Absent only if the read itself returned nothing. + */ + connectedRole?: string /** * For each declared table NOT in {@link searchedSchema}, the other schemas a * relation of that name does live in. This is what separates "your @@ -77,7 +83,26 @@ export interface ObservedState { * failure). Empty for a table that exists nowhere. */ elsewhere: Map - /** `table → column → domain_name` (null for a plain, non-domain type). */ + /** + * Declared tables that `pg_catalog` reports IN {@link searchedSchema} — the + * third state, and the reason this field exists. + * + * {@link columns} comes from `information_schema`, which PostgreSQL filters + * to objects the connected role holds a privilege on; this comes from + * `pg_class`, which it does not filter. A table in here but absent from + * `columns` therefore exists, right where the declaration says, and the role + * simply cannot see it. Without the distinction that table appears in neither + * map and gets "does not exist in any schema of this database" — a false + * statement that sends someone to re-run a migration that already ran. + */ + searchedSchemaRelations: Set + /** + * `table → column → domain_name` (null for a plain, non-domain type). + * + * Privilege-filtered, per above: absence means "not visible to this role", + * which is only the same as "not there" once + * {@link searchedSchemaRelations} has been consulted. + */ columns: Map> /** * `table.column → extractor names` found inside functional index @@ -234,21 +259,7 @@ export function validateSchemas( if (dbState.columns.has(table)) continue unreachable.add(table) - const others = dbState.elsewhere.get(table) ?? [] - - issues.push( - others.length > 0 - ? { - table, - severity: 'warning', - message: `Table "${table}" exists in schema ${others.map((schema) => `"${schema}"`).join(' / ')}, not in "${dbState.searchedSchema}" — validate only inspects current_schema() (the head of search_path), so nothing about this table could be checked. Point the connection at that schema to check it (e.g. ?options=-csearch_path%3D${others[0]}).`, - } - : { - table, - severity: 'error', - message: `Table "${table}" is declared in your encryption schema but does not exist in any schema of this database. The migration that creates it has not been applied.`, - }, - ) + issues.push(unreachableTableIssue(table, dbState)) } } @@ -377,6 +388,73 @@ export function validateSchemas( return issues } +/** + * Why a declared table produced no columns from the database, in the order the + * evidence settles it. + * + * Four situations reach here and only ONE of them is a failure of the + * project's migrations. Reporting the others as "the migration has not been + * applied" is not a hedge but a false statement — it sends someone to re-run a + * migration that already ran, on a database where the table is right there. + */ +function unreachableTableIssue( + table: string, + observed: ObservedState, +): ValidationIssue { + // A `schema.table` name arrives as that literal string, and both catalogue + // reads compare it whole against a bare `relname` / `table_name` — so it + // matches nothing anywhere and was reported as an unapplied migration. + // `@cipherstash/migrate` does accept the spelling (`splitTableName` in + // `packages/migrate/src/version.ts`, first dot wins), so the toolchain + // disagrees with itself. Resolving it properly needs a schema-aware column + // read, which is `fetchPhysicalColumns` — shared with `encrypt status` and + // scoped to `current_schema()` by construction. Splitting the name here and + // passing the bare half to that reader would be worse than the bug: on a + // connection whose `search_path` is `public`, `app.users` would silently + // validate against `public.users` and report another table's drift as this + // one's. So: say what was not checked, and do not assert anything else. + const dot = table.indexOf('.') + if (dot >= 0) { + return { + table, + severity: 'warning', + message: `Table "${table}" is declared with a schema qualifier, and validate cannot check schema-qualified table names — it inspects current_schema() only and matches table names unqualified, so nothing about this table was checked. This is not a report that the table is missing. Declare it unqualified and point the connection at its schema to check it (e.g. ?options=-csearch_path%3D${table.slice(0, dot)}).`, + } + } + + // `columns` is read from `information_schema`, which PostgreSQL filters to + // objects the role holds a privilege on; `searchedSchemaRelations` is read + // from `pg_class`, which it does not. A table in the second and not the first + // exists exactly where the declaration says and is merely invisible. + // + // Checked ahead of `elsewhere` deliberately: the declaration means the table + // in the searched schema, so pointing `search_path` at a same-named relation + // in some other schema is the wrong instruction even when one exists. + if (observed.searchedSchemaRelations.has(table)) { + const role = observed.connectedRole + return { + table, + severity: 'warning', + message: `Table "${table}" exists in schema "${observed.searchedSchema}" but is not visible to the connected role${role ? ` "${role}"` : ''} — information_schema reports only what the role holds a privilege on, so nothing about this table could be checked. This is a missing grant, not a missing migration${role ? `: GRANT SELECT ON ${quoteIdent(table)} TO ${quoteIdent(role)};` : '.'}`, + } + } + + const others = observed.elsewhere.get(table) ?? [] + if (others.length > 0) { + return { + table, + severity: 'warning', + message: `Table "${table}" exists in schema ${others.map((schema) => `"${schema}"`).join(' / ')}, not in "${observed.searchedSchema}" — validate only inspects current_schema() (the head of search_path), so nothing about this table could be checked. Point the connection at that schema to check it (e.g. ?options=-csearch_path%3D${others[0]}).`, + } + } + + return { + table, + severity: 'error', + message: `Table "${table}" is declared in your encryption schema but does not exist in any schema of this database. The migration that creates it has not been applied.`, + } +} + // --------------------------------------------------------------------------- // Reading the database // --------------------------------------------------------------------------- @@ -513,28 +591,46 @@ const INDEX_DEFS_SQL = ` WHERE n.nspname = current_schema() AND c.relname = ANY($1::text[])` -/** The schema both catalogue reads are scoped to. Reported in not-found findings. */ -const SEARCHED_SCHEMA_SQL = `SELECT current_schema() AS searched_schema` +/** + * The schema both catalogue reads are scoped to, and the role they run as. + * Reported in the not-found findings — which schema was searched, and (for the + * privilege case) which role could not see what is in it. + */ +const SEARCHED_SCHEMA_SQL = ` + SELECT current_schema() AS searched_schema, + current_user AS connected_role` /** - * Where else a declared table name lives. Deliberately NOT scoped to - * `current_schema()` — this is the query that tells a misconfigured - * `search_path` apart from an unapplied migration, and it can only do that by - * looking everywhere the other reads do not. + * Every schema a declared table name lives in, INCLUDING `current_schema()`. + * + * Deliberately unscoped, and deliberately not excluding the searched schema + * either. This is the query that tells three situations apart, and it needs + * both halves to do it: a name found only in another schema is a misconfigured + * `search_path`, a name found in the searched schema but absent from the + * privilege-filtered `information_schema` read is a missing grant, and a name + * found nowhere is an unapplied migration. `pg_class` is not privilege + * filtered, which is precisely why the second case is visible here and nowhere + * else. + * + * The searched/elsewhere split is computed in SQL rather than by comparing + * `nspname` against the other read's `searched_schema` in JS: that read falls + * back to `'public'` when `current_schema()` is NULL, and a fallback compared + * against real catalogue rows would classify a genuine `public` relation as + * visible on a connection whose `search_path` resolves to nothing. * * `relkind IN ('r','p','v','m','f')` covers ordinary, partitioned, view, * materialized-view and foreign relations: any of them can carry the encrypted * columns, and a partitioned table in particular is an ordinary choice for the * large tables this command is pointed at. */ -const OTHER_SCHEMAS_SQL = ` +const TABLE_SCHEMAS_SQL = ` SELECT c.relname AS table_name, - n.nspname AS other_schema + n.nspname AS relation_schema, + n.nspname = current_schema() AS is_searched_schema FROM pg_catalog.pg_class c JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace WHERE c.relname = ANY($1::text[]) AND c.relkind = ANY(ARRAY['r','p','v','m','f']) - AND n.nspname <> current_schema() ORDER BY n.nspname` /** Read everything the database rules need, in six catalogue queries. */ @@ -542,25 +638,34 @@ export async function readObservedState( client: pg.ClientBase, tables: ReadonlyArray, ): Promise { - const [installed, ore, schema, others, columns, indexes] = await Promise.all([ - client.query<{ eql_installed: boolean }>(EQL_INSTALLED_SQL), - client.query<{ ore_available: boolean }>(ORE_AVAILABLE_SQL), - client.query<{ searched_schema: string }>(SEARCHED_SCHEMA_SQL), - client.query<{ table_name: string; other_schema: string }>( - OTHER_SCHEMAS_SQL, - [tables], - ), - fetchPhysicalColumns(client, tables), - client.query<{ table_name: string; indexdef: string }>(INDEX_DEFS_SQL, [ - tables, - ]), - ]) + const [installed, ore, schema, relations, columns, indexes] = + await Promise.all([ + client.query<{ eql_installed: boolean }>(EQL_INSTALLED_SQL), + client.query<{ ore_available: boolean }>(ORE_AVAILABLE_SQL), + client.query<{ searched_schema: string; connected_role: string }>( + SEARCHED_SCHEMA_SQL, + ), + client.query<{ + table_name: string + relation_schema: string + is_searched_schema: boolean | null + }>(TABLE_SCHEMAS_SQL, [tables]), + fetchPhysicalColumns(client, tables), + client.query<{ table_name: string; indexdef: string }>(INDEX_DEFS_SQL, [ + tables, + ]), + ]) const elsewhere = new Map() - for (const row of others.rows) { + const searchedSchemaRelations = new Set() + for (const row of relations.rows) { + if (row.is_searched_schema === true) { + searchedSchemaRelations.add(row.table_name) + continue + } elsewhere.set(row.table_name, [ ...(elsewhere.get(row.table_name) ?? []), - row.other_schema, + row.relation_schema, ]) } @@ -568,7 +673,9 @@ export async function readObservedState( eqlInstalled: installed.rows[0]?.eql_installed === true, oreAvailable: ore.rows[0]?.ore_available === true, searchedSchema: schema.rows[0]?.searched_schema ?? 'public', + connectedRole: schema.rows[0]?.connected_role, elsewhere, + searchedSchemaRelations, columns, indexedExtractors: parseIndexedExtractors( indexes.rows.map((row) => ({ diff --git a/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts b/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts index 117b17760..772167f63 100644 --- a/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts +++ b/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts @@ -123,6 +123,22 @@ describe('loadEncryptSchemas against an untrusted client', () => { expect(schemas).toBeUndefined() }) + /** + * A non-string `tableName` doesn't crash — it propagates as + * `DeclaredColumn.table === undefined`, which `validateSchemas` then reports + * as a table missing from the database and exits 1 on. So dropping this arm + * turns the documented degrade into a hard failure naming a table called + * "undefined", and nothing else covers it: the well-formed-table case above + * still passes without it. + */ + it('rejects a table whose tableName is not a string', async () => { + writeProject(clientReturning(`[{ columnBuilders: {} }]`)) + + const { schemas } = await loadEncryptSchemas('./client.ts') + + expect(schemas).toBeUndefined() + }) + it('rejects a getSchemas() that does not return an array', async () => { writeProject(clientReturning(`{ users: { tableName: 'users' } }`)) @@ -130,4 +146,28 @@ describe('loadEncryptSchemas against an untrusted client', () => { expect(schemas).toBeUndefined() }) + + /** + * The one untrusted behaviour the shape checks structurally cannot cover: a + * `getSchemas()` that THROWS fails before there is any value to validate. An + * adapter-built client, a hand-rolled stub, or a getter with a side effect + * can all do it. Left unguarded the throw escapes `loadEncryptSchemas` and + * `main.ts` rethrows it as "Fatal error", exiting 1 — the opposite of the + * degrade contract this function documents. + */ + it('degrades to config-only when getSchemas() throws', async () => { + writeProject( + `export const encryptionClient = { + getEncryptConfig: () => (${CONFIG}), + getSchemas: () => { + throw new Error('adapter client cannot enumerate schemas') + }, + }`, + ) + + const { config, schemas } = await loadEncryptSchemas('./client.ts') + + expect(schemas).toBeUndefined() + expect(config.tables.users).toBeDefined() + }) }) diff --git a/packages/cli/src/config/index.ts b/packages/cli/src/config/index.ts index 6c13a6d79..7531a9407 100644 --- a/packages/cli/src/config/index.ts +++ b/packages/cli/src/config/index.ts @@ -282,10 +282,11 @@ export interface LoadedEncryptSchemas { * `information_schema.columns.domain_name` — needs the tables themselves. * * `schemas` is `undefined` when the project's installed `@cipherstash/stack` - * predates `getSchemas()`. That is a real customer state (the CLI and the - * library version independently), so it degrades to config-only rather than - * failing: the caller runs the subset of rules the encrypt config can answer - * and says which ones it skipped. + * predates `getSchemas()` — and, equally, when its `getSchemas()` throws or + * hands back something that isn't a list of tables. That is a real customer + * state (the CLI and the library version independently), so it degrades to + * config-only rather than failing: the caller runs the subset of rules the + * encrypt config can answer and says which ones it skipped. * * Exits with code 1 through the same refusals as {@link loadEncryptConfig}. */ @@ -310,7 +311,18 @@ export async function loadEncryptSchemas( return { config, schemas: undefined } } - const schemas = getSchemas.call(encryptClient) + // The call itself is untrusted, not just its return value: an adapter-built + // client, a stub, or a getter with a side effect can throw here, and the + // shape check below never gets a value to reject. Unguarded that throw + // escapes to `main.ts`, which rethrows it as "Fatal error" and exits 1 — + // the opposite of the degrade this function promises. Only the call is + // inside the `try`, so a bug in `isV3TableLike` still surfaces. + let schemas: readonly AnyV3Table[] + try { + schemas = getSchemas.call(encryptClient) + } catch { + return { config, schemas: undefined } + } // A client built by an adapter (or a hand-rolled stub) could return // something other than an array of tables. Verify the shape rather than diff --git a/packages/stack/__tests__/encryption-v3-only.test-d.ts b/packages/stack/__tests__/encryption-v3-only.test-d.ts index c7d610e73..0f50d3751 100644 --- a/packages/stack/__tests__/encryption-v3-only.test-d.ts +++ b/packages/stack/__tests__/encryption-v3-only.test-d.ts @@ -8,6 +8,10 @@ const users = encryptedTable('users', { createdAt: types.TimestampOrd('created_at'), }) +const orders = encryptedTable('orders', { + total: types.NumericOrdOre('total'), +}) + describe('v3-only Encryption contract', () => { it('returns the schema-derived client for a literal tuple', async () => { const client = await Encryption({ schemas: [users] }) @@ -50,6 +54,75 @@ describe('v3-only Encryption contract', () => { }) }) +/** + * `getSchemas()` is the first member to put `S` in an OUTPUT position. Every + * earlier use was `S[number]` or a constraint, so the readonly-vs-mutable + * distinction on the schema tuple was unobservable and + * `EncryptionClient<[T]>` and `EncryptionClient` were literally + * the same type. Both spellings arise from ordinary calls — the factory's + * `const S` infers the READONLY tuple from an array literal, while a caller + * that is itself generic over its schemas (`make` above) infers the MUTABLE + * one — and they must keep naming one client type, or the two call styles + * silently produce incompatible clients. + * + * `getSchemas(): Readonly` is what holds that, and it is also the only + * honest return type: the accessor hands back `Object.freeze(schemas)`, whose + * type is exactly `Readonly`. + * + * The first test below looks tautological and is not. `Readonly` is a + * homomorphic mapped type, which leaves `S`'s variance UNMEASURABLE, so the + * identity relation compares the two clients structurally and they agree. The + * equivalent-looking `readonly [...S]` makes the variance measurable, the + * relation short-circuits to comparing type arguments, and `[T]` is not + * identical to `readonly [T]` — so the two client types come apart even though + * every member still resolves the same. That failure mode is invisible from + * the source; this test is where it shows up. + */ +declare const mutableTupleClient: EncryptionClient< + [typeof users, typeof orders] +> + +describe('EncryptionClient.getSchemas', () => { + it('does not distinguish a mutable schema tuple from a readonly one', () => { + expectTypeOf< + EncryptionClient<[typeof users, typeof orders]> + >().toEqualTypeOf< + EncryptionClient + >() + }) + + it('returns a readonly tuple even when S itself is mutable', () => { + expectTypeOf(mutableTupleClient.getSchemas()).toEqualTypeOf< + readonly [typeof users, typeof orders] + >() + }) + + it('keeps each table precise rather than collapsing to AnyV3Table', async () => { + const client = await Encryption({ schemas: [users, orders] }) + + expectTypeOf(client.getSchemas()).toEqualTypeOf< + readonly [typeof users, typeof orders] + >() + // Positional and per-table. `stash eql validate` reads `getEqlType()` off + // each column, so an element type widened to `AnyV3Table` — or a tuple + // flattened to `readonly AnyV3Table[]` — would take the domain away. + expectTypeOf(client.getSchemas()[0]).toEqualTypeOf() + expectTypeOf(client.getSchemas()[1]).toEqualTypeOf() + expectTypeOf( + client.getSchemas()[0].columnBuilders.email.getEqlType(), + ).toEqualTypeOf<'public.eql_v3_text_eq'>() + }) + + it('rejects mutation of the tuple it hands back', async () => { + const client = await Encryption({ schemas: [users] }) + + // @ts-expect-error - the accessor returns the frozen tuple; mutating it + // would leave getSchemas() describing a schema set the client's + // reconstructor map was never built for. + client.getSchemas().push(orders) + }) +}) + /** * The exported config type must not launder an empty schema set — the same guard * `wasm-inline-schemas.test-d.ts` holds for `WasmEncryptionConfig`. diff --git a/packages/stack/src/encryption/client-v3.ts b/packages/stack/src/encryption/client-v3.ts index 9bbcaeb80..46ce46da3 100644 --- a/packages/stack/src/encryption/client-v3.ts +++ b/packages/stack/src/encryption/client-v3.ts @@ -328,8 +328,41 @@ export interface EncryptionClient< * } * } * ``` + * + * Returns `Readonly` rather than `S`, and the spelling is load-bearing on + * two counts. + * + * It is the honest type. The accessor hands back `Object.freeze(schemas)`, + * whose type is exactly `Readonly` — frozen because the per-table + * reconstructor map was derived from that tuple once, at construction (see + * `createEncryptionClient`). Nothing is cast to make the two line up. + * + * It also keeps `S`'s own mutability unobservable, which matters because + * this is the only member putting `S` in an OUTPUT position — everywhere + * else `S` appears as `S[number]` or as a constraint, both blind to + * `readonly`. Both spellings of the tuple arise from ordinary calls: the + * `const` type parameter on `Encryption` infers `readonly [T]` from an array + * literal, while a caller generic over its own schemas + * (`function make(s: S) { + * return Encryption({ schemas: s }) }`) infers the mutable `[T]`. Returning + * `S` raw made those two call styles produce client types that no longer + * matched, over a distinction the client cannot express. + * + * Keep the homomorphic mapped type; do NOT "simplify" it to the equivalent- + * looking `readonly [...S]`. A tuple spread gives TypeScript a MEASURABLE + * (covariant) variance for `S`, which lets the identity relation short- + * circuit to comparing type arguments — and `[T]` is not identical to + * `readonly [T]`, so `EncryptionClient<[T]>` and + * `EncryptionClient` stop being the same type even though + * every member resolves the same. `Readonly` leaves the variance + * unmeasurable, so the relation falls back to structural comparison and the + * two agree. `encryption-v3-only.test-d.ts` pins this. + * + * Normalizing costs no precision: `Readonly<[A, B]>` is `readonly [A, B]`, + * element for element, so `getSchemas()[0]` stays the exact table type that + * `stash eql validate` reads `getEqlType()` off. */ - getSchemas(): S + getSchemas(): Readonly } /** @@ -577,6 +610,9 @@ export function createEncryptionClient( // reconstruct for. The readonly tuple type already blocks that from // TypeScript; this closes it for callers that reach the client untyped — // which is exactly how the CLI loads it, out of the user's node_modules. + // + // No cast: `Object.freeze(schemas)` is typed `Readonly`, which is + // verbatim what the interface declares `getSchemas()` to return. getSchemas: () => frozenSchemas, } diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index ab27de790..d6ce2a71c 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -433,7 +433,7 @@ stash eql validate [--supabase] [--database-url ] Reads the tables passed to `Encryption({ schemas })` — through the client's `getSchemas()` accessor, so it sees the **concrete domain** of every column, which the built encrypt config does not carry — and checks them against the EQL v3 vocabulary. If a database is reachable it then checks the declaration against what that database actually has. -`getSchemas()` is a recent addition. Against a project on an older `@cipherstash/stack` validate says so and falls back to the built encrypt config: the index-derived rules still run, but every rule that needs a domain (ORE portability, drift, plain-column detection) is skipped. Upgrading `@cipherstash/stack` restores them. +`getSchemas()` is a recent addition. Against a project on an older `@cipherstash/stack` — or a client whose `getSchemas()` is missing, malformed, or throws — validate says so and falls back to the built encrypt config: the index-derived rules still run, but the rules that need a domain (ORE portability and drift) are skipped. Upgrading `@cipherstash/stack` restores them. Schema checks (no database needed): @@ -451,6 +451,8 @@ Database checks (skipped with a notice, not a failure, when no database is reach |---|---| | EQL v3 is not installed — reported once, and the remaining database checks are skipped | Error | | A declared table lives in a different schema than the one searched | Warning | +| A declared table is in the searched schema but invisible to the connected role (missing grant) | Warning | +| A declared table name carries a schema qualifier (`schema.table`) — not checked | Warning | | A declared table exists in no schema at all | Error | | A declared column is missing from a table that was found | Error | | The database column's domain differs from the declared one | Error | @@ -460,7 +462,16 @@ Database checks (skipped with a notice, not a failure, when no database is reach Exits 1 on errors only; warnings and info do not fail the command. -**Validate inspects one schema** — `current_schema()`, the head of `search_path` — but looks the table name up across all of them before deciding what a miss means. Found under another schema (Prisma `multiSchema`, a tenant schema, or a `schema.table` name passed to `encryptedTable`), it is a Warning naming that schema and the connection option to reach it: the project is healthy, just pointed elsewhere. Found under none, the migration has not run, and that is an Error. Findings are reported once per table, not once per column. +**Validate inspects one schema** — `current_schema()`, the head of `search_path` — but distinguishes four reasons a table can be missing from it, so only the last fails the command. Reported once per table, not once per column. + +| The table is… | Finding | +|---|---| +| in another schema (Prisma `multiSchema`, a tenant schema) | Warning naming that schema and the connection option to reach it | +| in the searched schema but invisible to the connected role | Warning with the `GRANT SELECT` to run — `information_schema` reports only what the role holds a privilege on, so a missing grant is not a missing migration | +| declared as `schema.table` | Warning — validate matches table names unqualified and cannot check this. Declare it unqualified and point the connection at its schema | +| absent everywhere | Error — the migration has not been applied | + +The `schema.table` case is deliberately not resolved by splitting the name: the only column reader is scoped to `current_schema()`, so `app.users` would silently validate against `public.users` and report an unrelated table's drift as this one's. An explicit "not checked" beats a confident wrong answer. **The `_ord_ore` finding is about portability.** `CREATE OPERATOR CLASS` requires superuser, so managed Postgres (Supabase and most hosted providers) installs EQL without the ORE btree operator class, and the bundle then poisons every `_ord_ore` domain with an always-raising CHECK. Prefer the `_ord` (OPE) twin unless you control the database role. With a database reachable, validate confirms which case you are in and upgrades the Warning to an Error when the operator class is genuinely absent. From 098fac7b981d4608c35389665f2a5ff74d4adde8 Mon Sep 17 00:00:00 2001 From: Toby Hede Date: Wed, 5 Aug 2026 08:56:44 +1000 Subject: [PATCH 7/7] fix(cli): stop information_schema masking an unapplied migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `TABLE_SCHEMAS_SQL` scanned every schema in the database. `information_schema` publishes views named `columns`, `domains`, `parameters`, `routines`, `sequences`, `tables` and `triggers` — all ordinary application table names — so a project declaring one of them that had not run its migration matched the system view, took the `elsewhere` branch of `unreachableTableIssue`, and was told to point `search_path` at `information_schema`. Being a Warning rather than an Error, the command then exited 0 on a genuinely unapplied migration. Excluded `pg_*` and `information_schema`, pinned by a regression test. The predicates use `!~ '^pg_'`, not `NOT LIKE 'pg\_%'`: the SQL is a JS template literal, which collapses `\_` to a bare `_` and leaves a LIKE wildcard that also swallows `pgbouncer` and `pgsodium`. Report which relation a bare table name resolved to when it is not unique. Both column readers match unqualified names against `current_schema()`, so a `users` in both `public` and Supabase's `auth` left every domain, plain-column and index finding describing whichever one `search_path` happened to pick, with nothing said about the choice. This is the unqualified twin of the `schema.table` collision `unreachableTableIssue` already refuses to guess at. Info, not Warning: it must not move the exit code or report an ordinary Supabase project as unclean, and unlike the not-found cases it did check a table — it is qualifying which, not reporting that nothing happened. `ObservedState.elsewhere`'s doc comment claimed it held only tables absent from the searched schema. It never did — it is populated per catalogue row — and the new rule reads exactly that overlap, so the comment now says so and warns against "tidying" it back. Close the review test gaps: - `validate-command.test.ts` (new, 13 tests) covers `validateCommand` and `tryReadObservedState`: the no-database-URL notice, the connect-error catch, the catalogue-read failure, the degraded-`getSchemas()` warning, count pluralisation, and the exit-code contract in both directions. Separate file because `vi.mock` is hoisted and file-wide — folding the loader, `pg` and `process.exit` stubs into the pure rule suite would apply them to every test there. - The two domain-less database branches now have tests that feed `collectDeclaredColumnsFromConfig` output with an `ObservedState` — the old-`@cipherstash/stack` plus reachable-database combination the degrade was written for. Each asserts the whole message, since the existing `stringContaining` assertions match both renderings of the ternary. - `load-encrypt-schemas.test.ts` splits the `tableName` guard into its two distinct inputs; the existing test was named for a non-string `tableName` but fed a fixture that omitted it. - `validate.test.ts` no longer contributes a `tsc` error: the `ste_vec` fixture was missing its required `prefix`, putting the package one over the budget recorded in `.github/workflows/tests.yml`. Skill, README and changeset carry the new Info rule; `skills/stash-cli` also gains the unqualified-name paragraph alongside the `schema.table` one. Verified: `pnpm --filter stash test` 1070 passed / 10 skipped; `code:check` clean; `tsc` back to the recorded 21 errors, none in changed files. --- .changeset/proud-ravens-repeat.md | 15 + packages/cli/README.md | 1 + .../eql/__tests__/validate-command.test.ts | 487 ++++++++++++++++++ .../commands/eql/__tests__/validate.test.ts | 256 ++++++++- packages/cli/src/commands/eql/validate.ts | 81 ++- .../__tests__/load-encrypt-schemas.test.ts | 18 +- skills/stash-cli/SKILL.md | 3 + 7 files changed, 846 insertions(+), 15 deletions(-) create mode 100644 packages/cli/src/commands/eql/__tests__/validate-command.test.ts diff --git a/.changeset/proud-ravens-repeat.md b/.changeset/proud-ravens-repeat.md index 7ad249270..86f5fc39d 100644 --- a/.changeset/proud-ravens-repeat.md +++ b/.changeset/proud-ravens-repeat.md @@ -41,6 +41,7 @@ reachable): | The database column is still plain (no EQL domain) | Error | | An `_ord_ore` domain where the EQL install could not create the ORE operator class | Error | | A queryable column with no functional index over its term extractor | Info | +| A declared table name that resolved in the searched schema also exists in another one | Info | `--exclude-operator-family` is removed: it warned that an `ore` index would not support `ORDER BY` without operator families, and the pinned EQL v3 bundle @@ -57,6 +58,20 @@ migration. Declared as `schema.table`: a Warning saying it was not checked, because validate matches table names unqualified. Absent everywhere: an Error. Reported once per table rather than once per column. +The relation lookup that answers those questions excludes `pg_*` and +`information_schema`. Unscoped it matched the system views named `columns`, +`domains`, `parameters`, `routines`, `sequences`, `tables` and `triggers` — all +ordinary application table names — so a project declaring one of them that had +not run its migration was told the table "exists in schema information_schema", +as a Warning, and the command exited 0 on a genuinely unapplied migration. + +An unqualified name found in more than one schema is now reported as an Info +naming the relation that was actually checked (`"public"."users"`) and the +other schemas holding that name. A bare name resolves through `search_path`, so +`users` in both `public` and Supabase's `auth` left it ambiguous which relation +every other finding described. Info, not Warning: it must not fail or +de-clean an ordinary Supabase project. + Two of those used to exit 1 and no longer do: a privilege-invisible table and a schema-qualified declaration were both reported as "does not exist in any schema", which sent people to re-run a migration that had already run. diff --git a/packages/cli/README.md b/packages/cli/README.md index f1808ccd4..34d8b63ca 100644 --- a/packages/cli/README.md +++ b/packages/cli/README.md @@ -243,6 +243,7 @@ Database checks (skipped with a notice when no database is reachable): | The column is still plain (no EQL domain) | Error | | An `_ord_ore` domain on a database whose EQL install could not create the ORE operator class | Error | | Queryable column with no functional index over its term extractor | Info | +| A declared table name that resolved in the searched schema also exists in another one | Info | The command exits with code 1 on errors (not on warnings or info). diff --git a/packages/cli/src/commands/eql/__tests__/validate-command.test.ts b/packages/cli/src/commands/eql/__tests__/validate-command.test.ts new file mode 100644 index 000000000..a23bc9557 --- /dev/null +++ b/packages/cli/src/commands/eql/__tests__/validate-command.test.ts @@ -0,0 +1,487 @@ +/** + * `validateCommand` / `tryReadObservedState` — the top-level orchestration of + * `stash eql validate`, which the rule suite next door cannot reach. + * + * A SEPARATE file from `validate.test.ts` on purpose. That one is a pure suite: + * it calls the exported rule functions directly and mocks nothing but clack. + * This one has to replace the config loader, the encryption-client loader and + * the `pg` driver, and intercept `process.exit`. `vi.mock` is hoisted and + * file-wide, so folding these in would silently apply them to every pure test + * over there — a loader stub those rules never asked for, and a `process.exit` + * that throws instead of exiting. Keeping the mocked half here is what keeps + * the pure half readable as pure. + * + * The e2e smoke test covers `eql validate` only as far as its config load + * failing ("Could not find stash.config.ts"), so everything past + * `loadStashConfig` — the degraded-`getSchemas()` warning, the no-database + * notice, the connect-error catch, and the exit-code contract — is asserted + * here or nowhere. + */ + +import type { AnyV3Table } from '@cipherstash/stack/eql/v3' +import { encryptedTable, types } from '@cipherstash/stack/eql/v3' +import type { EncryptConfig } from '@cipherstash/stack/schema' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { validateCommand } from '../validate.js' + +// clack is chrome — silence it and spy on the channels the command reports +// through. Same shape as the mock in `repair.test.ts` and `validate.test.ts`. +const clack = vi.hoisted(() => ({ + spinnerInstance: { start: vi.fn(), stop: vi.fn() }, + log: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + step: vi.fn(), + }, + intro: vi.fn(), + note: vi.fn(), + outro: vi.fn(), +})) +vi.mock('@clack/prompts', () => ({ + spinner: vi.fn(() => clack.spinnerInstance), + log: clack.log, + intro: clack.intro, + note: clack.note, + outro: clack.outro, +})) + +/** + * The command's two inputs, both of which normally come off the user's disk + * through jiti. Faking them is what lets the orchestration be driven through + * states no fixture project can reach on demand: an installed + * `@cipherstash/stack` that predates `getSchemas()`, or a config whose + * `databaseUrl` resolved to nothing. + */ +const loaders = vi.hoisted(() => ({ + loadStashConfig: vi.fn(), + loadEncryptSchemas: vi.fn(), +})) +vi.mock('@/config/index.js', () => ({ + loadStashConfig: loaders.loadStashConfig, + loadEncryptSchemas: loaders.loadEncryptSchemas, +})) + +// Fake the DRIVER, not `readObservedState` — the real catalogue SQL, the real +// six-way `Promise.all`, and the real `fetchPhysicalColumns` all stay under +// test, which is precisely the wiring at issue here. +const pgMock = vi.hoisted(() => ({ + connect: vi.fn(), + query: vi.fn(), + end: vi.fn(), + connectionStrings: [] as (string | undefined)[], +})) +vi.mock('pg', () => ({ + default: { + Client: vi.fn((config: { connectionString?: string }) => { + pgMock.connectionStrings.push(config?.connectionString) + return { connect: pgMock.connect, query: pgMock.query, end: pgMock.end } + }), + }, +})) + +/** + * `process.exit` is intercepted by THROWING, not by returning a stub value. + * The real call never returns, so a stub that does would let whatever follows + * it run under assertions written for a process that had stopped. + * `validateCommand` has no `catch`, so the sentinel unwinds straight out to + * the test — and `rejects.toBeInstanceOf(ProcessExited)` then asserts the + * command really did terminate there rather than merely log. + */ +class ProcessExited extends Error { + constructor(readonly code: number | string | null | undefined) { + super(`process.exit(${String(code)})`) + } +} +const exitSpy = vi.spyOn(process, 'exit').mockImplementation((code): never => { + throw new ProcessExited(code) +}) + +/** The blank line the command prints to separate the header from the report. */ +const consoleLog = vi.spyOn(console, 'log').mockImplementation(() => {}) + +const CLIENT = './src/encryption/index.ts' +const DATABASE_URL = 'postgres://user:pw@localhost:5432/app' + +/** The `EncryptConfig` shape the degraded, config-only path reads. */ +const configWith = (columns: EncryptConfig['tables'][string]): EncryptConfig => + ({ v: 1, tables: { t: columns } }) as EncryptConfig + +const EMPTY_CONFIG = { v: 1, tables: {} } as EncryptConfig + +/** + * Stage the command's inputs. + * + * `databaseUrl` defaults to empty, i.e. offline: that is the state most of + * these tests want, and a test that needs a database has to say so — which + * also means no test reaches a real socket by forgetting to. + * + * Leaving `schemas` unset is not "no tables"; it is the degraded path, exactly + * as `loadEncryptSchemas` reports a client with no `getSchemas()`. + */ +function given(input: { + schemas?: readonly AnyV3Table[] + config?: EncryptConfig + databaseUrl?: string +}): void { + loaders.loadStashConfig.mockResolvedValue({ + client: CLIENT, + databaseUrl: input.databaseUrl ?? '', + }) + loaders.loadEncryptSchemas.mockResolvedValue({ + config: input.config ?? EMPTY_CONFIG, + schemas: input.schemas, + }) +} + +/** + * Answer each catalogue read by the result ALIAS it selects, the way + * `validate.test.ts` routes its own fake client. Not by table name: three of + * the six reads mention `current_schema()`, so a looser match feeds one read's + * rows to another read's parser and the mistake is invisible (an unexpected + * shape simply parses to nothing). + */ +function respondWith(rowsByAlias: Record): void { + pgMock.query.mockImplementation((text: string) => { + const alias = Object.keys(rowsByAlias).find((key) => text.includes(key)) + return Promise.resolve({ rows: alias ? rowsByAlias[alias] : [] }) + }) +} + +/** A database in which everything the schema declares is present and correct. */ +const healthyDatabase = ( + columns: Array<{ + table_name: string + column_name: string + domain_name: string | null + }>, +) => ({ + 'AS eql_installed': [{ eql_installed: true }], + 'AS ore_available': [{ ore_available: true }], + 'AS searched_schema': [ + { searched_schema: 'public', connected_role: 'app_rw' }, + ], + 'AS relation_schema': [ + ...new Set(columns.map((column) => column.table_name)), + ].map((table_name) => ({ + table_name, + relation_schema: 'public', + is_searched_schema: true, + })), + 'information_schema.columns': columns, + pg_get_indexdef: [], +}) + +beforeEach(() => { + vi.clearAllMocks() + // Implementations, unlike call records, survive `clearAllMocks` — reset the + // driver doubles so one test's routing table cannot answer the next one's + // queries. + pgMock.query.mockReset().mockResolvedValue({ rows: [] }) + pgMock.connect.mockReset().mockResolvedValue(undefined) + pgMock.end.mockReset().mockResolvedValue(undefined) + pgMock.connectionStrings.length = 0 +}) + +describe('validateCommand — loading', () => { + it('threads the CLI flags into the config load and loads the declared client', async () => { + given({ + schemas: [encryptedTable('users', { email: types.TextSearch('email') })], + }) + + await validateCommand({ databaseUrl: 'postgres://flag', supabase: true }) + + // Both flags exist to reach the user's own `resolveDatabaseUrl()` during + // config evaluation; dropping either here is silent, because the command + // still works against whatever `DATABASE_URL` happens to be exported. + expect(loaders.loadStashConfig).toHaveBeenCalledWith({ + databaseUrlFlag: 'postgres://flag', + supabase: true, + }) + expect(loaders.loadEncryptSchemas).toHaveBeenCalledWith(CLIENT) + }) + + it.each([ + { + name: 'pluralises the table and column counts', + schemas: [ + encryptedTable('users', { + email: types.TextSearch('email'), + age: types.IntegerOrd('age'), + }), + encryptedTable('orders', { total: types.IntegerOrd('total') }), + ], + expected: 'Schema loaded: 2 tables, 3 encrypted columns', + }, + { + name: 'keeps both counts singular for one column on one table', + schemas: [encryptedTable('users', { email: types.TextSearch('email') })], + expected: 'Schema loaded: 1 table, 1 encrypted column', + }, + ])('$name', async ({ schemas, expected }) => { + given({ schemas }) + + await validateCommand({}) + + expect(clack.log.success).toHaveBeenCalledWith(expected) + }) + + /** + * The CLI and the project's `@cipherstash/stack` version independently, so a + * client with no `getSchemas()` is a real customer state. It must degrade to + * the config-only rules and SAY which rules that cost — silently running a + * subset would report a clean bill of health for checks that never ran. + */ + it('warns that the domain checks were skipped when getSchemas() is unavailable', async () => { + given({ + config: configWith({ + email: { cast_as: 'string', indexes: { unique: {} } }, + }), + }) + + await validateCommand({}) + + expect(clack.log.warn).toHaveBeenCalledWith( + expect.stringContaining('does not expose `getSchemas()`'), + ) + // And the columns still came from somewhere: the config-only collection. + expect(clack.log.success).toHaveBeenCalledWith( + 'Schema loaded: 1 table, 1 encrypted column', + ) + }) + + // The discriminating half — without it, a warning printed unconditionally + // would pass the test above and tell every user their domain checks were + // skipped when they were not. + it('says nothing about getSchemas() when the client exposes it', async () => { + given({ + schemas: [encryptedTable('users', { email: types.TextSearch('email') })], + }) + + await validateCommand({}) + + expect(clack.log.warn).not.toHaveBeenCalled() + }) +}) + +describe('validateCommand — reaching the database', () => { + /** + * The schema rules are worth running on a laptop with no database up, so a + * missing URL downgrades to schema-only rather than failing. Silence would + * be the bug: the user would read a clean report as "no drift" when drift + * was never looked for. + */ + it('skips the database checks, and says so, when no database URL resolved', async () => { + given({ + schemas: [encryptedTable('users', { email: types.TextSearch('email') })], + databaseUrl: '', + }) + + await expect(validateCommand({})).resolves.toBeUndefined() + + expect(clack.log.info).toHaveBeenCalledWith( + expect.stringContaining('No database URL resolved'), + ) + // Actionable, not just apologetic. + expect(clack.log.info).toHaveBeenCalledWith( + expect.stringContaining('--database-url'), + ) + // Stronger than "did not read": no connection was opened at all. + expect(pgMock.connectionStrings).toEqual([]) + expect(pgMock.connect).not.toHaveBeenCalled() + expect(exitSpy).not.toHaveBeenCalled() + }) + + /** + * Same posture for a URL that is present but unreachable — and this is the + * half that has to be checked twice. Degrading means returning `undefined`, + * NOT handing the rules a blank `ObservedState`: an all-empty observation + * reads as `eqlInstalled: false`, which is an error, which would exit 1 over + * a database nobody ever managed to read. + */ + it('degrades to the schema checks when the database cannot be reached', async () => { + given({ + schemas: [encryptedTable('users', { email: types.TextSearch('email') })], + databaseUrl: DATABASE_URL, + }) + pgMock.connect.mockRejectedValue( + new Error('connect ECONNREFUSED 127.0.0.1:5432'), + ) + + await expect(validateCommand({})).resolves.toBeUndefined() + + expect(clack.log.info).toHaveBeenCalledWith( + expect.stringContaining( + 'Could not read the database (connect ECONNREFUSED 127.0.0.1:5432)', + ), + ) + // The schema rules found nothing, and the unreachable database added + // nothing — a skipped database check is never itself a failure. + expect(clack.log.error).not.toHaveBeenCalled() + expect(clack.outro).toHaveBeenCalledWith('No issues found.') + expect(exitSpy).not.toHaveBeenCalled() + // The socket is closed on the failing path too. + expect(pgMock.end).toHaveBeenCalled() + }) + + // A query that fails after a successful connect lands in the same catch, and + // must be reported the same way: the read is what matters, not which half of + // it broke. + it('degrades the same way when the catalogue read itself fails', async () => { + given({ + schemas: [encryptedTable('users', { email: types.TextSearch('email') })], + databaseUrl: DATABASE_URL, + }) + pgMock.query.mockRejectedValue( + Object.assign(new Error('permission denied for schema pg_catalog'), { + code: '42501', + }), + ) + + await expect(validateCommand({})).resolves.toBeUndefined() + + expect(clack.log.info).toHaveBeenCalledWith( + expect.stringContaining('permission denied for schema pg_catalog'), + ) + expect(exitSpy).not.toHaveBeenCalled() + }) + + /** + * The happy path: connect, read, and let the database rules run. Asserted + * through a drift the schema rules alone cannot produce, so it cannot pass + * on a build where `readObservedState` was never called. + */ + it('validates against the live database when it can connect', async () => { + given({ + schemas: [encryptedTable('users', { email: types.TextSearch('email') })], + databaseUrl: DATABASE_URL, + }) + respondWith( + healthyDatabase([ + // The migration wrote the equality domain; the schema declares search. + { + table_name: 'users', + column_name: 'email', + domain_name: 'eql_v3_text_eq', + }, + ]), + ) + + await expect(validateCommand({})).rejects.toBeInstanceOf(ProcessExited) + + expect(pgMock.connectionStrings).toEqual([DATABASE_URL]) + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining( + 'users.email: Declared `public.eql_v3_text_search` but the database column is `eql_v3_text_eq`', + ), + ) + expect(exitSpy).toHaveBeenCalledWith(1) + expect(pgMock.end).toHaveBeenCalled() + }) + + // The other side of the same wiring: a database that agrees with the schema + // must produce no database findings, so the drift assertion above is a fact + // about the read and not about the fixture always disagreeing. + it('reports no drift when the live database matches the declaration', async () => { + given({ + schemas: [encryptedTable('users', { email: types.TextSearch('email') })], + databaseUrl: DATABASE_URL, + }) + respondWith( + healthyDatabase([ + { + table_name: 'users', + column_name: 'email', + domain_name: 'eql_v3_text_search', + }, + ]), + ) + + await expect(validateCommand({})).resolves.toBeUndefined() + + // Pinned against the read actually happening: "no findings" is also what a + // command that never opened the database would print, so the six catalogue + // reads are what make this a statement about a clean database. + expect(pgMock.query).toHaveBeenCalledTimes(6) + expect(clack.log.error).not.toHaveBeenCalled() + expect(exitSpy).not.toHaveBeenCalled() + }) +}) + +/** + * The exit-code contract. `reportIssues` returning `true` on an error already + * has coverage next door; what is asserted here is the WIRING from that + * boolean to the process's exit status — the only thing CI can see. + */ +describe('validateCommand — exit code', () => { + it('exits 1 when the report contains an error', async () => { + // A searchable boolean. No `types.*` factory can build one, so it arrives + // on the config-only path — and it is an error: with two possible values, + // an equality term is a direct read of the plaintext. + given({ + config: configWith({ + flag: { cast_as: 'boolean', indexes: { unique: {} } }, + }), + }) + + await expect(validateCommand({})).rejects.toBeInstanceOf(ProcessExited) + + expect(clack.log.error).toHaveBeenCalledWith( + expect.stringContaining('searchable boolean column leaks'), + ) + expect(exitSpy).toHaveBeenCalledTimes(1) + expect(exitSpy).toHaveBeenCalledWith(1) + }) + + /** + * The discriminating half, and the reason both directions are pinned: an + * unconditional `process.exit(1)` after the report would pass the test above + * and turn every portability warning and every "no functional index" hint + * into a red build. + */ + it('does not exit when the worst finding is a warning or an info', async () => { + given({ + schemas: [ + encryptedTable('t', { + age: types.IntegerOrdOre('age'), // warning: ORE needs a superuser + notes: types.Text('notes'), // info: storage-only + }), + ], + }) + + await expect(validateCommand({})).resolves.toBeUndefined() + + // Non-vacuous: findings really were produced and really were printed, so + // "did not exit" is a statement about the gate and not about an empty run. + expect(clack.log.warn).toHaveBeenCalledWith( + expect.stringContaining( + 't.age: eql_v3_integer_ord_ore needs the ORE btree operator class', + ), + ) + expect(clack.log.info).toHaveBeenCalledWith( + expect.stringContaining('t.notes: Storage-only column'), + ) + expect(clack.outro).toHaveBeenCalledWith('No errors found. 1 warning.') + expect(exitSpy).not.toHaveBeenCalled() + }) + + /** + * Zero findings returns before the report is printed at all. Worth its own + * test because the two paths render the SAME outro — only the blank-line + * separator, which exists solely to precede a list of issues, tells them + * apart. + */ + it('closes with "No issues found." without printing a report', async () => { + given({ + schemas: [encryptedTable('users', { email: types.TextSearch('email') })], + }) + + await expect(validateCommand({})).resolves.toBeUndefined() + + expect(clack.outro).toHaveBeenCalledTimes(1) + expect(clack.outro).toHaveBeenCalledWith('No issues found.') + expect(consoleLog).not.toHaveBeenCalled() + expect(exitSpy).not.toHaveBeenCalled() + }) +}) diff --git a/packages/cli/src/commands/eql/__tests__/validate.test.ts b/packages/cli/src/commands/eql/__tests__/validate.test.ts index 780924a39..1c7a1487d 100644 --- a/packages/cli/src/commands/eql/__tests__/validate.test.ts +++ b/packages/cli/src/commands/eql/__tests__/validate.test.ts @@ -74,6 +74,18 @@ function observing( } } +/** + * A one-table encrypt config, for the degraded config-only collection path. + * + * This is the shape `collectDeclaredColumnsFromConfig` reads when the project's + * `@cipherstash/stack` predates `getSchemas()`, so every column it yields has + * `eqlType: undefined` — which is exactly what the domain-less renderings of + * the database findings need, and why this lives at module scope rather than + * inside the hand-authored-config block that first needed it. + */ +const configWith = (columns: EncryptConfig['tables'][string]): EncryptConfig => + ({ v: 1, tables: { t: columns } }) as EncryptConfig + /** Every functional index a set of declared columns could want. */ function fullyIndexed( columns: ReturnType, @@ -283,10 +295,6 @@ describe('schema rules that guard hand-authored configs', () => { * generator, so they are exercised through the config-only collection path, * which is the one such a config actually arrives on. */ - const configWith = ( - columns: EncryptConfig['tables'][string], - ): EncryptConfig => ({ v: 1, tables: { t: columns } }) as EncryptConfig - it('rejects a searchable boolean', () => { const columns = collectDeclaredColumnsFromConfig( configWith({ flag: { cast_as: 'boolean', indexes: { unique: {} } } }), @@ -318,8 +326,15 @@ describe('schema rules that guard hand-authored configs', () => { }) it('rejects ste_vec without a json cast', () => { + // `prefix` is required on a `ste_vec` index and `EncryptedTable.build()` + // rewrites its `'enabled'` sentinel to `${table}/${column}`, so a real + // emitted config carries `'t/doc'` here. It is beside the point of this + // test — the rule keys off `cast_as` — but the config is a typed value and + // spelling it right is cheaper than a cast that erases the type. const columns = collectDeclaredColumnsFromConfig( - configWith({ doc: { cast_as: 'string', indexes: { ste_vec: {} } } }), + configWith({ + doc: { cast_as: 'string', indexes: { ste_vec: { prefix: 't/doc' } } }, + }), ) expect(validateSchemas(columns)).toEqual([ @@ -364,6 +379,98 @@ describe('database rules', () => { expect(validateSchemas(columns, observed)).toEqual([]) }) + /** + * A bare declared name resolves through `search_path`, so when the same name + * exists in the searched schema AND somewhere else, the declaration does not + * pin which relation the application actually reads — `auth.users` versus + * `public.users` on Supabase is the case everyone meets. Validate checked one + * of them and said nothing about having chosen. + * + * Info, not warning, for three reasons: `info` does not touch the exit code; + * `auth.users` exists in every Supabase project, so a warning would report + * healthy projects as not-clean; and the warning bucket means "nothing was + * checked", whereas this run did check something and is qualifying WHICH. + * + * The fixture is the healthy one above, byte for byte, plus the `elsewhere` + * override — that test asserts `toEqual([])`, so it is simultaneously the + * proof this finding is new and the negative control against the rule + * degenerating into "always fires". + */ + it('notes which relation it checked when the table name is shadowed', () => { + const observed = observing(healthy, { + indexedExtractors: fullyIndexed(columns), + elsewhere: new Map([['users', ['auth']]]), + }) + + const issues = validateSchemas(columns, observed) + + expect(issues).toEqual([ + { + severity: 'info', + table: 'users', + message: expect.stringContaining('also exists in schema "auth"'), + }, + ]) + // Naming only the others leaves the reader no better off: the finding has + // to say which relation the findings around it actually describe. + expect(issues[0].message).toContain('"public"."users"') + expect(issues[0].message).toContain('search_path%3Dauth') + }) + + it('names every other schema holding the shadowed name', () => { + const observed = observing(healthy, { + indexedExtractors: fullyIndexed(columns), + elsewhere: new Map([['users', ['auth', 'archive']]]), + }) + + const [issue] = validateSchemas(columns, observed) + + expect(issue.message).toContain('"auth" / "archive"') + }) + + /** + * The finding qualifies the column findings for its table, so it has to + * precede them — the ordering contract `validateSchemas` documents. Asserted + * structurally rather than by message, so a finding appended after the column + * loop (the obvious way to write this rule) fails here. + */ + it('places the shadowing note ahead of the column findings it qualifies', () => { + const observed = observing(healthy, { + elsewhere: new Map([['users', ['auth']]]), + }) + + expect( + validateSchemas(columns, observed).map((issue) => [ + issue.severity, + issue.column, + ]), + ).toEqual([ + ['info', undefined], + ['info', 'email'], + ['info', 'age'], + ]) + }) + + /** + * The unreachable branch already reports a table that is ONLY elsewhere, and + * says far more about it than this rule could. Both firing would print two + * findings that contradict each other on whether anything was checked. + */ + it('says nothing when the table is only elsewhere, leaving that to the warning', () => { + const observed = observing( + {}, + { + elsewhere: new Map([['users', ['app']]]), + indexedExtractors: fullyIndexed(columns), + }, + ) + + const issues = validateSchemas(columns, observed) + + expect(issues).toHaveLength(1) + expect(issues[0].severity).toBe('warning') + }) + /** * A table absent from the privilege-filtered `columns` read is FOUR different * situations, and only the last is a failure of the project's migrations: @@ -587,6 +694,36 @@ describe('database rules', () => { ]) }) + /** + * The same finding on the degraded config-only path — an old + * `@cipherstash/stack` with no `getSchemas()`, and a reachable database. That + * combination is the whole reason the domain-less fallback exists, and it was + * reachable only in production: every other config-only test calls + * `validateSchemas` with no `observed`, so the database rules never ran, and + * every database-rule test uses real `types.*` columns, which always carry a + * domain. + * + * Asserted as a whole string, not a substring: the point of the test is which + * ARM of `${domain ? ... : 'declared EQL'}` rendered, and only an exact match + * can tell them apart. + */ + it('names no domain in the migration hint when the degraded path has none', () => { + const domainless = collectDeclaredColumnsFromConfig( + configWith({ age: { cast_as: 'number', indexes: { ope: {} } } }), + ) + const observed = observing({ t: { other: 'eql_v3_integer_ord' } }) + + expect(validateSchemas(domainless, observed)).toEqual([ + { + severity: 'error', + table: 't', + column: 'age', + message: + 'Column "age" is declared in your encryption schema but does not exist on table "t". Add it in a migration with the declared EQL type.', + }, + ]) + }) + it('reports a column that is still plain', () => { const observed = observing( { users: { email: 'eql_v3_text_search', age: null } }, @@ -603,6 +740,32 @@ describe('database rules', () => { ]) }) + /** + * The degraded-path twin of the test above, for the same reason. Note that + * `stringContaining('is a plain (non-EQL) column')` — what that test asserts — + * matches BOTH renderings, so it could never have caught the domain-less arm + * dropping out. The sentence has to be asserted whole. + */ + it('claims no domain in the plain-column finding when the degraded path has none', () => { + const domainless = collectDeclaredColumnsFromConfig( + configWith({ age: { cast_as: 'number', indexes: { ope: {} } } }), + ) + const observed = observing( + { t: { age: null } }, + { indexedExtractors: fullyIndexed(domainless) }, + ) + + expect(validateSchemas(domainless, observed)).toEqual([ + { + severity: 'error', + table: 't', + column: 'age', + message: + 'Column "age" is a plain (non-EQL) column in the database, but your schema declares it encrypted. Encrypted payloads written to it are unconstrained and unqueryable.', + }, + ]) + }) + it('reports a domain that has drifted from the declaration', () => { const observed = observing( { @@ -997,6 +1160,89 @@ describe('readObservedState', () => { expect(observed.connectedRole).toBe('app_readonly') }) + /** + * The relation lookup scans EVERY schema, and `information_schema` publishes + * views named `columns`, `domains`, `parameters`, `routines`, `sequences`, + * `tables` and `triggers` — every one of them a plausible application table + * name. So a project that declares `domains` and has NOT run the migration + * matched the system view, landed in `elsewhere`, and was told its table + * "exists in schema information_schema — point your connection there". That + * is absurd advice, and because it is a warning rather than an error it also + * flips the exit code from 1 to 0: the unapplied migration ships silently. + * + * Verified against a live PostgreSQL 14: the unfiltered query returns all + * seven of those names out of `information_schema`, and returns none of them + * once the two predicates below are added. + * + * The fake stands in for the catalogue by honouring the query's own exclusion + * predicates, which is what a real server does. + */ + it('does not mistake an information_schema view for a declared table', async () => { + const excludesSystemSchemas = (text: string) => + /nspname\s*!~\s*'\^pg_'/.test(text) && + /nspname\s*<>\s*'information_schema'/.test(text) + + const client = { + query: (text: string) => + Promise.resolve({ + rows: + text.includes('AS relation_schema') && !excludesSystemSchemas(text) + ? [ + { + table_name: 'domains', + relation_schema: 'information_schema', + is_searched_schema: false, + }, + ] + : [], + }), + } as unknown as Parameters[0] + + const declared = collectDeclaredColumns([ + encryptedTable('domains', { name: types.TextEq('name') }), + ]) + const observed = await readObservedState(client, ['domains']) + + expect(observed.elsewhere.get('domains')).toBeUndefined() + + const [issue] = validateSchemas(declared, { + ...observed, + eqlInstalled: true, + }) + + // The migration genuinely has not run, so this must fail the command. + expect(issue.severity).toBe('error') + expect(issue.message).toContain('does not exist in any schema') + expect(issue.message).not.toContain('information_schema') + }) + + /** + * The predicates asserted as SQL text, alongside the behavioural test above: + * that one can only prove the rows are gone, not that they were excluded for + * the right reason on a server whose catalogue this fake does not model. + */ + it('excludes the system schemas from the relation lookup', async () => { + const queries: string[] = [] + const client = { + query: (text: string) => { + queries.push(text) + return Promise.resolve({ rows: [] }) + }, + } as unknown as Parameters[0] + + await readObservedState(client, ['users']) + + const lookup = queries.find((text) => text.includes('AS relation_schema')) + + expect(lookup).toBeDefined() + // The regex form, not `NOT LIKE 'pg\_%'`: this SQL is a JS template + // literal, which collapses `\_` to a bare `_` — a LIKE wildcard that also + // swallows `pgbouncer`, `pgsodium` and every other real `pg`-prefixed + // schema. Confirmed both in node and against PostgreSQL 14. + expect(lookup).toMatch(/nspname\s*!~\s*'\^pg_'/) + expect(lookup).toMatch(/nspname\s*<>\s*'information_schema'/) + }) + it('asks for the schemas of exactly the declared tables', async () => { const queries: Array<{ text: string; values?: unknown[] }> = [] const client = { diff --git a/packages/cli/src/commands/eql/validate.ts b/packages/cli/src/commands/eql/validate.ts index e7e742c20..3a3cb056f 100644 --- a/packages/cli/src/commands/eql/validate.ts +++ b/packages/cli/src/commands/eql/validate.ts @@ -76,11 +76,20 @@ export interface ObservedState { */ connectedRole?: string /** - * For each declared table NOT in {@link searchedSchema}, the other schemas a - * relation of that name does live in. This is what separates "your - * `search_path` points at the wrong schema" (recoverable, the table is right - * there) from "the migration that creates this table has not run" (a real - * failure). Empty for a table that exists nowhere. + * For each declared table, every schema OTHER than {@link searchedSchema} in + * which a relation of that name lives. + * + * Populated per catalogue ROW, so a name present in the searched schema AND + * elsewhere lands here AND in {@link searchedSchemaRelations} — this is NOT + * scoped to tables missing from {@link searchedSchema}, and must not be + * "tidied" into that invariant: {@link ambiguousTableIssue} reads exactly + * that overlap to report a shadowed table name, and narrowing this field + * would delete that rule with every test still passing except its own. + * + * With {@link searchedSchemaRelations} it also separates "your `search_path` + * points at the wrong schema" (recoverable, the table is right there) from + * "the migration that creates this table has not run" (a real failure). Empty + * for a table that exists in no other schema. */ elsewhere: Map /** @@ -256,10 +265,17 @@ export function validateSchemas( const unreachable = new Set() if (dbState !== undefined) { for (const table of new Set(columns.map((column) => column.table))) { - if (dbState.columns.has(table)) continue + if (!dbState.columns.has(table)) { + unreachable.add(table) + issues.push(unreachableTableIssue(table, dbState)) + continue + } - unreachable.add(table) - issues.push(unreachableTableIssue(table, dbState)) + // Reachable — but possibly not the only relation of that name. Reported + // here, in the same per-table pass, so it precedes the column findings it + // qualifies rather than trailing them. + const ambiguous = ambiguousTableIssue(table, dbState) + if (ambiguous !== undefined) issues.push(ambiguous) } } @@ -455,6 +471,39 @@ function unreachableTableIssue( } } +/** + * A declared table name that resolved here AND exists in another schema. + * + * The complement of {@link unreachableTableIssue}: that one explains why + * nothing was checked, this one qualifies what WAS. A bare name resolves + * through `search_path`, so when two schemas carry it the declaration does not + * pin which relation the application reads — and every finding around this one + * describes whichever `current_schema()` happened to resolve to. Saying which + * costs one line and is the difference between a report the reader can trust + * and one they have to go and verify by hand. + * + * Info, deliberately, on all three counts that matter: it does not move the + * exit code, so a healthy project stays green; `auth.users` exists in every + * Supabase project, so a warning here would report the entire platform as + * unclean; and warning is reserved for "nothing was checked", which is the + * opposite of this case. + * + * `undefined` when the name is unambiguous — the overwhelmingly common case. + */ +function ambiguousTableIssue( + table: string, + observed: ObservedState, +): ValidationIssue | undefined { + const others = observed.elsewhere.get(table) ?? [] + if (others.length === 0) return undefined + + return { + table, + severity: 'info', + message: `Table "${table}" was checked as ${quoteIdent(observed.searchedSchema)}.${quoteIdent(table)}, the relation current_schema() resolves it to — but a relation of the same name also exists in schema ${others.map((schema) => `"${schema}"`).join(' / ')}. A bare table name resolves through search_path, so the declaration does not pin which one your application reads, and everything else validate reports for this table describes the "${observed.searchedSchema}" one. If the encrypted columns live in the other, point the connection at that schema to check it instead (e.g. ?options=-csearch_path%3D${others[0]}).`, + } +} + // --------------------------------------------------------------------------- // Reading the database // --------------------------------------------------------------------------- @@ -622,6 +671,20 @@ const SEARCHED_SCHEMA_SQL = ` * materialized-view and foreign relations: any of them can carry the encrypted * columns, and a partitioned table in particular is an ordinary choice for the * large tables this command is pointed at. + * + * The system schemas are excluded because the scan is otherwise database-wide + * and `information_schema` publishes views named `columns`, `domains`, + * `parameters`, `routines`, `sequences`, `tables` and `triggers` — all ordinary + * application table names. A project declaring one of them that had NOT run its + * migration matched the system view, so instead of the error that says the + * migration never ran it got a warning telling it to point `search_path` at + * `information_schema` — and, being a warning, exit 0. + * + * The regex operators, not `NOT LIKE 'pg\_%'`: this SQL is a JS template + * literal, which collapses `\_` to a bare `_`. That leaves a LIKE + * single-character wildcard, which also matches `pgbouncer`, `pgsodium` and + * every other real `pg`-prefixed schema — hiding relations that genuinely + * answer the question. `!~ '^pg_'` needs no escaping to mean what it says. */ const TABLE_SCHEMAS_SQL = ` SELECT c.relname AS table_name, @@ -631,6 +694,8 @@ const TABLE_SCHEMAS_SQL = ` JOIN pg_catalog.pg_namespace n ON n.oid = c.relnamespace WHERE c.relname = ANY($1::text[]) AND c.relkind = ANY(ARRAY['r','p','v','m','f']) + AND n.nspname !~ '^pg_' + AND n.nspname <> 'information_schema' ORDER BY n.nspname` /** Read everything the database rules need, in six catalogue queries. */ diff --git a/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts b/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts index 772167f63..e4d3892ff 100644 --- a/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts +++ b/packages/cli/src/config/__tests__/load-encrypt-schemas.test.ts @@ -124,14 +124,20 @@ describe('loadEncryptSchemas against an untrusted client', () => { }) /** - * A non-string `tableName` doesn't crash — it propagates as + * A `tableName` that isn't a string doesn't crash — it propagates as * `DeclaredColumn.table === undefined`, which `validateSchemas` then reports * as a table missing from the database and exits 1 on. So dropping this arm * turns the documented degrade into a hard failure naming a table called * "undefined", and nothing else covers it: the well-formed-table case above * still passes without it. + * + * Two fixtures for that one arm, because they are different inputs: a client + * can omit `tableName` altogether (an older or hand-rolled stub) or carry a + * wrong-typed one. Both pass `columnBuilders: {}`, which makes + * `Object.values({}).every()` vacuously true — so the `tableName` check is + * the only thing rejecting either one. */ - it('rejects a table whose tableName is not a string', async () => { + it('rejects a table whose tableName is missing', async () => { writeProject(clientReturning(`[{ columnBuilders: {} }]`)) const { schemas } = await loadEncryptSchemas('./client.ts') @@ -139,6 +145,14 @@ describe('loadEncryptSchemas against an untrusted client', () => { expect(schemas).toBeUndefined() }) + it('rejects a table whose tableName is not a string', async () => { + writeProject(clientReturning(`[{ tableName: 42, columnBuilders: {} }]`)) + + const { schemas } = await loadEncryptSchemas('./client.ts') + + expect(schemas).toBeUndefined() + }) + it('rejects a getSchemas() that does not return an array', async () => { writeProject(clientReturning(`{ users: { tableName: 'users' } }`)) diff --git a/skills/stash-cli/SKILL.md b/skills/stash-cli/SKILL.md index d6ce2a71c..4dda49428 100644 --- a/skills/stash-cli/SKILL.md +++ b/skills/stash-cli/SKILL.md @@ -459,6 +459,7 @@ Database checks (skipped with a notice, not a failure, when no database is reach | The database column is still plain (no EQL domain) | Error | | An `_ord_ore` domain on a database whose EQL install could not create the ORE operator class | Error | | A queryable column with no functional index over its term extractor | Info | +| A declared table name that resolved in the searched schema also exists in another one | Info | Exits 1 on errors only; warnings and info do not fail the command. @@ -473,6 +474,8 @@ Exits 1 on errors only; warnings and info do not fail the command. The `schema.table` case is deliberately not resolved by splitting the name: the only column reader is scoped to `current_schema()`, so `app.users` would silently validate against `public.users` and report an unrelated table's drift as this one's. An explicit "not checked" beats a confident wrong answer. +**An unqualified name that exists in more than one schema is reported too**, as an Info. A bare declared name resolves through `search_path`, so when the same name lives in both the searched schema and another — `users` in `public` and in Supabase's `auth`, which nearly every project has — the declaration does not pin which relation the application reads. Validate names the one it checked (`"public"."users"`), names the others, and gives the connection option to inspect one of those instead. It stays Info rather than Warning on purpose: it must not fail an ordinary Supabase project or report it as unclean, and unlike the four cases above this one *did* check a table — it is qualifying which, not reporting that nothing happened. + **The `_ord_ore` finding is about portability.** `CREATE OPERATOR CLASS` requires superuser, so managed Postgres (Supabase and most hosted providers) installs EQL without the ORE btree operator class, and the bundle then poisons every `_ord_ore` domain with an always-raising CHECK. Prefer the `_ord` (OPE) twin unless you control the database role. With a database reachable, validate confirms which case you are in and upgrades the Warning to an Error when the operator class is genuinely absent. The "no functional index" Info applies only to term-carrying (queryable) columns — resolve it with the recipes in the `stash-indexing` skill. Storage-only columns (bare `types.T`, `types.Boolean`) have no index option by design, and `types.Json` is served by a GIN index over the column rather than a scalar extractor; neither is reported.