|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | + |
| 3 | +/** |
| 4 | + * [#14113] The aggregation ALIAS is escaped, not gated — TursoDriver's REMOTE |
| 5 | + * transport. |
| 6 | + * |
| 7 | + * ## The defect |
| 8 | + * |
| 9 | + * `RemoteTransport.aggregate` held the aggregation `alias` to |
| 10 | + * `SAFE_IDENTIFIER` (`/^[a-zA-Z_][a-zA-Z0-9_]*$/`). A dot fails that regex. |
| 11 | + * Every analytics measure is named `<cube>.<measure>` on the wire and |
| 12 | + * `ObjectQLStrategy` uses that name verbatim as the aggregation `alias` |
| 13 | + * (`objectql-strategy.ts`, `{ field, method, alias: measure }`), so EVERY cube |
| 14 | + * query that reached this face threw: |
| 15 | + * |
| 16 | + * ``` |
| 17 | + * RemoteTransport: unsafe identifier rejected: "showcase_delivery.count" |
| 18 | + * ``` |
| 19 | + * |
| 20 | + * a bare `Error` with no `code` and no `status`, which `mapDataError` then |
| 21 | + * serves as an opaque 500 — the #11455 / #8931 shape. |
| 22 | + * |
| 23 | + * ## Why the fix is escaping and NOT dropping the check |
| 24 | + * |
| 25 | + * The alias is interpolated RAW into `AS "${alias}"`, so an alias containing a |
| 26 | + * `"` would close the quoting and continue as grammar. The repair is the |
| 27 | + * standard doubled-quote escape inside a quoted SQL identifier (`"` → `""`) — |
| 28 | + * the ALIAS half of the distinction #13714 drew one face over, where |
| 29 | + * `SqlDriver.aliasIdentifierSql` routes the same position through knex's |
| 30 | + * `wrapIdentifier`. A qualified REFERENCE must be validated; a single output |
| 31 | + * NAME must be quoted and escaped. `AggregationNodeSchema` declares |
| 32 | + * `alias: z.string()` — an output-column key — and the in-memory, MongoDB and |
| 33 | + * (post-#13714) SQL faces all project it verbatim. This face was the outlier. |
| 34 | + * |
| 35 | + * ## Why a SQLite-backed client stub rather than a mocked `execute` |
| 36 | + * |
| 37 | + * Only EXECUTING the statement tells "escaped" apart from "broke out". A |
| 38 | + * string assertion alone would pass on an alias that terminates the quoting, |
| 39 | + * because the text still *looks* like a select list. libsql IS SQLite, so |
| 40 | + * `makeLibsqlSqliteStub` runs what this transport emits: an alias that escaped |
| 41 | + * its quoting is a syntax error (or a second statement better-sqlite3 refuses |
| 42 | + * to prepare), and a green read of the value back under the literal alias is |
| 43 | + * the proof. The emitted SQL is pinned too, so a future reader can see the |
| 44 | + * doubled quote rather than infer it. |
| 45 | + * |
| 46 | + * ## Reverse verification — direction predicted BEFORE it was run |
| 47 | + * |
| 48 | + * Restore `this.assertSafeIdentifier(alias)` above the `selectParts.push` and |
| 49 | + * emit `AS "${alias}"` again (the pre-#14113 two lines): |
| 50 | + * |
| 51 | + * - the dotted-alias cases (repro, emitted SQL, the un-bucketed cube shape) |
| 52 | + * go RED by THROWING inside the call — `unsafe identifier rejected: |
| 53 | + * "showcase_delivery.count"` — not on a comparison. |
| 54 | + * - the quote-escape cases go RED by throwing the same sentence, naming |
| 55 | + * `won"count` / the `DROP TABLE` text. They cannot go red on a broken-out |
| 56 | + * statement, because the restored guard refuses that input before any SQL is |
| 57 | + * built — which is exactly why the guard could not simply be deleted. |
| 58 | + * - the `field`-position control and the groupBy-alias control stay GREEN: |
| 59 | + * neither position is touched by this card, and that is what they are here |
| 60 | + * to hold. |
| 61 | + * - the default-alias case stays GREEN: `count_all` passes `SAFE_IDENTIFIER` |
| 62 | + * either way, so it pins the byte-identical emission across the change. |
| 63 | + * |
| 64 | + * Measured after writing the above — see the PR body for the run. |
| 65 | + */ |
| 66 | + |
| 67 | +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; |
| 68 | +import { TursoDriver } from './turso-driver.js'; |
| 69 | +import { makeLibsqlSqliteStub, asLibsqlClient, type LibsqlSqliteStub } from './libsql-sqlite-stub.testkit.js'; |
| 70 | + |
| 71 | +/** |
| 72 | + * Named for the cube in the field report the card was filed from, so the alias |
| 73 | + * under test (`showcase_delivery.count`) is the real wire spelling rather than |
| 74 | + * a stand-in. |
| 75 | + */ |
| 76 | +const DELIVERY_OBJECT = { |
| 77 | + name: 'showcase_delivery', |
| 78 | + fields: { |
| 79 | + id: { type: 'string' }, |
| 80 | + region: { type: 'string' }, |
| 81 | + amount: { type: 'number' }, |
| 82 | + }, |
| 83 | +}; |
| 84 | + |
| 85 | +const ROWS = [ |
| 86 | + { id: '1', region: 'west', amount: 10 }, |
| 87 | + { id: '2', region: 'west', amount: 20 }, |
| 88 | + { id: '3', region: 'east', amount: 30 }, |
| 89 | +]; |
| 90 | + |
| 91 | +describe('[#14113] RemoteTransport — the aggregation alias is escaped, not gated', () => { |
| 92 | + let driver: TursoDriver; |
| 93 | + let stub: LibsqlSqliteStub; |
| 94 | + |
| 95 | + beforeAll(async () => { |
| 96 | + stub = makeLibsqlSqliteStub(); |
| 97 | + driver = new TursoDriver({ url: 'libsql://alias-quoting.turso.io', client: asLibsqlClient(stub) }); |
| 98 | + await driver.connect(); |
| 99 | + // The mode this suite is about — the one with its own hand-written SQL. |
| 100 | + expect(driver.transportMode).toBe('remote'); |
| 101 | + await driver.syncSchema(DELIVERY_OBJECT.name, DELIVERY_OBJECT); |
| 102 | + for (const row of ROWS) await driver.create(DELIVERY_OBJECT.name, { ...row }); |
| 103 | + }); |
| 104 | + |
| 105 | + afterAll(async () => { |
| 106 | + await driver.disconnect(); |
| 107 | + stub.close(); |
| 108 | + }); |
| 109 | + |
| 110 | + /** |
| 111 | + * A transport backed by the same database, capturing the statements it sends |
| 112 | + * AND executing them — the capture alone would not prove the statement runs. |
| 113 | + */ |
| 114 | + const capturing = async () => { |
| 115 | + const seen: string[] = []; |
| 116 | + const spy = { |
| 117 | + ...stub, |
| 118 | + execute: async (stmt: unknown) => { |
| 119 | + seen.push((stmt as { sql: string }).sql); |
| 120 | + return stub.execute(stmt); |
| 121 | + }, |
| 122 | + }; |
| 123 | + const t = new TursoDriver({ url: 'libsql://alias-quoting.turso.io', client: asLibsqlClient(spy) }); |
| 124 | + await t.connect(); |
| 125 | + return { t, seen }; |
| 126 | + }; |
| 127 | + |
| 128 | + describe('direction 1 — a dotted `CUBE.MEASURE` alias now reaches the database', () => { |
| 129 | + it('the exact alias the card measured is served, on rows', async () => { |
| 130 | + const rows = await driver.aggregate(DELIVERY_OBJECT.name, { |
| 131 | + object: DELIVERY_OBJECT.name, |
| 132 | + aggregations: [{ function: 'count', alias: 'showcase_delivery.count' }], |
| 133 | + } as never); |
| 134 | + // The value comes back under the caller's own key — a dot is inert |
| 135 | + // inside a quoted identifier, which is the whole claim of this card. |
| 136 | + expect(rows).toHaveLength(1); |
| 137 | + expect((rows as Array<Record<string, unknown>>)[0]['showcase_delivery.count']).toBe(3); |
| 138 | + }); |
| 139 | + |
| 140 | + it('compiles to ONE quoted identifier, dot and all', async () => { |
| 141 | + const { t, seen } = await capturing(); |
| 142 | + await t.aggregate(DELIVERY_OBJECT.name, { |
| 143 | + object: DELIVERY_OBJECT.name, |
| 144 | + aggregations: [{ function: 'count', alias: 'showcase_delivery.count' }], |
| 145 | + } as never); |
| 146 | + expect(seen).toEqual([ |
| 147 | + 'SELECT count(*) AS "showcase_delivery.count" FROM "showcase_delivery"', |
| 148 | + ]); |
| 149 | + // ⛔ Not two segments. The failure this replaces is a face that treats an |
| 150 | + // alias as a qualified reference; the dot must stay INSIDE the quotes. |
| 151 | + expect(seen[0]).not.toContain('"showcase_delivery"."count"'); |
| 152 | + }); |
| 153 | + |
| 154 | + it('the un-bucketed cube shape — a grouped measure — is served end to end', async () => { |
| 155 | + // The path that actually reaches `driver.aggregate`: remote mode |
| 156 | + // publishes `queryDateGranularity: {}` (see `TursoDriver.supports`), so a |
| 157 | + // BUCKETED query falls back to `find()` + in-memory bucketing and never |
| 158 | + // arrives here. The UN-bucketed cube query is the one that ate the |
| 159 | + // refusal, and it carries a dimension in `groupBy` beside the measure. |
| 160 | + // |
| 161 | + // ⭐ The groupBy FIELD is a bare column name here, not a dotted one, and |
| 162 | + // that is measured rather than assumed: `ObjectQLStrategy.resolveFieldName` |
| 163 | + // resolves a dimension to `member.sql` or `member.split('.')[1]`, so only |
| 164 | + // the MEASURE arrives dotted. That asymmetry is why this card is confined |
| 165 | + // to the alias position of `aggregations`. |
| 166 | + const rows = await driver.aggregate(DELIVERY_OBJECT.name, { |
| 167 | + object: DELIVERY_OBJECT.name, |
| 168 | + groupBy: ['region'], |
| 169 | + aggregations: [ |
| 170 | + { function: 'count', alias: 'showcase_delivery.count' }, |
| 171 | + { function: 'sum', field: 'amount', alias: 'showcase_delivery.total_amount' }, |
| 172 | + ], |
| 173 | + } as never); |
| 174 | + const byRegion = Object.fromEntries( |
| 175 | + (rows as Array<Record<string, unknown>>).map((r) => [ |
| 176 | + r.region, |
| 177 | + [r['showcase_delivery.count'], r['showcase_delivery.total_amount']], |
| 178 | + ]), |
| 179 | + ); |
| 180 | + expect(byRegion).toEqual({ west: [2, 30], east: [1, 30] }); |
| 181 | + }); |
| 182 | + }); |
| 183 | + |
| 184 | + describe('direction 2 — an alias carrying a `"` is ESCAPED, not let through', () => { |
| 185 | + it('doubles the quote and still runs, returning the value under the literal alias', async () => { |
| 186 | + const alias = 'won"count'; |
| 187 | + const { t, seen } = await capturing(); |
| 188 | + const rows = await t.aggregate(DELIVERY_OBJECT.name, { |
| 189 | + object: DELIVERY_OBJECT.name, |
| 190 | + aggregations: [{ function: 'count', alias }], |
| 191 | + } as never); |
| 192 | + expect(seen).toEqual(['SELECT count(*) AS "won""count" FROM "showcase_delivery"']); |
| 193 | + // Executed, not merely emitted: an alias that broke out of its quoting |
| 194 | + // would be a syntax error here rather than a row. |
| 195 | + expect((rows as Array<Record<string, unknown>>)[0][alias]).toBe(3); |
| 196 | + }); |
| 197 | + |
| 198 | + it('an alias that tries to close the quoting and append a statement stays one name', async () => { |
| 199 | + const alias = 'bucket"; DROP TABLE showcase_delivery; --'; |
| 200 | + const { t, seen } = await capturing(); |
| 201 | + const rows = await t.aggregate(DELIVERY_OBJECT.name, { |
| 202 | + object: DELIVERY_OBJECT.name, |
| 203 | + aggregations: [{ function: 'count', alias }], |
| 204 | + } as never); |
| 205 | + expect(seen).toEqual([ |
| 206 | + 'SELECT count(*) AS "bucket""; DROP TABLE showcase_delivery; --" FROM "showcase_delivery"', |
| 207 | + ]); |
| 208 | + // The whole payload came back as a COLUMN NAME — it was data, never |
| 209 | + // grammar. |
| 210 | + expect((rows as Array<Record<string, unknown>>)[0][alias]).toBe(3); |
| 211 | + // And the table it named is still there, with every row. |
| 212 | + expect( |
| 213 | + stub.raw.prepare('select count(*) as c from showcase_delivery').all(), |
| 214 | + ).toEqual([{ c: 3 }]); |
| 215 | + }); |
| 216 | + }); |
| 217 | + |
| 218 | + describe('regression controls — the positions this card did NOT touch', () => { |
| 219 | + it('the `field` position still refuses an unsafe identifier, and sends nothing', async () => { |
| 220 | + // `SAFE_IDENTIFIER` is doing real work here: `field` becomes a column |
| 221 | + // REFERENCE, which is grammar. Escaping is the answer for a NAME only. |
| 222 | + const { t, seen } = await capturing(); |
| 223 | + const err = await t |
| 224 | + .aggregate(DELIVERY_OBJECT.name, { |
| 225 | + object: DELIVERY_OBJECT.name, |
| 226 | + aggregations: [{ function: 'sum', field: 'amount"; DROP TABLE showcase_delivery; --', alias: 'n' }], |
| 227 | + } as never) |
| 228 | + .then( |
| 229 | + () => { throw new Error('expected the transport to refuse an unsafe field'); }, |
| 230 | + (e) => e as Error, |
| 231 | + ); |
| 232 | + // The OFFENDING TEXT, not just the sentence (#6144) — an alias that is |
| 233 | + // itself safe is what makes this case reach the `field` check at all. |
| 234 | + expect(err.message).toContain('unsafe identifier rejected'); |
| 235 | + expect(err.message).toContain('amount"; DROP TABLE showcase_delivery; --'); |
| 236 | + expect(seen).toEqual([]); |
| 237 | + }); |
| 238 | + |
| 239 | + it('the `object` position still refuses an unsafe identifier', async () => { |
| 240 | + const { t, seen } = await capturing(); |
| 241 | + const err = await t |
| 242 | + .aggregate('showcase_delivery"; DROP TABLE showcase_delivery; --', { |
| 243 | + object: 'showcase_delivery"; DROP TABLE showcase_delivery; --', |
| 244 | + aggregations: [{ function: 'count', alias: 'n' }], |
| 245 | + } as never) |
| 246 | + .then( |
| 247 | + () => { throw new Error('expected the transport to refuse an unsafe object') }, |
| 248 | + (e) => e as Error, |
| 249 | + ); |
| 250 | + expect(err.message).toContain('unsafe identifier rejected'); |
| 251 | + expect(seen).toEqual([]); |
| 252 | + }); |
| 253 | + |
| 254 | + it('the groupBy alias position is UNCHANGED — still refused, and that is a separate card', async () => { |
| 255 | + // ⚠️ Deliberate scope line, pinned so it cannot drift silently. The |
| 256 | + // groupBy `alias` is the same class of thing (an output NAME) and |
| 257 | + // `driver-sql` escapes it post-#13714 (`aliasIdentifierSql` at its |
| 258 | + // groupBy select site), so this face diverges there too — but that |
| 259 | + // position carries a LANDED pin (#6401, `remote-transport-groupby-node`) |
| 260 | + // asserting the refusal, so reversing it is a judgement this card was not |
| 261 | + // dispatched to make. Filed separately rather than patched inline; this |
| 262 | + // control records the state it was left in. |
| 263 | + // |
| 264 | + // It is also NOT on the reproducing path: `ObjectQLStrategy` resolves a |
| 265 | + // dimension to a bare column name, so no analytics query sends a dotted |
| 266 | + // groupBy alias. |
| 267 | + const { t, seen } = await capturing(); |
| 268 | + const err = await t |
| 269 | + .aggregate(DELIVERY_OBJECT.name, { |
| 270 | + object: DELIVERY_OBJECT.name, |
| 271 | + groupBy: [{ field: 'region', alias: 'showcase_delivery.region' }], |
| 272 | + aggregations: [{ function: 'count', alias: 'showcase_delivery.count' }], |
| 273 | + } as never) |
| 274 | + .then( |
| 275 | + () => { throw new Error('expected the groupBy alias to still be refused') }, |
| 276 | + (e) => e as Error, |
| 277 | + ); |
| 278 | + expect(err.message).toContain('unsafe identifier rejected'); |
| 279 | + expect(err.message).toContain('showcase_delivery.region'); |
| 280 | + expect(seen).toEqual([]); |
| 281 | + }); |
| 282 | + |
| 283 | + it('the default alias is byte-identical to what it was before', async () => { |
| 284 | + // A caller who omits `alias` reads the result under `count_all` exactly |
| 285 | + // as they did pre-#14113 — this change moves no default. |
| 286 | + const { t, seen } = await capturing(); |
| 287 | + const rows = await t.aggregate(DELIVERY_OBJECT.name, { |
| 288 | + object: DELIVERY_OBJECT.name, |
| 289 | + aggregations: [{ function: 'count' }], |
| 290 | + } as never); |
| 291 | + expect(seen).toEqual(['SELECT count(*) AS "count_all" FROM "showcase_delivery"']); |
| 292 | + expect((rows as Array<Record<string, unknown>>)[0].count_all).toBe(3); |
| 293 | + }); |
| 294 | + }); |
| 295 | +}); |
0 commit comments