Access modes, protocol tests, and function-prefix allowlist - #3
Merged
Merged
Conversation
Add a third access mode (--access-mode=readonly) that sits between `unrestricted` and `restricted`, solving the problem of pglast rejecting legitimate complex read-only queries. Background: The existing `restricted` mode uses pglast to parse SQL into an AST and validate every statement, function call, and node type against whitelists. This catches write operations at the application layer, but also rejects valid read-only queries that pglast cannot parse: nested CTEs, PERCENTILE_CONT with WITHIN GROUP, complex window functions, and queries using extensions or newer PostgreSQL syntax. Users running analytical workloads hit pglast rejections on safe queries and are forced to switch to `unrestricted` mode, losing all protection. How it works: ReadOnlySqlDriver wraps a base SqlDriver using the decorator pattern and overrides execute_query() to: 1. Always set force_readonly=True (ignoring the caller's value) 2. Prepend /* crystaldba */ comment for query identification in logs 3. Enforce a configurable timeout (default 30s) via asyncio.timeout() The underlying SqlDriver.execute_query() with force_readonly=True executes queries inside a read-only transaction: BEGIN TRANSACTION READ ONLY -- PostgreSQL enforces no writes /* crystaldba */ <query> -- user's query runs here ROLLBACK -- transaction closed PostgreSQL itself rejects any INSERT, UPDATE, DELETE, DROP, CREATE, or other write operation inside a READ ONLY transaction — this is database-engine-level enforcement, not application-layer parsing. Protection comparison across modes: Mode SQL validation DB enforcement Complex queries ────────────────────────────────────────────────────────────────── unrestricted none none all allowed readonly none READ ONLY tx all allowed restricted pglast AST check READ ONLY tx limited by pglast Security trade-off: Readonly mode does not validate SQL syntax. A multi-statement payload like "COMMIT; DROP TABLE users" is not caught at the application layer. However, PostgreSQL's read-only transaction prevents the write from executing. This is an intentional trade-off: pglast's false rejections of valid analytical queries cause more real-world pain than the theoretical multi-statement attack vector, which is mitigated by the database transaction boundary. Changes: - src/postgres_mcp/sql/readonly_sql.py: new ReadOnlySqlDriver class (64 lines, decorator over SqlDriver, timeout + force_readonly) - src/postgres_mcp/server.py: AccessMode.READONLY enum value, get_sql_driver() branch returning ReadOnlySqlDriver(timeout=30), --access-mode=readonly CLI argument - src/postgres_mcp/sql/__init__.py: export ReadOnlySqlDriver - smithery.yaml: add "readonly" to access mode config - README.md: update "Protected SQL Execution" to document three levels - tests/unit/sql/test_readonly_sql.py: 8 unit tests (force_readonly override, no SQL validation, comment prepending, timeout, parameter forwarding, exception propagation, None result handling) - tests/unit/sql/test_readonly_enforcement.py: 3 parameterized tests verifying force_readonly behavior across all three access modes - tests/unit/test_access_mode.py: driver selection + CLI parsing tests
Allow specific function prefixes (e.g. st_ for PostGIS) to bypass the hardcoded ALLOWED_FUNCTIONS allowlist in restricted mode. The flag is repeatable and case-insensitive.
…l registration execute_sql was registered inline in main() based on --access-mode, so its description and annotations were only observable by running the server. configure_access_mode(mode) now sets the global and (re)registers the tool; main() calls it. Behaviour is unchanged.
Adds tests/integration/test_mcp_protocol.py: an in-process FastMCP server wired to a ClientSession over the SDK memory transport, against the real PostgreSQL container fixture. Covers list_tools (all 9 tools, schemas, required args, access-mode-dependent execute_sql annotations), call_tool round trips for every tool, restricted-mode write blocking observed through the protocol, and how tool errors, missing arguments and unknown tools surface to a client.
…CTED default + lazy pool)
…sts (kept crystaldba#193 warning + READONLY)
…lidate_tree + prefixes global)
…taldba#207 (typing + line length) Modernize typing in readonly_sql.py (UP045) and test_mcp_protocol.py (UP035), and wrap the long --access-mode help string (E501). crystaldba#173 removed the UP0xx ruff ignores, so these stragglers from the merged PRs need the same treatment. ruff clean, pyright 0 errors, pytest 221 passed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01D6w353jusev2fgmWQY1mS1
cupskeee
force-pushed
the
merge/deferred-four
branch
from
September 6, 2026 15:03
af7f0c1 to
7214265
Compare
cupskeee
changed the title
cupskeee
changed the title
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
4 participants
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.
Reconciles three deferred upstream improvements against the security work already in this fork. Checks: ruff clean, pyright 0 errors,
pytest tests/unit= 221 passed / 27 skipped.Included:
configure_access_moderefactor.readonlyaccess mode for analytical queries the validator over-rejects.--allow-function-prefixflag to permit e.g. PostGISst_*in restricted mode.Kept the RESTRICTED default and its startup warning and the lazy/idle connection pool. The prefix-allow is threaded through the recursive FROM-clause validation.
Deliberately excluded the volatility-gate change: it would swap the default-deny function allowlist for allowing any non-volatile function via an async catalog probe — a weaker, label-trusting posture on the security path. The opt-in prefix flag and the readonly mode cover the legitimate need without that broadening.
🤖 Generated with Claude Code