Merge lp:~cjwatson/wadllib/fix-mime-encoding into lp:wadllib

[Colin Watson]

Reimplement MIME multipart/form-data encoding without binary corruption bugs.

I tried to persuade the standard library to do what I wanted, but getting it to work right across all Python versions was too hard.

[Pierre Equoy (pieq)]

Hi,

is there an easy way to test this change to upload a binary attachment to the Staging instance of Launchpad?

[lp:~cjwatson/wadllib/fix-mime-encoding]

37. By Colin Watson on 2018-07-13

Test a few more variations of binary data.

[Colin Watson (cjwatson)]

You just need to check out this branch and install it in a virtualenv using pip.

Anyway, I've tested this myself against a development instance of Launchpad, explicitly testing the example @pieq gave in the linked bug as well as things like files ending with an \r byte, and everything now seems to round-trip successfully.

[Simon Davy (bloodearnest)]

LGTM, I agree that it's small enough in inline, so to speak.

review: Approve

=== modified file 'README.txt'
--- README.txt	2012-01-18 20:55:11 +0000
+++ README.txt	2018-07-13 10:07:36 +0000
@@ -1,6 +1,6 @@
 Navigate HTTP resources using WADL files as guides.
 
-wadllib should work with Python >= 2.4.
+wadllib should work with Python >= 2.6.
 
 ..
     Copyright (C) 2008-2009 Canonical Ltd.

=== modified file 'setup.py'
--- setup.py	2014-06-24 17:54:15 +0000
+++ setup.py	2018-07-13 10:07:36 +0000
@@ -44,14 +44,6 @@
     'setuptools',
     'lazr.uri',
     ]
