Follow-up to #1 (thanks @Mann1ng).
PR #1 closed the writable-CTE / subquery-DML / inline multi-statement bypasses by broadening _DESTRUCTIVE_PATTERN to match destructive keywords anywhere in the statement. That's the right security default, but it can false-positive on legitimate read-only queries that contain those keywords in string literals or identifiers, e.g.:
SELECT * FROM audit_log WHERE action = 'DELETE'
SELECT call FROM phone_log
The proper fix is recommendation #3 from docs/security-analysis.md: parse the statement into an AST (e.g. with sqlglot) and walk it for DML/DDL nodes anywhere in the tree — CTEs, subqueries, and multi-statement input included. That keeps the guard airtight without blocking legit queries.
Acceptance:
_is_read_only() uses AST inspection, regex kept only as a fallback for unparseable input (fail closed)- Existing
TestReadOnlyBypassPrevention tests still pass
- New tests: keyword-in-string-literal and keyword-as-identifier queries are allowed
Follow-up to #1 (thanks @Mann1ng).
PR #1 closed the writable-CTE / subquery-DML / inline multi-statement bypasses by broadening
_DESTRUCTIVE_PATTERNto match destructive keywords anywhere in the statement. That's the right security default, but it can false-positive on legitimate read-only queries that contain those keywords in string literals or identifiers, e.g.:The proper fix is recommendation #3 from docs/security-analysis.md: parse the statement into an AST (e.g. with sqlglot) and walk it for DML/DDL nodes anywhere in the tree — CTEs, subqueries, and multi-statement input included. That keeps the guard airtight without blocking legit queries.
Acceptance:
_is_read_only()uses AST inspection, regex kept only as a fallback for unparseable input (fail closed)TestReadOnlyBypassPreventiontests still pass