From ce333c70f39387862789a9239784d348efbbe971 Mon Sep 17 00:00:00 2001 From: Viet Tran Date: Wed, 18 Mar 2026 07:48:48 +0700 Subject: [PATCH] =?UTF-8?q?fix(security):=20followup=20hardening=20?= =?UTF-8?q?=E2=80=94=20ILIKE=20ESCAPE,=20allowlist=20logging,=20shell=20de?= =?UTF-8?q?ny,=20tests=20(#251)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add explicit ESCAPE '\' clause to all ILIKE queries (knowledge_graph, custom_tools, channel_instances, channel_contacts) for correct wildcard escaping regardless of PostgreSQL standard_conforming_strings setting - Log warning when filterAllowedKeys drops unknown fields for debuggability - Widen base64 decode shell deny pattern to catch -di, -dw0 variants - Add unit tests for filterAllowedKeys, pqStringArray, scanStringArray, pqStringArray↔scanStringArray roundtrip, limitedBuffer, base64 deny --- internal/http/validate.go | 7 +- internal/http/validate_test.go | 51 ++++++++++++ internal/store/pg/channel_contacts.go | 2 +- internal/store/pg/channel_instances.go | 2 +- internal/store/pg/custom_tools.go | 2 +- internal/store/pg/helpers_test.go | 102 ++++++++++++++++++++++++ internal/store/pg/knowledge_graph.go | 2 +- internal/tools/shell_deny_groups.go | 2 +- internal/tools/shell_deny_test.go | 104 +++++++++++++++++++++++++ 9 files changed, 268 insertions(+), 6 deletions(-) create mode 100644 internal/http/validate_test.go create mode 100644 internal/store/pg/helpers_test.go create mode 100644 internal/tools/shell_deny_test.go diff --git a/internal/http/validate.go b/internal/http/validate.go index 457063a1..c9965429 100644 --- a/internal/http/validate.go +++ b/internal/http/validate.go @@ -1,6 +1,9 @@ package http -import "regexp" +import ( + "log/slog" + "regexp" +) var slugRe = regexp.MustCompile(`^[a-z0-9]([a-z0-9-]*[a-z0-9])?$`) @@ -17,6 +20,8 @@ func filterAllowedKeys(updates map[string]any, allowed map[string]bool) map[stri for k, v := range updates { if allowed[k] { filtered[k] = v + } else { + slog.Warn("security.filtered_unknown_field", "field", k) } } return filtered diff --git a/internal/http/validate_test.go b/internal/http/validate_test.go new file mode 100644 index 00000000..d40fc7cd --- /dev/null +++ b/internal/http/validate_test.go @@ -0,0 +1,51 @@ +package http + +import ( + "testing" +) + +func TestFilterAllowedKeys(t *testing.T) { + allowed := map[string]bool{"name": true, "status": true} + + tests := []struct { + name string + updates map[string]any + wantKeys []string + }{ + { + name: "keeps allowed keys", + updates: map[string]any{"name": "foo", "status": "active"}, + wantKeys: []string{"name", "status"}, + }, + { + name: "filters disallowed keys", + updates: map[string]any{"name": "foo", "id": "inject", "owner_id": "hack"}, + wantKeys: []string{"name"}, + }, + { + name: "empty input returns empty", + updates: map[string]any{}, + wantKeys: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := filterAllowedKeys(tt.updates, allowed) + if tt.wantKeys == nil { + if len(result) != 0 { + t.Errorf("expected empty map, got %v", result) + } + return + } + if len(result) != len(tt.wantKeys) { + t.Errorf("expected %d keys, got %d: %v", len(tt.wantKeys), len(result), result) + } + for _, k := range tt.wantKeys { + if _, ok := result[k]; !ok { + t.Errorf("expected key %q in result", k) + } + } + }) + } +} diff --git a/internal/store/pg/channel_contacts.go b/internal/store/pg/channel_contacts.go index 6c3c3133..72f69e2d 100644 --- a/internal/store/pg/channel_contacts.go +++ b/internal/store/pg/channel_contacts.go @@ -56,7 +56,7 @@ func contactWhereClause(opts store.ContactListOpts) (string, []any, int) { escaped := strings.NewReplacer("%", "\\%", "_", "\\_").Replace(opts.Search) pattern := "%" + escaped + "%" conditions = append(conditions, fmt.Sprintf( - "(display_name ILIKE $%d OR username ILIKE $%d OR sender_id ILIKE $%d)", + "(display_name ILIKE $%d ESCAPE '\\' OR username ILIKE $%d ESCAPE '\\' OR sender_id ILIKE $%d ESCAPE '\\')", argIdx, argIdx, argIdx, )) args = append(args, pattern) diff --git a/internal/store/pg/channel_instances.go b/internal/store/pg/channel_instances.go index 49b2f242..15ee29d0 100644 --- a/internal/store/pg/channel_instances.go +++ b/internal/store/pg/channel_instances.go @@ -250,7 +250,7 @@ func buildChannelInstanceWhere(opts store.ChannelInstanceListOpts) (string, []an argIdx := 1 if opts.Search != "" { - conditions = append(conditions, fmt.Sprintf("(name ILIKE $%d OR display_name ILIKE $%d OR channel_type ILIKE $%d)", argIdx, argIdx, argIdx)) + conditions = append(conditions, fmt.Sprintf("(name ILIKE $%d ESCAPE '\\' OR display_name ILIKE $%d ESCAPE '\\' OR channel_type ILIKE $%d ESCAPE '\\')", argIdx, argIdx, argIdx)) escaped := strings.NewReplacer("%", "\\%", "_", "\\_").Replace(opts.Search) args = append(args, "%"+escaped+"%") } diff --git a/internal/store/pg/custom_tools.go b/internal/store/pg/custom_tools.go index 25ba2d65..37026bce 100644 --- a/internal/store/pg/custom_tools.go +++ b/internal/store/pg/custom_tools.go @@ -204,7 +204,7 @@ func buildCustomToolWhere(opts store.CustomToolListOpts) (string, []any) { argIdx++ } if opts.Search != "" { - conditions = append(conditions, fmt.Sprintf("(name ILIKE $%d OR description ILIKE $%d)", argIdx, argIdx)) + conditions = append(conditions, fmt.Sprintf("(name ILIKE $%d ESCAPE '\\' OR description ILIKE $%d ESCAPE '\\')", argIdx, argIdx)) escaped := strings.NewReplacer("%", "\\%", "_", "\\_").Replace(opts.Search) args = append(args, "%"+escaped+"%") } diff --git a/internal/store/pg/helpers_test.go b/internal/store/pg/helpers_test.go new file mode 100644 index 00000000..8bf2a43b --- /dev/null +++ b/internal/store/pg/helpers_test.go @@ -0,0 +1,102 @@ +package pg + +import ( + "testing" +) + +func TestPqStringArray(t *testing.T) { + tests := []struct { + name string + arr []string + want string + }{ + {"nil returns nil", nil, ""}, + {"simple elements", []string{"a", "b"}, `{"a","b"}`}, + {"element with comma", []string{"a,b", "c"}, `{"a,b","c"}`}, + {"element with quote", []string{`say "hi"`}, `{"say \"hi\""}`}, + {"element with backslash", []string{`a\b`}, `{"a\\b"}`}, + {"empty slice", []string{}, `{}`}, + {"empty string element", []string{""}, `{""}`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := pqStringArray(tt.arr) + if tt.arr == nil { + if got != nil { + t.Errorf("expected nil, got %v", got) + } + return + } + s, ok := got.(string) + if !ok { + t.Fatalf("expected string, got %T", got) + } + if s != tt.want { + t.Errorf("got %q, want %q", s, tt.want) + } + }) + } +} + +func TestScanStringArray(t *testing.T) { + tests := []struct { + name string + data []byte + want []string + }{ + {"nil data", nil, nil}, + {"empty braces", []byte("{}"), nil}, + {"simple unquoted", []byte("{a,b,c}"), []string{"a", "b", "c"}}, + {"quoted with comma", []byte(`{"a,b","c"}`), []string{"a,b", "c"}}, + {"quoted with escaped quote", []byte(`{"say \"hi\"","ok"}`), []string{`say "hi"`, "ok"}}, + {"quoted with backslash", []byte(`{"a\\b"}`), []string{`a\b`}}, + {"mixed quoted unquoted", []byte(`{foo,"bar,baz",qux}`), []string{"foo", "bar,baz", "qux"}}, + {"single element", []byte("{hello}"), []string{"hello"}}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var got []string + scanStringArray(tt.data, &got) + if tt.want == nil { + if got != nil { + t.Errorf("expected nil, got %v", got) + } + return + } + if len(got) != len(tt.want) { + t.Fatalf("got %v (len %d), want %v (len %d)", got, len(got), tt.want, len(tt.want)) + } + for i := range tt.want { + if got[i] != tt.want[i] { + t.Errorf("index %d: got %q, want %q", i, got[i], tt.want[i]) + } + } + }) + } +} + +func TestPqStringArrayRoundtrip(t *testing.T) { + // Verify encode→decode roundtrip for tricky values. + inputs := [][]string{ + {"simple"}, + {"a,b", "c"}, + {`say "hello"`, `back\slash`}, + {"", "empty"}, + } + + for _, arr := range inputs { + encoded := pqStringArray(arr).(string) + var decoded []string + scanStringArray([]byte(encoded), &decoded) + if len(decoded) != len(arr) { + t.Fatalf("roundtrip len mismatch: input %v, encoded %q, decoded %v", arr, encoded, decoded) + } + for i := range arr { + if decoded[i] != arr[i] { + t.Errorf("roundtrip mismatch at %d: input %q, got %q (encoded: %q)", i, arr[i], decoded[i], encoded) + } + } + } +} diff --git a/internal/store/pg/knowledge_graph.go b/internal/store/pg/knowledge_graph.go index c0fbd36b..10af36f0 100644 --- a/internal/store/pg/knowledge_graph.go +++ b/internal/store/pg/knowledge_graph.go @@ -128,7 +128,7 @@ func (s *PGKnowledgeGraphStore) SearchEntities(ctx context.Context, agentID, use SELECT id, agent_id, user_id, external_id, name, entity_type, description, properties, source_id, confidence, created_at, updated_at FROM kg_entities - WHERE %s AND (name ILIKE $%d OR description ILIKE $%d) + WHERE %s AND (name ILIKE $%d ESCAPE '\' OR description ILIKE $%d ESCAPE '\') ORDER BY confidence DESC, updated_at DESC LIMIT $%d`, where, idx, idx, idx+1) rows, err := s.db.QueryContext(ctx, q, args...) diff --git a/internal/tools/shell_deny_groups.go b/internal/tools/shell_deny_groups.go index 21a31c27..49dd9112 100644 --- a/internal/tools/shell_deny_groups.go +++ b/internal/tools/shell_deny_groups.go @@ -72,7 +72,7 @@ var DenyGroupRegistry = map[string]*DenyGroup{ Default: true, Patterns: []*regexp.Regexp{ regexp.MustCompile(`\beval\s*\$`), - regexp.MustCompile(`\bbase64\s+(-d|--decode)\b.*\|\s*(ba)?sh\b`), + regexp.MustCompile(`\bbase64\s+(-d\w*|--decode)\b.*\|\s*(ba)?sh\b`), }, }, "privilege_escalation": { diff --git a/internal/tools/shell_deny_test.go b/internal/tools/shell_deny_test.go new file mode 100644 index 00000000..15d24815 --- /dev/null +++ b/internal/tools/shell_deny_test.go @@ -0,0 +1,104 @@ +package tools + +import ( + "testing" +) + +func TestBase64DecodeShellDeny(t *testing.T) { + patterns := DenyGroupRegistry["code_injection"].Patterns + + denied := []string{ + "base64 -d payload.txt | sh", + "base64 --decode payload.txt | sh", + "base64 -di payload.txt | sh", + "base64 -dw0 payload.txt | bash", + "base64 --decode something | bash", + } + + allowed := []string{ + "base64 -w0 file.txt", // encode, no pipe to shell + "base64 -d file.txt", // decode without pipe to shell + "echo hello | base64", // encode + "base64 --decode file.txt", // decode without pipe to shell + } + + for _, cmd := range denied { + matched := false + for _, p := range patterns { + if p.MatchString(cmd) { + matched = true + break + } + } + if !matched { + t.Errorf("expected deny for %q", cmd) + } + } + + for _, cmd := range allowed { + matched := false + for _, p := range patterns { + if p.MatchString(cmd) { + matched = true + break + } + } + if matched { + t.Errorf("unexpected deny for %q", cmd) + } + } +} + +func TestLimitedBuffer(t *testing.T) { + t.Run("under limit", func(t *testing.T) { + lb := &limitedBuffer{max: 100} + lb.Write([]byte("hello")) + if lb.String() != "hello" { + t.Errorf("got %q", lb.String()) + } + if lb.truncated { + t.Error("should not be truncated") + } + }) + + t.Run("at limit", func(t *testing.T) { + lb := &limitedBuffer{max: 5} + n, err := lb.Write([]byte("hello")) + if err != nil || n != 5 { + t.Errorf("Write: n=%d err=%v", n, err) + } + if lb.truncated { + t.Error("exactly at limit should not be truncated") + } + }) + + t.Run("over limit truncates", func(t *testing.T) { + lb := &limitedBuffer{max: 5} + n, err := lb.Write([]byte("hello world")) + if err != nil { + t.Fatal(err) + } + if n != 11 { + t.Errorf("Write should report full len, got %d", n) + } + if !lb.truncated { + t.Error("should be truncated") + } + if lb.Len() != 5 { + t.Errorf("buffer len should be 5, got %d", lb.Len()) + } + want := "hello\n[output truncated at 1MB]" + if lb.String() != want { + t.Errorf("got %q, want %q", lb.String(), want) + } + }) + + t.Run("subsequent writes after truncation", func(t *testing.T) { + lb := &limitedBuffer{max: 3} + lb.Write([]byte("abc")) + lb.Write([]byte("def")) + if lb.Len() != 3 { + t.Errorf("buffer len should be 3, got %d", lb.Len()) + } + }) +}