From 0e6610b4d16c2de45fd621760613250d61cc428f Mon Sep 17 00:00:00 2001 From: geekypunk Date: Sat, 7 Nov 2026 10:16:55 -0600 Subject: [PATCH 1/6] fix: production database guardrails against lock pile-ups and unsafe SQL A confirmed Editor ALTER TABLE ... MODIFY ... NOT NULL on a production MySQL primary queued every write on the table for 240s. MySQL's default lock_wait_timeout is one year, so the Editor's client timeout was the only thing that stopped it. Applies to every role, admins included: - Pool-level lock guards (MySQL lock_wait_timeout/innodb_lock_wait_timeout=5, Postgres lock_timeout=5s) via Hikari connectionInitSql, re-applied by the executor after user SET preambles together with sql_safe_updates=1 and max_execution_time (30s on a writable server, client timeout on a replica). Fails closed if the guards cannot be applied. - MySQL ALTER runs online only: ALGORITHM=INSTANT, falling back to INPLACE, LOCK=NONE; COPY/DEFAULT/SHARED/EXCLUSIVE refused before confirmation. - New EXECUTE_DDL permission (ADMIN by default) for any mutation that is not row-only DML (DDL, CALL, SELECT INTO), also required by Index Advisor APPLY. - Administrative statements blocked (GRANT, user/role DDL, KILL, LOCK TABLES, FLUSH, replication control, SET GLOBAL/PERSIST, sql_log_bin). SET GLOBAL previously ran as a "session preamble" for read-only roles too. - MySQL executable comments (/*! */) and optimizer hints (/*+ */) refused: MySQL ran their body while every check saw an empty, read-only statement. - TRUNCATE blocked in the Editor; 10k server-side row cap for user-facing origins; actor tag comment on every statement; audit retention 90 days. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 48 ++++ .../IndexRecommendationController.java | 8 + .../java/com/dbaagent/model/Permission.java | 4 + .../provider/api/QueryExecutionProvider.java | 47 ++++ .../mysql/MySQLQueryExecutionProvider.java | 115 +++++++++ .../PostgresQueryExecutionProvider.java | 21 ++ .../dbaagent/service/ConnectionService.java | 9 + .../QueryExecutionPolicyException.java | 39 +++ .../service/QueryExecutionPolicyService.java | 121 ++++++++- .../service/QueryExecutorService.java | 99 +++++++- .../src/main/resources/application.properties | 4 +- .../MySQLQueryExecutionProviderGuardTest.java | 128 ++++++++++ .../QueryExecutionPolicyProdGuardTest.java | 231 ++++++++++++++++++ .../QueryExecutionPolicyServiceTest.java | 8 +- .../service/QueryExecutorGuardrailTest.java | 199 +++++++++++++++ .../service/QueryExecutorServiceTest.java | 8 +- src/lib/permissions.js | 1 + 17 files changed, 1075 insertions(+), 15 deletions(-) create mode 100644 backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java create mode 100644 backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java create mode 100644 backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java diff --git a/CLAUDE.md b/CLAUDE.md index 52aac161..9395db0f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -864,6 +864,54 @@ it against a real database — not a theoretical hardening pass. their mutation handling (covered by `anUnparseableWriteIsStillTreatedAsAMutation` and `aMalformedDataModifyingCteIsStillBlockedAsAWrite`). The MCP guard already reported this case honestly ("Only read-only SQL is allowed …") and was left alone. +- **Production guardrails (production ALTER incident).** An Editor + `ALTER TABLE … MODIFY … NOT NULL` queued every write on the table for exactly + 240s — the Editor's client timeout was the only thing that stopped it, because + MySQL's default `lock_wait_timeout` is one year. `security_event` named who ran + it; the DB-side login could not. What now applies to **every role, admins included**: + - **Session guards** come from the dialect (`QueryExecutionProvider.connectionGuardSql` + / `statementGuardSql`). The pool sets `lock_wait_timeout=5, innodb_lock_wait_timeout=5` + (Postgres: `lock_timeout='5s'`) via Hikari `connectionInitSql`, so background jobs get + it too. The executor **re-applies** them, plus `sql_safe_updates=1` and + `max_execution_time`, *after* the user's SET preambles — otherwise a preamble could + loosen them, and on a pooled connection that loosening leaks to the next borrower. + It fails closed: no guards, no statement. + - **`max_execution_time` keys off `@@global.read_only`**: reads get the client timeout + on a replica but at most 30s on a writable server. No per-connection setting to drift. + - **MySQL ALTER runs online or not at all.** `onlineDdlAttempts` appends + `ALGORITHM=INSTANT`, falling back to `INPLACE, LOCK=NONE` only on an + algorithm-unsupported error (1845/1846/"Try ALGORITHM="), never on a lock timeout. + `COPY`, `DEFAULT`, `SHARED`, `EXCLUSIVE` are refused *before* the confirmation prompt. + The clause is appended on a **new line**: on the same line a trailing `-- comment` + swallows it and the ALTER silently runs with the default (possibly blocking) algorithm. + - **`EXECUTE_DDL` is separate from DML** and ADMIN-only by default (grant DBAs via an + override/custom role). Confirmed DML is routine; DDL is not. + DDL is a leading DDL verb **or any mutation that is not row-only DML** (an allowlist: + INSERT/UPDATE/DELETE/MERGE/UPSERT/REPLACE) — a verb check alone missed `CALL` (a + procedure can run DDL) and Postgres `SELECT … INTO new_table`, and the keyword + fallback reports `RENAME` as `OTHER`. Index Advisor `APPLY` runs real `CREATE INDEX` + outside the policy, so it checks the same permission; without that it is decorative. + - **MySQL executable comments and optimizer hints are refused** (`/*! … */`, `/*+ … */`). + MySQL runs the body of `/*!50000 GRANT … */`, while every check here strips it as a + comment and `isReadOnlyQuery` then calls the empty remainder read-only — so it skipped + every guard, the confirmation prompt included. Hints (`MAX_EXECUTION_TIME`, `SET_VAR`) + override the session guards for one statement. + - **The SET scope check reads raw text, not literal-stripped text.** Stripping literals is + a second tokenizer that can disagree with MySQL's: `SET @a = "'", GLOBAL x = 1, @b = "'"` + defeated a two-pass regex, and a `NO_BACKSLASH_ESCAPES` preamble changes MySQL's own + tokenizing, so no scanner here can be trusted to match. `SET @x = 'global'` is refused + as a result — fail closed, on purpose. + - **Admin statements are blocked before classification** (`ADMIN_STATEMENT_BLOCKED`): + GRANT/REVOKE, CREATE/ALTER/DROP USER|ROLE, KILL, LOCK TABLES, FLUSH, SHUTDOWN, + replication/binlog control, and any SET reaching GLOBAL/PERSIST scope or `sql_log_bin` + — anywhere in the assignment list. SET used to be a "session preamble", so + `SET GLOBAL …; SELECT 1` ran for *read-only* roles. + - TRUNCATE is blocked in the Editor (it was MCP-only). User-facing origins get a + server-side 10k row cap whatever `limit` says (`INTERNAL`/`SCHEDULED` keep theirs). + - Every executed statement carries `/* deepsql origin=… user=… */`, so slow log, + processlist and Performance Insights attribute load to a person despite one shared + login. The username is reduced to `[A-Za-z0-9._@-]` so it can never close the comment. + - `security.audit.retention-days` is 90 (was 30). - **Keep the client timeout under the proxy's.** `docker/nginx/default.conf` gives up at `proxy_read_timeout 300s`; the Editor used to ask for 600s, so a 6-minute query returned an opaque 504 while still running. `QUERY_TIMEOUT_SECONDS = 240` diff --git a/backend/src/main/java/com/dbaagent/controller/IndexRecommendationController.java b/backend/src/main/java/com/dbaagent/controller/IndexRecommendationController.java index 656b5328..4b322b84 100644 --- a/backend/src/main/java/com/dbaagent/controller/IndexRecommendationController.java +++ b/backend/src/main/java/com/dbaagent/controller/IndexRecommendationController.java @@ -2,6 +2,7 @@ import com.dbaagent.model.IndexRecommendationEntity; import com.dbaagent.model.IndexRecommendationEvidence; +import com.dbaagent.model.Permission; import com.dbaagent.service.IndexRecommendationApplyService; import com.dbaagent.service.IndexRecommendationService; import com.dbaagent.service.security.AccessControlService; @@ -189,6 +190,13 @@ public ResponseEntity applyRecommen return ResponseEntity.notFound().build(); } accessControlService.assertCanManageConnectionContent(existing.getConnectionId()); + // APPLY runs real CREATE INDEX on the target, outside the SQL Editor's policy, + // so it needs the same permission an Editor schema change does. DRY_RUN only + // installs HypoPG hypothetical indexes and stays open. + if (m != IndexRecommendationApplyService.Mode.DRY_RUN) { + accessControlService.assertHasPermission(Permission.EXECUTE_DDL, + "Applying an index is a schema change and needs the \"Run schema changes\" permission."); + } IndexRecommendationApplyService.ApplyOptions opts = new IndexRecommendationApplyService.ApplyOptions(concurrent); IndexRecommendationApplyService.ApplyResult result = applyService.apply(id, m, confirm, opts); return ResponseEntity.ok(result); diff --git a/backend/src/main/java/com/dbaagent/model/Permission.java b/backend/src/main/java/com/dbaagent/model/Permission.java index dce662f9..d792a9fd 100644 --- a/backend/src/main/java/com/dbaagent/model/Permission.java +++ b/backend/src/main/java/com/dbaagent/model/Permission.java @@ -40,6 +40,10 @@ public enum Permission { RUN_ANALYSIS("Run analysis tasks (Key Columns, Schema, Anti-patterns)", Role.ADMIN, Role.DBA), RUN_INGESTION("Run slow query log ingestion", Role.ADMIN, Role.DBA), EXECUTE_PLAYBOOKS("Execute playbooks and automation", Role.ADMIN, Role.DBA), + // Separate from DML on purpose: confirmed writes are routine, + // schema changes are not, and one ALTER can queue every write on a table. + // ADMIN-only by default; grant it to a DBA via an override or a custom role. + EXECUTE_DDL("Run schema changes (CREATE, ALTER, DROP, RENAME, index apply) on connected databases", Role.ADMIN), USE_INDEX_ADVISOR("Apply Index Advisor recommendations", Role.ADMIN, Role.DBA), MANAGE_ALERTS("Acknowledge and manage alerts", Role.ADMIN, Role.DBA), diff --git a/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java index 9841ce0b..499cc4ef 100644 --- a/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java @@ -4,6 +4,7 @@ import java.sql.Connection; import java.sql.SQLException; +import java.util.List; /** * Provider interface for query execution operations. @@ -63,4 +64,50 @@ public interface QueryExecutionProvider { default String getSessionPidQuery() { return null; } + + /** + * Session settings applied once to every pooled connection when it opens, so every + * path (Editor, agent, dashboards, background jobs) fails fast instead of queueing + * behind a table lock. Null when the dialect needs none. + */ + default String connectionGuardSql() { + return null; + } + + /** + * Session settings re-applied immediately before each user or agent statement, + * after any SET preambles the user sent, so a preamble cannot loosen them. A + * superset of {@link #connectionGuardSql()}. + * + * @param timeoutSeconds the client-side timeout for this statement; 0 when unset + */ + default String statementGuardSql(int timeoutSeconds) { + return connectionGuardSql(); + } + + /** + * Variants of a DDL statement to try in order so it cannot block writes while it + * runs. The executor moves to the next variant only when + * {@link #isOnlineDdlUnsupported} says the previous one was refused for its + * algorithm. + * + * @throws IllegalArgumentException when the statement explicitly asks for a + * blocking algorithm or lock mode + */ + default List onlineDdlAttempts(String sql) { + return List.of(sql); + } + + /** True when a DDL attempt failed only because the requested online algorithm is unsupported. */ + default boolean isOnlineDdlUnsupported(SQLException e) { + return false; + } + + /** + * A plain-language explanation when the database refused a statement because one of + * the guards above tripped, or null for any other error. + */ + default String guardrailMessage(SQLException e) { + return null; + } } diff --git a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java index 92d82d5b..f52b194b 100644 --- a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java @@ -5,6 +5,8 @@ import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; +import java.sql.SQLException; +import java.util.List; import java.util.regex.Pattern; /** @@ -128,4 +130,117 @@ public String generateQuerySignature(String query) { public String getSessionPidQuery() { return "SELECT CONNECTION_ID()"; } + + // ---- production guardrails ------------------------------------------------- + // + // Added after a production incident: an ALTER TABLE ... MODIFY + // ... NOT NULL from the Editor queued every write on the table for four minutes. + // MySQL's default lock_wait_timeout is one year, so nothing on the server side + // stopped it; the Editor's 240s client timeout did. + + /** Seconds any statement may wait for a metadata or row lock before giving up. */ + private static final int LOCK_WAIT_SECONDS = 5; + /** Read time limit on a writable server (the primary). Replicas get the client timeout. */ + private static final int PRIMARY_READ_LIMIT_MS = 30_000; + + private static final String LOCK_GUARD = + "lock_wait_timeout = " + LOCK_WAIT_SECONDS + ", innodb_lock_wait_timeout = " + LOCK_WAIT_SECONDS; + + private static final Pattern ALTER_TABLE = Pattern.compile( + "^ALTER\\s+(?:ONLINE\\s+|IGNORE\\s+)?TABLE\\b", Pattern.CASE_INSENSITIVE); + // Value-anchored so a column named `algorithm` or `lock` does not read as a clause. + private static final Pattern ALGORITHM_CLAUSE = Pattern.compile( + "\\bALGORITHM\\s*=?\\s*(DEFAULT|INSTANT|INPLACE|COPY)\\b", Pattern.CASE_INSENSITIVE); + private static final Pattern LOCK_CLAUSE = Pattern.compile( + "\\bLOCK\\s*=?\\s*(DEFAULT|NONE|SHARED|EXCLUSIVE)\\b", Pattern.CASE_INSENSITIVE); + private static final Pattern QUOTED = Pattern.compile( + "'(?:[^'\\\\]|\\\\.|'')*'|\"(?:[^\"\\\\]|\\\\.)*\"|`[^`]*`"); + + private static final int ER_LOCK_WAIT_TIMEOUT = 1205; + private static final int ER_UPDATE_WITHOUT_KEY_IN_SAFE_MODE = 1175; + private static final int ER_QUERY_TIMEOUT = 3024; + private static final int ER_ALTER_OPERATION_NOT_SUPPORTED = 1845; + private static final int ER_ALTER_OPERATION_NOT_SUPPORTED_REASON = 1846; + + @Override + public String connectionGuardSql() { + return "SET SESSION " + LOCK_GUARD; + } + + /** + * Reads get the client timeout on a replica but at most 30s on a writable server, so + * heavy exploration lands on the replica instead of the primary bookings write to. + * {@code @@global.read_only} decides which one this is, so no per-connection setting + * can drift out of date. {@code max_execution_time} applies to SELECT only; writes + * and DDL are bounded by the lock waits and the JDBC timeout. + */ + @Override + public String statementGuardSql(int timeoutSeconds) { + int clientMs = timeoutSeconds > 0 ? timeoutSeconds * 1000 : PRIMARY_READ_LIMIT_MS; + return "SET SESSION " + LOCK_GUARD + ", sql_safe_updates = 1, max_execution_time = IF(@@global.read_only, " + + clientMs + ", LEAST(" + clientMs + ", " + PRIMARY_READ_LIMIT_MS + "))"; + } + + @Override + public List onlineDdlAttempts(String sql) { + if (sql == null || !ALTER_TABLE.matcher(stripLeadingComments(sql)).find()) { + return List.of(sql); + } + String base = sql.strip(); + while (base.endsWith(";")) { + base = base.substring(0, base.length() - 1).strip(); + } + String clauses = QUOTED.matcher(base).replaceAll("''"); + String algorithm = clauseValue(ALGORITHM_CLAUSE, clauses); + String lock = clauseValue(LOCK_CLAUSE, clauses); + + if ("COPY".equals(algorithm) || "DEFAULT".equals(algorithm) + || "SHARED".equals(lock) || "EXCLUSIVE".equals(lock) || "DEFAULT".equals(lock)) { + throw new IllegalArgumentException( + "DeepSQL only runs schema changes online (ALGORITHM=INSTANT, or INPLACE with LOCK=NONE). " + + "This one asks for a mode that blocks writes to the table while it runs. " + + "Make large rebuilds off-peak with gh-ost or pt-online-schema-change instead."); + } + // The appended clause starts on a new line: a trailing "-- comment" would + // otherwise swallow it, and the ALTER would run with MySQL's default algorithm. + if (algorithm == null && lock == null) { + return List.of(base + "\n, ALGORITHM=INSTANT", base + "\n, ALGORITHM=INPLACE, LOCK=NONE"); + } + if ("INPLACE".equals(algorithm) && lock == null) { + return List.of(base + "\n, LOCK=NONE"); + } + return List.of(sql); + } + + private static String clauseValue(Pattern clause, String sql) { + java.util.regex.Matcher m = clause.matcher(sql); + return m.find() ? m.group(1).toUpperCase(java.util.Locale.ROOT) : null; + } + + @Override + public boolean isOnlineDdlUnsupported(SQLException e) { + int code = e.getErrorCode(); + return code == ER_ALTER_OPERATION_NOT_SUPPORTED + || code == ER_ALTER_OPERATION_NOT_SUPPORTED_REASON + // Later InnoDB limits (row versions, max row size) word it the same way. + || (e.getMessage() != null && e.getMessage().contains("Try ALGORITHM=")); + } + + @Override + public String guardrailMessage(SQLException e) { + if (isOnlineDdlUnsupported(e)) { + return "MySQL cannot make this change without blocking writes to the table (" + e.getMessage() + "). " + + "Run it off-peak with gh-ost or pt-online-schema-change."; + } + return switch (e.getErrorCode()) { + case ER_LOCK_WAIT_TIMEOUT -> "Gave up after waiting " + LOCK_WAIT_SECONDS + " seconds for a lock: another " + + "session is using this table or these rows. DeepSQL does not queue behind locks, because a " + + "waiting statement makes every later write to the table wait too. Try again when it is quieter."; + case ER_UPDATE_WITHOUT_KEY_IN_SAFE_MODE -> "MySQL safe-updates mode refused this: the WHERE clause must " + + "filter on an indexed (key) column, or the statement must have a LIMIT."; + case ER_QUERY_TIMEOUT -> "Stopped at the " + PRIMARY_READ_LIMIT_MS / 1000 + "-second limit for reads on a " + + "primary database. Run heavy queries on the replica connection instead."; + default -> null; + }; + } } diff --git a/backend/src/main/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProvider.java index a2cf814e..f78a0961 100644 --- a/backend/src/main/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProvider.java @@ -132,4 +132,25 @@ public String generateQuerySignature(String query) { public String getSessionPidQuery() { return "SELECT pg_backend_pid()"; } + + /** + * An ALTER waiting for ACCESS EXCLUSIVE queues every later reader and writer behind + * it, same as a MySQL metadata lock. Give up after 5s instead of holding the line. + * The statement time limit stays with the JDBC timeout. + */ + @Override + public String connectionGuardSql() { + return "SET lock_timeout = '5s'"; + } + + @Override + public String guardrailMessage(java.sql.SQLException e) { + // 55P03 lock_not_available: raised when lock_timeout expires. + if ("55P03".equals(e.getSQLState())) { + return "Gave up after waiting 5 seconds for a lock: another session is using this table or these rows. " + + "DeepSQL does not queue behind locks, because a waiting statement makes every later query on the " + + "table wait too. Try again when it is quieter."; + } + return null; + } } diff --git a/backend/src/main/java/com/dbaagent/service/ConnectionService.java b/backend/src/main/java/com/dbaagent/service/ConnectionService.java index 9b915666..8f8bd868 100644 --- a/backend/src/main/java/com/dbaagent/service/ConnectionService.java +++ b/backend/src/main/java/com/dbaagent/service/ConnectionService.java @@ -295,6 +295,15 @@ private HikariDataSource createConnectionPool(String connectionId, ConnectionReq config.setKeepaliveTime(300_000); // 5 min config.setLeakDetectionThreshold(60000); + // Every connection in the pool gives up on a table lock after a few seconds + // instead of queueing behind it — background jobs included, not only the + // Editor. A statement waiting for a metadata lock makes every later write to + // that table wait too. + String guardSql = dialect.queryExecution().connectionGuardSql(); + if (guardSql != null) { + config.setConnectionInitSql(guardSql); + } + // Connection properties from provider with full SSL config Map connectionProperties = connectionProvider.getConnectionPropertiesWithSsl( tunnelPort, diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyException.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyException.java index 4a93c228..e0ccfba4 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyException.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyException.java @@ -13,6 +13,9 @@ public class QueryExecutionPolicyException extends RuntimeException { public static final String DB_WRITE_PRIVILEGE_DENIED = "DB_WRITE_PRIVILEGE_DENIED"; public static final String MULTI_STATEMENT_MISSING_SEMICOLONS = "MULTI_STATEMENT_MISSING_SEMICOLONS"; public static final String STATEMENT_NOT_PARSEABLE = "STATEMENT_NOT_PARSEABLE"; + public static final String ADMIN_STATEMENT_BLOCKED = "ADMIN_STATEMENT_BLOCKED"; + public static final String DDL_PERMISSION_REQUIRED = "DDL_PERMISSION_REQUIRED"; + public static final String DB_GUARDRAIL_TRIPPED = "DB_GUARDRAIL_TRIPPED"; private final String errorCode; private final HttpStatus httpStatus; @@ -107,6 +110,42 @@ public static QueryExecutionPolicyException unsafeMutation(String message, Strin ); } + public static QueryExecutionPolicyException adminStatementBlocked() { + return new QueryExecutionPolicyException( + ADMIN_STATEMENT_BLOCKED, + HttpStatus.FORBIDDEN, + "DeepSQL never runs server-administration statements (users and grants, global settings, " + + "KILL, LOCK TABLES, FLUSH, replication and binlog control). Use your database admin tooling.", + false, + "ADMIN", + List.of() + ); + } + + public static QueryExecutionPolicyException ddlPermissionRequired(String queryType) { + return new QueryExecutionPolicyException( + DDL_PERMISSION_REQUIRED, + HttpStatus.FORBIDDEN, + "Schema changes (CREATE, ALTER, DROP, RENAME) need the \"Run schema changes\" permission. " + + "Ask an admin to grant it, or have someone who holds it run this change.", + false, + queryType, + List.of() + ); + } + + /** The database refused the statement because one of DeepSQL's session guardrails tripped. */ + public static QueryExecutionPolicyException guardrailTripped(String message, String queryType) { + return new QueryExecutionPolicyException( + DB_GUARDRAIL_TRIPPED, + HttpStatus.CONFLICT, + message, + false, + queryType, + List.of() + ); + } + public static QueryExecutionPolicyException multiStatementMissingSemicolons() { return new QueryExecutionPolicyException( MULTI_STATEMENT_MISSING_SEMICOLONS, diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java index a3c3ab02..87bbad56 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java @@ -1,9 +1,11 @@ package com.dbaagent.service; +import com.dbaagent.model.Permission; import com.dbaagent.model.QueryExecutionOrigin; import com.dbaagent.model.QueryRequest; import com.dbaagent.provider.DatabaseProviderRegistry; import com.dbaagent.provider.api.QueryExecutionProvider; +import com.dbaagent.service.security.AccessControlService; import com.dbaagent.util.QueryNormalizer; import com.dbaagent.util.SqlStatementSplitter; import lombok.RequiredArgsConstructor; @@ -82,7 +84,45 @@ public class QueryExecutionPolicyService { Pattern.CASE_INSENSITIVE ); + // Server administration. Blocked on every surface for every role, confirmed or + // not: none of it is data work, and each one can take a production database down + // or widen access (users/grants, KILL, LOCK TABLES, FLUSH ... READ LOCK, + // replication and binlog control). Matched on the leading verb only, after + // comments and literals are stripped, so a `grant_id` column is fine. + private static final Pattern ADMIN_STATEMENT_PATTERN = Pattern.compile( + "^(?:GRANT|REVOKE|KILL|SHUTDOWN|FLUSH|INSTALL|UNINSTALL|PURGE|REASSIGN" + + "|LOCK\\s+(?:TABLES?|INSTANCE)|UNLOCK\\s+INSTANCE" + + "|(?:CREATE|ALTER|DROP|RENAME)\\s+(?:USER|ROLE)|ALTER\\s+SYSTEM" + + "|RESET\\s+(?:MASTER|REPLICA|SLAVE|BINARY|PERSIST)" + + "|CHANGE\\s+(?:MASTER|REPLICATION)|(?:START|STOP)\\s+(?:SLAVE|REPLICA|GROUP_REPLICATION)" + + "|SET\\s+(?:PASSWORD|ROLE|DEFAULT\\s+ROLE))\\b", + Pattern.CASE_INSENSITIVE + ); + // Any SET that reaches global or persisted scope, anywhere in its assignment list + // (`SET @x = 1, GLOBAL max_connections = 10` is valid MySQL), or that turns off + // binary logging for the session — replicas would silently diverge. + private static final Pattern SET_PREFIX_PATTERN = Pattern.compile("^SET\\b", Pattern.CASE_INSENSITIVE); + private static final Pattern SET_BEYOND_SESSION_PATTERN = Pattern.compile( + "\\b(?:GLOBAL|PERSIST|PERSIST_ONLY)\\b|@@(?:GLOBAL|PERSIST|PERSIST_ONLY)\\.|\\bsql_log_bin\\b", + Pattern.CASE_INSENSITIVE + ); + private static final Pattern DDL_VERB_PATTERN = Pattern.compile( + "^(?:CREATE|ALTER|DROP|RENAME|TRUNCATE)\\b", Pattern.CASE_INSENSITIVE); + // MySQL executes the body of /*! ... */ (and /*!50000 ... */), while every check in + // this class strips it as a comment and then sees an empty statement that + // isReadOnlyQuery calls read-only — so `/*!50000 GRANT ... */` skipped every guard. + // /*+ ... */ optimizer hints (MAX_EXECUTION_TIME, SET_VAR) override the session + // guards for one statement. Matched on the raw text: a literal containing "/*!" is + // refused too, which is the right side to be wrong on. + private static final Pattern EXECUTABLE_COMMENT_PATTERN = Pattern.compile("/\\*[!+]"); + // Mutations that only change rows. Anything else that mutates — DDL, CALL/DO (a + // procedure can run DDL), SELECT ... INTO a new table — needs EXECUTE_DDL. An + // allowlist, so a statement shape nobody anticipated lands on the stricter side. + private static final java.util.Set ROW_ONLY_MUTATIONS = + java.util.Set.of("INSERT", "UPDATE", "DELETE", "MERGE", "UPSERT", "REPLACE"); + private final DatabaseProviderRegistry providerRegistry; + private final AccessControlService accessControlService; public PolicyDecision enforce( QueryRequest request, @@ -101,6 +141,22 @@ public PolicyDecision enforce( statements = List.of(sql == null ? "" : sql); } + // Before classification: SET used to be waved through as a session preamble, + // which let SET GLOBAL run for any role, read-only ones included. + for (String statement : statements) { + if (statement != null && EXECUTABLE_COMMENT_PATTERN.matcher(statement).find()) { + throw QueryExecutionPolicyException.unsafeMutation( + "DeepSQL does not run MySQL executable comments (/*! ... */) or optimizer hints (/*+ ... */): " + + "the database runs what is inside them, but DeepSQL's safety checks cannot see it. " + + "Write the statement without them.", + "UNKNOWN" + ); + } + if (isAdministrativeStatement(statement)) { + throw QueryExecutionPolicyException.adminStatementBlocked(); + } + } + List classifications = new ArrayList<>(); for (String statement : statements) { classifications.add(classifyStatement(statement, executionProvider)); @@ -178,6 +234,36 @@ && isDropTableStatement(mutation.queryType(), statements.getFirst())) { ); } + if (origin == QueryExecutionOrigin.EDITOR && isTruncate(mutation.queryType(), statements.getFirst())) { + throw QueryExecutionPolicyException.unsafeMutation( + "TRUNCATE is not allowed from the SQL Editor: it empties the whole table and cannot be rolled back. " + + "Use DELETE with a WHERE clause, or your database admin tooling.", + mutation.queryType() + ); + } + + List confirmationNotes = new ArrayList<>(); + boolean ddl = isDdlStatement(statements.getFirst(), mutation.queryType()); + if (ddl && (origin == QueryExecutionOrigin.EDITOR || origin == QueryExecutionOrigin.MCP)) { + if (!accessControlService.hasPermission(Permission.EXECUTE_DDL)) { + throw QueryExecutionPolicyException.ddlPermissionRequired(mutation.queryType()); + } + // Refuse a blocking ALTER here, ahead of the confirmation prompt, rather than + // let someone confirm a statement the executor will then reject. + List attempts; + try { + attempts = executionProvider.onlineDdlAttempts(statements.getFirst()); + } catch (IllegalArgumentException blocking) { + throw QueryExecutionPolicyException.unsafeMutation(blocking.getMessage(), mutation.queryType()); + } + if (!attempts.equals(List.of(statements.getFirst()))) { + confirmationNotes.add("DeepSQL will run this online: ALGORITHM=INSTANT first, then ALGORITHM=INPLACE " + + "with LOCK=NONE. If neither can run without blocking writes, it stops and changes nothing."); + } + confirmationNotes.add("If the table is busy, DeepSQL gives up after waiting 5 seconds for its lock " + + "instead of queueing every other write to the table behind this change."); + } + if (mutation.requiresWhereClause && !mutation.hasWhereClause) { throw QueryExecutionPolicyException.unsafeMutation( "UPDATE or DELETE without a WHERE clause is blocked in the SQL Editor. Add a WHERE clause to limit the affected rows.", @@ -186,12 +272,45 @@ && isDropTableStatement(mutation.queryType(), statements.getFirst())) { } if (!effectiveContext.mutationConfirmed()) { - throw QueryExecutionPolicyException.confirmationRequired(mutation.queryType(), buildWarnings(mutation)); + List warnings = new ArrayList<>(buildWarnings(mutation)); + warnings.addAll(confirmationNotes); + throw QueryExecutionPolicyException.confirmationRequired(mutation.queryType(), warnings); } return new PolicyDecision(classifications, true, mutation.queryType()); } + /** + * Deliberately reads the raw text, without stripping string literals. Stripping is a + * second tokenizer that can disagree with MySQL's — `SET @a = "'", GLOBAL x = 1, @b = "'"` + * fooled a two-pass regex, and a NO_BACKSLASH_ESCAPES preamble changes MySQL's own + * tokenizing — so the check fails closed: `SET @x = 'global'` is refused too. + */ + private boolean isAdministrativeStatement(String statement) { + String head = stripLeadingComments(statement).trim(); + if (ADMIN_STATEMENT_PATTERN.matcher(head).find()) { + return true; + } + return SET_PREFIX_PATTERN.matcher(head).find() && SET_BEYOND_SESSION_PATTERN.matcher(head).find(); + } + + /** + * A leading DDL verb, or any mutation that is not row-only DML. The verb check alone + * missed CALL (a procedure can run DDL) and Postgres SELECT ... INTO, which creates a + * table; the keyword fallback also reports RENAME as "OTHER". + */ + private boolean isDdlStatement(String statement, String queryType) { + if (DDL_VERB_PATTERN.matcher(stripLeadingComments(statement)).find()) { + return true; + } + return queryType == null || !ROW_ONLY_MUTATIONS.contains(queryType.toUpperCase(Locale.ROOT)); + } + + private boolean isTruncate(String queryType, String statement) { + return (queryType != null && queryType.toUpperCase(Locale.ROOT).startsWith("TRUNCATE")) + || stripLeadingComments(statement).toUpperCase(Locale.ROOT).startsWith("TRUNCATE"); + } + private List buildWarnings(StatementClassification mutation) { List warnings = new ArrayList<>(); warnings.add("DeepSQL will submit this statement directly to the selected database connection."); diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java index ff2d55d5..c416ba73 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java @@ -3,6 +3,7 @@ import com.dbaagent.model.*; import com.dbaagent.provider.DatabaseProviderRegistry; import com.dbaagent.provider.api.IntrospectionProvider; +import com.dbaagent.provider.api.QueryExecutionProvider; import com.dbaagent.repository.KeyColumnAnalysisRepository; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; @@ -671,6 +672,21 @@ public QueryResult executeQuery( } } + // Re-assert the session guardrails after the user's preambles, so a + // `SET SESSION lock_wait_timeout = ...` in front of the query cannot loosen + // them, and so a value one caller changed never leaks to the next borrower + // of this pooled connection. Fails closed: a statement does not run without them. + QueryExecutionProvider executionProvider = providerRegistry.getDialect(connRequest.getDbType()).queryExecution(); + String guardSql = executionProvider.statementGuardSql(effectiveTimeout); + if (guardSql != null) { + try (Statement guardStmt = connection.createStatement()) { + guardStmt.execute(guardSql); + } catch (SQLException e) { + throw new SQLException("Could not apply DeepSQL's database safety settings, so the query was not run: " + + e.getMessage(), e.getSQLState(), e.getErrorCode(), e); + } + } + // The last (or only) statement is the query whose results we return String finalQuery = statements.get(statements.size() - 1); @@ -687,8 +703,8 @@ public QueryResult executeQuery( // Appending " LIMIT n" is kept as an optimization for simple SELECTs // (it lets the database stop early rather than stream rows we drop), // but correctness no longer depends on that textual check. - boolean limitRequested = queryRequest.getLimit() != null && queryRequest.getLimit() > 0; - int rowLimit = limitRequested ? queryRequest.getLimit() : 0; + int rowLimit = effectiveRowLimit(queryRequest.getLimit(), executionContext); + boolean limitRequested = rowLimit > 0; boolean limitApplied = false; String baseQuery = null; // original query without limit, for COUNT if (limitRequested) { @@ -712,17 +728,45 @@ public QueryResult executeQuery( } // Execute query - boolean hasResultSet; + // DDL runs as online-only variants (MySQL: INSTANT, then INPLACE with + // LOCK=NONE). Everything else is a single attempt of itself. + List attempts; try { - hasResultSet = stmt.execute(finalQuery); - } catch (SQLException executionError) { - if (policyDecision.mutating() && looksLikeWritePrivilegeDenied(executionError)) { - throw QueryExecutionPolicyException.dbWritePrivilegeDenied( - executionError.getMessage(), - policyDecision.primaryQueryType() - ); + attempts = policyDecision.mutating() + ? executionProvider.onlineDdlAttempts(finalQuery) + : List.of(finalQuery); + } catch (IllegalArgumentException blocking) { + throw QueryExecutionPolicyException.unsafeMutation( + blocking.getMessage(), policyDecision.primaryQueryType()); + } + String tag = actorTag(executionContext); + boolean hasResultSet = false; + for (int attempt = 0; attempt < attempts.size(); attempt++) { + try { + hasResultSet = stmt.execute(tag + attempts.get(attempt)); + break; + } catch (SQLException executionError) { + boolean anotherAttemptLeft = attempt < attempts.size() - 1; + if (anotherAttemptLeft && executionProvider.isOnlineDdlUnsupported(executionError)) { + log.info("Online DDL variant {} unsupported, trying next: {}", + attempt + 1, executionError.getMessage()); + continue; + } + if (policyDecision.mutating() && looksLikeWritePrivilegeDenied(executionError)) { + throw QueryExecutionPolicyException.dbWritePrivilegeDenied( + executionError.getMessage(), + policyDecision.primaryQueryType() + ); + } + String guardrail = executionProvider.guardrailMessage(executionError); + if (guardrail != null) { + log.warn("Database guardrail tripped for {} on {}: {}", + executionContext.actorUsername(), connectionId, executionError.getMessage()); + throw QueryExecutionPolicyException.guardrailTripped( + guardrail, policyDecision.primaryQueryType()); + } + throw executionError; } - throw executionError; } if (hasResultSet) { @@ -802,6 +846,39 @@ public QueryResult executeQuery( return userDataAccessPolicyService.redactResult(connectionId, result, executionContext); } + /** Hard ceiling on rows returned to a person or agent, whatever the request asks for. */ + static final int MAX_RESULT_ROWS = 10_000; + + /** + * The row cap was client-controlled: a request without {@code limit} streamed the + * whole table into memory. People and agents now get at most {@link #MAX_RESULT_ROWS}; + * DeepSQL's own background jobs keep what they ask for. + */ + static int effectiveRowLimit(Integer requested, QueryExecutionContext context) { + int asked = requested != null && requested > 0 ? requested : 0; + QueryExecutionOrigin origin = context == null ? null : context.origin(); + if (origin == QueryExecutionOrigin.INTERNAL || origin == QueryExecutionOrigin.SCHEDULED || origin == null) { + return asked; + } + return asked > 0 ? Math.min(asked, MAX_RESULT_ROWS) : MAX_RESULT_ROWS; + } + + /** + * A leading comment naming who ran the statement. Every DeepSQL query reaches the + * database as one shared login, so without this the slow log, processlist and + * Performance Insights cannot say which person (or agent) caused the load. + * The username is reduced to a safe charset so it can never close the comment. + */ + static String actorTag(QueryExecutionContext context) { + String origin = context == null || context.origin() == null + ? "internal" + : context.origin().name().toLowerCase(Locale.ROOT); + String user = context == null || context.actorUsername() == null + ? "" + : " user=" + context.actorUsername().replaceAll("[^A-Za-z0-9._@-]", "_"); + return "/* deepsql origin=" + origin + user + " */ "; + } + /** * Strips a trailing ORDER BY clause from a SQL query so it can be safely wrapped * in a COUNT(*) subquery. ORDER BY in a subquery without LIMIT is invalid in some diff --git a/backend/src/main/resources/application.properties b/backend/src/main/resources/application.properties index 23c59967..abddbdd0 100644 --- a/backend/src/main/resources/application.properties +++ b/backend/src/main/resources/application.properties @@ -255,7 +255,9 @@ training.history.retention.days=0 training.history.cleanup.cron=0 30 2 * * * # Security / user-log retention (security_event, user_session, auth_login_challenge) -security.audit.retention-days=30 +# 90 days: an incident review needed to know who ran a statement, and +# security_event (who, origin, SQL, rows) is the only per-person record of it. +security.audit.retention-days=90 # Growth Monitoring Configuration growth-monitoring.enabled=true diff --git a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java new file mode 100644 index 00000000..17fb9fd8 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java @@ -0,0 +1,128 @@ +package com.dbaagent.provider.mysql; + +import org.junit.jupiter.api.Test; + +import java.sql.SQLException; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * The guardrails added after a production incident: an + * {@code ALTER TABLE customers MODIFY ... NOT NULL} queued every write on the table for + * four minutes, until the Editor's 240s client timeout killed it. + */ +class MySQLQueryExecutionProviderGuardTest { + + private final MySQLQueryExecutionProvider provider = new MySQLQueryExecutionProvider(); + + // ---- online DDL ---------------------------------------------------------- + + @Test + void anAlterWithNoAlgorithmTriesInstantThenInplaceWithoutLocks() { + List attempts = provider.onlineDdlAttempts( + "ALTER TABLE customers MODIFY email varchar(512) NOT NULL;"); + + assertThat(attempts).containsExactly( + "ALTER TABLE customers MODIFY email varchar(512) NOT NULL\n, ALGORITHM=INSTANT", + "ALTER TABLE customers MODIFY email varchar(512) NOT NULL\n, ALGORITHM=INPLACE, LOCK=NONE"); + } + + @Test + void theAppendedClauseCannotBeSwallowedByATrailingLineComment() { + // Appended on the same line, ", ALGORITHM=INSTANT" would sit inside the + // comment and the ALTER would run with MySQL's default (possibly blocking) + // algorithm — a silent bypass of the whole guard. + List attempts = provider.onlineDdlAttempts("ALTER TABLE t ADD COLUMN c INT -- for reports"); + + assertThat(attempts.getFirst()).endsWith("-- for reports\n, ALGORITHM=INSTANT"); + } + + @Test + void inplaceWithoutALockClauseGetsLockNone() { + assertThat(provider.onlineDdlAttempts("ALTER TABLE t ADD INDEX i (c), ALGORITHM=INPLACE")) + .containsExactly("ALTER TABLE t ADD INDEX i (c), ALGORITHM=INPLACE\n, LOCK=NONE"); + } + + @Test + void explicitlyOnlineStatementsRunAsWritten() { + assertThat(provider.onlineDdlAttempts("ALTER TABLE t ADD COLUMN c INT, ALGORITHM=INSTANT")) + .containsExactly("ALTER TABLE t ADD COLUMN c INT, ALGORITHM=INSTANT"); + assertThat(provider.onlineDdlAttempts("ALTER TABLE t ADD INDEX i (c), ALGORITHM=INPLACE, LOCK=NONE")) + .containsExactly("ALTER TABLE t ADD INDEX i (c), ALGORITHM=INPLACE, LOCK=NONE"); + assertThat(provider.onlineDdlAttempts("ALTER TABLE t ADD INDEX i (c), LOCK = NONE")) + .containsExactly("ALTER TABLE t ADD INDEX i (c), LOCK = NONE"); + } + + @Test + void blockingAlgorithmsAndLocksAreRefused() { + for (String sql : List.of( + "ALTER TABLE t MODIFY c varchar(512), ALGORITHM=COPY", + "ALTER TABLE t MODIFY c varchar(512), ALGORITHM COPY", + "ALTER TABLE t MODIFY c varchar(512), ALGORITHM=DEFAULT", + "ALTER TABLE t MODIFY c varchar(512), LOCK=SHARED", + "ALTER TABLE t MODIFY c varchar(512), LOCK=EXCLUSIVE", + "ALTER TABLE t MODIFY c varchar(512), ALGORITHM=INPLACE, LOCK=DEFAULT")) { + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, + () -> provider.onlineDdlAttempts(sql), sql); + assertThat(e.getMessage()).contains("gh-ost"); + } + } + + @Test + void wordsInsideLiteralsAndIdentifiersAreNotClauses() { + // A comment string or a column named `lock` must not read as a LOCK clause. + assertThat(provider.onlineDdlAttempts("ALTER TABLE t COMMENT 'LOCK=SHARED please'")) + .hasSize(2); + assertThat(provider.onlineDdlAttempts("ALTER TABLE t ADD COLUMN `lock` INT")) + .hasSize(2); + } + + @Test + void nonAlterStatementsPassThrough() { + assertThat(provider.onlineDdlAttempts("CREATE TABLE t (id INT)")).containsExactly("CREATE TABLE t (id INT)"); + assertThat(provider.onlineDdlAttempts("UPDATE t SET a = 1 WHERE id = 2")) + .containsExactly("UPDATE t SET a = 1 WHERE id = 2"); + } + + @Test + void onlyAlgorithmNotSupportedErrorsTriggerTheFallback() { + assertThat(provider.isOnlineDdlUnsupported(new SQLException( + "ALGORITHM=INSTANT is not supported. Reason: Need to rebuild the table. Try ALGORITHM=COPY/INPLACE.", + "0A000", 1846))).isTrue(); + assertThat(provider.isOnlineDdlUnsupported(new SQLException("Lock wait timeout exceeded", "HY000", 1205))) + .isFalse(); + } + + // ---- session guards ------------------------------------------------------ + + @Test + void everyPooledConnectionGivesUpOnLocksAfterFiveSeconds() { + assertThat(provider.connectionGuardSql()) + .isEqualTo("SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5"); + } + + @Test + void perStatementGuardCapsReadsAtThirtySecondsOnlyOnAWritableServer() { + assertThat(provider.statementGuardSql(240)).isEqualTo( + "SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5, sql_safe_updates = 1, " + + "max_execution_time = IF(@@global.read_only, 240000, LEAST(240000, 30000))"); + assertThat(provider.statementGuardSql(0)).contains("IF(@@global.read_only, 30000, LEAST(30000, 30000))"); + } + + // ---- errors -------------------------------------------------------------- + + @Test + void guardTripsAreExplainedInPlainWords() { + assertThat(provider.guardrailMessage(new SQLException("Lock wait timeout exceeded", "HY000", 1205))) + .contains("5 seconds"); + assertThat(provider.guardrailMessage(new SQLException("You are using safe update mode", "HY000", 1175))) + .contains("indexed"); + assertThat(provider.guardrailMessage(new SQLException("maximum statement execution time exceeded", "HY000", 3024))) + .contains("replica"); + assertThat(provider.guardrailMessage(new SQLException("not supported. Try ALGORITHM=COPY", "0A000", 1846))) + .contains("gh-ost"); + assertThat(provider.guardrailMessage(new SQLException("Duplicate entry", "23000", 1062))).isNull(); + } +} diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java new file mode 100644 index 00000000..c6444409 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java @@ -0,0 +1,231 @@ +package com.dbaagent.service; + +import com.dbaagent.model.Permission; +import com.dbaagent.model.QueryRequest; +import com.dbaagent.provider.DatabaseProviderRegistry; +import com.dbaagent.provider.api.DatabaseDialect; +import com.dbaagent.provider.mysql.MySQLQueryExecutionProvider; +import com.dbaagent.service.security.AccessControlService; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.when; + +/** + * Production guardrails, exercised against the real MySQL provider. Every case runs + * as a confirmed admin: these are the statements that must stay blocked even for + * the people who can otherwise write. + */ +@ExtendWith(MockitoExtension.class) +class QueryExecutionPolicyProdGuardTest { + + @Mock private DatabaseProviderRegistry providerRegistry; + @Mock private DatabaseDialect databaseDialect; + @Mock private AccessControlService accessControlService; + + private QueryExecutionPolicyService service; + + @BeforeEach + void setUp() { + lenient().when(providerRegistry.getDialect(anyString())).thenReturn(databaseDialect); + lenient().when(databaseDialect.queryExecution()).thenReturn(new MySQLQueryExecutionProvider()); + lenient().when(providerRegistry.getCanonicalName(anyString())).thenReturn("mysql"); + lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(true); + service = new QueryExecutionPolicyService(providerRegistry, accessControlService); + } + + private QueryExecutionPolicyException blocked(String sql, QueryExecutionContext context) { + return assertThrows(QueryExecutionPolicyException.class, + () -> service.enforce(new QueryRequest(sql, null, null), context, "mysql"), sql); + } + + private static QueryExecutionContext confirmedAdminEditor() { + return QueryExecutionContext.editor("admin", true, true); + } + + @Test + void administrativeStatementsAreBlockedOnEverySurfaceEvenWhenConfirmed() { + for (String sql : List.of( + "SET GLOBAL max_connections = 10", + "SET PERSIST innodb_lock_wait_timeout = 1", + "SET @@global.read_only = 0", + "SET @x = 1, GLOBAL max_connections = 10", + "SET sql_log_bin = 0", + "GRANT ALL ON *.* TO 'x'@'%'", + "REVOKE SELECT ON db.* FROM 'x'@'%'", + "CREATE USER 'x'@'%' IDENTIFIED BY 'p'", + "ALTER USER 'app_user'@'%' IDENTIFIED BY 'p'", + "DROP USER 'x'@'%'", + "KILL 12345", + "KILL QUERY 12345", + "LOCK TABLES customers WRITE", + "FLUSH TABLES WITH READ LOCK", + "SHUTDOWN", + "RESET MASTER", + "PURGE BINARY LOGS BEFORE NOW()", + " /* sneaky */ GRANT ALL ON *.* TO 'x'@'%'")) { + assertThat(blocked(sql, confirmedAdminEditor()).getErrorCode()) + .as(sql).isEqualTo(QueryExecutionPolicyException.ADMIN_STATEMENT_BLOCKED); + assertThat(blocked(sql, QueryExecutionContext.mcp("admin", true, true)).getErrorCode()) + .as(sql).isEqualTo(QueryExecutionPolicyException.ADMIN_STATEMENT_BLOCKED); + } + } + + @Test + void executableCommentsAndOptimizerHintsAreRefused() { + // MySQL runs the body of /*! ... */, while every check here strips it as a comment + // and then sees an empty, "read-only" statement. /*+ ... */ hints such as + // MAX_EXECUTION_TIME or SET_VAR override the session guards for one statement. + for (String sql : List.of( + "/*!50000 GRANT ALL ON *.* TO 'x'@'%' */", + "/*! ALTER TABLE customers ADD COLUMN x INT */", + "/*!SET GLOBAL max_connections = 10*/", + "SELECT /*+ MAX_EXECUTION_TIME(600000) */ * FROM customers", + "UPDATE /*+ SET_VAR(innodb_lock_wait_timeout=31536000) */ customers SET a = 1 WHERE id = 2")) { + assertThat(blocked(sql, QueryExecutionContext.editor("analyst", false, false)).getErrorCode()) + .as(sql).isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + assertThat(blocked(sql, confirmedAdminEditor()).getErrorCode()) + .as(sql).isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + } + } + + @Test + void globalScopeCannotHideInsideMismatchedQuotes() { + // Two-pass quote stripping removed GLOBAL here; MySQL tokenizes it as code. And a + // NO_BACKSLASH_ESCAPES preamble changes MySQL's tokenizing, so no scanner on our + // side can be trusted to agree: the SET check reads the raw text and fails closed. + for (String sql : List.of( + "SET @a = \"'\", GLOBAL max_connections = 1, @b = \"'\"", + "SET SESSION sql_mode = 'NO_BACKSLASH_ESCAPES'; " + + "SET @a = '\\', GLOBAL max_connections = 1, @b = '\\'; SELECT 1")) { + assertThat(blocked(sql, QueryExecutionContext.editor("analyst", false, false)).getErrorCode()) + .as(sql).isEqualTo(QueryExecutionPolicyException.ADMIN_STATEMENT_BLOCKED); + } + } + + @Test + void schemaChangesThatDoNotStartWithADdlVerbStillNeedThePermission() { + lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(false); + for (String sql : List.of( + "CALL rebuild_customer_indexes()", + "SELECT * INTO customers_copy FROM customers")) { + assertThat(blocked(sql, QueryExecutionContext.editor("dba", true, true)).getErrorCode()) + .as(sql).isEqualTo(QueryExecutionPolicyException.DDL_PERMISSION_REQUIRED); + } + } + + @Test + void anAdministrativeStatementHiddenAsAPreambleIsStillBlocked() { + // SET used to be classified as a harmless session preamble, so a SET GLOBAL in + // front of a SELECT ran for any role, including read-only ones. + assertThat(blocked("SET GLOBAL max_connections = 10; SELECT 1", + QueryExecutionContext.editor("analyst", false, false)).getErrorCode()) + .isEqualTo(QueryExecutionPolicyException.ADMIN_STATEMENT_BLOCKED); + } + + @Test + void sessionVariablesStillWork() { + QueryExecutionPolicyService.PolicyDecision decision = service.enforce( + new QueryRequest("SET @since = '2026-11-01'; SELECT * FROM bookings WHERE created_at > @since", null, null), + QueryExecutionContext.editor("analyst", false, false), + "mysql"); + assertThat(decision.mutating()).isFalse(); + } + + @Test + void identifiersThatLookLikeAdminVerbsAreNotBlocked() { + // Same class of mistake as the old \bCOMMENT\b rule rejecting SELECT * FROM comment. + service.enforce(new QueryRequest("SELECT grant_id, kill_switch FROM `lock`", null, null), + QueryExecutionContext.editor("analyst", false, false), "mysql"); + } + + @Test + void truncateIsBlockedInTheEditorEvenForAConfirmedAdmin() { + assertThat(blocked("TRUNCATE TABLE customers", confirmedAdminEditor()).getErrorCode()) + .isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + } + + @Test + void ddlRequiresTheExecuteDdlPermission() { + when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(false); + QueryExecutionContext dba = QueryExecutionContext.editor("dba", true, true); + + for (String sql : List.of( + "ALTER TABLE customers MODIFY email varchar(512) NOT NULL", + "CREATE TABLE t (id INT)", + "CREATE INDEX i ON t (c)", + "DROP INDEX i ON t", + "RENAME TABLE a TO b")) { + assertThat(blocked(sql, dba).getErrorCode()).as(sql) + .isEqualTo(QueryExecutionPolicyException.DDL_PERMISSION_REQUIRED); + // MCP refuses every DROP outright, ahead of the permission check. + String mcpCode = sql.startsWith("DROP") + ? QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED + : QueryExecutionPolicyException.DDL_PERMISSION_REQUIRED; + assertThat(blocked(sql, QueryExecutionContext.mcp("dba", true, true)).getErrorCode()).as(sql) + .isEqualTo(mcpCode); + } + } + + @Test + void ddlCannotHideBehindAPreambleToSkipThePermissionCheck() { + // The DDL check reads the first statement. That is only sound because a run + // containing a mutation is limited to one statement; this pins that invariant. + lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(false); + for (String sql : List.of( + "SET @x = 1; ALTER TABLE customers MODIFY email varchar(512) NOT NULL", + "SELECT 1; ALTER TABLE customers MODIFY email varchar(512) NOT NULL", + "SET @x = 1; TRUNCATE TABLE customers")) { + QueryExecutionPolicyException e = blocked(sql, QueryExecutionContext.editor("dba", true, true)); + assertThat(e.getErrorCode()).as(sql).isIn( + QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED, + QueryExecutionPolicyException.DDL_PERMISSION_REQUIRED); + } + } + + @Test + void readOnlySurfacesCannotRunDdlAtAll() { + // API (dashboards) and chat are READ_ONLY_ONLY, so no permission check is needed there. + assertThat(blocked("ALTER TABLE customers ADD COLUMN x INT", QueryExecutionContext.api("admin", true)) + .getErrorCode()).isEqualTo(QueryExecutionPolicyException.EDITOR_MUTATION_FORBIDDEN); + assertThat(blocked("ALTER TABLE customers ADD COLUMN x INT", QueryExecutionContext.chat()) + .getErrorCode()).isEqualTo(QueryExecutionPolicyException.CHAT_MUTATION_BLOCKED); + } + + @Test + void dmlDoesNotNeedTheDdlPermission() { + lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(false); + QueryExecutionPolicyService.PolicyDecision decision = service.enforce( + new QueryRequest("UPDATE customers SET contact = NULL WHERE id = 7", null, null), + QueryExecutionContext.editor("dba", true, true), "mysql"); + assertThat(decision.mutating()).isTrue(); + } + + @Test + void aBlockingAlterIsRefusedBeforeTheConfirmationPrompt() { + // Unconfirmed: the refusal must come instead of the "are you sure?" dialog, not after it. + QueryExecutionPolicyException e = blocked( + "ALTER TABLE customers MODIFY email varchar(512) NOT NULL, ALGORITHM=COPY", + QueryExecutionContext.editor("admin", true, false)); + assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + assertThat(e.getMessage()).contains("gh-ost"); + } + + @Test + void theAlterConfirmationSaysHowItWillRun() { + QueryExecutionPolicyException e = blocked( + "ALTER TABLE customers MODIFY email varchar(512) NOT NULL", + QueryExecutionContext.editor("admin", true, false)); + assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.EDITOR_MUTATION_CONFIRMATION_REQUIRED); + assertThat(String.join(" ", e.getWarnings())).contains("LOCK=NONE").contains("5 seconds"); + } +} diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java index a766569e..c2c11106 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java @@ -1,6 +1,8 @@ package com.dbaagent.service; +import com.dbaagent.model.Permission; import com.dbaagent.model.QueryRequest; +import com.dbaagent.service.security.AccessControlService; import com.dbaagent.provider.DatabaseProviderRegistry; import com.dbaagent.provider.api.DatabaseDialect; import com.dbaagent.provider.api.QueryExecutionProvider; @@ -24,6 +26,7 @@ class QueryExecutionPolicyServiceTest { @Mock private DatabaseProviderRegistry providerRegistry; @Mock private DatabaseDialect databaseDialect; + @Mock private AccessControlService accessControlService; private QueryExecutionPolicyService service; @@ -38,7 +41,10 @@ void setUp() { when(providerRegistry.getDialect(anyString())).thenReturn(databaseDialect); when(databaseDialect.queryExecution()).thenReturn(realProvider); lenient().when(providerRegistry.getCanonicalName(anyString())).thenReturn("mysql"); - service = new QueryExecutionPolicyService(providerRegistry); + // Admins in these tests hold EXECUTE_DDL; the permission split itself is + // covered in QueryExecutionPolicyProdGuardTest. + lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(true); + service = new QueryExecutionPolicyService(providerRegistry, accessControlService); } @Test diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java new file mode 100644 index 00000000..149a8fa8 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java @@ -0,0 +1,199 @@ +package com.dbaagent.service; + +import com.dbaagent.model.ConnectionRequest; +import com.dbaagent.model.QueryRequest; +import com.dbaagent.provider.DatabaseProviderRegistry; +import com.dbaagent.provider.api.DatabaseDialect; +import com.dbaagent.provider.mysql.MySQLQueryExecutionProvider; +import com.dbaagent.repository.KeyColumnAnalysisRepository; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.InOrder; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.mockito.junit.jupiter.MockitoSettings; +import org.mockito.quality.Strictness; +import org.springframework.cache.CacheManager; + +import java.sql.Connection; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.ArgumentMatchers.startsWith; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.lenient; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Executor-side guardrails from a production incident, run against + * the real MySQL provider so the SQL asserted here is the SQL that ships. + */ +@ExtendWith(MockitoExtension.class) +// The executor calls Statement.execute several times per run (preambles, guards, the +// statement); strict stubbing would reject every call but the one a test stubs. +@MockitoSettings(strictness = Strictness.LENIENT) +class QueryExecutorGuardrailTest { + + private static final String GUARD = new MySQLQueryExecutionProvider().statementGuardSql(240); + + @Mock private ConnectionService connectionService; + @Mock private CredentialService credentialService; + @Mock private SchemaSnapshotService schemaSnapshotService; + @Mock private CacheManager cacheManager; + @Mock private CacheMetricsService cacheMetricsService; + @Mock private QueryLineageService queryLineageService; + @Mock private DatabaseProviderRegistry providerRegistry; + @Mock private DatabaseDialect dialect; + @Mock private QueryExecutionPolicyService queryExecutionPolicyService; + @Mock private UserDataAccessPolicyService userDataAccessPolicyService; + @Mock private com.dbaagent.service.telemetry.TelemetryCounters telemetryCounters; + @Mock private KeyColumnAnalysisRepository keyColumnAnalysisRepository; + @Mock private Connection connection; + @Mock private Statement statement; + + private QueryExecutorService executor; + + @BeforeEach + void setUp() throws Exception { + executor = new QueryExecutorService( + connectionService, credentialService, schemaSnapshotService, cacheManager, cacheMetricsService, + queryLineageService, providerRegistry, queryExecutionPolicyService, userDataAccessPolicyService, + telemetryCounters, keyColumnAnalysisRepository, new RunningQueryRegistry()); + + ConnectionRequest request = new ConnectionRequest(); + request.setDbType("mysql"); + when(credentialService.getDecryptedConnection("conn-1")).thenReturn(request); + lenient().when(providerRegistry.getCanonicalName("mysql")).thenReturn("mysql"); + when(providerRegistry.getDialect("mysql")).thenReturn(dialect); + when(dialect.queryExecution()).thenReturn(new MySQLQueryExecutionProvider()); + when(connectionService.getConnection("conn-1", request)).thenReturn(connection); + when(connection.createStatement()).thenReturn(statement); + // The session-pid probe is best effort; failing it here keeps it out of the way. + lenient().when(statement.executeQuery("SELECT CONNECTION_ID()")).thenThrow(new SQLException("no pid")); + lenient().when(userDataAccessPolicyService.redactResult(any(), any(), any())) + .thenAnswer(inv -> inv.getArgument(1)); + } + + private void policyReturns(boolean mutating, String queryType) { + when(queryExecutionPolicyService.enforce(any(), any(), eq("mysql"))) + .thenReturn(new QueryExecutionPolicyService.PolicyDecision(List.of(), mutating, queryType)); + } + + private QueryExecutionPolicyException runExpectingPolicyError(String sql) { + return assertThrows(QueryExecutionPolicyException.class, () -> executor.executeQuery( + "conn-1", new QueryRequest(sql, null, 240), QueryExecutionContext.editor("dba", true, true))); + } + + @Test + void guardsAreReappliedAfterTheUsersPreamblesAndBeforeTheStatement() throws Exception { + policyReturns(true, "UPDATE"); + + executor.executeQuery("conn-1", + new QueryRequest("SET SESSION lock_wait_timeout = 31536000; UPDATE customers SET contact = NULL WHERE id = 7", + null, 240), + QueryExecutionContext.editor("dba", true, true)); + + InOrder order = inOrder(statement); + order.verify(statement).execute("SET SESSION lock_wait_timeout = 31536000"); + order.verify(statement).execute(GUARD); + order.verify(statement).execute( + "/* deepsql origin=editor user=dba */ UPDATE customers SET contact = NULL WHERE id = 7"); + } + + @Test + void aStatementDoesNotRunWithoutItsGuards() throws Exception { + policyReturns(false, "SELECT"); + when(statement.execute(GUARD)).thenThrow(new SQLException("Unknown system variable", "HY000", 1193)); + + SQLException e = assertThrows(SQLException.class, () -> executor.executeQuery( + "conn-1", new QueryRequest("SELECT 1", null, 240), QueryExecutionContext.editor("dba", true, false))); + + assertThat(e.getMessage()).contains("safety settings"); + verify(statement, never()).execute(startsWith("/* deepsql")); + } + + @Test + void anAlterFallsBackFromInstantToInplaceWithoutLocks() throws Exception { + policyReturns(true, "ALTER"); + String alter = "ALTER TABLE customers MODIFY email varchar(512) NOT NULL"; + String tag = "/* deepsql origin=editor user=dba */ "; + when(statement.execute(tag + alter + "\n, ALGORITHM=INSTANT")).thenThrow(new SQLException( + "ALGORITHM=INSTANT is not supported. Reason: Need to rebuild the table. Try ALGORITHM=COPY/INPLACE.", + "0A000", 1846)); + + executor.executeQuery("conn-1", new QueryRequest(alter, null, 240), + QueryExecutionContext.editor("dba", true, true)); + + verify(statement).execute(tag + alter + "\n, ALGORITHM=INPLACE, LOCK=NONE"); + } + + @Test + void anAlterThatCannotRunOnlineChangesNothingAndSaysWhy() throws Exception { + policyReturns(true, "ALTER"); + when(statement.execute(startsWith("/* deepsql"))).thenThrow(new SQLException( + "ALGORITHM=INPLACE is not supported. Reason: Cannot change column type INPLACE. Try ALGORITHM=COPY.", + "0A000", 1846)); + + QueryExecutionPolicyException e = runExpectingPolicyError("ALTER TABLE customers MODIFY id BIGINT"); + + assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.DB_GUARDRAIL_TRIPPED); + assertThat(e.getMessage()).contains("gh-ost"); + } + + @Test + void aLockWaitTimeoutIsReportedAsTheGuardItIs() throws Exception { + policyReturns(true, "ALTER"); + when(statement.execute(startsWith("/* deepsql"))).thenThrow(new SQLException( + "Lock wait timeout exceeded; try restarting transaction", "HY000", 1205)); + + QueryExecutionPolicyException e = runExpectingPolicyError("ALTER TABLE customers ADD COLUMN x INT"); + + assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.DB_GUARDRAIL_TRIPPED); + assertThat(e.getMessage()).contains("5 seconds"); + // Not retried as INPLACE: a busy table is busy for every algorithm. + verify(statement, never()).execute(org.mockito.ArgumentMatchers.endsWith("LOCK=NONE")); + } + + @Test + void peopleAndAgentsGetAtMostTenThousandRowsWhateverTheyAskFor() { + assertThat(QueryExecutorService.effectiveRowLimit(null, QueryExecutionContext.editor("a", true, false))) + .isEqualTo(10_000); + assertThat(QueryExecutorService.effectiveRowLimit(50_000, QueryExecutionContext.mcp("a"))) + .isEqualTo(10_000); + assertThat(QueryExecutorService.effectiveRowLimit(1_000, QueryExecutionContext.editor("a", false, false))) + .isEqualTo(1_000); + assertThat(QueryExecutorService.effectiveRowLimit(null, QueryExecutionContext.api("a"))) + .isEqualTo(10_000); + // DeepSQL's own jobs keep what they ask for. + assertThat(QueryExecutorService.effectiveRowLimit(null, QueryExecutionContext.internal())).isZero(); + assertThat(QueryExecutorService.effectiveRowLimit(null, QueryExecutionContext.scheduled())).isZero(); + } + + @Test + void theActorTagCannotBeUsedToCloseTheComment() { + String tag = QueryExecutorService.actorTag(QueryExecutionContext.editor("x */ DROP TABLE t; /*", true, true)); + assertThat(tag.indexOf("*/")).isEqualTo(tag.length() - 3); + assertThat(tag).isEqualTo("/* deepsql origin=editor user=x____DROP_TABLE_t____ */ "); + } + + @Test + void readsAreUnaffectedBeyondTheTagAndTheCap() throws Exception { + policyReturns(false, "SELECT"); + when(statement.execute(anyString())).thenReturn(false); + + executor.executeQuery("conn-1", new QueryRequest("SELECT * FROM customers", null, 240), + QueryExecutionContext.mcp("agent")); + + verify(statement).setMaxRows(10_001); + verify(statement).execute("/* deepsql origin=mcp user=agent */ SELECT * FROM customers LIMIT 10001"); + } +} diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutorServiceTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutorServiceTest.java index a26d4790..b60a7b2d 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutorServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutorServiceTest.java @@ -40,6 +40,7 @@ class QueryExecutorServiceTest { @Mock private KeyColumnAnalysisRepository keyColumnAnalysisRepository; @Mock private Connection connection; @Mock private Statement statement; + @Mock private com.dbaagent.provider.api.DatabaseDialect dialect; private QueryExecutorService queryExecutorService; @@ -115,9 +116,14 @@ void executeQuery_writePrivilegeDenied_mapsToPolicyException() throws Exception when(providerRegistry.getCanonicalName("mysql")).thenReturn("mysql"); when(queryExecutionPolicyService.enforce(org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.any(), org.mockito.ArgumentMatchers.eq("mysql"))) .thenReturn(new QueryExecutionPolicyService.PolicyDecision(List.of(), true, "UPDATE")); + when(providerRegistry.getDialect("mysql")).thenReturn(dialect); + when(dialect.queryExecution()).thenReturn(new com.dbaagent.provider.mysql.MySQLQueryExecutionProvider()); when(connectionService.getConnection("conn-1", request)).thenReturn(connection); when(connection.createStatement()).thenReturn(statement); - when(statement.execute("UPDATE customers SET property_status = 'ACTIVE' WHERE customer_id = 9")) + when(statement.executeQuery("SELECT CONNECTION_ID()")).thenThrow(new SQLException("no pid")); + // Every statement now carries an actor tag; see QueryExecutorGuardrailTest. + org.mockito.Mockito.lenient().when(statement.execute("/* deepsql origin=editor user=admin */ " + + "UPDATE customers SET property_status = 'ACTIVE' WHERE customer_id = 9")) .thenThrow(new SQLException("UPDATE command denied to user", "42000")); QueryExecutionPolicyException exception = assertThrows( diff --git a/src/lib/permissions.js b/src/lib/permissions.js index 649f79c1..a5f72404 100644 --- a/src/lib/permissions.js +++ b/src/lib/permissions.js @@ -30,6 +30,7 @@ export const PERMISSIONS = { RUN_ANALYSIS: 'RUN_ANALYSIS', RUN_INGESTION: 'RUN_INGESTION', EXECUTE_PLAYBOOKS: 'EXECUTE_PLAYBOOKS', + EXECUTE_DDL: 'EXECUTE_DDL', USE_INDEX_ADVISOR: 'USE_INDEX_ADVISOR', MANAGE_ALERTS: 'MANAGE_ALERTS', From b6df624aeeae98f23475e66a138606542fd94b7f Mon Sep 17 00:00:00 2001 From: geekypunk Date: Sat, 7 Nov 2026 10:26:00 -0600 Subject: [PATCH 2/6] fix: close EXECUTE_DDL parser-differential gaps and MariaDB executable comments - Refuse MariaDB /*M! ... */ executable comments alongside MySQL /*! */ and /*+ */ hints (case-insensitive). - A mutation counts as row-only DML only when both its queryType and its real leading verb are row-only (or WITH). The keyword-fallback queryType sanitizes a prefix and can disagree with what the database runs; DO and Postgres EXPLAIN ANALYZE CREATE TABLE ... AS now require EXECUTE_DDL. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 11 +++++++---- .../service/QueryExecutionPolicyService.java | 17 ++++++++++++++--- .../QueryExecutionPolicyProdGuardTest.java | 18 +++++++++++++++++- 3 files changed, 38 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 9395db0f..dbd08b45 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -886,10 +886,13 @@ it against a real database — not a theoretical hardening pass. swallows it and the ALTER silently runs with the default (possibly blocking) algorithm. - **`EXECUTE_DDL` is separate from DML** and ADMIN-only by default (grant DBAs via an override/custom role). Confirmed DML is routine; DDL is not. - DDL is a leading DDL verb **or any mutation that is not row-only DML** (an allowlist: - INSERT/UPDATE/DELETE/MERGE/UPSERT/REPLACE) — a verb check alone missed `CALL` (a - procedure can run DDL) and Postgres `SELECT … INTO new_table`, and the keyword - fallback reports `RENAME` as `OTHER`. Index Advisor `APPLY` runs real `CREATE INDEX` + DDL is a leading DDL verb **or any mutation that is not row-only DML**. Row-only needs + both a row-only `queryType` (INSERT/UPDATE/DELETE/MERGE/UPSERT/REPLACE) **and** a + matching real leading verb (or `WITH`): the keyword-fallback `queryType` sanitizes a + prefix before matching, so it alone can disagree with what the database runs. A verb + check alone missed `CALL`, `DO`, Postgres `SELECT … INTO new_table` and + `EXPLAIN ANALYZE CREATE TABLE … AS` (Postgres executes it); the keyword fallback also + reports `RENAME` as `OTHER`. Executable comments include MariaDB's `/*M! … */`. Index Advisor `APPLY` runs real `CREATE INDEX` outside the policy, so it checks the same permission; without that it is decorative. - **MySQL executable comments and optimizer hints are refused** (`/*! … */`, `/*+ … */`). MySQL runs the body of `/*!50000 GRANT … */`, while every check here strips it as a diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java index 87bbad56..99b66c0b 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java @@ -114,12 +114,21 @@ public class QueryExecutionPolicyService { // /*+ ... */ optimizer hints (MAX_EXECUTION_TIME, SET_VAR) override the session // guards for one statement. Matched on the raw text: a literal containing "/*!" is // refused too, which is the right side to be wrong on. - private static final Pattern EXECUTABLE_COMMENT_PATTERN = Pattern.compile("/\\*[!+]"); + // MariaDB's /*M! ... */ is executable there and inert on MySQL; a "mysql" connection + // can point at either. \s* over-blocks `/* !`, which is harmless. + private static final Pattern EXECUTABLE_COMMENT_PATTERN = + Pattern.compile("/\\*\\s*(?:M?!|\\+)", Pattern.CASE_INSENSITIVE); // Mutations that only change rows. Anything else that mutates — DDL, CALL/DO (a // procedure can run DDL), SELECT ... INTO a new table — needs EXECUTE_DDL. An // allowlist, so a statement shape nobody anticipated lands on the stricter side. private static final java.util.Set ROW_ONLY_MUTATIONS = java.util.Set.of("INSERT", "UPDATE", "DELETE", "MERGE", "UPSERT", "REPLACE"); + // The statement's real leading verb must agree. queryType alone can come from the + // keyword fallback, which sanitizes a prefix before matching, so it can disagree with + // what the database executes. WITH covers data-modifying CTEs. EXPLAIN is absent on + // purpose: Postgres runs `EXPLAIN ANALYZE CREATE TABLE ... AS` for real. + private static final Pattern ROW_ONLY_LEADING_VERB = Pattern.compile( + "^(?:INSERT|UPDATE|DELETE|MERGE|UPSERT|REPLACE|WITH)\\b", Pattern.CASE_INSENSITIVE); private final DatabaseProviderRegistry providerRegistry; private final AccessControlService accessControlService; @@ -300,10 +309,12 @@ private boolean isAdministrativeStatement(String statement) { * table; the keyword fallback also reports RENAME as "OTHER". */ private boolean isDdlStatement(String statement, String queryType) { - if (DDL_VERB_PATTERN.matcher(stripLeadingComments(statement)).find()) { + String head = stripLeadingComments(statement); + if (DDL_VERB_PATTERN.matcher(head).find()) { return true; } - return queryType == null || !ROW_ONLY_MUTATIONS.contains(queryType.toUpperCase(Locale.ROOT)); + boolean rowOnlyType = queryType != null && ROW_ONLY_MUTATIONS.contains(queryType.toUpperCase(Locale.ROOT)); + return !(rowOnlyType && ROW_ONLY_LEADING_VERB.matcher(head).find()); } private boolean isTruncate(String queryType, String statement) { diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java index c6444409..ff549621 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java @@ -89,6 +89,9 @@ void executableCommentsAndOptimizerHintsAreRefused() { "/*!50000 GRANT ALL ON *.* TO 'x'@'%' */", "/*! ALTER TABLE customers ADD COLUMN x INT */", "/*!SET GLOBAL max_connections = 10*/", + // MariaDB's form: MySQL ignores it, MariaDB runs it. + "/*M!100000 GRANT ALL ON *.* TO 'x'@'%' */", + "/*m!100000 DROP TABLE customers */", "SELECT /*+ MAX_EXECUTION_TIME(600000) */ * FROM customers", "UPDATE /*+ SET_VAR(innodb_lock_wait_timeout=31536000) */ customers SET a = 1 WHERE id = 2")) { assertThat(blocked(sql, QueryExecutionContext.editor("analyst", false, false)).getErrorCode()) @@ -117,7 +120,10 @@ void schemaChangesThatDoNotStartWithADdlVerbStillNeedThePermission() { lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(false); for (String sql : List.of( "CALL rebuild_customer_indexes()", - "SELECT * INTO customers_copy FROM customers")) { + "SELECT * INTO customers_copy FROM customers", + // Postgres runs the wrapped statement under EXPLAIN ANALYZE: this creates a table. + "EXPLAIN ANALYZE CREATE TABLE customers_copy AS SELECT * FROM customers", + "DO RELEASE_LOCK('x')")) { assertThat(blocked(sql, QueryExecutionContext.editor("dba", true, true)).getErrorCode()) .as(sql).isEqualTo(QueryExecutionPolicyException.DDL_PERMISSION_REQUIRED); } @@ -201,6 +207,16 @@ void readOnlySurfacesCannotRunDdlAtAll() { .getErrorCode()).isEqualTo(QueryExecutionPolicyException.CHAT_MUTATION_BLOCKED); } + @Test + void aDataModifyingCteIsStillRowOnly() { + lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(false); + QueryExecutionPolicyService.PolicyDecision decision = service.enforce( + new QueryRequest("WITH stale AS (SELECT id FROM customers WHERE active = 0) " + + "DELETE FROM customers WHERE id IN (SELECT id FROM stale)", null, null), + QueryExecutionContext.editor("dba", true, true), "mysql"); + assertThat(decision.mutating()).isTrue(); + } + @Test void dmlDoesNotNeedTheDdlPermission() { lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(false); From a6c984b11607feb0be36e136cd12b37a076f7c79 Mon Sep 17 00:00:00 2001 From: geekypunk Date: Sat, 7 Nov 2026 10:58:51 -0600 Subject: [PATCH 3/6] fix: address code review on session guard lifecycle and DDL edge cases - Reset session guards to the pool baseline after every run (finally), so a per-run max_execution_time, sql_safe_updates or a user's SET no longer leaks to the next borrower, background jobs included. - Apply the guards before preambles as well as after, and tag preambles, so a heavy SELECT placed first in a read-only batch is still bounded. - Omit max_execution_time on MariaDB (served by the MySQL provider), where the fail-closed SET refused every query. - Postgres CREATE/DROP INDEX CONCURRENTLY runs with lock_timeout = 0, so it is not cut off midway leaving an INVALID index. - Fall back from INSTANT on servers that reject it as a syntax error (MySQL 5.7, Aurora MySQL 2); refuse partition ALTERs without an explicit clause. - Server-side row cap via setMaxRows only; LIMIT text is appended only when the client asked for a limit (FETCH FIRST / FOR UPDATE broke). - Data-modifying CTEs stay row-only DML; clearer read time-limit message. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 23 +++++- .../provider/api/QueryExecutionProvider.java | 20 ++++- .../mysql/MySQLQueryExecutionProvider.java | 43 +++++++++-- .../IndexRecommendationApplyService.java | 19 ++++- .../service/QueryExecutionPolicyService.java | 8 +- .../service/QueryExecutorService.java | 77 +++++++++++++++---- .../MySQLQueryExecutionProviderGuardTest.java | 45 ++++++++++- .../QueryExecutionPolicyProdGuardTest.java | 10 +++ .../service/QueryExecutorGuardrailTest.java | 45 ++++++++++- 9 files changed, 251 insertions(+), 39 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index dbd08b45..55ad89d8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -872,10 +872,22 @@ it against a real database — not a theoretical hardening pass. - **Session guards** come from the dialect (`QueryExecutionProvider.connectionGuardSql` / `statementGuardSql`). The pool sets `lock_wait_timeout=5, innodb_lock_wait_timeout=5` (Postgres: `lock_timeout='5s'`) via Hikari `connectionInitSql`, so background jobs get - it too. The executor **re-applies** them, plus `sql_safe_updates=1` and - `max_execution_time`, *after* the user's SET preambles — otherwise a preamble could - loosen them, and on a pooled connection that loosening leaks to the next borrower. - It fails closed: no guards, no statement. + it too. The executor applies the stricter per-run set (`sql_safe_updates=1`, + `max_execution_time`) **before** the preambles (a read-only batch may put a heavy + SELECT first), **again after** them (a preamble could loosen them), and **resets** to + the pool baseline in a `finally` (`statementGuardResetSql`): session variables outlive + the borrow, and without the reset a 5s Editor timeout or `sql_safe_updates` reached + background jobs on the same pool. Fails closed on apply; the reset is best effort, + since what it leaves behind is stricter, not looser. Preambles carry the actor tag too. + - **The MySQL provider also serves `mariadb`**, which has no `max_execution_time` — a + fail-closed SET of it refused every query on the connection. The guard takes the + server version (`DatabaseMetaData`) and omits it there. + - **Postgres `CREATE INDEX CONCURRENTLY` takes `lock_timeout = 0`** for its own run + (Index Advisor apply): its waits for older transactions honour `lock_timeout`, and + hitting 5s midway leaves an INVALID index. Its locks block neither reads nor writes. + - **The 10k cap is `setMaxRows` only.** `LIMIT` text is still appended only when the + client asked for a limit; appending it to every SELECT broke `FETCH FIRST`, + `FOR UPDATE` and `LOCK IN SHARE MODE`. - **`max_execution_time` keys off `@@global.read_only`**: reads get the client timeout on a replica but at most 30s on a writable server. No per-connection setting to drift. - **MySQL ALTER runs online or not at all.** `onlineDdlAttempts` appends @@ -884,6 +896,9 @@ it against a real database — not a theoretical hardening pass. `COPY`, `DEFAULT`, `SHARED`, `EXCLUSIVE` are refused *before* the confirmation prompt. The clause is appended on a **new line**: on the same line a trailing `-- comment` swallows it and the ALTER silently runs with the default (possibly blocking) algorithm. + Servers that predate INSTANT (MySQL 5.7, Aurora MySQL 2) reject it as `1064 … near + 'INSTANT'`, which also triggers the fallback. A partition ALTER with no explicit + clause is refused: partition options must come last, so an appended clause is invalid. - **`EXECUTE_DDL` is separate from DML** and ADMIN-only by default (grant DBAs via an override/custom role). Confirmed DML is routine; DDL is not. DDL is a leading DDL verb **or any mutation that is not row-only DML**. Row-only needs diff --git a/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java index 499cc4ef..83669632 100644 --- a/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java @@ -75,13 +75,25 @@ default String connectionGuardSql() { } /** - * Session settings re-applied immediately before each user or agent statement, - * after any SET preambles the user sent, so a preamble cannot loosen them. A - * superset of {@link #connectionGuardSql()}. + * Session settings applied before each user or agent run, and again after any SET + * preambles the user sent, so a preamble cannot loosen them. A superset of + * {@link #connectionGuardSql()}. * * @param timeoutSeconds the client-side timeout for this statement; 0 when unset + * @param serverVersion {@code DatabaseMetaData.getDatabaseProductVersion()}, or null + * when unknown — a dialect alias can point at a different + * server (MySQL's covers MariaDB) with different variables */ - default String statementGuardSql(int timeoutSeconds) { + default String statementGuardSql(int timeoutSeconds, String serverVersion) { + return connectionGuardSql(); + } + + /** + * Restores the pooled connection to its {@link #connectionGuardSql()} baseline after a + * run. Session variables outlive the borrow: without this, a per-statement limit or a + * user's own SET would carry over to the next borrower, background jobs included. + */ + default String statementGuardResetSql(String serverVersion) { return connectionGuardSql(); } diff --git a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java index f52b194b..87520321 100644 --- a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java @@ -153,6 +153,7 @@ public String getSessionPidQuery() { "\\bALGORITHM\\s*=?\\s*(DEFAULT|INSTANT|INPLACE|COPY)\\b", Pattern.CASE_INSENSITIVE); private static final Pattern LOCK_CLAUSE = Pattern.compile( "\\bLOCK\\s*=?\\s*(DEFAULT|NONE|SHARED|EXCLUSIVE)\\b", Pattern.CASE_INSENSITIVE); + private static final Pattern PARTITION_CLAUSE = Pattern.compile("\\bPARTITION(?:S|ING)?\\b", Pattern.CASE_INSENSITIVE); private static final Pattern QUOTED = Pattern.compile( "'(?:[^'\\\\]|\\\\.|'')*'|\"(?:[^\"\\\\]|\\\\.)*\"|`[^`]*`"); @@ -161,6 +162,7 @@ public String getSessionPidQuery() { private static final int ER_QUERY_TIMEOUT = 3024; private static final int ER_ALTER_OPERATION_NOT_SUPPORTED = 1845; private static final int ER_ALTER_OPERATION_NOT_SUPPORTED_REASON = 1846; + private static final int ER_PARSE_ERROR = 1064; @Override public String connectionGuardSql() { @@ -175,12 +177,29 @@ public String connectionGuardSql() { * and DDL are bounded by the lock waits and the JDBC timeout. */ @Override - public String statementGuardSql(int timeoutSeconds) { + public String statementGuardSql(int timeoutSeconds, String serverVersion) { + String guard = "SET SESSION " + LOCK_GUARD + ", sql_safe_updates = 1"; + if (isMariaDb(serverVersion)) { + // MariaDB has no max_execution_time (the SET would fail and, fail-closed, block + // every query). Its max_statement_time also stops writes and DDL mid-flight, so + // reads there stay bounded by the JDBC timeout alone. + return guard; + } int clientMs = timeoutSeconds > 0 ? timeoutSeconds * 1000 : PRIMARY_READ_LIMIT_MS; - return "SET SESSION " + LOCK_GUARD + ", sql_safe_updates = 1, max_execution_time = IF(@@global.read_only, " + return guard + ", max_execution_time = IF(@@global.read_only, " + clientMs + ", LEAST(" + clientMs + ", " + PRIMARY_READ_LIMIT_MS + "))"; } + @Override + public String statementGuardResetSql(String serverVersion) { + return "SET SESSION " + LOCK_GUARD + ", sql_safe_updates = DEFAULT" + + (isMariaDb(serverVersion) ? "" : ", max_execution_time = DEFAULT"); + } + + private static boolean isMariaDb(String serverVersion) { + return serverVersion != null && serverVersion.toLowerCase(java.util.Locale.ROOT).contains("mariadb"); + } + @Override public List onlineDdlAttempts(String sql) { if (sql == null || !ALTER_TABLE.matcher(stripLeadingComments(sql)).find()) { @@ -203,6 +222,14 @@ public List onlineDdlAttempts(String sql) { } // The appended clause starts on a new line: a trailing "-- comment" would // otherwise swallow it, and the ALTER would run with MySQL's default algorithm. + if (algorithm == null && lock == null && PARTITION_CLAUSE.matcher(clauses).find()) { + // Partition options must come last in an ALTER, so an appended clause is a + // syntax error. Ask for the clause up front instead of guessing where it goes. + throw new IllegalArgumentException( + "DeepSQL cannot check a partition change for online safety by itself. State it explicitly at the " + + "start, e.g. ALTER TABLE t ALGORITHM=INPLACE, LOCK=NONE, ADD PARTITION (...), or run it off-peak " + + "with gh-ost or pt-online-schema-change."); + } if (algorithm == null && lock == null) { return List.of(base + "\n, ALGORITHM=INSTANT", base + "\n, ALGORITHM=INPLACE, LOCK=NONE"); } @@ -220,10 +247,14 @@ private static String clauseValue(Pattern clause, String sql) { @Override public boolean isOnlineDdlUnsupported(SQLException e) { int code = e.getErrorCode(); + String message = e.getMessage() == null ? "" : e.getMessage(); return code == ER_ALTER_OPERATION_NOT_SUPPORTED || code == ER_ALTER_OPERATION_NOT_SUPPORTED_REASON // Later InnoDB limits (row versions, max row size) word it the same way. - || (e.getMessage() != null && e.getMessage().contains("Try ALGORITHM=")); + || message.contains("Try ALGORITHM=") + // MySQL 5.7 / Aurora MySQL 2 / older MariaDB do not know INSTANT at all and + // reject it as a syntax error pointing at the word we appended. + || (code == ER_PARSE_ERROR && message.contains("near 'INSTANT")); } @Override @@ -238,8 +269,10 @@ public String guardrailMessage(SQLException e) { + "waiting statement makes every later write to the table wait too. Try again when it is quieter."; case ER_UPDATE_WITHOUT_KEY_IN_SAFE_MODE -> "MySQL safe-updates mode refused this: the WHERE clause must " + "filter on an indexed (key) column, or the statement must have a LIMIT."; - case ER_QUERY_TIMEOUT -> "Stopped at the " + PRIMARY_READ_LIMIT_MS / 1000 + "-second limit for reads on a " - + "primary database. Run heavy queries on the replica connection instead."; + // Could be the 30s primary cap or the query's own timeout; the error does not say which. + case ER_QUERY_TIMEOUT -> "Stopped at the time limit for reads: your query timeout, and at most " + + PRIMARY_READ_LIMIT_MS / 1000 + " seconds on a primary database. Narrow the query, or run heavy " + + "reads on a replica connection."; default -> null; }; } diff --git a/backend/src/main/java/com/dbaagent/service/IndexRecommendationApplyService.java b/backend/src/main/java/com/dbaagent/service/IndexRecommendationApplyService.java index 9fb23976..fbbd38d4 100644 --- a/backend/src/main/java/com/dbaagent/service/IndexRecommendationApplyService.java +++ b/backend/src/main/java/com/dbaagent/service/IndexRecommendationApplyService.java @@ -194,7 +194,24 @@ public ApplyResult apply(String recommendationId, Mode mode, boolean confirmed, } else { try (Statement s = c.createStatement()) { log.info("Applying recommendation {} on {}: {}", rec.getId(), rec.getConnectionId(), ddl); - s.execute(ddl); + // CREATE/DROP INDEX CONCURRENTLY waits for every older transaction, + // and those waits honour the pool's 5s lock_timeout: hitting it midway + // leaves an INVALID index behind. Its locks do not block reads or + // writes, so waiting is safe here; restore the pool guard afterwards. + boolean concurrently = ddl.toUpperCase(java.util.Locale.ROOT).contains(" CONCURRENTLY "); + if (concurrently) { + s.execute("SET lock_timeout = 0"); + } + try { + s.execute(ddl); + } finally { + String guard = concurrently + ? providerRegistry.getDialect(req.getDbType()).queryExecution().connectionGuardSql() + : null; + if (guard != null) { + s.execute(guard); + } + } } } diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java index 99b66c0b..4ac260a6 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java @@ -121,8 +121,10 @@ public class QueryExecutionPolicyService { // Mutations that only change rows. Anything else that mutates — DDL, CALL/DO (a // procedure can run DDL), SELECT ... INTO a new table — needs EXECUTE_DDL. An // allowlist, so a statement shape nobody anticipated lands on the stricter side. + // "WRITE" is the text backstop's label for a CTE matching AS ( INSERT|UPDATE|DELETE|MERGE, + // which is always a row write; the leading-verb check below still has to agree. private static final java.util.Set ROW_ONLY_MUTATIONS = - java.util.Set.of("INSERT", "UPDATE", "DELETE", "MERGE", "UPSERT", "REPLACE"); + java.util.Set.of("INSERT", "UPDATE", "DELETE", "MERGE", "UPSERT", "REPLACE", "WRITE"); // The statement's real leading verb must agree. queryType alone can come from the // keyword fallback, which sanitizes a prefix before matching, so it can disagree with // what the database executes. WITH covers data-modifying CTEs. EXPLAIN is absent on @@ -313,7 +315,9 @@ private boolean isDdlStatement(String statement, String queryType) { if (DDL_VERB_PATTERN.matcher(head).find()) { return true; } - boolean rowOnlyType = queryType != null && ROW_ONLY_MUTATIONS.contains(queryType.toUpperCase(Locale.ROOT)); + // Data-modifying CTEs are labelled "DELETE (in CTE)" etc.; compare the verb. + String verb = queryType == null ? "" : queryType.trim().split("[\\s(]", 2)[0].toUpperCase(Locale.ROOT); + boolean rowOnlyType = ROW_ONLY_MUTATIONS.contains(verb); return !(rowOnlyType && ROW_ONLY_LEADING_VERB.matcher(head).find()); } diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java index c416ba73..56e834ce 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java @@ -661,6 +661,19 @@ public QueryResult executeQuery( runningQueryRegistry.register( queryRequest.getExecutionId(), connectionId, sessionPid, executionContext.actorUsername()); + // Session guardrails wrap the whole run: applied before the preambles (a + // read-only batch may put a heavy SELECT first), re-applied after them (so a + // `SET SESSION lock_wait_timeout = ...` cannot loosen them), and reset to the + // pool baseline in the finally below, because session variables outlive the + // borrow and would otherwise reach the next borrower, background jobs included. + // Fails closed: nothing runs without them. + QueryExecutionProvider executionProvider = providerRegistry.getDialect(connRequest.getDbType()).queryExecution(); + String serverVersion = serverVersion(connection); + String guardSql = executionProvider.statementGuardSql(effectiveTimeout, serverVersion); + String tag = actorTag(executionContext); + applyGuard(connection, guardSql); + try { + // Execute all preamble statements (SET @var, USE db, etc.) on the same connection // so that session variables are in scope for the final SELECT. for (int si = 0; si < statements.size() - 1; si++) { @@ -668,23 +681,11 @@ public QueryResult executeQuery( if (preamble == null || preamble.isBlank()) continue; try (Statement preambleStmt = connection.createStatement()) { if (effectiveTimeout > 0) preambleStmt.setQueryTimeout(effectiveTimeout); - preambleStmt.execute(preamble); + preambleStmt.execute(tag + preamble); } } - - // Re-assert the session guardrails after the user's preambles, so a - // `SET SESSION lock_wait_timeout = ...` in front of the query cannot loosen - // them, and so a value one caller changed never leaks to the next borrower - // of this pooled connection. Fails closed: a statement does not run without them. - QueryExecutionProvider executionProvider = providerRegistry.getDialect(connRequest.getDbType()).queryExecution(); - String guardSql = executionProvider.statementGuardSql(effectiveTimeout); - if (guardSql != null) { - try (Statement guardStmt = connection.createStatement()) { - guardStmt.execute(guardSql); - } catch (SQLException e) { - throw new SQLException("Could not apply DeepSQL's database safety settings, so the query was not run: " - + e.getMessage(), e.getSQLState(), e.getErrorCode(), e); - } + if (statements.size() > 1) { + applyGuard(connection, guardSql); } // The last (or only) statement is the query whose results we return @@ -714,7 +715,11 @@ public QueryResult executeQuery( String queryLower = queryTrimmed.toLowerCase(); boolean isSelect = queryLower.startsWith("select"); boolean alreadyHasLimit = queryLower.matches("(?s).*\\blimit\\s+\\d+.*"); - if (isSelect && !alreadyHasLimit) { + // Text append only when the client asked for a limit, as before. The + // server-side cap is enforced by setMaxRows alone: appending to every + // SELECT broke FETCH FIRST, FOR UPDATE and LOCK IN SHARE MODE. + boolean clientAskedForLimit = queryRequest.getLimit() != null && queryRequest.getLimit() > 0; + if (isSelect && !alreadyHasLimit && clientAskedForLimit) { baseQuery = queryTrimmed; if (baseQuery.endsWith(";")) { baseQuery = baseQuery.substring(0, baseQuery.length() - 1); @@ -739,7 +744,6 @@ public QueryResult executeQuery( throw QueryExecutionPolicyException.unsafeMutation( blocking.getMessage(), policyDecision.primaryQueryType()); } - String tag = actorTag(executionContext); boolean hasResultSet = false; for (int attempt = 0; attempt < attempts.size(); attempt++) { try { @@ -832,6 +836,9 @@ public QueryResult executeQuery( result.setRows(Arrays.asList(Arrays.asList(updateCount))); } } + } finally { + resetGuard(connection, executionProvider.statementGuardResetSql(serverVersion), connectionId); + } } finally { runningQueryRegistry.unregister(queryRequest.getExecutionId()); } @@ -846,6 +853,42 @@ public QueryResult executeQuery( return userDataAccessPolicyService.redactResult(connectionId, result, executionContext); } + private static void applyGuard(Connection connection, String guardSql) throws SQLException { + if (guardSql == null) { + return; + } + try (Statement guardStmt = connection.createStatement()) { + guardStmt.execute(guardSql); + } catch (SQLException e) { + throw new SQLException("Could not apply DeepSQL's database safety settings, so the query was not run: " + + e.getMessage(), e.getSQLState(), e.getErrorCode(), e); + } + } + + /** + * Best effort: a failed reset leaves the per-statement settings in place, which are + * stricter than the pool baseline, not looser — so it is logged, not thrown, rather + * than masking the query's own result or error. + */ + private void resetGuard(Connection connection, String resetSql, String connectionId) { + if (resetSql == null) { + return; + } + try (Statement resetStmt = connection.createStatement()) { + resetStmt.execute(resetSql); + } catch (SQLException | RuntimeException e) { + log.warn("Could not reset session guardrails on connection {}: {}", connectionId, e.getMessage()); + } + } + + private static String serverVersion(Connection connection) { + try { + return connection.getMetaData().getDatabaseProductVersion(); + } catch (SQLException | RuntimeException e) { + return null; + } + } + /** Hard ceiling on rows returned to a person or agent, whatever the request asks for. */ static final int MAX_RESULT_ROWS = 10_000; diff --git a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java index 17fb9fd8..66e18da6 100644 --- a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java +++ b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java @@ -105,10 +105,51 @@ void everyPooledConnectionGivesUpOnLocksAfterFiveSeconds() { @Test void perStatementGuardCapsReadsAtThirtySecondsOnlyOnAWritableServer() { - assertThat(provider.statementGuardSql(240)).isEqualTo( + assertThat(provider.statementGuardSql(240, "8.0.36")).isEqualTo( "SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5, sql_safe_updates = 1, " + "max_execution_time = IF(@@global.read_only, 240000, LEAST(240000, 30000))"); - assertThat(provider.statementGuardSql(0)).contains("IF(@@global.read_only, 30000, LEAST(30000, 30000))"); + assertThat(provider.statementGuardSql(0, null)).contains("IF(@@global.read_only, 30000, LEAST(30000, 30000))"); + } + + @Test + void mariaDbGetsNoMaxExecutionTimeBecauseItDoesNotHaveOne() { + // The MySQL provider also serves "mariadb" connections. An unknown variable fails the + // SET, and the executor fails closed — every query on the connection would be refused. + assertThat(provider.statementGuardSql(240, "10.11.6-MariaDB-log")).isEqualTo( + "SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5, sql_safe_updates = 1"); + assertThat(provider.statementGuardResetSql("10.11.6-MariaDB-log")).doesNotContain("max_execution_time"); + } + + @Test + void theResetRestoresThePoolBaselineSoNothingLeaksToTheNextBorrower() { + assertThat(provider.statementGuardResetSql("8.0.36")).isEqualTo( + "SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5, " + + "sql_safe_updates = DEFAULT, max_execution_time = DEFAULT"); + } + + @Test + void serversWithoutInstantFallBackToInplace() { + // MySQL 5.7 / Aurora MySQL 2 reject INSTANT as a syntax error at the word we appended. + assertThat(provider.isOnlineDdlUnsupported(new SQLException( + "You have an error in your SQL syntax; check the manual ... near 'INSTANT' at line 2", "42000", 1064))) + .isTrue(); + // A genuine syntax error elsewhere is not an excuse to retry. + assertThat(provider.isOnlineDdlUnsupported(new SQLException( + "You have an error in your SQL syntax; ... near 'COLUMNN c INT' at line 1", "42000", 1064))) + .isFalse(); + } + + @Test + void aPartitionChangeMustStateItsAlgorithmExplicitly() { + // Partition options come last in an ALTER, so an appended clause is a syntax error. + IllegalArgumentException e = assertThrows(IllegalArgumentException.class, + () -> provider.onlineDdlAttempts("ALTER TABLE t ADD PARTITION (PARTITION p3 VALUES LESS THAN (2030))")); + assertThat(e.getMessage()).contains("ALGORITHM=INPLACE, LOCK=NONE"); + assertThat(provider.onlineDdlAttempts( + "ALTER TABLE t ALGORITHM=INPLACE, LOCK=NONE, ADD PARTITION (PARTITION p3 VALUES LESS THAN (2030))")) + .hasSize(1); + // A column merely named like it is not a partition clause. + assertThat(provider.onlineDdlAttempts("ALTER TABLE t ADD COLUMN partition_key INT")).hasSize(2); } // ---- errors -------------------------------------------------------------- diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java index ff549621..52f9ef59 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java @@ -217,6 +217,16 @@ void aDataModifyingCteIsStillRowOnly() { assertThat(decision.mutating()).isTrue(); } + @Test + void aWriteInsideACteIsStillRowOnly() { + // Classified as "DELETE (in CTE)"; the label must not read as a schema change. + lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(false); + QueryExecutionPolicyService.PolicyDecision decision = service.enforce( + new QueryRequest("WITH d AS (DELETE FROM customers WHERE id = 5 RETURNING *) SELECT * FROM d", null, null), + QueryExecutionContext.editor("dba", true, true), "mysql"); + assertThat(decision.mutating()).isTrue(); + } + @Test void dmlDoesNotNeedTheDdlPermission() { lenient().when(accessControlService.hasPermission(Permission.EXECUTE_DDL)).thenReturn(false); diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java index 149a8fa8..f2826e78 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java @@ -43,7 +43,9 @@ @MockitoSettings(strictness = Strictness.LENIENT) class QueryExecutorGuardrailTest { - private static final String GUARD = new MySQLQueryExecutionProvider().statementGuardSql(240); + // The mocked Connection has no metadata, so the server version is unknown (null). + private static final String GUARD = new MySQLQueryExecutionProvider().statementGuardSql(240, null); + private static final String RESET = new MySQLQueryExecutionProvider().statementGuardResetSql(null); @Mock private ConnectionService connectionService; @Mock private CredentialService credentialService; @@ -102,11 +104,44 @@ void guardsAreReappliedAfterTheUsersPreamblesAndBeforeTheStatement() throws Exce null, 240), QueryExecutionContext.editor("dba", true, true)); + String tag = "/* deepsql origin=editor user=dba */ "; + InOrder order = inOrder(statement); + order.verify(statement).execute(GUARD); + order.verify(statement).execute(tag + "SET SESSION lock_wait_timeout = 31536000"); + order.verify(statement).execute(GUARD); + order.verify(statement).execute(tag + "UPDATE customers SET contact = NULL WHERE id = 7"); + order.verify(statement).execute(RESET); + } + + @Test + void aHeavyReadPlacedFirstInABatchStillRunsUnderTheGuards() throws Exception { + // A read-only batch may hold several SELECTs; the first used to run as a preamble + // with no read time limit and no actor tag. + policyReturns(false, "SELECT"); + + executor.executeQuery("conn-1", + new QueryRequest("SELECT * FROM bookings b JOIN payments p ON p.booking_id = b.id; SELECT 1", null, 240), + QueryExecutionContext.editor("dba", true, false)); + InOrder order = inOrder(statement); - order.verify(statement).execute("SET SESSION lock_wait_timeout = 31536000"); order.verify(statement).execute(GUARD); order.verify(statement).execute( - "/* deepsql origin=editor user=dba */ UPDATE customers SET contact = NULL WHERE id = 7"); + "/* deepsql origin=editor user=dba */ SELECT * FROM bookings b JOIN payments p ON p.booking_id = b.id"); + } + + @Test + void theSessionIsResetEvenWhenTheStatementFails() throws Exception { + // Session variables outlive the borrow: without the reset, this run's + // sql_safe_updates and max_execution_time would reach the next borrower, + // background jobs on the same pool included. + policyReturns(true, "UPDATE"); + when(statement.execute(startsWith("/* deepsql"))).thenThrow(new SQLException("Duplicate entry", "23000", 1062)); + + assertThrows(SQLException.class, () -> executor.executeQuery("conn-1", + new QueryRequest("UPDATE customers SET email = 'a@b.c' WHERE id = 7", null, 240), + QueryExecutionContext.editor("dba", true, true))); + + verify(statement).execute(RESET); } @Test @@ -193,7 +228,9 @@ void readsAreUnaffectedBeyondTheTagAndTheCap() throws Exception { executor.executeQuery("conn-1", new QueryRequest("SELECT * FROM customers", null, 240), QueryExecutionContext.mcp("agent")); + // The cap is setMaxRows only. Appending LIMIT text to a request that asked for no + // limit broke FETCH FIRST, FOR UPDATE and LOCK IN SHARE MODE. verify(statement).setMaxRows(10_001); - verify(statement).execute("/* deepsql origin=mcp user=agent */ SELECT * FROM customers LIMIT 10001"); + verify(statement).execute("/* deepsql origin=mcp user=agent */ SELECT * FROM customers"); } } From 2f5db431594bbb0f88d90f0922ca327cf3be7d64 Mon Sep 17 00:00:00 2001 From: geekypunk Date: Sat, 7 Nov 2026 12:08:31 -0600 Subject: [PATCH 4/6] fix: second review round on SQL guardrails Security: - Block Postgres role/identity switches (SET SESSION|LOCAL ROLE, SET SESSION AUTHORIZATION, SET role = ..., session_replication_role). - Run the dangerous-function denylist on every surface; it only ran for MCP and dashboards, so the Editor ran dblink_exec / pg_read_file as plain reads. Add pg_terminate_backend and pg_cancel_backend (Java and MCP JS lists). - Make McpSqlGuardService comment stripping linear: its lazy block-comment regex was quadratic on an unterminated /* (minutes of CPU per request on the MCP and public dashboard query paths). Same fix for the MySQL ALTER clause tokenizer, which now also ignores clauses inside comments. Correctness: - Postgres CONCURRENTLY index builds run with lock_timeout = 0 on every path via QueryExecutionProvider.ddlSessionSql (no INVALID indexes; no dialect SQL in IndexRecommendationApplyService). - Broader reset after each run: Postgres RESET ALL; MySQL also restores sql_mode, foreign_key_checks, unique_checks, time_zone. - Aurora readers detected via innodb_read_only. - Row cap: 10k default, explicit requests up to 100k (CSV export). - INTERNAL/SCHEDULED runs keep the pool baseline only. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 40 ++++++++++++--- .../provider/api/QueryExecutionProvider.java | 10 ++++ .../mysql/MySQLQueryExecutionProvider.java | 27 ++++++++-- .../PostgresQueryExecutionProvider.java | 18 +++++++ .../IndexRecommendationApplyService.java | 23 ++++----- .../dbaagent/service/McpSqlGuardService.java | 47 ++++++++++++++--- .../service/QueryExecutionPolicyService.java | 50 +++++++++---------- .../service/QueryExecutorService.java | 45 +++++++++++++---- .../MySQLQueryExecutionProviderGuardTest.java | 36 +++++++++++-- ...stgresQueryExecutionProviderGuardTest.java | 40 +++++++++++++++ .../service/McpSqlGuardServiceTest.java | 20 ++++++++ .../QueryExecutionPolicyProdGuardTest.java | 28 ++++++++++- .../service/QueryExecutorGuardrailTest.java | 24 ++++++++- mcp/CLAUDE.md | 2 +- mcp/deepsql-phase1-lib.js | 1 + 15 files changed, 340 insertions(+), 71 deletions(-) create mode 100644 backend/src/test/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProviderGuardTest.java diff --git a/CLAUDE.md b/CLAUDE.md index 55ad89d8..d139c683 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -882,12 +882,22 @@ it against a real database — not a theoretical hardening pass. - **The MySQL provider also serves `mariadb`**, which has no `max_execution_time` — a fail-closed SET of it refused every query on the connection. The guard takes the server version (`DatabaseMetaData`) and omits it there. - - **Postgres `CREATE INDEX CONCURRENTLY` takes `lock_timeout = 0`** for its own run - (Index Advisor apply): its waits for older transactions honour `lock_timeout`, and - hitting 5s midway leaves an INVALID index. Its locks block neither reads nor writes. - - **The 10k cap is `setMaxRows` only.** `LIMIT` text is still appended only when the - client asked for a limit; appending it to every SELECT broke `FETCH FIRST`, - `FOR UPDATE` and `LOCK IN SHARE MODE`. + - **Postgres `CREATE INDEX CONCURRENTLY` takes `lock_timeout = 0`** for its own run, on + every path (Editor, MCP, Index Advisor), via `QueryExecutionProvider.ddlSessionSql`: + its waits for older transactions honour `lock_timeout`, and hitting 5s midway leaves an + INVALID index. Its locks block neither reads nor writes. + - **The reset is broad, not just the guard variables.** Postgres: `RESET ALL` (a read like + `SELECT set_config('lock_timeout','0',false)` changes the session too — which is why the + reset runs after *every* user/agent run, even a lone SELECT). MySQL: the guards plus + `sql_mode`, `foreign_key_checks`, `unique_checks`, `time_zone` — an explicit list, since + MySQL has no SQL form of COM_RESET_CONNECTION. Other MySQL session variables still carry + over, as they always did. INTERNAL/SCHEDULED runs keep the pool baseline only. + - **Replicas are `@@global.read_only OR @@global.innodb_read_only`** — Aurora readers report + `read_only=0`. + - **Row cap: 10k by default, an explicit request up to 100k** (the Editor's CSV export asks + for `EXPORT_ROW_LIMIT = 100000`; a flat 10k ceiling silently truncated exports). Enforced by + `setMaxRows` only; `LIMIT` text is appended only when the client asked for a limit — + appending it to every SELECT broke `FETCH FIRST`, `FOR UPDATE` and `LOCK IN SHARE MODE`. - **`max_execution_time` keys off `@@global.read_only`**: reads get the client timeout on a replica but at most 30s on a writable server. No per-connection setting to drift. - **MySQL ALTER runs online or not at all.** `onlineDdlAttempts` appends @@ -914,6 +924,22 @@ it against a real database — not a theoretical hardening pass. comment and `isReadOnlyQuery` then calls the empty remainder read-only — so it skipped every guard, the confirmation prompt included. Hints (`MAX_EXECUTION_TIME`, `SET_VAR`) override the session guards for one statement. + - **Role and identity switches are admin statements**: `SET [SESSION|LOCAL] ROLE`, + `SET SESSION AUTHORIZATION`, and the GUC spellings `SET role = …` / + `session_replication_role`. A switch left on a pooled connection is inherited by the + next borrower. (`SET @role = 1` is a user variable and stays allowed.) + - **The dangerous-function denylist now runs on every surface.** It lived only in + `McpSqlGuardService` (MCP, dashboards), so the Editor ran `SELECT dblink_exec(…)` and + `SELECT pg_terminate_backend(pid)` as plain reads. The policy calls + `McpSqlGuardService.dangerousFunctionIn` on its own linear scrub; the list (now with + `pg_terminate_backend`, `pg_cancel_backend`) still mirrors `mcp/deepsql-phase1-lib.js`. + DeepSQL's own cancel uses direct JDBC (`ActiveQueryService`), not this path. + - **Comment stripping must be a linear scan, never a lazy regex.** `McpSqlGuardService`'s + `/\*[\s\S]*?\*/` rescanned to the end from every unterminated `/*` (`"a/*"` × 32k took + minutes) on MCP and the public dashboard query path; the policy has shared its linear + scanner since. The MySQL clause tokenizer lets every alternative close at end-of-input + with possessive repetition for the same reason. `hiddenWriteScan_staysLinearOnAdversarialInput` + is the canary — it caught this the moment the guard reached the Editor path. - **The SET scope check reads raw text, not literal-stripped text.** Stripping literals is a second tokenizer that can disagree with MySQL's: `SET @a = "'", GLOBAL x = 1, @b = "'"` defeated a two-pass regex, and a `NO_BACKSLASH_ESCAPES` preamble changes MySQL's own @@ -925,7 +951,7 @@ it against a real database — not a theoretical hardening pass. — anywhere in the assignment list. SET used to be a "session preamble", so `SET GLOBAL …; SELECT 1` ran for *read-only* roles. - TRUNCATE is blocked in the Editor (it was MCP-only). User-facing origins get a - server-side 10k row cap whatever `limit` says (`INTERNAL`/`SCHEDULED` keep theirs). + server-side row cap (10k default, 100k ceiling; `INTERNAL`/`SCHEDULED` keep theirs). - Every executed statement carries `/* deepsql origin=… user=… */`, so slow log, processlist and Performance Insights attribute load to a person despite one shared login. The username is reduced to `[A-Za-z0-9._@-]` so it can never close the comment. diff --git a/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java index 83669632..9a2691bb 100644 --- a/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/api/QueryExecutionProvider.java @@ -110,6 +110,16 @@ default List onlineDdlAttempts(String sql) { return List.of(sql); } + /** + * A session setting a specific DDL statement needs for its own run, or null. The + * executor's reset restores the baseline afterwards. Postgres CREATE/DROP INDEX + * CONCURRENTLY waits for every older transaction under lock_timeout; cut off midway it + * leaves an INVALID index, while its locks block neither reads nor writes. + */ + default String ddlSessionSql(String sql) { + return null; + } + /** True when a DDL attempt failed only because the requested online algorithm is unsupported. */ default boolean isOnlineDdlUnsupported(SQLException e) { return false; diff --git a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java index 87520321..10cfea54 100644 --- a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java @@ -154,8 +154,21 @@ public String getSessionPidQuery() { private static final Pattern LOCK_CLAUSE = Pattern.compile( "\\bLOCK\\s*=?\\s*(DEFAULT|NONE|SHARED|EXCLUSIVE)\\b", Pattern.CASE_INSENSITIVE); private static final Pattern PARTITION_CLAUSE = Pattern.compile("\\bPARTITION(?:S|ING)?\\b", Pattern.CASE_INSENSITIVE); + // One left-to-right pass over literals, identifiers and comments: at each position the + // first alternative that starts there wins, so a quote inside a comment, or a comment + // marker inside a string, cannot desynchronise the two. A clause inside a comment is not + // a clause — `-- , ALGORITHM=INPLACE, LOCK=NONE` used to read as a real one. MySQL's + // `--` comment needs whitespace after it (`--1` is arithmetic). + // Every alternative also accepts end-of-input as its close, with possessive repetition: + // an unterminated quote or comment is consumed once, to the end, instead of being + // rescanned from every opening marker (a lazy /\*.*?\*/ is quadratic on "a/*" x 32k). private static final Pattern QUOTED = Pattern.compile( - "'(?:[^'\\\\]|\\\\.|'')*'|\"(?:[^\"\\\\]|\\\\.)*\"|`[^`]*`"); + "'(?:[^'\\\\]|\\\\.|'')*+(?:'|$)" + + "|\"(?:[^\"\\\\]|\\\\.)*+(?:\"|$)" + + "|`[^`]*+(?:`|$)" + + "|/\\*(?:[^*]|\\*(?!/))*+(?:\\*/|$)" + + "|--(?=\\s|$)[^\\n]*+" + + "|#[^\\n]*+"); private static final int ER_LOCK_WAIT_TIMEOUT = 1205; private static final int ER_UPDATE_WITHOUT_KEY_IN_SAFE_MODE = 1175; @@ -186,13 +199,21 @@ public String statementGuardSql(int timeoutSeconds, String serverVersion) { return guard; } int clientMs = timeoutSeconds > 0 ? timeoutSeconds * 1000 : PRIMARY_READ_LIMIT_MS; - return guard + ", max_execution_time = IF(@@global.read_only, " + // Aurora readers report read_only=0 but innodb_read_only=1; either marks a replica. + return guard + ", max_execution_time = IF(@@global.read_only OR @@global.innodb_read_only, " + clientMs + ", LEAST(" + clientMs + ", " + PRIMARY_READ_LIMIT_MS + "))"; } + /** + * Restores the guard variables plus the session settings a user's preamble can use to + * weaken the next borrower's writes (FK checks, strict mode). ponytail: an explicit list, + * not COM_RESET_CONNECTION — MySQL has no SQL form of it, and the driver-level reset + * re-authenticates on every run. Other session variables still carry over, as before. + */ @Override public String statementGuardResetSql(String serverVersion) { return "SET SESSION " + LOCK_GUARD + ", sql_safe_updates = DEFAULT" + + ", sql_mode = DEFAULT, foreign_key_checks = DEFAULT, unique_checks = DEFAULT, time_zone = DEFAULT" + (isMariaDb(serverVersion) ? "" : ", max_execution_time = DEFAULT"); } @@ -209,7 +230,7 @@ public List onlineDdlAttempts(String sql) { while (base.endsWith(";")) { base = base.substring(0, base.length() - 1).strip(); } - String clauses = QUOTED.matcher(base).replaceAll("''"); + String clauses = QUOTED.matcher(base).replaceAll(" "); String algorithm = clauseValue(ALGORITHM_CLAUSE, clauses); String lock = clauseValue(LOCK_CLAUSE, clauses); diff --git a/backend/src/main/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProvider.java index f78a0961..be5bb3c3 100644 --- a/backend/src/main/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProvider.java @@ -143,6 +143,24 @@ public String connectionGuardSql() { return "SET lock_timeout = '5s'"; } + /** + * RESET ALL first: a user's preamble (or a read like {@code SELECT set_config(...)}) can + * change any session setting, and the pooled connection carries it to the next borrower. + * Startup parameters survive RESET ALL; the lock guard is then re-applied. + */ + @Override + public String statementGuardResetSql(String serverVersion) { + return "RESET ALL; " + connectionGuardSql(); + } + + private static final java.util.regex.Pattern CONCURRENTLY = + java.util.regex.Pattern.compile("\\bCONCURRENTLY\\b", java.util.regex.Pattern.CASE_INSENSITIVE); + + @Override + public String ddlSessionSql(String sql) { + return sql != null && CONCURRENTLY.matcher(sql).find() ? "SET lock_timeout = 0" : null; + } + @Override public String guardrailMessage(java.sql.SQLException e) { // 55P03 lock_not_available: raised when lock_timeout expires. diff --git a/backend/src/main/java/com/dbaagent/service/IndexRecommendationApplyService.java b/backend/src/main/java/com/dbaagent/service/IndexRecommendationApplyService.java index fbbd38d4..07109392 100644 --- a/backend/src/main/java/com/dbaagent/service/IndexRecommendationApplyService.java +++ b/backend/src/main/java/com/dbaagent/service/IndexRecommendationApplyService.java @@ -194,22 +194,21 @@ public ApplyResult apply(String recommendationId, Mode mode, boolean confirmed, } else { try (Statement s = c.createStatement()) { log.info("Applying recommendation {} on {}: {}", rec.getId(), rec.getConnectionId(), ddl); - // CREATE/DROP INDEX CONCURRENTLY waits for every older transaction, - // and those waits honour the pool's 5s lock_timeout: hitting it midway - // leaves an INVALID index behind. Its locks do not block reads or - // writes, so waiting is safe here; restore the pool guard afterwards. - boolean concurrently = ddl.toUpperCase(java.util.Locale.ROOT).contains(" CONCURRENTLY "); - if (concurrently) { - s.execute("SET lock_timeout = 0"); + // Some DDL needs its own session setting (Postgres CONCURRENTLY must not + // be cut off by the pool's lock timeout, or it leaves an INVALID index). + // The dialect decides; restore the pool baseline afterwards. + com.dbaagent.provider.api.QueryExecutionProvider dialect = + providerRegistry.getDialect(req.getDbType()).queryExecution(); + String ddlSession = dialect.ddlSessionSql(ddl); + if (ddlSession != null) { + s.execute(ddlSession); } try { s.execute(ddl); } finally { - String guard = concurrently - ? providerRegistry.getDialect(req.getDbType()).queryExecution().connectionGuardSql() - : null; - if (guard != null) { - s.execute(guard); + String baseline = ddlSession != null ? dialect.connectionGuardSql() : null; + if (baseline != null) { + s.execute(baseline); } } } diff --git a/backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java b/backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java index 9a6252f9..a6353ee4 100644 --- a/backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java +++ b/backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java @@ -12,8 +12,6 @@ public class McpSqlGuardService { private static final Pattern FIRST_KEYWORD_PATTERN = Pattern.compile("^([A-Za-z]+)"); private static final Pattern EXPLAIN_ANALYZE_PATTERN = Pattern.compile("\\bANALYZ[EA]\\b", Pattern.CASE_INSENSITIVE); - private static final Pattern BLOCK_COMMENT_PATTERN = Pattern.compile("/\\*[\\s\\S]*?\\*/"); - private static final Pattern LINE_COMMENT_PATTERN = Pattern.compile("(?m)--.*$"); private static final Pattern SINGLE_QUOTE_STRING_PATTERN = Pattern.compile("'(?:''|[^'])*'"); private static final Pattern DOUBLE_QUOTE_STRING_PATTERN = Pattern.compile("\"(?:[\"\"]|[^\"])*\""); private static final Pattern BACKTICK_STRING_PATTERN = Pattern.compile("`(?:``|[^`])*`"); @@ -72,7 +70,9 @@ public class McpSqlGuardService { "pg_read_file", "pg_read_binary_file", "pg_ls_dir", "pg_stat_file", "lo_import", "lo_export", // MySQL equivalents - "load_file" + "load_file", + // reach into other sessions: a "read" that disconnects or cancels production work + "pg_terminate_backend", "pg_cancel_backend" ); private static final String DANGEROUS_FUNCTION_ALTERNATION = @@ -166,7 +166,16 @@ public ValidationOutcome validateReadOnlySql(String sql, boolean allowExplain) { * hide or falsely trigger a match. */ String containsDangerousFunction(String statement) { - var matcher = DANGEROUS_FUNCTION_CALL.matcher(normalizeSqlForInspection(statement)); + return dangerousFunctionIn(normalizeSqlForInspection(statement)); + } + + /** + * The first dangerous function call in SQL whose comments and literals the caller has + * already stripped, or null. Shared with {@link QueryExecutionPolicyService} so the + * denylist has one definition across the Editor, MCP and dashboard surfaces. + */ + static String dangerousFunctionIn(String scrubbedSql) { + var matcher = DANGEROUS_FUNCTION_CALL.matcher(String.valueOf(scrubbedSql)); return matcher.find() ? matcher.group(1).toLowerCase(Locale.ROOT) : null; } @@ -368,9 +377,35 @@ List splitStatements(String sql) { .toList(); } - private String stripSqlComments(String sql) { + /** + * Linear scan, not a regex. {@code /\*[\s\S]*?\*}{@code /} rescans to the end of input + * from every unterminated {@code /*}: {@code "a/*"} repeated 32k times took minutes of + * CPU on a request thread — reachable from MCP and the public dashboard query endpoint. + * An unterminated block comment runs to the end, as the database would treat it. + */ + static String stripSqlComments(String sql) { String input = String.valueOf(sql); - return LINE_COMMENT_PATTERN.matcher(BLOCK_COMMENT_PATTERN.matcher(input).replaceAll(" ")).replaceAll(" "); + StringBuilder out = new StringBuilder(input.length()); + int i = 0; + int len = input.length(); + while (i < len) { + char c = input.charAt(i); + if (c == '/' && i + 1 < len && input.charAt(i + 1) == '*') { + int end = input.indexOf("*/", i + 2); + i = end < 0 ? len : end + 2; + out.append(' '); + continue; + } + if (c == '-' && i + 1 < len && input.charAt(i + 1) == '-') { + int nl = input.indexOf('\n', i + 2); + i = nl < 0 ? len : nl; + out.append(' '); + continue; + } + out.append(c); + i++; + } + return out.toString(); } private String stripSqlStringLiterals(String sql) { diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java index 4ac260a6..176f68ab 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java @@ -95,7 +95,9 @@ public class QueryExecutionPolicyService { + "|(?:CREATE|ALTER|DROP|RENAME)\\s+(?:USER|ROLE)|ALTER\\s+SYSTEM" + "|RESET\\s+(?:MASTER|REPLICA|SLAVE|BINARY|PERSIST)" + "|CHANGE\\s+(?:MASTER|REPLICATION)|(?:START|STOP)\\s+(?:SLAVE|REPLICA|GROUP_REPLICATION)" - + "|SET\\s+(?:PASSWORD|ROLE|DEFAULT\\s+ROLE))\\b", + // Postgres also takes SESSION/LOCAL before ROLE, and SESSION AUTHORIZATION: + // a role switch left on a pooled connection is inherited by the next borrower. + + "|SET\\s+(?:(?:SESSION|LOCAL)\\s+)?(?:PASSWORD|ROLE|DEFAULT\\s+ROLE|AUTHORIZATION|SESSION\\s+AUTHORIZATION))\\b", Pattern.CASE_INSENSITIVE ); // Any SET that reaches global or persisted scope, anywhere in its assignment list @@ -103,7 +105,10 @@ public class QueryExecutionPolicyService { // binary logging for the session — replicas would silently diverge. private static final Pattern SET_PREFIX_PATTERN = Pattern.compile("^SET\\b", Pattern.CASE_INSENSITIVE); private static final Pattern SET_BEYOND_SESSION_PATTERN = Pattern.compile( - "\\b(?:GLOBAL|PERSIST|PERSIST_ONLY)\\b|@@(?:GLOBAL|PERSIST|PERSIST_ONLY)\\.|\\bsql_log_bin\\b", + "\\b(?:GLOBAL|PERSIST|PERSIST_ONLY)\\b|@@(?:GLOBAL|PERSIST|PERSIST_ONLY)\\.|\\bsql_log_bin\\b" + // The GUC spellings of a role/identity switch (`SET role = x`, `SET LOCAL role TO x`) + // and Postgres trigger/FK suppression. Not preceded by @ — `SET @role = 1` is a variable. + + "|(? classifications = new ArrayList<>(); @@ -268,8 +287,8 @@ && isDropTableStatement(mutation.queryType(), statements.getFirst())) { throw QueryExecutionPolicyException.unsafeMutation(blocking.getMessage(), mutation.queryType()); } if (!attempts.equals(List.of(statements.getFirst()))) { - confirmationNotes.add("DeepSQL will run this online: ALGORITHM=INSTANT first, then ALGORITHM=INPLACE " - + "with LOCK=NONE. If neither can run without blocking writes, it stops and changes nothing."); + confirmationNotes.add("DeepSQL will run this in a mode that does not block writes to the table. " + + "If the database cannot make this change that way, DeepSQL stops and changes nothing."); } confirmationNotes.add("If the table is busy, DeepSQL gives up after waiting 5 seconds for its lock " + "instead of queueing every other write to the table behind this change."); @@ -581,28 +600,9 @@ private String detectHiddenWrite(String sql) { // matched a lazy wildcard between block-comment delimiters and backtracked // quadratically on an unterminated block comment: 96KB of input cost ~14s // of CPU. + /** Linear; one implementation shared with the MCP / dashboard guard. */ private String stripComments(String sql) { - StringBuilder out = new StringBuilder(sql.length()); - int i = 0; - int len = sql.length(); - while (i < len) { - char c = sql.charAt(i); - if (c == '/' && i + 1 < len && sql.charAt(i + 1) == '*') { - int end = sql.indexOf("*/", i + 2); - i = end < 0 ? len : end + 2; - out.append(' '); - continue; - } - if (c == '-' && i + 1 < len && sql.charAt(i + 1) == '-') { - int nl = sql.indexOf('\n', i + 2); - i = nl < 0 ? len : nl; - out.append(' '); - continue; - } - out.append(c); - i++; - } - return out.toString(); + return McpSqlGuardService.stripSqlComments(sql); } private boolean isUseStatement(String sql) { diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java index 56e834ce..5cd96c56 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutorService.java @@ -667,11 +667,19 @@ public QueryResult executeQuery( // pool baseline in the finally below, because session variables outlive the // borrow and would otherwise reach the next borrower, background jobs included. // Fails closed: nothing runs without them. + // + // DeepSQL's own INTERNAL/SCHEDULED SQL keeps only the pool baseline (lock + // guards from connectionInitSql): it is trusted code with its own timeouts. + // A guard identical to the baseline is not re-sent — the reset below restores + // the baseline after every run, so it is already in place. QueryExecutionProvider executionProvider = providerRegistry.getDialect(connRequest.getDbType()).queryExecution(); String serverVersion = serverVersion(connection); - String guardSql = executionProvider.statementGuardSql(effectiveTimeout, serverVersion); + boolean trustedOrigin = isTrustedOrigin(executionContext); + String guardSql = trustedOrigin ? null : executionProvider.statementGuardSql(effectiveTimeout, serverVersion); String tag = actorTag(executionContext); - applyGuard(connection, guardSql); + if (guardSql != null && !guardSql.equals(executionProvider.connectionGuardSql())) { + applyGuard(connection, guardSql); + } try { // Execute all preamble statements (SET @var, USE db, etc.) on the same connection @@ -744,6 +752,10 @@ public QueryResult executeQuery( throw QueryExecutionPolicyException.unsafeMutation( blocking.getMessage(), policyDecision.primaryQueryType()); } + String ddlSession = policyDecision.mutating() ? executionProvider.ddlSessionSql(finalQuery) : null; + if (ddlSession != null) { + applyGuard(connection, ddlSession); + } boolean hasResultSet = false; for (int attempt = 0; attempt < attempts.size(); attempt++) { try { @@ -837,7 +849,12 @@ public QueryResult executeQuery( } } } finally { - resetGuard(connection, executionProvider.statementGuardResetSql(serverVersion), connectionId); + // Always for user and agent SQL, even a lone SELECT: Postgres + // `SELECT set_config('lock_timeout', '0', false)` is a read that changes + // the session. Trusted origins only when they sent preambles. + if (!trustedOrigin || statements.size() > 1) { + resetGuard(connection, executionProvider.statementGuardResetSql(serverVersion), connectionId); + } } } finally { runningQueryRegistry.unregister(queryRequest.getExecutionId()); @@ -889,21 +906,29 @@ private static String serverVersion(Connection connection) { } } - /** Hard ceiling on rows returned to a person or agent, whatever the request asks for. */ - static final int MAX_RESULT_ROWS = 10_000; + /** Rows returned to a person or agent that asked for no limit. */ + static final int DEFAULT_RESULT_ROWS = 10_000; + /** Ceiling on an explicit request; matches the Editor's CSV export (EXPORT_ROW_LIMIT). */ + static final int MAX_RESULT_ROWS = 100_000; /** * The row cap was client-controlled: a request without {@code limit} streamed the - * whole table into memory. People and agents now get at most {@link #MAX_RESULT_ROWS}; - * DeepSQL's own background jobs keep what they ask for. + * whole table into memory. People and agents now get {@link #DEFAULT_RESULT_ROWS} unless + * they ask, and never more than {@link #MAX_RESULT_ROWS}. DeepSQL's own background jobs + * keep what they ask for. */ static int effectiveRowLimit(Integer requested, QueryExecutionContext context) { int asked = requested != null && requested > 0 ? requested : 0; - QueryExecutionOrigin origin = context == null ? null : context.origin(); - if (origin == QueryExecutionOrigin.INTERNAL || origin == QueryExecutionOrigin.SCHEDULED || origin == null) { + if (isTrustedOrigin(context)) { return asked; } - return asked > 0 ? Math.min(asked, MAX_RESULT_ROWS) : MAX_RESULT_ROWS; + return asked > 0 ? Math.min(asked, MAX_RESULT_ROWS) : DEFAULT_RESULT_ROWS; + } + + /** DeepSQL's own jobs, not a person or an agent. */ + private static boolean isTrustedOrigin(QueryExecutionContext context) { + QueryExecutionOrigin origin = context == null ? null : context.origin(); + return origin == null || origin == QueryExecutionOrigin.INTERNAL || origin == QueryExecutionOrigin.SCHEDULED; } /** diff --git a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java index 66e18da6..74059889 100644 --- a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java +++ b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java @@ -105,10 +105,36 @@ void everyPooledConnectionGivesUpOnLocksAfterFiveSeconds() { @Test void perStatementGuardCapsReadsAtThirtySecondsOnlyOnAWritableServer() { + // innodb_read_only too: Aurora readers report read_only=0. assertThat(provider.statementGuardSql(240, "8.0.36")).isEqualTo( "SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5, sql_safe_updates = 1, " - + "max_execution_time = IF(@@global.read_only, 240000, LEAST(240000, 30000))"); - assertThat(provider.statementGuardSql(0, null)).contains("IF(@@global.read_only, 30000, LEAST(30000, 30000))"); + + "max_execution_time = IF(@@global.read_only OR @@global.innodb_read_only, 240000, LEAST(240000, 30000))"); + assertThat(provider.statementGuardSql(0, null)) + .contains("IF(@@global.read_only OR @@global.innodb_read_only, 30000, LEAST(30000, 30000))"); + } + + @Test + void clauseDetectionStaysLinearOnUnterminatedCommentsAndQuotes() { + // A lazy /\*.*?\*/ rescans to the end from every unterminated opener. + for (String tail : List.of("/*" + "a/*".repeat(32_000), "'" + "a\\'".repeat(32_000), "`" + "a".repeat(64_000))) { + long startedAt = System.currentTimeMillis(); + provider.onlineDdlAttempts("ALTER TABLE t ADD COLUMN x INT " + tail); + assertThat(System.currentTimeMillis() - startedAt).as("must not backtrack").isLessThan(5_000L); + } + } + + @Test + void aClauseInsideACommentIsNotAClause() { + // Read as real, this ran the MODIFY with MySQL's default (COPY) algorithm, unguarded. + assertThat(provider.onlineDdlAttempts( + "ALTER TABLE bookings MODIFY status VARCHAR(20) NOT NULL -- , ALGORITHM=INPLACE, LOCK=NONE")) + .hasSize(2); + assertThat(provider.onlineDdlAttempts("ALTER TABLE t ADD COLUMN x INT /* LOCK=SHARED */")).hasSize(2); + assertThat(provider.onlineDdlAttempts("ALTER TABLE t ADD COLUMN x INT # ALGORITHM=COPY")).hasSize(2); + // A quote inside a comment must not desynchronise literal stripping: the real + // COPY clause on the next line is still seen, and refused. + assertThrows(IllegalArgumentException.class, + () -> provider.onlineDdlAttempts("ALTER TABLE t ADD COLUMN x INT -- don't\n, ALGORITHM=COPY")); } @Test @@ -122,9 +148,11 @@ void mariaDbGetsNoMaxExecutionTimeBecauseItDoesNotHaveOne() { @Test void theResetRestoresThePoolBaselineSoNothingLeaksToTheNextBorrower() { + // Also the settings a preamble could use to weaken the next borrower's writes. assertThat(provider.statementGuardResetSql("8.0.36")).isEqualTo( - "SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5, " - + "sql_safe_updates = DEFAULT, max_execution_time = DEFAULT"); + "SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5, sql_safe_updates = DEFAULT, " + + "sql_mode = DEFAULT, foreign_key_checks = DEFAULT, unique_checks = DEFAULT, time_zone = DEFAULT, " + + "max_execution_time = DEFAULT"); } @Test diff --git a/backend/src/test/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProviderGuardTest.java b/backend/src/test/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProviderGuardTest.java new file mode 100644 index 00000000..af5be642 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/provider/postgres/PostgresQueryExecutionProviderGuardTest.java @@ -0,0 +1,40 @@ +package com.dbaagent.provider.postgres; + +import org.junit.jupiter.api.Test; + +import java.sql.SQLException; + +import static org.assertj.core.api.Assertions.assertThat; + +class PostgresQueryExecutionProviderGuardTest { + + private final PostgresQueryExecutionProvider provider = new PostgresQueryExecutionProvider(); + + @Test + void everyPooledConnectionGivesUpOnLocksAfterFiveSeconds() { + assertThat(provider.connectionGuardSql()).isEqualTo("SET lock_timeout = '5s'"); + } + + @Test + void theResetClearsEverySessionSettingNotJustTheGuard() { + // A preamble, or a read like SELECT set_config(...), can change any setting + // (session_replication_role, search_path, ...); the pooled connection carries it on. + assertThat(provider.statementGuardResetSql(null)).isEqualTo("RESET ALL; SET lock_timeout = '5s'"); + } + + @Test + void aConcurrentIndexBuildIsNotCutOffByTheLockTimeout() { + // Cut off midway it leaves an INVALID index; its locks block neither reads nor writes. + assertThat(provider.ddlSessionSql("CREATE INDEX CONCURRENTLY idx ON orders (customer_id)")) + .isEqualTo("SET lock_timeout = 0"); + assertThat(provider.ddlSessionSql("drop index concurrently idx")).isEqualTo("SET lock_timeout = 0"); + assertThat(provider.ddlSessionSql("ALTER TABLE orders ADD COLUMN x int")).isNull(); + } + + @Test + void aLockTimeoutIsExplainedInPlainWords() { + assertThat(provider.guardrailMessage(new SQLException("canceling statement due to lock timeout", "55P03"))) + .contains("5 seconds"); + assertThat(provider.guardrailMessage(new SQLException("duplicate key", "23505"))).isNull(); + } +} diff --git a/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java b/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java index 1f327643..9c9c1db8 100644 --- a/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java @@ -213,4 +213,24 @@ void stillAllowsOrdinaryAnalyticQueries() { assertTrue(service.validateReadOnlySql( "SELECT count(*), sum(o.total) FROM public.orders o WHERE o.created_at >= '2026-01-01'", true).ok()); } + + @Test + void commentStrippingStaysLinearOnAnUnterminatedBlockComment() { + // The regex strip /\*[\s\S]*?\*/ rescanned to the end from every "/*": this input took + // minutes of CPU on a request thread, reachable from MCP and the public dashboard + // query endpoint. + String hostile = "SELECT 1 /*" + "a/*".repeat(32_000); + long startedAt = System.currentTimeMillis(); + service.validateReadOnlySql(hostile, true); + assertTrue(System.currentTimeMillis() - startedAt < 5_000L, "comment stripping must not backtrack"); + } + + @Test + void terminatesAndKillsAreDangerousFunctions() { + assertEquals("pg_terminate_backend", + service.containsDangerousFunction("SELECT pg_terminate_backend(pid) FROM pg_stat_activity")); + assertEquals("pg_cancel_backend", service.containsDangerousFunction("select PG_CANCEL_BACKEND (42)")); + // A column that merely starts with the name is not a call. + assertNull(service.containsDangerousFunction("SELECT pg_terminate_backend_count FROM audit")); + } } diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java index 52f9ef59..9b1045e1 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyProdGuardTest.java @@ -72,6 +72,13 @@ void administrativeStatementsAreBlockedOnEverySurfaceEvenWhenConfirmed() { "SHUTDOWN", "RESET MASTER", "PURGE BINARY LOGS BEFORE NOW()", + // Postgres role/identity switches: left on a pooled connection, inherited by the next borrower. + "SET SESSION ROLE app_owner", + "SET LOCAL ROLE app_owner", + "SET SESSION AUTHORIZATION app_owner", + "SET role = app_owner", + "SET LOCAL role TO app_owner", + "SET session_replication_role = replica", " /* sneaky */ GRANT ALL ON *.* TO 'x'@'%'")) { assertThat(blocked(sql, confirmedAdminEditor()).getErrorCode()) .as(sql).isEqualTo(QueryExecutionPolicyException.ADMIN_STATEMENT_BLOCKED); @@ -138,6 +145,24 @@ void anAdministrativeStatementHiddenAsAPreambleIsStillBlocked() { .isEqualTo(QueryExecutionPolicyException.ADMIN_STATEMENT_BLOCKED); } + @Test + void functionsThatReachOutsideTheQueryAreRefusedInTheEditorToo() { + // The denylist used to run only for MCP and dashboards; in the Editor these were plain reads. + for (String sql : List.of( + "SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE usename = 'app'", + "SELECT pg_cancel_backend(12345)", + "SELECT dblink_exec('dbname=app', 'DELETE FROM orders')")) { + assertThat(blocked(sql, QueryExecutionContext.editor("analyst", false, false)).getErrorCode()) + .as(sql).isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + } + } + + @Test + void aUserVariableNamedRoleIsNotARoleSwitch() { + service.enforce(new QueryRequest("SET @role = 1; SELECT @role", null, null), + QueryExecutionContext.editor("analyst", false, false), "mysql"); + } + @Test void sessionVariablesStillWork() { QueryExecutionPolicyService.PolicyDecision decision = service.enforce( @@ -252,6 +277,7 @@ void theAlterConfirmationSaysHowItWillRun() { "ALTER TABLE customers MODIFY email varchar(512) NOT NULL", QueryExecutionContext.editor("admin", true, false)); assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.EDITOR_MUTATION_CONFIRMATION_REQUIRED); - assertThat(String.join(" ", e.getWarnings())).contains("LOCK=NONE").contains("5 seconds"); + // Dialect-neutral wording: the policy no longer names MySQL algorithms. + assertThat(String.join(" ", e.getWarnings())).contains("does not block writes").contains("5 seconds"); } } diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java index f2826e78..518aa79f 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutorGuardrailTest.java @@ -129,6 +129,20 @@ void aHeavyReadPlacedFirstInABatchStillRunsUnderTheGuards() throws Exception { "/* deepsql origin=editor user=dba */ SELECT * FROM bookings b JOIN payments p ON p.booking_id = b.id"); } + @Test + void deepsqlsOwnJobsKeepThePoolBaselineOnly() throws Exception { + // INTERNAL/SCHEDULED SQL is trusted code with its own timeouts: no 30s primary cap, + // no sql_safe_updates, and no reset round trip when it sent no preambles. + policyReturns(false, "SELECT"); + + executor.executeQuery("conn-1", new QueryRequest("SELECT COUNT(*) FROM customers", null, 240), + QueryExecutionContext.internal()); + + verify(statement, never()).execute(GUARD); + verify(statement, never()).execute(RESET); + verify(statement).execute("/* deepsql origin=internal */ SELECT COUNT(*) FROM customers"); + } + @Test void theSessionIsResetEvenWhenTheStatementFails() throws Exception { // Session variables outlive the borrow: without the reset, this run's @@ -199,11 +213,17 @@ void aLockWaitTimeoutIsReportedAsTheGuardItIs() throws Exception { } @Test - void peopleAndAgentsGetAtMostTenThousandRowsWhateverTheyAskFor() { + void peopleAndAgentsGetTenThousandRowsUnlessTheyAskAndNeverMoreThanTheExportCeiling() { assertThat(QueryExecutorService.effectiveRowLimit(null, QueryExecutionContext.editor("a", true, false))) .isEqualTo(10_000); + // The Editor's CSV export asks for 100k (EXPORT_ROW_LIMIT); a 10k ceiling silently + // truncated exports that worked before. + assertThat(QueryExecutorService.effectiveRowLimit(100_000, QueryExecutionContext.editor("a", true, false))) + .isEqualTo(100_000); assertThat(QueryExecutorService.effectiveRowLimit(50_000, QueryExecutionContext.mcp("a"))) - .isEqualTo(10_000); + .isEqualTo(50_000); + assertThat(QueryExecutorService.effectiveRowLimit(5_000_000, QueryExecutionContext.mcp("a"))) + .isEqualTo(100_000); assertThat(QueryExecutorService.effectiveRowLimit(1_000, QueryExecutionContext.editor("a", false, false))) .isEqualTo(1_000); assertThat(QueryExecutorService.effectiveRowLimit(null, QueryExecutionContext.api("a"))) diff --git a/mcp/CLAUDE.md b/mcp/CLAUDE.md index d6389ee1..1b441808 100644 --- a/mcp/CLAUDE.md +++ b/mcp/CLAUDE.md @@ -80,7 +80,7 @@ in this version; ask before mid-session admin work. | `optimize_slow_query` | AI query REWRITE + plan diagnosis for one specific SQL. Single-query scoped, synchronous. Does NOT recommend indexes — index/pre-aggregation recs require whole-workload context (`get_index_recommendations` / Workload Analysis). Pass `avgExecutionTimeMs` to anchor the impact estimate. | | `get_index_recommendations` | **Workload-weighted DBA-grade index advisor.** Pre-computed top-N (default 5) recommendations ranked by net benefit (`Σ calls × mean_exec_time` − write-cost). Each result carries up to 5 contributing query fingerprints, the role each column played, and optional HypoPG cost-delta on Postgres. Covers both `CREATE_INDEX` and `DROP_INDEX` (unused + redundant-prefix) candidates. | | **`apply_index_recommendation`** | Apply (or dry-run) a recommendation against its target connection and measure the before/after benefit on contributing queries. `DRY_RUN` (default) uses HypoPG (Postgres-only) for zero-write cost-delta. `APPLY` runs real `CREATE/DROP INDEX CONCURRENTLY` (configurable via `concurrent`). `APPLY_AND_MEASURE` additionally runs `EXPLAIN ANALYZE` for wall-clock timings. Write modes require `confirm: true`. The DDL is server-generated from the recommendation row — clients never supply SQL. This is the only MCP path that can drop an index; `execute_sql` blocks `DROP`. | -| **`execute_sql`** | **Run a SQL statement.** Policy is server-enforced: developers can run SELECT/WITH/SHOW/EXPLAIN; admins can also run DML and non-destructive DDL (`CREATE`, `ALTER`, `CREATE INDEX`) with a two-step confirmation. `DROP` and `TRUNCATE` are blocked on this surface even for confirmed admins. EXPLAIN and EXPLAIN ANALYZE are just SQL — no separate flag. | +| **`execute_sql`** | **Run a SQL statement.** Policy is server-enforced: developers can run SELECT/WITH/SHOW/EXPLAIN; admins can also run DML and non-destructive DDL (`CREATE`, `ALTER`, `CREATE INDEX`) with a two-step confirmation. Schema changes also need the "Run schema changes" (`EXECUTE_DDL`) permission, and MySQL `ALTER` runs online only (it stops rather than block writes). `DROP` and `TRUNCATE` are blocked on this surface even for confirmed admins, as are server-admin statements (GRANT, KILL, `SET GLOBAL`, role switches), `pg_terminate_backend`/`dblink_*`-style functions, and MySQL `/*! */` / `/*+ */` comments. Every statement gives up after 5s waiting for a lock. EXPLAIN and EXPLAIN ANALYZE are just SQL — no separate flag. | | **`analyze_query_plan`** | **AI-enriched plan analysis** for a query. Returns the parsed plan tree, performance issues, index recommendations, and a written summary that takes the connection's schema and business rules into account. Pass `useAnalyze: true` to run `EXPLAIN ANALYZE` (actually executes the query). | | **`analyze_migration`** | **Check whether a DDL statement is safe to run, before running it.** Deterministic verdict (SAFE/CAUTION/DANGER/FAILS/UNKNOWN) verified against a real PostgreSQL — trust it over your own recollection of Postgres lock semantics. Returns the exact locks taken (per table — `ADD FOREIGN KEY` locks the referenced table too), whether the table is rewritten, a duration estimate scaled by live table size, and a safer alternative where one exists. PostgreSQL only; MySQL returns `UNKNOWN` rather than a guess. Read-only — never executes the statement. | diff --git a/mcp/deepsql-phase1-lib.js b/mcp/deepsql-phase1-lib.js index f988e4dc..dbe09047 100644 --- a/mcp/deepsql-phase1-lib.js +++ b/mcp/deepsql-phase1-lib.js @@ -41,6 +41,7 @@ const DANGEROUS_SQL_FUNCTIONS = [ "pg_read_file", "pg_read_binary_file", "pg_ls_dir", "pg_stat_file", "lo_import", "lo_export", "load_file", + "pg_terminate_backend", "pg_cancel_backend", ]; // The function being *called* — name, optional whitespace, open paren. Matching the bare name From ee23e367dd2e6b68520353f451a16478718dfcbb Mon Sep 17 00:00:00 2001 From: geekypunk Date: Sat, 7 Nov 2026 12:19:21 -0600 Subject: [PATCH 5/6] fix: guardrail bugs found against a real MySQL 8.4 server Adds MySQLGuardrailLiveTest, which runs the guardrails through the real executor, policy and pool config against a live server (skipped unless DEEPSQL_MYSQL_TEST_URL is set). It found two bugs the mocked suites could not: - HikariCP hardcodes SQLState 0A000 as connection-fatal, and MySQL raises "ALGORITHM=INSTANT is not supported" (1845) with that state. The pool evicted a healthy connection mid-run, so the INSTANT -> INPLACE fallback never worked through the real pool ("statement closed"). The pool now asks the dialect via a SQLExceptionOverride (ConnectionService.applyGuardrails). - SET SESSION foreign_key_checks = DEFAULT (and unique_checks) leaves the session at 0 on MySQL 8.4. The reset now restores each variable from @@global, so a user's preamble cannot disable FK checks for later borrowers. Measured on MySQL 8.4.11: an ALTER blocked by an open transaction gives up after 5.0s, and a writer queued behind it completes as soon as it does (4.0s); a row-lock wait gives up after 5.1s; the incident's MODIFY ... NOT NULL runs via INPLACE, LOCK=NONE; INT -> BIGINT is refused and unchanged. Co-Authored-By: Claude Opus 5.5 --- CLAUDE.md | 17 + .../mysql/MySQLQueryExecutionProvider.java | 17 +- .../dbaagent/service/ConnectionService.java | 38 +- .../mysql/MySQLGuardrailLiveTest.java | 401 ++++++++++++++++++ .../MySQLQueryExecutionProviderGuardTest.java | 9 +- 5 files changed, 468 insertions(+), 14 deletions(-) create mode 100644 backend/src/test/java/com/dbaagent/provider/mysql/MySQLGuardrailLiveTest.java diff --git a/CLAUDE.md b/CLAUDE.md index d139c683..7ba054cf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -892,8 +892,25 @@ it against a real database — not a theoretical hardening pass. `sql_mode`, `foreign_key_checks`, `unique_checks`, `time_zone` — an explicit list, since MySQL has no SQL form of COM_RESET_CONNECTION. Other MySQL session variables still carry over, as they always did. INTERNAL/SCHEDULED runs keep the pool baseline only. + **Restore with `= @@global.x`, never `= DEFAULT`:** measured on MySQL 8.4, + `SET SESSION foreign_key_checks = DEFAULT` (and `unique_checks`) leaves the session at 0 + while the global is 1 — the reset silently failed for exactly the integrity switches. + - **HikariCP evicts on SQLState `0A000`, and MySQL's "ALGORITHM=INSTANT is not supported" + (1845) is `0A000`.** Hikari 7.1 hardcodes that state as connection-fatal, so the pool threw + away a healthy connection mid-run and the INPLACE fallback died on "No operations allowed + after statement closed" — the fallback never worked once through the real pool. + `ConnectionService.applyGuardrails` sets a `SQLExceptionOverride` that asks the dialect + (`isOnlineDdlUnsupported`) instead. Plain JDBC does not reproduce it; mocked tests cannot. - **Replicas are `@@global.read_only OR @@global.innodb_read_only`** — Aurora readers report `read_only=0`. + - **`MySQLGuardrailLiveTest` runs all of this against a real server** through the real + executor, policy and pool config (skipped unless `DEEPSQL_MYSQL_TEST_URL` is set). Both bugs + above were found by it, not by the mocked suites. Point it only at a disposable server — it + creates/drops a schema and toggles `@@global.read_only`. A throwaway instance without + Docker: `mysqld --no-defaults --initialize-insecure --datadir=`, then start it with + `--port=33084 --bind-address=127.0.0.1 --socket=/tmp/x.sock` (macOS caps socket paths at + 103 chars), and run with + `DEEPSQL_MYSQL_TEST_URL='jdbc:mysql://127.0.0.1:33084/?user=root&allowPublicKeyRetrieval=true&useSSL=false'`. - **Row cap: 10k by default, an explicit request up to 100k** (the Editor's CSV export asks for `EXPORT_ROW_LIMIT = 100000`; a flat 10k ceiling silently truncated exports). Enforced by `setMaxRows` only; `LIMIT` text is appended only when the client asked for a limit — diff --git a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java index 10cfea54..b4ec6976 100644 --- a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java @@ -212,9 +212,20 @@ public String statementGuardSql(int timeoutSeconds, String serverVersion) { */ @Override public String statementGuardResetSql(String serverVersion) { - return "SET SESSION " + LOCK_GUARD + ", sql_safe_updates = DEFAULT" - + ", sql_mode = DEFAULT, foreign_key_checks = DEFAULT, unique_checks = DEFAULT, time_zone = DEFAULT" - + (isMariaDb(serverVersion) ? "" : ", max_execution_time = DEFAULT"); + // `= @@global.x`, not `= DEFAULT`: measured on MySQL 8.4, `SET SESSION + // foreign_key_checks = DEFAULT` (and unique_checks) leaves the session at 0 while + // the global is 1 — the reset silently failed for exactly the integrity switches. + return "SET SESSION " + LOCK_GUARD + + restore("sql_safe_updates", "sql_mode", "foreign_key_checks", "unique_checks", "time_zone") + + (isMariaDb(serverVersion) ? "" : restore("max_execution_time")); + } + + private static String restore(String... variables) { + StringBuilder sql = new StringBuilder(); + for (String v : variables) { + sql.append(", ").append(v).append(" = @@global.").append(v); + } + return sql.toString(); } private static boolean isMariaDb(String serverVersion) { diff --git a/backend/src/main/java/com/dbaagent/service/ConnectionService.java b/backend/src/main/java/com/dbaagent/service/ConnectionService.java index 8f8bd868..e106638a 100644 --- a/backend/src/main/java/com/dbaagent/service/ConnectionService.java +++ b/backend/src/main/java/com/dbaagent/service/ConnectionService.java @@ -6,6 +6,7 @@ import com.dbaagent.provider.api.ConnectionProvider; import com.dbaagent.provider.api.DatabaseDialect; import com.dbaagent.provider.api.PrivilegeCheckProvider; +import com.dbaagent.provider.api.QueryExecutionProvider; import com.zaxxer.hikari.HikariConfig; import com.zaxxer.hikari.HikariDataSource; import lombok.RequiredArgsConstructor; @@ -295,14 +296,7 @@ private HikariDataSource createConnectionPool(String connectionId, ConnectionReq config.setKeepaliveTime(300_000); // 5 min config.setLeakDetectionThreshold(60000); - // Every connection in the pool gives up on a table lock after a few seconds - // instead of queueing behind it — background jobs included, not only the - // Editor. A statement waiting for a metadata lock makes every later write to - // that table wait too. - String guardSql = dialect.queryExecution().connectionGuardSql(); - if (guardSql != null) { - config.setConnectionInitSql(guardSql); - } + applyGuardrails(config, dialect.queryExecution()); // Connection properties from provider with full SSL config Map connectionProperties = connectionProvider.getConnectionPropertiesWithSsl( @@ -320,6 +314,34 @@ private HikariDataSource createConnectionPool(String connectionId, ConnectionReq return new HikariDataSource(config); } + /** + * Pool-level guardrails, shared with the live MySQL test so it exercises the real config. + * + *
    + *
  • Every connection gives up on a table lock after a few seconds instead of queueing + * behind it — background jobs included. A statement waiting for a metadata lock + * makes every later write to that table wait too.
    • + *
  • HikariCP hardcodes SQLState {@code 0A000} as connection-fatal (verified in 7.1.0), + * and MySQL raises "ALGORITHM=INSTANT is not supported" (1845) with exactly that + * state. Without the override the pool evicted a healthy connection mid-run and the + * INPLACE fallback failed on a closed statement — found only against a real server. + * The dialect decides which errors are statement-level, not the pool.
    • + *
