Basekick-Labs / Basekick-Labs/arc

Assert every path literal in the rewritten SQL is one Arc emitted

Open
#764 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Go
Stars
677
Forks
53
Avg merge
9h 14m
Merged PRs (30d)
164

Description

Follow-up to #641, carrying its three unmet acceptance criteria.

## Why this rather than sandbox scoping

#641 asked for `allowed_directories` to be narrowed per query so that a foreign-path read fails at the sandbox regardless of whether `ValidateSQLRequest` caught the syntax. The design work (PR #763, `docs/progress/2026-09-12-duckdb-sandbox-scoping.md`) established that DuckDB 1.5.5 cannot do this on a shared handle: `allowed_directories` is GLOBAL-only, immutable after lockdown, and the lockdown is one-way. The only mechanism is a second DuckDB instance.

Routing to per-scope instances was rejected for a structural reason. The routing key can only come from `extractTableReferences`, which skips anything followed by `(`, so a bare `read_parquet('/db2/**')` yields no key. The queries the feature exists to contain are exactly the ones the router cannot see.

## Proposal

Enforce in Go, on the **final rewritten SQL** immediately before dispatch:

> every path literal in the SQL Arc is about to execute must be one Arc itself emitted, and must lie under an allowlist derived from the RBAC decisions just made.

This inverts the broken dependency. It inspects the SQL actually being executed rather than what the reference extractor managed to parse, so the unkeyed `read_parquet` case is caught by construction: a path Arc did not emit is rejected no matter what the extractor saw.

Why this enforcement point:

- Arc already has the paths. The transform builds every one through `getStoragePath` / `quotePath`, and the parallel path carries them explicitly as `ParallelQueryInfo.Paths`.
- It is the mechanism the arcx engine already uses (`internal/arcxengine/arcxengine.go`, `AllowedDirs`), so it can cover **both** execution engines. Narrowing arcx's `AllowedDirs` per request becomes a small change on top.
- One insertion point after `getTransformedSQL*` covers every dispatch site. There are at least 14 seams across 11 functions and four distinct Go-level handles, including `query.NewParallelExecutor(db.DB(), ...)`, which captures a raw `*sql.DB` at handler construction and serves the largest queries. Any seam left unguarded is a complete bypass, so a design needing per-seam retrofits is the wrong shape.
- Zero extra DuckDB instances, so none of the thread, memory-budget, credential-refresher, cold-start, or eviction costs that sank the pool design.
- It is a pure function, unit-testable without DuckDB.

Its weakness should be stated plainly: this is Arc code checking Arc code, not the engine refusing. That is a genuine concession relative to what #641 wanted. It is accepted because the rejected design did not actually deliver engine-level enforcement either, since the routing decision that selects the engine's scope is made by the same defeated parser.

## The fallback if this proves unworkable

A **single** storage-less DuckDB instance, created once at startup with an empty `allowed_directories`, serving only queries whose derived scope is empty. Costed out in the design note. It contains only the unkeyed class (the mixed shape `FROM db1.cpu, '/db2/x.parquet'` still reads through the full-root handle), and it still needs the routing hook at every dispatch site, which is why the assertion above is preferred. Its soundness invariant (no extracted table references implies no emitted storage path) was measured against Arc's real extractor and transform and holds.

## Acceptance

Inherited from #641:

- [ ] A query authorized for `db1.cpu` cannot read a `db2` file even if a validator bypass is assumed (test with `ValidateSQLRequest` stubbed to return nil)
- [ ] Local, S3 and Azure backends all covered
- [ ] No regression on legitimate cross-measurement / JOIN queries the rewriter emits

Plus:

- [ ] Covers the arcx engine, not only DuckDB
- [ ] Applied at a single point that all dispatch sites pass through, with a test that fails if a new dispatch site bypasses it
- [ ] No measurable cost on the query hot path

The stubbed-validator test should cover at least these shapes, across `/api/v1/query`, `/query/msgpack`, `/query/arrow` and `/query/estimate`, and with enough partitions to route through the parallel executor:

```
SELECT * FROM read_parquet('/db2/**/*.parquet')
SELECT * FROM parquet_scan('/db2/**/*.parquet')
SELECT * FROM "parquet_scan"('/db2/**/*.parquet')
SELECT * FROM glob('/**')
SELECT * FROM read_text('/etc/passwd')
WITH t AS (SELECT * FROM read_parquet('/db2/**')) SELECT * FROM t
SELECT * FROM db1.cpu, '/db2/x.parquet' b
SELECT * FROM db1.cpu WHERE x IN (SELECT y FROM read_parquet('/db2/**'))
```

The first six derive no scope today; the last two derive `[db1]`. All eight must be denied.

Contributor guide

Open the contributing guide

Research direction

Start at the getTransformedSQL* functions and trace the dispatch sites, including query.NewParallelExecutor(db.DB(), ...). Read ParallelQueryInfo.Paths and internal/arcxengine/arcxengine.go's AllowedDirs to understand the existing path flow. Done means one final-SQL enforcement point covers DuckDB and arcx, rejects every listed unauthorized query shape across the named endpoints and backends, and preserves legitimate cross-measurement and JOIN queries.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend-api-design, databases, security
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.