Merge branch 'main' into feat/preflight-migration-review

Author: venkateshsakamuri-lab

CLAUDE.md

@@ -804,6 +804,24 @@ it against a real database — not a theoretical hardening pass.
   the parse tree (`detectSelectWrite`) **and** runs a text backstop
   (`detectHiddenWrite`) so an unparseable variant fails closed instead of falling
   through to the keyword path.
+- **A verb-based guard cannot see a dangerous *function*, and `setReadOnly(true)` cannot stop
+  one that leaves the session.** `SELECT dblink_exec('dbname=app …','DELETE FROM orders')`
+  defeated **both** layers at once: it begins `SELECT` so the allowlist passes it and no
+  forbidden verb appears, and `dblink` opens a **new outbound connection** whose transaction is
+  not read-only — the flag constrains the session it is set on, never one the query dials out
+  and creates. Reproduced against a real PostgreSQL: inside `BEGIN TRANSACTION READ ONLY`, the
+  statement reported `DELETE 3` and the table went from 3 rows to 0; `pg_read_file` likewise
+  read the server's filesystem under read-only. Until public dashboard queries were bound to
+  published shapes this was reachable **unauthenticated** through the share endpoint, which
+  runs the same executor. `DANGEROUS_SQL_FUNCTIONS` now denies dblink*, `pg_read_file`,
+  `pg_read_binary_file`, `pg_ls_dir`, `pg_stat_file`, `lo_import`, `lo_export` and MySQL
+  `LOAD_FILE`. A denylist is the right shape *here* only because the allowlist governs verbs
+  and there is no allowlist of functions that may sit inside a `SELECT`. **Match the call, not
+  the name** — `(?<![\w$.])(name)\s*\(` — or you reject a `dblink_audit` table and a
+  `load_file_name` column, the same mistake the old `\bCOMMENT\b` rule made with
+  `SELECT * FROM comment`. Mirrored in `mcp/deepsql-phase1-lib.js`; a statement one guard
+  blocks and the other allows *is* the bypass, so parity is asserted over 20 payloads. See
+  `docs/security/2026-09-11-sql-guard-dangerous-functions.md`.
 - **Read-only contexts open read-only JDBC sessions.** `QueryExecutorService` calls
   `connection.setReadOnly(true)` whenever `mutationMode() == READ_ONLY_ONLY`, so
   PostgreSQL refuses the write itself even if classification is wrong. Classification

backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java

@@ -45,6 +45,51 @@ public class McpSqlGuardService {
         "COMMENT"
     );
 
+    /**
+     * Functions that read or write outside the current read-only session.
+     *
+     * <p>Every check above classifies by statement <em>verb</em>, so a {@code SELECT} that calls
+     * one of these passes cleanly: the allowlist sees SELECT and no forbidden verb appears.
+     * {@code connection.setReadOnly(true)} does not stop them either — {@code dblink} opens a
+     * <em>new outbound connection</em> whose transaction is not read-only, so the flag
+     * constrains the session it is set on but never one the query dials out and creates.
+     *
+     * <p>Verified against a real PostgreSQL, not inferred: inside an explicitly
+     * {@code BEGIN TRANSACTION READ ONLY}, {@code SELECT dblink_exec(..., 'DELETE FROM t')}
+     * reported {@code DELETE 3} and the table went from three rows to zero;
+     * {@code pg_read_file('/etc/hostname')} returned its contents from the database server's
+     * filesystem. Both of the product's layers failed at once.
+     *
+     * <p>A denylist is the wrong shape in general, but it is the right shape here: the guard's
+     * allowlist governs <em>verbs</em>, and there is no allowlist of functions to sit inside
+     * a SELECT. Names are matched as calls (see {@link #DANGEROUS_FUNCTION_CALL}) so an
+     * ordinary identifier of the same name still works.
+     */
+    private static final List<String> DANGEROUS_SQL_FUNCTIONS = List.of(
+        // outbound connections — escape the read-only session entirely
+        "dblink", "dblink_exec", "dblink_connect", "dblink_open", "dblink_send_query",
+        // server-side file access
+        "pg_read_file", "pg_read_binary_file", "pg_ls_dir", "pg_stat_file",
+        "lo_import", "lo_export",
+        // MySQL equivalents
+        "load_file"
+    );
+
+    private static final String DANGEROUS_FUNCTION_ALTERNATION =
+        String.join("|", DANGEROUS_SQL_FUNCTIONS);
+
+    /**
+     * A dangerous function being <em>called</em>: the name, optional whitespace, then an open
+     * paren. Matching the bare name would reject ordinary identifiers — plenty of schemas have
+     * a {@code dblink_audit} table or a {@code load_file_name} column, the same mistake
+     * CLAUDE.md records for the old {@code \bCOMMENT\b} rule that rejected
+     * {@code SELECT * FROM comment}. A leading word-boundary check keeps {@code my_dblink(} —
+     * a different function — from matching.
+     */
+    private static final Pattern DANGEROUS_FUNCTION_CALL = Pattern.compile(
+        "(?<![\\w$.])(" + DANGEROUS_FUNCTION_ALTERNATION + ")\\s*\\(",
+        Pattern.CASE_INSENSITIVE);
+
     private static final Set<String> FORBIDDEN_SQL_KEYWORD_SET = Set.copyOf(FORBIDDEN_SQL_KEYWORDS);
 
     private static final String FORBIDDEN_ALTERNATION = String.join("|", FORBIDDEN_SQL_KEYWORDS);
