From fbc8e5d1f7221dfb847591bd8f12be563af6f9c4 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 21:14:09 +0000 Subject: [PATCH] fix: second-pass audit findings on the audience flap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven findings, all verified against the code before acting. Crash: AudienceDetail keys its chips by pubkey with no dedup, while the Notifying row it replaces deduped via toSet(). pTags can legitimately repeat a pubkey — the voice-reply branch notifies the parent author on top of the notify list, and none of the three loadFromDraft paths called distinct() — so expanding the flap on such a draft throws a duplicate-key error. Fixed at both ends: the loaders dedupe, and the render guards. Removed the "No inbox relay" badge outright. It read LocalCache once inside a remember with no kind:10050 subscription, so on a cold start every member was badged and the badge never cleared. Worse, it overstated the consequence even when accurate: EventBroadcaster falls back to the recipient's linked relays when a DM relay list is missing, so the wrap is not undeliverable. A warning that fires for everyone and overstates its own severity is worse than none; doing it properly needs a subscription. Provenance: a list that un-mutes somebody recorded nothing for them, because they were not newcomers to pTags — so undoing the group chip removed their batch-mates and left them in a private note's audience. addToAudience now distinguishes "new to pTags" from "new to the effective audience" and claims both. Also: list ids are qualified by kind, since a people list and a follow pack may share a d tag and provenance keys on that string; a member in both the public and encrypted halves of a list is no longer badged as a disclosure risk they are not; the group chip counts only people who will actually be p-tagged, not muted ones; and the lock's haptic reports the direction it moved instead of always ToggleOn. The new tests caught a bug in this very commit: the newcomer filter used a mutating set membership check that reported every incoming pubkey as already known, silently emptying provenance. 21 selection tests (up from 19); full amethyst suite green at 1197. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01R1eVeWjMgG8WSU6jKk8d3o --- .../ui/note/creators/notify/AudienceFlap.kt | 5 +- .../note/creators/notify/AudienceSelection.kt | 45 ++++++++++++---- .../ui/note/creators/notify/AudienceSheet.kt | 11 ++-- .../loggedIn/home/ShortNotePostScreen.kt | 14 +++-- .../loggedIn/home/ShortNotePostViewModel.kt | 44 ++++++++++----- amethyst/src/main/res/values/strings.xml | 1 - .../creators/notify/AudienceSelectionTest.kt | 54 ++++++++++++++++--- 7 files changed, 130 insertions(+), 44 deletions(-) diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceFlap.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceFlap.kt index 220734e192..c8f4ba6079 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceFlap.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceFlap.kt @@ -406,7 +406,10 @@ private fun AudienceDetail( } } - audience.forEach { user -> + // Deduped before keying: Compose throws on a duplicate key, and pTags + // can carry the same pubkey twice (a draft round-trips whatever p tags + // the event had). The Notifying row this replaces deduped via toSet(). + audience.distinctBy { it.pubkeyHex }.forEach { user -> key(user.pubkeyHex) { AudienceMemberChip( user = user, diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSelection.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSelection.kt index 63f4118777..ded2d3ce43 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSelection.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSelection.kt @@ -71,8 +71,6 @@ data class AudienceMember( val isPrivateMember: Boolean = false, /** Already in the composer's audience; shown for a truthful count, not re-added. */ val isAlreadyInAudience: Boolean = false, - /** No NIP-17 DM inbox relay, so a gift wrap may not reach them. */ - val isMissingInboxRelay: Boolean = false, /** Muted or marked as a spammer by this account. */ val isHidden: Boolean = false, ) { @@ -115,15 +113,19 @@ object AudienceSelection { list: AudienceList, alreadyInAudience: Set, hiddenUsers: Set, - flagMissingInboxRelay: Boolean, ): List { + val publicIds = list.publicMembers.mapTo(mutableSetOf()) { it.pubkeyHex } val privateIds = list.privateMembers.mapTo(mutableSetOf()) { it.pubkeyHex } + return list.members().distinctBy { it.pubkeyHex }.map { user -> AudienceMember( user = user, - isPrivateMember = user.pubkeyHex in privateIds, + // Only members that appear *solely* in the encrypted half are a + // disclosure risk. Someone listed in both halves is already + // public, so warning about them would be noise that trains the + // user to ignore the badge that matters. + isPrivateMember = user.pubkeyHex in privateIds && user.pubkeyHex !in publicIds, isAlreadyInAudience = user.pubkeyHex in alreadyInAudience, - isMissingInboxRelay = flagMissingInboxRelay && user.dmInboxRelayList()?.relays()?.isNotEmpty() != true, isHidden = user.pubkeyHex in hiddenUsers, ) } @@ -185,17 +187,36 @@ object AudienceSelection { incoming: Collection, provenance: Map>, fromListTag: String?, + currentlyMuted: Set = emptySet(), ): AudienceAddition { - val known = current.mapTo(mutableSetOf()) { it.pubkeyHex } - val newcomers = incoming.filter { known.add(it.pubkeyHex) } + // Snapshot before filtering: the dedup below adds to its own set, so + // testing membership against that set afterwards would report every + // incoming pubkey as already known. + val existing = current.mapTo(mutableSetOf()) { it.pubkeyHex } - if (fromListTag == null || newcomers.isEmpty()) return AudienceAddition(newcomers, provenance) + val appended = mutableSetOf() + val newcomers = incoming.filter { it.pubkeyHex !in existing && appended.add(it.pubkeyHex) } + + // A muted person is in pTags but not in the audience, so a list that + // contains them genuinely changes the outcome by un-muting them. They + // count as introduced even though they are not new to pTags — otherwise + // undoing the list would leave them silently in a private note's + // audience after every one of their batch-mates had been removed. + val seen = mutableSetOf() + val introduced = + incoming.filter { + val alreadyInEffectiveAudience = it.pubkeyHex in existing && it.pubkeyHex !in currentlyMuted + !alreadyInEffectiveAudience && seen.add(it.pubkeyHex) + } + val unmutes = introduced.mapTo(mutableSetOf()) { it.pubkeyHex } + + if (fromListTag == null || introduced.isEmpty()) return AudienceAddition(newcomers, provenance, unmutes) val next = provenance.toMutableMap() - newcomers.forEach { user -> + introduced.forEach { user -> next[user.pubkeyHex] = (next[user.pubkeyHex] ?: emptySet()) + fromListTag } - return AudienceAddition(newcomers, next) + return AudienceAddition(newcomers, next, unmutes) } /** Members that can be bulk-toggled by "select all" — the already-added rows are locked on. */ @@ -255,9 +276,11 @@ object AudienceSelection { @Immutable data class AudienceAddition( - /** People the add genuinely introduced — the rest were in the audience already. */ + /** People not yet in `pTags` at all — these get appended. */ val newcomers: List, val provenance: Map>, + /** People this add brings into the effective audience, so their bell comes back on. */ + val unmutes: Set = emptySet(), ) @Immutable diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSheet.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSheet.kt index 4c1e3d6068..de9d76a50c 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSheet.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSheet.kt @@ -314,14 +314,11 @@ private fun AudienceReview( val listMaxHeight = (LocalConfiguration.current.screenHeightDp * 0.42f).dp val members = - remember(list, alreadyInAudience, hidden, isPrivate) { + remember(list, alreadyInAudience, hidden) { AudienceSelection.buildMembers( list = list, alreadyInAudience = alreadyInAudience, hiddenUsers = hidden.hiddenUsers + hidden.spammers, - // A public post is not fanned out per recipient, so an inbox - // relay is irrelevant there — flagging it would be noise. - flagMissingInboxRelay = isPrivate, ) } @@ -500,7 +497,6 @@ private fun AudienceMemberRow( member.isAlreadyInAudience -> MemberBadge(R.string.audience_badge_already_added, MaterialTheme.colorScheme.placeholderText) member.isHidden -> MemberBadge(R.string.audience_badge_muted, MaterialTheme.colorScheme.placeholderText) member.isPrivateMember -> MemberBadge(R.string.audience_badge_private_member, MaterialTheme.colorScheme.primary) - member.isMissingInboxRelay -> MemberBadge(R.string.audience_badge_no_inbox_relay, MaterialTheme.colorScheme.warningColor) } } } @@ -537,7 +533,10 @@ fun rememberAudienceLists(accountViewModel: AccountViewModel): List hex !in mutedNotifies } }, lists = audienceLists, ).toImmutableList() } @@ -833,10 +835,14 @@ private fun BottomRowActions( isActive = postViewModel.wantsPrivateNote, isLocked = postViewModel.privateNoteLocked, ) { + val nowPrivate = !postViewModel.wantsPrivateNote postViewModel.togglePrivateNote() // Sealing a note changes what Send is about to do, so the change - // is confirmed in the hand as well as on screen. - haptic.performHapticFeedback(HapticFeedbackType.ToggleOn) + // is confirmed in the hand as well as on screen — and in the + // direction it actually moved. + haptic.performHapticFeedback( + if (nowPrivate) HapticFeedbackType.ToggleOn else HapticFeedbackType.ToggleOff, + ) } } diff --git a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/home/ShortNotePostViewModel.kt b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/home/ShortNotePostViewModel.kt index 40b35dc554..bf33492da1 100644 --- a/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/home/ShortNotePostViewModel.kt +++ b/amethyst/src/main/java/com/vitorpamplona/amethyst/ui/screen/loggedIn/home/ShortNotePostViewModel.kt @@ -417,7 +417,14 @@ open class ShortNotePostViewModel : if (users.isEmpty()) return val current = pTags ?: emptyList() - val addition = AudienceSelection.addToAudience(current, users, notifyProvenance, fromListTag) + val addition = + AudienceSelection.addToAudience( + current = current, + incoming = users, + provenance = notifyProvenance, + fromListTag = fromListTag, + currentlyMuted = mutedNotifies, + ) if (addition.newcomers.isNotEmpty()) { pTags = current + addition.newcomers @@ -425,9 +432,8 @@ open class ShortNotePostViewModel : // Anyone re-added by a list gets their bell back: the list says they // are part of the audience, and a muted chip would silently drop them. - val addedIds = users.mapTo(mutableSetOf()) { it.pubkeyHex } - if (mutedNotifies.any { it in addedIds }) { - mutedNotifies = mutedNotifies - addedIds + if (mutedNotifies.any { it in addition.unmutes }) { + mutedNotifies = mutedNotifies - addition.unmutes } notifyProvenance = addition.provenance @@ -833,9 +839,13 @@ open class ShortNotePostViewModel : } pTags = - draftEvent.tags.filter { it.size > 1 && it[0] == "p" }.mapNotNull { - LocalCache.checkGetOrCreateUser(it[1]) - } + draftEvent.tags + .filter { it.size > 1 && it[0] == "p" } + .mapNotNull { LocalCache.checkGetOrCreateUser(it[1]) } + // A built event can legitimately repeat a p tag (the voice-reply + // branch notifies the parent author on top of the notify list), so + // the audience it round-trips through a draft has to be deduped. + .distinct() draftEvent.tags.filter { it.size > 3 && (it[0] == "e" || it[0] == "a") && it[3] == "fork" }.forEach { val note = LocalCache.checkGetOrCreateNote(it[1]) @@ -939,9 +949,13 @@ open class ShortNotePostViewModel : } pTags = - draftEvent.tags.filter { it.size > 1 && it[0] == "p" }.mapNotNull { - LocalCache.checkGetOrCreateUser(it[1]) - } + draftEvent.tags + .filter { it.size > 1 && it[0] == "p" } + .mapNotNull { LocalCache.checkGetOrCreateUser(it[1]) } + // A built event can legitimately repeat a p tag (the voice-reply + // branch notifies the parent author on top of the notify list), so + // the audience it round-trips through a draft has to be deduped. + .distinct() mutedNotifies = emptySet() notifyProvenance = emptyMap() @@ -1014,9 +1028,13 @@ open class ShortNotePostViewModel : } pTags = - draftEvent.tags.filter { it.size > 1 && it[0] == "p" }.mapNotNull { - LocalCache.checkGetOrCreateUser(it[1]) - } + draftEvent.tags + .filter { it.size > 1 && it[0] == "p" } + .mapNotNull { LocalCache.checkGetOrCreateUser(it[1]) } + // A built event can legitimately repeat a p tag (the voice-reply + // branch notifies the parent author on top of the notify list), so + // the audience it round-trips through a draft has to be deduped. + .distinct() mutedNotifies = emptySet() notifyProvenance = emptyMap() diff --git a/amethyst/src/main/res/values/strings.xml b/amethyst/src/main/res/values/strings.xml index 17dc4110f6..05c7e4aecc 100644 --- a/amethyst/src/main/res/values/strings.xml +++ b/amethyst/src/main/res/values/strings.xml @@ -1569,7 +1569,6 @@ Already added Muted Private member - No inbox relay Everyone on this note can see the full recipient list. %1$s, %2$s and %3$d other diff --git a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSelectionTest.kt b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSelectionTest.kt index 58ddc8f329..d163b805fa 100644 --- a/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSelectionTest.kt +++ b/amethyst/src/test/java/com/vitorpamplona/amethyst/ui/note/creators/notify/AudienceSelectionTest.kt @@ -63,8 +63,7 @@ class AudienceSelectionTest { list: AudienceList, alreadyIn: Set = emptySet(), hidden: Set = emptySet(), - flagInbox: Boolean = false, - ) = AudienceSelection.buildMembers(list, alreadyIn, hidden, flagInbox) + ) = AudienceSelection.buildMembers(list, alreadyIn, hidden) @Test fun ordinaryMembersStartSelected() { @@ -111,13 +110,15 @@ class AudienceSelectionTest { } @Test - fun missingInboxRelayIsOnlyFlaggedForPrivateNotes() { - val public = members(listOfPeople(public = listOf(alice)), flagInbox = false) - assertFalse(public.single().isMissingInboxRelay) + fun someoneInBothHalvesOfAListIsNotTreatedAsPrivate() { + // Carla is in the encrypted half but also publicly listed, so adding her + // discloses nothing new. Warning about her would be noise that trains the + // user to ignore the badge that matters. + val rows = members(listOfPeople(public = listOf(alice, carla), private = listOf(carla))) + val carlaRow = rows.first { it.pubkeyHex == carla.pubkeyHex } - // A user with no loaded relay list cannot be shown to have an inbox. - val private = members(listOfPeople(public = listOf(alice)), flagInbox = true) - assertTrue(private.single().isMissingInboxRelay) + assertFalse(carlaRow.isPrivateMember) + assertTrue(carla.pubkeyHex in AudienceSelection.defaultSelection(rows)) } @Test @@ -190,6 +191,43 @@ class AudienceSelectionTest { assertFalse(alice.pubkeyHex in removal.orphaned) } + @Test + fun aListThatUnMutesSomebodyClaimsThemToo() { + // Bruno is in pTags but muted, so he is not in the audience. The list + // un-mutes him, which genuinely changes the outcome — so undoing the list + // has to be able to take him back out again. + val addition = + AudienceSelection.addToAudience( + current = listOf(alice, bruno), + incoming = listOf(bruno), + provenance = emptyMap(), + fromListTag = "close-friends", + currentlyMuted = setOf(bruno.pubkeyHex), + ) + + assertTrue(addition.newcomers.isEmpty()) + assertEquals(setOf(bruno.pubkeyHex), addition.unmutes) + assertEquals(setOf("close-friends"), addition.provenance[bruno.pubkeyHex]) + + val removal = AudienceSelection.removeListFromProvenance(addition.provenance, "close-friends") + assertEquals(setOf(bruno.pubkeyHex), removal.orphaned) + } + + @Test + fun anUnmutedPersonAlreadyInTheAudienceIsNotClaimed() { + val addition = + AudienceSelection.addToAudience( + current = listOf(alice), + incoming = listOf(alice), + provenance = emptyMap(), + fromListTag = "close-friends", + currentlyMuted = emptySet(), + ) + + assertTrue(addition.unmutes.isEmpty()) + assertTrue(addition.provenance.isEmpty()) + } + @Test fun addingTheSameListTwiceDoesNotDuplicateProvenance() { val first = AudienceSelection.addToAudience(emptyList(), listOf(alice), emptyMap(), "work")