Add query support to local MCP server by bradhe · Pull Request #328 · tower/tower-cli

bradhe · 2026-07-21T10:59:36Z

Title says it all. Should make our output a little nicer/easier to use in some cases!

coderabbitai · 2026-07-21T10:59:44Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: aaa8a7b8-837a-44b4-8251-917ea0b3b65e

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch features/add-catalog-querying-to-mcp-server

_{Comment @coderabbitai help to get the list of available commands.}

konstantinoscs

I think we need to reconsider this. Basically this PR allows agents to run arbitary SQL that gets directly forwarded to duckDB.

DuckDB SQL can read local files, access the network, load extensions, and consume unbounded resources. DuckDB’s own security guidance says to treat untrusted SQL like Bash or Python code and recommends OS/container/WASM isolation. See:
https://duckdb.org/docs/current/operations_manual/securing_duckdb/overview

To be fair, this happens already in the human operated mode and probably this should be changed as well but an agent increases the security risk manyfold.

Btw, in the monorepo we avoid this by rusing WASM and adding read-only and single-statement "gates".

I think we need to do some sandboxing here

konstantinoscs

My previous security concern remains

konstantinoscs · 2026-07-23T08:26:39Z

    }

+    #[tool(
+        description = "Run a read-only SQL query against a Tower-managed storage (Iceberg) catalog. Tables must be fully qualified as <catalog>.<namespace>.<table>. Use tower_catalogs_show first to discover namespaces and tables. Returns the result columns and rows as JSON."


That MCP method only returns catalog properties so iiuc, it won't work. Have you tried this? Probably should add tests as well

Good catch — the MCP tower_catalogs_show only returned properties, so an agent couldn't actually discover namespaces/tables from it. It now lists tables too, via a shared list_catalog_tables helper (the same path the CLI show uses), returning tables: [{namespace, table}] (plus tables_error when the listing fails, without failing the whole call). Added unit tests for the query gates, row truncation, and session hardening. Fixed in aeaf9cb.

konstantinoscs · 2026-07-23T08:28:33Z

    }

+    #[tool(
+        description = "Run a read-only SQL query against a Tower-managed storage (Iceberg) catalog. Tables must be fully qualified as <catalog>.<namespace>.<table>. Use tower_catalogs_show first to discover namespaces and tables. Returns the result columns and rows as JSON."


Also, here we claim a "read-only" query but me understanding is that if something provides "read-write" credentials it will work. Agent mutations should be guarded imo

We can easily disallow read-write credentials to the catalog, you're right that we should come up with a way to sandbox the DuckDB session overall.

Done — removed the write option from the MCP tool entirely. It always vends read-only credentials and attaches READ_ONLY, and there's now a leading-keyword gate that rejects write/DDL statements as defence-in-depth. --write stays only on the human CLI path.

konstantinoscs · 2026-07-23T08:29:46Z

