Skip to content

Fix SQL injection vulnerability in cluster_name_override parameter #7830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fix-it-felix-sentry wants to merge 1 commit into
base: master
Choose a base branch
from
+40 −0
Open

Fix SQL injection vulnerability in cluster_name_override parameter #7830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions snuba/admin/clickhouse/copy_tables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import re
from dataclasses import dataclass
from typing import MutableMapping, Optional, Sequence, Tuple, TypedDict

Expand Down Expand Up @@ -140,6 +141,12 @@ def copy_tables(
if skip_on_cluster:
cluster_name = None
elif cluster_name_override:
# Validate cluster_name_override to prevent SQL injection
# Only allow alphanumeric characters, underscores, and hyphens
if not re.match(r"^[a-zA-Z0-9_-]+$", cluster_name_override):
raise ValueError(
"Invalid cluster name: only alphanumeric characters, underscores, and hyphens are allowed"
)
Copy link
Copy Markdown

@cursor cursor bot Mar 23, 2026

Choose a reason for hiding this comment

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

ValueError caught with misleading "Target host is invalid" message

Medium Severity

The new ValueError raised for invalid cluster names gets caught by the existing except ValueError handler in views.py line 489, which formats the response as "Target host is invalid: {err.args[0]}". Users attempting an invalid cluster name will see the confusing message "Target host is invalid: Invalid cluster name: only alphanumeric characters…" — misattributing the error to the target host instead of the cluster name.

Fix in Cursor Fix in Web

cluster_name = cluster_name_override
elif not cluster.is_single_node():
cluster_name = storage.get_cluster().get_clickhouse_cluster_name()
Expand Down
33 changes: 33 additions & 0 deletions tests/admin/clickhouse/test_copy_tables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,39 @@ def test_copy_tables_cluster_name_override() -> None:
assert result["cluster_name"] == override


@pytest.mark.redis_db
@pytest.mark.custom_clickhouse_db
def test_copy_tables_cluster_name_override_sql_injection_prevention() -> None:
"""
Test that cluster_name_override rejects invalid characters to prevent SQL injection.
Only alphanumeric characters, underscores, and hyphens should be allowed.
"""
run_migrations()
host = os.environ.get("CLICKHOUSE_HOST", "127.0.0.1")

# Test various SQL injection attempts
invalid_cluster_names = [
"'; DROP TABLE users; --",
"cluster'; DROP TABLE users; --",
"cluster' OR '1'='1",
'cluster"; DROP TABLE users; --',
"cluster name with spaces",
"cluster;name",
"cluster'name",
"cluster(name)",
"cluster.name",
]

for invalid_name in invalid_cluster_names:
with pytest.raises(ValueError, match="Invalid cluster name"):
copy_tables(
source_host=host,
storage_name="outcomes_raw",
dry_run=True,
cluster_name_override=invalid_name,
)


@pytest.mark.redis_db
@pytest.mark.custom_clickhouse_db
def test_verify_tables_on_replicas() -> None:
Expand Down
Loading