From ffb2c842af01c2ec29ccb3383be03880c6e0f95a Mon Sep 17 00:00:00 2001 From: dborup Date: Sat, 18 Jul 2026 20:01:14 +0200 Subject: [PATCH] fix: make handleNodes' geo_filter exclusion opt-in (?geoFilter=1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Configuring geo_filter alone was silently changing what GET /api/nodes returned — every node outside the polygon and not yet foreign_advert -tagged (which only happens on that node's next ADVERT after geo_filter is set) vanished from every view built on this endpoint, including the live map. There was no way to see them again short of waiting for each one to re-advertise. geo_filter's own purposes — the ingestor tagging foreign adverts, and the explicit prune-geo-filter admin flow — are untouched; this only gates the passive node-list declutter view behind an explicit query param, so turning geo_filter on to build analytics (e.g. the Foreign Traffic tab) doesn't have the side effect of hiding nodes from the map. Co-Authored-By: Claude Sonnet 5 --- cmd/server/nodes_geofilter_optin_test.go | 79 ++++++++++++++++++++++++ cmd/server/openapi.go | 1 + cmd/server/routes.go | 11 +++- 3 files changed, 90 insertions(+), 1 deletion(-) create mode 100644 cmd/server/nodes_geofilter_optin_test.go diff --git a/cmd/server/nodes_geofilter_optin_test.go b/cmd/server/nodes_geofilter_optin_test.go new file mode 100644 index 00000000..c96f29a2 --- /dev/null +++ b/cmd/server/nodes_geofilter_optin_test.go @@ -0,0 +1,79 @@ +package main + +import ( + "encoding/json" + "net/http/httptest" + "testing" +) + +// Regression test: configuring geo_filter alone must not change what +// GET /api/nodes returns. Before this fix, setting geo_filter silently +// hid every node outside the polygon that hadn't yet been re-tagged +// foreign_advert=1 by the ingestor (which only happens on that node's +// next ADVERT) — including the live map, which lists straight off this +// endpoint. The filter is now opt-in via ?geoFilter=1. +func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) { + apiKey := "a-strong-api-key-for-testing" + srv, router, _ := setupGeoFilterServer(t, apiKey) + srv.setGeoFilter(&GeoFilterConfig{ + LatMin: floatPtr(53.0), LatMax: floatPtr(59.0), + LonMin: floatPtr(6.0), LonMax: floatPtr(15.0), + }) + + mustExecDB(t, srv.db, `INSERT INTO nodes (public_key, name, lat, lon, foreign_advert) VALUES ('pk-inside', 'InsideNode', 55.7, 10.5, 0)`) + mustExecDB(t, srv.db, `INSERT INTO nodes (public_key, name, lat, lon, foreign_advert) VALUES ('pk-outside-untagged', 'OutsideUntagged', 44.4, 26.1, 0)`) + mustExecDB(t, srv.db, `INSERT INTO nodes (public_key, name, lat, lon, foreign_advert) VALUES ('pk-outside-tagged', 'OutsideTagged', 52.4, 10.8, 1)`) + + names := func(w *httptest.ResponseRecorder) map[string]bool { + var body map[string]interface{} + if err := json.Unmarshal(w.Body.Bytes(), &body); err != nil { + t.Fatalf("unmarshal: %v", err) + } + nodes, ok := body["nodes"].([]interface{}) + if !ok { + t.Fatal("expected nodes array") + } + out := make(map[string]bool, len(nodes)) + for _, n := range nodes { + m := n.(map[string]interface{}) + out[m["name"].(string)] = true + } + return out + } + + t.Run("default request returns every node regardless of geo_filter", func(t *testing.T) { + req := httptest.NewRequest("GET", "/api/nodes?limit=50", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + if w.Code != 200 { + t.Fatalf("expected 200, got %d", w.Code) + } + got := names(w) + for _, want := range []string{"InsideNode", "OutsideUntagged", "OutsideTagged"} { + if !got[want] { + t.Errorf("expected %s in default (unfiltered) response, got %v", want, got) + } + } + }) + + t.Run("geoFilter=1 excludes untagged out-of-polygon nodes but keeps foreign-tagged ones", func(t *testing.T) { + req := httptest.NewRequest("GET", "/api/nodes?limit=50&geoFilter=1", nil) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + if w.Code != 200 { + t.Fatalf("expected 200, got %d", w.Code) + } + got := names(w) + if !got["InsideNode"] { + t.Error("expected InsideNode (within polygon) to be present") + } + if !got["OutsideTagged"] { + t.Error("expected OutsideTagged (foreign_advert=1) to be present even though it's outside the polygon — #730") + } + if got["OutsideUntagged"] { + t.Error("expected OutsideUntagged (outside polygon, not yet foreign-tagged) to be excluded when geoFilter=1") + } + }) +} + +func floatPtr(f float64) *float64 { return &f } diff --git a/cmd/server/openapi.go b/cmd/server/openapi.go index afd233db..46464ecb 100644 --- a/cmd/server/openapi.go +++ b/cmd/server/openapi.go @@ -80,6 +80,7 @@ func routeDescriptions() map[string]routeMeta { QueryParams: []paramMeta{ {Name: "role", Description: "Filter by node role", Type: "string"}, {Name: "status", Description: "Filter by status (active/stale/offline)", Type: "string"}, + {Name: "geoFilter", Description: "Set to \"1\" to exclude nodes outside the configured geo_filter (unless foreign_advert-tagged). Opt-in — omitting it returns every node regardless of geo_filter.", Type: "string"}, }}, "GET /api/nodes/search": {Summary: "Search nodes", Description: "Search nodes by name or public key prefix.", Tag: "nodes", QueryParams: []paramMeta{{Name: "q", Description: "Search query", Type: "string", Required: true}}}, "GET /api/nodes/bulk-health": {Summary: "Bulk node health", Description: "Returns health status for all nodes in one call.", Tag: "nodes"}, diff --git a/cmd/server/routes.go b/cmd/server/routes.go index 9f47fe9f..d56f0207 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -1426,7 +1426,16 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) { } } } - if s.cfg.GeoFilter != nil { + // Opt-in only (?geoFilter=1): configuring geo_filter alone must not + // change what /api/nodes (and therefore the live map, which lists + // straight off this endpoint) returns by default. It used to — any node + // outside the polygon and not YET foreign_advert-tagged (which only + // happens on that node's next ADVERT after geo_filter was configured) + // would silently vanish from every view the moment geo_filter was set, + // with no per-request way to see them anyway. geo_filter's own + // ingestor-side tagging (and the explicit prune-geo-filter admin flow) + // are unaffected by this — this only gates the passive declutter view. + if s.cfg.GeoFilter != nil && q.Get("geoFilter") == "1" { filtered := nodes[:0] for _, node := range nodes { // Foreign-flagged nodes (#730) are kept even when their GPS lies