+        match crate::catalogs::query_catalog(&self.config, &request.name, environment, sql, write)
+            .await
+        {
+            Ok(result) => {


Results are unbounded right? That could lead into memory exhaustion. I can foresee an agent trying to do a query for millions of rows. We should either bound the result or return some truncated marker. In the web ui we have a limit of 100k rows.

This is a good suggestion.

Bounded — agent queries are now capped at 1000 rows and the response carries "truncated": true when more matched. I went below the UI's 100k because these rows land directly in a model's context; it's a single AGENT_MAX_ROWS const if we want to tune it.

konstantinoscs · 2026-07-23T08:32:32Z

+                        )
+                    })
+                    .collect();
+                Self::json_success(json!({


I think here you are silently overwriting duplicate column names. A join query could potentially produce duplicate names which are both valid but here the last one will win. In the UI we are using "positional arrays" exactly for that reason (see catalogService.ts)

Fixed — the MCP response now returns positional row arrays (rows: [[...]], one value per column in column order) like catalogService.ts, so duplicate names from a join aren't collapsed.

Address review feedback on the MCP catalog query tool, which forwarded arbitrary SQL straight to DuckDB. - Enforce read-only: drop the `write` option, always vend read-only credentials, and reject write/DDL statements by leading keyword. - Gate to a single statement. duckdb-rs `prepare` executes every statement but the last as a side effect, so multi-statement SQL would run its leading statements even though only the final one is returned. - Harden the DuckDB session after attach: disable local-filesystem access, block community extensions, and lock configuration. Network access stays on because httpfs is how the Iceberg catalog reads S3. - Cap agent results at 1000 rows with a `truncated` marker. - Return rows as positional arrays so duplicate columns from a join aren't silently collapsed. - `tower_catalogs_show` now lists namespaces/tables via a shared `list_catalog_tables` helper, so agents can discover what to query. The human CLI query path is left unsandboxed and uncapped.

bradhe · 2026-07-23T09:46:01Z

Reworked the agent (MCP) query path in aeaf9cb to be read-only and sandboxed rather than a raw DuckDB passthrough:

Read-only: no write option; always read-only creds + READ_ONLY attach; write/DDL rejected by leading keyword.
Single statement: gated before execution — duckdb-rs prepare runs every statement but the last as a side effect, so multi-statement SQL was a real hole.
Session hardening after attach: disabled_filesystems='LocalFileSystem' (blocks read_csv('/etc/passwd') & co.), allow_community_extensions=false, lock_configuration=true.
Bounded results: 1000-row cap with a truncated marker.

Honest caveat: httpfs has to stay enabled — it's how the attached Iceberg catalog reads S3 — so network access remains. This narrows the surface a lot but isn't full isolation; the OS/container/WASM sandboxing we discussed is the larger follow-up, and I left the human CLI path unsandboxed for now. Tests added; cargo test -p tower-cmd is green.

Two regression tests for the catalog query change: - Hermetic Rust test: write a small Iceberg table with DuckDB's `COPY ... (FORMAT iceberg)` and read it back through run_duckdb_query, so the iceberg reader and our column/positional-row extraction are exercised against real Iceberg metadata + parquet (NULLs and the row cap included). Needs the `iceberg` extension, fetched on first run. - behave scenarios (tests/integration/features/mcp_catalogs.feature) drive tower_catalogs_show / tower_catalogs_query over the real MCP stdio transport: show lists tables, a read-only SELECT returns positional rows, and write/multi-statement SQL is rejected. Gated on TOWER_TEST_CATALOG (+ a real TOWER_URL); skipped otherwise so the mock CI run is a no-op.

…orage Stand up MinIO via testcontainers, seed a real Iceberg table on it, and read it back through the hardened setup — proving the sandbox does not break object-store reads while still blocking local-filesystem access and configuration changes in the same session. This is the coverage the local-only tests can't give: the hardening disables LocalFileSystem, so a local Iceberg fixture can't exercise it. Reading real S3-backed Iceberg data confirms the reader goes through S3FileSystem (left enabled), so the production query path is not broken by the sandbox. The test manages the container lifecycle itself and self-skips when no Docker daemon is available, so `cargo test` still passes without Docker. Adds a dev-only testcontainers dependency.

Encode known attack vectors an agent query might attempt and assert the sandbox refuses each, so a future weakening of the gates or hardening fails a test: - data tampering (write/DDL) and statement smuggling -> gate-rejected - host filesystem reads (exfiltration) and writes (payloads) -> blocked - configuration escape (SET/RESET/PRAGMA to unwind the lockdown) -> blocked - arbitrary/community extension loading -> blocked - network SSRF via table functions (cloud-metadata / internal) -> blocked Complements the MinIO integration test, which proves the hardening does not break the legitimate S3 read path.

bradhe · 2026-07-24T08:24:19Z

Superseded by #331.

develop's query path diverged from this branch, so rather than rebasing I lifted the hardening out as its own foundation and re-applied it as a clean diff on top of develop. #331 carries your review feedback forward, meaning read-only enforcement, DuckDB session sandboxing, and bounded results, and adds an adversarial test suite that pins each of those invariants down (host filesystem reads/writes, statement smuggling, config escape, extension loading, SSRF) plus a testcontainers MinIO test proving the hardening does not break object-store Iceberg reads.

#331 is the primitives and tests only; the MCP tower_catalogs_query tool that consumes them lands in a follow-up. I'll leave closing this one to you.

…crate and wire the sandbox in The sandbox primitives from the previous commit had no caller: the gates, the session hardening, and the row cap all lived in an inline `#[allow(dead_code)]` module in catalogs.rs, proven only by tests. This moves Tower's DuckDB usage into a dedicated `tower-duckdb` crate and makes `tower catalogs query` its first real consumer, so the hardening runs in production rather than sitting on a shelf. The crate owns the whole DuckDB surface: opening an in-memory Session, running trusted setup, locking the session down for untrusted SQL (Session::harden), and executing a query into JSON rows with an optional row cap. The static text gates (reject write/DDL, reject multi-statement) live in its `guard` module. The value conversion, the query execution, and the full adversarial + integration test suite move with it, so the security invariants are tested next to the code they protect. tower-cmd no longer depends on duckdb directly; it goes through the crate. `catalogs query` now splits by mode. Read mode is the default and the sandboxed path: the SQL is gated before it reaches DuckDB (a smuggled second statement would otherwise run as a side effect of prepare, and a write gets a clear message instead of a raw engine error), the session is hardened after attach, and rows are capped with a truncation notice. Write mode stays the trusted power-user opt-in and runs as before. Once this lands, #328 will layer the MCP `tower_catalogs_query` tool on top of the same crate.

Add query support to local MCP server

bff4160

bradhe requested a review from socksy July 21, 2026 10:59

konstantinoscs requested changes Jul 23, 2026

View reviewed changes

bradhe added 3 commits July 23, 2026 11:12

bradhe mentioned this pull request Jul 24, 2026

feat: tower-duckdb crate — sandboxed catalog queries and adversarial tests #331

Open

Uh oh!

Conversation

bradhe commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

konstantinoscs left a comment

Choose a reason for hiding this comment

Uh oh!

konstantinoscs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bradhe commented Jul 23, 2026

Uh oh!

bradhe commented Jul 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading