Repository navigation
fix: production database guardrails against lock pile-ups and unsafe SQL - #130
Merged
Merged
Conversation
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 <[email protected]>
geekypunk
requested review from
a team and
venkateshsakamuri-lab
as code owners
November 7, 2026 16:22
…e 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 <[email protected]>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- 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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
…ial-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 <[email protected]>
3 of 4 tasks
geekypunk
added a commit
that referenced
this pull request
Nov 7, 2026
…an write (#131) ## Why Writes were gated on the built-in **ADMIN or DBA role name**, in both `AccessControlService.currentUserMayMutateSql` and the frontend's `useAuth`. A custom role could never write. That made it impossible to give `EXECUTE_DDL` (added in #130) to only some DBAs: a custom "DBA + schema changes" role would lose UPDATE/INSERT/DELETE entirely. ## What changes - **New `EXECUTE_WRITES` permission**, held by **ADMIN and DBA by default**, so no one's access changes on deploy. - **The backend and frontend gates check the permission, not the role name.** A DBA whose `EXECUTE_WRITES` is revoked by an override can no longer write; a custom role granted it can. - **No re-login needed.** Every authentication path (JWT, MCP token, View-as) loads effective permissions as authorities per request, so the check takes effect immediately. - **The Editor's "DBA mode" label is now "Write mode"**, since custom roles can see it. ## Test plan - [x] `AccessControlServiceTest` uses production-shaped authorities (`ROLE_*` plus permission names), with new cases for a custom role granted writes and a role name alone granting nothing - [x] 80 tests across the auth, policy and safety-scanner suites - [x] eslint: 0 errors - [ ] After deploy, create a custom role with DBA permissions plus `EXECUTE_WRITES` and `EXECUTE_DDL`, and assign it to the DBAs who need schema changes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 5.5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A confirmed Editor
ALTER TABLE … MODIFY … NOT NULLon a production MySQL primary queued every write on the table for exactly 240s. That was the Editor's client timeout, and it was the only thing that stopped the ALTER: MySQL's defaultlock_wait_timeoutis one year (measured, 31,536,000s on MySQL 8.4). The statement was cancelled, so the schema change never landed. DeepSQL's audit log recorded who ran it; the shared database login could not.This PR makes the guardrails apply to every role, admins included. Along the way it closes several holes that were already on
main, two of them serious (see below).What changes
Lock pile-ups and long reads
lock_wait_timeout+innodb_lock_wait_timeout, Postgreslock_timeout), background jobs includedConnectionService.applyGuardrails→ HikariconnectionInitSqlsql_safe_updates=1andmax_execution_time: 30s on a writable server, the client timeout on a replica (@@global.read_only OR @@global.innodb_read_only, so Aurora readers count). Applied before and after user preambles; fails closedQueryExecutorService,QueryExecutionProvider.statementGuardSqlRESET ALL; MySQL restores the guards plussql_mode,foreign_key_checks,unique_checks,time_zonefrom@@globalstatementGuardResetSqlALGORITHM=INSTANT, falling back toINPLACE, LOCK=NONE(also on 5.7 / Aurora 2, which reject INSTANT as a syntax error).COPY/DEFAULT/SHARED/EXCLUSIVEand partition changes without an explicit clause are refused before the confirmation promptonlineDdlAttempts, policy + executorCREATE/DROP INDEX CONCURRENTLYruns withlock_timeout = 0on every path, so it is never cut off midway (which would leave an INVALID index)QueryExecutionProvider.ddlSessionSqlWho may do what
EXECUTE_DDLpermission (ADMIN by default) for any mutation that isn't row-only DML (DDL,CALL,DO,SELECT … INTO,EXPLAIN ANALYZE CREATE …); also required by Index AdvisorAPPLYPermission, policy,IndexRecommendationControllersql_log_bin, Postgres role/identity switches (SET [SESSION|LOCAL] ROLE,SET SESSION AUTHORIZATION,session_replication_role)QueryExecutionPolicyService/*! */, MariaDB/*M! */) and optimizer hints (/*+ */) refused/* deepsql origin=… user=… */on every statement, so the slow log, processlist and Performance Insights attribute load to a personapplication.propertiesHoles on
mainthis closesSELECT dblink_exec(…)(which gets out of the read-only session) andpg_read_file(…)as plain reads, for any role. The denylist now runs on every surface and includespg_terminate_backend/pg_cancel_backend. The Java and MCP JS lists are kept in sync.McpSqlGuardServicecomment stripping was quadratic./\*[\s\S]*?\*/rescans to the end of the input from every unterminated/*, so"a/*"× 32k took minutes of CPU per request on the MCP and public dashboard query paths. It is now a linear scan, shared with the policy.SET GLOBAL …; SELECT 1ran for read-only roles, because everySETwas classified as a harmless session preamble./*!50000 GRANT … */skipped every check. MySQL executes the body; the classifier stripped it as a comment and called the empty remainder read-only.SET @a = "'", GLOBAL x = 1, @b = "'", andNO_BACKSLASH_ESCAPES). The SET scope check now reads the raw text and fails closed.Found only against a real MySQL 8.4
MySQLGuardrailLiveTestruns the real executor, policy and pool config against a live server. It is skipped unlessDEEPSQL_MYSQL_TEST_URLis set. It found two bugs that no mocked test could:0A000as connection-fatal (verified in 7.1.0), and MySQL raises "ALGORITHM=INSTANT is not supported" (1845) with exactly that state. The pool evicted a healthy connection mid-run, so the INSTANT→INPLACE fallback never worked through the real pool.applyGuardrailsnow sets aSQLExceptionOverridethat asks the dialect.SET SESSION foreign_key_checks = DEFAULTleaves the session at 0 on MySQL 8.4 (unique_checkstoo). The reset restores from@@globalinstead.Measured on MySQL 8.4.11:
MODIFY … VARCHAR(512) NOT NULLINPLACE, LOCK=NONE)MODIFY id BIGINTSET SESSION lock_wait_timeout = 31536000foreign_key_checks = 0Rollout notes
EXECUTE_DDL(override or custom role) to the DBAs who need it before deploying.sql_safe_updates=1rejects UPDATE/DELETE that don't filter on an indexed column and have no LIMIT. The error message explains why.EXPLAIN ANALYZEof a write needsEXECUTE_DDL. That's deliberate: Postgres executes the wrapped statement, and the wrapped-verb detection can be fooled by column names.COM_RESET_CONNECTIONwould re-authenticate on every run.mcp/deepsql-phase1-lib.jsgained two denylist entries, so it needs a patch release. The backend enforces the same list regardless.Test plan
MySQLQueryExecutionProviderGuardTest,PostgresQueryExecutionProviderGuardTest,QueryExecutionPolicyProdGuardTest,QueryExecutorGuardrailTestandMcpSqlGuardServiceTest(linear-time checks on hostile input)MySQLGuardrailLiveTest16/16main(no database / Spring env in this environment, plus 18 pre-existing assertion failures)EXECUTE_DDLbefore deploy🤖 Generated with Claude Code