From d3b4e7f4759ac97b3361ac02eed33cc011265e4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=8C=85=E5=91=A8=E6=B6=9B?= Date: Sun, 12 Jul 2026 23:54:59 -0700 Subject: [PATCH] fix(driver-sql): fail-loud on unknown filter operators; real IS NULL / IS NOT NULL; $not (#2704) The array-format where path forwarded any unrecognised operator straight to Knex; on a null comparand that silently became a whole-table match, so a permission/assignment-scoped list view could leak every row (an is_null / is_empty operator from the client). The object-format path silently degraded an unknown $op to equality and had no $null handling. - Render null predicates as real IS NULL / IS NOT NULL, unified with equals+null; != null -> IS NOT NULL. - Support the full spec operator set + client aliases in both filter shapes, incl. $between / $startsWith / $endsWith / $notContains / $null / $exists and the logical $not (negated sub-condition; CEL !expr scopes compile to it). - LIKE-escape contains/startsWith/endsWith with explicit ESCAPE '\'. - Throw on a genuinely unknown operator in BOTH paths (fail-loud, no leak). - spec: recognise client alias spellings (isnull/is_empty/...) -> $null. Adds sql-driver-null-operators.test.ts (17 cases across both paths). Closes #2704 --- .../driver-sql-fail-loud-null-operators.md | 31 +++ .../src/sql-driver-null-operators.test.ts | 119 ++++++++++ packages/plugins/driver-sql/src/sql-driver.ts | 222 +++++++++++++++--- packages/spec/src/data/filter.zod.ts | 17 +- 4 files changed, 355 insertions(+), 34 deletions(-) create mode 100644 .changeset/driver-sql-fail-loud-null-operators.md create mode 100644 packages/plugins/driver-sql/src/sql-driver-null-operators.test.ts diff --git a/.changeset/driver-sql-fail-loud-null-operators.md b/.changeset/driver-sql-fail-loud-null-operators.md new file mode 100644 index 0000000000..10ffae1b84 --- /dev/null +++ b/.changeset/driver-sql-fail-loud-null-operators.md @@ -0,0 +1,31 @@ +--- +"@objectstack/driver-sql": patch +"@objectstack/spec": patch +--- + +fix(driver-sql): fail-loud on unknown filter operators; real IS NULL / IS NOT NULL; $not support (#2704) + +The SQL driver used to forward any filter operator it didn't recognise straight +to Knex. On a null comparand that silently compiled to a whole-table match, so a +permission/assignment-scoped list view could leak every row (e.g. an +`is_null` / `is_empty` operator from the client). It also had no real +null-check: `field = null` never renders `IS NULL` in SQL. + +This change makes the driver: + +- Render null predicates as real SQL — `is_null` / `isnull` / `is_empty` + (and the not-null variants) → `IS NULL` / `IS NOT NULL`, unified with + `equals` + null; `!= null` → `IS NOT NULL`. +- Support the full spec operator set plus client alias spellings across both + filter shapes (array `[field, op, value]` and object `{field: {$op: value}}`): + `$between`, `$startsWith`, `$endsWith`, `$notContains`, `$null`, `$exists`, + and the logical `$not` (a negated sub-condition, matching driver-mongodb / + driver-memory — CEL `!expr` permission scopes compile to it). +- LIKE-escape `contains` / `startsWith` / `endsWith` values with an explicit + `ESCAPE '\'` so `%` / `_` in user input can't widen the match. +- **Throw on a genuinely unknown operator** in both paths instead of silently + passing it through — no more silent whole-table results. + +`@objectstack/spec` recognises the client alias operator spellings +(`isnull` / `is_empty` / …) in `VALID_AST_OPERATORS` and maps them to `$null` +so the array-AST → object-filter conversion is consistent with the driver. diff --git a/packages/plugins/driver-sql/src/sql-driver-null-operators.test.ts b/packages/plugins/driver-sql/src/sql-driver-null-operators.test.ts new file mode 100644 index 0000000000..938eee2976 --- /dev/null +++ b/packages/plugins/driver-sql/src/sql-driver-null-operators.test.ts @@ -0,0 +1,119 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { SqlDriver } from '../src/index.js'; + +/** + * Regression tests for issue #2704 — driver-sql must give IS NULL / IS NOT NULL + * filters a real SQL rendering and must NOT forward an unknown operator to Knex + * verbatim (which silently returned the whole table on a null comparand — a + * filter-bypass on permission/assignment-scoped list views). + */ +describe('SqlDriver — null / empty operators (#2704)', () => { + let driver: SqlDriver; + + beforeEach(async () => { + driver = new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }); + + const k = (driver as any).knex; + await k.schema.createTable('tasks', (t: any) => { + t.string('id').primary(); + t.string('title'); + t.string('assignee').nullable(); + }); + + await k('tasks').insert([ + { id: '1', title: 'A', assignee: 'alice' }, + { id: '2', title: 'B', assignee: null }, + { id: '3', title: 'C', assignee: 'carol' }, + { id: '4', title: 'D', assignee: null }, + ]); + }); + + afterEach(async () => { + await driver.disconnect(); + }); + + const ids = (rows: any[]) => rows.map((r) => r.id).sort(); + + describe('array-format where', () => { + it('equals + null → IS NULL (baseline that already worked)', async () => { + const rows = await driver.find('tasks', { where: [['assignee', '=', null]] } as any); + expect(ids(rows)).toEqual(['2', '4']); + }); + + it.each(['is_null', 'isnull', 'is_empty'])('%s → IS NULL', async (op) => { + const rows = await driver.find('tasks', { where: [['assignee', op, true]] } as any); + expect(ids(rows)).toEqual(['2', '4']); + }); + + it.each(['is_not_null', 'isnotnull', 'is_not_empty'])('%s → IS NOT NULL', async (op) => { + const rows = await driver.find('tasks', { where: [['assignee', op, true]] } as any); + expect(ids(rows)).toEqual(['1', '3']); + }); + + it('!= null → IS NOT NULL (not a `<> NULL` that matches nothing)', async () => { + const rows = await driver.find('tasks', { where: [['assignee', '!=', null]] } as any); + expect(ids(rows)).toEqual(['1', '3']); + }); + + it('unknown operator throws instead of returning the whole table', async () => { + await expect( + driver.find('tasks', { where: [['assignee', 'totally_bogus', null]] } as any), + ).rejects.toThrow(/Unsupported filter operator/); + }); + + it('count with is_null is scoped, not the whole table', async () => { + const count = await driver.count('tasks', { where: [['assignee', 'isnull', true]] } as any); + expect(count).toBe(2); + }); + }); + + describe('object-format where ($-operators)', () => { + it('$null: true → IS NULL', async () => { + const rows = await driver.find('tasks', { where: { assignee: { $null: true } } } as any); + expect(ids(rows)).toEqual(['2', '4']); + }); + + it('$null: false → IS NOT NULL', async () => { + const rows = await driver.find('tasks', { where: { assignee: { $null: false } } } as any); + expect(ids(rows)).toEqual(['1', '3']); + }); + + it('$ne: null → IS NOT NULL', async () => { + const rows = await driver.find('tasks', { where: { assignee: { $ne: null } } } as any); + expect(ids(rows)).toEqual(['1', '3']); + }); + + it('$startsWith → prefix LIKE', async () => { + const rows = await driver.find('tasks', { where: { assignee: { $startsWith: 'a' } } } as any); + expect(ids(rows)).toEqual(['1']); + }); + + it('$regex (better-auth contains) → substring LIKE, not exact match', async () => { + const rows = await driver.find('tasks', { where: { assignee: { $regex: 'aro' } } } as any); + expect(ids(rows)).toEqual(['3']); + }); + + it('$not (CEL `!expr` scope filter) → negated sub-condition, not a bogus "$not" column', async () => { + // `!(assignee == 'alice')` → { $not: { assignee: { $eq: 'alice' } } }. + // SQL `NOT (assignee = 'alice')` excludes alice AND is null-safe only for + // the rows it can evaluate — rows 2/4 have null assignee so `NOT (null = 'alice')` + // is UNKNOWN and they are excluded, leaving carol. + const rows = await driver.find('tasks', { + where: { $not: { assignee: { $eq: 'alice' } } }, + } as any); + expect(ids(rows)).toEqual(['3']); + }); + + it('unknown $-operator throws instead of a silent equality compare', async () => { + await expect( + driver.find('tasks', { where: { assignee: { $bogus: 1 } } } as any), + ).rejects.toThrow(/Unsupported filter operator/); + }); + }); +}); diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 95c545f1ea..c44d23218b 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -2972,34 +2972,7 @@ export class SqlDriver implements IDataDriver { const localField = this.mapSortField(fieldRaw); const field = this.remoteColumn(table, fieldRaw, localField); const coerced = this.coerceFilterValue(table, localField, value); - const apply = (b: any) => { - const method = nextJoin === 'or' ? 'orWhere' : 'where'; - const methodIn = nextJoin === 'or' ? 'orWhereIn' : 'whereIn'; - const methodNotIn = nextJoin === 'or' ? 'orWhereNotIn' : 'whereNotIn'; - - if (op === 'contains') { - this.applyContainsLike(b, method, field, value); - return; - } - - switch (op) { - case '=': - b[method](field, coerced); - break; - case '!=': - b[method](field, '<>', coerced); - break; - case 'in': - b[methodIn](field, coerced); - break; - case 'nin': - b[methodNotIn](field, coerced); - break; - default: - b[method](field, op, coerced); - } - }; - apply(builder); + this.applyAstComparison(builder, nextJoin, field, op, value, coerced); } else { const method = nextJoin === 'or' ? 'orWhere' : 'where'; (builder as any)[method]((qb: any) => { @@ -3021,9 +2994,140 @@ export class SqlDriver implements IDataDriver { * but the explicit clause is correct for all three). */ private applyContainsLike(builder: any, method: string, field: string, value: unknown): void { + this.applyLike(builder, method, field, value, 'contains'); + } + + /** + * Parameterized `LIKE`/`NOT LIKE` match with the LIKE metacharacters `%` / `_` + * (and the escape char `\`) escaped in the user value so they match literally + * — otherwise a value of `%` matches every row (a filter-bypass, P0). Binds an + * explicit `ESCAPE '\'` because SQLite does not honour a default escape + * character (MySQL/Postgres do, but the explicit clause is correct for all + * three). `shape` positions the wildcard: `contains` → `%v%`, `starts` → `v%`, + * `ends` → `%v`. + */ + private applyLike( + builder: any, + method: string, + field: string, + value: unknown, + shape: 'contains' | 'starts' | 'ends', + negate = false, + ): void { const escaped = String(value).replace(/[\\%_]/g, '\\$&'); + const pattern = shape === 'starts' ? `${escaped}%` : shape === 'ends' ? `%${escaped}` : `%${escaped}%`; + const keyword = negate ? 'NOT LIKE' : 'LIKE'; const rawMethod = method.startsWith('or') ? 'orWhereRaw' : 'whereRaw'; - builder[rawMethod]('?? LIKE ? ESCAPE ?', [field, `%${escaped}%`, '\\']); + builder[rawMethod](`?? ${keyword} ? ESCAPE ?`, [field, pattern, '\\']); + } + + /** + * Apply one comparison node from the array-format (`[field, op, value]`) + * `where` to the Knex builder, honouring the operator whitelist from + * `@objectstack/spec` (`VALID_AST_OPERATORS`) plus the alias spellings the + * ObjectUI client emits (`isnull` / `isnotnull` / `is_empty`, …). + * + * Why this is NOT a thin `builder.where(field, op, value)` passthrough + * (issue #2704): an unrecognised operator used to be forwarded to Knex + * verbatim. Knex then either rejected it with a 400 (`is_empty` → + * "operator not permitted", blanking the whole grid) or — when the comparand + * was `null` — silently compiled a clause that matched EVERY row + * (`isnull` / `is`). On a permission- or assignment-scoped list view that + * silent full-table scan is a data leak, strictly worse than an error. So + * null predicates compile to a real `IS NULL` / `IS NOT NULL` (unified with + * the `{field, equals, null}` path), and any operator off the whitelist + * throws instead of ever reaching Knex. + */ + protected applyAstComparison( + builder: any, + join: 'and' | 'or', + field: string, + op: string, + rawValue: unknown, + coerced: unknown, + ): void { + const where = join === 'or' ? 'orWhere' : 'where'; + const whereNull = join === 'or' ? 'orWhereNull' : 'whereNull'; + const whereNotNull = join === 'or' ? 'orWhereNotNull' : 'whereNotNull'; + const opLower = String(op).toLowerCase(); + + switch (opLower) { + // Equality — 2-arg form so Knex renders `IS NULL` for a null comparand, + // keeping the `{field, equals, null}` path working. + case '=': + case '==': + builder[where](field, coerced); + return; + case '!=': + case '<>': + // `<> NULL` matches nothing; a null comparand means "has any value". + if (coerced == null) builder[whereNotNull](field); + else builder[where](field, '<>', coerced); + return; + case '>': + case '>=': + case '<': + case '<=': + case 'like': + case 'ilike': + builder[where](field, opLower, coerced); + return; + case 'in': + builder[join === 'or' ? 'orWhereIn' : 'whereIn'](field, coerced as any[]); + return; + case 'nin': + case 'not_in': + case 'notin': + builder[join === 'or' ? 'orWhereNotIn' : 'whereNotIn'](field, coerced as any[]); + return; + case 'between': { + const arr = Array.isArray(coerced) ? coerced : []; + if (arr.length !== 2) { + throw new Error(`[sql-driver] operator "between" on field "${field}" requires a [min, max] value array.`); + } + builder[join === 'or' ? 'orWhereBetween' : 'whereBetween'](field, arr as [any, any]); + return; + } + case 'contains': + this.applyLike(builder, where, field, rawValue, 'contains'); + return; + case 'notcontains': + case 'not_contains': + this.applyLike(builder, where, field, rawValue, 'contains', true); + return; + case 'startswith': + case 'starts_with': + this.applyLike(builder, where, field, rawValue, 'starts'); + return; + case 'endswith': + case 'ends_with': + this.applyLike(builder, where, field, rawValue, 'ends'); + return; + // Null / empty predicates — value-independent, unified with `equals`+null. + case 'is_null': + case 'isnull': + case 'is_empty': + case 'isempty': + case 'empty': + builder[whereNull](field); + return; + case 'is_not_null': + case 'isnotnull': + case 'is_not_empty': + case 'isnotempty': + case 'not_empty': + case 'notempty': + case 'is_set': + case 'set': + builder[whereNotNull](field); + return; + default: + throw new Error( + `[sql-driver] Unsupported filter operator "${op}" on field "${field}". Supported operators: ` + + `=, !=, <, <=, >, >=, in, nin, between, contains, not_contains, starts_with, ends_with, ` + + `is_null, is_not_null (see @objectstack/spec VALID_AST_OPERATORS).`, + ); + } } protected applyFilterCondition(builder: Knex.QueryBuilder, condition: any, logicalOp: 'and' | 'or' = 'and', tableHint?: string | null) { @@ -3048,6 +3152,17 @@ export class SqlDriver implements IDataDriver { }); } }); + } else if (key === '$not' && value !== null && typeof value === 'object' && !Array.isArray(value)) { + // Spec LOGICAL_OPERATORS declares `$not` alongside `$and`/`$or`; both + // driver-mongodb and driver-memory implement it, and CEL `!expr` in a + // permission/scope rule compiles to `{ $not: {...} }` (cel-to-filter.ts). + // Without this branch `$not` fell through to the field handler, was + // treated as a column named "$not", and produced wrong SQL — the same + // class of silent filter-bypass this fix (issue #2704) closes. + const notMethod = logicalOp === 'or' ? 'orWhereNot' : 'whereNot'; + (builder as any)[notMethod]((qb: any) => { + this.applyFilterCondition(qb, value, 'and', table); + }); } else if (typeof value === 'object' && value !== null && !Array.isArray(value)) { const localField = this.mapSortField(key); const field = this.remoteColumn(table, key, localField); @@ -3059,7 +3174,9 @@ export class SqlDriver implements IDataDriver { (builder as any)[method](field, coerced); break; case '$ne': - (builder as any)[method](field, '<>', coerced); + // `<> NULL` matches nothing; a null comparand means "has any value". + if (coerced == null) (builder as any)[logicalOp === 'or' ? 'orWhereNotNull' : 'whereNotNull'](field); + else (builder as any)[method](field, '<>', coerced); break; case '$gt': (builder as any)[method](field, '>', coerced); @@ -3084,10 +3201,53 @@ export class SqlDriver implements IDataDriver { break; } case '$contains': + // `$regex` reaches SQL only via the better-auth adapter, which emits + // it for a `contains` search (a plain substring, not a real regex). + // SQL has no portable regex, so compile the intended substring LIKE + // — correct for that producer and safe (the value is LIKE-escaped), + // where the old equality default silently made it an exact match. + case '$regex': this.applyContainsLike(builder, method, field, opValue); break; + case '$notContains': + this.applyLike(builder, method, field, opValue, 'contains', true); + break; + case '$startsWith': + this.applyLike(builder, method, field, opValue, 'starts'); + break; + case '$endsWith': + this.applyLike(builder, method, field, opValue, 'ends'); + break; + case '$between': { + const arr = Array.isArray(coerced) ? coerced : []; + if (arr.length !== 2) { + throw new Error(`[sql-driver] operator "$between" on field "${field}" requires a [min, max] value array.`); + } + (builder as any)[logicalOp === 'or' ? 'orWhereBetween' : 'whereBetween'](field, arr as [any, any]); + break; + } + // `{ $null: true }` → IS NULL, `{ $null: false }` → IS NOT NULL. + // Also the SQL rendering of the AST `is_null`/`is_not_null` operators + // (spec `parseFilterAST` maps those to `$null`). Previously this fell + // to the equality default and compiled `field = true`, silently + // returning the wrong rows (issue #2704). + case '$null': + (builder as any)[opValue === false + ? (logicalOp === 'or' ? 'orWhereNotNull' : 'whereNotNull') + : (logicalOp === 'or' ? 'orWhereNull' : 'whereNull')](field); + break; + // Mongo `$exists`: a present field is a non-null column in SQL. + case '$exists': + (builder as any)[opValue === false + ? (logicalOp === 'or' ? 'orWhereNull' : 'whereNull') + : (logicalOp === 'or' ? 'orWhereNotNull' : 'whereNotNull')](field); + break; default: - (builder as any)[method](field, coerced); + throw new Error( + `[sql-driver] Unsupported filter operator "${op}" on field "${field}". Supported operators: ` + + `$eq, $ne, $gt, $gte, $lt, $lte, $in, $nin, $between, $contains, $notContains, ` + + `$startsWith, $endsWith, $regex, $null, $exists.`, + ); } } } else { diff --git a/packages/spec/src/data/filter.zod.ts b/packages/spec/src/data/filter.zod.ts index 6428ffa609..7d3468e389 100644 --- a/packages/spec/src/data/filter.zod.ts +++ b/packages/spec/src/data/filter.zod.ts @@ -356,7 +356,12 @@ export const VALID_AST_OPERATORS = new Set([ 'startswith', 'starts_with', 'endswith', 'ends_with', 'between', + // Null / empty predicates. `is_null` / `is_not_null` are canonical; `isnull`, + // `isnotnull`, `is_empty`, `is_not_empty` are the alias spellings the ObjectUI + // `data-objectstack` adapter emits and the driver-sql/#2704 fix accepts — kept + // in sync here so `parseFilterAST()` never treats them as an unknown operator. 'is_null', 'is_not_null', + 'isnull', 'isnotnull', 'is_empty', 'is_not_empty', ]); /** @@ -434,6 +439,10 @@ const AST_OPERATOR_MAP: Record = { 'between': '$between', 'is_null': '$null', 'is_not_null': '$null', + 'isnull': '$null', + 'isnotnull': '$null', + 'is_empty': '$null', + 'is_not_empty': '$null', }; /** @@ -448,11 +457,13 @@ function convertComparison(node: [string, string, unknown]): FilterCondition { return { [field]: value } as FilterCondition; } - // Null check operators - if (op === 'is_null') { + // Null / empty predicates — direction comes from the operator NAME, not the + // (filler) value: the ObjectUI client sends a truthy placeholder value for + // both `isnull` and `isnotnull`, so keying off `value` would collapse them. + if (op === 'is_null' || op === 'isnull' || op === 'is_empty') { return { [field]: { $null: true } } as FilterCondition; } - if (op === 'is_not_null') { + if (op === 'is_not_null' || op === 'isnotnull' || op === 'is_not_empty') { return { [field]: { $null: false } } as FilterCondition; }