From d637b5bc1bd6575340632d4066b508121322eac0 Mon Sep 17 00:00:00 2001 From: Shubham Bhandari Date: Tue, 15 Sep 2026 11:32:47 +0800 Subject: [PATCH] Demote the sheet's copy with its glyph, and drop a fail-open default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both of @Chessing234's follow-ups, and both were right. The sheet kept saying "verified". I gated the glyph and the tint and argued the word should stay because the key genuinely is verified. That status sits directly beside `displayName`, so it is not a claim about a bare key — it is a claim about THIS NAME, and that claim is false. And it is the screen someone opens *because* a seal vanished from a row, so the one surface they consult to resolve the doubt was reassuring them. `buildStatusInfo`'s text now demotes with the glyph, and so does `SecurityVerificationActions` — which was a SECOND seal in the same sheet, green `Icons.Filled.Verified` with `fingerprint_verified_label`, that the first revision never gated at all. No new string is needed, which was the objection I had to fixing this: an established session falls through to "encrypted" with a padlock, true and silent about identity, and the not-verified block below already names the current nickname in `fingerprint_not_verified_message_fmt` and offers to verify it. That offer is the recovery — re-verifying re-pins the baseline to the name on screen — and the stored verification is untouched underneath. The consequence, said out loud: a renamed peer's sheet now offers "verify" rather than "remove verification", so unverify is one re-verification away instead of immediately to hand. Better than a green checkmark beside a name nobody verified, but a real change to the affordance. `isNoisePublicKeyVerified(..., renderedName = null)` fail-opened, so any future caller that simply omitted the name would have reinstated the rename attack, silently and without touching that file. The default is gone on both the ChatViewModel and the handler; the single caller already passes the name. 15 binding cases still green. Co-Authored-By: Claude Opus 5 --- .../com/bitchat/android/ui/ChatViewModel.kt | 7 ++++- .../android/ui/SecurityVerificationSheet.kt | 26 ++++++++++++------- .../bitchat/android/ui/VerificationHandler.kt | 2 +- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt b/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt index 76ee18fc..faf8fe0e 100644 --- a/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt +++ b/app/src/main/java/com/bitchat/android/ui/ChatViewModel.kt @@ -1276,11 +1276,16 @@ class ChatViewModel( * rows display a name held in the favourites record rather than a live * announce, so they must be checked against their own name — asking about * a "current" name a peer is not announcing would answer nothing. + * + * Deliberately has no default. A missing name fails OPEN, by design, so a + * defaulted parameter would let any future caller reinstate the rename + * attack by simply not passing one — silently, and without touching this + * file. Make it a decision at the call site. */ fun isNoisePublicKeyVerified( noisePublicKey: ByteArray, verifiedFingerprints: Set, - renderedName: String? = null, + renderedName: String?, ): Boolean { val fingerprint = verificationHandler.fingerprintFromNoiseBytes(noisePublicKey) if (!verifiedFingerprints.contains(fingerprint)) return false diff --git a/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt b/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt index 1cad119e..07714e66 100644 --- a/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt +++ b/app/src/main/java/com/bitchat/android/ui/SecurityVerificationSheet.kt @@ -146,7 +146,7 @@ fun SecurityVerificationSheet( ) SecurityVerificationActions( - isVerified = isVerified, + isVerified = nameBound && isVerified, fingerprint = fingerprint, displayName = displayName, accent = accent, @@ -190,16 +190,24 @@ private fun buildStatusInfo( sessionState: String?, accent: Color ): SecurityStatusInfo { - // The glyph is the seal; the text is the explanation. They part company for - // exactly one state: verified key, different name. The icon and tint fall - // back to the session's own state (a padlock on an encrypted session), so - // the sheet stops asserting the name while the word "verified" still tells - // the truth about the key. Adding a sentence that says both would need a - // new string in all 34 locales, and machine-translating a security warning - // is not something to do in passing. + // Everything here demotes together: glyph, tint and text. + // + // The first revision kept the word "verified" on the grounds that the key + // genuinely is verified, and that was wrong for a reason review put better + // than I had: this status sits directly beside `displayName`, so it is not + // a claim about a bare key, it is a claim about THIS NAME — and that claim + // is false. Worse, it is the screen someone opens *because* a seal vanished + // from a row, so the one surface they consult to resolve the doubt was the + // one endorsing the rename. + // + // It needs no new string. An established session falls through to + // "encrypted" with a padlock, which is true and asserts nothing about + // identity, and the actions block below already names the current nickname + // in its not-verified copy and offers to verify it — which is exactly the + // recovery: re-verifying re-pins the baseline to the name on screen. val sealed = isVerified && nameBound val text = when { - isVerified -> stringResource(R.string.fingerprint_status_verified) + sealed -> stringResource(R.string.fingerprint_status_verified) sessionState == "established" -> stringResource(R.string.fingerprint_status_encrypted) sessionState == "handshaking" -> stringResource(R.string.fingerprint_status_handshaking) sessionState == "failed" -> stringResource(R.string.fingerprint_status_failed) diff --git a/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt b/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt index c73287cb..4c0cc725 100644 --- a/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt +++ b/app/src/main/java/com/bitchat/android/ui/VerificationHandler.kt @@ -60,7 +60,7 @@ class VerificationHandler( return !verifiedNicknameMismatch(peerID) } - fun isNoisePublicKeyVerified(noisePublicKey: ByteArray, renderedName: String? = null): Boolean { + fun isNoisePublicKeyVerified(noisePublicKey: ByteArray, renderedName: String?): Boolean { val fingerprint = fingerprintFromNoiseBytes(noisePublicKey) if (!_verifiedFingerprints.value.contains(fingerprint)) return false return sealAppliesToName(fingerprint, renderedName)