Make phone parsing stricter

go.mod

@@ -3,7 +3,7 @@ module github.com/nyaruka/gocommon
 go 1.21
 
 require (
-	github.com/aws/aws-sdk-go v1.52.3
+	github.com/aws/aws-sdk-go v1.52.4
 	github.com/gabriel-vasile/mimetype v1.4.3
 	github.com/go-chi/chi/v5 v5.0.12
 	github.com/go-playground/validator/v10 v10.20.0
@@ -14,7 +14,7 @@ require (
 	github.com/lib/pq v1.10.9
 	github.com/nyaruka/librato v1.1.1
 	github.com/nyaruka/null/v2 v2.0.3
-	github.com/nyaruka/phonenumbers v1.3.4
+	github.com/nyaruka/phonenumbers v1.3.5
 	github.com/pkg/errors v0.9.1
 	github.com/shopspring/decimal v1.4.0
 	github.com/stretchr/testify v1.9.0

go.sum

@@ -1,7 +1,7 @@
 filippo.io/edwards25519 v1.1.0 h1:FNf4tywRC1HmFuKW5xopWpigGjJKiJSV0Cqo0cJWDaA=
 filippo.io/edwards25519 v1.1.0/go.mod h1:BxyFTGdWcka3PhytdK4V28tE5sGfRvvvRV7EaN4VDT4=
-github.com/aws/aws-sdk-go v1.52.3 h1:BNPJmHOXNoM/iBWJKrvaQvJOweRcp3KLpzdb65CfQwU=
-github.com/aws/aws-sdk-go v1.52.3/go.mod h1:LF8svs817+Nz+DmiMQKTO3ubZ/6IaTpq3TjupRn3Eqk=
+github.com/aws/aws-sdk-go v1.52.4 h1:9VsBVJ2TKf8xPP3+yIPGSYcEBIEymXsJzQoFgQuyvA0=
+github.com/aws/aws-sdk-go v1.52.4/go.mod h1:LF8svs817+Nz+DmiMQKTO3ubZ/6IaTpq3TjupRn3Eqk=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
@@ -43,8 +43,8 @@ github.com/nyaruka/librato v1.1.1 h1:0nTYtJLl3Sn7lX3CuHsLf+nXy1k/tGV0OjVxLy3Et4s
 github.com/nyaruka/librato v1.1.1/go.mod h1:fme1Fu1PT2qvkaBZyw8WW+SrnFe2qeeCWpvqmAaKAKE=
 github.com/nyaruka/null/v2 v2.0.3 h1:rdmMRQyVzrOF3Jff/gpU/7BDR9mQX0lcLl4yImsA3kw=
 github.com/nyaruka/null/v2 v2.0.3/go.mod h1:OCVeCkCXwrg5/qE6RU0c1oUVZBy+ZDrT+xYg1XSaIWA=
-github.com/nyaruka/phonenumbers v1.3.4 h1:bF1Wdh++fxw09s3surhVeBhXEcUKG07pHeP8HQXqjn8=
-github.com/nyaruka/phonenumbers v1.3.4/go.mod h1:Ut+eFwikULbmCenH6InMKL9csUNLyxHuBLyfkpum11s=
+github.com/nyaruka/phonenumbers v1.3.5 h1:WZLbQn61j2E1OFnvpUTYbK/6hViUgl6tppJ55/E2iQM=
+github.com/nyaruka/phonenumbers v1.3.5/go.mod h1:Ut+eFwikULbmCenH6InMKL9csUNLyxHuBLyfkpum11s=
 github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
 github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
 github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
@@ -68,8 +68,6 @@ golang.org/x/sys v0.20.0 h1:Od9JTbYCk261bKm4M/mw7AklTlFYIa0bIp9BgSm1S8Y=
 golang.org/x/sys v0.20.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
 golang.org/x/text v0.15.0 h1:h1V/4gjBv8v9cjcR6+AR5+/cIYK5N/WAgiv4xlsEtAk=
 golang.org/x/text v0.15.0/go.mod h1:18ZOQIKpY8NJVqYksKHtTdi31H5itFRjB5/qKTNYzSU=
-google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI=
-google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
 google.golang.org/protobuf v1.34.1 h1:9ddQBjfCyZPOHPUiPxpYESBLc+T8P3E+Vo4IbKZgFWg=
 google.golang.org/protobuf v1.34.1/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=

urns/phone.go

@@ -11,38 +11,44 @@ import (
 )
 
 var nonTelCharsRegex = regexp.MustCompile(`[^0-9A-Za-z]`)
+var altShortCodeRegex = regexp.MustCompile(`^[1-9][0-9]{2,5}$`)
 
-// ParsePhone returns a validated phone URN. If it can parse a possible number then that is used.. otherwise any value
-// that validates as a phone URN is used.
+// ParsePhone returns a validated phone URN or an error.
 func ParsePhone(raw string, country i18n.Country) (URN, error) {
-	// strip all non-tel characters.. only preserving an optional leading +
+	number, err := ParseNumber(raw, country)
+	if err != nil {
+		return "", err
+	}
+
+	return NewFromParts(Phone.Prefix, number, "", "")
+}
+
+// ParseNumber tries to extact a possible number or shortcode from the given string, returning an error if it can't.
+func ParseNumber(raw string, country i18n.Country) (string, error) {
+	// strip all non-alphanumeric characters.. only preserving an optional leading +
 	raw = strings.TrimSpace(raw)
 	hasPlus := strings.HasPrefix(raw, "+")
 	raw = nonTelCharsRegex.ReplaceAllString(raw, "")
 	if hasPlus {
 		raw = "+" + raw
 	}
 
-	// if we're sufficienly long and don't start with a 0 then add a +
+	// if we're sufficiently long and don't start with a 0 then add a +
 	if len(raw) >= 11 && !strings.HasPrefix(raw, "0") {
 		raw = "+" + raw
 	}
 
 	number, err := parsePhoneOrShortcode(raw, country)
 	if err != nil {
-		if err == phonenumbers.ErrInvalidCountryCode {
-			return NilURN, errors.New("invalid country code")
-		}
-
-		return NewFromParts(Phone.Prefix, raw, "", "")
+		return "", err
 	}
 
-	return NewFromParts(Phone.Prefix, number, "", "")
+	return number, nil
 }
 
 // tries to extract a valid phone number or shortcode from the given string
-func parsePhoneOrShortcode(raw string, country i18n.Country) (string, error) {
-	parsed, err := phonenumbers.Parse(raw, string(country))
+func parsePhoneOrShortcode(input string, country i18n.Country) (string, error) {
+	parsed, err := phonenumbers.Parse(input, string(country))
 	if err != nil {
 		return "", err
 	}
@@ -55,7 +61,13 @@ func parsePhoneOrShortcode(raw string, country i18n.Country) (string, error) {
 		return phonenumbers.Format(parsed, phonenumbers.NATIONAL), nil
 	}
 
-	return "", errors.New("unable to parse phone number or shortcode")
+	// it seems libphonenumber's metadata regarding shortcodes is lacking so we also accept any sequence of 3-6 digits
+	// that doesn't start with a zero as a shortcode
+	if altShortCodeRegex.MatchString(input) {
+		return input, nil
+	}
+
+	return "", errors.New("not a possible number or shortcode")
 }
 
 // ToLocalPhone converts a phone URN to a local phone number.. without any leading zeros. Kinda weird but used by

urns/phone_test.go

@@ -1,83 +1,81 @@
-package urns
+package urns_test
 
 import (
 	"testing"
 
 	"github.com/nyaruka/gocommon/i18n"
+	"github.com/nyaruka/gocommon/urns"
 	"github.com/stretchr/testify/assert"
 )
 
 func TestParsePhone(t *testing.T) {
 	testCases := []struct {
-		input    string
-		country  i18n.Country
-		expected URN
+		input       string
+		country     i18n.Country
+		expectedURN urns.URN
+		expectedErr string
 	}{
-		{" 0788383383 ", "RW", "tel:+250788383383"},
-		{"+250788383383 ", "RW", "tel:+250788383383"}, // already has country code and leading +
-		{"250788383383 ", "RW", "tel:+250788383383"},  // already has country code and no leading +
-		{"+250788383383 ", "KE", "tel:+250788383383"}, // already has a different country code
-		{"(917)992-5253", "US", "tel:+19179925253"},
-		{"800-CABBAGE", "US", "tel:+18002222243"},
-		{"+62877747666", "ID", "tel:+62877747666"},
-		{"0877747666", "ID", "tel:+62877747666"},
-		{"07531669965", "GB", "tel:+447531669965"},
-		{"263780821000", "ZW", "tel:+263780821000"},
-		{"254791541111", "US", "tel:+254791541111"}, // international but missing + and wrong country
-
-		{"1", "RW", "tel:1"},
-		{"123456", "RW", "tel:123456"},
-		{"mtn", "RW", "tel:mtn"},
-		{"!mtn!", "RW", "tel:mtn"}, // non tel chars stripped
+		{"+250788123123", "", "tel:+250788123123", ""},    // international number fine without country
+		{"+250 788 123-123", "", "tel:+250788123123", ""}, // still fine if not E164 formatted
+		{"250788123123", "", "tel:+250788123123", ""},     // still fine without leading + because it's long enough
 
-		{"0788383383", "ZZ", NilURN}, // invalid country code
-		{"1234567890123456789012345678901234567890123456789012345678901234567890123456789", "RW", NilURN}, // too long
-	}
-
-	for i, tc := range testCases {
-		urn, err := ParsePhone(tc.input, tc.country)
+		{" 0788383383 ", "RW", "tel:+250788383383", ""},   // country code added
+		{"+250788383383 ", "RW", "tel:+250788383383", ""}, // already has country code and leading +
+		{"250788383383 ", "RW", "tel:+250788383383", ""},  // already has country code and no leading +
+		{"+250788383383 ", "KE", "tel:+250788383383", ""}, // already has a different country code
+		{"(917)992-5253", "US", "tel:+19179925253", ""},   // punctuation removed
+		{"800-CABBAGE", "US", "tel:+18002222243", ""},     // vanity numbers converted to digits
+		{"+62877747666", "ID", "tel:+62877747666", ""},
+		{"0877747666", "ID", "tel:+62877747666", ""},
+		{"07531669965", "GB", "tel:+447531669965", ""},
+		{"263780821000", "ZW", "tel:+263780821000", ""},
+		{"254791541111", "US", "tel:+254791541111", ""}, // international but missing + and wrong country
 
-		if tc.expected == NilURN {
-			assert.Error(t, err, "%d: expected error for %s, %s", i, tc.input, tc.country)
-		} else {
-			assert.NoError(t, err, "%d: unexpected error for %s, %s", i, tc.input, tc.country)
-			assert.Equal(t, tc.expected, urn, "%d: created URN mismatch for %s, %s", i, tc.input, tc.country)
-		}
-	}
-}
+		{"1234", "US", "tel:1234", ""},
+		{"12345", "US", "tel:12345", ""},
+		{"123", "RW", "tel:123", ""},
+		{"8080", "EC", "tel:8080", ""},
 
-func TestParsePhoneOrShortcode(t *testing.T) {
-	tcs := []struct {
-		input    string
-		country  i18n.Country
-		expected string
-	}{
-		{"+250788123123", "", "+250788123123"},    // international number fine without country
-		{"+250 788 123-123", "", "+250788123123"}, // still fine if not E164 formatted
+		// inputs that fail parsing by libphonenumber
+		{"1", "RW", "", "the phone number supplied is not a number"},
+		{"mtn", "RW", "", "the phone number supplied is not a number"},
 
-		{"0788123123", "RW", "+250788123123"},     // country code added
-		{" (206)555-1212 ", "US", "+12065551212"}, // punctiation removed
-		{"800-CABBAGE", "US", "+18002222243"},     // letters converted to numbers
-		{"12065551212", "US", "+12065551212"},     // country code but no +
-		{"10000", "US", "10000"},                  // valid short code for US
+		// inputa that fail checking for possible number or shortcode
+		{"99", "EC", "", "not a possible number or shortcode"},
+		{"567-1234", "US", "", "not a possible number or shortcode"}, // only dialable locally
 
-		{"5912705", "US", ""}, // is only possible as a local number so ignored
+		{"0788383383", "ZZ", "", "invalid country code"}, // invalid country code
+		{"1234567890123456789012345678901234567890123456789012345678901234567890123456789", "RW", "", "the string supplied is too long to be a phone number"}, // too long
 	}
 
-	for _, tc := range tcs {
-		parsed, err := parsePhoneOrShortcode(tc.input, tc.country)
+	for i, tc := range testCases {
+		urn, err := urns.ParsePhone(tc.input, tc.country)
+
+		if tc.expectedErr != "" {
+			if assert.EqualError(t, err, tc.expectedErr, "%d: expected error for %s, %s", i, tc.input, tc.country) {
+				assert.Equal(t, urns.NilURN, urn)
 
-		if tc.expected != "" {
-			assert.NoError(t, err, "unexpected error for '%s'", tc.input)
-			assert.Equal(t, tc.expected, parsed, "result mismatch for '%s'", tc.input)
+				// check parsing as just a number rather than a phone URN
+				num, err := urns.ParseNumber(tc.input, tc.country)
+				assert.EqualError(t, err, tc.expectedErr)
+				assert.Equal(t, "", num)
+			}
 		} else {
-			assert.Error(t, err, "expected error for '%s'", tc.input)
+			if assert.NoError(t, err, "%d: unexpected error for %s, %s", i, tc.input, tc.country) {
+				assert.Equal(t, tc.expectedURN, urn, "%d: URN mismatch for %s, %s", i, tc.input, tc.country)
+
+				// check parsing as just a number rather than a phone URN
+				num, err := urns.ParseNumber(tc.input, tc.country)
+				assert.NoError(t, err)
+				assert.Equal(t, tc.expectedURN.Path(), num)
+			}
 		}
 	}
 }
+
 func TestToLocalPhone(t *testing.T) {
 	tcs := []struct {
-		urn      URN
+		urn      urns.URN
 		country  i18n.Country
 		expected string
 	}{
@@ -86,6 +84,6 @@ func TestToLocalPhone(t *testing.T) {
 	}
 
 	for _, tc := range tcs {
-		assert.Equal(t, tc.expected, ToLocalPhone(tc.urn, tc.country), "local mismatch for '%s'", tc.urn)
+		assert.Equal(t, tc.expected, urns.ToLocalPhone(tc.urn, tc.country), "local mismatch for '%s'", tc.urn)
 	}
 }

urns/schemes.go

@@ -13,7 +13,7 @@ var emailRegex = regexp.MustCompile(`^[^\s@]+@[^\s@]+$`)
 var freshchatRegex = regexp.MustCompile(`^[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}/[0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12}$`)
 var viberRegex = regexp.MustCompile(`^[a-zA-Z0-9_=/+]{1,24}$`)
 var lineRegex = regexp.MustCompile(`^[a-zA-Z0-9_]{1,36}$`)
-var telRegex = regexp.MustCompile(`^\+?[a-zA-Z0-9]{1,64}$`)
+var phoneRegex = regexp.MustCompile(`^\+?\d{1,64}$`)
 var twitterHandleRegex = regexp.MustCompile(`^[a-zA-Z0-9_]{1,15}$`)
 var webchatRegex = regexp.MustCompile(`^[a-zA-Z0-9]{24}(:[^\s@]+@[^\s@]+)?$`)
 
@@ -129,7 +129,7 @@ var Phone = &Scheme{
 		// might have alpha characters in it
 		return strings.ToUpper(path)
 	},
-	Validate: func(path string) bool { return telRegex.MatchString(path) },
+	Validate: func(path string) bool { return phoneRegex.MatchString(path) },
 	Format: func(path string) string {
 		parsed, err := phonenumbers.Parse(path, "")
 		if err != nil {

urns/urns_test.go

@@ -161,12 +161,11 @@ func TestValidate(t *testing.T) {
 		{"tel:+250123", ""},
 		{"tel:1337", ""},
 		{"tel:1", ""}, // one digit shortcodes are a thing
-		{"tel:PRIZES", ""},
-		{"tel:cellbroadcastchannel50", ""},
 
 		// invalid tel numbers
-		{"tel:07883 83383", "invalid path component"}, // can't have spaces
 		{"tel:", "cannot be empty"},                   // need a path
+		{"tel:07883 83383", "invalid path component"}, // can't have spaces
+		{"tel:PRIZES", "invalid path component"},      // vanity numbers should be parsed into real digits
 
 		// twitter handles
 		{"twitter:jimmyjo", ""},