Add optional write access mode with DROP protection #93
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds optional write access capabilities to the ClickHouse MCP server with built-in safety mechanisms. By default, the server maintains read-only behavior, but can now be explicitly configured to allow write operations with DROP protection.
Key changes:
- Renamed
run_select_querytorun_queryto reflect broader SQL support - Added
CLICKHOUSE_ALLOW_WRITE_ACCESSflag to enable DDL/DML operations - Added
CLICKHOUSE_ALLOW_DROPflag for additional protection against destructive operations
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| mcp_clickhouse/mcp_env.py | Added two new configuration properties (allow_write_access and allow_drop) to control write permissions |
| mcp_clickhouse/mcp_server.py | Implemented DROP validation, refactored readonly setting logic, renamed query function, and added dynamic tool description |
| mcp_clickhouse/init.py | Updated exports to reflect function rename |
| tests/test_tool.py | Renamed function references and added comprehensive test coverage for write mode, DROP protection, and read-only mode |
| tests/test_mcp_server.py | Updated function calls to use new run_query name |
| README.md | Updated documentation to reflect new function name and write access features |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return | ||
|
|
||
| # Simple pattern matching for DROP operations | ||
| drop_pattern = r'\bDROP\s+(TABLE|DATABASE|VIEW|DICTIONARY)\b' |
There was a problem hiding this comment.
The regex pattern for DROP detection may not catch all variations. Consider ClickHouse-specific DROP operations like DROP TEMPORARY TABLE, DROP DETACHED TABLE, or DROP TABLE IF EXISTS. The pattern should include the optional TEMPORARY, DETACHED, and IF EXISTS keywords to ensure comprehensive DROP detection.
| drop_pattern = r'\bDROP\s+(TABLE|DATABASE|VIEW|DICTIONARY)\b' | |
| # Updated pattern to match DROP [TEMPORARY] [DETACHED] [IF EXISTS] (TABLE|DATABASE|VIEW|DICTIONARY) | |
| drop_pattern = ( | |
| r'\bDROP\s+' | |
| r'(TEMPORARY\s+)?' | |
| r'(DETACHED\s+)?' | |
| r'(IF\s+EXISTS\s+)?' | |
| r'(TABLE|DATABASE|VIEW|DICTIONARY)\b' | |
| ) |
| drop_pattern = r'\bDROP\s+(TABLE|DATABASE|VIEW|DICTIONARY)\b' | ||
| if re.search(drop_pattern, query, re.IGNORECASE): |
There was a problem hiding this comment.
The DROP detection can be bypassed using comments or string literals. For example, SELECT '/* DROP TABLE foo */' FROM bar or SELECT 'DROP TABLE' FROM foo would trigger false positives, while DR/**/OP TABLE foo could bypass detection. Consider using a proper SQL parser or at minimum stripping comments and string literals before pattern matching.
| def setUpClass(cls): | ||
| """Set up the environment before tests.""" | ||
| cls.env_patcher = patch.dict(os.environ, {"CLICKHOUSE_ALLOW_WRITE_ACCESS": "false"}) | ||
| cls.env_patcher.start() |
There was a problem hiding this comment.
The environment patcher is started in setUpClass but there's a risk it won't be stopped if setUpClass raises an exception before completing. Consider using addClassCleanup() instead of relying solely on tearDownClass() to ensure cleanup happens even on setup failures.
| cls.env_patcher.start() | |
| cls.env_patcher.start() | |
| cls.addClassCleanup(cls.env_patcher.stop) |
Summary
This PR enables LLMs to perform write operations on ClickHouse databases with built-in safety controls to prevent accidental data loss.
Write Access Mode
CLICKHOUSE_ALLOW_WRITE_ACCESSflag (default: false) enables DDL and DML operations (INSERT, UPDATE, CREATE, ALTER, DROP)readonly=0instead of the defaultreadonly=1Two-Tier
DROPProtectionCLICKHOUSE_ALLOW_DROP(default: false) provides additional safety for destructive operationsDROPoperationsINSERT,CREATE, etc.) only requireCLICKHOUSE_ALLOW_WRITE_ACCESS=trueUsage
Enable write access:
Enable write access including
DROP:Closes #24