@@ -102,9 +147,29 @@ public ValidationOutcome validateReadOnlySql(String sql, boolean allowExplain) {
             );
         }
 
+        String dangerousFunction = containsDangerousFunction(statement);
+        if (dangerousFunction != null) {
+            return ValidationOutcome.invalid(
+                "Blocked SQL function that reads or writes outside this session: "
+                    + dangerousFunction + "."
+            );
+        }
+
         return ValidationOutcome.valid(stripTrailingSemicolons(sql), keyword);
     }
 
+    /**
+     * The first dangerous function call in the statement, or null.
+     *
+     * <p>Inspected with comments and string literals stripped, so neither
+     * {@code /*x*} + {@code /dblink_exec(} nor a name mentioned inside a quoted literal can
+     * hide or falsely trigger a match.
+     */
+    String containsDangerousFunction(String statement) {
+        var matcher = DANGEROUS_FUNCTION_CALL.matcher(normalizeSqlForInspection(statement));
+        return matcher.find() ? matcher.group(1).toLowerCase(Locale.ROOT) : null;
+    }
+
     String normalizeSqlForInspection(String sql) {
         return compactWhitespace(stripSqlStringLiterals(stripSqlComments(sql)));
     }

backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java

@@ -122,4 +122,95 @@ void rejectsExplainOfDeleteButAllowsExplainOfCommentTable() {
         var commentPlan = service.validateReadOnlySql("EXPLAIN SELECT * FROM comment", true);
         assertTrue(commentPlan.ok());
     }
+
+    // ── dangerous functions ───────────────────────────────────────────────────
+    //
+    // The guard classifies by statement *verb*, so a SELECT that calls a dangerous
+    // function passes every check: the allowlist sees SELECT, and no forbidden verb
+    // appears anywhere. Verified against a real PostgreSQL 17 — inside an explicitly
+    // READ ONLY transaction, `SELECT dblink_exec(..., 'DELETE FROM t')` reported
+    // `DELETE 3` and the table went from 3 rows to 0.
+    //
+    // connection.setReadOnly(true) cannot stop it either: dblink opens a *new outbound
+    // connection* whose transaction is not read-only. The read-only flag constrains the
+    // session it is set on, never one the query dials out and creates. So both of the
+    // product's layers fail at once, and until the public dashboard path was bound to
+    // published query shapes this was reachable anonymously.
+
+    @Test
+    void rejectsDblinkExec() {
+        var result = service.validateReadOnlySql(
+            "SELECT dblink_exec('dbname=app user=postgres host=127.0.0.1','DELETE FROM orders')", true);
+
+        assertFalse(result.ok());
+        assertTrue(result.reason().toLowerCase().contains("dblink"),
+            "reason should name the function it refused, was: " + result.reason());
+    }
+
+    @Test
+    void rejectsEveryDblinkEntryPoint() {
+        for (String sql : new String[] {
+            "SELECT * FROM dblink('dbname=app','SELECT 1') AS t(a int)",
+            "SELECT dblink_connect('dbname=app')",
+            "SELECT dblink_send_query('conn','DELETE FROM orders')",
+            "SELECT dblink_open('conn','cur','SELECT 1')"
+        }) {
+            assertFalse(service.validateReadOnlySql(sql, true).ok(), "should refuse: " + sql);
+        }
+    }
+
+    @Test
+    void rejectsServerSideFileReads() {
+        for (String sql : new String[] {
+            "SELECT pg_read_file('/etc/passwd')",
+            "SELECT pg_read_binary_file('/etc/passwd')",
+            "SELECT pg_ls_dir('/var/lib/postgresql/data')",
+            "SELECT pg_stat_file('/etc/passwd')",
+            "SELECT lo_import('/etc/passwd')",
+            "SELECT lo_export(1,'/tmp/out')"
+        }) {
+            assertFalse(service.validateReadOnlySql(sql, true).ok(), "should refuse: " + sql);
+        }
+    }
+
+    @Test
+    void rejectsMySqlFileReads() {
+        assertFalse(service.validateReadOnlySql("SELECT LOAD_FILE('/etc/passwd')", true).ok());
+    }
+
+    @Test
+    void rejectsDangerousFunctionRegardlessOfSpacingOrCase() {
+        for (String sql : new String[] {
+            "SELECT DBLINK_EXEC('x','DELETE FROM t')",
+            "SELECT dblink_exec ('x','DELETE FROM t')",
+            "SELECT pg_read_file\n('/etc/passwd')",
+            "WITH x AS (SELECT pg_read_file('/etc/passwd') AS f) SELECT * FROM x"
+        }) {
+            assertFalse(service.validateReadOnlySql(sql, true).ok(), "should refuse: " + sql);
+        }
+    }
+
+    /**
+     * The guard must match a function *call*, not a name that merely appears. Column and
+     * table names are ordinary identifiers and plenty of schemas contain them — the same
+     * mistake CLAUDE.md records for the old \\bCOMMENT\\b rule, which rejected
+     * `SELECT * FROM comment`.
+     */
+    @Test
+    void stillAllowsIdentifiersThatMerelyResembleADangerousFunction() {
+        for (String sql : new String[] {
+            "SELECT * FROM public.dblink_audit",
+            "SELECT t.pg_read_file_count FROM public.stats t",
+            "SELECT load_file_name FROM public.imports",
+            "SELECT * FROM comment"
+        }) {
+            assertTrue(service.validateReadOnlySql(sql, true).ok(), "should allow: " + sql);
+        }
+    }
+
+    @Test
+    void stillAllowsOrdinaryAnalyticQueries() {
+        assertTrue(service.validateReadOnlySql(
+            "SELECT count(*), sum(o.total) FROM public.orders o WHERE o.created_at >= '2026-01-01'", true).ok());
+    }
 }

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

