fix(trino-driver/prestodb-driver): Сorrect SQL parameter escaping

[ovr]

No description provided.

[claude]

Claude finished @ovr's task in 1m 30s —— View job

---

Code Review

Verdict: Looks good. The change correctly replaces sqlstring (a MySQL-dialect escaper) with a proper ANSI escaper for Presto/Trino — closing a real injection vector where a payload like ' OR 1=1 -- used to render as '\' OR 1=1 --' and, since Presto doesn't recognize \' as an escape, allowed the literal to break out.

Details — 0 high, 2 medium, 2 low

Findings

Medium — Date values render as ISO 8601 with T separator
packages/cubejs-backend-shared/src/sql-escape.ts:81 uses value.toISOString(), producing '2024-01-02T03:04:05.000Z'. Presto/Trino string→timestamp coercions typically expect 'YYYY-MM-DD HH:MM:SS[.fff]' (space separator, no Z). If any caller passes a raw Date where a TIMESTAMP context is expected, this may now fail where sqlstring's space-separated format used to work. The schema compiler generally pre-stringifies dates, so this is likely dormant — worth confirming with an integration run against Presto.

Medium — Nested-array parenthesization semantics differ from sqlstring
sqlstring.escape([[1,2,3]]) produces (1, 2, 3), which was the idiomatic way to bind an IN (?) list. SqlEscaper.escapeValue (sql-escape.ts:84-86) joins with , but never wraps in parentheses, so a caller that wrote WHERE id IN (?) with [[1,2,3]] (double-wrapped) now emits WHERE id IN (1, 2, 3) at the outer level but yields a bare comma-list at the value slot. This is only a compatibility risk if any Presto caller relies on the nested-array pattern; a note in the JSDoc for escapeValue would help future readers.

Low — \?+ matches inside string literals in the input SQL
sql-escape.ts:111 — the regex greedily matches every ? in the template, including any inside pre-existing string literals (e.g. SELECT 'a?b', ?). This is inherited behavior from sqlstring and probably fine in practice, but if you ever want to harden it, a lightweight string-literal-aware scanner would remove the last remaining foot-gun.

Low — Missing CHANGELOG note / driver docs mention
Purely security-flavored fixes are worth calling out in release notes so Presto/Trino users know to upgrade. Consider a line in the PR description or a driver changelog entry.

Positives

• Placeholder handling (? value vs ?? identifier, ???+ left intact) is faithful to sqlstring's useful semantics without inheriting its backslash quirks.
• Test coverage is excellent: adversarial payload matrix, exhaustive small-string round-trip via stringsUpToLength, Unicode/ZWJ/combining-mark cases, and negative tests for non-finite numbers and unsupported objects.
• escapeBackslash split into a dialect flag with AnsiSqlDialect / MySqlDialect presets is clean and directly makes the bug impossible for ANSI dialects.
• Bigint, boolean, and NULL handling all match what Presto expects.
• Fixture-decoded ANSI/MySQL literals (decodeAnsiLiteral / decodeMySqlLiteral) provide strong invariants — any regression that lets a quote escape early would immediately fail these tests.

· Branch: `fix/trin-presto-param-escaper`

[claude]

Claude finished @ovr's task in 42s —— View job

---

Code Review (updated after 716c7a7)

Verdict: Still LGTM. The follow-up commit resolves the nested-array compatibility finding from my prior review — escapeValue([[1,2,3]]) now produces (1, 2, 3), matching sqlstring semantics for IN (?) and multi-row VALUES ? bindings. Tests cover both single- and double-nested cases plus an adversarial injection payload inside a nested array.

Details — 0 high, 1 medium, 2 low remaining

Resolved since last review

• ✅ Nested-array parenthesization: sql-escape.ts:84-88 now wraps inner arrays in (...), restoring sqlstring behavior. New tests at test/sql-escape.test.ts:272-282 cover IN (?), doubly-nested arrays, and VALUES ? multi-row insert. The updated adversarial test at line 285 confirms injection payloads are still escaped correctly inside the new parentheses.

Still open

Medium — Date values render as ISO 8601 with T separator
sql-escape.ts:81 still uses value.toISOString(), producing '2024-01-02T03:04:05.000Z'. Presto/Trino string→timestamp coercions typically expect 'YYYY-MM-DD HH:MM:SS[.fff]'. The schema compiler generally pre-stringifies dates so this is likely dormant, but worth confirming with a live Presto integration run. Fix this →

