Skip to content

Commit c3f7c5a

Browse files
committed
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.
1 parent fde2c49 commit c3f7c5a

9 files changed

Lines changed: 187 additions & 17 deletions

File tree

docs/MCP.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ The generated SQL is returned in the response (`generated_sql`) and is server-si
9393
- only `SELECT` / `WITH` allowed
9494
- semicolons forbidden
9595
- only `homer_lake.main.hep_proto_1_call` allowed
96-
- forbidden tokens: `insert`, `update`, `delete`, `drop`, `alter`, `truncate`, `copy`, `attach`, `detach`, `call`, `create`, `grant`, `revoke`
96+
- 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.
9797

9898
### `homer_query`
9999

docs/SECURITY.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,7 +70,7 @@ See [AUTH_LDAP_AND_OAUTH.md](./AUTH_LDAP_AND_OAUTH.md#internal-duckdb-authentica
7070

7171
- Allowed statement starts: `SELECT`, `WITH`, `SHOW`, `DESCRIBE`, `EXPLAIN`, `PRAGMA`
7272
- No semicolons (multi-statement blocked)
73-
- Blocked DML/DDL keywords and dangerous functions
73+
- Blocked DML/DDL keywords and dangerous functions (token-aware: keywords inside string literals / Call-IDs are ignored; real `CALL` / `DELETE` identifiers remain blocked)
7474

7575
Invalid SQL returns **400** with `SQL validation failed`. Grafana-style read-only panels using single `SELECT` statements continue to work.
7676

src/coordinator/sqlvalidator/validator.go

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -598,6 +598,56 @@ func ContainsUnsafeComment(sql string) bool {
598598
return false
599599
}
600600

601+
// ---- Forbidden identifiers (read-only paths) -------------------------------
602+
603+
// ForbiddenReadOnlyKeywords are statement keywords that must not appear as
604+
// identifiers outside string literals on read-only SELECT paths (node / MCP).
605+
// CALL remains blocked as a real DuckDB statement; matching is token-aware so
606+
// Call-IDs / session_ids that embed words like "call" are not rejected.
607+
var ForbiddenReadOnlyKeywords = map[string]bool{
608+
"ATTACH": true,
609+
"DETACH": true,
610+
"COPY": true,
611+
"PRAGMA": true,
612+
"INSTALL": true,
613+
"LOAD": true,
614+
"CALL": true,
615+
"CREATE": true,
616+
"ALTER": true,
617+
"DROP": true,
618+
"TRUNCATE": true,
619+
"INSERT": true,
620+
"UPDATE": true,
621+
"DELETE": true,
622+
"MERGE": true,
623+
"REPLACE": true,
624+
"GRANT": true,
625+
"REVOKE": true,
626+
"VACUUM": true,
627+
"ANALYZE": true,
628+
"EXPORT": true,
629+
"IMPORT": true,
630+
}
631+
632+
// ContainsForbiddenIdentifier reports whether sql contains any of the given
633+
// keywords as identifier tokens outside string literals. A naive whole-string
634+
// regex would false-positive on Call-IDs / session_ids that embed words like
635+
// "call" or "delete".
636+
func ContainsForbiddenIdentifier(sql string, forbidden map[string]bool) bool {
637+
if len(forbidden) == 0 {
638+
return false
639+
}
640+
for _, tok := range tokenize(sql) {
641+
if tok.kind != tkIdent {
642+
continue
643+
}
644+
if forbidden[tok.upper] {
645+
return true
646+
}
647+
}
648+
return false
649+
}
650+
601651
// ---- SafeString ------------------------------------------------------------
602652

603653
const maxSafeStringLen = 1000

src/coordinator/sqlvalidator/validator_test.go

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -824,6 +824,58 @@ func TestContainsUnsafeComment(t *testing.T) {
824824
}
825825
}
826826

