diff --git a/.changeset/read-scope-not-null-safe-and-empty.md b/.changeset/read-scope-not-null-safe-and-empty.md new file mode 100644 index 0000000000..d59d4f7983 --- /dev/null +++ b/.changeset/read-scope-not-null-safe-and-empty.md @@ -0,0 +1,69 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): 分析查询的 RLS read scope 不再被 `{ $not: {} }` 整表放行,`$not` 改为 NULL-safe + +**这是一次安全相关的行为变更,涉及分析查询的可见行集合。请读完再升级。** + +### 变更一(要害):`{ $not: {} }` 的 read scope 以前**完全不加 WHERE**,整表可见;现在是零行 + +`read-scope-sql.ts` 是 RLS / 租户 read scope 降解成 SQL 的**唯一**通道(ADR-0021 D-C), +被 `NativeSQLStrategy.applyReadScope` 与 `ObjectQLStrategy` 用来给分析查询加可见性约束。 +它以空字符串表示「无约束」(布尔常量 TRUE)。`compileNode({})` 返回空串,于是: + +``` +compileNode({}) → '' → if (inner) 为假 → $not 不产出任何子句 + → compileScopedFilterToSql 返回 '' + → applyReadScope 的 `if (!sql) return;` 接手 + → 生成的 SQL 里没有 WHERE +``` + +一条语义为 `NOT TRUE ≡ FALSE`(**什么都不给看**)的 read scope,实际效果是**整张表都给看**。 +同一段循环里 `$and` / `$or` 的空数组一直是 fail-closed 抛错的,只漏了 `$not` 这一格。 + +修复后 `{ $not: {} }` 编译为恒假子句 `1 = 0`,`applyReadScope` 照常拼进 WHERE,返回零行 —— +与 driver-sql 在 #5134 / PR #5243 上的口径一致。 + +**升级影响:** 如果你的 RLS 策略(或 `cel-to-filter.ts` 降解出的 CEL 规则)在某条路径上 +产出过 `{ $not: {} }`,该对象的分析查询此前是**无边界**的,现在会返回零行。行数从「全部」 +掉到「零」不是本次引入的收紧,而是那条策略本来就该有的答案 —— 请核对策略本身。 + +同源、方向相反的一处一并修正:`$or` 的空析取项 `{}` 以前被 `.filter(s => s.length > 0)` +丢掉,`{ $or: [{}, { a: 1 }] }` 收紧成 `a = 1`。`{}` 是 TRUE 析取项,TRUE 吸收整个析取, +所以现在整条 `$or` 为 TRUE(无约束)。被丢弃分支的绑定值同时被丢弃 —— 否则 `params` 里 +会留下没有 `?` 消费的值,把后面每一个占位符都错位到别人的值上。 + +### 变更二:`$not` 改为 NULL-safe + +SQL 是三值逻辑,`WHERE` 只保留 TRUE,所以裸 `NOT ("t"."stage" = ?)` 会把 `stage IS NULL` +的行整批丢掉;`driver-memory`、`formula` 以及 #5296 之后的 `driver-sql` 都**返回**这些行。 +同一条 read scope,普通查询与分析查询给出不同的可见集合。#5146 已由维护者判定以 JS 家族的 +答案为准,本次把这个编译器对齐过去 —— 它是仓内最后一个按三值逻辑回答 `$not` 的 SQL 家族实现。 + +`$not` 的操作数在取反前先被改写成**全域(total)谓词**: + +```sql +-- 之前 +NOT ("t"."stage" = ?) +-- 现在 +NOT (("t"."stage" IS NOT NULL AND "t"."stage" = ?)) +``` + +守卫**下推到每个叶子**而不是挂在 `NOT` 旁边:操作数一旦嵌套(`$not` 里套 `$or`),顶层的 +`OR col IS NULL` 会把 JS 家族排除的行重新放进来。守卫方向**逐算子**判定,不是一刀切 —— +`{ $not: { a: { $ne: 5 } } }` 语义是「a 就是 5」,无条件加 `OR a IS NULL` 会把 scope 排除的 +行交回去,正是本次要避免的静默放松。所以 `$ne` / `$nin` / `$notContains` 用 +`col IS NULL OR (…)`,`$eq` / `$in` / `$gt` / `$between` / `$contains` 一族用 +`col IS NOT NULL AND (…)`,而 `$null` / `$exists` / `$eq: null` / `$ne: null` 本就是全域谓词, +一个字节都不加。 + +**升级影响:** 形如 `{ $not: { stage: 'won' } }` 的 read scope,以前**不返回** `stage` 为 +NULL 的行,现在**返回**它们 —— 分析查询的行数与图表数值会随之变化。这是把分析侧对齐到其余 +后端,不是新增的放宽。 + +### 不变的部分 + +`$not` 路径以外一个字符都没动:普通比较仍然编译成原样的 SQL。fail-closed 的全部保证原封不动 +——未知算子、嵌套关系值、裸数组、不安全标识符、非 filter 节点的 `$not` 操作数,以及 +`$and: []` / `$or: []` 的空组合子(那一格是 #5322 的独立裁定)统统照旧抛错。 diff --git a/packages/services/service-analytics/src/__tests__/read-scope-not-null-safe.test.ts b/packages/services/service-analytics/src/__tests__/read-scope-not-null-safe.test.ts new file mode 100644 index 0000000000..deb966dec9 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/read-scope-not-null-safe.test.ts @@ -0,0 +1,417 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5297] The read-scope SQL lowering answers `$not` the way the rest of the + * repo answers it — and a `{$not: {}}` scope shows zero rows instead of the + * whole table. + * + * # Why this file exists at all + * + * `read-scope-sql-conformance.test.ts` already runs the shared + * `FILTER_LOGIC_CASES` table against a real SQLite engine, and it was green + * through both defects: that table deliberately carries no NULL rows and no + * boolean-identity case (those land with #5239 / the spec half of #5146). So the + * gate that looks like it covers this compiler could not see either bug. These + * cases are the pin until the shared table absorbs them. + * + * # The two defects + * + * **`{$not: {}}` ran the query completely unscoped.** `compileNode({})` returns + * `''` (the constant TRUE), `if (inner)` was false, the `$not` emitted nothing, + * `compileScopedFilterToSql` returned `''`, and `applyReadScope`'s + * `if (!sql) return;` then added no `WHERE` — so a read scope whose meaning is + * `NOT TRUE ≡ FALSE`, i.e. *show nothing*, showed **everything**. The seam is + * what makes it a bypass rather than a wrong string, which is why the assertion + * below goes through `NativeSQLStrategy.generateSql` and not only through + * `compileScopedFilterToSql`. `$and: []` / `$or: []` in the same loop were + * fail-closed all along; `$not` was the one uncovered square. + * + * **`$not` was not NULL-safe.** SQL is three-valued and a `WHERE` keeps only + * TRUE, so `NOT (stage = 'won')` dropped every row whose `stage` is NULL, while + * `driver-memory`, `formula` and (since #5296) `driver-sql` return them. Same + * declared read scope, different visible set per backend — and a CEL `!expr` in + * a permission rule lowers to exactly this shape (`cel-to-filter.ts`). + * + * # Where the expected ids come from + * + * Measured, not reasoned: the fixture is row-for-row the one in + * `driver-sql`'s `sql-driver-not-null-safe.test.ts`, and every id set below is + * the answer that file and the two JS-backend pins + * (`formula/src/matches-filter-not-null-safe.test.ts`, + * `driver-memory/src/memory-matcher-not-null-safe.test.ts`) assert for the same + * filter — all three suites were run against this fixture while writing these + * cases. Moving an expectation here re-opens the divergence #5146 closed. + * + * `sql.js` (pure WASM) is the engine, for the reason spelled out at the top of + * `read-scope-sql-conformance.test.ts`: a native binding cannot be relied on to + * load under CI's Node. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import type { Cube, FilterCondition } from '@objectstack/spec/data'; +import type { AnalyticsQuery, StrategyContext } from '@objectstack/spec/contracts'; + +import { compileScopedFilterToSql } from '../read-scope-sql.js'; +import { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; + +/** + * Rows 3 and 4 are the point: `stage` is NULL in both, row 3 additionally + * carries a NULL `amount` and row 4 a NULL `owner`, so a guard applied to the + * wrong column shows up as a wrong id rather than passing by luck. + */ +const FIXTURE = [ + { id: '1', stage: 'won', owner: 'u1', amount: 10 }, + { id: '2', stage: 'lost', owner: 'u2', amount: 20 }, + { id: '3', stage: null, owner: 'u1', amount: null }, + { id: '4', stage: null, owner: null, amount: 40 }, +]; + +const ALL = ['1', '2', '3', '4']; +const ALIAS = 't'; + +/** Point sql.js at the `.wasm` shipped inside its own package (Node-safe). */ +async function locateWasm(): Promise<((file: string) => string) | undefined> { + try { + const { createRequire } = await import('node:module'); + const require = createRequire(import.meta.url); + const pkgJsonPath = require.resolve('sql.js/package.json'); + const { dirname, join } = await import('node:path'); + const dir = dirname(pkgJsonPath); + return (file: string) => join(dir, 'dist', file); + } catch { + return undefined; + } +} + +// ── The read scope as the analytics strategy actually applies it ───────────── + +const cube: Cube = { + name: 'deals', + title: 'Deals', + sql: 'deal', + measures: { revenue: { name: 'revenue', label: 'Revenue', type: 'sum', sql: 'amount' } }, + dimensions: { stage: { name: 'stage', label: 'Stage', type: 'string', sql: 'stage' } }, + public: false, +}; + +const query: AnalyticsQuery = { + cube: 'deals', + measures: ['revenue'], + dimensions: ['stage'], + timezone: 'UTC', +}; + +/** + * Generate the analytics SQL for a query whose only constraint is `scope`, + * exercising `NativeSQLStrategy.applyReadScope` — the seam where a scope that + * compiles to nothing turns into a query with no `WHERE`. + */ +async function generateScoped(scope: FilterCondition): Promise<{ sql: string; params: unknown[] }> { + const ctx: StrategyContext = { + getCube: (name) => (name === 'deals' ? cube : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: false, inMemory: false }), + executeRawSql: async () => [], + getReadScope: (obj) => (obj === 'deal' ? scope : undefined), + }; + return new NativeSQLStrategy().generateSql(query, ctx); +} + +describe('[#5297] read-scope `$not` — boolean identities and NULL safety', () => { + let db: any; + + beforeAll(async () => { + const mod: any = await import('sql.js'); + const initSqlJs = mod.default ?? mod; + const locateFile = await locateWasm(); + const SQL = await initSqlJs(locateFile ? { locateFile } : undefined); + + db = new SQL.Database(); + db.run(`CREATE TABLE "deal" ("id" TEXT PRIMARY KEY, "stage" TEXT, "owner" TEXT, "amount" REAL);`); + const insert = db.prepare(`INSERT INTO "deal" ("id","stage","owner","amount") VALUES (?,?,?,?)`); + for (const r of FIXTURE) insert.run([r.id, r.stage, r.owner, r.amount]); + insert.free(); + }); + + afterAll(() => { + db?.close(); + }); + + /** + * The rows a read scope admits, executed rather than asserted as a string. + * + * `''` is the compiler's TRUE — the shape for which `applyReadScope` adds no + * `WHERE` — so it is executed as the unconstrained query it stands for. That + * substitution is exactly the production behaviour, which is why a scope that + * MEANT `FALSE` had to stop compiling to `''`. + */ + const ids = (scope: unknown): string[] => { + const { sql, params } = compileScopedFilterToSql(scope as FilterCondition, ALIAS); + const stmt = db.prepare( + `SELECT "id" FROM "deal" AS "${ALIAS}" WHERE ${sql.length > 0 ? sql : '1 = 1'} ORDER BY "id"`, + ); + stmt.bind(params as any[]); + const got: string[] = []; + while (stmt.step()) got.push(String(stmt.get()[0])); + stmt.free(); + return got; + }; + + // ── Defect 2: the RLS bypass ─────────────────────────────────────────────── + + describe('`{$not: {}}` is FALSE, and the strategy actually applies it', () => { + it('compiles to a constant-false clause instead of the empty string', () => { + expect(compileScopedFilterToSql({ $not: {} } as FilterCondition, ALIAS)).toEqual({ + sql: '1 = 0', + params: [], + }); + }); + + it('admits zero rows — NOT TRUE ≡ FALSE', () => { + expect(ids({ $not: {} })).toEqual([]); + }); + + it('reaches the generated analytics SQL as a real WHERE — the bypass seam', async () => { + // Was: `compileScopedFilterToSql` → `''` → `applyReadScope`'s + // `if (!sql) return;` → a query with NO `WHERE` at all, i.e. the whole + // table returned under a scope that means "nothing is visible". + const { sql } = await generateScoped({ $not: {} } as FilterCondition); + expect(sql).toContain('WHERE'); + expect(sql).toContain('(1 = 0)'); + }); + + it('stays FALSE nested inside a scope that also carries real predicates', () => { + expect(ids({ $and: [{ owner: 'u1' }, { $not: {} }] })).toEqual([]); + expect(ids({ owner: 'u1', $not: {} })).toEqual([]); + }); + + it('a `$not` of a FALSE clause is TRUE again', () => { + // `NOT (1 = 0)` — the double-identity direction, so the constant is not + // just a string that happens to appear. + expect(ids({ $not: { $not: {} } })).toEqual(ALL); + }); + }); + + // ── Defect 2, mirror direction: the absorbed `$or` branch ────────────────── + + describe('a `{}` disjunct makes the whole `$or` TRUE', () => { + it('compiles to no constraint, and binds nothing', () => { + // Was `("t"."owner" = ?)` with one bound param: the TRUE branch was + // filtered out and the scope silently NARROWED to the surviving branch. + expect(compileScopedFilterToSql({ $or: [{}, { owner: 'u1' }] } as FilterCondition, ALIAS)).toEqual({ + sql: '', + params: [], + }); + }); + + it('admits every row', () => { + expect(ids({ $or: [{}, { owner: 'u1' }] })).toEqual(ALL); + expect(ids({ $or: [{ owner: 'u1' }, {}] })).toEqual(ALL); + }); + + it('drops the discarded branch\'s bindings with it — no orphaned param', async () => { + // The failure this guards is worse than a wide scope: a value left in + // `params` with no `?` to consume it shifts every later placeholder onto + // the wrong value, so a tenant predicate binds someone else's id. + const { params } = await generateScoped({ + $or: [{}, { owner: 'u1' }], + } as FilterCondition); + expect(params).not.toContain('u1'); + expect(params).toEqual([]); + }); + + it('an all-`{}` `$or` is TRUE too, and a `$and` is unchanged by a `{}` member', () => { + expect(ids({ $or: [{}, {}] })).toEqual(ALL); + expect(compileScopedFilterToSql({ $and: [{}, { owner: 'u1' }] } as FilterCondition, ALIAS)).toEqual({ + sql: '("t"."owner" = ?)', + params: ['u1'], + }); + }); + }); + + // ── Defect 1: NULL-safe negation ─────────────────────────────────────────── + + describe('a NULL column does not satisfy the negated condition', () => { + it('`$not` on an implicit equality returns the NULL rows', () => { + // Was ['2'] — rows 3 and 4 fell into UNKNOWN and disappeared. + expect(ids({ $not: { stage: 'won' } })).toEqual(['2', '3', '4']); + }); + + it('the guard rides the leaf, so the emitted SQL negates a TOTAL predicate', () => { + const { sql } = compileScopedFilterToSql({ $not: { stage: 'won' } } as FilterCondition, ALIAS); + expect(sql).toBe('NOT (("t"."stage" IS NOT NULL AND "t"."stage" = ?))'); + }); + + it('`$not` over MULTIPLE columns admits a row that is NULL in EITHER', () => { + expect(ids({ $not: { stage: 'won', owner: 'u1' } })).toEqual(['2', '3', '4']); + const { sql } = compileScopedFilterToSql( + { $not: { stage: 'won', owner: 'u1' } } as FilterCondition, + ALIAS, + ); + expect(sql).toContain('"t"."stage" IS NOT NULL'); + expect(sql).toContain('"t"."owner" IS NOT NULL'); + }); + + it('the RLS shape: a CEL `!(stage == "won")` scope stops hiding stage-less rows', () => { + expect(ids({ $not: { stage: 'won' } })).toHaveLength(3); + }); + + it('the same scope through the strategy binds one param and one NOT', async () => { + const { sql, params } = await generateScoped({ $not: { stage: 'won' } } as FilterCondition); + expect(sql).toContain('NOT ('); + expect(sql).toContain('"deal"."stage" IS NOT NULL'); + expect(params).toEqual(['won']); + }); + }); + + describe('the guard composes through nested combinators', () => { + it('`$not` of a `$or` excludes a NULL row whose OTHER branch matches', () => { + // Row 3 has a NULL stage but owner = 'u1', so the inner `$or` IS satisfied + // and the negation must reject it. A `NOT (…) OR stage IS NULL` hoisted to + // the top would hand row 3 back — the reason the guard is per leaf. + expect(ids({ $not: { $or: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '4']); + }); + + it('`$not` of a `$and` admits every row failing either conjunct', () => { + expect(ids({ $not: { $and: [{ stage: 'won' }, { owner: 'u1' }] } })).toEqual(['2', '3', '4']); + }); + + it('a double negation is the positive scope again, NULL rows excluded', () => { + expect(ids({ $not: { $not: { stage: 'won' } } })).toEqual(['1']); + expect(ids({ $not: { $not: { stage: 'won' } } })).toEqual(ids({ stage: 'won' })); + }); + + it('`$not` still ANDs with its sibling keys', () => { + expect(ids({ $not: { stage: 'won' }, owner: 'u1' })).toEqual(['3']); + }); + + it('a `$not` nested inside a `$or` branch stays NULL-safe', () => { + expect(ids({ $or: [{ $not: { stage: 'won' } }, { owner: 'u2' }] })).toEqual(['2', '3', '4']); + }); + }); + + describe('each operator is guarded in the direction it answers for a missing value', () => { + it('`$not` of `$ne` is NOT widened — it still means "the column IS that value"', () => { + // A blanket `OR stage IS NULL` would hand back rows 3 and 4, i.e. rows the + // scope excludes. On an RLS predicate that is the widening class outright. + expect(ids({ $not: { stage: { $ne: 'won' } } })).toEqual(['1']); + expect(compileScopedFilterToSql({ $not: { stage: { $ne: 'won' } } } as FilterCondition, ALIAS).sql) + .toContain('"t"."stage" IS NULL'); + }); + + it('`$not` of `$nin` is not widened either', () => { + expect(ids({ $not: { stage: { $nin: ['won'] } } })).toEqual(['1']); + }); + + it('`$not` of `$in` returns the NULL rows', () => { + expect(ids({ $not: { stage: { $in: ['won'] } } })).toEqual(['2', '3', '4']); + }); + + it('`$not` of an ordering comparison returns the NULL rows', () => { + expect(ids({ $not: { amount: { $gt: 15 } } })).toEqual(['1', '3']); + }); + + it('`$not` of `$between` returns the NULL rows', () => { + // `$between` is in this compiler's vocabulary but not in `driver-sql`'s + // guard table; it is a positive comparison, so it takes the same default. + expect(ids({ $not: { amount: { $between: [15, 30] } } })).toEqual(['1', '3', '4']); + }); + + it('`$not` of `$contains` / `$startsWith` / `$endsWith` returns the NULL rows', () => { + expect(ids({ $not: { stage: { $contains: 'w' } } })).toEqual(['2', '3', '4']); + expect(ids({ $not: { stage: { $startsWith: 'w' } } })).toEqual(['2', '3', '4']); + expect(ids({ $not: { stage: { $endsWith: 'n' } } })).toEqual(['2', '3', '4']); + }); + + it('`$not` of `$notContains` does NOT return them — the mirror case', () => { + // The one operator where the two JS backends disagree for a null-valued + // field; `formula` is followed, as `driver-sql` follows it, so this + // compiler casts no vote on a disagreement filed elsewhere. + expect(ids({ $not: { stage: { $notContains: 'w' } } })).toEqual(['1']); + }); + + it('`$not` of a null predicate is untouched — it was already two-valued', () => { + expect(ids({ $not: { stage: { $null: true } } })).toEqual(['1', '2']); + expect(ids({ $not: { stage: { $null: false } } })).toEqual(['3', '4']); + expect(ids({ $not: { stage: { $exists: true } } })).toEqual(['3', '4']); + expect(ids({ $not: { stage: { $exists: false } } })).toEqual(['1', '2']); + // No guard is wrapped around a predicate that can never be UNKNOWN. + expect(compileScopedFilterToSql({ $not: { stage: { $null: true } } } as FilterCondition, ALIAS).sql) + .toBe('NOT ("t"."stage" IS NULL)'); + }); + + it('`$not` of an explicit `null` comparand is untouched', () => { + expect(ids({ $not: { stage: null } })).toEqual(['1', '2']); + expect(ids({ $not: { stage: { $eq: null } } })).toEqual(['1', '2']); + expect(ids({ $not: { stage: { $ne: null } } })).toEqual(['3', '4']); + }); + + it('an empty `$in` / `$nin` under a `$not` keeps its constant value', () => { + expect(ids({ $not: { stage: { $in: [] } } })).toEqual(ALL); + expect(ids({ $not: { stage: { $nin: [] } } })).toEqual([]); + }); + }); + + // ── Nothing outside `$not` moves, and nothing stopped failing closed ─────── + + describe('only the `$not` path is rewritten', () => { + it('a plain comparison compiles to exactly the SQL it always did', () => { + expect(compileScopedFilterToSql({ stage: 'won' } as FilterCondition, ALIAS).sql) + .toBe('"t"."stage" = ?'); + expect(compileScopedFilterToSql({ stage: { $ne: 'won' } } as FilterCondition, ALIAS).sql) + .toBe('"t"."stage" <> ?'); + expect(compileScopedFilterToSql({ amount: { $gt: 15 } } as FilterCondition, ALIAS).sql) + .toBe('"t"."amount" > ?'); + }); + + it('a plain comparison returns the rows it always did', () => { + expect(ids({ stage: 'won' })).toEqual(['1']); + // Still three-valued OUTSIDE a negation: `<> 'won'` drops the NULL rows. + // That divergence from the JS backends is real and out of #5146's scope. + expect(ids({ stage: { $ne: 'won' } })).toEqual(['2']); + expect(ids({})).toEqual(ALL); + }); + }); + + describe('the fail-closed guarantees survive the rewrite', () => { + it('an empty `$and` / `$or` still THROWS, inside a `$not` as well as outside', () => { + expect(() => compileScopedFilterToSql({ $and: [] } as FilterCondition, ALIAS)) + .toThrowError(/non-empty array/); + expect(() => compileScopedFilterToSql({ $or: [] } as FilterCondition, ALIAS)) + .toThrowError(/non-empty array/); + // The empty-combinator square is its own ruling (#5322) — the rewrite must + // not quietly turn either of them into a boolean identity on the way past. + expect(() => compileScopedFilterToSql({ $not: { $or: [] } } as FilterCondition, ALIAS)) + .toThrowError(/non-empty array/); + expect(() => compileScopedFilterToSql({ $not: { $and: [] } } as FilterCondition, ALIAS)) + .toThrowError(/non-empty array/); + }); + + it('an unknown operator inside a `$not` still THROWS rather than being guarded', () => { + expect(() => compileScopedFilterToSql({ $not: { f: { $regex: '.*' } } } as FilterCondition, ALIAS)) + .toThrowError(/unsupported operator/); + }); + + it('a nested relation / bare array / zero-operator spec inside a `$not` still THROWS', () => { + expect(() => compileScopedFilterToSql({ $not: { account: { region: 'NA' } } } as FilterCondition, ALIAS)) + .toThrowError(/nested\/relation value/); + expect(() => compileScopedFilterToSql({ $not: { stage: ['won'] } } as FilterCondition, ALIAS)) + .toThrowError(/bare array value/); + expect(() => compileScopedFilterToSql({ $not: { stage: {} } } as FilterCondition, ALIAS)) + .toThrowError(/nested\/relation value/); + }); + + it('a non-node `$not` operand is still refused, not rewritten', () => { + expect(() => compileScopedFilterToSql({ $not: null } as unknown as FilterCondition, ALIAS)) + .toThrowError(/must be a filter object/); + expect(() => compileScopedFilterToSql({ $not: 'x' } as unknown as FilterCondition, ALIAS)) + .toThrowError(/must be a filter object/); + expect(() => compileScopedFilterToSql({ $not: [] } as unknown as FilterCondition, ALIAS)) + .toThrowError(/must be a filter object/); + }); + + it('an unsafe identifier inside a `$not` is still refused', () => { + expect(() => compileScopedFilterToSql({ $not: { 'id; DROP TABLE x': 'v' } } as FilterCondition, ALIAS)) + .toThrowError(/unsafe field identifier/); + }); + }); +}); diff --git a/packages/services/service-analytics/src/read-scope-sql.ts b/packages/services/service-analytics/src/read-scope-sql.ts index 0c0b4988df..c8104cf2ac 100644 --- a/packages/services/service-analytics/src/read-scope-sql.ts +++ b/packages/services/service-analytics/src/read-scope-sql.ts @@ -22,10 +22,52 @@ import type { FilterCondition } from '@objectstack/spec/data'; * Supports the operators the RLS layer and common policies emit: implicit * equality, `$eq/$ne/$gt/$gte/$lt/$lte/$in/$nin/$between/$contains/$notContains/ * $startsWith/$endsWith/$null/$exists`, and `$and/$or/$not` combinators. + * + * ## `''` means TRUE, and that is a value — not "nothing happened" + * + * `compileNode` returns `''` for a node that constrains nothing (`{}`, an + * all-TRUE `$and`). That empty string is the boolean constant TRUE, and the two + * places where a compiler forgets it are exactly where this one used to be + * wrong (#5297): + * + * - TRUE is the AND identity, so dropping it from a `$and` is correct — but it + * ABSORBS a `$or`, so one TRUE disjunct makes the whole `$or` TRUE. Filtering + * it out of `$or` (`{$or: [{}, {a: 1}]}` → `a = 1`) silently NARROWED. + * - `NOT TRUE ≡ FALSE`, so `{$not: {}}` is the zero-row predicate. Emitting + * nothing for it made `compileScopedFilterToSql` return `''`, and + * `applyReadScope`'s `if (!sql) return;` then added no `WHERE` at all — a + * read scope that should have shown zero rows exposed the whole table. In an + * RLS lowering that is a permission bypass, not a rounding error. + * + * So a group is now compiled into its own bind buffer and only committed when it + * survives, and FALSE has a spelling ({@link FALSE_CLAUSE}) instead of being + * representable only as silence. + * + * ## `$not` is NULL-safe (#5146) + * + * SQL is three-valued and a `WHERE` keeps only TRUE, so a bare `NOT (col = ?)` + * drops every row whose `col` is NULL — while `driver-memory` and `formula` + * (and, since #5296, `driver-sql`) return those rows. One read scope, two + * visible sets, chosen by which backend answered. #5146 ruled the JS answer + * canonical; {@link nullSafeNegationOperand} here is the same rewrite + * `sql-driver.ts` applies, so an analytics query and an ordinary `find()` scope + * the same rows. */ const IDENT = /^[a-z_][a-z0-9_]*$/i; +/** + * The FALSE constant. `''` is this compiler's TRUE, so FALSE needs a spelling of + * its own — `1 = 0` is already what an empty `$in` lowers to, and what + * `driver-sql` emits for the same identity (#5243). + */ +const FALSE_CLAUSE = '1 = 0'; + +/** A node the compiler can walk: a plain object, not `null` and not an array. */ +function isFilterNode(v: unknown): v is Record { + return v !== null && typeof v === 'object' && !Array.isArray(v); +} + function quoteIdent(name: string, kind: string): string { if (typeof name !== 'string' || !IDENT.test(name)) { throw new Error(`[read-scope-sql] unsafe ${kind} identifier "${String(name)}" — refusing to build read scope (fail-closed).`); @@ -43,26 +85,61 @@ export function compileScopedFilterToSql( return { sql, params }; } -/** Compile a filter node into a boolean SQL expression ('' = empty/no constraint). */ +/** + * Compile a child node into its OWN bind buffer. + * + * A group can turn out to be a boolean identity only after its children have + * been compiled — and compiling them appends to `params`. Binding straight into + * the parent's array and then discarding the clause would leave those values + * behind with no `?` to consume them, shifting every later placeholder onto the + * wrong value: a read scope that binds the wrong tenant id is worse than one + * that is merely too wide. Buffer per child, commit only what survives. + */ +function compileSub(node: unknown, qAlias: string): { sql: string; params: unknown[] } { + const params: unknown[] = []; + const sql = compileNode(node, qAlias, params); + return { sql, params }; +} + +/** Compile a filter node into a boolean SQL expression ('' = TRUE, no constraint). */ function compileNode(node: unknown, qAlias: string, params: unknown[]): string { - if (node === null || typeof node !== 'object' || Array.isArray(node)) { + if (!isFilterNode(node)) { throw new Error('[read-scope-sql] read scope must be a filter object (fail-closed).'); } const clauses: string[] = []; - for (const [key, value] of Object.entries(node as Record)) { + for (const [key, value] of Object.entries(node)) { if (key === '$and' || key === '$or') { if (!Array.isArray(value) || value.length === 0) { throw new Error(`[read-scope-sql] "${key}" requires a non-empty array (fail-closed).`); } - const parts = (value as unknown[]) - .map((child) => compileNode(child, qAlias, params)) - .filter((s) => s.length > 0); - if (parts.length === 0) continue; + const compiled = (value as unknown[]).map((child) => compileSub(child, qAlias)); + // A `''` branch is the constant TRUE. It ABSORBS a disjunction — one TRUE + // disjunct makes the whole `$or` TRUE — so the group contributes nothing + // rather than collapsing to its remaining branches, which would have + // narrowed `{$or: [{}, {a: 1}]}` to `a = 1` (#5297). + if (key === '$or' && compiled.some((c) => c.sql.length === 0)) continue; + // For `$and` the same constant is the identity, so it just drops out. + const kept = compiled.filter((c) => c.sql.length > 0); + if (kept.length === 0) continue; + for (const part of kept) params.push(...part.params); const joiner = key === '$and' ? ' AND ' : ' OR '; - clauses.push(`(${parts.join(joiner)})`); + clauses.push(`(${kept.map((c) => c.sql).join(joiner)})`); } else if (key === '$not') { - const inner = compileNode(value, qAlias, params); - if (inner) clauses.push(`NOT (${inner})`); + // NULL-safe negation (#5146): totalise the operand's leaves first, so + // `NOT (…)` can never be UNKNOWN and this compiler admits the same rows + // `driver-sql` / `driver-memory` / `formula` admit. A non-node operand is + // left alone so `compileNode` still rejects it with its own message. + const operand = isFilterNode(value) ? nullSafeNegationOperand(value) : value; + const inner = compileSub(operand, qAlias); + if (inner.sql.length === 0) { + // `NOT TRUE ≡ FALSE`. Emitting nothing here is what let a `{$not: {}}` + // read scope through `applyReadScope`'s `if (!sql) return;` and ran the + // analytics query completely unscoped (#5297). + clauses.push(FALSE_CLAUSE); + } else { + params.push(...inner.params); + clauses.push(`NOT (${inner.sql})`); + } } else if (key.startsWith('$')) { throw new Error(`[read-scope-sql] unsupported top-level operator "${key}" (fail-closed).`); } else { @@ -116,7 +193,7 @@ function compileOperator(col: string, op: string, val: unknown, field: string, p case '$lte': return `${col} <= ${bind(params, val)}`; case '$in': { if (!Array.isArray(val)) throw new Error(`[read-scope-sql] $in for "${field}" needs an array (fail-closed).`); - if (val.length === 0) return '1 = 0'; // IN () matches nothing — safe + if (val.length === 0) return FALSE_CLAUSE; // IN () matches nothing — safe return `${col} IN (${val.map((v) => bind(params, v)).join(', ')})`; } case '$nin': { @@ -138,3 +215,168 @@ function compileOperator(col: string, op: string, val: unknown, field: string, p throw new Error(`[read-scope-sql] unsupported operator "${op}" on "${field}" (fail-closed).`); } } + +// ── [#5146] NULL-safe `$not` ───────────────────────────────────────────────── + +/** + * What one field constraint needs so its compiled SQL is TOTAL — TRUE or FALSE + * for every row, never UNKNOWN. + * + * - `'none'` — already total (`IS NULL` / `IS NOT NULL`), or a shape + * this compiler refuses outright, which must keep refusing. + * - `'requireValue'` — a NULL column does NOT satisfy it: `col IS NOT NULL AND (…)`. + * - `'allowNull'` — a NULL column DOES satisfy it: `col IS NULL OR (…)`. + */ +type NullGuard = 'none' | 'requireValue' | 'allowNull'; + +/** + * Does a NULL column satisfy this one operator, under the semantics the JS + * backends (`driver-memory`'s `match`, `formula`'s `matchesFilterCondition`) + * give it? They evaluate a missing value in ordinary two-valued JS — `undefined + * !== 'won'` is simply `true` — and #5146 ruled that answer canonical. + * + * This is `sql-driver.ts`'s `nullValueSatisfiesOperator` table, entry for entry, + * with two deliberate differences that come from THIS file's emitter rather than + * from a different reading of #5146: + * + * - `$null` / `$exists` are read by TRUTHINESS here, because + * {@link compileOperator} writes them as `val ? … : …`. `driver-sql` reads + * them by identity against `false` because its emitter does. Each guard + * matches its own emitter — that is the invariant, not the literal test. + * - `$between` exists in this compiler and not in that table; it is a + * positive comparison, so it takes the default (a value that is not there + * does not lie between two bounds) exactly as the other comparisons do. + * + * The default is the large positive-comparison family (`$gt`/`$in`/`$contains`/ + * …), every member of which answers `false` for a value that is not there. An + * operator this compiler does not support also lands here; it is guarded and + * then still throws from {@link compileOperator}, so fail-closed is preserved. + */ +function nullValueSatisfiesOperator(op: string, value: unknown): boolean { + switch (op) { + // `$eq: null` IS the null predicate; any other comparand is a value test. + case '$eq': return value === null; + // Mirror image: `$ne: null` compiles to `IS NOT NULL`, which a NULL fails. + case '$ne': return value !== null; + // Truthiness, matching this file's emitter (see the note above). + case '$null': return Boolean(value); + case '$exists': return !value; + // Negative-polarity set / substring tests hold vacuously for an absent value. + case '$nin': return true; + // `$notContains` is the one operator where the two JS backends disagree for + // a null-valued field (`driver-memory` answers false, `formula` true). + // `formula` is followed because `driver-sql` follows it, so this compiler + // does not cast a vote on a disagreement that is filed elsewhere. + case '$notContains': return true; + default: return false; + } +} + +/** Is this operator's compiled SQL already total for a NULL column? */ +function operatorIsNullTotal(op: string, value: unknown): boolean { + switch (op) { + // Compile to `IS NULL` / `IS NOT NULL` — two-valued by construction. + case '$null': + case '$exists': + return true; + // A null comparand makes these null PREDICATES too, not comparisons. + case '$eq': + case '$ne': + return value === null; + default: + return false; + } +} + +/** + * The guard one field constraint needs. A constraint is the AND of its + * operators, so it is total when every operator is, and a NULL column satisfies + * it only when it satisfies all of them. + */ +function nullGuardForFieldSpec(spec: unknown): NullGuard { + // `{ field: null }` compiles to `IS NULL` — already total. + if (spec === null) return 'none'; + // A scalar / Date is an implicit `=`; a NULL column fails it. A bare array is + // REFUSED by `compileField`; classifying it here keeps that refusal reachable + // (the unrewritten `{field: […]}` conjunct still throws its own message). + if (typeof spec !== 'object' || spec instanceof Date || Array.isArray(spec)) return 'requireValue'; + const entries = Object.entries(spec as Record); + // `{ field: {} }` and any non-`$` key are shapes `compileField` throws on. + // Passing them through unrewritten is what preserves the exact error; a guard + // wrapped around them would only change which message the caller sees. + if (entries.length === 0) return 'none'; + let total = true; + let nullSatisfies = true; + for (const [op, value] of entries) { + if (!operatorIsNullTotal(op, value)) total = false; + if (!nullValueSatisfiesOperator(op, value)) nullSatisfies = false; + } + if (total) return 'none'; + return nullSatisfies ? 'allowNull' : 'requireValue'; +} + +/** + * [#5146] Rewrite the operand of a `$not` so every leaf compiles to a TOTAL + * predicate — which is what makes `NOT (…)` mean here what it means in + * `driver-memory`, `formula` and (since #5296) `driver-sql`. + * + * # Why the guard rides the LEAF, not the `NOT` + * + * For a flat operand `NOT (a IS NOT NULL AND a = ?)` and `NOT (a = ?) OR a IS + * NULL` are the same predicate. They stop being the same as soon as the operand + * nests: hoisting the guard above a `$not` whose operand is a `$or` re-admits + * rows the JS backends exclude — a NULL `a` would satisfy the whole negation + * even when the `$or`'s OTHER branch is satisfied. Totalising each leaf makes + * the rewrite compositional instead: De Morgan is sound over two-valued leaves, + * so `$and`, `$or` and a nested `$not` all stay correct with no special cases. + * On an RLS lowering that difference is rows a policy excludes becoming visible, + * so it is the whole reason this is a rewrite and not a suffix. + * + * # Why polarity is per operator + * + * A blanket `OR col IS NULL` would WIDEN the negative-polarity operators: + * `{$not: {a: {$ne: 5}}}` means "a is 5", and both JS backends exclude a NULL + * row from it. Adding an unconditional null escape there would hand back exactly + * the rows the scope excludes. So each leaf is guarded in the direction its own + * operator answers, per {@link nullValueSatisfiesOperator}. + * + * The rewrite runs ONLY inside a `$not`; an ordinary comparison's SQL is + * untouched, so nothing outside a negation changes shape. A nested `$not` is + * left alone on purpose — its own branch totalises its operand, and + * `NOT ` is itself total, so recursing would stack a redundant guard on + * the same column. + */ +function nullSafeNegationOperand(node: Record): Record { + const out: Record = {}; + const guarded: unknown[] = []; + for (const [key, value] of Object.entries(node)) { + if ((key === '$and' || key === '$or') && Array.isArray(value)) { + // A non-node element is passed through so `compileNode` still rejects it. + out[key] = value.map((element) => (isFilterNode(element) ? nullSafeNegationOperand(element) : element)); + continue; + } + if (key.startsWith('$')) { + // `$not` (handled by its own branch) and anything else `$`-prefixed keep + // whatever this compiler does with them today — the rewrite rules on NULL, + // not on the operator vocabulary, and an unknown one must still throw. + out[key] = value; + continue; + } + const guard = nullGuardForFieldSpec(value); + if (guard === 'none') { + out[key] = value; + } else if (guard === 'requireValue') { + // `col IS NOT NULL AND (…)` — both conjuncts of the enclosing node. + guarded.push({ [key]: { $null: false } }, { [key]: value }); + } else { + // `col IS NULL OR (…)` — one conjunct, so the OR binds tighter than the + // AND this node's keys form. + guarded.push({ $or: [{ [key]: { $null: true } }, { [key]: value }] }); + } + } + if (guarded.length > 0) { + const existing = Array.isArray(out.$and) ? out.$and : []; + out.$and = [...existing, ...guarded]; + } + return out; +}