security: enforce application-layer validation for DDL/DML in read-only mode#151
Open
shuvajyotikar13 wants to merge 2 commits intoClickHouse:mainfrom
Open
security: enforce application-layer validation for DDL/DML in read-only mode#151shuvajyotikar13 wants to merge 2 commits intoClickHouse:mainfrom
shuvajyotikar13 wants to merge 2 commits intoClickHouse:mainfrom
Conversation
…ly mode Refactor _validate_query_for_destructive_ops to enhance security checks for read-only access and clarify documentation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The Problem:
Authorization Bypass in Default Configuration
The current implementation of _validate_query_for_destructive_ops contains a logic flaw that creates a "fail-open" security state when the server is deployed in its default read-only mode (allow_write_access = False).
Specifically, the function returns early without inspecting the SQL string if write access is disabled, delegating all security responsibility to the clickhouse-connect driver's readonly=1 flag. However, ClickHouse evaluates permissions server-side; if the authenticated database user possesses elevated privileges, the server silently overrides the driver's read-only request. This allows an autonomous agent or a malicious actor (via prompt injection) to execute destructive DDL commands like DROP TABLE despite the server being configured for safety.
The Fix: Application-Layer Regex Firewall
This PR introduces a strict validation gate that executes regardless of the allow_write_access setting.
Comment Stripping: The patch strips SQL comments (-- and /* */) before evaluation to prevent attackers from "hiding" destructive commands from the regex.
Prefix Allow-listing: When in read-only mode, the server now enforces a strict allow-list of safe SQL prefixes: SELECT, WITH, SHOW, DESCRIBE, EXPLAIN, and EXISTS.
Protocol Compliance: Ensures that any violation results in a standard ToolError, which is handled gracefully by the FastMCP framework.
Verification & Testing
I have added a new test suite, tests/test_clickhouse_security.py, which mocks the server configuration to prove the bypass exists in the unpatched version.
Test 1 (Read-Only Logic Bypass): Confirms that a DROP TABLE command is ignored by the current validation logic when allow_write_access is False. (Fails on main, Passes with this PR).
Test 2 (Intent Validation): Confirms that the existing regex logic is still respected when writes are enabled but drops are restricted.
Enterprise Impact
In an enterprise agentic workflow, this vulnerability represents a critical Privilege Escalation vector. By enforcing validation at the application layer, we ensure that even if a database user is over-privileged, the MCP server acts as a definitive guardrail against accidental or malicious data destruction.