Low — \?+ matches inside string literals
sql-escape.ts:113 — the placeholder regex matches every ? in the template, including any inside pre-existing string literals (e.g. SELECT 'a?b', ?). Inherited from sqlstring; likely fine in practice, but a literal-aware scanner would remove the last foot-gun.

Low — Missing CHANGELOG / release-notes mention
Security-flavored fixes are worth calling out so Presto/Trino users know to upgrade.

Positives

• Placeholder handling (? value vs ?? identifier, ???+ left intact) is faithful to sqlstring without inheriting its backslash quirks.
• Test coverage is excellent: adversarial payload matrix, exhaustive small-string round-trip via stringsUpToLength, Unicode/ZWJ/combining-mark cases, negative tests for non-finite numbers and unsupported objects, plus the new nested-array compatibility tests.
• escapeBackslash split into a dialect flag with AnsiSqlDialect / MySqlDialect presets is clean and makes the bug impossible for ANSI dialects.
• Bigint, boolean, and NULL handling all match what Presto expects.
• Fixture-decoded ANSI/MySQL literals (decodeAnsiLiteral / decodeMySqlLiteral) provide strong invariants — any regression that lets a quote escape early would immediately fail these tests.

· Branch: fix/trin-presto-param-escaper ·

[codecov]

Codecov Report

❌ Patch coverage is 92.42424% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.59%. Comparing base (9f13a7f) to head (149e466).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
packages/cubejs-backend-shared/src/sql-escape.ts	96.72%	1 Missing and 1 partial ⚠️
...ackages/cubejs-prestodb-driver/src/PrestoDriver.ts	0.00%	1 Missing and 1 partial ⚠️
...s/cubejs-schema-compiler/src/adapter/BaseFilter.ts	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #11345       +/-   ##
===========================================
+ Coverage   59.57%   79.59%   +20.01%     
===========================================
  Files         216      474      +258     
  Lines       17526    98300    +80774     
  Branches     3549     3570       +21     
===========================================
+ Hits        10441    78238    +67797     
- Misses       6574    19549    +12975     
- Partials      511      513        +2     

Flag	Coverage Δ
cube-backend	59.72% <92.42%> (+0.15%)	⬆️
cubesql	83.91% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @ovr's task in 51s —— View job

---

Code Review (updated after 958a5da)

Verdict: Still LGTM. The new commit widens sqlstring value-compatibility (buffers, plain objects, non-finite numbers). Behavior now closely mirrors sqlstring.escape while keeping the ANSI-quote fix that motivated the PR.

Details — 0 high, 1 medium, 3 low

New in 958a5da

• ✅ Non-finite numbers now render verbatim (Infinity, -Infinity, NaN) instead of throwing. Matches sqlstring and preserves back-compat. Note: these are still not valid Presto/Trino literals at execution time — a caller that used to get an early Error will now get a downstream engine error. Deliberate, but worth flagging.
• ✅ Buffers render as ANSI hex literals X'…' — correct for Presto/Trino VARBINARY.
• ✅ Plain objects render as "key" = value pairs (SET-clause pattern), with function-valued properties skipped and nested objects stringified to '[object Object]'. Matches sqlstring exactly.

Still open

Medium — Date values render as ISO 8601 with T separator
sql-escape.ts:82 still uses value.toISOString(), producing '2024-01-02T03:04:05.000Z'. Presto/Trino string→timestamp coercions typically expect 'YYYY-MM-DD HH:MM:SS[.fff]'. Likely dormant since the schema compiler pre-stringifies dates, but worth confirming via a live Presto run. Fix this →

Low — Object-to-SET expansion is a foot-gun for untrusted keys
sql-escape.ts:107-114 — keys become identifiers via escapeIdentifier, which is safe against quote breakout but still allows an untrusted object to inject arbitrary column names into a WHERE ? or SET ? clause. This exactly matches sqlstring's behavior, so it's compat-preserving, but a JSDoc note on escapeValue warning callers not to pass untrusted objects would help future readers.

Low — \?+ matches inside string literals
sql-escape.ts:133 — placeholder regex matches every ? in the template, including any inside pre-existing string literals (e.g. SELECT 'a?b', ?). Inherited from sqlstring; fine in practice.

Low — Missing CHANGELOG / release-notes mention
Security-flavored fix; worth calling out in release notes so Presto/Trino users know to upgrade.

Positives

• Placeholder handling (? vs ??, ???+ left intact) faithful to sqlstring.
• Nested-array parenthesization restored ((1, 2, 3) for IN (?) / VALUES ?).
• escapeBackslash dialect flag with AnsiSqlDialect / MySqlDialect presets makes the original bug impossible for ANSI dialects.
• Buffer, bigint, boolean, NULL, and object handling all match sqlstring semantics — genuine drop-in replacement.
• Test coverage remains excellent, and the new commit adds explicit coverage for buffer, object-SET, function-property-skip, and '[object Object]' stringification cases.
• Fixture-decoded ANSI/MySQL literals give strong invariants — regressions on quote escaping would fail immediately.

· Branch: fix/trin-presto-param-escaper ·

[claude]

Claude finished @ovr's task in 1m 48s —— View job

---

Code Review (updated after 419851b)

Verdict: Still LGTM. The two new commits — c6fa08e (scalar-value normalization in format) and 419851b (LIKE-pattern escape for literal backslashes) — are focused, well-tested, and preserve sqlstring compatibility while extending the ANSI-escape fix into the LIKE-filter path on both TS and Rust sides.

Details — 0 high, 1 medium, 3 low

New in c6fa08e — normalize scalar format values

• ✅ format(sql, values) now accepts a bare scalar (string/number/bool) and wraps it in a one-element list, matching sqlstring.format. Null/undefined short-circuits to the original SQL, which also matches. Test coverage at packages/cubejs-backend-shared/test/sql-escape.test.ts:349-358 and :432-435 is complete: string, empty string, 0, false, and ?? identifier scalars.
• Type signature widened from unknown[] to unknown — that's the correct shape given the new behavior. Callers passing arrays are unchanged; callers passing scalars are newly accepted rather than silently mis-behaving.

New in 419851b — escape literal \ in LIKE values

