fix(admin): Allow ClickHouse replace* functions in read-only query validation#7876
fix(admin): Allow ClickHouse replace* functions in read-only query validation#7876
Conversation
…lidation The keyword blocklist used a substring match for "replace", causing ClickHouse functions like replaceAll() and replaceRegexpAll() to be incorrectly rejected. Use a word-boundary regex match for "replace" so only the SQL DML keyword (e.g. REPLACE TABLE) is blocked. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adjusts the snuba-admin read-only query validator to avoid falsely blocking ClickHouse string functions whose names start with replace, while still blocking REPLACE DML statements.
Changes:
- Import
reand switchreplacekeyword detection from substring matching to a word-boundary regex (\\breplace\\b). - Keep existing substring checks for other disallowed keywords unchanged.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for kw in disallowed_keywords: | ||
| if kw in lowered: | ||
| if kw == "replace": | ||
| if re.search(r"\breplace\b", lowered): |
There was a problem hiding this comment.
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.
| 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): |
|
@xurui-c probs doesn't hurt to add a test to this |
The snuba-admin query validator blocked any query containing the substring
replace, which caused ClickHouse functions likereplaceAll(),replaceRegexpAll(), etc. to be rejected with a validation error.The intent of the
replaceentry in the blocklist is to prevent SQL DML statements likeREPLACE TABLEorREPLACE INTO, not to block legitimate ClickHouse string functions. Switching to a word-boundary regex match (\breplace\b) for this keyword preserves the DML protection while allowing function names that begin withreplace.