From 48731286eb336633d42291ed4b49c84b33baacef Mon Sep 17 00:00:00 2001 From: AsamK Date: Thu, 10 Sep 2026 16:16:55 +0200 Subject: [PATCH] Prevent storage sync loop on PNI-only records with different identity keys --- .../syncStorage/ContactRecordProcessor.java | 64 ++++++++++++++----- .../DefaultStorageRecordProcessor.java | 9 ++- .../syncStorage/StorageSyncModels.java | 3 +- .../StorageRecordProcessorTest.java | 7 ++ 4 files changed, 65 insertions(+), 18 deletions(-) diff --git a/lib/src/main/java/org/asamk/signal/manager/syncStorage/ContactRecordProcessor.java b/lib/src/main/java/org/asamk/signal/manager/syncStorage/ContactRecordProcessor.java index 3829f8d2..ad152ef4 100644 --- a/lib/src/main/java/org/asamk/signal/manager/syncStorage/ContactRecordProcessor.java +++ b/lib/src/main/java/org/asamk/signal/manager/syncStorage/ContactRecordProcessor.java @@ -80,6 +80,28 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor 0 && aci != null && pni == null && e164.isEmpty(); } + static boolean shouldUseRemoteIdentityKey( + final boolean isPrimaryDevice, + final boolean statesDiffer, + final int remoteIdentityKeySize, + final int localIdentityKeySize, + final long localUnregisteredAtTimestamp, + final boolean unrepairableIdentityKeyConflict + ) { + return remoteIdentityKeySize > 0 && (statesDiffer + || localIdentityKeySize == 0 + || localUnregisteredAtTimestamp > 0 + || (unrepairableIdentityKeyConflict && !isPrimaryDevice)); + } + + @Override + protected String describeRecord(final SignalContactRecord record) { + final var proto = record.getProto(); + final var aci = ACI.parseOrNull(proto.aci, proto.aciBinary); + final var pni = PNI.parseOrNull(proto.pni, proto.pniBinary); + return "[" + firstNonNull(aci, pni) + "]"; + } + /** * Error cases: * - You can't have a contact record without an ACI or PNI. @@ -154,32 +176,29 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor 0 + && localIdentityKeySize > 0 + && !remote.identityKey.equals(local.identityKey); + final var conflictAci = firstNonNull(localAci, remoteAci); + final var unrepairableIdentityKeyConflict = identityKeysExistAndConflict && conflictAci == null; - if (remoteIdentityKeySize > 0 && (!account.isPrimaryDevice() || statesDiffer || localIdentityKeySize == 0)) { + if (shouldUseRemoteIdentityKey(account.isPrimaryDevice(), + statesDiffer, + remoteIdentityKeySize, + localIdentityKeySize, + local.unregisteredAtTimestamp, + unrepairableIdentityKeyConflict)) { identityState = remote.identityState; identityKey = remote.identityKey; } else { identityState = local.identityState; - // Only use local's identity key if: - // 1. Contact has ACI or PNI - // 2. Remote also has an identity key (if remote size=0, respect that decision) - if (hasLocalIdentity && localIdentityKeySize > 0 && remoteIdentityKeySize > 0) { + if (hasLocalIdentity && localIdentityKeySize > 0) { identityKey = local.identityKey; } else { identityKey = ByteString.EMPTY; } } - if (localAci != null - && local.identityKey.size() > 0 - && remote.identityKey.size() > 0 - && !local.identityKey.equals(remote.identityKey)) { - logger.debug("The local and remote identity keys do not match for {}. Enqueueing a profile fetch.", - localAci); - final var address = getRecipientAddress(local); - jobExecutor.enqueueJob(new DownloadProfileJob(address)); - } - PNI pni; String e164; if (account.isPrimaryDevice()) { @@ -213,6 +232,17 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor 0 && remoteIdentityKeySize == 0) { + logger.debug("Remote identity key is missing for {}. Keeping ours.", firstNonNull(localAci, localPni)); + } + final var remoteProfileKey = remote.profileKey.size() == 0 || KeyUtils.profileKeyOrNull(remote.profileKey.toByteArray()) == null ? ByteString.EMPTY @@ -237,7 +267,9 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor> implemen return Collections.unmodifiableSet(updatedStorageIds); } + /** + * Optional extra identifying detail about a record, included in every log line for it. + */ + protected String describeRecord(E record) { + return ""; + } + private void debug(StorageId i, E record, String message) { - logger.debug("[{}][{}] {}", i, record.getClass().getSimpleName(), message); + logger.debug("[{}][{}]{} {}", i, record.getClass().getSimpleName(), describeRecord(record), message); } /** diff --git a/lib/src/main/java/org/asamk/signal/manager/syncStorage/StorageSyncModels.java b/lib/src/main/java/org/asamk/signal/manager/syncStorage/StorageSyncModels.java index e721d540..18276eaa 100644 --- a/lib/src/main/java/org/asamk/signal/manager/syncStorage/StorageSyncModels.java +++ b/lib/src/main/java/org/asamk/signal/manager/syncStorage/StorageSyncModels.java @@ -100,7 +100,8 @@ public final class StorageSyncModels { final var builder = SignalContactRecord.Companion.newBuilder(recipient.getStorageRecord()) .e164(address.number().orElse("")) .username(address.username().orElse("")) - .pniSignatureVerified(recipient.isPniSignatureVerified()) + .pniSignatureVerified(address.pni().map(PNI::isValid).orElse(false) + && recipient.isPniSignatureVerified()) .profileKey(recipient.getProfileKey() == null ? ByteString.EMPTY : ByteString.of(recipient.getProfileKey().serialize())); diff --git a/lib/src/test/java/org/asamk/signal/manager/syncStorage/StorageRecordProcessorTest.java b/lib/src/test/java/org/asamk/signal/manager/syncStorage/StorageRecordProcessorTest.java index 2a0c6b75..1ce68a07 100644 --- a/lib/src/test/java/org/asamk/signal/manager/syncStorage/StorageRecordProcessorTest.java +++ b/lib/src/test/java/org/asamk/signal/manager/syncStorage/StorageRecordProcessorTest.java @@ -23,6 +23,13 @@ class StorageRecordProcessorTest { assertFalse(ContactRecordProcessor.shouldSplitForStorageSync(1, aci, null, "+12025550123")); } + @Test + void linkedDeviceUsesRemoteIdentityKeyForUnrepairableConflict() { + assertTrue(ContactRecordProcessor.shouldUseRemoteIdentityKey(false, false, 33, 33, 0, true)); + assertFalse(ContactRecordProcessor.shouldUseRemoteIdentityKey(true, false, 33, 33, 0, true)); + assertFalse(ContactRecordProcessor.shouldUseRemoteIdentityKey(false, false, 33, 33, 0, false)); + } + @Test void keepsOlderLocalStickerDeletion() { assertTrue(StickerPackRecordProcessor.shouldKeepLocalDeletion(200, 100));