From 16309d87cd3462978472ac367aee4637a603fa54 Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Fri, 14 Feb 2025 11:40:28 -0500 Subject: [PATCH] Improve logging on some backup validation errors. --- .../securesms/backup/v2/ArchiveValidator.kt | 52 ++++++++++++++++--- .../InternalBackupPlaygroundViewModel.kt | 6 ++- .../securesms/jobs/BackupMessagesJob.kt | 8 ++- .../linkdevice/LinkDeviceRepository.kt | 6 ++- .../org/signal/core/util/logging/Scrubber.kt | 37 ++++++++++--- .../signal/core/util/logging/ScrubberTest.kt | 16 ++++++ 6 files changed, 107 insertions(+), 18 deletions(-) diff --git a/app/src/main/java/org/thoughtcrime/securesms/backup/v2/ArchiveValidator.kt b/app/src/main/java/org/thoughtcrime/securesms/backup/v2/ArchiveValidator.kt index 91c47bad21..3412d93239 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/backup/v2/ArchiveValidator.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/backup/v2/ArchiveValidator.kt @@ -10,6 +10,8 @@ import org.signal.libsignal.messagebackup.MessageBackup import org.signal.libsignal.messagebackup.ValidationError import org.thoughtcrime.securesms.database.SignalDatabase import org.thoughtcrime.securesms.keyvalue.SignalStore +import org.thoughtcrime.securesms.recipients.Recipient +import org.thoughtcrime.securesms.recipients.RecipientId import org.thoughtcrime.securesms.util.isStory import org.thoughtcrime.securesms.util.isStoryReaction import org.whispersystems.signalservice.api.backup.MessageBackupKey @@ -40,11 +42,36 @@ object ArchiveValidator { } catch (e: IOException) { ValidationResult.ReadError(e) } catch (e: ValidationError) { - val sentTimestamp = "\\d{10,}+".toRegex().find(e.message ?: "")?.value?.toLongOrNull() - ValidationResult.ValidationError( - exception = e, - messageDetails = sentTimestamp?.let { fetchMessageDetails(it) } ?: emptyList() - ) + if (e.message?.contains("have the same phone number") == true) { + val recipientIds = """RecipientId\((\d+)\)""".toRegex() + .findAll(e.message ?: "") + .map { it.groupValues[1] } + .mapNotNull { it.toLongOrNull() } + .map { RecipientId.from(it) } + .toList() + + val recipientIdA = recipientIds.getOrNull(0) + val recipientIdB = recipientIds.getOrNull(1) + + val e164A = recipientIdA?.let { Recipient.resolved(it).e164.orElse("UNKNOWN") }.let { "KEEP_E164::$it" } + val e164B = recipientIdB?.let { Recipient.resolved(it).e164.orElse("UNKNOWN") }.let { "KEEP_E164::$it" } + + ValidationResult.RecipientDuplicateE164Error( + exception = e, + details = DuplicateRecipientDetails( + recipientIdA = recipientIds.getOrNull(0), + recipientIdB = recipientIds.getOrNull(1), + e164A = e164A, + e164B = e164B + ) + ) + } else { + val sentTimestamp = "\\d{10,}+".toRegex().find(e.message ?: "")?.value?.toLongOrNull() + ValidationResult.MessageValidationError( + exception = e, + messageDetails = sentTimestamp?.let { fetchMessageDetails(it) } ?: emptyList() + ) + } } } @@ -74,10 +101,14 @@ object ArchiveValidator { sealed interface ValidationResult { data object Success : ValidationResult data class ReadError(val exception: IOException) : ValidationResult - data class ValidationError( - val exception: org.signal.libsignal.messagebackup.ValidationError, + data class MessageValidationError( + val exception: ValidationError, val messageDetails: List ) : ValidationResult + data class RecipientDuplicateE164Error( + val exception: ValidationError, + val details: DuplicateRecipientDetails + ) : ValidationResult } data class MessageDetails( @@ -97,4 +128,11 @@ object ArchiveValidator { val originalMessageId: Long, val isLatestRevision: Boolean ) + + data class DuplicateRecipientDetails( + val recipientIdA: RecipientId?, + val recipientIdB: RecipientId?, + val e164A: String?, + val e164B: String? + ) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/backup/InternalBackupPlaygroundViewModel.kt b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/backup/InternalBackupPlaygroundViewModel.kt index d30f94a296..71449cfcd5 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/backup/InternalBackupPlaygroundViewModel.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/components/settings/app/internal/backup/InternalBackupPlaygroundViewModel.kt @@ -151,10 +151,14 @@ class InternalBackupPlaygroundViewModel : ViewModel() { val message = when (result) { is ArchiveValidator.ValidationResult.ReadError -> "Failed to read backup file!" ArchiveValidator.ValidationResult.Success -> "Validation passed!" - is ArchiveValidator.ValidationResult.ValidationError -> { + is ArchiveValidator.ValidationResult.MessageValidationError -> { Log.w(TAG, "Validation failed! Details: ${result.messageDetails}", result.exception) "Validation failed :( Check the logs for details." } + is ArchiveValidator.ValidationResult.RecipientDuplicateE164Error -> { + Log.w(TAG, "Validation failed with a duplicate recipient! Details: ${result.details}", result.exception) + "Validation failed :( Check the logs for details." + } } _state.value = _state.value.copy(statusMessage = message) } diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobs/BackupMessagesJob.kt b/app/src/main/java/org/thoughtcrime/securesms/jobs/BackupMessagesJob.kt index e42b32028e..8543f734c2 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobs/BackupMessagesJob.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/jobs/BackupMessagesJob.kt @@ -226,11 +226,17 @@ class BackupMessagesJob private constructor( return BackupFileResult.Retry } - is ArchiveValidator.ValidationResult.ValidationError -> { + is ArchiveValidator.ValidationResult.MessageValidationError -> { Log.w(TAG, "The backup file fails validation! Message: ${result.exception.message}, Details: ${result.messageDetails}") ArchiveUploadProgress.onValidationFailure() return BackupFileResult.Failure } + + is ArchiveValidator.ValidationResult.RecipientDuplicateE164Error -> { + Log.w(TAG, "The backup file fails validation with a duplicate recipient! Message: ${result.exception.message}, Details: ${result.details}") + ArchiveUploadProgress.onValidationFailure() + return BackupFileResult.Failure + } } stopwatch.split("validate") diff --git a/app/src/main/java/org/thoughtcrime/securesms/linkdevice/LinkDeviceRepository.kt b/app/src/main/java/org/thoughtcrime/securesms/linkdevice/LinkDeviceRepository.kt index 9b8b295779..71ac6b5945 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/linkdevice/LinkDeviceRepository.kt +++ b/app/src/main/java/org/thoughtcrime/securesms/linkdevice/LinkDeviceRepository.kt @@ -277,10 +277,14 @@ object LinkDeviceRepository { Log.w(TAG, "[createAndUploadArchive] Failed to read the file during validation!", result.exception) return LinkUploadArchiveResult.BackupCreationFailure(result.exception) } - is ArchiveValidator.ValidationResult.ValidationError -> { + is ArchiveValidator.ValidationResult.MessageValidationError -> { Log.w(TAG, "[createAndUploadArchive] The backup file fails validation! Details: ${result.messageDetails}", result.exception) return LinkUploadArchiveResult.BackupCreationFailure(result.exception) } + is ArchiveValidator.ValidationResult.RecipientDuplicateE164Error -> { + Log.w(TAG, "[createAndUploadArchive] The backup file fails validation with a duplicate recipient! Details: ${result.details}", result.exception) + return LinkUploadArchiveResult.BackupCreationFailure(result.exception) + } } stopwatch.split("validate-backup") diff --git a/core-util-jvm/src/main/java/org/signal/core/util/logging/Scrubber.kt b/core-util-jvm/src/main/java/org/signal/core/util/logging/Scrubber.kt index 288bd77015..87f3d0805f 100644 --- a/core-util-jvm/src/main/java/org/signal/core/util/logging/Scrubber.kt +++ b/core-util-jvm/src/main/java/org/signal/core/util/logging/Scrubber.kt @@ -22,8 +22,8 @@ object Scrubber { * Supposedly, the shortest international phone numbers in use contain seven digits. * Handles URL encoded +, %2B */ - private val E164_PATTERN = Pattern.compile("(\\+|%2B)(\\d{7,15})") - private val E164_ZERO_PATTERN = Pattern.compile("\\b0(\\d{10})\\b") + private val E164_PATTERN = Pattern.compile("(KEEP_E164::)?(\\+|%2B)(\\d{7,15})") + private val E164_ZERO_PATTERN = Pattern.compile("\\b(KEEP_E164::)?0(\\d{10})\\b") /** The second group will be censored.*/ private val CRUDE_EMAIL_PATTERN = Pattern.compile("\\b([^\\s/])([^\\s/]*@[^\\s]+)") @@ -104,17 +104,29 @@ object Scrubber { private fun CharSequence.scrubE164(): CharSequence { return scrub(this, E164_PATTERN) { matcher, output -> - output - .append("E164:") - .append(hash(matcher.group(2))) + if (matcher.group(1) != null && matcher.group(1)!!.isNotEmpty()) { + output + .append("KEEP_E164::") + .append((matcher.group(2) + matcher.group(3)).censorMiddle(2, 2)) + } else { + output + .append("E164:") + .append(hash(matcher.group(3))) + } } } private fun CharSequence.scrubE164Zero(): CharSequence { return scrub(this, E164_ZERO_PATTERN) { matcher, output -> - output - .append("E164:") - .append(hash(matcher.group(1))) + if (matcher.group(1) != null && matcher.group(1)!!.isNotEmpty()) { + output + .append("KEEP_E164::") + .append(("0" + matcher.group(2)).censorMiddle(2, 2)) + } else { + output + .append("E164:") + .append(hash(matcher.group(2))) + } } } @@ -204,6 +216,15 @@ object Scrubber { } } + private fun String.censorMiddle(leading: Int, trailing: Int): String { + val totalKept = leading + trailing + if (this.length < totalKept) { + return "*".repeat(this.length) + } + val middle = "*".repeat(this.length - totalKept) + return this.take(leading) + middle + this.takeLast(trailing) + } + private fun scrub(input: CharSequence, pattern: Pattern, processMatch: MatchProcessor): CharSequence { val output = StringBuilder(input.length) val matcher: Matcher = pattern.matcher(input) diff --git a/core-util-jvm/src/test/java/org/signal/core/util/logging/ScrubberTest.kt b/core-util-jvm/src/test/java/org/signal/core/util/logging/ScrubberTest.kt index 3f1b581f89..dd36538977 100644 --- a/core-util-jvm/src/test/java/org/signal/core/util/logging/ScrubberTest.kt +++ b/core-util-jvm/src/test/java/org/signal/core/util/logging/ScrubberTest.kt @@ -74,6 +74,22 @@ class ScrubberTest(private val input: String, private val expected: String) { "Longest number +155556789012345", "Longest number E164:<90596>" ), + arrayOf( + "An E164 number KEEP_E164::+15551234567", + "An E164 number KEEP_E164::+1********67" + ), + arrayOf( + "A UK number KEEP_E164::+447700900000", + "A UK number KEEP_E164::+4*********00" + ), + arrayOf( + "A Japanese number KEEP_E164::08011112222", + "A Japanese number KEEP_E164::08*******22" + ), + arrayOf( + "A Japanese number (KEEP_E164::08011112222)", + "A Japanese number (KEEP_E164::08*******22)" + ), arrayOf( "One more than longest number +1234567890123456", "One more than longest number E164:<78d5b>6"