From c3f7c5a92e710dce3eedc950f7bc5bb565379844 Mon Sep 17 00:00:00 2001 From: Alexandr Dubovikov Date: Sun, 2 Aug 2026 21:59:59 +0200 Subject: [PATCH] fix(sql): stop false-positive forbidden keywords in Call-IDs Make node/MCP read-only keyword checks token-aware via sqlvalidator so words like "call" inside quoted session_id values are allowed while real CALL/DML identifiers remain blocked. Bump version to 11.0.305. --- docs/MCP.md | 2 +- docs/SECURITY.md | 2 +- src/coordinator/sqlvalidator/validator.go | 50 ++++++++++++++++ .../sqlvalidator/validator_test.go | 52 +++++++++++++++++ src/mcp/mcp.go | 12 +--- src/mcp/mcp_test.go | 20 ++++++- src/node/node.go | 7 ++- src/node/rewrite_query_test.go | 57 +++++++++++++++++++ src/version.go | 2 +- 9 files changed, 187 insertions(+), 17 deletions(-) diff --git a/docs/MCP.md b/docs/MCP.md index df09e19f..816ffc30 100644 --- a/docs/MCP.md +++ b/docs/MCP.md @@ -93,7 +93,7 @@ The generated SQL is returned in the response (`generated_sql`) and is server-si - only `SELECT` / `WITH` allowed - semicolons forbidden - only `homer_lake.main.hep_proto_1_call` allowed -- forbidden tokens: `insert`, `update`, `delete`, `drop`, `alter`, `truncate`, `copy`, `attach`, `detach`, `call`, `create`, `grant`, `revoke` +- forbidden tokens (as SQL identifiers, not inside string literals): `insert`, `update`, `delete`, `drop`, `alter`, `truncate`, `copy`, `attach`, `detach`, `call`, `create`, `grant`, `revoke`, plus the shared read-only keyword set (`load`, `pragma`, `merge`, …). Call-IDs that embed words like `call` are allowed. ### `homer_query` diff --git a/docs/SECURITY.md b/docs/SECURITY.md index 24053809..29b61d9d 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -70,7 +70,7 @@ See [AUTH_LDAP_AND_OAUTH.md](./AUTH_LDAP_AND_OAUTH.md#internal-duckdb-authentica - Allowed statement starts: `SELECT`, `WITH`, `SHOW`, `DESCRIBE`, `EXPLAIN`, `PRAGMA` - No semicolons (multi-statement blocked) -- Blocked DML/DDL keywords and dangerous functions +- Blocked DML/DDL keywords and dangerous functions (token-aware: keywords inside string literals / Call-IDs are ignored; real `CALL` / `DELETE` identifiers remain blocked) Invalid SQL returns **400** with `SQL validation failed`. Grafana-style read-only panels using single `SELECT` statements continue to work. diff --git a/src/coordinator/sqlvalidator/validator.go b/src/coordinator/sqlvalidator/validator.go index 69118c8d..b74d3ed7 100644 --- a/src/coordinator/sqlvalidator/validator.go +++ b/src/coordinator/sqlvalidator/validator.go @@ -598,6 +598,56 @@ func ContainsUnsafeComment(sql string) bool { return false } +// ---- Forbidden identifiers (read-only paths) ------------------------------- + +// ForbiddenReadOnlyKeywords are statement keywords that must not appear as +// identifiers outside string literals on read-only SELECT paths (node / MCP). +// CALL remains blocked as a real DuckDB statement; matching is token-aware so +// Call-IDs / session_ids that embed words like "call" are not rejected. +var ForbiddenReadOnlyKeywords = map[string]bool{ + "ATTACH": true, + "DETACH": true, + "COPY": true, + "PRAGMA": true, + "INSTALL": true, + "LOAD": true, + "CALL": true, + "CREATE": true, + "ALTER": true, + "DROP": true, + "TRUNCATE": true, + "INSERT": true, + "UPDATE": true, + "DELETE": true, + "MERGE": true, + "REPLACE": true, + "GRANT": true, + "REVOKE": true, + "VACUUM": true, + "ANALYZE": true, + "EXPORT": true, + "IMPORT": true, +} + +// ContainsForbiddenIdentifier reports whether sql contains any of the given +// keywords as identifier tokens outside string literals. A naive whole-string +// regex would false-positive on Call-IDs / session_ids that embed words like +// "call" or "delete". +func ContainsForbiddenIdentifier(sql string, forbidden map[string]bool) bool { + if len(forbidden) == 0 { + return false + } + for _, tok := range tokenize(sql) { + if tok.kind != tkIdent { + continue + } + if forbidden[tok.upper] { + return true + } + } + return false +} + // ---- SafeString ------------------------------------------------------------ const maxSafeStringLen = 1000 diff --git a/src/coordinator/sqlvalidator/validator_test.go b/src/coordinator/sqlvalidator/validator_test.go index 3ad4f7ca..ab8c24fd 100644 --- a/src/coordinator/sqlvalidator/validator_test.go +++ b/src/coordinator/sqlvalidator/validator_test.go @@ -824,6 +824,58 @@ func TestContainsUnsafeComment(t *testing.T) { } } +func TestContainsForbiddenIdentifier(t *testing.T) { + tests := []struct { + name string + sql string + want bool + }{ + { + name: "call inside session_id literal", + sql: "SELECT * FROM t WHERE session_id = 'foo-call-bar'", + want: false, + }, + { + name: "hep_proto_1_call table name", + sql: "SELECT * FROM hep_proto_1_call", + want: false, + }, + { + name: "real CALL statement", + sql: "SELECT * FROM t CALL some_proc()", + want: true, + }, + { + name: "delete inside literal allowed", + sql: "SELECT * FROM t WHERE x = 'delete'", + want: false, + }, + { + name: "DELETE as identifier blocked", + sql: "SELECT * FROM t WHERE DELETE", + want: true, + }, + { + name: "empty forbidden map", + sql: "SELECT CALL FROM t", + want: false, // tested with nil map below + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + forbidden := ForbiddenReadOnlyKeywords + if tt.name == "empty forbidden map" { + forbidden = nil + } + got := ContainsForbiddenIdentifier(tt.sql, forbidden) + if got != tt.want { + t.Errorf("ContainsForbiddenIdentifier(%q) = %v, want %v", tt.sql, got, tt.want) + } + }) + } +} + func TestHasLimitToken(t *testing.T) { tests := []struct { name string diff --git a/src/mcp/mcp.go b/src/mcp/mcp.go index 56f65566..02cf7529 100644 --- a/src/mcp/mcp.go +++ b/src/mcp/mcp.go @@ -17,6 +17,7 @@ import ( "github.com/mark3labs/mcp-go/mcp" "github.com/mark3labs/mcp-go/server" "github.com/sipcapture/homer-core/src/config" + "github.com/sipcapture/homer-core/src/coordinator/sqlvalidator" logger "github.com/sipcapture/homer-core/src/utils/logging" ) @@ -103,10 +104,6 @@ var ( "INVITE", "BYE", "REGISTER", "OPTIONS", "ACK", "CANCEL", "PRACK", "UPDATE", "INFO", "REFER", "SUBSCRIBE", "NOTIFY", "PUBLISH", "MESSAGE", } - bannedSQLTokens = []string{ - "insert", "update", "delete", "drop", "alter", "truncate", - "copy", "attach", "detach", "call", "create", "grant", "revoke", - } ) func New(cfg *config.MCPConfig) (*Module, error) { @@ -685,11 +682,8 @@ func validateSQL(sql string) error { return fmt.Errorf("only homer_lake.main.hep_proto_1_call is allowed") } - for _, token := range bannedSQLTokens { - re := regexp.MustCompile(`(?i)\b` + token + `\b`) - if re.MatchString(trimmed) { - return fmt.Errorf("forbidden SQL token: %s", token) - } + if sqlvalidator.ContainsForbiddenIdentifier(trimmed, sqlvalidator.ForbiddenReadOnlyKeywords) { + return fmt.Errorf("forbidden SQL token") } return nil } diff --git a/src/mcp/mcp_test.go b/src/mcp/mcp_test.go index 63b3cfa1..ddd76b59 100644 --- a/src/mcp/mcp_test.go +++ b/src/mcp/mcp_test.go @@ -59,6 +59,13 @@ func TestValidateSQLAllowsCallTableName(t *testing.T) { } } +func TestValidateSQLAllowsForbiddenWordsInLiterals(t *testing.T) { + sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE session_id = 'foo-call-bar' OR note = 'drop table'" + if err := validateSQL(sql); err != nil { + t.Fatalf("expected keywords inside string literals to be allowed, got: %v", err) + } +} + func TestValidateSQLRejectsSemicolon(t *testing.T) { sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE method = 'INVITE';" if err := validateSQL(sql); err == nil { @@ -67,9 +74,18 @@ func TestValidateSQLRejectsSemicolon(t *testing.T) { } func TestValidateSQLRejectsDropToken(t *testing.T) { - sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE note = 'drop table'" + // Bare DROP identifier must still be rejected; words inside string + // literals are allowed (see TestValidateSQLAllowsForbiddenWordsInLiterals). + sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE DROP" + if err := validateSQL(sql); err == nil { + t.Fatalf("expected DROP identifier SQL to be rejected") + } +} + +func TestValidateSQLRejectsCallStatement(t *testing.T) { + sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE 1=1 CALL some_proc()" if err := validateSQL(sql); err == nil { - t.Fatalf("expected DROP token SQL to be rejected") + t.Fatalf("expected CALL statement SQL to be rejected") } } diff --git a/src/node/node.go b/src/node/node.go index f773f9fe..8be8a684 100644 --- a/src/node/node.go +++ b/src/node/node.go @@ -511,8 +511,6 @@ func mergeSelectResults(a, b []map[string]interface{}, colsA, colsB []string, li // UNIONs and the overall statement shape are built dynamically. As defence in // depth we still reject anything that is not a single read-only SELECT before // it reaches the driver, so a malformed/stacked statement can never run here. -var dangerousSQLPattern = regexp.MustCompile(`(?i)\b(attach|detach|copy|pragma|install|load|call|create|alter|drop|truncate|insert|update|delete|merge|replace|grant|revoke|vacuum|analyze|export|import)\b`) - func validateUserSQL(query string) error { trimmed := strings.TrimSpace(query) if trimmed == "" { @@ -534,7 +532,10 @@ func validateUserSQL(query string) error { return fmt.Errorf("SQL contains forbidden comment or statement separator") } - if dangerousSQLPattern.MatchString(trimmed) { + // Token-aware keyword check: ignore string literals so Call-IDs that embed + // words like "call" / "delete" do not false-positive (same class of bug as + // "--" inside session_id before ContainsUnsafeComment). + if sqlvalidator.ContainsForbiddenIdentifier(trimmed, sqlvalidator.ForbiddenReadOnlyKeywords) { return fmt.Errorf("SQL contains forbidden keyword") } diff --git a/src/node/rewrite_query_test.go b/src/node/rewrite_query_test.go index 425c28a8..6423da04 100644 --- a/src/node/rewrite_query_test.go +++ b/src/node/rewrite_query_test.go @@ -552,6 +552,63 @@ func TestValidateUserSQL_CommentMarkers(t *testing.T) { } } +func TestValidateUserSQL_KeywordsInLiterals(t *testing.T) { + tests := []struct { + name string + query string + wantErr string // empty = pass + }{ + { + name: "session_id with call token passes", + query: "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE session_id = 'foo-call-bar'", + }, + { + name: "session_id exactly call passes", + query: "SELECT * FROM t WHERE session_id = 'call'", + }, + { + name: "delete/load/update inside literals pass", + query: "SELECT * FROM t WHERE session_id = 'delete-me' OR cid = 'load-copy-update'", + }, + { + name: "hep_proto_1_call table name is not CALL keyword", + query: "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE method = 'INVITE'", + }, + { + name: "real CALL statement rejected", + query: "SELECT * FROM t WHERE 1=1 CALL some_proc()", + wantErr: "forbidden keyword", + }, + { + name: "real DELETE identifier rejected", + query: "SELECT * FROM t WHERE DELETE", + wantErr: "forbidden keyword", + }, + { + name: "real INSERT identifier rejected", + query: "SELECT INSERT FROM t", + wantErr: "forbidden keyword", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateUserSQL(tt.query) + if tt.wantErr == "" { + if err != nil { + t.Errorf("expected no error, got: %v", err) + } + } else { + if err == nil { + t.Errorf("expected error containing %q, got nil", tt.wantErr) + } else if !strings.Contains(err.Error(), tt.wantErr) { + t.Errorf("expected error containing %q, got: %v", tt.wantErr, err) + } + } + }) + } +} + func TestPrepareFlightSQLDataSQLUsesThresholdDecision(t *testing.T) { n := defaultMemoryUnionNode() diff --git a/src/version.go b/src/version.go index 7bdb0a26..2627970b 100644 --- a/src/version.go +++ b/src/version.go @@ -24,7 +24,7 @@ import ( // Version information for homer-core var ( // VERSION_APPLICATION is the application version - VERSION_APPLICATION = "11.0.304" + VERSION_APPLICATION = "11.0.305" // BuildDate is the build date BuildDate = ""