diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index 1e8be8f62..567b418ae 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -86,6 +86,9 @@ jobs: - name: Staging disk-monitor unit tests (issue #1684) run: bash scripts/staging/test-disk-monitor.sh + - name: QA SQL parameter-binding unit tests (issue #1977) + run: bash qa/scripts/test-blacklist-sql.sh + - name: Lint CSS variables (issue #1128) run: | set -e @@ -100,6 +103,7 @@ jobs: node test-confidence-indicator.js node test-1659-analytics-warmup.js node test-channels-merge-1498-unit.js + node test-packets-local-channels.js node test-issue-1518-home-url.js node test-channel-decrypt-insecure-context.js node test-live-region-filter.js diff --git a/channel-rainbow.json b/channel-rainbow.json index 73ad3a237..23e1e7829 100644 --- a/channel-rainbow.json +++ b/channel-rainbow.json @@ -295,5 +295,28 @@ "#bogota": "6d0864985b64350ce4cbfebf4979e970", "#peru": "7e6fc347bf29a4c128ac3156865bd521", "#lima": "5f167ce354eca08ab742463df10ef255", - "Public": "8b3387e9c5cdea6ac9e5edbaa115cd72" + "Public": "8b3387e9c5cdea6ac9e5edbaa115cd72", + "#Public": "7534fe292203ccdce93db79377d4b566", + "#newbie": "ff2ecc59f59dbf051a10d880f0b4308d", + "#power": "c4d23a51329aeaee767234644d52cff8", + "#garden": "56ff6fe3500872c2765dbc178350005c", + "#jokes": "abe610d61dee508c0ac18793c36c41c3", + "#space": "5caa27568ba7a216bcc09fcb69c77ea1", + "#path": "29eed6c759d96346b30c869e23fd86f8", + "#hamradio": "83c8b01997654265938da8765cbc7db9", + "#amrron": "68ec397a1dd4f3416911d00211ed0ef6", + "#family": "7d1b805d4bf059c2ef06ed221794663b", + "#testing": "cde5e82cf515647dcb547a79a4f065d1", + "#wicked": "05e990d48943ee03c943c41dea0ddfe1", + "#gay": "3a612fa3bfc9f3b439cc230b01cbbc00", + "#t": "7b5336e85005be805b496bc27c722ff3", + "#doom": "7034910637d6b9b6c2cc3bcd8dfe9bb5", + "#empty": "3351cfa0dad99e1777cce210c983b561", + "#puppy": "c20cf85aab9561e6e65ac4a19a852198", + "#naked": "f423f23ddcf9e472c2ace38dce7591bc", + "#cruising": "550857053360e3492755251f9142e74c", + "#nightowls": "1411c5c3732d3df1b855644f78c47417", + "#worldcup": "d5786cc7bcee5a4828bbb20c3825c5a9", + "#nofilter": "cab31f589959debcd1721b7d252259b1", + "#politics": "64cc9fdaad67f03f23b4cec8c9a4468f" } diff --git a/cmd/ingestor/config.go b/cmd/ingestor/config.go index be1d55f4f..3c022a71f 100644 --- a/cmd/ingestor/config.go +++ b/cmd/ingestor/config.go @@ -144,7 +144,13 @@ func (c *Config) ClientRxCoverageEnabled() bool { type RetentionConfig struct { NodeDays int `json:"nodeDays"` ObserverDays int `json:"observerDays"` - MetricsDays int `json:"metricsDays"` + // ObserverPurgeDays hard-deletes observers that observerDays already + // soft-deleted, once they are older than this and no observation, + // metric or dropped-packet row still references them; 0 disables. + // Only meaningful above observerDays and packetDays — below those the + // reference guards keep every candidate row anyway. + ObserverPurgeDays int `json:"observerPurgeDays"` + MetricsDays int `json:"metricsDays"` // PacketDays is the retention window for transmissions (#1283). // Ownership moved from cmd/server to cmd/ingestor; 0 disables. PacketDays int `json:"packetDays"` @@ -163,6 +169,15 @@ func (c *Config) PacketDaysOrZero() int { return 0 } +// ObserverPurgeDaysOrZero returns the configured retention.observerPurgeDays +// or 0 (disabled) if not set. +func (c *Config) ObserverPurgeDaysOrZero() int { + if c.Retention != nil && c.Retention.ObserverPurgeDays > 0 { + return c.Retention.ObserverPurgeDays + } + return 0 +} + // ClientRxDaysOrZero returns the configured retention.clientRxDays or 0 // (disabled) if not set. func (c *Config) ClientRxDaysOrZero() int { diff --git a/cmd/ingestor/db.go b/cmd/ingestor/db.go index e37094e00..1cf4bf408 100644 --- a/cmd/ingestor/db.go +++ b/cmd/ingestor/db.go @@ -1413,8 +1413,68 @@ func (s *Store) UpsertObserverAt(id, name, iata string, meta *ObserverMeta, last } normalizedIATA := strings.TrimSpace(strings.ToUpper(iata)) - var model, firmware, clientVersion, radio interface{} - var batteryMv, uptimeSecs, noiseFloor, canRelay interface{} + model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay := observerMetaColumns(meta) + + _, err := s.stmtUpsertObserver.Exec( + id, name, normalizedIATA, lastSeen, lastSeen, model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, + name, normalizedIATA, ingestNow, lastSeen, model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, + ) + if err != nil { + s.Stats.WriteErrors.Add(1) + return err + } + s.Stats.ObserverUpserts.Add(1) + + // Reactivate if this observer was previously marked inactive + s.db.Exec(`UPDATE observers SET inactive = 0 WHERE id = ? AND inactive = 1`, id) + return nil +} + +// UpsertObserverRetained applies the metadata from a RETAINED status message +// without treating it as a sign of life. The broker replays retained messages +// on every subscribe, so an ingestor restart would otherwise stamp last_seen +// with the restart time for every observer that ever published one — making +// dead observers permanently un-ageable by RemoveStaleObservers, and undoing +// any inactive flag it did manage to set. +// +// Deliberately narrower than UpsertObserverAt: it updates metadata columns on +// an existing row only. It does not advance last_seen, does not clear +// inactive, does not bump packet_count, and does not INSERT — a retained-only +// observer the analyzer has never heard from live describes a past that may be +// months old and does not belong in the list. A live message from the same +// observer arrives moments later and creates the row through the normal path. +func (s *Store) UpsertObserverRetained(id, name, iata string, meta *ObserverMeta) error { + normalizedIATA := strings.TrimSpace(strings.ToUpper(iata)) + model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay := observerMetaColumns(meta) + + _, err := s.db.Exec(` + UPDATE observers SET + name = COALESCE(?, name), + iata = COALESCE(?, iata), + model = COALESCE(?, model), + firmware = COALESCE(?, firmware), + client_version = COALESCE(?, client_version), + radio = COALESCE(?, radio), + battery_mv = COALESCE(?, battery_mv), + uptime_secs = COALESCE(?, uptime_secs), + noise_floor = COALESCE(?, noise_floor), + can_relay = COALESCE(?, can_relay), + can_relay_seen = CASE WHEN ? IS NULL THEN can_relay_seen ELSE 1 END + WHERE id = ?`, + name, normalizedIATA, model, firmware, clientVersion, radio, + batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, id, + ) + if err != nil { + s.Stats.WriteErrors.Add(1) + return err + } + return nil +} + +// observerMetaColumns flattens an *ObserverMeta into the driver args the +// observer upserts bind. A nil field stays nil so the COALESCE in the SQL +// leaves the existing column untouched (#1290). +func observerMetaColumns(meta *ObserverMeta) (model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay interface{}) { if meta != nil { if meta.Model != nil { model = *meta.Model @@ -1449,20 +1509,7 @@ func (s *Store) UpsertObserverAt(id, name, iata string, meta *ObserverMeta, last } } } - - _, err := s.stmtUpsertObserver.Exec( - id, name, normalizedIATA, lastSeen, lastSeen, model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, - name, normalizedIATA, ingestNow, lastSeen, model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay, canRelay, - ) - if err != nil { - s.Stats.WriteErrors.Add(1) - return err - } - s.Stats.ObserverUpserts.Add(1) - - // Reactivate if this observer was previously marked inactive - s.db.Exec(`UPDATE observers SET inactive = 0 WHERE id = ? AND inactive = 1`, id) - return nil + return model, firmware, clientVersion, radio, batteryMv, uptimeSecs, noiseFloor, canRelay } // Close checkpoints the WAL and closes the database. @@ -1839,6 +1886,42 @@ func (s *Store) RemoveStaleObservers(observerDays int) (int64, error) { return removed, nil } +// PurgeStaleObservers hard-deletes rows RemoveStaleObservers already soft-deleted, +// once they are older than purgeDays and nothing references them any more. It is +// the second stage of observer retention: the soft-delete hides the observer, +// this reclaims the row after its packets have aged out via packetDays. +// +// observations.observer_idx is a bare rowid with no foreign key, so deleting a +// still-referenced observer silently orphans history — packets_v stops resolving +// the observer and the packets are mis-attributed. The three NOT EXISTS guards +// are what make the delete safe; they are correctness, not defensive padding. +// Each is an index seek per candidate row (observers is O(100)), so the whole +// statement stays cheap enough for the daily retention tick. +// +// purgeDays <= 0 disables the purge (the default). +func (s *Store) PurgeStaleObservers(purgeDays int) (int64, error) { + if purgeDays <= 0 { + return 0, nil // disabled + } + cutoff := time.Now().UTC().AddDate(0, 0, -purgeDays).Format(time.RFC3339) + // Tagged for /api/perf writer-lock visibility (#1340). + result, err := s.instrumentedExec("purge_observers", ` + DELETE FROM observers + WHERE inactive = 1 + AND last_seen < ? + AND NOT EXISTS (SELECT 1 FROM observations o WHERE o.observer_idx = observers.rowid) + AND NOT EXISTS (SELECT 1 FROM observer_metrics m WHERE m.observer_id = observers.id) + AND NOT EXISTS (SELECT 1 FROM dropped_packets d WHERE d.observer_id = observers.id)`, cutoff) + if err != nil { + return 0, fmt.Errorf("purge stale observers: %w", err) + } + purged, _ := result.RowsAffected() + if purged > 0 { + log.Printf("Purged %d inactive observer(s) with no remaining data (not seen in %d days)", purged, purgeDays) + } + return purged, nil +} + // DroppedPacket holds data for a packet rejected during ingest. type DroppedPacket struct { Hash string diff --git a/cmd/ingestor/main.go b/cmd/ingestor/main.go index ac102da01..8138d8457 100644 --- a/cmd/ingestor/main.go +++ b/cmd/ingestor/main.go @@ -201,14 +201,10 @@ func main() { // half-open TCP socket and re-dial when paho.IsConnected==true // but no messages have flowed past the stall threshold. Throttled // per source by the watchdog itself (forceReconnectThrottle). - // Disconnect(250) gives in-flight publishes 250ms to drain; - // Connect() returns immediately and paho's reconnect machinery - // takes over from there. Captured-by-value `client` is the same - // pointer used everywhere else for this source. - liveness.ForceReconnectFn = func() { - client.Disconnect(250) - client.Connect() - } + // Captured-by-value `client` is the same pointer used everywhere + // else for this source. See buildForceReconnectFn for why this is + // NOT simply "Disconnect(250) then Connect()". + liveness.ForceReconnectFn = buildForceReconnectFn(client, tag) // PR #1216 r2 item 3: tag collisions used to log.Fatalf, which // killed the entire ingestor over one config typo and recreated // the #1212 total-ingest-stop class this PR exists to prevent. @@ -264,6 +260,14 @@ func main() { observerDays := cfg.ObserverDaysOrDefault() store.RemoveStaleObservers(observerDays) + // Observer purge: second stage, hard-deletes long-inactive observers whose + // packets have already aged out. Always runs after the soft-delete so a row + // crossing both thresholds is finalised in a single pass. 0 = disabled. + observerPurgeDays := cfg.ObserverPurgeDaysOrZero() + if _, err := store.PurgeStaleObservers(observerPurgeDays); err != nil { + log.Printf("[prune] error: %v", err) + } + // Metrics retention: prune old metrics on startup metricsDays := cfg.MetricsRetentionDays() store.PruneOldMetrics(metricsDays) @@ -323,9 +327,11 @@ func main() { go func() { time.Sleep(90 * time.Second) // stagger after metrics prune store.RemoveStaleObservers(observerDays) + store.PurgeStaleObservers(observerPurgeDays) store.RunIncrementalVacuum(vacuumPages) for range observerRetentionTicker.C { store.RemoveStaleObservers(observerDays) + store.PurgeStaleObservers(observerPurgeDays) store.RunIncrementalVacuum(vacuumPages) } }() @@ -554,6 +560,54 @@ func buildMQTTOpts(source MQTTSource) *mqtt.ClientOptions { return opts } +// buildForceReconnectFn builds the watchdog's forced-reconnect action for a +// source (#1335, hardened against a race found while investigating a 100+ +// minute reconnect failure). +// +// paho's own client.IsConnected() — used as liveness.IsConnectedFn — reports +// true not only when genuinely connected but for the ENTIRE time paho's +// background AutoReconnect/ConnectRetry loop is retrying (status +// reconnecting/connecting). So the watchdog's LivenessStalled classification +// (IsConnected==true, no messages) fires just as often for "paho is actively, +// correctly retrying a still-down broker" as it does for the true #1335 +// half-open-TCP case. Naively doing Disconnect(250) then Connect() in the +// first case is actively harmful: paho's Disconnecting() must block until the +// CURRENT in-flight connection attempt plus its backoff sleep unwind (up to +// ConnectTimeout+MaxReconnectInterval, tens of seconds) before status +// actually reaches `disconnected`. Disconnect(250) returns to the caller +// after the 250ms quiesce regardless, so the following Connect() usually runs +// while status is still the transitional `disconnecting` state — paho then +// returns an error token (silently discarded by the old code) AND, because +// Disconnect() was called at all, tears down paho's own retry loop for good +// ("user requested no auto reconnection"). The client is left with nothing +// retrying until the watchdog's next trigger fires, which can repeat the same +// race — compounding into very long outages. +// +// client.IsConnectionOpen() (unlike IsConnected()) is strictly status == +// connected — never true while paho is reconnecting/connecting — so it +// reliably distinguishes "genuinely connected, maybe half-open" (safe to +// Disconnect then Connect; Disconnecting() does not need to wait on any +// in-flight retry loop from status connected, so it completes well within +// the 250ms quiesce) from "paho is already retrying on its own" (must NOT +// call Disconnect; Connect() alone is a safe no-op per paho when a retry is +// already under way, and properly starts a fresh attempt on the rare +// occasion status has actually settled to disconnected). +func buildForceReconnectFn(client mqtt.Client, tag string) func() { + return func() { + if client.IsConnectionOpen() { + client.Disconnect(250) + } + // Connect() resolves synchronously (Error() readable immediately, + // no Wait() needed) for both error returns and the "already + // retrying, treated as a safe no-op" success case — only a genuine + // fresh connection attempt leaves the token pending in the + // background, and we must not block this call on that. + if token := client.Connect(); token.Error() != nil { + log.Printf("MQTT [%s] WATCHDOG force-reconnect Connect() failed: %v", tag, token.Error()) + } + } +} + func handleMessage(store *Store, tag string, source MQTTSource, m mqtt.Message, channelKeys map[string]string, regionKeys map[string][]byte, cfg *Config) { // Liveness watchdog (#1212): record receipt before any processing so a // slow handler still counts as "source is alive". Cheap atomic store. @@ -632,6 +686,22 @@ func handleMessage(store *Store, tag string, source MQTTSource, m mqtt.Message, name, _ := msg["origin"].(string) iata := parts[1] meta := extractObserverMeta(msg) + // A replayed status message is the broker handing us the observer's + // last published snapshot — it is not evidence the observer is alive + // now. Stamping last_seen from it resurrects dead observers, so the + // replay path only refreshes metadata. + // + // Two ways to recognise one: the retain flag (our own subscribe), and + // the payload itself (a replay that reached us through the mosquitto + // bridge, where the flag does not survive the hop — see + // statusIsLiveness). + if m.Retained() || !statusIsLiveness(msg, time.Now().UTC()) { + if err := store.UpsertObserverRetained(observerID, name, iata, meta); err != nil { + log.Printf("MQTT [%s] retained observer status error: %v", tag, err) + } + log.Print(formatStatusLog(tag, firstNonEmpty(name, observerID), iata)) + return + } // observer.last_seen is "when did the analyzer last hear from this // observer" — fundamentally an ingest-time question. Passing "" makes // UpsertObserverAt use time.Now(), independent of the envelope timestamp diff --git a/cmd/ingestor/main_test.go b/cmd/ingestor/main_test.go index c3a2e0095..817afe7b0 100644 --- a/cmd/ingestor/main_test.go +++ b/cmd/ingestor/main_test.go @@ -105,13 +105,14 @@ func TestUnixTime(t *testing.T) { // mockMessage implements mqtt.Message for testing handleMessage type mockMessage struct { - topic string - payload []byte + topic string + payload []byte + retained bool } func (m *mockMessage) Duplicate() bool { return false } func (m *mockMessage) Qos() byte { return 0 } -func (m *mockMessage) Retained() bool { return false } +func (m *mockMessage) Retained() bool { return m.retained } func (m *mockMessage) Topic() string { return m.topic } func (m *mockMessage) MessageID() uint16 { return 0 } func (m *mockMessage) Payload() []byte { return m.payload } diff --git a/cmd/ingestor/maintenance.go b/cmd/ingestor/maintenance.go index fea189fe5..fbcbda716 100644 --- a/cmd/ingestor/maintenance.go +++ b/cmd/ingestor/maintenance.go @@ -10,42 +10,98 @@ import ( "github.com/meshcore-analyzer/dbschema" ) +// pruneBatchTransmissions bounds how many transmissions (and their child +// observations) one prune transaction deletes before it commits and +// releases the writer lock. +// +// Releasing between batches is the entire point. writerMu serialises every +// wrapped writer call, so while the prune holds it the MQTT ingest path is +// blocked. Go's sync.Mutex switches to FIFO handoff once a waiter has been +// blocked for 1ms (starvation mode), so an ingest goroutine queued behind +// the prune is served at the next batch boundary instead of after the whole +// retention day. +// +// The bound is on transmissions, but hold time scales with the rows actually +// deleted, and each transmission carries an unbounded number of observations. +// At ~16 observations per transmission a batch is ~4k row deletes and a few +// hundred milliseconds; an instance with a denser observation ratio gets a +// proportionally longer hold from the same batch size. 250 also keeps the +// commit count low (a 16k-transmission day is 64 transactions, not 16k). +const pruneBatchTransmissions = 250 + +// pruneAgedTransmissionIDs selects the next batch of transmissions older than +// the cutoff. Both statements of a batch embed it, so they resolve the same +// set: nothing modifies `transmissions` between them inside the transaction. +// +// The ORDER BY must be satisfiable from idx_transmissions_first_seen. That +// index carries the rowid as its tiebreaker, so "first_seen, id" is walked +// straight off it and the LIMIT stays deterministic even when timestamps tie. +// Ordering by id alone looks equivalent but makes SQLite abandon the index for +// a rowid SCAN. That is harmless while rows are being deleted — the oldest +// rows have the lowest rowids and match at once — but the batch that finds +// nothing, which is the steady state whenever nothing has aged out, walks the +// whole table under writerMu. TestPruneAgedTransmissionIDsUsesFirstSeenIndex +// pins the plan. +const pruneAgedTransmissionIDs = `SELECT id FROM transmissions WHERE first_seen < ? ORDER BY first_seen, id LIMIT ?` + +// The two statements of one prune batch. Child observations go first (no +// CASCADE in SQLite). +const ( + pruneObservationsBatch = `DELETE FROM observations WHERE transmission_id IN (` + pruneAgedTransmissionIDs + `)` + pruneTransmissionsBatch = `DELETE FROM transmissions WHERE id IN (` + pruneAgedTransmissionIDs + `)` +) + // PruneOldPackets deletes transmissions (and their child observations) // older than `days`. Returns count of transmissions deleted. // // Owned by the ingestor per #1283: the writer process is the only one // allowed to hold the DB write lock; previously this lived in // cmd/server/db.go and raced ingestor INSERTs (SQLITE_BUSY). +// +// Deletion is chunked into bounded transactions so ingest is never blocked +// for longer than a single batch. Deleting a whole retention day in one +// transaction held the writer lock for ~35s on a mesh doing ~260k +// observations/day, stalling MQTT ingest for the same duration. +// +// On error the transmissions deleted by already-committed batches are +// returned alongside it — those rows are gone, so reporting 0 would be +// wrong. func (s *Store) PruneOldPackets(days int) (int64, error) { if days <= 0 { return 0, nil } cutoff := time.Now().UTC().AddDate(0, 0, -days).Format(time.RFC3339) - // Tagged for writer-perf visibility (#1340). - var n int64 - err := s.WriterTx("prune_packets", func(tx *sql.Tx) error { - // Delete child observations first (no CASCADE in SQLite). - if _, err := tx.Exec(`DELETE FROM observations WHERE transmission_id IN ( - SELECT id FROM transmissions WHERE first_seen < ? - )`, cutoff); err != nil { - return fmt.Errorf("prune observations: %w", err) - } - - res, err := tx.Exec(`DELETE FROM transmissions WHERE first_seen < ?`, cutoff) + var total int64 + for { + var batch int64 + // Tagged for writer-perf visibility (#1340). + err := s.WriterTx("prune_packets", func(tx *sql.Tx) error { + if _, err := tx.Exec(pruneObservationsBatch, cutoff, pruneBatchTransmissions); err != nil { + return fmt.Errorf("prune observations: %w", err) + } + res, err := tx.Exec(pruneTransmissionsBatch, cutoff, pruneBatchTransmissions) + if err != nil { + return fmt.Errorf("prune transmissions: %w", err) + } + batch, _ = res.RowsAffected() + return nil + }) if err != nil { - return fmt.Errorf("prune transmissions: %w", err) + return total, err + } + total += batch + // A short batch proves nothing is left below the cutoff: the subquery + // found fewer rows than it was allowed to take. Only a batch that came + // back exactly full needs another pass. + if batch < pruneBatchTransmissions { + break } - n, _ = res.RowsAffected() - return nil - }) - if err != nil { - return 0, err } - if n > 0 { - log.Printf("[prune] deleted %d transmissions older than %d days", n, days) + if total > 0 { + log.Printf("[prune] deleted %d transmissions older than %d days", total, days) } - return n, nil + return total, nil } // PruneOldClientReceptions deletes mobile client-RX coverage rows older than diff --git a/cmd/ingestor/mqtt_force_reconnect_race_test.go b/cmd/ingestor/mqtt_force_reconnect_race_test.go new file mode 100644 index 000000000..8697cd3ea --- /dev/null +++ b/cmd/ingestor/mqtt_force_reconnect_race_test.go @@ -0,0 +1,150 @@ +package main + +import ( + "bytes" + "errors" + "log" + "strings" + "testing" + "time" + + mqtt "github.com/eclipse/paho.mqtt.golang" +) + +// fakeToken is a minimal mqtt.Token whose Error() is resolvable immediately +// without calling Wait(), matching how paho's real ConnectToken behaves on +// both of its synchronous return paths (the "already retrying, treated as a +// safe no-op" success case and any error case) — see buildForceReconnectFn's +// doc comment for why the caller must not need to Wait() before reading +// Error() (doing so would block indefinitely on a genuine, still in-flight +// reconnect attempt). +type fakeToken struct{ err error } + +func (t *fakeToken) Wait() bool { return true } +func (t *fakeToken) WaitTimeout(time.Duration) bool { return true } +func (t *fakeToken) Done() <-chan struct{} { ch := make(chan struct{}); close(ch); return ch } +func (t *fakeToken) Error() error { return t.err } + +// fakeClient implements mqtt.Client, recording calls to the three methods +// buildForceReconnectFn actually uses (IsConnectionOpen, Disconnect, +// Connect). Every other method panics — buildForceReconnectFn must never +// touch subscriptions, publishes, or options, so a call there indicates the +// fix drifted from its intended scope. +type fakeClient struct { + isConnectionOpen bool + connectErr error + + disconnectCalled bool + connectCalled bool + callOrder []string +} + +func (c *fakeClient) IsConnected() bool { panic("not used by buildForceReconnectFn") } +func (c *fakeClient) IsConnectionOpen() bool { + c.callOrder = append(c.callOrder, "IsConnectionOpen") + return c.isConnectionOpen +} +func (c *fakeClient) Connect() mqtt.Token { + c.connectCalled = true + c.callOrder = append(c.callOrder, "Connect") + return &fakeToken{err: c.connectErr} +} +func (c *fakeClient) Disconnect(quiesce uint) { + c.disconnectCalled = true + c.callOrder = append(c.callOrder, "Disconnect") +} +func (c *fakeClient) Publish(topic string, qos byte, retained bool, payload any) mqtt.Token { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) Subscribe(topic string, qos byte, callback mqtt.MessageHandler) mqtt.Token { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) SubscribeMultiple(filters map[string]byte, callback mqtt.MessageHandler) mqtt.Token { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) Unsubscribe(topics ...string) mqtt.Token { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) AddRoute(topic string, callback mqtt.MessageHandler) { + panic("not used by buildForceReconnectFn") +} +func (c *fakeClient) OptionsReader() mqtt.ClientOptionsReader { + panic("not used by buildForceReconnectFn") +} + +// Case 1 (#1335, the genuine half-open-TCP case): paho reports the +// connection as strictly open. This is the one case where Disconnect() is +// safe — status==connected means Disconnecting() does not need to wait on +// any in-flight retry loop, so it completes well within the quiesce window. +// buildForceReconnectFn must still Disconnect before Connect here. +func TestBuildForceReconnectFn_DisconnectsWhenConnectionOpen(t *testing.T) { + c := &fakeClient{isConnectionOpen: true} + fn := buildForceReconnectFn(c, "half-open-source") + fn() + + if !c.disconnectCalled { + t.Error("expected Disconnect to be called when IsConnectionOpen()==true (half-open TCP case, #1335)") + } + if !c.connectCalled { + t.Error("expected Connect to be called") + } + want := []string{"IsConnectionOpen", "Disconnect", "Connect"} + if len(c.callOrder) != len(want) { + t.Fatalf("expected call order %v, got %v", want, c.callOrder) + } + for i, name := range want { + if c.callOrder[i] != name { + t.Errorf("expected call order %v, got %v", want, c.callOrder) + break + } + } +} + +// Case 2 (the race this fix closes): paho is already mid-retry +// (reconnecting, or ConnectRetry's connecting), so IsConnectionOpen() is +// false even though the looser IsConnected() (used elsewhere for liveness +// classification) would report true. Calling Disconnect() here is what +// caused the original bug: it races paho's internal status transition and +// can permanently kill an in-flight retry loop via a botched, silently +// swallowed reconnect. buildForceReconnectFn must skip Disconnect and just +// call Connect(), which paho treats as a safe no-op if a retry is already +// under way. +func TestBuildForceReconnectFn_SkipsDisconnectWhenAlreadyRetrying(t *testing.T) { + c := &fakeClient{isConnectionOpen: false} + fn := buildForceReconnectFn(c, "mid-retry-source") + fn() + + if c.disconnectCalled { + t.Error("Disconnect must NOT be called while paho is already retrying (IsConnectionOpen()==false) — this races paho's status machine and can kill the in-flight retry loop") + } + if !c.connectCalled { + t.Error("expected Connect to still be called (safe no-op per paho when already retrying, or a real reconnect if paho had actually settled to disconnected)") + } +} + +// Case 3: Connect() returns an error token (e.g. the errStatusMustBeDisconnected +// class this whole bug hinged on — Connect() called while paho's status is +// transitionally "disconnecting"). The old code discarded the returned token +// entirely; the fix must surface it via log output instead of silently +// dropping it. +func TestBuildForceReconnectFn_LogsConnectError(t *testing.T) { + c := &fakeClient{isConnectionOpen: false, connectErr: errors.New("status can only transition to connecting from disconnected")} + fn := buildForceReconnectFn(c, "erroring-source") + + var buf bytes.Buffer + origOut := log.Writer() + origFlags := log.Flags() + log.SetOutput(&buf) + log.SetFlags(0) + defer func() { + log.SetOutput(origOut) + log.SetFlags(origFlags) + }() + + fn() + + logged := buf.String() + if !strings.Contains(logged, "erroring-source") || !strings.Contains(logged, "status can only transition") { + t.Errorf("expected Connect() error to be logged with the source tag, got: %q", logged) + } +} diff --git a/cmd/ingestor/mqtt_watchdog_1749_test.go b/cmd/ingestor/mqtt_watchdog_1749_test.go index 105c18ad2..43e407829 100644 --- a/cmd/ingestor/mqtt_watchdog_1749_test.go +++ b/cmd/ingestor/mqtt_watchdog_1749_test.go @@ -58,15 +58,8 @@ func TestMQTTStallWatchdog_EscalateOnPersistentDisconnect_1749(t *testing.T) { t.Fatalf("setup: %v", err) } - tick := make(chan time.Time) - done := make(chan struct{}) - defer close(done) - - exited := make(chan struct{}) - go func() { - runLivenessWatchdogLoop(tick, done, threshold, func(args ...any) {}) - close(exited) - }() + tick, stopLoop := startWatchdogTestLoop(t, threshold, func(args ...any) {}) + defer stopLoop() // Feed ticks spanning > (multiplier × threshold) of wall clock so // the escalation path fires. We control the `now` parameter by @@ -114,10 +107,8 @@ func TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749(t *testing.T) { t.Fatalf("setup: %v", err) } - tick := make(chan time.Time) - done := make(chan struct{}) - defer close(done) - go runLivenessWatchdogLoop(tick, done, threshold, func(args ...any) {}) + tick, stopLoop := startWatchdogTestLoop(t, threshold, func(args ...any) {}) + defer stopLoop() base := time.Now() // Pre-stamp DisconnectedSinceUnix so that the first tick is @@ -259,10 +250,8 @@ func TestMQTTStallWatchdog_LastTickUnixExposed_1749(t *testing.T) { watchdogLastTickUnix.Store(before) }) - tick := make(chan time.Time) - done := make(chan struct{}) - defer close(done) - go runLivenessWatchdogLoop(tick, done, time.Minute, func(args ...any) {}) + tick, stopLoop := startWatchdogTestLoop(t, time.Minute, func(args ...any) {}) + defer stopLoop() stamp := time.Now().Add(48 * time.Hour) // guaranteed > before select { diff --git a/cmd/ingestor/mqtt_watchdog_1810_test.go b/cmd/ingestor/mqtt_watchdog_1810_test.go index aeefd4a0f..b68fa75d3 100644 --- a/cmd/ingestor/mqtt_watchdog_1810_test.go +++ b/cmd/ingestor/mqtt_watchdog_1810_test.go @@ -212,10 +212,8 @@ func TestWatchdog_EscalationWarnThrottled_1810(t *testing.T) { } } - tick := make(chan time.Time) - done := make(chan struct{}) - defer close(done) - go runLivenessWatchdogLoop(tick, done, threshold, emit) + tick, stopLoop := startWatchdogTestLoop(t, threshold, emit) + defer stopLoop() base := time.Now() // First tick: stamps DisconnectedSinceUnix (no escalation yet). diff --git a/cmd/ingestor/mqtt_watchdog_testhelpers_test.go b/cmd/ingestor/mqtt_watchdog_testhelpers_test.go index 348c2f2d1..1db9b99a5 100644 --- a/cmd/ingestor/mqtt_watchdog_testhelpers_test.go +++ b/cmd/ingestor/mqtt_watchdog_testhelpers_test.go @@ -1,6 +1,7 @@ package main import ( + "sync" "testing" "time" ) @@ -45,3 +46,31 @@ func sendTickOrFail(t *testing.T, tick chan<- time.Time, stamp time.Time, timeou t.Fatalf("%s: tick blocked after %s — loop dead?", label, timeout) } } + +// startWatchdogTestLoop is setupWatchdogTestLoop for the common case: a test +// that wants the loop running for its own duration and nothing more. The +// returned stop closes done AND waits for the loop goroutine to return. It is +// safe to call more than once. +// +// Waiting is the part that must not be skipped. Closing done only asks the +// loop to stop; the goroutine can still be inside a scan, and that scan walks +// the package-level livenessRegistry. The next test registers its own source +// there, so a loop that has not returned yet will process that source on a +// tick carrying the previous test's fabricated clock. +// +// That is measured, not theoretical. With four call sites closing done and +// walking away, TestMQTTStallWatchdog_DisconnectedEscalationThrottled_1749 +// failed 2 to 3 times per 20 runs of the watchdog tests and never once in 50 +// runs on its own. Debug tracing showed two loops reaching maybeForceReconnect +// for the same source with tick clocks 420s apart, both reading +// LastForceReconnectUnix as 0 before either wrote it, so the throttle the test +// asserts on let both through. +func startWatchdogTestLoop(t *testing.T, threshold time.Duration, emit func(...any)) (tick chan time.Time, stop func()) { + t.Helper() + tick, done, exited := setupWatchdogTestLoop(t, threshold, emit) + var once sync.Once + return tick, func() { + once.Do(func() { close(done) }) + <-exited + } +} diff --git a/cmd/ingestor/observer_purge_test.go b/cmd/ingestor/observer_purge_test.go new file mode 100644 index 000000000..8a3f6a6a6 --- /dev/null +++ b/cmd/ingestor/observer_purge_test.go @@ -0,0 +1,222 @@ +package main + +import ( + "testing" + "time" +) + +// staleObserver inserts an observer already soft-deleted (inactive = 1) with +// last_seen ageDays in the past, and returns its rowid. +func staleObserver(t *testing.T, s *Store, id string) int64 { + t.Helper() + if err := s.UpsertObserver(id, id, "LAX", nil); err != nil { + t.Fatal(err) + } + old := time.Now().UTC().AddDate(0, 0, -90).Format(time.RFC3339) + if _, err := s.db.Exec(`UPDATE observers SET last_seen = ?, inactive = 1 WHERE id = ?`, old, id); err != nil { + t.Fatal(err) + } + var rowid int64 + if err := s.db.QueryRow(`SELECT rowid FROM observers WHERE id = ?`, id).Scan(&rowid); err != nil { + t.Fatal(err) + } + return rowid +} + +func observerExists(t *testing.T, s *Store, id string) bool { + t.Helper() + var n int + if err := s.db.QueryRow(`SELECT COUNT(*) FROM observers WHERE id = ?`, id).Scan(&n); err != nil { + t.Fatal(err) + } + return n > 0 +} + +func TestPurgeStaleObserversDeletesUnreferencedRow(t *testing.T) { + store := newTestStore(t) + staleObserver(t, store, "obs-gone") + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 1 { + t.Errorf("purged=%d, want 1", purged) + } + if observerExists(t, store, "obs-gone") { + t.Error("obs-gone still present, want hard-deleted") + } +} + +// Regression: an observer whose rowid is still referenced by observations must +// survive the purge — deleting it orphans observations.observer_idx, which +// breaks the packets_v join and mis-attributes historical packets. +func TestPurgeStaleObserversKeepsObserverWithObservations(t *testing.T) { + store := newTestStore(t) + rowid := staleObserver(t, store, "obs-referenced") + + if _, err := store.db.Exec( + `INSERT INTO transmissions (id, raw_hex, hash, first_seen) VALUES (1, 'AA', 'h1', ?)`, + time.Now().UTC().Format(time.RFC3339), + ); err != nil { + t.Fatal(err) + } + if _, err := store.db.Exec( + `INSERT INTO observations (transmission_id, observer_idx, timestamp) VALUES (1, ?, 0)`, rowid, + ); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (row is referenced by observations)", purged) + } + if !observerExists(t, store, "obs-referenced") { + t.Error("obs-referenced was deleted, want kept") + } + + var orphans int + if err := store.db.QueryRow( + `SELECT COUNT(*) FROM observations WHERE observer_idx IS NOT NULL + AND observer_idx NOT IN (SELECT rowid FROM observers)`, + ).Scan(&orphans); err != nil { + t.Fatal(err) + } + if orphans != 0 { + t.Errorf("orphaned observations=%d, want 0", orphans) + } +} + +func TestPurgeStaleObserversKeepsObserverWithMetrics(t *testing.T) { + store := newTestStore(t) + staleObserver(t, store, "obs-metrics") + + if _, err := store.db.Exec( + `INSERT INTO observer_metrics (observer_id, timestamp) VALUES ('obs-metrics', ?)`, + time.Now().UTC().Format(time.RFC3339), + ); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (row is referenced by observer_metrics)", purged) + } + if !observerExists(t, store, "obs-metrics") { + t.Error("obs-metrics was deleted, want kept") + } +} + +func TestPurgeStaleObserversKeepsObserverWithDroppedPackets(t *testing.T) { + store := newTestStore(t) + staleObserver(t, store, "obs-dropped") + + if _, err := store.db.Exec( + `INSERT INTO dropped_packets (reason, observer_id) VALUES ('bad-sig', 'obs-dropped')`, + ); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (row is referenced by dropped_packets)", purged) + } + if !observerExists(t, store, "obs-dropped") { + t.Error("obs-dropped was deleted, want kept") + } +} + +// An observer old enough to purge but not yet soft-deleted must be left to +// RemoveStaleObservers — the hard purge only ever finalises an inactive row. +func TestPurgeStaleObserversKeepsActiveObserver(t *testing.T) { + store := newTestStore(t) + if err := store.UpsertObserver("obs-active", "Active", "LAX", nil); err != nil { + t.Fatal(err) + } + old := time.Now().UTC().AddDate(0, 0, -90).Format(time.RFC3339) + if _, err := store.db.Exec(`UPDATE observers SET last_seen = ? WHERE id = ?`, old, "obs-active"); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (inactive = 0)", purged) + } + if !observerExists(t, store, "obs-active") { + t.Error("obs-active was deleted, want kept") + } +} + +func TestPurgeStaleObserversKeepsObserverInsideWindow(t *testing.T) { + store := newTestStore(t) + if err := store.UpsertObserver("obs-recent", "Recent", "LAX", nil); err != nil { + t.Fatal(err) + } + recent := time.Now().UTC().AddDate(0, 0, -20).Format(time.RFC3339) + if _, err := store.db.Exec( + `UPDATE observers SET last_seen = ?, inactive = 1 WHERE id = ?`, recent, "obs-recent", + ); err != nil { + t.Fatal(err) + } + + purged, err := store.PurgeStaleObservers(30) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("purged=%d, want 0 (last_seen inside the 30-day window)", purged) + } + if !observerExists(t, store, "obs-recent") { + t.Error("obs-recent was deleted, want kept") + } +} + +func TestPurgeStaleObserversDisabled(t *testing.T) { + store := newTestStore(t) + staleObserver(t, store, "obs-kept") + + for _, days := range []int{0, -1} { + purged, err := store.PurgeStaleObservers(days) + if err != nil { + t.Fatal(err) + } + if purged != 0 { + t.Errorf("PurgeStaleObservers(%d) purged=%d, want 0 (disabled)", days, purged) + } + } + if !observerExists(t, store, "obs-kept") { + t.Error("obs-kept was deleted, want kept while purge is disabled") + } +} + +func TestObserverPurgeDaysOrZero(t *testing.T) { + tests := []struct { + name string + cfg *Config + want int + }{ + {"nil retention", &Config{}, 0}, + {"unset", &Config{Retention: &RetentionConfig{}}, 0}, + {"configured", &Config{Retention: &RetentionConfig{ObserverPurgeDays: 30}}, 30}, + {"negative disables", &Config{Retention: &RetentionConfig{ObserverPurgeDays: -1}}, 0}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.cfg.ObserverPurgeDaysOrZero(); got != tt.want { + t.Errorf("ObserverPurgeDaysOrZero() = %d, want %d", got, tt.want) + } + }) + } +} diff --git a/cmd/ingestor/prune_chunked_test.go b/cmd/ingestor/prune_chunked_test.go new file mode 100644 index 000000000..a54130b00 --- /dev/null +++ b/cmd/ingestor/prune_chunked_test.go @@ -0,0 +1,286 @@ +package main + +import ( + "fmt" + "path/filepath" + "strings" + "testing" + "time" + + _ "modernc.org/sqlite" +) + +// seedAgedTransmissions inserts n transmissions stamped `age` days in the +// past, each with obsPerTx child observations, and returns nothing — the +// caller asserts against the store. Kept local to this file so the batching +// tests own their fixture shape. +func seedAgedTransmissions(t *testing.T, store *Store, n, obsPerTx, ageDays int) { + t.Helper() + ts := time.Now().UTC().AddDate(0, 0, -ageDays).Format(time.RFC3339) + for i := 0; i < n; i++ { + res, err := store.db.Exec( + `INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, payload_version, decoded_json) + VALUES (?, ?, ?, 0, 1, 1, '{}')`, + "AA", fmt.Sprintf("h%d-%d", ageDays, i), ts, + ) + if err != nil { + t.Fatalf("seed tx %d: %v", i, err) + } + txID, err := res.LastInsertId() + if err != nil { + t.Fatalf("seed tx %d LastInsertId: %v", i, err) + } + for j := 0; j < obsPerTx; j++ { + if _, err := store.db.Exec( + `INSERT INTO observations (transmission_id, observer_idx, direction, snr, rssi, score, path_json, timestamp) + VALUES (?, ?, 'rx', 1.0, -100, 0, '[]', ?)`, + txID, j, time.Now().Unix(), + ); err != nil { + t.Fatalf("seed obs %d/%d: %v", i, j, err) + } + } + } +} + +func countRows(t *testing.T, store *Store, table string) int { + t.Helper() + var n int + if err := store.db.QueryRow(`SELECT COUNT(*) FROM ` + table).Scan(&n); err != nil { + t.Fatalf("count %s: %v", table, err) + } + return n +} + +func openPruneStore(t *testing.T, name string) *Store { + t.Helper() + store, err := OpenStore(filepath.Join(t.TempDir(), name)) + if err != nil { + t.Fatalf("OpenStore: %v", err) + } + t.Cleanup(func() { store.Close() }) + return store +} + +// TestPruneOldPacketsDeletesInBoundedBatches is the perf assertion for the +// chunked prune: it enforces that a retention day is deleted across many +// bounded transactions rather than one long-held writer lock. +// +// The old implementation issued exactly ONE WriterTx regardless of row +// count, holding writerMu — and therefore blocking MQTT ingest — for the +// whole delete. Measured on a production instance doing ~260k +// observations/day that was prune_packets hold_ms_p50 = 33905, with +// mqtt_handler wait_ms_max = 35797. +// +// Counting prune_packets transactions is the deterministic proxy for that +// characteristic: N transactions means the lock was released N-1 times, so +// the worst-case ingest stall is one batch rather than the full day. +func TestPruneOldPacketsDeletesInBoundedBatches(t *testing.T) { + store := openPruneStore(t, "prune-batched.db") + + // Two full batches plus a partial one. The partial batch is what ends the + // loop, so no terminating empty batch should run. + const aged = pruneBatchTransmissions*2 + 37 + seedAgedTransmissions(t, store, aged, 2, 10) + + ResetWriterStatsForTest() + + n, err := store.PruneOldPackets(5) + if err != nil { + t.Fatalf("PruneOldPackets: %v", err) + } + if n != aged { + t.Fatalf("expected %d transmissions pruned, got %d", aged, n) + } + + // 2 full batches + 1 partial batch, which proves nothing is left. + wantTx := int64(3) + got := store.WriterStatsSnapshot()["prune_packets"] + if got.Count != wantTx { + t.Fatalf("expected %d prune_packets transactions for %d rows at batch size %d, got %d "+ + "(1 means the delete was not chunked and holds the writer lock for the whole retention day)", + wantTx, aged, pruneBatchTransmissions, got.Count) + } + + if remaining := countRows(t, store, "transmissions"); remaining != 0 { + t.Fatalf("expected all aged transmissions gone, %d remain", remaining) + } + if remaining := countRows(t, store, "observations"); remaining != 0 { + t.Fatalf("expected all child observations gone, %d remain", remaining) + } +} + +// TestPruneOldPacketsExactMultipleTerminates covers the one case the +// short-batch exit cannot catch on its own: when the aged rows are an exact +// multiple of the batch size, the last batch comes back full, so the loop +// must run one more — empty — batch to learn that nothing is left. +func TestPruneOldPacketsExactMultipleTerminates(t *testing.T) { + store := openPruneStore(t, "prune-exact.db") + + const aged = pruneBatchTransmissions * 2 + seedAgedTransmissions(t, store, aged, 1, 10) + + ResetWriterStatsForTest() + + n, err := store.PruneOldPackets(5) + if err != nil { + t.Fatalf("PruneOldPackets: %v", err) + } + if n != aged { + t.Fatalf("expected %d pruned, got %d", aged, n) + } + + // 2 full batches + 1 empty batch that finds nothing and ends the loop. + if got := store.WriterStatsSnapshot()["prune_packets"].Count; got != 3 { + t.Fatalf("expected 3 prune_packets transactions for an exact multiple of the batch size, got %d", got) + } + if remaining := countRows(t, store, "transmissions"); remaining != 0 { + t.Fatalf("expected all aged transmissions gone, %d remain", remaining) + } + if remaining := countRows(t, store, "observations"); remaining != 0 { + t.Fatalf("expected all child observations gone, %d remain", remaining) + } +} + +// TestPruneOldPacketsSpansBatchesAndKeepsFreshRows covers the correctness +// risk the batching introduces: with a LIMIT on both statements, a cutoff +// that straddles several batches must still delete every aged row and no +// fresh one, and must not orphan observations. +func TestPruneOldPacketsSpansBatchesAndKeepsFreshRows(t *testing.T) { + store := openPruneStore(t, "prune-mixed.db") + + const aged = pruneBatchTransmissions + 11 + const fresh = 17 + const obsPerTx = 3 + seedAgedTransmissions(t, store, aged, obsPerTx, 10) + seedAgedTransmissions(t, store, fresh, obsPerTx, 0) + + n, err := store.PruneOldPackets(5) + if err != nil { + t.Fatalf("PruneOldPackets: %v", err) + } + if n != aged { + t.Fatalf("expected %d pruned, got %d", aged, n) + } + + if remaining := countRows(t, store, "transmissions"); remaining != fresh { + t.Fatalf("expected %d fresh transmissions kept, got %d", fresh, remaining) + } + if remaining := countRows(t, store, "observations"); remaining != fresh*obsPerTx { + t.Fatalf("expected %d observations kept, got %d", fresh*obsPerTx, remaining) + } + + // No observation may reference a transmission that is gone. + var orphans int + if err := store.db.QueryRow(` + SELECT COUNT(*) FROM observations o + LEFT JOIN transmissions t ON t.id = o.transmission_id + WHERE t.id IS NULL`).Scan(&orphans); err != nil { + t.Fatalf("orphan check: %v", err) + } + if orphans != 0 { + t.Fatalf("expected no orphaned observations, got %d", orphans) + } +} + +// TestPruneOldPacketsDisabledTakesNoWriterLock pins the 0/negative +// short-circuit: retention disabled must not open a transaction at all. +func TestPruneOldPacketsDisabledTakesNoWriterLock(t *testing.T) { + store := openPruneStore(t, "prune-disabled.db") + seedAgedTransmissions(t, store, 5, 1, 10) + + ResetWriterStatsForTest() + + for _, days := range []int{0, -1} { + n, err := store.PruneOldPackets(days) + if err != nil { + t.Fatalf("PruneOldPackets(%d): %v", days, err) + } + if n != 0 { + t.Fatalf("PruneOldPackets(%d) = %d, want 0", days, n) + } + } + + if got := store.WriterStatsSnapshot()["prune_packets"].Count; got != 0 { + t.Fatalf("expected no prune_packets transactions when retention is disabled, got %d", got) + } + if remaining := countRows(t, store, "transmissions"); remaining != 5 { + t.Fatalf("expected 5 transmissions untouched, got %d", remaining) + } +} + +// TestPruneOldPacketsNothingToDeleteRunsOneEmptyBatch documents the +// steady-state cost when nothing has aged out yet: a single empty batch, then +// the loop exits. That batch is only cheap because the subquery is walked off +// idx_transmissions_first_seen — see +// TestPruneAgedTransmissionIDsUsesFirstSeenIndex. +func TestPruneOldPacketsNothingToDeleteRunsOneEmptyBatch(t *testing.T) { + store := openPruneStore(t, "prune-noop.db") + seedAgedTransmissions(t, store, 9, 2, 0) + + ResetWriterStatsForTest() + + n, err := store.PruneOldPackets(5) + if err != nil { + t.Fatalf("PruneOldPackets: %v", err) + } + if n != 0 { + t.Fatalf("expected 0 pruned, got %d", n) + } + if got := store.WriterStatsSnapshot()["prune_packets"].Count; got != 1 { + t.Fatalf("expected exactly 1 empty prune_packets transaction, got %d", got) + } + if remaining := countRows(t, store, "transmissions"); remaining != 9 { + t.Fatalf("expected 9 transmissions kept, got %d", remaining) + } +} + +// TestPruneAgedTransmissionIDsUsesFirstSeenIndex pins the query plan of the +// batch subquery and of both statements that embed it. +// +// Ordering the batch by id instead of first_seen makes SQLite drop +// idx_transmissions_first_seen for a rowid SCAN. On a 1M-row table that took +// the terminating, nothing-left batch from ~10µs to ~73ms — once per statement, +// under writerMu, in the state an instance is in whenever nothing has aged out. +// Transaction counts cannot see that regression, so the plan is the assertion. +func TestPruneAgedTransmissionIDsUsesFirstSeenIndex(t *testing.T) { + store := openPruneStore(t, "prune-plan.db") + seedAgedTransmissions(t, store, 20, 2, 10) + + cutoff := time.Now().UTC().AddDate(0, 0, -5).Format(time.RFC3339) + for name, q := range map[string]string{ + "batch subquery": pruneAgedTransmissionIDs, + "observations delete": pruneObservationsBatch, + "transmissions delete": pruneTransmissionsBatch, + } { + rows, err := store.db.Query("EXPLAIN QUERY PLAN "+q, cutoff, pruneBatchTransmissions) + if err != nil { + t.Fatalf("%s: EXPLAIN QUERY PLAN: %v", name, err) + } + var steps []string + for rows.Next() { + var id, parent, notused int + var detail string + if err := rows.Scan(&id, &parent, ¬used, &detail); err != nil { + rows.Close() + t.Fatalf("%s: scan plan row: %v", name, err) + } + steps = append(steps, detail) + } + if err := rows.Err(); err != nil { + rows.Close() + t.Fatalf("%s: plan rows: %v", name, err) + } + rows.Close() + plan := strings.Join(steps, " | ") + + if !strings.Contains(plan, "idx_transmissions_first_seen") { + t.Errorf("%s: plan does not use idx_transmissions_first_seen: %s", name, plan) + } + if strings.Contains(plan, "SCAN transmissions") { + t.Errorf("%s: plan scans transmissions, so the empty terminating batch walks the whole table under writerMu: %s", name, plan) + } + if strings.Contains(plan, "TEMP B-TREE") { + t.Errorf("%s: plan sorts in a temp b-tree instead of walking the index in order: %s", name, plan) + } + } +} diff --git a/cmd/ingestor/retained_status_test.go b/cmd/ingestor/retained_status_test.go new file mode 100644 index 000000000..64a7ffc47 --- /dev/null +++ b/cmd/ingestor/retained_status_test.go @@ -0,0 +1,171 @@ +package main + +import ( + "testing" + "time" +) + +// A retained MQTT status message is a snapshot the broker replays on every +// subscribe — it says what the observer last published, not that the observer +// is alive now. Treating the replay as liveness makes dead observers immortal: +// last_seen jumps to the ingestor's restart time, RemoveStaleObservers can +// never age them out, and any row it did mark gets reactivated on the next +// restart. Observed on live 2026-08-11: 23 observers, all stamped 15s after +// container start, 18 of them with no real packet in over a month. + +const retainedStatusTopic = "meshcore/LAX/obs-retained/status" + +func retainedStatusMsg(topic string, payload string) *mockMessage { + return &mockMessage{topic: topic, payload: []byte(payload), retained: true} +} + +// seedObserver inserts an observer and forces last_seen to ageDays ago. +func seedObserver(t *testing.T, s *Store, id string, ageDays int) string { + t.Helper() + if err := s.UpsertObserver(id, id, "LAX", nil); err != nil { + t.Fatal(err) + } + old := time.Now().UTC().AddDate(0, 0, -ageDays).Format(time.RFC3339) + if _, err := s.db.Exec(`UPDATE observers SET last_seen = ? WHERE id = ?`, old, id); err != nil { + t.Fatal(err) + } + return old +} + +func observerLastSeen(t *testing.T, s *Store, id string) string { + t.Helper() + var got string + if err := s.db.QueryRow(`SELECT last_seen FROM observers WHERE id = ?`, id).Scan(&got); err != nil { + t.Fatal(err) + } + return got +} + +func TestRetainedStatusDoesNotAdvanceLastSeen(t *testing.T) { + store := newTestStore(t) + want := seedObserver(t, store, "obs-retained", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg(retainedStatusTopic, `{"origin":"obs-retained","noise_floor":-95.5}`), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-retained"); got != want { + t.Errorf("last_seen = %s, want %s (retained replay is not liveness)", got, want) + } +} + +func TestRetainedStatusDoesNotReactivateInactiveObserver(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-retained", 60) + if _, err := store.db.Exec(`UPDATE observers SET inactive = 1 WHERE id = ?`, "obs-retained"); err != nil { + t.Fatal(err) + } + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg(retainedStatusTopic, `{"origin":"obs-retained","noise_floor":-95.5}`), + nil, nil, &Config{}) + + var inactive int + if err := store.db.QueryRow(`SELECT inactive FROM observers WHERE id = ?`, "obs-retained").Scan(&inactive); err != nil { + t.Fatal(err) + } + if inactive != 1 { + t.Errorf("inactive = %d, want 1 (retained replay must not resurrect a soft-deleted observer)", inactive) + } +} + +// A retained snapshot from an observer the analyzer has never heard from live +// describes a past that may be months old. Creating a row for it puts a +// permanently dead observer in the list. +func TestRetainedStatusDoesNotCreateUnknownObserver(t *testing.T) { + store := newTestStore(t) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg("meshcore/LAX/obs-never-seen/status", `{"origin":"Ghost","noise_floor":-95.5}`), + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow(`SELECT COUNT(*) FROM observers WHERE id = ?`, "obs-never-seen").Scan(&count); err != nil { + t.Fatal(err) + } + if count != 0 { + t.Errorf("observers row count = %d, want 0 (retained-only observer must not be created)", count) + } +} + +// The metrics sample is stamped with ingest time, so a replay would file a +// months-old reading as a present-tense measurement. +func TestRetainedStatusDoesNotInsertMetricsSample(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-retained", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg(retainedStatusTopic, `{"origin":"obs-retained","noise_floor":-95.5,"battery_mv":3900}`), + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow( + `SELECT COUNT(*) FROM observer_metrics WHERE observer_id = ?`, "obs-retained", + ).Scan(&count); err != nil { + t.Fatal(err) + } + if count != 0 { + t.Errorf("observer_metrics rows = %d, want 0 (retained replay is not a new sample)", count) + } +} + +// The retained snapshot is still the observer's last known state, so its +// metadata is worth keeping — only the liveness signal is suppressed. +func TestRetainedStatusStillUpdatesMetadata(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-retained", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + retainedStatusMsg(retainedStatusTopic, `{"origin":"obs-retained","firmware":"v1.2.3","noise_floor":-95.5}`), + nil, nil, &Config{}) + + var firmware string + var noiseFloor float64 + if err := store.db.QueryRow( + `SELECT firmware, noise_floor FROM observers WHERE id = ?`, "obs-retained", + ).Scan(&firmware, &noiseFloor); err != nil { + t.Fatal(err) + } + if firmware != "v1.2.3" { + t.Errorf("firmware = %q, want %q", firmware, "v1.2.3") + } + if noiseFloor != -95.5 { + t.Errorf("noise_floor = %v, want -95.5", noiseFloor) + } +} + +// Regression guard for #1465: a live status message must still stamp last_seen +// with ingest time. Only the retained flag changes the behaviour. +func TestLiveStatusStillAdvancesLastSeen(t *testing.T) { + store := newTestStore(t) + before := seedObserver(t, store, "obs-live", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + &mockMessage{topic: "meshcore/LAX/obs-live/status", payload: []byte(`{"origin":"obs-live","noise_floor":-95.5}`)}, + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-live"); got == before { + t.Errorf("last_seen = %s, want it advanced past %s (live status is liveness)", got, before) + } +} + +func TestLiveStatusStillCreatesUnknownObserver(t *testing.T) { + store := newTestStore(t) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + &mockMessage{topic: "meshcore/LAX/obs-fresh/status", payload: []byte(`{"origin":"Fresh","noise_floor":-95.5}`)}, + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow(`SELECT COUNT(*) FROM observers WHERE id = ?`, "obs-fresh").Scan(&count); err != nil { + t.Fatal(err) + } + if count != 1 { + t.Errorf("observers row count = %d, want 1 (live status creates the observer)", count) + } +} diff --git a/cmd/ingestor/status_liveness.go b/cmd/ingestor/status_liveness.go new file mode 100644 index 000000000..9b9ef2893 --- /dev/null +++ b/cmd/ingestor/status_liveness.go @@ -0,0 +1,50 @@ +package main + +import ( + "strings" + "time" +) + +// statusLivenessMaxAge is how far a status payload's own timestamp may lag +// before the message stops counting as evidence the observer is alive now. +// +// Deliberately generous: observer clocks are naive (#1478) and can sit a whole +// timezone off, so anything under a day has to be accepted. The replays this +// guards against are months stale, so the margin costs nothing. +const statusLivenessMaxAge = 24 * time.Hour + +// statusIsLiveness reports whether a status message proves the observer was +// alive when we received it. +// +// The retain flag alone cannot answer that. Between EMQX and the ingestor sits +// a mosquitto bridge, and MQTT only sets RETAIN on messages delivered in +// response to a *new* subscription. The bridge runs cleansession, so every +// reconnect resubscribes and pulls EMQX's entire retained set — which it then +// republishes into its own broker as ordinary traffic. The ingestor is already +// subscribed there, so those months-old snapshots arrive with RETAIN cleared, +// indistinguishable from live status. On 2026-08-13 an EMQX restart replayed +// them and recreated 17 long-dead observers in two seconds. +// +// So judge the payload instead of the transport: +// - a status that is not "online" is the observer's own offline notice (its +// LWT, or a replay of it) and is never proof of life; +// - a timestamp older than statusLivenessMaxAge describes a past that may be +// months old. +// +// A payload with no timestamp, or one we cannot parse, is unjudgeable and +// keeps the previous behaviour — this guard removes false liveness, it does +// not invent new grounds to discard observers. +func statusIsLiveness(msg map[string]interface{}, now time.Time) bool { + if s, ok := msg["status"].(string); ok && s != "" && !strings.EqualFold(s, "online") { + return false + } + raw, _ := msg["timestamp"].(string) + if raw == "" { + return true + } + t, _, err := parseEnvelopeTime(raw) + if err != nil { + return true + } + return !t.Before(now.Add(-statusLivenessMaxAge)) +} diff --git a/cmd/ingestor/status_liveness_test.go b/cmd/ingestor/status_liveness_test.go new file mode 100644 index 000000000..6f96b07d1 --- /dev/null +++ b/cmd/ingestor/status_liveness_test.go @@ -0,0 +1,203 @@ +package main + +import ( + "fmt" + "testing" + "time" +) + +// The retain flag alone does not identify a replay. Our live topology puts a +// mosquitto bridge between EMQX and the ingestor, and MQTT only sets RETAIN on +// messages delivered in response to a *new* subscription. When the bridge +// reconnects (it runs cleansession, so every reconnect resubscribes), EMQX +// replays its whole retained set to the bridge, which republishes it into its +// own broker — and the ingestor, already subscribed, receives those months-old +// snapshots with RETAIN cleared, indistinguishable from live traffic. +// +// Observed on live 2026-08-13: EMQX restarted at 13:09 UTC and 17 long-dead +// observers were recreated inside two seconds, among them BE-BGS-RRY120-RES +// whose retained payload was published 2026-04-02 and says "offline". +// +// So judge the payload, not the transport: a status message is evidence of +// life only if it claims to be online and carries a timestamp from roughly now. + +func liveStatusMsg(topic, payload string) *mockMessage { + return &mockMessage{topic: topic, payload: []byte(payload), retained: false} +} + +func statusPayload(origin, status string, ts time.Time) string { + return fmt.Sprintf(`{"origin":%q,"status":%q,"timestamp":%q,"noise_floor":-95.5}`, + origin, status, ts.Format("2006-01-02T15:04:05.000000")) +} + +func TestStaleStatusPayloadDoesNotAdvanceLastSeen(t *testing.T) { + store := newTestStore(t) + want := seedObserver(t, store, "obs-zombie", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-zombie/status", + statusPayload("obs-zombie", "online", time.Now().UTC().AddDate(0, -4, 0))), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-zombie"); got != want { + t.Errorf("last_seen = %s, want %s (a 4-month-old payload is a replay, not liveness)", got, want) + } +} + +// The exact live case: an observer purged from the DB must not come back when +// the bridge replays its months-old retained snapshot with RETAIN cleared. +func TestStaleStatusPayloadDoesNotCreateObserver(t *testing.T) { + store := newTestStore(t) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/OST/obs-purged/status", + statusPayload("BE-BGS-RRY120-RES", "offline", time.Now().UTC().AddDate(0, -4, 0))), + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow(`SELECT COUNT(*) FROM observers WHERE id = ?`, "obs-purged").Scan(&count); err != nil { + t.Fatal(err) + } + if count != 0 { + t.Errorf("observers row count = %d, want 0 (replayed snapshot must not resurrect a purged observer)", count) + } +} + +// An offline notice is the observer's own death certificate — the broker +// replaying it, or the LWT firing, is never proof of life. +func TestOfflineStatusIsNotLiveness(t *testing.T) { + store := newTestStore(t) + want := seedObserver(t, store, "obs-offline", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-offline/status", + statusPayload("obs-offline", "offline", time.Now().UTC())), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-offline"); got != want { + t.Errorf("last_seen = %s, want %s (an offline status is not liveness)", got, want) + } +} + +func TestStaleStatusPayloadDoesNotReactivateInactiveObserver(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-zombie", 60) + if _, err := store.db.Exec(`UPDATE observers SET inactive = 1 WHERE id = ?`, "obs-zombie"); err != nil { + t.Fatal(err) + } + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-zombie/status", + statusPayload("obs-zombie", "online", time.Now().UTC().AddDate(0, -4, 0))), + nil, nil, &Config{}) + + var inactive int + if err := store.db.QueryRow(`SELECT inactive FROM observers WHERE id = ?`, "obs-zombie").Scan(&inactive); err != nil { + t.Fatal(err) + } + if inactive != 1 { + t.Errorf("inactive = %d, want 1 (a replay must not undo the soft-delete)", inactive) + } +} + +// ─── The guard must not swallow real observers ────────────────────────────── + +func TestFreshStatusStillCountsAsLiveness(t *testing.T) { + store := newTestStore(t) + stale := seedObserver(t, store, "obs-live", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-live/status", + statusPayload("obs-live", "online", time.Now().UTC())), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-live"); got == stale { + t.Errorf("last_seen = %s, want it advanced (a fresh online status is liveness)", got) + } +} + +// Observer clocks are naive and can sit a whole timezone off (#1478). A UTC-12 +// observer must not be mistaken for a replay. +func TestNaiveClockSkewStillCountsAsLiveness(t *testing.T) { + store := newTestStore(t) + stale := seedObserver(t, store, "obs-skewed", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-skewed/status", + statusPayload("obs-skewed", "online", time.Now().UTC().Add(-12*time.Hour))), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-skewed"); got == stale { + t.Errorf("last_seen = %s, want it advanced (12h naive skew is a timezone, not a replay)", got) + } +} + +func TestStatusWithoutTimestampStillCountsAsLiveness(t *testing.T) { + store := newTestStore(t) + stale := seedObserver(t, store, "obs-nots", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-nots/status", `{"origin":"obs-nots","noise_floor":-95.5}`), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-nots"); got == stale { + t.Errorf("last_seen = %s, want it advanced (no timestamp to judge — keep the old behaviour)", got) + } +} + +func TestStatusWithUnparseableTimestampStillCountsAsLiveness(t *testing.T) { + store := newTestStore(t) + stale := seedObserver(t, store, "obs-badts", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-badts/status", + `{"origin":"obs-badts","status":"online","timestamp":"not-a-time","noise_floor":-95.5}`), + nil, nil, &Config{}) + + if got := observerLastSeen(t, store, "obs-badts"); got == stale { + t.Errorf("last_seen = %s, want it advanced (unjudgeable timestamp — keep the old behaviour)", got) + } +} + +// A replayed snapshot is still the observer's last known state: metadata is +// kept, only the liveness signal is suppressed. +func TestStaleStatusPayloadStillUpdatesMetadata(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-zombie", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-zombie/status", + fmt.Sprintf(`{"origin":"obs-zombie","status":"online","timestamp":%q,"firmware":"v1.14.1","noise_floor":-95.5}`, + time.Now().UTC().AddDate(0, -4, 0).Format("2006-01-02T15:04:05.000000"))), + nil, nil, &Config{}) + + var firmware string + if err := store.db.QueryRow(`SELECT firmware FROM observers WHERE id = ?`, "obs-zombie").Scan(&firmware); err != nil { + t.Fatal(err) + } + if firmware != "v1.14.1" { + t.Errorf("firmware = %q, want %q (metadata from a replay is still the last known state)", firmware, "v1.14.1") + } +} + +// No metrics sample either — a months-old reading filed at ingest time reads +// as a present-tense measurement. +func TestStaleStatusPayloadDoesNotInsertMetricsSample(t *testing.T) { + store := newTestStore(t) + seedObserver(t, store, "obs-zombie", 60) + + handleMessage(store, "test", MQTTSource{Name: "test"}, + liveStatusMsg("meshcore/LAX/obs-zombie/status", + statusPayload("obs-zombie", "online", time.Now().UTC().AddDate(0, -4, 0))), + nil, nil, &Config{}) + + var count int + if err := store.db.QueryRow( + `SELECT COUNT(*) FROM observer_metrics WHERE observer_id = ?`, "obs-zombie", + ).Scan(&count); err != nil { + t.Fatal(err) + } + if count != 0 { + t.Errorf("observer_metrics rows = %d, want 0 (a replayed reading is not a new sample)", count) + } +} diff --git a/cmd/server/config.go b/cmd/server/config.go index 40b463623..b25add72a 100644 --- a/cmd/server/config.go +++ b/cmd/server/config.go @@ -279,6 +279,10 @@ type Config struct { // so browsers enforce same-origin policy. Set to ["*"] to allow all origins. CORSAllowedOrigins []string `json:"corsAllowedOrigins,omitempty"` + // WebSocket carries the /ws transport limits from #1794. Omitted entirely + // means: rate limit at its default, connection cap off, no deny list. + WebSocket *WebSocketConfig `json:"webSocket,omitempty"` + DebugAffinity bool `json:"debugAffinity,omitempty"` // MapDarkTileProvider selects the default dark-mode basemap provider for @@ -619,6 +623,81 @@ func LoadConfig(baseDirs ...string) (*Config, error) { return cfg, nil // defaults } +// WebSocketConfig holds the /ws transport limits (#1794). These sit behind +// the CheckOrigin allowlist (#1793) and catch non-browser clients, which can +// omit or forge Origin. +type WebSocketConfig struct { + // MaxConnsPerIP caps concurrent /ws connections from one client address. + // DEFAULT 0, meaning OFF, and that is a deliberate departure from the + // value floated on #1794. + // + // A cap of 5 is safe only when one address means one household. It does + // not on mobile: carrier-grade NAT puts thousands of unrelated subscribers + // behind a single public IPv4, so a low cap would refuse real visitors on + // phones while barely inconveniencing a scraper that can rent more + // addresses. Operators who know their audience can set it; we must not + // pick it for them. + MaxConnsPerIP int `json:"maxConnsPerIP,omitempty"` + + // UpgradesPerMinPerIP caps handshakes per minute from one client address. + // DEFAULT 30, on. Unlike the connection cap this is safe under CGNAT: a + // real client upgrades a handful of times per minute even while + // reconnecting, so 30 leaves ordinary traffic untouched while flattening + // the reconnect loop that makes scrapers expensive. + UpgradesPerMinPerIP *int `json:"upgradesPerMinPerIP,omitempty"` + + // TrustedProxies lists the addresses or CIDRs of reverse proxies whose + // X-Forwarded-For may be believed. REQUIRED for the limits to do anything + // behind nginx/Caddy/Traefik/ingress: without it every visitor shares the + // proxy's address, so the limits are skipped and a warning is logged. + // Never list an address you do not control; anyone reaching the server + // from a trusted address can name any client IP they like. + TrustedProxies []string `json:"trustedProxies,omitempty"` + + // Deny blocks addresses outright at the upgrade, before the handshake. + // Accepts both bare addresses ("1.2.3.4") and CIDRs ("1.2.3.0/24"). + // Applies even when clients cannot be told apart, because it is an + // explicit instruction rather than an inference. + Deny []string `json:"deny,omitempty"` +} + +// WSMaxConnsPerIP returns the configured concurrent-connection cap, 0 = off. +func (c *Config) WSMaxConnsPerIP() int { + if c.WebSocket == nil || c.WebSocket.MaxConnsPerIP < 0 { + return 0 + } + return c.WebSocket.MaxConnsPerIP +} + +// WSUpgradesPerMinPerIP returns the upgrade-rate cap, defaulting to 30 when +// unset. A pointer distinguishes "not configured" (use the default) from an +// explicit 0, which an operator uses to turn the limit off. +func (c *Config) WSUpgradesPerMinPerIP() int { + if c.WebSocket == nil || c.WebSocket.UpgradesPerMinPerIP == nil { + return 30 + } + if v := *c.WebSocket.UpgradesPerMinPerIP; v > 0 { + return v + } + return 0 +} + +// WSTrustedProxies returns the configured reverse-proxy addresses, if any. +func (c *Config) WSTrustedProxies() []string { + if c.WebSocket == nil { + return nil + } + return c.WebSocket.TrustedProxies +} + +// WSDeny returns the configured deny entries, if any. +func (c *Config) WSDeny() []string { + if c.WebSocket == nil { + return nil + } + return c.WebSocket.Deny +} + func (c *Config) applyListLimitsDefaults() { if c.ListLimits == nil { c.ListLimits = &ListLimitsConfig{} diff --git a/cmd/server/coverage_test.go b/cmd/server/coverage_test.go index d4bba14df..4be9a2845 100644 --- a/cmd/server/coverage_test.go +++ b/cmd/server/coverage_test.go @@ -3341,17 +3341,20 @@ func TestHashSizeTransportDirectZeroHopSkipped(t *testing.T) { db.conn.Exec(`INSERT INTO observers (id, name, iata, last_seen, first_seen, packet_count) VALUES ('obs1', 'Obs', 'SJC', ?, '2026-01-01T00:00:00Z', 10)`, recent) - // RouteDirect (2) zero-hop: path byte 0x40 → hop_count=0, hash_size bits=01 - // Should be skipped (existing behavior) + // RouteDirect (2) zero-hop with an all-zero path byte: the sender wiped the + // size bits, so there is nothing to read. Should be skipped. + // (This fixture used to be 0x40. A zero hop count with the size bits SET is + // a deliberate declaration — see TestZeroHopDirectAdvertDeclaredSizeIsEvidence + // — so the skip is keyed on the byte's content, not on the route type.) db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) - VALUES ('1240aabbccdd', 'direct_zh', ?, 2, 4, '{"pubKey":"bbbb000000000001","name":"Direct-ZH","type":"ADVERT"}')`, recent) + VALUES ('1200aabbccdd', 'direct_zh', ?, 2, 4, '{"pubKey":"bbbb000000000001","name":"Direct-ZH","type":"ADVERT"}')`, recent) db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) VALUES (1, 1, 10.0, -90, '[]', ?)`, recentEpoch) - // RouteTransportDirect (3) zero-hop: 4 transport bytes + path byte 0x40 → hop_count=0 - // Should ALSO be skipped (this was the missing case) + // RouteTransportDirect (3) zero-hop: 4 transport bytes + all-zero path byte + // Should ALSO be skipped (this was the missing case in #747) db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) - VALUES ('130102030440aabb', 'tdirect_zh', ?, 3, 4, '{"pubKey":"bbbb000000000002","name":"TDirect-ZH","type":"ADVERT"}')`, recent) + VALUES ('130102030400aabb', 'tdirect_zh', ?, 3, 4, '{"pubKey":"bbbb000000000002","name":"TDirect-ZH","type":"ADVERT"}')`, recent) db.conn.Exec(`INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) VALUES (2, 1, 10.0, -90, '[]', ?)`, recentEpoch) diff --git a/cmd/server/db.go b/cmd/server/db.go index 0fbd4b3c4..872656067 100644 --- a/cmd/server/db.go +++ b/cmd/server/db.go @@ -1041,8 +1041,11 @@ func (db *DB) GetNodes(limit, offset int, role, search, before, lastHeard, sortB if !db.isV3() { joinCond = "obs.id = o.observer_id" } + // #1143: from_pubkey is a dedicated, indexed column populated at + // ingest (and backfilled) for ADVERT rows specifically so pubkey + // lookups don't need to JSON_EXTRACT + parse decoded_json per row. subq := fmt.Sprintf(`public_key IN ( - SELECT DISTINCT JSON_EXTRACT(t.decoded_json, '$.pubKey') + SELECT DISTINCT t.from_pubkey FROM transmissions t JOIN observations o ON o.transmission_id = t.id JOIN observers obs ON %s diff --git a/cmd/server/hash_migrate.go b/cmd/server/hash_migrate.go index cd5929221..a85d58bc1 100644 --- a/cmd/server/hash_migrate.go +++ b/cmd/server/hash_migrate.go @@ -10,9 +10,23 @@ import ( // both the DB and the in-memory byHash index. The migration is idempotent: // once all hashes match the current formula it completes instantly. func migrateContentHashesAsync(store *PacketStore, batchSize int, yieldDuration time.Duration) { + // #1856: every DB failure below continues to the next batch, so the loop + // always reaches the deferred completion. Setting the flag unconditionally + // therefore reported success after migrating nothing, which is exactly what + // happens on the read-only handle the server holds since #1283: begin, + // prepare and commit all fail, every batch is skipped, and /api/stats then + // answers hashMigrationComplete: true. + failedBatches := 0 defer func() { if r := recover(); r != nil { log.Printf("[hash-migrate] panic recovered: %v", r) + failedBatches++ + } + if failedBatches > 0 { + log.Printf("[hash-migrate] INCOMPLETE: %d batch(es) could not be written; "+ + "hashMigrationComplete stays false. On a read-only DB handle this is expected "+ + "and the migration belongs in the ingestor (#1856).", failedBatches) + return } store.hashMigrationComplete.Store(true) }() @@ -57,12 +71,14 @@ func migrateContentHashesAsync(store *PacketStore, batchSize int, yieldDuration dbTx, err := store.db.conn.Begin() if err != nil { log.Printf("[hash-migrate] begin tx: %v", err) + failedBatches++ continue } stmt, err := dbTx.Prepare("UPDATE transmissions SET hash = ? WHERE id = ?") if err != nil { log.Printf("[hash-migrate] prepare: %v", err) dbTx.Rollback() + failedBatches++ continue } @@ -83,6 +99,7 @@ func migrateContentHashesAsync(store *PacketStore, batchSize int, yieldDuration if err := dbTx.Commit(); err != nil { log.Printf("[hash-migrate] commit: %v", err) + failedBatches++ continue } diff --git a/cmd/server/hash_migrate_test.go b/cmd/server/hash_migrate_test.go index f1c816de9..0cc7f21e6 100644 --- a/cmd/server/hash_migrate_test.go +++ b/cmd/server/hash_migrate_test.go @@ -76,3 +76,47 @@ func TestMigrateContentHashesAsync_NoOp(t *testing.T) { t.Error("hash should remain in index") } } + +// #1856: a migration that could not write anything must not report completion. +// +// In production the server holds a mode=ro handle (#1283), so Begin, Prepare and +// Commit all fail, every batch takes a `continue`, and the loop reaches the +// deferred completion having migrated nothing. Before this fix the flag was set +// unconditionally there, so /api/stats answered hashMigrationComplete: true +// after doing no work at all. +// +// Closing the handle stands in for the read-only one: it is deterministic and it +// exercises the identical failure path (Begin returns an error, batch skipped). +func TestMigrateContentHashesAsyncDoesNotClaimCompletionWhenWritesFail(t *testing.T) { + db := setupTestDBv2(t) + store := NewPacketStore(db, nil) + + rawHex := "0A00D69FD7A5A7475DB07337749AE61FA53A4788E976" + wrongHash := "deadbeef12345678" + if _, err := db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type) + VALUES (?, ?, datetime('now'), 0, 2)`, rawHex, wrongHash); err != nil { + t.Fatal(err) + } + if err := store.Load(); err != nil { + t.Fatal(err) + } + if store.byHash[wrongHash] == nil { + t.Fatal("expected packet under the wrong hash before migration") + } + + // Make every write fail, the way a read-only handle does in production. + if err := db.conn.Close(); err != nil { + t.Fatal(err) + } + + migrateContentHashesAsync(store, 100, time.Millisecond) + + if store.hashMigrationComplete.Load() { + t.Error("hashMigrationComplete must stay false when no batch could be written; " + + "reporting true here is what #1856 called self-reported success") + } + if store.byHash[wrongHash] == nil { + t.Error("the in-memory index must be left alone when the DB write failed, " + + "otherwise memory and disk disagree") + } +} diff --git a/cmd/server/main.go b/cmd/server/main.go index 1c31a277a..7928f73c2 100644 --- a/cmd/server/main.go +++ b/cmd/server/main.go @@ -349,6 +349,8 @@ func main() { // WebSocket hub hub := NewHub() hub.SetAllowedOrigins(cfg.CORSAllowedOrigins) + hub.ConfigureLimits(cfg.WSMaxConnsPerIP(), cfg.WSUpgradesPerMinPerIP(), + cfg.WSTrustedProxies(), cfg.WSDeny()) // #1794 hub.upgrader.EnableCompression = cfg.WSCompressionEnabled() // HTTP server diff --git a/cmd/server/neighbor_persist.go b/cmd/server/neighbor_persist.go index 5204147be..134b6b530 100644 --- a/cmd/server/neighbor_persist.go +++ b/cmd/server/neighbor_persist.go @@ -132,9 +132,14 @@ func resolvePathForObs(pathJSON, observerID string, tx *StoreTx, pm *prefixMap, contextPKs = append(contextPKs, strings.ToLower(fromNode)) } resolved := make([]*string, len(hops)) + // Reuse a single ctx buffer across hops instead of allocating per hop. + // Capacity +2 for the previous-hop resolved PK that may be appended. + ctx := make([]string, len(contextPKs), len(contextPKs)+2) + copy(ctx, contextPKs) + ctxLen := len(ctx) for i, hop := range hops { - ctx := make([]string, len(contextPKs), len(contextPKs)+2) - copy(ctx, contextPKs) + // Reset to base context (trim any previously appended resolved PK) + ctx = ctx[:ctxLen] if i > 0 && resolved[i-1] != nil { ctx = append(ctx, *resolved[i-1]) } diff --git a/cmd/server/pathhop_rebuild_1904_test.go b/cmd/server/pathhop_rebuild_1904_test.go new file mode 100644 index 000000000..7848cb6af --- /dev/null +++ b/cmd/server/pathhop_rebuild_1904_test.go @@ -0,0 +1,105 @@ +package main + +import ( + "sync" + "testing" +) + +// Issue #1904: buildPathHopIndex() rebuilds byPathHop from raw path_json +// hops and opens by discarding the map. But byPathHop also holds resolved +// full-pubkey keys fed by indexResolvedPathHops, and those pubkey strings +// are retained nowhere (#800 dropped the per-StoreTx ResolvedPath field in +// favour of a hash-only index), so a plain rebuild silently drops every +// resolved attribution. On a cold start that leaves relay_count_*, +// last_relayed and transported_scopes empty for every node until live +// ingestion refills the index. +// +// The rebuild must therefore RETAIN resolved entries — but only for +// transmissions still in s.packets. Eviction's removeTxFromPathHopIndex +// only strips raw hops (it derives them from txGetParsedPath), so evicted +// transmissions linger under their resolved keys; the rebuild is where +// they get dropped. + +const ( + hop1904Raw = "a3" + hop1904Resolved = "a3f19c2b7d4e5081aa22bb33cc44dd55ee66ff778899000112233445566778899" + hop1904Evicted = "b7002211ffeeddccbbaa99887766554433221100aabbccddeeff001122334455" +) + +// pathHopTx builds a transmission whose wire path is a single raw hop. +func pathHopTx(id int, path string) *StoreTx { + return &StoreTx{ID: id, Hash: "pathhop-tx-" + path, PathJSON: `["` + hop1904Raw + `"]`} +} + +func newPathHopStore(packets []*StoreTx, idx map[string][]*StoreTx) *PacketStore { + return &PacketStore{packets: packets, byPathHop: idx, mu: sync.RWMutex{}} +} + +func hopKeyHas(idx map[string][]*StoreTx, key string, tx *StoreTx) bool { + for _, t := range idx[key] { + if t == tx { + return true + } + } + return false +} + +func TestBuildPathHopIndex_RetainsResolvedHops_1904(t *testing.T) { + tx := pathHopTx(1, hop1904Raw) + // Shape produced by a chunk scan: raw hop from path_json PLUS the + // resolved full pubkey that indexResolvedPathHops added. + store := newPathHopStore([]*StoreTx{tx}, map[string][]*StoreTx{ + hop1904Raw: {tx}, + hop1904Resolved: {tx}, + }) + + store.buildPathHopIndex() + + if !hopKeyHas(store.byPathHop, hop1904Raw, tx) { + t.Fatalf("raw hop %q lost by rebuild", hop1904Raw) + } + if !hopKeyHas(store.byPathHop, hop1904Resolved, tx) { + t.Fatalf("resolved pubkey hop %q dropped by rebuild — #1904", hop1904Resolved) + } +} + +func TestBuildPathHopIndex_DropsResolvedHopsOfEvictedTx_1904(t *testing.T) { + live := pathHopTx(1, hop1904Raw) + evicted := pathHopTx(2, hop1904Raw) // NOT in s.packets any more + + store := newPathHopStore([]*StoreTx{live}, map[string][]*StoreTx{ + hop1904Raw: {live}, + hop1904Resolved: {live}, + hop1904Evicted: {evicted}, + }) + + store.buildPathHopIndex() + + if hopKeyHas(store.byPathHop, hop1904Evicted, evicted) { + t.Fatal("evicted transmission retained under its resolved key — the rebuild must not turn eviction's known gap into a permanent leak") + } + if _, ok := store.byPathHop[hop1904Evicted]; ok { + t.Fatalf("empty key %q left behind after dropping its only entry", hop1904Evicted) + } + if !hopKeyHas(store.byPathHop, hop1904Resolved, live) { + t.Fatal("live resolved hop dropped while pruning evicted ones") + } +} + +func TestBuildPathHopIndex_NoDuplicateOnRepeatedBuild_1904(t *testing.T) { + tx := pathHopTx(1, hop1904Raw) + store := newPathHopStore([]*StoreTx{tx}, map[string][]*StoreTx{ + hop1904Raw: {tx}, + hop1904Resolved: {tx}, + }) + + store.buildPathHopIndex() + store.buildPathHopIndex() + + if got := len(store.byPathHop[hop1904Raw]); got != 1 { + t.Fatalf("raw hop bucket has %d entries after two builds, want 1", got) + } + if got := len(store.byPathHop[hop1904Resolved]); got != 1 { + t.Fatalf("resolved hop bucket has %d entries after two builds, want 1", got) + } +} diff --git a/cmd/server/readonly_invariant_test.go b/cmd/server/readonly_invariant_test.go index cc2fdde70..896621826 100644 --- a/cmd/server/readonly_invariant_test.go +++ b/cmd/server/readonly_invariant_test.go @@ -92,6 +92,7 @@ func TestServerDBHasNoWriteMethods(t *testing.T) { "PruneOldPackets", "PruneOldMetrics", "RemoveStaleObservers", + "PurgeStaleObservers", // #738 / one-click geo-prune: the DELETE must live on the // ingestor's *Store. The server's HTTP handler now enqueues a // marker file (see internal/prunequeue); it does not write. diff --git a/cmd/server/routes.go b/cmd/server/routes.go index 6928c698e..cc2514627 100644 --- a/cmd/server/routes.go +++ b/cmd/server/routes.go @@ -886,8 +886,10 @@ func (s *Server) handleHealth(w http.ResponseWriter, r *http.Request) { uptime := time.Since(s.startedAt).Seconds() wsClients := 0 + var wsDeny, wsRate, wsConnCap int64 if s.hub != nil { wsClients = s.hub.ClientCount() + wsDeny, wsRate, wsConnCap = s.hub.limits.counts() // #1794; nil-safe } // Real packet store stats @@ -959,8 +961,9 @@ func (s *Server) handleHealth(w http.ResponseWriter, r *http.Request) { P95Ms: round(percentile(sortedPauses, 0.95), 1), P99Ms: round(percentile(sortedPauses, 0.99), 1), }, - Cache: cs, - WebSocket: WebSocketStatsResp{Clients: wsClients}, + Cache: cs, + WebSocket: WebSocketStatsResp{Clients: wsClients, + RejectedDeny: wsDeny, RejectedRate: wsRate, RejectedConnCap: wsConnCap}, PacketStore: HealthPacketStoreStats{ Packets: pktCount, EstimatedMB: pktEstMB, diff --git a/cmd/server/store.go b/cmd/server/store.go index 7a4a6ba86..d6efc649c 100644 --- a/cmd/server/store.go +++ b/cmd/server/store.go @@ -178,7 +178,6 @@ type PacketStore struct { byNode map[string][]*StoreTx // pubkey → transmissions nodeHashes map[string]map[string]bool // pubkey → Set byPathHop map[string][]*StoreTx // lowercase hop/pubkey → transmissions with that hop in path - relayTimes map[string][]int64 // lowercase pubkey → sorted unix-millis of relay events (full pubkeys only) byPayloadType map[int][]*StoreTx // payload_type → transmissions loaded bool totalObs int @@ -476,6 +475,13 @@ type PacketStore struct { trackedBytes int64 // running total of estimated packet store memory memoryEstimator func() float64 // injectable for tests; nil = use runtime.ReadMemStats (stats only) + // Per-store ReadMemStats cache (5s TTL). Fields (not package-level vars) so + // that test helpers constructing &PacketStore{...} directly get independent + // cache state, avoiding order-dependent test failures. + estMemMu sync.Mutex + estMemVal float64 + estMemAt time.Time + // Short-lived cache for the observations aggregate in GetStoreStats (30s TTL). // Avoids a per-/api/stats full-table scan; values accurate to ~30s which is // sufficient for dashboard display. @@ -646,7 +652,6 @@ func NewPacketStore(db *DB, cfg *PacketStoreConfig, cacheTTLs ...map[string]inte byObserver: make(map[string][]*StoreObs), byNode: make(map[string][]*StoreTx), byPathHop: make(map[string][]*StoreTx), - relayTimes: make(map[string][]int64), nodeHashes: make(map[string]map[string]bool), byPayloadType: make(map[int][]*StoreTx), rfCache: make(map[string]*cachedResult), @@ -4155,14 +4160,81 @@ func (s *PacketStore) buildSubpathIndex() { len(s.spIndex), s.spTotalPaths) } -// buildPathHopIndex scans all packets and populates byPathHop. +// buildPathHopIndex rebuilds byPathHop: raw wire hops from every packet's +// path_json, plus the resolved full-pubkey hops carried over from the +// previous index (see retainResolvedPathHops). // Must be called with s.mu held. func (s *PacketStore) buildPathHopIndex() { - s.byPathHop = make(map[string][]*StoreTx, 4096) + prev := s.byPathHop + s.byPathHop = make(map[string][]*StoreTx, max(4096, len(prev))) for _, tx := range s.packets { addTxToPathHopIndex(s.byPathHop, tx) } - log.Printf("[store] Built path-hop index: %d unique keys", len(s.byPathHop)) + retained := s.retainResolvedPathHops(prev) + log.Printf("[store] Built path-hop index: %d unique keys (%d resolved-hop entries retained)", + len(s.byPathHop), retained) +} + +// retainResolvedPathHops re-merges the entries of a pre-rebuild byPathHop +// that the raw-hop pass above cannot reproduce: the resolved full-pubkey +// keys fed by indexResolvedPathHops. Their pubkey strings are retained +// nowhere — #800 dropped the per-StoreTx ResolvedPath field in favour of a +// hash-only membership index — so a plain rebuild silently discarded every +// resolved relay attribution, leaving relay counts and transported scopes +// empty after each cold load until live ingestion refilled them (#1904). +// +// Only transmissions still in s.packets are carried over. This matters: +// eviction's removeTxFromPathHopIndex strips raw hops only (it derives them +// from txGetParsedPath), so evicted transmissions linger in prev under their +// resolved keys. Filtering them here is what keeps the index bounded by the +// eviction policy instead of turning that gap into a permanent leak. +// +// Cost is O(entries in prev) with one reused scratch map, and it runs only +// where buildPathHopIndex already runs — cold load and background-fill +// completion — never on an ingest or request path. +// +// Returns the number of entries carried over. Must be called with s.mu held, +// after s.byPathHop has been rebuilt from raw hops. +func (s *PacketStore) retainResolvedPathHops(prev map[string][]*StoreTx) int { + if len(prev) == 0 { + return 0 + } + live := make(map[*StoreTx]struct{}, len(s.packets)) + for _, tx := range s.packets { + live[tx] = struct{}{} + } + + // Reused across keys (cleared per key) so a large index does not churn + // one map allocation per key. Guards against both a key that the raw + // pass already produced and repeated appends of the same tx in prev — + // indexResolvedPathHops dedups within a call, not across the several + // observations of one transmission. + seen := make(map[*StoreTx]struct{}, 16) + retained := 0 + for key, list := range prev { + if len(list) == 0 { + continue + } + clear(seen) + for _, tx := range s.byPathHop[key] { + seen[tx] = struct{}{} + } + for _, tx := range list { + if tx == nil { + continue + } + if _, ok := live[tx]; !ok { + continue + } + if _, dup := seen[tx]; dup { + continue + } + seen[tx] = struct{}{} + s.byPathHop[key] = append(s.byPathHop[key], tx) + retained++ + } + } + return retained } // addTxToPathHopIndex indexes a transmission under each unique raw hop key. @@ -4587,13 +4659,24 @@ func estimateStoreObsBytes(obs *StoreObs) int64 { // estimatedMemoryMB returns current Go heap allocation in MB. // Kept for stats/debug endpoints only — NOT used in eviction decisions. // In tests, memoryEstimator can be set to inject a deterministic value. +// Caches the result for 5 seconds because runtime.ReadMemStats() stops the +// world and this is called from stats/debug endpoints that may be polled. +// The cache is per-store (not package-level) so that test helpers constructing +// &PacketStore{...} directly get independent cache state, avoiding +// order-dependent test failures from a shared global cache. func (s *PacketStore) estimatedMemoryMB() float64 { if s.memoryEstimator != nil { return s.memoryEstimator() } - var ms runtime.MemStats - runtime.ReadMemStats(&ms) - return float64(ms.HeapAlloc) / 1048576.0 + s.estMemMu.Lock() + defer s.estMemMu.Unlock() + if time.Since(s.estMemAt) > 5*time.Second { + var ms runtime.MemStats + runtime.ReadMemStats(&ms) + s.estMemVal = float64(ms.HeapAlloc) / 1048576.0 + s.estMemAt = time.Now() + } + return s.estMemVal } // trackedMemoryMB returns the self-accounted packet store memory in MB. @@ -8196,14 +8279,17 @@ func (s *PacketStore) computeAnalyticsHashSizes(region, area string) map[string] name = pk } } - // Skip zero-hop direct adverts for hash_size — the - // path byte is locally generated and unreliable. - // Still count the packet and update lastSeen. - isZeroHop := (routeType == uint64(RouteDirect) || routeType == uint64(RouteTransportDirect)) && (actualPathByte&0x3F) == 0 + // Skip zero-hop direct adverts whose path byte is entirely zero — + // there the size bits were wiped by the sender and say nothing. + // A non-zero byte with a zero hop count (0x40 / 0x80) is a + // deliberate size declaration; keep it. Same rule as + // computeNodeHashSizeInfo. Skipped packets still count and still + // update lastSeen. + isUndeclaredZeroHop := (routeType == uint64(RouteDirect) || routeType == uint64(RouteTransportDirect)) && actualPathByte == 0x00 if byNode[pk] == nil { role := nodeRoleByPK[pk] // empty if unknown initHS := hashSize - if isZeroHop { + if isUndeclaredZeroHop { initHS = 0 } byNode[pk] = map[string]interface{}{ @@ -8213,7 +8299,7 @@ func (s *PacketStore) computeAnalyticsHashSizes(region, area string) map[string] } } byNode[pk]["packets"] = byNode[pk]["packets"].(int) + 1 - if !isZeroHop { + if !isUndeclaredZeroHop { byNode[pk]["hashSize"] = hashSize } byNode[pk]["lastSeen"] = tx.FirstSeen @@ -8811,9 +8897,15 @@ func (s *PacketStore) computeNodeHashSizeInfo() map[string]*hashSizeNodeInfo { if err != nil { continue } - // Direct zero-hop adverts (route types 2 and 3) use path byte 0x00 - // locally and can misreport multibyte hash mode as 1-byte. - if (routeType == RouteDirect || routeType == RouteTransportDirect) && (pathByte&0x3F) == 0 { + // Direct zero-hop adverts carry no path, so the hop count is 0. Whether + // the SIZE bits are meaningful depends on the sender: firmware that + // predates meshcore-dev/MeshCore#3293 does `path_len = 0`, wiping the + // whole byte including the two size bits, so 0x00 says nothing about + // the node's path.hash.mode. A sender that writes the size through + // setPathHashSizeAndCount() emits 0x40 / 0x80 with a zero hop count — + // that is a deliberate declaration and the only way those bits can be + // non-zero here. Skip on the byte's content, not on the route type. + if (routeType == RouteDirect || routeType == RouteTransportDirect) && pathByte == 0x00 { continue } hs := int((pathByte>>6)&0x3) + 1 diff --git a/cmd/server/types.go b/cmd/server/types.go index c24b5341b..b3d81c7df 100644 --- a/cmd/server/types.go +++ b/cmd/server/types.go @@ -586,6 +586,13 @@ type PerfCacheStats struct { type WebSocketStatsResp struct { Clients int `json:"clients"` + // #1794: upgrade rejections since boot, split by cause so an operator can + // tell a deny-list hit from a client that is merely reconnecting too fast. + // omitempty: an install with no limits configured sends none of these + // rather than three permanent zeroes. + RejectedDeny int64 `json:"rejectedDeny,omitempty"` + RejectedRate int64 `json:"rejectedRate,omitempty"` + RejectedConnCap int64 `json:"rejectedConnCap,omitempty"` } type HealthPacketStoreStats struct { diff --git a/cmd/server/websocket.go b/cmd/server/websocket.go index ba4adc90d..a8b177305 100644 --- a/cmd/server/websocket.go +++ b/cmd/server/websocket.go @@ -17,7 +17,8 @@ type Hub struct { mu sync.RWMutex clients map[*Client]bool upgrader websocket.Upgrader - allowedOrigins []string // exact-match allowlist for /ws CheckOrigin (see SetAllowedOrigins) + allowedOrigins []string // exact-match allowlist for /ws CheckOrigin (see SetAllowedOrigins) + limits *wsLimiter // #1794: per-IP caps and deny list; nil allows everything } // SetAllowedOrigins configures the exact-match origin allowlist consulted by @@ -78,6 +79,32 @@ type Client struct { conn *websocket.Conn send chan []byte closeOnce sync.Once + // release returns this connection's slot to the per-IP concurrent cap + // (#1794). Always non-nil so Unregister needs no nil check; it is a no-op + // when limits are disabled. + release func() +} + +// ConfigureLimits installs the #1794 transport limits. Called once at +// startup, before the listener binds, so no upgrade can race a half-built +// limiter. Passing zero values for both caps leaves only the deny list +// active, which is a legitimate configuration. +func (h *Hub) ConfigureLimits(maxConnsPerIP, upgradesPerMin int, trustedProxies, deny []string) { + l := newWSLimiter() + l.maxConnsPerIP = maxConnsPerIP + l.upgradesPerMin = upgradesPerMin + l.trustedProxies = parseCIDRList(trustedProxies, "webSocket.trustedProxies") + l.denyNets = parseCIDRList(deny, "webSocket.deny") + h.mu.Lock() + h.limits = l + h.mu.Unlock() + if len(l.denyNets) > 0 { + log.Printf("[ws] deny list active: %d network(s)", len(l.denyNets)) + } + if len(l.trustedProxies) == 0 && (maxConnsPerIP > 0 || upgradesPerMin > 0) { + log.Printf("[ws] per-IP limits configured with no trustedProxies: they apply only to " + + "directly-connected clients. Behind a reverse proxy, set webSocket.trustedProxies.") + } } func NewHub() *Hub { @@ -112,6 +139,12 @@ func (h *Hub) Unregister(c *Client) { c.closeOnce.Do(func() { close(c.send) }) } h.mu.Unlock() + // #1794: outside the lock — release takes the limiter's own mutex, and + // holding two locks in one order here and the other order elsewhere is how + // deadlocks are made. Idempotent, so a double Unregister cannot over-credit. + if c.release != nil { + c.release() + } log.Printf("[ws] client disconnected (%d total)", h.ClientCount()) } @@ -151,15 +184,33 @@ func (h *Hub) Broadcast(msg interface{}) { // ServeWS handles the WebSocket upgrade and runs the client. func (h *Hub) ServeWS(w http.ResponseWriter, r *http.Request) { + // #1794: decide BEFORE the upgrade. Rejecting afterwards would already + // have allocated the connection and completed the handshake, which is the + // resource this is meant to protect. + ok, reason, key, release := h.limits.allow(r) + if !ok { + status := http.StatusForbidden + if reason == wsRejectRate { + // 429 tells a well-behaved client to back off; 403 would read as + // "never come back" for what is a temporary refusal. + status = http.StatusTooManyRequests + } + log.Printf("[ws] reject ip=%s reason=%s", key, reason) + http.Error(w, "websocket upgrade refused", status) + return + } + conn, err := h.upgrader.Upgrade(w, r, nil) if err != nil { log.Printf("[ws] upgrade error: %v", err) + release() // the slot was charged above and this connection never happened return } client := &Client{ - conn: conn, - send: make(chan []byte, 256), + conn: conn, + send: make(chan []byte, 256), + release: release, } h.Register(client) diff --git a/cmd/server/ws_limits.go b/cmd/server/ws_limits.go new file mode 100644 index 000000000..9638b9e33 --- /dev/null +++ b/cmd/server/ws_limits.go @@ -0,0 +1,274 @@ +package main + +// Issue #1794: per-IP limits and a deny list for the /ws upgrade, as defence +// in depth behind the CheckOrigin allowlist from #1793. +// +// CheckOrigin only stops browsers. A Go, Python or curl client can omit the +// Origin header entirely or forge one, connect, and sit in the hub. These +// limits work on the transport instead: who is connecting, how often, and how +// many at once. +// +// THE THING THAT MAKES THIS DANGEROUS TO SHIP NAIVELY, stated up front: +// most CoreScope installs run behind nginx, Caddy, Traefik or a k8s ingress +// (cdn_detection.go says so in as many words). For those, r.RemoteAddr is the +// proxy, 127.0.0.1 for every visitor on earth. A per-IP cap keyed on that +// address protects nobody; it counts the whole internet as one client and +// hands the sixth legitimate browser tab a 403. That is a self-inflicted +// outage, not hardening. +// +// So enforcement is conditional on being able to tell clients apart: +// - X-Forwarded-For is honoured ONLY when the request arrives from an +// address the operator listed in trustedProxies. Otherwise it is an +// attacker-supplied header, and trusting it would let anyone forge a +// fresh source IP per connection, which is worse than no limit at all. +// - If the peer looks like a local reverse proxy and no trustedProxies is +// configured, clients cannot be distinguished, so the per-IP limits are +// SKIPPED and one warning is logged telling the operator what to set. +// The deny list still applies, because an explicit block is the +// operator's own instruction rather than an inference. +// +// Not shipped: a default deny list of hosting-provider ranges. It was +// proposed on the issue (44 CIDRs for one VPS host, after a single scraper +// was seen). Blanket-blocking a provider by default breaks legitimate +// operators who host there, is undiscoverable by the person locked out, and +// ages badly as ranges are reassigned. Operators who want it can configure it. + +import ( + "log" + "net" + "net/http" + "strings" + "sync" + "sync/atomic" + "time" +) + +// wsUpgradeWindow is the span the upgrade budget is counted over. One minute +// matches the config field name (upgradesPerMinPerIP) so an operator reading +// the config can predict the behaviour without reading this file. +const wsUpgradeWindow = time.Minute + +// wsBucketIdleGC is how long an idle per-IP record is kept before collection. +// Longer than the window, so a client that pauses briefly cannot earn a fresh +// budget by being forgotten. +const wsBucketIdleGC = 10 * time.Minute + +// wsRejectReason is the label used both in the log line and in the +// /api/stats counters, so an operator can match one to the other. +type wsRejectReason string + +const ( + wsRejectDeny wsRejectReason = "deny" + wsRejectRate wsRejectReason = "rate" + wsRejectConnCap wsRejectReason = "conncap" +) + +type wsIPState struct { + conns int + upgrades []time.Time // timestamps inside the current window + lastSeenAt time.Time +} + +// wsLimiter holds the parsed configuration and the live per-IP state. A nil +// *wsLimiter allows everything, so a server that never configures limits +// behaves exactly as it did before this change. +type wsLimiter struct { + mu sync.Mutex + + maxConnsPerIP int // 0 disables the concurrent-connection cap + upgradesPerMin int // 0 disables the upgrade-rate limit + + trustedProxies []*net.IPNet + denyNets []*net.IPNet + + state map[string]*wsIPState + + rejectedDeny atomic.Int64 + rejectedRate atomic.Int64 + rejectedConnCap atomic.Int64 + + // warnedIndistinct fires the "set trustedProxies" warning once, rather + // than on every upgrade from behind an unconfigured proxy. + warnedIndistinct sync.Once +} + +func newWSLimiter() *wsLimiter { + return &wsLimiter{state: make(map[string]*wsIPState)} +} + +// parseCIDRList turns operator strings into networks. A bare address is +// accepted and treated as a single-host network, because "1.2.3.4" is what an +// operator naturally writes and silently ignoring it would be the worst +// possible failure mode for a deny list. An unparseable entry is logged and +// skipped rather than aborting startup: one typo must not take the server +// down, but it must not pass unnoticed either. +func parseCIDRList(entries []string, what string) []*net.IPNet { + out := make([]*net.IPNet, 0, len(entries)) + for _, raw := range entries { + e := strings.TrimSpace(raw) + if e == "" { + continue + } + if _, n, err := net.ParseCIDR(e); err == nil { + out = append(out, n) + continue + } + if ip := net.ParseIP(e); ip != nil { + bits := 32 + if ip.To4() == nil { + bits = 128 + } + out = append(out, &net.IPNet{IP: ip, Mask: net.CIDRMask(bits, bits)}) + continue + } + log.Printf("[ws] WARNING: %s entry %q is neither an IP nor a CIDR, ignored", what, e) + } + return out +} + +func ipInAny(ip net.IP, nets []*net.IPNet) bool { + if ip == nil { + return false + } + for _, n := range nets { + if n.Contains(ip) { + return true + } + } + return false +} + +// clientIP resolves the address the limits are counted against, and reports +// whether that address actually identifies a client. +// +// distinct is false when the peer is a loopback or private address and no +// trustedProxies is configured: the request has almost certainly crossed a +// reverse proxy whose X-Forwarded-For we are not allowed to believe, so every +// visitor looks identical and per-IP limits would punish the wrong people. +func (l *wsLimiter) clientIP(r *http.Request) (ip net.IP, distinct bool) { + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + host = r.RemoteAddr + } + peer := net.ParseIP(strings.TrimSpace(host)) + if peer == nil { + return nil, false + } + if ipInAny(peer, l.trustedProxies) { + // First hop is the client; later hops were appended by intermediaries + // we have no reason to trust individually. + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + first := strings.TrimSpace(strings.Split(xff, ",")[0]) + if fwd := net.ParseIP(first); fwd != nil { + return fwd, true + } + } + // A trusted proxy that forwarded no XFF still identifies itself. + return peer, true + } + if peer.IsLoopback() || peer.IsPrivate() || peer.IsLinkLocalUnicast() { + return peer, false + } + return peer, true +} + +// allow decides whether an upgrade may proceed. On rejection it returns the +// reason. On success it returns a release func the caller MUST invoke when +// the connection ends, so the concurrent-connection count comes back down. +func (l *wsLimiter) allow(r *http.Request) (ok bool, reason wsRejectReason, key string, release func()) { + noop := func() {} + if l == nil { + return true, "", "", noop + } + ip, distinct := l.clientIP(r) + + // The deny list is an explicit operator instruction, so it applies even + // when clients cannot be told apart. Behind an unconfigured proxy it + // simply never matches, which is visible rather than silently wrong. + if ipInAny(ip, l.denyNets) { + l.rejectedDeny.Add(1) + return false, wsRejectDeny, ip.String(), noop + } + if l.maxConnsPerIP <= 0 && l.upgradesPerMin <= 0 { + return true, "", "", noop + } + if !distinct { + l.warnedIndistinct.Do(func() { + log.Printf("[ws] WARNING: per-IP limits are configured but every request arrives from %s, "+ + "so clients cannot be told apart and the limits are NOT enforced. "+ + "Set webSocket.trustedProxies to your reverse proxy's address to enable them.", ip) + }) + return true, "", "", noop + } + + key = ip.String() + now := time.Now() + + l.mu.Lock() + defer l.mu.Unlock() + l.gcLocked(now) + + st := l.state[key] + if st == nil { + st = &wsIPState{} + l.state[key] = st + } + st.lastSeenAt = now + + if l.upgradesPerMin > 0 { + cutoff := now.Add(-wsUpgradeWindow) + kept := st.upgrades[:0] + for _, t := range st.upgrades { + if t.After(cutoff) { + kept = append(kept, t) + } + } + st.upgrades = kept + if len(st.upgrades) >= l.upgradesPerMin { + l.rejectedRate.Add(1) + return false, wsRejectRate, key, noop + } + } + if l.maxConnsPerIP > 0 && st.conns >= l.maxConnsPerIP { + l.rejectedConnCap.Add(1) + return false, wsRejectConnCap, key, noop + } + + // Both budgets have room, so charge them together: a rejected upgrade + // must never consume rate budget for a connection it did not get. + if l.upgradesPerMin > 0 { + st.upgrades = append(st.upgrades, now) + } + st.conns++ + + var once sync.Once + return true, "", key, func() { + once.Do(func() { + l.mu.Lock() + defer l.mu.Unlock() + if s := l.state[key]; s != nil && s.conns > 0 { + s.conns-- + s.lastSeenAt = time.Now() + } + }) + } +} + +// gcLocked drops records with no live connections and no recent activity, so +// the map cannot grow without bound when the server is scanned from many +// addresses. Caller must hold l.mu. +func (l *wsLimiter) gcLocked(now time.Time) { + for k, st := range l.state { + if st.conns == 0 && now.Sub(st.lastSeenAt) > wsBucketIdleGC { + delete(l.state, k) + } + } +} + +// counts reports the rejection tallies for /api/stats. +func (l *wsLimiter) counts() (deny, rate, connCap int64) { + if l == nil { + return 0, 0, 0 + } + return l.rejectedDeny.Load(), l.rejectedRate.Load(), l.rejectedConnCap.Load() +} diff --git a/cmd/server/ws_limits_test.go b/cmd/server/ws_limits_test.go new file mode 100644 index 000000000..cf025770f --- /dev/null +++ b/cmd/server/ws_limits_test.go @@ -0,0 +1,296 @@ +package main + +// Tests for the #1794 /ws transport limits. The five cases the issue lists as +// TDD requirements are marked with the exact wording from the issue body, so a +// reviewer can match them off without reading the implementation. + +import ( + "net/http" + "net/http/httptest" + "testing" + "time" +) + +func req(remoteAddr, xff string) *http.Request { + r := httptest.NewRequest(http.MethodGet, "/ws", nil) + r.RemoteAddr = remoteAddr + if xff != "" { + r.Header.Set("X-Forwarded-For", xff) + } + return r +} + +func limiter(maxConns, perMin int, trusted, deny []string) *wsLimiter { + l := newWSLimiter() + l.maxConnsPerIP = maxConns + l.upgradesPerMin = perMin + l.trustedProxies = parseCIDRList(trusted, "trusted") + l.denyNets = parseCIDRList(deny, "deny") + return l +} + +// "Deny CIDR match → 403, hub count unchanged." +func TestWSDeny_CIDRMatchRejects(t *testing.T) { + l := limiter(0, 0, nil, []string{"203.0.113.0/24"}) + ok, reason, _, _ := l.allow(req("203.0.113.7:1234", "")) + if ok || reason != wsRejectDeny { + t.Fatalf("denied CIDR: ok=%v reason=%q, want rejected with %q", ok, reason, wsRejectDeny) + } + if ok, _, _, _ := l.allow(req("203.0.114.7:1234", "")); !ok { + t.Error("an address outside the denied range must still connect") + } + if d, _, _ := l.counts(); d != 1 { + t.Errorf("rejectedDeny = %d, want 1", d) + } +} + +func TestWSDeny_BareAddressIsAcceptedAsSingleHost(t *testing.T) { + // Operators write "1.2.3.4", not "1.2.3.4/32". Silently ignoring the bare + // form would be the worst possible failure for a deny list: it looks + // configured and blocks nothing. + l := limiter(0, 0, nil, []string{"198.51.100.9"}) + if ok, _, _, _ := l.allow(req("198.51.100.9:5000", "")); ok { + t.Error("bare address in the deny list must block") + } + if ok, _, _, _ := l.allow(req("198.51.100.10:5000", "")); !ok { + t.Error("the neighbouring address must not be blocked") + } +} + +func TestWSDeny_UnparseableEntryIsSkippedNotFatal(t *testing.T) { + // One typo must not take the deny list, or the server, down. + nets := parseCIDRList([]string{"not-an-ip", "203.0.113.0/24"}, "deny") + if len(nets) != 1 { + t.Fatalf("parsed %d networks, want 1 (the valid entry)", len(nets)) + } +} + +// "6th conn from same IP with cap=5 → 403." +func TestWSConnCap_SixthConnectionRejected(t *testing.T) { + l := limiter(5, 0, nil, nil) + for i := 1; i <= 5; i++ { + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); !ok { + t.Fatalf("connection %d of 5 was rejected", i) + } + } + ok, reason, _, _ := l.allow(req("203.0.113.5:1", "")) + if ok || reason != wsRejectConnCap { + t.Fatalf("6th connection: ok=%v reason=%q, want rejected with %q", ok, reason, wsRejectConnCap) + } + if _, _, c := l.counts(); c != 1 { + t.Errorf("rejectedConnCap = %d, want 1", c) + } +} + +func TestWSConnCap_ReleaseFreesTheSlot(t *testing.T) { + l := limiter(1, 0, nil, nil) + _, _, _, release := l.allow(req("203.0.113.5:1", "")) + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); ok { + t.Fatal("cap=1 must refuse the second concurrent connection") + } + release() + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); !ok { + t.Error("after release the slot must be reusable") + } +} + +func TestWSConnCap_ReleaseIsIdempotent(t *testing.T) { + // Unregister can run twice for one client; double-crediting would let an + // IP accumulate slots it never gave back. + l := limiter(1, 0, nil, nil) + _, _, _, release := l.allow(req("203.0.113.5:1", "")) + release() + release() + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); !ok { + t.Fatal("first reconnect must be allowed") + } + if ok, _, _, _ := l.allow(req("203.0.113.5:1", "")); ok { + t.Error("cap=1 must still hold: the double release must not have credited two slots") + } +} + +// "31 upgrades/min from same IP → 31st rejected." +func TestWSRate_ThirtyFirstUpgradeRejected(t *testing.T) { + l := limiter(0, 30, nil, nil) + for i := 1; i <= 30; i++ { + if ok, _, _, _ := l.allow(req("203.0.113.8:1", "")); !ok { + t.Fatalf("upgrade %d of 30 was rejected", i) + } + } + ok, reason, _, _ := l.allow(req("203.0.113.8:1", "")) + if ok || reason != wsRejectRate { + t.Fatalf("31st upgrade: ok=%v reason=%q, want rejected with %q", ok, reason, wsRejectRate) + } + if _, r, _ := l.counts(); r != 1 { + t.Errorf("rejectedRate = %d, want 1", r) + } +} + +func TestWSRate_BudgetIsPerAddress(t *testing.T) { + l := limiter(0, 2, nil, nil) + l.allow(req("203.0.113.1:1", "")) + l.allow(req("203.0.113.1:1", "")) + if ok, _, _, _ := l.allow(req("203.0.113.1:1", "")); ok { + t.Fatal("the first address should be out of budget") + } + if ok, _, _, _ := l.allow(req("203.0.113.2:1", "")); !ok { + t.Error("a different address must have its own budget") + } +} + +func TestWSRate_OldAttemptsLeaveTheWindow(t *testing.T) { + l := limiter(0, 2, nil, nil) + key := "203.0.113.3" + l.state[key] = &wsIPState{ + upgrades: []time.Time{time.Now().Add(-2 * wsUpgradeWindow), time.Now().Add(-90 * time.Second)}, + lastSeenAt: time.Now(), + } + if ok, _, _, _ := l.allow(req(key+":1", "")); !ok { + t.Error("attempts older than the window must not count against the budget") + } +} + +func TestWSRate_RejectedUpgradeDoesNotConsumeBudget(t *testing.T) { + // A refusal must not push the client further into debt, or a client that + // keeps retrying could never recover even once its window cleared. + l := limiter(1, 5, nil, nil) + l.allow(req("203.0.113.4:1", "")) // takes the only conn slot, 1 upgrade used + for i := 0; i < 3; i++ { + l.allow(req("203.0.113.4:1", "")) // all rejected on the conn cap + } + l.mu.Lock() + used := len(l.state["203.0.113.4"].upgrades) + l.mu.Unlock() + if used != 1 { + t.Errorf("upgrades charged = %d, want 1 (rejections must not be charged)", used) + } +} + +// "Trusted proxy + XFF spoofing → rate counted against the XFF IP, not the proxy." +func TestWSClientIP_TrustedProxyUsesForwardedFor(t *testing.T) { + l := limiter(0, 1, []string{"10.0.0.0/8"}, nil) + if ok, _, key, _ := l.allow(req("10.0.0.2:1", "203.0.113.20, 10.0.0.2")); !ok || key != "203.0.113.20" { + t.Fatalf("key = %q (ok=%v), want the first XFF hop 203.0.113.20", key, ok) + } + // Budget of 1 is now spent for that client, but a different client behind + // the SAME proxy must be unaffected. This is the whole point of honouring + // XFF: one visitor must not exhaust the budget for everyone else. + if ok, _, _, _ := l.allow(req("10.0.0.2:1", "203.0.113.21, 10.0.0.2")); !ok { + t.Error("a second client behind the same trusted proxy must have its own budget") + } + if ok, _, _, _ := l.allow(req("10.0.0.2:1", "203.0.113.20, 10.0.0.2")); ok { + t.Error("the first client's budget should still be spent") + } +} + +// "Untrusted RemoteAddr + XFF spoofing → XFF ignored, rate counted against RemoteAddr." +func TestWSClientIP_UntrustedPeerIgnoresForwardedFor(t *testing.T) { + l := limiter(0, 1, []string{"10.0.0.0/8"}, nil) + // A public peer that is not a configured proxy. If XFF were believed here, + // an attacker would mint a fresh identity per request and the limit would + // be worthless. + if ok, _, key, _ := l.allow(req("203.0.113.30:1", "198.51.100.1")); !ok || key != "203.0.113.30" { + t.Fatalf("key = %q (ok=%v), want the peer address, not the forged header", key, ok) + } + if ok, _, _, _ := l.allow(req("203.0.113.30:1", "198.51.100.2")); ok { + t.Error("changing the forged header must not buy a fresh budget") + } +} + +func TestWSClientIP_TrustedProxyWithoutForwardedForUsesPeer(t *testing.T) { + l := limiter(0, 5, []string{"10.0.0.0/8"}, nil) + if _, _, key, _ := l.allow(req("10.0.0.2:1", "")); key != "10.0.0.2" { + t.Errorf("key = %q, want the proxy's own address when it forwards nothing", key) + } +} + +// The safety property this design exists for: behind an unconfigured reverse +// proxy every visitor shares one address, so enforcing a per-IP cap would +// refuse real users. Limits must be skipped, not applied. +func TestWSLimits_SkippedWhenClientsCannotBeToldApart(t *testing.T) { + l := limiter(1, 1, nil, nil) // caps of 1, and no trustedProxies + for i := 0; i < 10; i++ { + if ok, reason, _, _ := l.allow(req("127.0.0.1:1", "203.0.113.9")); !ok { + t.Fatalf("request %d refused (%s): loopback peer with no trustedProxies must not be limited", i, reason) + } + } + if _, r, c := l.counts(); r != 0 || c != 0 { + t.Errorf("rejections rate=%d conncap=%d, want 0 and 0", r, c) + } +} + +func TestWSLimits_PrivatePeerAlsoCountsAsIndistinct(t *testing.T) { + l := limiter(1, 1, nil, nil) + for i := 0; i < 5; i++ { + if ok, _, _, _ := l.allow(req("192.168.1.10:1", "")); !ok { + t.Fatalf("request %d refused: a private peer with no trustedProxies must not be limited", i) + } + } +} + +// The deny list is an explicit operator instruction, so unlike the caps it +// still applies when clients cannot be told apart. +func TestWSDeny_AppliesEvenWhenIndistinct(t *testing.T) { + l := limiter(0, 0, nil, []string{"127.0.0.1"}) + if ok, reason, _, _ := l.allow(req("127.0.0.1:1", "")); ok || reason != wsRejectDeny { + t.Errorf("ok=%v reason=%q, want the deny list to apply regardless", ok, reason) + } +} + +func TestWSLimits_NilLimiterAllowsEverything(t *testing.T) { + // A Hub built without ConfigureLimits must behave exactly as before. + var l *wsLimiter + ok, _, _, release := l.allow(req("203.0.113.99:1", "")) + if !ok { + t.Fatal("a nil limiter must allow the upgrade") + } + release() // must not panic + if d, r, c := l.counts(); d != 0 || r != 0 || c != 0 { + t.Errorf("nil limiter counts = %d/%d/%d, want zeroes", d, r, c) + } +} + +func TestWSLimits_IdleStateIsCollected(t *testing.T) { + // Without collection, scanning the server from many addresses would grow + // the map forever. + l := limiter(0, 5, nil, nil) + l.state["203.0.113.50"] = &wsIPState{conns: 0, lastSeenAt: time.Now().Add(-2 * wsBucketIdleGC)} + l.state["203.0.113.51"] = &wsIPState{conns: 1, lastSeenAt: time.Now().Add(-2 * wsBucketIdleGC)} + l.mu.Lock() + l.gcLocked(time.Now()) + _, idleKept := l.state["203.0.113.50"] + _, liveKept := l.state["203.0.113.51"] + l.mu.Unlock() + if idleKept { + t.Error("an idle record with no connections should be collected") + } + if !liveKept { + t.Error("a record with a live connection must never be collected") + } +} + +func TestWSConfig_Defaults(t *testing.T) { + // The connection cap is deliberately OFF by default: carrier-grade NAT + // puts many unrelated subscribers behind one address, so a low cap would + // refuse real visitors on mobile networks. + var c Config + if got := c.WSMaxConnsPerIP(); got != 0 { + t.Errorf("default maxConnsPerIP = %d, want 0 (off)", got) + } + if got := c.WSUpgradesPerMinPerIP(); got != 30 { + t.Errorf("default upgradesPerMinPerIP = %d, want 30", got) + } + zero := 0 + c.WebSocket = &WebSocketConfig{UpgradesPerMinPerIP: &zero} + if got := c.WSUpgradesPerMinPerIP(); got != 0 { + t.Errorf("explicit 0 = %d, want 0: an operator must be able to turn the rate limit off", got) + } + five := 5 + c.WebSocket = &WebSocketConfig{UpgradesPerMinPerIP: &five, MaxConnsPerIP: 3} + if got := c.WSUpgradesPerMinPerIP(); got != 5 { + t.Errorf("configured rate = %d, want 5", got) + } + if got := c.WSMaxConnsPerIP(); got != 3 { + t.Errorf("configured cap = %d, want 3", got) + } +} diff --git a/cmd/server/zerohop_hashsize_test.go b/cmd/server/zerohop_hashsize_test.go new file mode 100644 index 000000000..6509989e6 --- /dev/null +++ b/cmd/server/zerohop_hashsize_test.go @@ -0,0 +1,187 @@ +package main + +import ( + "fmt" + "testing" + "time" +) + +// Zero-hop direct adverts carry no path, so the hop count is always 0. Whether +// the two SIZE bits next to it mean anything depends on the sender: +// +// - Firmware predating meshcore-dev/MeshCore#3293 does `path_len = 0`, which +// wipes the whole byte. 0x00 therefore says nothing about path.hash.mode +// and must not be read as "1 byte". +// - A sender that writes the size through setPathHashSizeAndCount() emits +// 0x40 (2 bytes) or 0x80 (3 bytes) with a zero hop count. Nothing else can +// produce those bits here, so they are a deliberate declaration. +// +// Before this fix the skip was keyed on the route type and swallowed both +// cases. These tests pin the distinction. The two multi-byte fixtures are real +// packets off the Czech mesh (869.4 MHz), captured 2026-08-24/25. + +// insertAdvert adds one ADVERT transmission plus a matching observation. +func insertAdvert(t *testing.T, db *DB, id int, rawHex, hash, pubKey, name string, routeType int, seen string, seenEpoch int64) { + t.Helper() + if _, err := db.conn.Exec( + `INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json) + VALUES (?, ?, ?, ?, 4, ?)`, + rawHex, hash, seen, routeType, + fmt.Sprintf(`{"pubKey":"%s","name":"%s","type":"ADVERT"}`, pubKey, name), + ); err != nil { + t.Fatalf("insert transmission %s: %v", hash, err) + } + if _, err := db.conn.Exec( + `INSERT INTO observations (transmission_id, observer_idx, snr, rssi, path_json, timestamp) + VALUES (?, 1, 10.0, -90, '[]', ?)`, id, seenEpoch, + ); err != nil { + t.Fatalf("insert observation %s: %v", hash, err) + } +} + +func zeroHopStore(t *testing.T) (*DB, string, int64) { + t.Helper() + db := setupTestDB(t) + now := time.Now().UTC().Add(-1 * time.Hour) + recent := now.Format(time.RFC3339) + db.conn.Exec(`INSERT INTO observers (id, name, iata, last_seen, first_seen, packet_count) + VALUES ('obs1', 'Obs', 'PRG', ?, '2026-01-01T00:00:00Z', 10)`, recent) + return db, recent, now.Unix() +} + +// TestZeroHopDirectAdvertDeclaredSizeIsEvidence: a zero-hop DIRECT advert whose +// path byte is 0x40 declares a 2-byte path hash and must be counted. +// Fixture: tth-hrebecna, 2026-08-25T09:29:40Z, header 0x12 (route 2, ADVERT). +func TestZeroHopDirectAdvertDeclaredSizeIsEvidence(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const hrebecnaPK = "5361aa08929b6248bb61cb1fab0fdb97d6dba8ae44f53a44427f63e7798a6b6b" + insertAdvert(t, db, 1, + "1240"+hrebecnaPK+"83608d6a3d38eeb187a94a745daf2ffb3191d1ae49fb82f30a083ba081ffbb20"+ + "266331879c931267408acd04bcb8c8aec558d106ab40e942c70ce8e214af68529faa580b9243bd000322a5c300"+ + "7474682d6872656265636e61", + "zh_direct_2b", hrebecnaPK, "tth-hrebecna", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + info := store.GetNodeHashSizeInfo() + + ni, ok := info[hrebecnaPK] + if !ok { + t.Fatal("node with a 0x40 zero-hop advert missing from hash size info — the declared size was dropped") + } + if ni.HashSize != 2 { + t.Errorf("want HashSize=2 from path byte 0x40, got %d", ni.HashSize) + } + if ni.Inconsistent { + t.Error("single advert must not be flagged inconsistent") + } +} + +// TestZeroHopDirectAdvertThreeByteDeclaration: same rule at 3 bytes (0x80). +// Fixture: TTH-L1, 2026-08-24T15:36:45Z. +func TestZeroHopDirectAdvertThreeByteDeclaration(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const pk = "e11e55684170982339efe2266a423c9040f909339646ee2adeea19c21ed2b02a" + insertAdvert(t, db, 1, + "1280"+pk+"0c658c6afcf586f0935c87b563dc0770f1ccb558126d16e4805eb4c556d511bf"+ + "c1f5d832a760bbe2f08d73b62c068c1a269425faadb2f575a69bcfc53ebae0cb65b71d0981"+ + "5454482d4c31", + "zh_direct_3b", pk, "TTH-L1", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + info := store.GetNodeHashSizeInfo() + + ni, ok := info[pk] + if !ok { + t.Fatal("node with a 0x80 zero-hop advert missing from hash size info") + } + if ni.HashSize != 3 { + t.Errorf("want HashSize=3 from path byte 0x80, got %d", ni.HashSize) + } +} + +// TestZeroHopDirectAdvertAllZeroByteStillSkipped: 0x00 carries no information — +// the sender wiped the size bits — so the node must stay absent rather than be +// recorded as 1 byte. This is the behaviour #649 asked for and it must survive. +func TestZeroHopDirectAdvertAllZeroByteStillSkipped(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const pk = "aaaa000000000000000000000000000000000000000000000000000000000001" + insertAdvert(t, db, 1, "1200"+pk+"cafe", "zh_direct_zero", pk, "Legacy-Node", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + + if ni, ok := store.GetNodeHashSizeInfo()[pk]; ok { + t.Errorf("node seen only via a 0x00 zero-hop advert must have no hash size evidence, got %d", ni.HashSize) + } +} + +// TestZeroHopTransportDirectDeclaredSize: route type 3 puts the path byte at +// offset 5, behind four transport code bytes. The same content rule applies. +func TestZeroHopTransportDirectDeclaredSize(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const declared = "aaaa000000000000000000000000000000000000000000000000000000000002" + const wiped = "aaaa000000000000000000000000000000000000000000000000000000000003" + + // header 0x13 = payload_type 4 (ADVERT), route type 3; 4 transport bytes; path byte 0x40. + insertAdvert(t, db, 1, "1301020304"+"40"+declared+"cafe", "zh_td_2b", declared, "TD-Declared", 3, recent, epoch) + // Same shape, path byte 0x00 → no evidence. + insertAdvert(t, db, 2, "1305060708"+"00"+wiped+"cafe", "zh_td_zero", wiped, "TD-Wiped", 3, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + info := store.GetNodeHashSizeInfo() + + if ni, ok := info[declared]; !ok { + t.Error("transport-direct node with 0x40 at offset 5 missing from hash size info") + } else if ni.HashSize != 2 { + t.Errorf("transport-direct: want HashSize=2, got %d", ni.HashSize) + } + if _, ok := info[wiped]; ok { + t.Error("transport-direct node with an all-zero path byte must have no evidence") + } +} + +// TestZeroHopDeclaredSizeReachesMultiByteCapability: the capability view derives +// "confirmed" from advert evidence, so a node heard only via zero-hop adverts +// should now be confirmed rather than left unknown. This is the user-visible +// half — it is what the map's multi-byte overlay reads. +func TestZeroHopDeclaredSizeReachesMultiByteCapability(t *testing.T) { + db, recent, epoch := zeroHopStore(t) + defer db.Close() + + const pk = "5361aa08929b6248bb61cb1fab0fdb97d6dba8ae44f53a44427f63e7798a6b6b" + db.conn.Exec(`INSERT INTO nodes (public_key, name, role, last_seen, first_seen, advert_count) + VALUES (?, 'tth-hrebecna', 'repeater', ?, ?, 1)`, pk, recent, recent) + insertAdvert(t, db, 1, "1240"+pk+"cafe", "zh_cap", pk, "tth-hrebecna", 2, recent, epoch) + + store := NewPacketStore(db, nil) + store.Load() + + var found *MultiByteCapEntry + for _, e := range store.computeMultiByteCapability(nil) { + if e.PublicKey == pk { + entry := e + found = &entry + break + } + } + if found == nil { + t.Fatal("node missing from multi-byte capability output") + } + if found.Status != "confirmed" { + t.Errorf("want status=confirmed from a declared zero-hop advert, got %q (evidence %q)", found.Status, found.Evidence) + } + if found.MaxHashSize != 2 { + t.Errorf("want MaxHashSize=2, got %d", found.MaxHashSize) + } +} diff --git a/config.example.json b/config.example.json index 327e32d91..8b33967c8 100644 --- a/config.example.json +++ b/config.example.json @@ -10,9 +10,10 @@ "retention": { "nodeDays": 7, "observerDays": 14, + "observerPurgeDays": 0, "packetDays": 30, "clientRxDays": 30, - "_comment": "nodeDays: nodes not seen in N days moved to inactive_nodes (default 7). observerDays: observers not sending data in N days are removed (-1 = keep forever, default 14). packetDays: transmissions older than N days are deleted (0 = disabled). clientRxDays: mobile client-RX coverage rows (client_receptions/client_observers) older than N days are deleted (0 = disabled) — bounds the opt-in coverage tables (#1727). NOTE (#1283): all retention fields are consumed by the INGESTOR process. The server is read-only and never prunes." + "_comment": "nodeDays: nodes not seen in N days moved to inactive_nodes (default 7). observerDays: observers not sending data in N days are removed (-1 = keep forever, default 14). observerPurgeDays: observers already marked inactive AND not seen in N days are hard-deleted, but only when no observation, observer_metrics or dropped_packets row still references them (0 = disabled, the default). Set it above both observerDays and packetDays — below those the reference guards keep every row anyway. packetDays: transmissions older than N days are deleted (0 = disabled). clientRxDays: mobile client-RX coverage rows (client_receptions/client_observers) older than N days are deleted (0 = disabled) — bounds the opt-in coverage tables (#1727). NOTE (#1283): all retention fields are consumed by the INGESTOR process. The server is read-only and never prunes." }, "db": { "vacuumOnStartup": false, @@ -35,6 +36,16 @@ "_comment_ingestorStats": "Ingestor publishes a 1-Hz stats snapshot consumed by the server's /api/perf/io and /api/perf/write-sources endpoints (#1120). Path is configured via the CORESCOPE_INGESTOR_STATS environment variable on the INGESTOR process. Default: /tmp/corescope-ingestor-stats.json. The writer uses O_NOFOLLOW + 0o600, so a pre-planted symlink in /tmp cannot be used to clobber an arbitrary file. SECURITY: in shared-tmp environments (multi-tenant hosts), point CORESCOPE_INGESTOR_STATS at a private directory like /var/lib/corescope/ingestor-stats.json that only the corescope user can write to.", "corsAllowedOrigins": [], "_comment_corsAllowedOrigins": "Cross-origin allowlist for embed scenarios (#1369) AND for /ws WebSocket upgrades (#1793 — CSWSH defense per OWASP WebSocket Security Cheat Sheet). Exact-match origins, e.g. [\"https://blog.example.com\", \"https://embed.example.com\"]. Same-origin requests (Origin host == request Host) and non-browser clients (no Origin header) are always allowed for /ws. When empty (default), no Access-Control-* headers are sent and browsers enforce same-origin; cross-origin /ws upgrades are rejected. When non-empty, only the listed origins receive CORS headers, and Access-Control-Allow-Methods is limited to GET, HEAD, OPTIONS (the cross-domain surface is read-only — same-origin admin writes are unaffected). Use [\"*\"] to allow any origin for CORS XHR (NOT recommended for write-capable deployments); note that \"*\" is deliberately NOT honored for /ws upgrades — list explicit origins to permit cross-origin WebSocket clients (OWASP guidance: avoid wildcards in CSWSH defense). Operators can override per-deployment with the CORS_ALLOWED_ORIGINS environment variable (comma-separated). No credentialed CORS is enabled. To embed the map or channels pages cross-domain, add the embedding origin here and use the URL pattern '/#/map?embed=1' or '/#/channels?embed=1' — embed mode hides the top-nav, bottom-nav, and side drawer for full-bleed iframe rendering.", + "webSocket": { + "maxConnsPerIP": 0, + "_comment_maxConnsPerIP": "#1794 - concurrent /ws connections allowed from one client address. 0 = off, and that is the shipped default ON PURPOSE: carrier-grade NAT puts thousands of unrelated mobile subscribers behind one public IPv4, so a low cap refuses real visitors while a scraper simply rents more addresses. Set it only if you know your audience is not behind shared NAT.", + "upgradesPerMinPerIP": 30, + "_comment_upgradesPerMinPerIP": "#1794 - handshakes per minute from one client address. On by default. Safe under CGNAT: a real client upgrades a handful of times per minute even while reconnecting, so 30 leaves ordinary traffic alone while flattening a scraper reconnect loop. Set 0 to disable.", + "trustedProxies": [], + "_comment_trustedProxies": "#1794 - addresses or CIDRs of YOUR reverse proxy. REQUIRED for the per-IP limits to do anything behind nginx/Caddy/Traefik/ingress: without it every visitor shares the proxy's address, so the limits are skipped and one warning is logged at startup. X-Forwarded-For is believed only from these addresses, because anyone else can forge it. Never list an address you do not control.", + "deny": [], + "_comment_deny": "#1794 - addresses or CIDRs refused at the /ws upgrade, before the handshake. Accepts both \"1.2.3.4\" and \"1.2.3.0/24\". Applies even behind an unconfigured proxy, since it is your explicit instruction. Rejections are counted in /api/stats under websocket.rejectedDeny." + }, "https": { "cert": "/path/to/cert.pem", "key": "/path/to/key.pem", @@ -89,10 +100,18 @@ "provider": "", "token": "" }, + "_comment_opentopomap": "OpenTopoMap topographical tile provider. Limited to zoom level 17.", + "opentopomap": { + "enabled": false + }, "_comment_stamen": "Stamen (hosted by Stadia). WARNING: Tokens are sent to the browser. Apply origin/referrer restrictions.", "stamen": { "enabled": false, "token": "" + }, + "_comment_usgs": "USGS topographical tile provider. Limited to zoom level 16.", + "usgs": { + "enabled": false } } } diff --git a/public/analytics.js b/public/analytics.js index 846ca04bb..4139f49ed 100644 --- a/public/analytics.js +++ b/public/analytics.js @@ -246,7 +246,20 @@ } // Re-render when distance unit or theme changes - _themeRefreshHandler = function () { renderTab(_currentTab); }; + _themeRefreshHandler = function () { + // #1925: never full-rebuild the neighbor-graph tab on theme-refresh. + // Every page load fires one theme-refresh ~300ms after + // /api/config/theme resolves (app.js dispatches 'theme-changed', then + // debounces 300ms). renderTab() here replaced el.innerHTML, which reset + // the role checkboxes to their defaults and rebuilt _ngState from the + // full graph, discarding any filtering the user had applied in the + // meantime. Restarting the renderer keeps the current filter state and + // still picks up the new theme: node colors are read live per frame + // from window.ROLE_COLORS, role swatches use CSS tokens, and the one + // cached value (_labelColor) is re-read on restart. + if (_currentTab === 'neighbor-graph' && _ngState) { startGraphRenderer(); return; } + renderTab(_currentTab); + }; window.addEventListener('theme-refresh', _themeRefreshHandler); loadAnalytics(); @@ -1514,18 +1527,18 @@ async function renderCollisionTab(el, data, collisionData) { el.innerHTML = `

Collisions actually observed in packet traffic — among repeaters grouped by their configured hash size. For theoretical address conflicts that would occur if all repeaters used a given hash size, see the Prefix Tool tab.

-

Inconsistent Hash Sizes

↑ top
+

Inconsistent Hash Sizes

↑ top

Repeaters and room servers sending adverts with varying hash sizes in the last 7 days. Originally caused by a firmware bug where automatic adverts ignored the configured multibyte path setting, fixed in repeater v1.14.1. Companion nodes are excluded.

Loading…
@@ -1533,7 +1546,7 @@

Hash Usage Matrix

- ↑ top + ↑ top
@@ -1547,7 +1560,7 @@
-

Collision Risk

↑ top
+

Collision Risk

↑ top
Loading…
`; @@ -1584,9 +1597,15 @@ // Repeaters and routing nodes no longer needed — collision data is server-computed - let currentBytes = 1; function refreshHashViews(bytes) { - currentBytes = bytes; + // #1914: keep both the bookmark and section links on this byte size. + if (window.URLState) { + const newHash = URLState.updateHashParams({ bytes }, location.hash); + if (newHash !== location.hash) history.replaceState(null, '', newHash); + el.querySelectorAll('[data-hash-section]').forEach(link => { + link.href = URLState.updateHashParams({ section: link.dataset.hashSection }, newHash); + }); + } hideMatrixTip(); // Update selector button states document.querySelectorAll('.hash-byte-btn').forEach(b => { @@ -1615,7 +1634,9 @@ btn.addEventListener('click', () => refreshHashViews(Number(btn.dataset.bytes))); }); - refreshHashViews(1); + // Read on every render so tab, filter and theme refreshes retain the view. + const urlBytes = new URLSearchParams(location.hash.split('?')[1] || '').get('bytes'); + refreshHashViews(['1', '2', '3'].includes(urlBytes) ? Number(urlBytes) : 1); } function renderHashTimeline(hourly) { diff --git a/public/app.js b/public/app.js index 52bab2ce8..0674d5d1e 100644 --- a/public/app.js +++ b/public/app.js @@ -1170,16 +1170,20 @@ function navigate() { closeNav(); // Backward-compat redirect: #/traces/ → #/tools/trace/ (issue #944). + // Uses replaceState (not location.hash =) so this redirect doesn't add its + // own history entry, otherwise the back button gets stuck bouncing off it. if (location.hash.startsWith('#/traces/')) { - location.hash = location.hash.replace('#/traces/', '#/tools/trace/'); + history.replaceState(null, '', location.hash.replace('#/traces/', '#/tools/trace/')); + navigate(); return; } // Backward-compat redirect: #/roles → #/analytics?tab=roles (issue #1085). // The Roles page was folded into the Analytics tab strip; old links and - // bookmarks must keep working. + // bookmarks must keep working. Uses replaceState for the same reason as above. if (location.hash === '#/roles' || location.hash.startsWith('#/roles?') || location.hash.startsWith('#/roles/')) { - location.hash = '#/analytics?tab=roles'; + history.replaceState(null, '', '#/analytics?tab=roles'); + navigate(); return; } diff --git a/public/index.html b/public/index.html index 73a0a9b6a..3e6e91dcb 100644 --- a/public/index.html +++ b/public/index.html @@ -13,7 +13,10 @@ - + diff --git a/public/live.js b/public/live.js index 266cf7342..536ecf80b 100644 --- a/public/live.js +++ b/public/live.js @@ -856,7 +856,13 @@ resolved_path: pkt.resolved_path, _ts: new Date(pkt.timestamp || pkt.created_at).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: raw, path: { hops } }, - snr: pkt.snr, rssi: pkt.rssi, observer: pkt.observer_name + snr: pkt.snr, rssi: pkt.rssi, observer: pkt.observer_name, + // #1898: the region filter matches on observer_id (packetMatchesRegion, + // line ~85). Without it every replayed packet has observer_id undefined, + // so the filter skips them all and drops the whole group. observer_iata + // is carried too so obsIataBadgeHtml does not have to fall back to the + // roster map for replayed packets. + observer_id: pkt.observer_id, observer_iata: pkt.observer_iata }; } diff --git a/public/map-tile-providers.js b/public/map-tile-providers.js index 6266b81f9..fca6d1d0a 100644 --- a/public/map-tile-providers.js +++ b/public/map-tile-providers.js @@ -49,8 +49,11 @@ 'positron-dark': { provider: 'carto', label: 'Carto Positron', url: function() { return _getCartoBase() + '/light_all/{z}/{x}/{y}{r}.png'; }, invertFilter: INVERT_CSS, type: 'dark', attribution: '© OpenStreetMap © CartoDB', maxZoom: 19 }, 'osm-standard': { provider: 'osm', label: 'OSM Standard', url: _getOsmUrl, invertFilter: null, type: 'light', attribution: '© OpenStreetMap contributors, Maps © Mapbox/Thunderforest/MapTiler', maxZoom: 18 }, 'osm-dark': { provider: 'osm', label: 'OSM Standard', url: _getOsmUrl, invertFilter: INVERT_CSS, type: 'dark', attribution: '© OpenStreetMap contributors, Maps © Mapbox/Thunderforest/MapTiler', maxZoom: 18 }, + 'opentopomap': { provider: 'opentopomap', label: 'OpenTopoMap', url: function() { return 'https://{s}.tile.opentopomap.org/{z}/{x}/{y}.png'; }, invertFilter: null, type: 'light', attribution: '© OpenStreetMap contributors, SRTM, © OpenTopoMap (CC-BY-SA)', maxZoom: 17 }, 'stamen-toner-lite': { provider: 'stamen', label: 'Stamen Toner Lite', url: _getStamenUrl, invertFilter: null, type: 'light', attribution: '© Stadia Maps © Stamen Design © OpenStreetMap', maxZoom: 20 }, 'stamen-toner-dark': { provider: 'stamen', label: 'Stamen Toner Lite', url: _getStamenUrl, invertFilter: INVERT_CSS, type: 'dark', attribution: '© Stadia Maps © Stamen Design © OpenStreetMap', maxZoom: 20 }, + 'usgs-topo': { provider: 'usgs', label: 'USGS Topographic', url: function() { return 'https://basemap.nationalmap.gov/arcgis/rest/services/USGSTopo/MapServer/tile/{z}/{y}/{x}'; }, invertFilter: null, type: 'light', attribution: 'U.S. Geological Survey, National Geospatial Program', maxZoom: 16 }, + 'usgs-imagery': { provider: 'usgs', label: 'USGS Imagery', url: function() { return 'https://basemap.nationalmap.gov/arcgis/rest/services/USGSImageryTopo/MapServer/tile/{z}/{y}/{x}'; }, invertFilter: null, type: 'light', attribution: 'U.S. Geological Survey, National Geospatial Program', maxZoom: 16 }, 'esri-darkgray-labels': { provider: 'esri', label: 'Esri Dark Gray Canvas', url: function() { return 'https://server.arcgisonline.com/ArcGIS/rest/services/Canvas/World_Dark_Gray_Base/MapServer/tile/{z}/{y}/{x}'; }, invertFilter: null, type: 'dark', attribution: 'Tiles © Esri', maxZoom: 19 } }; @@ -65,6 +68,8 @@ var HAS_CARTO = !_cfg || !_cfg.providers || !_cfg.providers.carto || _cfg.providers.carto.enabled !== false; var HAS_OSM = _cfg && _cfg.providers && _cfg.providers.osm && _cfg.providers.osm.enabled; + var HAS_OPENTOPOMAP = _cfg && _cfg.providers && _cfg.providers.opentopomap && _cfg.providers.opentopomap.enabled; + var HAS_USGS = _cfg && _cfg.providers && _cfg.providers.usgs && _cfg.providers.usgs.enabled; var HAS_STAMEN = _cfg && _cfg.providers && _cfg.providers.stamen && _cfg.providers.stamen.enabled && !!_cfg.providers.stamen.token; var HAS_ESRI = true; // Kept for backwards compatibility @@ -73,7 +78,9 @@ var style = BASE_STYLES[key]; if (style.provider === 'carto' && HAS_CARTO) REGISTRY[key] = style; if (style.provider === 'osm' && HAS_OSM) REGISTRY[key] = style; + if (style.provider === 'opentopomap' && HAS_OPENTOPOMAP) REGISTRY[key] = style; if (style.provider === 'stamen' && HAS_STAMEN) REGISTRY[key] = style; + if (style.provider === 'usgs' && HAS_USGS) REGISTRY[key] = style; if (style.provider === 'esri' && HAS_ESRI) REGISTRY[key] = style; } diff --git a/public/nodes.js b/public/nodes.js index db73c94b4..2b33921ae 100644 --- a/public/nodes.js +++ b/public/nodes.js @@ -786,7 +786,6 @@ First Seen${renderNodeTimestampHtml(n.first_seen)} Total Packets${stats.totalTransmissions || stats.totalPackets || n.advert_count || 0}${stats.totalObservations && stats.totalObservations !== (stats.totalTransmissions || stats.totalPackets) ? ' (seen ' + stats.totalObservations + '×)' : ''} Packets Today${stats.packetsToday || 0} - ${stats.avgSnr != null ? `Avg SNR${Number(stats.avgSnr).toFixed(1)} dB` : ''} ${stats.avgHops ? `Avg Hops${stats.avgHops}` : ''} ${hasLoc ? `Location${Number(n.lat).toFixed(5)}, ${Number(n.lon).toFixed(5)}` : ''} ${hasEstLoc ? `${hasLoc ? 'Neighbor Estimate' : 'Location'} (estimated)~${Number(n.estimated_lat).toFixed(5)}, ~${Number(n.estimated_lon).toFixed(5)} (from ${n.estimated_contributor_count} neighbor${n.estimated_contributor_count === 1 ? '' : 's'}${hasLoc ? ', ' + Number(n.estimated_distance_km).toFixed(1) + ' km from reported position' : ', no real GPS fix'})` : ''} @@ -1070,17 +1069,18 @@ } document.querySelector('#fullPathsSection h4').textContent = `Paths Through This Node (${pathData.totalPaths} unique, ${pathData.totalTransmissions} transmissions)`; const COLLAPSE_LIMIT = 10; + const currentPubkey = n.public_key.toLowerCase(); function renderPaths(paths) { return paths.map(p => { const chain = p.hops.map(h => { - const isThis = h.pubkey === n.public_key; + const isThis = h.pubkey && h.pubkey.toLowerCase() === currentPubkey; if (window.HopDisplay) { const entry = { name: h.name, pubkey: h.pubkey, ambiguous: h.ambiguous, conflicts: h.conflicts, totalGlobal: h.totalGlobal, totalRegional: h.totalRegional, globalFallback: h.globalFallback, unreliable: h.unreliable }; const html = HopDisplay.renderHop(h.prefix, entry); return isThis ? html.replace('class="', 'class="hop-current ') : html; } const name = escapeHtml(h.name || h.prefix); - const link = h.pubkey ? `${name}` : `${name}`; + const link = h.pubkey ? `${name}` : `${name}`; return link; }).join(' → '); return `
@@ -1838,7 +1838,6 @@
First Seen
${renderNodeTimestampHtml(n.first_seen)}
Total Packets
${totalPackets}
Packets Today
${stats.packetsToday || 0}
- ${stats.avgSnr != null ? `
Avg SNR
${Number(stats.avgSnr).toFixed(1)} dB
` : ''} ${stats.avgHops ? `
Avg Hops
${stats.avgHops}
` : ''} ${hasLoc ? `
Location
${Number(n.lat).toFixed(5)}, ${Number(n.lon).toFixed(5)}
` : ''} ${hasEstLoc ? `
${hasLoc ? 'Neighbor Estimate' : 'Location'} (estimated)
~${Number(n.estimated_lat).toFixed(5)}, ~${Number(n.estimated_lon).toFixed(5)} (from ${n.estimated_contributor_count} neighbor${n.estimated_contributor_count === 1 ? '' : 's'}${hasLoc ? ', ' + Number(n.estimated_distance_km).toFixed(1) + ' km from reported position' : ', no real GPS fix'})
` : ''} @@ -1990,12 +1989,13 @@ document.querySelector('#pathsSection h4').textContent = `Paths Through This Node (${pathData.totalPaths} unique path${pathData.totalPaths !== 1 ? 's' : ''}, ${pathData.totalTransmissions} transmissions)`; const COLLAPSE_LIMIT = 10; const showAll = pathData.paths.length <= COLLAPSE_LIMIT; + const currentPubkey = n.public_key.toLowerCase(); function renderPaths(paths) { return paths.map(p => { const chain = p.hops.map(h => { - const isThis = h.pubkey === n.public_key; + const isThis = h.pubkey && h.pubkey.toLowerCase() === currentPubkey; const name = escapeHtml(h.name || h.prefix); - const link = h.pubkey ? `${name}` : `${name}`; + const link = h.pubkey ? `${name}` : `${name}`; return link; }).join(' → '); return `
diff --git a/public/observers.js b/public/observers.js index ceeb270e1..54c9a90f1 100644 --- a/public/observers.js +++ b/public/observers.js @@ -538,7 +538,7 @@ window.preserveCompareSelection = function preserveCompareSelection(prevIds, tbo var content = window.SlideOver.open({ title: (o.name || o.id) }); var naiveChipHTML = window.ObserversNaiveChip.render(o); content.innerHTML = - (naiveChipHTML ? '
' + naiveChipHTML + ' Clock is naive — per-packet timing clamped to ingest time.
' : '') + + (naiveChipHTML ? '
' + naiveChipHTML + ' Clock is naive — per-packet timing clamped to ingest time. Clears itself 24h after the last skew event.
' : '') + '
' + '
Status
' + h.label + '
' + '
Region
' + (o.iata ? '' + o.iata + '' : '—') + '
' + diff --git a/public/packets.js b/public/packets.js index fd157240a..68c061a02 100644 --- a/public/packets.js +++ b/public/packets.js @@ -1067,6 +1067,92 @@ else _docColMenuCloseHandler = handler; } + // --- Locally-added channels in the channel filter --------------------- + // Channels the operator adds in their own browser (Channels page → "Add + // channel") live only in localStorage — the keys never leave the client + // (channel-decrypt.js). The server therefore cannot decrypt their traffic + // and files it under the synthetic channel hash "enc_", which + // /api/channels omits. Result: a channel you just added is invisible in + // the packets channel picker. + // + // Fix: derive the same "enc_" value client-side from the stored key + // (SHA-256(key)[0], exactly what the ingestor writes) and offer those + // channels as extra options. /api/packets?channel=enc_ already + // filters on that value server-side, so no backend change is needed. + + /** + * Read the browser's stored channel keys and map each to the server-side + * channel-hash value its packets are stored under. + * Cost: one SHA-256 per stored key (a handful), once per page init. + * @returns {Promise>} + */ + async function collectLocalChannels() { + const CD = window.ChannelDecrypt; + if (!CD || typeof CD.getStoredKeys !== 'function') return []; + let keys; + try { keys = CD.getStoredKeys() || {}; } catch (e) { return []; } + const out = []; + for (const name of Object.keys(keys)) { + const keyHex = keys[name]; + if (!keyHex || typeof keyHex !== 'string') continue; + let hashByte; + try { + const keyBytes = CD.hexToBytes(keyHex); + if (!keyBytes || keyBytes.length !== 16) continue; + hashByte = await CD.computeChannelHash(keyBytes); + } catch (e) { continue; } + if (typeof hashByte !== 'number') continue; + const label = (typeof CD.getLabel === 'function' && CD.getLabel(name)) || name; + out.push({ + // Uppercase 2-digit hex — matches cmd/ingestor/decoder.go ("%02X"). + value: 'enc_' + hashByte.toString(16).padStart(2, '0').toUpperCase(), + name: name, + label: label + }); + } + return out; + } + + /** + * Merge the server channel list with locally-added ones into the two + * option groups the picker renders. Pure — unit-tested. + * A local channel is dropped when the server already exposes it (same + * hash value, or same name because the server holds the key too). + * @returns {{server: Array<{value:string,label:string}>, local: Array<{value:string,label:string}>}} + */ + function buildChannelOptions(serverChannels, localChannels) { + const byLabel = (a, b) => { + const an = (a.label || '').toLowerCase(); + const bn = (b.label || '').toLowerCase(); + return an < bn ? -1 : an > bn ? 1 : 0; + }; + const server = []; + const seenValues = new Set(); + const seenNames = new Set(); + for (const ch of serverChannels || []) { + const value = (ch && (ch.hash || ch.name)) || ''; + if (!value || seenValues.has(value)) continue; + seenValues.add(value); + seenNames.add(String(ch.name || value).toLowerCase()); + server.push({ value: value, label: ch.name || value }); + } + const local = []; + for (const lc of localChannels || []) { + if (!lc || !lc.value) continue; + if (seenValues.has(lc.value)) continue; + if (seenNames.has(String(lc.name || '').toLowerCase())) continue; + seenValues.add(lc.value); + local.push({ value: lc.value, label: lc.label || lc.name }); + } + return { server: server.sort(byLabel), local: local.sort(byLabel) }; + } + + // Exported for test-packets-local-channels.js (no DOM required). + if (typeof window !== 'undefined') { + window._packetsBuildChannelOptionsForTest = buildChannelOptions; + window._packetsCollectLocalChannelsForTest = collectLocalChannels; + } + function renderTimestampCell(isoString) { if (typeof formatTimestampWithTooltip !== 'function' || typeof getTimestampMode !== 'function') { return escapeHtml(typeof timeAgo === 'function' ? timeAgo(isoString) : '—'); @@ -1851,31 +1937,37 @@ opt.selected = true; channelSel.appendChild(opt); } - api('/channels').then(data => { + Promise.all([ + api('/channels').catch(() => null), + collectLocalChannels() + ]).then(([data, localChannels]) => { const channels = (data && data.channels) || []; // Build options via DOM API: channel names are network-supplied // and must NOT be interpolated into innerHTML (XSS, #812). // Sort alphabetically (case-insensitive) for predictable picker order; // the API returns last-activity order which is unstable for a dropdown. - const sorted = channels.slice().sort((a, b) => { - const an = (a.name || a.hash || '').toLowerCase(); - const bn = (b.name || b.hash || '').toLowerCase(); - return an < bn ? -1 : an > bn ? 1 : 0; - }); + const groups = buildChannelOptions(channels, localChannels); channelSel.textContent = ''; const allOpt = document.createElement('option'); allOpt.value = ''; allOpt.textContent = 'All Channels'; channelSel.appendChild(allOpt); let matched = false; - for (const ch of sorted) { - const v = ch.hash || ch.name || ''; - if (!v) continue; + const addOption = (o, parent) => { const opt = document.createElement('option'); - opt.value = v; - opt.textContent = ch.name || v; - if (v === filters.channel) { opt.selected = true; matched = true; } - channelSel.appendChild(opt); + opt.value = o.value; + opt.textContent = o.label; + if (o.value === filters.channel) { opt.selected = true; matched = true; } + parent.appendChild(opt); + }; + for (const o of groups.server) addOption(o, channelSel); + // Browser-added channels the server can't see, grouped so it's + // obvious they come from keys stored in this browser only. + if (groups.local.length) { + const grp = document.createElement('optgroup'); + grp.label = 'My Channels (this browser)'; + for (const o of groups.local) addOption(o, grp); + channelSel.appendChild(grp); } // If current filter isn't in the list (encrypted hash, stale, or // race with cache), keep it as a selected option so the UI reflects state. @@ -2775,6 +2867,47 @@ }); } + // applyObserverFilter decides which already-loaded packets remain visible + // under the current observer filter. Extracted into its own function + // (rather than left inline in renderTableRows) specifically so tests can + // exercise the real production logic instead of a hand-copied + // reimplementation — see #1748 PR review (kent-beck): a test that only + // checks a copy of this logic doesn't fail if this function regresses. + // + // #1748: In grouped mode, the server already filters transmissions + // correctly (buildTransmissionWhere emits an EXISTS subquery over ALL + // observations of the transmission, not just the displayed one — see + // cmd/server/db.go). Each row's `observer_id` here is only the + // *representative* observer chosen for display (longest observed path), + // which may legitimately differ from the observer that satisfied the + // filter. Re-filtering client-side against that single representative — + // with `_children` still unpopulated at this point (only fetched lazily + // on row-expand or observer-sort-change) — hid every multi-observer + // transmission whose representative happened not to be one of the + // selected observers. In practice this meant a transmission only stayed + // visible when the filtered observer was also the one with the longest + // path (which is why the report described it as "works only for + // whichever observer logged it first" in dense meshes, where + // longest-path and earliest-seen correlate). The server-side EXISTS + // filter is authoritative for grouped rows, so no client-side + // re-filtering is needed or correct here. + // + // Flat/expanded mode (groupByHash === false) has no such + // representative-vs-actual mismatch — buildPacketWhere filters each + // observation row by its own exact observer_id — but we keep the + // defensive re-filter for that path since it costs nothing and guards + // against any future flat-mode server change. + function applyObserverFilter(displayPackets, filters, groupByHash, hashOnly) { + if (hashOnly || !filters.observer) return displayPackets; + if (groupByHash) return displayPackets; + const obsIds = new Set(filters.observer.split(',')); + return displayPackets.filter(p => { + if (obsIds.has(p.observer_id)) return true; + if (p._children) return p._children.some(c => obsIds.has(String(c.observer_id))); + return false; + }); + } + async function renderTableRows() { const tbody = document.getElementById('pktBody'); if (!tbody) return; @@ -2817,14 +2950,7 @@ const types = filters.type.split(',').map(Number); displayPackets = displayPackets.filter(p => types.includes(p.payload_type)); } - if (!hashOnly && filters.observer) { - const obsIds = new Set(filters.observer.split(',')); - displayPackets = displayPackets.filter(p => { - if (obsIds.has(p.observer_id)) return true; - if (p._children) return p._children.some(c => obsIds.has(String(c.observer_id))); - return false; - }); - } + displayPackets = applyObserverFilter(displayPackets, filters, groupByHash, hashOnly); // Packet Filter Language const pfCount = document.getElementById('packetFilterCount'); @@ -3503,7 +3629,11 @@ id: o.id, hash: pkt.hash, raw: o.raw_hex || pkt.raw_hex, _ts: new Date(o.timestamp).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: oDec, path: { hops: oPath } }, - snr: o.snr, rssi: o.rssi, observer: obsName(o.observer_id) + snr: o.snr, rssi: o.rssi, observer: obsName(o.observer_id), + // #1900: carry the id itself, not just the resolved name. The Live + // region filter matches on observer_id, so without it the replay + // silently renders nothing whenever a region is selected. + observer_id: o.observer_id, observer_iata: o.observer_iata }); } } else { @@ -3511,7 +3641,8 @@ id: pkt.id, hash: pkt.hash, raw: pkt.raw_hex, _ts: new Date(pkt.timestamp).getTime(), decoded: { header: { payloadTypeName: typeName }, payload: decoded, path: { hops: pathHops } }, - snr: pkt.snr, rssi: pkt.rssi, observer: obsName(pkt.observer_id) + snr: pkt.snr, rssi: pkt.rssi, observer: obsName(pkt.observer_id), + observer_id: pkt.observer_id, observer_iata: pkt.observer_iata }); } sessionStorage.setItem('replay-packet', JSON.stringify(replayPackets)); @@ -4074,6 +4205,7 @@ buildFlatRowHtml, _calcVisibleRange, buildPacketsParams, + applyObserverFilter, renderTableRows, _setPackets: function(p) { packets = p; }, _setFilter: function(k, v) { filters[k] = v; }, diff --git a/public/rx-coverage.js b/public/rx-coverage.js index b2e3824eb..a8f1b2c0b 100644 --- a/public/rx-coverage.js +++ b/public/rx-coverage.js @@ -26,7 +26,7 @@ function pageHtml() { return '
' + - '

🗺️ Mobile RX coverage

' + + '

Mobile RX coverage

' + '
Where roaming CoreScope-RX clients heard nodes. Colour = best signal per cell. Get the companion app →
' + '
' + dayBtn(1) + dayBtn(7) + dayBtn(14) + dayBtn(30) + '
' + '
strongmediumweakno signal
' + diff --git a/public/style.css b/public/style.css index 6642eb74d..4205b5331 100644 --- a/public/style.css +++ b/public/style.css @@ -3119,7 +3119,15 @@ button.ch-item.ch-item-encrypted .ch-badge { filter: grayscale(0.6); } .hop-unreliable-btn { background: none; border: none; color: var(--status-yellow, #f59e0b); font-size: 13px; cursor: help; vertical-align: middle; margin-left: 2px; padding: 0 2px; line-height: 1; } .hop-global-fallback { border-bottom: 1px dashed var(--status-red); } -.hop-current { font-weight: 700 !important; color: var(--link-color) !important; } +.hop-current { + display: inline-block; + padding: 0 4px; + /* Outline preserves the shared renderer's dashed ambiguity border. */ + outline: 1px solid var(--link-color); + border-radius: 4px; + font-weight: 700 !important; + color: var(--link-color) !important; +} /* Self-loop subpath rows */ .subpath-selfloop { opacity: 0.6; } diff --git a/qa/scripts/blacklist-test.sh b/qa/scripts/blacklist-test.sh index 94b5b4127..535f477f0 100755 --- a/qa/scripts/blacklist-test.sh +++ b/qa/scripts/blacklist-test.sh @@ -25,65 +25,24 @@ # ssh-failed → cannot reach/control target # restart-stuck → /api/stats not 200 within RESTART_WAIT_S # hide-failed → blacklisted pubkey still surfaced via API (§10.1 fail) -# retain-failed → blacklisted pubkey absent from DB (§10.2 fail) +# retain-failed → blacklisted pubkey absent from DB (§10.2 fail), or the +# §10.2 probe could not run at all — no sqlite3 on the target +# able to bind a parameter. The message names what is needed; +# there is no fallback to interpolated SQL. # teardown-failed→ post-test removal did not restore listing # # Exit code = number of failures (0 = pass). # PUBLIC repo: zero PII — no real pubkeys, IPs, or hostnames as defaults. +# +# Structure: helpers live at top level and the imperative body lives in main(), +# so test-blacklist-sql.sh can source this file and exercise individual helpers +# without running the suite. Same idiom as scripts/staging/disk-monitor.sh. set -uo pipefail -BASELINE_URL="${1:-}" -TARGET_URL="${2:-}" -if [[ -z "$BASELINE_URL" || -z "$TARGET_URL" ]]; then - echo "usage: $0 BASELINE_URL TARGET_URL (TEST_NODE_PUBKEY+TARGET_* via env)" >&2 - exit 2 -fi - -TEST_PUBKEY="${TEST_NODE_PUBKEY:-}" -TARGET_SSH_HOST="${TARGET_SSH_HOST:-}" -TARGET_SSH_KEY="${TARGET_SSH_KEY:-/root/.ssh/id_ed25519}" -TARGET_CONFIG_PATH="${TARGET_CONFIG_PATH:-}" -TARGET_CONTAINER="${TARGET_CONTAINER:-}" -TARGET_DB_PATH="${TARGET_DB_PATH:-}" -ADMIN_API_TOKEN="${ADMIN_API_TOKEN:-}" - -if [[ -z "$TEST_PUBKEY" || -z "$TARGET_SSH_HOST" || -z "$TARGET_CONFIG_PATH" || -z "$TARGET_CONTAINER" ]]; then - echo "error: TEST_NODE_PUBKEY, TARGET_SSH_HOST, TARGET_CONFIG_PATH, TARGET_CONTAINER are required" >&2 - exit 2 -fi - -# Hard input validation — these strings are interpolated into remote shell/SQL. -# Pubkey must be hex (MeshCore pubkeys are hex-encoded ed25519 prefixes). -if ! [[ "$TEST_PUBKEY" =~ ^[0-9a-fA-F]+$ ]]; then - echo "error: TEST_NODE_PUBKEY must be hex (got: redacted)" >&2 - exit 2 -fi -# Container name must match docker's allowed chars: [a-zA-Z0-9][a-zA-Z0-9_.-]* -if ! [[ "$TARGET_CONTAINER" =~ ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$ ]]; then - echo "error: TARGET_CONTAINER has illegal chars" >&2 - exit 2 -fi -# Config path must be an absolute, sane path (no spaces, quotes, $, ;, etc.). -if ! [[ "$TARGET_CONFIG_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then - echo "error: TARGET_CONFIG_PATH must be a sane absolute path" >&2 - exit 2 -fi -if [[ -n "$TARGET_DB_PATH" ]] && ! [[ "$TARGET_DB_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then - echo "error: TARGET_DB_PATH must be a sane absolute path" >&2 - exit 2 -fi - -CURL_TIMEOUT="${CURL_TIMEOUT:-60}" -RESTART_WAIT_S="${RESTART_WAIT_S:-120}" - -SSH_OPTS=(-i "$TARGET_SSH_KEY" -o StrictHostKeyChecking=accept-new -o ConnectTimeout=15 -o BatchMode=yes) +SSH_OPTS=() # populated by main() from TARGET_SSH_KEY ssh_t() { ssh "${SSH_OPTS[@]}" "$TARGET_SSH_HOST" "$@"; } -TMP=$(mktemp -d) -fails=0 -TEARDOWN_DONE=0 - # ----------------------------------------------------------------------------- # Teardown — MANDATORY in all exit paths. # ----------------------------------------------------------------------------- @@ -106,7 +65,6 @@ teardown() { rm -rf "$TMP" exit "$rc" } -trap teardown EXIT INT TERM # ----------------------------------------------------------------------------- # Helpers @@ -197,75 +155,237 @@ node_visible() { } # ----------------------------------------------------------------------------- -# §10.1 — hide +# §10.2 DB probe — bind the pubkey, do not interpolate it (issue #1977) # ----------------------------------------------------------------------------- -echo "=== §10.1 add $TEST_PUBKEY to nodeBlacklist ===" -if ! add_to_blacklist; then fails=$((fails+1)); exit "$fails"; fi -if ! restart_target; then fails=$((fails+1)); exit "$fails"; fi -if ! wait_for_stats; then fails=$((fails+1)); exit "$fails"; fi +# Batch flags, all in service of "the count is parseable and errors are visible": +# -bail stop at the first SQL error instead of running on +# -init /dev/null ignore the operator's ~/.sqliterc — a stray .mode there +# would make the count unparseable +# -noheader -list stdout is exactly the number, nothing else +SQLITE_ARGS=(-batch -bail -init /dev/null -noheader -list) +# Round-trip probe token. The value is arbitrary; it only has to come back intact. +SQLITE_PROBE_TOKEN="corescope-probe-ok" +SQLITE_RUNNER="" # "container" | "host", set by resolve_sqlite_runner +RETAIN_COUNT="" # set by read_retain_count -detail_code=$(fetch_code "$TARGET_URL/api/nodes/$TEST_PUBKEY" "$TMP/detail.json") -list_code=$(fetch_code "$TARGET_URL/api/nodes?limit=10000" "$TMP/list.json") -in_list=0 -if [[ "$list_code" == "200" ]] && grep -qF -- "\"$TEST_PUBKEY\"" "$TMP/list.json"; then - in_list=1 -fi -if [[ "$detail_code" == "404" || "$in_list" == "0" ]]; then - echo " ✅ hide ok: detail=$detail_code in_list=$in_list" -else - echo " ❌ hide-failed: detail=$detail_code in_list=$in_list — pubkey still surfaced" - fails=$((fails+1)) -fi +# Hex-encode a value for embedding in SQL as a blob literal. +# +# Why hex rather than quoting: the output alphabet is [0-9a-f], so no byte the +# caller passes can terminate a string literal or add a dot-command argument. +# That holds for arbitrary input, which is the point — the SQL layer stops +# depending on main()'s hex gate in order to be safe. +# +# `od -v` is load-bearing: without it od collapses runs of identical lines to +# '*' and long repetitive values encode wrongly. +sql_hex_literal() { + printf "x'%s'" "$(printf '%s' "$1" | od -An -v -tx1 | tr -d ' \n')" +} -topo_code=$(fetch_code "$TARGET_URL/api/topology" "$TMP/topo.json") -if [[ "$topo_code" != "200" ]]; then - echo " ⚠️ /api/topology HTTP $topo_code — skipping topology assertion" -elif grep -qF -- "$TEST_PUBKEY" "$TMP/topo.json"; then - echo " ❌ hide-failed: /api/topology references blacklisted pubkey" - fails=$((fails+1)) -else - echo " ✅ topology clean" -fi +# SQL for the §10.2 count, fed to sqlite3 on stdin. The SELECT text is a +# constant; the pubkey arrives as a bound parameter. +# +# Note the nested cast rather than `.parameter set :pubkey ''`: +# dot-command arguments are split on whitespace, so a value containing a space +# (e.g. "' OR 1=1 --") makes sqlite3 print the .parameter help to STDOUT, exit +# 0, and leave :pubkey unbound. COUNT(*) then returns 0 — which reads exactly +# like a passing security fix. -bail does not catch it either. +transmission_count_sql() { + printf '.parameter init\n' + printf '.parameter set :pubkey "cast(%s as text)"\n' "$(sql_hex_literal "$1")" + printf 'SELECT COUNT(*) FROM transmissions WHERE from_node = :pubkey;\n' +} -# ----------------------------------------------------------------------------- -# §10.2 — DB retain -# ----------------------------------------------------------------------------- -echo "=== §10.2 verify packets retained in DB ===" -count="" -if [[ -n "$ADMIN_API_TOKEN" ]]; then - # Read auth header from stdin so the token never enters argv (ps-safe). - code=$(printf 'header = "Authorization: Bearer %s"\n' "$ADMIN_API_TOKEN" | \ - curl -s -m "$CURL_TIMEOUT" -K - -o "$TMP/admin.json" -w "%{http_code}" \ - "$TARGET_URL/api/admin/transmissions?from_node=$TEST_PUBKEY&count=1" 2>/dev/null || echo "000") - if [[ "$code" == "200" ]]; then - count=$(jq -r '.count // ((.transmissions // []) | length)' "$TMP/admin.json" 2>/dev/null || echo "") +# Capability probe: bind a known value and read it back. A version number only +# implies that .parameter works; binding something and getting it back proves it +# on the binary actually in front of us, which is the operator's, not ours. +sqlite_probe_sql() { + printf '.parameter init\n' + printf '.parameter set :probe "cast(%s as text)"\n' "$(sql_hex_literal "$SQLITE_PROBE_TOKEN")" + printf 'SELECT :probe;\n' +} + +# Find a sqlite3 that can bind a parameter — in the container first, then on the +# host. Sets SQLITE_RUNNER; returns 1 if neither qualifies. There is deliberately +# no interpolating fallback: that would leave the vulnerable path in place under +# a nicer name. +# +# Probe stderr is collected rather than discarded, but only printed if BOTH +# probes fail. The container miss is the known-normal case — the app image has +# no sqlite3 (pure-Go driver, no CGO; Dockerfile:15) — so surfacing it on every +# run would be noise. +resolve_sqlite_runner() { + local probe out + probe=$(sqlite_probe_sql) + SQLITE_RUNNER="" + out=$(ssh_t "docker exec -i $(printf %q "$TARGET_CONTAINER") sqlite3 ${SQLITE_ARGS[*]} :memory:" \ + <<<"$probe" 2>>"$TMP/sqlite-probe.err") + if [[ "$out" == "$SQLITE_PROBE_TOKEN" ]]; then SQLITE_RUNNER="container"; return 0; fi + out=$(ssh_t "sqlite3 ${SQLITE_ARGS[*]} :memory:" <<<"$probe" 2>>"$TMP/sqlite-probe.err") + if [[ "$out" == "$SQLITE_PROBE_TOKEN" ]]; then SQLITE_RUNNER="host"; return 0; fi + return 1 +} + +# Run SQL from stdin against TARGET_DB_PATH via the resolved runner. Stderr is +# left alone so the caller can capture it, and the exit status is sqlite3's. +# The SQL crosses on stdin, so only the container name and db path still need +# printf %q for the remote shell. docker exec needs -i to attach stdin. +run_sqlite() { + case "$SQLITE_RUNNER" in + container) ssh_t "docker exec -i $(printf %q "$TARGET_CONTAINER") sqlite3 ${SQLITE_ARGS[*]} $(printf %q "$TARGET_DB_PATH")" ;; + host) ssh_t "sqlite3 ${SQLITE_ARGS[*]} $(printf %q "$TARGET_DB_PATH")" ;; + *) echo "run_sqlite: no runner resolved" >&2; return 127 ;; + esac +} + +# Read the retained-transmission count into RETAIN_COUNT. Prints a classified +# "retain-failed" line and returns 1 on failure, so §10.2 has exactly one place +# that increments $fails. +read_retain_count() { + RETAIN_COUNT="" + local code + if [[ -n "$ADMIN_API_TOKEN" ]]; then + # Read auth header from stdin so the token never enters argv (ps-safe). + code=$(printf 'header = "Authorization: Bearer %s"\n' "$ADMIN_API_TOKEN" | \ + curl -s -m "$CURL_TIMEOUT" -K - -o "$TMP/admin.json" -w "%{http_code}" \ + "$TARGET_URL/api/admin/transmissions?from_node=$TEST_PUBKEY&count=1" 2>/dev/null || echo "000") + if [[ "$code" == "200" ]]; then + RETAIN_COUNT=$(jq -r '.count // ((.transmissions // []) | length)' "$TMP/admin.json" 2>/dev/null || echo "") + fi + if [[ -n "$RETAIN_COUNT" ]]; then return 0; fi fi -fi -if [[ -z "$count" ]]; then + if [[ -z "$TARGET_DB_PATH" ]]; then echo " ❌ retain-failed: TARGET_DB_PATH unset and no ADMIN_API_TOKEN — cannot probe" + return 1 + fi + if ! resolve_sqlite_runner; then + echo " ❌ retain-failed: no sqlite3 able to bind a parameter on the target" + echo " tried: docker exec -i $TARGET_CONTAINER sqlite3, then sqlite3 on $TARGET_SSH_HOST" + echo " need: the sqlite3 CLI reachable over ssh, supporting '.parameter set'" + cat "$TMP/sqlite-probe.err" >&2 + return 1 + fi + echo " sqlite3 runner: $SQLITE_RUNNER" + if ! RETAIN_COUNT=$(run_sqlite <<<"$(transmission_count_sql "$TEST_PUBKEY")" 2>"$TMP/sqlite.err"); then + echo " ❌ retain-failed: sqlite3 query failed via $SQLITE_RUNNER" + cat "$TMP/sqlite.err" >&2 + RETAIN_COUNT="" + return 1 + fi + return 0 +} + +# ----------------------------------------------------------------------------- +# main +# ----------------------------------------------------------------------------- +main() { + BASELINE_URL="${1:-}" + TARGET_URL="${2:-}" + if [[ -z "$BASELINE_URL" || -z "$TARGET_URL" ]]; then + echo "usage: $0 BASELINE_URL TARGET_URL (TEST_NODE_PUBKEY+TARGET_* via env)" >&2 + exit 2 + fi + + TEST_PUBKEY="${TEST_NODE_PUBKEY:-}" + TARGET_SSH_HOST="${TARGET_SSH_HOST:-}" + TARGET_SSH_KEY="${TARGET_SSH_KEY:-/root/.ssh/id_ed25519}" + TARGET_CONFIG_PATH="${TARGET_CONFIG_PATH:-}" + TARGET_CONTAINER="${TARGET_CONTAINER:-}" + TARGET_DB_PATH="${TARGET_DB_PATH:-}" + ADMIN_API_TOKEN="${ADMIN_API_TOKEN:-}" + + if [[ -z "$TEST_PUBKEY" || -z "$TARGET_SSH_HOST" || -z "$TARGET_CONFIG_PATH" || -z "$TARGET_CONTAINER" ]]; then + echo "error: TEST_NODE_PUBKEY, TARGET_SSH_HOST, TARGET_CONFIG_PATH, TARGET_CONTAINER are required" >&2 + exit 2 + fi + + # Hard input validation — these strings are interpolated into the remote shell. + # §10.2's SQL binds TEST_PUBKEY as a parameter rather than interpolating it, so + # for the SQL layer this gate is defence in depth rather than the only guard + # (issue #1977). Keep it: redundant is not the same as wrong. + # Pubkey must be hex (MeshCore pubkeys are hex-encoded ed25519 prefixes). + if ! [[ "$TEST_PUBKEY" =~ ^[0-9a-fA-F]+$ ]]; then + echo "error: TEST_NODE_PUBKEY must be hex (got: redacted)" >&2 + exit 2 + fi + # Container name must match docker's allowed chars: [a-zA-Z0-9][a-zA-Z0-9_.-]* + if ! [[ "$TARGET_CONTAINER" =~ ^[a-zA-Z0-9][a-zA-Z0-9_.-]*$ ]]; then + echo "error: TARGET_CONTAINER has illegal chars" >&2 + exit 2 + fi + # Config path must be an absolute, sane path (no spaces, quotes, $, ;, etc.). + if ! [[ "$TARGET_CONFIG_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then + echo "error: TARGET_CONFIG_PATH must be a sane absolute path" >&2 + exit 2 + fi + if [[ -n "$TARGET_DB_PATH" ]] && ! [[ "$TARGET_DB_PATH" =~ ^/[A-Za-z0-9_./-]+$ ]]; then + echo "error: TARGET_DB_PATH must be a sane absolute path" >&2 + exit 2 + fi + + CURL_TIMEOUT="${CURL_TIMEOUT:-60}" + RESTART_WAIT_S="${RESTART_WAIT_S:-120}" + + SSH_OPTS=(-i "$TARGET_SSH_KEY" -o StrictHostKeyChecking=accept-new -o ConnectTimeout=15 -o BatchMode=yes) + + TMP=$(mktemp -d) + fails=0 + TEARDOWN_DONE=0 + trap teardown EXIT INT TERM + + # --------------------------------------------------------------------------- + # §10.1 — hide + # --------------------------------------------------------------------------- + echo "=== §10.1 add $TEST_PUBKEY to nodeBlacklist ===" + if ! add_to_blacklist; then fails=$((fails+1)); exit "$fails"; fi + if ! restart_target; then fails=$((fails+1)); exit "$fails"; fi + if ! wait_for_stats; then fails=$((fails+1)); exit "$fails"; fi + + detail_code=$(fetch_code "$TARGET_URL/api/nodes/$TEST_PUBKEY" "$TMP/detail.json") + list_code=$(fetch_code "$TARGET_URL/api/nodes?limit=10000" "$TMP/list.json") + in_list=0 + if [[ "$list_code" == "200" ]] && grep -qF -- "\"$TEST_PUBKEY\"" "$TMP/list.json"; then + in_list=1 + fi + if [[ "$detail_code" == "404" || "$in_list" == "0" ]]; then + echo " ✅ hide ok: detail=$detail_code in_list=$in_list" + else + echo " ❌ hide-failed: detail=$detail_code in_list=$in_list — pubkey still surfaced" + fails=$((fails+1)) + fi + + topo_code=$(fetch_code "$TARGET_URL/api/topology" "$TMP/topo.json") + if [[ "$topo_code" != "200" ]]; then + echo " ⚠️ /api/topology HTTP $topo_code — skipping topology assertion" + elif grep -qF -- "$TEST_PUBKEY" "$TMP/topo.json"; then + echo " ❌ hide-failed: /api/topology references blacklisted pubkey" fails=$((fails+1)) else - # TEST_PUBKEY is hex-validated → safe to inline single-quoted in SQL. - # Container/db path also validated; printf %q for defense in depth. - q="SELECT COUNT(*) FROM transmissions WHERE from_node = '$TEST_PUBKEY';" - qq=$(printf %q "$q") - if ! count=$(ssh_t "docker exec $(printf %q "$TARGET_CONTAINER") sqlite3 $(printf %q "$TARGET_DB_PATH") $qq" 2>/dev/null); then - count=$(ssh_t "sqlite3 $(printf %q "$TARGET_DB_PATH") $qq" 2>/dev/null || echo "") - fi + echo " ✅ topology clean" fi -fi -if [[ -z "$count" ]]; then - echo " ❌ retain-failed: could not read transmissions count" - fails=$((fails+1)) -elif [[ "$count" =~ ^[0-9]+$ ]] && (( count > 0 )); then - echo " ✅ DB retains $count packets from $TEST_PUBKEY" -else - echo " ❌ retain-failed: count=$count (expected > 0)" - fails=$((fails+1)) -fi + # --------------------------------------------------------------------------- + # §10.2 — DB retain + # --------------------------------------------------------------------------- + echo "=== §10.2 verify packets retained in DB ===" + if ! read_retain_count; then + # read_retain_count already printed the classified reason. Counting here and + # nowhere else: the old code incremented $fails for the "TARGET_DB_PATH + # unset" case and then again for the empty count it left behind. + fails=$((fails+1)) + elif [[ "$RETAIN_COUNT" =~ ^[0-9]+$ ]] && (( RETAIN_COUNT > 0 )); then + echo " ✅ DB retains $RETAIN_COUNT packets from $TEST_PUBKEY" + else + echo " ❌ retain-failed: count=$RETAIN_COUNT (expected > 0)" + fails=$((fails+1)) + fi + + echo "=== summary: $fails failure(s) before teardown ===" + # trap handles teardown + exit + exit "$fails" +} -echo "=== summary: $fails failure(s) before teardown ===" -# trap handles teardown + exit -exit "$fails" +# Only run main when executed directly (not when sourced by tests). +if [ "${BASH_SOURCE[0]}" = "${0}" ]; then + main "$@" +fi diff --git a/qa/scripts/test-blacklist-sql.sh b/qa/scripts/test-blacklist-sql.sh new file mode 100755 index 000000000..770d6e065 --- /dev/null +++ b/qa/scripts/test-blacklist-sql.sh @@ -0,0 +1,135 @@ +#!/usr/bin/env bash +# test-blacklist-sql.sh — unit tests for the §10.2 SQL construction in +# qa/scripts/blacklist-test.sh (issue #1977). Sources the script and exercises +# its pure helpers, plus a real local sqlite3 against a throwaway fixture DB. +# +# Run: bash qa/scripts/test-blacklist-sql.sh +# Exits non-zero if any case fails. +# +# The point of the sqlite3 group is that BOTH directions are asserted. A test +# that only checks "the injection payload returns 0" passes just as happily when +# the query is silently broken and returns 0 for everything, so the legitimate +# pubkey must be shown to still return the row it should. + +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +# shellcheck source=blacklist-test.sh +. "$SCRIPT_DIR/blacklist-test.sh" + +PASS=0 +FAIL=0 + +assert_eq() { + local label="$1" expected="$2" actual="$3" + if [ "$expected" = "$actual" ]; then + PASS=$((PASS + 1)) + else + FAIL=$((FAIL + 1)) + echo "FAIL: $label — expected '$expected' got '$actual'" >&2 + fi +} + +assert_match() { + local label="$1" pattern="$2" actual="$3" + if [[ "$actual" =~ $pattern ]]; then + PASS=$((PASS + 1)) + else + FAIL=$((FAIL + 1)) + echo "FAIL: $label — '$actual' does not match /$pattern/" >&2 + fi +} + +# ----- sql_hex_literal ------------------------------------------------------ +# The security property: whatever goes in, the SQL text it produces is drawn +# from [0-9a-f] only. No caller-supplied byte can close a string literal or add +# a dot-command argument. Needs no sqlite3, so this group always runs. +assert_eq "hex of deadbeef" "x'6465616462656566'" "$(sql_hex_literal deadbeef)" +assert_eq "hex of empty" "x''" "$(sql_hex_literal "")" + +HEX_ONLY="^x'[0-9a-f]*'\$" +assert_match "alphabet: sql quote payload" "$HEX_ONLY" "$(sql_hex_literal "' OR 1=1 --")" +assert_match "alphabet: drop table" "$HEX_ONLY" "$(sql_hex_literal '"; DROP TABLE transmissions; --')" +assert_match "alphabet: backslash" "$HEX_ONLY" "$(sql_hex_literal 'a\b')" +assert_match "alphabet: dollar and backtick" "$HEX_ONLY" "$(sql_hex_literal '$(id) `id`')" +assert_match "alphabet: embedded newline" "$HEX_ONLY" "$(sql_hex_literal "$(printf 'a\nb')")" +assert_match "alphabet: multibyte" "$HEX_ONLY" "$(sql_hex_literal 'héllo')" + +# `od` without -v collapses runs of identical lines to '*'. A long repetitive +# value is the case that catches losing the flag. +LONG=$(printf 'x%.0s' $(seq 1 4096)) +LONG_HEX=$(sql_hex_literal "$LONG") +assert_match "alphabet: 4096 repeated bytes" "$HEX_ONLY" "$LONG_HEX" +# 4096 bytes → 8192 hex digits, plus the 3 chars of x''. A collapsed run would +# be far shorter and would also fail the alphabet check on '*'. +assert_eq "no od line-collapse in 4096-byte value" "8192" "$(( ${#LONG_HEX} - 3 ))" + +# ----- against a real sqlite3 ---------------------------------------------- +if ! command -v sqlite3 >/dev/null 2>&1; then + echo "SKIP: sqlite3 not on PATH — skipping the ${#SQLITE_ARGS[@]}-flag query group" >&2 + echo " (the alphabet assertions above still ran)" >&2 +else + FIXTURE_DIR=$(mktemp -d) + trap 'rm -rf "$FIXTURE_DIR"' EXIT + DB="$FIXTURE_DIR/fixture.db" + EMPTY_DB="$FIXTURE_DIR/no-table.db" + sqlite3 "$DB" \ + "CREATE TABLE transmissions(from_node TEXT); INSERT INTO transmissions VALUES('deadbeef'),('cafebabe');" + sqlite3 "$EMPTY_DB" "CREATE TABLE unrelated(x);" + + run_local() { sqlite3 "${SQLITE_ARGS[@]}" "$1"; } + + # The capability probe must round-trip on this machine, or the assertions + # below would be testing nothing. + assert_eq "probe round-trips" "$SQLITE_PROBE_TOKEN" "$(sqlite_probe_sql | run_local :memory:)" + + # POSITIVE CONTROL: a legitimate pubkey still returns its row. Without this, + # a silently broken query looks like a passing security fix. + out=$(transmission_count_sql deadbeef | run_local "$DB"); rc=$? + assert_eq "legit pubkey → its row" "1" "$out" + assert_eq "legit pubkey → exit 0" "0" "$rc" + assert_eq "other legit pubkey" "1" "$(transmission_count_sql cafebabe | run_local "$DB")" + assert_eq "absent pubkey → 0" "0" "$(transmission_count_sql abc123 | run_local "$DB")" + + # NEGATIVE: the payload binds as a literal that matches nothing. The table + # holds 2 rows, so a structural injection would return 2, not 0. + out=$(transmission_count_sql "' OR 1=1 --" | run_local "$DB"); rc=$? + assert_eq "injection payload → 0 rows" "0" "$out" + assert_eq "injection payload → exit 0" "0" "$rc" + assert_eq "table really does hold 2 rows" "2" \ + "$(run_local "$DB" <<<'SELECT COUNT(*) FROM transmissions;')" + + # Interpolating the same payload the old way returns the whole table. This is + # the behaviour the change removes; asserting it keeps the test honest about + # what "0" above is worth. + legacy="SELECT COUNT(*) FROM transmissions WHERE from_node = '' OR 1=1 --';" + assert_eq "old interpolated form leaked the table" "2" "$(run_local "$DB" <<<"$legacy")" + + # Multibyte and whitespace values bind as themselves rather than erroring. + sqlite3 "$DB" "INSERT INTO transmissions VALUES('héllo wörld');" + assert_eq "multibyte value with a space binds" "1" \ + "$(transmission_count_sql 'héllo wörld' | run_local "$DB")" + + # ERROR SURFACING: a broken query must be distinguishable from an empty + # result — non-zero exit and something on stderr, not a silent "". + err_file="$FIXTURE_DIR/err" + out=$(transmission_count_sql deadbeef | run_local "$EMPTY_DB" 2>"$err_file"); rc=$? + if [ "$rc" -ne 0 ]; then PASS=$((PASS + 1)); else + FAIL=$((FAIL + 1)); echo "FAIL: missing table — expected non-zero exit, got $rc" >&2 + fi + if [ -s "$err_file" ]; then PASS=$((PASS + 1)); else + FAIL=$((FAIL + 1)); echo "FAIL: missing table — expected a message on stderr" >&2 + fi + assert_eq "missing table → no count on stdout" "" "$out" + + # run_sqlite with no resolved runner must refuse rather than guess. + SQLITE_RUNNER="" + if run_sqlite /dev/null 2>&1; then + FAIL=$((FAIL + 1)); echo "FAIL: run_sqlite with no runner — expected non-zero exit" >&2 + else + PASS=$((PASS + 1)) + fi +fi + +echo "test-blacklist-sql.sh: $PASS passed, $FAIL failed" +[ "$FAIL" -eq 0 ] diff --git a/test-all.sh b/test-all.sh index b831175e7..3262dc089 100755 --- a/test-all.sh +++ b/test-all.sh @@ -11,6 +11,7 @@ echo "" echo "── Unit Tests ──" node test-packet-filter.js node test-packet-filter-ux.js +node test-packets-local-channels.js node test-aging.js node test-issue-1065-gesture-hints-gates.js node test-frontend-helpers.js @@ -50,6 +51,7 @@ node test-issue-1648-m2-emoji-scan.js node test-issue-1648-m3-emoji-scan.js node test-issue-1648-m6-final-sweep.js node test-issue-1648-m6-lint-self.js +node test-issue-1890-og-url.js node test-traces.js node test-live-multibyte-filter.js diff --git a/test-frontend-helpers.js b/test-frontend-helpers.js index 5dbd47e57..57373501d 100644 --- a/test-frontend-helpers.js +++ b/test-frontend-helpers.js @@ -3608,6 +3608,97 @@ console.log('\n=== packets.js: savedTimeWindowMin defaults ==='); }); } +// ===== Packets page: observer filter in grouped mode (issue #1748) ===== +{ + console.log('\n--- Observer filter — grouped vs. flat mode (#1748) ---'); + + // Load the REAL applyObserverFilter from packets.js via sandbox, rather + // than testing a hand-copied reimplementation of its logic. Per #1748 PR + // review (kent-beck): a test that only checks a copy doesn't fail if the + // actual production function regresses. Mirrors the same loadInCtx + // pattern used below for _calcVisibleRange. + const obsFilterCtx = makeSandbox(); + obsFilterCtx.registerPage = (name, handlers) => {}; + obsFilterCtx.onWS = () => {}; + obsFilterCtx.offWS = () => {}; + obsFilterCtx.api = () => Promise.resolve({}); + obsFilterCtx.window.getParsedPath = () => []; + obsFilterCtx.window.getParsedDecoded = () => ({}); + loadInCtx(obsFilterCtx, 'public/packets.js'); + const applyObserverFilter = obsFilterCtx.window._packetsTestAPI.applyObserverFilter; + + // A transmission the server correctly returned for observer "B" (it was + // one of several observers of this hash) but whose displayed + // "representative" observer is "A" (longest path) — exactly the shape + // QueryGroupedPackets returns. `_children` is undefined, matching the + // state on initial load (only fetched lazily on expand/obs-sort-change). + const groupedRowRepresentativeNotFiltered = { + hash: 'hash1', observer_id: 'A', observer_count: 3, _children: undefined, + }; + // A single-observer transmission whose only observer is NOT in the filter — + // must still be excluded (it genuinely wasn't observed by the filtered + // observer; the server wouldn't have returned it in the first place, but + // the client function must not accidentally let everything through). + const groupedRowGenuinelyExcludedByServer = { + hash: 'hash2', observer_id: 'C', observer_count: 1, _children: undefined, + }; + // A flat/expanded-mode row: single observation, own observer_id. + const flatRowMatching = { hash: 'hash3', observer_id: 'B' }; + const flatRowNonMatching = { hash: 'hash4', observer_id: 'A' }; + // A flat/expanded-mode row with already-loaded children, only one of + // which matches — the pre-existing children-aware fallback path. + const flatRowWithMatchingChild = { + hash: 'hash5', observer_id: 'A', + _children: [{ observer_id: 'A' }, { observer_id: 'B' }], + }; + + test('grouped mode: keeps a multi-observer row whose representative is not the filtered observer (#1748 core bug)', () => { + const result = applyObserverFilter([groupedRowRepresentativeNotFiltered], { observer: 'B' }, true, false); + assert.strictEqual(result.length, 1, 'server already guaranteed observer B saw this transmission'); + }); + + test('grouped mode: does not need _children to keep a valid server-filtered row', () => { + // The defining regression: _children is undefined (not yet fetched), + // and observer_id (the representative) does not match — pre-fix this + // returned zero rows. + const result = applyObserverFilter([groupedRowRepresentativeNotFiltered], { observer: 'B' }, true, false); + assert.strictEqual(result.length, 1); + }); + + test('grouped mode: passes through rows unfiltered (server EXISTS filter is authoritative)', () => { + // The client no longer second-guesses the server in grouped mode at all — + // this row would never have been returned by the server if observer C + // weren't a match, so the client trusts it. + const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer], { observer: 'B' }, true, false); + assert.strictEqual(result.length, 1, 'grouped mode does not re-filter; server already applied the correct filter'); + }); + + test('flat mode: keeps a row whose own observer_id matches', () => { + const result = applyObserverFilter([flatRowMatching], { observer: 'B' }, false, false); + assert.strictEqual(result.length, 1); + }); + + test('flat mode: excludes a row whose own observer_id does not match and has no children', () => { + const result = applyObserverFilter([flatRowNonMatching], { observer: 'B' }, false, false); + assert.strictEqual(result.length, 0); + }); + + test('flat mode: falls back to matching children when representative does not match', () => { + const result = applyObserverFilter([flatRowWithMatchingChild], { observer: 'B' }, false, false); + assert.strictEqual(result.length, 1, 'observer B is among the already-loaded children'); + }); + + test('hashOnly bypasses the observer filter entirely (pinned-hash view)', () => { + const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer, flatRowNonMatching], { observer: 'B' }, true, true); + assert.strictEqual(result.length, 2, 'hashOnly must return every row unfiltered, matching renderTableRows()'); + }); + + test('no observer filter set: all rows pass through unchanged', () => { + const result = applyObserverFilter([groupedRowGenuinelyExcludedByServer, flatRowNonMatching], {}, false, false); + assert.strictEqual(result.length, 2); + }); +} + // ===== Packets page: virtual scroll infrastructure ===== { console.log('\nPackets page — virtual scroll:'); diff --git a/test-issue-1146-path-link-contrast-e2e.js b/test-issue-1146-path-link-contrast-e2e.js index 77a465fa6..7c496611b 100644 --- a/test-issue-1146-path-link-contrast-e2e.js +++ b/test-issue-1146-path-link-contrast-e2e.js @@ -14,15 +14,21 @@ * theme, then asserts the computed link colour vs. its background * yields a contrast ratio ≥ 4.5:1. * - * Currently FAILS (link color resolves to rgb(0,0,238)). - * After the style.css fix it PASSES. + * #1153 also checks that the selected node has a visible enclosing marker + * in an 18-hop chain, without marking a different node with the same prefix. + * Covers the desktop side/full views and mobile full view in both themes. * * Usage: BASE_URL=http://localhost:13581 node test-issue-1146-path-link-contrast-e2e.js + * Optional SCREENSHOT_DIR saves PNG evidence; use an ignored output directory. */ 'use strict'; const { chromium } = require('playwright'); const BASE = process.env.BASE_URL || 'http://localhost:13581'; +assert(['localhost', '127.0.0.1', '[::1]'].includes(new URL(BASE).hostname), + 'Browser regression tests must use a local server'); +const SCREENSHOT_DIR = process.env.SCREENSHOT_DIR; +if (SCREENSHOT_DIR) require('fs').mkdirSync(SCREENSHOT_DIR, { recursive: true }); let passed = 0, failed = 0; async function step(name, fn) { @@ -83,20 +89,41 @@ async function effectiveBgFor(page, selector) { console.log(`\n=== #1146 path-link contrast E2E against ${BASE} ===`); - const hopPubkey = 'a1b2c3d4e5f60718293a4b5c6d7e8f9001122334455667788990aabbccddeeff'; + const hopPubkey = 'a1'.repeat(32); + const targetName = 'Selected & relay'; + let selectedPubkey; + let targetHopKey; + let siblingKey; // Mock paths API for ANY node so test is deterministic. await page.route('**/api/nodes/*/paths*', (route) => { - route.fulfill({ + selectedPubkey = decodeURIComponent(new URL(route.request().url()).pathname.split('/')[3]); + // Full public-key identity must survive hex casing; a shared prefix alone + // must never qualify. Neither fixture node requires resolver changes. + targetHopKey = selectedPubkey === selectedPubkey.toUpperCase() + ? selectedPubkey.toLowerCase() : selectedPubkey.toUpperCase(); + siblingKey = targetHopKey.slice(0, -1) + (targetHopKey.endsWith('0') ? '1' : '0'); + const hops = Array.from({ length: 18 }, (_, i) => ({ + pubkey: hopPubkey.slice(0, -2) + i.toString(16).padStart(2, '0'), + prefix: 'a1', name: 'Relay ' + (i + 1), + })); + hops[8] = { pubkey: targetHopKey, prefix: targetHopKey.slice(0, 2), name: targetName, unreliable: true, ambiguous: true }; + hops[9] = { + pubkey: siblingKey, prefix: targetHopKey.slice(0, 2), name: 'Same-prefix sibling', ambiguous: true, + conflicts: [ + { pubkey: targetHopKey, name: targetName, regional: true }, + { pubkey: siblingKey, name: 'Same-prefix sibling', regional: true }, + ], + }; + hops[10] = { prefix: targetHopKey.slice(0, 2), name: 'Unresolved prefix' }; + return route.fulfill({ status: 200, contentType: 'application/json', body: JSON.stringify({ totalPaths: 1, totalTransmissions: 5, paths: [{ - hops: [ - { pubkey: hopPubkey, prefix: 'a1', name: 'TestHop' }, - ], + hops, count: 5, lastSeen: new Date().toISOString(), sampleHash: 'deadbeef00', @@ -107,10 +134,10 @@ async function effectiveBgFor(page, selector) { await step('Load nodes page and force dark theme', async () => { await page.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); - await page.evaluate(() => { - document.documentElement.setAttribute('data-theme', 'dark'); - }); await page.waitForSelector('#nodesBody tr[data-key]', { timeout: 15000 }); + if (await page.locator('html').getAttribute('data-theme') !== 'dark') { + await page.locator('#darkModeToggle').click(); + } }); await step('Open side panel for first node and wait for paths', async () => { @@ -121,7 +148,7 @@ async function effectiveBgFor(page, selector) { const el = document.getElementById('pathsContent'); return el && el.querySelector('a[href^="#/nodes/"]'); }, - { timeout: 15000 } + null, { timeout: 15000 } ); }); @@ -145,6 +172,102 @@ async function effectiveBgFor(page, selector) { `covered by the .data-table td a rule and inherit UA blue.`); }); + async function assertCurrentHop(selector, hasHopDisplay) { + await page.waitForFunction((sel) => document.querySelector(sel)?.textContent.includes('Relay 18'), selector); + const targetSelector = selector + ' a[href="#/nodes/' + targetHopKey + '"]'; + const result = await page.evaluate(({ selector, targetHopKey, siblingKey }) => { + const container = document.querySelector(selector); + const chain = container.firstElementChild.firstElementChild; + const target = chain.querySelector('a[href="#/nodes/' + targetHopKey + '"]'); + const sibling = chain.querySelector('a[href="#/nodes/' + siblingKey + '"]'); + target.scrollIntoView({ block: 'center' }); + const style = getComputedStyle(target); + const rect = target.getBoundingClientRect(); + const chainRect = chain.getBoundingClientRect(); + const outline = parseFloat(style.outlineWidth) >= 1 && style.outlineStyle === 'solid'; + const border = ['Top', 'Right', 'Bottom', 'Left'].every(side => + parseFloat(style['border' + side + 'Width']) >= 1 && style['border' + side + 'Style'] === 'solid'); + return { + marked: Array.from(chain.querySelectorAll('.hop-current')).map(el => el.getAttribute('href')), + ring: outline || border, ringColor: outline ? style.outlineColor : style.borderTopColor, + outline, dashedBottom: style.borderBottomStyle === 'dashed' && parseFloat(style.borderBottomWidth) >= 1, + textColor: style.color, padding: parseFloat(style.paddingLeft), + text: target.textContent, injectedElements: target.childElementCount, + siblingMarked: sibling.classList.contains('hop-current'), + fits: rect.left >= chainRect.left - 1 && rect.right <= chainRect.right + 1, + fragments: target.getClientRects().length, + warning: !!chain.querySelector('.hop-unreliable-btn'), + conflict: !!chain.querySelector('.hop-conflict-btn'), + siblingAmbiguous: sibling.classList.contains('hop-ambiguous'), + hopCount: chain.children.length - chain.querySelectorAll('button').length, + }; + }, { selector, targetHopKey, siblingKey }); + assert(result.hopCount === 18, 'Expected all 18 hops in the rendered chain'); + assert(result.ring, 'Selected hop must have a visible enclosing border or outline, beyond bold/color'); + assert(result.marked.length === 1 && result.marked[0] === '#/nodes/' + targetHopKey, + 'Exactly the selected full public key must be marked, including mixed-case hex'); + assert(!result.siblingMarked, 'Same-prefix sibling must not be marked as this node'); + assert(result.padding > 0 && result.fits && result.fragments === 1, + 'Selected hop marker must enclose the name and fit within the wrapping chain'); + assert(result.text === targetName && result.injectedElements === 0, 'Selected hop name must stay escaped'); + const bg = parseRgb(await effectiveBgFor(page, targetSelector)); + assert(contrast(parseRgb(result.ringColor), bg) >= 3, 'Selected hop ring must contrast with the active theme'); + assert(contrast(parseRgb(result.textColor), bg) >= 4.5, 'Selected hop text must remain readable'); + if (hasHopDisplay) { + assert(result.warning && result.conflict && result.siblingAmbiguous, + 'Shared hop rendering must preserve unreliable and ambiguous indicators'); + assert(result.outline && result.dashedBottom, + 'The selected ambiguous hop must keep its dashed bottom border alongside the enclosing outline'); + } + if (SCREENSHOT_DIR) { + const theme = await page.locator('html').getAttribute('data-theme'); + const filename = `${selector.slice(1)}-${page.viewportSize().width}-${theme}${hasHopDisplay ? '' : '-plain'}.png`; + await page.locator(selector).screenshot({ path: require('path').join(SCREENSHOT_DIR, filename) }); + } + } + + await step('#1153 desktop side panel: selected hop stands out in an 18-hop dark chain', + () => assertCurrentHop('#pathsContent', false)); + + for (const theme of ['light', 'dark']) { + if (await page.locator('html').getAttribute('data-theme') !== theme) { + await page.locator('#darkModeToggle').click(); + } + await step('#1153 desktop side panel: ' + theme + ' theme', async () => { + await page.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); + assert(await page.locator('html').getAttribute('data-theme') === theme, 'Expected the selected theme'); + await page.locator('#nodesBody tr[data-key]').first().click(); + await assertCurrentHop('#pathsContent', false); + }); + await step('#1153 desktop full view: ' + theme + ' theme', async () => { + await page.locator('#nodesRight .node-detail-btn').click(); + await assertCurrentHop('#fullPathsContent', true); + }); + } + + for (const theme of ['light', 'dark']) { + // The theme toggle lives in the desktop header; exercise the same theme + // at mobile width without depending on the unrelated navigation drawer. + await page.setViewportSize({ width: 1280, height: 900 }); + if (await page.locator('html').getAttribute('data-theme') !== theme) { + await page.locator('#darkModeToggle').click(); + } + await page.setViewportSize({ width: 390, height: 844 }); + await step('#1153 mobile row click opens a readable full chain: ' + theme + ' theme', async () => { + await page.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); + assert(await page.locator('html').getAttribute('data-theme') === theme, 'Expected the selected theme'); + await page.locator('#nodesBody tr[data-key]').first().click(); + await assertCurrentHop('#fullPathsContent', true); + }); + } + + await step('#1153 full-view fallback also marks only the selected node', async () => { + await page.route('**/hop-display.js*', route => route.fulfill({ contentType: 'text/javascript', body: '' })); + await page.reload({ waitUntil: 'domcontentloaded' }); + await assertCurrentHop('#fullPathsContent', false); + assert(await page.evaluate(() => !window.HopDisplay), 'Expected the real full-view fallback'); + }); + await browser.close(); console.log(`\n${passed} passed, ${failed} failed`); diff --git a/test-issue-1281-location-row-e2e.js b/test-issue-1281-location-row-e2e.js index a92be9a4d..ffde5cc6f 100644 --- a/test-issue-1281-location-row-e2e.js +++ b/test-issue-1281-location-row-e2e.js @@ -14,6 +14,8 @@ * 3. The 📍map link uses class="loc-map-link" with a themed link color * (was --accent pre-M5; now --link-color after #1668/PR #1696 AA-contrast * fix). The hard guarantee is: NOT the default UA blue rgb(0,0,238). + * 4. #1149: node side/full stats omit aggregate SNR, retaining Heard By + * readings on desktop and mobile. Node API fixtures keep SNR non-null. * * Usage: BASE_URL=http://localhost:13581 node test-issue-1281-location-row-e2e.js */ @@ -157,6 +159,83 @@ async function findPacketDetailByType(page, predicate, maxRows = 40) { 'Link color is UA-default blue — class is missing or CSS rule does not match'); }); + const pubkey = 'ab'.repeat(32); + const now = new Date().toISOString(); + const node = { + public_key: pubkey, name: 'SNR fixture node', role: 'repeater', + first_seen: now, last_seen: now, advert_count: 1005, + }; + const health = { + stats: { totalPackets: 1005, packetsToday: 7, avgSnr: 6.3, avgHops: 2, lastHeard: now }, + observers: [ + { observer_id: 'observer-a', observer_name: 'Observer A', packetCount: 1000, avgSnr: 12.5, avgRssi: -45 }, + { observer_id: 'observer-b', observer_name: 'Observer B', packetCount: 5, avgSnr: -8.5, avgRssi: -110 }, + ], + recentPackets: [], + }; + + for (const view of [ + { name: 'desktop side panel', width: 1280, full: false }, + { name: 'desktop full detail', width: 1280, full: true }, + { name: 'mobile full detail', width: 390, full: true }, + ]) { + await step('#1149 ' + view.name + ' retains observer SNR without an aggregate headline', async () => { + const nodePage = await browser.newPage({ viewport: { width: view.width, height: 900 } }); + try { + await nodePage.route('**/api/nodes**', async (route) => { + const pathname = new URL(route.request().url()).pathname; + const fixtures = { + '/api/nodes': { nodes: [node], total: 1, counts: { repeaters: 1 } }, + '/api/nodes/clock-skew': [], + ['/api/nodes/' + pubkey]: { node, recentAdverts: [] }, + ['/api/nodes/' + pubkey + '/health']: health, + ['/api/nodes/' + pubkey + '/neighbors']: { neighbors: [] }, + ['/api/nodes/' + pubkey + '/paths']: { paths: [] }, + ['/api/nodes/' + pubkey + '/clock-skew']: {}, + }; + if (Object.prototype.hasOwnProperty.call(fixtures, pathname)) { + await route.fulfill({ json: fixtures[pathname] }); + } else { + await route.continue(); + } + }); + await nodePage.goto(BASE + '/#/nodes', { waitUntil: 'domcontentloaded' }); + await nodePage.locator('#nodesBody tr[data-key="' + pubkey + '"]').click(); + if (view.full && view.width > 640) { + await nodePage.locator('#nodesRight .node-detail-btn').click(); + } + + const stats = nodePage.locator(view.full ? '#node-stats' : '#nodesRight .detail-meta'); + const readings = nodePage.locator(view.full ? '#node-observers tbody tr' : '#nodesRight .observer-row'); + await readings.first().waitFor({ state: 'visible' }); + assert(await readings.count() === 2, 'Heard By must retain both observers'); + for (const observer of health.observers) { + const row = readings.filter({ hasText: observer.observer_name }); + const text = (await row.innerText()).replace(/\s+/g, ' '); + const snr = observer.avgSnr.toFixed(1) + (view.full ? ' dB' : 'dB'); + assert(text.includes(snr), observer.observer_name + ' must retain SNR ' + snr + ', got: ' + text); + } + for (const [label, value] of [['Total Packets', '1005'], ['Packets Today', '7'], ['Avg Hops', '2']]) { + const metric = stats.getByText(label, { exact: true }); + const actual = await metric.evaluate(el => el.nextElementSibling.textContent.trim()); + assert(actual === value, label + ' must remain ' + value + ', got: ' + actual); + } + const headlineCount = await stats.getByText('Avg SNR', { exact: true }).count(); + assert(headlineCount === 0, + 'Expected no aggregate Avg SNR headline with stats.avgSnr=6.3, got ' + headlineCount); + if (process.env.SCREENSHOT_DIR) { + await stats.scrollIntoViewIfNeeded(); + await nodePage.screenshot({ + path: require('path').join(process.env.SCREENSHOT_DIR, 'issue-1149-' + view.name.replace(/ /g, '-') + '.png'), + fullPage: true, + }); + } + } finally { + await nodePage.close(); + } + }); + } + await browser.close(); console.log(`\n${passed} passed, ${failed} failed`); diff --git a/test-issue-1306-collisions-terminology-e2e.js b/test-issue-1306-collisions-terminology-e2e.js index 1905b25f3..688ce2aca 100644 --- a/test-issue-1306-collisions-terminology-e2e.js +++ b/test-issue-1306-collisions-terminology-e2e.js @@ -15,6 +15,8 @@ * 3. The Collisions (Hash Issues) tab body contains the reverse * cross-reference link back to `#/analytics?tab=prefix-tool` * framed around "actually observed" / packet traffic wording. + * 4. #1914: shared Hash Issues links restore the selected byte size and + * its server data; selection survives navigation and refreshes. * * Usage: BASE_URL=http://localhost:13581 node test-issue-1306-collisions-terminology-e2e.js */ @@ -32,6 +34,7 @@ function assert(c, m) { if (!c) throw new Error(m || 'assertion failed'); } async function openPrefixTool(page) { await page.goto(BASE + '/#/analytics?tab=prefix-tool', { waitUntil: 'domcontentloaded' }); + await page.waitForFunction(() => window.__collisionsThemeReady); await page.waitForSelector('#ptOverview', { timeout: 15000 }); // expand the overview body (collapsed by default) await page.evaluate(() => { @@ -40,9 +43,45 @@ async function openPrefixTool(page) { }); } -async function openCollisions(page) { - await page.goto(BASE + '/#/analytics?tab=collisions', { waitUntil: 'domcontentloaded' }); +async function openCollisions(page, query = '') { + await page.goto('about:blank'); + await page.goto(BASE + '/#/analytics?tab=collisions' + query, { waitUntil: 'domcontentloaded' }); await page.waitForSelector('#hashMatrixSection', { timeout: 15000 }); + await page.waitForFunction(() => document.querySelector('#hashMatrix .analytics-stat-value')); +} + +function hashParams(page) { + return new URLSearchParams(new URL(page.url()).hash.split('?')[1] || ''); +} + +async function refreshCollisions(page, action) { + const previous = await page.locator('#hashMatrix').elementHandle(); + await action(); + await page.waitForFunction(el => !el.isConnected, previous); + await previous.dispose(); +} + +async function assertHashView(page, bytes, collisionData) { + await page.waitForFunction(() => window.__collisionsThemeReady); + await page.waitForFunction(() => document.querySelector('#hashMatrix .analytics-stat-value')); + const selected = page.locator('#hashByteSelector .hash-byte-btn.active'); + assert(await selected.count() === 1, 'Exactly one byte-size button should be selected'); + const actual = await selected.getAttribute('data-bytes'); + assert(actual === String(bytes), `Expected ${bytes}-byte selection, got ${actual}`); + const riskTitle = await page.locator('#collisionRiskTitle').textContent(); + assert(riskTitle.includes(bytes + '-Byte Collision Risk'), 'Risk title does not match selected byte size'); + const usingCard = page.locator('#hashMatrix .analytics-stat-card').nth(1); + assert((await usingCard.textContent()).includes('Using ' + bytes + '-byte ID'), 'Matrix uses the wrong byte-size data'); + const expected = collisionData.by_size[String(bytes)]; + assert(!!expected, 'Fixture must include ' + bytes + '-byte collision data'); + const usingCount = Number((await usingCard.locator('.analytics-stat-value').textContent()).replace(/,/g, '')); + assert(usingCount === (expected.stats.using_this_size || 0), 'Matrix count differs from the selected server data'); + assert(await page.locator('#hashMatrix .hash-matrix-table').count() === (bytes === 3 ? 0 : 1), 'Expected a grid only for 1-byte and 2-byte views'); + const prefixes = await page.locator('#collisionList tbody tr > td:first-child').allTextContents(); + assert(JSON.stringify(prefixes) === JSON.stringify((expected.collisions || []).map(c => c.prefix)), 'Collision risk rows differ from the selected server data'); + if (!prefixes.length) { + assert((await page.locator('#collisionList').textContent()).includes('No ' + bytes + '-byte'), 'Empty collision result should name the selected byte size'); + } } (async () => { @@ -64,6 +103,11 @@ async function openCollisions(page) { } const ctx = await browser.newContext({ viewport: { width: 1400, height: 1200 } }); + // Initial theme loading rebuilds analytics once; wait for it before + // interacting with an expanded panel or asserting the restored view. + await ctx.addInitScript(() => { + window.addEventListener('theme-refresh', () => { window.__collisionsThemeReady = true; }, { once: true }); + }); const page = await ctx.newPage(); page.setDefaultTimeout(15000); page.on('pageerror', (e) => console.error('[pageerror]', e.message)); @@ -108,6 +152,122 @@ async function openCollisions(page) { assert(xref >= 1, 'Collisions tab missing cross-reference link to #/analytics?tab=prefix-tool'); }); + const collisionResponse = await page.request.get(BASE + '/api/analytics/hash-collisions'); + assert(collisionResponse.ok(), 'Local collision API must respond successfully'); + const collisionData = await collisionResponse.json(); + + for (const section of ['hashMatrixSection', 'collisionRiskSection']) { + for (const bytes of [1, 2, 3]) { + await step(`#1914: ${bytes}-byte ${section} deep link restores selection and data`, async () => { + await openCollisions(page, '§ion=' + section + '&bytes=' + bytes); + await assertHashView(page, bytes, collisionData); + assert(hashParams(page).get('section') === section, 'Deep link must retain the target section'); + }); + } + } + + await step('#1914: byte clicks update a reloadable URL without losing other parameters', async () => { + await openCollisions(page, '§ion=collisionRiskSection&window=7d&filter=kept'); + for (const bytes of [2, 3, 1]) { + await page.locator(`.hash-byte-btn[data-bytes="${bytes}"]`).click(); + const params = hashParams(page); + assert(params.get('bytes') === String(bytes), `Byte click should write bytes=${bytes}, got ${params.get('bytes')}`); + assert(params.get('section') === 'collisionRiskSection' && params.get('window') === '7d' && params.get('filter') === 'kept', 'Byte click must preserve other parameters'); + await page.reload({ waitUntil: 'domcontentloaded' }); + await assertHashView(page, bytes, collisionData); + } + }); + + await step('#1914: section and top links preserve byte size and other parameters', async () => { + await openCollisions(page, '&bytes=3&window=7d&filter=kept'); + for (const section of ['hashMatrixSection', 'collisionRiskSection', 'inconsistentHashSection']) { + const link = page.locator(`#hashIssuesToc a[href*="section=${section}"]`); + const params = new URLSearchParams((await link.getAttribute('href')).split('?')[1]); + assert(params.get('bytes') === '3' && params.get('window') === '7d' && params.get('filter') === 'kept', 'Section href must carry the selected view for copying or opening'); + await link.click(); + await assertHashView(page, 3, collisionData); + assert(hashParams(page).get('section') === section, 'Section click must update its URL target'); + } + await page.locator('#collisionRiskSection a', { hasText: 'top' }).click(); + await assertHashView(page, 3, collisionData); + assert(!hashParams(page).has('section') && hashParams(page).get('bytes') === '3', 'Top link should clear only the section'); + }); + + await step('#1914: time-window, tab and theme refreshes retain the chosen byte size', async () => { + await openCollisions(page); + await page.locator('.hash-byte-btn[data-bytes="2"]').click(); + await refreshCollisions(page, () => page.locator('#analyticsTimeWindow').selectOption('24h')); + await page.waitForFunction(() => new URLSearchParams(location.hash.split('?')[1]).get('window') === '24h'); + await assertHashView(page, 2, collisionData); + await page.locator('#analyticsTabs [data-tab="hashsizes"]').click(); + await page.locator('#analyticsTabs [data-tab="collisions"]').click(); + await assertHashView(page, 2, collisionData); + await page.evaluate(() => window.dispatchEvent(new Event('theme-refresh'))); + await assertHashView(page, 2, collisionData); + assert(hashParams(page).get('bytes') === '2' && hashParams(page).get('window') === '24h', 'Refreshes must preserve the view URL'); + }); + + for (const value of [null, '', '0', '4', '-1', '2.5', '2bytes', '02', 'NaN']) { + await step(`#1914: ${value === null ? 'missing' : JSON.stringify(value)} byte size safely defaults to 1`, async () => { + await openCollisions(page, value === null ? '' : '&bytes=' + encodeURIComponent(value)); + await assertHashView(page, 1, collisionData); + }); + } + + for (const filter of ['region', 'area']) { + await openCollisions(page, '&bytes=3'); + const container = page.locator(filter === 'region' ? '#analyticsRegionFilter' : '#analyticsAreaFilter'); + const option = container.locator(`[data-${filter}]:not([data-${filter}="__all__"])`).first(); + const config = await (await page.request.get(BASE + '/api/config/' + filter + 's')).json(); + if (filter === 'region' ? Object.keys(config).length < 2 : config.length === 0) { + console.log(` #1914 ${filter} refresh: SKIP (local fixture has no selectable ${filter} filter)`); + continue; + } + await step(`#1914: ${filter} filter refresh retains byte size and selects its server data`, async () => { + await option.waitFor({ state: 'attached' }); + const selected = await option.getAttribute('data-' + filter); + const dropdown = container.locator('.region-dropdown-trigger'); + if (await dropdown.count()) await dropdown.click(); + await refreshCollisions(page, () => option.click()); + const response = await page.request.get(BASE + '/api/analytics/hash-collisions?' + filter + '=' + encodeURIComponent(selected)); + assert(response.ok(), 'Filtered collision API must respond successfully'); + await assertHashView(page, 3, await response.json()); + assert(hashParams(page).get('bytes') === '3', 'Filter refresh must preserve the byte-size URL'); + if (await dropdown.count()) await dropdown.click(); + await refreshCollisions(page, () => container.locator(`[data-${filter}="__all__"]`).click()); + await assertHashView(page, 3, collisionData); + }); + } + + await step('#1914: nonempty risk rows follow deep-linked and clicked byte sizes', async () => { + const prefixesBySize = { 1: ['A1'], 2: ['B2C3', 'B2D4'], 3: ['D4E5F6'] }; + const routePattern = '**/api/analytics/hash-collisions*'; + await page.route(routePattern, async route => { + const response = await route.fetch(); + const json = await response.json(); + // Keep the real matrix data, but distinguish each risk list even + // when the local fixture has no observed collisions. + for (const [bytes, prefixes] of Object.entries(prefixesBySize)) { + json.by_size[bytes].collisions = prefixes.map(prefix => ({ + prefix, appearances: 1, with_coords: 0, classification: 'unknown', nodes: [] + })); + } + await route.fulfill({ response, json }); + }); + try { + await openCollisions(page, '§ion=collisionRiskSection&bytes=2'); + await page.waitForFunction(() => window.__collisionsThemeReady); + for (const bytes of [2, 3, 1]) { + if (bytes !== 2) await page.locator(`.hash-byte-btn[data-bytes="${bytes}"]`).click(); + const prefixes = await page.locator('#collisionList tbody tr > td:first-child').allTextContents(); + assert(JSON.stringify(prefixes) === JSON.stringify(prefixesBySize[bytes]), + `Expected ${bytes}-byte risk prefixes ${prefixesBySize[bytes]}, got ${prefixes}`); + } + } finally { + await page.unroute(routePattern); + } + }); + await browser.close(); console.log('\n' + passed + ' passed, ' + failed + ' failed'); diff --git a/test-issue-1758-ng-filter-rerenders-e2e.js b/test-issue-1758-ng-filter-rerenders-e2e.js index 1a75fdb43..f51e323ea 100644 --- a/test-issue-1758-ng-filter-rerenders-e2e.js +++ b/test-issue-1758-ng-filter-rerenders-e2e.js @@ -204,6 +204,34 @@ function buildFixture() { if (reFiltered.canvasDisplay === 'none') fail('second filter-down: expected #ngCanvas visible again'); console.log(' ✓ second filter-down: render restored across repeated cycles'); + // --- #1925: a theme-refresh must not discard the applied filter. + // Every page load fires one 'theme-refresh' about 300ms after + // /api/config/theme resolves. It used to call renderTab(), which replaced + // el.innerHTML (resetting the role checkboxes to their defaults) and + // rebuilt _ngState from the full graph, putting #ngSkipMsg back. On an + // idle machine the test finished ~90ms before that landed; under CI load + // it landed mid-test, which is both failure modes of #1925. + // Dispatching the event directly makes that deterministic, so this asserts + // the cause rather than waiting for the race to show up. + await page.evaluate(() => window.dispatchEvent(new CustomEvent('theme-refresh'))); + await page.waitForTimeout(500); + const afterTheme = await page.evaluate(() => { + const canvas = document.getElementById('ngCanvas'); + const cb = document.querySelector('#ngRoleChecks input[data-role="companion"]'); + return { + hasSkip: !!document.getElementById('ngSkipMsg'), + canvasDisplay: canvas ? getComputedStyle(canvas).display : '(no canvas)', + companionChecked: cb ? cb.checked : '(no checkbox)', + }; + }); + if (afterTheme.companionChecked !== false) { + fail('theme-refresh reset the role filter (companion re-checked): ' + + JSON.stringify(afterTheme)); + } + if (afterTheme.hasSkip) fail('theme-refresh brought #ngSkipMsg back: ' + JSON.stringify(afterTheme)); + if (afterTheme.canvasDisplay === 'none') fail('theme-refresh hid #ngCanvas again'); + console.log(' ✓ theme-refresh preserves the applied filter (#1925)'); + await browser.close(); console.log('\nPASS: #1758 neighbor-graph filter re-render lifecycle holds'); diff --git a/test-issue-1890-og-url.js b/test-issue-1890-og-url.js new file mode 100644 index 000000000..8db009a2e --- /dev/null +++ b/test-issue-1890-og-url.js @@ -0,0 +1,61 @@ +#!/usr/bin/env node +/* Issue #1890 — index.html must not hardcode one instance's URL in its + * Open Graph tags. + * + * `` shipped to + * every self-hosted CoreScope. Facebook, Messenger and other OG consumers treat + * og:url as the canonical destination, so clicking a shared preview from ANY + * instance navigated to that one host instead of the instance the link came + * from. With no og:url present, consumers fall back to the URL they crawled, + * which is correct for every deployment without any configuration. + * + * og:image is deliberately NOT covered: it points at the project's own asset on + * raw.githubusercontent.com, which is a shared project resource, not a + * redirect target. + */ +'use strict'; + +const fs = require('fs'); +const path = require('path'); +const assert = require('assert'); + +const INDEX = path.resolve(__dirname, 'public', 'index.html'); +const html = fs.readFileSync(INDEX, 'utf8'); + +let passed = 0, failed = 0; +function test(name, fn) { + try { + fn(); + passed++; + console.log(` ✅ ${name}`); + } catch (e) { + failed++; + console.log(` ❌ ${name}: ${e.message}`); + } +} + +console.log('\n=== #1890: index.html carries no instance-specific canonical URL ==='); + +test('no og:url meta tag pinning a single instance', () => { + const m = html.match(/]*property=["']og:url["'][^>]*>/i); + assert.ok(!m, `og:url must not be hardcoded; found: ${m && m[0]}`); +}); + +test('no rel="canonical" pinning a single instance', () => { + const m = html.match(/]*rel=["']canonical["'][^>]*>/i); + assert.ok(!m, `canonical link must not be hardcoded; found: ${m && m[0]}`); +}); + +test('the analyzer.00id.net host appears nowhere in index.html', () => { + assert.ok(!/00id\.net/i.test(html), 'index.html still references 00id.net'); +}); + +test('og:title and og:description are still present', () => { + // The fix removes one tag, not the embed. Guard against over-deletion. + assert.ok(/property=["']og:title["']/i.test(html), 'og:title missing'); + assert.ok(/property=["']og:description["']/i.test(html), 'og:description missing'); + assert.ok(/property=["']og:image["']/i.test(html), 'og:image missing'); +}); + +console.log(`\n #1890 og:url: ${passed} passed, ${failed} failed\n`); +process.exit(failed ? 1 : 0); diff --git a/test-live-region-filter.js b/test-live-region-filter.js index cf914727e..5a5126368 100644 --- a/test-live-region-filter.js +++ b/test-live-region-filter.js @@ -14,7 +14,13 @@ function test(name, fn) { function makeSandbox() { const ctx = { - window: { addEventListener: () => {}, dispatchEvent: () => {}, devicePixelRatio: 1 }, + window: { + addEventListener: () => {}, dispatchEvent: () => {}, devicePixelRatio: 1, + // live.js:14 captures these at load time from packet-helpers.js, which + // this sandbox does not load. They must exist before live.js is evaluated. + getParsedDecoded: (p) => { try { return p && p.decoded_json ? JSON.parse(p.decoded_json) : {}; } catch (e) { return {}; } }, + getParsedPath: (p) => { try { return p && p.path_json ? JSON.parse(p.path_json) : []; } catch (e) { return []; } }, + }, document: { readyState: 'complete', createElement: () => ({ style:{}, classList:{add(){},remove(){},contains(){return false;}}, setAttribute(){}, addEventListener(){}, getContext: () => ({clearRect(){},fillRect(){},beginPath(){},arc(){},fill(){},scale(){},fillText(){}}) }), @@ -135,7 +141,42 @@ test('observer iata map can be updated and used by filter', () => { assert.strictEqual(fn([{observer_id:'newobs'}], { 'newobs': 'LAX' }, ['LAX']), true); }); +// --- #1898 / #1900: replayed packets must carry observer_id --- +// dbPacketToLive() used to return only observer (the resolved name), so every +// VCR-replayed packet had observer_id undefined. packetMatchesRegion skips a +// packet whose observer_id is null and returns false when none match, so with a +// region selected the replay rendered nothing at all, and the Replay button on +// packet detail silently no-opped. Both issues are the same omission. +const toLive = ctx.window._liveDbPacketToLive; +assert.ok(toLive, '_liveDbPacketToLive must be exposed'); + +test('#1898: dbPacketToLive carries observer_id through', () => { + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', + observer_id: 'obs1', observer_name: 'Obs One' }); + assert.strictEqual(live.observer_id, 'obs1', + 'observer_id must survive; the region filter matches on it'); +}); + +test('#1898: a replayed packet survives an active region filter', () => { + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', + observer_id: 'obs1', observer_name: 'Obs One' }); + assert.strictEqual(fn([live], { obs1: 'BRU' }, ['BRU']), true, + 'with observer_id carried the group matches and is rendered'); +}); + +test('#1898: dropping observer_id is what broke it (guards the regression)', () => { + assert.strictEqual(fn([{ observer: 'Obs One' }], { obs1: 'BRU' }, ['BRU']), false, + 'a packet with no observer_id is skipped, so the group is dropped'); +}); + +test('#1898: observer_iata is carried so the badge needs no roster lookup', () => { + const live = toLive({ id: 1, hash: 'h', timestamp: '2026-01-01T00:00:00Z', + observer_id: 'obs1', observer_iata: 'BRU' }); + assert.strictEqual(live.observer_iata, 'BRU'); +}); + console.log(`\n${'═'.repeat(40)}`); console.log(` live region filter tests: ${passed} passed, ${failed} failed`); + console.log(`${'═'.repeat(40)}\n`); if (failed > 0) process.exit(1); diff --git a/test-packets-local-channels.js b/test-packets-local-channels.js new file mode 100644 index 000000000..c73914a90 --- /dev/null +++ b/test-packets-local-channels.js @@ -0,0 +1,222 @@ +/** + * Unit tests for the packets channel-picker option merge. + * + * Locally-added channels (keys stored in the browser only) are invisible to + * the server, so /api/channels never lists them. packets.js derives their + * server-side "enc_" channel hash client-side and offers them as an + * extra option group. buildChannelOptions() does that merge; it is pure and + * exported as window._packetsBuildChannelOptionsForTest. + * + * Covers: sorting, server-list passthrough, local append, dedupe by hash + * value, dedupe by name (server holds the key too), and junk input. + */ +'use strict'; +const vm = require('vm'); +const fs = require('fs'); +const assert = require('assert'); + +// packets.js expects a browser at call time; the merge helper touches none +// of it. Load in a tolerant sandbox and grab the exported helper — it is +// assigned before any DOM-dependent code runs. +const noop = () => {}; +const fakeEl = { + addEventListener: noop, removeEventListener: noop, appendChild: noop, remove: noop, + querySelector: () => null, querySelectorAll: () => [], setAttribute: noop, + getAttribute: () => null, classList: { add: noop, remove: noop, toggle: noop, contains: () => false }, + style: {}, dataset: {}, textContent: '', innerHTML: '', +}; +const doc = { + readyState: 'complete', createElement: () => ({ ...fakeEl }), head: fakeEl, body: fakeEl, + getElementById: () => null, querySelector: () => null, querySelectorAll: () => [], + addEventListener: noop, removeEventListener: noop, +}; +const win = { + addEventListener: noop, removeEventListener: noop, + matchMedia: () => ({ matches: false, addEventListener: noop, removeEventListener: noop }), + // packets.js fails loud if this module is missing (#1799). + PayloadLabels: { SHORT_BY_ID: {} }, +}; +const ctx = { + window: win, document: doc, console, Date, Math, JSON, Set, Map, Array, Object, Promise, Error, + setTimeout, clearTimeout, setInterval, clearInterval, + history: { replaceState: noop, pushState: noop }, + location: { hash: '', href: '', pathname: '/' }, + navigator: { userAgent: 'node' }, + localStorage: { getItem: () => null, setItem: noop, removeItem: noop }, + registerPage: noop, + api: () => Promise.resolve({}), + fetch: () => Promise.resolve({ json: () => Promise.resolve({}) }), +}; +ctx.self = ctx; +vm.createContext(ctx); +try { + vm.runInContext(fs.readFileSync('public/packets.js', 'utf8'), ctx); +} catch (e) { + // Downstream init may throw on the stub DOM — fine, the helper is exported + // near the top of the module. +} + +const build = win._packetsBuildChannelOptionsForTest; +assert.strictEqual(typeof build, 'function', 'buildChannelOptions must be exported for tests'); + +// buildChannelOptions returns objects created inside the vm realm, whose +// prototypes differ from this realm's — deepStrictEqual would reject them on +// identity alone. Compare structurally. +const plain = (v) => JSON.parse(JSON.stringify(v)); + +let passed = 0; +function test(name, fn) { + fn(); + passed++; + console.log(' ok - ' + name); +} + +console.log('packets channel-picker option merge'); + +test('empty inputs produce empty groups', () => { + const r = build([], []); + assert.deepStrictEqual(plain(r), { server: [], local: [] }); + const r2 = build(null, undefined); + assert.deepStrictEqual(plain(r2), { server: [], local: [] }); +}); + +test('server channels are sorted case-insensitively by label', () => { + const r = build([{ hash: 'zeta', name: 'zeta' }, { hash: 'Alpha', name: 'Alpha' }, { hash: 'mid', name: 'mid' }], []); + assert.deepStrictEqual(plain(r.server.map(o => o.value)), ['Alpha', 'mid', 'zeta']); + assert.strictEqual(r.local.length, 0); +}); + +test('server channel without a name falls back to its hash', () => { + const r = build([{ hash: 'enc_A5' }], []); + assert.deepStrictEqual(plain(r.server), [{ value: 'enc_A5', label: 'enc_A5' }]); +}); + +test('local channels are appended as their own group', () => { + const r = build( + [{ hash: 'public', name: 'public' }], + [{ value: 'enc_A5', name: '#mytown', label: '#mytown' }] + ); + assert.deepStrictEqual(plain(r.server), [{ value: 'public', label: 'public' }]); + assert.deepStrictEqual(plain(r.local), [{ value: 'enc_A5', label: '#mytown' }]); +}); + +test('local channel is dropped when the server already lists that hash', () => { + const r = build( + [{ hash: 'enc_A5', name: 'enc_A5' }], + [{ value: 'enc_A5', name: '#mytown', label: '#mytown' }] + ); + assert.strictEqual(r.local.length, 0); +}); + +test('local channel is dropped when the server decrypts it under the same name', () => { + const r = build( + [{ hash: '#MyTown', name: '#MyTown' }], + [{ value: 'enc_A5', name: '#mytown', label: '#mytown' }] + ); + assert.strictEqual(r.local.length, 0, 'name match is case-insensitive'); +}); + +test('duplicate local entries collapse to one option', () => { + const r = build([], [ + { value: 'enc_A5', name: '#a', label: '#a' }, + { value: 'enc_A5', name: '#b', label: '#b' }, + ]); + assert.deepStrictEqual(plain(r.local), [{ value: 'enc_A5', label: '#a' }]); +}); + +test('local options are sorted and use their display label', () => { + const r = build([], [ + { value: 'enc_02', name: 'psk:deadbeef', label: 'Work PSK' }, + { value: 'enc_01', name: '#alpha', label: '#alpha' }, + ]); + assert.deepStrictEqual(plain(r.local), [ + { value: 'enc_01', label: '#alpha' }, + { value: 'enc_02', label: 'Work PSK' }, + ]); +}); + +test('valueless / junk entries are skipped', () => { + const r = build([{ name: '' }, null, {}], [null, { name: '#x' }, { value: '' }]); + assert.deepStrictEqual(plain(r), { server: [], local: [] }); +}); + +// --- collectLocalChannels(): stored keys → "enc_" ------------------- +// Stubs ChannelDecrypt with the real crypto (node) so the derived value is +// checked against a genuine SHA-256(key)[0], not a fixture. + +const crypto = require('crypto'); +const collect = win._packetsCollectLocalChannelsForTest; +assert.strictEqual(typeof collect, 'function', 'collectLocalChannels must be exported for tests'); + +function fakeChannelDecrypt(keys, labels) { + return { + getStoredKeys: () => keys, + getLabel: (name) => (labels && labels[name]) || '', + hexToBytes: (hex) => Uint8Array.from(Buffer.from(hex, 'hex')), + computeChannelHash: async (keyBytes) => crypto.createHash('sha256').update(keyBytes).digest()[0], + }; +} + +function deriveKeyHex(channelName) { + return crypto.createHash('sha256').update(channelName).digest().subarray(0, 16).toString('hex'); +} + +function expectedEncValue(channelName) { + const key = crypto.createHash('sha256').update(channelName).digest().subarray(0, 16); + const hashByte = crypto.createHash('sha256').update(key).digest()[0]; + return 'enc_' + hashByte.toString(16).padStart(2, '0').toUpperCase(); +} + +async function asyncTest(name, fn) { + await fn(); + passed++; + console.log(' ok - ' + name); +} + +(async () => { + console.log('\ncollectLocalChannels'); + + await asyncTest('no ChannelDecrypt module → no local channels', async () => { + win.ChannelDecrypt = undefined; + assert.deepStrictEqual(plain(await collect()), []); + }); + + await asyncTest('derives the server-side enc_ value from the stored key', async () => { + // '#alpha' and '#zulu' hash to different bytes; both must round-trip. + const keys = { '#alpha': deriveKeyHex('#alpha'), '#zulu': deriveKeyHex('#zulu') }; + win.ChannelDecrypt = fakeChannelDecrypt(keys); + const out = plain(await collect()); + assert.strictEqual(out.length, 2); + assert.deepStrictEqual(out[0], { value: expectedEncValue('#alpha'), name: '#alpha', label: '#alpha' }); + assert.deepStrictEqual(out[1], { value: expectedEncValue('#zulu'), name: '#zulu', label: '#zulu' }); + // Uppercase, zero-padded — matches cmd/ingestor/decoder.go's "%02X". + for (const o of out) assert.ok(/^enc_[0-9A-F]{2}$/.test(o.value), 'bad value ' + o.value); + }); + + await asyncTest('uses the stored display label when present', async () => { + win.ChannelDecrypt = fakeChannelDecrypt( + { 'psk:deadbeef': deriveKeyHex('#work') }, + { 'psk:deadbeef': 'Work PSK' } + ); + const out = plain(await collect()); + assert.deepStrictEqual(out, [{ value: expectedEncValue('#work'), name: 'psk:deadbeef', label: 'Work PSK' }]); + }); + + await asyncTest('skips malformed keys instead of throwing', async () => { + win.ChannelDecrypt = fakeChannelDecrypt({ + '#short': 'aabb', // not 16 bytes + '#empty': '', + '#null': null, + '#good': deriveKeyHex('#good'), + }); + const out = plain(await collect()); + assert.deepStrictEqual(out, [{ value: expectedEncValue('#good'), name: '#good', label: '#good' }]); + }); + + await asyncTest('a throwing getStoredKeys degrades to no local channels', async () => { + win.ChannelDecrypt = { getStoredKeys: () => { throw new Error('quota'); } }; + assert.deepStrictEqual(plain(await collect()), []); + }); + + console.log(`\n${passed} tests passed`); +})().catch((e) => { console.error(e); process.exit(1); });