From 132120a88eb044a8ee6adafa33d03b159242131a Mon Sep 17 00:00:00 2001 From: jack <212554440+jackjackbits@users.noreply.github.com> Date: Sun, 26 Jul 2026 21:01:38 +0200 Subject: [PATCH] Close the three holes the #1486 review found (#1488) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix the three follow-ups from the #1486 review Three defects shipped with the censorship-resilience merge, all confirmed against main: 1. Source-manifest verification silently accepted added files. shasum -c checks only the files the manifest lists, and the Xcode project compiles every source file present in the tree — so a hostile mirror could pass verification by adding a file rather than modifying one. The manifest header and VERIFYING-A-BUILD.md now require the completeness check (git status --porcelain, or a path diff for tarballs) alongside the hash check. 2. A relay removed while Tor was bootstrapping reconnected anyway. dropRelays never subtracted from pendingTorConnectionURLs, and a custom relay passes the allow-list filter, so draining the pending queue resurrected a relay someone had explicitly deleted. 3. Turning Tor off mid-bootstrap read as 'network may be blocking tor'. shutdownCompletely left the detached 75s poll loop running, which then stamped bootstrapDidStall over the clean shutdown state; and the stall handler guarded on torEnforced, which is compile-time true in release, instead of the runtime preference. The poll loop is now generation-fenced (shutdown, dormancy, and restart each invalidate it) and the handler consults persistedTorPreference(). Both app-side fixes carry regression tests proven to fail pre-fix. Co-Authored-By: Claude Fable 5 * Address Codex review: ignored files and manifest placement git status --porcelain omits ignored paths, and .gitignore covers build/ — a planted bitchat/build/Evil.swift would compile via the synchronized group while the documented check stayed silent. The checkout check now uses --ignored. The downloaded manifest also has to live outside the tree, or it trips the completeness checks itself; the doc now says so and references it at /tmp throughout. Co-Authored-By: Claude Fable 5 --------- Co-authored-by: jack Co-authored-by: Claude Fable 5 --- .github/workflows/source-manifest.yml | 8 +++- bitchat/Nostr/NostrRelayManager.swift | 3 ++ .../Extensions/ChatViewModel+Tor.swift | 4 ++ .../ChatViewModelExtensionsTests.swift | 34 ++++++++++++++ .../Services/NostrRelayManagerTests.swift | 40 +++++++++++++++++ docs/VERIFYING-A-BUILD.md | 25 +++++++++-- localPackages/Arti/Sources/TorManager.swift | 44 +++++++++++-------- 7 files changed, 135 insertions(+), 23 deletions(-) diff --git a/.github/workflows/source-manifest.yml b/.github/workflows/source-manifest.yml index 6f8a0c60..e05ae34f 100644 --- a/.github/workflows/source-manifest.yml +++ b/.github/workflows/source-manifest.yml @@ -63,7 +63,13 @@ jobs: echo "#" echo "# Verify a checkout of this ref with:" echo "# shasum -a 256 -c files.sha256" - echo "# The git tree hash above is the single value that covers all of it:" + echo "# Hash checking alone ignores files this manifest does not list, and" + echo "# the Xcode project compiles any source file present in the tree. So" + echo "# also confirm nothing extra is present:" + echo "# git status --porcelain --ignored # git checkout: must print nothing" + echo "# or, for a tarball, diff this manifest's path list against find(1)." + echo "# Full instructions: docs/VERIFYING-A-BUILD.md" + echo "# The git tree hash above is the single value covering all tracked content:" echo "# git rev-parse HEAD^{tree}" echo "#" } > SOURCE-MANIFEST.txt diff --git a/bitchat/Nostr/NostrRelayManager.swift b/bitchat/Nostr/NostrRelayManager.swift index 5873247e..fca24465 100644 --- a/bitchat/Nostr/NostrRelayManager.swift +++ b/bitchat/Nostr/NostrRelayManager.swift @@ -848,6 +848,9 @@ final class NostrRelayManager: ObservableObject { } } messageQueueLock.unlock() + // A relay queued while Tor bootstraps would otherwise reconnect when + // the queue drains, overriding the explicit removal. + pendingTorConnectionURLs.subtract(urls) relays.removeAll { urls.contains($0.url) } updateConnectionStatus() } diff --git a/bitchat/ViewModels/Extensions/ChatViewModel+Tor.swift b/bitchat/ViewModels/Extensions/ChatViewModel+Tor.swift index 52eaca7c..3bf7a5a1 100644 --- a/bitchat/ViewModels/Extensions/ChatViewModel+Tor.swift +++ b/bitchat/ViewModels/Extensions/ChatViewModel+Tor.swift @@ -64,6 +64,10 @@ extension ChatViewModel { @objc func handleTorBootstrapDidStall() { Task { @MainActor in guard TorManager.shared.torEnforced else { return } + // torEnforced is a compile-time constant in release builds; the + // runtime preference is what says whether anyone is waiting on + // Tor. Turning Tor off mid-bootstrap must not read as blocking. + guard NetworkActivationService.persistedTorPreference() else { return } guard !self.torStallAnnounced else { return } self.torStallAnnounced = true self.addGeohashOnlySystemMessage( diff --git a/bitchatTests/ChatViewModelExtensionsTests.swift b/bitchatTests/ChatViewModelExtensionsTests.swift index e4a0ab85..a1d12550 100644 --- a/bitchatTests/ChatViewModelExtensionsTests.swift +++ b/bitchatTests/ChatViewModelExtensionsTests.swift @@ -1374,3 +1374,37 @@ private func makeImageData() throws -> Data { return data #endif } + +// MARK: - Tor Extension Tests + +struct ChatViewModelTorExtensionTests { + + /// Turning Tor off mid-bootstrap must not read as "the network is + /// blocking tor": `torEnforced` is a compile-time constant, so the stall + /// handler has to consult the runtime preference before announcing. + @Test @MainActor + func bootstrapStall_withTorPreferenceOff_announcesNothing() async { + let key = NetworkActivationService.torPreferenceKey + let previous = UserDefaults.standard.object(forKey: key) + defer { + if let previous { + UserDefaults.standard.set(previous, forKey: key) + } else { + UserDefaults.standard.removeObject(forKey: key) + } + } + let (viewModel, _) = makeTestableViewModel() + + UserDefaults.standard.set(false, forKey: key) + viewModel.handleTorBootstrapDidStall() + try? await Task.sleep(nanoseconds: 50_000_000) + #expect(viewModel.torStallAnnounced == false) + + // The same stall with the preference on (the persisted default) is + // exactly what must still be announced. + UserDefaults.standard.set(true, forKey: key) + viewModel.handleTorBootstrapDidStall() + try? await Task.sleep(nanoseconds: 50_000_000) + #expect(viewModel.torStallAnnounced == true) + } +} diff --git a/bitchatTests/Services/NostrRelayManagerTests.swift b/bitchatTests/Services/NostrRelayManagerTests.swift index c50d1ba4..8e3849f0 100644 --- a/bitchatTests/Services/NostrRelayManagerTests.swift +++ b/bitchatTests/Services/NostrRelayManagerTests.swift @@ -44,6 +44,46 @@ final class NostrRelayManagerTests: XCTestCase { XCTAssertTrue(context.sessionFactory.allConnections.allSatisfy { $0.cancelCallCount >= 1 }) } + /// A relay removed while its connection is queued behind Tor bootstrap + /// must stay removed: draining the pending set used to resurrect it, + /// because `dropRelays` never touched `pendingTorConnectionURLs` and a + /// custom relay is in neither the default set nor the allow-list filter. + func test_relayRemovedWhileWaitingForTor_staysRemovedWhenTorBecomesReady() async { + let customURL = "wss://custom-removed.example" + let center = NotificationCenter() + let customRelays = MutableRelayList(urls: [customURL]) + let context = makeContext( + permission: .authorized, + userTorEnabled: true, + torEnforced: true, + torIsReady: false, + notificationCenter: center, + customRelays: customRelays + ) + + // Defaults plus the custom relay all queue while Tor bootstraps. + context.manager.connect() + XCTAssertTrue(context.sessionFactory.requestedURLs.isEmpty) + XCTAssertEqual(context.torWaiter.awaitCallCount, 1) + + // The relay is removed by hand before Tor is ready. + customRelays.urls = [] + center.post(name: NostrRelaySettings.didChangeNotification, object: nil) + // The settings sink hops through the main queue; let it land. + try? await Task.sleep(nanoseconds: 20_000_000) + + context.torWaiter.resolve(true) + + let defaultsConnected = await waitUntil { + context.sessionFactory.requestedURLs.count == self.expectedDefaultRelayCount + } + XCTAssertTrue(defaultsConnected) + XCTAssertFalse( + context.sessionFactory.requestedURLs.contains(customURL), + "a relay removed while Tor was bootstrapping must not reconnect when the pending queue drains" + ) + } + func test_connect_waitsForTorReadinessBeforeCreatingSessions() async { let context = makeContext(permission: .authorized, userTorEnabled: true, torEnforced: true, torIsReady: false) diff --git a/docs/VERIFYING-A-BUILD.md b/docs/VERIFYING-A-BUILD.md index 818c831b..23b5181b 100644 --- a/docs/VERIFYING-A-BUILD.md +++ b/docs/VERIFYING-A-BUILD.md @@ -18,26 +18,43 @@ In order of how much verification is possible: Every tagged release has a `SOURCE-MANIFEST.txt` produced by `.github/workflows/source-manifest.yml`. It records the tag, the commit, the git tree hash, and a SHA-256 for every tracked file. -Check a copy of the source against it: +Keep the downloaded manifest *outside* the source tree (say, `/tmp`) — a stray copy inside the checkout would itself trip the completeness checks below. Then check a copy of the source against it: ```sh -# From the root of the source you obtained -grep -v '^#' SOURCE-MANIFEST.txt > /tmp/files.sha256 +# From the root of the source you obtained, with the manifest at /tmp +grep -v '^#' /tmp/SOURCE-MANIFEST.txt > /tmp/files.sha256 shasum -a 256 -c /tmp/files.sha256 ``` Any `FAILED` line means that file differs from the released source. Investigate before building. +That check alone is not enough. `shasum -c` verifies the files the manifest lists and says nothing about files it does not list — and the Xcode project compiles every source file present in the tree automatically, so a hostile mirror can pass the hash check by leaving every listed file intact and *adding* one. Confirm nothing extra is present: + +```sh +# The manifest's path list must match the tree exactly — no missing files, no extras +grep -v '^#' /tmp/SOURCE-MANIFEST.txt | sed 's/^[0-9a-f]* //' | LC_ALL=C sort > /tmp/manifest-paths +find . -type f ! -path './.git/*' | sed 's|^\./||' | LC_ALL=C sort > /tmp/actual-paths +diff /tmp/manifest-paths /tmp/actual-paths # must print nothing +``` + +In a git checkout the same assurance is one command — it also catches extra files, because they show as untracked. `--ignored` matters: `.gitignore` covers paths like `build/`, plain `git status` would not report a planted file there, and Xcode compiles it all the same: + +```sh +git status --porcelain --ignored # must print nothing before you build +``` + The single value that covers the whole tree is the git tree hash in the manifest header: ```sh git rev-parse HEAD^{tree} # must equal the "tree:" line in the manifest ``` +Note the tree hash covers tracked content only; it does not see untracked files sitting in the working directory, which is why the emptiness checks above come first. + The manifest itself carries a provenance attestation tying it to the workflow run that produced it, so a manifest handed to you along with a mirror is checkable too: ```sh -gh attestation verify SOURCE-MANIFEST.txt --repo permissionlesstech/bitchat +gh attestation verify /tmp/SOURCE-MANIFEST.txt --repo permissionlesstech/bitchat ``` That last step is what makes this resistant to a hostile mirror. Without it, whoever gives you the source can give you a matching manifest. diff --git a/localPackages/Arti/Sources/TorManager.swift b/localPackages/Arti/Sources/TorManager.swift index 1bb42b63..1e9f2eb8 100644 --- a/localPackages/Arti/Sources/TorManager.swift +++ b/localPackages/Arti/Sources/TorManager.swift @@ -84,6 +84,10 @@ public final class TorManager: ObservableObject { private var shutdownsInFlight = 0 private var startPendingAfterShutdown = false private var bootstrapMonitorStarted = false + // Fences the detached poll loop: shutdown, dormancy, and restart each bump + // this, so a loop from a previous attempt cannot run out its deadline and + // report a stall over state that a newer lifecycle event already owns. + private var bootstrapGeneration = 0 private var pathMonitor: NWPathMonitor? private var isAppForeground: Bool = true private var lastRestartAt: Date? = nil @@ -268,24 +272,25 @@ public final class TorManager: ObservableObject { private func startBootstrapMonitor() { guard !bootstrapMonitorStarted else { return } bootstrapMonitorStarted = true + bootstrapGeneration += 1 + let generation = bootstrapGeneration Task.detached(priority: .utility) { [weak self] in - await self?.bootstrapPollLoop() + await self?.bootstrapPollLoop(generation: generation) } } - private func bootstrapPollLoop() async { + private func bootstrapPollLoop(generation: Int) async { let deadline = Date().addingTimeInterval(75) var didComplete = false while Date() < deadline { + guard generation == bootstrapGeneration else { return } let progress = Int(arti_bootstrap_progress()) let summary = getBootstrapSummary() - await MainActor.run { - self.bootstrapProgress = progress - self.bootstrapSummary = summary - if progress >= 100 { self.isStarting = false } - self.recomputeReady() - } + self.bootstrapProgress = progress + self.bootstrapSummary = summary + if progress >= 100 { self.isStarting = false } + self.recomputeReady() if progress >= 100 { didComplete = true @@ -296,17 +301,17 @@ public final class TorManager: ObservableObject { // Running out the deadline is a reportable outcome, not silence. The // loop previously just ended, leaving `isStarting` true forever, so a - // blocked network was indistinguishable from a slow one. + // blocked network was indistinguishable from a slow one. A deliberate + // shutdown mid-bootstrap is not a stall, hence the generation check. if !didComplete { - await MainActor.run { - self.isStarting = false - self.bootstrapDidStall = true - SecureLogger.warning( - "TorManager: bootstrap did not complete within its deadline (progress=\(self.bootstrapProgress)); network may be blocking Tor", - category: .session - ) - NotificationCenter.default.post(name: .TorBootstrapDidStall, object: nil) - } + guard generation == bootstrapGeneration else { return } + self.isStarting = false + self.bootstrapDidStall = true + SecureLogger.warning( + "TorManager: bootstrap did not complete within its deadline (progress=\(self.bootstrapProgress)); network may be blocking Tor", + category: .session + ) + NotificationCenter.default.post(name: .TorBootstrapDidStall, object: nil) } } @@ -352,6 +357,7 @@ public final class TorManager: ObservableObject { // Clear isStarting so foreground recovery can proceed if bootstrap was interrupted. SecureLogger.debug("TorManager: goDormantOnBackground() called", category: .session) Task { @MainActor in + self.bootstrapGeneration += 1 self.isReady = false self.socksReady = false self.isStarting = false @@ -361,6 +367,7 @@ public final class TorManager: ObservableObject { public func shutdownCompletely() { SecureLogger.debug("TorManager: shutdownCompletely() called", category: .session) startPendingAfterShutdown = false + bootstrapGeneration += 1 shutdownsInFlight += 1 Task.detached { [weak self] in guard let self = self else { return } @@ -398,6 +405,7 @@ public final class TorManager: ObservableObject { SecureLogger.debug("TorManager: restartArti() starting", category: .session) await MainActor.run { NotificationCenter.default.post(name: .TorWillRestart, object: nil) + self.bootstrapGeneration += 1 self.isReady = false self.socksReady = false self.bootstrapProgress = 0