s3api: Properly decode MPU request parameters before using them

Specifically, parameters that may contain non-ASCII characters,
such as the prefix and marker to list current uploads.

Change-Id: Icfae68825f94ddf2412c0274c3d500e265117e8e

2 files changed, 70 insertions, 52 deletions

diff --git a/swift/common/middleware/s3api/controllers/multi_upload.py b/swift/common/middleware/s3api/controllers/multi_upload.py
index f86fde156..40c70ef9a 100644
--- a/swift/common/middleware/s3api/controllers/multi_upload.py
+++ b/swift/common/middleware/s3api/controllers/multi_upload.py
@@ -71,7 +71,8 @@ from swift.common import constraints
 from swift.common.swob import Range, bytes_to_wsgi, normalize_etag, wsgi_to_str
 from swift.common.utils import json, public, reiterate, md5
 from swift.common.db import utf8encode
-from swift.common.request_helpers import get_container_update_override_key
+from swift.common.request_helpers import get_container_update_override_key, \
+    get_param
 
 from six.moves.urllib.parse import quote, urlparse
 
@@ -174,16 +175,16 @@ class PartController(Controller):
                                   'Unexpected query string parameter')
 
         try:
-            part_number = int(req.params['partNumber'])
+            part_number = int(get_param(req, 'partNumber'))
             if part_number < 1 or self.conf.max_upload_part_num < part_number:
                 raise Exception()
         except Exception:
             err_msg = 'Part number must be an integer between 1 and %d,' \
                       ' inclusive' % self.conf.max_upload_part_num
-            raise InvalidArgument('partNumber', req.params['partNumber'],
+            raise InvalidArgument('partNumber', get_param(req, 'partNumber'),
                                   err_msg)
 
-        upload_id = req.params['uploadId']
+        upload_id = get_param(req, 'uploadId')
         _get_upload_info(req, self.app, upload_id)
 
         req.container_name += MULTIUPLOAD_SUFFIX
@@ -292,13 +293,13 @@ class UploadsController(Controller):
                     non_delimited_uploads.append(upload)
             return non_delimited_uploads, sorted(common_prefixes)
 
-        encoding_type = req.params.get('encoding-type')
+        encoding_type = get_param(req, 'encoding-type')
         if encoding_type is not None and encoding_type != 'url':
             err_msg = 'Invalid Encoding Method specified in Request'
             raise InvalidArgument('encoding-type', encoding_type, err_msg)
 
-        keymarker = req.params.get('key-marker', '')
-        uploadid = req.params.get('upload-id-marker', '')
+        keymarker = get_param(req, 'key-marker', '')
+        uploadid = get_param(req, 'upload-id-marker', '')
         maxuploads = req.get_validated_param(
             'max-uploads', DEFAULT_MAX_UPLOADS, DEFAULT_MAX_UPLOADS)
 
@@ -312,7 +313,7 @@ class UploadsController(Controller):
         elif keymarker:
             query.update({'marker': '%s/~' % (keymarker)})
         if 'prefix' in req.params:
-            query.update({'prefix': req.params['prefix']})
+            query.update({'prefix': get_param(req, 'prefix')})
 
         container = req.container_name + MULTIUPLOAD_SUFFIX
         uploads = []
@@ -341,8 +342,8 @@ class UploadsController(Controller):
                            is_part.search(obj.get('name', '')) is None]
             new_prefixes = []
             if 'delimiter' in req.params:
-                prefix = req.params.get('prefix', '')
-                delimiter = req.params['delimiter']
+                prefix = get_param(req, 'prefix', '')
+                delimiter = get_param(req, 'delimiter')
                 new_uploads, new_prefixes = separate_uploads(
                     new_uploads, prefix, delimiter)
             uploads.extend(new_uploads)
@@ -369,9 +370,10 @@ class UploadsController(Controller):
         SubElement(result_elem, 'NextKeyMarker').text = nextkeymarker
         SubElement(result_elem, 'NextUploadIdMarker').text = nextuploadmarker
         if 'delimiter' in req.params:
