Skip to content

fix(cli): reject multi-statement sql mcp queries#2811

Merged
mergify[bot] merged 2 commits into
mainfrom
fix/issue-2799-sql-mcp-single-statement
Jun 7, 2026
Merged

fix(cli): reject multi-statement sql mcp queries#2811
mergify[bot] merged 2 commits into
mainfrom
fix/issue-2799-sql-mcp-single-statement

Conversation

@tmchow

@tmchow tmchow commented Jun 7, 2026

Copy link
Copy Markdown
Owner

Summary

Generated MCP sql tools now reject SELECT-prefix multi-statement payloads before the read-only keyword allowlist runs. A query like SELECT 1; ATTACH DATABASE ... no longer reaches SQLite, while legitimate single-statement reads still allow trailing terminators, trailing comments, and semicolons inside quoted SQL tokens.

This keeps the readOnlyHint contract true for printed CLIs that expose SQL over the local SQLite store, including the modernc.org/sqlite cases where ATTACH DATABASE and VACUUM INTO can write outside the primary read-only handle.

Closes #2799

Verification

  • go test ./internal/generator -run '^TestGenerateMCPSQLToolUsesReadOnlyStore$'
  • scripts/verify-generator-output.sh generate-golden-api generate-mcp-api
  • go test ./...
  • scripts/golden.sh verify

@mergify

mergify Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

Your pull request matches the following merge protections and will not be merged until they are valid.

🟢 require-ready-label-and-ci

Wonderful, this rule succeeded.
  • #changes-requested-reviews-by = 0
  • #review-threads-unresolved = 0
  • check-success = build-and-test
  • check-success = generated-test
  • check-success = go-lint
  • check-success = golden
  • check-success = pr-title
  • check-success = test
  • any of:
    • label = ready-to-merge
    • all of:
      • head = release-please--branches--main
      • title ~= ^chore\(main\): release
  • any of:
    • -files ~= ^(\.github/workflows/|\.github/scripts/|scripts/|\.github/CODEOWNERS$)
    • author = tmchow
    • approved-reviews-by = mvanhorn
    • approved-reviews-by = tmchow
    • author = mvanhorn
  • any of:
    • check-success = Greptile Review
    • label = queued
    • check-neutral = Greptile Review
    • check-skipped = Greptile Review
    • head ~= ^mergify/merge-queue/
    • all of:
      • head = release-please--branches--main
      • title ~= ^chore\(main\): release

@greptile-apps

greptile-apps Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR closes a second-order bypass in the MCP SQL read-only gate: a SELECT-prefix query like SELECT 1; ATTACH DATABASE ... previously passed the allowlist check because the leading keyword was valid, even though the semicolon-separated tail contained a mutating statement. The fix adds hasTrailingSQLStatement, a byte-level scanner that respects all five SQLite quoting contexts (', ", `, [...], /* */, --), and calls it on the stripped query before the SELECT/WITH prefix check.

  • hasTrailingSQLStatement is added to the template and propagated to all four golden output files; the generator test now also asserts the generated package compiles via requireGeneratedCompiles.
  • New test vectors cover the exact attack payloads (multi-statement with ATTACH DATABASE, DROP TABLE, VACUUM INTO, and CTE form), as well as legitimate queries where semicolons appear inside string literals or identifiers.

Confidence Score: 5/5

The change is safe to merge — it tightens the existing security gate without altering any passing behavior, and all four generated golden outputs are updated consistently.

The new hasTrailingSQLStatement byte scanner is logically sound: it correctly tracks all five SQLite quoting contexts, the early-return on the first terminator-with-no-trailing-SQL is safe because stripLeadingSQLNoise (called as the trailing check) itself strips nested semicolons and noise, and the generator test now enforces both symbol presence and successful compilation of the emitted code.

No files require special attention.

Important Files Changed

Filename Overview
internal/generator/templates/mcp_tools.go.tmpl Adds hasTrailingSQLStatement with correct quote/comment-aware byte scanning; validateReadOnlyQuery now calls it before the allowlist check. Logic and early-return semantics are sound.
internal/generator/templates/mcp_tools_test.go.tmpl Adds TestHasTrailingSQLStatement with 16 cases covering all quoting types and comment forms, plus new rejection vectors in TestValidateReadOnlyQuery_RejectsBypassVectors.
internal/generator/generator_test.go Asserts hasTrailingSQLStatement and TestHasTrailingSQLStatement symbols are present in generated output, and adds requireGeneratedCompiles to catch template regressions at compile time.
testdata/golden/expected/generate-golden-api/printing-press-golden/internal/mcp/tools.go Golden file updated consistently with the template change; diff is identical to the other three golden outputs.
testdata/golden/expected/generate-golden-api-rich-auth/printing-press-rich-auth/internal/mcp/tools.go Golden file updated consistently with the template change.
testdata/golden/expected/generate-mcp-api/mcp-cloudflare/internal/mcp/tools.go Golden file updated consistently with the template change.
testdata/golden/expected/generate-tier-routing-api/tier-routing-golden/internal/mcp/tools.go Golden file updated consistently with the template change.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[handleSQL receives query] --> B[validateReadOnlyQuery]
    B --> C[stripLeadingSQLNoise\nremove leading whitespace,\ncomments, semicolons]
    C --> D{hasTrailingSQLStatement?}
    D -- true --> E[error: only a single\nSELECT or WITH statement\nis allowed]
    D -- false --> F{HasPrefix SELECT\nor WITH?}
    F -- false --> G[error: only SELECT\nqueries are allowed]
    F -- true --> H[OpenReadOnly DB\nmode=ro]
    H --> I[Execute query]
Loading

Reviews (2): Last reviewed commit: "fix(cli): clarify sql mcp validation mes..." | Re-trigger Greptile

Comment thread internal/generator/templates/mcp_tools.go.tmpl Outdated
Comment thread internal/generator/templates/mcp_tools_test.go.tmpl
@tmchow tmchow added the ready-to-merge Allow Mergify to queue and merge this PR when protections pass label Jun 7, 2026
mergify Bot added a commit that referenced this pull request Jun 7, 2026
@mergify mergify Bot added the queued PR is in the Mergify merge queue label Jun 7, 2026
mergify Bot added a commit that referenced this pull request Jun 7, 2026
mergify Bot added a commit that referenced this pull request Jun 7, 2026
mergify Bot added a commit that referenced this pull request Jun 7, 2026
mergify Bot added a commit that referenced this pull request Jun 7, 2026
@mergify mergify Bot merged commit c3d4e1e into main Jun 7, 2026
31 checks passed
@mergify mergify Bot deleted the fix/issue-2799-sql-mcp-single-statement branch June 7, 2026 20:37
@mergify

mergify Bot commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Merge Queue Status

This pull request spent 26 minutes 53 seconds in the queue, including 22 minutes 13 seconds running CI.

Required conditions to merge

@mergify mergify Bot removed the queued PR is in the Mergify merge queue label Jun 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-to-merge Allow Mergify to queue and merge this PR when protections pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Security: generated read-only sql MCP tool gate is bypassable via multi-statement payload (arbitrary file write)

1 participant