Enforced encoding before stipping hmac and gettoken from params

Author: MarkusH

CHANGELOG.md

@@ -25,6 +25,13 @@
 
 * Removed the deprecated `cp` argument from `laterpay.ItemDefinition`
 
+* Reliably ignore `hmac` and `gettoken` parameters when creating the signature
+  message. In the past `signing.sign()` and `signing.verify()` stripped those
+  keys when a `dict()` was passed from the passed function arguments but not
+  for lists or tuples. Note that as a result the provided parameters are not
+  touched anymore and calling either function will not have side-effects on
+  the provided arguments.
+
 
 ## 4.6.0
 

laterpay/signing.py

@@ -16,6 +16,7 @@
 from . import compat
 
 ALLOWED_METHODS = ('GET', 'POST', 'PUT', 'DELETE', 'OPTIONS', 'HEAD')
+MESSAGE_FORMAT = '{method}&{url}&{params}'
 
 
 def time_independent_HMAC_compare(a, b):
@@ -145,29 +146,34 @@ def create_base_message(params, url, method='POST'):
 
     http://docs.laterpay.net/platform/intro/signing_urls/
     """
-    msg = '{method}&{url}&{params}'
-
-    method = compat.encode_if_unicode(method).upper()
-
-    url = quote(compat.encode_if_unicode(url), safe='')
-
+    # Process method
+    method = compat.stringify(method).upper()
     if method not in ALLOWED_METHODS:
         raise ValueError('method should be one of: {}'.format(ALLOWED_METHODS))
 
+    # Process params
     params = normalise_param_structure(params)
-
+    if 'hmac' in params:
+        params.pop('hmac')
+    if 'gettoken' in params:
+        params.pop('gettoken')
     params = {
+        # urlquote all keys and values
         quote(key, safe=''): [quote(value, safe='') for value in values]
         for key, values
         in six.iteritems(params)
     }
-
     params = _sort_params(params)
-
     param_str = '&'.join('{}={}'.format(k, v) for k, v in params)
     param_str = quote(param_str, safe='')
 
-    return msg.format(method=method, url=url, params=param_str)
+    # Process url
+    url = compat.stringify(url)
+    url_parsed = urlparse(url)
+    url = url_parsed.scheme + "://" + url_parsed.netloc + url_parsed.path
+    url = quote(url, safe='')
+
+    return MESSAGE_FORMAT.format(method=method, url=url, params=param_str)
 
 
 def sign(secret, params, url, method='POST'):
@@ -182,21 +188,8 @@ def sign(secret, params, url, method='POST'):
     :param method: HTTP method used to transport the signed data
                    ('POST' is default)
     """
-    if 'hmac' in params:
-        params.pop('hmac')
-
-    if 'gettoken' in params:
-        params.pop('gettoken')
-
-    secret = compat.encode_if_unicode(secret)
-
-    url_parsed = urlparse(url)
-    base_url = url_parsed.scheme + "://" + url_parsed.netloc + url_parsed.path
-
-    msg = create_base_message(params, base_url, method=method)
-
-    mac = create_HMAC(secret, msg)
-
+    message = create_base_message(params, url, method=method)
+    mac = create_HMAC(secret, message)
     return mac
 
 

tests/test_signing.py

@@ -86,44 +86,50 @@ def test_create_message_wrong_method(self):
             signing.create_base_message(params, url, method='WRONG')
 
     def test_sign(self):
+        signature = '5b341f4321476715ab1ae252794783c6dffa32dbdcc94512193ea3cf'
         params = {
             u'parĄm1': u'valuĘ',
-            'param2': ['value2', 'value3'],
-            'hmac': 'to-be-removed',
-            'gettoken': 'to-be-removed-too',
+            b'par\xc4\x84m1': ['value2', 'value3'],
+            b'hmac': 'will-be-removed',
+            u'gettoken': 'will-be-removed-too',
         }
-        url = u'https://endpoint.com/api'
+        url = b'https://endpoint.com/api'
+        secret = b'secret'
 
-        secret = u'secret'  # unicode is what we usually get from api/db..
+        # sha224 hmac
+        self.assertEqual(signing.sign(secret, params, url), signature)
 
-        mac = signing.sign(secret, params, url)
-        self.assertNotIn('hmac', params)
-        self.assertNotIn('gettoken', params)
+        # Test that `hmac` and `gettoken` are not being removed from the
+        # original arguments passed to the function
+        self.assertIn(b'hmac', params)
+        self.assertIn(u'gettoken', params)
 
-        # sha224 hmac
-        self.assertEqual(
-            mac,
-            '346f3d53ad762f3ed3fb7f2427dec2bbfaf0338bb7f91f0460aff15c',
-        )
+        del params[b'hmac']
+        del params[u'gettoken']
+        self.assertEqual(signing.sign(secret, params, url), signature)
 
     def test_sign_unicode_secret(self):
+        signature = '635cef6498fc5f1a829275cc1b24a191d5267d6023034e3e0953e4c6'
         params = {
             u'parĄm1': u'valuĘ',
             'param2': ['value2', 'value3'],
-            'hmac': 'will-be-removed',
-            'gettoken': 'will-be-removed-too',
+            u'hmac': 'to-be-removed',
+            b'gettoken': 'to-be-removed-too',
         }
         url = u'https://endpoint.com/api'
-
         secret = u'☃🐍'  # unicode is what we usually get from api/db..
 
-        mac = signing.sign(secret, params, url)
-
         # sha224 hmac
-        self.assertEqual(
-            mac,
-            '635cef6498fc5f1a829275cc1b24a191d5267d6023034e3e0953e4c6',
-        )
+        self.assertEqual(signing.sign(secret, params, url), signature)
+
+        # Test that `hmac` and `gettoken` are not being removed from the
+        # original arguments passed to the function
+        self.assertIn(u'hmac', params)
+        self.assertIn(b'gettoken', params)
+
+        del params[u'hmac']
+        del params[b'gettoken']
+        self.assertEqual(signing.sign(secret, params, url), signature)
 
     def test_verify_byte_signature(self):
         params = {