Repository: defnull/multipart

Issue:
Newlines, quotes and backslashes in field names or filenames not decoded correctly.
The [HTML5 specification](https://html.spec.whatwg.org/multipage/form-control-infrastructure.html#multipart-form-data) defines that **"field names and filenames for file fields [...] must be escaped by replacing any 0x0A (LF) bytes with the byte sequence `%0A`, 0x0D (CR) with `%0D` and 0x22 (") with `%22`. The user agent must not perform any other escapes."** and tests show that modern browsers actually do that. The traditional header quoting (which involves backslash-escaping quotes and backslashes) is NOT applied. The string is (always?) quoted, but since it cannot contain quotes that need backslash-escaping, backslashes are left alone and not doubled.

This means that Multipart currently decodes field names or filenames incorrectly if they contain `"`, `\r`, `\n` or two backslashes in a row. The first three characters are percent-encoded by browsers but not percent-decoded by multipart, and two backslashes in a row are not escaped (doubled) by browsers but multipart un-escapes them.

For the record: other control characters (e.g. null bytes) are encoded as `\xef\xbf\xbd`, which is the UTF8 sequence for the unicode "Replacement character". Good to know.

Interestingly, this is not full percent-encoding as mentioned (but not required or suggested) by RFC-7578. Only three specific characters are percent-encoded, other non-ASCII characters are fair game and encoded as UTF8. There may be other clients that do full percent-encoding, but since we cannot know that for sure, and such a behavior would be a-typical for browsers, it's probably best to not touch those. Implementing the correct behavior in multipart would not destroy any information in those full-percent-encoded strings, though. Applications that know they have to deal with outdated clients would still be able to decode those strings.

The impact of this bug is probably very low. Field names or filenames containing newlines or backslashes should be extremely rare and quotes are also very uncommon. But we aim to fully support whatever browsers may send, and that's part of the deal.

Discussion:

Solution patch:
diff --git a/CHANGELOG.rst b/CHANGELOG.rst
index d7dd95b..8276e88 100644
--- a/CHANGELOG.rst
+++ b/CHANGELOG.rst
@@ -15,20 +15,25 @@ Release 1.2
 This release improves error handling, fixes several parser edge-cases and adds
 new functionality. API changes are backwards compatible.
 
-* feat: New `is_form_request(environ)` helper.
-* feat: Split up `MultipartError`` into more specific exceptions and added HTTP
-  status code info to each one. All exceptions are subclasses of `MultipartError`.
-* feat: Added `parse_form_data(ignore_errors)` parameter to throw exceptions in
+* feat: Split up `MultipartError`` into more specific exceptions and add HTTP
+  status code hints. All exceptions are subclasses of `MultipartError`.
+* feat: New `parse_form_data(ignore_errors)` parameter to throw exceptions in
   non-strict mode, or suppress exceptions in strict mode. Default behavior does
-  not change.
-* fix: `parse_form_data` no longer checks the request method and `is_form_request`
-  also ignores it. All methods can carry parse-able form data, including unknown
-  methods. The only reliable way is to check the `Content-Type` header, which
-  both functions do.
+  not change (throw in strict-mode, ignore in non-strict mode).
+* feat: New `is_form_request(environ)` helper.
+* feat: New specialized `content_disposition_[un]quote` functions.
+* feat: `parse_options_header()` can now use different unquote functions. The
+  default does not change.
+* fix: `parse_form_data()` no longer checks the request method and the new
+  `is_form_request` function also ignores it. All methods can carry parse-able
+  form data, including unknown methods. The only reliable way is to check the
+  `Content-Type` header, which both functions do.
 * fix: First boundary not detected if separated by chunk border.
 * fix: Allow CRLF in front of first boundary, even in strict mode.
 * fix: Fail fast if first boundary is broken or part of the preamble.
 * fix: Fail if stream ends without finding any boundary at all.
+* fix: Use modern WHATWG quoting rules for field names and filenames (#60).
+  Legacy quoting is still supported as a fallback.
 
 Release 1.1
 ===========
diff --git a/multipart.py b/multipart.py
index 8276cf7..e8c0858 100644
--- a/multipart.py
+++ b/multipart.py
@@ -184,6 +184,11 @@ _option = r'; *(%s) *= *(%s)' % (_token, _value)
 _re_option = re.compile(_option)
 
 def header_quote(val):
+    """ Quote header option values if necessary.
+
+        Note: This is NOT the way modern browsers quote field names or filenames
+        in Content-Disposition headers. See :func:`content_disposition_quote`
+    """
     if _re_istoken.match(val):
         return val
 
@@ -191,6 +196,11 @@ def header_quote(val):
 
 
 def header_unquote(val, filename=False):
+    """ Unquote header option values.
+
+        Note: This is NOT the way modern browsers quote field names or filenames
+        in Content-Disposition headers. See :func:`content_disposition_unquote`
+    """
     if val[0] == val[-1] == '"':
         val = val[1:-1]
 
@@ -203,7 +213,45 @@ def header_unquote(val, filename=False):
     return val
 
 
-def parse_options_header(header, options=None):
+def content_disposition_quote(val):
+    """ Quote field names or filenames for Content-Disposition headers the
+        same way modern browsers do it (see WHATWG HTML5 specification).
+    """
+    val = val.replace("\r", "%0D").replace("\n", "%0A").replace('"', "%22")
+    return '"' + val + '"'
+
+
+def content_disposition_unquote(val, filename=False):
+    """ Unquote field names or filenames from Content-Disposition headers.
+
+        Legacy quoting mechanisms are detected to some degree and also supported,
+        but there are rare ambiguous edge cases where we have to guess. If in
+        doubt, this function assumes a modern browser and follows the WHATWG
+        HTML5 specification (limited percent-encoding, no backslash-encoding).
+    """
+
+    if '"' == val[0] == val[-1]:
+        val = val[1:-1]
+        if '\\"' in val:  # Legacy backslash-escaped quoted strings
+            val = val.replace("\\\\", "\\").replace('\\"', '"')
+        elif "%" in val:  # Modern (HTML5) limited percent-encoding
+            val = val.replace("%0D", "\r").replace("%0A", "\n").replace("%22", '"')
+        # ie6/windows bug: full path instead of just filename
+        if filename and (val[1:3] == ":\\" or val[:2] == "\\\\"):
+            val = val.rpartition("\\")[-1]
+    elif "%" in val:  # Modern (HTML5) limited percent-encoding
+        val = val.replace("%0D", "\r").replace("%0A", "\n").replace("%22", '"')
+    return val
+
+
+def parse_options_header(header, options=None, unquote=header_unquote):
+    """ Parse Content-Type (or similar) headers into a primary value 
+        and an options-dict.
+
+        Note: For Content-Disposition headers you need a different unquote
+        function. See `content_disposition_unquote`.
+
+    """
     i = header.find(";")
     if i < 0:
         return header.lower().strip(), {}
@@ -211,7 +259,7 @@ def parse_options_header(header, options=None):
     options = options or {}
     for key, val in _re_option.findall(header, i):
         key = key.lower()
-        options[key] = header_unquote(val, key == "filename")
+        options[key] = unquote(val, key == "filename")
 
     return header[:i].lower().strip(), options
 
@@ -543,7 +591,7 @@ class MultipartSegment:
 
         for h,v in self.headerlist:
             if h == "Content-Disposition":
-                dtype, args = parse_options_header(v)
+                dtype, args = parse_options_header(v, unquote=content_disposition_unquote)
                 if dtype != "form-data":
                     raise ParserError("Invalid Content-Disposition segment header: Wrong type")
                 if "name" not in args and self._parser.strict:


Test patch:
diff --git a/test/test_header_utils.py b/test/test_header_utils.py
index c157036..dac1c23 100644
--- a/test/test_header_utils.py
+++ b/test/test_header_utils.py
@@ -1,7 +1,9 @@
 # -*- coding: utf-8 -*-
+import functools
 import unittest
 import multipart
 
+
 class TestHeaderParser(unittest.TestCase):
 
     def test_token_unquote(self):
@@ -11,17 +13,34 @@ class TestHeaderParser(unittest.TestCase):
         self.assertEqual('ie.exe', unquote('"\\\\network\\ie.exe"', True))
         self.assertEqual('ie.exe', unquote('"c:\\wondows\\ie.exe"', True))
 
+        unquote = multipart.content_disposition_unquote
+        self.assertEqual('foo', unquote('"foo"'))
+        self.assertEqual('foo"bar', unquote('foo%22bar'))
+        self.assertEqual('foo"bar', unquote('"foo%22bar"'))
+        self.assertEqual('foo"bar', unquote('"foo\\"bar"'))
+        self.assertEqual('ie.exe', unquote('"\\\\network\\ie.exe"', True))
+        self.assertEqual('ie.exe', unquote('"c:\\wondows\\ie.exe"', True))
+
     def test_token_quote(self):
         quote = multipart.header_quote
         self.assertEqual(quote('foo'), 'foo')
         self.assertEqual(quote('foo"bar'), '"foo\\"bar"')
 
+        quote = multipart.content_disposition_quote
+        self.assertEqual(quote('foo'), '"foo"')
+        self.assertEqual(quote('foo"bar'), '"foo%22bar"')
+
     def test_options_parser(self):
         parse = multipart.parse_options_header
         head = 'form-data; name="Test"; '
         self.assertEqual(parse(head+'filename="Test.txt"')[0], 'form-data')
         self.assertEqual(parse(head+'filename="Test.txt"')[1]['name'], 'Test')
         self.assertEqual(parse(head+'filename="Test.txt"')[1]['filename'], 'Test.txt')
-        self.assertEqual(parse(head+'FileName="Te\\"st.txt"')[1]['filename'], 'Te"st.txt')
+        self.assertEqual(parse(head+'FileName="Te\\"s\\\\t.txt"')[1]['filename'], 'Te"s\\t.txt')
         self.assertEqual(parse(head+'filename="C:\\test\\bla.txt"')[1]['filename'], 'bla.txt')
         self.assertEqual(parse(head+'filename="\\\\test\\bla.txt"')[1]['filename'], 'bla.txt')
+        self.assertEqual(parse(head+'filename="täst.txt"')[1]['filename'], 'täst.txt')
+
+        parse = functools.partial(multipart.parse_options_header, unquote=multipart.content_disposition_unquote)
+        self.assertEqual(parse(head+'FileName="Te%22s\\\\t.txt"')[1]['filename'], 'Te"s\\\\t.txt')
+
diff --git a/test/test_push_parser.py b/test/test_push_parser.py
index 4e89ecc..4f9c888 100644
--- a/test/test_push_parser.py
+++ b/test/test_push_parser.py
@@ -63,10 +63,12 @@ class PushTestBase(unittest.TestCase):
             yield current, b''.join(data)
 
     def get_segment(self, index_or_name):
+        allnames = []
         for i, (segment, body) in enumerate(self.compact_events()):
+            allnames.append(segment.name)
             if index_or_name == i or index_or_name == segment.name:
                 return segment, body
-        self.fail(f"Segment not found: {index_or_name}")
+        self.fail(f"Segment {index_or_name!r} not found in {allnames!r}")
 
 
 class TestPushParser(PushTestBase):
@@ -846,3 +848,25 @@ class TestWerkzeugExamples(PushTestBase):
                 self.assertEqual(segment.filename, None)
                 self.assertEqual(segment.content_type, None)
                 self.assertEqual(body.decode(segment.charset or 'utf8'), forms[field])
+
+
+
+class TestRealWorldExamples(PushTestBase):
+    def test_special_characters(self):
+        """ Test the ultimate segment name/filename from hell. """
+        teststring = 'test \\ \\\\ ; ö " = ;'
+        firefox_131 = ['---------------------------3697486332756351920303607403',
+b'-----------------------------3697486332756351920303607403\r\nContent-Disposition: form-data; name="test \\ \\\\ ; \xc3\xb6 %22 = ;"; filename="test \\ \\\\ ; \xc3\xb6 %22 = ;"\r\nContent-Type: application/octet-stream\r\n\r\ntest \\ \\\\ ; \xc3\xb6 " = ;\r\n-----------------------------3697486332756351920303607403--\r\n']
+        chrome_129 = ["----WebKitFormBoundary9duA54BXJUGUymtb", 
+b'------WebKitFormBoundary9duA54BXJUGUymtb\r\nContent-Disposition: form-data; name="test \\ \\\\ ; \xc3\xb6 %22 = ;"; filename="test \\ \\\\ ; \xc3\xb6 %22 = ;"\r\nContent-Type: application/octet-stream\r\n\r\ntest \\ \\\\ ; \xc3\xb6 " = ;\r\n------WebKitFormBoundary9duA54BXJUGUymtb--\r\n']
+
+        for boundary, body in [firefox_131, chrome_129]:
+            print(repr(boundary))
+            print(repr(body))
+            self.reset(boundary=boundary, strict=True, header_charset='utf8')
+            self.parse(body)
+            segment, body = self.get_segment(teststring)
+            self.assertEqual(segment.name, teststring)
+            self.assertEqual(segment.filename, teststring)
+            self.assertEqual(body, teststring.encode("utf8"))
+
diff --git a/test/utils.py b/test/utils.py
index 684cb40..c7da39c 100644
--- a/test/utils.py
+++ b/test/utils.py
@@ -48,7 +48,7 @@ class BaseParserTest(unittest.TestCase):
         line = to_bytes(header) + b': ' + to_bytes(value)
         for opt, val in opts.items():
             if val is not None:
-                line += b"; " + to_bytes(opt) + b'=' + to_bytes(multipart.header_quote(val))
+                line += b"; " + to_bytes(opt) + b'=' + to_bytes(multipart.content_disposition_quote(val))
         self.write(line + b'\r\n')
 
     def write_field(self, name, data, filename=None, content_type=None):