fix(security): 2 improvements across 2 files by tomaioo · Pull Request #711 · ClickHouse/clickhouse-connect

tomaioo · 2026-04-17T00:07:06Z

Summary

fix(security): 2 improvements across 2 files

Problem

Severity: High | File: clickhouse_connect/cc_sqlalchemy/inspector.py:L15

The get_engine function builds SQL using f-string interpolation with schema and table_name directly inside quoted literals. If these values are attacker-controlled (or come from untrusted input through reflection APIs), a crafted value containing quotes can alter the query.

Solution

Use parameterized SQL instead of string interpolation, e.g. text("SELECT engine_full FROM system.tables WHERE database = :schema AND name = :name") with bound params. Also validate schema/table identifiers against a strict allowlist pattern.

Changes

clickhouse_connect/cc_sqlalchemy/inspector.py (modified)
clickhouse_connect/cc_sqlalchemy/ddl/custom.py (modified)

- Security: SQL injection risk in SQLAlchemy inspector metadata query - Security: DDL injection risk in CreateDatabase replicated engine arguments Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>

CLAassistant · 2026-04-17T00:07:13Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ joe-clickhouse
❌ tomaioo
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

Copilot

Pull request overview

This PR aims to harden SQL construction in the SQLAlchemy integration by reducing SQL-injection risk when querying ClickHouse system metadata and by escaping user-provided values embedded into DDL strings.

Changes:

Replace f-string SQL interpolation with bound parameters in get_engine when querying system.tables.
Add string escaping for Replicated database engine arguments in CreateDatabase.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`clickhouse_connect/cc_sqlalchemy/inspector.py`	Uses bind parameters for the `system.tables` lookup in `get_engine`.
`clickhouse_connect/cc_sqlalchemy/ddl/custom.py`	Introduces a helper to escape string arguments embedded in Replicated database DDL construction.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-19T20:01:19Z

 from clickhouse_connect.driver.binding import quote_identifier


+def _escape_sql_string(value):
+    return str(value).replace("\\", "\\\\").replace("'", "\\'")


