From fa13a4be0cab3207c9af4f4c70937592a541d419 Mon Sep 17 00:00:00 2001 From: Jeffrey Aven Date: Fri, 2 Oct 2026 16:30:33 +1000 Subject: [PATCH 1/6] security-hardening - MCP mode gate classifies every statement in a payload, using the execution layer's splitter; the most privileged statement decides. - MCP validate_select_query takes exactly one statement; hierarchy identifiers pass the forbidden-character guard. - MCP HTTP transport gains an optional bearer token (server.auth_token_env_var), cross-origin protection and a warning when exposed beyond loopback without a token. - Wire extended protocol parameters are substituted token-aware, with backslash escaping and SQL NULL carried out of band. - A rejected AUTH no longer alters the shared auth context. - The wire server warns when bound beyond loopback without client CAs. Co-Authored-By: Claude Fable 5.1 --- internal/stackql/driver/driver.go | 4 +- .../stackql/mcpbackend/mcp_service_stackql.go | 39 ++++++-- internal/stackql/paramdecoder/paramdecoder.go | 12 +-- .../stackql/paramdecoder/paramdecoder_test.go | 41 +++++--- internal/stackql/planbuilder/plan_builder.go | 12 ++- internal/stackql/psqlwire/server.go | 16 ++++ internal/stackql/queryshape/queryshape.go | 94 +++++++++++-------- .../stackql/queryshape/queryshape_test.go | 36 ++++--- .../testdata/substitute_params_cases.json | 48 ++++++++++ pkg/mcp_server/README.md | 17 +++- pkg/mcp_server/config.go | 6 ++ pkg/mcp_server/policy/policy.go | 68 +++++++++++--- pkg/mcp_server/policy/policy_test.go | 10 ++ pkg/mcp_server/server.go | 54 ++++++++++- pkg/mcp_server/server_test.go | 64 +++++++++++++ pkg/mcp_server/tools_test.go | 17 ++++ .../stackql_test_tooling/StackQLInterfaces.py | 9 ++ .../stackql_test_tooling/mcp_stdio_client.py | 11 ++- .../stackql_test_tooling/psycopg_client.py | 5 + test/robot/functional/mcp.robot | 85 +++++++++++++++++ .../stackql_mocked_from_cmd_line.robot | 16 ++++ test/robot/functional/stackql_sessions.robot | 35 +++++++ 22 files changed, 593 insertions(+), 106 deletions(-) diff --git a/internal/stackql/driver/driver.go b/internal/stackql/driver/driver.go index 8d5fa2d0..5df7d722 100644 --- a/internal/stackql/driver/driver.go +++ b/internal/stackql/driver/driver.go @@ -320,11 +320,11 @@ func (dr *basicStackQLDriver) HandleExecute( } } // Decode params (handles both text and binary formats). - decodedStrings, err := dr.paramDecoder.DecodeParams(paramOIDs, paramFormats, paramValues) + decodedValues, err := dr.paramDecoder.DecodeParams(paramOIDs, paramFormats, paramValues) if err != nil { return nil, fmt.Errorf("parameter decoding error: %w", err) } - resolved := queryshape.SubstituteDecodedParams(query, decodedStrings) + resolved := queryshape.SubstituteDecodedParams(query, decodedValues) return dr.HandleSimpleQuery(ctx, resolved) } diff --git a/internal/stackql/mcpbackend/mcp_service_stackql.go b/internal/stackql/mcpbackend/mcp_service_stackql.go index 410d4c76..2afaf701 100644 --- a/internal/stackql/mcpbackend/mcp_service_stackql.go +++ b/internal/stackql/mcpbackend/mcp_service_stackql.go @@ -27,11 +27,11 @@ var ( const ( unlimitedRowLimit int = -1 - // forbiddenRegistryCharacters mirrors the CLI registry command's guard - // (see internal/stackql/cmd/registry.go). Interrogator methods that - // interpolate user-supplied registry identifiers reject these characters - // rather than substituting / escaping them, matching CLI semantics. - forbiddenRegistryCharacters string = ` ;\` + // forbiddenIdentifierCharacters extends the CLI registry command's guard + // (see internal/stackql/cmd/registry.go) to all whitespace. Interrogator + // methods that interpolate user-supplied identifiers reject these + // characters rather than substituting / escaping them. + forbiddenIdentifierCharacters string = " ;\\\t\r\n" ) // serverBuildInfo carries the runtime + build-time metadata reported by the @@ -110,6 +110,17 @@ func NewSimpleStackqlInterrogator() StackqlInterrogator { return &simpleStackqlInterrogator{} } +// checkHierarchyIdentifiers stops a second statement riding in on an +// interpolated hierarchy identifier. +func checkHierarchyIdentifiers(hI dto.HierarchyInput) error { + for _, identifier := range []string{hI.Provider, hI.Service, hI.Resource, hI.Method} { + if strings.ContainsAny(identifier, forbiddenIdentifierCharacters) { + return fmt.Errorf("forbidden characters in provider, service, resource or method") + } + } + return nil +} + func (s *simpleStackqlInterrogator) GetShowProviders(_ dto.HierarchyInput, likeStr string) (string, error) { sb := strings.Builder{} sb.WriteString("SHOW PROVIDERS") @@ -125,6 +136,9 @@ func (s *simpleStackqlInterrogator) GetShowServices(hI dto.HierarchyInput, likeS if hI.Provider == "" { return "", fmt.Errorf("provider not specified") } + if err := checkHierarchyIdentifiers(hI); err != nil { + return "", err + } sb := strings.Builder{} sb.WriteString("SHOW SERVICES IN ") sb.WriteString(hI.Provider) @@ -140,6 +154,9 @@ func (s *simpleStackqlInterrogator) GetShowResources(hI dto.HierarchyInput, like if hI.Provider == "" || hI.Service == "" { return "", fmt.Errorf("provider and / or service not specified") } + if err := checkHierarchyIdentifiers(hI); err != nil { + return "", err + } sb := strings.Builder{} sb.WriteString("SHOW RESOURCES IN ") sb.WriteString(hI.Provider) @@ -157,6 +174,9 @@ func (s *simpleStackqlInterrogator) GetShowMethods(hI dto.HierarchyInput) (strin if hI.Provider == "" || hI.Service == "" || hI.Resource == "" { return "", fmt.Errorf("provider, service and / or resource not specified") } + if err := checkHierarchyIdentifiers(hI); err != nil { + return "", err + } sb := strings.Builder{} sb.WriteString("SHOW METHODS IN ") sb.WriteString(hI.Provider) @@ -171,6 +191,9 @@ func (s *simpleStackqlInterrogator) GetDescribeMethod(hI dto.HierarchyInput) (st if hI.Provider == "" || hI.Service == "" || hI.Resource == "" || hI.Method == "" { return "", fmt.Errorf("provider, service, resource and / or method not specified") } + if err := checkHierarchyIdentifiers(hI); err != nil { + return "", err + } sb := strings.Builder{} sb.WriteString("DESCRIBE METHOD EXTENDED ") sb.WriteString(hI.Provider) @@ -191,7 +214,7 @@ func (s *simpleStackqlInterrogator) GetQueryJSON(qI dto.QueryJSONInput) (string, } func (s *simpleStackqlInterrogator) GetRegistryList(provider string) (string, error) { - if provider != "" && strings.ContainsAny(provider, forbiddenRegistryCharacters) { + if provider != "" && strings.ContainsAny(provider, forbiddenIdentifierCharacters) { return "", fmt.Errorf("forbidden characters in provider") } sb := strings.Builder{} @@ -208,8 +231,8 @@ func (s *simpleStackqlInterrogator) GetRegistryPull(provider, version string) (s if provider == "" { return "", fmt.Errorf("provider not specified") } - if strings.ContainsAny(provider, forbiddenRegistryCharacters) || - strings.ContainsAny(version, forbiddenRegistryCharacters) { + if strings.ContainsAny(provider, forbiddenIdentifierCharacters) || + strings.ContainsAny(version, forbiddenIdentifierCharacters) { return "", fmt.Errorf("forbidden characters in provider or version") } sb := strings.Builder{} diff --git a/internal/stackql/paramdecoder/paramdecoder.go b/internal/stackql/paramdecoder/paramdecoder.go index fcebe7b3..3c631838 100644 --- a/internal/stackql/paramdecoder/paramdecoder.go +++ b/internal/stackql/paramdecoder/paramdecoder.go @@ -13,9 +13,10 @@ import ( ) // Decoder decodes raw parameter bytes according to their format codes -// and OIDs, returning string representations for each. +// and OIDs, returning string representations for each. SQL NULL is a nil +// entry, so it cannot collide with a client value. type Decoder interface { - DecodeParams(paramOIDs []uint32, paramFormats []int16, paramValues [][]byte) ([]string, error) + DecodeParams(paramOIDs []uint32, paramFormats []int16, paramValues [][]byte) ([]*string, error) } // NewDecoder creates a new parameter decoder. @@ -27,11 +28,10 @@ type standardDecoder struct{} func (d *standardDecoder) DecodeParams( paramOIDs []uint32, paramFormats []int16, paramValues [][]byte, -) ([]string, error) { - result := make([]string, len(paramValues)) +) ([]*string, error) { + result := make([]*string, len(paramValues)) for i, val := range paramValues { if val == nil { - result[i] = "NULL" continue } format := resolveFormat(paramFormats, i) @@ -43,7 +43,7 @@ func (d *standardDecoder) DecodeParams( if err != nil { return nil, fmt.Errorf("parameter $%d: %w", i+1, err) } - result[i] = decoded + result[i] = &decoded } return result, nil } diff --git a/internal/stackql/paramdecoder/paramdecoder_test.go b/internal/stackql/paramdecoder/paramdecoder_test.go index edec1381..6be81cc1 100644 --- a/internal/stackql/paramdecoder/paramdecoder_test.go +++ b/internal/stackql/paramdecoder/paramdecoder_test.go @@ -19,7 +19,7 @@ func TestDecodeTextParams(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] != "hello" || results[1] != "world" { + if *results[0] != "hello" || *results[1] != "world" { t.Errorf("got %v", results) } } @@ -34,8 +34,23 @@ func TestDecodeNullParam(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] != "NULL" { - t.Errorf("got %q, want NULL", results[0]) + if results[0] != nil { + t.Errorf("got %q, want nil", *results[0]) + } +} + +func TestDecodeTextNullStringIsNotNull(t *testing.T) { + d := paramdecoder.NewDecoder() + results, err := d.DecodeParams( + []uint32{uint32(oid.T_text)}, + nil, + [][]byte{[]byte("NULL")}, + ) + if err != nil { + t.Fatal(err) + } + if results[0] == nil || *results[0] != "NULL" { + t.Errorf("got %v, want the string NULL", results[0]) } } @@ -51,8 +66,8 @@ func TestDecodeBinaryInt4(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] != "42" { - t.Errorf("got %q, want 42", results[0]) + if *results[0] != "42" { + t.Errorf("got %q, want 42", *results[0]) } } @@ -68,8 +83,8 @@ func TestDecodeBinaryInt8(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] != "9999999999" { - t.Errorf("got %q, want 9999999999", results[0]) + if *results[0] != "9999999999" { + t.Errorf("got %q, want 9999999999", *results[0]) } } @@ -85,8 +100,8 @@ func TestDecodeBinaryFloat8(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] != "3.14" { - t.Errorf("got %q, want 3.14", results[0]) + if *results[0] != "3.14" { + t.Errorf("got %q, want 3.14", *results[0]) } } @@ -100,7 +115,7 @@ func TestDecodeBinaryBool(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] != "true" || results[1] != "false" { + if *results[0] != "true" || *results[1] != "false" { t.Errorf("got %v", results) } } @@ -117,7 +132,7 @@ func TestDecodeMixedFormats(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] != "hello" || results[1] != "100" { + if *results[0] != "hello" || *results[1] != "100" { t.Errorf("got %v", results) } } @@ -132,7 +147,7 @@ func TestDecodeUnknownOIDBinaryFallsBackToText(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] != "raw-bytes" { - t.Errorf("got %q, want raw-bytes", results[0]) + if *results[0] != "raw-bytes" { + t.Errorf("got %q, want raw-bytes", *results[0]) } } diff --git a/internal/stackql/planbuilder/plan_builder.go b/internal/stackql/planbuilder/plan_builder.go index 6908e278..8e652c9a 100644 --- a/internal/stackql/planbuilder/plan_builder.go +++ b/internal/stackql/planbuilder/plan_builder.go @@ -307,13 +307,19 @@ func (pgb *standardPlanGraphBuilder) handleAuth(pbi planbuilderinput.PlanBuilder //nolint:revive // acceptable for now func(pc primitive.IPrimitiveCtx) internaldto.ExecutorOutput { authType := strings.ToLower(node.Type) + // Stage on a copy: a rejected AUTH must not alter the shared context. + candidate := authCtx.Clone() if node.KeyFilePath != "" { - authCtx.KeyFilePath = node.KeyFilePath + candidate.KeyFilePath = node.KeyFilePath } if node.KeyEnvVar != "" { - authCtx.KeyEnvVar = node.KeyEnvVar + candidate.KeyEnvVar = node.KeyEnvVar + } + _, err = prov.Auth(candidate, authType, true) + if err == nil { + authCtx.KeyFilePath = candidate.KeyFilePath + authCtx.KeyEnvVar = candidate.KeyEnvVar } - _, err = prov.Auth(authCtx, authType, true) return internaldto.NewExecutorOutput(nil, nil, nil, nil, err) }) pgb.planGraphHolder.GetPrimitiveGraph().CreatePrimitiveNode(pr) diff --git a/internal/stackql/psqlwire/server.go b/internal/stackql/psqlwire/server.go index a97046d6..05aa44e4 100644 --- a/internal/stackql/psqlwire/server.go +++ b/internal/stackql/psqlwire/server.go @@ -6,6 +6,8 @@ import ( "encoding/base64" "encoding/json" "fmt" + "net" + "os" "github.com/sirupsen/logrus" "github.com/stackql/any-sdk/pkg/dto" @@ -98,7 +100,21 @@ func MakeWireServer(sbe sqlbackend.SQLBackendFactory, cfg dto.RuntimeCtx) (IWire }, nil } +// isLoopbackHost reports whether a listen host is confined to loopback. +func isLoopbackHost(host string) bool { + if host == "localhost" { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + func (sws *SimpleWireServer) Serve() error { + if sws.server.ClientAuth != tls.RequireAndVerifyClientCert && !isLoopbackHost(sws.rtCtx.PGSrvAddress) { + fmt.Fprintf(os.Stderr, "warning: server address %s is reachable beyond loopback "+ + "with no client authentication; set clientCAs in --%s\n", + sws.rtCtx.PGSrvAddress, dto.PgSrvRawTLSCfgKey) + } sws.logger.Info( fmt.Sprintf("PostgreSQL server is up and running at [%s:%d]", sws.rtCtx.PGSrvAddress, diff --git a/internal/stackql/queryshape/queryshape.go b/internal/stackql/queryshape/queryshape.go index 52ecf84e..62862de4 100644 --- a/internal/stackql/queryshape/queryshape.go +++ b/internal/stackql/queryshape/queryshape.go @@ -18,7 +18,6 @@ package queryshape import ( "math" - "regexp" "strconv" "strings" @@ -183,51 +182,64 @@ func columnMetadataToSQLColumns(cols []typing.ColumnMetadata) []sqldata.ISQLColu return result } -var paramPlaceholderRegex = regexp.MustCompile(`\$(\d+)`) - -// SubstituteParams replaces $1, $2, ... placeholders with their bound values. -// NULL parameters (nil entries in paramValues) are substituted as the literal NULL. -// String values are single-quote escaped. -// -//nolint:revive // paramFormats retained for future binary format support -func SubstituteParams(query string, paramFormats []int16, paramValues [][]byte) string { //nolint:revive // future use - if len(paramValues) == 0 { +// SubstituteDecodedParams replaces $1, $2, ... placeholders with their +// decoded values as string literals; nil values become NULL. The query is +// scanned with the parser's own tokenizer, so a $n inside a string literal, +// quoted identifier or comment is left alone. +func SubstituteDecodedParams(query string, decodedValues []*string) string { + if len(decodedValues) == 0 { return query } - return paramPlaceholderRegex.ReplaceAllStringFunc(query, func(match string) string { - idxStr := match[1:] // strip leading $ - idx, err := strconv.Atoi(idxStr) - if err != nil || idx < 1 || idx > len(paramValues) { - return match // leave unrecognised placeholders as-is + var sb strings.Builder + copied := 0 + tokenizer := sqlparser.NewStringTokenizer(query) + // Special comments would otherwise yield tokens with their own positions. + tokenizer.SkipSpecialComments = true + for { + token, val := tokenizer.Scan() + if token == 0 { + break } - val := paramValues[idx-1] - if val == nil { - return "NULL" + idx, isPlaceholder := placeholderIndex(token, val) + if !isPlaceholder || idx > len(decodedValues) { + continue } - text := string(val) - escaped := strings.ReplaceAll(text, "'", "''") - return "'" + escaped + "'" - }) + // Position sits one past the tokenizer's lookahead character. + end := tokenizer.Position - 1 + start := end - len(val) + // A quoted identifier yields the same token but not the same source text. + if start < copied || query[start:end] != string(val) { + continue + } + sb.WriteString(query[copied:start]) + sb.WriteString(encodeParam(decodedValues[idx-1])) + copied = end + } + sb.WriteString(query[copied:]) + return sb.String() } -// SubstituteDecodedParams replaces $1, $2, ... placeholders with -// pre-decoded string values. "NULL" values are substituted unquoted; -// all other values are single-quote escaped. -func SubstituteDecodedParams(query string, decodedValues []string) string { - if len(decodedValues) == 0 { - return query +// placeholderIndex returns n for a $n identifier token. +func placeholderIndex(token int, val []byte) (int, bool) { + if token != sqlparser.ID || len(val) < 2 || val[0] != '$' { + return 0, false } - return paramPlaceholderRegex.ReplaceAllStringFunc(query, func(match string) string { - idxStr := match[1:] - idx, err := strconv.Atoi(idxStr) - if err != nil || idx < 1 || idx > len(decodedValues) { - return match - } - val := decodedValues[idx-1] - if val == "NULL" { - return "NULL" - } - escaped := strings.ReplaceAll(val, "'", "''") - return "'" + escaped + "'" - }) + idx, err := strconv.Atoi(string(val[1:])) + if err != nil || idx < 1 { + return 0, false + } + return idx, true +} + +// encodeParam renders a value as a string literal. Backslashes are doubled +// for the stackql parser, which treats them as escapes; quotes are doubled +// rather than backslash-escaped because internally routed queries reach the +// backing RDBMS verbatim, where only a doubled quote stays inside the literal. +func encodeParam(val *string) string { + if val == nil { + return "NULL" + } + escaped := strings.ReplaceAll(*val, `\`, `\\`) + escaped = strings.ReplaceAll(escaped, "'", "''") + return "'" + escaped + "'" } diff --git a/internal/stackql/queryshape/queryshape_test.go b/internal/stackql/queryshape/queryshape_test.go index ffd9aa76..7e25a3cf 100644 --- a/internal/stackql/queryshape/queryshape_test.go +++ b/internal/stackql/queryshape/queryshape_test.go @@ -4,6 +4,8 @@ import ( "encoding/json" "os" "testing" + + "github.com/stackql/stackql-parser/go/vt/sqlparser" ) type extractTableCase struct { @@ -38,19 +40,7 @@ type SubstituteParamsCase struct { Expected string `json:"expected"` } -func (c *SubstituteParamsCase) toByteSlices() [][]byte { - result := make([][]byte, len(c.ParamValues)) - for i, v := range c.ParamValues { - if v == nil { - result[i] = nil - } else { - result[i] = []byte(*v) - } - } - return result -} - -func TestSubstituteParams(t *testing.T) { +func TestSubstituteDecodedParams(t *testing.T) { data, err := os.ReadFile("testdata/substitute_params_cases.json") if err != nil { t.Fatalf("failed to read testdata: %v", err) @@ -61,10 +51,26 @@ func TestSubstituteParams(t *testing.T) { } for _, tc := range cases { t.Run(tc.Description, func(t *testing.T) { - got := SubstituteParams(tc.Query, nil, tc.toByteSlices()) + got := SubstituteDecodedParams(tc.Query, tc.ParamValues) if got != tc.Expected { - t.Errorf("SubstituteParams(%q, ...) = %q, want %q", tc.Query, got, tc.Expected) + t.Errorf("SubstituteDecodedParams(%q, ...) = %q, want %q", tc.Query, got, tc.Expected) } }) } } + +// A substituted value must reach the parser as exactly one string literal. +func TestSubstituteDecodedParamsStaysOneLiteral(t *testing.T) { + for _, val := range []string{ + `x\`, `\'`, `'`, `\\`, `a' OR 1 = 1 -- `, `"; select 1; "`, "tab\there", `\n`, "$1", + } { + resolved := SubstituteDecodedParams("SELECT $1", []*string{&val}) + tokenizer := sqlparser.NewStringTokenizer(resolved) + tokenizer.Scan() // SELECT + token, got := tokenizer.Scan() + next, _ := tokenizer.Scan() + if token != sqlparser.STRING || string(got) != val || next != 0 { + t.Errorf("value %q became %q: token %d %q, then token %d", val, resolved, token, got, next) + } + } +} diff --git a/internal/stackql/queryshape/testdata/substitute_params_cases.json b/internal/stackql/queryshape/testdata/substitute_params_cases.json index b64a382a..2ef29389 100644 --- a/internal/stackql/queryshape/testdata/substitute_params_cases.json +++ b/internal/stackql/queryshape/testdata/substitute_params_cases.json @@ -64,5 +64,53 @@ "query": "SELECT * FROM t WHERE name = $1", "paramValues": [""], "expected": "SELECT * FROM t WHERE name = ''" + }, + { + "description": "client string NULL stays a string", + "query": "SELECT * FROM t WHERE name = $1", + "paramValues": ["NULL"], + "expected": "SELECT * FROM t WHERE name = 'NULL'" + }, + { + "description": "placeholder inside string literal left alone", + "query": "SELECT '$1' AS v, $1 AS w", + "paramValues": ["x"], + "expected": "SELECT '$1' AS v, 'x' AS w" + }, + { + "description": "placeholder inside quoted identifier left alone", + "query": "SELECT \"$1\", $1 FROM t", + "paramValues": ["x"], + "expected": "SELECT \"$1\", 'x' FROM t" + }, + { + "description": "placeholder inside comments left alone", + "query": "SELECT $1 /* $2 */ FROM t -- $2", + "paramValues": ["a", "b"], + "expected": "SELECT 'a' /* $2 */ FROM t -- $2" + }, + { + "description": "trailing backslash cannot swallow the closing quote", + "query": "SELECT * FROM t WHERE a = $1 AND b = $2", + "paramValues": ["x\\", " OR 1 = 1 -- "], + "expected": "SELECT * FROM t WHERE a = 'x\\\\' AND b = ' OR 1 = 1 -- '" + }, + { + "description": "backslash then quote", + "query": "SELECT * FROM t WHERE a = $1", + "paramValues": ["\\'"], + "expected": "SELECT * FROM t WHERE a = '\\\\'''" + }, + { + "description": "double digit placeholder", + "query": "SELECT $10, $1", + "paramValues": ["1", "2", "3", "4", "5", "6", "7", "8", "9", "ten"], + "expected": "SELECT 'ten', '1'" + }, + { + "description": "multibyte text before a placeholder at end of query", + "query": "SELECT 'é' AS v, $1", + "paramValues": ["x"], + "expected": "SELECT 'é' AS v, 'x'" } ] diff --git a/pkg/mcp_server/README.md b/pkg/mcp_server/README.md index 1e6e59eb..1b286deb 100644 --- a/pkg/mcp_server/README.md +++ b/pkg/mcp_server/README.md @@ -303,6 +303,17 @@ JSON example — a single-purpose server that exposes only `server_info`: When the server is launched via the `stackql mcp` (or `stackql srv --mcp.server.type=...`) command, these fields are parsed from the same `--mcp.config` JSON blob as the rest of the configuration — no additional flag is required. For example, `stackql mcp --mcp.config='{"server": { "transport": "http", "address": "127.0.0.1:9915"}, "enabled_tools": ["server_info"]}'`. +## HTTP Client Authentication + +The HTTP transport is unauthenticated unless `server.auth_token_env_var` names an environment variable holding a bearer token. When set, every request must carry `Authorization: Bearer ` or it is answered with `401`; the server refuses to start if the variable is unset or empty. The token is read from the environment so it never appears in `--mcp.config` or the process arguments. + +```sh +export STACKQL_MCP_TOKEN="$(openssl rand -hex 32)" +stackql mcp --mcp.config='{"server": {"transport": "http", "address": "127.0.0.1:9876", "auth_token_env_var": "STACKQL_MCP_TOKEN"}}' +``` + +Binding an address beyond loopback without a token prints a warning at startup. Cross-origin browser requests are refused regardless. The stdio transport is unaffected. + ## Server Modes `Config.Server.Mode` chooses one of four safety contracts. All four allow SELECT and metadata reads; they differ in how they handle mutations and lifecycle operations. @@ -319,6 +330,10 @@ When the server is launched via the `stackql mcp` (or `stackql srv --mcp.server. - If the client advertised the elicitation capability at initialise, the server sends an `elicitation/create` request with a short message describing the action and the SQL. The user accepts, declines, or cancels. - If the client did NOT advertise elicitation, the tool is refused with a message that explains the gap and points the operator at `full_access` mode. +A payload holding several statements is gated by its most privileged statement, so `SELECT 1; DELETE ...` is treated as a `DELETE`. `validate_select_query` accepts exactly one statement. + +Approval is answered by the client, so it guards against an agent acting without its user, not against an untrusted caller; pair the HTTP transport with [client authentication](#http-client-authentication) where callers are not trusted. + The mode is global per server. There is no per-tool override in this release. ### Default-mode change (breaking) @@ -444,7 +459,7 @@ No MCP SDK dependency is required as the package implements the MCP protocol dir 1. **Full WebSocket Implementation**: Complete WebSocket transport support 2. **Stdio Transport**: Complete stdio JSON-RPC implementation -3. **Authentication**: Add authentication and authorization support +3. **Authorization**: Per-client identity and roles (bearer token authentication is available for the HTTP transport) 4. **Streaming**: Support for streaming large query results 5. **Caching**: Query result caching for improved performance 6. **Metrics**: Prometheus metrics for monitoring and observability \ No newline at end of file diff --git a/pkg/mcp_server/config.go b/pkg/mcp_server/config.go index b95de241..938ae0e6 100644 --- a/pkg/mcp_server/config.go +++ b/pkg/mcp_server/config.go @@ -206,6 +206,10 @@ type ServerConfig struct { TLSCertFile string `json:"tls_cert_file,omitempty" yaml:"tls_cert_file,omitempty"` TLSKeyFile string `json:"tls_key_file,omitempty" yaml:"tls_key_file,omitempty"` + // AuthTokenEnvVar names an env var holding a bearer token. When set, the + // HTTP transport requires `Authorization: Bearer ` on every request. + AuthTokenEnvVar string `json:"auth_token_env_var,omitempty" yaml:"auth_token_env_var,omitempty"` + TransportCfg map[string]any `json:"transport_cfg,omitempty" yaml:"transport_cfg,omitempty"` // Stateless serves Streamable HTTP without sessions (no Mcp-Session-Id), @@ -261,6 +265,7 @@ type serverConfigWire struct { Version string `json:"version" yaml:"version"` TLSCertFile string `json:"tls_cert_file,omitempty" yaml:"tls_cert_file,omitempty"` TLSKeyFile string `json:"tls_key_file,omitempty" yaml:"tls_key_file,omitempty"` + AuthTokenEnvVar string `json:"auth_token_env_var,omitempty" yaml:"auth_token_env_var,omitempty"` TransportCfg map[string]any `json:"transport_cfg,omitempty" yaml:"transport_cfg,omitempty"` Stateless bool `json:"stateless,omitempty" yaml:"stateless,omitempty"` ProtocolVersion string `json:"protocol_version,omitempty" yaml:"protocol_version,omitempty"` @@ -282,6 +287,7 @@ func (s *ServerConfig) fromWire(w serverConfigWire) { s.Version = w.Version s.TLSCertFile = w.TLSCertFile s.TLSKeyFile = w.TLSKeyFile + s.AuthTokenEnvVar = w.AuthTokenEnvVar s.TransportCfg = w.TransportCfg s.Stateless = w.Stateless s.ProtocolVersion = w.ProtocolVersion diff --git a/pkg/mcp_server/policy/policy.go b/pkg/mcp_server/policy/policy.go index 6d635b61..3f05c58e 100644 --- a/pkg/mcp_server/policy/policy.go +++ b/pkg/mcp_server/policy/policy.go @@ -6,6 +6,8 @@ package policy import ( "regexp" "strings" + + "github.com/stackql/stackql-parser/go/vt/sqlparser" ) // Server modes. These are the legal values for Config.Server.Mode. @@ -17,9 +19,9 @@ const ( ) // QueryClass identifies the kind of statement a query tool is being asked to run. -// The classifier is intentionally shallow: it looks at the first token only, -// except for a leading WITH, which is resolved by scanning for a -// data-modifying keyword (see ClassifyQuery). +// The classifier is intentionally shallow: it looks at the first token of each +// statement only, except for a leading WITH, which is resolved by scanning for +// a data-modifying keyword (see ClassifyQuery). type QueryClass int const ( @@ -52,19 +54,61 @@ func (c QueryClass) String() string { // whether the common table expression heads a read-only statement. var withMutationRegexp = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE|REPLACE|MERGE|UPSERT|EXEC)\b`) -// ClassifyQuery returns the class of the SQL by inspecting only the first -// whitespace-separated token. It does not parse the statement. Empty or -// unrecognised inputs return QueryClassUnknown. +// ClassifyQuery returns the class of the SQL. The payload is split exactly as +// the execution layer splits it, so a mutation cannot hide behind a leading +// read-only statement; the most privileged statement decides the class. +// Empty or unrecognised inputs return QueryClassUnknown. +func ClassifyQuery(sql string) QueryClass { + class := QueryClassUnknown + rank := -1 + for _, stmt := range statements(sql) { + stmtClass := classifyStatement(stmt) + if stmtRank := classPrivilege(stmtClass); stmtRank > rank { + class, rank = stmtClass, stmtRank + } + } + return class +} + +// IsSingleStatement reports whether the SQL holds exactly one statement. +func IsSingleStatement(sql string) bool { + return len(statements(sql)) == 1 +} + +// statements splits a payload with the execution layer's own splitter and +// returns the trimmed, non-blank pieces. +func statements(sql string) []string { + // Error ignored as in the execution layer, which runs whatever pieces come back. + pieces, _ := sqlparser.SplitStatementToPieces(sql) + rv := make([]string, 0, len(pieces)) + for _, piece := range pieces { + if trimmed := strings.TrimSpace(piece); trimmed != "" { + rv = append(rv, trimmed) + } + } + return rv +} + +// classPrivilege ranks classes so that GateDecision is never more permissive +// for a higher rank, in any mode. +func classPrivilege(class QueryClass) int { + if class == QueryClassSelect { + return 0 + } + if class == QueryClassMutationCreate { + return 1 + } + return 2 +} + +// classifyStatement classifies a single trimmed statement by inspecting only +// the first whitespace-separated token. It does not parse the statement. // // A leading WITH is the one case that needs more than the first token: SQL // allows a common table expression to head a read-only SELECT but also an // INSERT, UPDATE or DELETE, so classifying every WITH as read-only would let // a mutation through the gate. Such statements are resolved by classifyWith. -func ClassifyQuery(sql string) QueryClass { - trimmed := strings.TrimSpace(sql) - if trimmed == "" { - return QueryClassUnknown - } +func classifyStatement(trimmed string) QueryClass { // First whitespace-separated token. var verb string if idx := strings.IndexAny(trimmed, " \t\r\n"); idx >= 0 { @@ -201,7 +245,7 @@ func (p policy) Reason() string { return p.reason } // NewPolicy is the factory. When sql is empty (no SQL input on a metadata // tool, for example), defaultClass becomes the effective class; otherwise the -// classifier inspects the SQL's first token. Mode is normalised internally +// classifier inspects every statement in the SQL. Mode is normalised internally // so an empty / unknown value behaves like ModeSafe. func NewPolicy(mode, sql string, defaultClass QueryClass) Policy { class := defaultClass diff --git a/pkg/mcp_server/policy/policy_test.go b/pkg/mcp_server/policy/policy_test.go index 392ee98e..2be5181a 100644 --- a/pkg/mcp_server/policy/policy_test.go +++ b/pkg/mcp_server/policy/policy_test.go @@ -39,6 +39,16 @@ func TestClassifyQuery(t *testing.T) { {"WITH t AS (SELECT 1) INSERT INTO x SELECT * FROM t", policy.QueryClassUnknown}, {"with t as (select 1) update x set a = 1", policy.QueryClassUnknown}, {"WITH", policy.QueryClassSelect}, // degenerate, no mutation keyword present + // Every statement is classified; the most privileged one decides. + {"SELECT 1; delete from t", policy.QueryClassMutationDelete}, + {"SELECT 1;\nEXEC a.b.c", policy.QueryClassLifecycle}, + {"select 1; insert into t values (1)", policy.QueryClassMutationCreate}, + {"insert into t values (1); select 1", policy.QueryClassMutationCreate}, + {"SELECT 1; create table t (x int)", policy.QueryClassUnknown}, + {"SELECT;DELETE FROM t", policy.QueryClassMutationDelete}, + {"select 1; select 2;", policy.QueryClassSelect}, + {"select ';' as semi", policy.QueryClassSelect}, // quoted separator does not split + {";", policy.QueryClassUnknown}, } for _, c := range cases { t.Run(c.sql, func(t *testing.T) { diff --git a/pkg/mcp_server/server.go b/pkg/mcp_server/server.go index 0b689331..f6f5c68c 100644 --- a/pkg/mcp_server/server.go +++ b/pkg/mcp_server/server.go @@ -2,17 +2,21 @@ package mcp_server //nolint:revive // fine for now import ( "context" + "crypto/subtle" "encoding/json" "errors" "fmt" "io" + "net" "net/http" + "os" "sync" "time" "github.com/sirupsen/logrus" "golang.org/x/sync/semaphore" + "github.com/modelcontextprotocol/go-sdk/auth" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stackql/stackql/pkg/mcp_server/audit" @@ -62,7 +66,15 @@ func (s *simpleMCPServer) runHTTPServer(server *mcp.Server, config *Config) erro return server }, &mcp.StreamableHTTPOptions{Stateless: config.IsStateless()}) - handlerWithLogging := loggingHandler(handler, s.logger) + securedHandler, authErr := secureHTTPHandler(handler, config) + if authErr != nil { + return authErr + } + if config.Server.AuthTokenEnvVar == "" && !isLoopbackAddress(address) { + fmt.Fprintf(stderrSink(), "warning: MCP server address %s is reachable beyond loopback "+ + "with no client authentication; set server.auth_token_env_var\n", address) + } + handlerWithLogging := loggingHandler(securedHandler, s.logger) s.logger.Debugf("MCP server listening on %s", address) @@ -83,6 +95,42 @@ func (s *simpleMCPServer) runHTTPServer(server *mcp.Server, config *Config) erro return nil } +// secureHTTPHandler adds cross-origin protection and, when +// server.auth_token_env_var is set, bearer token authentication. +func secureHTTPHandler(handler http.Handler, config *Config) (http.Handler, error) { + handler = http.NewCrossOriginProtection().Handler(handler) + envVar := config.Server.AuthTokenEnvVar + if envVar == "" { + return handler, nil + } + token := os.Getenv(envVar) + if token == "" { + return nil, fmt.Errorf("server.auth_token_env_var names an unset or empty env var %q", envVar) + } + verifier := func(_ context.Context, presented string, _ *http.Request) (*auth.TokenInfo, error) { + if subtle.ConstantTimeCompare([]byte(presented), []byte(token)) != 1 { + return nil, auth.ErrInvalidToken + } + return &auth.TokenInfo{}, nil + } + // A static token carries no expiry. + opts := &auth.RequireBearerTokenOptions{AllowMissingExpiration: true} + return auth.RequireBearerToken(verifier, opts)(handler), nil +} + +// isLoopbackAddress reports whether a listen address is confined to loopback. +func isLoopbackAddress(address string) bool { + host, _, err := net.SplitHostPort(address) + if err != nil { + return false + } + if host == "localhost" { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + // addPromptIfEnabled registers a prompt only when cfg.IsPromptEnabled allows it. func addPromptIfEnabled(s *mcp.Server, cfg *Config, p *mcp.Prompt, h mcp.PromptHandler) { if !cfg.IsPromptEnabled(p.Name) { @@ -416,6 +464,10 @@ func registerTools(server *mcp.Server, cfg *Config, backend Backend, logger *log if formatErr != nil { return nil, dto.ValidationResultDTO{}, formatErr } + // Only the first statement would sit inside the backend's EXPLAIN. + if !policy.IsSingleStatement(args.SQL) { + return nil, dto.ValidationResultDTO{}, fmt.Errorf("validate_select_query accepts exactly one statement") + } rowsBack, err := backend.ValidateQuery(ctx, args.SQL) isValid := err == nil var errs []string diff --git a/pkg/mcp_server/server_test.go b/pkg/mcp_server/server_test.go index 73c56f61..d9534bca 100644 --- a/pkg/mcp_server/server_test.go +++ b/pkg/mcp_server/server_test.go @@ -3,6 +3,8 @@ package mcp_server //nolint:testpackage,revive // fine for now import ( "context" "encoding/json" + "net/http" + "net/http/httptest" "reflect" "strings" "testing" @@ -184,3 +186,65 @@ func TestNewMCPServerWithExampleBackend(t *testing.T) { t.Fatal("Server should not be nil") } } + +func TestSecureHTTPHandler(t *testing.T) { + ok := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.WriteHeader(http.StatusOK) }) + status := func(h http.Handler, headers map[string]string) int { + req := httptest.NewRequest(http.MethodPost, "http://127.0.0.1:9876/", strings.NewReader("{}")) + for k, v := range headers { + req.Header.Set(k, v) + } + rec := httptest.NewRecorder() + h.ServeHTTP(rec, req) + return rec.Code + } + + open, err := secureHTTPHandler(ok, DefaultHTTPConfig()) + if err != nil { + t.Fatalf("no token configured: %v", err) + } + if got := status(open, nil); got != http.StatusOK { + t.Errorf("no token configured: status %d, want 200", got) + } + if got := status(open, map[string]string{"Origin": "https://evil.example"}); got != http.StatusForbidden { + t.Errorf("cross-origin request: status %d, want 403", got) + } + + cfg := DefaultHTTPConfig() + cfg.Server.AuthTokenEnvVar = "STACKQL_MCP_TEST_TOKEN" + if _, err = secureHTTPHandler(ok, cfg); err == nil { + t.Error("an unset token env var must fail startup") + } + t.Setenv("STACKQL_MCP_TEST_TOKEN", "s3cret") + guarded, err := secureHTTPHandler(ok, cfg) + if err != nil { + t.Fatalf("token configured: %v", err) + } + for name, tc := range map[string]struct { + header string + want int + }{ + "missing": {"", http.StatusUnauthorized}, + "wrong": {"Bearer nope", http.StatusUnauthorized}, + "right": {"Bearer s3cret", http.StatusOK}, + } { + headers := map[string]string{} + if tc.header != "" { + headers["Authorization"] = tc.header + } + if got := status(guarded, headers); got != tc.want { + t.Errorf("%s token: status %d, want %d", name, got, tc.want) + } + } +} + +func TestIsLoopbackAddress(t *testing.T) { + for address, want := range map[string]bool{ + "127.0.0.1:9876": true, "localhost:9876": true, "[::1]:9876": true, + "0.0.0.0:9876": false, ":9876": false, "192.168.1.10:9876": false, "example.com:9876": false, + } { + if got := isLoopbackAddress(address); got != want { + t.Errorf("isLoopbackAddress(%q) = %v, want %v", address, got, want) + } + } +} diff --git a/pkg/mcp_server/tools_test.go b/pkg/mcp_server/tools_test.go index 8c2685cd..0878c437 100644 --- a/pkg/mcp_server/tools_test.go +++ b/pkg/mcp_server/tools_test.go @@ -633,6 +633,23 @@ func TestMode_ReadOnly_RefusesAllMutationsAndLifecycle(t *testing.T) { } } +func TestMode_ReadOnly_RefusesSmuggledStatements(t *testing.T) { + be := &testBackend{} + cs := connectInProcess(t, readOnlyConfig(), be) + + // A mutation behind a leading SELECT is gated like the mutation itself. + callExpectingError(t, cs, "run_select_query", + map[string]any{"sql": "select 1; delete from t"}, "read_only") + callExpectingError(t, cs, "run_mutation_query", + map[string]any{"sql": "select 1; create table t (x int)"}, "read_only") + // Validation wraps only the first statement in EXPLAIN, so it takes just one. + callExpectingError(t, cs, "validate_select_query", + map[string]any{"sql": "select 1; delete from t"}, "exactly one statement") + if be.lastQueryJSON.SQL != "" || be.lastExecQuery != "" || be.lastValidateSQL != "" { + t.Errorf("backend should not have been called: %+v", be) + } +} + func TestMode_Safe_RefusesMutationsWithoutElicitation(t *testing.T) { be := &testBackend{} cs := connectInProcess(t, DefaultConfig(), be) // default is safe diff --git a/test/python/stackql_test_tooling/StackQLInterfaces.py b/test/python/stackql_test_tooling/StackQLInterfaces.py index f6466976..1cc4905a 100644 --- a/test/python/stackql_test_tooling/StackQLInterfaces.py +++ b/test/python/stackql_test_tooling/StackQLInterfaces.py @@ -784,6 +784,15 @@ def should_PG_client_prepared_query_results_have_length(self, conn_str :str, que return self.should_be_equal(len(result), expected_length) + @keyword + def should_PG_client_bound_query_results_equal(self, conn_str :str, query :str, params :typing.Tuple, expected_output :typing.List[typing.Dict], **kwargs): + """Execute a parameterised query via psycopg (v3, extended query protocol with server side binding) and verify the rows.""" + client = PsycoPGClient(conn_str) + result = client.exec_bound_query(query, params) + self.log(result) + return self.lists_should_be_equal(result, expected_output) + + @keyword def should_PG_client_V2_session_inline_equal(self, conn_str :str, queries :typing.List[str], expected_output :typing.List[typing.Dict], **kwargs): client = PsycoPG2Client(conn_str) diff --git a/test/python/stackql_test_tooling/mcp_stdio_client.py b/test/python/stackql_test_tooling/mcp_stdio_client.py index 26d53ac5..0e884ced 100644 --- a/test/python/stackql_test_tooling/mcp_stdio_client.py +++ b/test/python/stackql_test_tooling/mcp_stdio_client.py @@ -650,10 +650,11 @@ def _http_delete(url, headers=None): return err.code -def run_http_legacy_roundtrip(url): +def run_http_legacy_roundtrip(url, extra_headers=None): """2025-06-18 client over Streamable HTTP: initialize (session id issued), initialized, then tools/list on that session. Returns the negotiated - version, whether a session id was issued and the tool names.""" + version, whether a session id was issued and the tool names. + extra_headers (eg Authorization) are sent on every request.""" status, headers, init = _http_post(url, { "jsonrpc": "2.0", "id": 1, @@ -663,9 +664,11 @@ def run_http_legacy_roundtrip(url): "capabilities": {}, "clientInfo": {"name": "robot-legacy-http", "version": "0.1.0"}, }, - }) + }, extra_headers) session_id = headers.get("mcp-session-id", "") - session_headers = {"Mcp-Session-Id": session_id} if session_id else {} + session_headers = dict(extra_headers or {}) + if session_id: + session_headers["Mcp-Session-Id"] = session_id _http_post(url, {"jsonrpc": "2.0", "method": "notifications/initialized"}, session_headers) _, _, listed = _http_post(url, {"jsonrpc": "2.0", "id": 2, "method": "tools/list"}, session_headers) return { diff --git a/test/python/stackql_test_tooling/psycopg_client.py b/test/python/stackql_test_tooling/psycopg_client.py index 20dcb879..6afb02a8 100644 --- a/test/python/stackql_test_tooling/psycopg_client.py +++ b/test/python/stackql_test_tooling/psycopg_client.py @@ -63,3 +63,8 @@ def run_queries(self, queries :typing.List[str]) -> typing.List[typing.Dict]: def run_queries_strict(self, queries :typing.List[str]) -> typing.List[typing.Dict]: return self._run_queries_strict(queries) + + def exec_bound_query(self, query :str, params :tuple) -> typing.List[typing.Dict]: + """Execute with server side binding; psycopg (v3) sends Parse/Bind/Execute when params are supplied.""" + with self._connection.execute(query, params) as r: + return [row for row in r] diff --git a/test/robot/functional/mcp.robot b/test/robot/functional/mcp.robot index 2b060a65..db67a975 100644 --- a/test/robot/functional/mcp.robot +++ b/test/robot/functional/mcp.robot @@ -231,6 +231,20 @@ Start MCP Servers ... \-\-tls.allowInsecure ... stdout=${CURDIR}${/}tmp${/}Stackql-MCP-Server-Pinned-Legacy.txt ... stderr=${CURDIR}${/}tmp${/}Stackql-MCP-Server-Pinned-Legacy-stderr.txt + # server.auth_token_env_var: every HTTP request must carry the bearer token. + Start Process ${STACKQL_EXE} + ... mcp + ... \-\-mcp.server.type\=http + ... \-\-mcp.config + ... {"server": {"transport": "http", "address": "127.0.0.1:9931", "auth_token_env_var": "STACKQL_MCP_ROBOT_TOKEN", "mode": "full_access", "audit": {"disabled": true}} } + ... \-\-registry + ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} + ... \-\-auth + ... ${AUTH_CFG_STR} + ... \-\-tls.allowInsecure + ... env:STACKQL_MCP_ROBOT_TOKEN=robot-mcp-token + ... stdout=${CURDIR}${/}tmp${/}Stackql-MCP-Server-Bearer-Auth.txt + ... stderr=${CURDIR}${/}tmp${/}Stackql-MCP-Server-Bearer-Auth-stderr.txt Sleep 5s Parse MCP JSON Output @@ -869,6 +883,40 @@ MCP HTTP Mode Read Only Allows Desc Alias Dictionary Should Contain Key ${desc_obj} rows Should Not Be Empty ${desc_obj['rows']} +MCP HTTP Mode Read Only Refuses Smuggled Statements + [Documentation] The read_only server at 9920 gates every statement in a payload: a mutation behind a leading SELECT, or riding a hierarchy identifier, is refused. + Pass Execution If "%{IS_SKIP_MCP_TEST=false}" == "true" Some platforms do not have the MCP client available + ${smuggled}= Run Process ${STACKQL_MCP_CLIENT_EXE} + ... exec + ... \-\-client\-type\=http + ... \-\-url\=http://127.0.0.1:9920 + ... \-\-exec.action run_select_query + ... \-\-exec.args {"sql":"select 1; delete from google.compute.firewalls where project \= 'mutable\-project' and firewall \= 'deletable\-firewall';"} + ... stdout=${CURDIR}${/}tmp${/}MCP-Mode-ReadOnly-smuggled-select.txt + ... stderr=${CURDIR}${/}tmp${/}MCP-Mode-ReadOnly-smuggled-select-stderr.txt + Should Not Be Equal As Integers ${smuggled.rc} 0 + Should Contain ${smuggled.stderr} read_only + ${validate}= Run Process ${STACKQL_MCP_CLIENT_EXE} + ... exec + ... \-\-client\-type\=http + ... \-\-url\=http://127.0.0.1:9920 + ... \-\-exec.action validate_select_query + ... \-\-exec.args {"sql":"select 1; delete from google.compute.firewalls where project \= 'mutable\-project' and firewall \= 'deletable\-firewall';"} + ... stdout=${CURDIR}${/}tmp${/}MCP-Mode-ReadOnly-smuggled-validate.txt + ... stderr=${CURDIR}${/}tmp${/}MCP-Mode-ReadOnly-smuggled-validate-stderr.txt + Should Not Be Equal As Integers ${validate.rc} 0 + Should Contain ${validate.stderr} exactly one statement + ${hierarchy}= Run Process ${STACKQL_MCP_CLIENT_EXE} + ... exec + ... \-\-client\-type\=http + ... \-\-url\=http://127.0.0.1:9920 + ... \-\-exec.action list_services + ... \-\-exec.args {"provider":"google; delete from google.compute.firewalls where project \= 'mutable\-project' and firewall \= 'deletable\-firewall'"} + ... stdout=${CURDIR}${/}tmp${/}MCP-Mode-ReadOnly-smuggled-hierarchy.txt + ... stderr=${CURDIR}${/}tmp${/}MCP-Mode-ReadOnly-smuggled-hierarchy-stderr.txt + Should Not Be Equal As Integers ${hierarchy.rc} 0 + Should Contain ${hierarchy.stderr} forbidden characters + MCP HTTP Mode Safe Refuses Mutations Without Elicitation [Documentation] Server at 9912 starts with mode=full_access (existing scenarios assume that). ... We verify the safe-mode no-elicitation path via the *default* mode on a @@ -1788,3 +1836,40 @@ MCP Server Refuses Unsupported Protocol Version Should Not Be Equal As Integers ${result.rc} 0 Should Contain ${result.stderr} invalid server.protocol_version "2020-01-01" Should Contain ${result.stderr} legal: auto, 2026-07-28, 2025-11-25 + +MCP HTTP Bearer Token Authentication + [Documentation] The server at 9931 sets server.auth_token_env_var: requests with no token + ... or a wrong one get 401, the right token gets a working session, and a + ... cross-origin browser request is refused even with the token. + Pass Execution If "%{IS_SKIP_MCP_TEST=false}" == "true" Some platforms do not have the MCP client available + ${anon}= Evaluate stackql_test_tooling.mcp_stdio_client.run_http_legacy_roundtrip('http://127.0.0.1:9931') modules=stackql_test_tooling.mcp_stdio_client + Should Be Equal As Integers ${anon['status']} 401 + Should Be Equal ${anon['tools']} ${{[]}} + ${wrong}= Evaluate stackql_test_tooling.mcp_stdio_client.run_http_legacy_roundtrip('http://127.0.0.1:9931', {'Authorization': 'Bearer not-the-token'}) modules=stackql_test_tooling.mcp_stdio_client + Should Be Equal As Integers ${wrong['status']} 401 + ${authed}= Evaluate stackql_test_tooling.mcp_stdio_client.run_http_legacy_roundtrip('http://127.0.0.1:9931', {'Authorization': 'Bearer robot-mcp-token'}) modules=stackql_test_tooling.mcp_stdio_client + Should Be Equal As Integers ${authed['status']} 200 + Should Be True ${authed['session_issued']} + List Should Contain Value ${authed['tools']} server_info + ${cross}= Evaluate stackql_test_tooling.mcp_stdio_client.run_http_legacy_roundtrip('http://127.0.0.1:9931', {'Authorization': 'Bearer robot-mcp-token', 'Origin': 'https://cross-origin.example'}) modules=stackql_test_tooling.mcp_stdio_client + Should Be Equal As Integers ${cross['status']} 403 + +MCP Server Refuses Unset Bearer Token Env Var + [Documentation] server.auth_token_env_var naming an unset env var fails at startup + ... rather than serving unauthenticated. + Pass Execution If "%{IS_SKIP_MCP_TEST=false}" == "true" Some platforms do not have the MCP client available + ${result}= Run Process ${STACKQL_EXE} + ... mcp + ... \-\-mcp.server.type\=http + ... \-\-mcp.config + ... {"server": {"transport": "http", "address": "127.0.0.1:9932", "auth_token_env_var": "STACKQL_MCP_ROBOT_TOKEN_UNSET", "audit": {"disabled": true}} } + ... \-\-registry + ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} + ... \-\-auth + ... ${AUTH_CFG_STR} + ... \-\-tls.allowInsecure + ... timeout=90s + ... stdout=${CURDIR}${/}tmp${/}MCP-Unset-Bearer-Token.txt + ... stderr=${CURDIR}${/}tmp${/}MCP-Unset-Bearer-Token-stderr.txt + Should Not Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stderr} server.auth_token_env_var names an unset or empty env var diff --git a/test/robot/functional/stackql_mocked_from_cmd_line.robot b/test/robot/functional/stackql_mocked_from_cmd_line.robot index c23d8225..74179c75 100644 --- a/test/robot/functional/stackql_mocked_from_cmd_line.robot +++ b/test/robot/functional/stackql_mocked_from_cmd_line.robot @@ -11787,3 +11787,19 @@ Preview Iac Resource Without Provider Is Refused By Omnisdk ... --preview\=${preview} ... stdout=${CURDIR}${/}tmp${/}Preview-Iac-Resource-No-Provider.tmp ... stderr=${CURDIR}${/}tmp${/}Preview-Iac-Resource-No-Provider-stderr.tmp + +Rejected Auth Login Leaves Credential Source Unchanged + [Documentation] A failing AUTH LOGIN must not redirect where later statements source the provider credential. + Should Stackql Exec Inline Contain Both Streams + ... ${STACKQL_EXE} + ... ${OKTA_SECRET_STR} + ... ${GITHUB_SECRET_STR} + ... ${K8S_SECRET_STR} + ... ${REGISTRY_NO_VERIFY_CFG_STR} + ... ${AUTH_CFG_STR} + ... ${SQL_BACKEND_CFG_STR_CANONICAL} + ... auth login google sa '/no/such/sa-key.json' ''; select name from google.storage.buckets where project \= 'stackql-demo' order by name desc; + ... stackql-demo-src-bucket + ... no such file + ... stdout=${CURDIR}/tmp/Rejected-Auth-Login-Leaves-Credential-Source-Unchanged.tmp + ... stderr=${CURDIR}/tmp/Rejected-Auth-Login-Leaves-Credential-Source-Unchanged-stderr.tmp diff --git a/test/robot/functional/stackql_sessions.robot b/test/robot/functional/stackql_sessions.robot index 102d0e7d..816883f8 100644 --- a/test/robot/functional/stackql_sessions.robot +++ b/test/robot/functional/stackql_sessions.robot @@ -96,6 +96,27 @@ Shell Session Invalid Registry Config Exits Cleanly Should Not Contain ${result.stderr} panic [Teardown] NONE +PG Server Warns When Exposed Without Client Authentication + [Documentation] Binding beyond loopback with no client CAs warns on stderr at startup; a loopback bind does not. + Pass Execution If "${EXECUTION_PLATFORM}" == "docker" Native server start only + ${exposed} = Run Process ${STACKQL_EXE} + ... srv \-\-registry\=${REGISTRY_NO_VERIFY_CFG_STR.get_config_str('native')} + ... \-\-pgsrv\.address\=0.0.0.0 + ... \-\-pgsrv\.port\=5667 + ... timeout=10s on_timeout=kill + ... stdout=${CURDIR}/tmp/PG-Server-Warns-When-Exposed.tmp + ... stderr=${CURDIR}/tmp/PG-Server-Warns-When-Exposed-stderr.tmp + Should Contain ${exposed.stderr} reachable beyond loopback with no client authentication + ${loopback} = Run Process ${STACKQL_EXE} + ... srv \-\-registry\=${REGISTRY_NO_VERIFY_CFG_STR.get_config_str('native')} + ... \-\-pgsrv\.address\=127.0.0.1 + ... \-\-pgsrv\.port\=5668 + ... timeout=10s on_timeout=kill + ... stdout=${CURDIR}/tmp/PG-Server-Warns-When-Exposed-loopback.tmp + ... stderr=${CURDIR}/tmp/PG-Server-Warns-When-Exposed-loopback-stderr.tmp + Should Not Contain ${loopback.stderr} no client authentication + [Teardown] NONE + PG Session GC Manual Behaviour Canonical [Tags] shard-group:pg-server-generation Should PG Client Session Inline Equal Strict @@ -264,6 +285,20 @@ PG Session Server Survives Defective Query ... stdout=${CURDIR}/tmp/PG-Session-Server-Survives-Defective-Query-and-Subsequently-Serves-Valid-Query.tmp [Teardown] NONE +PG Session Extended Query Bound Params Stay Data + [Documentation] Bound parameters are data, never SQL: the string NULL is not SQL NULL, + ... a trailing backslash cannot swallow the closing quote, and a + ... placeholder inside a string literal is not substituted. + ${params} = Evaluate ('NULL', 'x' + chr(92), 'y') + ${expected} = Evaluate [{'a': 'text', 'b': 'ne', 'c': 'y', 'd': '$1'}] + Should PG Client Bound Query Results Equal + ... ${PSQL_UNENCRYPTED_CONN_STR} + ... SELECT CASE WHEN %s \= 'NULL' THEN 'text' ELSE 'null' END AS a, CASE WHEN %s \= 'never' THEN 'eq' ELSE 'ne' END AS b, %s AS c, '$1' AS d + ... ${params} + ... ${expected} + ... stdout=${CURDIR}/tmp/PG-Session-Extended-Query-Bound-Params-Stay-Data.tmp + [Teardown] NONE + PG Session Postgres Client V2 Typed Queries Pass Execution If "${SQL_BACKEND}" == "postgres_tcp" TODO: FIX THIS... Skipping postgres backend test likely due to typing issues Should PG Client V2 Session Inline Equal From 64b7b626c51f3091f9acb0f707b26cf63c8c8dc6 Mon Sep 17 00:00:00 2001 From: Jeffrey Aven Date: Fri, 2 Oct 2026 16:31:15 +1000 Subject: [PATCH 2/6] readme: note that srv clients are unauthenticated without mTLS Co-Authored-By: Claude Fable 5.1 --- README.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 7ef10c9c..db1a6d27 100644 --- a/README.md +++ b/README.md @@ -333,7 +333,8 @@ StackQL can be used via the interactive REPL shell, or via the `exec` command or * Server ```sh # serve client requests over the Postgres wire protocol (psycopg2, etc.) - stackql srv --auth="${AUTH}" + # clients are not authenticated unless mTLS is configured via --pgsrv.tls, so keep the listener on loopback otherwise + stackql srv --pgsrv.address=127.0.0.1 --auth="${AUTH}" ``` _For more examples, please check our [Blog][blog]_ From 7630daea0e3eefde65d6fe76f861a44ae6edb967 Mon Sep 17 00:00:00 2001 From: Jeffrey Aven Date: Fri, 2 Oct 2026 17:08:47 +1000 Subject: [PATCH 3/6] robot: platform-neutral error assertion for rejected auth login Co-Authored-By: Claude Fable 5.1 --- test/robot/functional/stackql_mocked_from_cmd_line.robot | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/robot/functional/stackql_mocked_from_cmd_line.robot b/test/robot/functional/stackql_mocked_from_cmd_line.robot index 74179c75..369d8584 100644 --- a/test/robot/functional/stackql_mocked_from_cmd_line.robot +++ b/test/robot/functional/stackql_mocked_from_cmd_line.robot @@ -11800,6 +11800,6 @@ Rejected Auth Login Leaves Credential Source Unchanged ... ${SQL_BACKEND_CFG_STR_CANONICAL} ... auth login google sa '/no/such/sa-key.json' ''; select name from google.storage.buckets where project \= 'stackql-demo' order by name desc; ... stackql-demo-src-bucket - ... no such file + ... service account credentials error ... stdout=${CURDIR}/tmp/Rejected-Auth-Login-Leaves-Credential-Source-Unchanged.tmp ... stderr=${CURDIR}/tmp/Rejected-Auth-Login-Leaves-Credential-Source-Unchanged-stderr.tmp From 343a21f43faf8c611250da9d9cd956c2f6194a9d Mon Sep 17 00:00:00 2001 From: Jeffrey Aven Date: Fri, 2 Oct 2026 20:30:36 +1000 Subject: [PATCH 4/6] address review feedback - driver.go: drop the variable rename, the file is unchanged from main. - paramdecoder: return a DecodedParam abstraction (GetValue, IsNull), one per parameter, instead of nullable pointers. - statement splitting moves out of the MCP policy package into pkg/sqlsplit. - remove the loopback startup warnings (wire server and MCP) and their tests. - plan_builder: comment states that Auth clones the context it is given. Co-Authored-By: Claude Fable 5.1 --- internal/stackql/driver/driver.go | 4 +-- internal/stackql/paramdecoder/paramdecoder.go | 35 +++++++++++++++---- .../stackql/paramdecoder/paramdecoder_test.go | 30 ++++++++-------- internal/stackql/planbuilder/plan_builder.go | 3 +- internal/stackql/psqlwire/server.go | 16 --------- internal/stackql/queryshape/queryshape.go | 13 +++---- .../stackql/queryshape/queryshape_test.go | 27 ++++++++++++-- pkg/mcp_server/README.md | 2 +- pkg/mcp_server/policy/policy.go | 29 +++------------ pkg/mcp_server/server.go | 21 ++--------- pkg/mcp_server/server_test.go | 11 ------ pkg/sqlsplit/sqlsplit.go | 22 ++++++++++++ pkg/sqlsplit/sqlsplit_test.go | 31 ++++++++++++++++ test/robot/functional/stackql_sessions.robot | 21 ----------- 14 files changed, 141 insertions(+), 124 deletions(-) create mode 100644 pkg/sqlsplit/sqlsplit.go create mode 100644 pkg/sqlsplit/sqlsplit_test.go diff --git a/internal/stackql/driver/driver.go b/internal/stackql/driver/driver.go index 5df7d722..8d5fa2d0 100644 --- a/internal/stackql/driver/driver.go +++ b/internal/stackql/driver/driver.go @@ -320,11 +320,11 @@ func (dr *basicStackQLDriver) HandleExecute( } } // Decode params (handles both text and binary formats). - decodedValues, err := dr.paramDecoder.DecodeParams(paramOIDs, paramFormats, paramValues) + decodedStrings, err := dr.paramDecoder.DecodeParams(paramOIDs, paramFormats, paramValues) if err != nil { return nil, fmt.Errorf("parameter decoding error: %w", err) } - resolved := queryshape.SubstituteDecodedParams(query, decodedValues) + resolved := queryshape.SubstituteDecodedParams(query, decodedStrings) return dr.HandleSimpleQuery(ctx, resolved) } diff --git a/internal/stackql/paramdecoder/paramdecoder.go b/internal/stackql/paramdecoder/paramdecoder.go index 3c631838..e28d1057 100644 --- a/internal/stackql/paramdecoder/paramdecoder.go +++ b/internal/stackql/paramdecoder/paramdecoder.go @@ -12,11 +12,33 @@ import ( "github.com/lib/pq/oid" ) +// DecodedParam is one decoded parameter. SQL NULL is flagged rather than +// inferred from the value, so a client string "NULL" stays a string. +type DecodedParam interface { + GetValue() string + IsNull() bool +} + +type standardDecodedParam struct { + value string + isNull bool +} + +func newDecodedParam(value string) DecodedParam { + return &standardDecodedParam{value: value} +} + +func newNullDecodedParam() DecodedParam { + return &standardDecodedParam{value: "NULL", isNull: true} +} + +func (p *standardDecodedParam) GetValue() string { return p.value } +func (p *standardDecodedParam) IsNull() bool { return p.isNull } + // Decoder decodes raw parameter bytes according to their format codes -// and OIDs, returning string representations for each. SQL NULL is a nil -// entry, so it cannot collide with a client value. +// and OIDs, returning one DecodedParam for each. type Decoder interface { - DecodeParams(paramOIDs []uint32, paramFormats []int16, paramValues [][]byte) ([]*string, error) + DecodeParams(paramOIDs []uint32, paramFormats []int16, paramValues [][]byte) ([]DecodedParam, error) } // NewDecoder creates a new parameter decoder. @@ -28,10 +50,11 @@ type standardDecoder struct{} func (d *standardDecoder) DecodeParams( paramOIDs []uint32, paramFormats []int16, paramValues [][]byte, -) ([]*string, error) { - result := make([]*string, len(paramValues)) +) ([]DecodedParam, error) { + result := make([]DecodedParam, len(paramValues)) for i, val := range paramValues { if val == nil { + result[i] = newNullDecodedParam() continue } format := resolveFormat(paramFormats, i) @@ -43,7 +66,7 @@ func (d *standardDecoder) DecodeParams( if err != nil { return nil, fmt.Errorf("parameter $%d: %w", i+1, err) } - result[i] = &decoded + result[i] = newDecodedParam(decoded) } return result, nil } diff --git a/internal/stackql/paramdecoder/paramdecoder_test.go b/internal/stackql/paramdecoder/paramdecoder_test.go index 6be81cc1..38dd7342 100644 --- a/internal/stackql/paramdecoder/paramdecoder_test.go +++ b/internal/stackql/paramdecoder/paramdecoder_test.go @@ -19,7 +19,7 @@ func TestDecodeTextParams(t *testing.T) { if err != nil { t.Fatal(err) } - if *results[0] != "hello" || *results[1] != "world" { + if results[0].GetValue() != "hello" || results[1].GetValue() != "world" { t.Errorf("got %v", results) } } @@ -34,8 +34,8 @@ func TestDecodeNullParam(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] != nil { - t.Errorf("got %q, want nil", *results[0]) + if !results[0].IsNull() || results[0].GetValue() != "NULL" { + t.Errorf("got %q (null %t), want NULL flagged null", results[0].GetValue(), results[0].IsNull()) } } @@ -49,8 +49,8 @@ func TestDecodeTextNullStringIsNotNull(t *testing.T) { if err != nil { t.Fatal(err) } - if results[0] == nil || *results[0] != "NULL" { - t.Errorf("got %v, want the string NULL", results[0]) + if results[0].IsNull() || results[0].GetValue() != "NULL" { + t.Errorf("got %q (null %t), want the string NULL", results[0].GetValue(), results[0].IsNull()) } } @@ -66,8 +66,8 @@ func TestDecodeBinaryInt4(t *testing.T) { if err != nil { t.Fatal(err) } - if *results[0] != "42" { - t.Errorf("got %q, want 42", *results[0]) + if results[0].GetValue() != "42" { + t.Errorf("got %q, want 42", results[0].GetValue()) } } @@ -83,8 +83,8 @@ func TestDecodeBinaryInt8(t *testing.T) { if err != nil { t.Fatal(err) } - if *results[0] != "9999999999" { - t.Errorf("got %q, want 9999999999", *results[0]) + if results[0].GetValue() != "9999999999" { + t.Errorf("got %q, want 9999999999", results[0].GetValue()) } } @@ -100,8 +100,8 @@ func TestDecodeBinaryFloat8(t *testing.T) { if err != nil { t.Fatal(err) } - if *results[0] != "3.14" { - t.Errorf("got %q, want 3.14", *results[0]) + if results[0].GetValue() != "3.14" { + t.Errorf("got %q, want 3.14", results[0].GetValue()) } } @@ -115,7 +115,7 @@ func TestDecodeBinaryBool(t *testing.T) { if err != nil { t.Fatal(err) } - if *results[0] != "true" || *results[1] != "false" { + if results[0].GetValue() != "true" || results[1].GetValue() != "false" { t.Errorf("got %v", results) } } @@ -132,7 +132,7 @@ func TestDecodeMixedFormats(t *testing.T) { if err != nil { t.Fatal(err) } - if *results[0] != "hello" || *results[1] != "100" { + if results[0].GetValue() != "hello" || results[1].GetValue() != "100" { t.Errorf("got %v", results) } } @@ -147,7 +147,7 @@ func TestDecodeUnknownOIDBinaryFallsBackToText(t *testing.T) { if err != nil { t.Fatal(err) } - if *results[0] != "raw-bytes" { - t.Errorf("got %q, want raw-bytes", *results[0]) + if results[0].GetValue() != "raw-bytes" { + t.Errorf("got %q, want raw-bytes", results[0].GetValue()) } } diff --git a/internal/stackql/planbuilder/plan_builder.go b/internal/stackql/planbuilder/plan_builder.go index 8e652c9a..dfb68d00 100644 --- a/internal/stackql/planbuilder/plan_builder.go +++ b/internal/stackql/planbuilder/plan_builder.go @@ -307,7 +307,8 @@ func (pgb *standardPlanGraphBuilder) handleAuth(pbi planbuilderinput.PlanBuilder //nolint:revive // acceptable for now func(pc primitive.IPrimitiveCtx) internaldto.ExecutorOutput { authType := strings.ToLower(node.Type) - // Stage on a copy: a rejected AUTH must not alter the shared context. + // Auth works on its own clone of whatever it is given, so stage the + // client-supplied sources on a copy and keep them only on success. candidate := authCtx.Clone() if node.KeyFilePath != "" { candidate.KeyFilePath = node.KeyFilePath diff --git a/internal/stackql/psqlwire/server.go b/internal/stackql/psqlwire/server.go index 05aa44e4..a97046d6 100644 --- a/internal/stackql/psqlwire/server.go +++ b/internal/stackql/psqlwire/server.go @@ -6,8 +6,6 @@ import ( "encoding/base64" "encoding/json" "fmt" - "net" - "os" "github.com/sirupsen/logrus" "github.com/stackql/any-sdk/pkg/dto" @@ -100,21 +98,7 @@ func MakeWireServer(sbe sqlbackend.SQLBackendFactory, cfg dto.RuntimeCtx) (IWire }, nil } -// isLoopbackHost reports whether a listen host is confined to loopback. -func isLoopbackHost(host string) bool { - if host == "localhost" { - return true - } - ip := net.ParseIP(host) - return ip != nil && ip.IsLoopback() -} - func (sws *SimpleWireServer) Serve() error { - if sws.server.ClientAuth != tls.RequireAndVerifyClientCert && !isLoopbackHost(sws.rtCtx.PGSrvAddress) { - fmt.Fprintf(os.Stderr, "warning: server address %s is reachable beyond loopback "+ - "with no client authentication; set clientCAs in --%s\n", - sws.rtCtx.PGSrvAddress, dto.PgSrvRawTLSCfgKey) - } sws.logger.Info( fmt.Sprintf("PostgreSQL server is up and running at [%s:%d]", sws.rtCtx.PGSrvAddress, diff --git a/internal/stackql/queryshape/queryshape.go b/internal/stackql/queryshape/queryshape.go index 62862de4..aa8856fc 100644 --- a/internal/stackql/queryshape/queryshape.go +++ b/internal/stackql/queryshape/queryshape.go @@ -25,6 +25,7 @@ import ( "github.com/stackql/psql-wire/pkg/sqldata" "github.com/stackql/stackql-parser/go/vt/sqlparser" "github.com/stackql/stackql/internal/stackql/handler" + "github.com/stackql/stackql/internal/stackql/paramdecoder" "github.com/stackql/stackql/internal/stackql/planbuilder" "github.com/stackql/stackql/internal/stackql/sql_system" "github.com/stackql/stackql/internal/stackql/typing" @@ -183,10 +184,10 @@ func columnMetadataToSQLColumns(cols []typing.ColumnMetadata) []sqldata.ISQLColu } // SubstituteDecodedParams replaces $1, $2, ... placeholders with their -// decoded values as string literals; nil values become NULL. The query is -// scanned with the parser's own tokenizer, so a $n inside a string literal, +// decoded values as string literals; null parameters become NULL. The query +// is scanned with the parser's own tokenizer, so a $n inside a string literal, // quoted identifier or comment is left alone. -func SubstituteDecodedParams(query string, decodedValues []*string) string { +func SubstituteDecodedParams(query string, decodedValues []paramdecoder.DecodedParam) string { if len(decodedValues) == 0 { return query } @@ -235,11 +236,11 @@ func placeholderIndex(token int, val []byte) (int, bool) { // for the stackql parser, which treats them as escapes; quotes are doubled // rather than backslash-escaped because internally routed queries reach the // backing RDBMS verbatim, where only a doubled quote stays inside the literal. -func encodeParam(val *string) string { - if val == nil { +func encodeParam(param paramdecoder.DecodedParam) string { + if param.IsNull() { return "NULL" } - escaped := strings.ReplaceAll(*val, `\`, `\\`) + escaped := strings.ReplaceAll(param.GetValue(), `\`, `\\`) escaped = strings.ReplaceAll(escaped, "'", "''") return "'" + escaped + "'" } diff --git a/internal/stackql/queryshape/queryshape_test.go b/internal/stackql/queryshape/queryshape_test.go index 7e25a3cf..e6fe029f 100644 --- a/internal/stackql/queryshape/queryshape_test.go +++ b/internal/stackql/queryshape/queryshape_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/stackql/stackql-parser/go/vt/sqlparser" + "github.com/stackql/stackql/internal/stackql/paramdecoder" ) type extractTableCase struct { @@ -40,6 +41,28 @@ type SubstituteParamsCase struct { Expected string `json:"expected"` } +func (c *SubstituteParamsCase) toByteSlices() [][]byte { + result := make([][]byte, len(c.ParamValues)) + for i, v := range c.ParamValues { + if v == nil { + result[i] = nil + } else { + result[i] = []byte(*v) + } + } + return result +} + +// decodeTextParams runs text-format wire values through the real decoder. +func decodeTextParams(t *testing.T, paramValues [][]byte) []paramdecoder.DecodedParam { + t.Helper() + decoded, err := paramdecoder.NewDecoder().DecodeParams(nil, nil, paramValues) + if err != nil { + t.Fatalf("failed to decode params: %v", err) + } + return decoded +} + func TestSubstituteDecodedParams(t *testing.T) { data, err := os.ReadFile("testdata/substitute_params_cases.json") if err != nil { @@ -51,7 +74,7 @@ func TestSubstituteDecodedParams(t *testing.T) { } for _, tc := range cases { t.Run(tc.Description, func(t *testing.T) { - got := SubstituteDecodedParams(tc.Query, tc.ParamValues) + got := SubstituteDecodedParams(tc.Query, decodeTextParams(t, tc.toByteSlices())) if got != tc.Expected { t.Errorf("SubstituteDecodedParams(%q, ...) = %q, want %q", tc.Query, got, tc.Expected) } @@ -64,7 +87,7 @@ func TestSubstituteDecodedParamsStaysOneLiteral(t *testing.T) { for _, val := range []string{ `x\`, `\'`, `'`, `\\`, `a' OR 1 = 1 -- `, `"; select 1; "`, "tab\there", `\n`, "$1", } { - resolved := SubstituteDecodedParams("SELECT $1", []*string{&val}) + resolved := SubstituteDecodedParams("SELECT $1", decodeTextParams(t, [][]byte{[]byte(val)})) tokenizer := sqlparser.NewStringTokenizer(resolved) tokenizer.Scan() // SELECT token, got := tokenizer.Scan() diff --git a/pkg/mcp_server/README.md b/pkg/mcp_server/README.md index 1b286deb..28310266 100644 --- a/pkg/mcp_server/README.md +++ b/pkg/mcp_server/README.md @@ -312,7 +312,7 @@ export STACKQL_MCP_TOKEN="$(openssl rand -hex 32)" stackql mcp --mcp.config='{"server": {"transport": "http", "address": "127.0.0.1:9876", "auth_token_env_var": "STACKQL_MCP_TOKEN"}}' ``` -Binding an address beyond loopback without a token prints a warning at startup. Cross-origin browser requests are refused regardless. The stdio transport is unaffected. +Cross-origin browser requests are refused regardless. The stdio transport is unaffected. ## Server Modes diff --git a/pkg/mcp_server/policy/policy.go b/pkg/mcp_server/policy/policy.go index 3f05c58e..33f9af4e 100644 --- a/pkg/mcp_server/policy/policy.go +++ b/pkg/mcp_server/policy/policy.go @@ -7,7 +7,7 @@ import ( "regexp" "strings" - "github.com/stackql/stackql-parser/go/vt/sqlparser" + "github.com/stackql/stackql/pkg/sqlsplit" ) // Server modes. These are the legal values for Config.Server.Mode. @@ -54,14 +54,14 @@ func (c QueryClass) String() string { // whether the common table expression heads a read-only statement. var withMutationRegexp = regexp.MustCompile(`(?i)\b(INSERT|UPDATE|DELETE|REPLACE|MERGE|UPSERT|EXEC)\b`) -// ClassifyQuery returns the class of the SQL. The payload is split exactly as -// the execution layer splits it, so a mutation cannot hide behind a leading -// read-only statement; the most privileged statement decides the class. +// ClassifyQuery returns the class of the SQL. Every statement in the payload +// (see sqlsplit.Statements) is classified, so a mutation cannot hide behind a +// leading read-only statement; the most privileged statement decides the class. // Empty or unrecognised inputs return QueryClassUnknown. func ClassifyQuery(sql string) QueryClass { class := QueryClassUnknown rank := -1 - for _, stmt := range statements(sql) { + for _, stmt := range sqlsplit.Statements(sql) { stmtClass := classifyStatement(stmt) if stmtRank := classPrivilege(stmtClass); stmtRank > rank { class, rank = stmtClass, stmtRank @@ -70,25 +70,6 @@ func ClassifyQuery(sql string) QueryClass { return class } -// IsSingleStatement reports whether the SQL holds exactly one statement. -func IsSingleStatement(sql string) bool { - return len(statements(sql)) == 1 -} - -// statements splits a payload with the execution layer's own splitter and -// returns the trimmed, non-blank pieces. -func statements(sql string) []string { - // Error ignored as in the execution layer, which runs whatever pieces come back. - pieces, _ := sqlparser.SplitStatementToPieces(sql) - rv := make([]string, 0, len(pieces)) - for _, piece := range pieces { - if trimmed := strings.TrimSpace(piece); trimmed != "" { - rv = append(rv, trimmed) - } - } - return rv -} - // classPrivilege ranks classes so that GateDecision is never more permissive // for a higher rank, in any mode. func classPrivilege(class QueryClass) int { diff --git a/pkg/mcp_server/server.go b/pkg/mcp_server/server.go index f6f5c68c..327c816e 100644 --- a/pkg/mcp_server/server.go +++ b/pkg/mcp_server/server.go @@ -7,7 +7,6 @@ import ( "errors" "fmt" "io" - "net" "net/http" "os" "sync" @@ -24,6 +23,7 @@ import ( "github.com/stackql/stackql/pkg/mcp_server/policy" "github.com/stackql/stackql/pkg/mcp_server/render" "github.com/stackql/stackql/pkg/sink" + "github.com/stackql/stackql/pkg/sqlsplit" ) const ( @@ -70,10 +70,6 @@ func (s *simpleMCPServer) runHTTPServer(server *mcp.Server, config *Config) erro if authErr != nil { return authErr } - if config.Server.AuthTokenEnvVar == "" && !isLoopbackAddress(address) { - fmt.Fprintf(stderrSink(), "warning: MCP server address %s is reachable beyond loopback "+ - "with no client authentication; set server.auth_token_env_var\n", address) - } handlerWithLogging := loggingHandler(securedHandler, s.logger) s.logger.Debugf("MCP server listening on %s", address) @@ -118,19 +114,6 @@ func secureHTTPHandler(handler http.Handler, config *Config) (http.Handler, erro return auth.RequireBearerToken(verifier, opts)(handler), nil } -// isLoopbackAddress reports whether a listen address is confined to loopback. -func isLoopbackAddress(address string) bool { - host, _, err := net.SplitHostPort(address) - if err != nil { - return false - } - if host == "localhost" { - return true - } - ip := net.ParseIP(host) - return ip != nil && ip.IsLoopback() -} - // addPromptIfEnabled registers a prompt only when cfg.IsPromptEnabled allows it. func addPromptIfEnabled(s *mcp.Server, cfg *Config, p *mcp.Prompt, h mcp.PromptHandler) { if !cfg.IsPromptEnabled(p.Name) { @@ -465,7 +448,7 @@ func registerTools(server *mcp.Server, cfg *Config, backend Backend, logger *log return nil, dto.ValidationResultDTO{}, formatErr } // Only the first statement would sit inside the backend's EXPLAIN. - if !policy.IsSingleStatement(args.SQL) { + if len(sqlsplit.Statements(args.SQL)) != 1 { return nil, dto.ValidationResultDTO{}, fmt.Errorf("validate_select_query accepts exactly one statement") } rowsBack, err := backend.ValidateQuery(ctx, args.SQL) diff --git a/pkg/mcp_server/server_test.go b/pkg/mcp_server/server_test.go index d9534bca..9ef31ffd 100644 --- a/pkg/mcp_server/server_test.go +++ b/pkg/mcp_server/server_test.go @@ -237,14 +237,3 @@ func TestSecureHTTPHandler(t *testing.T) { } } } - -func TestIsLoopbackAddress(t *testing.T) { - for address, want := range map[string]bool{ - "127.0.0.1:9876": true, "localhost:9876": true, "[::1]:9876": true, - "0.0.0.0:9876": false, ":9876": false, "192.168.1.10:9876": false, "example.com:9876": false, - } { - if got := isLoopbackAddress(address); got != want { - t.Errorf("isLoopbackAddress(%q) = %v, want %v", address, got, want) - } - } -} diff --git a/pkg/sqlsplit/sqlsplit.go b/pkg/sqlsplit/sqlsplit.go new file mode 100644 index 00000000..80c793bf --- /dev/null +++ b/pkg/sqlsplit/sqlsplit.go @@ -0,0 +1,22 @@ +// Package sqlsplit splits SQL text into statements the way the execution +// layer does, so callers reason about the same statements that will run. +package sqlsplit + +import ( + "strings" + + "github.com/stackql/stackql-parser/go/vt/sqlparser" +) + +// Statements returns the trimmed, non-blank statements in a payload. +func Statements(sql string) []string { + // Error ignored as in the execution layer, which runs whatever pieces come back. + pieces, _ := sqlparser.SplitStatementToPieces(sql) + rv := make([]string, 0, len(pieces)) + for _, piece := range pieces { + if trimmed := strings.TrimSpace(piece); trimmed != "" { + rv = append(rv, trimmed) + } + } + return rv +} diff --git a/pkg/sqlsplit/sqlsplit_test.go b/pkg/sqlsplit/sqlsplit_test.go new file mode 100644 index 00000000..b9757cf5 --- /dev/null +++ b/pkg/sqlsplit/sqlsplit_test.go @@ -0,0 +1,31 @@ +package sqlsplit_test + +import ( + "reflect" + "testing" + + "github.com/stackql/stackql/pkg/sqlsplit" +) + +func TestStatements(t *testing.T) { + cases := []struct { + sql string + want []string + }{ + {"", []string{}}, + {" ", []string{}}, + {";", []string{}}, + {"select 1", []string{"select 1"}}, + {"select 1;", []string{"select 1"}}, + {" select 1 ;\n delete from t ; ", []string{"select 1", "delete from t"}}, + {"select ';' as semi", []string{"select ';' as semi"}}, // quoted separator does not split + {"select 1 /* ; */; select 2", []string{"select 1 /* ; */", "select 2"}}, + } + for _, c := range cases { + t.Run(c.sql, func(t *testing.T) { + if got := sqlsplit.Statements(c.sql); !reflect.DeepEqual(got, c.want) { + t.Errorf("Statements(%q) = %q, want %q", c.sql, got, c.want) + } + }) + } +} diff --git a/test/robot/functional/stackql_sessions.robot b/test/robot/functional/stackql_sessions.robot index 816883f8..e3225d7f 100644 --- a/test/robot/functional/stackql_sessions.robot +++ b/test/robot/functional/stackql_sessions.robot @@ -96,27 +96,6 @@ Shell Session Invalid Registry Config Exits Cleanly Should Not Contain ${result.stderr} panic [Teardown] NONE -PG Server Warns When Exposed Without Client Authentication - [Documentation] Binding beyond loopback with no client CAs warns on stderr at startup; a loopback bind does not. - Pass Execution If "${EXECUTION_PLATFORM}" == "docker" Native server start only - ${exposed} = Run Process ${STACKQL_EXE} - ... srv \-\-registry\=${REGISTRY_NO_VERIFY_CFG_STR.get_config_str('native')} - ... \-\-pgsrv\.address\=0.0.0.0 - ... \-\-pgsrv\.port\=5667 - ... timeout=10s on_timeout=kill - ... stdout=${CURDIR}/tmp/PG-Server-Warns-When-Exposed.tmp - ... stderr=${CURDIR}/tmp/PG-Server-Warns-When-Exposed-stderr.tmp - Should Contain ${exposed.stderr} reachable beyond loopback with no client authentication - ${loopback} = Run Process ${STACKQL_EXE} - ... srv \-\-registry\=${REGISTRY_NO_VERIFY_CFG_STR.get_config_str('native')} - ... \-\-pgsrv\.address\=127.0.0.1 - ... \-\-pgsrv\.port\=5668 - ... timeout=10s on_timeout=kill - ... stdout=${CURDIR}/tmp/PG-Server-Warns-When-Exposed-loopback.tmp - ... stderr=${CURDIR}/tmp/PG-Server-Warns-When-Exposed-loopback-stderr.tmp - Should Not Contain ${loopback.stderr} no client authentication - [Teardown] NONE - PG Session GC Manual Behaviour Canonical [Tags] shard-group:pg-server-generation Should PG Client Session Inline Equal Strict From 95e9434c26661d9e979625785b1181e417bb4ba3 Mon Sep 17 00:00:00 2001 From: Jeffrey Aven Date: Sat, 3 Oct 2026 12:19:53 +1000 Subject: [PATCH 5/6] address review feedback, round 2 - MCP HTTP: serving without client authentication is an explicit opt-in (server.allow_unauthenticated); with neither that nor server.auth_token_env_var the HTTP transport refuses to start. - MCP HTTP: the pre-shared token is checked by a plain constant-time bearer middleware; no claimless SDK TokenInfo. - AUTH: robot scenario proving a successful AUTH LOGIN still changes the shared credential source, alongside the rejected-AUTH scenario. - robot servers, contributor docs and launch configs opt in explicitly. Co-Authored-By: Claude Fable 5.1 --- .vscode/launch.json | 6 +- docs/mcp.md | 16 +++--- pkg/mcp_server/README.md | 9 +-- pkg/mcp_server/config.go | 7 +++ pkg/mcp_server/server.go | 34 +++++++---- pkg/mcp_server/server_test.go | 19 +++++-- test/robot/functional/mcp.robot | 56 +++++++++++++------ .../stackql_mocked_from_cmd_line.robot | 16 ++++++ 8 files changed, 113 insertions(+), 50 deletions(-) diff --git a/.vscode/launch.json b/.vscode/launch.json index e918f076..b5c4b933 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -207,9 +207,9 @@ "id": "mcpSrvCfgStr", "description": "MCP Server Configuration", "options": [ - "{\"server\": {\"transport\": \"http\", \"address\": \"127.0.0.1:9992\"} }" + "{\"server\": {\"transport\": \"http\", \"address\": \"127.0.0.1:9992\", \"allow_unauthenticated\": true} }" ], - "default": "{\"server\": {\"transport\": \"http\", \"address\": \"127.0.0.1:9992\"} }" + "default": "{\"server\": {\"transport\": \"http\", \"address\": \"127.0.0.1:9992\", \"allow_unauthenticated\": true} }" }, { "type": "pickString", @@ -548,7 +548,7 @@ "--export.alias=${input:exportAliasString}", "--pgsrv.debug.enable=${input:serverDebugPublish}", "--mcp.server.type=reverse_proxy", - "--mcp.config={\"server\": {\"transport\": \"http\", \"address\": \"127.0.0.1:9914\"}, \"backend\": {\"dsn\": \"postgres://stackql:stackql@127.0.0.1:5885?default_query_exec_mode=simple_protocol&standard_conforming_strings=on&client_encoding=UTF8\"}}" + "--mcp.config={\"server\": {\"transport\": \"http\", \"address\": \"127.0.0.1:9914\", \"allow_unauthenticated\": true}, \"backend\": {\"dsn\": \"postgres://stackql:stackql@127.0.0.1:5885?default_query_exec_mode=simple_protocol&standard_conforming_strings=on&client_encoding=UTF8\"}}" ], }, { diff --git a/docs/mcp.md b/docs/mcp.md index 9dbb08af..918568c9 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -14,7 +14,7 @@ We have a nice debug config for running an MCP server with `vscode`, please see ```bash -./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992"} }' +./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "allow_unauthenticated": true} }' ``` @@ -26,7 +26,7 @@ Read-only (mutations and lifecycle refused immediately): ```bash -./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "mode": "read_only"} }' +./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "allow_unauthenticated": true, "mode": "read_only"} }' ``` @@ -34,7 +34,7 @@ Delete-safe (INSERTs/UPDATEs proceed; DELETEs and EXECs need approval): ```bash -./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "mode": "delete_safe"} }' +./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "allow_unauthenticated": true, "mode": "delete_safe"} }' ``` @@ -42,7 +42,7 @@ Full access (everything proceeds without approval - use only when the client and ```bash -./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "mode": "full_access"} }' +./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "allow_unauthenticated": true, "mode": "full_access"} }' ``` @@ -54,7 +54,7 @@ Audit is on by default. To opt out: ```bash -./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "audit": {"disabled": true}} }' +./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "allow_unauthenticated": true, "audit": {"disabled": true}} }' ``` @@ -210,7 +210,7 @@ Transport behaviour: - **Streamable HTTP** defaults to the stateful, session-per-client model (`Mcp-Session-Id`), which the SDK serves for revisions up to `2025-11-25`; a `2026-07-28` request is answered with JSON-RPC error `-32022` (`UnsupportedProtocolVersion`, SDK v1.8.0; a plain HTTP 400 before) whose `data.supported` lists the handshake revisions, and `server/discover` advertises the same, so the client negotiates down and existing HTTP hosts keep their sessions and elicitation unchanged. Set `"stateless": true` in `server` (or pin `protocol_version` to `2026-07-28`, below) to serve `2026-07-28` natively: no `Mcp-Session-Id` is issued or read, `GET` and `DELETE` answer 405 (there is no session to tear down), `tools/list` / `prompts/list` / `resources/list` are connection-invariant, and gated writes use the input-required round trip. A sessionless server still accepts a legacy `initialize` and serves reads to that client, but cannot retain the elicitation capability a legacy client declared at initialise (the SDK gives each request an ephemeral session), so legacy HTTP clients cannot approve gated writes on it. Pick stateless for current-revision hosts, stateful for a mixed legacy fleet. ```bash -./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "stateless": true} }' +./build/stackql mcp --mcp.server.type=http --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "allow_unauthenticated": true, "stateless": true} }' ``` The server holds no cross-call state: mode, audit and provider auth are process-level configuration, so nothing needed to move behind explicit handles (SEP-2567). The robot suite drives stdio and both HTTP models with a 2025-06-18 handshake client and a 2026-07-28 stateless client, including the gated write on each revision. @@ -220,7 +220,7 @@ The server holds no cross-call state: mode, audit and provider auth are process- By default the server advertises every revision the SDK supports and negotiates per client. To narrow that: ```bash -./build/stackql mcp --mcp.server.type=http --mcp.protocol.version=2026-07-28 --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992"} }' +./build/stackql mcp --mcp.server.type=http --mcp.protocol.version=2026-07-28 --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "allow_unauthenticated": true} }' ./build/stackql mcp --mcp.server.type=stdio --mcp.protocol.version=2025-11-25 ``` @@ -271,7 +271,7 @@ The only sink shipped in this release is `file`. One JSON object per line, fsyn ```bash ./build/stackql mcp \ --mcp.server.type=http \ - --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "audit": {"file": {"path": "/var/log/stackql-mcp.log", "max_size_mb": 100, "max_backups": 5, "max_age_days": 30}}} }' + --mcp.config '{"server": {"transport": "http", "address": "127.0.0.1:9992", "allow_unauthenticated": true, "audit": {"file": {"path": "/var/log/stackql-mcp.log", "max_size_mb": 100, "max_backups": 5, "max_age_days": 30}}} }' ``` Two ways to specify the location: diff --git a/pkg/mcp_server/README.md b/pkg/mcp_server/README.md index 28310266..8b6f9a8c 100644 --- a/pkg/mcp_server/README.md +++ b/pkg/mcp_server/README.md @@ -295,24 +295,25 @@ JSON example — a single-purpose server that exposes only `server_info`: { "server": { "transport": "http", - "address": "127.0.0.1:9915" + "address": "127.0.0.1:9915", + "allow_unauthenticated": true }, "enabled_tools": ["server_info"] } ``` -When the server is launched via the `stackql mcp` (or `stackql srv --mcp.server.type=...`) command, these fields are parsed from the same `--mcp.config` JSON blob as the rest of the configuration — no additional flag is required. For example, `stackql mcp --mcp.config='{"server": { "transport": "http", "address": "127.0.0.1:9915"}, "enabled_tools": ["server_info"]}'`. +When the server is launched via the `stackql mcp` (or `stackql srv --mcp.server.type=...`) command, these fields are parsed from the same `--mcp.config` JSON blob as the rest of the configuration — no additional flag is required. For example, `stackql mcp --mcp.config='{"server": { "transport": "http", "address": "127.0.0.1:9915", "allow_unauthenticated": true}, "enabled_tools": ["server_info"]}'`. ## HTTP Client Authentication -The HTTP transport is unauthenticated unless `server.auth_token_env_var` names an environment variable holding a bearer token. When set, every request must carry `Authorization: Bearer ` or it is answered with `401`; the server refuses to start if the variable is unset or empty. The token is read from the environment so it never appears in `--mcp.config` or the process arguments. +The HTTP transport requires client authentication: `server.auth_token_env_var` names an environment variable holding a pre-shared bearer token, and every request must carry `Authorization: Bearer ` or it is answered with `401`. The server refuses to start if the variable is unset or empty. The token is read from the environment so it never appears in `--mcp.config` or the process arguments. ```sh export STACKQL_MCP_TOKEN="$(openssl rand -hex 32)" stackql mcp --mcp.config='{"server": {"transport": "http", "address": "127.0.0.1:9876", "auth_token_env_var": "STACKQL_MCP_TOKEN"}}' ``` -Cross-origin browser requests are refused regardless. The stdio transport is unaffected. +Serving HTTP with no client authentication is an explicit opt-in, `"allow_unauthenticated": true`; without a token or the opt-in the HTTP transport refuses to start. Cross-origin browser requests are refused regardless. The stdio transport is unaffected. ## Server Modes diff --git a/pkg/mcp_server/config.go b/pkg/mcp_server/config.go index 938ae0e6..7927005f 100644 --- a/pkg/mcp_server/config.go +++ b/pkg/mcp_server/config.go @@ -210,6 +210,11 @@ type ServerConfig struct { // HTTP transport requires `Authorization: Bearer ` on every request. AuthTokenEnvVar string `json:"auth_token_env_var,omitempty" yaml:"auth_token_env_var,omitempty"` + // AllowUnauthenticated is the explicit opt-in to serve the HTTP transport + // with no client authentication. Without it or AuthTokenEnvVar the HTTP + // transport refuses to start. + AllowUnauthenticated bool `json:"allow_unauthenticated,omitempty" yaml:"allow_unauthenticated,omitempty"` + TransportCfg map[string]any `json:"transport_cfg,omitempty" yaml:"transport_cfg,omitempty"` // Stateless serves Streamable HTTP without sessions (no Mcp-Session-Id), @@ -266,6 +271,7 @@ type serverConfigWire struct { TLSCertFile string `json:"tls_cert_file,omitempty" yaml:"tls_cert_file,omitempty"` TLSKeyFile string `json:"tls_key_file,omitempty" yaml:"tls_key_file,omitempty"` AuthTokenEnvVar string `json:"auth_token_env_var,omitempty" yaml:"auth_token_env_var,omitempty"` + AllowUnauthenticated bool `json:"allow_unauthenticated,omitempty" yaml:"allow_unauthenticated,omitempty"` TransportCfg map[string]any `json:"transport_cfg,omitempty" yaml:"transport_cfg,omitempty"` Stateless bool `json:"stateless,omitempty" yaml:"stateless,omitempty"` ProtocolVersion string `json:"protocol_version,omitempty" yaml:"protocol_version,omitempty"` @@ -288,6 +294,7 @@ func (s *ServerConfig) fromWire(w serverConfigWire) { s.TLSCertFile = w.TLSCertFile s.TLSKeyFile = w.TLSKeyFile s.AuthTokenEnvVar = w.AuthTokenEnvVar + s.AllowUnauthenticated = w.AllowUnauthenticated s.TransportCfg = w.TransportCfg s.Stateless = w.Stateless s.ProtocolVersion = w.ProtocolVersion diff --git a/pkg/mcp_server/server.go b/pkg/mcp_server/server.go index 327c816e..beb94552 100644 --- a/pkg/mcp_server/server.go +++ b/pkg/mcp_server/server.go @@ -9,13 +9,13 @@ import ( "io" "net/http" "os" + "strings" "sync" "time" "github.com/sirupsen/logrus" "golang.org/x/sync/semaphore" - "github.com/modelcontextprotocol/go-sdk/auth" "github.com/modelcontextprotocol/go-sdk/mcp" "github.com/stackql/stackql/pkg/mcp_server/audit" @@ -91,27 +91,39 @@ func (s *simpleMCPServer) runHTTPServer(server *mcp.Server, config *Config) erro return nil } -// secureHTTPHandler adds cross-origin protection and, when -// server.auth_token_env_var is set, bearer token authentication. +// secureHTTPHandler adds cross-origin protection and client authentication. +// The HTTP transport requires the pre-shared token named by +// server.auth_token_env_var; serving without one is an explicit opt-in via +// server.allow_unauthenticated. func secureHTTPHandler(handler http.Handler, config *Config) (http.Handler, error) { handler = http.NewCrossOriginProtection().Handler(handler) envVar := config.Server.AuthTokenEnvVar if envVar == "" { + if !config.Server.AllowUnauthenticated { + return nil, fmt.Errorf("the HTTP transport requires server.auth_token_env_var, " + + "or server.allow_unauthenticated: true to serve without client authentication") + } return handler, nil } token := os.Getenv(envVar) if token == "" { return nil, fmt.Errorf("server.auth_token_env_var names an unset or empty env var %q", envVar) } - verifier := func(_ context.Context, presented string, _ *http.Request) (*auth.TokenInfo, error) { - if subtle.ConstantTimeCompare([]byte(presented), []byte(token)) != 1 { - return nil, auth.ErrInvalidToken + return requireBearerToken(token, handler), nil +} + +// requireBearerToken answers 401 unless the request presents the pre-shared token. +func requireBearerToken(token string, next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fields := strings.Fields(r.Header.Get("Authorization")) + if len(fields) != 2 || !strings.EqualFold(fields[0], "Bearer") || + subtle.ConstantTimeCompare([]byte(fields[1]), []byte(token)) != 1 { + w.Header().Set("WWW-Authenticate", "Bearer") + http.Error(w, "unauthorized", http.StatusUnauthorized) + return } - return &auth.TokenInfo{}, nil - } - // A static token carries no expiry. - opts := &auth.RequireBearerTokenOptions{AllowMissingExpiration: true} - return auth.RequireBearerToken(verifier, opts)(handler), nil + next.ServeHTTP(w, r) + }) } // addPromptIfEnabled registers a prompt only when cfg.IsPromptEnabled allows it. diff --git a/pkg/mcp_server/server_test.go b/pkg/mcp_server/server_test.go index 9ef31ffd..5103662d 100644 --- a/pkg/mcp_server/server_test.go +++ b/pkg/mcp_server/server_test.go @@ -199,12 +199,17 @@ func TestSecureHTTPHandler(t *testing.T) { return rec.Code } - open, err := secureHTTPHandler(ok, DefaultHTTPConfig()) + if _, err := secureHTTPHandler(ok, DefaultHTTPConfig()); err == nil { + t.Error("no token and no opt-in must fail startup") + } + optedIn := DefaultHTTPConfig() + optedIn.Server.AllowUnauthenticated = true + open, err := secureHTTPHandler(ok, optedIn) if err != nil { - t.Fatalf("no token configured: %v", err) + t.Fatalf("opted into unauthenticated service: %v", err) } if got := status(open, nil); got != http.StatusOK { - t.Errorf("no token configured: status %d, want 200", got) + t.Errorf("opted into unauthenticated service: status %d, want 200", got) } if got := status(open, map[string]string{"Origin": "https://evil.example"}); got != http.StatusForbidden { t.Errorf("cross-origin request: status %d, want 403", got) @@ -224,9 +229,11 @@ func TestSecureHTTPHandler(t *testing.T) { header string want int }{ - "missing": {"", http.StatusUnauthorized}, - "wrong": {"Bearer nope", http.StatusUnauthorized}, - "right": {"Bearer s3cret", http.StatusOK}, + "missing": {"", http.StatusUnauthorized}, + "wrong": {"Bearer nope", http.StatusUnauthorized}, + "no scheme": {"s3cret", http.StatusUnauthorized}, + "right": {"Bearer s3cret", http.StatusOK}, + "scheme case": {"bearer s3cret", http.StatusOK}, } { headers := map[string]string{} if tc.header != "" { diff --git a/test/robot/functional/mcp.robot b/test/robot/functional/mcp.robot index db67a975..c059ad61 100644 --- a/test/robot/functional/mcp.robot +++ b/test/robot/functional/mcp.robot @@ -10,7 +10,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9912", "mode": "full_access", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9912", "mode": "full_access", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -20,7 +20,7 @@ Start MCP Servers ... srv ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9913", "mode": "full_access", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9913", "mode": "full_access", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -32,7 +32,7 @@ Start MCP Servers ... srv ... \-\-mcp.server.type\=reverse_proxy ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9914", "mode": "full_access", "audit": {"disabled": true}}, "backend": {"dsn": "postgres:\/\/stackql:stackql@127.0.0.1:5445?default_query_exec_mode\=simple_protocol"} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9914", "mode": "full_access", "audit": {"disabled": true}}, "backend": {"dsn": "postgres:\/\/stackql:stackql@127.0.0.1:5445?default_query_exec_mode\=simple_protocol"} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -44,7 +44,7 @@ Start MCP Servers ... srv ... \-\-mcp.server.type\=reverse_proxy ... \-\-mcp.config - ... {"server": {"tls_cert_file": "test/server/mtls/credentials/pg_server_cert.pem", "tls_key_file": "test/server/mtls/credentials/pg_server_key.pem", "transport": "http", "address": "127.0.0.1:9004", "mode": "full_access", "audit": {"disabled": true}}, "backend": {"dsn": "postgres:\/\/stackql:stackql@127.0.0.1:5446?default_query_exec_mode\=simple_protocol"} } + ... {"server": {"tls_cert_file": "test/server/mtls/credentials/pg_server_cert.pem", "tls_key_file": "test/server/mtls/credentials/pg_server_key.pem", "transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9004", "mode": "full_access", "audit": {"disabled": true}}, "backend": {"dsn": "postgres:\/\/stackql:stackql@127.0.0.1:5446?default_query_exec_mode\=simple_protocol"} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -58,7 +58,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9915", "mode": "full_access", "audit": {"disabled": true}}, "enabled_tools": ["server_info"] } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9915", "mode": "full_access", "audit": {"disabled": true}}, "enabled_tools": ["server_info"] } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -70,7 +70,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9916", "read_only": true, "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9916", "read_only": true, "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -82,7 +82,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9917", "mode": "full_access", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9917", "mode": "full_access", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -97,7 +97,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9920", "mode": "read_only", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9920", "mode": "read_only", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -109,7 +109,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9921", "mode": "delete_safe", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9921", "mode": "delete_safe", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -121,7 +121,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9922", "mode": "full_access", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9922", "mode": "full_access", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -135,7 +135,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9923", "mode": "full_access", "audit": {"file": {"path": "mcp-audit-9923.log"}}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9923", "mode": "full_access", "audit": {"file": {"path": "mcp-audit-9923.log"}}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -149,7 +149,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9924", "mode": "full_access", "render": "json", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9924", "mode": "full_access", "render": "json", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -164,7 +164,7 @@ Start MCP Servers ... \-\-mcp.server.type\=http ... \-\-mcp.log.format\=otel ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9925", "mode": "full_access", "audit": {"file": {"path": "mcp-audit-otel-9925.log"}}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9925", "mode": "full_access", "audit": {"file": {"path": "mcp-audit-otel-9925.log"}}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -178,7 +178,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9926", "stateless": true, "mode": "safe", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9926", "stateless": true, "mode": "safe", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -192,7 +192,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9927", "mode": "full_access", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9927", "mode": "full_access", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_MOCKED_CFG_STR.get_config_str('native')} ... \-\-approot @@ -211,7 +211,7 @@ Start MCP Servers ... \-\-mcp.server.type\=http ... \-\-mcp.protocol.version\=2026-07-28 ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9928", "mode": "safe", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9928", "mode": "safe", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -223,7 +223,7 @@ Start MCP Servers ... mcp ... \-\-mcp.server.type\=http ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9929", "protocol_version": "2025-11-25", "mode": "full_access", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9929", "protocol_version": "2025-11-25", "mode": "full_access", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -1824,7 +1824,7 @@ MCP Server Refuses Unsupported Protocol Version ... \-\-mcp.server.type\=http ... \-\-mcp.protocol.version\=2020-01-01 ... \-\-mcp.config - ... {"server": {"transport": "http", "address": "127.0.0.1:9930", "audit": {"disabled": true}} } + ... {"server": {"transport": "http", "allow_unauthenticated": true, "address": "127.0.0.1:9930", "audit": {"disabled": true}} } ... \-\-registry ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} ... \-\-auth @@ -1873,3 +1873,23 @@ MCP Server Refuses Unset Bearer Token Env Var ... stderr=${CURDIR}${/}tmp${/}MCP-Unset-Bearer-Token-stderr.txt Should Not Be Equal As Integers ${result.rc} 0 Should Contain ${result.stderr} server.auth_token_env_var names an unset or empty env var + +MCP Server Refuses Unauthenticated HTTP Without Opt In + [Documentation] The HTTP transport needs server.auth_token_env_var, or the explicit + ... server.allow_unauthenticated opt-in; with neither it refuses to start. + Pass Execution If "%{IS_SKIP_MCP_TEST=false}" == "true" Some platforms do not have the MCP client available + ${result}= Run Process ${STACKQL_EXE} + ... mcp + ... \-\-mcp.server.type\=http + ... \-\-mcp.config + ... {"server": {"transport": "http", "address": "127.0.0.1:9933", "audit": {"disabled": true}} } + ... \-\-registry + ... ${REGISTRY_NO_VERIFY_CFG_JSON_STR} + ... \-\-auth + ... ${AUTH_CFG_STR} + ... \-\-tls.allowInsecure + ... timeout=90s + ... stdout=${CURDIR}${/}tmp${/}MCP-Unauthenticated-Without-Opt-In.txt + ... stderr=${CURDIR}${/}tmp${/}MCP-Unauthenticated-Without-Opt-In-stderr.txt + Should Not Be Equal As Integers ${result.rc} 0 + Should Contain ${result.stderr} requires server.auth_token_env_var diff --git a/test/robot/functional/stackql_mocked_from_cmd_line.robot b/test/robot/functional/stackql_mocked_from_cmd_line.robot index 369d8584..8c58c0ec 100644 --- a/test/robot/functional/stackql_mocked_from_cmd_line.robot +++ b/test/robot/functional/stackql_mocked_from_cmd_line.robot @@ -11803,3 +11803,19 @@ Rejected Auth Login Leaves Credential Source Unchanged ... service account credentials error ... stdout=${CURDIR}/tmp/Rejected-Auth-Login-Leaves-Credential-Source-Unchanged.tmp ... stderr=${CURDIR}/tmp/Rejected-Auth-Login-Leaves-Credential-Source-Unchanged-stderr.tmp + +Successful Auth Login Changes Credential Source + [Documentation] The configured google credential path does not exist, so the SELECT can only succeed because the preceding AUTH LOGIN changed the shared credential source. + Should Stackql Exec Inline Contain Both Streams + ... ${STACKQL_EXE} + ... ${OKTA_SECRET_STR} + ... ${GITHUB_SECRET_STR} + ... ${K8S_SECRET_STR} + ... ${REGISTRY_NO_VERIFY_CFG_STR} + ... ${AUTH_CFG_DEFECTIVE_STR} + ... ${SQL_BACKEND_CFG_STR_CANONICAL} + ... auth login google sa '${AUTH_GOOGLE_SA_KEY_PATH}' ''; select name from google.storage.buckets where project \= 'stackql-demo' order by name desc; + ... stackql-demo-src-bucket + ... ${EMPTY} + ... stdout=${CURDIR}/tmp/Successful-Auth-Login-Changes-Credential-Source.tmp + ... stderr=${CURDIR}/tmp/Successful-Auth-Login-Changes-Credential-Source-stderr.tmp From 13e2aae17dc5606238d096b4fc82ae22f58da2e2 Mon Sep 17 00:00:00 2001 From: Jeffrey Aven Date: Sat, 3 Oct 2026 19:21:59 +1000 Subject: [PATCH 6/6] revert AUTH staging; session isolation is out of scope Co-Authored-By: Claude Fable 5.1 --- internal/stackql/planbuilder/plan_builder.go | 13 ++------ .../stackql_mocked_from_cmd_line.robot | 32 ------------------- 2 files changed, 3 insertions(+), 42 deletions(-) diff --git a/internal/stackql/planbuilder/plan_builder.go b/internal/stackql/planbuilder/plan_builder.go index dfb68d00..6908e278 100644 --- a/internal/stackql/planbuilder/plan_builder.go +++ b/internal/stackql/planbuilder/plan_builder.go @@ -307,20 +307,13 @@ func (pgb *standardPlanGraphBuilder) handleAuth(pbi planbuilderinput.PlanBuilder //nolint:revive // acceptable for now func(pc primitive.IPrimitiveCtx) internaldto.ExecutorOutput { authType := strings.ToLower(node.Type) - // Auth works on its own clone of whatever it is given, so stage the - // client-supplied sources on a copy and keep them only on success. - candidate := authCtx.Clone() if node.KeyFilePath != "" { - candidate.KeyFilePath = node.KeyFilePath + authCtx.KeyFilePath = node.KeyFilePath } if node.KeyEnvVar != "" { - candidate.KeyEnvVar = node.KeyEnvVar - } - _, err = prov.Auth(candidate, authType, true) - if err == nil { - authCtx.KeyFilePath = candidate.KeyFilePath - authCtx.KeyEnvVar = candidate.KeyEnvVar + authCtx.KeyEnvVar = node.KeyEnvVar } + _, err = prov.Auth(authCtx, authType, true) return internaldto.NewExecutorOutput(nil, nil, nil, nil, err) }) pgb.planGraphHolder.GetPrimitiveGraph().CreatePrimitiveNode(pr) diff --git a/test/robot/functional/stackql_mocked_from_cmd_line.robot b/test/robot/functional/stackql_mocked_from_cmd_line.robot index 8c58c0ec..c23d8225 100644 --- a/test/robot/functional/stackql_mocked_from_cmd_line.robot +++ b/test/robot/functional/stackql_mocked_from_cmd_line.robot @@ -11787,35 +11787,3 @@ Preview Iac Resource Without Provider Is Refused By Omnisdk ... --preview\=${preview} ... stdout=${CURDIR}${/}tmp${/}Preview-Iac-Resource-No-Provider.tmp ... stderr=${CURDIR}${/}tmp${/}Preview-Iac-Resource-No-Provider-stderr.tmp - -Rejected Auth Login Leaves Credential Source Unchanged - [Documentation] A failing AUTH LOGIN must not redirect where later statements source the provider credential. - Should Stackql Exec Inline Contain Both Streams - ... ${STACKQL_EXE} - ... ${OKTA_SECRET_STR} - ... ${GITHUB_SECRET_STR} - ... ${K8S_SECRET_STR} - ... ${REGISTRY_NO_VERIFY_CFG_STR} - ... ${AUTH_CFG_STR} - ... ${SQL_BACKEND_CFG_STR_CANONICAL} - ... auth login google sa '/no/such/sa-key.json' ''; select name from google.storage.buckets where project \= 'stackql-demo' order by name desc; - ... stackql-demo-src-bucket - ... service account credentials error - ... stdout=${CURDIR}/tmp/Rejected-Auth-Login-Leaves-Credential-Source-Unchanged.tmp - ... stderr=${CURDIR}/tmp/Rejected-Auth-Login-Leaves-Credential-Source-Unchanged-stderr.tmp - -Successful Auth Login Changes Credential Source - [Documentation] The configured google credential path does not exist, so the SELECT can only succeed because the preceding AUTH LOGIN changed the shared credential source. - Should Stackql Exec Inline Contain Both Streams - ... ${STACKQL_EXE} - ... ${OKTA_SECRET_STR} - ... ${GITHUB_SECRET_STR} - ... ${K8S_SECRET_STR} - ... ${REGISTRY_NO_VERIFY_CFG_STR} - ... ${AUTH_CFG_DEFECTIVE_STR} - ... ${SQL_BACKEND_CFG_STR_CANONICAL} - ... auth login google sa '${AUTH_GOOGLE_SA_KEY_PATH}' ''; select name from google.storage.buckets where project \= 'stackql-demo' order by name desc; - ... stackql-demo-src-bucket - ... ${EMPTY} - ... stdout=${CURDIR}/tmp/Successful-Auth-Login-Changes-Credential-Source.tmp - ... stderr=${CURDIR}/tmp/Successful-Auth-Login-Changes-Credential-Source-stderr.tmp