From 3040e8caeda175ca6b29953cb222aa28772f5dbc Mon Sep 17 00:00:00 2001 From: strelok89 Date: Mon, 19 Oct 2026 15:11:03 +0300 Subject: [PATCH 1/3] feat(tools): opt-in read-only mode with readOnlyHint annotations (DO-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 --- README.md | 18 +++ cmd/firebolt-mcp-server/main.go | 9 ++ cmd/firebolt-mcp-server/prepare_tools.go | 9 +- pkg/server/config.go | 1 + pkg/server/server_test.go | 22 +++ pkg/tools/tool_connect/connect.go | 1 + pkg/tools/tool_connect_core/connect_core.go | 1 + pkg/tools/tool_docs/docs.go | 1 + pkg/tools/tool_docs_llm/docs_llm.go | 1 + pkg/tools/tool_query/query.go | 40 ++++- pkg/tools/tool_query/readonly.go | 131 +++++++++++++++++ pkg/tools/tool_query/readonly_test.go | 155 ++++++++++++++++++++ pkg/tools/tool_search/search.go | 3 + pkg/tools/tools.go | 10 ++ 14 files changed, 396 insertions(+), 6 deletions(-) create mode 100644 pkg/tools/tool_query/readonly.go create mode 100644 pkg/tools/tool_query/readonly_test.go diff --git a/README.md b/README.md index fd8fd71..c29c60c 100644 --- a/README.md +++ b/README.md @@ -271,6 +271,24 @@ While this provides a good starting point for LLMs ensuring it has full context To disable this requirement, set the `--skip-docs-proof` CLI bool flag (or the `FIREBOLT_MCP_SKIP_DOCS_PROOF` environment variable) to `false`. +#### Read-only mode + +Set the `--read-only` CLI bool flag (or the `FIREBOLT_MCP_READ_ONLY` environment variable) to restrict `firebolt_query` +to a single `SELECT` or `WITH` statement and declare it read-only in its MCP tool annotations +(`readOnlyHint: true`, `destructiveHint: false`, `idempotentHint: true`, `openWorldHint: false`). +Clients that cache results of read-only tools can then replay them instead of re-running the query. + +The statement check skips comments, string literals and quoted identifiers. It rejects a second statement after `;`, +any statement that does not start with `SELECT` or `WITH`, and any bare write or DDL keyword (`INSERT`, `UPDATE`, `DELETE`, `MERGE`, +`TRUNCATE`, `COPY`, `CREATE`, `ALTER`, `DROP`, `UNDROP`, `VACUUM`, `GRANT`, `REVOKE`, `CALL`). +It is conservative: `SHOW`, `EXPLAIN` and pipe syntax starting with `FROM` are rejected, and so is an unquoted identifier that matches one of these keywords. + +The check is defence in depth and is not a security boundary. Enable read-only mode only when the service account's +Firebolt role has no write or DDL privileges. Clients trust the annotation and may cache and replay results. + +The other tools (`firebolt_connect`, `firebolt_connect_core`, `firebolt_docs_overview`, `firebolt_docs_llm`, `firebolt_docs_search`) +never write, so they always declare `readOnlyHint: true`. + ## Architecture Firebolt MCP Server implements the [Model Context Protocol](https://modelcontextprotocol.io/introduction), providing: diff --git a/cmd/firebolt-mcp-server/main.go b/cmd/firebolt-mcp-server/main.go index eb90587..0a52ce8 100644 --- a/cmd/firebolt-mcp-server/main.go +++ b/cmd/firebolt-mcp-server/main.go @@ -105,6 +105,14 @@ func main() { "will not be forced to gather more starting context and become smarter, but this means more tokens consumed and slower responses.", Sources: cli.EnvVars("FIREBOLT_MCP_SKIP_DOCS_PROOF"), }, + &cli.BoolFlag{ + Name: "read-only", + Category: "MCP Tools Configuration", + Value: false, + Usage: "Accept only a single SELECT or WITH statement in firebolt_query and annotate it as read-only, " + + "so MCP clients may cache its results. Use only with a Firebolt user whose role cannot write.", + Sources: cli.EnvVars("FIREBOLT_MCP_READ_ONLY"), + }, }, Action: run, } @@ -129,6 +137,7 @@ func run(ctx context.Context, cmd *cli.Command) error { Environment: cmd.String("environment"), CoreURL: cmd.String("core-url"), SkipDocsProof: cmd.Bool("skip-docs-proof"), + ReadOnly: cmd.Bool("read-only"), } if err := cfg.Validate(); err != nil { diff --git a/cmd/firebolt-mcp-server/prepare_tools.go b/cmd/firebolt-mcp-server/prepare_tools.go index 916a994..d2c646c 100644 --- a/cmd/firebolt-mcp-server/prepare_tools.go +++ b/cmd/firebolt-mcp-server/prepare_tools.go @@ -28,6 +28,11 @@ func prepareToolsAndResourceTemplates(ctx context.Context, logger *slog.Logger, docsProof = &docsProofToken } + var queryOpts []tool_query.Option + if cfg.ReadOnly { + queryOpts = append(queryOpts, tool_query.WithReadOnly()) + } + // resources shared between Firebolt and Firebolt Core resourceDocs := resources.NewDocs(fireboltdocs.FS, docsProofToken) @@ -42,7 +47,7 @@ func prepareToolsAndResourceTemplates(ctx context.Context, logger *slog.Logger, tool_connect_core.NewConnectCore(resourceCoreAccounts, resourceDatabases, resourceEngines, docsProof, cfg.DisableResources), tool_docs.NewDocs(resourceDocs, cfg.DisableResources, true), tool_docs_llm.NewDocsLLM(resourceLLMDocs, cfg.DisableResources), - tool_query.NewQuery(dbPool), + tool_query.NewQuery(dbPool, queryOpts...), } resourceTemplates := []server.ResourceTemplate{ resourceDocs, @@ -86,7 +91,7 @@ func prepareToolsAndResourceTemplates(ctx context.Context, logger *slog.Logger, tools := []server.Tool{ tool_connect.NewConnect(resourceAccounts, resourceDatabases, resourceEngines, docsProof, cfg.DisableResources), tool_docs.NewDocs(resourceDocs, cfg.DisableResources, false), - tool_query.NewQuery(dbPool), + tool_query.NewQuery(dbPool, queryOpts...), docsSearchTool, } diff --git a/pkg/server/config.go b/pkg/server/config.go index 33f475c..11e9b78 100644 --- a/pkg/server/config.go +++ b/pkg/server/config.go @@ -17,6 +17,7 @@ type Config struct { CoreURL string SkipDocsProof bool + ReadOnly bool } func (c Config) Validate() error { diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 469265f..28d8798 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -17,8 +17,10 @@ import ( "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "github.com/firebolt-db/mcp-server/pkg/clients/database/databasemock" "github.com/firebolt-db/mcp-server/pkg/prompts" "github.com/firebolt-db/mcp-server/pkg/server" + "github.com/firebolt-db/mcp-server/pkg/tools/tool_query" ) // TestNewServer checks that NewServer registers tools, prompts, and resource templates correctly. @@ -439,6 +441,26 @@ func (f *funcTool) Register(s *mcp.Server) { }) } +// TestServeStreamableHTTP_ToolAnnotations checks that tool annotations survive +// registration and serialization, so clients see them in tools/list. +func TestServeStreamableHTTP_ToolAnnotations(t *testing.T) { + addr := freeAddr(t) + tools := []server.Tool{tool_query.NewQuery(databasemock.NewPoolMock(), tool_query.WithReadOnly())} + srv := server.NewServer(slogt.New(t), "0.0.0", "streamable-http", addr, tools, nil, nil) + cancel, serveErr := serve(t, srv) + + session := connect(t, addr, false) + res, err := session.ListTools(t.Context(), nil) + require.NoError(t, err) + require.Len(t, res.Tools, 1) + require.NotNil(t, res.Tools[0].Annotations) + assert.True(t, res.Tools[0].Annotations.ReadOnlyHint) + require.NoError(t, session.Close()) + + cancel() + require.NoError(t, <-serveErr, "Serve should shut down cleanly") +} + // mockTool is a mock implementation of the Tool interface for testing. type mockTool struct { mock.Mock diff --git a/pkg/tools/tool_connect/connect.go b/pkg/tools/tool_connect/connect.go index cdf7a21..7553692 100644 --- a/pkg/tools/tool_connect/connect.go +++ b/pkg/tools/tool_connect/connect.go @@ -81,6 +81,7 @@ func (t *Connect) Tool() *mcp.Tool { Title: "Firebolt Connect", Description: "Returns a list of Firebolt accounts, databases, and engines you have access to. " + "This information is required before using the `firebolt_query` tool.", + Annotations: tools.ReadOnlyAnnotations(), } } diff --git a/pkg/tools/tool_connect_core/connect_core.go b/pkg/tools/tool_connect_core/connect_core.go index 9ef6ac4..d4ebbe6 100644 --- a/pkg/tools/tool_connect_core/connect_core.go +++ b/pkg/tools/tool_connect_core/connect_core.go @@ -75,6 +75,7 @@ func (t *ConnectCore) Tool() *mcp.Tool { Title: "Firebolt Core Connect", Description: "Returns a list of databases you have access to in Firebolt Core. " + "This information is required before using the `firebolt_query` tool.", + Annotations: tools.ReadOnlyAnnotations(), } } diff --git a/pkg/tools/tool_docs/docs.go b/pkg/tools/tool_docs/docs.go index 033b344..2abb2a0 100644 --- a/pkg/tools/tool_docs/docs.go +++ b/pkg/tools/tool_docs/docs.go @@ -44,6 +44,7 @@ func (t *Docs) Tool() *mcp.Tool { "Use this tool when need to get general information about Firebolt or need to connect to and use Firebolt. " + "Firebolt differs significantly from other databases, so it's important to gather some initial information before providing accurate answers. " + "To search for detailed information about using Firebolt, query syntax, object types etc use `firebolt_docs_search` tool", + Annotations: tools.ReadOnlyAnnotations(), } } diff --git a/pkg/tools/tool_docs_llm/docs_llm.go b/pkg/tools/tool_docs_llm/docs_llm.go index 4baa438..d73f5e3 100644 --- a/pkg/tools/tool_docs_llm/docs_llm.go +++ b/pkg/tools/tool_docs_llm/docs_llm.go @@ -40,6 +40,7 @@ func (t *DocsLLM) Tool() *mcp.Tool { Description: "Returns Firebolt documentation articles prepared to be used by LLMs. " + "Use this tool without parameters to list index of all Firebolt documentation articles. " + "Use this tool with an article URL (provide a full url as stated in the index article) to retrieve a specific documentation article.", + Annotations: tools.ReadOnlyAnnotations(), } } diff --git a/pkg/tools/tool_query/query.go b/pkg/tools/tool_query/query.go index 985f0af..6a699d1 100644 --- a/pkg/tools/tool_query/query.go +++ b/pkg/tools/tool_query/query.go @@ -8,6 +8,7 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/firebolt-db/mcp-server/pkg/clients/database" + "github.com/firebolt-db/mcp-server/pkg/tools" ) type Input struct { @@ -25,16 +26,32 @@ type Output struct { // It handles connecting to the appropriate account, database, and engine before // executing the query and returning results. type Query struct { - dbPool database.Pool + dbPool database.Pool + readOnly bool +} + +// Option configures the Query tool. +type Option func(*Query) + +// WithReadOnly restricts the tool to a single SELECT or WITH statement and +// declares it read-only in its MCP annotations, so clients may cache its results. +func WithReadOnly() Option { + return func(q *Query) { + q.readOnly = true + } } // NewQuery creates a new instance of the Query tool with the provided database connection pool. // The connection pool is responsible for managing connections to different Firebolt // account/database/engine combinations. -func NewQuery(dbPool database.Pool) *Query { - return &Query{ +func NewQuery(dbPool database.Pool, opts ...Option) *Query { + q := &Query{ dbPool: dbPool, } + for _, opt := range opts { + opt(q) + } + return q } // Tool returns the mcp.Tool definition for the Query tool. @@ -42,11 +59,21 @@ func NewQuery(dbPool database.Pool) *Query { // - The tool's name and description // - Required and optional parameters with their descriptions func (t *Query) Tool() *mcp.Tool { - return &mcp.Tool{ + tool := &mcp.Tool{ Name: "firebolt_query", Title: "Firebolt Query", Description: "Execute an SQL query against Firebolt.", } + if t.readOnly { + tool.Description += " Read-only: only a single SELECT or WITH statement is accepted." + tool.Annotations = &mcp.ToolAnnotations{ + ReadOnlyHint: true, + DestructiveHint: tools.Ptr(false), + IdempotentHint: true, + OpenWorldHint: tools.Ptr(false), + } + } + return tool } // Register adds the Query tool to the provided MCP server instance. @@ -68,6 +95,11 @@ func (t *Query) Handler() mcp.ToolHandlerFor[Input, *Output] { if input.Account == "" { return nil, nil, fmt.Errorf("bad request: account parameter is required") } + if t.readOnly { + if err := validateReadOnly(input.Query); err != nil { + return nil, nil, err + } + } var ( dbName *string diff --git a/pkg/tools/tool_query/readonly.go b/pkg/tools/tool_query/readonly.go new file mode 100644 index 0000000..b811859 --- /dev/null +++ b/pkg/tools/tool_query/readonly.go @@ -0,0 +1,131 @@ +package tool_query + +import ( + "errors" + "fmt" + "strings" + "unicode" +) + +// writeKeywords are rejected anywhere in a read-only query, outside of literals and comments. +// This catches data-modifying CTEs such as `WITH x AS (...) INSERT INTO ...`. +var writeKeywords = map[string]struct{}{ + "INSERT": {}, "UPDATE": {}, "DELETE": {}, "MERGE": {}, "TRUNCATE": {}, "COPY": {}, + "CREATE": {}, "ALTER": {}, "DROP": {}, "UNDROP": {}, "VACUUM": {}, + "GRANT": {}, "REVOKE": {}, "CALL": {}, +} + +var errMultipleStatements = errors.New("bad request: read-only mode: only a single statement is allowed") + +// validateReadOnly accepts a single SELECT or WITH statement and rejects anything else. +// It is defence in depth on top of the Firebolt role's grants, not a substitute for them: +// a keyword scan can be bypassed, the role cannot. +func validateReadOnly(sql string) error { + words, err := scanWords(sql) + if err != nil { + return err + } + if len(words) == 0 { + return errors.New("bad request: read-only mode: query contains no statement") + } + + if first := words[0]; first != "SELECT" && first != "WITH" { + return fmt.Errorf("bad request: read-only mode: only SELECT or WITH statements are allowed, got %s", first) + } + for _, w := range words { + if _, ok := writeKeywords[w]; ok { + return fmt.Errorf("bad request: read-only mode: %s is not allowed", w) + } + } + + return nil +} + +// scanWords returns the upper-cased bare words of sql, skipping string literals, quoted +// identifiers and comments. It fails if anything but comments follows a `;`. +func scanWords(sql string) ([]string, error) { + var ( + words []string + ended bool + r = []rune(sql) + n = len(r) + ) + + for i := 0; i < n; { + c := r[i] + switch { + case unicode.IsSpace(c): + i++ + case c == '-' && i+1 < n && r[i+1] == '-': + for i < n && r[i] != '\n' { + i++ + } + case c == '/' && i+1 < n && r[i+1] == '*': + j := i + 2 + for j+1 < n && (r[j] != '*' || r[j+1] != '/') { + j++ + } + if j+1 >= n { + return nil, errors.New("bad request: read-only mode: unterminated comment") + } + i = j + 2 + case ended: + return nil, errMultipleStatements + case c == ';': + ended = true + i++ + case c == '\'' || c == '"': + backslashEscapes := c == '\'' && isEscapeStringPrefix(r, i) + j, err := skipQuoted(r, i, backslashEscapes) + if err != nil { + return nil, err + } + i = j + case isIdentStart(c): + j := i + for j < n && isIdentPart(r[j]) { + j++ + } + words = append(words, strings.ToUpper(string(r[i:j]))) + i = j + default: + i++ + } + } + + return words, nil +} + +// skipQuoted returns the index just past the quoted run starting at r[start]. +// A doubled quote is an escaped quote; in E'...' strings a backslash also escapes. +func skipQuoted(r []rune, start int, backslashEscapes bool) (int, error) { + quote := r[start] + for i := start + 1; i < len(r); i++ { + switch { + case backslashEscapes && r[i] == '\\': + i++ + case r[i] != quote: + case i+1 < len(r) && r[i+1] == quote: + i++ + default: + return i + 1, nil + } + } + return 0, errors.New("bad request: read-only mode: unterminated quoted string") +} + +// isEscapeStringPrefix reports whether the quote at r[i] opens an E'...' escape string. +func isEscapeStringPrefix(r []rune, i int) bool { + if i == 0 || (r[i-1] != 'E' && r[i-1] != 'e') { + return false + } + return i == 1 || !isIdentPart(r[i-2]) +} + +func isIdentStart(c rune) bool { + return unicode.IsLetter(c) || c == '_' +} + +func isIdentPart(c rune) bool { + return unicode.IsLetter(c) || unicode.IsDigit(c) || c == '_' || c == '$' +} diff --git a/pkg/tools/tool_query/readonly_test.go b/pkg/tools/tool_query/readonly_test.go new file mode 100644 index 0000000..e95dcf1 --- /dev/null +++ b/pkg/tools/tool_query/readonly_test.go @@ -0,0 +1,155 @@ +package tool_query_test + +import ( + "context" + "testing" + + "github.com/modelcontextprotocol/go-sdk/mcp" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/firebolt-db/mcp-server/pkg/clients/database" + "github.com/firebolt-db/mcp-server/pkg/clients/database/databasemock" + "github.com/firebolt-db/mcp-server/pkg/tools/tool_query" +) + +func TestQuery_Tool_Annotations(t *testing.T) { + mockPool := databasemock.NewPoolMock() + + t.Run("default has no annotations", func(t *testing.T) { + tool := tool_query.NewQuery(mockPool).Tool() + assert.Nil(t, tool.Annotations) + assert.Equal(t, "Execute an SQL query against Firebolt.", tool.Description) + }) + + t.Run("read-only declares read-only hints", func(t *testing.T) { + tool := tool_query.NewQuery(mockPool, tool_query.WithReadOnly()).Tool() + require.NotNil(t, tool.Annotations) + assert.True(t, tool.Annotations.ReadOnlyHint) + assert.True(t, tool.Annotations.IdempotentHint) + require.NotNil(t, tool.Annotations.DestructiveHint) + assert.False(t, *tool.Annotations.DestructiveHint) + require.NotNil(t, tool.Annotations.OpenWorldHint) + assert.False(t, *tool.Annotations.OpenWorldHint) + assert.Contains(t, tool.Description, "Read-only") + }) +} + +func TestQuery_Handler_ReadOnly_Allows(t *testing.T) { + queries := []string{ + "SELECT 1", + "select * from t", + " \n\tSELECT 1", + "SELECT 1;", + "SELECT 1; -- trailing comment", + "SELECT 1; /* trailing */", + "-- leading comment\nSELECT 1", + "/* leading */ SELECT 1", + "WITH x AS (SELECT 1 AS a) SELECT a FROM x", + "SELECT 'drop table t; insert into t' AS s", + "SELECT 'it''s; delete' AS s", + `SELECT E'it\'s; delete' AS s`, + `SELECT "update", "delete" FROM t`, + "SELECT 1 -- DROP TABLE t", + "SELECT updated_at, created_by FROM t", + } + + for _, q := range queries { + t.Run(q, func(t *testing.T) { + var executed string + conn := databasemock.NewConnectionMock().WithQueryFunc( + func(ctx context.Context, sql string, args ...any) ([]map[string]any, error) { + executed = sql + return []map[string]any{{"ok": true}}, nil + }, + ) + pool := databasemock.NewPoolMock() + pool.RegisterConnection(database.PoolParams{AccountName: "test-account"}, conn) + + queryTool := tool_query.NewQuery(pool, tool_query.WithReadOnly()) + _, out, err := queryTool.Handler()(t.Context(), &mcp.CallToolRequest{}, tool_query.Input{ + Query: q, + Account: "test-account", + }) + + require.NoError(t, err) + require.NotNil(t, out) + assert.Equal(t, q, executed) + }) + } +} + +func TestQuery_Handler_ReadOnly_Rejects(t *testing.T) { + queries := []string{ + "INSERT INTO t VALUES (1)", + "insert into t select * from s", + "UPDATE t SET a = 1", + "DELETE FROM t", + "TRUNCATE TABLE t", + "DROP TABLE t", + "CREATE TABLE t (a INT)", + "ALTER ENGINE e SET NODES = 2", + "GRANT SELECT ON t TO r", + "REVOKE SELECT ON t FROM r", + "VACUUM t", + "COPY t FROM 's3://bucket/'", + "CALL recommend_ddl(t, (SELECT 1))", + "STOP ENGINE e", + "SHOW ENGINES", + "/* x */ DROP TABLE t", + "-- x\nDROP TABLE t", + "SELECT 1; DROP TABLE t", + "SELECT 1;DROP TABLE t", + "SELECT 1; SELECT 2", + "SELECT 1;;", + "SELECT 1; 'x'", + "WITH x AS (SELECT 1) INSERT INTO t SELECT * FROM x", + "SELECT * FROM t; -- ok\nDELETE FROM t", + `SELECT E'\'' ; DROP TABLE t; --'`, + "SELECT 'unterminated", + "SELECT /* unterminated", + "", + " ", + "-- only a comment", + } + + for _, q := range queries { + t.Run(q, func(t *testing.T) { + pool := databasemock.NewPoolMock() + queryTool := tool_query.NewQuery(pool, tool_query.WithReadOnly()) + + result, out, err := queryTool.Handler()(t.Context(), &mcp.CallToolRequest{}, tool_query.Input{ + Query: q, + Account: "test-account", + }) + + require.Error(t, err) + assert.Contains(t, err.Error(), "bad request") + assert.Nil(t, result) + assert.Nil(t, out) + assert.Equal(t, 0, pool.GetConnectionCallCount(database.PoolParams{AccountName: "test-account"}), + "a rejected query must not reach the database") + }) + } +} + +func TestQuery_Handler_Default_AllowsWrites(t *testing.T) { + var executed string + conn := databasemock.NewConnectionMock().WithQueryFunc( + func(ctx context.Context, sql string, args ...any) ([]map[string]any, error) { + executed = sql + return []map[string]any{}, nil + }, + ) + pool := databasemock.NewPoolMock() + pool.RegisterConnection(database.PoolParams{AccountName: "test-account"}, conn) + + queryTool := tool_query.NewQuery(pool) + _, _, err := queryTool.Handler()(t.Context(), &mcp.CallToolRequest{}, tool_query.Input{ + Query: "INSERT INTO t VALUES (1)", + Account: "test-account", + }) + + require.NoError(t, err) + assert.Equal(t, "INSERT INTO t VALUES (1)", executed) +} diff --git a/pkg/tools/tool_search/search.go b/pkg/tools/tool_search/search.go index 71b8014..d1c5d0f 100644 --- a/pkg/tools/tool_search/search.go +++ b/pkg/tools/tool_search/search.go @@ -10,6 +10,8 @@ import ( "github.com/modelcontextprotocol/go-sdk/mcp" "golang.org/x/oauth2" "golang.org/x/oauth2/clientcredentials" + + "github.com/firebolt-db/mcp-server/pkg/tools" ) const ( @@ -46,6 +48,7 @@ func (t *Search) Tool() *mcp.Tool { Title: "Firebolt Documentation Search", Description: "Returns search results from Firebolt knowledge base using RAG. " + "Use this tool to find answers to your questions about how to use Firebolt. ", + Annotations: tools.ReadOnlyAnnotations(), } } diff --git a/pkg/tools/tools.go b/pkg/tools/tools.go index 7d7892c..1899a1f 100644 --- a/pkg/tools/tools.go +++ b/pkg/tools/tools.go @@ -17,3 +17,13 @@ func TextOrResourceContent(disableResources bool, i *mcp.ResourceContents) mcp.C Resource: i, } } + +// ReadOnlyAnnotations marks a tool that does not modify its environment. +func ReadOnlyAnnotations() *mcp.ToolAnnotations { + return &mcp.ToolAnnotations{ReadOnlyHint: true} +} + +// Ptr returns a pointer to v, for optional annotation fields. +func Ptr[T any](v T) *T { + return &v +} From 6886a78703c5215b9609c01ae74bc3ebac71b3c5 Mon Sep 17 00:00:00 2001 From: strelok89 Date: Mon, 19 Oct 2026 15:52:22 +0300 Subject: [PATCH 2/3] fix(tools): treat dollar-quoted strings as literals in read-only check 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 --- README.md | 2 +- pkg/tools/tool_query/readonly.go | 27 +++++++++++++++++++++++++++ pkg/tools/tool_query/readonly_test.go | 9 +++++++++ 3 files changed, 37 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index c29c60c..77baa0c 100644 --- a/README.md +++ b/README.md @@ -278,7 +278,7 @@ to a single `SELECT` or `WITH` statement and declare it read-only in its MCP too (`readOnlyHint: true`, `destructiveHint: false`, `idempotentHint: true`, `openWorldHint: false`). Clients that cache results of read-only tools can then replay them instead of re-running the query. -The statement check skips comments, string literals and quoted identifiers. It rejects a second statement after `;`, +The statement check skips comments, string literals (including dollar-quoted `$$...$$` and `$tag$...$tag$`) and quoted identifiers. It rejects a second statement after `;`, any statement that does not start with `SELECT` or `WITH`, and any bare write or DDL keyword (`INSERT`, `UPDATE`, `DELETE`, `MERGE`, `TRUNCATE`, `COPY`, `CREATE`, `ALTER`, `DROP`, `UNDROP`, `VACUUM`, `GRANT`, `REVOKE`, `CALL`). It is conservative: `SHOW`, `EXPLAIN` and pipe syntax starting with `FROM` are rejected, and so is an unquoted identifier that matches one of these keywords. diff --git a/pkg/tools/tool_query/readonly.go b/pkg/tools/tool_query/readonly.go index b811859..52dd3c8 100644 --- a/pkg/tools/tool_query/readonly.go +++ b/pkg/tools/tool_query/readonly.go @@ -74,6 +74,12 @@ func scanWords(sql string) ([]string, error) { case c == ';': ended = true i++ + case c == '$' && (i == 0 || !isIdentPart(r[i-1])): + j, err := skipDollarQuoted(r, i) + if err != nil { + return nil, err + } + i = j case c == '\'' || c == '"': backslashEscapes := c == '\'' && isEscapeStringPrefix(r, i) j, err := skipQuoted(r, i, backslashEscapes) @@ -114,6 +120,27 @@ func skipQuoted(r []rune, start int, backslashEscapes bool) (int, error) { return 0, errors.New("bad request: read-only mode: unterminated quoted string") } +// skipDollarQuoted returns the index just past a $$...$$ or $tag$...$tag$ literal starting +// at r[start], or start+1 when the `$` does not open one (for example a $1 parameter). +func skipDollarQuoted(r []rune, start int) (int, error) { + j := start + 1 + if j < len(r) && isIdentStart(r[j]) { + for j < len(r) && (isIdentStart(r[j]) || unicode.IsDigit(r[j])) { + j++ + } + } + if j >= len(r) || r[j] != '$' { + return start + 1, nil + } + tag := r[start : j+1] + for k := j + 1; k+len(tag) <= len(r); k++ { + if string(r[k:k+len(tag)]) == string(tag) { + return k + len(tag), nil + } + } + return 0, errors.New("bad request: read-only mode: unterminated dollar-quoted string") +} + // isEscapeStringPrefix reports whether the quote at r[i] opens an E'...' escape string. func isEscapeStringPrefix(r []rune, i int) bool { if i == 0 || (r[i-1] != 'E' && r[i-1] != 'e') { diff --git a/pkg/tools/tool_query/readonly_test.go b/pkg/tools/tool_query/readonly_test.go index e95dcf1..647edcf 100644 --- a/pkg/tools/tool_query/readonly_test.go +++ b/pkg/tools/tool_query/readonly_test.go @@ -52,6 +52,9 @@ func TestQuery_Handler_ReadOnly_Allows(t *testing.T) { `SELECT "update", "delete" FROM t`, "SELECT 1 -- DROP TABLE t", "SELECT updated_at, created_by FROM t", + "SELECT $$drop table t; -- not a comment$$ AS s", + "SELECT $q$it's $$ nested; delete$q$ AS s", + "SELECT a$b FROM t", } for _, q := range queries { @@ -106,6 +109,12 @@ func TestQuery_Handler_ReadOnly_Rejects(t *testing.T) { "WITH x AS (SELECT 1) INSERT INTO t SELECT * FROM x", "SELECT * FROM t; -- ok\nDELETE FROM t", `SELECT E'\'' ; DROP TABLE t; --'`, + "SELECT $$ -- $$; DROP TABLE t", + "SELECT $$ /* $$; DROP TABLE t /* */", + "SELECT $$ ' $$; DELETE FROM t --'", + "WITH x AS (SELECT $$--$$) INSERT INTO t SELECT * FROM x", + "SELECT $tag$ -- $tag$; DROP TABLE t", + "SELECT $$unterminated", "SELECT 'unterminated", "SELECT /* unterminated", "", From a94c60b3c17b72c2412876eafa661acb17b03069 Mon Sep 17 00:00:00 2001 From: strelok89 Date: Wed, 21 Oct 2026 08:25:02 +0300 Subject: [PATCH 3/3] fix(tools): scan dollar quotes that follow a keyword in read-only check 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 --- pkg/tools/tool_query/readonly.go | 6 ++++-- pkg/tools/tool_query/readonly_test.go | 6 ++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/tools/tool_query/readonly.go b/pkg/tools/tool_query/readonly.go index 52dd3c8..ea6355c 100644 --- a/pkg/tools/tool_query/readonly.go +++ b/pkg/tools/tool_query/readonly.go @@ -74,7 +74,7 @@ func scanWords(sql string) ([]string, error) { case c == ';': ended = true i++ - case c == '$' && (i == 0 || !isIdentPart(r[i-1])): + case c == '$': j, err := skipDollarQuoted(r, i) if err != nil { return nil, err @@ -153,6 +153,8 @@ func isIdentStart(c rune) bool { return unicode.IsLetter(c) || c == '_' } +// isIdentPart excludes `$`: Firebolt identifiers cannot contain it, so `LIKE$$...$$` is a +// keyword followed by a literal. func isIdentPart(c rune) bool { - return unicode.IsLetter(c) || unicode.IsDigit(c) || c == '_' || c == '$' + return unicode.IsLetter(c) || unicode.IsDigit(c) || c == '_' } diff --git a/pkg/tools/tool_query/readonly_test.go b/pkg/tools/tool_query/readonly_test.go index 647edcf..503a7a0 100644 --- a/pkg/tools/tool_query/readonly_test.go +++ b/pkg/tools/tool_query/readonly_test.go @@ -55,6 +55,8 @@ func TestQuery_Handler_ReadOnly_Allows(t *testing.T) { "SELECT $$drop table t; -- not a comment$$ AS s", "SELECT $q$it's $$ nested; delete$q$ AS s", "SELECT a$b FROM t", + "SELECT * FROM t WHERE s LIKE$$a%$$", + "SELECT * FROM t WHERE a = $1", } for _, q := range queries { @@ -114,6 +116,10 @@ func TestQuery_Handler_ReadOnly_Rejects(t *testing.T) { "SELECT $$ ' $$; DELETE FROM t --'", "WITH x AS (SELECT $$--$$) INSERT INTO t SELECT * FROM x", "SELECT $tag$ -- $tag$; DROP TABLE t", + "SELECT 'a' LIKE$$ -- $$; DROP TABLE t", + "SELECT 1 WHERE x BETWEEN$$a$$AND$$ -- $$; DELETE FROM t", + "SELECT x$$ /* $$; DROP TABLE t /* */", + "SELECT 1 WHERE s LIKE$tag$ -- $tag$; DROP TABLE t", "SELECT $$unterminated", "SELECT 'unterminated", "SELECT /* unterminated",