From 0f565e4aae17559e6affaaa648fa07695b047892 Mon Sep 17 00:00:00 2001 From: efiten Date: Sun, 19 Apr 2026 16:46:35 +0200 Subject: [PATCH] fix(geo-prune): address PR #738 review feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix TOCTOU race: confirm now requires pubkeys from preview in request body; server intersects with still-outside nodes so exactly the previewed set is deleted (no more, no less) - Add cascade comment to DeleteNodesByPubkeys documenting that only the nodes table is affected (no FK constraints today) - Log each deleted node by name + pubkey for operator visibility - Return deleted node list in confirm response so UI shows what happened - Check lat/lon nil directly instead of passing 0.0 to NodePassesGeoFilter - Update confirm test to send pubkeys body; add test for missing body → 400 Co-Authored-By: Claude Sonnet 4.6 --- cmd/server/db.go | 2 ++ cmd/server/routes.go | 42 +++++++++++++++++++++++++++++---------- cmd/server/routes_test.go | 29 ++++++++++++++++++++++----- public/customize-v2.js | 7 +++++-- 4 files changed, 63 insertions(+), 17 deletions(-) diff --git a/cmd/server/db.go b/cmd/server/db.go index f065a72e..82ae2082 100644 --- a/cmd/server/db.go +++ b/cmd/server/db.go @@ -2384,6 +2384,8 @@ func (db *DB) GetNodesForGeoPrune() ([]NodeForGeoPrune, error) { } // DeleteNodesByPubkeys deletes nodes by their public keys and returns the count deleted. +// Only the nodes table is affected — references in transmissions or other tables are +// not cascaded (no FK constraints exist today; revisit if schema adds them). func (db *DB) DeleteNodesByPubkeys(pubkeys []string) (int64, error) { if len(pubkeys) == 0 { return 0, nil diff --git a/cmd/server/routes.go b/cmd/server/routes.go index c504b322..6e027ad9 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -2654,6 +2654,8 @@ func (s *Server) handleAdminPrune(w http.ResponseWriter, r *http.Request) { // handlePruneGeoFilter identifies (dry_run=true, default) or deletes (confirm=true) // nodes whose GPS coordinates fall outside the currently configured geo_filter. // Nodes with no GPS fix are always kept. Requires geo_filter to be configured. +// Confirm requires the pubkeys from the preview in the request body to prevent +// TOCTOU races: only nodes in the passed list AND still outside the filter are deleted. func (s *Server) handlePruneGeoFilter(w http.ResponseWriter, r *http.Request) { if s.cfg.GeoFilter == nil || len(s.cfg.GeoFilter.Polygon) < 3 { writeError(w, http.StatusBadRequest, "no geo_filter configured") @@ -2675,14 +2677,10 @@ func (s *Server) handlePruneGeoFilter(w http.ResponseWriter, r *http.Request) { var outside []nodeResult for _, n := range nodes { - var lat, lon float64 - if n.Lat != nil { - lat = *n.Lat + if n.Lat == nil || n.Lon == nil { + continue // no GPS — always keep } - if n.Lon != nil { - lon = *n.Lon - } - if !NodePassesGeoFilter(lat, lon, s.cfg.GeoFilter) { + if !NodePassesGeoFilter(*n.Lat, *n.Lon, s.cfg.GeoFilter) { outside = append(outside, nodeResult{PubKey: n.PubKey, Name: n.Name, Lat: n.Lat, Lon: n.Lon}) } } @@ -2697,9 +2695,29 @@ func (s *Server) handlePruneGeoFilter(w http.ResponseWriter, r *http.Request) { return } - // Confirmed — delete the nodes - pubkeys := make([]string, len(outside)) - for i, n := range outside { + // Confirmed delete — require pubkeys from the preview to prevent TOCTOU: + // only nodes that were shown in preview AND are still outside the filter are deleted. + var body struct { + Pubkeys []string `json:"pubkeys"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil || len(body.Pubkeys) == 0 { + writeError(w, http.StatusBadRequest, "confirm requires pubkeys from preview in request body") + return + } + allowed := make(map[string]bool, len(body.Pubkeys)) + for _, pk := range body.Pubkeys { + allowed[pk] = true + } + + var toDelete []nodeResult + for _, n := range outside { + if allowed[n.PubKey] { + toDelete = append(toDelete, n) + } + } + + pubkeys := make([]string, len(toDelete)) + for i, n := range toDelete { pubkeys[i] = n.PubKey } deleted, err := s.db.DeleteNodesByPubkeys(pubkeys) @@ -2707,10 +2725,14 @@ func (s *Server) handlePruneGeoFilter(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusInternalServerError, "delete failed") return } + for _, n := range toDelete { + log.Printf("[geo-prune] deleted node %q (%s)", n.Name, n.PubKey) + } log.Printf("[geo-prune] deleted %d nodes outside geo filter", deleted) writeJSON(w, map[string]interface{}{ "dryRun": false, "deleted": deleted, + "nodes": toDelete, }) } diff --git a/cmd/server/routes_test.go b/cmd/server/routes_test.go index 3e01761e..8d0d1a49 100644 --- a/cmd/server/routes_test.go +++ b/cmd/server/routes_test.go @@ -4263,23 +4263,29 @@ func TestPruneGeoFilterEndpoint(t *testing.T) { t.Run("confirm=true deletes outside nodes", func(t *testing.T) { srv, router := setupPruneGeoFilterServer(t, apiKey, gf) - req := httptest.NewRequest("POST", "/api/admin/prune-geo-filter?confirm=true", nil) + body := strings.NewReader(`{"pubkeys":["aaaa111122223333"]}`) + req := httptest.NewRequest("POST", "/api/admin/prune-geo-filter?confirm=true", body) req.Header.Set("X-API-Key", apiKey) + req.Header.Set("Content-Type", "application/json") w := httptest.NewRecorder() router.ServeHTTP(w, req) if w.Code != 200 { t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) } - var body map[string]interface{} - json.Unmarshal(w.Body.Bytes(), &body) - if body["dryRun"] != false { + var resp map[string]interface{} + json.Unmarshal(w.Body.Bytes(), &resp) + if resp["dryRun"] != false { t.Error("expected dryRun=false") } - deleted, _ := body["deleted"].(float64) + deleted, _ := resp["deleted"].(float64) if deleted != 1 { t.Errorf("expected 1 deleted, got %v", deleted) } + nodes, _ := resp["nodes"].([]interface{}) + if len(nodes) != 1 { + t.Errorf("expected 1 node in response, got %d", len(nodes)) + } // Verify node is actually gone from DB var count int @@ -4294,6 +4300,19 @@ func TestPruneGeoFilterEndpoint(t *testing.T) { } }) + t.Run("confirm=true without pubkeys body returns 400", func(t *testing.T) { + _, router := setupPruneGeoFilterServer(t, apiKey, gf) + + req := httptest.NewRequest("POST", "/api/admin/prune-geo-filter?confirm=true", nil) + req.Header.Set("X-API-Key", apiKey) + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + + if w.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d: %s", w.Code, w.Body.String()) + } + }) + t.Run("returns 400 when no geo filter configured", func(t *testing.T) { _, router := setupPruneGeoFilterServer(t, apiKey, nil) diff --git a/public/customize-v2.js b/public/customize-v2.js index af5c9ceb..d0d1cbce 100644 --- a/public/customize-v2.js +++ b/public/customize-v2.js @@ -1464,9 +1464,11 @@ if (!apiKey) { _gfPruneMsg(container, 'API key required.', false); return; } var count = _gfPruneNodes.length; if (!confirm('Delete ' + count + ' node' + (count !== 1 ? 's' : '') + ' from the database? This cannot be undone.')) return; + var pubkeys = _gfPruneNodes.map(function (n) { return n.pubkey; }); fetch('/api/admin/prune-geo-filter?confirm=true', { method: 'POST', - headers: { 'X-API-Key': apiKey } + headers: { 'X-API-Key': apiKey, 'Content-Type': 'application/json' }, + body: JSON.stringify({ pubkeys: pubkeys }) }).then(function (r) { if (!r.ok) return r.json().then(function (e) { throw new Error(e.error || ('HTTP ' + r.status)); }); return r.json(); @@ -1474,7 +1476,8 @@ _gfPruneNodes = []; var resultEl = container.querySelector('#cv2-gf-prune-result'); if (resultEl) resultEl.style.display = 'none'; - _gfPruneMsg(container, 'Deleted ' + data.deleted + ' node' + (data.deleted !== 1 ? 's' : '') + '.', true); + var n = data.deleted; + _gfPruneMsg(container, 'Deleted ' + n + ' node' + (n !== 1 ? 's' : '') + '.', true); }).catch(function (e) { _gfPruneMsg(container, 'Error: ' + e.message, false); }); }