Python data sources over data frames, with a read-only SQL guard - #239
Merged
Conversation
sqlglot is a runtime dependency: the read-only SQL guard parses the statements it screens. pandas and polars are dev-only, since a caller passing frames already has one installed.
lock_down() is a security control, not a nicety: a source built from frames must not reach the host filesystem. Every test is an attempt rather than an assertion about configuration, because configuration that looks right but does not stop the attempt is worth nothing. connect() puts DuckDB's extension and home directories under temp. Deployed environments such as Connect have a read-only install location, and home_directory is process-global, so it cannot be passed in config once any instance exists.
The guard rejects anything that is not a single read-only statement. It parses rather than matching text, because a text check is not sound here: DuckDB accepts DML after a CTE list, so a first-word test lets 'WITH t AS (SELECT 1) DELETE FROM sales' through, and screening for the paren that ends the CTE only moves the question to what a comment or a quote hides. Nested block comments and dollar quoting are both lexical facts a hand-written scanner has to know; sqlglot already knows them. Read-only statement forms are an allowlist, so a form nobody thought of fails closed instead of falling through a denylist. The whole tree is searched, not just the root, because PostgreSQL runs data-modifying CTEs and SELECT INTO creates a table, and both parse with a Select root. sqlglot's hierarchy is not uniform (Drop and Alter descend straight from Expression), so the write set names those alongside the DML and DDL bases, and a test pins that no read-only type overlaps it. A locking read such as FOR UPDATE changes no rows but blocks writers, so it is refused too. The dialect comes from the source, so DuckDB syntax such as EXCLUDE, QUALIFY and FROM-first parses instead of being refused.
data_source() dispatches to explicit constructors. from_frames() writes pandas or polars frames into the in-process DuckDB and locks it down, which is what makes a frames source unable to read the host filesystem. Engine and pins-board sources land with their own constructors. DataSource is a dataclass rather than a pydantic model: its checks run against live external state, which pydantic cannot express, so they live in the constructors where a bad argument can still be named. A small backend protocol sits underneath, so the frames path can hold a raw DuckDB connection while a engine-backed source can hold an engine. Table names come from caller-supplied keyword arguments and reach SQL before the lockdown runs, so they are escaped, and the staging relation gets a generated name rather than one derived from the caller's.
With none, pyrefly consults the repository's git ignore files. A worktree checked out under the ignored .worktrees/ directory therefore has every Python file skipped, so the command type-checks nothing and still exits 0.
Tokenizing fails before parsing does and raises TokenError rather than ParseError, so an unterminated string leaked a sqlglot exception to a caller expecting ValueError. Catch their shared base instead.
jat255
marked this pull request as draft
September 2, 2026 00:00
This was referenced Sep 2, 2026
jat255
marked this pull request as ready for review
September 2, 2026 00:16
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
1 participant
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.
First PR of M2, the Python data layer. It lands
data_source()for data frames, the in-process DuckDB it writes them into, and the read-only SQL guard that screens every query. Engine and pins-board sources follow in the next PR on this stack; kataesadcloses with this one,ec9dstays open for them.The SQL guard is a parser, not a text check
This is the part worth review attention, and it is a departure from how
pkg-rdoes it.The guard started as a denylist plus a first-word test, matching
pkg-r/R/data-source.R. That is not sound. DuckDB accepts DML after a CTE list, soWITH t AS (SELECT 1) DELETE FROM salespasses a first-word test and empties the table. Screening for the paren that closes the CTE only moves the question to what a comment or a quote hides, and DuckDB nests block comments and supports dollar quoting. Three separate bypasses came out of review that way, each reproduced against a live frame-backed source. Implementing our own guard is an exercise in whack-a-mole, when there are better options available in Python.So the guard parses with
sqlglotinto an AST and checks what the statement is. Read-only forms are an allowlist, so a form nobody anticipated fails closed instead of falling through a denylist, which is also what refusesCOPY,ATTACH,INSTALLandLOAD. It searches the whole tree rather than just the root, since you could have write operations deeper inside a CTE. Locking reads such asFOR UPDATEare refused too: they change no rows but block writers.sqlglotis a new runtime dependency. It has no dependencies of its own, and it parses every DuckDB construct tested here, includingEXCLUDE,QUALIFY,ASOF JOINand FROM-first syntax, so failing closed on a parse error does not cost real queries.pkg-r'scheck_query()classifies statements the same way, so it accepts DML after a CTE list, data-modifying CTEs,SELECT INTOand locking reads, and wrongly refuses a semicolon inside a string literal. Verified against the R function directly and filed as #243, rather than fixed here, because it's the R implementation's issue.Rejected alternative
DuckDB's own connection-level read-only mode refuses every bypass, because the engine is a real parser, but it had too many limitations to use:
Also here
Frame names reach SQL before the lockdown runs, so they are escaped and the staging relation gets a generated name. Previously a name containing a quote created a table under a truncated name, and
my salesororder-linesfailed outright.The pyrefly CI step now takes explicit paths. Without them it consults the repo's git ignore files, so a worktree under the ignored
.worktrees/directory type-checked nothing and still exited 0.Verification
202 tests, ruff and pyrefly clean. The security tests are written as attempts rather than as assertions about configuration: each bypass is replayed against a live source and the table checked afterwards.