827+
func TestContainsForbiddenIdentifier(t *testing.T) {
828+
tests := []struct {
829+
name string
830+
sql string
831+
want bool
832+
}{
833+
{
834+
name: "call inside session_id literal",
835+
sql: "SELECT * FROM t WHERE session_id = 'foo-call-bar'",
836+
want: false,
837+
},
838+
{
839+
name: "hep_proto_1_call table name",
840+
sql: "SELECT * FROM hep_proto_1_call",
841+
want: false,
842+
},
843+
{
844+
name: "real CALL statement",
845+
sql: "SELECT * FROM t CALL some_proc()",
846+
want: true,
847+
},
848+
{
849+
name: "delete inside literal allowed",
850+
sql: "SELECT * FROM t WHERE x = 'delete'",
851+
want: false,
852+
},
853+
{
854+
name: "DELETE as identifier blocked",
855+
sql: "SELECT * FROM t WHERE DELETE",
856+
want: true,
857+
},
858+
{
859+
name: "empty forbidden map",
860+
sql: "SELECT CALL FROM t",
861+
want: false, // tested with nil map below
862+
},
863+
}
864+
865+
for _, tt := range tests {
866+
t.Run(tt.name, func(t *testing.T) {
867+
forbidden := ForbiddenReadOnlyKeywords
868+
if tt.name == "empty forbidden map" {
869+
forbidden = nil
870+
}
871+
got := ContainsForbiddenIdentifier(tt.sql, forbidden)
872+
if got != tt.want {
873+
t.Errorf("ContainsForbiddenIdentifier(%q) = %v, want %v", tt.sql, got, tt.want)
874+
}
875+
})
876+
}
877+
}
878+
827879
func TestHasLimitToken(t *testing.T) {
828880
tests := []struct {
829881
name string

src/mcp/mcp.go

Lines changed: 3 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/mark3labs/mcp-go/mcp"
1818
"github.com/mark3labs/mcp-go/server"
1919
"github.com/sipcapture/homer-core/src/config"
20+
"github.com/sipcapture/homer-core/src/coordinator/sqlvalidator"
2021
logger "github.com/sipcapture/homer-core/src/utils/logging"
2122
)
2223

@@ -103,10 +104,6 @@ var (
103104
"INVITE", "BYE", "REGISTER", "OPTIONS", "ACK", "CANCEL", "PRACK",
104105
"UPDATE", "INFO", "REFER", "SUBSCRIBE", "NOTIFY", "PUBLISH", "MESSAGE",
105106
}
106-
bannedSQLTokens = []string{
107-
"insert", "update", "delete", "drop", "alter", "truncate",
108-
"copy", "attach", "detach", "call", "create", "grant", "revoke",
109-
}
110107
)
111108

112109
func New(cfg *config.MCPConfig) (*Module, error) {
@@ -685,11 +682,8 @@ func validateSQL(sql string) error {
685682
return fmt.Errorf("only homer_lake.main.hep_proto_1_call is allowed")
686683
}
687684

688-
for _, token := range bannedSQLTokens {
689-
re := regexp.MustCompile(`(?i)\b` + token + `\b`)
690-
if re.MatchString(trimmed) {
691-
return fmt.Errorf("forbidden SQL token: %s", token)
692-
}
685+
if sqlvalidator.ContainsForbiddenIdentifier(trimmed, sqlvalidator.ForbiddenReadOnlyKeywords) {
686+
return fmt.Errorf("forbidden SQL token")
693687
}
694688
return nil
695689
}

src/mcp/mcp_test.go

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,13 @@ func TestValidateSQLAllowsCallTableName(t *testing.T) {
5959
}
6060
}
6161

