Only url-quote Keys when encoding-type=url

Previously, we'd even url-quote ETags, leading to weird things like

    <ETag>%22a27a822070b843ed6407f4f38afdaa20%22</ETag>

in listings, where those '%22's should be double quotes.

Change-Id: I0f5e0e410c8297a4898caae00c196fa3b3862100

swift/common/middleware/s3api/controllers/bucket.py

@@ -16,6 +16,8 @@
 from base64 import standard_b64encode as b64encode
 from base64 import standard_b64decode as b64decode
 
+from six.moves.urllib.parse import quote
+
 from swift.common.http import HTTP_OK
 from swift.common.utils import json, public, config_true_value
 
@@ -171,11 +173,12 @@ def GET(self, req):
                 SubElement(elem, 'Marker').text = req.params.get('marker')
                 if is_truncated and 'delimiter' in req.params:
                     if 'name' in objects[-1]:
-                        SubElement(elem, 'NextMarker').text = \
-                            objects[-1]['name']
-                    if 'subdir' in objects[-1]:
-                        SubElement(elem, 'NextMarker').text = \
-                            objects[-1]['subdir']
+                        name = objects[-1]['name']
+                    else:
+                        name = objects[-1]['subdir']
+                    if encoding_type == 'url':
+                        name = quote(name)
+                    SubElement(elem, 'NextMarker').text = name
             elif listing_type == 'version-2':
                 if is_truncated:
                     if 'name' in objects[-1]:
@@ -197,22 +200,26 @@ def GET(self, req):
         if 'delimiter' in req.params:
             SubElement(elem, 'Delimiter').text = req.params['delimiter']
 
-        if encoding_type is not None:
+        if encoding_type == 'url':
             SubElement(elem, 'EncodingType').text = encoding_type
 
         SubElement(elem, 'IsTruncated').text = \
             'true' if is_truncated else 'false'
 
         for o in objects:
             if 'subdir' not in o:
+                name = o['name']
+                if encoding_type == 'url':
+                    name = quote(name)
+
                 if listing_type == 'object-versions':
                     contents = SubElement(elem, 'Version')
-                    SubElement(contents, 'Key').text = o['name']
+                    SubElement(contents, 'Key').text = name
                     SubElement(contents, 'VersionId').text = 'null'
                     SubElement(contents, 'IsLatest').text = 'true'
                 else:
                     contents = SubElement(elem, 'Contents')
-                    SubElement(contents, 'Key').text = o['name']
+                    SubElement(contents, 'Key').text = name
                 SubElement(contents, 'LastModified').text = \
                     o['last_modified'][:-3] + 'Z'
                 if 's3_etag' in o:
@@ -231,9 +238,12 @@ def GET(self, req):
         for o in objects:
             if 'subdir' in o:
                 common_prefixes = SubElement(elem, 'CommonPrefixes')
-                SubElement(common_prefixes, 'Prefix').text = o['subdir']
+                name = o['subdir']
+                if encoding_type == 'url':
+                    name = quote(name)
+                SubElement(common_prefixes, 'Prefix').text = name
 
-        body = tostring(elem, encoding_type=encoding_type)
+        body = tostring(elem)
 
         return HTTPOk(body=body, content_type='application/xml')
 

swift/common/middleware/s3api/controllers/multi_upload.py

@@ -67,7 +67,7 @@
 from swift.common.utils import json, public
 from swift.common.db import utf8encode
 
-from six.moves.urllib.parse import urlparse  # pylint: disable=F0401
+from six.moves.urllib.parse import quote, urlparse
 
 from swift.common.middleware.s3api.controllers.base import Controller, \
     bucket_operation, object_operation, check_container_existence
@@ -319,7 +319,10 @@ def object_to_upload(object_info):
         # created.
         for u in uploads:
             upload_elem = SubElement(result_elem, 'Upload')
-            SubElement(upload_elem, 'Key').text = u['key']
+            name = u['key']
+            if encoding_type == 'url':
+                name = quote(name)
+            SubElement(upload_elem, 'Key').text = name
             SubElement(upload_elem, 'UploadId').text = u['upload_id']
             initiator_elem = SubElement(upload_elem, 'Initiator')
             SubElement(initiator_elem, 'ID').text = req.user_id
