Repository: twilio/twilio-python

Issue:
Incoming SMS webhook failing validation (non standard SSL port)
Hi,

I have an incoming SMS webhook using HTTPS but on a non standard port (4433). The incoming POST fails validation unless the changes made in #392 are reverted (i.e. the port number left in for validation). The [security notes](https://www.twilio.com/docs/api/security#notes) don't mention non standard ports. Should only the standard port be removed if using HTTPS?

https://github.com/twilio/twilio-python/blob/119fe50863b2e9df89a56711e8d3410288709593/twilio/request_validator.py#L77

Discussion:

Patch:
diff --git a/twilio/request_validator.py b/twilio/request_validator.py
index 72cdb6298..c24a52acd 100644
--- a/twilio/request_validator.py
+++ b/twilio/request_validator.py
@@ -21,19 +21,42 @@ def compare(string1, string2):
     result = True
     for c1, c2 in izip(string1, string2):
         result &= c1 == c2
+
     return result
 
 
 def remove_port(uri):
     """Remove the port number from a URI
 
-    :param uri: full URI that Twilio requested on your server
+    :param uri: parsed URI that Twilio requested on your server
 
     :returns: full URI without a port number
     :rtype: str
     """
+    if not uri.port:
+        return uri.geturl()
+
     new_netloc = uri.netloc.split(':')[0]
     new_uri = uri._replace(netloc=new_netloc)
+
+    return new_uri.geturl()
+
+
+def add_port(uri):
+    """Add the port number to a URI
+
+    :param uri: parsed URI that Twilio requested on your server
+
+    :returns: full URI with a port number
+    :rtype: str
+    """
+    if uri.port:
+        return uri.geturl()
+    
+    port = 443 if uri.scheme == "https" else 80
+    new_netloc = uri.netloc + ":" + str(port)
+    new_uri = uri._replace(netloc=new_netloc)
+
     return new_uri.geturl()
 
 
@@ -82,17 +105,21 @@ class RequestValidator(object):
             params = {}
 
         parsed_uri = urlparse(uri)
-        if parsed_uri.scheme == "https" and parsed_uri.port:
-            uri = remove_port(parsed_uri)
+        uri_with_port = add_port(parsed_uri)
+        uri_without_port = remove_port(parsed_uri)
 
         valid_signature = False  # Default fail
+        valid_signature_with_port = False
         valid_body_hash = True  # May not receive body hash, so default succeed
 
         query = parse_qs(parsed_uri.query)
         if "bodySHA256" in query and isinstance(params, string_types):
             valid_body_hash = compare(self.compute_hash(params), query["bodySHA256"][0])
-            valid_signature = compare(self.compute_signature(uri, {}), signature)
-        else:
-            valid_signature = compare(self.compute_signature(uri, params), signature)
+            params = {}
+
+        #  check signature of uri with and without port, 
+        #  since sig generation on back end is inconsistent
+        valid_signature = compare(self.compute_signature(uri_without_port, params), signature)
+        valid_signature_with_port = compare(self.compute_signature(uri_with_port, params), signature)
 
-        return valid_signature and valid_body_hash
+        return valid_body_hash and (valid_signature or valid_signature_with_port)


Test patch:
diff --git a/tests/unit/test_request_validator.py b/tests/unit/test_request_validator.py
index c7f67d818..245c5ae43 100644
--- a/tests/unit/test_request_validator.py
+++ b/tests/unit/test_request_validator.py
@@ -53,6 +53,20 @@ class ValidationTest(unittest.TestCase):
         uri = self.uri.replace(".com", ".com:1234")
         assert_true(self.validator.validate(uri, self.params, self.expected))
 
+    def test_validation_removes_port_on_http(self):
+        expected = "Zmvh+3yNM1Phv2jhDCwEM3q5ebU="  # hash of http uri with port 1234
+        uri = self.uri.replace(".com", ".com:1234").replace("https", "http")
+        assert_true(self.validator.validate(uri, self.params, expected))
+
+    def test_validation_adds_port_on_https(self):
+        expected = "kvajT1Ptam85bY51eRf/AJRuM3w="  # hash of uri with port 443
+        assert_true(self.validator.validate(self.uri, self.params, expected))
+
+    def test_validation_adds_port_on_http(self):
+        uri = self.uri.replace("https", "http")
+        expected = "0ZXoZLH/DfblKGATFgpif+LLRf4="  # hash of uri with port 80
+        assert_true(self.validator.validate(uri, self.params, expected))
+
     def test_validation_of_body_succeeds(self):
         uri = self.uriWithBody
         is_valid = self.validator.validate(uri, self.body, "a9nBmqA0ju/hNViExpshrM61xv4=")