-            SubElement(result_elem, 'Delimiter').text = req.params['delimiter']
+            SubElement(result_elem, 'Delimiter').text = \
+                get_param(req, 'delimiter')
         if 'prefix' in req.params:
-            SubElement(result_elem, 'Prefix').text = req.params['prefix']
+            SubElement(result_elem, 'Prefix').text = get_param(req, 'prefix')
         SubElement(result_elem, 'MaxUploads').text = str(maxuploads)
         if encoding_type is not None:
             SubElement(result_elem, 'EncodingType').text = encoding_type
@@ -492,12 +494,12 @@ class UploadController(Controller):
             except ValueError:
                 return False
 
-        encoding_type = req.params.get('encoding-type')
+        encoding_type = get_param(req, 'encoding-type')
         if encoding_type is not None and encoding_type != 'url':
             err_msg = 'Invalid Encoding Method specified in Request'
             raise InvalidArgument('encoding-type', encoding_type, err_msg)
 
-        upload_id = req.params['uploadId']
+        upload_id = get_param(req, 'uploadId')
         _get_upload_info(req, self.app, upload_id)
 
         maxparts = req.get_validated_param(
@@ -572,7 +574,7 @@ class UploadController(Controller):
         SubElement(result_elem, 'MaxParts').text = str(maxparts)
         if 'encoding-type' in req.params:
             SubElement(result_elem, 'EncodingType').text = \
-                req.params['encoding-type']
+                get_param(req, 'encoding-type')
         SubElement(result_elem, 'IsTruncated').text = \
             'true' if truncated else 'false'
 
@@ -595,7 +597,7 @@ class UploadController(Controller):
         """
         Handles Abort Multipart Upload.
         """
-        upload_id = req.params['uploadId']
+        upload_id = get_param(req, 'uploadId')
         _get_upload_info(req, self.app, upload_id)
 
         # First check to see if this multi-part upload was already
@@ -641,7 +643,7 @@ class UploadController(Controller):
         """
         Handles Complete Multipart Upload.
         """
-        upload_id = req.params['uploadId']
+        upload_id = get_param(req, 'uploadId')
         resp = _get_upload_info(req, self.app, upload_id)
         headers = {'Accept': 'application/json',
                    sysmeta_header('object', 'upload-id'): upload_id}
diff --git a/test/functional/s3api/test_multi_upload.py b/test/functional/s3api/test_multi_upload.py
index 8038a9239..e5c61661a 100644
--- a/test/functional/s3api/test_multi_upload.py
+++ b/test/functional/s3api/test_multi_upload.py
@@ -24,7 +24,7 @@ import boto
 from distutils.version import StrictVersion
 
 import six
-from six.moves import zip, zip_longest
+from six.moves import urllib, zip, zip_longest
 
 import test.functional as tf
 from swift.common.middleware.s3api.etree import fromstring, tostring, \
@@ -145,40 +145,56 @@ class TestS3ApiMultiUpload(S3ApiBase):
         self.assertEqual(len(uploads), len(keys))  # sanity
 
         # List Multipart Uploads
-        query = 'uploads'
-        status, headers, body = \
-            self.conn.make_request('GET', bucket, query=query)
-        self.assertEqual(status, 200)
-        self.assertCommonResponseHeaders(headers)
-        self.assertTrue('content-type' in headers)
-        self.assertEqual(headers['content-type'], 'application/xml')
-        self.assertTrue('content-length' in headers)
-        self.assertEqual(headers['content-length'], str(len(body)))
-        elem = fromstring(body, 'ListMultipartUploadsResult')
-        self.assertEqual(elem.find('Bucket').text, bucket)
-        self.assertIsNone(elem.find('KeyMarker').text)
-        self.assertEqual(elem.find('NextKeyMarker').text, uploads[-1][0])
-        self.assertIsNone(elem.find('UploadIdMarker').text)
-        self.assertEqual(elem.find('NextUploadIdMarker').text, uploads[-1][1])
-        self.assertEqual(elem.find('MaxUploads').text, '1000')
-        self.assertTrue(elem.find('EncodingType') is None)
-        self.assertEqual(elem.find('IsTruncated').text, 'false')
-        self.assertEqual(len(elem.findall('Upload')), 3)
-        for (expected_key, expected_upload_id), u in \
-                zip(uploads, elem.findall('Upload')):
-            key = u.find('Key').text
-            upload_id = u.find('UploadId').text
-            self.assertEqual(expected_key, key)
-            self.assertEqual(expected_upload_id, upload_id)
-            self.assertEqual(u.find('Initiator/ID').text,
-                             self.conn.user_id)
-            self.assertEqual(u.find('Initiator/DisplayName').text,
-                             self.conn.user_id)
-            self.assertEqual(u.find('Owner/ID').text, self.conn.user_id)
-            self.assertEqual(u.find('Owner/DisplayName').text,
-                             self.conn.user_id)
-            self.assertEqual(u.find('StorageClass').text, 'STANDARD')
-            self.assertTrue(u.find('Initiated').text is not None)
+        expected_uploads_list = [uploads]
+        for upload in uploads:
+            expected_uploads_list.append([upload])
+        for expected_uploads in expected_uploads_list:
+            query = 'uploads'
+            if len(expected_uploads) == 1:
+                query += '&' + urllib.parse.urlencode(
+                    {'prefix': expected_uploads[0][0]})
+            status, headers, body = \
+                self.conn.make_request('GET', bucket, query=query)
+            self.assertEqual(status, 200)
+            self.assertCommonResponseHeaders(headers)
+            self.assertTrue('content-type' in headers)
+            self.assertEqual(headers['content-type'], 'application/xml')
+            self.assertTrue('content-length' in headers)
+            self.assertEqual(headers['content-length'], str(len(body)))
+            elem = fromstring(body, 'ListMultipartUploadsResult')
+            self.assertEqual(elem.find('Bucket').text, bucket)
+            self.assertIsNone(elem.find('KeyMarker').text)
+            if len(expected_uploads) > 1:
+                self.assertEqual(elem.find('NextKeyMarker').text,
+                                 expected_uploads[-1][0])
+            else:
+                self.assertIsNone(elem.find('NextKeyMarker').text)
+            self.assertIsNone(elem.find('UploadIdMarker').text)
+            if len(expected_uploads) > 1:
+                self.assertEqual(elem.find('NextUploadIdMarker').text,
+                                 expected_uploads[-1][1])
+            else:
+                self.assertIsNone(elem.find('NextUploadIdMarker').text)
+            self.assertEqual(elem.find('MaxUploads').text, '1000')
+            self.assertTrue(elem.find('EncodingType') is None)
+            self.assertEqual(elem.find('IsTruncated').text, 'false')
+            self.assertEqual(len(elem.findall('Upload')),
+                             len(expected_uploads))
+            for (expected_key, expected_upload_id), u in \
+                    zip(expected_uploads, elem.findall('Upload')):
+                key = u.find('Key').text
+                upload_id = u.find('UploadId').text
+                self.assertEqual(expected_key, key)
+                self.assertEqual(expected_upload_id, upload_id)
+                self.assertEqual(u.find('Initiator/ID').text,
+                                 self.conn.user_id)
+                self.assertEqual(u.find('Initiator/DisplayName').text,
+                                 self.conn.user_id)
+                self.assertEqual(u.find('Owner/ID').text, self.conn.user_id)
+                self.assertEqual(u.find('Owner/DisplayName').text,
+                                 self.conn.user_id)
+                self.assertEqual(u.find('StorageClass').text, 'STANDARD')
+                self.assertTrue(u.find('Initiated').text is not None)
 
         # Upload Part
         key, upload_id = uploads[0]