Validate E164's in ContactRecords.

signalapp · Mar 3, 2023 · 8334db5 · 8334db5
1 parent 3382843
commit 8334db5
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 0 deletions.
diff --git 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.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 @@ boolean isInvalid(@NonNull SignalContactRecord remote) {
     } 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 int compare(@NonNull SignalContactRecord lhs, @NonNull SignalContactRecor
     }
   }
 
+  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
@@ -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