From e5b886c2c3eb56e27c2f2f131ee46d46fbd69854 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 16:07:50 +0000 Subject: [PATCH] fix(analytics): refuse an `undefined` comparand in a read scope instead of binding it (#6125) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `compileScopedFilterToSql` compiled a comparand-position `undefined` into legal SQL with `undefined` in the bind list. The driver renders that as NULL, every comparison against NULL is UNKNOWN, and the read scope matched ZERO rows with no log line — indistinguishable from a scope that worked. Re-measured on d8e8d9cbc with the refusal disabled, alias `t`: { d: undefined } -> "t"."d" = ? [undefined] { d: { $gt: undefined } } -> "t"."d" > ? [undefined] { d: { $in: [undefined] } } -> "t"."d" IN (?) [undefined] { $not: { d: undefined } } -> NOT (("t"."d" IS NOT NULL AND "t"."d" = ?)) [undefined] One gate at the top of `compileField` — after `quoteIdent`, before any `bind()` — refuses all four positions with ONE wording that varies only by path (#5240). The envelope is this module's existing `READ_SCOPE_COMPILE_FAILED` / 500, not #6050's `INVALID_FILTER` / 400: a read scope is compiled by the platform from CEL and stored metadata, so a 400 would bill the caller for something they neither wrote nor can change. `null` is untouched — SQL and binds byte for byte, pinned as its own control group, because refusing it alongside `undefined` is the way this change could do harm. Per the #6125 ruling, `@objectstack/formula` (a third semantics, #5299), `driver-memory` / `driver-mongodb` (#5499 freeze) and `driver-sql` / `driver-turso` (#6050, already landed) are deliberately not touched. Fixes #6125 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WyvqvKMG6asi9aXjKE6xtx --- .../read-scope-undefined-comparand-refusal.md | 42 +++ .../read-scope-refusal-envelope.test.ts | 43 ++- .../read-scope-undefined-comparand.test.ts | 271 ++++++++++++++++++ .../service-analytics/src/comparand-shape.ts | 10 + .../service-analytics/src/read-scope-sql.ts | 172 +++++++++++ 5 files changed, 527 insertions(+), 11 deletions(-) create mode 100644 .changeset/read-scope-undefined-comparand-refusal.md create mode 100644 packages/services/service-analytics/src/__tests__/read-scope-undefined-comparand.test.ts diff --git a/.changeset/read-scope-undefined-comparand-refusal.md b/.changeset/read-scope-undefined-comparand-refusal.md new file mode 100644 index 0000000000..4da6a605b5 --- /dev/null +++ b/.changeset/read-scope-undefined-comparand-refusal.md @@ -0,0 +1,42 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(analytics): read scope 里的 `undefined` 比较数改为拒收，不再编成绑了 `undefined` 的合法 SQL (#6125) + +**⚠️ 行为变更。** `compileScopedFilterToSql` 遇到比较数位置上的 `undefined`，从「编出合法 SQL、绑一个 `undefined`、匹配零行、零日志」改为 `READ_SCOPE_COMPILE_FAILED` / **500** 拒收。 + +## 实测到的毛病 + +#6050 于 2026-08-07 裁定（B 案）：比较数位置的 `undefined` 一律拒收，并落在了**已证实可触达**的 `driver-sql` / `driver-turso` 两面。#6125 在同一轮把仓内其余求值面逐格实测，同一个形状拿到五种读法；本条改的是其中一格 —— `service-analytics` 的 `read-scope-sql.ts`。在 `d8e8d9cbc` 上把本次拒收关掉复测，alias `t`、字段 `d`，四格与 #6125 正文表一致： + +| read scope | 编译结果 | 绑定表 | +|---|---|---| +| `{ d: undefined }` | `"t"."d" = ?` | `[undefined]` | +| `{ d: { $gt: undefined } }` | `"t"."d" > ?` | `[undefined]` | +| `{ d: { $in: [undefined] } }` | `"t"."d" IN (?)` | `[undefined]` | +| `{ $not: { d: undefined } }` | `NOT (("t"."d" IS NOT NULL AND "t"."d" = ?))` | `[undefined]` | + +绑定表里是 JS 的 `undefined` 本身，不是 `null`：`applyReadScope`（`native-sql-strategy.ts`）在把 `?` 改写成 `$N` 时原样 `push(scopeParams[i])`。所以 NULL 是**驱动**对一个 JS `undefined` 的读法 —— 同一格在不肯猜的驱动上则是一句裸 `Undefined binding(s)` 崩溃。一次绑定、两种败法，取决于数据源恰好挂的是哪个驱动，这正是它该在编译器处拒收、而不是在某一个消费者处修补的理由。 + +方向与 #6050 不同，如实记：那边是**越权**（`{ owner_id: ctx.user?.id }` 在 Turso remote 上编成 `IS NULL`，匹配全环境行）；这边是 fail-**closed** —— 匹配零行，永远不会多给行。所以它不是潜伏的权限绕过，#6125 也没有按那个级别定级。之所以照样拒收：一个「答了没人问的问题、且一条日志都不报」的 read scope，与一个真的生效了的 read scope 在外部完全无法区分。本次改动的价值就是把沉默变成响亮。 + +## 修法 + +一道闸落在 `compileField` 的开头 —— 在 `quoteIdent` 之后（不安全标识符是注入向量，保留它自己的措辞与优先级），在任何 `bind()` 之前。 + +拒收的**位置**逐个清点，因为「比较数」是位置而不是类型：直接比较数（`{ d: undefined }`）、单值算子的比较数（`$eq`/`$ne`/`$gt`/`$gte`/`$lt`/`$lte` 与 LIKE 族）、列表算子数组的**成员**（`$in`/`$nin`/`$between`）。四格共用**一条**措辞，只有 `path` 不同（#5240「一个条件，一种措辞」）。 + +信封沿用本模块自述的那一个（`READ_SCOPE_COMPILE_FAILED` / 500），不是 #6050 的 `INVALID_FILTER` / 400：read scope 的 filter 由平台自己从 CEL 与库存 metadata 编译而来，不是调用方输入 —— 报 400 等于让调用方去修一个他既没写、也改不动的东西。消息里指名要修的是**生产者**（管理员写的共享规则 / 权限集、它的 CEL 下降、或进程内拼这条 FilterCondition 的代码），并按 #5367 只进日志、不进响应体。 + +三个位置**故意不扫**，各自因为本模块已经用更贴切的诊断拒了它：`$null` / `$exists`（比较数是声明的布尔量，不是比较数位置）、直接位置上的裸数组（`compileField` 整体拒「用 `{ $in: [...] }`」）、以及约束对象里的非 `$` 键（那是嵌套关系，改写成 `null` 一样编不过 —— 这一条是与 `driver-sql` 孪生实现的唯一有意分歧，来自本模块拒收嵌套关系，而不是对 #6050 的另一种读法）。 + +## ⛔ `null` 一字未动 + +`{ d: null }` / `{ $eq: null }` → `IS NULL`；`{ $ne: null }` → `IS NOT NULL`；`$null` / `$exists`、`$in: [null]`、`$nin: [null]`、`$between: [null, 5]`、`$contains: null`（`%null%`，#5526）、以及 `$not` 下的各式 —— SQL 与绑定表逐字节不变。这是本次改动唯一可能造成伤害的方向（模块里每张极性表都只用一个 `===` 把 `null` 与 `undefined` 分开），所以它有自己的对照组回归 pin。 + +## 刻意不动的邻居 + +- ⛔ `@objectstack/formula` 把同一个 `undefined` 读作「这个键在记录里不存在」—— 那是**第三种语义**，不是第三个 bug 拼写，也正是 #5299 在争的问题。在这里顺手改掉等于替 #5299 拍板。 +- ⛔ `driver-memory` / `driver-mongodb` 维持 #5499 投入冻结，只 pin 不改。后果是本编译器与 `driver-memory` 在这一格上从此不一致 —— 这是裁决接受的代价，解冻时一并还，账记在 #6125。 +- ⛔ `driver-sql` / `driver-turso` 已由 #6050 落地，未触碰。 diff --git a/packages/services/service-analytics/src/__tests__/read-scope-refusal-envelope.test.ts b/packages/services/service-analytics/src/__tests__/read-scope-refusal-envelope.test.ts index 4e3b436991..ba60e1edb1 100644 --- a/packages/services/service-analytics/src/__tests__/read-scope-refusal-envelope.test.ts +++ b/packages/services/service-analytics/src/__tests__/read-scope-refusal-envelope.test.ts @@ -41,6 +41,18 @@ * code and all ten fail with `code` / `status` `undefined`, while the block above * stays green. * + * ## [#6125] An eleventh site, and why it is registered HERE + * + * The `undefined` comparand refusal (row ⑥) was added on 2026-08-07 by #6125's + * ruling. It is in this table for the invariant at the bottom of the file rather + * than for its own sake: what #5352 cost was a module where SOME refusals + * carried the envelope, which is indistinguishable from none of them at the HTTP + * boundary. So the rule this table encodes is that the inventory grows whenever + * the module gains a refusing site — a new `throw` that is not listed here is + * the defect returning. Row ⑥'s own behaviour (four comparand positions, and the + * `null` control group that must NOT move) is pinned in + * `read-scope-undefined-comparand.test.ts`; only its ENVELOPE is asserted here. + * * The end-to-end consequence — that the REST face answers * `500 ANALYTICS_QUERY_FAILED` with the policy content WITHHELD from the body and * intact in `logError` — is `packages/rest`'s @@ -69,7 +81,7 @@ function refusalFor(filter: unknown, alias = 'crm_opportunity'): Refusal | undef /** * Every refusing site in `read-scope-sql.ts`, in source order. * - * ELEVEN rows over TEN throw sites: `quoteIdent` is one site reached with two + * TWELVE rows over ELEVEN throw sites: `quoteIdent` is one site reached with two * `kind` values, and both are listed on purpose. That alias-vs-field split was * option C on #5367's decision card — the one place where the two triggers are * genuinely different (a bad alias is OUR generator, a bad field is the admin's @@ -129,42 +141,49 @@ const REFUSALS: Array<{ sensitive: '$nor', }, { - name: '⑥ bare array value', + name: '⑥ undefined in a comparand position', + site: 'compileField: undefined comparand', + filter: { owner_id: undefined }, + message: /comparand at "owner_id" is undefined — refusing to build read scope \(fail-closed\)/, + sensitive: 'owner_id', + }, + { + name: '⑦ bare array value', site: 'compileField: bare array value', filter: { region_code: ['emea', 'apac'] }, message: /bare array value for "region_code" — use \{ \$in: \[\.\.\.\] \} \(fail-closed\)/, sensitive: 'region_code', }, { - name: '⑦ nested / relation value', + name: '⑧ nested / relation value', site: 'compileField: nested/relation value', filter: { owner: { manager_id: 'u1' } }, message: /"owner" has a nested\/relation value which is not supported in a read scope \(fail-closed\)/, sensitive: 'owner', }, { - name: '⑧ $in without an array', + name: '⑨ $in without an array', site: 'compileOperator: $in needs an array', filter: { region_code: { $in: 'emea' } }, message: /\$in for "region_code" needs an array \(fail-closed\)/, sensitive: 'region_code', }, { - name: '⑨ $nin without an array', + name: '⑩ $nin without an array', site: 'compileOperator: $nin needs an array', filter: { region_code: { $nin: 'emea' } }, message: /\$nin for "region_code" needs an array \(fail-closed\)/, sensitive: 'region_code', }, { - name: '⑩ $between without [min, max]', + name: '⑪ $between without [min,max]', site: 'compileOperator: $between needs [min,max]', filter: { credit_limit: { $between: [10] } }, message: /\$between for "credit_limit" needs \[min,max\] \(fail-closed\)/, sensitive: 'credit_limit', }, { - name: '⑪ unsupported operator', + name: '⑫ unsupported operator', site: 'compileOperator: unsupported operator', filter: { owner_email: { $regex: 'admin@' } }, message: /unsupported operator "\$regex" on "owner_email" \(fail-closed\)/, @@ -255,10 +274,12 @@ describe('[#5367] every read-scope refusal carries the ADR-0112 envelope (READ_S // #5352's lesson, stated as a guard: seven of `filter-normalizer.ts`'s nine // sites carrying an envelope was indistinguishable from none of them at the // HTTP boundary, because the commonest input hit one of the two bare ones. - // Eleven inputs over the module's ten throw sites (see the table's note on - // `quoteIdent`), and every one of them enveloped. - expect(REFUSALS).toHaveLength(11); - expect(new Set(REFUSALS.map((c) => c.site)).size).toBe(10); + // Twelve inputs over the module's ELEVEN throw sites (see the table's note + // on `quoteIdent`), and every one of them enveloped. [#6125] added the + // eleventh site; these two numbers are the ratchet that makes a future + // unenveloped `throw` fail HERE instead of at an HTTP boundary. + expect(REFUSALS).toHaveLength(12); + expect(new Set(REFUSALS.map((c) => c.site)).size).toBe(11); for (const c of REFUSALS) { expect(refusalFor(c.filter, c.alias)?.code, `${c.site} is still bare`).toBe('READ_SCOPE_COMPILE_FAILED'); } diff --git a/packages/services/service-analytics/src/__tests__/read-scope-undefined-comparand.test.ts b/packages/services/service-analytics/src/__tests__/read-scope-undefined-comparand.test.ts new file mode 100644 index 0000000000..1a729e0742 --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/read-scope-undefined-comparand.test.ts @@ -0,0 +1,271 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#6125, PM ruling 2026-08-07] `undefined` in a comparand position of a READ + * SCOPE is refused — `READ_SCOPE_COMPILE_FAILED` / 500 — and `null` is not. + * + * ## What was wrong + * + * #6050 ruled (2026-08-07, ruling B) that `undefined` sitting where a comparand + * belongs is refused, and landed that on `driver-sql` / `driver-turso`, the two + * surfaces where the shape was PROVEN reachable (`{ owner_id: ctx.user?.id }`). + * #6125 measured the rest of the repo in the same round and found five readings + * of one shape; this compiler was the cell that compiled **legal SQL with an + * `undefined` in the bind list**. Measured here on `d8e8d9cbc` with the refusal + * disabled, alias `t`: + * + * | read scope | compiled to | bind list | + * |---|---|---| + * | `{ d: undefined }` | `"t"."d" = ?` | `[undefined]` | + * | `{ d: { $gt: undefined } }` | `"t"."d" > ?` | `[undefined]` | + * | `{ d: { $in: [undefined] } }` | `"t"."d" IN (?)` | `[undefined]` | + * | `{ $not: { d: undefined } }` | `NOT (("t"."d" IS NOT NULL AND "t"."d" = ?))` | `[undefined]` | + * + * `applyReadScope` pushes those binds through verbatim, so the driver renders + * them as NULL, every comparison against NULL is UNKNOWN, and the scope matched + * ZERO rows — with no log line anywhere. Fail-CLOSED, so unlike #6050 this was + * never a latent permission bypass; the defect being fixed is that a read scope + * answering a question nobody asked is indistinguishable from one that worked. + * + * ## The two halves of this file, and which one is the change + * + * `describe('the four measured cells …')` is the change: run it against + * pre-#6125 code and all four fail, because all four COMPILE. + * + * `describe('the null control group …')` is the risk. `null` is a declared + * comparand with settled semantics, and the way this change could do harm is by + * refusing it alongside `undefined` — the two live one `===` apart in every + * polarity table in the module. Every row there passes both before and after, + * SQL and binds byte for byte. + * + * `describe('what the sweep deliberately leaves alone')` records the boundary of + * the ruling: the positions that are NOT comparands, and the shapes this module + * already refuses with a truer diagnosis. + * + * ## Scope of the ruling, so a later reader does not "finish the job" + * + * ⛔ `@objectstack/formula` reads the same `undefined` as a THIRD semantics — + * "the key is absent from the record" — which is a meaningful distinction and + * the open question in #5299. It is deliberately untouched; changing it here + * would settle #5299 as a side effect. ⛔ `driver-memory` / `driver-mongodb` + * remain pin-only under the #5499 freeze, so this compiler and `driver-memory` + * now answer this cell differently on purpose (a debt owed at thaw, recorded in + * #6125). The envelope is this module's existing one, NOT #6050's + * `INVALID_FILTER` / 400: a read scope is compiled by the platform from CEL and + * stored metadata, so a 400 would bill the caller for something they neither + * wrote nor can change. + */ + +import { describe, it, expect } from 'vitest'; +import type { FilterCondition } from '@objectstack/spec/data'; +import { compileScopedFilterToSql } from '../read-scope-sql.js'; + +/** The ADR-0112 fields an HTTP boundary classifies on. */ +interface Refusal extends Error { + code?: unknown; + status?: unknown; +} + +const ALIAS = 't'; + +function refusalFor(filter: FilterCondition): Refusal | undefined { + try { + compileScopedFilterToSql(filter, ALIAS); + return undefined; + } catch (e) { + return e as Refusal; + } +} + +/** + * The four cells #6125's table measured — one per comparand POSITION, which is + * why the list is four and not one. `path` is the only thing the message varies + * on (#5240: one condition, one wording). + */ +const REFUSED: Array<{ name: string; filter: FilterCondition; path: string; wasSql: string }> = [ + { + name: 'the DIRECT comparand — { d: undefined }', + filter: { d: undefined }, + path: '"d"', + wasSql: '"t"."d" = ?', + }, + { + name: "an OPERATOR's comparand — { d: { $gt: undefined } }", + filter: { d: { $gt: undefined } }, + path: '"d".$gt', + wasSql: '"t"."d" > ?', + }, + { + name: 'a LIST MEMBER — { d: { $in: [undefined] } }', + filter: { d: { $in: [undefined] } }, + path: '"d".$in[0]', + wasSql: '"t"."d" IN (?)', + }, + { + name: 'a leaf under $not — { $not: { d: undefined } }', + filter: { $not: { d: undefined } }, + path: '"d"', + wasSql: 'NOT (("t"."d" IS NOT NULL AND "t"."d" = ?))', + }, +]; + +/** + * `null` comparands — the control group, and the reason it is this long. + * + * Every one of these is a DECLARED comparand whose meaning is settled, and every + * one of them sits one `===` away from the value being refused: the module's + * emitter arms (`$eq`/`$ne`), `operatorIsNullTotal` and + * `nullValueSatisfiesOperator` all branch on `value === null`. A refusal + * written one character wider takes this whole table with it, and — because + * `IS NULL` lowering is what an RLS policy uses to scope unowned rows — it would + * take it with a 500 on a policy that is correct. + * + * The `$not` rows are here for the second failure mode: the #5146 rewrite + * reaches leaves through `nullSafeNegationOperand`, so a guard placed on the + * wrong side of it changes the SHAPE rather than throwing, which no + * throw-assertion would catch. + */ +const NULL_CONTROL: Array<{ name: string; filter: FilterCondition; sql: string; params: unknown[] }> = [ + { name: '{ d: null } — the implicit null predicate', filter: { d: null }, sql: '"t"."d" IS NULL', params: [] }, + { name: '{ $eq: null }', filter: { d: { $eq: null } }, sql: '"t"."d" IS NULL', params: [] }, + { name: '{ $ne: null }', filter: { d: { $ne: null } }, sql: '"t"."d" IS NOT NULL', params: [] }, + { name: '{ $in: [null] } — null is a bindable MEMBER', filter: { d: { $in: [null] } }, sql: '"t"."d" IN (?)', params: [null] }, + { + name: '{ $nin: [null] } — NULL-safe negative (#5298)', + filter: { d: { $nin: [null] } }, + sql: '("t"."d" IS NULL OR "t"."d" NOT IN (?))', + params: [null], + }, + { + name: '{ $between: [null, 5] } — null is a bindable BOUND', + filter: { d: { $between: [null, 5] } }, + sql: '"t"."d" BETWEEN ? AND ?', + params: [null, 5], + }, + { name: '{ $not: { d: null } } — already total, no guard added', filter: { $not: { d: null } }, sql: 'NOT ("t"."d" IS NULL)', params: [] }, + { + name: '{ $not: { d: { $ne: null } } } — the #5146 rewrite still leaves it alone', + filter: { $not: { d: { $ne: null } } }, + sql: 'NOT ("t"."d" IS NOT NULL)', + params: [], + }, + { name: '{ $null: true }', filter: { d: { $null: true } }, sql: '"t"."d" IS NULL', params: [] }, + { name: '{ $exists: false }', filter: { d: { $exists: false } }, sql: '"t"."d" IS NULL', params: [] }, + { + name: 'a $contains of null still renders %null% (#5526)', + filter: { d: { $contains: null } }, + sql: '"t"."d" LIKE ? ESCAPE ?', + params: ['%null%', '\\'], + }, +]; + +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#6125] the four measured cells are REFUSED, in this module’s own envelope', () => { + for (const c of REFUSED) { + it(`refuses ${c.name}`, () => { + const err = refusalFor(c.filter); + expect(err, `it still COMPILED — pre-#6125 this lowered to ${c.wasSql}`).toBeInstanceOf(Error); + expect(err?.code).toBe('READ_SCOPE_COMPILE_FAILED'); + // 500, not #6050's 400: this filter is compiled by the platform from CEL + // and stored metadata, so the caller is not its author and cannot fix it. + expect(err?.status).toBe(500); + expect(err?.code).not.toBe('INVALID_FILTER'); + }); + + it(`names the position it refused: ${c.path}`, () => { + expect(String(refusalFor(c.filter)?.message)).toContain(`comparand at ${c.path} is undefined`); + }); + + it(`no longer emits its old lowering: ${c.wasSql}`, () => { + // The other direction of the same fact. A refusal that threw for some + // unrelated reason would satisfy the assertions above; this one fails if + // the pre-#6125 SQL ever comes back, whatever else the module does. + let sql: string | undefined; + try { + sql = compileScopedFilterToSql(c.filter, ALIAS).sql; + } catch { + sql = undefined; + } + expect(sql).toBeUndefined(); + }); + } + + it('says it ONE way — only the path differs (#5240)', () => { + // Four positions, four `path`s, one sentence. If a later change gives one of + // them its own bespoke wording, this is the line that objects. + const skeletons = REFUSED.map((c) => + String(refusalFor(c.filter)?.message).replace(`comparand at ${c.path} is`, 'comparand at is'), + ); + expect(new Set(skeletons).size).toBe(1); + expect(skeletons[0]).toContain('comparand at is undefined'); + }); + + it('the message tells the operator to fix the PRODUCER, not the caller', () => { + // The disclosure/attribution half of #5367: the message is for the log, and + // what it must not do is send a tenant to "fix their request". + const msg = String(refusalFor(REFUSED[0].filter)?.message); + expect(msg).toContain('never the caller of this query'); + expect(msg).toContain('sharing rule'); + }); +}); + +describe('[#6125] the null control group is UNTOUCHED — SQL and binds, byte for byte', () => { + for (const c of NULL_CONTROL) { + it(`still compiles: ${c.name}`, () => { + const out = compileScopedFilterToSql(c.filter, ALIAS); + expect(out.sql).toBe(c.sql); + expect(out.params).toEqual(c.params); + }); + } + + it('null and undefined are told apart at the SAME position', () => { + // The pair, side by side, on the one position where confusing them is + // cheapest: `null` IS the null predicate, `undefined` is refused. + expect(compileScopedFilterToSql({ d: null }, ALIAS).sql).toBe('"t"."d" IS NULL'); + expect(refusalFor({ d: undefined })?.code).toBe('READ_SCOPE_COMPILE_FAILED'); + }); +}); + +describe('[#6125] what the sweep deliberately leaves alone', () => { + it('$null / $exists take a declared BOOLEAN — not a comparand position', () => { + // Their comparand is a flag, so `undefined` there is not the shape #6050 + // ruled on. `driver-sql`'s twin skips them for the same reason — but it can + // hand them to a boolean-domain gate (#5347 / #5369) and THIS module has + // none, so today they lower by truthiness. That is a different cell, + // measured and filed as #6387 rather than decided as a rider here; when it + // is ruled on, these two lines are what changes. + expect(compileScopedFilterToSql({ d: { $null: undefined } }, ALIAS).sql).toBe('"t"."d" IS NOT NULL'); + expect(compileScopedFilterToSql({ d: { $exists: undefined } }, ALIAS).sql).toBe('"t"."d" IS NULL'); + }); + + it('a bare array keeps its OWN refusal, undefined member or not', () => { + const err = refusalFor({ d: [1, undefined] }); + expect(err?.code).toBe('READ_SCOPE_COMPILE_FAILED'); + expect(String(err?.message)).toContain('bare array value for "d"'); + }); + + it('a nested relation keeps its OWN refusal — the truer diagnosis', () => { + // Relabelling this as "the comparand is undefined" would send the operator + // to write `null`, and `{ owner: { manager_id: null } }` does not compile + // either. The one deliberate divergence from `driver-sql`'s twin. + const err = refusalFor({ owner: { manager_id: undefined } }); + expect(err?.code).toBe('READ_SCOPE_COMPILE_FAILED'); + expect(String(err?.message)).toContain('has a nested/relation value'); + }); + + it('the boolean identities still reduce — the refusal is not reached through them', () => { + // `{}`-shaped constants have no comparand at all, so nothing here changes. + expect(compileScopedFilterToSql({ $and: [] }, ALIAS).sql).toBe(''); + expect(compileScopedFilterToSql({ $or: [] }, ALIAS).sql).toBe('1 = 0'); + expect(compileScopedFilterToSql({ $not: {} }, ALIAS).sql).toBe('1 = 0'); + }); + + it('an undefined leaf is still reached when a SIBLING is a boolean identity', () => { + // The evaluation-order trap #5348 / #5327 named on the driver side: this + // compiler `.map()`s every child into its own buffer BEFORE applying the + // `$or` TRUE-absorption, so a `{}` disjunct cannot hide a malformed one. + expect(refusalFor({ $or: [{}, { d: undefined }] })?.code).toBe('READ_SCOPE_COMPILE_FAILED'); + expect(refusalFor({ $and: [{}, { d: { $in: [undefined] } }] })?.code).toBe('READ_SCOPE_COMPILE_FAILED'); + }); +}); diff --git a/packages/services/service-analytics/src/comparand-shape.ts b/packages/services/service-analytics/src/comparand-shape.ts index 8f53544012..93aca0b77c 100644 --- a/packages/services/service-analytics/src/comparand-shape.ts +++ b/packages/services/service-analytics/src/comparand-shape.ts @@ -83,6 +83,16 @@ export function isBindableComparand(value: unknown): boolean { * no `undefined`) and {@link comparand} already normalises it to `null` rather * than refusing it (#5526, #5332) — refusing it here would invent a * disagreement instead of closing one. + * + * ⚠️ [#6125] `undefined` no longer REACHES either predicate from the read-scope + * door: `read-scope-sql.ts` refuses a comparand-position `undefined` upstream of + * both, per #6050's ruling B pushed down by #6125. The branch stays because + * these two predicates are a value-for-value mirror of `driver-sql`'s twins + * (held by `__tests__/like-metacharacter-escape.test.ts`), and those keep it for + * exactly the same reason — refused upstream there too, since #6050. Narrowing + * the fence here would break the mirror without removing a reachable answer. + * The `where` door is unaffected either way: {@link comparand} still normalises + * `undefined` to `null` before either predicate sees it. */ export function isRenderableTextComparand(value: unknown): boolean { if (value === null || value === undefined) return true; diff --git a/packages/services/service-analytics/src/read-scope-sql.ts b/packages/services/service-analytics/src/read-scope-sql.ts index 8cc5c5544a..cebe0c420c 100644 --- a/packages/services/service-analytics/src/read-scope-sql.ts +++ b/packages/services/service-analytics/src/read-scope-sql.ts @@ -156,6 +156,30 @@ import { * vocabulary had no measured pull), because a 4xx cannot be fixed by the client * and therefore misreports the condition, and because 422 would have left the * disclosure question to be re-decided message by message. + * + * ## An `undefined` comparand is refused, not bound (#6125, PM ruling 2026-08-07) + * + * That makes ELEVEN refusing sites; the envelope above is what all eleven carry, + * and {@link undefinedComparandError} is the eleventh. #6050 ruled on 2026-08-07 + * that `undefined` in a comparand position is refused everywhere (ruling B), and + * implemented it on `driver-sql` / `driver-turso` — the surfaces where the shape + * was PROVEN reachable. This module was measured in the same round and answered + * a fourth way again: legal SQL, one bound NULL, and not a single log line. + * + * The #6125 ruling scoped the push-down to THIS file and kept its own envelope + * (`READ_SCOPE_COMPILE_FAILED` / 500 — see above): a read scope is compiled by + * the platform from CEL and stored metadata, so telling the caller to fix their + * request would name the wrong author. `@objectstack/formula` reads the same + * value as a THIRD semantics ("the key is absent from the record") and is + * deliberately left alone — deciding it here would settle #5299's + * key-missing-vs-value-null question as a side effect — and `driver-memory` / + * `driver-mongodb` stay pin-only under the #5499 freeze. + * + * The eleventh message was measured against `looksLikeInternalErrorLeak` before + * being added, because the section above turns on that predicate answering FALSE + * for this family: it does for all four positions, so the new refusal is + * withheld from the response BY DECLARATION exactly like the other ten, and no + * message-sniffing list learns a twelfth phrase. */ const IDENT = /^[a-z_][a-z0-9_]*$/i; @@ -302,6 +326,12 @@ function compileNode(node: unknown, qAlias: string, params: unknown[]): string { function compileField(field: string, value: unknown, qAlias: string, params: unknown[]): string { const col = `${qAlias}.${quoteIdent(field, 'field')}`; + // [#6125] `undefined` in a comparand position, refused before anything binds — + // and after `quoteIdent`, so an unsafe identifier (the injection vector) keeps + // its own message and its precedence. See {@link assertDefinedComparands} for + // why this one call site covers the whole tree. + assertDefinedComparands(field, value); + // Scalar / null → implicit equality. if (value === null) return `${col} IS NULL`; if (typeof value !== 'object' || value instanceof Date) { @@ -410,6 +440,148 @@ function assertRenderableText(op: string, field: string, val: unknown): void { throw readScopeCompileError(`[read-scope-sql] ${unrenderableTextComparandMessage(op, field, val)}`); } +/** + * [#6125, PM ruling 2026-08-07] `undefined` in a COMPARAND position. + * + * ONE wording for all four positions #6125 measured (#5240 — one condition, one + * wording); only `path` varies, because only the position does. What the four + * had in common is why a shared sentence is right rather than merely shorter: + * every one of them compiled to legal SQL with the JS value `undefined` in the + * bind list, which the external driver renders as NULL — and every comparison + * against NULL is UNKNOWN, so the scope matched ZERO rows and said nothing. + * + * Re-measured on `origin/main` (`d8e8d9cbc`) with the refusal disabled, alias + * `t`, field `d` — the same four rows #6125's table recorded on `cba7454df`: + * + * | read scope | compiled to | bind list | + * |---|---|---| + * | `{ d: undefined }` | `"t"."d" = ?` | `[undefined]` | + * | `{ d: { $gt: undefined } }` | `"t"."d" > ?` | `[undefined]` | + * | `{ d: { $in: [undefined] } }` | `"t"."d" IN (?)` | `[undefined]` | + * | `{ $not: { d: undefined } }` | `NOT (("t"."d" IS NOT NULL AND "t"."d" = ?))` | `[undefined]` | + * + * ⚠️ `[undefined]`, not `[null]` — one correction to the issue's table. Nothing + * in this package coerces it: `applyReadScope` (`native-sql-strategy.ts`) pushes + * `scopeParams[i]` into the driver's bind array verbatim while it renumbers + * `?` → `$N`. So the NULL is the DRIVER's reading of a JS `undefined`, which is + * also why the same cell reads as a bare `Undefined binding(s)` crash on the + * drivers that refuse to guess (#6050's LOCAL column). Two failure modes from + * one bind, decided by which driver the datasource happens to be — the reason + * this is refused at the compiler and not repaired at any one consumer. + * + * ⛔ What deliberately does NOT move: `null`. `{ d: null }`, `{ $eq: null }`, + * `{ $ne: null }`, `$null` and `$exists` keep their exact lowering — `null` IS a + * declared comparand and IS the null predicate, and the whole point of this + * refusal is the JS value that cannot be told apart from an ABSENT key. Pinned + * as its own control group in `read-scope-undefined-comparand.test.ts`, because + * refusing `null` along with `undefined` is the way this change could do harm. + * + * ## Why the direction here is not #6050's direction + * + * On `driver-sql` the same shape was over-reach: `{ owner_id: ctx.user?.id }` + * with a missing id compiled to `IS NULL` on Turso's remote transport and + * matched every env-wide row. Here it is fail-CLOSED — zero rows, never extra + * rows — so this is not a latent permission bypass and was not graded as one. + * It is refused anyway because a read scope that answers a question nobody asked, + * with no log line, is indistinguishable from one that worked: the value of this + * change is turning silence into noise, which is exactly the grading #6125's + * ruling recorded. + */ +function undefinedComparandError(field: string, path: string): Error { + return readScopeCompileError( + `[read-scope-sql] comparand at ${path} is undefined — refusing to build read scope (fail-closed). ` + + `@objectstack/spec FieldOperatorsSchema declares no undefined comparand, and in JavaScript a key ` + + `whose value is undefined cannot be told apart from an ABSENT key — yet the two mean OPPOSITE ` + + `things (a predicate versus no constraint at all), so there is no reading of it that is not a ` + + `guess. It used to compile: undefined went into the bind list, the driver read it as SQL NULL, ` + + `every comparison against NULL is UNKNOWN, and the scope matched ZERO rows in silence. Write null if the null ` + + `predicate was meant ({ "${field}": null } or { "${field}": { "$null": true } }), or omit the key ` + + `when the value is genuinely absent. The producer to fix is whoever BUILT this read scope — an ` + + `admin-authored sharing rule / permission set, its CEL lowering, or the in-process code that ` + + `assembled the FilterCondition — never the caller of this query, who cannot author it (#6050 ` + + `ruling B, pushed down to this compiler by #6125).`, + ); +} + +/** + * [#6125] Refuse every `undefined` sitting in a comparand position of ONE field + * constraint. + * + * The positions are enumerated rather than swept, because "comparand" is a + * POSITION and not a type: + * + * - the DIRECT comparand — `{ d: undefined }`, the implicit `=`; + * - an OPERATOR's comparand — `{ d: { $gt: undefined } }`, `$eq`, `$ne`, the + * LIKE family, every other single-value operator; + * - a MEMBER of a list operator's array — `{ d: { $in: [undefined] } }`, + * `$nin`, `$between`. The array itself IS `$in`'s legitimate comparand; + * each element is a comparand in its own right, which is the same split + * {@link assertCompilableMembers} already makes. + * + * Three positions are deliberately NOT swept, each because this module already + * refuses the enclosing shape with a TRUER diagnosis — #5240's rule read in the + * direction that matters here, since a second wording for a shape that is + * refused either way only sends the operator to the wrong repair: + * + * - `$null` / `$exists`. Their comparand is a declared BOOLEAN — a flag, not a + * value to compare against — so `undefined` there is not a comparand at all. + * `driver-sql`'s twin skips them for the same reason. ⚠️ Unlike that twin, + * THIS module has no boolean-domain gate to hand them to (#5347 / #5369 were + * never pushed down here): `{ $null: undefined }` still lowers by truthiness + * to `IS NOT NULL` and `{ $null: "false" }` — the STRING, which is truthy — + * lands on the side opposite the `false` it was written to mean. That is a + * different cell, measured and filed as #6387 rather than decided as a rider + * on this one. + * - a bare ARRAY in direct comparand position (`{ d: [1, undefined] }`). + * {@link compileField} refuses the array as a whole ("use `{ $in: [...] }`"), + * and inspecting its members here would relabel a shape refused either way. + * - a NON-`$` key inside the constraint object (`{ owner: { manager_id: + * undefined } }`). That is a nested relation, which {@link compileField} + * refuses outright; answering "the comparand is undefined" would send the + * operator to write `null` there, and `{ owner: { manager_id: null } }` does + * not compile either. This is the one deliberate divergence from + * `driver-sql`'s twin, and it comes from THIS module refusing nested + * relations — not from a different reading of #6050. + * + * ## Why the call site is {@link compileField} and not a pre-pass + * + * `driver-sql` refuses on its separate validating walk because its emitter + * short-circuits: a boolean identity can resolve an enclosing node before a + * malformed sibling is ever visited, so a gate in the emitter would be + * conditional on evaluation order. THIS compiler has no such blind spot — + * {@link compileNode} `.map()`s every `$and`/`$or` child into its own buffer + * BEFORE any identity is applied (the `$or` TRUE-absorption and the `$and` + * identity filter both read the fully-compiled list), and + * {@link nullSafeNegationOperand} rewrites a `$not` operand without dropping a + * single leaf. Every comparand therefore reaches `compileField`, which is also + * the only path to {@link bind} — one gate, on the one road. + * + * The other half of `driver-sql`'s "runs FIRST" argument does not transfer + * either, and that is worth stating rather than copying: there, the refusal had + * to precede the `$not` rewrite because the polarity tables spelled `=== null` + * while the `$ne` emitter spelled `== null`, so the two disagreed about + * `undefined` itself. Here {@link nullValueSatisfiesOperator}, + * {@link operatorIsNullTotal} and every arm of {@link compileOperator} spell it + * `=== null` alike, so the tables and the emitter agree that `undefined` is "a + * value" — the rewrite for a `{ $not: … }` operand runs, produces a leaf, and + * that leaf is refused. Nothing inconsistent is being outrun; the silent NULL + * bind is. + */ +function assertDefinedComparands(field: string, spec: unknown): void { + const root = `"${field}"`; + if (spec === undefined) throw undefinedComparandError(field, root); + if (!isFilterNode(spec)) return; + for (const [op, opValue] of Object.entries(spec)) { + if (!op.startsWith('$') || op === '$null' || op === '$exists') continue; + const opPath = `${root}.${op}`; + if (opValue === undefined) throw undefinedComparandError(field, opPath); + if (!Array.isArray(opValue)) continue; + opValue.forEach((member, index) => { + if (member === undefined) throw undefinedComparandError(field, `${opPath}[${index}]`); + }); + } +} + function compileOperator(col: string, op: string, val: unknown, field: string, params: unknown[]): string { switch (op) { case '$eq': return val === null ? `${col} IS NULL` : `${col} = ${bind(params, val)}`;