Merge lp:~gholt/swift/xmlcontrolchars into lp:~hudson-openstack/swift/trunk

[gholt]

Update to comply with XML 1.1: No NULLs in names allowed; control chars are converted to entities (ex: )

[lp:~gholt/swift/xmlcontrolchars]

317. By gholt on 2011-06-17

Fixed typo/bug with 20 instead of 0x20

[David Goetz (david-goetz)]

Looks good to me

review: Approve

[John Dickinson (notmyname)]

looks good to me

review: Approve

=== modified file 'swift/account/server.py'
--- swift/account/server.py	2011-06-10 18:36:02 +0000
+++ swift/account/server.py	2011-06-17 01:04:33 +0000
@@ -29,7 +29,7 @@
 
 from swift.common.db import AccountBroker
 from swift.common.utils import get_logger, get_param, hash_path, \
-    normalize_timestamp, split_path, storage_directory
+    normalize_timestamp, split_path, storage_directory, XML_EXTRA_ENTITIES
 from swift.common.constraints import ACCOUNT_LISTING_LIMIT, \
     check_mount, check_float, check_utf8
 from swift.common.db_replicator import ReplicatorRpc
@@ -79,6 +79,9 @@
         try:
             drive, part, account, container = split_path(unquote(req.path),
                                                          3, 4)
+            if (account and not check_utf8(account)) or \
+                    (container and not check_utf8(container)):
+                raise ValueError('NULL characters not allowed in names')
         except ValueError, err:
             return HTTPBadRequest(body=str(err), content_type='text/plain',
                                   request=req)
@@ -201,8 +204,8 @@
             marker = get_param(req, 'marker', '')
             end_marker = get_param(req, 'end_marker')
             query_format = get_param(req, 'format')
