Skip to content

fix(clickhouse): escape SQL identifiers and literals in metadata/DDL paths (#1914) - #2174

Open
HandSonic wants to merge 5 commits into
OtterMind:mainfrom
HandSonic:fix/sqli-clickhouse
Open

fix(clickhouse): escape SQL identifiers and literals in metadata/DDL paths (#1914)#2174
HandSonic wants to merge 5 commits into
OtterMind:mainfrom
HandSonic:fix/sqli-clickhouse

Conversation

@HandSonic

@HandSonic HandSonic commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Related issue

Part of the SQL-injection hardening tracked in #1914 (multi-plugin effort; this PR covers ClickHouse). Prior art: #2052 (Oracle escapeSqlLiteral), #2053 (SQL Server identifier quoting).

Summary

These are second-order injection paths: values such as table/schema/view/index names originate from the connected database's own metadata, so exploitation requires a maliciously named object in a target database. The fix is still worthwhile hardening and matches the pattern already merged for Oracle and SQL Server.

What changed:

  • New ClickHouseSqlEscapes helper:
    • escapeSqlLiteral — doubles backslashes first, then single quotes (ClickHouse treats backslash as an escape character inside string literals).
    • quoteIdentifier/escapeIdentifier — strips one surrounding backtick pair and doubles every embedded backtick for quoted-identifier positions.
  • Applied across metadata/DDL SQL-building sites in 9 files (see diff; pom change is test-scope JUnit only). The naive format() helper was fixed at the source, so all callers now get backtick doubling.
  • ENGINE clause and DEFAULT expressions are guarded by strict whitelist/regex validation; ) is forbidden inside the parenthesized argument to block paren-breakout (e.g. Memory() ORDER BY tuple() -- x).
  • Quoted string DEFAULT literals (e.g. 'abc') remain supported: the inner content is escaped with escapeSqlLiteral before re-quoting, so 'a');DROP TABLE t;--' is emitted as a harmless literal.
  • Column types outside the enum are handled fail-closed: requireColumnTypeExpression validates the declared type shape (letters/digits/underscore; spaces, commas, quotes and = — for Enum8('a'=1) — only inside balanced parentheses) instead of emitting it verbatim. The fallback path keeps Nullable(...) wrapping and DEFAULT handling (validated) so unknown-type columns no longer lose them. getByType no longer strips digits (which broke Int32/Int64 resolution); it trims and cuts at ( before the map lookup.
  • exportFunctionDdl now quotes the function name in DROP FUNCTION IF EXISTS; buildCreateDatabase quotes the database name via the helper.
  • Added ClickHouseSqlEscapesTest covering backslash+quote literal doubling, backtick identifier doubling, malicious-name neutralization, engine/default paren-breakout rejection, quoted string defaults, mixed-case/digit type resolution, and unknown-type fallback validation.

Affected surfaces

  • Frontend / Web
  • Backend / API / Storage
  • Database plugin / Driver
  • JCEF / Desktop packaging
  • CI / Build / Release
  • Documentation only

Verification

  • Command: mvn -B -pl chat2db-community-plugins/chat2db-community-clickhouse -f chat2db-community-server/pom.xml -Dmaven.test.skip=false -DskipTests=false -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.test.failure.ignore=false clean test
  • Result: Tests run: 24, Failures: 0, Errors: 0, Skipped: 0 (includes pre-existing module tests).
  • Manual verification: escaping is identity for names without special characters; a name containing a backtick is emitted doubled inside a backtick-quoted identifier; invalid ENGINE/DEFAULT/column-type expressions are rejected with IllegalArgumentException.

Risk and compatibility

  • Public API or stored data: N/A — only changes generated SQL for metadata/DDL paths.
  • Database or driver compatibility: quoted output matches ClickHouse backtick identifier and backslash-aware literal rules; behavior for ordinary names is unchanged.
  • Network, privacy, or security: closes second-order SQL-injection paths in generated metadata/DDL SQL.
  • Community / Local / Pro boundary: N/A.
  • Backward compatibility: names containing backticks that previously produced broken/injectable SQL now produce correctly escaped SQL; invalid ENGINE/DEFAULT/column-type expressions are now rejected instead of emitted verbatim. Plain quoted string defaults (e.g. 'abc') continue to work.

Reviewer map

  • Start here: ClickHouseSqlEscapes.java (new helper, incl. requireColumnTypeExpression) and ClickHouseSqlEscapesTest.java.
  • Then: the fixed format() helper and its callers; validateEngine/validateDefaultExpression; ClickHouseColumnTypeEnum.getByType/buildCreateColumnSqlSafely; exportFunctionDdl in ClickHouseDBManager.
  • Failure condition: a metadata/DDL SQL string still interpolates an unescaped name or literal.
  • Rollback or disable path: revert the commit on this branch.

Contributor declaration

  • I linked the Issue that defines this change.
  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: The fix, verification, and PR description were produced with AI coding assistance.

@HandSonic
HandSonic requested a review from openai0229 as a code owner July 26, 2026 09:53
@openai0229 openai0229 moved this to In Review in Chat2DB Community Jul 26, 2026
@HandSonic
HandSonic marked this pull request as draft July 26, 2026 09:56
@HandSonic

Copy link
Copy Markdown
Contributor Author

Converting to draft: final adversarial review pass is still in progress for this branch. Will mark ready for review once it completes. (process note: PR was opened prematurely by automation)

@openai0229 openai0229 moved this from In Review to In Progress in Chat2DB Community Jul 26, 2026
@HandSonic
HandSonic force-pushed the fix/sqli-clickhouse branch from 75e522e to 8ac520b Compare July 26, 2026 10:13
@HandSonic
HandSonic force-pushed the fix/sqli-clickhouse branch from 8ac520b to 9bcd7e9 Compare July 26, 2026 10:19
@HandSonic
HandSonic marked this pull request as ready for review July 26, 2026 10:28
@openai0229 openai0229 moved this from In Progress to In Review in Chat2DB Community Jul 26, 2026
…r per maintainer review (OtterMind#1914)

- new ClickHouseIdentifierProcessor (SPI ISQLIdentifierProcessor): quoteIdentifier
  with backtick doubling, escapeString with backslash + single-quote doubling
- ClickHouseMetaData overrides getSQLIdentifierProcessor(); metadata call sites use it
- builders/managers/enums use ClickHouseIdentifierProcessor.INSTANCE
- non-escapable validation moved to ClickHouseSqlGuards (column type expressions,
  engine whitelist, column default expressions)
- ClickHouseSqlEscapes removed; tests migrated (24 green)
…-quote for DDL paths (OtterMind#1914)

- quoteIdentifier(String) is conditional again: null/blank pass through,
  plain non-keyword identifiers stay unquoted, everything else is wrapped
  in backticks with embedded backticks doubled; versioned overload delegates
- new quoteIdentifierAlways(String) carries the old unconditional semantics
  for DDL-generation call sites migrated from ClickHouseSqlEscapes
- quoteIdentifierIgnoreCase stays the always-quote, case-preserving variant
- override removeIdentifierQuote/isQuoteIdentifier so backtick-quoted
  identifiers round-trip through completion/matching consumers
- add ClickHouse reserved keyword set used by the conditional check
- tests cover conditional vs always behavior incl. null passthrough (30 green)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Review

Development

Successfully merging this pull request may close these issues.

2 participants