Skip to content

Fix SQL injection in connector sample path and validate source queries - #688

Open
vishwasvaidya-cloudsufi wants to merge 2 commits into
developfrom
sql-identifier-escaping
Open

vishwasvaidya-cloudsufi wants to merge 2 commits into
developfrom
sql-identifier-escaping

Conversation

@vishwasvaidya-cloudsufi

@vishwasvaidya-cloudsufi vishwasvaidya-cloudsufi commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

What's this about

A VRP report found a SQL injection in the Cloud SQL PostgreSQL connector's sample path. A table path containing a " could break out of the quoted name and run extra statements, like an UPDATE, using the connection's saved credential. The same unescaped pattern existed in every connector.

What changed

Connectors: table names and the strata column are now properly escaped, so a quote inside a
name can't break out anymore. This is the fix for the reported vulnerability.

Source plugins (Import Query / Bounding Query): these run user-written SQL under the
connection's credential, so they're now validated to be a single read-only SELECT before
reaching the database. The check is lexical-primary (aware of strings, comments and quoted
identifiers), with JSqlParser as a secondary check, so it:

  • blocks stacked statements, writes/DDL, INTO, and FOR UPDATE/SHARE (including inside
    subqueries);
  • rejects constructs that parsers and drivers lex differently ($$…$$/$tag$…$tag$, Oracleq'…', MySQL /*! … */ executable comments);
  • still accepts valid dialect SQL that a strict parser would wrongly reject (TABLESAMPLE, FOR READ ONLY, LOCKING …, doubled backticks, MySQL \' and # comments).

Use the Database action plugins for SET, EXEC, UPDATE and anything that isn't a plain SELECT.

Split-By is now validated as a single column name (it's concatenated into generated SQL at
runtime).

Init queries: SET LANGUAGE / SET SESSION sql_mode now escape the interpolated value.

SQL Server [bracket] names keep working.

Design note (please read before "fixing" it)

The validator is a security gate, not a SQL linter. A malformed-but-harmless query (e.g.
select;) is intentionally allowed through to fail at the database. Valid-but-exotic string
literals ($$…$$, q'…', /*! … */) are rejected as a deliberate trade-off against the lexer-mismatch bypasses.

Heads up (behavior change)

Existing pipelines whose Import or Bounding Query isn't a single SELECT (multiple statements,SET …; SELECT, EXEC/CALL, FOR UPDATE) will fail and need to move that logic to an action. A small set of valid-but-exotic literals (dollar quotes, Oracle q'…', MySQL /*!*/ hints) are also rejected — worth a release note.

Testing

Unit tests for escaping in every connector (including the exact payload from the report), extensive validator tests (bypass attempts and valid dialect SQL), and end-to-end pipeline tests (HSQLDB) confirming bad import/bounding/split-by queries are blocked and no data is changed. The per-plugin e2e "invalid bounding query" scenarios were switched to a stacked write and assert the validation error. All CI suites, including the live-database e2e, pass.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces SQL identifier quoting to prevent SQL injection vulnerabilities across various database connectors, and adds query validation using jsqlparser to ensure that import and bounding queries are read-only SELECT statements. Feedback on these changes highlights several potential NullPointerException risks when database or schema parameters are null during identifier quoting, as well as a recommendation to use Locale.ROOT with toLowerCase() to avoid locale-sensitive string comparison issues.

@vishwasvaidya-cloudsufi
vishwasvaidya-cloudsufi force-pushed the sql-identifier-escaping branch 2 times, most recently from 54ea798 to 890fafd Compare November 2, 2026 05:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants