|
63 | 63 | * reversed — the condition it named ("only one face would read it") is what |
64 | 64 | * stopped being true. |
65 | 65 | * |
66 | | - * # Reverse verification — direction predicted BEFORE it was run, per case |
| 66 | + * # `alias` is ESCAPED, not gated — as of #14235 |
| 67 | + * |
| 68 | + * `RemoteTransport.aggregate` emits a caller-supplied output NAME in exactly |
| 69 | + * two positions. #13714 routed BOTH of `driver-sql`'s through |
| 70 | + * `SqlDriver.aliasIdentifierSql`; #14113 moved this transport's AGGREGATION |
| 71 | + * alias to the same escaping and deliberately left the groupBy alias alone, |
| 72 | + * because the groupBy position carried the landed #6401 pin below asserting the |
| 73 | + * refusal. #14235 is the card that reverses that pin on the record: an |
| 74 | + * output-column key is a NAME — quoted and escaped — and a column REFERENCE is |
| 75 | + * grammar, so `field` keeps `assertSafeIdentifier` and `outKey` no longer has |
| 76 | + * it. `GroupByNodeSchema.alias` is the same class of key as |
| 77 | + * `AggregationNodeSchema.alias`, and the in-memory face projects |
| 78 | + * `g.alias ?? g.field` verbatim, so this face was the last one refusing names |
| 79 | + * the contract permits — with a bare `Error` before #14287, an opaque 500 after |
| 80 | + * `mapDataError`, and a 400 `INVALID_REQUEST` since. |
| 81 | + * |
| 82 | + * ⚠️ The `[#6401] refuses an unsafe identifier in \`alias\`` case named in the |
| 83 | + * #6212 record below **no longer exists** — it is replaced in place by |
| 84 | + * `[#14235] ESCAPES an alias that would close the quoting`, on the same input. |
| 85 | + * The record is left as it was measured rather than rewritten to match today's |
| 86 | + * cases: it is the #6212 ablation, not this one. |
| 87 | + * |
| 88 | + * ## Reverse verification (#14235) — direction predicted BEFORE it was run |
| 89 | + * |
| 90 | + * Restore the two pre-#14235 lines at the groupBy select site — |
| 91 | + * `this.assertSafeIdentifier(outKey)` above the push, and |
| 92 | + * `` `"${field}" AS "${outKey}"` `` as the aliased emission: |
| 93 | + * |
| 94 | + * - the two capture cases (`ESCAPES an alias that would close the quoting`, |
| 95 | + * `a dotted or spaced alias round-trips`) go RED by THROWING inside the call |
| 96 | + * — `unsafe identifier rejected: "bucket"; DROP TABLE deal; --"` / |
| 97 | + * `"Region Name"` — not on a comparison. |
| 98 | + * - both executing cases go RED the same way, inside `driver.aggregate`. |
| 99 | + * - the `field`-position control (`still refuses an unsafe identifier inside a |
| 100 | + * structured entry`) stays GREEN — untouched by this change, and that is |
| 101 | + * exactly what it is here to hold. |
| 102 | + * - `projects \`alias\` as the column name` and `an alias equal to the field |
| 103 | + * name emits no self-rename` stay GREEN: `bucket` and `stage` pass |
| 104 | + * `SAFE_IDENTIFIER` either way, so they pin byte-identical emission across |
| 105 | + * the change. |
| 106 | + * - every date-bucket, parity and string-form case stays GREEN. |
| 107 | + * |
| 108 | + * MEASURED, case for case as predicted — **4 failed / 14 passed of 18**: |
| 109 | + * |
| 110 | + * ``` |
| 111 | + * ESCAPES an alias that would close Error: RemoteTransport: unsafe identifier |
| 112 | + * the quoting rejected: "bucket"; DROP TABLE deal; --" |
| 113 | + * a dotted or spaced alias …rejected: "Region Name", then |
| 114 | + * round-trips …rejected: "deal.stage_bucket" |
| 115 | + * a dotted alias comes back under …rejected: "deal.stage_bucket" |
| 116 | + * the result key (executing) |
| 117 | + * an alias that tries to close the …rejected: "bucket"; DROP TABLE deal; --" |
| 118 | + * quoting (executing) |
| 119 | + * ── green ── |
| 120 | + * still refuses an unsafe identifier inside a structured entry (the CONTROL) |
| 121 | + * projects `alias` as the column name · no self-rename for alias === field |
| 122 | + * the string-form control · all five date-bucket refusals · both parity cases |
| 123 | + * ``` |
| 124 | + * |
| 125 | + * Not one failure came through a comparison: the restored gate throws before |
| 126 | + * any SQL is built, which is why the guard could not simply be deleted from the |
| 127 | + * `field` position and why the control above is the case that holds it. |
| 128 | + * |
| 129 | + * ⚠️ No `dist/` leg applies to this ablation. The suite imports the mutated |
| 130 | + * unit as `./remote-transport.js` — a RELATIVE, in-package specifier that |
| 131 | + * vitest resolves to `src/remote-transport.ts`, not through the package's |
| 132 | + * `exports` — and `vitest.config.ts` declares no alias (it sets |
| 133 | + * `disableConsoleIntercept` and nothing else). The mutation was proved on disk |
| 134 | + * by grep counts on both the injected and the removed text and by |
| 135 | + * `git hash-object` against the HEAD blob, and the restore by the same hash |
| 136 | + * matching again plus an empty `git diff HEAD`. |
| 137 | + * |
| 138 | + * # Reverse verification (#6212) — direction predicted BEFORE it was run, per case |
67 | 139 | * |
68 | 140 | * Restore `const groupBy: string[] = Array.isArray(query?.groupBy) ? … : []` |
69 | 141 | * (with a cast, since the narrowed signature no longer permits it): |
|
115 | 187 | * rather than merely that something was thrown. |
116 | 188 | */ |
117 | 189 |
|
118 | | -import { describe, it, expect, vi } from 'vitest'; |
| 190 | +import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; |
119 | 191 | import { SqlDriver } from '@objectstack/driver-sql'; |
120 | 192 | import { RemoteTransport } from './remote-transport.js'; |
| 193 | +import { TursoDriver } from './turso-driver.js'; |
| 194 | +import { makeLibsqlSqliteStub, asLibsqlClient, type LibsqlSqliteStub } from './libsql-sqlite-stub.testkit.js'; |
121 | 195 |
|
122 | 196 | interface WireBearingError extends Error { |
123 | 197 | code?: string; |
@@ -218,25 +292,59 @@ describe('[#6212] RemoteTransport compiles the GroupByNode union', () => { |
218 | 292 | expect(calls[0].sql).toBe('SELECT "stage", count("stage") AS "n" FROM "deal" GROUP BY "stage"'); |
219 | 293 | }); |
220 | 294 |
|
221 | | - it('[#6401] refuses an unsafe identifier in `alias`, not only in `field`', async () => { |
222 | | - // The alias is caller-supplied text that now reaches the statement as a |
223 | | - // quoted identifier, so it needs the gate `field` already has. The |
224 | | - // assertion names the OFFENDING TEXT, not just the sentence (#6144): a |
225 | | - // `field` that is itself safe is what makes this case reach the alias |
226 | | - // check at all. |
| 295 | + it('[#14235] ESCAPES an alias that would close the quoting — one inert name, one statement', async () => { |
| 296 | + // ⚠️ This case REPLACES the #6401 pin that asserted the same input is |
| 297 | + // REFUSED (`unsafe identifier rejected`, naming the offending text). That |
| 298 | + // pin was the deliberate call when the alias was newly read here, and |
| 299 | + // reversing it is a recorded, non-silent reversal rather than a rider: |
| 300 | + // #13714 routed BOTH of driver-sql's output-name positions through |
| 301 | + // `aliasIdentifierSql`, #14113 moved this transport's aggregation alias |
| 302 | + // to escaping, and #14235 brings the second output-name position of the |
| 303 | + // same method to the same line. An output-column key is a NAME: it is |
| 304 | + // quoted and escaped, never gated. |
| 305 | + // |
| 306 | + // The old assertion is REPLACED, not dropped — the exact input it named |
| 307 | + // is the input here, and what is pinned now is the statement it produces. |
227 | 308 | const { t, calls } = transportWithCapturingClient(); |
228 | | - const err = await t |
229 | | - .aggregate('deal', { |
230 | | - groupBy: [{ field: 'stage', alias: 'bucket"; DROP TABLE deal; --' }], |
231 | | - aggregations: [{ function: 'count', alias: 'n' }], |
232 | | - }) |
233 | | - .then( |
234 | | - () => { throw new Error('expected the transport to refuse an unsafe alias'); }, |
235 | | - (e) => e as Error, |
| 309 | + const rows = await t.aggregate('deal', { |
| 310 | + groupBy: [{ field: 'stage', alias: 'bucket"; DROP TABLE deal; --' }], |
| 311 | + aggregations: [{ function: 'count', alias: 'n' }], |
| 312 | + }); |
| 313 | + expect(rows).toEqual([]); |
| 314 | + // The whole payload is ONE column name, the quote doubled — the standard |
| 315 | + // escape inside a quoted SQL identifier. It is data, never grammar. |
| 316 | + expect(calls).toHaveLength(1); |
| 317 | + expect(calls[0].sql).toBe( |
| 318 | + 'SELECT "stage" AS "bucket""; DROP TABLE deal; --", count(*) AS "n" FROM "deal" GROUP BY "stage"', |
| 319 | + ); |
| 320 | + // ⛔ ONE statement, not two: a payload that had broken out of its quoting |
| 321 | + // would appear as a second one here. The executing block at the foot of |
| 322 | + // this file proves the same thing against a real database, which is the |
| 323 | + // only instrument that tells "escaped" apart from "broke out". |
| 324 | + expect(calls[0].sql.match(/SELECT/g)).toHaveLength(1); |
| 325 | + // And the grouping key is still the FIELD, exactly as for a bare alias. |
| 326 | + expect(calls[0].sql.endsWith('GROUP BY "stage"')).toBe(true); |
| 327 | + }); |
| 328 | + |
| 329 | + it('[#14235] a dotted or spaced alias round-trips as one quoted output column', async () => { |
| 330 | + // The reachable population the card measured: a caller writing |
| 331 | + // `groupBy: [{ field, alias }]` through the Query Protocol directly. |
| 332 | + // Both spellings work on the in-memory, MongoDB and SQL faces and were an |
| 333 | + // opaque 500 on this one — no `code`, no `status`, out of `mapDataError`. |
| 334 | + for (const alias of ['Region Name', 'deal.stage_bucket']) { |
| 335 | + const { t, calls } = transportWithCapturingClient(); |
| 336 | + await t.aggregate('deal', { |
| 337 | + groupBy: [{ field: 'stage', alias }], |
| 338 | + aggregations: [{ function: 'count', field: 'stage', alias: 'n' }], |
| 339 | + }); |
| 340 | + expect(calls).toHaveLength(1); |
| 341 | + expect(calls[0].sql).toBe( |
| 342 | + `SELECT "stage" AS "${alias}", count("stage") AS "n" FROM "deal" GROUP BY "stage"`, |
236 | 343 | ); |
237 | | - expect(err.message).toContain('unsafe identifier rejected'); |
238 | | - expect(err.message).toContain('bucket"; DROP TABLE deal; --'); |
239 | | - expect(calls).toEqual([]); |
| 344 | + // ⛔ The dot stays INSIDE the quotes. The failure this rules out is a |
| 345 | + // face that reads an output NAME as a qualified REFERENCE. |
| 346 | + expect(calls[0].sql).not.toContain('"deal"."stage_bucket"'); |
| 347 | + } |
240 | 348 | }); |
241 | 349 |
|
242 | 350 | it('still refuses an unsafe identifier inside a structured entry', async () => { |
@@ -394,4 +502,70 @@ describe('[#6212] RemoteTransport compiles the GroupByNode union', () => { |
394 | 502 | expect(err.message).toContain("dialect 'better-sqlite3'"); |
395 | 503 | }); |
396 | 504 | }); |
| 505 | + |
| 506 | + // ── [#14235] Executed, not merely emitted ───────────────────────────────── |
| 507 | + |
| 508 | + /** |
| 509 | + * ⚠️ Only EXECUTING the statement tells "escaped" apart from "broke out" — |
| 510 | + * #14113's reasoning one position over, and the reason its pin is backed by a |
| 511 | + * real database rather than a captured string. A capture assertion alone |
| 512 | + * passes on an alias that terminates the quoting, because the text still |
| 513 | + * *looks* like a select list. libsql IS SQLite, so the stub runs exactly what |
| 514 | + * this transport emits: an alias that escaped its quoting is a syntax error |
| 515 | + * (or a second statement better-sqlite3 refuses to prepare), and reading the |
| 516 | + * value back under the literal alias is the proof that it did not. |
| 517 | + */ |
| 518 | + describe('[#14235] the escaped groupBy alias is inert against a real database', () => { |
| 519 | + const DEAL = { |
| 520 | + name: 'deal', |
| 521 | + fields: { id: { type: 'string' }, stage: { type: 'string' }, amount: { type: 'number' } }, |
| 522 | + }; |
| 523 | + let driver: TursoDriver; |
| 524 | + let stub: LibsqlSqliteStub; |
| 525 | + |
| 526 | + beforeAll(async () => { |
| 527 | + stub = makeLibsqlSqliteStub(); |
| 528 | + driver = new TursoDriver({ url: 'libsql://groupby-alias.turso.io', client: asLibsqlClient(stub) }); |
| 529 | + await driver.connect(); |
| 530 | + // The mode this block is about — the one with its own hand-written SQL. |
| 531 | + expect(driver.transportMode).toBe('remote'); |
| 532 | + await driver.syncSchema(DEAL.name, DEAL); |
| 533 | + for (const row of [ |
| 534 | + { id: '1', stage: 'won', amount: 10 }, |
| 535 | + { id: '2', stage: 'won', amount: 20 }, |
| 536 | + { id: '3', stage: 'lost', amount: 30 }, |
| 537 | + ]) { |
| 538 | + await driver.create(DEAL.name, { ...row }); |
| 539 | + } |
| 540 | + }); |
| 541 | + |
| 542 | + afterAll(async () => { |
| 543 | + await driver.disconnect(); |
| 544 | + stub.close(); |
| 545 | + }); |
| 546 | + |
| 547 | + it('a dotted alias comes back under the result key the caller asked for, on rows', async () => { |
| 548 | + const rows = (await driver.aggregate(DEAL.name, { |
| 549 | + object: DEAL.name, |
| 550 | + groupBy: [{ field: 'stage', alias: 'deal.stage_bucket' }], |
| 551 | + aggregations: [{ function: 'sum', field: 'amount', alias: 'deal.total' }], |
| 552 | + } as never)) as Array<Record<string, unknown>>; |
| 553 | + const byBucket = Object.fromEntries(rows.map((r) => [r['deal.stage_bucket'], r['deal.total']])); |
| 554 | + // A dot is inert inside a quoted identifier — the whole claim of the card. |
| 555 | + expect(byBucket).toEqual({ won: 30, lost: 30 }); |
| 556 | + }); |
| 557 | + |
| 558 | + it('an alias that tries to close the quoting and append a statement leaves the table standing', async () => { |
| 559 | + const alias = 'bucket"; DROP TABLE deal; --'; |
| 560 | + const rows = (await driver.aggregate(DEAL.name, { |
| 561 | + object: DEAL.name, |
| 562 | + groupBy: [{ field: 'stage', alias }], |
| 563 | + aggregations: [{ function: 'count', alias: 'n' }], |
| 564 | + } as never)) as Array<Record<string, unknown>>; |
| 565 | + // The payload came back as a COLUMN NAME — it was data, never grammar. |
| 566 | + expect(rows.map((r) => r[alias]).sort()).toEqual(['lost', 'won']); |
| 567 | + // And the table it named is still there, with every row. |
| 568 | + expect(stub.raw.prepare('select count(*) as c from deal').all()).toEqual([{ c: 3 }]); |
| 569 | + }); |
| 570 | + }); |
397 | 571 | }); |
0 commit comments