Repository: boto/botocore
Pull request #434 (2015-01-14 21:44:29)

Fix canonical query string sorting for Signature Version 4
We were previously sorting by first creating a list of strings
of `'%s=%s' % (key, value)`.  But really we should be:

1. Sorting by key names
2. In the case of repeated keys, sort then by values.

Fixes aws/aws-cli#976

Added unit tests.  Also verified the previously failing CLI command now works:

```
$ aws cloudsearchdomain search --endpoint-url myendpoint --search-query 'George Lucas' --query-options '{"defaultOperator": "and", "fields":["directors^10"]}' --query hits.hit[].fields.[title[0],rating[0]] --output table
---------------------------------------------------------
|                        Search                         |
+------------------------------------------------+------+
|  Star Wars                                     |  8.7 |
|  Star Wars: Episode III - Revenge of the Sith  |  7.7 |
|  Star Wars: Episode I - The Phantom Menace     |  6.5 |
|  Star Wars: Episode II - Attack of the Clones  |  6.7 |
|  American Graffiti                             |  7.5 |
|  THX 1138                                      |  6.7 |
+------------------------------------------------+------+
```

cc @kyleknap @danielgtaylor 


Patch:
diff --git a/botocore/auth.py b/botocore/auth.py
index 6b21abf3c..77b433e17 100644
--- a/botocore/auth.py
+++ b/botocore/auth.py
@@ -171,7 +171,6 @@ class SigV4Auth(BaseSigner):
         return header_map
 
     def canonical_query_string(self, request):
-        cqs = ''
         # The query string can come from two parts.  One is the
         # params attribute of the request.  The other is from the request
         # url (in which case we have to re-split the url into its components
@@ -180,33 +179,31 @@ class SigV4Auth(BaseSigner):
             return self._canonical_query_string_params(request.params)
         else:
             return self._canonical_query_string_url(urlsplit(request.url))
-        return cqs
 
     def _canonical_query_string_params(self, params):
         l = []
-        for param in params:
+        for param in sorted(params):
             value = str(params[param])
             l.append('%s=%s' % (quote(param, safe='-_.~'),
                                 quote(value, safe='-_.~')))
-        l = sorted(l)
         cqs = '&'.join(l)
         return cqs
 
     def _canonical_query_string_url(self, parts):
-        buf = ''
+        canonical_query_string = ''
         if parts.query:
-            qsa = parts.query.split('&')
-            qsa = [a.split('=', 1) for a in qsa]
-            split_qsa = []
-            for q in qsa:
-                if len(q) == 2:
-                    split_qsa.append('%s=%s' % (q[0], q[1]))
-                elif len(q) == 1:
-                    split_qsa.append('%s=' % q[0])
-            if len(split_qsa) > 0:
-                split_qsa.sort()
-                buf += '&'.join(split_qsa)
-        return buf
+            # [(key, value), (key2, value2)]
+            key_val_pairs = []
+            for pair in parts.query.split('&'):
+                key, _, value = pair.partition('=')
+                key_val_pairs.append((key, value))
+            sorted_key_vals = []
+            # Sort by the key names, and in the case of
+            # repeated keys, then sort by the value.
+            for key, value in sorted(key_val_pairs):
+                sorted_key_vals.append('%s=%s' % (key, value))
+            canonical_query_string = '&'.join(sorted_key_vals)
+        return canonical_query_string
 
     def canonical_headers(self, headers_to_sign):
         """


Test patch:
diff --git a/tests/unit/auth/test_signers.py b/tests/unit/auth/test_signers.py
index b66609735..f6a7468d5 100644
--- a/tests/unit/auth/test_signers.py
+++ b/tests/unit/auth/test_signers.py
@@ -290,6 +290,31 @@ class TestS3SigV4Auth(unittest.TestCase):
         self.assertEqual('marker=%C3%A4%C3%B6%C3%BC-01.txt&prefix=', cqs)
 
 
+class TestSigV4(unittest.TestCase):
+    def setUp(self):
+        self.credentials = botocore.credentials.Credentials(
+            access_key='foo', secret_key='bar')
+
+    def test_canonical_query_string(self):
+        request = AWSRequest()
+        request.url = (
+            'https://search-testdomain1-j67dwxlet67gf7ghwfmik2c67i.us-west-2.'
+            'cloudsearch.amazonaws.com/'
+            '2013-01-01/search?format=sdk&pretty=true&'
+            'q.options=%7B%22defaultOperator%22%3A%20%22and%22%2C%20%22'
+            'fields%22%3A%5B%22directors%5E10%22%5D%7D&q=George%20Lucas'
+        )
+        request.method = 'GET'
+        auth = botocore.auth.SigV4Auth(
+            self.credentials, 'cloudsearchdomain', 'us-west-2')
+        actual = auth.canonical_query_string(request)
+        # Here 'q' should come before 'q.options'.
+        expected = ("format=sdk&pretty=true&q=George%20Lucas&q.options=%7B%22"
+                    "defaultOperator%22%3A%20%22and%22%2C%20%22fields%22%3A%5B"
+                    "%22directors%5E10%22%5D%7D")
+        self.assertEqual(actual, expected)
+
+
 class TestSigV4Resign(unittest.TestCase):
 
     maxDiff = None