-if sys.version < '2.5':
-    # This module is in the standard library as of Python 2.5.  In Python 3,
-    # it doesn't work when fetched separately.
-    install_requires.append('elementtree')
-if sys.version < '2.6':
-    # This module is in the standard library as of Python 2.6.  In Python 3,
-    # it doesn't work when fetched separately.
-    install_requires.extend('simplejson')
 
 setup(
     name='wadllib',

=== modified file 'src/wadllib/NEWS.txt'
--- src/wadllib/NEWS.txt	2013-02-25 18:53:09 +0000
+++ src/wadllib/NEWS.txt	2018-07-13 10:07:36 +0000
@@ -2,6 +2,16 @@
 NEWS for wadllib
 ================
 
+1.3.3
+=====
+
+- Drop support for Python < 2.6.
+- Add tox testing support.
+- Implement a subset of MIME multipart/form-data encoding locally rather
+  than using the standard library's email module, which doesn't have good
+  handling of binary parts and corrupts bytes in them that look like line
+  endings in various ways depending on the Python version.  [bug=1729754]
+
 1.3.2 (2013-02-25)
 ==================
 

=== modified file 'src/wadllib/README.txt'
--- src/wadllib/README.txt	2015-02-11 04:19:08 +0000
+++ src/wadllib/README.txt	2018-07-13 10:07:36 +0000
@@ -295,10 +295,7 @@
 to another object. The 'link' property of a parameter lets you examine
 links before following them.
 
-    >>> try:
-    ...     import simplejson as json
-    ... except ImportError:
-    ...     import json
+    >>> import json
     >>> links_wadl = application_for('links-wadl.xml')
     >>> service_root = links_wadl.get_resource_by_path('')
     >>> representation = json.dumps(
@@ -568,13 +565,63 @@
    ...     "http://www.example.com/", binary_stream)
    >>> service_root = binary_wadl.get_resource_by_path('')
 
-   >>> method = service_root.get_method('post', 'multipart/form-data')
-   >>> media_type, doc = method.build_representation(
-   ...     text_field="text", binary_field="\x01\x02")
-   >>> print(media_type)
-   multipart/form-data; boundary=...
-   >>> b'\x01\x02' in doc
-   True
+Define a helper that processes the representation the same way
+zope.publisher would.
+
+   >>> import cgi
+   >>> import io
+   >>> def assert_message_parts(media_type, doc, expected):
+   ...     if sys.version_info[0] == 3 and sys.version_info[1] < 3:
+   ...         # We can't do much due to https://bugs.python.org/issue18013.
+   ...         for value in expected:
+   ...             if not isinstance(value, bytes):
+   ...                 value = value.encode('UTF-8')
+   ...             assert value in doc
+   ...         return
+   ...     environ = {
+   ...         'REQUEST_METHOD': 'POST',
+   ...         'CONTENT_TYPE': media_type,
+   ...         'CONTENT_LENGTH': str(len(doc)),
+   ...         }
+   ...     kwargs = (
+   ...         {'encoding': 'UTF-8'} if sys.version_info[0] >= 3 else {})
+   ...     fs = cgi.FieldStorage(
+   ...         fp=io.BytesIO(doc), environ=environ, keep_blank_values=1,
+   ...         **kwargs)
+   ...     values = []
+   ...     def append_values(fields):
+   ...         for field in fields:
+   ...             if field.list:
+   ...                 append_values(field.list)
+   ...             else:
+   ...                 values.append(field.value)
+   ...     append_values(fs.list)
+   ...     assert values == expected, (
+   ...         'Expected %s, got %s' % (expected, values))
+
+   >>> method = service_root.get_method('post', 'multipart/form-data')
+   >>> media_type, doc = method.build_representation(
+   ...     text_field="text", binary_field=b"\x01\x02\r\x81\r")
+   >>> print(media_type)
+   multipart/form-data; boundary=...
+   >>> assert_message_parts(media_type, doc, ['text', b'\x01\x02\r\x81\r'])
+
+   >>> method = service_root.get_method('post', 'multipart/form-data')
+   >>> media_type, doc = method.build_representation(
+   ...     text_field="text\n", binary_field=b"\x01\x02\r\x81\n\r")
+   >>> print(media_type)
+   multipart/form-data; boundary=...
+   >>> assert_message_parts(
+   ...     media_type, doc, ['text\r\n', b'\x01\x02\r\x81\n\r'])
+
+   >>> method = service_root.get_method('post', 'multipart/form-data')
+   >>> media_type, doc = method.build_representation(
+   ...     text_field="text\r\nmore\r\n",
+   ...     binary_field=b"\x01\x02\r\n\x81\r\x82\n")
+   >>> print(media_type)
+   multipart/form-data; boundary=...
+   >>> assert_message_parts(
+   ...     media_type, doc, ['text\r\nmore\r\n', b'\x01\x02\r\n\x81\r\x82\n'])
 
    >>> method = service_root.get_method('post', 'text/unknown')
    >>> method.build_representation(field="value")

=== modified file 'src/wadllib/application.py'
--- src/wadllib/application.py	2015-02-11 03:56:31 +0000
+++ src/wadllib/application.py	2018-07-13 10:07:36 +0000
@@ -1,4 +1,4 @@
-# Copyright 2008-2013 Canonical Ltd.  All rights reserved.
+# Copyright 2008-2018 Canonical Ltd.  All rights reserved.
 
 # This file is part of wadllib.
 #
@@ -41,38 +41,20 @@
     'WADLError',
     ]
 
-try:
-    # Make sure to try the Python 2 form first; we don't want to use
-    # io.StringIO in Python 2, because it is strict about only accepting
-    # unicode input.
-    from cStringIO import StringIO as BytesIO
-except ImportError:
-    from io import BytesIO
 import datetime
-try:
-    from email.mime.multipart import MIMEMultipart
-    from email.mime.nonmultipart import MIMENonMultipart
-except ImportError:
-    # We must be on 2.4.
-    from email.MIMEMultipart import MIMEMultipart
-    from email.MIMENonMultipart import MIMENonMultipart
-
+from email.utils import quote
+import io
+import json
+import random
+import re
+import sys
 import time
 try:
     from urllib.parse import urlencode
 except ImportError:
     from urllib import urlencode
