diff --git a/.changeset/analytics-like-escape.md b/.changeset/analytics-like-escape.md new file mode 100644 index 0000000000..ce101ae687 --- /dev/null +++ b/.changeset/analytics-like-escape.md @@ -0,0 +1,39 @@ +--- +"@objectstack/service-analytics": patch +--- + +fix(service-analytics): the three SQL compilers compare LIKE values literally (#5567) + +`$contains` / `$notContains` / `$startsWith` / `$endsWith` build a `LIKE` pattern +around the comparand the author wrote. All three of this package's SQL compilers +concatenated that comparand straight into a wildcard position — no escaping, no +`ESCAPE` clause — so `_` (LIKE's single-character wildcard) and `%` (its +multi-character one) stopped being literals. Measured on real SQLite, over the +rows `x_admin` / `xyadmin` / `off 50% now` / `off 5012 now`: + +| `where` | returned | correct | +|----------------------------------|---------------|---------| +| `{name: {$contains: '_admin'}}` | `['1','2']` | `['1']` | +| `{name: {$contains: '50%'}}` | `['3','4']` | `['3']` | +| `{name: {$startsWith: 'x_'}}` | `['1','2']` | `['1']` | +| `{name: {$endsWith: '0% now'}}` | `['3','4']` | `['3']` | + +Every row is a **widening** — rows the author excluded came back — and +`$notContains` is the mirror image, excluding rows the author kept. One of the +three call sites is the ADR-0021 D-C read-scope (tenant + RLS) lowering, where a +wider predicate is over-reach rather than a loose filter (the #5347 / #5324 +ruling on that same file). Prime Directive #3 forces machine names to +`snake_case`, so essentially every machine-name comparand carries a `_` and hit +this silently. + +All three compilers now escape the comparand and bind an explicit +`ESCAPE` argument, matching what `driver-sql`'s `applyLike` has always done — so +the same filter selects the same rows whichever strategy answers, and the +`/analytics/sql` echo describes the statement that ran instead of a wider one. + +**No authoring change.** A comparand with no `_`, `%` or `\` binds exactly the +pattern it bound before; only its meaning when it *does* carry one changes, from +wildcard to literal. If you were relying on a comparand acting as a wildcard, +that was never a declared capability of these operators — the spec describes them +as substring / prefix / suffix matches — and `driver-sql` already read it +literally, so the reading you got depended on which strategy served the query. diff --git a/packages/plugins/driver-sql/src/sql-driver.ts b/packages/plugins/driver-sql/src/sql-driver.ts index 11703e6fe9..58d87efeeb 100644 --- a/packages/plugins/driver-sql/src/sql-driver.ts +++ b/packages/plugins/driver-sql/src/sql-driver.ts @@ -6272,6 +6272,16 @@ export class SqlDriver implements IDataDriver { * character (MySQL/Postgres do, but the explicit clause is correct for all * three). `shape` positions the wildcard: `contains` → `%v%`, `starts` → `v%`, * `ends` → `%v`. + * + * **Second implementation, deliberately** (#5567): + * `packages/services/service-analytics/src/like-pattern.ts` carries the same + * transform — same escaped character class, same three shapes, same bound + * `ESCAPE` — because `service-analytics` depends on no driver and this is a + * private method taking a knex builder, so there is nothing for it to import. + * That file's header explains the choice; it is held to THIS expression, character + * for character, by `service-analytics`'s `like-metacharacter-escape.test.ts`. + * A third hand-copy is the thing to refuse: import from one of the two, or add + * a consumer to that test. */ private applyLike( builder: any, diff --git a/packages/services/service-analytics/src/__tests__/like-metacharacter-escape.test.ts b/packages/services/service-analytics/src/__tests__/like-metacharacter-escape.test.ts new file mode 100644 index 0000000000..9d8e9f5edb --- /dev/null +++ b/packages/services/service-analytics/src/__tests__/like-metacharacter-escape.test.ts @@ -0,0 +1,437 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5567] The analytics SQL compilers compare LIKE values LITERALLY. + * + * Three compilers in this package built a LIKE pattern by concatenating the + * author's comparand straight into a wildcard position, with no escaping and no + * `ESCAPE` clause: + * + * - `read-scope-sql.ts`'s `compileOperator` — the ADR-0021 D-C read scope + * (tenant + RLS) lowering; + * - `native-sql-strategy.ts`'s `likePattern` — the statement that actually + * EXECUTES; + * - `objectql-strategy.ts`'s `LIKE_SQL_OPS` — the `/analytics/sql` echo, whose + * only reason to exist is reproducing execution (#5333). + * + * `_` is LIKE's single-character wildcard and `%` its multi-character one, so + * every comparand carrying one meant something other than what the author wrote. + * Measured on real SQLite before the fix, through `compileScopedFilterToSql`: + * + * | `where` | got | should be | + * |-------------------------------------|----------------|-----------| + * | `{name: {$contains: '_admin'}}` | `['1','2']` | `['1']` | + * | `{name: {$contains: '50%'}}` | `['3','4']` | `['3']` | + * | `{name: {$startsWith: 'x_'}}` | `['1','2']` | `['1']` | + * | `{name: {$endsWith: '0% now'}}` | `['3','4']` | `['3']` | + * | `{name: {$notContains: '_admin'}}` | `['3','4','5','6']` | `['2','3','4','5','6']` | + * + * On the read scope that WIDENING is a permission bypass, not a loose filter — + * the same "amplification is over-reach, not a degraded filter" ruling #5347 / + * #5324 made on this very file. And Prime Directive #3 forces machine names to + * `snake_case`, which guarantees that essentially every machine-name comparand + * carries at least one `_`. + * + * ## The direction this file pins, per case + * + * Four of the five rows above are plain **before-red / after-green**: the + * pre-fix compilers hand back rows the author excluded (or, for `$notContains`, + * exclude rows the author kept), and the exact-set assertions below fail on the + * pre-fix tree. + * + * The literal-backslash case is **deliberately NOT claimed as before-red on + * SQLite**, and saying so is the point rather than a caveat. `\` is only a + * metacharacter once an escape character is in force: SQLite has no default one + * (which is why `driver-sql` binds an explicit `ESCAPE`), so a pre-fix pattern + * `%a\b%` matched the literal `a\b` there by accident. The same pre-fix pattern + * on Postgres/MySQL — where `\` IS the default escape character — reads `\b` as + * a literal `b` and matches `ab` instead: a miss AND a false hit, in one + * comparand. So the backslash row is pinned at the layer where the fix is + * visible on the engine this suite can run (the bound pattern string) plus a + * row-set non-regression, and the dialect half is argued from the documentation + * quoted in the PR rather than asserted here. + * + * That asymmetry is also why the two halves of the fix must land together: the + * escaping ALONE would break SQLite (`%\_admin%` with no escape character in + * force is a search for a literal backslash), and the `ESCAPE` clause alone + * changes nothing. Both, or neither. + * + * ## Why `sql.js` + * + * Same reason as `read-scope-sql-conformance.test.ts` and + * `native-sql-filter-logic-conformance.test.ts`: `better-sqlite3`'s native + * binding is loadable only by the exact Node ABI it was built for and aborts the + * vitest worker on CI's Node. `sql.js` is the pure-WASM engine `driver-sql` + * itself falls back to. + */ + +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 { NativeSQLStrategy } from '../strategies/native-sql-strategy.js'; +import { ObjectQLStrategy } from '../strategies/objectql-strategy.js'; +import { compileScopedFilterToSql } from '../read-scope-sql.js'; +import { escapeLikePattern, likePattern, LIKE_ESCAPE_CHAR } from '../like-pattern.js'; + +/** + * The issue's four measured rows, plus a pair for the escape character itself. + * + * Each metacharacter row is paired with a DECOY that differs from it by exactly + * the metacharacter's wildcard reading, so every assertion is an exact set and + * cannot pass by luck: + * + * - `x_admin` / `xyadmin` — `_` as "any single character". + * - `off 50% now` / `off 5012 now` — `%` as "any run of characters". + * - `p a\b q` / `p ab q` — `\` as an escape PREFIX, which drops itself and + * makes the next character literal. + */ +const FIXTURE = [ + { id: '1', name: 'x_admin' }, + { id: '2', name: 'xyadmin' }, + { id: '3', name: 'off 50% now' }, + { id: '4', name: 'off 5012 now' }, + { id: '5', name: 'p a\\b q' }, + { id: '6', name: 'p ab q' }, +]; + +const ALL_IDS = ['1', '2', '3', '4', '5', '6']; + +const CUBE: Cube = { + name: 'people', + title: 'People', + sql: 'person', + measures: { total: { name: 'total', label: 'Total', type: 'count', sql: '*' } }, + dimensions: { + id: { name: 'id', label: 'Id', type: 'string', sql: 'id' }, + name: { name: 'name', label: 'Name', type: 'string', sql: 'name' }, + }, + public: false, +} as unknown as Cube; + +const query = (where: unknown): AnalyticsQuery => + ({ + cube: 'people', + measures: ['total'], + dimensions: ['id'], + timezone: 'UTC', + where, + }) as AnalyticsQuery; + +/** 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; + } +} + +describe('[#5567] analytics LIKE compilers escape their comparand', () => { + let db: any; + /** `ctx` for the raw-SQL strategy: `$n` → `?`, exactly as `plugin.ts` bridges. */ + let nativeCtx: StrategyContext; + /** `ctx` for the ObjectQL strategy — used for its `generateSql` echo only. */ + let echoCtx: StrategyContext; + + 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 "person" ("id" TEXT PRIMARY KEY, "name" TEXT);`); + const insert = db.prepare(`INSERT INTO "person" ("id","name") VALUES (?,?)`); + for (const r of FIXTURE) insert.run([r.id, r.name]); + insert.free(); + + const base = { + getCube: (name: string) => (name === 'people' ? CUBE : undefined), + queryCapabilities: () => ({ nativeSql: true, objectqlAggregate: true, inMemory: false }), + }; + nativeCtx = { + ...base, + executeRawSql: async (_object: string, sql: string, params: unknown[]) => run(sql, params).rows, + } as StrategyContext; + echoCtx = { ...base } as StrategyContext; + }); + + afterAll(() => { + db?.close(); + }); + + /** Run a `$n`-placeholder statement on the fixture, as every consumer does. */ + const run = (sql: string, params: unknown[]): { rows: Record[]; ids: string[] } => { + const stmt = db.prepare(sql.replace(/\$\d+/g, '?')); + stmt.bind(params as any[]); + const rows: Record[] = []; + while (stmt.step()) rows.push(stmt.getAsObject()); + stmt.free(); + return { rows, ids: rows.map((r) => String(r.id)).sort((a, b) => a.localeCompare(b)) }; + }; + + // ── The helper the three compilers now share ──────────────────────────────── + + describe('escapeLikePattern', () => { + it('escapes the two wildcards and the escape character itself', () => { + expect(escapeLikePattern('_admin')).toBe('\\_admin'); + expect(escapeLikePattern('50%')).toBe('50\\%'); + expect(escapeLikePattern('a_b')).toBe('a\\_b'); + expect(escapeLikePattern('a\\b')).toBe('a\\\\b'); + }); + + it('leaves a comparand with no metacharacter byte-identical', () => { + expect(escapeLikePattern('plain')).toBe('plain'); + expect(escapeLikePattern('a.b')).toBe('a.b'); + }); + + it('is the same transform `driver-sql`\'s `applyLike` applies', () => { + // Mirrored, not imported: `service-analytics` depends on no driver (see + // its package.json). The expression is duplicated HERE, in a test, so the + // two implementations are held to each other by a failing assertion + // rather than by two TSDoc comments alone. + const driverSqlEscape = (v: unknown) => String(v).replace(/[\\%_]/g, '\\$&'); + for (const v of ['_admin', '50%', 'a\\b', 'plain', '%_\\', '']) { + expect(escapeLikePattern(v), JSON.stringify(v)).toBe(driverSqlEscape(v)); + } + }); + + it('positions the wildcard by shape', () => { + expect(likePattern('contains', '_admin')).toBe('%\\_admin%'); + expect(likePattern('starts', '_admin')).toBe('\\_admin%'); + expect(likePattern('ends', '_admin')).toBe('%\\_admin'); + }); + }); + + // ── Binding layer: the issue's measured table, all three compilers ────────── + + describe("the issue's binding table — every compiler binds the ESCAPED pattern", () => { + /** The pattern + escape char `read-scope-sql` binds for one operator. */ + const readScopeBinds = (op: string, value: string): unknown[] => + compileScopedFilterToSql({ name: { [op]: value } } as FilterCondition, 'person').params; + + /** The params the EXECUTED statement binds. */ + const nativeBinds = async (op: string, value: string): Promise => + (await new NativeSQLStrategy().generateSql(query({ name: { [op]: value } }), nativeCtx)).params; + + /** The params the `/analytics/sql` echo shows. */ + const echoBinds = async (op: string, value: string): Promise => + (await new ObjectQLStrategy().generateSql(query({ name: { [op]: value } }), echoCtx)).params; + + const TABLE: Array<{ op: string; value: string; pattern: string }> = [ + { op: '$contains', value: '_admin', pattern: '%\\_admin%' }, + { op: '$startsWith', value: '_admin', pattern: '\\_admin%' }, + { op: '$endsWith', value: '_admin', pattern: '%\\_admin' }, + { op: '$contains', value: '50%', pattern: '%50\\%%' }, + { op: '$contains', value: 'a_b', pattern: '%a\\_b%' }, + { op: '$notContains', value: '_admin', pattern: '%\\_admin%' }, + { op: '$contains', value: 'a\\b', pattern: '%a\\\\b%' }, + ]; + + for (const { op, value, pattern } of TABLE) { + it(`${op} ${JSON.stringify(value)} binds ${JSON.stringify(pattern)} + the escape char, on all three`, async () => { + const expected = [pattern, LIKE_ESCAPE_CHAR]; + expect(readScopeBinds(op, value), 'read-scope-sql').toEqual(expected); + expect(await nativeBinds(op, value), 'native-sql-strategy').toEqual(expected); + expect(await echoBinds(op, value), 'objectql-strategy echo').toEqual(expected); + }); + } + + it('declares the ESCAPE clause in the SQL of all three — escaping alone breaks SQLite', () => { + // Half a fix is a different bug: a pattern carrying `\_` with no escape + // character in force is a search for a literal backslash, so on SQLite + // (no default escape char) escaping without the clause returns NO rows. + const { sql } = compileScopedFilterToSql({ name: { $contains: 'x' } } as FilterCondition, 'person'); + expect(sql).toBe('"person"."name" LIKE ? ESCAPE ?'); + }); + + it('a comparand with no metacharacter binds the pattern it always did', async () => { + // Zero-regression: the only change for an ordinary value is the appended + // ESCAPE clause, which is a no-op for a pattern that carries no `\`. + for (const [op, pattern] of [ + ['$contains', '%plain%'], + ['$startsWith', 'plain%'], + ['$endsWith', '%plain'], + ['$notContains', '%plain%'], + ] as Array<[string, string]>) { + expect(readScopeBinds(op, 'plain')[0], `read-scope ${op}`).toBe(pattern); + expect((await nativeBinds(op, 'plain'))[0], `native ${op}`).toBe(pattern); + expect((await echoBinds(op, 'plain'))[0], `echo ${op}`).toBe(pattern); + } + }); + }); + + // ── Row sets: the issue's measured table, on real SQLite ──────────────────── + + describe("the issue's row table — read-scope-sql, exact sets", () => { + /** Rows a read scope admits — `compileScopedFilterToSql` + the fixture. */ + const scopedIds = (scope: FilterCondition): string[] => { + const { sql, params } = compileScopedFilterToSql(scope, 'person'); + return run(`SELECT "id" FROM "person" AS "person" WHERE ${sql}`, params).ids; + }; + + const CASES: Array<{ where: FilterCondition; expected: string[]; was: string }> = [ + // The two rows the issue measured. + { where: { name: { $contains: '_admin' } }, expected: ['1'], was: "['1','2'] — `_` matched the `y` of `xyadmin`" }, + { where: { name: { $contains: '50%' } }, expected: ['3'], was: "['3','4'] — `%` matched the `12` of `off 5012 now`" }, + // The three operators the issue measured only at the binding layer. + { where: { name: { $startsWith: 'x_' } }, expected: ['1'], was: "['1','2']" }, + { where: { name: { $endsWith: '0% now' } }, expected: ['3'], was: "['3','4']" }, + { + where: { name: { $notContains: '_admin' } }, + // The one operator whose direction is NARROWING: it excluded row 2 as + // well, so the author lost a row they kept rather than gaining one. + expected: ['2', '3', '4', '5', '6'], + was: "['3','4','5','6'] — row 2 wrongly excluded", + }, + ]; + + for (const { where, expected, was } of CASES) { + it(`${JSON.stringify(where)} → ${JSON.stringify(expected)} (was ${was})`, () => { + expect(scopedIds(where)).toEqual(expected); + // Belt and braces on the widening direction: the whole fixture back is + // the shape a read scope must never produce (#5347 / #5324). + expect(scopedIds(where).length).toBeLessThan(ALL_IDS.length); + }); + } + + it('the escape character itself compares literally', () => { + // Pinned as a pattern-level fact plus a row-set NON-regression, not as a + // before-red row set: see this file's header. On SQLite `\` is literal + // either way, because there is no default escape character to make it a + // metacharacter; the pre-fix pattern `%a\b%` reads as a literal `b` on + // Postgres/MySQL, where `\` IS the default, and matches row 6 instead. + const { sql, params } = compileScopedFilterToSql( + { name: { $contains: 'a\\b' } } as FilterCondition, + 'person', + ); + expect(params).toEqual(['%a\\\\b%', LIKE_ESCAPE_CHAR]); + expect(run(`SELECT "id" FROM "person" AS "person" WHERE ${sql}`, params).ids).toEqual(['5']); + }); + + it('an ordinary substring still finds its rows', () => { + expect(scopedIds({ name: { $contains: 'admin' } })).toEqual(['1', '2']); + expect(scopedIds({ name: { $startsWith: 'off' } })).toEqual(['3', '4']); + expect(scopedIds({ name: { $endsWith: 'now' } })).toEqual(['3', '4']); + }); + }); + + // ── Why the two halves cannot be separated ────────────────────────────────── + + /** + * The engine's own answers, measured directly rather than through a compiler. + * + * These four rows are why `likePattern` and the `ESCAPE` binding are one fix + * and not two independent improvements. They are asserted here so a later + * "simplification" that drops either half fails with a reason attached instead + * of just moving a row count. + */ + describe('SQLite itself: escaping and the ESCAPE clause are one fix', () => { + const ids = (pattern: string, escape?: string): string[] => + run( + `SELECT "id" FROM "person" WHERE "name" LIKE ?${escape === undefined ? '' : ' ESCAPE ?'}`, + escape === undefined ? [pattern] : [pattern, escape], + ).ids; + + it('neither half alone: raw+no-clause widens, escaped+no-clause returns NOTHING', () => { + // What the compilers did before this change — the issue's measured row. + expect(ids('%_admin%')).toEqual(['1', '2']); + // Escaping ALONE: with no escape character in force, `\_` is a search for + // a literal backslash followed by `_`, which no row contains. Silently + // zero rows is a worse failure than the widening it replaces. + expect(ids('%\\_admin%')).toEqual([]); + // The clause ALONE: nothing in the pattern is escaped, so nothing changes. + expect(ids('%_admin%', '\\')).toEqual(['1', '2']); + // Both: the comparand compares literally. + expect(ids('%\\_admin%', '\\')).toEqual(['1']); + }); + + it('a BOUND escape argument is accepted — it need not be a SQL literal', () => { + // The premise of binding rather than writing `ESCAPE '\'` into the text. + expect(ids('%50\\%%', '\\')).toEqual(['3']); + }); + + it('why `\\` is a dialect divergence pre-fix, simulated on this engine', () => { + // Postgres and MySQL both DEFAULT to `\` as the escape character, so a + // pre-fix `LIKE '%a\b%'` there behaves exactly as the SQLite statement + // with the clause supplied — the middle row below. This is a simulation of + // the documented default, not a run against those engines (this suite has + // neither), and it is the reason the backslash case is not claimed as + // before-red on SQLite. + expect(ids('%a\\b%'), 'SQLite pre-fix: no default escape char, `\\` literal').toEqual(['5']); + expect(ids('%a\\b%', '\\'), 'PG/MySQL pre-fix: `\\b` reads as a literal `b`').toEqual(['6']); + expect(ids('%a\\\\b%', '\\'), 'post-fix: the same row on every dialect').toEqual(['5']); + }); + }); + + // ── Row sets through the statement that actually executes ─────────────────── + + describe('NativeSQLStrategy — the statement that runs', () => { + const ids = async (where: unknown): Promise => { + const result = await new NativeSQLStrategy().execute(query(where), nativeCtx); + return result.rows.map((r) => String(r.id)).sort((a, b) => a.localeCompare(b)); + }; + + it("the issue's two rows, through the executed statement", async () => { + expect(await ids({ name: { $contains: '_admin' } })).toEqual(['1']); + expect(await ids({ name: { $contains: '50%' } })).toEqual(['3']); + }); + + it('the three operators the issue measured only at the binding layer', async () => { + expect(await ids({ name: { $startsWith: 'x_' } })).toEqual(['1']); + expect(await ids({ name: { $endsWith: '0% now' } })).toEqual(['3']); + expect(await ids({ name: { $notContains: '_admin' } })).toEqual(['2', '3', '4', '5', '6']); + }); + + it('an ordinary substring is unaffected', async () => { + expect(await ids({ name: { $contains: 'admin' } })).toEqual(['1', '2']); + }); + }); + + // ── The echo reproduces execution (#3601 / #3602 / #3650, #5333) ──────────── + + describe('the `/analytics/sql` echo agrees with execution', () => { + const echoed = async (where: unknown) => + new ObjectQLStrategy().generateSql(query(where), echoCtx); + + const METACHAR_CASES = [ + { name: { $contains: '_admin' } }, + { name: { $contains: '50%' } }, + { name: { $startsWith: 'x_' } }, + { name: { $endsWith: '0% now' } }, + { name: { $notContains: '_admin' } }, + { name: { $contains: 'a\\b' } }, + ]; + + it('binds the same pattern the executed statement binds', async () => { + for (const where of METACHAR_CASES) { + const echo = await echoed(where); + const native = await new NativeSQLStrategy().generateSql(query(where), nativeCtx); + expect(echo.params, JSON.stringify(where)).toEqual(native.params); + } + }); + + it('and running the echo returns the rows the query returns, not more', async () => { + for (const [where, expected] of [ + [{ name: { $contains: '_admin' } }, ['1']], + [{ name: { $contains: '50%' } }, ['3']], + [{ name: { $startsWith: 'x_' } }, ['1']], + [{ name: { $endsWith: '0% now' } }, ['3']], + [{ name: { $notContains: '_admin' } }, ['2', '3', '4', '5', '6']], + ] as Array<[unknown, string[]]>) { + const { sql, params } = await echoed(where); + expect(run(sql, params).ids, `echo ${JSON.stringify(where)}`).toEqual(expected); + } + }); + + it('renders the ESCAPE clause, so the statement an author copies out runs the same', async () => { + const { sql } = await echoed({ name: { $contains: 'x' } }); + expect(sql).toMatch(/name LIKE \$\d+ ESCAPE \$\d+/); + }); + }); +}); diff --git a/packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts b/packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts index b408cfc92c..23a629a098 100644 --- a/packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts +++ b/packages/services/service-analytics/src/__tests__/objectql-echo-operator-coverage.test.ts @@ -235,24 +235,33 @@ describe('[#5333] `/analytics/sql` echo — every authorable operator renders a // ── The issue's measured table, one case per row ──────────────────────────── describe("the issue's measured table", () => { + /* + * [#5567] Each pattern is now followed by a bound `ESCAPE` argument, so the + * echo describes the comparand `driver-sql` actually compares (its + * `applyLike` has always escaped and bound `ESCAPE`). None of these three + * comparands carries a `_` or `%`, so the PATTERN is byte-identical to what + * #5333 pinned — the second bind is the whole delta. The metacharacter cases, + * where the pattern itself changes and the row set with it, are in + * `like-metacharacter-escape.test.ts`. + */ it('`$startsWith` echoes `LIKE` with the prefix pattern — was no WHERE at all', async () => { const { sql, params } = await echo({ stage: { $startsWith: 'w' } }); expect(sql).toContain('WHERE'); - expect(sql).toContain('stage LIKE $1'); - expect(params).toEqual(['w%']); + expect(sql).toContain('stage LIKE $1 ESCAPE $2'); + expect(params).toEqual(['w%', '\\']); }); it('`$endsWith` echoes `LIKE` with the suffix pattern — was no WHERE at all', async () => { const { sql, params } = await echo({ stage: { $endsWith: 'n' } }); expect(sql).toContain('WHERE'); - expect(sql).toContain('stage LIKE $1'); - expect(params).toEqual(['%n']); + expect(sql).toContain('stage LIKE $1 ESCAPE $2'); + expect(params).toEqual(['%n', '\\']); }); it('`$contains` still echoes the substring pattern — the row that already worked', async () => { const { sql, params } = await echo({ stage: { $contains: 'o' } }); - expect(sql).toContain('stage LIKE $1'); - expect(params).toEqual(['%o%']); + expect(sql).toContain('stage LIKE $1 ESCAPE $2'); + expect(params).toEqual(['%o%', '\\']); }); it('the LIKE patterns are the ones the EXECUTED statement binds', async () => { diff --git a/packages/services/service-analytics/src/__tests__/read-scope-sql.test.ts b/packages/services/service-analytics/src/__tests__/read-scope-sql.test.ts index 4c03d540ec..b4935e6bdb 100644 --- a/packages/services/service-analytics/src/__tests__/read-scope-sql.test.ts +++ b/packages/services/service-analytics/src/__tests__/read-scope-sql.test.ts @@ -53,8 +53,13 @@ describe('compileScopedFilterToSql', () => { it('comparison + string operators', () => { expect(compileScopedFilterToSql({ amount: { $gte: 100 } }, 't').sql).toBe('"t"."amount" >= ?'); + // [#5567] The LIKE family binds its pattern AND the escape character, so the + // comparand compares literally. `'A'` carries no metacharacter, so the + // pattern itself is unchanged — the second bind is the whole delta here. + // Metacharacter coverage (and the row sets) live in + // `like-metacharacter-escape.test.ts`. expect(compileScopedFilterToSql({ name: { $startsWith: 'A' } }, 't')).toEqual({ - sql: '"t"."name" LIKE ?', params: ['A%'], + sql: '"t"."name" LIKE ? ESCAPE ?', params: ['A%', '\\'], }); }); diff --git a/packages/services/service-analytics/src/like-pattern.ts b/packages/services/service-analytics/src/like-pattern.ts new file mode 100644 index 0000000000..c7a1508d12 --- /dev/null +++ b/packages/services/service-analytics/src/like-pattern.ts @@ -0,0 +1,120 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * LIKE pattern construction for this package's three SQL compilers (#5567). + * + * A `$contains` / `$notContains` / `$startsWith` / `$endsWith` comparand is a + * LITERAL the author typed. Concatenating it straight into a wildcard position + * silently reinterprets it as a pattern, because `_` is LIKE's single-character + * wildcard and `%` its multi-character one: + * + * - `{name: {$contains: '_admin'}}` matched `xyadmin` as well as `x_admin`; + * - `{name: {$contains: '50%'}}` matched `off 5012 now` as well as `off 50% now`. + * + * Both directions are WIDENING, and one of the three call sites is + * `read-scope-sql.ts` — the ADR-0021 D-C read-scope (tenant + RLS) lowering, + * where a wider predicate is over-reach rather than a loose filter (#5347 / + * #5324, on that same file). Prime Directive #3 forces machine names to + * `snake_case`, so essentially every machine-name comparand carries a `_` and + * hits this silently. + * + * ## The two halves are one fix + * + * Escaping the value and declaring the escape character are not independent + * steps — either alone is a different bug: + * + * - Escaping alone: `%\_admin%` with no escape character in force is a search + * for a literal backslash. SQLite has NO default escape character, so this + * would return zero rows there. + * - The clause alone: nothing in the pattern is escaped, so nothing changes. + * + * Hence {@link likePattern} always produces a pattern escaped for + * {@link LIKE_ESCAPE_CHAR}, and every emitter pairs it with an `ESCAPE` binding. + * + * ## Why the escape character is BOUND, never written as a literal + * + * Every emitter here binds it as an ordinary placeholder (`LIKE ? ESCAPE ?`) + * rather than writing `ESCAPE '\'` into the SQL text. Two reasons, both load-bearing: + * + * 1. **The literal spelling is not portable.** MySQL applies C escape syntax + * inside string literals — "If you want a LIKE string to contain a literal + * `\`, you must double it" — so the backslash escape character is spelled + * `'\\'` there and `'\'` on SQLite/Postgres. These compilers do not know + * which dialect will run their output. A bound value is escaped by the + * driver for its own dialect, so there is exactly one spelling here. + * 2. **It rides the existing placeholder plumbing.** `read-scope-sql.ts` emits + * `?` and BOTH of its consumers (`NativeSQLStrategy.applyReadScope`, + * `ObjectQLStrategy.generateSql`) renumber `?` → `$N` while pushing the + * matching value. Because the escape character is a bound value it is + * carried by that rewrite with no change at the upper layer — which answers + * the "which layer does the ESCAPE clause belong to" question in the issue: + * the predicate layer, entirely, because nothing above it has to know. + * + * Dialect support for the clause itself, confirmed against the vendors' own + * reference manuals (quoted in PR for #5567): Postgres defaults to backslash and + * accepts `ESCAPE`; MySQL assumes `\` unless `NO_BACKSLASH_ESCAPES` is set and + * accepts `ESCAPE` with an argument that "must evaluate as a constant at + * execution time" (a bound placeholder is); SQLite honours NO default escape + * character at all, which is the reason the explicit clause is required rather + * than merely tidy. + * + * ## Relationship to `driver-sql`'s `applyLike` + * + * This is deliberately the same transform `SqlDriver.applyLike` + * (`packages/plugins/driver-sql/src/sql-driver.ts`) applies — same escaped + * character class, same three wildcard shapes, same bound `ESCAPE` — and its + * TSDoc points back here. It is a SECOND implementation on purpose, not an + * oversight: + * + * - `service-analytics` depends on no driver (see its `package.json`: only + * `@objectstack/core` and `@objectstack/spec`), and `applyLike` is a private + * method on a knex builder — it takes a builder and a field, not a string, + * so there is nothing importable even if the dependency existed. + * - Promoting it to a shared package would add a new public surface to + * `@objectstack/core` for three call sites inside one package. Not worth a + * new export until a fourth consumer outside this package needs it. + * + * What keeps the two from drifting is not these comments: it is + * `__tests__/like-metacharacter-escape.test.ts`, which asserts + * {@link escapeLikePattern} against `applyLike`'s expression character for + * character. A third hand-copy of this logic anywhere is the thing to refuse — + * import from here, or add a consumer to that test. + */ + +/** + * Where the wildcard sits relative to the comparand. Named exactly as + * `driver-sql`'s `applyLike` names its `shape` parameter, so the two read alike: + * `contains` → `%v%`, `starts` → `v%`, `ends` → `%v`. + */ +export type LikeShape = 'contains' | 'starts' | 'ends'; + +/** + * The escape character every emitter in this package binds into its `ESCAPE` + * clause. A single backslash — the value `driver-sql` binds, and the default + * Postgres and MySQL already assume. + */ +export const LIKE_ESCAPE_CHAR = '\\'; + +/** + * Escape the LIKE metacharacters (`%`, `_`) and the escape character itself + * (`\`) so a comparand matches literally. + * + * Character for character the expression `driver-sql`'s `applyLike` uses; the + * shared test holds them to each other. + */ +export function escapeLikePattern(value: unknown): string { + return String(value).replace(/[\\%_]/g, '\\$&'); +} + +/** + * Build the LIKE pattern for one comparand: escaped, then wrapped in the + * wildcards `shape` calls for. + * + * The result MUST be bound together with {@link LIKE_ESCAPE_CHAR} as the + * predicate's `ESCAPE` argument — see the escaping-alone note in this file's + * header for what happens on SQLite otherwise. + */ +export function likePattern(shape: LikeShape, value: unknown): string { + const escaped = escapeLikePattern(value); + return shape === 'starts' ? `${escaped}%` : shape === 'ends' ? `%${escaped}` : `%${escaped}%`; +} diff --git a/packages/services/service-analytics/src/read-scope-sql.ts b/packages/services/service-analytics/src/read-scope-sql.ts index cb2b38066f..620d27f69d 100644 --- a/packages/services/service-analytics/src/read-scope-sql.ts +++ b/packages/services/service-analytics/src/read-scope-sql.ts @@ -1,6 +1,7 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. import type { FilterCondition } from '@objectstack/spec/data'; +import { likePattern, LIKE_ESCAPE_CHAR } from './like-pattern.js'; /** * Compile an RLS / tenant read-scope `FilterCondition` into a parameterized, @@ -66,6 +67,22 @@ import type { FilterCondition } from '@objectstack/spec/data'; * canonical; {@link nullSafeNegationOperand} here is the same rewrite * `sql-driver.ts` applies, so an analytics query and an ordinary `find()` scope * the same rows. + * + * ## The LIKE family compares LITERALS (#5567) + * + * `_` is LIKE's single-character wildcard and `%` its multi-character one, so a + * comparand concatenated straight into a pattern position stops meaning what the + * author wrote: `{owner_name: {$contains: '_admin'}}` also admitted `xyadmin`, + * and `{$contains: '50%'}` also admitted `off 5012 now`. Every LIKE arm below + * therefore binds an ESCAPED pattern plus its escape character — see + * `like-pattern.ts` for the transform, for why the escape character is a bound + * value rather than a SQL literal, and for its correspondence with `driver-sql`'s + * `applyLike`. + * + * On THIS compiler that widening was the #5347 / #5324 shape again: a read scope + * admitting rows the policy did not is over-reach, not a degraded filter. Note + * the file was fail-closed everywhere else — the LIKE family was the one place an + * author's literal was silently reinterpreted rather than refused. */ const IDENT = /^[a-z_][a-z0-9_]*$/i; @@ -212,6 +229,27 @@ function bind(params: unknown[], v: unknown): string { return '?'; } +/** + * [#5567] Bind a LIKE pattern together with its escape character: `? ESCAPE ?`. + * + * Both are ordinary bound values, so this whole concern stays inside the + * predicate: `applyReadScope` (`native-sql-strategy.ts`) and `generateSql` + * (`objectql-strategy.ts`) rewrite `?` → `$N` while pushing the matching value + * from `params`, and they carry the escape character for free — neither consumer + * needed a change. A SQL literal `ESCAPE '\'` would have pushed the problem up a + * layer AND been unportable: MySQL strips one backslash inside a string literal, + * so the literal spelling differs per dialect while a bound value does not. + * + * The clause is not optional decoration. SQLite honours no default escape + * character, so the escaped pattern alone would search for a literal backslash + * there and match nothing — the two halves are one fix (see `like-pattern.ts`). + */ +function bindLike(params: unknown[], pattern: string): string { + // Left-to-right evaluation of the template puts the pattern in `params` before + // the escape character, which is the order the `?` appear. + return `${bind(params, pattern)} ESCAPE ${bind(params, LIKE_ESCAPE_CHAR)}`; +} + 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)}`; @@ -234,10 +272,12 @@ function compileOperator(col: string, op: string, val: unknown, field: string, p if (!Array.isArray(val) || val.length !== 2) throw new Error(`[read-scope-sql] $between for "${field}" needs [min,max] (fail-closed).`); return `${col} BETWEEN ${bind(params, val[0])} AND ${bind(params, val[1])}`; } - case '$contains': return `${col} LIKE ${bind(params, `%${String(val)}%`)}`; - case '$notContains': return `${col} NOT LIKE ${bind(params, `%${String(val)}%`)}`; - case '$startsWith': return `${col} LIKE ${bind(params, `${String(val)}%`)}`; - case '$endsWith': return `${col} LIKE ${bind(params, `%${String(val)}`)}`; + // [#5567] The comparand is a LITERAL, so it is escaped and the escape + // character is bound with it. See {@link bindLike}. + case '$contains': return `${col} LIKE ${bindLike(params, likePattern('contains', val))}`; + case '$notContains': return `${col} NOT LIKE ${bindLike(params, likePattern('contains', val))}`; + case '$startsWith': return `${col} LIKE ${bindLike(params, likePattern('starts', val))}`; + case '$endsWith': return `${col} LIKE ${bindLike(params, likePattern('ends', val))}`; case '$null': return val ? `${col} IS NULL` : `${col} IS NOT NULL`; case '$exists': return val ? `${col} IS NOT NULL` : `${col} IS NULL`; default: diff --git a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts index 9d274981cf..2817ec964a 100644 --- a/packages/services/service-analytics/src/strategies/native-sql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/native-sql-strategy.ts @@ -11,6 +11,7 @@ import { type NormalizedFilterNode, } from './filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; +import { likePattern, LIKE_ESCAPE_CHAR, type LikeShape } from '../like-pattern.js'; import { nextUtcCalendarDay } from '@objectstack/core'; /** @@ -662,12 +663,17 @@ export class NativeSQLStrategy implements AnalyticsStrategy { contains: 'LIKE', notContains: 'NOT LIKE', startsWith: 'LIKE', endsWith: 'LIKE', }; - /** The LIKE pattern each string operator wraps its comparand in. */ - const likePattern: Record string> = { - contains: (v) => `%${v}%`, - notContains: (v) => `%${v}%`, - startsWith: (v) => `${v}%`, - endsWith: (v) => `%${v}`, + /** + * Where each string operator puts the wildcard. [#5567] The pattern itself is + * built by the shared `likePattern`, which ESCAPES the comparand — `_` and + * `%` are LIKE wildcards, so the old inline table quietly turned an author's + * literal into a pattern (`$contains: '_admin'` also matched `xyadmin`). + * `objectql-strategy.ts`'s `LIKE_SQL_OPS` carries the same table for the + * `/analytics/sql` echo of this statement; they move together. + */ + const likeShape: Record = { + contains: 'contains', notContains: 'contains', + startsWith: 'starts', endsWith: 'ends', }; // Null predicates and the LIKE family read the column as stored — the former @@ -690,10 +696,15 @@ export class NativeSQLStrategy implements AnalyticsStrategy { // The LIKE family reads the column as stored — a substring/prefix/suffix // match is on the raw text — so it keeps the un-normalised reference. - const pattern = likePattern[operator]; - if (pattern) { - params.push(pattern(values[0])); - return `${rawCol} ${sqlOp} $${params.length}`; + const shape = likeShape[operator]; + if (shape) { + // [#5567] Escaped pattern AND an explicit `ESCAPE`, bound together: the + // escaping alone would search for a literal backslash on SQLite (no + // default escape character there), the clause alone would change nothing. + params.push(likePattern(shape, values[0])); + const patternRef = `$${params.length}`; + params.push(LIKE_ESCAPE_CHAR); + return `${rawCol} ${sqlOp} ${patternRef} ESCAPE $${params.length}`; } // A bare-day `lte` bound means "through that whole day" (#3777): compile diff --git a/packages/services/service-analytics/src/strategies/objectql-strategy.ts b/packages/services/service-analytics/src/strategies/objectql-strategy.ts index 3232ccaa88..f412dd9799 100644 --- a/packages/services/service-analytics/src/strategies/objectql-strategy.ts +++ b/packages/services/service-analytics/src/strategies/objectql-strategy.ts @@ -12,6 +12,7 @@ import { type NormalizedFilterNode, } from './filter-normalizer.js'; import { compileScopedFilterToSql } from '../read-scope-sql.js'; +import { likePattern, LIKE_ESCAPE_CHAR, type LikeShape } from '../like-pattern.js'; import { nextUtcCalendarDay } from '@objectstack/core'; import { rebucketCrossObject, @@ -27,20 +28,28 @@ const SCALAR_SQL_OPS: Record = { }; /** - * The LIKE family: SQL spelling + the pattern each wraps its comparand in. + * The LIKE family: SQL spelling + where each one puts the wildcard. * * Deliberately the same pair of tables `NativeSQLStrategy.buildFilterClause` - * carries (`opMap` / `likePattern`), because this file renders a description of + * carries (`opMap` / `likeShape`), because this file renders a description of * the statement THAT compiler produces. Keeping them as one table here is the * point of #5333: `startsWith` / `endsWith` were in neither the branch above nor * `SCALAR_SQL_OPS`, so they fell to the unmapped exit and the predicate vanished * from the echo while the query it documents ran `LIKE 'w%'`. + * + * [#5567] The pattern comes from the shared `likePattern`, which ESCAPES the + * comparand, and the renderer binds an explicit `ESCAPE` alongside it. That is + * not cosmetic for an echo: the execution this file describes goes through the + * engine to `driver-sql`, whose `applyLike` has always escaped and bound + * `ESCAPE`. Rendering the raw comparand meant the echoed statement was WIDER + * than the query it claims to reproduce whenever the comparand carried a `_` or + * `%` — the #3601 / #3602 / #3650 failure this render block exists to prevent. */ -const LIKE_SQL_OPS: Record string }> = { - contains: { sql: 'LIKE', pattern: (v) => `%${v}%` }, - notContains: { sql: 'NOT LIKE', pattern: (v) => `%${v}%` }, - startsWith: { sql: 'LIKE', pattern: (v) => `${v}%` }, - endsWith: { sql: 'LIKE', pattern: (v) => `%${v}` }, +const LIKE_SQL_OPS: Record = { + contains: { sql: 'LIKE', shape: 'contains' }, + notContains: { sql: 'NOT LIKE', shape: 'contains' }, + startsWith: { sql: 'LIKE', shape: 'starts' }, + endsWith: { sql: 'LIKE', shape: 'ends' }, }; /** One cross-object grouping dimension planned for FK-expand (#3654). */ @@ -655,8 +664,13 @@ export class ObjectQLStrategy implements AnalyticsStrategy { // substring match reads the column as stored. const like = LIKE_SQL_OPS[operator]; if (like) { - params.push(like.pattern(values[0])); - return `${col} ${like.sql} $${params.length}`; + // [#5567] Escaped pattern + an explicit `ESCAPE`, matching what + // `driver-sql`'s `applyLike` binds for the same operator — so an author + // who copies this statement out runs the predicate that ran. + params.push(likePattern(like.shape, values[0])); + const patternRef = `$${params.length}`; + params.push(LIKE_ESCAPE_CHAR); + return `${col} ${like.sql} ${patternRef} ESCAPE $${params.length}`; } const op = SCALAR_SQL_OPS[operator];