← back to Cli Printing Press
fix(cli): close MCP sql tool exfiltration vector with allowlist + read-only handle (#709)
b196bd813a0a4eb58f763b7d4d38563208c1a5d0 · 2026-05-08 00:23:04 -0700 · Trevin Chow
The generated MCP `sql` and `search` tools advertised
ReadOnlyHintAnnotation(true) but opened a read-write SQLite handle behind
a 6-keyword prefix blocklist that VACUUM INTO and ATTACH DATABASE bypass.
Two empirical findings shaped the fix:
- modernc.org/sqlite silently drops `?mode=ro` unless the DSN starts with
`file:`. The reference fix in multimail-cli@85af2bf had this hidden bug;
its security came entirely from the keyword blocklist, not mode=ro.
- mode=ro does NOT block VACUUM INTO (writes a snapshot to a new file) or
ATTACH DATABASE (opens a separate writable handle). Application-layer
rejection is required for those vectors.
Code review then surfaced a comment-prefix bypass class:
`/* x */ VACUUM INTO ...`, `-- x\nATTACH DATABASE ...`, and `;VACUUM INTO`
all bypass `strings.HasPrefix(strings.ToUpper(strings.TrimSpace(query)))`
because TrimSpace doesn't understand SQL comments. SQLite happily strips
comments before parsing the first keyword. Empirically verified six bypass
forms all created an exfil file under the supposedly read-only handle.
Switched from blocklist to allowlist with proper noise stripping:
- New `validateReadOnlyQuery` helper accepts only SELECT/WITH after
`stripLeadingSQLNoise` strips leading whitespace, line comments,
block comments, and semicolons.
- New `OpenReadOnly(dbPath)` constructor uses `file:` URI prefix to make
`mode=ro` actually engage. CTE-wrapped writes (allowlist passes WITH)
fail at the driver layer.
- handleSQL dispatches through validateReadOnlyQuery; both handleSQL and
handleSearch open the store via OpenReadOnly.
`WITH` is intentionally allowed: three already-published printed CLIs
(pokeapi, producthunt, fedex) accept WITH-prefix CTEs as legitimate
read-only queries; the MCP surface keeps parity with the human CLI
surface per the agent-native rule.
New `mcp_tools_test.go.tmpl` ships into every printed CLI's mcp package,
exercising 29 attack inputs (direct writes, CTE-wrapped writes,
comment-prefix bypasses, statement-separator bypasses) plus positive
cases. Generator-side `TestGenerateMCPSQLToolUsesReadOnlyStore` pins the
emission shape so a regression fails fast. Three golden fixtures
regenerated.
Closes #693.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Files touched
A docs/solutions/security-issues/mcp-sql-search-readonly-bypass-2026-05-08.mdM internal/generator/generator.goM internal/generator/generator_test.goM internal/generator/templates/mcp_tools.go.tmplA internal/generator/templates/mcp_tools_test.go.tmplM internal/generator/templates/store.go.tmplM internal/generator/templates/store_schema_version_test.go.tmplM testdata/golden/expected/generate-golden-api-rich-auth/printing-press-rich-auth/internal/mcp/tools.goM testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.goM testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go
Diff
commit b196bd813a0a4eb58f763b7d4d38563208c1a5d0
Author: Trevin Chow <trevin@trevinchow.com>
Date: Fri May 8 00:23:04 2026 -0700
fix(cli): close MCP sql tool exfiltration vector with allowlist + read-only handle (#709)
The generated MCP `sql` and `search` tools advertised
ReadOnlyHintAnnotation(true) but opened a read-write SQLite handle behind
a 6-keyword prefix blocklist that VACUUM INTO and ATTACH DATABASE bypass.
Two empirical findings shaped the fix:
- modernc.org/sqlite silently drops `?mode=ro` unless the DSN starts with
`file:`. The reference fix in multimail-cli@85af2bf had this hidden bug;
its security came entirely from the keyword blocklist, not mode=ro.
- mode=ro does NOT block VACUUM INTO (writes a snapshot to a new file) or
ATTACH DATABASE (opens a separate writable handle). Application-layer
rejection is required for those vectors.
Code review then surfaced a comment-prefix bypass class:
`/* x */ VACUUM INTO ...`, `-- x\nATTACH DATABASE ...`, and `;VACUUM INTO`
all bypass `strings.HasPrefix(strings.ToUpper(strings.TrimSpace(query)))`
because TrimSpace doesn't understand SQL comments. SQLite happily strips
comments before parsing the first keyword. Empirically verified six bypass
forms all created an exfil file under the supposedly read-only handle.
Switched from blocklist to allowlist with proper noise stripping:
- New `validateReadOnlyQuery` helper accepts only SELECT/WITH after
`stripLeadingSQLNoise` strips leading whitespace, line comments,
block comments, and semicolons.
- New `OpenReadOnly(dbPath)` constructor uses `file:` URI prefix to make
`mode=ro` actually engage. CTE-wrapped writes (allowlist passes WITH)
fail at the driver layer.
- handleSQL dispatches through validateReadOnlyQuery; both handleSQL and
handleSearch open the store via OpenReadOnly.
`WITH` is intentionally allowed: three already-published printed CLIs
(pokeapi, producthunt, fedex) accept WITH-prefix CTEs as legitimate
read-only queries; the MCP surface keeps parity with the human CLI
surface per the agent-native rule.
New `mcp_tools_test.go.tmpl` ships into every printed CLI's mcp package,
exercising 29 attack inputs (direct writes, CTE-wrapped writes,
comment-prefix bypasses, statement-separator bypasses) plus positive
cases. Generator-side `TestGenerateMCPSQLToolUsesReadOnlyStore` pins the
emission shape so a regression fails fast. Three golden fixtures
regenerated.
Closes #693.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
.../mcp-sql-search-readonly-bypass-2026-05-08.md | 167 +++++++++++++++++++++
internal/generator/generator.go | 5 +
internal/generator/generator_test.go | 78 +++++++++-
internal/generator/templates/mcp_tools.go.tmpl | 68 +++++++--
.../generator/templates/mcp_tools_test.go.tmpl | 110 ++++++++++++++
internal/generator/templates/store.go.tmpl | 20 +++
.../templates/store_schema_version_test.go.tmpl | 55 +++++++
.../printing-press-rich-auth/internal/mcp/tools.go | 66 +++++++-
.../printing-press-golden/internal/mcp/tools.go | 68 +++++++--
.../tier-routing-golden/internal/mcp/tools.go | 66 +++++++-
10 files changed, 666 insertions(+), 37 deletions(-)
diff --git a/docs/solutions/security-issues/mcp-sql-search-readonly-bypass-2026-05-08.md b/docs/solutions/security-issues/mcp-sql-search-readonly-bypass-2026-05-08.md
new file mode 100644
index 00000000..2046352a
--- /dev/null
+++ b/docs/solutions/security-issues/mcp-sql-search-readonly-bypass-2026-05-08.md
@@ -0,0 +1,167 @@
+---
+title: Generated MCP sql tool opened read-write SQLite under a false readOnlyHint
+date: "2026-05-08"
+category: docs/solutions/security-issues/
+module: internal/generator/templates
+problem_type: security_issue
+component: tooling
+severity: high
+symptoms:
+ - "MCP sql and search tools declared ReadOnlyHintAnnotation(true) but opened a read-write SQLite handle, so MCP hosts auto-approved invocations that could write"
+ - "Keyword-prefix blocklist was defeated by SQL comment prefixes (/* x */ VACUUM INTO, -- x\\nATTACH DATABASE) and by leading semicolons"
+ - "VACUUM INTO wrote a full database snapshot to an attacker-chosen path under mode=ro"
+ - "ATTACH DATABASE opened a separate writable handle under mode=ro"
+root_cause: missing_permission
+resolution_type: code_fix
+tags:
+ - mcp
+ - sqlite
+ - readonly
+ - allowlist
+ - sql-injection
+ - modernc-sqlite
+ - generator-template
+related_components:
+ - database
+ - assistant
+---
+
+# Generated MCP sql tool opened read-write SQLite under a false readOnlyHint
+
+## Problem
+
+Generated MCP `sql` and `search` tools in every printed CLI shipping `VisionSet.Store` (49 of 52 in the public library) declared `ReadOnlyHintAnnotation(true)` to signal MCP hosts that auto-approval is safe. The underlying SQLite handle was opened read-write via `store.OpenWithContext`, and the only write-gate was a 6-keyword prefix blocklist applied with `strings.HasPrefix(strings.ToUpper(strings.TrimSpace(query)), keyword)`. Two independently bypassable attack classes existed:
+
+1. **Blocklist gap.** `VACUUM INTO` and `ATTACH DATABASE` were not in the blocklist. Either command executes against the read-write handle and exfiltrates or creates a writable copy of the local DB.
+2. **Comment-prefix bypass.** `TrimSpace` strips outer whitespace but does not understand SQL comment syntax. SQLite ignores leading `--` line comments, `/* */` block comments, and `;` statement separators before parsing the first keyword; the blocklist did not. Any blocked keyword reachable after such a prefix bypassed the gate entirely.
+
+Combined effect: a prompt injection in synced data could exfiltrate the local SQLite store through an MCP tool that hosts auto-approved as read-only, with no permission prompt.
+
+## Symptoms
+
+- `/* x */ VACUUM INTO '/tmp/exfil.db'` submitted to the MCP `sql` tool passes the blocklist and writes a full DB snapshot to disk.
+- `-- x\nVACUUM INTO ...` and `/**/VACUUM INTO ...` and `; VACUUM INTO ...` all pass the blocklist identically.
+- `/* x */ ATTACH DATABASE 'file:/tmp/x?mode=rwc' AS evil` passes the blocklist and opens a second writable handle at an attacker-controlled path.
+- Both vectors succeed on a handle opened with `?mode=ro` when the DSN lacks the `file:` URI prefix — modernc.org/sqlite silently drops `?mode=ro` without `file:`.
+- MCP hosts (Claude Code, Cursor) auto-approve the tool call without a permission prompt because `ReadOnlyHintAnnotation(true)` is set.
+
+## What Didn't Work
+
+- **Expanded blocklist without noise stripping.** The community proposal (issue #693) added `ATTACH`, `DETACH`, `PRAGMA`, `REINDEX`, `VACUUM`, and `WITH` to the prefix blocklist. This correctly identified the missing keywords but left the comment-prefix bypass class intact — any keyword in the expanded list is still reachable after a leading comment or semicolon.
+- **`WITH` was wrong to add.** Three already-published printed CLIs (pokeapi, producthunt, fedex) accept `WITH`-prefix CTEs as legitimate read-only queries in their hand-rolled novel `sql` commands. Blocking `WITH` on the MCP surface breaks agent-native parity (any action a CLI user can take, an agent can also take). `WITH` was excluded from the final solution; CTE-wrapped writes are caught by the `mode=ro` layer.
+- **`mode=ro` without `file:` URI prefix.** Empirical probe against modernc.org/sqlite showed that `dbPath+"?mode=ro&..."` (the exact DSN form the community proposal and the multimail-cli@85af2bf reference fix shipped) silently drops the `?mode=ro` parameter. INSERT succeeded against the supposedly read-only handle. Security in those implementations came entirely from the blocklist, not from `mode=ro`. The `file:` prefix is required to activate the URI parameter.
+- **`mode=ro` alone is not sufficient.** Even with the correct `file:` URI form, `mode=ro` does not block `VACUUM INTO` or `ATTACH DATABASE` in modernc.org/sqlite. Both commands succeed and create files on disk under a correctly opened read-only handle. Application-layer rejection is still required for those vectors.
+
+## Solution
+
+Switched from blocklist to allowlist with proper SQL noise stripping. Changes land in two generator templates plus a new test template.
+
+**`internal/generator/templates/mcp_tools.go.tmpl` — two new helpers, `handleSQL` and `handleSearch` updated:**
+
+```go
+// validateReadOnlyQuery applies an allowlist (SELECT or WITH) after
+// stripping leading whitespace, line comments, block comments, and
+// semicolons. SELECT and WITH are the only allowed leading keywords;
+// CTE-wrapped writes are caught by OpenReadOnly's mode=ro one layer down.
+func validateReadOnlyQuery(query string) error {
+ upper := strings.ToUpper(stripLeadingSQLNoise(query))
+ if !strings.HasPrefix(upper, "SELECT") && !strings.HasPrefix(upper, "WITH") {
+ return fmt.Errorf("only SELECT queries are allowed")
+ }
+ return nil
+}
+
+// stripLeadingSQLNoise removes leading whitespace, SQL line comments
+// (-- to end of line), block comments (/* ... */), and statement
+// separators (;). SQLite skips these before parsing the first keyword.
+func stripLeadingSQLNoise(query string) string {
+ for {
+ query = strings.TrimLeft(query, " \t\r\n;")
+ switch {
+ case strings.HasPrefix(query, "--"):
+ if idx := strings.IndexByte(query, '\n'); idx >= 0 {
+ query = query[idx+1:]
+ continue
+ }
+ return ""
+ case strings.HasPrefix(query, "/*"):
+ if idx := strings.Index(query[2:], "*/"); idx >= 0 {
+ query = query[2+idx+2:]
+ continue
+ }
+ return ""
+ default:
+ return query
+ }
+ }
+}
+```
+
+`handleSQL` dispatches through `validateReadOnlyQuery` before opening the store, and switches from `store.OpenWithContext(ctx, dbPath())` to `store.OpenReadOnly(dbPath())`. `handleSearch` switches to `store.OpenReadOnly(dbPath())` (search input is bound, not arbitrary SQL — but the tool surface advertises read-only, so the connection should match).
+
+**`internal/generator/templates/store.go.tmpl` — new `OpenReadOnly` constructor:**
+
+```go
+func OpenReadOnly(dbPath string) (*Store, error) {
+ db, err := sql.Open("sqlite",
+ "file:"+dbPath+"?mode=ro&_journal_mode=WAL&_busy_timeout=5000&_foreign_keys=ON&_temp_store=MEMORY&_mmap_size=268435456")
+ if err != nil {
+ return nil, fmt.Errorf("opening database (read-only): %w", err)
+ }
+ db.SetMaxOpenConns(2)
+ return &Store{db: db, path: dbPath}, nil
+}
+```
+
+The `file:` prefix is load-bearing; without it modernc.org/sqlite silently drops `?mode=ro` and the connection opens read-write.
+
+**`internal/generator/templates/mcp_tools_test.go.tmpl` (new) — ships into every generated CLI's `mcp` package:** exercises 29 attack inputs (direct writes, CTE-wrapped writes, comment-prefix bypasses, statement-separator bypasses) plus positive cases (SELECT, WITH-prefix CTEs, queries with leading comments). Wired in `internal/generator/generator.go` to render alongside `mcp_tools.go.tmpl` whenever `VisionSet.Store` is enabled.
+
+## Why This Works
+
+Two layers, both load-bearing:
+
+| Layer | What it catches | What it misses |
+|-------|----------------|----------------|
+| Allowlist (`SELECT` or `WITH` only, after noise stripping) | `VACUUM INTO`, `ATTACH DATABASE`, `PRAGMA`, all DDL/DML, comment-prefixed bypasses of any of the above | (nothing the next layer doesn't catch) |
+| `mode=ro` (`file:` URI + `mode=ro` DSN) | `INSERT`/`UPDATE`/`DELETE`/`REPLACE` direct writes; CTE-wrapped writes (`WITH x AS (...) INSERT ...`) | `VACUUM INTO`, `ATTACH DATABASE` — handled by the allowlist |
+
+Each layer covers what the other cannot. A comment-prefix attack that strips cleanly through `stripLeadingSQLNoise` to reveal a non-SELECT/WITH keyword is rejected by the allowlist before the store is opened. A CTE-wrapped write that passes the `WITH` allowlist is rejected by `mode=ro` at the driver level.
+
+The `ReadOnlyHintAnnotation(true)` annotation is now accurate: the tool cannot write to any SQLite database reachable from its handle.
+
+This fix follows the same tightening pattern as an earlier `newMCPClient()` change that defaulted `NoCache=true` for the MCP transport (auto memory [claude]). Both bugs were "the CLI behavior was acceptable for humans but leaked privilege or staleness through to the agent surface" — the MCP surface consistently requires stricter defaults than the CLI surface.
+
+## Prevention
+
+**Template-shipped test covers 29 attack inputs.** `mcp_tools_test.go.tmpl` exercises:
+
+- Direct writes: `INSERT`, `UPDATE`, `DELETE`, `REPLACE`, `DROP`, `CREATE`, `ALTER`, `PRAGMA`, `REINDEX`, `DETACH`
+- Comment-prefix bypasses: `/* x */ VACUUM INTO ...`, `-- x\nVACUUM INTO ...`, `/**/ATTACH ...`, `; VACUUM INTO ...`, `/* x */ ATTACH DATABASE ...`
+- Statement-separator bypasses: `;VACUUM INTO ...`, `; ; VACUUM INTO ...`
+- Positive cases: `SELECT ...`, `WITH x AS (SELECT 1) SELECT * FROM x`, queries with leading `-- comment\n` before SELECT
+
+Runs on every CI build in every generated CLI. CTE-wrapped write rejection is pinned by `TestOpenReadOnly_RejectsWrites` in `store_schema_version_test.go.tmpl`.
+
+**Generator-side test pins the emission shape.** `TestGenerateMCPSQLToolUsesReadOnlyStore` asserts:
+
+- `OpenReadOnly` is emitted in `store.go` with the `"file:"+dbPath+"?mode=ro` literal in the DSN
+- Both `validateReadOnlyQuery` and `stripLeadingSQLNoise` exist in `tools.go`
+- `handleSQL` dispatches through `validateReadOnlyQuery`
+- The allowlist contains both `SELECT` and `WITH`
+
+**Three rules for future maintainers working on SQL gates in generated CLIs:**
+
+- **Strip SQL noise before keyword checks.** Any prefix-style gate must strip leading whitespace, `--` line comments, `/* */` block comments, and `;` separators before the keyword comparison. SQLite ignores these before parsing the first keyword; gates that don't match this behavior are bypassable by construction.
+- **Use `file:` URI prefix for modernc.org/sqlite read-only handles.** `dbPath+"?mode=ro"` silently drops the parameter. The DSN must start with `file:` for `?mode=ro` to take effect.
+- **`mode=ro` does not block `VACUUM INTO` or `ATTACH DATABASE`.** These vectors must be rejected at the application layer (allowlist or explicit blocklist entry) before reaching the driver; they cannot be stopped by the handle's open mode alone.
+
+**Pattern reminder.** Defects in `internal/generator/templates/mcp_tools.go.tmpl` consistently fire on the MCP surface only and are invisible to CLI smoke checks. This is the second class-level defect found in this template (see Related Issues). Treat any change to MCP-surface defaults — connection mode, cache settings, tool annotations, query gates — as a candidate for surface-specific divergence and pin coverage at the generator level, not just at the printed-CLI level.
+
+## Related Issues
+
+- GitHub: [mvanhorn/cli-printing-press#693](https://github.com/mvanhorn/cli-printing-press/issues/693) — the original community report this fix addresses.
+- `docs/solutions/logic-errors/mcp-handler-conflates-path-and-query-positional-params-2026-05-05.md` — prior class-level defect in the same `mcp_tools.go.tmpl` file. Same failure shape: MCP surface silently wrong while CLI surface passes smoke checks. Same fix shape: generator-template correction plus published-CLI backfill. The two defects share template surface and failure pattern but are distinct security events.
+- `docs/solutions/design-patterns/http-client-cache-invalidate-on-mutation-2026-05-05.md` — parallel pattern at the HTTP-cache layer. Same theme: MCP-surface defaults must be stricter than CLI-surface defaults, and the generator constructor is the right place to enforce them. The earlier fix tightened freshness; this one tightens write privilege.
+- `docs/solutions/security-issues/filepath-join-traversal-with-user-input-2026-03-29.md` — prior allowlist-over-blocklist pattern. Same lesson: when a deny-list grows in response to bypasses, switch to an allow-list grounded in what the surface should accept.
+- `docs/solutions/best-practices/cross-repo-coordination-with-printing-press-library-2026-05-06.md` — applies to the cross-repo regen step needed to backfill all 49+ published CLIs with the new `mcp_tools_test.go.tmpl`.
diff --git a/internal/generator/generator.go b/internal/generator/generator.go
index 94c97f11..ad883cfa 100644
--- a/internal/generator/generator.go
+++ b/internal/generator/generator.go
@@ -2331,6 +2331,11 @@ func (g *Generator) renderMCPToolFiles(schema []TableDef) error {
if err := g.renderTemplate("mcp_tools.go.tmpl", filepath.Join("internal", "mcp", "tools.go"), mcpData); err != nil {
return fmt.Errorf("rendering MCP tools: %w", err)
}
+ if g.VisionSet.Store {
+ if err := g.renderTemplate("mcp_tools_test.go.tmpl", filepath.Join("internal", "mcp", "tools_test.go"), mcpData); err != nil {
+ return fmt.Errorf("rendering MCP tools tests: %w", err)
+ }
+ }
if len(g.Spec.MCP.Intents) > 0 {
if err := g.renderTemplate("mcp_intents.go.tmpl", filepath.Join("internal", "mcp", "intents.go"), mcpData); err != nil {
return fmt.Errorf("rendering MCP intents: %w", err)
diff --git a/internal/generator/generator_test.go b/internal/generator/generator_test.go
index 2b469101..c57eae61 100644
--- a/internal/generator/generator_test.go
+++ b/internal/generator/generator_test.go
@@ -75,9 +75,9 @@ func TestGenerateProjectsCompile(t *testing.T) {
// Bump it AND add to mustInclude above when adding always-emitted
// templates. Per-spec dynamic files (per-resource command files,
// generated tests) account for the difference between fixtures.
- {name: "stytch", specPath: filepath.Join("..", "..", "testdata", "stytch.yaml"), expectedFiles: 53},
- {name: "clerk", specPath: filepath.Join("..", "..", "testdata", "clerk.yaml"), expectedFiles: 58},
- {name: "loops", specPath: filepath.Join("..", "..", "testdata", "loops.yaml"), expectedFiles: 55},
+ {name: "stytch", specPath: filepath.Join("..", "..", "testdata", "stytch.yaml"), expectedFiles: 54},
+ {name: "clerk", specPath: filepath.Join("..", "..", "testdata", "clerk.yaml"), expectedFiles: 59},
+ {name: "loops", specPath: filepath.Join("..", "..", "testdata", "loops.yaml"), expectedFiles: 56},
}
for _, tt := range tests {
@@ -1908,6 +1908,78 @@ func TestGenerateStoreMigrateUsesBeginImmediate(t *testing.T) {
"migrate must read PRAGMA user_version BEFORE entering withMigrationLock so newer-DB rejection happens before lock acquisition")
}
+// TestGenerateMCPSQLToolUsesReadOnlyStore guards the agent-native security
+// model. The MCP sql and search tools advertise readOnlyHint=true to MCP
+// hosts so the host auto-approves invocations; a false readOnlyHint on a
+// mutating tool lets prompt-injected synced data exfiltrate the local
+// database without a permission prompt. Two layers must both be present:
+// (1) handleSQL gates the query through validateReadOnlyQuery — an
+// allowlist of SELECT/WITH applied AFTER stripping leading SQL comments
+// and semicolons that SQLite skips before parsing — and (2) the store
+// handle is OpenReadOnly, whose mode=ro driver flag rejects direct and
+// CTE-wrapped writes. Re-wiring these handlers to OpenWithContext, or
+// reverting to a HasPrefix-on-blocklist gate that misses comment-prefixed
+// bypasses, silently re-opens the exfiltration vector. Behavioral
+// coverage (the stripper and allowlist functioning end-to-end on attack
+// inputs) lives in the emitted tools_test.go; this test pins the
+// machine-level emission shape so a regression fails fast.
+func TestGenerateMCPSQLToolUsesReadOnlyStore(t *testing.T) {
+ t.Parallel()
+
+ apiSpec := minimalSpec("ro-canary")
+ outputDir := filepath.Join(t.TempDir(), naming.CLI(apiSpec.Name))
+ gen := New(apiSpec, outputDir)
+ gen.VisionSet = VisionTemplateSet{Store: true, Search: true, MCP: true}
+ require.NoError(t, gen.Generate())
+
+ storeSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "store", "store.go"))
+ require.NoError(t, err)
+ storeCode := stripGoComments(string(storeSrc))
+ assert.Contains(t, storeCode, `func OpenReadOnly(`,
+ "store package must expose OpenReadOnly so MCP read tools open a write-rejecting connection")
+ // modernc.org/sqlite only honors SQLite URI query parameters when
+ // the DSN starts with "file:". Without the prefix, ?mode=ro is
+ // silently dropped and writes succeed against the supposedly
+ // read-only handle.
+ assert.Contains(t, storeCode, `"file:"+dbPath+"?mode=ro`,
+ "OpenReadOnly DSN must use the file: URI prefix with mode=ro")
+
+ mcpSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "mcp", "tools.go"))
+ require.NoError(t, err)
+ mcpCode := stripGoComments(string(mcpSrc))
+
+ assert.NotRegexp(t, `(?s)func handleSQL\(.*store\.OpenWithContext\(`, mcpCode,
+ "handleSQL must use store.OpenReadOnly, not OpenWithContext")
+ assert.NotRegexp(t, `(?s)func handleSearch\(.*store\.OpenWithContext\(`, mcpCode,
+ "handleSearch must use store.OpenReadOnly, not OpenWithContext")
+ assert.Regexp(t, `(?s)func handleSQL\(.*store\.OpenReadOnly\(`, mcpCode,
+ "handleSQL must open the store via OpenReadOnly")
+ assert.Regexp(t, `(?s)func handleSearch\(.*store\.OpenReadOnly\(`, mcpCode,
+ "handleSearch must open the store via OpenReadOnly")
+
+ assert.Contains(t, mcpCode, `func validateReadOnlyQuery(`,
+ "mcp package must expose validateReadOnlyQuery — the gate that handleSQL must call before opening the store")
+ assert.Contains(t, mcpCode, `func stripLeadingSQLNoise(`,
+ "mcp package must expose stripLeadingSQLNoise — without it the gate's HasPrefix check is bypassable by SQL comments and semicolons that SQLite skips before parsing")
+ // handleSQL must dispatch through validateReadOnlyQuery; a regression
+ // to inline HasPrefix-on-blocklist would silently restore the
+ // comment-prefix bypass.
+ assert.Regexp(t, `(?s)func handleSQL\(.*validateReadOnlyQuery\(`, mcpCode,
+ "handleSQL must call validateReadOnlyQuery before opening the store")
+ // Allowlist: SELECT and WITH only. WITH supports SELECT-form CTEs;
+ // CTE-wrapped writes are caught one layer down by mode=ro.
+ assert.Contains(t, mcpCode, `"SELECT"`,
+ "validateReadOnlyQuery allowlist must include SELECT")
+ assert.Contains(t, mcpCode, `"WITH"`,
+ "validateReadOnlyQuery allowlist must include WITH for SELECT-form CTEs")
+
+ mcpTestSrc, err := os.ReadFile(filepath.Join(outputDir, "internal", "mcp", "tools_test.go"))
+ require.NoError(t, err)
+ mcpTestCode := string(mcpTestSrc)
+ assert.Contains(t, mcpTestCode, "TestValidateReadOnlyQuery_RejectsBypassVectors",
+ "behavioral coverage of comment-prefix and statement-separator bypass vectors must ship into every printed CLI's mcp package")
+}
+
// stripGoComments removes // line comments and /* ... */ block comments from
// Go source. Crude but sufficient for canary assertions on emitted templates;
// it doesn't try to parse string literals (none of the asserted substrings
diff --git a/internal/generator/templates/mcp_tools.go.tmpl b/internal/generator/templates/mcp_tools.go.tmpl
index 6c6fd743..831f8f2f 100644
--- a/internal/generator/templates/mcp_tools.go.tmpl
+++ b/internal/generator/templates/mcp_tools.go.tmpl
@@ -151,7 +151,7 @@ func RegisterTools(s *server.MCPServer) {
s.AddTool(
mcplib.NewTool("sql",
mcplib.WithDescription("Run read-only SQL against local database. Use for ad-hoc analysis, aggregations, and joins across synced resources. Requires sync first."),
- mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT only). Tables match resource names.")),
+ mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT or WITH...SELECT). Tables match resource names.")),
mcplib.WithReadOnlyHintAnnotation(true),
mcplib.WithDestructiveHintAnnotation(false),
),
@@ -392,7 +392,7 @@ func handleSearch(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.Call
limit = int(v)
}
- db, err := store.OpenWithContext(ctx, dbPath())
+ db, err := store.OpenReadOnly(dbPath())
if err != nil {
return mcplib.NewToolResultError(fmt.Sprintf("opening database: %v", err)), nil
}
@@ -410,6 +410,60 @@ func handleSearch(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.Call
{{- if .VisionSet.Store}}
+// validateReadOnlyQuery gates the MCP sql tool. The agent contract advertised
+// to the host is ReadOnlyHintAnnotation(true); a false annotation on a
+// mutating tool lets MCP hosts auto-approve writes and is treated as a real
+// bug per the project's agent-native security model.
+//
+// The gate is an allowlist (SELECT or WITH only) applied AFTER stripping the
+// leading whitespace, line comments, block comments, and semicolons that
+// SQLite itself ignores before parsing. A naive HasPrefix check on a
+// keyword blocklist is bypassable by prefixing the dangerous statement with
+// "/* x */" or "-- x\n" — TrimSpace strips outer whitespace but does not
+// understand SQL comment syntax. Combined with the empirical fact that
+// modernc.org/sqlite's mode=ro does NOT block VACUUM INTO (writes a snapshot
+// to a new file) or ATTACH DATABASE (opens a separate writable handle),
+// such a bypass produces silent exfiltration to an attacker-chosen path.
+//
+// SELECT and WITH are the only allowed leading keywords. WITH supports
+// SELECT-form CTEs; CTE-wrapped writes ("WITH x AS (...) INSERT ...") are
+// caught by OpenReadOnly's mode=ro one layer down. PRAGMA, ATTACH, VACUUM,
+// and every other DDL/DML keyword fail at this gate before reaching SQLite.
+func validateReadOnlyQuery(query string) error {
+ upper := strings.ToUpper(stripLeadingSQLNoise(query))
+ if !strings.HasPrefix(upper, "SELECT") && !strings.HasPrefix(upper, "WITH") {
+ return fmt.Errorf("only SELECT queries are allowed")
+ }
+ return nil
+}
+
+// stripLeadingSQLNoise removes leading whitespace, SQL line comments
+// (-- to end of line), block comments (/* ... */), and statement
+// separators (;) from query. SQLite skips these before parsing the first
+// keyword, so a security gate that does not strip them mismatches what the
+// driver actually executes.
+func stripLeadingSQLNoise(query string) string {
+ for {
+ query = strings.TrimLeft(query, " \t\r\n;")
+ switch {
+ case strings.HasPrefix(query, "--"):
+ if idx := strings.IndexByte(query, '\n'); idx >= 0 {
+ query = query[idx+1:]
+ continue
+ }
+ return ""
+ case strings.HasPrefix(query, "/*"):
+ if idx := strings.Index(query[2:], "*/"); idx >= 0 {
+ query = query[2+idx+2:]
+ continue
+ }
+ return ""
+ default:
+ return query
+ }
+ }
+}
+
func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToolResult, error) {
args := req.GetArguments()
query, ok := args["query"].(string)
@@ -417,15 +471,11 @@ func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToo
return mcplib.NewToolResultError("query is required"), nil
}
- // Block write operations
- upper := strings.ToUpper(strings.TrimSpace(query))
- for _, prefix := range []string{"INSERT", "UPDATE", "DELETE", "DROP", "ALTER", "CREATE"} {
- if strings.HasPrefix(upper, prefix) {
- return mcplib.NewToolResultError("only SELECT queries are allowed"), nil
- }
+ if err := validateReadOnlyQuery(query); err != nil {
+ return mcplib.NewToolResultError(err.Error()), nil
}
- db, err := store.OpenWithContext(ctx, dbPath())
+ db, err := store.OpenReadOnly(dbPath())
if err != nil {
return mcplib.NewToolResultError(fmt.Sprintf("opening database: %v", err)), nil
}
diff --git a/internal/generator/templates/mcp_tools_test.go.tmpl b/internal/generator/templates/mcp_tools_test.go.tmpl
new file mode 100644
index 00000000..dfdd857c
--- /dev/null
+++ b/internal/generator/templates/mcp_tools_test.go.tmpl
@@ -0,0 +1,110 @@
+// Copyright {{currentYear}} {{.Owner}}. Licensed under Apache-2.0. See LICENSE.
+// Generated by CLI Printing Press (https://github.com/mvanhorn/cli-printing-press). DO NOT EDIT.
+
+package mcp
+
+import (
+ "strings"
+ "testing"
+)
+
+// TestValidateReadOnlyQuery_AllowsSelectAndWITH pins the contract: the MCP
+// sql tool's allowlist accepts SELECT and WITH-prefix queries, including
+// CTEs, mixed case, leading whitespace, leading SQL comments, and leading
+// statement separators. SELECT-form CTEs ("WITH x AS (SELECT ...) SELECT")
+// must work because novel CLI sql commands in the public library accept
+// them as legitimate read-only queries; the MCP surface keeps parity.
+func TestValidateReadOnlyQuery_AllowsSelectAndWITH(t *testing.T) {
+ allowed := []string{
+ "SELECT 1",
+ "select * from resources",
+ " SELECT 1",
+ "\tSELECT 1",
+ "\nSELECT 1",
+ ";SELECT 1",
+ "-- comment\nSELECT 1",
+ "/* comment */ SELECT 1",
+ "/* comment */SELECT 1",
+ "/**/SELECT 1",
+ "-- one\n-- two\nSELECT 1",
+ "/* a *//* b */ SELECT 1",
+ "WITH r AS (SELECT 1) SELECT * FROM r",
+ "with r as (select 1) select * from r",
+ }
+ for _, q := range allowed {
+ if err := validateReadOnlyQuery(q); err != nil {
+ t.Errorf("validateReadOnlyQuery(%q) = %v, want nil", q, err)
+ }
+ }
+}
+
+// TestValidateReadOnlyQuery_RejectsBypassVectors covers the comment-prefix
+// bypass class that defeated the earlier prefix-blocklist gate. mode=ro on
+// modernc.org/sqlite does not block VACUUM INTO (writes a fresh file) or
+// ATTACH DATABASE (opens a separate writable handle), so the gate is the
+// only defense against those vectors. A successful bypass at this layer
+// would let an MCP-trusting agent silently exfiltrate the local database.
+func TestValidateReadOnlyQuery_RejectsBypassVectors(t *testing.T) {
+ rejected := []string{
+ "VACUUM INTO '/tmp/x.db'",
+ "ATTACH DATABASE 'file:/tmp/x.db?mode=rwc' AS evil",
+ "INSERT INTO resources VALUES ('x', 'y', '{}')",
+ "UPDATE resources SET resource_type = 'evil'",
+ "DELETE FROM resources",
+ "REPLACE INTO resources VALUES ('seed', 'evil', '{}')",
+ "DROP TABLE resources",
+ "PRAGMA writable_schema = ON",
+ "REINDEX",
+ "DETACH DATABASE x",
+ "/* x */ VACUUM INTO '/tmp/exfil.db'",
+ "/* x */VACUUM INTO '/tmp/exfil.db'",
+ "-- x\nVACUUM INTO '/tmp/exfil.db'",
+ "/**/VACUUM INTO '/tmp/exfil.db'",
+ "/* x */ ATTACH DATABASE 'file:/tmp/x.db?mode=rwc' AS evil",
+ "-- x\nATTACH DATABASE '/tmp/x.db' AS evil",
+ ";VACUUM INTO '/tmp/x.db'",
+ "; ; VACUUM INTO '/tmp/x.db'",
+ "/* a */ /* b */ INSERT INTO t VALUES (1)",
+ "/* outer /* not nested */ */ SELECT 1", // SQLite doesn't nest, so trailing "*/" closes; second SELECT remains. Reject — the gate must err on the side of caution when the leading shape is suspicious.
+ "-- only a comment",
+ "/* only a comment */",
+ "",
+ " ",
+ ";",
+ }
+ for _, q := range rejected {
+ if err := validateReadOnlyQuery(q); err == nil {
+ t.Errorf("validateReadOnlyQuery(%q) = nil, want error", q)
+ }
+ }
+}
+
+// TestStripLeadingSQLNoise checks the helper directly so a regression in the
+// stripping logic (off-by-one on /* */ length, missing newline handling on
+// --) surfaces close to the source rather than only via the integration
+// behavior of validateReadOnlyQuery.
+func TestStripLeadingSQLNoise(t *testing.T) {
+ cases := []struct {
+ in, want string
+ }{
+ {"SELECT 1", "SELECT 1"},
+ {" SELECT 1", "SELECT 1"},
+ {"\t\nSELECT 1", "SELECT 1"},
+ {";SELECT 1", "SELECT 1"},
+ {";; ;SELECT 1", "SELECT 1"},
+ {"-- x\nSELECT 1", "SELECT 1"},
+ {"-- x\n-- y\nSELECT 1", "SELECT 1"},
+ {"/* x */SELECT 1", "SELECT 1"},
+ {"/**/SELECT 1", "SELECT 1"},
+ {"/* x */ /* y */ SELECT 1", "SELECT 1"},
+ {"-- only", ""},
+ {"/* only", ""},
+ {"", ""},
+ }
+ for _, c := range cases {
+ got := stripLeadingSQLNoise(c.in)
+ if !strings.EqualFold(got, c.want) {
+ t.Errorf("stripLeadingSQLNoise(%q) = %q, want %q", c.in, got, c.want)
+ }
+ }
+}
diff --git a/internal/generator/templates/store.go.tmpl b/internal/generator/templates/store.go.tmpl
index 60c6e4ce..b4e96594 100644
--- a/internal/generator/templates/store.go.tmpl
+++ b/internal/generator/templates/store.go.tmpl
@@ -54,6 +54,26 @@ func Open(dbPath string) (*Store, error) {
return OpenWithContext(context.Background(), dbPath)
}
+// OpenReadOnly opens an existing SQLite store at dbPath in read-only mode.
+// mode=ro rejects direct and CTE-wrapped writes (INSERT, UPDATE, DELETE,
+// REPLACE, "WITH x AS (...) INSERT ...") at the driver level. Skips
+// MkdirAll and migrate; the file is expected to exist.
+//
+// The file: URI prefix is load-bearing: modernc.org/sqlite only honors
+// SQLite's URI query parameters (mode, cache, etc.) when the DSN starts
+// with "file:". Without the prefix, "?mode=ro" is silently dropped and
+// the connection opens read-write. Underscore-prefixed driver pragmas
+// (_journal_mode, _busy_timeout, etc.) work either way; they're parsed
+// out of the DSN by the driver before sqlite3_open_v2.
+func OpenReadOnly(dbPath string) (*Store, error) {
+ db, err := sql.Open("sqlite", "file:"+dbPath+"?mode=ro&_journal_mode=WAL&_busy_timeout=5000&_foreign_keys=ON&_temp_store=MEMORY&_mmap_size=268435456")
+ if err != nil {
+ return nil, fmt.Errorf("opening database (read-only): %w", err)
+ }
+ db.SetMaxOpenConns(2)
+ return &Store{db: db, path: dbPath}, nil
+}
+
// OpenWithContext opens or creates the SQLite store at dbPath. The
// context is honored by the migration path: cancellation interrupts the
// retry-on-SQLITE_BUSY loop and propagates ctx.Err() back to the caller
diff --git a/internal/generator/templates/store_schema_version_test.go.tmpl b/internal/generator/templates/store_schema_version_test.go.tmpl
index 219d0ea2..9700a99b 100644
--- a/internal/generator/templates/store_schema_version_test.go.tmpl
+++ b/internal/generator/templates/store_schema_version_test.go.tmpl
@@ -253,6 +253,61 @@ func TestSchemaVersion_ReopenIsIdempotent(t *testing.T) {
t.Fatalf("reopened version = %d, want %d", v, StoreSchemaVersion)
}
}
+
+// TestOpenReadOnly_RejectsWrites pins the contract: direct and CTE-wrapped
+// writes against the main DB fail under mode=ro. Deliberately does not
+// assert VACUUM INTO and ATTACH DATABASE — modernc.org/sqlite allows both
+// under mode=ro, so those defenses live in the handleSQL keyword blocklist.
+func TestOpenReadOnly_RejectsWrites(t *testing.T) {
+ dbPath := filepath.Join(t.TempDir(), "data.db")
+
+ rw, err := Open(dbPath)
+ if err != nil {
+ t.Fatalf("seed open: %v", err)
+ }
+ if _, err := rw.DB().Exec(`INSERT INTO resources (id, resource_type, data) VALUES ('seed', 'thing', '{}')`); err != nil {
+ t.Fatalf("seed insert: %v", err)
+ }
+ rw.Close()
+
+ ro, err := OpenReadOnly(dbPath)
+ if err != nil {
+ t.Fatalf("open read-only: %v", err)
+ }
+ defer ro.Close()
+
+ writes := []struct {
+ name string
+ stmt string
+ }{
+ {"insert", `INSERT INTO resources (id, resource_type, data) VALUES ('x', 'y', '{}')`},
+ {"update", `UPDATE resources SET resource_type = 'hijacked' WHERE id = 'seed'`},
+ {"delete", `DELETE FROM resources WHERE id = 'seed'`},
+ {"replace", `REPLACE INTO resources (id, resource_type, data) VALUES ('seed', 'evil', '{}')`},
+ // CTE-wrapped INSERT is load-bearing: it justifies leaving WITH
+ // out of the handleSQL blocklist so SELECT-form CTEs work.
+ {"cte_insert", `WITH stale AS (SELECT id FROM resources) INSERT INTO resources (id, resource_type, data) SELECT id || '-evil', 'thing', '{}' FROM stale`},
+ }
+ for _, w := range writes {
+ if _, err := ro.DB().Exec(w.stmt); err == nil {
+ t.Errorf("%s succeeded under mode=ro; expected rejection. stmt=%q", w.name, w.stmt)
+ }
+ }
+
+ var count int
+ if err := ro.DB().QueryRow(`SELECT COUNT(*) FROM resources`).Scan(&count); err != nil {
+ t.Fatalf("read-only SELECT failed: %v", err)
+ }
+ if count != 1 {
+ t.Fatalf("SELECT returned %d rows, want 1 (only the seed should remain)", count)
+ }
+ if err := ro.DB().QueryRow(`WITH r AS (SELECT id FROM resources WHERE id = 'seed') SELECT COUNT(*) FROM r`).Scan(&count); err != nil {
+ t.Fatalf("read-only WITH...SELECT CTE failed: %v", err)
+ }
+ if count != 1 {
+ t.Fatalf("CTE SELECT returned %d rows, want 1", count)
+ }
+}
{{- range $table := .Tables}}
{{- if hasBackfillColumns $table}}
diff --git a/testdata/golden/expected/generate-golden-api-rich-auth/printing-press-rich-auth/internal/mcp/tools.go b/testdata/golden/expected/generate-golden-api-rich-auth/printing-press-rich-auth/internal/mcp/tools.go
index 1b03aa82..3ff3fb22 100644
--- a/testdata/golden/expected/generate-golden-api-rich-auth/printing-press-rich-auth/internal/mcp/tools.go
+++ b/testdata/golden/expected/generate-golden-api-rich-auth/printing-press-rich-auth/internal/mcp/tools.go
@@ -37,7 +37,7 @@ func RegisterTools(s *server.MCPServer) {
s.AddTool(
mcplib.NewTool("sql",
mcplib.WithDescription("Run read-only SQL against local database. Use for ad-hoc analysis, aggregations, and joins across synced resources. Requires sync first."),
- mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT only). Tables match resource names.")),
+ mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT or WITH...SELECT). Tables match resource names.")),
mcplib.WithReadOnlyHintAnnotation(true),
mcplib.WithDestructiveHintAnnotation(false),
),
@@ -221,6 +221,60 @@ func dbPath() string {
// Note: MCP tools use their own dbPath() because they are in a separate package (main, not cli).
// The CLI's defaultDBPath() in the cli package uses the same canonical path.
+// validateReadOnlyQuery gates the MCP sql tool. The agent contract advertised
+// to the host is ReadOnlyHintAnnotation(true); a false annotation on a
+// mutating tool lets MCP hosts auto-approve writes and is treated as a real
+// bug per the project's agent-native security model.
+//
+// The gate is an allowlist (SELECT or WITH only) applied AFTER stripping the
+// leading whitespace, line comments, block comments, and semicolons that
+// SQLite itself ignores before parsing. A naive HasPrefix check on a
+// keyword blocklist is bypassable by prefixing the dangerous statement with
+// "/* x */" or "-- x\n" — TrimSpace strips outer whitespace but does not
+// understand SQL comment syntax. Combined with the empirical fact that
+// modernc.org/sqlite's mode=ro does NOT block VACUUM INTO (writes a snapshot
+// to a new file) or ATTACH DATABASE (opens a separate writable handle),
+// such a bypass produces silent exfiltration to an attacker-chosen path.
+//
+// SELECT and WITH are the only allowed leading keywords. WITH supports
+// SELECT-form CTEs; CTE-wrapped writes ("WITH x AS (...) INSERT ...") are
+// caught by OpenReadOnly's mode=ro one layer down. PRAGMA, ATTACH, VACUUM,
+// and every other DDL/DML keyword fail at this gate before reaching SQLite.
+func validateReadOnlyQuery(query string) error {
+ upper := strings.ToUpper(stripLeadingSQLNoise(query))
+ if !strings.HasPrefix(upper, "SELECT") && !strings.HasPrefix(upper, "WITH") {
+ return fmt.Errorf("only SELECT queries are allowed")
+ }
+ return nil
+}
+
+// stripLeadingSQLNoise removes leading whitespace, SQL line comments
+// (-- to end of line), block comments (/* ... */), and statement
+// separators (;) from query. SQLite skips these before parsing the first
+// keyword, so a security gate that does not strip them mismatches what the
+// driver actually executes.
+func stripLeadingSQLNoise(query string) string {
+ for {
+ query = strings.TrimLeft(query, " \t\r\n;")
+ switch {
+ case strings.HasPrefix(query, "--"):
+ if idx := strings.IndexByte(query, '\n'); idx >= 0 {
+ query = query[idx+1:]
+ continue
+ }
+ return ""
+ case strings.HasPrefix(query, "/*"):
+ if idx := strings.Index(query[2:], "*/"); idx >= 0 {
+ query = query[2+idx+2:]
+ continue
+ }
+ return ""
+ default:
+ return query
+ }
+ }
+}
+
func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToolResult, error) {
args := req.GetArguments()
query, ok := args["query"].(string)
@@ -228,15 +282,11 @@ func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToo
return mcplib.NewToolResultError("query is required"), nil
}
- // Block write operations
- upper := strings.ToUpper(strings.TrimSpace(query))
- for _, prefix := range []string{"INSERT", "UPDATE", "DELETE", "DROP", "ALTER", "CREATE"} {
- if strings.HasPrefix(upper, prefix) {
- return mcplib.NewToolResultError("only SELECT queries are allowed"), nil
- }
+ if err := validateReadOnlyQuery(query); err != nil {
+ return mcplib.NewToolResultError(err.Error()), nil
}
- db, err := store.OpenWithContext(ctx, dbPath())
+ db, err := store.OpenReadOnly(dbPath())
if err != nil {
return mcplib.NewToolResultError(fmt.Sprintf("opening database: %v", err)), nil
}
diff --git a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.go b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.go
index 41eed8dc..ed9200fe 100644
--- a/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.go
+++ b/testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.go
@@ -125,7 +125,7 @@ func RegisterTools(s *server.MCPServer) {
s.AddTool(
mcplib.NewTool("sql",
mcplib.WithDescription("Run read-only SQL against local database. Use for ad-hoc analysis, aggregations, and joins across synced resources. Requires sync first."),
- mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT only). Tables match resource names.")),
+ mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT or WITH...SELECT). Tables match resource names.")),
mcplib.WithReadOnlyHintAnnotation(true),
mcplib.WithDestructiveHintAnnotation(false),
),
@@ -321,7 +321,7 @@ func handleSearch(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.Call
limit = int(v)
}
- db, err := store.OpenWithContext(ctx, dbPath())
+ db, err := store.OpenReadOnly(dbPath())
if err != nil {
return mcplib.NewToolResultError(fmt.Sprintf("opening database: %v", err)), nil
}
@@ -336,6 +336,60 @@ func handleSearch(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.Call
return mcplib.NewToolResultText(string(data)), nil
}
+// validateReadOnlyQuery gates the MCP sql tool. The agent contract advertised
+// to the host is ReadOnlyHintAnnotation(true); a false annotation on a
+// mutating tool lets MCP hosts auto-approve writes and is treated as a real
+// bug per the project's agent-native security model.
+//
+// The gate is an allowlist (SELECT or WITH only) applied AFTER stripping the
+// leading whitespace, line comments, block comments, and semicolons that
+// SQLite itself ignores before parsing. A naive HasPrefix check on a
+// keyword blocklist is bypassable by prefixing the dangerous statement with
+// "/* x */" or "-- x\n" — TrimSpace strips outer whitespace but does not
+// understand SQL comment syntax. Combined with the empirical fact that
+// modernc.org/sqlite's mode=ro does NOT block VACUUM INTO (writes a snapshot
+// to a new file) or ATTACH DATABASE (opens a separate writable handle),
+// such a bypass produces silent exfiltration to an attacker-chosen path.
+//
+// SELECT and WITH are the only allowed leading keywords. WITH supports
+// SELECT-form CTEs; CTE-wrapped writes ("WITH x AS (...) INSERT ...") are
+// caught by OpenReadOnly's mode=ro one layer down. PRAGMA, ATTACH, VACUUM,
+// and every other DDL/DML keyword fail at this gate before reaching SQLite.
+func validateReadOnlyQuery(query string) error {
+ upper := strings.ToUpper(stripLeadingSQLNoise(query))
+ if !strings.HasPrefix(upper, "SELECT") && !strings.HasPrefix(upper, "WITH") {
+ return fmt.Errorf("only SELECT queries are allowed")
+ }
+ return nil
+}
+
+// stripLeadingSQLNoise removes leading whitespace, SQL line comments
+// (-- to end of line), block comments (/* ... */), and statement
+// separators (;) from query. SQLite skips these before parsing the first
+// keyword, so a security gate that does not strip them mismatches what the
+// driver actually executes.
+func stripLeadingSQLNoise(query string) string {
+ for {
+ query = strings.TrimLeft(query, " \t\r\n;")
+ switch {
+ case strings.HasPrefix(query, "--"):
+ if idx := strings.IndexByte(query, '\n'); idx >= 0 {
+ query = query[idx+1:]
+ continue
+ }
+ return ""
+ case strings.HasPrefix(query, "/*"):
+ if idx := strings.Index(query[2:], "*/"); idx >= 0 {
+ query = query[2+idx+2:]
+ continue
+ }
+ return ""
+ default:
+ return query
+ }
+ }
+}
+
func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToolResult, error) {
args := req.GetArguments()
query, ok := args["query"].(string)
@@ -343,15 +397,11 @@ func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToo
return mcplib.NewToolResultError("query is required"), nil
}
- // Block write operations
- upper := strings.ToUpper(strings.TrimSpace(query))
- for _, prefix := range []string{"INSERT", "UPDATE", "DELETE", "DROP", "ALTER", "CREATE"} {
- if strings.HasPrefix(upper, prefix) {
- return mcplib.NewToolResultError("only SELECT queries are allowed"), nil
- }
+ if err := validateReadOnlyQuery(query); err != nil {
+ return mcplib.NewToolResultError(err.Error()), nil
}
- db, err := store.OpenWithContext(ctx, dbPath())
+ db, err := store.OpenReadOnly(dbPath())
if err != nil {
return mcplib.NewToolResultError(fmt.Sprintf("opening database: %v", err)), nil
}
diff --git a/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go b/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go
index 70382322..75b1cb8c 100644
--- a/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go
+++ b/testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go
@@ -55,7 +55,7 @@ func RegisterTools(s *server.MCPServer) {
s.AddTool(
mcplib.NewTool("sql",
mcplib.WithDescription("Run read-only SQL against local database. Use for ad-hoc analysis, aggregations, and joins across synced resources. Requires sync first."),
- mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT only). Tables match resource names.")),
+ mcplib.WithString("query", mcplib.Required(), mcplib.Description("SQL query (SELECT or WITH...SELECT). Tables match resource names.")),
mcplib.WithReadOnlyHintAnnotation(true),
mcplib.WithDestructiveHintAnnotation(false),
),
@@ -240,6 +240,60 @@ func dbPath() string {
// Note: MCP tools use their own dbPath() because they are in a separate package (main, not cli).
// The CLI's defaultDBPath() in the cli package uses the same canonical path.
+// validateReadOnlyQuery gates the MCP sql tool. The agent contract advertised
+// to the host is ReadOnlyHintAnnotation(true); a false annotation on a
+// mutating tool lets MCP hosts auto-approve writes and is treated as a real
+// bug per the project's agent-native security model.
+//
+// The gate is an allowlist (SELECT or WITH only) applied AFTER stripping the
+// leading whitespace, line comments, block comments, and semicolons that
+// SQLite itself ignores before parsing. A naive HasPrefix check on a
+// keyword blocklist is bypassable by prefixing the dangerous statement with
+// "/* x */" or "-- x\n" — TrimSpace strips outer whitespace but does not
+// understand SQL comment syntax. Combined with the empirical fact that
+// modernc.org/sqlite's mode=ro does NOT block VACUUM INTO (writes a snapshot
+// to a new file) or ATTACH DATABASE (opens a separate writable handle),
+// such a bypass produces silent exfiltration to an attacker-chosen path.
+//
+// SELECT and WITH are the only allowed leading keywords. WITH supports
+// SELECT-form CTEs; CTE-wrapped writes ("WITH x AS (...) INSERT ...") are
+// caught by OpenReadOnly's mode=ro one layer down. PRAGMA, ATTACH, VACUUM,
+// and every other DDL/DML keyword fail at this gate before reaching SQLite.
+func validateReadOnlyQuery(query string) error {
+ upper := strings.ToUpper(stripLeadingSQLNoise(query))
+ if !strings.HasPrefix(upper, "SELECT") && !strings.HasPrefix(upper, "WITH") {
+ return fmt.Errorf("only SELECT queries are allowed")
+ }
+ return nil
+}
+
+// stripLeadingSQLNoise removes leading whitespace, SQL line comments
+// (-- to end of line), block comments (/* ... */), and statement
+// separators (;) from query. SQLite skips these before parsing the first
+// keyword, so a security gate that does not strip them mismatches what the
+// driver actually executes.
+func stripLeadingSQLNoise(query string) string {
+ for {
+ query = strings.TrimLeft(query, " \t\r\n;")
+ switch {
+ case strings.HasPrefix(query, "--"):
+ if idx := strings.IndexByte(query, '\n'); idx >= 0 {
+ query = query[idx+1:]
+ continue
+ }
+ return ""
+ case strings.HasPrefix(query, "/*"):
+ if idx := strings.Index(query[2:], "*/"); idx >= 0 {
+ query = query[2+idx+2:]
+ continue
+ }
+ return ""
+ default:
+ return query
+ }
+ }
+}
+
func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToolResult, error) {
args := req.GetArguments()
query, ok := args["query"].(string)
@@ -247,15 +301,11 @@ func handleSQL(ctx context.Context, req mcplib.CallToolRequest) (*mcplib.CallToo
return mcplib.NewToolResultError("query is required"), nil
}
- // Block write operations
- upper := strings.ToUpper(strings.TrimSpace(query))
- for _, prefix := range []string{"INSERT", "UPDATE", "DELETE", "DROP", "ALTER", "CREATE"} {
- if strings.HasPrefix(upper, prefix) {
- return mcplib.NewToolResultError("only SELECT queries are allowed"), nil
- }
+ if err := validateReadOnlyQuery(query); err != nil {
+ return mcplib.NewToolResultError(err.Error()), nil
}
- db, err := store.OpenWithContext(ctx, dbPath())
+ db, err := store.OpenReadOnly(dbPath())
if err != nil {
return mcplib.NewToolResultError(fmt.Sprintf("opening database: %v", err)), nil
}
← c46cf015 fix(cli): correct named-array envelope detection and api_key
·
back to Cli Printing Press
·
fix(cli): source store columns from response schema, not que 9e039c0b →