From 8c37e6b2d46fd697f0d983e719006e85606d97f3 Mon Sep 17 00:00:00 2001 From: Cody Henthorne Date: Tue, 25 Aug 2026 12:32:27 -0400 Subject: [PATCH] Prevent storage sync loop on PNI-only records with different identity keys. --- .../securesms/database/RecipientTable.kt | 13 +- .../storage/ContactRecordProcessor.kt | 35 ++- .../storage/DefaultStorageRecordProcessor.kt | 9 +- .../securesms/storage/StorageSyncModels.kt | 2 +- .../storage/ContactRecordProcessorTest.kt | 263 ++++++++++++++++++ 5 files changed, 305 insertions(+), 17 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt index 40ad838dc2..73642b06af 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/database/RecipientTable.kt @@ -989,9 +989,10 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da try { val oldIdentityRecord = identityStore.getIdentityRecord(recipientId) - if (update.new.proto.identityKey.isNotEmpty() && update.new.proto.signalAci != null) { + if (update.new.proto.identityKey.isNotEmpty() && (update.new.proto.signalAci != null || update.new.proto.signalPni != null)) { + val serviceId: ServiceId = update.new.proto.signalAci ?: update.new.proto.signalPni!! val identityKey = IdentityKey(update.new.proto.identityKey.toByteArray(), 0) - identities.updateIdentityAfterSync(update.new.proto.signalAci!!.toString(), recipientId, identityKey, StorageSyncModels.remoteToLocalIdentityStatus(update.new.proto.identityState)) + identities.updateIdentityAfterSync(serviceId.toString(), recipientId, identityKey, StorageSyncModels.remoteToLocalIdentityStatus(update.new.proto.identityState)) } val newIdentityRecord = identityStore.getIdentityRecord(recipientId) @@ -2599,7 +2600,8 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da REGISTERED to RegisteredState.NOT_REGISTERED.id, UNREGISTERED_TIMESTAMP to System.currentTimeMillis(), E164 to null, - PNI_COLUMN to null + PNI_COLUMN to null, + PNI_SIGNATURE_VERIFIED to 0 ) if (update(id, contentValues)) { @@ -2659,7 +2661,8 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da .update(TABLE_NAME) .values( PNI_COLUMN to null, - E164 to null + E164 to null, + PNI_SIGNATURE_VERIFIED to 0 ) .where("$ID = ?", record.id) .run() @@ -4478,7 +4481,7 @@ open class RecipientTable(context: Context, databaseHelper: SignalDatabase) : Da put(MUTE_UNTIL, contact.proto.mutedUntilTimestamp) put(STORAGE_SERVICE_ID, Base64.encodeWithPadding(contact.id.raw)) put(HIDDEN, contact.proto.hidden) - put(PNI_SIGNATURE_VERIFIED, contact.proto.pniSignatureVerified.toInt()) + put(PNI_SIGNATURE_VERIFIED, (contact.proto.pniSignatureVerified && contact.proto.signalPni?.isValid == true).toInt()) put(NICKNAME_GIVEN_NAME, nickname.givenName.nullIfBlank()) put(NICKNAME_FAMILY_NAME, nickname.familyName.nullIfBlank()) put(NICKNAME_JOINED_NAME, nickname.toString().nullIfBlank()) diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.kt b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.kt index 412c9d4821..536e65b4fa 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.kt @@ -13,9 +13,9 @@ import org.thoughtcrime.securesms.crypto.ProfileKeyUtil import org.thoughtcrime.securesms.database.RecipientTable import org.thoughtcrime.securesms.database.SignalDatabase import org.thoughtcrime.securesms.database.model.RecipientRecord -import org.thoughtcrime.securesms.jobs.RetrieveProfileJob.Companion.enqueue +import org.thoughtcrime.securesms.jobs.RetrieveProfileJob import org.thoughtcrime.securesms.keyvalue.SignalStore -import org.thoughtcrime.securesms.recipients.Recipient.Companion.trustedPush +import org.thoughtcrime.securesms.recipients.Recipient import org.thoughtcrime.securesms.recipients.RecipientId import org.thoughtcrime.securesms.storage.StorageSyncModels.localToRemoteRecord import org.whispersystems.signalservice.api.storage.SignalContactRecord @@ -118,6 +118,10 @@ class ContactRecordProcessor( } } + override fun describeRecord(record: SignalContactRecord): String { + return "[${record.proto.signalAci ?: record.proto.signalPni}]" + } + override fun getMatching(remote: SignalContactRecord, keyGenerator: StorageKeyGenerator): Optional { var found: Optional = remote.proto.signalAci?.let { recipientTable.getByAci(it) } ?: Optional.empty() @@ -166,9 +170,14 @@ class ContactRecordProcessor( val mergedIdentityState: IdentityState val mergedIdentityKey: ByteArray? + val identityKeysExistsAndConflict = remote.proto.identityKey.isNotEmpty() && local.proto.identityKey.isNotEmpty() && remote.proto.identityKey != local.proto.identityKey + val conflictAci = localAci ?: remoteAci + val unrepairableIdentityKeyConflict = identityKeysExistsAndConflict && conflictAci == null + if ((remote.proto.identityState != local.proto.identityState && remote.proto.identityKey.isNotEmpty()) || (remote.proto.identityKey.isNotEmpty() && local.proto.identityKey.isEmpty()) || - (remote.proto.identityKey.isNotEmpty() && local.proto.unregisteredAtTimestamp > 0) + (remote.proto.identityKey.isNotEmpty() && local.proto.unregisteredAtTimestamp > 0) || + (unrepairableIdentityKeyConflict && !SignalStore.account.isPrimaryDevice) ) { mergedIdentityState = remote.proto.identityState mergedIdentityKey = remote.proto.identityKey.takeIf { it.isNotEmpty() }?.toByteArray() @@ -177,11 +186,6 @@ class ContactRecordProcessor( mergedIdentityKey = local.proto.identityKey.takeIf { it.isNotEmpty() }?.toByteArray() } - if (localAci != null && mergedIdentityKey != null && remote.proto.identityKey.isNotEmpty() && !mergedIdentityKey.contentEquals(remote.proto.identityKey.toByteArray())) { - Log.w(TAG, "The local and remote identity keys do not match for " + localAci + ". Enqueueing a profile fetch.") - enqueue(trustedPush(localAci, localPni, local.proto.e164).id, true) - } - val mergedPni: PNI? val mergedE164: String? @@ -224,6 +228,19 @@ class ContactRecordProcessor( mergedE164 = remote.proto.e164.nullIfBlank() ?: local.proto.e164.nullIfBlank() } + if (identityKeysExistsAndConflict) { + if (conflictAci != null) { + Log.w(TAG, "Identity keys conflict for $conflictAci. Enqueueing a profile fetch.") + SignalDatabase.runPostSuccessfulTransaction { + RetrieveProfileJob.enqueue(Recipient.trustedPush(conflictAci, mergedPni, mergedE164).id, true) + } + } else { + Log.w(TAG, "Identity keys conflict for $localPni. No ACI, so no profile fetch is possible.") + } + } else if (mergedIdentityKey != null && remote.proto.identityKey.isEmpty()) { + Log.w(TAG, "Remote identity key is missing for ${localAci ?: localPni}. Keeping ours.") + } + val merged = SignalContactRecord.newBuilder(remote.serializedUnknowns).apply { e164 = mergedE164 ?: "" aciBinary = local.proto.aciBinary.nullIfEmpty() ?: remote.proto.aciBinary @@ -249,7 +266,7 @@ class ContactRecordProcessor( systemFamilyName = if (SignalStore.account.isPrimaryDevice) local.proto.systemFamilyName else remote.proto.systemFamilyName systemNickname = remote.proto.systemNickname nickname = remote.proto.nickname - pniSignatureVerified = remote.proto.pniSignatureVerified || local.proto.pniSignatureVerified + pniSignatureVerified = (remote.proto.pniSignatureVerified || local.proto.pniSignatureVerified) && mergedPni?.isValid == true note = remote.proto.note.nullIfBlank() ?: "" avatarColor = if (SignalStore.account.isPrimaryDevice) local.proto.avatarColor else remote.proto.avatarColor }.build().toSignalContactRecord(StorageId.forContact(keyGenerator.generate())) diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/DefaultStorageRecordProcessor.kt b/app/src/main/java/org/thoughtcrime/securesms/storage/DefaultStorageRecordProcessor.kt index e172eab555..bfd5fb4ad6 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/DefaultStorageRecordProcessor.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/DefaultStorageRecordProcessor.kt @@ -74,12 +74,17 @@ abstract class DefaultStorageRecordProcessor> : StorageRecor return base.serializedUnknowns.contentEquals(test.serializedUnknowns) && base.proto == test.proto } + /** + * Optional extra identifying detail about a record, included in every log line for it. + */ + open fun describeRecord(record: E): String = "" + private fun info(i: Int, record: E, message: String) { - Log.i(TAG, "[$i][${record.javaClass.getSimpleName()}] $message") + Log.i(TAG, "[$i][${record.javaClass.simpleName}]${describeRecord(record)} $message") } private fun warn(i: Int, record: E, message: String) { - Log.w(TAG, "[$i][${record.javaClass.getSimpleName()}] $message") + Log.w(TAG, "[$i][${record.javaClass.simpleName}]${describeRecord(record)} $message") } /** diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.kt b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.kt index b6bffb33ea..2ae7dd5aca 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/storage/StorageSyncModels.kt @@ -220,7 +220,7 @@ object StorageSyncModels { unregisteredAtTimestamp = recipient.syncExtras.unregisteredTimestamp hidden = recipient.hiddenState != Recipient.HiddenState.NOT_HIDDEN username = recipient.username ?: "" - pniSignatureVerified = recipient.syncExtras.pniSignatureVerified + pniSignatureVerified = recipient.pni?.isValid == true && recipient.syncExtras.pniSignatureVerified nickname = recipient.nickname.takeUnless { it.isEmpty }?.let { ContactRecord.Name(given = it.givenName, family = it.familyName) } note = recipient.note ?: "" avatarColor = localToRemoteAvatarColor(recipient.avatarColor) diff --git a/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt b/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt index 2f16d17781..e2e9c27d4f 100644 --- a/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt +++ b/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt @@ -4,6 +4,9 @@ import io.mockk.every import io.mockk.mockk import io.mockk.mockkObject import io.mockk.unmockkObject +import io.mockk.verify +import okio.ByteString +import okio.ByteString.Companion.toByteString import org.junit.After import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse @@ -16,7 +19,11 @@ import org.signal.core.models.ServiceId.ACI import org.signal.core.models.ServiceId.PNI import org.signal.core.util.logging.Log import org.thoughtcrime.securesms.database.RecipientTable +import org.thoughtcrime.securesms.database.SignalDatabase +import org.thoughtcrime.securesms.jobs.RetrieveProfileJob import org.thoughtcrime.securesms.keyvalue.SignalStore +import org.thoughtcrime.securesms.recipients.Recipient +import org.thoughtcrime.securesms.recipients.RecipientId import org.thoughtcrime.securesms.testutil.EmptyLogger import org.whispersystems.signalservice.api.storage.SignalContactRecord import org.whispersystems.signalservice.api.storage.StorageId @@ -39,6 +46,9 @@ class ContactRecordProcessorTest { @After fun tearDown() { unmockkObject(SignalStore) + unmockkObject(Recipient.Companion) + unmockkObject(RetrieveProfileJob.Companion) + unmockkObject(SignalDatabase.Companion) } @Test @@ -397,6 +407,256 @@ class ContactRecordProcessorTest { assertEquals("Spidey Friend", result.proto.note) } + @Test + fun `merge, identityKeys conflict on primary, keepLocal`() { + // GIVEN + val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable) + + val local = buildRecord( + STORAGE_ID_A, + record = ContactRecord( + pniBinary = PNI_B.toByteStringWithoutPrefix(), + e164 = E164_B, + identityKey = IDENTITY_KEY_A + ) + ) + + val remote = buildRecord( + STORAGE_ID_B, + record = ContactRecord( + pniBinary = PNI_B.toByteStringWithoutPrefix(), + e164 = E164_B, + identityKey = IDENTITY_KEY_B + ) + ) + + // WHEN + val result = subject.merge(remote, local, TestKeyGenerator(STORAGE_ID_C)) + + // THEN + assertEquals(IDENTITY_KEY_A, result.proto.identityKey) + } + + @Test + fun `merge, identityKeys conflict on linked device, useRemote`() { + // GIVEN + every { SignalStore.account.isPrimaryDevice } returns false + val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable) + + val local = buildRecord( + STORAGE_ID_A, + record = ContactRecord( + pniBinary = PNI_B.toByteStringWithoutPrefix(), + e164 = E164_B, + identityKey = IDENTITY_KEY_A + ) + ) + + val remote = buildRecord( + STORAGE_ID_B, + record = ContactRecord( + pniBinary = PNI_B.toByteStringWithoutPrefix(), + e164 = E164_B, + identityKey = IDENTITY_KEY_B + ) + ) + + // WHEN + val result = subject.merge(remote, local, TestKeyGenerator(STORAGE_ID_C)) + + // THEN + assertEquals(IDENTITY_KEY_B, result.proto.identityKey) + } + + @Test + fun `merge, identityKeys conflict on linked device but has ACI, keepLocal`() { + // GIVEN + every { SignalStore.account.isPrimaryDevice } returns false + mockkObject(Recipient.Companion) + mockkObject(RetrieveProfileJob.Companion) + mockkObject(SignalDatabase.Companion) + every { Recipient.trustedPush(any(), any(), any()) } returns mockk(relaxed = true) + every { RetrieveProfileJob.enqueue(any(), any()) } returns Unit + every { SignalDatabase.runPostSuccessfulTransaction(any()) } answers { firstArg().run() } + + val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable) + + val local = buildRecord( + STORAGE_ID_A, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B, + identityKey = IDENTITY_KEY_A + ) + ) + + val remote = buildRecord( + STORAGE_ID_B, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B, + identityKey = IDENTITY_KEY_B + ) + ) + + // WHEN + val result = subject.merge(remote, local, TestKeyGenerator(STORAGE_ID_C)) + + // THEN the profile fetch can repair this, so we keep our own key rather than deferring + assertEquals(IDENTITY_KEY_A, result.proto.identityKey) + verify { RetrieveProfileJob.enqueue(any(), any()) } + } + + @Test + fun `merge, identityKeys match on linked device, keepLocal`() { + // GIVEN + every { SignalStore.account.isPrimaryDevice } returns false + val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable) + + val local = buildRecord( + STORAGE_ID_A, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B, + identityKey = IDENTITY_KEY_A + ) + ) + + val remote = buildRecord( + STORAGE_ID_B, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B, + identityKey = IDENTITY_KEY_A + ) + ) + + // WHEN + val result = subject.merge(remote, local, TestKeyGenerator(STORAGE_ID_C)) + + // THEN + assertEquals(IDENTITY_KEY_A, result.proto.identityKey) + } + + @Test + fun `merge, local identityKey missing on primary, useRemote`() { + // GIVEN + val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable) + + val local = buildRecord( + STORAGE_ID_A, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B + ) + ) + + val remote = buildRecord( + STORAGE_ID_B, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B, + identityKey = IDENTITY_KEY_B + ) + ) + + // WHEN + val result = subject.merge(remote, local, TestKeyGenerator(STORAGE_ID_C)) + + // THEN + assertEquals(IDENTITY_KEY_B, result.proto.identityKey) + } + + @Test + fun `merge, remote identityKey missing on linked device, keepLocal`() { + // GIVEN + every { SignalStore.account.isPrimaryDevice } returns false + val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable) + + val local = buildRecord( + STORAGE_ID_A, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B, + identityKey = IDENTITY_KEY_A + ) + ) + + val remote = buildRecord( + STORAGE_ID_B, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B + ) + ) + + // WHEN + val result = subject.merge(remote, local, TestKeyGenerator(STORAGE_ID_C)) + + // THEN + assertEquals(IDENTITY_KEY_A, result.proto.identityKey) + } + + @Test + fun `merge, pniSignatureVerified but no PNI, clearsFlag`() { + // GIVEN + val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable) + + val local = buildRecord( + STORAGE_ID_A, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B + ) + ) + + val remote = buildRecord( + STORAGE_ID_B, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + e164 = E164_B, + pniSignatureVerified = true + ) + ) + + // WHEN + val result = subject.merge(remote, local, TestKeyGenerator(STORAGE_ID_C)) + + // THEN a verified PNI signature is meaningless without a PNI, so it must not be propagated + assertFalse(result.proto.pniSignatureVerified) + } + + @Test + fun `merge, pniSignatureVerified with PNI, keepsFlag`() { + // GIVEN + val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable) + + val local = buildRecord( + STORAGE_ID_A, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + pniBinary = PNI_B.toByteStringWithoutPrefix(), + e164 = E164_B + ) + ) + + val remote = buildRecord( + STORAGE_ID_B, + record = ContactRecord( + aciBinary = ACI_B.toByteString(), + pniBinary = PNI_B.toByteStringWithoutPrefix(), + e164 = E164_B, + pniSignatureVerified = true + ) + ) + + // WHEN + val result = subject.merge(remote, local, TestKeyGenerator(STORAGE_ID_C)) + + // THEN + assertTrue(result.proto.pniSignatureVerified) + } + private fun buildRecord(id: StorageId = STORAGE_ID_A, record: ContactRecord): SignalContactRecord { return SignalContactRecord(id, record) } @@ -421,6 +681,9 @@ class ContactRecordProcessorTest { const val E164_A = "+12221234567" const val E164_B = "+13331234567" + val IDENTITY_KEY_A: ByteString = byteArrayOf(1, 1, 1, 1).toByteString() + val IDENTITY_KEY_B: ByteString = byteArrayOf(2, 2, 2, 2).toByteString() + @JvmStatic @BeforeClass fun setUpClass() {