From 7cff2e398778e2ca9b67f693d31afa1a35b44a68 Mon Sep 17 00:00:00 2001 From: heyaim <223061694+heyaim@users.noreply.github.com> Date: Sat, 10 Oct 2026 06:22:41 -0500 Subject: [PATCH] Handle relay port integer overflow without aborting parsing A relay row with valid coordinates and a digit-only port outside the integer range caused directory parsing to throw a NumberFormatException. The surrounding load handlers caught the exception, but that parse returned no relay list. This did not establish an application crash or a delivery failure. Changed the conversion to skip that row. Added a regression containing valid rows before and after it, plus ordinary-port controls. Narrowed the source, selection and test descriptions: sharing a source alone does not establish equal loaded snapshots or successful message delivery. Used the Kotlin preferences helper for asynchronous writes. The integer-overflow port test reproduced the parsing failure before the fix and passed afterward. The full test suites passed with three unrelated skips, and debug builds passed. Lint reported errors and warnings, with none in the changed files. Required physical Mesh Lab validation remains unperformed. --- .github/workflows/fetch-georelays.yml | 4 +- .../bitchat/android/nostr/RelayDirectory.kt | 34 ++++------- .../android/nostr/RelayDirectoryPortTest.kt | 22 +++++++ .../android/nostr/RelayDirectoryTest.kt | 44 ++++++------- docs/client-rewrite-contracts.md | 61 +++++++++++-------- 5 files changed, 94 insertions(+), 71 deletions(-) create mode 100644 app/src/test/kotlin/com/bitchat/android/nostr/RelayDirectoryPortTest.kt diff --git a/.github/workflows/fetch-georelays.yml b/.github/workflows/fetch-georelays.yml index f2300bd0c..24d9814ed 100644 --- a/.github/workflows/fetch-georelays.yml +++ b/.github/workflows/fetch-georelays.yml @@ -19,8 +19,8 @@ jobs: token: ${{ secrets.GITHUB_TOKEN }} - name: Fetch GeoRelays - # Must match RelayDirectory.ASSET_FILE_URL, the file the app fetches at - # runtime; the bundled asset is a snapshot of the same file. + # Must match RelayDirectory.ASSET_FILE_URL. This job refreshes the + # bundled asset from the app's runtime source. run: | wget https://raw.githubusercontent.com/permissionlesstech/bitchat/refs/heads/main/relays/online_relays_gps.csv mv online_relays_gps.csv ./app/src/main/assets/nostr_relays.csv diff --git a/app/src/main/java/com/bitchat/android/nostr/RelayDirectory.kt b/app/src/main/java/com/bitchat/android/nostr/RelayDirectory.kt index fd7d5d2a5..6c07d5f0a 100644 --- a/app/src/main/java/com/bitchat/android/nostr/RelayDirectory.kt +++ b/app/src/main/java/com/bitchat/android/nostr/RelayDirectory.kt @@ -3,6 +3,7 @@ package com.bitchat.android.nostr import android.app.Application import android.content.SharedPreferences import android.util.Log +import androidx.core.content.edit import java.io.BufferedReader import java.io.File import java.io.FileInputStream @@ -27,11 +28,8 @@ object RelayDirectory { private const val TAG = "RelayDirectory" - // The same file iOS reads (GeoRelayDirectory.swift). Both platforms take the 5 - // nearest relays from their directory and use them without the defaults, so a - // geohash message crosses platforms only if the two selections share a relay. - // Reading different files made the selections diverge. Selecting from the same - // file, with rows keyed and ordered the same way, keeps them aligned. + // The source URL matches iOS GeoRelayDirectory. Download timing and parser + // acceptance can leave clients with different loaded snapshots. internal const val ASSET_FILE_URL = "https://raw.githubusercontent.com/permissionlesstech/bitchat/refs/heads/main/relays/online_relays_gps.csv" // Download cache of ASSET_FILE_URL above, and the bundled list the weekly job // refreshes from it; the file names predate the source's move to online_relays_gps.csv. @@ -82,7 +80,7 @@ object RelayDirectory { } } - // Start periodic staleness check every minute + // Recheck staleness after a one-minute delay following each iteration. startPeriodicRefresh(application) } catch (e: Exception) { Log.e(TAG, "Failed to initialize RelayDirectory: ${e.message}") @@ -108,11 +106,8 @@ object RelayDirectory { return closestRelays(snapshot, lat, lon, nRelays) } - // Distance ties are the directory's common case, not an edge: rows are geocoded - // to city centroids, so whole groups of relays sit at one exact coordinate. iOS - // breaks ties by host so every device with the same directory picks the same set - // (GeoRelayDirectory.closestRelays). Urls here are canonical hosts behind a fixed - // prefix, so ordering by url reproduces iOS's order. + // City-level coordinates create distance ties. Break them by endpoint key, + // following GeoRelayDirectory.closestRelays for the accepted directory rows. internal fun closestRelays(entries: List, lat: Double, lon: Double, nRelays: Int): List = entries .map { it to haversineMeters(lat, lon, it.latitude, it.longitude) } @@ -169,7 +164,7 @@ object RelayDirectory { val tmpFile = File.createTempFile("relays_", ".csv", application.cacheDir) val ok = downloadToFile(ASSET_FILE_URL, tmpFile) if (!ok) { - Log.w(TAG, "Failed to fetch latest relays; keeping current list (will fallback to bundled if none)") + Log.w(TAG, "Failed to fetch latest relays; keeping current list") tmpFile.delete() return } @@ -197,7 +192,7 @@ object RelayDirectory { relays.addAll(parsed) } - getPrefs(application).edit().putLong(KEY_LAST_UPDATE_MS, System.currentTimeMillis()).apply() + getPrefs(application).edit { putLong(KEY_LAST_UPDATE_MS, System.currentTimeMillis()) } Log.i(TAG, "✅ Using downloaded relay list (${dest.absolutePath}), entries=$entries, sha256=$hash, updatedAtMs=${getPrefs(application).getLong(KEY_LAST_UPDATE_MS, 0L)}") } catch (e: Exception) { @@ -213,7 +208,7 @@ object RelayDirectory { Log.w(TAG, "HTTP ${'$'}{resp.code} when fetching $url") return false } - val body = resp.body ?: return false + val body = resp.body FileOutputStream(dest).use { out -> body.byteStream().use { input -> input.copyTo(out) @@ -273,7 +268,7 @@ object RelayDirectory { internal fun parseCsv(input: InputStream): List { val result = mutableListOf() // The directory lists some relays twice, once bare and once with an explicit - // :443, which is the same server over wss. Without this check both copies can + // :443, which has the same endpoint key over wss. Without this check both copies can // land in a nearest-N selection, and one of its slots connects nowhere new. val seenEndpoints = HashSet() BufferedReader(InputStreamReader(input)).use { reader -> @@ -299,11 +294,8 @@ object RelayDirectory { } /** - * The host string iOS builds for the same row (GeoRelayDirectory's - * validatedDirectoryAddress): host lowercased, an explicit port kept unless it is - * 443, the wss default. A relay on :8443 stays distinct from one on :443. Dedup - * and tie ordering both key on this, which is what keeps the two platforms' - * selections aligned row for row. + * Endpoint key for deduplication and distance ties: lowercase host, with + * port 443 omitted. This parser does not implement iOS directory validation. */ internal fun canonicalHost(url: String): String { val hostPort = url.substringAfter("://").substringBefore("/") @@ -311,7 +303,7 @@ object RelayDirectory { val hasPort = idx > 0 && idx < hostPort.length - 1 && hostPort.substring(idx + 1).all { it.isDigit() } val host = (if (hasPort) hostPort.substring(0, idx) else hostPort).lowercase() - val port = if (hasPort) hostPort.substring(idx + 1).toInt() else 443 + val port = if (hasPort) (hostPort.substring(idx + 1).toIntOrNull() ?: return "") else 443 return if (port == 443) host else "$host:$port" } diff --git a/app/src/test/kotlin/com/bitchat/android/nostr/RelayDirectoryPortTest.kt b/app/src/test/kotlin/com/bitchat/android/nostr/RelayDirectoryPortTest.kt new file mode 100644 index 000000000..871e3bb67 --- /dev/null +++ b/app/src/test/kotlin/com/bitchat/android/nostr/RelayDirectoryPortTest.kt @@ -0,0 +1,22 @@ +package com.bitchat.android.nostr + +import org.junit.Assert.assertEquals +import org.junit.Test + +class RelayDirectoryPortTest { + @Test + fun `an oversized numeric port does not discard surrounding valid rows`() { + val csv = "Relay URL,Latitude,Longitude\n" + + "first.example,0,0\n" + + "oversized.example:999999999999999999999,1,2\n" + + "last.example:8443,3,4\n" + val entries = RelayDirectory.parseCsv(csv.byteInputStream()) + assertEquals(listOf("wss://first.example", "wss://last.example:8443"), entries.map { it.url }) + } + + @Test + fun `ordinary numeric ports retain their endpoint keys`() { + assertEquals("relay.example", RelayDirectory.canonicalHost("wss://relay.example:443")) + assertEquals("relay.example:8443", RelayDirectory.canonicalHost("wss://relay.example:8443")) + } +} diff --git a/app/src/test/kotlin/com/bitchat/android/nostr/RelayDirectoryTest.kt b/app/src/test/kotlin/com/bitchat/android/nostr/RelayDirectoryTest.kt index d5541d912..ab08a59b1 100644 --- a/app/src/test/kotlin/com/bitchat/android/nostr/RelayDirectoryTest.kt +++ b/app/src/test/kotlin/com/bitchat/android/nostr/RelayDirectoryTest.kt @@ -5,19 +5,7 @@ import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue import org.junit.Test -/** - * Pins the three properties cross-platform geohash delivery depends on. Both platforms - * take the 5 relays nearest to a geohash and use them without the default relays, so a - * geohash message crosses platforms only if the two selections intersect. - * - * 1. Android reads the SAME directory file iOS reads. - * 2. Rows are deduplicated and emitted by the same key iOS builds: host lowercased, - * an explicit port kept unless it is 443. The directory lists many relays twice, - * once bare and once as host:443, and both forms are one server over wss. - * 3. Distance ties order by that key, the way iOS orders them. Rows are geocoded to - * city centroids, so whole tie groups sit at one coordinate and tie order decides - * most selections. - */ +/** Covers the configured source URL, endpoint deduplication and deterministic distance ties. */ class RelayDirectoryTest { private fun parse(csv: String) = @@ -42,7 +30,7 @@ class RelayDirectoryTest { } @Test - fun `port 443 variant of a host is the same server and is not listed twice`() { + fun `port 443 variant of a host is the same endpoint key and is not listed twice`() { val entries = parse( header + "relay.example.com,10.0,20.0\n" + @@ -53,9 +41,8 @@ class RelayDirectoryTest { } @Test - fun `a nonstandard port is a different server and stays`() { - // The live directory lists some relays bare and on a nonstandard port. - // iOS keeps both too; it drops only an explicit 443. + fun `a nonstandard port is a distinct endpoint and stays`() { + // A nondefault port has a distinct endpoint key. val entries = parse( header + "port-variant.relay.example,1.0,1.0\n" + @@ -75,6 +62,7 @@ class RelayDirectoryTest { ) assertEquals(1, entries.size) assertEquals(10.0, entries[0].latitude, 0.0) + assertEquals(20.0, entries[0].longitude, 0.0) } @Test @@ -90,9 +78,7 @@ class RelayDirectoryTest { @Test fun `distance ties order by host the way ios orders them`() { - // The directory's dominant shape: a whole tie group at one shared coordinate - // (the live file has 137 rows at a single point). File order is - // deliberately not alphabetical; the selection must not depend on it. + // Shared coordinates create ties; shuffled rows exercise the endpoint tie-break. val csv = header + "delta.example.com,12.34,56.78\n" + "foxtrot.example.com,12.34,56.78\n" + @@ -114,10 +100,10 @@ class RelayDirectoryTest { } @Test - fun `five nearest means five distinct servers`() { + fun `five nearest means five distinct endpoint keys`() { // A shape the live directory produces: the nearest relay listed twice // (bare and :443), which used to occupy two of the five selection slots and - // push out the fifth distinct server. + // push out the fifth distinct endpoint. val csv = header + "nearest.example.com,10.10,20.20\n" + "nearest.example.com:443,10.10,20.20\n" + @@ -128,7 +114,17 @@ class RelayDirectoryTest { "faraway.example.com,80.0,120.0\n" val five = RelayDirectory.closestRelays(parse(csv), 10.10, 20.20, 5) assertEquals(5, five.size) - assertEquals("every selected relay is a distinct server", 5, five.toSet().size) - assertTrue("the fifth distinct server makes the cut", five.contains("wss://fifth.example.com")) + assertEquals("selected endpoint keys are distinct", 5, five.toSet().size) + assertTrue("the fifth distinct endpoint makes the cut", five.contains("wss://fifth.example.com")) + assertEquals( + listOf( + "wss://nearest.example.com", + "wss://second.example.com", + "wss://third.example.com", + "wss://fourth.example.com", + "wss://fifth.example.com" + ), + five + ) } } diff --git a/docs/client-rewrite-contracts.md b/docs/client-rewrite-contracts.md index f37809f0d..f7f6bd398 100644 --- a/docs/client-rewrite-contracts.md +++ b/docs/client-rewrite-contracts.md @@ -1,25 +1,31 @@ # Client rewrite compatibility contracts This document defines the behavior a from-scratch BitChat client must preserve. -The executable source of truth is the JVM test suite under -`app/src/test/**/contracts`, together with the pre-existing protocol, security, -mesh, and state tests. +The JVM contract, protocol, security, mesh and state tests provide executable +checks for parts of this contract. The remaining implementation work and milestone progress are tracked in [test-implementation-plan.md](test-implementation-plan.md). ## Required contract layers -| Layer | Compatibility promise | Primary tests | +The listed tests cover parts of each layer; they do not verify every requirement +in the corresponding row. + +| Layer | Required compatibility behavior | Selected tests (partial coverage) | |---|---|---| | Outer mesh packet | v1/v2 header widths, big-endian fields, flags, section order, route placement, signature placement, padding, compression, signing bytes | `BinaryProtocolTest`, `ClientRewriteWireContractTest` | | Chat payload | Flag bits, millisecond timestamp, UTF-8 byte lengths, encrypted-content substitution, optional-field order | `ClientRewriteWireContractTest` | | Inner payloads | Noise type bytes, private-message TLVs, peer-state TLVs, file-transfer TLVs, live-voice bursts, fragment header, sync request TLVs | `ClientRewriteWireContractTest`, `AuthenticatedPeerStateTest`, `PrivateMediaTransferPreparerTest`, `VoiceBurstPacketTest`, `FragmentManagerTest` | -| Identity/security | Announcement extensions, capability bitfield endianness, Noise static-key binding, handshake identity binding, signatures | `IdentityAnnouncementTest`, `NoiseSessionManagerIdentityBindingTest`, `ClientRewritePrimitiveContractTest` | -| Sync/routing | Stable packet IDs, GCS bitstream, replay collapse, TTL handling, relay choice, confirmed graph edges | `ClientRewritePrimitiveContractTest`, `GCSFilterTest`, `PacketRelayManagerTest`, `MeshGraphServiceTest`, `TransportBridgeServiceTest` | -| Nostr | Bech32, secp256k1 key derivation, NIP-01 event IDs/signatures, NIP-44 authenticated encryption, NIP-13 PoW, authenticated NIP-17 seals, 22h outbound envelope randomization | `ClientRewriteNostrContractTest`, `NostrProtocolTest` | +| Identity/security | Announcement extensions, capability bitfield endianness, Noise static-key binding, handshake identity binding, signatures | `IdentityAnnouncementTest`, `NoiseSessionManagerIdentityBindingTest` | +| Sync/routing | Stable packet IDs, GCS bitstream, replay collapse, TTL handling, relay choice, confirmed graph edges | `GCSFilterTest`, `PacketRelayManagerTest`, `MeshGraphServiceTest`, `TransportBridgeServiceTest`, `AppStateStoreTest` | +| Nostr | Bech32, secp256k1 key derivation, NIP-01 event IDs/signatures, NIP-44 authenticated encryption, NIP-13 PoW, authenticated NIP-17 seals, 22h outbound envelope randomization | `NostrProtocolTest` (authenticated-seal cases and outbound timestamp bounds) | | Application state | Peer unions, canonical private conversations, chronological history, delivery/read behavior, media migration policy | `AppStateStoreTest`, `PrivateChatManagerTest`, `MediaSendingManagerMigrationTest` | +The private-history clock-skew case in `AppStateStoreTest` expects arrival +order, which can differ from timestamp order. That test does not establish the +chronological-history requirement above. + ## Golden-vector policy Golden vectors compare literal externally visible bytes or hashes. Do not update @@ -28,22 +34,28 @@ wire protocol is intentionally versioned and interoperating clients are updated together. Round-trip tests remain useful but are not sufficient on their own: an encoder -and decoder can share the same defect. Each critical wire format therefore has -at least one literal vector. +and decoder can share the same defect. Protocol round-trip tests must be paired with literal golden vectors for critical +externally visible formats. Round-trip success alone does not establish compatibility. NIP-17 receivers should reserve safety slack beyond the maximum timestamp -randomization used by senders. Android caps outbound seal and gift-wrap -randomization at 22h, leaving 2 hours of slack inside iOS's 24-hour -subscription window, while retaining its 48-hour receive lookback. +randomization used by senders. Android's default seal and gift-wrap timestamp +randomization is up to 22h. iOS's DM subscription lookback is 24h, a two-hour +difference. `NostrClient.subscribeToPrivateMessages` requests a 48-hour lookback. +These intervals do not establish delivery under arbitrary delays or clock skew. -Geohash relay selection is part of the cross-client contract. Both platforms -read `relays/online_relays_gps.csv` from the bitchat repo, key each row by its -host string (lowercased, an explicit port kept unless it is 443, the wss -default), deduplicate by that key, and order candidates by distance with ties -broken by the same key. Clients that select differently can end up on disjoint -relay sets for the same geohash and silently fail to exchange messages. -`RelayDirectoryTest` covers the Android side; iOS implements the same rules in -`GeoRelayDirectory`. +Geohash directory downloads and the weekly bundled-asset update use +`relays/online_relays_gps.csv` from the bitchat repo, the source used by iOS +`GeoRelayDirectory`. Android deduplicates parsed rows by a lowercase host key, +omits port 443, includes other ports when integer conversion succeeds, and +breaks distance ties by that key. `RelayDirectoryTest` covers these selection rules. + +Matching selections depend on the loaded entries. Bundled and cached copies +can differ between clients; a configured source URL does not establish equal +snapshots or successful delivery. Parsing remains permissive: rows with fewer +than three fields or unparseable coordinates are skipped, and duplicate +endpoints keep the first accepted row. iOS also checks hosts and coordinate ranges +and rejects conflicting duplicate rows. For a host followed by a digit-only port, an integer-conversion failure skips +that row. `RelayDirectoryPortTest` covers oversized ports between valid rows. ## Rewrite acceptance gate @@ -53,15 +65,16 @@ From a configured Android development environment, run: ./gradlew clientRewriteContractTest ``` -The task runs the new golden vectors and the complete existing unit suite. A -rewrite is compatible only when this gate passes. Tests should be ported +The task runs `:app:testDebugUnitTest`, including the wire contract tests. +It must pass before accepting a rewrite. Passing it does not establish Wear or physical-device compatibility. Tests should be ported unchanged when package boundaries change; adapter façades are preferable to weakening assertions. ## Device-only acceptance -Local JVM tests cannot prove Android radio and lifecycle behavior. Before -shipping a rewrite, run the following on at least two physical devices: +Local JVM tests alone do not establish radio operation or lifecycle behavior +on physical Android devices. Before shipping a rewrite, run the following on +at least two physical devices: 1. BLE discovery, connection, disconnect, reconnect, and multi-hop relay. 2. Runtime permission denial/retry for Bluetooth, location, notifications, and