fix(sql): bind paths as parameters, validate session ids at the boundary, prove constant-only SQL text - #8
Conversation
…ary, prove constant-only SQL text
Bandit reported 29 B608 string-built-query sites that ruff's `# noqa: S608` never
silenced. Most interpolated constants; the ones that mattered spliced corpus paths
into statement text through `sql_literal`, and the one outside influence on those
paths was the session id derived from a transcript filename.
atif-corpus: `domain.session_id` is the boundary a derived id must pass before it
becomes a corpus path (`^[A-Za-z0-9][A-Za-z0-9._-]*$`, at most 255 chars). The
scanner skips a failing name with a logged reason, `SourceScan` and
`MaterializationReport` carry `rejected_session_ids`, the CLI report adds
`rejected` / `rejected_session_ids` (additive), and a corpus dir an older version
wrote under such a name is kept rather than ghosted.
atif-duck: the twin `domain.session_id` is applied in `register_raw` before any
per-session path is built; a rejected dir registers nothing, is logged once, and
is reported in `RawSources.rejected_session_ids`; `columnar_coverage` skips it the
same way. The read_json readers take their glob or file list as ONE bound
parameter. The parquet readers are `con.read_parquet(files)` relations registered
as views (CREATE VIEW cannot be prepared); the `sql(..., params=)` shape measured
3x the memory and is documented as rejected. `SqlFragment` (NewType over str)
marks the text `sql_literal`, the catalog and the projections produce, and every
SQL-building helper takes and returns it. ATTACH and the producer's one-row
session projection are the two statements DuckDB will not prepare and keep
`sql_literal`; a test each fails when the wrapping is removed.
atif-embed: `corpus_text_rows` binds its trajectory batch as a list parameter.
Tests: adversarial corpus under a root named `o'brien ?; --$1` whose content
carries SQL text registers as data on both paths and row-for-row equal; bad dir
names register nothing; an AST audit (`tests/sql_text_audit.py`) over
registry.py, columnar.py, analytics.py and corpus_text_rows.py fails on any
placeholder that is not a constant, a projection call or `sql_literal(...)`, and
was proven against a planted `{user_input}`. Twin pins on both sides.
Every remaining interpolation carries `# noqa: S608 # nosec B608 - <reason>`;
`mise run security:bandit` reports 0 B608 (was 29). Equivalence on the 300
session panel corpus: byte-identical output for the three panel statements,
wall time and peak RSS at parity with origin/main.
pyproject: `exclude-newer = "7 days"` closes Semgrep uv-missing-dependency-cooldown;
the 21 packages the window would have moved today are exempted up to the instant
it landed so uv.lock keeps every pin (options-only lock diff).
MicroVM defensive review of atif-sql main 31b61d5Reviewed 2026-09-12 inside AWS Lambda MicroVMs (Firecracker, aarch64) with the Environment
Image route: own Dockerfile derived from Two exec-environment facts cost a run each: the daemon spawns execs with no Sample corpus
Static counts, local vs VMSame commands both sides (
Dynamic pass
The shipped CLI could not run a single query in either guestEvery Probe table (one-thread control)To exercise the sandbox at all, the probes were re-run through a 12-line shim that calls the shipped
Nothing in b through k succeeded except the three rows marked in bold, and each of those is a finding below. The panel statements answer identically to the host. Filesystem diffBaseline: sha256 of every file under
The lance extension files are Adversarial data assertionsAll nine adversarial session ids materialized and appear as rows of Reviewer agent pass
The rest of its list (registration OOM before the cap, spill-dir writes, settings enumeration, connection-local catalog shadowing, multi-statement SQL accepted) agrees with the probe table above. It confirmed the SET ordering inside Cost
Total priced: about $0.64, plus three unpriced server-side image builds (the two review images and the agent image, roughly 5 to 6 minutes each) and the Bedrock token spend of one Opus 5 session of 22 minutes, which Findings, ranked by reachReach classes: (1) attacker controls transcript filenames; (2) attacker controls transcript contents or size; (3) the person typing SQL, against what the sandbox claims to prevent; (0) no attacker needed. 1.
|
…, no extension installs at query time (#10) * fix(query): size registration to the host, private spill dir, no root, no extension installs at query time Closes the five open findings of the MicroVM defensive review of main 4571e14 and pins findings 6 and 7 (closed by #8) with their exact reproductions. 1. Registration OOM under ~1.9 GiB RAM per core. The eager read_json readers reserve about 2x maximum_object_size per thread, so the 1 GiB constant cost 2 GiB a thread and `SET threads=4; SET memory_limit='6GB'` failed to register any corpus with two or more edges.jsonl files. The bound is now sized from the largest file the reader opens (plus a quarter and 1 MiB, floored at DuckDB's 16 MiB default, capped at 1 GiB), and query applies memory_limit, threads and temp_directory BEFORE register(). The cap derives from available RAM (MemAvailable, 80% ceilings, 8 GiB target, 512 MiB floor) and the thread count from the cap (one per 2 GiB, capped at sched_getaffinity). ATIF_SQL_QUERY_MEMORY_LIMIT and ATIF_SQL_QUERY_THREADS override both; malformed values exit 64. The old _query_memory_limit_bytes is the same function, re-derived. Panel on the 300-session corpus: byte-identical output unlimited and under 6GB/4 threads; unlimited wall 0.88-0.98 s -> 0.73-0.79 s. 2. Root could overwrite the corpus through a granted parquet with COPY ... (USE_TMP_FILE false). DuckDB 1.5.5 has no read-only grant (measured: no write switch in duckdb_settings(), and a read_parquet relation bound before enable_external_access=false is refused without a grant), so two layers: query, search and analyze refuse uid 0 with exit 77 root_refused unless ATIF_SQL_ALLOW_ROOT=1 (logged warning), and query refuses every file-facing statement kind before executing anything (COPY, COPY_DATABASE, EXPORT, ATTACH, DETACH, INSTALL, LOAD, PREPARE, EXECUTE) via DuckDB's own extract_statements on the hardened connection, exit 70 sandbox_refused, for any uid. Probe d2 now fails as any uid. 3. The spill directory was <corpus_root>/.duckdb_tmp and writable by caller SQL, persisting files inside the corpus. It is now a per-process tempfile.mkdtemp (0700) under the system temp dir, the sole allowed_directories entry, removed on every exit path after the connection closes. Nothing under the corpus is writable. 4. register_vss ran INSTALL lance (a 242 MB download) during every registration. It now LOADs the extension only when duckdb_extensions() says it is installed, skips the load entirely when no store directory exists, and binds message_embeddings empty with a warning otherwise. query sets autoinstall_known_extensions=false and autoload_known_extensions=false before registration. Installing is an explicit act: `atif-sql embed --install-extension`, or a real embed run. `atif-sql status` reports vector_search ready | no_store | extension_missing; `search` exits 78 extension_missing rather than pretending the store is empty. 5. duckdb_settings() / current_setting() expose the grants and so every session id. DuckDB cannot hide a setting and the grants must be per file, so this is documented as accepted: the caller is the local user and query is not a privilege boundary. Findings 6 and 7: a transcript named `..jsonl` (stem `.`) and one named `*.jsonl`, materialized over three victims, are rejected and the tree is untouched; a session dir named `*` or `???` counts every session once on both read paths and grants no glob. Tests that fail on main 4571e14 and pass here (verified on a scratch checkout): registration under 4 threads/6GB and 16 threads/8GB (CLI and registry), COPY into the spill dir persists nothing, USE_TMP_FILE false over a granted parquet is refused, root is refused, and a corpus with a store and no extension installs nothing. * test: register the lance install fixture under a public name conftest.py re-exports the fixture modules with `import *`, which skips underscore names, so `_lance_extension_present` never registered and a runner with no cached extension (CI) ran the store-binding tests without lance. Reproduced locally with an empty HOME: 6 failures, then 407 passed once the fixture had a public name and installed the extension itself.
1 participant
What changed
Bandit reported 29
B608string-built-query sites; the code carried# noqa: S608, which ruff honors and Bandit does not. Most sites interpolated constants. The ones that mattered spliced corpus paths into statement text throughsql_literal, and the one outside influence on those paths was the session id derived from a transcript filename.Boundary (atif-corpus, atif-duck).
domain.session_idin each package pins the same rule,^[A-Za-z0-9][A-Za-z0-9._-]*$and at most 255 characters (twinned the wayAgentSourceis, each side's test reads the other's source). Every id Claude Code (UUID v4) and Codex (UUID v7) produce passes. The scanner skips a failing name with a logged reason;SourceScanandMaterializationReportgainrejected_session_ids; the CLI report addsrejectedandrejected_session_idsbeside the existing fields. A corpus dir an older version wrote under such a name is kept, never ghosted.register_rawapplies the twin before building any per-session path: a rejected dir registers nothing, is logged once, and is reported inRawSources.rejected_session_ids;columnar_coverageskips it the same way.Bound paths (atif-duck, atif-embed). The
read_jsonreaders (meta, edges, loss, trajectory list) take their glob or file list as one bound parameter. The parquet readers arecon.read_parquet(files)relations registered as views, sinceCREATE VIEW ... read_parquet(?)is refused ("this type of statement can't be prepared"); thecon.sql("... read_parquet(?)", params=...)shape was tried and measured about 3x the memory, so it's documented as rejected.corpus_text_rowsbinds its trajectory batch as a list parameter.ATTACH(Lance) and the producer's one-row session projection are the two statements DuckDB won't prepare and keepsql_literal.Types and the audit.
SqlFragment(aNewTypeoverstr) marks the textsql_literal, the catalog and the projections produce; every SQL-building helper takes and returns it.packages/atif-duck/tests/sql_text_audit.pywalksregistry.py,columnar.py,analytics.pyand atif-embed'scorpus_text_rows.pyand fails on any f-string placeholder that isn't a module constant, a catalog constant, a projection call orsql_literal(...). Proven against a plantedf"SELECT * FROM {user_input}"inregistry.py(caught, then removed).Adversarial fixtures. A corpus under a root named
o'brien ?; --$1whose session content carries'); DROP TABLE x; --,$1 and ? and $2, backslashes, newlines and 20-deep nesting registers as data on both the JSON and columnar paths, row for row equal; six bad directory names register nothing and are reported once each. The producer with a quoted session id and the Lance store under a quoted path each fail when thesql_literalwrapping is removed (proven, then restored).Markers. Every remaining interpolation carries
# noqa: S608 # nosec B608 - <what it interpolates>. No blanket skip in[tool.bandit].mise run security:bandit: B608 29 -> 0.Cooldown.
exclude-newer = "7 days"closes Semgrepuv-missing-dependency-cooldown(the rule accepts only a relative window of at least seven days; an absolute instant is flagged too). Re-resolving under the window today would have moved 21 locked packages (ruff, ty, litellm, numpy, boto3, ...) down to week-old releases, and ty 0.0.78 rejects a# ty: ignorerule name main already uses, so those 21 are exempted under[tool.uv.exclude-newer-package]up to the instant the window landed. Theuv.lockdiff is options-only; every pin stays.mise run security:semgrep: 1 -> 0.Docs.
docs/CONTRACT.md,packages/atif-duck/README.md,AGENTS.md,docs/reference/cli.md.Equivalence and timing
Corpus: 300 Claude Code sessions materialized from
atif-sql-frontier/corpus/raw/claude(all columnar). Reference: an untouched worktree oforigin/main(31b61d5). Three runs each,/usr/bin/time -f '%e %M'(seconds, peak KB),--format jsonon a pipe. Output byte-identical for all three statements (sha256 equal across every run of both binaries).Two shapes were measured and rejected on the way:
con.sql("... read_parquet(?)", params=...)relation views (about 3.0 s / 1.85 GB) and explicit file lists in place of globs for the meta/edges/loss readers (about 1.22 s; DuckDB reads 300 small files roughly twice as fast through a glob).Gates
mise run checkgreen (1399 passed, 1 skipped; was 1329 passed, 1 skipped).mise run docs:gategreen (64 tests).uv run cz checkpasses. atif-duck's equivalence tests (test_columnar.py::TestEquivalence) pass unchanged.Code-scanning alerts this addresses
Bandit B608: #6, #7, #8, #9, #10, #11, #12, #13, #15, #16, #17, #18, #19, #20, #23, #24, #25, #26, #27, #28, #29, #30, #31, #32, #33, #34, #35, #36, #37. Semgrep
uv-missing-dependency-cooldown: #21. (Closesdoesn't apply to code-scanning alerts; they close on the next scan ofmain.)Not done
test_parity_live.py::TestLiveParity::test_claude_codefails on this machine when run outsidemise run check, on this branch and on the untouched reference worktree alike: the diverging session is the live transcript of the Claude Code session that produced this PR, still being written while harbor and our converter read it at different instants. It passed inside thecheckrun recorded above.