refactor: enforce read-only queries via SQLite PRAGMA instead of regex - #13
Merged
Conversation
Replace the keyword blocklist in assertReadOnlyQuery with engine-level enforcement: withSaveDb now runs `PRAGMA query_only = ON` on the in-memory database, so SQLite itself rejects any write — including cases a text scan would miss, such as a `WITH … DELETE` CTE. The static guard keeps only what the engine can't cover: rejecting stacked statements (which also shuts out ATTACH/DETACH) and giving a fast, friendly error for a non-SELECT/WITH opener. This removes the regex false positives, so legitimate reads like `SELECT REPLACE(...)` or `LIKE '%create%'` now work. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors read-only enforcement for save SQL queries by moving from a regex-based keyword blocklist to SQLite engine-level enforcement via PRAGMA query_only = ON, while keeping a small static guard to prevent stacked statements and non-SELECT/WITH openers.
Changes:
- Enable SQLite read-only mode (
PRAGMA query_only = ON) for everywithSaveDbsession. - Simplify
assertReadOnlyQueryto focus on single-statement + opener validation, and map engine read-only failures to a clearer user message. - Update/extend tests to reflect the new enforcement approach and eliminate prior false positives.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/save-db.ts |
Sets PRAGMA query_only = ON on the in-memory DB before tool callbacks run. |
src/tools/query-save.ts |
Removes regex keyword blocklist; relies on engine enforcement and adds friendlier error mapping. |
test/save-db.test.ts |
Updates DB mock to include run() and adds an order/assertion test for PRAGMA configuration. |
test/query-save.test.ts |
Updates guard tests to accept previously-false-positive read queries and reflect engine enforcement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…rals, comments, and quoted identifiers
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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
Replaces the regex keyword blocklist in
assertReadOnlyQuerywith two complementary layers:withSaveDbrunsPRAGMA query_only = ONon the in-memory database, so SQLite itself rejects any write. This is the reliable backstop.assertReadOnlyQuerynow delegates SQL parsing tosql-query-identifierinstead of scanning the raw query text, and requires the input to be exactly one read (LISTING) statement.Together this removes the false positives the blocklist produced on legitimate read queries, and rejects write attempts up front — including ones a text scan would miss, such as a
WITH … DELETECTE, or a;hidden inside a string literal.What changed
src/save-db.ts— applyPRAGMA query_only = ONright after loading the save, before the callback runs any query.src/tools/query-save.ts—assertReadOnlyQueryusessql-query-identifier'sidentify()(non-strict,sqlitedialect) to reject empty input, stacked statements, and anything whose single statement isn't a read (executionType !== "LISTING"). Because it tokenizes SQL properly, a;inside a string literal, comment or quoted identifier is no longer mistaken for a statement separator, and CTEs are classified by their leaf operation (WITH … SELECTreads,WITH … DELETEwrites).explainQueryErrorstill maps the engine's read-only error and missing-table/column errors to clear messages.package.json— add thesql-query-identifierdependency (~100 KB, no runtime deps).Behaviour
WITH x AS (...) DELETE FROM fooSELECT ';' AS x/... WHERE note = 'a;b'SELECT REPLACE('a','b','c')... LIKE '%create%'ATTACH/PRAGMA/ DDL openersPRAGMA query_only = ONremains the engine-level backstop, so even a write that somehow reached the database would still fail withattempt to write a readonly database.