+ */ + public static void applyGuardrails(HikariConfig config, QueryExecutionProvider queryExecution) { + String guardSql = queryExecution.connectionGuardSql(); + if (guardSql != null) { + config.setConnectionInitSql(guardSql); + } + config.setExceptionOverride(new com.zaxxer.hikari.SQLExceptionOverride() { + // Fully qualified: inside this type, `Override` names its nested enum. + @java.lang.Override + public Override adjudicate(java.sql.SQLException e) { + return queryExecution.isOnlineDdlUnsupported(e) ? Override.DO_NOT_EVICT : Override.CONTINUE_EVICT; + } + }); + } + /** * Builds JDBC URL for the connection. * Delegates to the appropriate database provider. diff --git a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLGuardrailLiveTest.java b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLGuardrailLiveTest.java new file mode 100644 index 00000000..b1c0eb05 --- /dev/null +++ b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLGuardrailLiveTest.java @@ -0,0 +1,401 @@ +package com.dbaagent.provider.mysql; + +import com.dbaagent.model.ConnectionRequest; +import com.dbaagent.model.Permission; +import com.dbaagent.model.QueryRequest; +import com.dbaagent.model.QueryResult; +import com.dbaagent.provider.DatabaseProviderRegistry; +import com.dbaagent.provider.api.DatabaseDialect; +import com.dbaagent.repository.KeyColumnAnalysisRepository; +import com.dbaagent.service.CacheMetricsService; +import com.dbaagent.service.ConnectionService; +import com.dbaagent.service.CredentialService; +import com.dbaagent.service.QueryExecutionContext; +import com.dbaagent.service.QueryExecutionPolicyException; +import com.dbaagent.service.QueryExecutionPolicyService; +import com.dbaagent.service.QueryExecutorService; +import com.dbaagent.service.QueryLineageService; +import com.dbaagent.service.RunningQueryRegistry; +import com.dbaagent.service.SchemaSnapshotService; +import com.dbaagent.service.UserDataAccessPolicyService; +import com.dbaagent.service.security.AccessControlService; +import com.dbaagent.service.telemetry.TelemetryCounters; +import com.zaxxer.hikari.HikariConfig; +import com.zaxxer.hikari.HikariDataSource; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInstance; +import org.junit.jupiter.api.condition.EnabledIfEnvironmentVariable; +import org.springframework.cache.CacheManager; + +import java.sql.Connection; +import java.sql.DriverManager; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Statement; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.TimeUnit; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * The production guardrails against a real MySQL server, through the real executor and + * policy. Mocks stand in only for credential lookup and Spring wiring. + * + *

Skipped unless {@code DEEPSQL_MYSQL_TEST_URL} points at a disposable server, e.g. + * {@code jdbc:mysql://127.0.0.1:33084/?user=root&allowPublicKeyRetrieval=true&useSSL=false}. + * It creates and drops its own schema, and toggles {@code @@global.read_only} on that + * server, so never point it at a shared database. + */ +@EnabledIfEnvironmentVariable(named = "DEEPSQL_MYSQL_TEST_URL", matches = ".+") +@TestInstance(TestInstance.Lifecycle.PER_CLASS) +class MySQLGuardrailLiveTest { + + private static final String SCHEMA = "deepsql_guardrail_test"; + private static final String CONN_ID = "live-mysql"; + + private final MySQLQueryExecutionProvider provider = new MySQLQueryExecutionProvider(); + private String baseUrl; + private HikariDataSource pool; + private QueryExecutorService executor; + + @BeforeAll + void setUp() throws Exception { + baseUrl = System.getenv("DEEPSQL_MYSQL_TEST_URL"); + try (Connection c = DriverManager.getConnection(baseUrl); Statement s = c.createStatement()) { + s.execute("DROP DATABASE IF EXISTS " + SCHEMA); + s.execute("CREATE DATABASE " + SCHEMA); + } + + // One connection, so every run reuses it: what one run leaves behind, the next sees. + HikariConfig config = new HikariConfig(); + config.setJdbcUrl(schemaUrl()); + config.setMaximumPoolSize(1); + // The production pool config, not a copy of it: the eviction bug lived here. + ConnectionService.applyGuardrails(config, provider); + pool = new HikariDataSource(config); + + ConnectionRequest request = new ConnectionRequest(); + request.setDbType("mysql"); + CredentialService credentials = mock(CredentialService.class); + when(credentials.getDecryptedConnection(CONN_ID)).thenReturn(request); + ConnectionService connections = mock(ConnectionService.class); + when(connections.getConnection(CONN_ID, request)).thenAnswer(inv -> pool.getConnection()); + + DatabaseDialect dialect = mock(DatabaseDialect.class); + when(dialect.queryExecution()).thenReturn(provider); + DatabaseProviderRegistry registry = mock(DatabaseProviderRegistry.class); + when(registry.getDialect(anyString())).thenReturn(dialect); + when(registry.getCanonicalName(anyString())).thenReturn("mysql"); + + AccessControlService access = mock(AccessControlService.class); + when(access.hasPermission(Permission.EXECUTE_DDL)).thenReturn(true); + UserDataAccessPolicyService dataPolicy = mock(UserDataAccessPolicyService.class); + when(dataPolicy.redactResult(any(), any(), any())).thenAnswer(inv -> inv.getArgument(1)); + + executor = new QueryExecutorService( + connections, credentials, mock(SchemaSnapshotService.class), mock(CacheManager.class), + mock(CacheMetricsService.class), mock(QueryLineageService.class), registry, + new QueryExecutionPolicyService(registry, access), dataPolicy, mock(TelemetryCounters.class), + mock(KeyColumnAnalysisRepository.class), new RunningQueryRegistry()); + } + + @AfterAll + void tearDown() throws Exception { + if (pool != null) { + pool.close(); + } + try (Connection c = DriverManager.getConnection(baseUrl); Statement s = c.createStatement()) { + s.execute("SET GLOBAL read_only = 0"); + s.execute("DROP DATABASE IF EXISTS " + SCHEMA); + } + } + + @BeforeEach + void freshTable() throws Exception { + try (Connection c = raw(); Statement s = c.createStatement()) { + s.execute("DROP TABLE IF EXISTS customers"); + s.execute("CREATE TABLE customers (id INT PRIMARY KEY, email VARCHAR(255) NULL, " + + "contact VARCHAR(255) NULL, note VARCHAR(50) NULL) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4"); + s.execute("INSERT INTO customers VALUES (1,'a@x.io','1',NULL),(2,'b@x.io','2',NULL),(3,'c@x.io','3',NULL)"); + } + } + + // ---- helpers --------------------------------------------------------------------- + + private String schemaUrl() { + int q = baseUrl.indexOf('?'); + return q < 0 ? baseUrl + SCHEMA : baseUrl.substring(0, q) + SCHEMA + baseUrl.substring(q); + } + + private Connection raw() throws SQLException { + return DriverManager.getConnection(schemaUrl()); + } + + private QueryResult run(String sql, QueryExecutionContext context) throws SQLException { + return executor.executeQuery(CONN_ID, new QueryRequest(sql, null, 240), context); + } + + private static QueryExecutionContext confirmedDba() { + return QueryExecutionContext.editor("dba", true, true); + } + + private static String cell(QueryResult result, int col) { + return String.valueOf(result.getRows().get(0).get(col)); + } + + private String columnDefinition(String column) throws SQLException { + try (Connection c = raw(); Statement s = c.createStatement(); ResultSet rs = s.executeQuery( + "SELECT COLUMN_TYPE, IS_NULLABLE FROM information_schema.COLUMNS WHERE TABLE_SCHEMA = '" + SCHEMA + + "' AND TABLE_NAME = 'customers' AND COLUMN_NAME = '" + column + "'")) { + rs.next(); + return rs.getString(1) + " " + rs.getString(2); + } + } + + // ---- the incident ---------------------------------------------------------------- + + @Test + void theServerDefaultIsToWaitAYearForAMetadataLock() throws Exception { + // The root cause, measured rather than recalled. + try (Connection c = DriverManager.getConnection(baseUrl); Statement s = c.createStatement(); + ResultSet rs = s.executeQuery("SELECT @@global.lock_wait_timeout")) { + rs.next(); + assertThat(rs.getLong(1)).isEqualTo(31_536_000L); + } + } + + @Test + void aBlockedAlterGivesUpAfterFiveSecondsAndWritersQueuedBehindItGoThrough() throws Exception { + // An open transaction holds a shared metadata lock, as an app session does. + try (Connection holder = raw()) { + holder.setAutoCommit(false); + try (Statement s = holder.createStatement()) { + s.executeQuery("SELECT * FROM customers WHERE id = 1").close(); + } + + long alterStart = System.nanoTime(); + CompletableFuture alter = CompletableFuture.supplyAsync(() -> { + try { + run("ALTER TABLE customers MODIFY email VARCHAR(512) NOT NULL", confirmedDba()); + return null; + } catch (Throwable t) { + return t; + } + }); + + // A writer arriving while the ALTER waits queues behind its pending exclusive + // lock — the pile-up. It has no guard of its own (an app session). + TimeUnit.SECONDS.sleep(1); + long writerStart = System.nanoTime(); + CompletableFuture writer = CompletableFuture.runAsync(() -> { + try (Connection w = raw(); Statement s = w.createStatement()) { + s.executeUpdate("INSERT INTO customers VALUES (4,'d@x.io','4',NULL)"); + } catch (SQLException e) { + throw new RuntimeException(e); + } + }); + + Throwable alterError = alter.get(30, TimeUnit.SECONDS); + double alterSeconds = (System.nanoTime() - alterStart) / 1e9; + writer.get(30, TimeUnit.SECONDS); + double writerSeconds = (System.nanoTime() - writerStart) / 1e9; + holder.rollback(); + + System.out.printf("[live] ALTER gave up after %.2fs; queued writer finished after %.2fs%n", + alterSeconds, writerSeconds); + assertThat(alterError).isInstanceOf(QueryExecutionPolicyException.class); + assertThat(((QueryExecutionPolicyException) alterError).getErrorCode()) + .isEqualTo(QueryExecutionPolicyException.DB_GUARDRAIL_TRIPPED); + assertThat(alterError.getMessage()).contains("5 seconds"); + assertThat(alterSeconds).isBetween(4.5, 9.0); + // The writer waited (it really was queued) but only until the ALTER gave up. + assertThat(writerSeconds).isBetween(2.0, 9.0); + assertThat(columnDefinition("email")).isEqualTo("varchar(255) YES"); + } + } + + @Test + void theIncidentAlterRunsOnlineWhenNothingHoldsTheTable() throws Exception { + // INSTANT cannot change NULL-ability: 1846, then INPLACE with LOCK=NONE succeeds. + run("ALTER TABLE customers MODIFY email VARCHAR(512) NOT NULL", confirmedDba()); + assertThat(columnDefinition("email")).isEqualTo("varchar(512) NO"); + } + + @Test + void anInstantChangeRunsInstant() throws Exception { + run("ALTER TABLE customers ADD COLUMN tier TINYINT NULL", confirmedDba()); + assertThat(columnDefinition("tier")).isEqualTo("tinyint YES"); + } + + @Test + void aChangeThatCannotRunOnlineChangesNothingAndSaysWhy() throws Exception { + // INT -> BIGINT needs a table copy: refused by INSTANT and by INPLACE. + QueryExecutionPolicyException e = assertThrows(QueryExecutionPolicyException.class, + () -> run("ALTER TABLE customers MODIFY id BIGINT", confirmedDba())); + System.out.println("[live] " + e.getMessage()); + assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.DB_GUARDRAIL_TRIPPED); + assertThat(e.getMessage()).contains("gh-ost"); + assertThat(columnDefinition("id")).isEqualTo("int NO"); + } + + // ---- session guards -------------------------------------------------------------- + + @Test + void everyPooledConnectionStartsWithTheLockGuard() throws Exception { + try (Connection c = pool.getConnection(); Statement s = c.createStatement(); + ResultSet rs = s.executeQuery("SELECT @@session.lock_wait_timeout, @@session.innodb_lock_wait_timeout")) { + rs.next(); + assertThat(rs.getLong(1)).isEqualTo(5); + assertThat(rs.getLong(2)).isEqualTo(5); + } + } + + @Test + void aPreambleCannotLoosenTheGuardsForTheStatement() throws Exception { + QueryResult result = run("SET SESSION lock_wait_timeout = 31536000; " + + "SELECT @@session.lock_wait_timeout, @@session.sql_safe_updates, @@session.max_execution_time", + QueryExecutionContext.editor("analyst", false, false)); + assertThat(cell(result, 0)).isEqualTo("5"); + assertThat(cell(result, 1)).isEqualTo("1"); + // A writable server: reads capped at 30s whatever the client asked for (240s). + assertThat(cell(result, 2)).isEqualTo("30000"); + } + + @Test + void nothingAUserSetLeaksToTheNextBorrower() throws Exception { + run("SET SESSION foreign_key_checks = 0; SET SESSION sql_mode = ''; SELECT 1", + QueryExecutionContext.editor("dba", true, false)); + + try (Connection c = pool.getConnection(); Statement s = c.createStatement(); ResultSet rs = s.executeQuery( + "SELECT @@session.foreign_key_checks, @@session.sql_mode = @@global.sql_mode, " + + "@@session.sql_safe_updates, @@session.max_execution_time, @@session.lock_wait_timeout")) { + rs.next(); + assertThat(rs.getInt(1)).as("foreign_key_checks").isEqualTo(1); + assertThat(rs.getInt(2)).as("sql_mode restored").isEqualTo(1); + assertThat(rs.getInt(3)).as("sql_safe_updates").isEqualTo(0); + assertThat(rs.getLong(4)).as("max_execution_time").isEqualTo(0); + assertThat(rs.getLong(5)).as("lock_wait_timeout baseline").isEqualTo(5); + } + } + + @Test + void aReplicaGetsTheClientTimeoutNotTheThirtySecondCap() throws Exception { + try (Connection c = DriverManager.getConnection(baseUrl); Statement s = c.createStatement()) { + s.execute("SET GLOBAL read_only = 1"); + } + try { + QueryResult result = run("SELECT @@session.max_execution_time", + QueryExecutionContext.editor("analyst", false, false)); + assertThat(cell(result, 0)).isEqualTo("240000"); + } finally { + try (Connection c = DriverManager.getConnection(baseUrl); Statement s = c.createStatement()) { + s.execute("SET GLOBAL read_only = 0"); + } + } + } + + @Test + void maxExecutionTimeReallyStopsAReadAndIsExplained() throws Exception { + try (Connection c = raw(); Statement s = c.createStatement()) { + s.execute("SET SESSION max_execution_time = 300"); + SQLException e = assertThrows(SQLException.class, () -> s.executeQuery( + "SELECT COUNT(*) FROM information_schema.COLUMNS a, information_schema.COLUMNS b, " + + "information_schema.COLUMNS c").close()); + assertThat(e.getErrorCode()).isEqualTo(3024); + assertThat(provider.guardrailMessage(e)).contains("time limit"); + } + } + + @Test + void safeUpdatesRefusesAWriteThatDoesNotFilterOnAKey() throws Exception { + QueryExecutionPolicyException e = assertThrows(QueryExecutionPolicyException.class, + () -> run("UPDATE customers SET note = 'x' WHERE note IS NULL", confirmedDba())); + assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.DB_GUARDRAIL_TRIPPED); + assertThat(e.getMessage()).contains("indexed"); + + QueryResult keyed = run("UPDATE customers SET note = 'x' WHERE id = 1", confirmedDba()); + assertThat(keyed.getRowCount()).isEqualTo(1); + } + + @Test + void aRowLockWaitAlsoGivesUpAfterFiveSeconds() throws Exception { + try (Connection holder = raw()) { + holder.setAutoCommit(false); + try (Statement s = holder.createStatement()) { + s.executeUpdate("UPDATE customers SET note = 'held' WHERE id = 2"); + } + long start = System.nanoTime(); + QueryExecutionPolicyException e = assertThrows(QueryExecutionPolicyException.class, + () -> run("UPDATE customers SET note = 'mine' WHERE id = 2", confirmedDba())); + double seconds = (System.nanoTime() - start) / 1e9; + holder.rollback(); + System.out.printf("[live] row lock wait gave up after %.2fs%n", seconds); + assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.DB_GUARDRAIL_TRIPPED); + assertThat(seconds).isBetween(4.5, 9.0); + } + } + + // ---- what the policy refuses, and why it must ------------------------------------ + + @Test + void mysqlReallyExecutesTheBodyOfAnExecutableComment() throws Exception { + // The threat model, proven: the server runs it, while a comment-stripping check sees nothing. + try (Connection c = raw(); Statement s = c.createStatement()) { + s.execute("/*!50000 CREATE TABLE exec_comment_proof (id INT) */"); + try (ResultSet rs = s.executeQuery("SELECT COUNT(*) FROM information_schema.TABLES WHERE TABLE_SCHEMA = '" + + SCHEMA + "' AND TABLE_NAME = 'exec_comment_proof'")) { + rs.next(); + assertThat(rs.getInt(1)).isEqualTo(1); + } + s.execute("DROP TABLE exec_comment_proof"); + } + QueryExecutionPolicyException e = assertThrows(QueryExecutionPolicyException.class, + () -> run("/*!50000 CREATE TABLE exec_comment_proof (id INT) */", confirmedDba())); + assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + } + + @Test + void setGlobalIsRefusedEvenForAConfirmedAdmin() { + QueryExecutionPolicyException e = assertThrows(QueryExecutionPolicyException.class, + () -> run("SET GLOBAL max_connections = 10", QueryExecutionContext.editor("admin", true, true))); + assertThat(e.getErrorCode()).isEqualTo(QueryExecutionPolicyException.ADMIN_STATEMENT_BLOCKED); + } + + @Test + void theActorTagReachesTheServer() throws Exception { + // What the slow log and Performance Insights would see. + run("SELECT 'tag-probe' AS probe", QueryExecutionContext.editor("dba", true, false)); + try (Connection c = raw(); Statement s = c.createStatement(); ResultSet rs = s.executeQuery( + // events_statements_history (last N per thread) is on by default; _long is not. + "SELECT SQL_TEXT FROM performance_schema.events_statements_history " + + "WHERE SQL_TEXT LIKE '%tag-probe%' AND SQL_TEXT NOT LIKE '%events_statements%' " + + "ORDER BY EVENT_ID DESC LIMIT 1")) { + assertThat(rs.next()).as("statement recorded").isTrue(); + assertThat(rs.getString(1)).startsWith("/* deepsql origin=editor user=dba */ SELECT 'tag-probe'"); + } + } + + @Test + void theOnlineAttemptsAreValidSqlOnThisServer() { + // Guards against appended-clause syntax drift across MySQL versions. + assertThat(provider.onlineDdlAttempts("ALTER TABLE customers ADD COLUMN y INT -- trailing")) + .allSatisfy(sql -> assertThat(sql).contains("\n, ALGORITHM=")); + assertThat(List.of(provider.statementGuardSql(240, "8.4.11"), provider.statementGuardResetSql("8.4.11"))) + .allSatisfy(sql -> { + try (Connection c = raw(); Statement s = c.createStatement()) { + s.execute(sql); + } catch (SQLException ex) { + throw new AssertionError(sql + " -> " + ex.getMessage(), ex); + } + }); + } +} diff --git a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java index 74059889..e21df651 100644 --- a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java +++ b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java @@ -149,10 +149,13 @@ void mariaDbGetsNoMaxExecutionTimeBecauseItDoesNotHaveOne() { @Test void theResetRestoresThePoolBaselineSoNothingLeaksToTheNextBorrower() { // Also the settings a preamble could use to weaken the next borrower's writes. + // `= @@global.x`, not `= DEFAULT`: on MySQL 8.4, `foreign_key_checks = DEFAULT` + // leaves the session at 0 (see MySQLGuardrailLiveTest). assertThat(provider.statementGuardResetSql("8.0.36")).isEqualTo( - "SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5, sql_safe_updates = DEFAULT, " - + "sql_mode = DEFAULT, foreign_key_checks = DEFAULT, unique_checks = DEFAULT, time_zone = DEFAULT, " - + "max_execution_time = DEFAULT"); + "SET SESSION lock_wait_timeout = 5, innodb_lock_wait_timeout = 5, " + + "sql_safe_updates = @@global.sql_safe_updates, sql_mode = @@global.sql_mode, " + + "foreign_key_checks = @@global.foreign_key_checks, unique_checks = @@global.unique_checks, " + + "time_zone = @@global.time_zone, max_execution_time = @@global.max_execution_time"); } @Test From ebd0375427fac4f6e792aceaec8482ba520d075c Mon Sep 17 00:00:00 2001 From: geekypunk Date: Sat, 7 Nov 2026 12:44:49 -0600 Subject: [PATCH 6/6] fix: make MySQL ALGORITHM/LOCK clause patterns linear (CodeQL polynomial-redos) `\s*=?\s*` let the engine split a long run of spaces between the two quantifiers every possible way before failing. Possessive quantifiers keep the same matches in linear time. Co-Authored-By: Claude Opus 5.5 --- .../provider/mysql/MySQLQueryExecutionProvider.java | 6 ++++-- .../mysql/MySQLQueryExecutionProviderGuardTest.java | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java index b4ec6976..a6a6f637 100644 --- a/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java +++ b/backend/src/main/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProvider.java @@ -149,10 +149,12 @@ public String getSessionPidQuery() { private static final Pattern ALTER_TABLE = Pattern.compile( "^ALTER\\s+(?:ONLINE\\s+|IGNORE\\s+)?TABLE\\b", Pattern.CASE_INSENSITIVE); // Value-anchored so a column named `algorithm` or `lock` does not read as a clause. + // Possessive: `\s*=?\s*` lets the engine split a long run of spaces between the two + // `\s*` every possible way before failing — quadratic (CodeQL java/polynomial-redos). private static final Pattern ALGORITHM_CLAUSE = Pattern.compile( - "\\bALGORITHM\\s*=?\\s*(DEFAULT|INSTANT|INPLACE|COPY)\\b", Pattern.CASE_INSENSITIVE); + "\\bALGORITHM\\s*+(?:=\\s*+)?(DEFAULT|INSTANT|INPLACE|COPY)\\b", Pattern.CASE_INSENSITIVE); private static final Pattern LOCK_CLAUSE = Pattern.compile( - "\\bLOCK\\s*=?\\s*(DEFAULT|NONE|SHARED|EXCLUSIVE)\\b", Pattern.CASE_INSENSITIVE); + "\\bLOCK\\s*+(?:=\\s*+)?(DEFAULT|NONE|SHARED|EXCLUSIVE)\\b", Pattern.CASE_INSENSITIVE); private static final Pattern PARTITION_CLAUSE = Pattern.compile("\\bPARTITION(?:S|ING)?\\b", Pattern.CASE_INSENSITIVE); // One left-to-right pass over literals, identifiers and comments: at each position the // first alternative that starts there wins, so a quote inside a comment, or a comment diff --git a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java index e21df651..14f296d5 100644 --- a/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java +++ b/backend/src/test/java/com/dbaagent/provider/mysql/MySQLQueryExecutionProviderGuardTest.java @@ -116,7 +116,9 @@ void perStatementGuardCapsReadsAtThirtySecondsOnlyOnAWritableServer() { @Test void clauseDetectionStaysLinearOnUnterminatedCommentsAndQuotes() { // A lazy /\*.*?\*/ rescans to the end from every unterminated opener. - for (String tail : List.of("/*" + "a/*".repeat(32_000), "'" + "a\\'".repeat(32_000), "`" + "a".repeat(64_000))) { + for (String tail : List.of("/*" + "a/*".repeat(32_000), "'" + "a\\'".repeat(32_000), "`" + "a".repeat(64_000), + // CodeQL java/polynomial-redos: ALGORITHM followed by a long run of spaces and no value. + ("ALGORITHM" + " ".repeat(2_000) + "x ").repeat(25), ("LOCK" + " ".repeat(2_000) + "x ").repeat(25))) { long startedAt = System.currentTimeMillis(); provider.onlineDdlAttempts("ALTER TABLE t ADD COLUMN x INT " + tail); assertThat(System.currentTimeMillis() - startedAt).as("must not backtrack").isLessThan(5_000L);