_escape_sql_string duplicates escaping logic that already exists in clickhouse_connect.driver.binding.escape_str/format_str and currently escapes fewer characters (e.g., it doesn't handle tabs/newlines/backticks that the driver escape logic treats as special). To keep escaping consistent across the codebase and avoid edge-case parsing issues, consider reusing the driver-provided escape/format helpers instead of maintaining a separate implementation here.

Suggested change

from clickhouse_connect.driver.binding import quote_identifier

def _escape_sql_string(value):

return str(value).replace("\\", "\\\\").replace("'", "\\'")

from clickhouse_connect.driver.binding import escape_str, quote_identifier

def _escape_sql_string(value):

return escape_str(str(value))

Copilot · 2026-04-19T20:01:19Z

+    result_set = connection.execute(text("SELECT engine_full FROM system.tables WHERE database = :schema and name = :table_name"),
+                                    {'schema': schema, 'table_name': table_name})


The PR description mentions adding strict allowlist validation for schema/table identifiers in addition to parameterizing the query, but this change only adds bind parameters. Either implement the identifier validation (if still required) or update the PR description to match the actual mitigation being shipped.

Copilot · 2026-04-19T20:01:19Z

+                stmt += (
+                    f" ('{_escape_sql_string(zoo_path)}', "
+                    f"'{_escape_sql_string(shard_name)}', "
+                    f"'{_escape_sql_string(replica_name)}'"
+                )


This change alters how Replicated database engine arguments are rendered/escaped, but current test coverage only exercises CreateDatabase for non-Replicated engines. Add a focused test that validates the generated SQL for engine='Replicated' (including escaping of quotes/backslashes in the args) to prevent regressions and ensure the security fix stays intact.

Copilot · 2026-04-19T20:01:20Z

+                stmt += (
+                    f" ('{_escape_sql_string(zoo_path)}', "
+                    f"'{_escape_sql_string(shard_name)}', "
+                    f"'{_escape_sql_string(replica_name)}'"


The Replicated database DDL string being built here still appears to be missing the closing ")" after the replica_name argument. As written, the final statement would end with ..., '{replica}' and leave the opening parenthesis from Replicated ( unbalanced, which should produce invalid SQL. Add the closing parenthesis (and any expected spacing) when appending these arguments.

Suggested change

f"'{_escape_sql_string(replica_name)}'"

f"'{_escape_sql_string(replica_name)}')"

The missing ) here exists on main, and is indeed a needed fix that we should do here since we're touching this line anyway. It also means no test exercises this path so we should get a test around this as well. Again, using the existing format_str

stmt += ( f" ({format_str(zoo_path)}, " f"{format_str(shard_name)}, " f"{format_str(replica_name)})" )

…y-inspect

joe-clickhouse

Hi @tomaioo thanks for the fixes. Left a few comments in the code for your review.

joe-clickhouse · 2026-04-21T21:04:41Z

+                stmt += (
+                    f" ('{_escape_sql_string(zoo_path)}', "
+                    f"'{_escape_sql_string(shard_name)}', "
+                    f"'{_escape_sql_string(replica_name)}'"


The missing ) here exists on main, and is indeed a needed fix that we should do here since we're touching this line anyway. It also means no test exercises this path so we should get a test around this as well. Again, using the existing format_str

stmt += ( f" ({format_str(zoo_path)}, " f"{format_str(shard_name)}, " f"{format_str(replica_name)})" )

joe-clickhouse · 2026-04-21T21:08:10Z

+def _escape_sql_string(value):
+    return str(value).replace("\\", "\\\\").replace("'", "\\'")


The codebase already has format_str in binding.py. So let's delete _escape_sql_string entirely and just reuse those to match existing convention.

…nd is indeed a needed Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>

joe-clickhouse · 2026-05-05T21:54:55Z

To get this wrapped up, I'm closing this PR but the work is reimplemented in #728 thanks @tomaioo!

tomaioo added 2 commits April 16, 2026 17:07

fix(security): 2 improvements across 2 files

201f38d

- Security: SQL injection risk in SQLAlchemy inspector metadata query - Security: DDL injection risk in CreateDatabase replicated engine arguments Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>

fix(security): 2 improvements across 2 files

f14cc69

- Security: SQL injection risk in SQLAlchemy inspector metadata query - Security: DDL injection risk in CreateDatabase replicated engine arguments Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>

tomaioo requested review from joe-clickhouse and peter-leonov-ch as code owners April 17, 2026 00:07

mshustov requested a review from Copilot April 19, 2026 19:57

Copilot started reviewing on behalf of mshustov April 19, 2026 19:58 View session

Copilot AI reviewed Apr 19, 2026

View reviewed changes

Merge branch 'main' into fix/security/sql-injection-risk-in-sqlalchem…

c66f13f

…y-inspect

joe-clickhouse reviewed Apr 21, 2026

View reviewed changes

fix: address review feedback — The missing ) here exists on main, a…

5aab6d4

…nd is indeed a needed Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>

joe-clickhouse mentioned this pull request May 5, 2026

use bound params and format_str in DDL/inspector #728

Merged

3 tasks

joe-clickhouse closed this May 5, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(security): 2 improvements across 2 files#711

fix(security): 2 improvements across 2 files#711
tomaioo wants to merge 4 commits intoClickHouse:mainfrom
tomaioo:fix/security/sql-injection-risk-in-sqlalchemy-inspect

tomaioo commented Apr 17, 2026

Uh oh!

CLAassistant commented Apr 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

Copilot AI Apr 19, 2026

Uh oh!

joe-clickhouse Apr 21, 2026

Uh oh!

joe-clickhouse left a comment

Uh oh!

joe-clickhouse Apr 21, 2026

Uh oh!

joe-clickhouse Apr 21, 2026

Uh oh!

joe-clickhouse commented May 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		result_set = connection.execute(text("SELECT engine_full FROM system.tables WHERE database = :schema and name = :table_name"),
		{'schema': schema, 'table_name': table_name})

	f"'{_escape_sql_string(replica_name)}'"
	f"'{_escape_sql_string(replica_name)}')"

		def _escape_sql_string(value):
		return str(value).replace("\\", "\\\\").replace("'", "\\'")

Conversation

tomaioo commented Apr 17, 2026

Summary

Problem

Solution

Changes

Uh oh!

CLAassistant commented Apr 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 19, 2026

Choose a reason for hiding this comment

Uh oh!

joe-clickhouse Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

joe-clickhouse left a comment

Choose a reason for hiding this comment

Uh oh!

joe-clickhouse Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

joe-clickhouse Apr 21, 2026

Choose a reason for hiding this comment

Uh oh!

joe-clickhouse commented May 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

CLAassistant commented Apr 17, 2026 •

edited

Loading