R `check_query()` accepts writes that do not begin with a write keyword

[jat255]

check_query() classifies a statement by its first word, so anything beginning with SELECT or WITH is accepted whatever follows. Six statement forms that write, lock, or create tables pass the guard.

This is a follow-up to #58. That issue reported one instance, stacked statements, and it was fixed by the semicolon check. The classification weakness #58 also described, and the parser-based fix it proposed, were not addressed, and the cases below are what remains.

check_query() at pkg-r/R/data-source.R matches the denylist against the start of the normalized string, then requires the string to start with SELECT or WITH. Neither test looks past the first word.

What is accepted

Verified against check_query() on main (R 4.6.0, DuckDB 1.5.5). All six return without error.

WITH t AS (SELECT 1) DELETE FROM sales
WITH t AS (SELECT 1) INSERT INTO sales VALUES (9)
WITH t AS (SELECT 1) UPDATE sales SET x = 5
WITH d AS (DELETE FROM sales RETURNING *) SELECT * FROM d
SELECT * INTO new_table FROM sales
SELECT * FROM sales FOR UPDATE

The first three are DML after a CTE list: DuckDB accepts it, and the guard sees only WITH. The fourth is a data-modifying CTE, where the statement reads at its root but its CTE deletes. The fifth creates a table. The sixth changes no rows but takes locks that block writers, so it is not read-only in the sense the tool advertises.

End to end through the public path, not just the guard:

pkgload::load_all("pkg-r")
src <- data_source(sales = data.frame(x = 1:3))
source_query(src, "SELECT count(*) AS n FROM sales")$n
#> [1] 3
source_query(src, "WITH t AS (SELECT 1) DELETE FROM sales")
#> executes, with a DBI warning about dbFetch on a DELETE
source_query(src, "SELECT count(*) AS n FROM sales")$n
#> [1] 0

The owned DuckDB connection's lock_down() does not help: it disables extension loading and filesystem access, not local DML.

As in #58, this means read_only_hint = TRUE and the "runs only read-only SELECT queries" wording currently overstate the guarantee.

Two false rejections, from the same cause

The guard also refuses valid read-only queries, because the semicolon test runs against raw text:

SELECT * FROM sales WHERE note = ';'
SELECT 1 -- ; DROP TABLE sales

#58's suggested coverage included "do not reject keywords or semicolons inside literals, quoted identifiers, or comments". That case is still open.

Suggested fix

Parse the statement and classify it, rather than matching its prefix. This is what #58 proposed, and the Python package has since done it in #239 using sqlglot: read-only statement forms are an allowlist so an unanticipated form fails closed, the whole AST is searched rather than the root so a data-modifying CTE or SELECT INTO cannot ride under a SELECT, and locking clauses are rejected.

A text-based fix is not worth attempting. The Python guard was fixed three times as a text scanner before being replaced. Skipping the CTE list with a paren scan was defeated by a ) inside a comment, and then by DuckDB's nested block comments, because each fix needs to know one more lexical rule than the last. There is no sqlglot for R, so the options are DuckDB's own parser through json_serialize_sql() (as #58 set out in detail, though it only covers DuckDB sources) or a read-only connection where the backend supports one.

Worth deciding alongside the fix: the two packages' guards now disagree about what they accept, and the accept/reject verdicts are the kind of contract tests/shared/ exists for. Pinning them there would keep the two from drifting further.

Filed rather than fixed alongside the Python work in #239, because this is a separate package and wants its own review.