fix(security): sandbox DuckDB user queries via enable_external_access=false + allowed_directories#442
Conversation
…=false + allowed_directories
Closes a critical file-read vulnerability: any token with permissions: [] could
read arbitrary local files via DuckDB's I/O function family (read_csv_auto,
read_json, read_text, read_blob, glob, parquet_metadata, parquet_schema, etc.).
The user-SQL denylist blocked only read_parquet/arc_partition_agg and RBAC
inspected FROM/JOIN clauses, so scalar table functions in the SELECT list
slipped past both layers. Combined with httpfs loading, the same primitive
reached SSRF (instance-metadata IPs), auth.db exfiltration, and cross-tenant
Parquet reads.
Replaces the maintenance-treadmill denylist with a structural fix at the
DuckDB layer:
- After every INSTALL/LOAD completes, SET GLOBAL allowed_directories to
Arc's storage roots (local + hot S3/Azure + cold-tier S3/Azure),
plus DuckDB spill, plus a dedicated import-upload subdir, plus the
compaction temp dir.
- SET GLOBAL enable_external_access = false (one-way at runtime).
- Verified by reading back the flag via QueryRowContext with a 5s
context-bounded budget.
Operator-facing changes:
- storage.local_path, database.temp_directory, compaction.temp_directory,
and arcx.storage_root are now resolved to absolute, forward-slash paths
before being passed to DuckDB. Defaults continue to work; non-absolute
operator values are normalised against the process working directory at
startup. Paths containing control bytes, Unicode formatting characters,
or line/paragraph separators (Cc, Cf, Zl, Zp) are rejected with a Fatal.
- System roots (/, /etc, /usr, /bin, /sbin, /boot, /proc, /sys, /dev,
/tmp, /var, /home, /root, /Users) are refused as storage.local_path
so a typo cannot neuter the sandbox.
- Multipart import uploads moved from os.TempDir() to a dedicated
arc-uploads subdirectory under database.temp_directory. The directory
is created 0o700, rejected if pre-staged as a symlink, and its
ancestor chain is resolved with EvalSymlinks (Warn + use resolved
path on mismatch, so Docker bind-mounts and /var → /private/var on
macOS continue to work).
- DELETE on S3-backed deployments and the profile-mode query temp file
now share the same allowlisted directory; both fail-closed at request
time when misconfigured.
- The arcx loader is now a one-shot LOAD + SET GLOBAL during
configureDatabase; the previous per-connection connInitFn relied on
the incorrect premise that DuckDB extension state is per-connection.
Tests added:
- TestSandbox (CVE reproduction + full I/O family + SSRF + COPY TO local
+ COPY TO s3:// outside allowlist + EXPORT DATABASE + INSTALL after
lockdown + cross-connection enforcement + range() remains callable +
lockdown is one-way).
- TestBuildAllowedDirectories (table-driven: hot/cold S3 full-URI dedup,
same-bucket-different-prefix, leading/interior-slash collapse,
parent-traversal '..' fallback, trailing-slash idempotence,
empty-config behavior).
- TestSandboxEmptyAllowlistLogsButDoesNotPanic.
The existing arcx tests confirm arcx_version() and SET GLOBAL
arcx.storage_root propagate database-wide across distinct concurrent
pool connections.
Reported by Alex Manson (@NeuroWinter, https://neurowinter.com/).
![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 implements a robust security sandbox for DuckDB to prevent arbitrary file-read vulnerabilities. It replaces the previous denylist with a structural fix using DuckDB's allowed_directories and enable_external_access settings, ensuring all file access is restricted to a set of pre-defined, absolute, and normalized paths. The changes include a dedicated upload directory for imports and staging, simplified extension loading, and enhanced path validation against control characters and symlink bypasses. Review feedback focused on maintaining path consistency across the codebase, specifically recommending the use of normalized forward-slash paths for handler initialization and profiling outputs to ensure compatibility on Windows and alignment with the sandbox allowlist.
…ths for Windows portability Addresses Gemini Code Assist review on PR #442: - cmd/arc/main.go: pass `dbConfig.UploadDir` (already ToSlash-normalized on line 397) to NewImportHandler/NewDeleteHandler instead of the local `uploadDir` variable, which on Windows would contain backslashes from filepath.Join + filepath.EvalSymlinks. The sandbox allowlist uses the forward-slash form; the handlers must match or the OS layer opens a path the sandbox didn't whitelist. - internal/database/duckdb.go + duckdb_arrow.go: wrap profilePath with filepath.ToSlash before PRAGMA profiling_output interpolation. The path from os.CreateTemp uses native separators on Windows; without ToSlash, the SQL value mismatches what's in allowed_directories. No correctness change on Linux/macOS (filepath.ToSlash is a no-op there). Closes the four Gemini-flagged Windows-portability gaps with no other behavior change.
|
@gemini-code-assist review one more time please |
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive security sandbox for DuckDB to prevent unauthorized file access. It introduces a path allowlist using DuckDB's allowed_directories and disables external access after initialization. Key changes include robust path normalization, validation against control characters, and ensuring all temporary file operations (imports, deletes, profiling) occur within allowlisted directories. Additionally, the extension loading mechanism was simplified to a database-wide approach. Review feedback suggests extending system root protection and symlink resolution to all configured paths, and normalizing the extension path for consistency.
| switch dbConfig.LocalStorageRoot { | ||
| case "/", "/etc", "/etc/", "/usr", "/usr/", "/bin", "/bin/", "/sbin", "/sbin/", | ||
| "/boot", "/boot/", "/proc", "/proc/", "/sys", "/sys/", "/dev", "/dev/", | ||
| "/tmp", "/tmp/", "/var", "/var/", "/var/tmp", "/var/tmp/", | ||
| "/home", "/home/", "/root", "/root/", "/Users", "/Users/": | ||
| log.Fatal().Str("path", dbConfig.LocalStorageRoot).Msg("storage.local_path refuses to start with a system root, shared-tmp, or user-home root; pick a dedicated data directory") | ||
| } |
There was a problem hiding this comment.
The system root protection check is only applied to LocalStorageRoot. It should be extended to TempDirectory and CompactionTempDirectory as well, since allowing these to be set to a system root (like / or /etc) would effectively grant user SQL access to those sensitive directories via the sandbox allowlist.
| switch dbConfig.LocalStorageRoot { | |
| case "/", "/etc", "/etc/", "/usr", "/usr/", "/bin", "/bin/", "/sbin", "/sbin/", | |
| "/boot", "/boot/", "/proc", "/proc/", "/sys", "/sys/", "/dev", "/dev/", | |
| "/tmp", "/tmp/", "/var", "/var/", "/var/tmp", "/var/tmp/", | |
| "/home", "/home/", "/root", "/root/", "/Users", "/Users/": | |
| log.Fatal().Str("path", dbConfig.LocalStorageRoot).Msg("storage.local_path refuses to start with a system root, shared-tmp, or user-home root; pick a dedicated data directory") | |
| } | |
| for _, p := range []string{dbConfig.LocalStorageRoot, dbConfig.TempDirectory, dbConfig.CompactionTempDirectory} { | |
| switch p { | |
| case "/", "/etc", "/etc/", "/usr", "/usr/", "/bin", "/bin/", "/sbin", "/sbin/", | |
| "/boot", "/boot/", "/proc", "/proc/", "/sys", "/sys/", "/dev", "/dev/", | |
| "/tmp", "/tmp/", "/var", "/var/", "/var/tmp", "/var/tmp/", | |
| "/home", "/home/", "/root", "/root/", "/Users", "/Users/": | |
| log.Fatal().Str("path", p).Msg("Configured path refuses to start with a system root, shared-tmp, or user-home root; pick a dedicated data directory") | |
| } | |
| } |
| if resolved, err := filepath.EvalSymlinks(uploadDir); err != nil { | ||
| log.Fatal().Err(err).Str("path", uploadDir).Msg("Failed to resolve import upload directory symlinks") | ||
| } else if resolved != uploadDir { | ||
| log.Warn(). | ||
| Str("original", uploadDir). | ||
| Str("resolved", resolved). | ||
| Msg("Import upload directory resolves through a symlink; using the resolved path as the sandbox allowlist entry") | ||
| uploadDir = resolved | ||
| } |
There was a problem hiding this comment.
The symlink resolution logic is only applied to uploadDir. However, if LocalStorageRoot, TempDirectory, or CompactionTempDirectory are located on symlinked paths (common in Docker or macOS environments where /var is a symlink to /private/var), DuckDB may see the resolved path while the sandbox allowlist contains the unresolved one, leading to unexpected permission errors. Consider applying EvalSymlinks to all local directory configurations used in the sandbox allowlist.
| if dbConfig.ArcxStorageRoot != "" { | ||
| dbConfig.ArcxStorageRoot = resolveAbsPath("arcx.storage_root", dbConfig.ArcxStorageRoot) | ||
| } |
There was a problem hiding this comment.
The ArcxExtensionPath is not normalized or validated for control characters. For consistency with other path configurations and to ensure the LOAD statement is robust against CWD changes or SQL corruption, it should be processed via resolveAbsPath before being used.
| if dbConfig.ArcxStorageRoot != "" { | |
| dbConfig.ArcxStorageRoot = resolveAbsPath("arcx.storage_root", dbConfig.ArcxStorageRoot) | |
| } | |
| if dbConfig.ArcxStorageRoot != "" { | |
| dbConfig.ArcxStorageRoot = resolveAbsPath("arcx.storage_root", dbConfig.ArcxStorageRoot) | |
| } | |
| if dbConfig.ArcxExtensionPath != "" { | |
| dbConfig.ArcxExtensionPath = resolveAbsPath("database.arcx_extension_path", dbConfig.ArcxExtensionPath) | |
| } |
… ArcxExtensionPath Addresses Gemini Code Assist 2nd review on PR #442: - TempDirectory and CompactionTempDirectory are also added to the DuckDB sandbox allowlist; the system-root deny-list (\"/\", \"/etc\", \"/tmp\", etc.) was previously only applied to LocalStorageRoot. Extended to all three so a typo like `database.temp_directory = \"/etc\"` cannot silently allowlist /etc. - EvalSymlinks Warn-and-substitute was only applied to uploadDir. The same mismatch can happen for the other three local-dir configs: filepath.Abs does not resolve symlinks, so on macOS (/var → /private/var), Docker bind-mounts, or K8s subPath the sandbox literal-string can mismatch the kernel's resolved path. Now applied uniformly to LocalStorageRoot, TempDirectory, and CompactionTempDirectory. - ArcxExtensionPath is interpolated into a `LOAD '<path>'` statement but was bypassing resolveAbsPath. Routed through it now (control-char rejection + Abs + ToSlash) for symmetry with every other operator path. Side effect: the three local directories must exist before EvalSymlinks is called, so main.go creates them with 0o700 first. MkdirAll is idempotent — configureDatabase's own MkdirAll(TempDirectory) becomes a no-op on the second call.
|
@gemini-code-assist review please |
There was a problem hiding this comment.
Code Review
This pull request implements a security sandbox for DuckDB to prevent arbitrary file access by authenticated callers. It introduces a structural fix by defining an allowlist of directories and disabling external access at the database layer. Key changes include path normalization and validation in the main entry point, securing the import upload directory, and refactoring extension loading to be database-wide. Feedback focuses on ensuring cross-platform path consistency by normalizing Windows backslashes to forward slashes for temporary files and directories to match the sandbox allowlist.
| // Create temp file locally for the rewritten data. The destination | ||
| // directory MUST be inside DuckDB's allowed_directories — main.go | ||
| // passes the same sandbox-allowlisted path as the import-upload dir. | ||
| tempFile, err := os.CreateTemp(h.tempDir, "arc-delete-*.parquet") |
There was a problem hiding this comment.
On Windows, os.CreateTemp returns a path with backslashes. Since the DuckDB sandbox allowlist (configured in main.go and sandbox.go) uses forward slashes for all entries, any local paths passed to DuckDB must be normalized. Ensure that the tempPath derived from this tempFile is converted using filepath.ToSlash before being used in the COPY statement at line 834, similar to how profiling paths are handled in internal/database/duckdb.go.
| }) | ||
| } | ||
| // Save to temp file inside the sandbox-allowlisted upload directory. | ||
| tempDir, err := os.MkdirTemp(h.uploadDir, "arc-import-*") |
There was a problem hiding this comment.
On Windows, os.MkdirTemp returns a path with backslashes. To ensure compatibility with the DuckDB sandbox allowlist (which uses forward slashes), the resulting tempDir should be normalized. This ensures that all paths constructed from it (like the one passed to importFile) match the allowlist prefixes.
tempDir, err := os.MkdirTemp(h.uploadDir, "arc-import-*")
tempDir = filepath.ToSlash(tempDir)…ort + delete handlers Addresses Gemini Code Assist 3rd review on PR #442: On Windows, os.MkdirTemp and os.CreateTemp return native-separator paths (backslashes). The DuckDB sandbox allowlist uses forward slashes (main.go normalizes every entry via filepath.ToSlash). Without ToSlash on the per-request temp paths: - internal/api/import.go: read_csv/read_parquet on the uploaded file would mismatch the allowlist prefix → permission error on every import. - internal/api/delete.go: COPY ... TO '<tempPath>' interpolates the path verbatim into SQL → same mismatch → DELETE fails on S3 backends on Windows hosts. ToSlash is a no-op on Linux/macOS (filepath.Separator == '/'), so no behavior change there.
|
@gemini-code-assist review please |
There was a problem hiding this comment.
Code Review
This pull request implements a comprehensive security sandbox for DuckDB to prevent arbitrary file access by authenticated callers. It introduces a structural fix using DuckDB's "allowed_directories" and "enable_external_access" settings, supported by robust path normalization, control character rejection, and symlink resolution logic. The update also simplifies extension loading to a one-shot database-wide operation and secures temporary staging areas for imports and deletions. Feedback suggests further hardening the configuration by adding Windows-specific system roots to the "deniedRoots" list to prevent accidental broad access on Windows deployments.
| deniedRoots := map[string]bool{ | ||
| "/": true, "/etc": true, "/etc/": true, "/usr": true, "/usr/": true, | ||
| "/bin": true, "/bin/": true, "/sbin": true, "/sbin/": true, | ||
| "/boot": true, "/boot/": true, "/proc": true, "/proc/": true, | ||
| "/sys": true, "/sys/": true, "/dev": true, "/dev/": true, | ||
| "/tmp": true, "/tmp/": true, "/var": true, "/var/": true, | ||
| "/var/tmp": true, "/var/tmp/": true, | ||
| "/home": true, "/home/": true, "/root": true, "/root/": true, | ||
| "/Users": true, "/Users/": true, | ||
| } |
There was a problem hiding this comment.
The deniedRoots map provides excellent protection against accidental misconfiguration that would otherwise neuter the sandbox. While it covers common POSIX system roots, consider adding Windows-specific roots (e.g., C:\, D:\) if Windows support is a priority, although the current implementation acknowledges Windows-on-server is an unusual deployment for this project.
There was a problem hiding this comment.
We do not support Windows or Windows Server.
|
@gemini-code-assist review please |
There was a problem hiding this comment.
Code Review
This pull request implements a security hardening measure for the DuckDB integration by introducing a sandbox mechanism. It replaces the previous denylist approach with a structural allowlist (allowed_directories) and disables external access (enable_external_access = false) at the DuckDB layer. The changes include robust path normalization, symlink resolution, and control character rejection for operator-supplied paths to prevent bypasses. Additionally, the import and delete handlers were updated to ensure staging directories are correctly allowlisted, and a comprehensive test suite (TestSandbox) was added to verify the security boundary. I have kept the review comment regarding the deniedRoots check as it suggests a valid improvement for the path validation logic.
| if pair.value != "" && deniedRoots[pair.value] { | ||
| log.Fatal().Str("setting", pair.name).Str("path", pair.value).Msg("Configured path refuses to start with a system root, shared-tmp, or user-home root; pick a dedicated data directory") | ||
| } |
There was a problem hiding this comment.
The deniedRoots check currently only performs exact matches against a predefined list of system directories. While this prevents typos like / or /etc, it does not block subdirectories of these sensitive roots (e.g., /etc/shadow or /root/.ssh). Although the operator owns the configuration, extending this check to block any path that has a denied root as a prefix would provide stronger protection against accidental misconfiguration.
… not just exact /etc Addresses Gemini Code Assist 5th review on PR #442. The previous deny-list used exact-string equality, which caught `local_path = "/etc"` but not `local_path = "/etc/arc-data"` — the latter would still neuter the sandbox by allowlisting all of `/etc/arc-data/` and (if any file landed there from another source) exfiltrating it through Arc. Replace the map with a prefix-anchored check that matches: - exact root: `/etc` - trailing-slash form: `/etc/` - any subdirectory: `/etc/...` Anchored on `root + "/"` so `/etcd-data` is NOT matched by `/etc` — only true subdirectories or the bare directory. Narrowed the deny-list to roots where ANY subdirectory is suspect (/etc, /usr, /bin, /sbin, /boot, /proc, /sys, /dev, /root). Dropped /var, /home, /tmp, /Users from deny because production deployments legitimately put Arc data under /var/lib/arc, /home/arc-user/data, or /Users/<dev>/arc-data — exact-match was already protecting against the typo (`local_path = "/var"`) and prefix-match here would block real deployments. Operator still owns those areas.
|
@gemini-code-assist review please |
There was a problem hiding this comment.
Code Review
This pull request implements a structural security fix to address a DuckDB I/O sandbox vulnerability that allowed arbitrary file reads. The change replaces a previous denylist with a robust allowlist mechanism using DuckDB's allowed_directories and enable_external_access settings. Key improvements include strict path normalization and validation in the startup sequence to prevent bypasses via symlinks or control characters, and the redirection of temporary file operations (imports, S3 deletes, and profiling) into allowlisted directories. Feedback suggests considering a fail-fast approach (using log.Fatal) instead of a warning when the allowlist is empty, as this state effectively disables most file-based functionality.
| if len(dirs) == 0 { | ||
| componentLogger.Warn().Msg("sandbox allowlist is empty — every file-touching query will fail; check Config.LocalStorageRoot / TempDirectory / UploadDir / CompactionTempDirectory / S3Bucket / ColdS3Bucket / AzureContainer / ColdAzureContainer wiring") | ||
| } |
There was a problem hiding this comment.
While logging a warning for an empty allowlist is good, consider if the application should fail-fast (log.Fatal) if the allowlist is empty, as an empty allowlist effectively disables all DuckDB file-based functionality, which is likely a misconfiguration in production.
…mpty Addresses Gemini Code Assist 6th review on PR #442. Gemini argued the empty-allowlist branch should Fatal because it disables all DuckDB file-based functionality. They're right that production should never reach that state — every operator path contributes to the allowlist, and main.go defaults TempDirectory to "./.tmp" so the allowlist is always non-empty. But the Fatal belongs in main.go, not in internal/database/sandbox.go. The library function is also called by: - TestSandboxEmptyAllowlistLogsButDoesNotPanic (which explicitly asserts the Warn-and-continue semantics; deleting that test to accommodate a Fatal would also delete the documentation of what happens when a library caller passes a minimal Config). - Programmatic embeddings that construct database.Config{...minimal...} directly — round-1 review #1 flagged this and the test was written for them. The production guard in main.go fires AFTER all path resolution and symlink handling completes, before database.New. main.go's "./.tmp" fallback for TempDirectory makes this branch effectively unreachable today; the guard is a safety net for a future refactor that drops the fallback. The Warn in sandbox.go remains so library callers still get a clear signal.
|
@gemini-code-assist review please |
There was a problem hiding this comment.
Code Review
This pull request implements a robust security sandbox for DuckDB to prevent arbitrary file-read vulnerabilities. It replaces the previous denylist approach with a structural fix using DuckDB's allowed_directories and enable_external_access settings. Key changes include path normalization and validation in the main entry point, ensuring all temporary and upload directories are within the allowlist, and transitioning extension loading to a database-wide model. Comprehensive tests were added to verify the sandbox enforcement across various I/O functions and connection pools. I have no feedback to provide.
…ner + anchor + normalise PublicPrefixes match (#443) * fix(security): gate pprof behind ARC_DEBUG_PPROF on a localhost listener + anchor + normalise PublicPrefixes match Closes audit finding #2 from the 2026-05-19 external report (Alex Manson / @NeuroWinter). Pre-26.06.1 internal/api/server.go called app.Use(pprof.New()) unconditionally on the public Fiber app, and cmd/arc/main.go added /debug/pprof to the auth middleware's PublicPrefixes list. Any network-reachable caller — no token, no auth — could fetch heap dumps (leaking in-flight SQL strings, msgpack records, cached *TokenInfo), leak goroutine stacks, or pin a CPU core via /debug/pprof/profile?seconds=N. This PR removes pprof from the public Fiber app entirely. The endpoints are now opt-in via ARC_DEBUG_PPROF=1 on a separate listener bound to 127.0.0.1:6060 by default. ARC_DEBUG_PPROF_ADDR overrides the bind; a non-loopback override additionally requires ARC_DEBUG_PPROF_ALLOW_NON_LOOPBACK=1 so a single typo cannot expose pprof to the network. The listener uses a private *http.ServeMux (not http.DefaultServeMux), shuts down via srv.Close() instead of srv.Shutdown(ctx) so a long /debug/pprof/profile?seconds=N capture cannot starve the coordinator's 30s shutdown budget, and bounds slow-client attacks via ReadHeaderTimeout/WriteTimeout/IdleTimeout. Bundled defense-in-depth fix to the auth middleware's PublicPrefixes matcher (same shape as gemini-flagged deniedRoots gap on PR #442): - Anchored match: requires exact-equal or true-subdirectory (prefix + "/"). Sibling paths like /metricsX, /metrics-secret no longer slip through. - path.Clean normalisation before match: non-canonical request shapes like /metrics//foo, /metrics/./x, /metrics/../sensitive are normalised to their canonical form before the bypass branch checks them. Without normalisation a request for /metrics/../api/v1/query would lexically pass the anchored HasPrefix check; after normalisation it becomes /api/v1/query and correctly requires auth. - Empty-prefix guard: skips empty entries so a future config bug that lands "" in PublicPrefixes can't open the whole API. With /debug/pprof removed from the public prefix list, the matcher changes are not currently reachable for any production route — they're guards for any prefix added in the future. Tests added: - TestServer_PprofNotRegisteredOnPublicApp (12 pprof paths against the public Fiber app, all must 404). - TestMiddleware_PublicPrefixes_AnchoredMatch (10 subtests covering exact/trailing-slash/subdir bypass + sibling-byte-prefix shapes + parent-traversal escape shapes + empty-prefix guard). - TestStartDebugPprofIfEnabled_NoopWhenDisabled, _BindsAndServes, TestIsTruthy, TestIsLoopbackBindAddr. Reported by Alex Manson (@NeuroWinter, https://neurowinter.com/). Three rounds of internal multi-agent review caught: shutdown DoS chain via long pprof captures starving downstream shutdown hooks (HIGH), un-normalised c.Path() admitting double-slash / dot-segment / parent- traversal into the bypass branch (HIGH), factually wrong DefaultServeMux comment, duplicate component logger field, listener race window between RegisterHook and goroutine schedule, missing WriteTimeout/IdleTimeout, non-loopback bind needing two opt-ins, empty-string PublicPrefixes guard, builtin min vs reinvented minInt, duplicate /metrics append, stale 30-second http-server comment. All addressed. * fix(security): align release notes with code + strengthen pprof test against the gemini-r3 suggestion Addresses Gemini Code Assist review on PR #443: - G1 (release notes:55): said pprof logs a "Warn" for non-loopback binds, but the code upgrades to Error AND requires the second opt-in env var ARC_DEBUG_PPROF_ALLOW_NON_LOOPBACK=1. Drift introduced by my own earlier fixup; release notes now match. - G2 (release notes:65): said the anchored-match test covers "7 paths" but the actual test has 10 subtests (expanded during the review fixup). Updated. - G3 (debug_pprof.go:104): Gemini suggested removing the explicit HandleFunc registrations for /debug/pprof/{cmdline,profile,symbol, trace}, claiming pprof.Index covers them. Empirically refuted: pprof.Index dispatches via runtime/pprof.Lookup, which contains only the runtime profiles (heap, goroutine, allocs, etc.). The four special endpoints are separate functions in net/http/pprof and require their own registrations — without them, requests return 404 "Unknown profile". Strengthened TestStartDebugPprofIfEnabled_BindsAndServes to hit /debug/pprof/cmdline and /debug/pprof/symbol so a future contributor accepting the same suggestion blindly would catch the regression in CI. (profile and trace skipped because they actively capture data over seconds.) Reply posted on the Gemini comment thread with the empirical evidence. * fix(security): normalise trailing slash on PublicPrefixes entries before anchored match Addresses Gemini Code Assist 2nd review on PR #443. A configured prefix with a trailing slash (e.g. `PublicPrefixes = ["/metrics/"]`) silently broke the anchored match because `prefix + "/"` became `/metrics//`, which matches no real request path. Operator who reads "this is a directory prefix" and adds the slash for clarity would silently lose bypass on every subpath under `/metrics/`. Today this is unreachable: neither DefaultMiddlewareConfig nor cmd/arc/main.go uses a trailing-slash entry. The fix is a footgun guard for any future config that does. Fix: `prefix = strings.TrimSuffix(prefix, "/")` inside the loop, before the existing anchored check. Symmetric with the request-path normalisation (`path.Clean`) above; both sides are now canonical before equality and HasPrefix run. Deliberately NOT path.Clean on the prefix because it's operator-supplied static config (not attacker-controlled) and Clean would also collapse a configured `//foo` to `/foo` — masking the misconfiguration instead of treating it as written. Empty-prefix guard moved after the TrimSuffix so a configured `/` (which the trim just emptied) is also caught. Tests added: - TestMiddleware_PublicPrefixes_TrailingSlashNormalisation runs the same 4-case matrix against both `/metrics` and `/metrics/` configured prefixes; both must produce byte-identical bypass/require-auth results. --------- Co-authored-by: Ignacio Van Droogenbroeck <ignacio@vandroogenbroeck.net>
Gemini round 2 reversed its round-1 suggestion: parallelising the two cache sweeps adds more overhead (goroutine spawn + WaitGroup sync ~ hundreds of ns) than it removes (the cleanup() itself is ~µs at realistic key cardinality). Round 1 was a pessimisation. Reverting to sequential, but keeping the comment noting the trigger for revisiting: a profile showing the sweep itself blocking concurrent get() calls under a hot-cache workload. Also drops the "concurrent sweep" bullet from the 26.06.1 release notes. Declining the round-2 metadata-cleanup suggestion (remove PR numbers and "Gemini round N" references from comments) — the project convention, established across PRs #442–#449, is to keep these audit trails in code. Future reviewers benefit from seeing "this shape exists because round N flagged X."
* fix(pruning): wire periodic janitor for partition pruner caches (Q2) internal/pruning/partition_pruner.go has two TTL caches — globCache (30s) and partitionCache (60s) — both used on every query. Their get() returns "expired" as a miss but does NOT evict the stale entry; neither cache has a max-size cap. The public CleanupGlobCache() and CleanupPartitionCache() methods existed since the 2024-12 and 2026-01 cache PRs but had ZERO production callers (verified: only definitions + test files + a progress doc reference them). High-cardinality workloads accumulate map entries monotonically until either InvalidateAllCaches() runs post- compaction or the process exits — one component of the 24h RSS climb on the satellite-data demo. Fix: PartitionPruner.StartCleanup(ctx, interval) spawns a janitor goroutine that sweeps both caches at DefaultCleanupInterval = 30s (matches the shorter TTL → bounds worst-case retention at ~2× TTL). Exits cleanly on ctx cancel. Idempotent via cleanupStarted atomic — repeat calls log a warn and return without spawning a second goroutine. QueryHandler.StartBackgroundWorkers(ctx) is the seam from main.go; cmd/arc/main.go wires it via context.WithCancel + a shutdown hook at PriorityHTTPServer (= 10, the earliest tier — the janitor has no flush state, just a ticker + an in-memory map). Matches the WAL maintenance pattern at line 730. Identified by the 2026-05-22 memory-retention triage. * fix(pruning): concurrent cache sweeps + release notes (Gemini round 1) Gemini medium finding: run glob + partition cache cleanups on independent goroutines per tick. The two mutexes are unrelated (globCache.mu vs partitionCache.mu) so a future hot-glob-cache that grows between sweeps shouldn't delay the partition sweep behind it, or vice versa. WaitGroup keeps the tick boundary sensible — both must finish before the next ticker fires. Also adds the missing release notes entry under "Bug fixes" in RELEASE_NOTES_2026.06.1.md (oversight in 3329e68). * fix(pruning): revert concurrent sweep to sequential (Gemini round 2) Gemini round 2 reversed its round-1 suggestion: parallelising the two cache sweeps adds more overhead (goroutine spawn + WaitGroup sync ~ hundreds of ns) than it removes (the cleanup() itself is ~µs at realistic key cardinality). Round 1 was a pessimisation. Reverting to sequential, but keeping the comment noting the trigger for revisiting: a profile showing the sweep itself blocking concurrent get() calls under a hot-cache workload. Also drops the "concurrent sweep" bullet from the 26.06.1 release notes. Declining the round-2 metadata-cleanup suggestion (remove PR numbers and "Gemini round N" references from comments) — the project convention, established across PRs #442–#449, is to keep these audit trails in code. Future reviewers benefit from seeing "this shape exists because round N flagged X." * fix(pruning): clamp tiny cleanup intervals (Gemini round 3) Gemini round-3 finding (1 of 2): a caller passing sub-millisecond interval would pin a CPU in a tight ticker loop. Adds a 1ms floor via minCleanupInterval; sub-floor requests log a Warn naming both the requested and clamped values. 1ms is several orders of magnitude below any plausible production interval AND below every fast-iteration test in this package (which use 5-10ms). Declining round-3 finding (2 of 2) — "reset cleanupStarted on goroutine termination to support hot-reload": YAGNI. There is no hot-reload path today and adding the reset expands the API surface (state-machine invariants, more atomics, tests for the overlap window between cancel and second-start). Will revisit when a concrete restart use case lands. * docs(pruning): trim ephemeral review references + spelling (Gemini round 4) Drops the "Gemini round N / PR #450" stamps and the 2026-05-22-triage reference from comments. For a 4-round PR on a small fix, the audit trail belongs in the PR description and commit log, not in the source. (The X1 PR kept references in-source because 12 review rounds genuinely needed the trail; calibrate per PR size.) Also "parallelising" → "parallelizing" to match the project's predominantly US spelling. No behavior change. --------- Co-authored-by: Ignacio Van Droogenbroeck <ignacio@vandroogenbroeck.net>
1 participant
Summary
Closes a critical file-read vulnerability where authenticated users (even with
permissions: []) could read arbitrary local files via DuckDB's I/O function family (read_csv_auto,read_json,read_text,read_blob,glob,parquet_metadata,parquet_schema, etc.). The user-SQL denylist only blockedread_parquet/arc_partition_agg, and RBAC inspected onlyFROM/JOINclauses, so scalar table functions inSELECTslipped past both layers. Combined with the loadedhttpfsextension, the same primitive reached SSRF (instance-metadata IPs),auth.dbexfiltration, and cross-tenant Parquet reads.Replaces the maintenance-treadmill denylist with a structural fix at the DuckDB layer: after every
INSTALL/LOADcompletes,SET GLOBAL allowed_directories = [...]for Arc's storage roots + tier prefixes + import upload dir + compaction temp dir, thenSET GLOBAL enable_external_access = false(one-way at runtime), then verify the flag flipped via a context-bounded readback.Reported by Alex Manson (@NeuroWinter, https://neurowinter.com/).
Operator-facing changes
storage.local_path,database.temp_directory,compaction.temp_directory, andarcx.storage_rootare resolved to absolute, forward-slash paths before being passed to DuckDB. Defaults continue to work; non-absolute operator values are normalised against the process CWD at startup.Fatal./,/etc,/usr,/bin,/sbin,/boot,/proc,/sys,/dev,/tmp,/var,/home,/root,/Users) are refused asstorage.local_pathso a typo cannot neuter the sandbox.os.TempDir()to a dedicatedarc-uploadssubdirectory underdatabase.temp_directory. The dir is created0700, rejected if pre-staged as a symlink, and its ancestor chain is resolved withEvalSymlinks(Warn + use resolved path on mismatch, so Docker bind-mounts and macOS/var → /private/varcontinue to work).DELETEon S3 backends and profile-mode query temp files now share the same allowlisted directory; both fail-closed at request time when misconfigured.LOAD+SET GLOBALduringconfigureDatabase; the previous per-connectionconnInitFnrelied on the incorrect premise that DuckDB extension state is per-connection.Test plan
go build ./...cleango test ./internal/database/... ./internal/api/...— all greenTestSandboxsubtests: CVE reproduction + full I/O family + SSRF +COPY TOlocal +COPY TO 's3://...'outside allowlist +EXPORT DATABASE+INSTALLafter lockdown + cross-connection enforcement +range()remains callable + lockdown is one-wayTestBuildAllowedDirectories— 12 table cases covering hot/cold S3 full-URI dedup, same-bucket-different-prefix, leading/interior-slash collapse, parent-traversal..fallback, trailing-slash idempotence, empty-config behaviorTestSandboxEmptyAllowlistLogsButDoesNotPanic— confirms the sandbox locks down even with an empty allowlist (operator misconfiguration fails fast)TestArcxLoadsAndReportsVersion,TestArcxStorageRootIsSetOnEveryConn) — pass with the realarcx.duckdb_extensionbinary, confirmingarcx_version()andSET GLOBAL arcx.storage_rootpropagate database-wide across distinct concurrent pool connectionsmake build && ./arcsmoke test against local Parquet data — JSON query path returns correct results across 20-query benchmark suiteInternal review
Three rounds of multi-agent post-implementation review per
.claude/CLAUDE.md:..in S3 prefix, same-bucket-different-prefix dedup bug, compaction wiring divergence) + 2 mediums (SQL injection via profile path, doc drift in release notes).All findings addressed across the same commit. The structural CVE fix is unchanged from round 1; rounds 2-3 hardened the wiring and closed defensive gaps that production deployments would otherwise have hit.
🤖 Generated with Claude Code