Skip to content

feat(tools): opt-in read-only mode with readOnlyHint annotations - #83

Open
strelok89 wants to merge 4 commits into
firebolt-db:mainfrom
bigabid:DO-259-read-only-mode
Open

strelok89 wants to merge 4 commits into
firebolt-db:mainfrom
bigabid:DO-259-read-only-mode

Conversation

@strelok89

@strelok89 strelok89 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Adds an opt-in --read-only flag (FIREBOLT_MCP_READ_ONLY). With it set, firebolt_query:

  • declares MCP tool annotations: readOnlyHint: true, destructiveHint: false, idempotentHint: true, openWorldHint: false. MCP clients that cache results of read-only tools (for example, claude.ai artifacts) can then replay a query instead of running it on the engine again.
  • accepts only a single SELECT or WITH statement. Anything else is rejected with a bad request error before a connection is acquired.

Without the flag, firebolt_query behaves exactly as before: no annotations, no statement check.

firebolt_connect, firebolt_connect_core, firebolt_docs_overview, firebolt_docs_llm and firebolt_docs_search never write, so they now always declare readOnlyHint: true.

Statement check

This is defence in depth and is not a security boundary. The flag is meant for service accounts whose role already cannot write. The scanner:

  • skips comments, string literals (including E'...' backslash escapes), dollar-quoted literals ($$...$$ and $tag$...$tag$) and quoted identifiers;
  • treats every $ as a possible dollar-quote opener, even directly after a keyword (LIKE$$...$$), because Firebolt unquoted identifiers cannot contain $;
  • rejects unterminated comments, string literals and dollar-quoted literals;
  • rejects a second statement after ; (trailing comments are allowed);
  • requires the first keyword to be SELECT or WITH;
  • rejects bare write/DDL keywords anywhere, which catches data-modifying CTEs such as WITH x AS (...) INSERT ....

It errs on the side of rejecting. SHOW, EXPLAIN, pipe syntax that starts with FROM, and unquoted identifiers that match a write keyword are all rejected. The README documents this.

Tests

  • Allow and reject tables for the handler in read-only mode. Rejected queries never reach the pool.
  • The reject table includes the two dollar-quote bypasses Cursor Bugbot found in review (SELECT $$ -- $$; DROP TABLE t and SELECT 'a' LIKE$$ -- $$; DROP TABLE t) and their variants. Each fails against the scanner as it was before its fix.
  • Annotations are present in read-only mode and nil by default.
  • Writes still pass through when the flag is off.
  • A tools/list round-trip over streamable HTTP confirms the annotations survive registration and serialization.

🤖 Generated with Claude Code


Note

Medium Risk
Read-only mode changes query acceptance and MCP caching semantics for firebolt_query; misconfiguration could block legitimate SQL or give clients a false sense of safety if relied on without DB-level grants.

Overview
Adds an opt-in --read-only flag (FIREBOLT_MCP_READ_ONLY) that wires through server config into firebolt_query via WithReadOnly().

When enabled, firebolt_query advertises full MCP read-only hints (readOnlyHint, destructiveHint: false, idempotentHint, openWorldHint: false) and runs a pre-connection SQL guard that allows only one SELECT or WITH statement and rejects write/DDL keywords, multi-statement batches, and conservative cases like SHOW/EXPLAIN. Default behavior is unchanged (no annotations, no validation).

Connect and docs tools (firebolt_connect, firebolt_connect_core, docs overview/LLM/search) now always set readOnlyHint: true through shared ReadOnlyAnnotations(). README documents the mode and its limits (defence in depth, not a security boundary).

Tests cover allow/reject matrices for the scanner, annotation presence, default write passthrough, and a streamable HTTP tools/list round-trip for annotation serialization.

Reviewed by Cursor Bugbot for commit a94c60b. Bugbot is set up for automated code reviews on this repo. Configure here.

…259)

Add a --read-only flag (FIREBOLT_MCP_READ_ONLY). With it set, firebolt_query
accepts only a single SELECT or WITH statement and declares readOnlyHint,
idempotentHint, destructiveHint=false and openWorldHint=false. MCP clients
that cache read-only tool results can then replay dashboard queries instead
of re-running them on the engine.

The statement check is defence in depth on top of the Firebolt role's grants:
it skips comments, literals and quoted identifiers, rejects a second
statement, a non-SELECT/WITH lead, and bare write/DDL keywords (catching
data-modifying CTEs).

firebolt_connect, firebolt_connect_core, firebolt_docs_overview,
firebolt_docs_llm and firebolt_docs_search never write, so they declare
readOnlyHint unconditionally. Without the flag firebolt_query is unchanged.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale Bugbot comment from a previous run.

Comment thread pkg/tools/tool_query/readonly.go
scanWords did not recognise $$...$$ or $tag$...$tag$ literals, so a `--`,
`/*` or quote inside one was read as a real comment or quote. A statement
such as `SELECT $$ -- $$; DROP TABLE t` hid the second statement from the
check and passed in read-only mode. Skip dollar-quoted literals like other
strings and reject unterminated ones.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 6886a78. Configure here.

Comment thread pkg/tools/tool_query/readonly.go
Firebolt unquoted identifiers cannot contain `$`, so `LIKE$$ -- $$` is a
keyword followed by a literal. The scanner treated `$` as an identifier
character, read `LIKE$$` as one word, and then took the `--` inside the
literal as a comment, hiding a following write.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kop

kop commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

Thanks for this contribution!

We've reviewed this PR, but we're not sure yet whether we should merge it into main. The read-only check runs on the client, so its behaviour has to stay in sync with the driver and the server. Keeping them in sync is hard, and if they drift the option becomes misleading. We don't want users to enable it assuming it gives them a truly read-only mode.

We need some time to decide on the right approach here, and we'll follow up on this PR once we have.

@strelok89

strelok89 commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

Thanks for this contribution!

We've reviewed this PR, but we're not sure yet whether we should merge it into main. The read-only check runs on the client, so its behaviour has to stay in sync with the driver and the server. Keeping them in sync is hard, and if they drift the option becomes misleading. We don't want users to enable it assuming it gives them a truly read-only mode.

We need some time to decide on the right approach here, and we'll follow up on this PR once we have.

@kop we are running it on companywide mcp gateway(obot). and publishing it as "connector" in claude.ai admin panel on organization wide. that is the use case. we are not running this per laptop

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants