fix(security): block SQL functions that read or write outside the session

[notSumit25]

Both read-only layers bypassed by one statement

SELECT dblink_exec('dbname=app user=postgres host=127.0.0.1', 'DELETE FROM orders')

The guard failed because McpSqlGuardService classifies by statement verb. This begins SELECT (on the allowlist) and none of the 15 forbidden verbs appears anywhere. dblink_exec is a function call — invisible to a verb-based parser.

connection.setReadOnly(true) failed for a subtler reason worth understanding: dblink opens a new outbound connection, whose transaction is not read-only. The flag constrains the session it's set on — it cannot constrain one the query dials out and creates.

CLAUDE.md calls setReadOnly(true) the backstop that "keeps the next parser gap from becoming data loss." That holds for ordinary writes (SELECT … INTO, nextval, lo_import are all correctly refused). It does not hold for a function that leaves the session.

Reproduced against a real PostgreSQL, not inferred

Isolated zz_sec schema, created and dropped for the test:

rows before                                      3
BEGIN TRANSACTION READ ONLY
SELECT dblink_exec('dbname=dba_agent …','DELETE FROM zz_sec.victim')
 dblink_exec
-------------
 DELETE 3
COMMIT
rows after                                       0

A DELETE ran to completion inside an explicitly read-only transaction. Same story for filesystem reads:

BEGIN TRANSACTION READ ONLY
SELECT length(pg_read_file('/etc/hostname'))   ->  13

And the shipped guard returned ALLOWED for every one: dblink_exec, dblink, pg_read_file, pg_ls_dir, pg_read_binary_file, lo_import, LOAD_FILE.

Until #111 bound public dashboard queries to published shapes, this was reachable unauthenticated through the share endpoint — same executor.

The fix

private static final List<String> DANGEROUS_SQL_FUNCTIONS = List.of(
    "dblink", "dblink_exec", "dblink_connect", "dblink_open", "dblink_send_query",
    "pg_read_file", "pg_read_binary_file", "pg_ls_dir", "pg_stat_file",
    "lo_import", "lo_export",
    "load_file");

A denylist is usually the wrong shape. It's right here because the allowlist governs verbs, and there is no allowlist of functions permitted inside a SELECT — the legitimate set is open-ended, the escaping set is small and nameable.

Matched as a call, not a name: (?<![\w$.])(name)\s*\(. Matching the bare name would reject a dblink_audit table or a load_file_name column — the exact mistake CLAUDE.md records for the old \bCOMMENT\b rule that rejected SELECT * FROM comment. The boundary also prevents a different function like my_dblink( from matching. Inspection runs on text with comments and string literals stripped, so /*x*/dblink_exec( can't hide and a name inside a quoted literal can't falsely trigger.

Both guards, or neither

McpSqlGuardService.java and mcp/deepsql-phase1-lib.js are a functional mirror. A statement one blocks and the other allows is the bypass — so the change landed in both, and parity is verified directly: 20 payloads (14 attacks, 6 legitimate including the identifier false-positives), 0 mismatches.

Verification

Step	Result
Tests before fix (RED)	5 failures, all "expected false but was true"
Tests after fix (GREEN)	19 pass
Denylist stubbed to return null (mutation)	5 fail again — tests guard the fix
Java/JS parity, 20 payloads	0 mismatches
Live attack replayed post-fix	blocked; table still 3 rows
Backend suites	84 tests, 0 failures
MCP suite	272 tests, 0 failures

zz_sec and the dblink extension were dropped — database back to its prior state.

Residual work (deliberately not here)

• ExplainPlanService opens its own connection and never calls setReadOnly(true) — grep setReadOnly over src/main/java returns exactly one hit, in QueryExecutorService. The function denylist now covers that path, but the database-level backstop is still absent there.
• COPY … FROM/TO PROGRAM is blocked by the COPY verb being forbidden, not by this denylist — sufficient today, but worth knowing if the verb list is ever narrowed.
• Revoking EXECUTE on these functions from the connection role, and not provisioning superuser connection users, remains the stronger control. This reduces blast radius; it does not replace database permissions.

Write-up: docs/security/2026-09-11-sql-guard-dangerous-functions.md

🤖 Generated with Claude Code