posit-dev / posit-dev/commons

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

Open
#243 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug r
Dominant language
Python
Stars
44
Forks
1
Avg merge
1d 7h
Merged PRs (30d)
142

Description

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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in pkg-r/R/data-source.R at check_query(), then compare the Python guard from #239 and the shared contract in tests/shared/. Exercise check_query() and source_query() with the listed cases; done means write, lock, and table-creating forms are rejected, semicolons in literals and comments remain accepted, and the R and Python verdicts stay aligned.

Written by the indexing model from the issue text.

Assessment

Tech stack
r, sql
Domain
databases, security, testing-qa
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.