From c0e6508359c61850f2dd0a6defcf7248b0491cb0 Mon Sep 17 00:00:00 2001 From: dborup Date: Tue, 6 Oct 2026 03:21:17 +0200 Subject: [PATCH] test(channels): close the #257 round-2 test gaps and document the hide trade-off Relates to #276 - hiddenChannels is on the public GET /api/channels, so a new test pins that it only ever names channels with stored traffic: an unreviewed suggestion is never published there (mutant G1, the EXISTS clause dropped, now fails). - A plain /api/channels request (no includeEncrypted) hands hideRevoked GetChannels' cached slice itself; a new test checks the cache is unchanged afterwards (mutant G2, compacting in place, now fails). - The node harness gains the per-path TTL cache the real api() has, so the invalidateApiCache('/channels') in refreshChannelList is covered on both the revoke and the approval path (mutant F3 now fails). - The "not approved", rather than "was approved before", rule is kept and documented: a pending suggestion can hide a name whose stored messages were decrypted through a config entry since removed. A test pins both that and the config-name exception. --- cmd/server/channel_revoked_hidden_test.go | 187 ++++++++++++++++++++++ cmd/server/openapi.go | 2 +- docs/api-spec.md | 20 ++- docs/user-guide/channels.md | 4 + test-channels-client-state-152.js | 90 ++++++++++- 5 files changed, 299 insertions(+), 4 deletions(-) diff --git a/cmd/server/channel_revoked_hidden_test.go b/cmd/server/channel_revoked_hidden_test.go index 36407622f..76e93cb97 100644 --- a/cmd/server/channel_revoked_hidden_test.go +++ b/cmd/server/channel_revoked_hidden_test.go @@ -526,3 +526,190 @@ func BenchmarkHideRevoked(b *testing.B) { } } } + +// --------------------------------------------------------------------------- +// #276: follow-ups to the round-2 review of #257. +// --------------------------------------------------------------------------- + +// addChannelTrafficAt stores one decoded message on a channel with an +// explicit timestamp, so the order GetChannels returns (last_activity DESC) +// is deterministic and a test can place a channel in the middle of the list. +func (ps *proposalServer) addChannelTrafficAt(name, firstSeen string) { + ps.t.Helper() + hash := fmt.Sprintf("%x", name) // unique per name + if _, err := ps.srv.db.conn.Exec(`INSERT INTO transmissions (raw_hex, hash, first_seen, route_type, payload_type, decoded_json, channel_hash) + VALUES ('EEFF', ?, ?, 1, 5, ?, ?)`, + hash, firstSeen, `{"type":"CHAN","channel":"`+name+`","text":"alice: hello","sender":"alice"}`, name); err != nil { + ps.t.Fatal(err) + } + ps.srv.db.channelsCache.reset() +} + +// hiddenNamesFromList returns the hiddenChannels field of GET /api/channels. +func (ps *proposalServer) hiddenNamesFromList(query string) []string { + ps.t.Helper() + w := ps.do("GET", "/api/channels"+query, "", "") + if w.Code != 200 { + ps.t.Fatalf("GET /api/channels%s = %d: %s", query, w.Code, w.Body) + } + return decode[ChannelListResponse](ps.t, w).HiddenChannels +} + +// cachedChannelNames returns the names in GetChannels' cached slice, in order. +func (ps *proposalServer) cachedChannelNames() []string { + ps.t.Helper() + cached, err := ps.srv.db.GetChannels("") + if err != nil { + ps.t.Fatal(err) + } + names := make([]string, len(cached)) + for i, ch := range cached { + names[i] = fmt.Sprint(ch["name"]) + } + return names +} + +// #276 (3), privacy-relevant: hiddenChannels is on the public GET +// /api/channels, while proposals themselves are only visible through the +// authenticated admin route. The EXISTS (… transmissions …) clause is what +// keeps a proposal that has no stored traffic out of the field: without it +// every pending suggestion — a name anyone can submit — would be published +// there. Nothing in the list changes either way, so only this assertion +// catches it (mutant G1: the EXISTS clause dropped). +func TestHiddenChannelsNeverNamesAProposalWithoutTraffic(t *testing.T) { + ps := newRevokeFixture(t) + // One real hidden channel, so hiddenChannels is populated at all. + ps.addChannelTraffic("#helloworld") + ps.insert("aaaaaaaaaaaaaaaa", "#helloworld", channelregistry.StatusRevoked) + // Proposals without stored traffic, in every not-approved status. The + // pending one is the privacy case: an unreviewed suggestion. + ps.insert("bbbbbbbbbbbbbbbb", "#pendingsecret", channelregistry.StatusPending) + ps.insert("cccccccccccccccc", "#rejectedsecret", channelregistry.StatusRejected) + ps.insert("dddddddddddddddd", "#revokednotraffic", channelregistry.StatusRevoked) + ps.expireApproved() + + got := ps.hiddenNamesFromList("") + if !reflect.DeepEqual(got, []string{"#helloworld"}) { + t.Fatalf("hiddenChannels = %v, want only the channel that has stored traffic ([#helloworld])", got) + } + for _, name := range []string{"#pendingsecret", "#rejectedsecret", "#revokednotraffic"} { + if containsName(got, name) { + t.Fatalf("%s has no stored traffic and must never be published in hiddenChannels: %v", name, got) + } + } + // Same on ?includeEncrypted=true, the other path into hideRevoked. + if got := ps.hiddenNamesFromList("?includeEncrypted=true"); !reflect.DeepEqual(got, []string{"#helloworld"}) { + t.Fatalf("hiddenChannels with includeEncrypted = %v", got) + } + // The names were never listed either, so hiding them would be pointless + // as well as public. + if listed := ps.listedNames(""); containsName(listed, "#pendingsecret") { + t.Fatalf("precondition: a proposal without traffic is not in the list anyway: %v", listed) + } +} + +// #276 (2): on the default request (no includeEncrypted) hideRevoked receives +// GetChannels' cached slice itself — handleChannels only copies on the +// includeEncrypted path, which is the only path +// TestRevokedFilterDoesNotMutateCacheAndKeepsEncrypted covers. Compacting in +// place there corrupts the shared cache for every later request (mutant G2: +// `out := resp.Channels`). +func TestPlainChannelListRequestDoesNotMutateTheCachedSlice(t *testing.T) { + ps := newRevokeFixture(t) + // Distinct timestamps: GetChannels orders by last_activity DESC, so the + // hidden channel sits in the middle and compacting in place would shift + // the channels after it over it. + ps.addChannelTrafficAt("#zulu", "2024-03-03T00:00:00Z") + ps.addChannelTrafficAt("#helloworld", "2024-02-02T00:00:00Z") + ps.addChannelTrafficAt("#alpha", "2024-01-01T00:00:00Z") + ps.insert("aaaaaaaaaaaaaaaa", "#helloworld", channelregistry.StatusRevoked) + ps.expireApproved() + + before := ps.cachedChannelNames() + idx := -1 + for i, n := range before { + if n == "#helloworld" { + idx = i + } + } + if idx < 0 || idx >= len(before)-1 { + t.Fatalf("precondition: the hidden channel must sit before the last cached entry so an in-place compaction is detectable, cache = %v", before) + } + + for i := 0; i < 3; i++ { + got := ps.listedNames("") + if containsName(got, "#helloworld") { + t.Fatalf("request %d: revoked channel listed: %v", i, got) + } + if !containsName(got, "#zulu") || !containsName(got, "#alpha") { + t.Fatalf("request %d: the other channels must stay listed: %v", i, got) + } + } + + after := ps.cachedChannelNames() + if !reflect.DeepEqual(before, after) { + t.Fatalf("the plain request altered GetChannels' cached slice:\nbefore %v\nafter %v", before, after) + } +} + +// #276 (1) / round-2 finding 1, semantics pinned deliberately: a name with +// stored traffic that the ingestor no longer decrypts through its config (an +// operator removed it from hashChannels/channelKeys, or it is past the +// 4096-name cap of builtin-channels.json) is hidden by an anonymous pending +// suggestion, before any administrator acted on it. +// +// This is kept, not fixed, and the trade-off is documented in +// docs/api-spec.md and docs/user-guide/channels.md. "Hide only names that +// were ever approved" needs a marker that survives a re-suggestion, and the +// obvious candidate does not exist: submitChannelProposal resurrects a +// revoked row with `reviewed_at = NULL` (that reset is what makes a +// resubmission idempotent), so a reviewed_at rule cannot tell a fresh +// suggestion from a re-suggestion after a revoke — it would re-open the hole +// #257 round 2 closed, where anyone could unhide a revoked channel by +// suggesting the name again. A new history column is an ingestor-side schema +// change, out of scope here, and cmd/server stays read-only. +// +// What bounds the trade-off: the effect is list-level only (the history stays +// readable), it needs stored traffic for that exact name, a name the ingestor +// still decrypts through its config is never hidden, the administrator sees +// the suggestion in the pending queue, and one Approve lists the channel +// again. +func TestAnonymousPendingSuggestionHidesAHistoricalConfigChannel(t *testing.T) { + ps := newRevokeFixture(t) // the ingestor publishes #chat as a config name + ps.addChannelTraffic("#oldcfg") + ps.addChannelTraffic("#chat") + ps.expireApproved() + if got := ps.listedNames(""); !containsName(got, "#oldcfg") || !containsName(got, "#chat") { + t.Fatalf("precondition: both channels are listed without any proposal: %v", got) + } + + // An anonymous suggestion the administrator has not reviewed: this is the + // row submitChannelProposal inserts for an unknown name with + // autoApprove off. + ps.insert("aaaaaaaaaaaaaaaa", "#oldcfg", channelregistry.StatusPending) + // The same for a name the ingestor still decrypts through its config. + ps.insert("bbbbbbbbbbbbbbbb", "#chat", channelregistry.StatusPending) + ps.expireApproved() + + got := ps.listedNames("") + if containsName(got, "#oldcfg") { + t.Fatalf("documented trade-off: a pending suggestion hides a name the ingestor no longer decrypts: %v", got) + } + if !containsName(got, "#chat") { + t.Fatalf("a config-decrypted name must never be hidden, whatever its proposal says: %v", got) + } + if hidden := ps.hiddenNamesFromList(""); !reflect.DeepEqual(hidden, []string{"#oldcfg"}) { + t.Fatalf("hiddenChannels = %v, want [#oldcfg]", hidden) + } + // The history is untouched and still readable by name. + w := ps.do("GET", "/api/channels/%23oldcfg/messages", "", "") + if w.Code != 200 || decode[ChannelMessagesResponse](t, w).Total != 1 { + t.Fatalf("the history of a hidden channel must stay readable: %d %s", w.Code, w.Body) + } + // One administrator action undoes it. + ps.setStatus("aaaaaaaaaaaaaaaa", channelregistry.StatusApproved) + ps.expireApproved() + if got := ps.listedNames(""); !containsName(got, "#oldcfg") { + t.Fatalf("approving the suggestion must list the channel again: %v", got) + } +} diff --git a/cmd/server/openapi.go b/cmd/server/openapi.go index d379bc60a..98536f1e0 100644 --- a/cmd/server/openapi.go +++ b/cmd/server/openapi.go @@ -150,7 +150,7 @@ func routeDescriptions() map[string]routeMeta { }}, // Channels - "GET /api/channels": {Summary: "List channels", Description: "Returns known mesh channels with message counts. approvedChannels ([{name, hash}]) lists the shared hashtag channels an administrator approved, even before they carry traffic; omitted when empty. A channel with stored messages whose shared-channel proposal is not approved (revoked, suggested again and pending, or that re-suggestion rejected) is left out of the list (#251), unless the ingestor also decrypts that name through its built-in/config list (rainbow table, hashChannels, channelKeys); only approving the proposal lists it again, with its history. hiddenChannels ([string], omitted when empty) names the channels left out, so the page does not re-create their rows from live packets. The messages stay readable per channel (GET /api/channels/{hash}/messages) and nothing is deleted. Decided per channel with stored messages from a short-lived snapshot of the proposals table (10s, dropped early when an approve/revoke result is read), never a per-request query and without a row cap. When the ingestor's built-in names file is missing or unreadable nothing is hidden. GET /api/analytics/channels is not filtered and still counts these channels.", Tag: "channels"}, + "GET /api/channels": {Summary: "List channels", Description: "Returns known mesh channels with message counts. approvedChannels ([{name, hash}]) lists the shared hashtag channels an administrator approved, even before they carry traffic; omitted when empty. A channel with stored messages whose shared-channel proposal is not approved (revoked, suggested again and pending, or that re-suggestion rejected) is left out of the list (#251), unless the ingestor also decrypts that name through its built-in/config list (rainbow table, hashChannels, channelKeys); only approving the proposal lists it again, with its history. hiddenChannels ([string], omitted when empty) names the channels left out, so the page does not re-create their rows from live packets; it only names channels that have stored messages (never an unreviewed suggestion), it is global and not filtered by region, and another open tab keeps the set it last loaded, also after a re-approval. The messages stay readable per channel (GET /api/channels/{hash}/messages) and nothing is deleted. Decided per channel with stored messages from a short-lived snapshot of the proposals table (10s, dropped early when an approve/revoke result is read), never a per-request query and without a row cap. When the ingestor's built-in names file is missing or unreadable nothing is hidden. GET /api/analytics/channels is not filtered and still counts these channels.", Tag: "channels"}, "GET /api/channels/{hash}/messages": {Summary: "Get channel messages", Description: "Returns messages for a specific channel.", Tag: "channels"}, // Shared channel proposals (internal/channelregistry). The server only diff --git a/docs/api-spec.md b/docs/api-spec.md index 481d3b72b..becae10c0 100644 --- a/docs/api-spec.md +++ b/docs/api-spec.md @@ -1363,7 +1363,13 @@ returns the history of a hidden channel. `hiddenChannels` (`string[]`, omitted when empty) names the channels that were left out this way. The Channels page uses it so a live WebSocket packet for a -stored message does not create the list row again. +stored message does not create the list row again. It only names channels that +have stored messages, so an unreviewed suggestion is never published here. + +`hiddenChannels` is global and not filtered by `region`: a region request +returns the same set, and another open tab keeps the set it last loaded — +including after a re-approval, where live messages do not re-create the row in +that tab until its list reloads. The decision is read once per 10 s snapshot (the one behind `approvedChannels`, dropped early when an approve or revoke result is read), per channel that has @@ -1376,6 +1382,18 @@ messages are still stored then appears again. with their message and sender counts (no message text). Hiding is a list-level measure, not a confidentiality control: the history stays readable by name. +**Known limit: a suggestion can hide a name no administrator acted on.** The +rule is "not approved", not "was approved before", so a channel whose stored +messages were decrypted through the config, and whose name the ingestor no +longer decrypts (removed from `hashChannels`/`channelKeys`, or past the +4096-name cap of `builtin-channels.json`), is left out of the list while a +suggestion for that name is pending. Distinguishing the two would need a +history marker that survives a re-suggestion — `reviewed_at` is reset when a +revoked proposal is suggested again, which is what makes a resubmission +idempotent — so the trade-off is kept: it is list-level only, the history stays +readable, a name the ingestor still decrypts is never hidden, the suggestion is +in the administrator's pending queue, and approving it lists the channel again. + --- ## GET /api/channels/:hash/messages diff --git a/docs/user-guide/channels.md b/docs/user-guide/channels.md index 8e4226b44..23dd19f12 100644 --- a/docs/user-guide/channels.md +++ b/docs/user-guide/channels.md @@ -92,6 +92,10 @@ Letter case matters: `#HelloWorld` and `#helloworld` are different channels with A removed channel shows up under the **Revoked** tab. Suggesting the same name again later starts a fresh review from **Pending** — it is never auto-approved just because it was approved before. The channel stays out of the list while that suggestion is pending and if you reject it; only **Approve** lists it again, with its history. The analytics page still counts messages of removed channels (counts only, no text), and a removed channel's history stays readable by its name. +The hidden set is the same for every region, and another open tab keeps the set it last loaded until its list reloads — also after you approve a channel again, so live messages do not bring its row back in that tab until then. + +Because the rule is "not approved" rather than "was approved before", a pending suggestion can also take out a channel you never reviewed: one whose messages were decrypted through a `channelKeys`/`hashChannels` entry you have since removed from `config.json`. Such a channel is listed until someone suggests its name. Nothing is deleted, its history stays readable, a name your configuration still decrypts is never hidden, and you see the suggestion under **Pending** — approving it lists the channel again. + The local **Add Channel** tools (PSK channels, **Monitor Hashtag Channel**) are unchanged and still only affect your browser. ## Region filter diff --git a/test-channels-client-state-152.js b/test-channels-client-state-152.js index 328b2ca3b..5963ad61b 100644 --- a/test-channels-client-state-152.js +++ b/test-channels-client-state-152.js @@ -204,8 +204,7 @@ function makeHarness(opts) { // live PSK decrypt and the unread bump), called without the debounce. ctx.debouncedOnWS = (fn) => { h.wsHandler = fn; return fn; }; ctx.debounce = (fn) => fn; - ctx.invalidateApiCache = () => {}; - ctx.api = (p) => { + const fetchApi = (p) => { if (p.indexOf('/channels') === 0 && p.indexOf('/messages') === -1) { h.channelRequests.push(p); return h.respondChannels(p); @@ -217,6 +216,38 @@ function makeHarness(opts) { if (p.indexOf('/observers') === 0) return Promise.resolve({ observers: [] }); return Promise.resolve({ messages: [], packets: [], channels: [] }); }; + // #276: opts.apiTtlCache gives the harness the per-path TTL cache the real + // api() has (public/app.js), including bust and the prefix-wide + // invalidateApiCache(). Without it a page that forgets to invalidate the + // cache looks correct here, because every call reaches the responder; the + // browser would serve a stale list for up to CLIENT_TTL.channels. + const apiCache = new Map(); + h.apiCacheKeys = () => Array.from(apiCache.keys()); + h.apiCacheHits = 0; + if (opts.apiTtlCache) { + ctx.invalidateApiCache = (prefix) => { + for (const key of Array.from(apiCache.keys())) { + if (key.startsWith(prefix || '')) apiCache.delete(key); + } + }; + ctx.api = async (p, o) => { + const ttl = (o && o.ttl) || 0; + const bust = !!(o && o.bust); + if (!bust && ttl > 0) { + const cached = apiCache.get(p); + if (cached && RealDate.now() < cached.expires) { + h.apiCacheHits++; + return cached.data; + } + } + const data = await fetchApi(p, o); + if (ttl > 0) apiCache.set(p, { data, expires: RealDate.now() + ttl }); + return data; + }; + } else { + ctx.invalidateApiCache = () => {}; + ctx.api = fetchApi; + } ctx.CLIENT_TTL = { channels: 15000, observers: 120000, channelMessages: 10000, nodeDetail: 10000 }; ctx.escapeHtml = (s) => String(s == null ? '' : s); ctx.truncate = (s, n) => { s = String(s || ''); return s.length > n ? s.slice(0, n) : s; }; @@ -347,6 +378,11 @@ function bigMessages(chars, tag) { return [{ sender: 'S', text: tag + ':' + 'x'.repeat(chars), timestamp: '2026-01-01T00:00:00Z', packetHash: 'h-' + tag }]; } +// The /channels entries of the harness TTL cache (#276), in insertion order. +function channelCacheKeys(h) { + return h.apiCacheKeys().filter((k) => k.indexOf('/channels') === 0); +} + function serverChannel(hash, extra) { return Object.assign({ hash, @@ -565,6 +601,56 @@ async function test(name, fn) { assert.strictEqual(h.row('#helloworld'), undefined, 'the revoked channel must be gone'); }); + // #276 (4): refreshChannelList() must drop the whole /channels prefix from + // the client cache, not only bust the one request it makes. Both the + // revoke (#251) and the approval (#232) path go through it, and the cached + // entry for every other region / encrypted variant of /channels — and for + // /channels//messages — would otherwise keep answering from the 15 s + // TTL. Needs the harness TTL cache (apiTtlCache): without it every call + // reaches the responder and a missing invalidateApiCache('/channels') is + // invisible. + await test('#276: after a revoke, a region switch back must not serve the pre-revoke list from the client cache (#251)', async () => { + const h = makeHarness({ apiTtlCache: true }); + const withChannel = { channels: [serverChannel('public'), serverChannel('#helloworld')], approvedChannels: [{ hash: '#helloworld', name: '#helloworld' }] }; + h.respondChannels = () => Promise.resolve(withChannel); + await h.init(); + assert.ok(h.row('#helloworld'), 'precondition: listed in the all-regions view'); + // A second /channels variant gets its own cache entry. + h.regionParam = 'CPH'; + h.regionChange(); + await flush(); + assert.deepStrictEqual(channelCacheKeys(h), ['/channels', '/channels?region=CPH'], 'both /channels variants are cached'); + // The admin revoke in this tab. + h.respondChannels = () => Promise.resolve({ channels: [serverChannel('public')], hiddenChannels: ['#helloworld'] }); + h.revokedCallback('#helloworld'); + await flush(); + assert.strictEqual(h.row('#helloworld'), undefined, 'precondition: gone right after the revoke'); + // Back to all regions, within the 15 s TTL and without a bust. + h.regionParam = ''; + h.regionChange(); + await flush(); + assert.strictEqual(h.row('#helloworld'), undefined, 'a revoked channel must not come back from the client cache on a region switch'); + assert.ok(!channelCacheKeys(h).includes('/channels') || h.apiCacheHits === 0, 'the pre-revoke /channels entry must not be served (cache hits: ' + h.apiCacheHits + ')'); + }); + + await test('#276: after an approval, a region switch back must not serve the pre-approval list from the client cache (#232)', async () => { + const h = makeHarness({ apiTtlCache: true }); + h.respondChannels = () => Promise.resolve({ channels: [serverChannel('public')] }); + await h.init(); + h.regionParam = 'CPH'; + h.regionChange(); + await flush(); + assert.deepStrictEqual(channelCacheKeys(h), ['/channels', '/channels?region=CPH'], 'both /channels variants are cached'); + h.respondChannels = () => Promise.resolve({ channels: [serverChannel('public'), serverChannel('#helloworld')], approvedChannels: [{ hash: '#helloworld', name: '#helloworld' }] }); + h.approvedCallback('#helloworld'); + await flush(); + assert.ok(h.row('#helloworld'), 'precondition: listed right after the approval'); + h.regionParam = ''; + h.regionChange(); + await flush(); + assert.ok(h.row('#helloworld'), 'an approved channel must still be listed after a region switch, not hidden again by the client cache'); + }); + // #232: with auto-approve on, the suggest poller sees `approved` and must // refresh this tab's list through the same onApproved path as the admin // decision, keeping the open PSK conversation and the unread counts.