@@ -0,0 +1,125 @@
+# A SELECT could write through both read-only layers
+
+*Found 2026-09-10 in a repository-wide security audit; reproduced against a live PostgreSQL
+2026-09-11. Severity: critical.*
+
+## What was wrong
+
+Both of the product's read-only defences were bypassed by one statement:
+
+```sql
+SELECT dblink_exec('dbname=app user=postgres host=127.0.0.1', 'DELETE FROM orders')
+```
+
+**The guard failed** because `McpSqlGuardService` classifies by statement *verb*. The statement
+begins `SELECT`, which is on `ALLOWED_READ_ONLY_KEYWORDS`, and none of the 15
+`FORBIDDEN_SQL_KEYWORDS` appears anywhere in it. `dblink_exec` is a *function call* — invisible
+to a verb-based parser.
+
+**`connection.setReadOnly(true)` failed** for a subtler reason, and this is the part worth
+understanding: `dblink` opens a **new outbound connection** to the database. That second
+connection runs its own transaction, which is not read-only. The read-only flag constrains the
+session it is set on; it cannot constrain a session the query itself dials out and creates.
+
+CLAUDE.md describes `setReadOnly(true)` as the backstop that "keeps the *next* parser gap from
+becoming data loss". That holds for ordinary writes — `SELECT … INTO`, `nextval`, `lo_import`
+and volatile writer functions are all correctly refused by it. It does not hold for a function
+that leaves the session.
+
+## Reproduced, not inferred
+
+Against a real PostgreSQL, in an 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.
+
+The same class of function reads the database server's filesystem, also under read-only:
+
+```
+BEGIN TRANSACTION READ ONLY
+SELECT length(pg_read_file('/etc/hostname'))   ->  13
+```
+
+And the guard permitted every one of them. Running the shipped `validateReadOnlySql` over the
+payloads directly returned `ALLOWED` for `dblink_exec`, `dblink`, `pg_read_file`, `pg_ls_dir`,
+`pg_read_binary_file`, `lo_import` and `LOAD_FILE`.
+
+Until public dashboard queries were bound to their published shapes, this was reachable from
+the **unauthenticated** share endpoint, which runs through the same executor.
+
+## The fix
+
+A denylist of functions that read or write outside the session, checked after the verb checks
+in both guards:
+
+```java
+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 is the right shape *here* because the guard's
+allowlist governs **verbs**, and there is no allowlist of functions that may appear inside a
+`SELECT` — the set of legitimate functions is open-ended, while the set that escapes the
+session is small and nameable.
+
+**Matched as a call, not as a name.** The pattern requires the name, optional whitespace, then
+an open paren, with a leading boundary check:
+
+```java
+"(?<![\\w$.])(" + DANGEROUS_FUNCTION_ALTERNATION + ")\\s*\\("
+```
+
+Matching the bare name would reject ordinary identifiers — a `dblink_audit` table, a
+`load_file_name` column — which is exactly the mistake CLAUDE.md records for the old
+`\bCOMMENT\b` rule that rejected `SELECT * FROM comment`. The boundary also stops a different
+function such as `my_dblink(` from matching. Inspection runs on text with comments and string
+literals already stripped, so neither `/*x*/dblink_exec(` nor a name inside a quoted literal
+can hide or falsely trigger a match.
+
+## Both guards, or neither
+
+`McpSqlGuardService.java` and `mcp/deepsql-phase1-lib.js` are a functional mirror of each
+other. 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 queries including
+the identifier false-positives — run through both implementations, **0 mismatches**.
+
+## Verification
+
+| Step | Result |
+|---|---|
+| Tests before the fix (RED) | 5 failures, all "expected false but was true" |
+| Tests after the fix (GREEN) | 19 pass |
+| Denylist stubbed to `return null` (mutation) | 5 fail again — the tests guard the fix |
+| Java/JS parity over 20 payloads | 0 mismatches |
+| Live attack replayed after the fix | blocked; table still 3 rows, unchanged |
+| Backend suites | 82 tests, 0 failures |
+| MCP suite | 272 tests, 0 failures |
+
+The `zz_sec` schema and the `dblink` extension created for this test were dropped; the database
+is back to its prior state.
+
+## Residual work
+
+- **`ExplainPlanService` opens its own connection and never calls `setReadOnly(true)`** — a
+  `grep` for `setReadOnly` over `src/main/java` returns exactly one hit, in
+  `QueryExecutorService`. The guard now covers the function class on that path too, but the
+  database-level backstop is still absent there.
+- **`COPY … FROM/TO PROGRAM`** is blocked today by the `COPY` verb being on the forbidden list,
+  not by this denylist. That is sufficient, but it means the protection depends on a verb rule
+  rather than the function rule, which is 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. The guard reduces blast radius; it
+  does not replace database-level permissions.

mcp/deepsql-phase1-lib.js

@@ -27,6 +27,27 @@ const FORBIDDEN_SQL_KEYWORDS = [
   "COMMENT",
 ];
 
+// Functions that read or write outside the current read-only session. Every check in this
+// guard classifies by statement *verb*, so a SELECT calling one of these passes cleanly, and
+// connection.setReadOnly(true) cannot stop them: dblink opens a new outbound connection whose
+// transaction is not read-only. Verified against a real PostgreSQL — inside an explicit
+// BEGIN TRANSACTION READ ONLY, `SELECT dblink_exec(..., 'DELETE FROM t')` reported DELETE 3
+// and the table went from three rows to zero. Mirrors DANGEROUS_SQL_FUNCTIONS in
+// McpSqlGuardService.java; the two must stay in sync or a statement one blocks the other allows.
+const DANGEROUS_SQL_FUNCTIONS = [
+  "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",
+];
+
+// The function being *called* — name, optional whitespace, open paren. Matching the bare name
+// would reject ordinary identifiers like a dblink_audit table or a load_file_name column.
+const DANGEROUS_FUNCTION_CALL = new RegExp(
+  `(?<![\\w$.])(${DANGEROUS_SQL_FUNCTIONS.join("|")})\\s*\\(`,
+  "i"
+);
+
 const FORBIDDEN_SQL_KEYWORD_SET = new Set(FORBIDDEN_SQL_KEYWORDS);
 const FORBIDDEN_ALTERNATION = FORBIDDEN_SQL_KEYWORDS.join("|");
 const CTE_MUTATION_PATTERN = new RegExp(
@@ -1185,6 +1206,11 @@ function containsForbiddenKeyword(sql) {
   return findForbiddenMutation(inspect);
 }
 
+function containsDangerousFunction(sql) {
+  const match = DANGEROUS_FUNCTION_CALL.exec(normalizeSqlForInspection(sql));
+  return match ? match[1].toLowerCase() : null;
+}
+
 function validateReadOnlySql(sql, { allowExplain = true } = {}) {
   if (!sql || !String(sql).trim()) {
     return {
@@ -1232,6 +1258,14 @@ function validateReadOnlySql(sql, { allowExplain = true } = {}) {
     };
   }
 
+  const dangerousFunction = containsDangerousFunction(statement);
+  if (dangerousFunction) {
+    return {
+      ok: false,
+      reason: `Blocked SQL function that reads or writes outside this session: ${dangerousFunction}.`,
+    };
+  }
+
   return {
     ok: true,
     normalizedQuery: stripTrailingSemicolons(sql),
@@ -2803,6 +2837,7 @@ module.exports = {
   clampInteger,
   compactWhitespace,
   containsForbiddenKeyword,
+  containsDangerousFunction,
   createConfigFromEnv,
   firstKeyword,
   getAuthToken,