mirror of
https://github.com/permissionlesstech/bitchat-android.git
synced 2026-09-19 04:59:59 +00:00
Three review findings, all confirmed and all mine
Codex raised three P1s on the first revision. I checked each against the code before touching anything; all three are real, and the first two are the same mistake in two costumes — one predicate asked two different questions. 1. An announced suffix is not a UI decoration. `sealApplies` stripped a trailing `#abcd` from whatever it was given, including the LIVE announced nickname. A peer announces whatever string it likes, and `#` plus four hex is a legal thing to announce — this app's own `splitSuffix` exists because announced names carry them. So the single predicate was wrong in both directions at once: a key pinned as `medic` could rename to `medic#cafe` and keep its seal, and a key honestly verified as `medic#cafe` lost its seal without renaming at all. There are now two functions, `sealAppliesToAnnounced` and `sealAppliesToRendered`, because there are two questions: what a peer claims, and what a row shows after the list may have decorated it. The rendered one tries the raw name before undecorating, so a name that genuinely ends in a suffix still matches itself. The iOS patch has exactly this split and I collapsed it here. 2. The check was not reactive. `PeopleSection` caches `isPeerVerified` with `remember(verifiedFingerprints, peerFingerprints, connectedPeers)`, and the per-peer sheet with `remember(peerID, verifiedFingerprints)`. My gate reads the announced nickname inside those lambdas, and a rename changes none of those keys — so the NAME repainted while the cached `true`, and the seal, survived until something unrelated invalidated the cache. That is the connected peer still showing a seal after renaming: the live impersonation, and the single case this patch exists to stop. `peerNicknames` is now a key at both sites. 3. The watch was never patched at all. There is a `:wear` module in settings.gradle.kts with its own `WearPeerIdentityState`, its own verify button and its own seal, and it calls `setVerifiedFingerprint` without a nickname while deriving `isVerified` from the fingerprint set alone. Every peer verified on a watch failed open for ever. I missed it because I audited `app/` and assumed that was the app — I never read settings.gradle.kts. It now pins the announced name on verify and applies the binding in `snapshot`. 15 binding cases, up from 13. Mutation-checked: put the stripping back into the announced check and the new case fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Shubham Bhandari
parent
351f8861c7
commit
a01fff5aed
@ -63,19 +63,49 @@ object NicknameBinding {
|
||||
return name.substring(0, name.length - 5)
|
||||
}
|
||||
|
||||
/** Two nicknames that are the same name, by [bindingKey]. */
|
||||
private fun sameName(a: String, b: String): Boolean = bindingKey(a) == bindingKey(b)
|
||||
|
||||
/**
|
||||
* Does a seal earned under [pinned] still apply to a peer announcing
|
||||
* [current]?
|
||||
* Does a seal earned under [pinned] still apply to a peer ANNOUNCING
|
||||
* [announced]?
|
||||
*
|
||||
* Fails **open** when nothing was pinned. Peers verified by builds from
|
||||
* before this existed have no baseline, and dropping their seals on upgrade
|
||||
* would teach people to ignore the signal — which costs more than the
|
||||
* narrow case it would catch.
|
||||
* No suffix stripping. What a peer announces is its own string, and a `#`
|
||||
* plus four hex is a perfectly legal thing to announce — this app's own
|
||||
* `splitSuffix` exists because announced names carry them. Stripping here
|
||||
* would let a key pinned as `medic` rename to `medic#cafe` and keep its
|
||||
* seal, and would drop the seal of a key honestly verified as `medic#cafe`.
|
||||
*
|
||||
* Fails **open** when nothing was pinned, or when there is no announced
|
||||
* name yet. Peers verified by builds from before this existed have no
|
||||
* baseline, and dropping their seals on upgrade would teach people to
|
||||
* ignore the signal — which costs more than the narrow case it would catch.
|
||||
*/
|
||||
fun sealApplies(pinned: String?, current: String?): Boolean {
|
||||
if (pinned.isNullOrEmpty()) return true
|
||||
val shown = withoutCollisionSuffix(current.orEmpty())
|
||||
if (shown.isEmpty()) return true
|
||||
return bindingKey(pinned) == bindingKey(shown)
|
||||
fun sealAppliesToAnnounced(pinned: String?, announced: String?): Boolean {
|
||||
if (pinned.isNullOrEmpty() || announced.isNullOrEmpty()) return true
|
||||
return sameName(pinned, announced)
|
||||
}
|
||||
|
||||
/**
|
||||
* Does a seal earned under [pinned] still apply beside [rendered] — a name
|
||||
* as it appears on a row, which the list may have decorated?
|
||||
*
|
||||
* Two peers claiming one nickname are told apart with a `#abcd` the UI
|
||||
* appends, so comparing the decorated string alone would drop the seal of
|
||||
* the peer being impersonated at exactly the moment it matters. A raw match
|
||||
* is tried first, so a name that genuinely ends in a suffix still matches
|
||||
* itself; only then is one suffix removed.
|
||||
*
|
||||
* This is deliberately a SECOND function rather than a flag. The first
|
||||
* version of this patch had one, stripping unconditionally, and it was
|
||||
* wrong in both directions at once — which is what happens when one
|
||||
* predicate is asked two questions.
|
||||
*/
|
||||
fun sealAppliesToRendered(pinned: String?, rendered: String?): Boolean {
|
||||
if (pinned.isNullOrEmpty() || rendered.isNullOrEmpty()) return true
|
||||
if (sameName(pinned, rendered)) return true
|
||||
val undecorated = withoutCollisionSuffix(rendered)
|
||||
if (undecorated.isEmpty()) return true
|
||||
return sameName(pinned, undecorated)
|
||||
}
|
||||
}
|
||||
|
||||
@ -286,8 +286,8 @@ class SecureIdentityStateManager {
|
||||
* before this existed have none, and dropping their seals on upgrade would
|
||||
* teach people to ignore the signal.
|
||||
*/
|
||||
fun verifiedNicknameMismatch(fingerprint: String, currentNickname: String?): Boolean =
|
||||
!NicknameBinding.sealApplies(getVerifiedNickname(fingerprint), currentNickname)
|
||||
fun verifiedNicknameMismatch(fingerprint: String, announcedNickname: String?): Boolean =
|
||||
!NicknameBinding.sealAppliesToAnnounced(getVerifiedNickname(fingerprint), announcedNickname)
|
||||
|
||||
private fun pinVerifiedNickname(fingerprint: String, nickname: String) {
|
||||
val key = fingerprint.lowercase()
|
||||
|
||||
@ -709,7 +709,14 @@ fun PeopleSection(
|
||||
}
|
||||
}
|
||||
|
||||
val peerVerifiedStates = remember(verifiedFingerprints, peerFingerprints, connectedPeers) {
|
||||
// `peerNicknames` is a key because the verification check now reads the
|
||||
// peer's announced name. Without it a rename repaints the NAME from the
|
||||
// state flow while this cached `true` — and the seal — survives until
|
||||
// some unrelated key happens to change. That is the live impersonation
|
||||
// still showing a seal, which is the whole case this is meant to stop.
|
||||
val peerVerifiedStates = remember(
|
||||
verifiedFingerprints, peerFingerprints, connectedPeers, peerNicknames
|
||||
) {
|
||||
connectedPeers.associateWith { peerID ->
|
||||
viewModel.isPeerVerified(peerID, verifiedFingerprints)
|
||||
}
|
||||
@ -1790,7 +1797,8 @@ fun PrivateChatSheet(
|
||||
}
|
||||
}
|
||||
|
||||
val isVerified = remember(peerID, verifiedFingerprints) {
|
||||
// Keyed on the announced nickname as well — see PeopleSection above.
|
||||
val isVerified = remember(peerID, verifiedFingerprints, peerNicknames[peerID]) {
|
||||
viewModel.isPeerVerified(peerID, verifiedFingerprints)
|
||||
}
|
||||
|
||||
|
||||
@ -383,7 +383,8 @@ class VerificationHandler(
|
||||
* them: the row has to be checked against its own name.
|
||||
*/
|
||||
fun sealAppliesToName(fingerprint: String, renderedName: String?): Boolean =
|
||||
NicknameBinding.sealApplies(identityManager.getVerifiedNickname(fingerprint), renderedName)
|
||||
NicknameBinding.sealAppliesToRendered(
|
||||
identityManager.getVerifiedNickname(fingerprint), renderedName)
|
||||
|
||||
private fun resolvePeerDisplayName(peerID: String): String {
|
||||
val nick = try { meshService.getPeerInfo(peerID)?.nickname } catch (_: Exception) { null }
|
||||
|
||||
@ -22,12 +22,12 @@ class NicknameBindingTest {
|
||||
// Eve is verified while announcing "ravi", then announces "medic". Every
|
||||
// device that trusts the real medic would otherwise show a second
|
||||
// trusted-looking medic.
|
||||
assertFalse(NicknameBinding.sealApplies("ravi", "medic"))
|
||||
assertFalse(NicknameBinding.sealAppliesToAnnounced("ravi", "medic"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `the seal stands while the name is unchanged`() {
|
||||
assertTrue(NicknameBinding.sealApplies("ravi", "ravi"))
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi", "ravi"))
|
||||
}
|
||||
|
||||
// ---- what counts as the same name -------------------------------------
|
||||
@ -36,8 +36,8 @@ class NicknameBindingTest {
|
||||
fun `recasing your own nickname is not a rename`() {
|
||||
// A rename is meant to break the binding; a recase is not. Without the
|
||||
// case fold, changing "Ravi" to "ravi" silently dropped the seal.
|
||||
assertTrue(NicknameBinding.sealApplies("Ravi", "ravi"))
|
||||
assertTrue(NicknameBinding.sealApplies("ravi", "RAVI"))
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced("Ravi", "ravi"))
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi", "RAVI"))
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -45,7 +45,7 @@ class NicknameBindingTest {
|
||||
val precomposed = "José" // José
|
||||
val decomposed = "José" // Jose + combining acute
|
||||
assertEquals(NicknameBinding.bindingKey(precomposed), NicknameBinding.bindingKey(decomposed))
|
||||
assertTrue(NicknameBinding.sealApplies(precomposed, decomposed))
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced(precomposed, decomposed))
|
||||
}
|
||||
|
||||
@Test
|
||||
@ -63,9 +63,9 @@ class NicknameBindingTest {
|
||||
// so it is a different name and must break the binding — folding
|
||||
// look-alikes is a different question, and answering it here would let
|
||||
// a verification for one name quietly cover another.
|
||||
assertFalse(NicknameBinding.sealApplies("Medic", "Ｍedic"))
|
||||
assertFalse(NicknameBinding.sealAppliesToAnnounced("Medic", "Ｍedic"))
|
||||
// ...and so does a Cyrillic М.
|
||||
assertFalse(NicknameBinding.sealApplies("Medic", "Меdic"))
|
||||
assertFalse(NicknameBinding.sealAppliesToAnnounced("Medic", "Меdic"))
|
||||
}
|
||||
|
||||
// ---- the collision suffix ---------------------------------------------
|
||||
@ -75,7 +75,7 @@ class NicknameBindingTest {
|
||||
// Two peers claiming one nickname render as "medic#a1b2" and
|
||||
// "medic#c3d4". Comparing the decorated string would drop the seal of
|
||||
// the peer being impersonated, at exactly the moment it matters most.
|
||||
assertTrue(NicknameBinding.sealApplies("medic", "medic#a1b2"))
|
||||
assertTrue(NicknameBinding.sealAppliesToRendered("medic", "medic#a1b2"))
|
||||
assertEquals("medic", NicknameBinding.withoutCollisionSuffix("medic#a1b2"))
|
||||
}
|
||||
|
||||
@ -98,9 +98,9 @@ class NicknameBindingTest {
|
||||
// splitSuffix strips every "@" in the string — right for parsing a
|
||||
// mention, wrong for comparing a name, since "ravi@hq" would then
|
||||
// compare unequal to itself.
|
||||
assertTrue(NicknameBinding.sealApplies("ravi@hq", "ravi@hq"))
|
||||
assertTrue(NicknameBinding.sealApplies("ravi@hq", "ravi@hq#a1b2"))
|
||||
assertFalse(NicknameBinding.sealApplies("ravi@hq", "ravi"))
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi@hq"))
|
||||
assertTrue(NicknameBinding.sealAppliesToRendered("ravi@hq", "ravi@hq#a1b2"))
|
||||
assertFalse(NicknameBinding.sealAppliesToAnnounced("ravi@hq", "ravi"))
|
||||
}
|
||||
|
||||
// ---- failing open ------------------------------------------------------
|
||||
@ -110,17 +110,47 @@ class NicknameBindingTest {
|
||||
// Peers verified by builds from before this existed have no baseline.
|
||||
// Dropping their seals on upgrade would teach people to ignore the
|
||||
// signal, which costs more than the narrow case it would catch.
|
||||
assertTrue(NicknameBinding.sealApplies(null, "medic"))
|
||||
assertTrue(NicknameBinding.sealApplies("", "medic"))
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced(null, "medic"))
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced("", "medic"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `an empty current name never suppresses`() {
|
||||
// A row with no name to show yet is not evidence of a rename.
|
||||
assertTrue(NicknameBinding.sealApplies("medic", null))
|
||||
assertTrue(NicknameBinding.sealApplies("medic", ""))
|
||||
// ...including one that is nothing BUT a suffix.
|
||||
assertTrue(NicknameBinding.sealApplies("medic", "#a1b2"))
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", null))
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced("medic", ""))
|
||||
// A row whose name is nothing BUT a decoration leaves nothing to
|
||||
// compare, so it fails open too. Note this is a RENDERED-name case: a
|
||||
// peer that actually announces "#a1b2" has renamed, and says so.
|
||||
assertTrue(NicknameBinding.sealAppliesToRendered("medic", "#a1b2"))
|
||||
assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "#a1b2"))
|
||||
}
|
||||
|
||||
// ---- announced vs rendered: the split that was missing -----------------
|
||||
|
||||
@Test
|
||||
fun `an announced suffix is not a UI decoration`() {
|
||||
// The first version of this patch stripped a trailing #abcd from the
|
||||
// LIVE announced name, which is wrong in both directions at once. A
|
||||
// peer announces whatever string it likes, and "#" plus four hex is a
|
||||
// legal thing to announce — this app's own splitSuffix exists because
|
||||
// announced names carry them.
|
||||
//
|
||||
// Stripping let the attack straight through...
|
||||
assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "medic#cafe"))
|
||||
// ...and dropped the seal of a key honestly verified under a name that
|
||||
// simply ends that way.
|
||||
assertTrue(NicknameBinding.sealAppliesToAnnounced("medic#cafe", "medic#cafe"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun `a rendered row tries the raw name before undecorating it`() {
|
||||
// On a row the list may have appended #abcd to tell two namesakes
|
||||
// apart, so that has to come off — but only after the raw name has had
|
||||
// its chance, or a name that genuinely ends in a suffix loses its seal.
|
||||
assertTrue(NicknameBinding.sealAppliesToRendered("medic", "medic#a1b2"))
|
||||
assertTrue(NicknameBinding.sealAppliesToRendered("medic#cafe", "medic#cafe"))
|
||||
assertFalse(NicknameBinding.sealAppliesToRendered("medic", "zebra#a1b2"))
|
||||
}
|
||||
|
||||
// ---- the key itself ----------------------------------------------------
|
||||
@ -135,8 +165,8 @@ class NicknameBindingTest {
|
||||
|
||||
@Test
|
||||
fun `unrelated names do not match`() {
|
||||
assertFalse(NicknameBinding.sealApplies("medic", "zebra"))
|
||||
assertFalse(NicknameBinding.sealApplies("medic", "medic2"))
|
||||
assertFalse(NicknameBinding.sealApplies("medic", "medi"))
|
||||
assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "zebra"))
|
||||
assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "medic2"))
|
||||
assertFalse(NicknameBinding.sealAppliesToAnnounced("medic", "medi"))
|
||||
}
|
||||
}
|
||||
|
||||
@ -4,6 +4,7 @@ import android.content.Context
|
||||
import com.bitchat.android.favorites.FavoriteRelationship
|
||||
import com.bitchat.android.favorites.FavoritesChangeListener
|
||||
import com.bitchat.android.favorites.FavoritesPersistenceService
|
||||
import com.bitchat.android.identity.NicknameBinding
|
||||
import com.bitchat.android.identity.SecureIdentityStateManager
|
||||
import com.bitchat.watch.mesh.WearMeshService
|
||||
import kotlinx.coroutines.flow.MutableStateFlow
|
||||
@ -67,10 +68,18 @@ object WearPeerIdentityState : FavoritesChangeListener {
|
||||
val relationship = relationship(peerID, mesh)
|
||||
val fingerprint = mesh?.getPeerFingerprint(peerID)
|
||||
?: relationship?.peerNoisePublicKey?.let(identityManager::generateFingerprint)
|
||||
// The watch draws the same seal as the phone and has to bind it the same
|
||||
// way: a key verified here can otherwise rename onto a nickname the
|
||||
// wearer trusts and keep the glyph. The announced name comes from the
|
||||
// watch's own mesh, so the check is the live one.
|
||||
val announced = mesh?.getPeerNickname(peerID)
|
||||
?: mesh?.getPeerInfo(peerID)?.nickname?.takeIf(String::isNotBlank)
|
||||
val isVerified = fingerprint != null &&
|
||||
identityManager.getVerifiedFingerprints().any {
|
||||
it.equals(fingerprint, ignoreCase = true)
|
||||
}
|
||||
} &&
|
||||
NicknameBinding.sealAppliesToAnnounced(
|
||||
identityManager.getVerifiedNickname(fingerprint), announced)
|
||||
val isFavorite = relationship?.isFavorite == true
|
||||
val theyFavoritedUs = relationship?.theyFavoritedUs == true
|
||||
return WearPeerIdentitySnapshot(
|
||||
@ -110,7 +119,11 @@ object WearPeerIdentityState : FavoritesChangeListener {
|
||||
): Boolean {
|
||||
check(initialized) { "WearPeerIdentityState must be initialized by the application" }
|
||||
val fingerprint = snapshot(peerID, mesh).fingerprint ?: return false
|
||||
identityManager.setVerifiedFingerprint(fingerprint.lowercase(), verified)
|
||||
// Pin the name this key is announcing, exactly as the phone does.
|
||||
// Without it every peer verified on a watch would fail open for ever.
|
||||
val announced = mesh?.getPeerNickname(peerID)
|
||||
?: mesh?.getPeerInfo(peerID)?.nickname?.takeIf(String::isNotBlank)
|
||||
identityManager.setVerifiedFingerprint(fingerprint.lowercase(), verified, announced)
|
||||
publishChange()
|
||||
return true
|
||||
}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user