fix: enforce read-only mode at the database engine, not just the keyword classifier#342
Merged
Merged
Conversation
…ord classifier Read-only mode was enforced only by a leading-keyword classifier, which a real database parses differently than our string checks. The connection-level `config.readonly` backstop existed but was never wired (it read a non-existent `source.readonly` field), so the classifier was the sole layer. This let function-based and dialect-specific writes through in read-only mode. Add engine-level read-only enforcement scoped per tool via `options.readonly` (already plumbed into every executeSQL), so the database itself rejects writes the classifier missed: - Postgres: run read-only calls inside `BEGIN READ ONLY` (single + multi path). Blocks `SELECT setval()`, INSERT/UPDATE/DELETE, data-modifying CTEs. - SQLite: toggle `PRAGMA query_only=ON` around read-only calls. Blocks header-writing pragmas and other writes on the shared connection. - MySQL/MariaDB: wrap read-only calls in `START TRANSACTION READ ONLY`. Blocks DML writes. (DDL implicitly commits and escapes the transaction; stacked-DDL payloads are handled by the classifier fix below.) Harden the classifier (no SQL parser required): - SQLite: assignment-form `PRAGMA x = ...` is now classified as a write. - MySQL/MariaDB: `--` only starts a comment when followed by whitespace/control/ EOL, matching the engine. `SELECT 1--1;DROP TABLE t` now splits into two statements so the hidden DROP is rejected before execution. Remove the dead `source.readonly` wiring in manager.ts; document that read-only is enforced per-tool at execution time, not per-source at the connection level. Addresses GHSA-mwwr-p57h-56pf, GHSA-j656-3hf2-fvjc, GHSA-m689-287g-5xpc, GHSA-7rgf-cwgq-c2qc. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens DBHub’s read-only guarantees by enforcing read-only behavior at the database engine level (per-tool via ExecuteOptions.readonly) rather than relying solely on keyword-based classification, and hardens the existing classifier against known bypasses.
Changes:
- Added engine-level read-only backstops in PostgreSQL (
BEGIN READ ONLY), SQLite (PRAGMA query_only=ONaround execution), and MySQL/MariaDB (START TRANSACTION READ ONLY). - Hardened read-only classification/splitting for SQLite
PRAGMA ... = ...writes and MySQL/MariaDB--comment semantics. - Removed dead per-source readonly wiring and added unit + integration tests for the new enforcement paths.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/sql-parser.ts | Dialect-correct MySQL/MariaDB -- comment scanning to prevent statement-hiding bypasses. |
| src/utils/allowed-keywords.ts | Classifier hardening for SQLite assignment-form PRAGMA to avoid misclassifying writes as read-only. |
| src/utils/tests/allowed-keywords.test.ts | Unit coverage for SQLite PRAGMA assignment rejection and MySQL/MariaDB -- statement-splitting behavior. |
| src/connectors/sqlite/index.ts | Enforces read-only per execution via PRAGMA query_only, restoring writability afterward. |
| src/connectors/postgres/index.ts | Enforces read-only per execution by wrapping statements/batches in BEGIN READ ONLY. |
| src/connectors/mysql/index.ts | Enforces read-only per execution using START TRANSACTION READ ONLY plus commit/rollback handling. |
| src/connectors/mariadb/index.ts | Same as MySQL: per-execution read-only transaction backstop with commit/rollback handling. |
| src/connectors/manager.ts | Removes dead source.readonly mapping; documents per-tool enforcement model. |
| src/connectors/tests/sqlite.integration.test.ts | Integration coverage for SQLite per-tool options.readonly engine backstop and state restoration. |
| src/connectors/tests/postgres.integration.test.ts | Integration coverage for Postgres per-tool read-only transaction enforcement (e.g., setval()). |
| src/connectors/tests/mysql.integration.test.ts | Integration coverage for MySQL per-tool read-only transaction blocking DML and preserving later writability. |
| src/connectors/tests/mariadb.integration.test.ts | Integration coverage for MariaDB per-tool read-only transaction blocking DML and preserving later writability. |
…error Address Copilot review feedback on PR #342: - Postgres read-only branch: wrap ROLLBACK in try/catch (a failed rollback, e.g. on a dropped connection, no longer masks the original query error) and log parameters on error to match the non-read-only path. - SQLite: make the `PRAGMA query_only = OFF` restore in finally best-effort so it can't mask the expected read-only rejection. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mt rollback Address second Copilot review pass on PR #342: - sql-parser: MySQL/MariaDB `--` starts a comment when followed by a control char too, not just bytes <= 0x20. MySQL's lexer uses my_isspace()||my_iscntrl(), which includes ASCII DEL (0x7F). Add a unit test for the DEL case. - postgres: make the multi-statement transaction ROLLBACK best-effort too, so a failed rollback can't mask the original error (matches the single-statement path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ry_only toggle Address third Copilot review pass on PR #342: - Classifier: SQLite accepts `PRAGMA name(value)` as an alias for `PRAGMA name = value`, so the `=`-only check missed parens-form writes (e.g. `PRAGMA user_version(1337)`, `PRAGMA query_only(0)`). Now reject the parenthesized form unless the pragma is a known introspection pragma (table_info/index_info/index_list/foreign_key_list). - Engine: re-assert `PRAGMA query_only = ON` before each statement in the multi-statement path so a mid-batch `PRAGMA query_only = OFF` (or `query_only(0)`) cannot disable the backstop for later statements — the engine layer is now self-sufficient even if a caller skips the keyword classifier. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
tianzhou
added a commit
that referenced
this pull request
Jun 24, 2026
Release containing the read-only enforcement hardening (#342): engine-level read-only backstops (Postgres BEGIN READ ONLY, SQLite query_only, MySQL/MariaDB START TRANSACTION READ ONLY) plus classifier hardening for SQLite write-pragmas and MySQL/MariaDB `--` comment parsing. Merging this to main triggers the npm trusted-publish workflow. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Read-only mode was enforced only by a leading-keyword classifier. A real database parses SQL differently than our string checks, so several writes slipped through in read-only mode. The connection-level
config.readonlybackstop existed but was never wired up —manager.tsread asource.readonlyfield that doesn't exist onSourceConfigand is rejected by the TOML loader — so the classifier was the only live layer.This PR adds engine-level read-only enforcement, scoped per-tool via
options.readonly(already plumbed into everyexecuteSQL), so the database itself rejects writes the classifier missed. The classifier stays as a best-effort first layer and gets two parser-free hardening fixes.Changes
Engine-level enforcement (per tool, respects mixed read-only/writable tools on a shared connection):
BEGIN READ ONLY(single + multi path)SELECT setval(), INSERT/UPDATE/DELETE, data-modifying CTEsPRAGMA query_only=ONaround the callwal_checkpoint, all writesSTART TRANSACTION READ ONLYClassifier hardening (no SQL parser introduced):
PRAGMA x = ...now classified as a write.--only begins a comment when followed by whitespace/control/EOL, matching the engine — soSELECT 1--1;DROP TABLE tsplits into two statements and the hidden DROP is rejected before execution.Cleanup: removed the dead
source.readonlywiring; documented that read-only is enforced per-tool at execution time.Known limitation (documented in code)
On MySQL/MariaDB, DDL (
DROP/CREATE) performs an implicit commit that ends the read-only transaction, so DDL escapes the engine backstop. Stacked-DDL payloads are instead caught upstream by the classifier comment-scanner fix. The engine transaction reliably blocks DML.The privileged-role Postgres vectors (
lo_export,dblink,COPY ... TO PROGRAM) are not closeable by a read-only transaction and remain a least-privilege / documentation concern.Testing
---comment classifier fixes; full unit suite green (pre-existing env-specific ssh-config/env failures unrelated).setval(), stacked--UPDATE, INSERT, and PRAGMA writes are rejected, and the shared connection stays writable for non-read-only tools.pnpm run build:backend: success.Addresses GHSA-mwwr-p57h-56pf, GHSA-j656-3hf2-fvjc, GHSA-m689-287g-5xpc, GHSA-7rgf-cwgq-c2qc.
🤖 Generated with Claude Code