From 7141acca7aed76a92f4aa325cbfd356c69543e58 Mon Sep 17 00:00:00 2001 From: dborup Date: Sat, 18 Jul 2026 22:19:21 +0200 Subject: [PATCH] fix: address remaining bot-review findings on the geo_filter/Foreign Traffic work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - MAJOR: config.example.json was missing geoFilterExemptNodeList entirely — every other Config field in this repo gets a sibling _comment_ entry (established, actively-used convention throughout the file, ~20 existing examples), documented here to match. Also corrected the adjacent geo_filter _comment, which predates this PR and claimed it "restricts ingestion" — it doesn't; that's foreignAdverts.mode=drop's job, unrelated to this flag. - MINOR: ?geoFilter= only recognized the literal string "1" to enable and treated anything else (including "true") as disabling — a real footgun since "true" reads as an obviously valid boolean value. Now accepts 1/true and 0/false as explicit overrides; any other value (including absent) falls through to the deployment default instead of silently flipping it off. - Tightened the Foreign Traffic tab's stop-hook test: it previously only asserted stop() doesn't throw, which a no-op stub would also pass. Sandbox's setInterval/clearInterval are now real spies tracking live/cleared ids, so the test verifies render() registers exactly one interval and stop() actually clears that exact id (idempotently on a second call). - NIT: relays.sort now wraps unscoped_relay_count_24h in Number(...) defensively (AGENTS.md § Type Safety — cast at the boundary). - NIT: foreignNodes.sort now parses last_seen once per node (decorate-sort-undecorate) instead of twice per comparator call. Co-Authored-By: Claude Sonnet 5 --- cmd/server/nodes_geofilter_optin_test.go | 16 ++++++++++ cmd/server/openapi.go | 2 +- cmd/server/routes.go | 8 +++-- config.example.json | 4 ++- public/analytics.js | 12 +++++--- test-analytics-foreign-traffic-tab.js | 37 ++++++++++++++++++++++-- 6 files changed, 68 insertions(+), 11 deletions(-) diff --git a/cmd/server/nodes_geofilter_optin_test.go b/cmd/server/nodes_geofilter_optin_test.go index e2f54bd8..da739881 100644 --- a/cmd/server/nodes_geofilter_optin_test.go +++ b/cmd/server/nodes_geofilter_optin_test.go @@ -83,6 +83,22 @@ func TestHandleNodes_GeoFilterExcludedByDefault(t *testing.T) { } }) + t.Run("geoFilter=true/false are accepted synonyms for 1/0", func(t *testing.T) { + if !get("&geoFilter=false")["OutsideUntagged"] { + t.Error("expected OutsideUntagged to be present with geoFilter=false") + } + if get("&geoFilter=true")["OutsideUntagged"] { + t.Error("expected OutsideUntagged to be excluded with geoFilter=true") + } + }) + + t.Run("an unrecognized geoFilter value falls back to the deployment default instead of silently disabling", func(t *testing.T) { + got := get("&geoFilter=yes") + if got["OutsideUntagged"] { + t.Error("expected geoFilter=yes (not a recognized value) to fall back to the default (filtered), not silently disable filtering") + } + }) + t.Run("GeoFilterExemptNodeList=true makes the default request return everything, without needing ?geoFilter=0", func(t *testing.T) { srv.cfg.GeoFilterExemptNodeList = true defer func() { srv.cfg.GeoFilterExemptNodeList = false }() diff --git a/cmd/server/openapi.go b/cmd/server/openapi.go index a1f335d2..f5f92b78 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: "Overrides the deployment's geo_filter node-list default for this one request: \"1\" excludes nodes outside the configured geo_filter (unless foreign_advert-tagged), \"0\" returns every node regardless. Omitting it uses the deployment default — geo_filter applies to the node list unless config.json sets geoFilterExemptNodeList=true.", Type: "string"}, + {Name: "geoFilter", Description: "Overrides the deployment's geo_filter node-list default for this one request: \"1\"/\"true\" excludes nodes outside the configured geo_filter (unless foreign_advert-tagged), \"0\"/\"false\" returns every node regardless. Any other value (including omitting it) uses the deployment default — geo_filter applies to the node list unless config.json sets geoFilterExemptNodeList=true.", 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 168ed63c..b3780a28 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -1440,8 +1440,12 @@ func (s *Server) handleNodes(w http.ResponseWriter, r *http.Request) { // tagging (and the explicit prune-geo-filter admin flow) are // unaffected either way — this only gates the passive declutter view. applyGeoFilter := !s.cfg.GeoFilterExemptNodeList - if v := q.Get("geoFilter"); v != "" { - applyGeoFilter = v == "1" + switch q.Get("geoFilter") { + case "1", "true": + applyGeoFilter = true + case "0", "false": + applyGeoFilter = false + // Any other value (including absent) falls through to the deployment default. } if s.cfg.GeoFilter != nil && applyGeoFilter { filtered := nodes[:0] diff --git a/config.example.json b/config.example.json index b5c0f55b..b6f96731 100644 --- a/config.example.json +++ b/config.example.json @@ -238,12 +238,14 @@ [37.20, -122.52] ], "bufferKm": 20, - "_comment": "Optional. Restricts ingestion and API responses to nodes within the polygon + bufferKm. Polygon is an array of [lat, lon] pairs (minimum 3). Use the GeoFilter tab in the Customizer (requires apiKey) or the GeoFilter Builder (`/geofilter-builder.html`) to draw a polygon visually and export a config snippet. Remove this section to disable filtering. Nodes with no GPS fix are always allowed through." + "_comment": "Optional. By default, also restricts GET /api/nodes (and therefore the live map) to nodes within the polygon + bufferKm — foreign_advert-tagged nodes are always kept regardless (#730, see foreignAdverts below); see geoFilterExemptNodeList to opt out of that. The prune-geo-filter admin flow always uses this polygon regardless of geoFilterExemptNodeList. Polygon is an array of [lat, lon] pairs (minimum 3). Use the GeoFilter tab in the Customizer (requires apiKey) or the GeoFilter Builder (`/geofilter-builder.html`) to draw a polygon visually and export a config snippet. Remove this section to disable filtering entirely. Nodes with no GPS fix are always allowed through." }, "foreignAdverts": { "mode": "flag", "_comment": "Controls how the ingestor handles ADVERTs whose GPS is OUTSIDE the geo_filter polygon (#730). 'flag' (default): store the advert/node and tag it foreign_advert=1 so operators can see bridged/leaked nodes via the API ('foreign': true on /api/nodes). 'drop': legacy behavior — silently discard the advert (no log, no node row). Only applies when geo_filter is configured; otherwise has no effect." }, + "geoFilterExemptNodeList": false, + "_comment_geoFilterExemptNodeList": "Optional, default false. When geo_filter is configured, GET /api/nodes (and therefore the live map) excludes out-of-polygon nodes by default — the long-standing #730 declutter behavior, preserved automatically for every deployment upgrading from before this field existed. Set to true if you want geo_filter purely for foreign_advert classification/analytics (e.g. a 'Foreign Traffic' investigation) WITHOUT also hiding out-of-polygon nodes from the node list/map. Per-request ?geoFilter=0 / ?geoFilter=1 on /api/nodes overrides either default for a single call. Has no effect on ingestion (foreignAdverts.mode) or the prune-geo-filter admin flow, both of which are unaffected by this flag.", "areas": { "_comment": "Optional. GPS-based display filter. Each entry defines a geographic area by polygon ([lat, lon] pairs) or bounding box (latMin/latMax/lonMin/lonMax). Packets and nodes are attributed to an area based on the transmitting node's own GPS coordinates — not the observer's location. Areas appear as a filter pill bar in the dashboard. Remove this section to disable the area filter UI.", "BAY": { diff --git a/public/analytics.js b/public/analytics.js index dca98ad6..340d2887 100644 --- a/public/analytics.js +++ b/public/analytics.js @@ -4877,7 +4877,7 @@ function destroy() { _stopRolesRefresh(); _stopScopesRefresh(); _stopForeignTraf const relays = allNodes.filter(function(n) { return (n.role === 'repeater' || n.role === 'room') && (n.unscoped_relay_count_24h || 0) > 0; }); - relays.sort(function(a, b) { return (b.unscoped_relay_count_24h || 0) - (a.unscoped_relay_count_24h || 0); }); + relays.sort(function(a, b) { return Number(b.unscoped_relay_count_24h || 0) - Number(a.unscoped_relay_count_24h || 0); }); const foreignCount = allNodes.filter(function(n) { return n.foreign; }).length; let body; @@ -4909,9 +4909,13 @@ function destroy() { _stopRolesRefresh(); _stopScopesRefresh(); _stopForeignTraf // first. Only set going forward from when geo_filter was // configured — this list grows as nodes send their next ADVERT. const foreignNodes = allNodes.filter(function(n) { return n.foreign; }); - foreignNodes.sort(function(a, b) { - return new Date(b.last_seen || 0) - new Date(a.last_seen || 0); - }); + // Parse last_seen once per node (decorate-sort-undecorate) instead + // of twice per comparator call — cheap either way at this list + // size, but avoids the repeated Date() allocations on principle. + foreignNodes + .map(function(n) { return { n: n, t: new Date(n.last_seen || 0).getTime() }; }) + .sort(function(a, b) { return b.t - a.t; }) + .forEach(function(entry, i) { foreignNodes[i] = entry.n; }); let foreignNodesBody; if (foreignNodes.length > 0) { const foreignRows = foreignNodes.map(function(n) { diff --git a/test-analytics-foreign-traffic-tab.js b/test-analytics-foreign-traffic-tab.js index 2502e6a9..a83ecaf3 100644 --- a/test-analytics-foreign-traffic-tab.js +++ b/test-analytics-foreign-traffic-tab.js @@ -47,7 +47,7 @@ function makeSandbox() { console, Date, Infinity, Math, Array, Object, String, Number, JSON, RegExp, Error, TypeError, parseInt, parseFloat, isNaN, isFinite, encodeURIComponent, decodeURIComponent, - setTimeout: () => {}, clearTimeout: () => {}, setInterval: () => {}, clearInterval: () => {}, + setTimeout: () => {}, clearTimeout: () => {}, fetch: () => Promise.resolve({ json: () => Promise.resolve({}) }), performance: { now: () => Date.now() }, localStorage: (() => { const s = {}; return { getItem: k => s[k] || null, setItem: (k, v) => { s[k] = String(v); }, removeItem: k => { delete s[k]; } }; })(), @@ -59,6 +59,26 @@ function makeSandbox() { dispatchEvent: () => {}, requestAnimationFrame: (cb) => setTimeout(cb, 0), }; + // Spies (not just no-ops) so tests can verify a timer that was really + // registered gets really cleared, instead of only asserting stop() + // doesn't throw — a `function stop(){}` no-op would pass that alone. + // Defined as closures (not object methods) so they work under the + // sandboxed code's 'use strict', where a bare `setInterval(...)` call + // has `this === undefined`. + let nextIntervalId = 1; + const liveIntervalIds = new Set(); + const clearedIntervalIds = []; + ctx.__liveIntervalIds = liveIntervalIds; + ctx.__clearedIntervalIds = clearedIntervalIds; + ctx.setInterval = function () { + const id = nextIntervalId++; + liveIntervalIds.add(id); + return id; + }; + ctx.clearInterval = function (id) { + liveIntervalIds.delete(id); + clearedIntervalIds.push(id); + }; vm.createContext(ctx); return ctx; } @@ -191,15 +211,26 @@ function fakeEl() { assert.ok(el.innerHTML.includes('None yet'), 'placeholder message should be shown when no node is foreign-flagged'); }); - await testAsync('a stop-refresh hook is exported and safe to call repeatedly (idempotent)', async () => { + await testAsync('rendering registers a real interval, and stop() actually clears it (not a no-op)', async () => { const ctx = makeAnalyticsSandbox([]); const stop = ctx.window._analyticsStopForeignTrafficRefresh; assert.strictEqual(typeof stop, 'function', '_stopForeignTrafficRefresh must be exported for testing/cleanup'); - // Before any render, and after — must not throw either way. + + // Calling stop() before any render must not throw (no timer registered yet). stop(); + assert.strictEqual(ctx.__clearedIntervalIds.length, 0, 'stop() before any render should not call clearInterval at all'); + await ctx.window._analyticsRenderForeignTrafficTab(fakeEl()); + assert.strictEqual(ctx.__liveIntervalIds.size, 1, 'render should register exactly one live interval'); + const [registeredId] = ctx.__liveIntervalIds; + stop(); + assert.strictEqual(ctx.__liveIntervalIds.size, 0, 'stop() should leave zero live intervals'); + assert.deepStrictEqual(ctx.__clearedIntervalIds, [registeredId], 'stop() should call clearInterval with the exact id that render registered'); + + // Idempotent: calling stop() again with nothing live must not clear anything a second time. stop(); + assert.deepStrictEqual(ctx.__clearedIntervalIds, [registeredId], 'a second stop() call must not clear anything again — the timer reference should already be null'); }); console.log('\n════════════════════════════════════════');