• ✅ packages/cubejs-schema-compiler/src/adapter/BaseFilter.ts:112 now escapes \ in addition to _/%. Previously a value like folder\name_% produced folder\\name\_\%-shaped SQL that mis-tracked which \ was the escape — the standalone \n could break the LIKE match or, with a rogue trailing \, dangle at the end of the pattern.
• ✅ Presto-side wiring is consistent: PrestodbQuery.ts:180 declares like_escape_char = '\', the Rust planner reads it in plan.rs:811-826, and like.rs:11-16 applies escape_like_pattern per value. escape_like_pattern (like.rs:53-63) is a tight per-char loop with String::with_capacity, no regex — good.
• ✅ Test at prestodb-query.test.ts:57-79 exercises the round-trip (ESCAPE '\' in SQL + folder\\\\name\\_\\% parameter), and the Rust test at to_sql.rs:446-458 mirrors the same fixture.

Still open

Medium — Date values render as ISO 8601 with T separator
sql-escape.ts:82 still uses value.toISOString(), producing '2024-01-02T03:04:05.000Z'. Presto/Trino string→timestamp coercions typically expect 'YYYY-MM-DD HH:MM:SS[.fff]' (space separator, no trailing Z). Likely dormant since the schema compiler pre-stringifies dates, but worth confirming via a live Presto integration run. Fix this →

Low — BaseFilter.escapeWildcardChars change is dialect-agnostic
BaseFilter.ts:112 — the base implementation now escapes \ for every non-MSSQL driver, but only Oracle/Snowflake/Presto explicitly emit ESCAPE '\'. Other dialects (Postgres, BigQuery, etc.) call LIKE without an ESCAPE clause, so a literal \ in user input still ends up as \\ in the value slot — harmless in most cases, but not strictly correct for a driver whose LIKE dialect treats \ as literal. If any downstream user reports LIKE regressions after this change, this is the first place to look. Preexisting issue amplified, not introduced.

Low — \?+ matches inside string literals
sql-escape.ts:133 — placeholder regex still matches every ? in the template, including any inside pre-existing string literals (e.g. SELECT 'a?b', ?). Inherited from sqlstring; fine in practice.

Low — Missing CHANGELOG / release-notes mention
Security-flavored fix in a well-used driver; worth calling out so Presto/Trino users know to upgrade.

Positives

• Scalar-normalization matches sqlstring.format exactly, including the null-short-circuit and ?? identifier path.
• LIKE-escape fix is symmetric across TS and Rust planners — both paths get the same value shape and the same ESCAPE '\' clause.
• Rust like_escape_char() (plan.rs:811-826) rejects multi-char templates loudly, so a mis-configured template can't silently corrupt escaping. Correct use of contains_template for optional-per-driver semantics.
• escape_like_pattern in Rust preallocates and iterates once — no regex overhead per value.
• Test fixtures cover the tricky combinatorics (\ + _ + % in one value, plain-scalar format inputs, false/0/empty-string edge cases).
• escapeBackslash dialect flag with AnsiSqlDialect / MySqlDialect presets remains a clean line-in-the-sand for the original bug.

· Branch: fix/trin-presto-param-escaper · ·

[claude]

Claude finished @ovr's task in 51s —— View job

---

Code Review (updated after 7e5e5c6)

Verdict: Still LGTM. 7e5e5c6 narrows the public API surface — AnsiSqlDialect, MySqlDialect, and SqlEscaper are now module-local; only SqlDialectEscapeRules (interface) and the top-level escapeAnsiSqlValue / escapeMysqlSqlValue / format helpers remain exported via sql-escape.ts. Good hygiene — callers can't reach in and hand-construct escapers with custom rules, so the ANSI-vs-MySQL choice stays a two-option switch rather than an open extension point.

Details — 0 high, 1 medium, 3 low remaining

New in 7e5e5c6

• ✅ Removed export from AnsiSqlDialect, MySqlDialect, and SqlEscaper at sql-escape.ts:19,26,33. Test file trimmed to exercise only the public helpers (-29 lines of tests referencing internals). No behavior change; strictly encapsulation.
• The re-export at packages/cubejs-backend-shared/src/index.ts:32 (export * from './sql-escape') now correctly surfaces only the intended public API.

Still open

Medium — Date values render as ISO 8601 with T separator
sql-escape.ts:82 still uses value.toISOString(), producing '2024-01-02T03:04:05.000Z'. Presto/Trino string→timestamp coercions typically expect 'YYYY-MM-DD HH:MM:SS[.fff]' (space separator, no trailing Z). Likely dormant since the schema compiler pre-stringifies dates, but worth confirming via a live Presto integration run. Fix this →

Low — BaseFilter.escapeWildcardChars change is dialect-agnostic
BaseFilter.ts:112 — the base implementation now escapes \ for every non-MSSQL driver, but only Oracle/Snowflake/Presto explicitly emit ESCAPE '\'. Other dialects (Postgres, BigQuery, etc.) call LIKE without an ESCAPE clause, so a literal \ in user input still ends up as \\ in the value slot — harmless in most cases, but not strictly correct for a driver whose LIKE dialect treats \ as literal. First place to look if any downstream user reports LIKE regressions.

Low — \?+ matches inside string literals
sql-escape.ts:133 — placeholder regex still matches every ? in the template, including any inside pre-existing string literals (e.g. SELECT 'a?b', ?). Inherited from sqlstring; fine in practice.

Low — Missing CHANGELOG / release-notes mention
Security-flavored fix in a well-used driver; worth calling out so Presto/Trino users know to upgrade.

Positives

• Public API is now tight: interface + three top-level functions. Internals cannot be misused.
• Placeholder handling (? vs ??, ???+ left intact) faithful to sqlstring.
• Nested-array parenthesization matches sqlstring for IN (?) / VALUES ?.
• Buffer, bigint, boolean, NULL, object, and non-finite-number handling all match sqlstring.
• LIKE-escape fix is symmetric across TS and Rust planners.
• Rust like_escape_char() rejects multi-char templates loudly; escape_like_pattern preallocates and iterates once — no regex overhead.
• Test coverage remains excellent even after trimming internals-only tests.

· Branch: fix/trin-presto-param-escaper · ·

[claude]

Claude finished @ovr's task in 41s —— View job

---

Code Review (updated after 76beda9)

Verdict: Still LGTM. 0 high, 1 medium, 3 low — no change from prior pass. The new commit adds PrestoDriver-level unit tests only; no source behavior changed.

Details

New in 76beda9

• ✅ packages/cubejs-prestodb-driver/test/unit/params-escaping.test.ts (+75) exercises prepareQueryWithParams end-to-end at the driver boundary. Good adversarial coverage:
  • LIKE-escape passthrough for pre-escaped \_ / \% (lines 17-28) — confirms the driver does not re-escape wildcards emitted by the schema compiler.
  • Literal \ in LIKE parameter not double-escaped (lines 30-41).
  • Quote-injection payload with trailing -- inside a LIKE literal (lines 43-52) — proves the ANSI single-quote doubling holds under a realistic drop-table attempt.
  • Backslash-then-quote payload (lines 54-63) — the exact case that broke under sqlstring's MySQL dialect and motivated this PR.
  • Multiple-placeholder ordering (lines 65-74) — string + string + number mixed with a LIKE clause.

Still open (unchanged)

Medium — Date values render as ISO 8601 with T separator
packages/cubejs-backend-shared/src/sql-escape.ts:82 still uses value.toISOString(). Presto/Trino string→timestamp coercions typically expect 'YYYY-MM-DD HH:MM:SS[.fff]'. Likely dormant since the schema compiler pre-stringifies dates. Fix this →

Low — BaseFilter.escapeWildcardChars change is dialect-agnostic
packages/cubejs-schema-compiler/src/adapter/BaseFilter.ts:112 now escapes \ for every non-MSSQL driver, but only Oracle/Snowflake/Presto explicitly emit ESCAPE '\'. First place to look if any downstream user reports LIKE regressions.

Low — \?+ matches inside string literals
sql-escape.ts:133 — placeholder regex matches every ? including any inside pre-existing string literals. Inherited from sqlstring; fine in practice.

Low — Missing CHANGELOG / release-notes mention
Security-flavored fix; worth calling out so Presto/Trino users know to upgrade.

Positives (cumulative)

• End-to-end coverage now spans: escaper unit tests, schema-compiler LIKE-clause tests (TS + Rust), and driver-level parameter-binding tests. Three independent layers reinforce the invariant.
• Public API stayed narrow after 7e5e5c6 (interface + three helpers).
• Nested-array parenthesization, buffer/bigint/boolean/NULL/object/non-finite handling all mirror sqlstring.
• LIKE-escape fix is symmetric across TS and Rust planners.
• Rust like_escape_char() rejects multi-char templates loudly; escape_like_pattern preallocates and iterates once.

· Branch: fix/trin-presto-param-escaper ·

[claude]

Claude finished @ovr's task in 38s —— View job

---

Code Review (updated after 149e466)

Verdict: Still LGTM. 0 high, 1 medium, 3 low — no change from prior pass. 149e466 adds three new driver-level unit tests only; no source behavior changed.

Details

New in 149e466

• ✅ packages/cubejs-prestodb-driver/test/unit/params-escaping.test.ts:65-97 adds three tests that lock in the boundary between "wildcard escaping is the schema compiler's job" and "the driver treats % as opaque data":
  • Literal % in an equality parameter ('100% cotton') stays verbatim — no accidental wildcard escaping in non-LIKE contexts.
  • Raw % in a LIKE parameter passes through untouched — confirms the driver does not double-escape wildcards on top of what the compiler already emitted.
  • Quote-injection payload combined with % (50%' OR 1=1 --) — verifies the ANSI single-quote doubling still works when the value also contains wildcard characters.

These complement the existing adversarial cases (\', -- tail, multi-placeholder ordering) — coverage is now genuinely thorough for the driver's stated responsibilities.

Still open (unchanged)

Medium — Date values render as ISO 8601 with T separator
packages/cubejs-backend-shared/src/sql-escape.ts:82 still uses value.toISOString(). Presto/Trino string→timestamp coercions typically expect 'YYYY-MM-DD HH:MM:SS[.fff]'. Likely dormant since the schema compiler pre-stringifies dates. Fix this →

Low — BaseFilter.escapeWildcardChars change is dialect-agnostic
packages/cubejs-schema-compiler/src/adapter/BaseFilter.ts:112 now escapes \ for every non-MSSQL driver, but only Oracle/Snowflake/Presto explicitly emit ESCAPE '\'. First place to look if any downstream user reports LIKE regressions.

Low — \?+ matches inside string literals
sql-escape.ts:133 — placeholder regex matches every ? including any inside pre-existing string literals. Inherited from sqlstring; fine in practice.

Low — Missing CHANGELOG / release-notes mention
Security-flavored fix; worth calling out so Presto/Trino users know to upgrade.

Positives (cumulative)

• End-to-end coverage now spans three independent layers: escaper unit tests, schema-compiler LIKE-clause tests (TS + Rust), and driver-level parameter-binding tests — including the new "wildcard-is-data-not-syntax" boundary.
• Public API stayed narrow after 7e5e5c6 (interface + three helpers).
• Nested-array parenthesization, buffer/bigint/boolean/NULL/object/non-finite handling all mirror sqlstring.
• LIKE-escape fix is symmetric across TS and Rust planners.
• Rust like_escape_char() rejects multi-char templates loudly; escape_like_pattern preallocates and iterates once.

· Branch: fix/trin-presto-param-escaper ·