Prevent storage sync loop on PNI-only records with different identity keys.

2026-09-21 09:15:14 +01:00 · 2026-08-25 12:32:27 -04:00
parent 87df7029ad
commit 8c37e6b2d4
5 changed files with 305 additions and 17 deletions
@@ -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())
@@ -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<SignalContactRecord> {
    var found: Optional<RecipientId> = 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()))
@@ -74,12 +74,17 @@ abstract class DefaultStorageRecordProcessor<E : SignalRecord<*>> : 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")
  }

  /**
@@ -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)
@@ -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<RecipientId>(), any()) } returns Unit
+    every { SignalDatabase.runPostSuccessfulTransaction(any<Runnable>()) } answers { firstArg<Runnable>().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<RecipientId>(), 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() {