From a67d0b18ff874c4fd42f8711f03849218c65a0d6 Mon Sep 17 00:00:00 2001 From: Moxie Marlinspike Date: Tue, 1 Aug 2017 14:04:51 -0700 Subject: [PATCH] Be more liberal with external address formatting A string like "bonbon" should just be "bonbon". That is apparently a valid SMS source address. // FREEBIE --- .../securesms/database/Address.java | 112 ++++++++++++++---- .../securesms/database/AddressTest.java | 50 ++++++++ 2 files changed, 141 insertions(+), 21 deletions(-) create mode 100644 test/unitTest/java/org/thoughtcrime/securesms/database/AddressTest.java diff --git a/src/org/thoughtcrime/securesms/database/Address.java b/src/org/thoughtcrime/securesms/database/Address.java index 69d49a39fa..d90e669c75 100644 --- a/src/org/thoughtcrime/securesms/database/Address.java +++ b/src/org/thoughtcrime/securesms/database/Address.java @@ -6,19 +6,24 @@ import android.os.Parcel; import android.os.Parcelable; import android.support.annotation.NonNull; import android.support.annotation.Nullable; -import android.text.TextUtils; +import android.support.annotation.VisibleForTesting; import android.util.Log; +import com.google.i18n.phonenumbers.NumberParseException; +import com.google.i18n.phonenumbers.PhoneNumberUtil; +import com.google.i18n.phonenumbers.Phonenumber; +import com.google.i18n.phonenumbers.ShortNumberInfo; + import org.thoughtcrime.securesms.util.GroupUtil; import org.thoughtcrime.securesms.util.NumberUtil; -import org.thoughtcrime.securesms.util.ShortCodeUtil; import org.thoughtcrime.securesms.util.TextSecurePreferences; import org.thoughtcrime.securesms.util.Util; -import org.whispersystems.signalservice.api.util.InvalidNumberException; -import org.whispersystems.signalservice.api.util.PhoneNumberFormatter; +import java.util.HashSet; import java.util.LinkedList; import java.util.List; +import java.util.Set; +import java.util.regex.Pattern; public class Address implements Parcelable, Comparable
{ @@ -62,23 +67,8 @@ public class Address implements Parcelable, Comparable
{ return addresses; } - public static Address fromExternal(@NonNull Context context, @Nullable String external) - { - if (external == null) return new Address("Unknown"); - - try { - String localNumber = TextSecurePreferences.getLocalNumber(context); - - if (GroupUtil.isEncodedGroup(external)) return new Address(external); - else if (NumberUtil.isValidEmail(external)) return new Address(external); - else if (ShortCodeUtil.isShortCode(localNumber, external)) return new Address(external.replaceAll("[^0-9+]", "")); - - return new Address(PhoneNumberFormatter.formatNumber(external, localNumber)); - } catch (InvalidNumberException e) { - Log.w(TAG, e); - if (TextUtils.isEmpty(external.trim())) return new Address("Unknown"); - else return new Address(external.trim()); - } + public static Address fromExternal(@NonNull Context context, @Nullable String external) { + return new Address(new ExternalAddressFormatter(TextSecurePreferences.getLocalNumber(context)).format(external)); } public static Address[] fromParcelable(Parcelable[] parcelables) { @@ -153,4 +143,84 @@ public class Address implements Parcelable, Comparable
{ public int compareTo(@NonNull Address other) { return address.compareTo(other.address); } + + @VisibleForTesting + static class ExternalAddressFormatter { + + private static final String TAG = ExternalAddressFormatter.class.getSimpleName(); + + private static final Set SHORT_COUNTRIES = new HashSet() {{ + add("NU"); + add("TK"); + add("NC"); + add("AC"); + }}; + + private final Phonenumber.PhoneNumber localNumber; + private final String localNumberString; + private final String localCountryCode; + + private final PhoneNumberUtil phoneNumberUtil = PhoneNumberUtil.getInstance(); + private final Pattern ALPHA_PATTERN = Pattern.compile("[a-zA-Z]"); + + public ExternalAddressFormatter(String localNumber) { + try { + this.localNumberString = localNumber; + this.localNumber = phoneNumberUtil.parse(localNumber, null); + this.localCountryCode = phoneNumberUtil.getRegionCodeForNumber(this.localNumber); + } catch (NumberParseException e) { + throw new AssertionError(e); + } + } + + public String format(@Nullable String number) { + if (number == null) return "Unknown"; + if (number.startsWith("__textsecure_group__!")) return number; + if (ALPHA_PATTERN.matcher(number).matches()) return number.trim(); + + String bareNumber = number.replaceAll("[^0-9+]", ""); + + if (bareNumber.length() == 0) { + if (number.trim().length() == 0) return "Unknown"; + else return number.trim(); + } + + // libphonenumber doesn't seem to be correct for Germany and Finland + if (bareNumber.length() <= 6 && ("DE".equals(localCountryCode) || "FI".equals(localCountryCode) || "SK".equals(localCountryCode))) { + return bareNumber; + } + + // libphonenumber seems incorrect for Russia and a few other countries with 4 digit short codes. + if (bareNumber.length() <= 4 && !SHORT_COUNTRIES.contains(localCountryCode)) { + return bareNumber; + } + + try { + Phonenumber.PhoneNumber parsedNumber = phoneNumberUtil.parse(bareNumber, localCountryCode); + + if (ShortNumberInfo.getInstance().isPossibleShortNumberForRegion(parsedNumber, localCountryCode)) { + return bareNumber; + } + + return phoneNumberUtil.format(parsedNumber, PhoneNumberUtil.PhoneNumberFormat.E164); + } catch (NumberParseException e) { + Log.w(TAG, e); + if (bareNumber.charAt(0) == '+') + return bareNumber; + + String localNumberImprecise = localNumberString; + + if (localNumberImprecise.charAt(0) == '+') + localNumberImprecise = localNumberImprecise.substring(1); + + if (localNumberImprecise.length() == bareNumber.length() || bareNumber.length() > localNumberImprecise.length()) + return "+" + number; + + int difference = localNumberImprecise.length() - bareNumber.length(); + + return "+" + localNumberImprecise.substring(0, difference) + bareNumber; + } + } + } + } diff --git a/test/unitTest/java/org/thoughtcrime/securesms/database/AddressTest.java b/test/unitTest/java/org/thoughtcrime/securesms/database/AddressTest.java new file mode 100644 index 0000000000..6c156866a0 --- /dev/null +++ b/test/unitTest/java/org/thoughtcrime/securesms/database/AddressTest.java @@ -0,0 +1,50 @@ +package org.thoughtcrime.securesms.database; + + +import org.junit.Before; +import org.junit.Test; + +import static junit.framework.Assert.assertEquals; + +public class AddressTest { + + @Before + public void setup() {} + + @Test + public void testAddressString() throws Exception { + Address.ExternalAddressFormatter formatter = new Address.ExternalAddressFormatter("+14152222222"); + assertEquals(formatter.format("bonbon"), "bonbon"); + } + + @Test + public void testAddressShortCode() throws Exception { + Address.ExternalAddressFormatter formatter = new Address.ExternalAddressFormatter("+14152222222"); + assertEquals(formatter.format("40404"), "40404"); + } + + @Test + public void testEmailAddress() throws Exception { + Address.ExternalAddressFormatter formatter = new Address.ExternalAddressFormatter("+14152222222"); + assertEquals(formatter.format("junk@junk.net"), "junk@junk.net"); + } + + @Test + public void testNumberArbitrary() throws Exception { + Address.ExternalAddressFormatter formatter = new Address.ExternalAddressFormatter("+14152222222"); + assertEquals(formatter.format("(415) 111-1122"), "+14151111122"); + assertEquals(formatter.format("(415) 111 1123"), "+14151111123"); + assertEquals(formatter.format("415-111-1124"), "+14151111124"); + assertEquals(formatter.format("415.111.1125"), "+14151111125"); + assertEquals(formatter.format("+1 415.111.1126"), "+14151111126"); + assertEquals(formatter.format("+1 415 111 1127"), "+14151111127"); + assertEquals(formatter.format("+1 (415) 111 1128"), "+14151111128"); + } + + @Test + public void testGroup() throws Exception { + Address.ExternalAddressFormatter formatter = new Address.ExternalAddressFormatter("+14152222222"); + assertEquals(formatter.format("__textsecure_group__!foobar"), "__textsecure_group__!foobar"); + } + +}