From 5a81fcf3dd9102186f81c153027d8ec59a637ab0 Mon Sep 17 00:00:00 2001 From: ecgang Date: Sun, 26 Jul 2026 13:34:52 -0700 Subject: [PATCH] Bound the group claim to what the code actually proves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A cross-model pass caught the new doc comment claiming more than the code does. `isGroup` tests the `group_` prefix only — `PeerID(str:)` assigns a prefix by `hasPrefix` and never validates the bare — so the branch admits any persisted id claiming to be a group, not a group proven to exist. The comment now says so, along with why that is acceptable here: the value is written by our own selection path into local state, never parsed from the network, and a corrupted one restores an empty group rather than the phantom DM this predicate exists to prevent. Two other overstatements struck. "A group id is never blocked in practice" was unsupported and load-bearing for nothing. And the geoDM branch resolves to false for the *production* closure only — the injected seam will return true for a stub that accepts a geoDM, so a passing test there is not evidence that geoDM restore works. Said plainly, since the whole point of the previous commit was to stop the comment claiming behaviour the code lacks. Two tests: id classes are mutually exclusive, so the branch ordering is not load-bearing the day that stops being true; and the malformed-group case is pinned as deliberate, so if the id ever becomes untrusted this test is what has to change. Co-Authored-By: Claude Opus 5 (1M context) --- bitchat/App/AppRuntime.swift | 41 ++++++++++++++----- .../ConversationStoreLastActiveTests.swift | 38 +++++++++++++++++ 2 files changed, 69 insertions(+), 10 deletions(-) diff --git a/bitchat/App/AppRuntime.swift b/bitchat/App/AppRuntime.swift index 9b550842..0823647e 100644 --- a/bitchat/App/AppRuntime.swift +++ b/bitchat/App/AppRuntime.swift @@ -250,9 +250,29 @@ final class AppRuntime: ObservableObject { /// empty for every group without ever consulting the group. Left to the /// peer terms a group would therefore never restore — silently, every /// time. `startPrivateChat` gates group re-entry on nothing at all ("no - /// peer identity, favorites, handshake … just select the chat"), and there - /// is no phantom to guard against: the conversation is local state, not a - /// claim about reachability. So groups are admitted outright. + /// peer identity, favorites, handshake … just select the chat"), so + /// admitting groups here restores what re-entry would have opened. (Not + /// quite an identity: the block veto above has no counterpart in + /// `startPrivateChat`'s group branch, so restore is the narrower of the + /// two. Moot today — `isBlocked` needs a resolved fingerprint and a group + /// id never has one — but stated rather than leaned on.) + /// + /// Note the bound on that admission: `isGroup` tests the `group_` prefix + /// only — `PeerID(str:)` assigns a prefix by `hasPrefix` and never + /// validates the bare — so this branch admits any persisted id *claiming* + /// to be a group, not a group proven to exist. + /// + /// That is acceptable, but for a narrower reason than "the id is trusted". + /// A group id can certainly originate remotely: an invite carries one, and + /// `GroupProtocol` derives the conversation id from it. Every such id is + /// built by `PeerID(groupID:)`, which hex-encodes the raw bytes, so a + /// group learned from the network still has a structurally valid bare. A + /// malformed `group_` id therefore implies corrupted local persistence + /// rather than hostile input. And the failure it produces is an empty + /// group, not a DM compose box aimed at an unreachable peer — which is the + /// specific hazard this predicate exists to prevent. Admitting only groups + /// that still exist locally would need a membership lookup this predicate + /// does not take. /// /// Geohash/Nostr ids are screened before the peer terms, so nothing can /// bypass the check by matching earlier. A geoChat id is not a direct chat @@ -266,19 +286,20 @@ final class AppRuntime: ObservableObject { /// alone, and `getFavoriteStatus(forPeerID:)` matches by rebuilding /// `PeerID(publicKey:)` — which carries no prefix — so it can never equal a /// `nostr_`-prefixed id no matter what is favorited. The geoDM branch below - /// is the correct shape for the day a Nostr-keyed lookup exists; until - /// then it resolves to `false` and the fallback to the conversation list is - /// the whole behaviour. Documented rather than fixed here: adding that - /// lookup means new favorites plumbing, which is not this change. + /// is kept because it is the right shape once a Nostr-keyed lookup exists, + /// but read it precisely: it resolves to `false` for the *production* + /// closure only. The injected seam will happily return `true` for a stub + /// that accepts a geoDM, so a passing test here is not evidence that geoDM + /// restore works. Documented rather than fixed: wiring the real lookup + /// means new favorites plumbing, which is not this change. static func isDirectChatRestorable( _ peerID: PeerID, isPeerFavorited: (PeerID) -> Bool, hasStoredCryptographicIdentity: (PeerID) -> Bool, isPeerBlocked: (PeerID) -> Bool ) -> Bool { - // Blocked is a veto, never one term among several. Kept ahead of the - // group branch so the veto is unconditional; a group id is never - // blocked in practice, so the order costs nothing. + // Blocked is a veto, never one term among several — including over the + // group branch below, so no id class can sidestep it. guard !isPeerBlocked(peerID) else { return false } // A private group is local state, not a claim about reaching a peer. // The peer terms below cannot represent it and would refuse it. diff --git a/bitchatTests/ConversationStoreLastActiveTests.swift b/bitchatTests/ConversationStoreLastActiveTests.swift index 60464b29..f7fc07b3 100644 --- a/bitchatTests/ConversationStoreLastActiveTests.swift +++ b/bitchatTests/ConversationStoreLastActiveTests.swift @@ -375,6 +375,44 @@ final class ConversationStoreLastActiveTests: XCTestCase { XCTAssertEqual(identityLookups, 0, "group restore consulted the identity term") } + func test_idClassesAreMutuallyExclusive() { + // The group branch sits between the geoChat guard and the geoDM one, + // so its placement would be load-bearing if an id could belong to two + // classes at once. `PeerID` assigns exactly one prefix, so it cannot — + // pin that, because the day it stops being true the ordering silently + // decides which rule wins. + let group = PeerID(str: "group_" + String(repeating: "ab", count: 16)) + let geoDM = PeerID(str: "nostr_0123456789abcdef") + let geoChat = PeerID(str: "nostr:someGeohashChannel") + + XCTAssertTrue(group.isGroup) + XCTAssertFalse(group.isGeoDM) + XCTAssertFalse(group.isGeoChat) + + XCTAssertFalse(geoDM.isGroup) + XCTAssertFalse(geoChat.isGroup) + } + + func test_malformedGroupIDIsStillAdmitted_documentingTheBound() { + // `isGroup` tests the prefix only — `PeerID(str:)` never validates the + // bare — so this branch admits any persisted id claiming to be a + // group. That is deliberate and bounded: the value is written by our + // own selection path into local state, never parsed from the network, + // and the failure it can produce is an empty group rather than the + // phantom DM this predicate exists to prevent. Pinned so that if the + // id ever becomes untrusted, this test is the thing that has to change. + let malformed = PeerID(str: "group_not-hex") + XCTAssertTrue(malformed.isGroup) + XCTAssertTrue( + AppRuntime.isDirectChatRestorable( + malformed, + isPeerFavorited: { _ in false }, + hasStoredCryptographicIdentity: { _ in false }, + isPeerBlocked: { _ in false } + ) + ) + } + func test_blockedGroupIsNotRestorable() { // Block stays an unconditional veto, ahead of the group branch. let group = PeerID(str: "group_" + String(repeating: "ef", count: 16))