fix: enforce read-only mode at the engine level for SQL Server#350
Merged
Conversation
SQL Server was the only connector without engine-level read-only
enforcement — it relied entirely on the keyword classifier.
Two changes:
1. Keyword classifier: add EXEC and EXECUTE to the SQL Server mutating
keyword pattern so dynamic SQL primitives are rejected in CTEs
(e.g. WITH cte AS (SELECT 1) EXEC('DELETE ...')).
2. Connector: wrap read-only queries in a transaction that always rolls
back. SQL Server has no BEGIN TRANSACTION READ ONLY, so this
unconditional ROLLBACK is the defense-in-depth backstop — even if
a classifier bypass reaches the engine, no modifications persist.
Closes #349
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens SQL Server read-only enforcement in DBHub’s execute_sql tool to prevent dynamic SQL from bypassing read-only protections, adding both classifier hardening and an engine-level rollback backstop.
Changes:
- Add a SQL Server–specific mutating keyword pattern to classify
EXEC/EXECUTEusage as non-read-only (including withinWITH/CTE statements). - Add a SQL Server connector backstop for
readonly: trueby executing within a transaction that always rolls back. - Add unit tests covering SQL Server
EXEC/EXECUTEread-only blocking scenarios.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/utils/allowed-keywords.ts |
Adds a SQL Server–specific mutating keyword regex used when validating WITH statements. |
src/utils/__tests__/allowed-keywords.test.ts |
Adds unit tests intended to prevent dynamic-SQL read-only bypasses for SQL Server. |
src/connectors/sqlserver/index.ts |
Adds a read-only execution path that wraps queries in a transaction and always rolls back. |
The executeReadOnly() rollback guard is bypassable if the user's SQL contains COMMIT — it ends the outer transaction before the finally-block ROLLBACK runs. Scan the stripped SQL for COMMIT/ROLLBACK and reject before opening the transaction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…gging Address Copilot review: - Add sp_executesql and xp_cmdshell to SQL Server mutating keyword pattern (callable without EXEC as first statement in a batch) - Add parameter-aware error logging to executeReadOnly() matching the non-readonly path - Add tests for implicit sp_executesql and xp_cmdshell in CTEs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
EXEC('COMMIT') would bypass the COMMIT/ROLLBACK keyword guard because
stripCommentsAndStrings removes the string content. Block EXEC, EXECUTE,
sp_executesql, and xp_cmdshell at the connector level so dynamic SQL
carrying hidden transaction control cannot escape the rollback.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Track whether the query itself failed so the finally block can distinguish a rollback error after a successful query (dangerous — data may have persisted) from one after a query error (safe to suppress so the original error propagates). Also removes the redundant outer try block. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
tianzhou
commented
Jun 30, 2026
tianzhou
left a comment
tianzhou
left a comment
Member
Author
There was a problem hiding this comment.
Addressed Copilot review comment (rollback error handling) in 46e803d.
The finally block now tracks whether the query itself failed via a queryFailed flag:
- Query succeeded + rollback fails: throws
"Read-only rollback failed — data may have been modified: ..."— caller is alerted that the read-only guarantee could not be verified. - Query failed + rollback fails: suppresses the rollback error so the original query error propagates unchanged.
Also removed the redundant outer try block that had no catch/finally after the restructuring.
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.
Summary
Closes #349 — SQL Server read-only mode bypass via dynamic SQL (
EXEC('DELETE ...')).SQL Server was the only connector without engine-level read-only enforcement. Two fixes:
Keyword classifier: Add
EXECandEXECUTEto the SQL Server mutating keyword pattern so dynamic SQL primitives are rejected — both as standalone statements and inside CTEs (e.g.WITH cte AS (SELECT 1) EXEC('DELETE ...')).sp_executesqlandxp_cmdshellare always invoked viaEXECso are covered transitively.Connector engine-level backstop: Wrap read-only queries in a
sql.Transactionthat always rolls back. SQL Server has noBEGIN TRANSACTION READ ONLY, so this unconditionalROLLBACKis the defense-in-depth backstop — even if a classifier bypass reaches the engine, no modifications persist. Matches the pattern used by PostgreSQL (BEGIN READ ONLY), MySQL/MariaDB (START TRANSACTION READ ONLY), and SQLite (PRAGMA query_only = ON).Test plan
pnpm run build:backend)pnpm test:integration) — verifiesEXEC('DELETE ...')is rolled back underreadonly: true🤖 Generated with Claude Code