-try:
-    import simplejson as json
-except ImportError:
-    import json
-try:
-    import xml.etree.cElementTree as ET
-except ImportError:
-    try:
-        import cElementTree as ET
-    except ImportError:
-        import elementtree.ElementTree as ET
+import xml.etree.cElementTree as ET
+
 from lazr.uri import URI, merge
 
 from wadllib import (
@@ -823,6 +805,105 @@
         """The media type of the representation described here."""
         return self.resolve_definition().tag.attrib['mediaType']
 
+    def _make_boundary(self, all_parts):
+        """Make a random boundary that does not appear in `all_parts`."""
+        _width = len(repr(sys.maxsize - 1))
+        _fmt = '%%0%dd' % _width
+        token = random.randrange(sys.maxsize)
+        boundary = ('=' * 15) + (_fmt % token) + '=='
+        if all_parts is None:
+            return boundary
+        b = boundary
+        counter = 0
+        while True:
+            pattern = ('^--' + re.escape(b) + '(--)?$').encode('ascii')
+            if not re.search(pattern, all_parts, flags=re.MULTILINE):
+                break
+            b = boundary + '.' + str(counter)
+            counter += 1
+        return b
+
+    def _write_headers(self, buf, headers):
+        """Write MIME headers to a file object."""
+        for key, value in headers:
+            buf.write(key.encode('UTF-8'))
+            buf.write(b': ')
+            buf.write(value.encode('UTF-8'))
+            buf.write(b'\r\n')
+        buf.write(b'\r\n')
+
+    def _write_boundary(self, buf, boundary, closing=False):
+        """Write a multipart boundary to a file object."""
+        buf.write(b'--')
+        buf.write(boundary.encode('UTF-8'))
+        if closing:
+            buf.write(b'--')
+        buf.write(b'\r\n')
+
+    def _generate_multipart_form(self, parts):
+        """Generate a multipart/form-data message.
+
+        This is very loosely based on the email module in the Python standard
+        library.  However, that module doesn't really support directly embedding
+        binary data in a form: various versions of Python have mangled line
+        separators in different ways, and none of them get it quite right.
+        Since we only need a tiny subset of MIME here, it's easier to implement
+        it ourselves.
+
+        :return: a tuple of two elements: the Content-Type of the message, and
+            the entire encoded message as a byte string.
+        """
+        # Generate the subparts first so that we can calculate a safe boundary.
+        encoded_parts = []
+        for is_binary, name, value in parts:
+            buf = io.BytesIO()
+            if is_binary:
+                ctype = 'application/octet-stream'
+                # RFC 7578 says that the filename parameter isn't mandatory
+                # in our case, but without it cgi.FieldStorage tries to
+                # decode as text on Python 3.
+                cdisp = 'form-data; name="%s"; filename="%s"' % (
+                    quote(name), quote(name))
+            else:
+                ctype = 'text/plain; charset="utf-8"'
+                cdisp = 'form-data; name="%s"' % quote(name)
+            self._write_headers(buf, [
+                ('MIME-Version', '1.0'),
+                ('Content-Type', ctype),
+                ('Content-Disposition', cdisp),
+                ])
+            if is_binary:
+                if not isinstance(value, bytes):
+                    raise TypeError('bytes payload expected: %s' % type(value))
+                buf.write(value)
+            else:
+                if not isinstance(value, str):
+                    raise TypeError('str payload expected: %s' % type(value))
+                lines = re.split(r'\r\n|\r|\n', value)
+                for line in lines[:-1]:
+                    buf.write(line.encode('UTF-8'))
+                    buf.write(b'\r\n')
+                buf.write(lines[-1].encode('UTF-8'))
+            encoded_parts.append(buf.getvalue())
+
+        # Create a suitable boundary.
+        boundary = self._make_boundary(b'\r\n'.join(encoded_parts))
+
+        # Now we can write the multipart headers, followed by all the parts.
+        buf = io.BytesIO()
+        ctype = 'multipart/form-data; boundary="%s"' % quote(boundary)
+        self._write_headers(buf, [
+            ('MIME-Version', '1.0'),
+            ('Content-Type', ctype),
+            ])
+        for encoded_part in encoded_parts:
+            self._write_boundary(buf, boundary)
+            buf.write(encoded_part)
+            buf.write(b'\r\n')
+        self._write_boundary(buf, boundary, closing=True)
+
+        return ctype, buf.getvalue()
+
     def bind(self, param_values, **kw_param_values):
         """Bind the definition to parameter values, creating a document.
 
@@ -836,29 +917,13 @@
         if media_type == 'application/x-www-form-urlencoded':
             doc = urlencode(sorted(validated_values.items()))
         elif media_type == 'multipart/form-data':
-            outer = MIMEMultipart()
-            outer.set_type('multipart/form-data')
+            parts = []
             missing = object()
             for param in params:
                 value = validated_values.get(param.name, missing)
                 if value is not missing:
-                    if param.type == 'binary':
-                        maintype, subtype = 'application', 'octet-stream'
-                        params = {}
-                    else:
-                        maintype, subtype = 'text', 'plain'
-                        params = {'charset' : 'utf-8'}
-                    inner = MIMENonMultipart(maintype, subtype, **params)
-                    inner.set_payload(value)
-                    inner['Content-Disposition'] = (
-                        'form-data; name="%s"' % param.name)
-                    outer.attach(inner)
-            if hasattr(outer, "as_bytes"):
-                doc = outer.as_bytes()
-            else:
-                doc = outer.as_string(unixfrom=False)
-            media_type = (outer.get_content_type() +
-                          '; boundary="%s"' % outer.get_boundary())
+                    parts.append((param.type == 'binary', param.name, value))
+            media_type, doc = self._generate_multipart_form(parts)
         elif media_type == 'application/json':
             doc = json.dumps(validated_values)
         else:
@@ -1104,7 +1169,9 @@
 
     def _from_string(self, markup):
         """Turns markup into a document."""
-        return self._from_stream(BytesIO(markup))
+        if not isinstance(markup, bytes):
+            markup = markup.encode("UTF-8")
+        return self._from_stream(io.BytesIO(markup))
 
     def get_resource_type(self, resource_type_url):
         """Retrieve a resource type by the URL of its description."""

=== modified file 'src/wadllib/tests/test_docs.py'
--- src/wadllib/tests/test_docs.py	2009-03-20 20:30:07 +0000
+++ src/wadllib/tests/test_docs.py	2018-07-13 10:07:36 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  All rights reserved.
+# Copyright 2009-2018 Canonical Ltd.  All rights reserved.
 #
 # This file is part of wadllib
 #
@@ -16,16 +16,20 @@
 
 """Test harness."""
 
+from __future__ import absolute_import, print_function
+
 __metaclass__ = type
 __all__ = [
-    'additional_tests',
+    'load_tests',
     ]
 
+import __future__
 import atexit
 import doctest
 import os
+
 import pkg_resources
-import unittest
+
 
 DOCTEST_FLAGS = (
     doctest.ELLIPSIS |
@@ -33,7 +37,7 @@
     doctest.REPORT_NDIFF)
 
 
-def additional_tests():
+def load_tests(loader, tests, pattern):
     doctest_files = [
         os.path.abspath(
             pkg_resources.resource_filename('wadllib', 'README.txt'))]
@@ -44,7 +48,11 @@
                     os.path.abspath(
                         pkg_resources.resource_filename(
                             'wadllib', 'docs/%s' % name)))
-    kwargs = dict(module_relative=False, optionflags=DOCTEST_FLAGS)
+    globs = {
+        future_item: getattr(__future__, future_item)
+        for future_item in ('absolute_import', 'print_function')}
+    kwargs = dict(
+        module_relative=False, globs=globs, optionflags=DOCTEST_FLAGS)
     atexit.register(pkg_resources.cleanup_resources)
-    return unittest.TestSuite((
-        doctest.DocFileSuite(*doctest_files, **kwargs)))
+    tests.addTest(doctest.DocFileSuite(*doctest_files, **kwargs))
+    return tests

=== added file 'tox.ini'
--- tox.ini	1970-01-01 00:00:00 +0000
+++ tox.ini	2018-07-13 10:07:36 +0000
@@ -0,0 +1,10 @@
+[tox]
+envlist = 
+    py27,py32,py33,py34,py35,py36,py37
+
+[testenv]
+commands =
+    zope-testrunner --test-path src --tests-pattern ^tests {posargs}
+deps =
+    .[test]
+    zope-testrunner

=== modified file 'versions.cfg'
--- versions.cfg	2011-11-25 19:27:47 +0000
+++ versions.cfg	2018-07-13 10:07:36 +0000
@@ -26,10 +26,6 @@
 
 # Required by:
 # wadllib==1.2.0
-elementtree = 1.2.6-20050316
-
-# Required by:
-# wadllib==1.2.0
 lazr.uri = 1.0.2
 
 # Required by:
@@ -46,10 +42,6 @@
 setuptools = 0.6c11
 
 # Required by:
-# wadllib==1.2.0
-simplejson = 2.2.1
-
-# Required by:
 # z3c.recipe.sphinxdoc==0.0.8
 zc.buildout = 1.5.2