fix: SqlFormatter - fail closed on SQL parameter count mismatch - #11576
fix: SqlFormatter - fail closed on SQL parameter count mismatch#11576ovr wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
javascript
|
Claude finished @ovr's task in 2m 57s —— View job Review: sound fix, but the guard has a hole and one behavior break needs calling out — 2 medium, 3 low (4 inline comments)Full review — findings, security/perf notes, and task listTask list
VerdictThe direction is right. Findings
SecurityNo new exposure; this closes one. The mismatch error deliberately omits SQL and values, so no data-model text or parameter contents leak into logs. Worth confirming the drivers' error paths don't re-append the raw query when this throws. PerformanceThe loop no longer breaks once values are exhausted — it must scan the whole string to count. Irrelevant in practice (one regex pass over a query string, no allocation per non-placeholder match), and it's inherent to counting. DocumentationNo user-facing docs needed, but since this converts silently-tolerated input into a thrown error on a driver hot path, it belongs in the changelog / breaking-behavior notes for the Athena, Presto, ksql and Pinot drivers (see finding 3). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| protected static parameterCountMismatch(placeholderCount: number, valueCount: number): Error { | ||
| return new Error( | ||
| `SQL parameter count mismatch: ${placeholderCount} placeholder(s) but ${valueCount} value(s) supplied. ` + | ||
| 'A literal \'?\' in SQL must be escaped or bound as a parameter.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
The remediation hint is not actionable: this formatter provides no escape syntax for a literal ?. ?? means "identifier placeholder", ???+ is left verbatim (so ??? is the only accidental workaround), and there is no \?. A user whose data-model SQL contains regexp_like(x, 'a+?') or url_extract_parameter(url, '?') gets told to "escape" something they can't escape.
Suggest either rewording to point at the real fixes (remove the stray ?, or pass a matching number of values), or actually implementing an escape (e.g. treat \? as a literal ?) and then keeping this wording.
Also: as a protected static on a class that is neither exported nor subclassed, this indirection buys nothing. A module-level factory — or better, an exported SqlParameterCountMismatchError extends Error so the four drivers (AthenaDriver, PrestoDriver, KsqlDriver, PinotDriver) can catch it and attach query context — would be more useful.
| protected static parameterCountMismatch(placeholderCount: number, valueCount: number): Error { | |
| return new Error( | |
| `SQL parameter count mismatch: ${placeholderCount} placeholder(s) but ${valueCount} value(s) supplied. ` + | |
| 'A literal \'?\' in SQL must be escaped or bound as a parameter.' | |
| ); | |
| } | |
| protected static parameterCountMismatch(placeholderCount: number, valueCount: number): Error { | |
| return new Error( | |
| `SQL parameter count mismatch: ${placeholderCount} placeholder(s) but ${valueCount} value(s) supplied. ` + | |
| 'Remove any literal \'?\' from the SQL text or supply one value per placeholder.' | |
| ); | |
| } | |
| } |
SqlEscaper.format substituted `?`/`??` placeholders by scanning the SQL
string and silently tolerated a mismatch between placeholder and value
counts — surplus values were dropped and surplus placeholders left as a
literal `?`. Because the compiler emits exactly one `?` per parameter, any
such mismatch means a stray literal `?` in the SQL text (or a wrong-length
values array), which shifts every subsequent value into the wrong slot.
Count the consumable placeholders (`?`/`??` runs) in the single formatting
pass and throw when the total does not equal `values.length`, reporting both
counts. The empty-values array is checked too, so an unparametrized query is
no longer exempt from the check. This turns silent, wrong-position
substitution into a hard error across every interpolating driver (Presto,
Trino, Athena, Dremio, ksqlDB, Pinot, and the JDBC-backed engines) at one
shared point. A missing/omitted values argument (null/undefined) still passes
the SQL through unchanged, since no substitution is requested.
Tests updated to assert the fail-closed behavior and cover literal `?` in
SQL text, an empty values array, surplus/insufficient values, and `??`
identifier counting.
BREAKING CHANGE: these four drivers format user data-model SQL, and a literal
`?` that previously rendered fine now throws — including in unparametrized
queries, which used to skip the check entirely. The old loop stopped once all
values were consumed, so a stray `?` positioned AFTER every real placeholder
(or in a query with no parameters at all) was left verbatim, e.g.
formatAnsi("SELECT ?, regexp_like(a, 'x+?')", ['v']) returned
"SELECT 'v', regexp_like(a, 'x+?')". Failing closed in both directions is
deliberate; a lazy-quantifier regex or a `?` inside a string literal in the
data model must now be bound as a parameter rather than passed through the `?`
placeholder channel.
dabc3ee to
70b1cfe
Compare
Re-review after force-push: two prior findings fixed, but the
|
| # | Sev | Where | Issue |
|---|---|---|---|
| 1 | Medium | src/sql-escape.ts:140-148 |
Removing the [] exemption closed the hole but removed the last workaround for a literal ? — and the formatter still has no escape syntax. regexp_like(a, 'x+?') with no bound params now hard-fails on all six drivers with no user-side fix. Suggest implementing \?, or keeping the [] exemption. |
| 2 | Low | src/sql-escape.ts:188-193 |
Error message still says the ? "must be escaped" — instructing the user toward a fix that does not exist. Also protected static on a class that is neither exported nor subclassed. (existing thread, deliberately left unresolved) |
| 3 | Low | src/sql-escape.ts:164-179 |
if (valuesIndex < valueList.length) is unreachable-false on any returning path, so the skipped work is always discarded. Counting in a cheap pre-pass makes the invariant structural and drops the eslint-disable. Also fixes error-ordering vs. escapeValue's Unsupported parameter type throw. |
| 4 | Low | test/sql-escape.test.ts:570-583 |
Nothing asserts the message carries both counts (the point of the error); format(sql) vs format(sql, []) asymmetry is pinned but unnamed; guard only exercised through ansi/presto. |
Finding 1 is the one I'd want a decision on before merge — not because the direction is wrong (failing closed is right), but because the blast radius is bigger than the description says. Non-greedy quantifiers (.*?, x+?) inside regexp_like / regexp_extract / regexp_replace are ordinary in Athena/Trino/Presto data models, and every driver normalizes to format(query, values || []), so the zero-params path is the common one. Either add \? as an escape (then the error message becomes true), or keep [] exempt — the corruption vector needs ≥1 value to shift anything, so the actual hazard closes either way.
Security
Still net-positive; this closes an injection-adjacent hazard rather than opening one. The error deliberately omits SQL text and parameter values, so nothing from the data model or the parameters leaks into logs. Worth confirming the drivers' catch paths don't re-append the raw query when this throws.
Performance
Non-issue. The loop must scan the whole string to count, and the pre-pass in finding 3 adds a second scan — both are one regex walk over a query string with no per-match allocation.
Documentation
Needs a release / changelog note. Previously-valid data models become hard query failures on six drivers: athena, prestodb (incl. Trino), ksql, pinot, dremio — and I listed only four last round.
· branch fix/backend-shared-sql-param-count-mismatch · View job run
| public format(sql: string, values?: unknown): string { | ||
| if (values === null || values === undefined) { | ||
| return sql; | ||
| } | ||
|
|
||
| const valueList = Array.isArray(values) ? values : [values]; | ||
| if (valueList.length === 0) { | ||
| return sql; | ||
| } | ||
|
|
||
| const placeholders = /\?+/g; | ||
|
|
There was a problem hiding this comment.
Closing the [] exemption removed the last workaround for a literal ?, and there is no escape syntax to replace it. This is the widest-reaching behavior change in the PR and it isn't in the description.
Before: valueList.length === 0 returned sql verbatim, so a data model whose SQL contained a ? but bound no parameters worked. Now:
formatAnsi("SELECT regexp_like(a, 'x+?') FROM t", []) // throws (pinned at test:581)Every consumer normalizes to [], so that path is the common one, not an edge case — six call sites, not the four I listed earlier:
cubejs-athena-driver/src/AthenaDriver.ts:71(params: unknown[])cubejs-prestodb-driver/src/PrestoDriver.ts:219cubejs-ksql-driver/src/KsqlDriver.ts:170cubejs-pinot-driver/src/PinotDriver.ts:149cubejs-dremio-driver/driver/DremioDriver.js:21,211
The realistic breakage is non-greedy regex quantifiers — .*?, x+?, \d{2,}? — inside regexp_like / regexp_extract / regexp_replace, which are ordinary in Athena/Trino/Presto data models, plus a bare ? in any string literal. Such a model becomes unqueryable with no user-side fix: \? is not recognized, and the only thing that survives is the accidental ???+ run. The error message compounds it by instructing the user to escape something the formatter cannot escape.
I'd support one of:
- Add an escape — treat
\?as a literal?that consumes no value and isn't counted, then the current message becomes true and models have a migration path. Cheapest inside the existing loop: widen the regex to/\\\?|\?+/gandcontinue(emitting?) on the escaped branch. - Keep the
[]exemption and accept that only the mismatch-with-values case fails closed — weaker, but the corruption vector (stray?shifting real values) requires at least one value, so it still closes the actual hazard.
Either way this needs a release note for all six drivers, since it turns previously-valid data models into hard query failures.
| placeholderCount += 1; | ||
|
|
||
| if (valuesIndex < valueList.length) { | ||
| const rendered = match[0].length === 2 | ||
| ? this.escapeIdentifier(String(valueList[valuesIndex])) | ||
| : this.escapeValue(valueList[valuesIndex]); | ||
|
|
||
| result += sql.slice(chunkIndex, match.index) + rendered; | ||
| chunkIndex = placeholders.lastIndex; | ||
| valuesIndex += 1; | ||
| } | ||
| match = placeholders.exec(sql); | ||
| } | ||
|
|
||
| if (placeholderCount !== valueList.length) { | ||
| throw SqlEscaper.parameterCountMismatch(placeholderCount, valueList.length); | ||
| } |
There was a problem hiding this comment.
Minor, but the shape is misleading: valuesIndex < valueList.length at line 166 can only be false on a path that throws at 177, so every string this branch skips building is discarded. The guard reads as if partial substitution is a supported outcome, when it never escapes the function.
Counting is cheap and independent of escaping, so the two concerns can separate — validate first, then substitute with no conditional at all:
const placeholderCount = (sql.match(/\?+/g) ?? [])
.filter((run) => run.length <= 2).length;
if (placeholderCount !== valueList.length) {
throw SqlEscaper.parameterCountMismatch(placeholderCount, valueList.length);
}…after which the substitution loop drops the if and the eslint-disable no-cond-assign. Two passes over a query string is not a cost anyone can measure, and it makes the invariant "we only ever substitute when the counts already match" structural rather than something a reader has to derive from the throw fifteen lines down.
Related ordering nit: escapeValue can throw Unsupported parameter type (line 131) before the count check runs, so a call that is both count-mismatched and carries a symbol/function value reports the less actionable of the two errors. Validating up front fixes that too.
| it('leaves the no-substitution paths non-throwing', () => { | ||
| expect(formatAnsi('SELECT 1')).toBe('SELECT 1'); | ||
| expect(formatAnsi('SELECT 1', [])).toBe('SELECT 1'); | ||
| expect(formatAnsi('SELECT ???', [])).toBe('SELECT ???'); | ||
| expect(formatAnsi('SELECT ?')).toBe('SELECT ?'); | ||
| expect(formatAnsi('SELECT ?', null)).toBe('SELECT ?'); | ||
| expect(formatAnsi('SELECT ?', undefined)).toBe('SELECT ?'); | ||
| }); | ||
|
|
||
| it('rejects a placeholder against an empty values array', () => { | ||
| expect(() => formatAnsi('SELECT ?', [])).toThrow(/parameter count mismatch/); | ||
| expect(() => formatAnsi("SELECT regexp_like(a, 'x+?') FROM t", [])) | ||
| .toThrow(/parameter count mismatch/); | ||
| }); |
There was a problem hiding this comment.
The regression case I asked about last round is pinned now (line 549) — thanks, that's the important one. Two narrow gaps left in this block:
- Nothing asserts the counts in the message. Every case matches
/parameter count mismatch/, which passes even if the two numbers were swapped or both wrong — and reporting both counts is the stated point of the new error. One case with.toThrow(/2 placeholder\(s\) but 1 value\(s\)/)would lock it in. format(sql)vsformat(sql, [])asymmetry isn't explained. Lines 571-576 and 579-580 sit adjacent and disagree:formatAnsi('SELECT ?')returns'SELECT ?',formatAnsi('SELECT ?', [])throws. That's a defensible contract (undefined= "no parameter binding requested" vs[]= "a values array of length 0"), and the tests do pin both — but the test names don't say which is which, so a future reader hitting the throw can't tell whether theundefinedpath is intentional or just untightened. Renaming to something like'treats a missing values argument as no-binding-requested (does not enforce the count)'carries the intent.
Also: the guard is only driven through formatAnsi / presto. Since the counting lives in format and not in the dialect rules it can't be dialect-dependent, but one formatMySql or formatSparkSql case would keep it that way if someone later moves the check.
2 participants
SqlEscaper.format substituted
?/??placeholders by scanning the SQL string and silently tolerated a mismatch between placeholder and value counts — surplus values were dropped and surplus placeholders left as a literal?. Because the compiler emits exactly one?per parameter, any such mismatch means a stray literal?in the SQL text (or a wrong-length values array), which shifts every subsequent value into the wrong slot.Count the consumable placeholders (
?/??runs) in the single formatting pass and throw when the total does not equalvalues.length, reporting both counts. This turns silent, wrong-position substitution into a hard error.