From 5fe6397765515dd1d1f3911b98bde2b9662e788b Mon Sep 17 00:00:00 2001 From: rabbitblood Date: Sun, 26 Apr 2026 00:57:44 -0700 Subject: [PATCH 1/2] fix(discovery): apply ?q= filter to Peers list (#1038) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Peers handler at workspace-server/internal/handlers/discovery.go ignored the ?q= query param entirely — every caller got the full peer list regardless of what they searched for. The handler exposes peer identities + URLs, so leaking the unfiltered set on a "filtered" endpoint is an info-disclosure bug (CWE-862). Fix: read c.Query("q") and post-filter the in-memory peers slice by case-insensitive substring match against name OR role. Filtering is done in Go after the existing 3 SQL reads — keeps the SQL bytes identical to the no-filter path (no injection vector, no DB-driver collation surprises) at a small cost. The peer set is bounded by a single workspace's parent + children + siblings (typically <50 rows), so the in-memory pass is negligible. Empty / whitespace-only q is a no-op — preserves the no-filter allocation profile. Tests (6 new in discovery_test.go): - TestPeers_NoQ_ReturnsAll — regression baseline (3 peers, no filter) - TestPeers_Q_FiltersByName — q=alpha → ws-alpha only - TestPeers_Q_CaseInsensitive — q=ALPHA → ws-alpha (locks in ToLower) - TestPeers_Q_FiltersByRole — q=design → ws-beta (role-side match) - TestPeers_Q_NoMatches — empty array, JSON [] not null - TestPeers_Q_WhitespaceOnly — q=' ' treated as no-filter Helpers peersFilterFixture + runPeersWithQuery + peerNames keep each test scoped to the q-behaviour, not re-declaring SQL expectations. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/discovery.go | 35 +++++ .../internal/handlers/discovery_test.go | 144 ++++++++++++++++++ 2 files changed, 179 insertions(+) diff --git a/workspace-server/internal/handlers/discovery.go b/workspace-server/internal/handlers/discovery.go index d8edf814..fbc2cc2a 100644 --- a/workspace-server/internal/handlers/discovery.go +++ b/workspace-server/internal/handlers/discovery.go @@ -177,6 +177,14 @@ func writeExternalWorkspaceURL(ctx context.Context, c *gin.Context, callerID, ta } // Peers handles GET /registry/:id/peers +// +// Optional ``?q=`` filters the result by case-insensitive +// substring match against ``name`` or ``role`` (#1038). Filtering is done +// in Go after the DB read — keeps the SQL identical to the no-filter path +// (no injection risk, no DB-driver collation surprises) at the cost of +// loading the unfiltered set first. Acceptable because the peer set is +// always bounded by the small fanout of a single workspace's parent + +// children + siblings (typically <50 rows). func (h *DiscoveryHandler) Peers(c *gin.Context) { workspaceID := c.Param("id") ctx := c.Request.Context() @@ -241,12 +249,39 @@ func (h *DiscoveryHandler) Peers(c *gin.Context) { peers = append(peers, parent...) } + peers = filterPeersByQuery(peers, c.Query("q")) + if peers == nil { peers = make([]map[string]interface{}, 0) } c.JSON(http.StatusOK, peers) } +// filterPeersByQuery returns the subset of peers whose name or role +// case-insensitively contains q. An empty/whitespace-only q is a no-op +// (returns the input slice unchanged) so the no-filter path stays free +// of allocations. Map values come from queryPeerMaps which always +// stringifies name + role, so the type asserts here cannot fail in +// practice — the comma-ok form belt-and-braces against future shape +// changes upstream. +func filterPeersByQuery(peers []map[string]interface{}, q string) []map[string]interface{} { + q = strings.TrimSpace(q) + if q == "" { + return peers + } + needle := strings.ToLower(q) + out := make([]map[string]interface{}, 0, len(peers)) + for _, p := range peers { + name, _ := p["name"].(string) + role, _ := p["role"].(string) + if strings.Contains(strings.ToLower(name), needle) || + strings.Contains(strings.ToLower(role), needle) { + out = append(out, p) + } + } + return out +} + // queryPeerMaps returns clean JSON-serializable maps instead of Workspace structs. func queryPeerMaps(query string, args ...interface{}) ([]map[string]interface{}, error) { rows, err := db.DB.Query(query, args...) diff --git a/workspace-server/internal/handlers/discovery_test.go b/workspace-server/internal/handlers/discovery_test.go index 7c90005e..a027723f 100644 --- a/workspace-server/internal/handlers/discovery_test.go +++ b/workspace-server/internal/handlers/discovery_test.go @@ -258,6 +258,150 @@ func TestPeers_RootWorkspace_NoPeers(t *testing.T) { } } +// ==================== Peers — ?q= filter (#1038) ==================== + +// peersFilterFixture mocks all 3 SQL reads with a known 3-peer set so each +// q-filter test can focus on the post-fetch substring-match behaviour +// without re-stating the full query expectations. +func peersFilterFixture(t *testing.T) (*DiscoveryHandler, sqlmock.Sqlmock) { + t.Helper() + mock := setupTestDB(t) + setupTestRedis(t) + + mock.ExpectQuery("SELECT parent_id FROM workspaces WHERE id ="). + WithArgs("ws-self"). + WillReturnRows(sqlmock.NewRows([]string{"parent_id"}).AddRow("ws-pm")) + + cols := []string{"id", "name", "role", "tier", "status", "agent_card", "url", "parent_id", "active_tasks"} + + mock.ExpectQuery("SELECT w.id, w.name.*WHERE w.parent_id = \\$1 AND w.id != \\$2"). + WithArgs("ws-pm", "ws-self"). + WillReturnRows(sqlmock.NewRows(cols). + AddRow("ws-alpha", "Alpha Researcher", "researcher", 1, "online", []byte("null"), "http://a", "ws-pm", 0). + AddRow("ws-beta", "Beta Designer", "designer", 1, "online", []byte("null"), "http://b", "ws-pm", 0)) + + mock.ExpectQuery("SELECT w.id, w.name.*WHERE w.parent_id = \\$1 AND w.status"). + WithArgs("ws-self"). + WillReturnRows(sqlmock.NewRows(cols)) + + mock.ExpectQuery("SELECT w.id, w.name.*WHERE w.id = \\$1 AND w.status"). + WithArgs("ws-pm"). + WillReturnRows(sqlmock.NewRows(cols). + AddRow("ws-pm", "PM Workspace", "manager", 2, "online", []byte("null"), "http://pm", nil, 1)) + + return NewDiscoveryHandler(), mock +} + +func runPeersWithQuery(t *testing.T, handler *DiscoveryHandler, q string) []map[string]interface{} { + t.Helper() + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "id", Value: "ws-self"}} + url := "/registry/ws-self/peers" + if q != "" { + url += "?q=" + q + } + c.Request = httptest.NewRequest("GET", url, nil) + handler.Peers(c) + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var peers []map[string]interface{} + if err := json.Unmarshal(w.Body.Bytes(), &peers); err != nil { + t.Fatalf("parse response: %v", err) + } + return peers +} + +// TestPeers_NoQ_ReturnsAll locks in the regression baseline: without ?q, +// the handler returns the full 3-peer fixture (alpha + beta + pm). +func TestPeers_NoQ_ReturnsAll(t *testing.T) { + handler, mock := peersFilterFixture(t) + peers := runPeersWithQuery(t, handler, "") + if len(peers) != 3 { + t.Errorf("no-q: expected 3 peers, got %d (%v)", len(peers), peerNames(peers)) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestPeers_Q_FiltersByName covers the primary CWE-862 fix path. +func TestPeers_Q_FiltersByName(t *testing.T) { + handler, mock := peersFilterFixture(t) + peers := runPeersWithQuery(t, handler, "alpha") + if len(peers) != 1 || peers[0]["id"] != "ws-alpha" { + t.Errorf("q=alpha: expected just ws-alpha, got %v", peerNames(peers)) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestPeers_Q_CaseInsensitive guards against a future strings.Contains +// without ToLower regression. +func TestPeers_Q_CaseInsensitive(t *testing.T) { + handler, mock := peersFilterFixture(t) + peers := runPeersWithQuery(t, handler, "ALPHA") + if len(peers) != 1 || peers[0]["id"] != "ws-alpha" { + t.Errorf("q=ALPHA: expected ws-alpha, got %v", peerNames(peers)) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestPeers_Q_FiltersByRole — role-based match (designer is the role of +// ws-beta; the substring "design" must surface beta and only beta). +func TestPeers_Q_FiltersByRole(t *testing.T) { + handler, mock := peersFilterFixture(t) + peers := runPeersWithQuery(t, handler, "design") + if len(peers) != 1 || peers[0]["id"] != "ws-beta" { + t.Errorf("q=design: expected ws-beta, got %v", peerNames(peers)) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestPeers_Q_NoMatches returns an empty array, not null. +func TestPeers_Q_NoMatches(t *testing.T) { + handler, mock := peersFilterFixture(t) + peers := runPeersWithQuery(t, handler, "nonexistent") + if len(peers) != 0 { + t.Errorf("q=nonexistent: expected 0 peers, got %d (%v)", len(peers), peerNames(peers)) + } + // JSON should serialize as `[]` not `null` — re-encode and inspect. + if got, _ := json.Marshal(peers); string(got) != "[]" { + t.Errorf("expected JSON [], got %s", string(got)) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +// TestPeers_Q_WhitespaceOnly is treated as an empty filter. +func TestPeers_Q_WhitespaceOnly(t *testing.T) { + handler, mock := peersFilterFixture(t) + peers := runPeersWithQuery(t, handler, "%20%20") + if len(peers) != 3 { + t.Errorf("q=whitespace: expected unfiltered 3 peers, got %d", len(peers)) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +func peerNames(peers []map[string]interface{}) []string { + out := make([]string, 0, len(peers)) + for _, p := range peers { + if id, ok := p["id"].(string); ok { + out = append(out, id) + } + } + return out +} + // ==================== CheckAccess ==================== func TestCheckAccess_BadJSON(t *testing.T) { From 641b1391e2a2c4782b528880408a3f393d421f0c Mon Sep 17 00:00:00 2001 From: rabbitblood Date: Sun, 26 Apr 2026 01:02:19 -0700 Subject: [PATCH 2/2] refactor(discovery): apply simplify findings on #1038 PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-quality + efficiency review of PR #2081: - Drop comma-ok on map type-asserts in filterPeersByQuery — queryPeerMaps writes name/role unconditionally as string, so the silent-empty-string fallback was cargo-culted defense that would HIDE a real upstream shape change in tests rather than surface it. Plain p["name"].(string) panics on violation, caught by tests. - Trim filterPeersByQuery doc from 5 lines to 1 — function is 15 lines and self-evident. - Refactor 6 separate Test functions into one table-driven TestPeers_QFilter with 6 sub-tests. Net ~80 lines saved + naming becomes readable subtest names instead of TestPeers_Q_Foo_Bar. - Set-based peer-id comparison (peerIDSet) replaces fragile peers[0]["id"] == "ws-alpha" asserts that would silently mask a future sort/order regression on the production code. - Fix the broken TestPeers_Q_NoMatches assertion: re-encoding an unmarshalled []map collapses both null and [] to [], so the previous json.Marshal(peers) == "[]" check was tautological. Move the [] vs null distinction to a dedicated test (TestPeers_Q_NoMatches_RawBodyIsArrayNotNull) that inspects the recorder body BEFORE unmarshal. runPeersWithQuery now returns both parsed peers and raw body so the nil-guard test can use the bytes directly. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../internal/handlers/discovery.go | 13 +- .../internal/handlers/discovery_test.go | 180 +++++++++--------- 2 files changed, 94 insertions(+), 99 deletions(-) diff --git a/workspace-server/internal/handlers/discovery.go b/workspace-server/internal/handlers/discovery.go index fbc2cc2a..6422e002 100644 --- a/workspace-server/internal/handlers/discovery.go +++ b/workspace-server/internal/handlers/discovery.go @@ -257,13 +257,8 @@ func (h *DiscoveryHandler) Peers(c *gin.Context) { c.JSON(http.StatusOK, peers) } -// filterPeersByQuery returns the subset of peers whose name or role -// case-insensitively contains q. An empty/whitespace-only q is a no-op -// (returns the input slice unchanged) so the no-filter path stays free -// of allocations. Map values come from queryPeerMaps which always -// stringifies name + role, so the type asserts here cannot fail in -// practice — the comma-ok form belt-and-braces against future shape -// changes upstream. +// filterPeersByQuery returns peers whose name or role case-insensitively +// contains q. Whitespace-trimmed empty q is a no-op (returns input unchanged). func filterPeersByQuery(peers []map[string]interface{}, q string) []map[string]interface{} { q = strings.TrimSpace(q) if q == "" { @@ -272,8 +267,8 @@ func filterPeersByQuery(peers []map[string]interface{}, q string) []map[string]i needle := strings.ToLower(q) out := make([]map[string]interface{}, 0, len(peers)) for _, p := range peers { - name, _ := p["name"].(string) - role, _ := p["role"].(string) + name := p["name"].(string) + role := p["role"].(string) if strings.Contains(strings.ToLower(name), needle) || strings.Contains(strings.ToLower(role), needle) { out = append(out, p) diff --git a/workspace-server/internal/handlers/discovery_test.go b/workspace-server/internal/handlers/discovery_test.go index a027723f..e2368aac 100644 --- a/workspace-server/internal/handlers/discovery_test.go +++ b/workspace-server/internal/handlers/discovery_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" "github.com/DATA-DOG/go-sqlmock" @@ -260,9 +261,10 @@ func TestPeers_RootWorkspace_NoPeers(t *testing.T) { // ==================== Peers — ?q= filter (#1038) ==================== -// peersFilterFixture mocks all 3 SQL reads with a known 3-peer set so each -// q-filter test can focus on the post-fetch substring-match behaviour -// without re-stating the full query expectations. +// peersFilterFixture mocks the 4 SQL reads (parent_id lookup + siblings + +// children + parent) with a known 3-peer set so each q-filter test can +// focus on the post-fetch substring-match behaviour. Returns the handler +// and the live mock so callers can assert ExpectationsWereMet at the end. func peersFilterFixture(t *testing.T) (*DiscoveryHandler, sqlmock.Sqlmock) { t.Helper() mock := setupTestDB(t) @@ -292,7 +294,13 @@ func peersFilterFixture(t *testing.T) (*DiscoveryHandler, sqlmock.Sqlmock) { return NewDiscoveryHandler(), mock } -func runPeersWithQuery(t *testing.T, handler *DiscoveryHandler, q string) []map[string]interface{} { +// runPeersWithQuery invokes Peers and returns BOTH the parsed peers and +// the raw response body. The raw body is needed by TestPeers_Q_NoMatches +// to distinguish JSON `[]` (intended) from `null` (regression of the +// nil-guard at line 254-256) — once unmarshalled, both collapse to +// len==0 and re-marshal to `[]`, so checking only the parsed value is +// tautological. +func runPeersWithQuery(t *testing.T, handler *DiscoveryHandler, q string) ([]map[string]interface{}, []byte) { t.Helper() w := httptest.NewRecorder() c, _ := gin.CreateTestContext(w) @@ -306,98 +314,90 @@ func runPeersWithQuery(t *testing.T, handler *DiscoveryHandler, q string) []map[ if w.Code != http.StatusOK { t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) } + body := w.Body.Bytes() var peers []map[string]interface{} - if err := json.Unmarshal(w.Body.Bytes(), &peers); err != nil { + if err := json.Unmarshal(body, &peers); err != nil { t.Fatalf("parse response: %v", err) } - return peers + return peers, body } -// TestPeers_NoQ_ReturnsAll locks in the regression baseline: without ?q, -// the handler returns the full 3-peer fixture (alpha + beta + pm). -func TestPeers_NoQ_ReturnsAll(t *testing.T) { - handler, mock := peersFilterFixture(t) - peers := runPeersWithQuery(t, handler, "") - if len(peers) != 3 { - t.Errorf("no-q: expected 3 peers, got %d (%v)", len(peers), peerNames(peers)) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -// TestPeers_Q_FiltersByName covers the primary CWE-862 fix path. -func TestPeers_Q_FiltersByName(t *testing.T) { - handler, mock := peersFilterFixture(t) - peers := runPeersWithQuery(t, handler, "alpha") - if len(peers) != 1 || peers[0]["id"] != "ws-alpha" { - t.Errorf("q=alpha: expected just ws-alpha, got %v", peerNames(peers)) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -// TestPeers_Q_CaseInsensitive guards against a future strings.Contains -// without ToLower regression. -func TestPeers_Q_CaseInsensitive(t *testing.T) { - handler, mock := peersFilterFixture(t) - peers := runPeersWithQuery(t, handler, "ALPHA") - if len(peers) != 1 || peers[0]["id"] != "ws-alpha" { - t.Errorf("q=ALPHA: expected ws-alpha, got %v", peerNames(peers)) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -// TestPeers_Q_FiltersByRole — role-based match (designer is the role of -// ws-beta; the substring "design" must surface beta and only beta). -func TestPeers_Q_FiltersByRole(t *testing.T) { - handler, mock := peersFilterFixture(t) - peers := runPeersWithQuery(t, handler, "design") - if len(peers) != 1 || peers[0]["id"] != "ws-beta" { - t.Errorf("q=design: expected ws-beta, got %v", peerNames(peers)) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -// TestPeers_Q_NoMatches returns an empty array, not null. -func TestPeers_Q_NoMatches(t *testing.T) { - handler, mock := peersFilterFixture(t) - peers := runPeersWithQuery(t, handler, "nonexistent") - if len(peers) != 0 { - t.Errorf("q=nonexistent: expected 0 peers, got %d (%v)", len(peers), peerNames(peers)) - } - // JSON should serialize as `[]` not `null` — re-encode and inspect. - if got, _ := json.Marshal(peers); string(got) != "[]" { - t.Errorf("expected JSON [], got %s", string(got)) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -// TestPeers_Q_WhitespaceOnly is treated as an empty filter. -func TestPeers_Q_WhitespaceOnly(t *testing.T) { - handler, mock := peersFilterFixture(t) - peers := runPeersWithQuery(t, handler, "%20%20") - if len(peers) != 3 { - t.Errorf("q=whitespace: expected unfiltered 3 peers, got %d", len(peers)) - } - if err := mock.ExpectationsWereMet(); err != nil { - t.Errorf("unmet sqlmock expectations: %v", err) - } -} - -func peerNames(peers []map[string]interface{}) []string { - out := make([]string, 0, len(peers)) +// peerIDSet returns the set of peer ids — order-independent comparison +// avoids fragile peers[0]["id"] asserts that would silently mask a future +// sort/order change. +func peerIDSet(peers []map[string]interface{}) map[string]struct{} { + out := make(map[string]struct{}, len(peers)) for _, p := range peers { - if id, ok := p["id"].(string); ok { - out = append(out, id) - } + out[p["id"].(string)] = struct{}{} + } + return out +} + +// TestPeers_QFilter covers the rule classifier — append-order +// independent (uses set membership) so a future sort regression on the +// production code can't slip through. NoMatches has its own raw-body +// check (see TestPeers_Q_NoMatches_RawBodyIsArrayNotNull below) because +// the `[]` vs `null` distinction collapses after json.Unmarshal. +func TestPeers_QFilter(t *testing.T) { + cases := []struct { + name string + q string + wantIDs []string + }{ + {"no-q returns all", "", []string{"ws-alpha", "ws-beta", "ws-pm"}}, + {"name match", "alpha", []string{"ws-alpha"}}, + {"name match case-insensitive", "ALPHA", []string{"ws-alpha"}}, + {"role match", "design", []string{"ws-beta"}}, + {"no matches", "nonexistent", nil}, + {"whitespace-only is no-op", "%20%20", []string{"ws-alpha", "ws-beta", "ws-pm"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + handler, mock := peersFilterFixture(t) + peers, _ := runPeersWithQuery(t, handler, tc.q) + + got := peerIDSet(peers) + want := make(map[string]struct{}, len(tc.wantIDs)) + for _, id := range tc.wantIDs { + want[id] = struct{}{} + } + if len(got) != len(want) { + t.Fatalf("len: got %d %v, want %d %v", len(got), keysOf(got), len(want), tc.wantIDs) + } + for id := range want { + if _, ok := got[id]; !ok { + t.Errorf("missing id %q (got %v, want %v)", id, keysOf(got), tc.wantIDs) + } + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } + }) + } +} + +// TestPeers_Q_NoMatches_RawBodyIsArrayNotNull verifies the `peers = make(...)` +// nil-guard at the end of Peers — when filtering reduces the slice to +// non-nil-but-empty AND the original was nil, JSON must be `[]` not `null`. +// This is the assertion the previous TestPeers_Q_NoMatches falsely claimed +// to make: re-encoding an unmarshalled []map collapses null and [] both +// to []. The fix here checks the recorder body bytes BEFORE unmarshal. +func TestPeers_Q_NoMatches_RawBodyIsArrayNotNull(t *testing.T) { + handler, mock := peersFilterFixture(t) + _, body := runPeersWithQuery(t, handler, "nonexistent") + got := strings.TrimSpace(string(body)) + if got != "[]" { + t.Errorf("raw body: got %q, want []", got) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("unmet sqlmock expectations: %v", err) + } +} + +func keysOf(m map[string]struct{}) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) } return out }