@@ -335,7 +338,7 @@ def object_to_upload(object_info):
             elem = SubElement(result_elem, 'CommonPrefixes')
             SubElement(elem, 'Prefix').text = p
 
-        body = tostring(result_elem, encoding_type=encoding_type)
+        body = tostring(result_elem)
 
         return HTTPOk(body=body, content_type='application/xml')
 
@@ -456,7 +459,10 @@ def filter_part_num_marker(o):
 
         result_elem = Element('ListPartsResult')
         SubElement(result_elem, 'Bucket').text = req.container_name
-        SubElement(result_elem, 'Key').text = req.object_name
+        name = req.object_name
+        if encoding_type == 'url':
+            name = quote(name)
+        SubElement(result_elem, 'Key').text = name
         SubElement(result_elem, 'UploadId').text = upload_id
 
         initiator_elem = SubElement(result_elem, 'Initiator')
@@ -484,7 +490,7 @@ def filter_part_num_marker(o):
             SubElement(part_elem, 'ETag').text = '"%s"' % i['hash']
             SubElement(part_elem, 'Size').text = str(i['bytes'])
 
-        body = tostring(result_elem, encoding_type=encoding_type)
+        body = tostring(result_elem)
 
         return HTTPOk(body=body, content_type='application/xml')
 

swift/common/middleware/s3api/etree.py

@@ -14,7 +14,6 @@
 # limitations under the License.
 
 import lxml.etree
-from urllib import quote
 from copy import deepcopy
 from pkg_resources import resource_stream  # pylint: disable-msg=E0611
 import six
@@ -86,7 +85,7 @@ def fromstring(text, root_tag=None, logger=None):
     return elem
 
 
-def tostring(tree, encoding_type=None, use_s3ns=True):
+def tostring(tree, use_s3ns=True):
     if use_s3ns:
         nsmap = tree.nsmap.copy()
         nsmap[None] = XMLNS_S3
@@ -96,16 +95,6 @@ def tostring(tree, encoding_type=None, use_s3ns=True):
         root.extend(deepcopy(tree.getchildren()))
         tree = root
 
-    if encoding_type == 'url':
-        tree = deepcopy(tree)
-        for e in tree.iter():
-            # Some elements are not url-encoded even when we specify
-            # encoding_type=url.
-            blacklist = ['LastModified', 'ID', 'DisplayName', 'Initiated']
-            if e.tag not in blacklist:
-                if isinstance(e.text, six.string_types):
-                    e.text = quote(e.text)
-
     return lxml.etree.tostring(tree, xml_declaration=True, encoding='UTF-8')
 
 

test/unit/common/middleware/s3api/test_bucket.py

@@ -17,6 +17,8 @@
 import cgi
 import mock
 
+from six.moves.urllib.parse import quote
+
 from swift.common import swob
 from swift.common.swob import Request
 from swift.common.utils import json
@@ -164,7 +166,33 @@ def test_bucket_GET(self):
 
         self.assertEqual(len(names), len(self.objects))
         for i in self.objects:
-            self.assertTrue(i[0] in names)
+            self.assertIn(i[0], names)
+
+    def test_bucket_GET_url_encoded(self):
+        bucket_name = 'junk'
+        req = Request.blank('/%s?encoding-type=url' % bucket_name,
+                            environ={'REQUEST_METHOD': 'GET'},
+                            headers={'Authorization': 'AWS test:tester:hmac',
+                                     'Date': self.get_date_header()})
+        status, headers, body = self.call_s3api(req)
+        self.assertEqual(status.split()[0], '200')
+
+        elem = fromstring(body, 'ListBucketResult')
+        name = elem.find('./Name').text
+        self.assertEqual(name, bucket_name)
+
+        objects = elem.iterchildren('Contents')
+
+        names = []
+        for o in objects:
+            names.append(o.find('./Key').text)
+            self.assertEqual('2011-01-05T02:19:14.275Z',
+                             o.find('./LastModified').text)
+            self.assertEqual('"0"', o.find('./ETag').text)
+
+        self.assertEqual(len(names), len(self.objects))
+        for i in self.objects:
+            self.assertIn(quote(i[0]), names)
 
     def test_bucket_GET_subdir(self):
         bucket_name = 'junk-subdir'