-        except UnicodeDecodeError, err:
-            return HTTPBadRequest(body='parameters not utf8',
+        except (UnicodeDecodeError, ValueError), err:
+            return HTTPBadRequest(body='parameters not utf8 or contain NULLs',
                                   content_type='text/plain', request=req)
         if query_format:
             req.accept = 'application/%s' % query_format.lower()
@@ -228,7 +231,7 @@
             output_list = ['<?xml version="1.0" encoding="UTF-8"?>',
                            '<account name="%s">' % account]
             for (name, object_count, bytes_used, is_subdir) in account_list:
-                name = saxutils.escape(name)
+                name = saxutils.escape(name, XML_EXTRA_ENTITIES)
                 if is_subdir:
                     output_list.append('<subdir name="%s" />' % name)
                 else:

=== modified file 'swift/common/constraints.py'
--- swift/common/constraints.py	2011-01-04 23:34:43 +0000
+++ swift/common/constraints.py	2011-06-17 01:04:33 +0000
@@ -159,13 +159,16 @@
 
 def check_utf8(string):
     """
-    Validate if a string is valid UTF-8.
+    Validate if a string is valid UTF-8 and has no NULL characters.
 
     :param string: string to be validated
-    :returns: True if the string is valid utf-8, False otherwise
+    :returns: True if the string is valid utf-8 and has no NULL characters,
+              False otherwise
     """
     try:
         string.decode('UTF-8')
-        return True
     except UnicodeDecodeError:
         return False
+    if '\x00' in string:
+        return False
+    return True

=== modified file 'swift/common/utils.py'
--- swift/common/utils.py	2011-06-05 23:22:35 +0000
+++ swift/common/utils.py	2011-06-17 01:04:33 +0000
@@ -42,6 +42,7 @@
 from eventlet.green import socket, subprocess, ssl, thread, threading
 import netifaces
 
+from swift.common.constraints import check_utf8
 from swift.common.exceptions import LockTimeout, MessageTimeout
 
 # logging doesn't import patched as cleanly as one would like
@@ -74,6 +75,8 @@
 # Used when reading config values
 TRUE_VALUES = set(('true', '1', 'yes', 'on', 't', 'y'))
 
+# Used with xml.sax.saxutils.escape
+XML_EXTRA_ENTITIES = dict((chr(x), '&#x%x;' % x) for x in xrange(1, 0x20))
 
 def validate_configuration():
     if HASH_PATH_SUFFIX == '':
@@ -110,8 +113,8 @@
     :returns: HTTP request parameter value
     """
     value = req.str_params.get(name, default)
-    if value:
-        value.decode('utf8')    # Ensure UTF8ness
+    if value and not check_utf8(value):
+        raise ValueError('Not valid UTF-8 or contains NULL characters')
     return value
 
 

=== modified file 'swift/container/server.py'
--- swift/container/server.py	2011-06-10 18:36:02 +0000
+++ swift/container/server.py	2011-06-17 01:04:33 +0000
@@ -32,7 +32,7 @@
 
 from swift.common.db import ContainerBroker
 from swift.common.utils import get_logger, get_param, hash_path, \
-    normalize_timestamp, storage_directory, split_path
+    normalize_timestamp, storage_directory, split_path, XML_EXTRA_ENTITIES
 from swift.common.constraints import CONTAINER_LISTING_LIMIT, \
     check_mount, check_float, check_utf8
 from swift.common.bufferedhttp import http_connect
@@ -167,6 +167,10 @@
         try:
             drive, part, account, container, obj = split_path(
                 unquote(req.path), 4, 5, True)
+            if (account and not check_utf8(account)) or \
+                    (container and not check_utf8(container)) or \
+                    (obj and not check_utf8(obj)):
+                raise ValueError('NULL characters not allowed in names')
         except ValueError, err:
             return HTTPBadRequest(body=str(err), content_type='text/plain',
                                 request=req)
@@ -277,7 +281,7 @@
                     return HTTPPreconditionFailed(request=req,
                         body='Maximum limit is %d' % CONTAINER_LISTING_LIMIT)
             query_format = get_param(req, 'format')
-        except UnicodeDecodeError, err:
+        except (UnicodeDecodeError, ValueError), err:
             return HTTPBadRequest(body='parameters not utf8',
                                   content_type='text/plain', request=req)
         if query_format:
@@ -312,21 +316,23 @@
             xml_output = []
             for (name, created_at, size, content_type, etag) in container_list:
                 # escape name and format date here
-                name = saxutils.escape(name)
+                name = saxutils.escape(name, XML_EXTRA_ENTITIES)
                 created_at = datetime.utcfromtimestamp(
                     float(created_at)).isoformat()
                 if content_type is None:
                     xml_output.append('<subdir name="%s"><name>%s</name>'
                                       '</subdir>' % (name, name))
                 else:
-                    content_type = saxutils.escape(content_type)
+                    content_type = saxutils.escape(content_type,
+                                                   XML_EXTRA_ENTITIES)
                     xml_output.append('<object><name>%s</name><hash>%s</hash>'\
                            '<bytes>%d</bytes><content_type>%s</content_type>'\
                            '<last_modified>%s</last_modified></object>' % \
                            (name, etag, size, content_type, created_at))
             container_list = ''.join([
                 '<?xml version="1.0" encoding="UTF-8"?>\n',
-                '<container name=%s>' % saxutils.quoteattr(container),
+                '<container name=%s>' % 
+                    saxutils.quoteattr(container, XML_EXTRA_ENTITIES),
                 ''.join(xml_output), '</container>'])
         else:
             if not container_list:

=== modified file 'test/functionalnosetests/test_account.py'
--- test/functionalnosetests/test_account.py	2010-09-03 16:20:28 +0000
+++ test/functionalnosetests/test_account.py	2011-06-17 01:04:33 +0000
@@ -2,6 +2,7 @@
 
 import unittest
 from nose import SkipTest
+from uuid import uuid4
 
 from swift.common.constraints import MAX_META_COUNT, MAX_META_NAME_LENGTH, \
     MAX_META_OVERALL_SIZE, MAX_META_VALUE_LENGTH
@@ -132,6 +133,32 @@
         resp.read()
         self.assertEquals(resp.status, 400)
 
+    def test_name_control_chars(self):
+        if skip:
+            raise SkipTest
+
+        container = uuid4().hex
+
+        def put(url, token, parsed, conn):
+            conn.request('PUT', '%s/%s%%01test' %
+                (parsed.path, container), '',
+                {'X-Auth-Token': token, 'Content-Length': '0'})
+            return check_response(conn)
+
+        resp = retry(put)
+        resp.read()
+        self.assertTrue(resp.status in (201, 202))
+
+        def get(url, token, parsed, conn):
+            conn.request('GET', '%s?format=xml' % (parsed.path,), '',
+                {'X-Auth-Token': token})
+            return check_response(conn)
+
+        resp = retry(get)
+        body = resp.read()
+        self.assertEquals(resp.status, 200)
+        self.assertTrue('<name>%s&#x1;test</name>' % (container,) in body)
+
 
 if __name__ == '__main__':
     unittest.main()

=== modified file 'test/functionalnosetests/test_container.py'
--- test/functionalnosetests/test_container.py	2011-03-24 23:12:36 +0000
+++ test/functionalnosetests/test_container.py	2011-06-17 01:04:33 +0000
@@ -522,6 +522,50 @@
         resp.read()
         self.assertEquals(resp.status, 201)
 
+    def test_name_control_chars(self):
+        if skip:
+            raise SkipTest
+
+        def put(url, token, parsed, conn):
+            conn.request('PUT', '%s/%s%%00test' % (parsed.path, self.name), '',
+                {'X-Auth-Token': token})
+            return check_response(conn)
+
+        resp = retry(put)
+        resp.read()
+        # NULLs not allowed
+        self.assertEquals(resp.status, 412)
+
+        def put(url, token, parsed, conn):
+            conn.request('PUT', '%s/%s%%01test' % (parsed.path, self.name), '',
+                {'X-Auth-Token': token})
+            return check_response(conn)
+
+        resp = retry(put)
+        resp.read()
+        # 0x01 allowed
+        self.assertTrue(resp.status in (201, 202))
+
+        def put(url, token, parsed, conn):
+            conn.request('PUT', '%s/%s/object%%01test' %
+                (parsed.path, self.name), '',
+                {'X-Auth-Token': token, 'Content-Length': '0'})
+            return check_response(conn)
+
+        resp = retry(put)
+        resp.read()
+        self.assertTrue(resp.status in (201, 202))
+
+        def get(url, token, parsed, conn):
+            conn.request('GET', '%s/%s?format=xml' % (parsed.path, self.name),
+                '', {'X-Auth-Token': token})
+            return check_response(conn)
+
+        resp = retry(get)
+        body = resp.read()
+        self.assertEquals(resp.status, 200)
+        self.assertTrue('<name>object&#x1;test</name>' in body)
+
 
 if __name__ == '__main__':
     unittest.main()

=== modified file 'test/functionalnosetests/test_object.py'
--- test/functionalnosetests/test_object.py	2010-12-28 19:33:36 +0000
+++ test/functionalnosetests/test_object.py	2011-06-17 01:04:33 +0000
@@ -541,6 +541,30 @@
         resp.read()
         self.assertEquals(resp.status, 204)
 
+    def test_name_control_chars(self):
+        if skip:
+            raise SkipTest
+
+        def put(url, token, parsed, conn):
+            conn.request('PUT', '%s/%s/obj%%00test' % (parsed.path,
+                self.container), 'test', {'X-Auth-Token': token})
+            return check_response(conn)
+
+        resp = retry(put)
+        resp.read()
+        # NULLs not allowed
+        self.assertEquals(resp.status, 412)
+
+        def put(url, token, parsed, conn):
+            conn.request('PUT', '%s/%s/obj%%01test' % (parsed.path,
+                self.container), 'test', {'X-Auth-Token': token})
+            return check_response(conn)
+
+        resp = retry(put)
+        resp.read()
+        # 0x01 allowed
+        self.assertEquals(resp.status, 201)
+
 
 if __name__ == '__main__':
     unittest.main()

=== modified file 'test/unit/account/test_server.py'
--- test/unit/account/test_server.py	2011-06-10 18:36:02 +0000
+++ test/unit/account/test_server.py	2011-06-17 01:04:33 +0000
@@ -22,6 +22,7 @@
 import simplejson
 import xml.dom.minidom
 from webob import Request
+from xml.parsers.expat import ExpatError
 
 from swift.account.server import AccountController, ACCOUNT_LISTING_LIMIT
 from swift.common.utils import normalize_timestamp
@@ -450,7 +451,8 @@
                                      'X-Bytes-Used': '0',
                                      'X-Timestamp': normalize_timestamp(0)})
         self.controller.PUT(req)
-        req = Request.blank('/sda1/p/a/c2', environ={'REQUEST_METHOD': 'PUT'},
+        req = Request.blank('/sda1/p/a/c2%04',
+                            environ={'REQUEST_METHOD': 'PUT'},
                             headers={'X-Put-Timestamp': '2',
                                      'X-Delete-Timestamp': '0',
                                      'X-Object-Count': '0',
@@ -462,7 +464,15 @@
         resp = self.controller.GET(req)
         self.assertEquals(resp.content_type, 'application/xml')
         self.assertEquals(resp.status_int, 200)
-        dom = xml.dom.minidom.parseString(resp.body)
+        try:
+            dom = xml.dom.minidom.parseString(resp.body)
+        except ExpatError, err:
+            # Expat doesn't like control characters, which are XML 1.1
+            # compatible. Soooo, we have to replace them. We'll do a specific
+            # replace in this case, but real code that uses Expat will need
+            # something more resilient.
+            dom = xml.dom.minidom.parseString(
+                resp.body.replace('&#x4;', '\\x04'))
         self.assertEquals(dom.firstChild.nodeName, 'account')
         listing = \
             [n for n in dom.firstChild.childNodes if n.nodeName != '#text']
@@ -483,7 +493,7 @@
         self.assertEquals(sorted([n.nodeName for n in container]),
                           ['bytes', 'count', 'name'])
         node = [n for n in container if n.nodeName == 'name'][0]
-        self.assertEquals(node.firstChild.nodeValue, 'c2')
+        self.assertEquals(node.firstChild.nodeValue, 'c2\\x04')
         node = [n for n in container if n.nodeName == 'count'][0]
         self.assertEquals(node.firstChild.nodeValue, '0')
         node = [n for n in container if n.nodeName == 'bytes'][0]
@@ -495,7 +505,8 @@
                                      'X-Bytes-Used': '2',
                                      'X-Timestamp': normalize_timestamp(0)})
         self.controller.PUT(req)
-        req = Request.blank('/sda1/p/a/c2', environ={'REQUEST_METHOD': 'PUT'},
+        req = Request.blank('/sda1/p/a/c2%04',
+                            environ={'REQUEST_METHOD': 'PUT'},
                             headers={'X-Put-Timestamp': '2',
                                      'X-Delete-Timestamp': '0',
                                      'X-Object-Count': '3',
@@ -506,7 +517,15 @@
                             environ={'REQUEST_METHOD': 'GET'})
         resp = self.controller.GET(req)
         self.assertEquals(resp.status_int, 200)
-        dom = xml.dom.minidom.parseString(resp.body)
+        try:
+            dom = xml.dom.minidom.parseString(resp.body)
+        except ExpatError, err:
+            # Expat doesn't like control characters, which are XML 1.1
+            # compatible. Soooo, we have to replace them. We'll do a specific
+            # replace in this case, but real code that uses Expat will need
+            # something more resilient.
+            dom = xml.dom.minidom.parseString(
+                resp.body.replace('&#x4;', '\\x04'))
         self.assertEquals(dom.firstChild.nodeName, 'account')
         listing = \
             [n for n in dom.firstChild.childNodes if n.nodeName != '#text']
@@ -526,7 +545,7 @@
         self.assertEquals(sorted([n.nodeName for n in container]),
                           ['bytes', 'count', 'name'])
         node = [n for n in container if n.nodeName == 'name'][0]
-        self.assertEquals(node.firstChild.nodeValue, 'c2')
+        self.assertEquals(node.firstChild.nodeValue, 'c2\\x04')
         node = [n for n in container if n.nodeName == 'count'][0]
         self.assertEquals(node.firstChild.nodeValue, '3')
         node = [n for n in container if n.nodeName == 'bytes'][0]
@@ -959,6 +978,35 @@
             resp = self.controller.GET(req)
             self.assert_(resp.status_int in (204, 412), resp.status_int)
 
+    def test_params_no_null(self):
+        self.controller.PUT(Request.blank('/sda1/p/a',
+                            headers={'X-Timestamp': normalize_timestamp(1)},
+                            environ={'REQUEST_METHOD': 'PUT'}))
+        for param in ('delimiter', 'format', 'limit', 'marker',
+                      'prefix'):
+            req = Request.blank('/sda1/p/a?%s=\x00' % param,
+                                environ={'REQUEST_METHOD': 'GET'})
+            resp = self.controller.GET(req)
+            self.assertEquals(resp.status_int, 400)
+
+    def test_PUT_account_no_null(self):
+        req = Request.blank('/sda1/p/test\x00test',
+            environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1'})
+        resp = self.controller.PUT(req)
+        self.assertEquals(resp.status_int, 400)
+
+    def test_PUT_container_no_null(self):
+        req = Request.blank('/sda1/p/a',
+            environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1'})
+        resp = self.controller.PUT(req)
+        self.assertEquals(resp.status_int, 201)
+        req = Request.blank('/sda1/p/a/test\x00test',
+            environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_PUT_TIMESTAMP': '1',
+                     'HTTP_X_DELETE_TIMESTAMP': '0',
+                     'HTTP_X_OBJECT_COUNT': '0', 'HTTP_X_BYTES_USED': '0'})
+        resp = self.controller.PUT(req)
+        self.assertEquals(resp.status_int, 400)
+
 
 if __name__ == '__main__':
     unittest.main()

=== modified file 'test/unit/container/test_server.py'
--- test/unit/container/test_server.py	2011-06-10 18:36:02 +0000
+++ test/unit/container/test_server.py	2011-06-17 01:04:33 +0000
@@ -246,6 +246,24 @@
         resp = self.controller.PUT(req)
         self.assertEquals(resp.status_int, 201)
 
+    def test_PUT_container_no_null(self):
+        req = Request.blank('/sda1/p/a/test\x00test',
+            environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1'})
+        resp = self.controller.PUT(req)
+        self.assertEquals(resp.status_int, 400)
+
+    def test_PUT_object_no_null(self):
+        req = Request.blank('/sda1/p/a/test',
+            environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1'})
+        resp = self.controller.PUT(req)
+        self.assertEquals(resp.status_int, 201)
+        req = Request.blank('/sda1/p/a/test/test\x00test',
+            environ={'REQUEST_METHOD': 'PUT', 'HTTP_X_TIMESTAMP': '1',
+                     'HTTP_X_SIZE': '0', 'HTTP_X_CONTENT_TYPE': 'text/plain',
+                     'HTTP_X_ETAG': 'd41d8cd98f00b204e9800998ecf8427e'})
+        resp = self.controller.PUT(req)
+        self.assertEquals(resp.status_int, 400)
+
     def test_PUT_account_update(self):
         bindsock = listen(('127.0.0.1', 0))
         def accept(return_code, expected_timestamp):
@@ -582,25 +600,25 @@
         resp = self.controller.PUT(req)
         # fill the container
         for i in range(3):
-            req = Request.blank('/sda1/p/a/xmlc/%s'%i, environ=
-                    {'REQUEST_METHOD': 'PUT',
-                    'HTTP_X_TIMESTAMP': '1',
-                    'HTTP_X_CONTENT_TYPE': 'text/plain',
-                    'HTTP_X_ETAG': 'x',
-                    'HTTP_X_SIZE': 0})
+            req = Request.blank('/sda1/p/a/xmlc/%s%%%02x' % (i, i + 1),
+                environ={'REQUEST_METHOD': 'PUT',
+                         'HTTP_X_TIMESTAMP': '1',
+                         'HTTP_X_CONTENT_TYPE': 'text/plain',
+                         'HTTP_X_ETAG': 'x',
+                         'HTTP_X_SIZE': 0})
             resp = self.controller.PUT(req)
             self.assertEquals(resp.status_int, 201)
         xml_body = '<?xml version="1.0" encoding="UTF-8"?>\n' \
             '<container name="xmlc">' \
-                '<object><name>0</name><hash>x</hash><bytes>0</bytes>' \
-                    '<content_type>text/plain</content_type>' \
-                    '<last_modified>1970-01-01T00:00:01' \
-                    '</last_modified></object>' \
-                '<object><name>1</name><hash>x</hash><bytes>0</bytes>' \
-                    '<content_type>text/plain</content_type>' \
-                    '<last_modified>1970-01-01T00:00:01' \
-                    '</last_modified></object>' \
-                '<object><name>2</name><hash>x</hash><bytes>0</bytes>' \
+                '<object><name>0&#x1;</name><hash>x</hash><bytes>0</bytes>' \
+                    '<content_type>text/plain</content_type>' \
+                    '<last_modified>1970-01-01T00:00:01' \
+                    '</last_modified></object>' \
+                '<object><name>1&#x2;</name><hash>x</hash><bytes>0</bytes>' \
+                    '<content_type>text/plain</content_type>' \
+                    '<last_modified>1970-01-01T00:00:01' \
+                    '</last_modified></object>' \
+                '<object><name>2&#x3;</name><hash>x</hash><bytes>0</bytes>' \
                     '<content_type>text/plain</content_type>' \
                     '<last_modified>1970-01-01T00:00:01' \
                     '</last_modified></object>' \
@@ -822,6 +840,17 @@
             resp = self.controller.GET(req)
             self.assert_(resp.status_int in (204, 412), resp.status_int)
 
+    def test_params_no_null(self):
+        self.controller.PUT(Request.blank('/sda1/p/a/c',
+                            headers={'X-Timestamp': normalize_timestamp(1)},
+                            environ={'REQUEST_METHOD': 'PUT'}))
+        for param in ('delimiter', 'format', 'limit', 'marker', 'path',
+                      'prefix'):
+            req = Request.blank('/sda1/p/a/c?%s=\x00' % param,
+                                environ={'REQUEST_METHOD': 'GET'})
+            resp = self.controller.GET(req)
+            self.assertEquals(resp.status_int, 400)
+
 
 if __name__ == '__main__':
     unittest.main()

=== modified file 'test/unit/proxy/test_server.py'
--- test/unit/proxy/test_server.py	2011-06-14 22:20:23 +0000
+++ test/unit/proxy/test_server.py	2011-06-17 01:04:33 +0000
@@ -2111,6 +2111,20 @@
         exp = 'HTTP/1.1 412'
         self.assertEquals(headers[:len(exp)], exp)
 
+    def test_chunked_put_bad_utf8_null(self):
+        # Check invalid utf-8
+        (prolis, acc1lis, acc2lis, con2lis, con2lis, obj1lis, obj2lis) = \
+                 _test_sockets
+        sock = connect_tcp(('localhost', prolis.getsockname()[1]))
+        fd = sock.makefile()
+        fd.write('GET /v1/a%00 HTTP/1.1\r\nHost: localhost\r\n'
+            'Connection: close\r\nX-Auth-Token: t\r\n'
+            'Content-Length: 0\r\n\r\n')
+        fd.flush()
+        headers = readuntil2crlfs(fd)
+        exp = 'HTTP/1.1 412'
+        self.assertEquals(headers[:len(exp)], exp)
+
     def test_chunked_put_bad_path_no_controller(self):
         # Check bad path, no controller
         (prolis, acc1lis, acc2lis, con2lis, con2lis, obj1lis, obj2lis) = \