62+
func TestValidateSQLAllowsForbiddenWordsInLiterals(t *testing.T) {
63+
sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE session_id = 'foo-call-bar' OR note = 'drop table'"
64+
if err := validateSQL(sql); err != nil {
65+
t.Fatalf("expected keywords inside string literals to be allowed, got: %v", err)
66+
}
67+
}
68+
6269
func TestValidateSQLRejectsSemicolon(t *testing.T) {
6370
sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE method = 'INVITE';"
6471
if err := validateSQL(sql); err == nil {
@@ -67,9 +74,18 @@ func TestValidateSQLRejectsSemicolon(t *testing.T) {
6774
}
6875

6976
func TestValidateSQLRejectsDropToken(t *testing.T) {
70-
sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE note = 'drop table'"
77+
// Bare DROP identifier must still be rejected; words inside string
78+
// literals are allowed (see TestValidateSQLAllowsForbiddenWordsInLiterals).
79+
sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE DROP"
80+
if err := validateSQL(sql); err == nil {
81+
t.Fatalf("expected DROP identifier SQL to be rejected")
82+
}
83+
}
84+
85+
func TestValidateSQLRejectsCallStatement(t *testing.T) {
86+
sql := "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE 1=1 CALL some_proc()"
7187
if err := validateSQL(sql); err == nil {
72-
t.Fatalf("expected DROP token SQL to be rejected")
88+
t.Fatalf("expected CALL statement SQL to be rejected")
7389
}
7490
}
7591

src/node/node.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -511,8 +511,6 @@ func mergeSelectResults(a, b []map[string]interface{}, colsA, colsB []string, li
511511
// UNIONs and the overall statement shape are built dynamically. As defence in
512512
// depth we still reject anything that is not a single read-only SELECT before
513513
// it reaches the driver, so a malformed/stacked statement can never run here.
514-
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`)
515-
516514
func validateUserSQL(query string) error {
517515
trimmed := strings.TrimSpace(query)
518516
if trimmed == "" {
@@ -534,7 +532,10 @@ func validateUserSQL(query string) error {
534532
return fmt.Errorf("SQL contains forbidden comment or statement separator")
535533
}
536534

537-
if dangerousSQLPattern.MatchString(trimmed) {
535+
// Token-aware keyword check: ignore string literals so Call-IDs that embed
536+
// words like "call" / "delete" do not false-positive (same class of bug as
537+
// "--" inside session_id before ContainsUnsafeComment).
538+
if sqlvalidator.ContainsForbiddenIdentifier(trimmed, sqlvalidator.ForbiddenReadOnlyKeywords) {
538539
return fmt.Errorf("SQL contains forbidden keyword")
539540
}
540541

src/node/rewrite_query_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -552,6 +552,63 @@ func TestValidateUserSQL_CommentMarkers(t *testing.T) {
552552
}
553553
}
554554

555+
func TestValidateUserSQL_KeywordsInLiterals(t *testing.T) {
556+
tests := []struct {
557+
name string
558+
query string
559+
wantErr string // empty = pass
560+
}{
561+
{
562+
name: "session_id with call token passes",
563+
query: "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE session_id = 'foo-call-bar'",
564+
},
565+
{
566+
name: "session_id exactly call passes",
567+
query: "SELECT * FROM t WHERE session_id = 'call'",
568+
},
569+
{
570+
name: "delete/load/update inside literals pass",
571+
query: "SELECT * FROM t WHERE session_id = 'delete-me' OR cid = 'load-copy-update'",
572+
},
573+
{
574+
name: "hep_proto_1_call table name is not CALL keyword",
575+
query: "SELECT * FROM homer_lake.main.hep_proto_1_call WHERE method = 'INVITE'",
576+
},
577+
{
578+
name: "real CALL statement rejected",
579+
query: "SELECT * FROM t WHERE 1=1 CALL some_proc()",
580+
wantErr: "forbidden keyword",
581+
},
582+
{
583+
name: "real DELETE identifier rejected",
584+
query: "SELECT * FROM t WHERE DELETE",
585+
wantErr: "forbidden keyword",
586+
},
587+
{
588+
name: "real INSERT identifier rejected",
589+
query: "SELECT INSERT FROM t",
590+
wantErr: "forbidden keyword",
591+
},
592+
}
593+
594+
for _, tt := range tests {
595+
t.Run(tt.name, func(t *testing.T) {
596+
err := validateUserSQL(tt.query)
597+
if tt.wantErr == "" {
598+
if err != nil {
599+
t.Errorf("expected no error, got: %v", err)
600+
}
601+
} else {
602+
if err == nil {
603+
t.Errorf("expected error containing %q, got nil", tt.wantErr)
604+
} else if !strings.Contains(err.Error(), tt.wantErr) {
605+
t.Errorf("expected error containing %q, got: %v", tt.wantErr, err)
606+
}
607+
}
608+
})
609+
}
610+
}
611+
555612
func TestPrepareFlightSQLDataSQLUsesThresholdDecision(t *testing.T) {
556613
n := defaultMemoryUnionNode()
557614

src/version.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import (
2424
// Version information for homer-core
2525
var (
2626
// VERSION_APPLICATION is the application version
27-
VERSION_APPLICATION = "11.0.304"
27+
VERSION_APPLICATION = "11.0.305"
2828

2929
// BuildDate is the build date
3030
BuildDate = ""

0 commit comments

Comments
 (0)