Merge pull request #651 from hoylen/fix-uri-normalization

Fixed space encoding in base string URI used in the signature base st…

4 files changed, 71 insertions, 31 deletions

diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index 9e0efda..f49fb92 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -3,6 +3,7 @@ Changelog
 
 TBD
 ------------------
+* #650 Fixed space encoding in base string URI used in the signature base string.
 * #652: Fixed OIDC /token response which wrongly returned "&state=None"
 
 3.0.1 (2019-01-24)
diff --git a/oauthlib/oauth1/rfc5849/__init__.py b/oauthlib/oauth1/rfc5849/__init__.py
index 7313286..4f462bb 100644
--- a/oauthlib/oauth1/rfc5849/__init__.py
+++ b/oauthlib/oauth1/rfc5849/__init__.py
@@ -133,12 +133,11 @@ class Client(object):
         log.debug("Collected params: {0}".format(collected_params))
 
         normalized_params = signature.normalize_parameters(collected_params)
-        normalized_uri = signature.normalize_base_string_uri(uri,
-                                                             headers.get('Host', None))
+        normalized_uri = signature.base_string_uri(uri, headers.get('Host', None))
         log.debug("Normalized params: {0}".format(normalized_params))
         log.debug("Normalized URI: {0}".format(normalized_uri))
 
-        base_string = signature.construct_base_string(request.http_method,
+        base_string = signature.signature_base_string(request.http_method,
                                                       normalized_uri, normalized_params)
 
         log.debug("Signing: signature base string: {0}".format(base_string))
diff --git a/oauthlib/oauth1/rfc5849/signature.py b/oauthlib/oauth1/rfc5849/signature.py
index e90d6f3..f899aca 100644
--- a/oauthlib/oauth1/rfc5849/signature.py
+++ b/oauthlib/oauth1/rfc5849/signature.py
@@ -40,9 +40,10 @@ except ImportError:
 
 log = logging.getLogger(__name__)
 
-def construct_base_string(http_method, base_string_uri,
+
+def signature_base_string(http_method, base_str_uri,
                           normalized_encoded_request_parameters):
-    """**String Construction**
+    """**Construct the signature base string.**
     Per `section 3.4.1.1`_ of the spec.
 
     For example, the HTTP request::
@@ -90,7 +91,7 @@ def construct_base_string(http_method, base_string_uri,
     #
     # .. _`Section 3.4.1.2`: https://tools.ietf.org/html/rfc5849#section-3.4.1.2
     # .. _`Section 3.4.6`: https://tools.ietf.org/html/rfc5849#section-3.4.6
-    base_string += utils.escape(base_string_uri)
+    base_string += utils.escape(base_str_uri)
 
     # 4.  An "&" character (ASCII code 38).
     base_string += '&'
@@ -105,9 +106,9 @@ def construct_base_string(http_method, base_string_uri,
     return base_string
 
 
-def normalize_base_string_uri(uri, host=None):
+def base_string_uri(uri, host=None):
     """**Base String URI**
-    Per `section 3.4.1.2`_ of the spec.
+    Per `section 3.4.1.2`_ of RFC 5849.
 
     For example, the HTTP request::
 
@@ -177,7 +178,31 @@ def normalize_base_string_uri(uri, host=None):
         if (scheme, port) in default_ports:
             netloc = host
 
-    return urlparse.urlunparse((scheme, netloc, path, params, '', ''))
+    v = urlparse.urlunparse((scheme, netloc, path, params, '', ''))
+
+    # RFC 5849 does not specify which characters are encoded in the
+    # "base string URI", nor how they are encoded - which is very bad, since
+    # the signatures won't match if there are any differences. Fortunately,
+    # most URIs only use characters that are clearly not encoded (e.g. digits
+    # and A-Z, a-z), so have avoided any differences between implementations.
+    #
+    # The example from its section 3.4.1.2 illustrates that spaces in
+    # the path are percent encoded. But it provides no guidance as to what other
+    # characters (if any) must be encoded (nor how); nor if characters in the
+    # other components are to be encoded or not.
+    #
+    # This implementation **assumes** that **only** the space is percent-encoded
+    # and it is done to the entire value (not just to spaces in the path).
+    #
+    # This code may need to be changed if it is discovered that other characters
+    # are expected to be encoded.
+    #
+    # Note: the "base string URI" returned by this function will be encoded
+    # again before being concatenated into the "signature base string". So any
+    # spaces in the URI will actually appear in the "signature base string"
+    # as "%2520" (the "%20" further encoded according to section 3.6).
+
+    return v.replace(' ', '%20')
 
 
 # ** Request Parameters **
@@ -624,13 +649,15 @@ def verify_hmac_sha1(request, client_secret=None,
 
     """
     norm_params = normalize_parameters(request.params)
-    uri = normalize_base_string_uri(request.uri)
-    base_string = construct_base_string(request.http_method, uri, norm_params)
-    signature = sign_hmac_sha1(base_string, client_secret,
+    bs_uri = base_string_uri(request.uri)
+    sig_base_str = signature_base_string(request.http_method, bs_uri,
+                                         norm_params)
+    signature = sign_hmac_sha1(sig_base_str, client_secret,
                                resource_owner_secret)
     match = safe_string_equals(signature, request.signature)
     if not match:
-        log.debug('Verify HMAC-SHA1 failed: sig base string: %s', base_string)
+        log.debug('Verify HMAC-SHA1 failed: signature base string: %s',
+                  sig_base_str)
     return match
 
 
@@ -657,16 +684,18 @@ def verify_rsa_sha1(request, rsa_public_key):
     .. _`RFC2616 section 5.2`: https://tools.ietf.org/html/rfc2616#section-5.2
     """
     norm_params = normalize_parameters(request.params)
-    uri = normalize_base_string_uri(request.uri)
-    message = construct_base_string(request.http_method, uri, norm_params).encode('utf-8')
+    bs_uri = base_string_uri(request.uri)
+    sig_base_str = signature_base_string(request.http_method, bs_uri,
+                                         norm_params).encode('utf-8')
     sig = binascii.a2b_base64(request.signature.encode('utf-8'))
 
     alg = _jwt_rs1_signing_algorithm()
     key = _prepare_key_plus(alg, rsa_public_key)
 
-    verify_ok = alg.verify(message, key, sig)
+    verify_ok = alg.verify(sig_base_str, key, sig)
     if not verify_ok:
-        log.debug('Verify RSA-SHA1 failed: sig base string: %s', message)
+        log.debug('Verify RSA-SHA1 failed: signature base string: %s',
+                  sig_base_str)
     return verify_ok
 
 
diff --git a/tests/oauth1/rfc5849/test_signatures.py b/tests/oauth1/rfc5849/test_signatures.py
index 48609e5..bb0dc78 100644
--- a/tests/oauth1/rfc5849/test_signatures.py
+++ b/tests/oauth1/rfc5849/test_signatures.py
@@ -3,8 +3,8 @@ from __future__ import absolute_import, unicode_literals
 
 from oauthlib.common import unicode_type
 from oauthlib.oauth1.rfc5849.signature import (collect_parameters,
-                                               construct_base_string,
-                                               normalize_base_string_uri,
+                                               signature_base_string,
+                                               base_string_uri,
                                                normalize_parameters,
                                                sign_hmac_sha1,
                                                sign_hmac_sha1_with_client,
@@ -79,7 +79,7 @@ class SignatureTests(TestCase):
             resource_owner_secret = self.resource_owner_secret
         )
 
-    def test_construct_base_string(self):
+    def test_signature_base_string(self):
         """
         Example text to be turned into a base string::
 
@@ -104,20 +104,20 @@ class SignatureTests(TestCase):
             D%2522137131201%2522%252Coauth_nonce%253D%25227d8f3e4a%2522%252Coau
             th_signature%253D%2522bYT5CMsGcbgUdFHObYMEfcx6bsw%25253D%2522
         """
-        self.assertRaises(ValueError, construct_base_string,
+        self.assertRaises(ValueError, signature_base_string,
                           self.http_method,
                           self.base_string_url,
                           self.normalized_encoded_request_parameters)
-        self.assertRaises(ValueError, construct_base_string,
+        self.assertRaises(ValueError, signature_base_string,
                           self.http_method.decode('utf-8'),
                           self.base_string_url,
                           self.normalized_encoded_request_parameters)
-        self.assertRaises(ValueError, construct_base_string,
+        self.assertRaises(ValueError, signature_base_string,
                           self.http_method.decode('utf-8'),
                           self.base_string_url.decode('utf-8'),
                           self.normalized_encoded_request_parameters)
 
-        base_string = construct_base_string(
+        base_string = signature_base_string(
             self.http_method.decode('utf-8'),
             self.base_string_url.decode('utf-8'),
             self.normalized_encoded_request_parameters.decode('utf-8')
@@ -125,7 +125,7 @@ class SignatureTests(TestCase):
 
         self.assertEqual(self.control_base_string, base_string)
 
-    def test_normalize_base_string_uri(self):
+    def test_base_string_uri(self):
         """
         Example text to be turned into a normalized base string uri::
 
@@ -137,33 +137,44 @@ class SignatureTests(TestCase):
             https://www.example.net:8080/
         """
 
+        # test first example from RFC 5849 section 3.4.1.2.
+        # Note: there is a space between "r" and "v"
+        uri = 'http://EXAMPLE.COM:80/r v/X?id=123'
+        self.assertEqual(base_string_uri(uri),
+                         'http://example.com/r%20v/X')
+
+        # test second example from RFC 5849 section 3.4.1.2.
+        uri = 'https://www.example.net:8080/?q=1'
+        self.assertEqual(base_string_uri(uri),
+                         'https://www.example.net:8080/')
+
         # test for unicode failure
         uri = b"www.example.com:8080"
-        self.assertRaises(ValueError, normalize_base_string_uri, uri)
+        self.assertRaises(ValueError, base_string_uri, uri)
 
         # test for missing scheme
         uri = "www.example.com:8080"
-        self.assertRaises(ValueError, normalize_base_string_uri, uri)
+        self.assertRaises(ValueError, base_string_uri, uri)
 
         # test a URI with the default port
         uri = "http://www.example.com:80/"
-        self.assertEqual(normalize_base_string_uri(uri),
+        self.assertEqual(base_string_uri(uri),
                          "http://www.example.com/")
 
         # test a URI missing a path
         uri = "http://www.example.com"
-        self.assertEqual(normalize_base_string_uri(uri),
+        self.assertEqual(base_string_uri(uri),
                          "http://www.example.com/")
 
         # test a relative URI
         uri = "/a-host-relative-uri"
         host = "www.example.com"
-        self.assertRaises(ValueError, normalize_base_string_uri, (uri, host))
+        self.assertRaises(ValueError, base_string_uri, (uri, host))
 
         # test overriding the URI's netloc with a host argument
         uri = "http://www.example.com/a-path"
         host = "alternatehost.example.com"
-        self.assertEqual(normalize_base_string_uri(uri, host),
+        self.assertEqual(base_string_uri(uri, host),
                          "http://alternatehost.example.com/a-path")
 
     def test_collect_parameters(self):