Repository navigation
security-hardening - #804
Conversation
- 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 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
This change is functionally nil; not required
There was a problem hiding this comment.
If a nullable type is needed on the return value line items then create an abstraction. You are changing the behaviour in terms of response cardinality and i see no justification given the function signature and prior behaviour.
| authCtx.KeyFilePath = candidate.KeyFilePath | ||
| authCtx.KeyEnvVar = candidate.KeyEnvVar | ||
| } | ||
| _, err = prov.Auth(authCtx, authType, true) |
There was a problem hiding this comment.
Why are you discading this, which appears to have a significant side effect.
Functional equivalence, inclusive of side effects needs to be proven in tests. Please tell me where this is proven.
|
|
||
| // isLoopbackHost reports whether a listen host is confined to loopback. | ||
| func isLoopbackHost(host string) bool { | ||
| if host == "localhost" { |
There was a problem hiding this comment.
I'm pretty sure this can be hacked at least in posix systems, so is meaningless
| // 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 |
There was a problem hiding this comment.
This analysis is unrealted to mcp and therefore should not be in this package, even if mcp server is the sole caller at this time
- 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 <noreply@anthropic.com>
| if subtle.ConstantTimeCompare([]byte(presented), []byte(token)) != 1 { | ||
| return nil, auth.ErrInvalidToken | ||
| } | ||
| return &auth.TokenInfo{}, nil |
There was a problem hiding this comment.
I cannot imagine why this structure should be empty. Please explain or rectify. We would like to use accepted idioms.
| handler = http.NewCrossOriginProtection().Handler(handler) | ||
| envVar := config.Server.AuthTokenEnvVar | ||
| if envVar == "" { | ||
| return handler, nil |
There was a problem hiding this comment.
Insecure transport shouild be opt in with config, not simply empty env var
| //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 |
There was a problem hiding this comment.
what exactly are the changes to this method fixing? Looks to be at best creating an auth stack where one reverts to last successful on failure. Please consicely justify this behaviour or simply leave this unchanged.
NOTE: do not add a wall of comments here. A small PR comment which is consise is fine.
- 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 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Summary
Security hardening for the
srv(Postgres wire) andmcpserver surfaces, following an AI-assisted source audit by the GitHub Security Lab. Each item below is a fix that fits existing conventions; items that need a design decision are listed separately and are not changed here.;-separated statement, so a mutation could follow a leadingSELECT.policy.ClassifyQuerynow classifies every statement and gates on the most privileged one. The splitting lives in a new general package,pkg/sqlsplit, which wraps the executor's own splitter (sqlparser.SplitStatementToPieces).validate_select_queryEXPLAIN. Keeps the tool's read-only hint truthful.provider/service/resource/methodare interpolated intoSHOW/DESCRIBE; they now go through the existing forbidden-character guard, extended to all whitespace.server.auth_token_env_varnames an env var holding a pre-shared bearer token (constant-time compare,401otherwise). Serving without authentication is an explicit opt-in,server.allow_unauthenticated: true; with neither the HTTP transport refuses to start. Startup also fails if the token variable is empty. Cross-origin browser requests are refused (http.CrossOriginProtection).Parse/Bind/Executeparameters are substituted by scanning with the parser's tokenizer instead of a regex:$ninside string literals, quoted identifiers and comments is untouched; backslashes are escaped so a value cannot swallow its closing quote; the decoder returns aDecodedParam(GetValue,IsNull) per parameter, so SQL NULL is flagged rather than inferred from the textNULL. The unused legacySubstituteParamsis removed.Behaviour changes to review
validate_select_queryerrors on more than one statement; hierarchy identifiers containing whitespace,;or\are rejected.stackql mcpwith the default server type,stackql srv --mcp.server.type=http|reverse_proxy) now needsserver.auth_token_env_varorserver.allow_unauthenticated: truein--mcp.config, or it exits at startup with a message naming both. stdio is unaffected. Existing HTTP deployments must add one of the two keys.Origin/ cross-siteSec-Fetch-Siteget403. Non-browser clients are unaffected.\now arrive exact for provider queries. Known limitation: queries routed verbatim to the backing RDBMS (no provider table) see such backslashes doubled, because one text has to stay contained under both the stackql parser and standard SQL quoting. Real server-side binding is the long-term fix.Deferred for maintainer decision
Not changed here because each alters an established default or contract, or lives in another repository:
--pgsrv.address(currently all interfaces) and the image's default command.NATIVEQUERYand for client-suppliedAUTHcredential sources in server sessions (theAUTHstatement is unchanged in this PR; session isolation is a separate piece of work).any-sdk.Test plan
New robot scenarios (each fails on
main, passes here):MCP HTTP Mode Read Only Refuses Smuggled StatementsMCP HTTP Bearer Token AuthenticationMCP Server Refuses Unset Bearer Token Env VarMCP Server Refuses Unauthenticated HTTP Without Opt InPG Session Extended Query Bound Params Stay DataUnit tests added or extended in
pkg/mcp_server,pkg/mcp_server/policy,pkg/sqlsplit,internal/stackql/queryshapeandinternal/stackql/paramdecoder.go test ./...andgolangci-lint(v2.5.0) pass locally.Full functional robot suite locally (native, SQLite backend): 674 tests, 652 passed. The 22 failures are specific to the dev box (21 mTLS scenarios rejected for client key file permissions on a mounted drive, 1 transient file permission error that passes when rerun) and occur without this change; CI is the reference run.
🤖 Generated with Claude Code