From 0a444940960eab91147b570c0a6af4b8f763448c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 15:13:38 +0000 Subject: [PATCH 1/2] fix(driver-turso): escape the aggregation alias instead of gating it (#14113) --- ...ransport-aggregation-alias-quoting.test.ts | 295 ++++++++++++++++++ .../driver-turso/src/remote-transport.ts | 52 ++- 2 files changed, 345 insertions(+), 2 deletions(-) create mode 100644 packages/drivers/driver-turso/src/remote-transport-aggregation-alias-quoting.test.ts diff --git a/packages/drivers/driver-turso/src/remote-transport-aggregation-alias-quoting.test.ts b/packages/drivers/driver-turso/src/remote-transport-aggregation-alias-quoting.test.ts new file mode 100644 index 0000000000..9db8cce7d6 --- /dev/null +++ b/packages/drivers/driver-turso/src/remote-transport-aggregation-alias-quoting.test.ts @@ -0,0 +1,295 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14113] The aggregation ALIAS is escaped, not gated — TursoDriver's REMOTE + * transport. + * + * ## The defect + * + * `RemoteTransport.aggregate` held the aggregation `alias` to + * `SAFE_IDENTIFIER` (`/^[a-zA-Z_][a-zA-Z0-9_]*$/`). A dot fails that regex. + * Every analytics measure is named `.` on the wire and + * `ObjectQLStrategy` uses that name verbatim as the aggregation `alias` + * (`objectql-strategy.ts`, `{ field, method, alias: measure }`), so EVERY cube + * query that reached this face threw: + * + * ``` + * RemoteTransport: unsafe identifier rejected: "showcase_delivery.count" + * ``` + * + * a bare `Error` with no `code` and no `status`, which `mapDataError` then + * serves as an opaque 500 — the #11455 / #8931 shape. + * + * ## Why the fix is escaping and NOT dropping the check + * + * The alias is interpolated RAW into `AS "${alias}"`, so an alias containing a + * `"` would close the quoting and continue as grammar. The repair is the + * standard doubled-quote escape inside a quoted SQL identifier (`"` → `""`) — + * the ALIAS half of the distinction #13714 drew one face over, where + * `SqlDriver.aliasIdentifierSql` routes the same position through knex's + * `wrapIdentifier`. A qualified REFERENCE must be validated; a single output + * NAME must be quoted and escaped. `AggregationNodeSchema` declares + * `alias: z.string()` — an output-column key — and the in-memory, MongoDB and + * (post-#13714) SQL faces all project it verbatim. This face was the outlier. + * + * ## Why a SQLite-backed client stub rather than a mocked `execute` + * + * Only EXECUTING the statement tells "escaped" apart from "broke out". A + * string assertion alone would pass on an alias that terminates the quoting, + * because the text still *looks* like a select list. libsql IS SQLite, so + * `makeLibsqlSqliteStub` runs what this transport emits: an alias that escaped + * its quoting is a syntax error (or a second statement better-sqlite3 refuses + * to prepare), and a green read of the value back under the literal alias is + * the proof. The emitted SQL is pinned too, so a future reader can see the + * doubled quote rather than infer it. + * + * ## Reverse verification — direction predicted BEFORE it was run + * + * Restore `this.assertSafeIdentifier(alias)` above the `selectParts.push` and + * emit `AS "${alias}"` again (the pre-#14113 two lines): + * + * - the dotted-alias cases (repro, emitted SQL, the un-bucketed cube shape) + * go RED by THROWING inside the call — `unsafe identifier rejected: + * "showcase_delivery.count"` — not on a comparison. + * - the quote-escape cases go RED by throwing the same sentence, naming + * `won"count` / the `DROP TABLE` text. They cannot go red on a broken-out + * statement, because the restored guard refuses that input before any SQL is + * built — which is exactly why the guard could not simply be deleted. + * - the `field`-position control and the groupBy-alias control stay GREEN: + * neither position is touched by this card, and that is what they are here + * to hold. + * - the default-alias case stays GREEN: `count_all` passes `SAFE_IDENTIFIER` + * either way, so it pins the byte-identical emission across the change. + * + * Measured after writing the above — see the PR body for the run. + */ + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { TursoDriver } from './turso-driver.js'; +import { makeLibsqlSqliteStub, asLibsqlClient, type LibsqlSqliteStub } from './libsql-sqlite-stub.testkit.js'; + +/** + * Named for the cube in the field report the card was filed from, so the alias + * under test (`showcase_delivery.count`) is the real wire spelling rather than + * a stand-in. + */ +const DELIVERY_OBJECT = { + name: 'showcase_delivery', + fields: { + id: { type: 'string' }, + region: { type: 'string' }, + amount: { type: 'number' }, + }, +}; + +const ROWS = [ + { id: '1', region: 'west', amount: 10 }, + { id: '2', region: 'west', amount: 20 }, + { id: '3', region: 'east', amount: 30 }, +]; + +describe('[#14113] RemoteTransport — the aggregation alias is escaped, not gated', () => { + let driver: TursoDriver; + let stub: LibsqlSqliteStub; + + beforeAll(async () => { + stub = makeLibsqlSqliteStub(); + driver = new TursoDriver({ url: 'libsql://alias-quoting.turso.io', client: asLibsqlClient(stub) }); + await driver.connect(); + // The mode this suite is about — the one with its own hand-written SQL. + expect(driver.transportMode).toBe('remote'); + await driver.syncSchema(DELIVERY_OBJECT.name, DELIVERY_OBJECT); + for (const row of ROWS) await driver.create(DELIVERY_OBJECT.name, { ...row }); + }); + + afterAll(async () => { + await driver.disconnect(); + stub.close(); + }); + + /** + * A transport backed by the same database, capturing the statements it sends + * AND executing them — the capture alone would not prove the statement runs. + */ + const capturing = async () => { + const seen: string[] = []; + const spy = { + ...stub, + execute: async (stmt: unknown) => { + seen.push((stmt as { sql: string }).sql); + return stub.execute(stmt); + }, + }; + const t = new TursoDriver({ url: 'libsql://alias-quoting.turso.io', client: asLibsqlClient(spy) }); + await t.connect(); + return { t, seen }; + }; + + describe('direction 1 — a dotted `CUBE.MEASURE` alias now reaches the database', () => { + it('the exact alias the card measured is served, on rows', async () => { + const rows = await driver.aggregate(DELIVERY_OBJECT.name, { + object: DELIVERY_OBJECT.name, + aggregations: [{ function: 'count', alias: 'showcase_delivery.count' }], + } as never); + // The value comes back under the caller's own key — a dot is inert + // inside a quoted identifier, which is the whole claim of this card. + expect(rows).toHaveLength(1); + expect((rows as Array>)[0]['showcase_delivery.count']).toBe(3); + }); + + it('compiles to ONE quoted identifier, dot and all', async () => { + const { t, seen } = await capturing(); + await t.aggregate(DELIVERY_OBJECT.name, { + object: DELIVERY_OBJECT.name, + aggregations: [{ function: 'count', alias: 'showcase_delivery.count' }], + } as never); + expect(seen).toEqual([ + 'SELECT count(*) AS "showcase_delivery.count" FROM "showcase_delivery"', + ]); + // ⛔ Not two segments. The failure this replaces is a face that treats an + // alias as a qualified reference; the dot must stay INSIDE the quotes. + expect(seen[0]).not.toContain('"showcase_delivery"."count"'); + }); + + it('the un-bucketed cube shape — a grouped measure — is served end to end', async () => { + // The path that actually reaches `driver.aggregate`: remote mode + // publishes `queryDateGranularity: {}` (see `TursoDriver.supports`), so a + // BUCKETED query falls back to `find()` + in-memory bucketing and never + // arrives here. The UN-bucketed cube query is the one that ate the + // refusal, and it carries a dimension in `groupBy` beside the measure. + // + // ⭐ The groupBy FIELD is a bare column name here, not a dotted one, and + // that is measured rather than assumed: `ObjectQLStrategy.resolveFieldName` + // resolves a dimension to `member.sql` or `member.split('.')[1]`, so only + // the MEASURE arrives dotted. That asymmetry is why this card is confined + // to the alias position of `aggregations`. + const rows = await driver.aggregate(DELIVERY_OBJECT.name, { + object: DELIVERY_OBJECT.name, + groupBy: ['region'], + aggregations: [ + { function: 'count', alias: 'showcase_delivery.count' }, + { function: 'sum', field: 'amount', alias: 'showcase_delivery.total_amount' }, + ], + } as never); + const byRegion = Object.fromEntries( + (rows as Array>).map((r) => [ + r.region, + [r['showcase_delivery.count'], r['showcase_delivery.total_amount']], + ]), + ); + expect(byRegion).toEqual({ west: [2, 30], east: [1, 30] }); + }); + }); + + describe('direction 2 — an alias carrying a `"` is ESCAPED, not let through', () => { + it('doubles the quote and still runs, returning the value under the literal alias', async () => { + const alias = 'won"count'; + const { t, seen } = await capturing(); + const rows = await t.aggregate(DELIVERY_OBJECT.name, { + object: DELIVERY_OBJECT.name, + aggregations: [{ function: 'count', alias }], + } as never); + expect(seen).toEqual(['SELECT count(*) AS "won""count" FROM "showcase_delivery"']); + // Executed, not merely emitted: an alias that broke out of its quoting + // would be a syntax error here rather than a row. + expect((rows as Array>)[0][alias]).toBe(3); + }); + + it('an alias that tries to close the quoting and append a statement stays one name', async () => { + const alias = 'bucket"; DROP TABLE showcase_delivery; --'; + const { t, seen } = await capturing(); + const rows = await t.aggregate(DELIVERY_OBJECT.name, { + object: DELIVERY_OBJECT.name, + aggregations: [{ function: 'count', alias }], + } as never); + expect(seen).toEqual([ + 'SELECT count(*) AS "bucket""; DROP TABLE showcase_delivery; --" FROM "showcase_delivery"', + ]); + // The whole payload came back as a COLUMN NAME — it was data, never + // grammar. + expect((rows as Array>)[0][alias]).toBe(3); + // And the table it named is still there, with every row. + expect( + stub.raw.prepare('select count(*) as c from showcase_delivery').all(), + ).toEqual([{ c: 3 }]); + }); + }); + + describe('regression controls — the positions this card did NOT touch', () => { + it('the `field` position still refuses an unsafe identifier, and sends nothing', async () => { + // `SAFE_IDENTIFIER` is doing real work here: `field` becomes a column + // REFERENCE, which is grammar. Escaping is the answer for a NAME only. + const { t, seen } = await capturing(); + const err = await t + .aggregate(DELIVERY_OBJECT.name, { + object: DELIVERY_OBJECT.name, + aggregations: [{ function: 'sum', field: 'amount"; DROP TABLE showcase_delivery; --', alias: 'n' }], + } as never) + .then( + () => { throw new Error('expected the transport to refuse an unsafe field'); }, + (e) => e as Error, + ); + // The OFFENDING TEXT, not just the sentence (#6144) — an alias that is + // itself safe is what makes this case reach the `field` check at all. + expect(err.message).toContain('unsafe identifier rejected'); + expect(err.message).toContain('amount"; DROP TABLE showcase_delivery; --'); + expect(seen).toEqual([]); + }); + + it('the `object` position still refuses an unsafe identifier', async () => { + const { t, seen } = await capturing(); + const err = await t + .aggregate('showcase_delivery"; DROP TABLE showcase_delivery; --', { + object: 'showcase_delivery"; DROP TABLE showcase_delivery; --', + aggregations: [{ function: 'count', alias: 'n' }], + } as never) + .then( + () => { throw new Error('expected the transport to refuse an unsafe object') }, + (e) => e as Error, + ); + expect(err.message).toContain('unsafe identifier rejected'); + expect(seen).toEqual([]); + }); + + it('the groupBy alias position is UNCHANGED — still refused, and that is a separate card', async () => { + // ⚠️ Deliberate scope line, pinned so it cannot drift silently. The + // groupBy `alias` is the same class of thing (an output NAME) and + // `driver-sql` escapes it post-#13714 (`aliasIdentifierSql` at its + // groupBy select site), so this face diverges there too — but that + // position carries a LANDED pin (#6401, `remote-transport-groupby-node`) + // asserting the refusal, so reversing it is a judgement this card was not + // dispatched to make. Filed separately rather than patched inline; this + // control records the state it was left in. + // + // It is also NOT on the reproducing path: `ObjectQLStrategy` resolves a + // dimension to a bare column name, so no analytics query sends a dotted + // groupBy alias. + const { t, seen } = await capturing(); + const err = await t + .aggregate(DELIVERY_OBJECT.name, { + object: DELIVERY_OBJECT.name, + groupBy: [{ field: 'region', alias: 'showcase_delivery.region' }], + aggregations: [{ function: 'count', alias: 'showcase_delivery.count' }], + } as never) + .then( + () => { throw new Error('expected the groupBy alias to still be refused') }, + (e) => e as Error, + ); + expect(err.message).toContain('unsafe identifier rejected'); + expect(err.message).toContain('showcase_delivery.region'); + expect(seen).toEqual([]); + }); + + it('the default alias is byte-identical to what it was before', async () => { + // A caller who omits `alias` reads the result under `count_all` exactly + // as they did pre-#14113 — this change moves no default. + const { t, seen } = await capturing(); + const rows = await t.aggregate(DELIVERY_OBJECT.name, { + object: DELIVERY_OBJECT.name, + aggregations: [{ function: 'count' }], + } as never); + expect(seen).toEqual(['SELECT count(*) AS "count_all" FROM "showcase_delivery"']); + expect((rows as Array>)[0].count_all).toBe(3); + }); + }); +}); diff --git a/packages/drivers/driver-turso/src/remote-transport.ts b/packages/drivers/driver-turso/src/remote-transport.ts index 116fca9ce9..03cafbad40 100644 --- a/packages/drivers/driver-turso/src/remote-transport.ts +++ b/packages/drivers/driver-turso/src/remote-transport.ts @@ -1370,10 +1370,19 @@ export class RemoteTransport { // while emitting `count(distinct "stage")`. That the ALIAS follows the // declared name rather than the lowering is what keeps a caller's result // key predictable from their own query. + // + // [#14113] The alias is ESCAPED here, not gated — see + // {@link RemoteTransport.aliasIdentifierSql}. `assertSafeIdentifier` used + // to guard this position too, and it refused EVERY analytics cube query + // on a remote datasource: a measure is named `.` on the + // wire and `ObjectQLStrategy` uses that name verbatim as the aggregation + // `alias`, so the dot failed `SAFE_IDENTIFIER` and this face threw a bare + // `Error` with no `code`/`status` — an opaque 500 out of `mapDataError` + // for a query that is spelled correctly. The `field` position above keeps + // the guard, which is where it does real work. const alias = agg.alias || `${func}_${field === '*' ? 'all' : field}`; - this.assertSafeIdentifier(alias); const argSql = lowering.distinct ? `distinct ${fieldSql}` : fieldSql; - selectParts.push(`${lowering.sql}(${argSql}) AS "${alias}"`); + selectParts.push(`${lowering.sql}(${argSql}) AS ${this.aliasIdentifierSql(alias)}`); } if (selectParts.length === 0) selectParts.push('*'); @@ -1839,6 +1848,45 @@ export class RemoteTransport { } } + /** + * [#14113] Quote a single output NAME as a SQL identifier, doubling any + * embedded quote. The ALIAS half of the distinction #13714 drew one face + * over, where the same position routes through knex's `wrapIdentifier` + * ({@link SqlDriver.aliasIdentifierSql}) for exactly this reason. + * + * ## Why an alias is escaped where a reference is validated + * + * A column REFERENCE may legitimately be qualified, so it is checked by + * {@link RemoteTransport.assertSafeIdentifier} and must keep going through + * it. An ALIAS is one name by definition: `AggregationNodeSchema` declares + * it `z.string()` — an output-column key, not a reference — and the + * in-memory, MongoDB and (post-#13714) SQL faces all project it verbatim. + * Holding it to `SAFE_IDENTIFIER` made this face the outlier: it refused + * names the contract permits while its own emission could carry them safely. + * A dot is inert inside `AS "…"`, and `.` is the name EVERY + * analytics measure arrives under. + * + * ## ⛔ Escaping is not "dropping the check" + * + * The alias reaches the statement RAW inside `AS "…"`, so an alias + * containing a `"` would close the quoting and continue as grammar. + * Doubling it is the standard escape inside a quoted SQL identifier and is + * what keeps the alias a NAME rather than a way into the statement: + * + * ``` + * bucket"; DROP TABLE deal; -- → "bucket""; DROP TABLE deal; --" + * ``` + * + * one inert column name, which SQLite returns the value under verbatim. + * `String()` mirrors the sibling face, so a non-string a JS caller put in + * `alias` is escaped rather than concatenated. + * + * ⛔ NOT for column references — those keep {@link assertSafeIdentifier}. + */ + private aliasIdentifierSql(alias: string): string { + return `"${String(alias).replace(/"/g, '""')}"`; + } + /** * Build a CREATE TABLE SQL string for the given object definition. * Shared by syncSchema() and syncSchemasBatch() to avoid duplication. From 689d223f59823940012d20e0dc679b111d1ab4d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 15:42:10 +0000 Subject: [PATCH 2/2] chore(changeset): turso remote aggregation alias escaping (#14113) --- .../turso-remote-aggregation-alias-escaped.md | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 .changeset/turso-remote-aggregation-alias-escaped.md diff --git a/.changeset/turso-remote-aggregation-alias-escaped.md b/.changeset/turso-remote-aggregation-alias-escaped.md new file mode 100644 index 0000000000..1b5756b519 --- /dev/null +++ b/.changeset/turso-remote-aggregation-alias-escaped.md @@ -0,0 +1,36 @@ +--- +"@objectstack/driver-turso": patch +--- + +fix(driver-turso): escape the aggregation alias instead of gating it, so remote-mode analytics cube queries stop 500ing (#14113) + +`RemoteTransport.aggregate` (Turso **remote** mode) held the aggregation +`alias` to `SAFE_IDENTIFIER` (`/^[a-zA-Z_][a-zA-Z0-9_]*$/`). A dot fails that +regex. Every analytics measure is named `.` on the wire and +`ObjectQLStrategy` uses that name verbatim as the aggregation `alias`, so +**every** cube query that reached this face threw +`RemoteTransport: unsafe identifier rejected: "showcase_delivery.count"` — a +bare `Error` with no `code` and no `status`, which `mapDataError` then served +as an opaque 500 for a query that is spelled correctly. + +The alias is now **escaped rather than gated**: it may be any string, and the +quote character is doubled (`"` → `""`), the standard escape inside a quoted +SQL identifier. This is the ALIAS half of the distinction `driver-sql` drew at +**#13714**, where the same position routes through knex's `wrapIdentifier` +(`SqlDriver.aliasIdentifierSql`) — a qualified **reference** must be +validated, a single output **name** must be quoted and escaped. +`AggregationNodeSchema` declares `alias: z.string()`, an output-column key, +and the in-memory, MongoDB and (post-#13714) SQL faces all project it +verbatim; this face was the outlier. + +⛔ **Not** "drop the check". The alias reaches the statement raw inside +`AS "…"`, so an alias containing a `"` would close the quoting and continue as +grammar. `bucket"; DROP TABLE deal; --` now compiles to the single inert +column name `"bucket""; DROP TABLE deal; --"` and is returned as a column +name, executed against a real SQLite-backed client rather than asserted as a +string — the only instrument that tells "escaped" apart from "broke out". + +The `field` and `object` positions keep `assertSafeIdentifier` unchanged: those +become column and table **references**, which are grammar. Default aliases are +byte-identical (`count_all` still spells itself the same way), and the +`groupBy` alias position is untouched by this change.