Skip to content

fix(admin): Allow ClickHouse replace* functions in read-only query validation #7876

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

Merged
xurui-c merged 2 commits into from
Apr 16, 2026
+21 −1
Merged

fix(admin): Allow ClickHouse replace* functions in read-only query validation #7876

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2a97d73
fix(admin): Allow ClickHouse replace* functions in read-only query va…
xurui-c Apr 15, 2026
c3d4716
test(admin): Add test coverage for replace keyword validation
xurui-c Apr 16, 2026
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
6 changes: 5 additions & 1 deletion snuba/admin/clickhouse/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from __future__ import annotations

import re
from typing import MutableMapping

from sql_metadata import Parser, QueryType # type: ignore
Expand Down Expand Up @@ -241,7 +242,10 @@ def validate_ro_query(sql_query: str, allowed_tables: set[str] | None = None) ->
]

for kw in disallowed_keywords:
if kw in lowered:
if kw == "replace":
if re.search(r"\breplace\b", lowered):
Copy link

Copilot AI Apr 15, 2026

Choose a reason for hiding this comment

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

The updated replace handling changes validation semantics, but there’s no corresponding test coverage to ensure replaceAll()/replaceRegexpAll() are allowed while DML like REPLACE INTO/TABLE is still rejected. Please add/adjust tests in the existing query validation test suite to cover both cases, so future keyword changes don’t regress this behavior.

Suggested change
if re.search(r"\breplace\b", lowered):
# Reject DML forms such as REPLACE INTO / REPLACE TABLE while
# allowing read-only function calls like replaceAll() and
# replaceRegexpAll().
if re.search(r"\breplace\s+(into|table)\b", lowered):

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

@xurui-c xurui-c Apr 16, 2026

Choose a reason for hiding this comment

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

Added tests in test_query_validation.py covering both cases: replaceAll()/replaceRegexpAll() are allowed, and REPLACE INTO/REPLACE TABLE are still rejected.

— Claude Code

raise InvalidCustomQuery(f"{kw} is not allowed in the query")
elif kw in lowered:
raise InvalidCustomQuery(f"{kw} is not allowed in the query")

parsed = Parser(lowered)
Expand Down
16 changes: 16 additions & 0 deletions tests/admin/clickhouse/test_query_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,19 @@ def test_allowed_tables_with_left_array_join() -> None:
"SELECT * FROM my_table, other_table LEFT ARRAY JOIN tags.key AS tag_key, tags.raw_value AS tag_value",
allowed_tables={"my_table"},
)


def test_replace_functions_allowed() -> None:
# ClickHouse replace* functions should be allowed in read-only queries
validate_ro_query("SELECT replaceAll(message, 'foo', 'bar') FROM my_table")
validate_ro_query("SELECT replaceRegexpAll(message, '[0-9]+', 'N') FROM my_table")
validate_ro_query("SELECT replaceOne(message, 'x', 'y') FROM my_table")
validate_ro_query("SELECT replaceRegexpOne(message, 'x', 'y') FROM my_table")


def test_replace_dml_rejected() -> None:
# DML forms of REPLACE must still be blocked
with pytest.raises(InvalidCustomQuery):
validate_ro_query("REPLACE INTO my_table VALUES (1, 2, 3)")
with pytest.raises(InvalidCustomQuery):
validate_ro_query("REPLACE TABLE my_table SELECT * FROM other_table")
Loading