From 7cc5aae6ace8efcb92fcbbd4080c080b09454b2f Mon Sep 17 00:00:00 2001 From: dborup Date: Sat, 18 Jul 2026 21:42:08 +0200 Subject: [PATCH] fix: add GeoFilterAppliesToNodeList escape hatch for the geoFilter opt-in flip MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per bot review on PR #1852 (comment 5012514227): the ?geoFilter=1 opt-in change (ffb2c84) flips a public API's default behavior with no migration path. On THIS deployment geo_filter was never configured before this session, so nothing was ever relying on the old filtered behavior here — but the PR targets the shared upstream codebase, and any other deployment that already had geo_filter configured (the pre-existing #730 declutter feature) would silently start getting an unfiltered node list after upgrading to this code, with no way back. Adds Config.GeoFilterAppliesToNodeList (default false, matching the new non-surprising behavior for anyone configuring geo_filter fresh). A deployment that intentionally relies on the old always-on filtering sets it to true in config.json to keep exactly the behavior it had before this PR. Co-Authored-By: Claude Sonnet 5 --- cmd/server/config.go | 8 ++++++++ cmd/server/nodes_geofilter_optin_test.go | 22 +++++++++++++++++++++ cmd/server/openapi.go | 2 +- cmd/server/routes.go | 25 ++++++++++++++---------- 4 files changed, 46 insertions(+), 11 deletions(-) diff --git a/cmd/server/config.go b/cmd/server/config.go index c461eaf1..cd767c54 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -142,6 +142,14 @@ type Config struct { // GOMEMLIMIT environment variable, when set, takes precedence. Runtime *RuntimeConfig `json:"runtime,omitempty"` GeoFilter *GeoFilterConfig `json:"geo_filter,omitempty"` + // GeoFilterAppliesToNodeList restores the pre-opt-in behavior where + // configuring geo_filter alone made GET /api/nodes exclude out-of- + // polygon nodes by default (no ?geoFilter=1 needed). Off by default — + // deployments that already relied on the old always-on filtering + // (added under #730) can set this to keep it; new deployments get the + // non-surprising default of geo_filter not silently changing what the + // node list/map returns. + GeoFilterAppliesToNodeList bool `json:"geoFilterAppliesToNodeList,omitempty"` Areas map[string]AreaEntry `json:"areas,omitempty"` diff --git a/cmd/server/nodes_geofilter_optin_test.go b/cmd/server/nodes_geofilter_optin_test.go index c96f29a2..cbdd310b 100644 --- a/cmd/server/nodes_geofilter_optin_test.go +++ b/cmd/server/nodes_geofilter_optin_test.go @@ -74,6 +74,28 @@ func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) { t.Error("expected OutsideUntagged (outside polygon, not yet foreign-tagged) to be excluded when geoFilter=1") } }) + + t.Run("GeoFilterAppliesToNodeList=true restores the pre-opt-in always-on behavior without needing ?geoFilter=1", func(t *testing.T) { + srv.cfg.GeoFilterAppliesToNodeList = true + defer func() { srv.cfg.GeoFilterAppliesToNodeList = false }() + + 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) + 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 to be excluded by default when GeoFilterAppliesToNodeList=true, without needing ?geoFilter=1") + } + }) } func floatPtr(f float64) *float64 { return &f } diff --git a/cmd/server/openapi.go b/cmd/server/openapi.go index 46464ecb..36c12843 100644 --- a/cmd/server/openapi.go +++ b/cmd/server/openapi.go @@ -80,7 +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"}, + {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, unless the deployment's config.json sets geoFilterAppliesToNodeList=true to restore the pre-opt-in always-on behavior.", 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 d56f0207..513a7773 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -1426,16 +1426,21 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) { } } } - // 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" { + // Opt-in by default (?geoFilter=1), unless the deployment has + // GeoFilterAppliesToNodeList set to restore the pre-existing always-on + // behavior. Configuring geo_filter alone must not, by default, change + // what /api/nodes (and therefore the live map, which lists straight off + // this endpoint) returns — 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 otherwise silently vanish from + // every view the moment geo_filter was set, with no per-request way to + // see them anyway. A deployment that intentionally relies on the old + // #730 declutter behavior can set GeoFilterAppliesToNodeList to keep + // it. geo_filter's own ingestor-side tagging (and the explicit + // prune-geo-filter admin flow) are unaffected either way — this only + // gates the passive declutter view. + applyGeoFilter := s.cfg.GeoFilterAppliesToNodeList || q.Get("geoFilter") == "1" + if s.cfg.GeoFilter != nil && applyGeoFilter { filtered := nodes[:0] for _, node := range nodes { // Foreign-flagged nodes (#730) are kept even when their GPS lies