mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-10-06 03:38:23 +00:00
fix: second-pass audit findings on the audience flap
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R1eVeWjMgG8WSU6jKk8d3o
This commit is contained in:
+4
-1
@@ -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,
|
||||
|
||||
+34
-11
@@ -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<HexKey>,
|
||||
hiddenUsers: Set<HexKey>,
|
||||
flagMissingInboxRelay: Boolean,
|
||||
): List<AudienceMember> {
|
||||
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<User>,
|
||||
provenance: Map<HexKey, Set<String>>,
|
||||
fromListTag: String?,
|
||||
currentlyMuted: Set<HexKey> = 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<HexKey>()
|
||||
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<HexKey>()
|
||||
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<User>,
|
||||
val provenance: Map<HexKey, Set<String>>,
|
||||
/** People this add brings into the effective audience, so their bell comes back on. */
|
||||
val unmutes: Set<HexKey> = emptySet(),
|
||||
)
|
||||
|
||||
@Immutable
|
||||
|
||||
+5
-6
@@ -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<AudienceList
|
||||
|
||||
private fun PeopleList.toAudienceList(kind: AudienceListKind) =
|
||||
AudienceList(
|
||||
id = identifierTag,
|
||||
// Qualified by kind: a people list and a follow pack are free to share a
|
||||
// d tag, and provenance keys on this string — an unqualified id would let
|
||||
// one list's chip carry the other's title and remove both batches at once.
|
||||
id = kind.name + ":" + identifierTag,
|
||||
kind = kind,
|
||||
title = title,
|
||||
publicMembers = publicMembersList,
|
||||
|
||||
+10
-4
@@ -303,11 +303,13 @@ private fun NewPostScreenBody(
|
||||
val audience = remember(postViewModel.pTags) { postViewModel.pTags?.toImmutableList() ?: persistentListOf() }
|
||||
val mutedNotifies = remember(postViewModel.mutedNotifies) { postViewModel.mutedNotifies.toImmutableSet() }
|
||||
val groupChips =
|
||||
remember(postViewModel.notifyProvenance, audience, audienceLists) {
|
||||
remember(postViewModel.notifyProvenance, audience, mutedNotifies, audienceLists) {
|
||||
AudienceSelection
|
||||
.activeGroupChips(
|
||||
provenance = postViewModel.notifyProvenance,
|
||||
audience = audience.mapTo(mutableSetOf()) { it.pubkeyHex },
|
||||
// Muted people are in pTags but will not be p-tagged, so
|
||||
// counting them would have the chip over-report its batch.
|
||||
audience = audience.mapNotNullTo(mutableSetOf()) { it.pubkeyHex.takeIf { hex -> 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,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
+31
-13
@@ -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()
|
||||
|
||||
|
||||
@@ -1569,7 +1569,6 @@
|
||||
<string name="audience_badge_already_added">Already added</string>
|
||||
<string name="audience_badge_muted">Muted</string>
|
||||
<string name="audience_badge_private_member">Private member</string>
|
||||
<string name="audience_badge_no_inbox_relay">No inbox relay</string>
|
||||
<string name="audience_recipients_are_visible">Everyone on this note can see the full recipient list.</string>
|
||||
<plurals name="audience_summary_others">
|
||||
<item quantity="one">%1$s, %2$s and %3$d other</item>
|
||||
|
||||
+46
-8
@@ -63,8 +63,7 @@ class AudienceSelectionTest {
|
||||
list: AudienceList,
|
||||
alreadyIn: Set<String> = emptySet(),
|
||||
hidden: Set<String> = 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")
|
||||
|
||||
Reference in New Issue
Block a user