From 8334db52730a62b859e1878f927c7f59439c7dfd Mon Sep 17 00:00:00 2001
From: Greyson Parrelli <greyson@signal.org>
Date: Thu, 23 Feb 2023 17:45:20 -0500
Subject: [PATCH] Validate E164's in ContactRecords.

---
 .../storage/ContactRecordProcessor.java       |  10 ++
 .../storage/ContactRecordProcessorTest.kt     | 102 ++++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java
index 854b0bec5..18a37b3f1 100644
--- a/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java
+++ b/app/src/main/java/org/thoughtcrime/securesms/storage/ContactRecordProcessor.java
@@ -28,11 +28,14 @@ import java.util.Objects;
 import java.util.Optional;
 import java.util.TreeMap;
 import java.util.TreeSet;
+import java.util.regex.Pattern;
 
 public class ContactRecordProcessor extends DefaultStorageRecordProcessor<SignalContactRecord> {
 
   private static final String TAG = Log.tag(ContactRecordProcessor.class);
 
+  private static final Pattern E164_PATTERN = Pattern.compile("^\\+[1-9]\\d{0,18}$");
+
   private final RecipientTable recipientTable;
 
   private final ACI    selfAci;
@@ -141,6 +144,9 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
     } else if (!FeatureFlags.phoneNumberPrivacy() && remote.getServiceId().equals(remote.getPni().orElse(null))) {
       Log.w(TAG, "Found a PNI-only ContactRecord when PNP is disabled -- marking as invalid.");
       return true;
+    } else if (remote.getNumber().isPresent() && !isValidE164(remote.getNumber().get())) {
+      Log.w(TAG, "Found a record with an invalid E164. Marking as invalid.");
+      return true;
     } else {
       return false;
     }
@@ -321,6 +327,10 @@ public class ContactRecordProcessor extends DefaultStorageRecordProcessor<Signal
     }
   }
 
+  private static boolean isValidE164(String value) {
+    return E164_PATTERN.matcher(value).matches();
+  }
+
   private static boolean doParamsMatch(@NonNull SignalContactRecord contact,
                                        @Nullable byte[] unknownFields,
                                        @NonNull ServiceId serviceId,
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 666f2562e..8cf104b63 100644
--- a/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt
+++ b/app/src/test/java/org/thoughtcrime/securesms/storage/ContactRecordProcessorTest.kt
@@ -188,6 +188,108 @@ class ContactRecordProcessorTest {
     assertFalse(result)
   }
 
+  @Test
+  fun `isInvalid, valid E164, true`() {
+    // GIVEN
+    val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable)
+
+    val record = buildRecord {
+      setServiceId(ACI_B.toString())
+      setServiceE164(E164_B)
+    }
+
+    // WHEN
+    val result = subject.isInvalid(record)
+
+    // THEN
+    assertFalse(result)
+  }
+
+  @Test
+  fun `isInvalid, invalid E164 (missing +), true`() {
+    // GIVEN
+    val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable)
+
+    val record = buildRecord {
+      setServiceId(ACI_B.toString())
+      setServiceE164("15551234567")
+    }
+
+    // WHEN
+    val result = subject.isInvalid(record)
+
+    // THEN
+    assertTrue(result)
+  }
+
+  @Test
+  fun `isInvalid, invalid E164 (contains letters), true`() {
+    // GIVEN
+    val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable)
+
+    val record = buildRecord {
+      setServiceId(ACI_B.toString())
+      setServiceE164("+1555ABC4567")
+    }
+
+    // WHEN
+    val result = subject.isInvalid(record)
+
+    // THEN
+    assertTrue(result)
+  }
+
+  @Test
+  fun `isInvalid, invalid E164 (no numbers), true`() {
+    // GIVEN
+    val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable)
+
+    val record = buildRecord {
+      setServiceId(ACI_B.toString())
+      setServiceE164("+")
+    }
+
+    // WHEN
+    val result = subject.isInvalid(record)
+
+    // THEN
+    assertTrue(result)
+  }
+
+  @Test
+  fun `isInvalid, invalid E164 (too many numbers), true`() {
+    // GIVEN
+    val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable)
+
+    val record = buildRecord {
+      setServiceId(ACI_B.toString())
+      setServiceE164("+12345678901234567890")
+    }
+
+    // WHEN
+    val result = subject.isInvalid(record)
+
+    // THEN
+    assertTrue(result)
+  }
+
+  @Test
+  fun `isInvalid, invalid E164 (starts with zero), true`() {
+    // GIVEN
+    val subject = ContactRecordProcessor(ACI_A, PNI_A, E164_A, recipientTable)
+
+    val record = buildRecord {
+      setServiceId(ACI_B.toString())
+      setServiceE164("+05551234567")
+    }
+
+    // WHEN
+    val result = subject.isInvalid(record)
+
+    // THEN
+    assertTrue(result)
+  }
+
   @Test
   fun `merge, e164MatchesButPnisDont pnpEnabled, keepLocal`() {
     // GIVEN