Repository navigation
fix(tiering): escape paths in router read_parquet builders (#307) - #501
Conversation
…ense-in-depth) The internal/tiering router's read_parquet()-building helpers (buildReadParquet, BuildReadParquetExpr, BuildMultiTierQuery) interpolated file paths into DuckDB SQL without escaping single quotes. These helpers are not on the live query path — the query engine builds tiered read_parquet() calls via its own already-escaped quotePath helper — so this is hardening of currently-unwired builders rather than a live-exposure fix. Routing paths through sqlutil.EscapeStringLiteral (the same chokepoint the API layer uses) means any future caller that wires these up inherits the escaping. Also documents that BuildMultiTierQuery's whereClause is interpolated raw — a separate injection sink a future caller must validate — and adds router_test.go pinning the escape contract (verified to fail without the fix). The real-world taint path (unvalidated MQTT topic-mapping names) is tracked separately in #300.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request hardens the internal/tiering router's read_parquet() query builders by escaping single quotes in interpolated file paths to prevent potential SQL injection. It also adds comprehensive unit tests to verify this escaping behavior and updates the release notes. The review feedback highlights a critical bug in the unwired buildFullPath helper method, which ignores its backendType parameter and always uses the cold backend configuration, potentially leading to incorrect path generation for the hot tier in the future.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Code Review
This pull request hardens the tiering router's query-building helpers by escaping single quotes in interpolated file paths to prevent SQL injection, accompanied by new unit tests and release notes. Feedback highlights a critical bug in the buildFullPath helper function, which ignores the backendType parameter and incorrectly applies cold backend configurations to hot tier queries.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…mini round 1)
buildFullPath ignored its backendType parameter and always switched on
coldBackend, so a hot-tier ("local") path would be wrongly prefixed
with the cold s3://|azure:// URI when a cold backend is configured.
Pre-existing; only reachable from the unwired BuildMultiTierQuery, so
no production query is affected (the live tiered path in query.go's
buildMultiTierReadParquet builds hot and cold paths separately and
correctly). Guarded with an explicit backendType=="local" early return
so the builders are correct for any future caller. Test pins the guard.
|
@gemini-code-assist Round-1 finding addressed in the latest commit: For the record on exploitability: this is latent, not a production bug. The only caller of |
There was a problem hiding this comment.
Code Review
This pull request hardens the internal/tiering router by escaping single quotes in interpolated DuckDB read_parquet() paths to prevent SQL injection (defense-in-depth, #307) and fixes a bug where hot-tier paths were incorrectly prefixed with cold backend URIs. It also adds comprehensive unit tests to verify these changes. The reviewer recommends adding defensive checks for r.manager == nil in BuildMultiTierQuery and buildFullPath to prevent potential nil pointer dereference panics when the router is initialized without a manager.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…ound 2) BuildMultiTierQuery and buildFullPath are exported but unwired; a future caller (or a test) constructing a Router without a manager would panic on r.manager deref. Guard both with an early return. No production effect — NewRouter always supplies a manager.
|
@gemini-code-assist Round-2 findings addressed: |
There was a problem hiding this comment.
Code Review
This pull request hardens the tiering router's read_parquet() query builders by escaping single quotes in interpolated file paths to prevent potential SQL injection as a defense-in-depth measure. It also ensures that hot-tier local paths are returned unchanged without being incorrectly prefixed by a cold backend's URI, and adds corresponding unit tests to verify these behaviors. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
…502) BuildReadParquetExpr, BuildMultiTierQuery, buildReadParquet, and buildFullPath have had zero callers since they were introduced in the original 2-tier commit (2575746) — the query engine builds tiered read_parquet() calls inline in internal/api/query.go (buildMultiTier- ReadParquet → quotePath), never through the router. Confirmed dead by exhaustive cross-repo reference search: the four functions referenced only each other and their tests. PR #501 had just escaped and nil-guarded them as defense-in-depth, but since they are confirmed unreachable (not pending/warm-tier scaffolding — warm was removed before the 2-tier code landed), deleting is the right call: less surface to maintain, and it removes the raw-whereClause injection sink BuildMultiTierQuery carried. The live router methods (GetStoragePathsForQuery, GetGlobPathsForQuery, getBackendName) are untouched. Drops the now-unused sqlutil, storage, and strings imports. Co-authored-by: Ignacio Van Droogenbroeck <ignacio@vandroogenbroeck.net>
1 participant
Closes #307.
Summary
The
internal/tieringrouter'sread_parquet()-building helpers interpolated storage file paths into DuckDB SQL with'%s'and no single-quote escaping. They now route every path throughsqlutil.EscapeStringLiteral(internal/sql/mask.go) — the same chokepoint the live query path already uses viaquotePath.Important framing (found during review)
These helpers —
buildReadParquet,BuildReadParquetExpr,BuildMultiTierQuery— are not on the live query path. The only router method production calls isGetGlobPathsForQuery(3 sites ininternal/api/query.go), which returns raw glob strings; the query engine then builds tieredread_parquet()calls through its ownbuildMultiTierReadParquet→quotePath→EscapeStringLiteral, which was already escaped. The patched functions have zero production callers.So this is defense-in-depth / hardening of currently-unwired builders, not a live-exposure fix. They appear to be built ahead of a tiered-query-routing path that isn't wired yet — escaping them now means a future caller inherits the protection. The release-notes entry and code comments are framed accordingly (no overstated "live SQL injection" claim).
Two reviewers (general correctness + security checklist) independently confirmed: single-quote doubling is the complete and sufficient escape for DuckDB's plain
'...'literal inread_parquet()argument position (backslashes are literal;E'...'/dollar-quoting require a sigil the code never emits); no double-escaping (live callers escape downstream independently); no import cycle.Also addressed
BuildMultiTierQuery'swhereClauseis interpolated raw — documented as a separate injection sink a future caller must validate (or rework to structured predicates). Not escaped here because it's a SQL fragment, not a path literal.isValidMeasurementName), so high(tiering): SQL injection in buildReadParquet via path interpolation #307 is not reachable from the HTTP path regardless.Test plan
go build ./cmd/... ./internal/...,go vet ./internal/tiering/,gofmt -lcleango test -race ./internal/tiering/ -count=1passesrouter_test.gopins the escape contract (single/multi-path, quote-bearing, injection-shaped); verified to FAIL without the escape🤖 Generated with Claude Code