Repository: pallets/werkzeug (python)

Issue:
parse_options_header quote handling
I wrote a new version of cgi.parse_header / werkzeug.parse_options_header with better handling for various special cases that unfortunately occur. The new implementation is also twice faster than the two others.

````python
import re
import typing

Options = typing.Dict[str, str]  # key=value fields in various headers

token, quoted = r"([\w!#$%&'*+\-.^_`|~]+)", r'"([^"]*)"'
parameter = re.compile(fr";\s*{token}=(?:{token}|{quoted})", re.ASCII)
firefox_quote_escape = re.compile(r'\\"(?!; |\s*$)')

# RFC's quoted-pair escapes are mostly ignored by browsers. Chrome, Firefox and
# curl all have different escaping, that we try to handle as well as possible,
# even though no client espaces in a way that would allow perfect handling.

def parse_content_header(value: str) -> typing.Tuple[str, Options]:
    """Parse content-type and content-disposition header values.

    E.g. 'form-data; name=upload; filename=\"file.txt\"' to
    ('form-data', {'name': 'upload', 'filename': 'file.txt'})

    Mostly identical to cgi.parse_header and werkzeug.parse_options_header
    but runs faster and handles special characters better. Unescapes quotes.
    """
    value = firefox_quote_escape.sub("%22", value)
    pos = value.find(";")
    if pos == -1:
        options = {}
    else:
        options = {
            m.group(1).lower(): m.group(2) or m.group(3).replace("%22", '"')
            for m in parameter.finditer(value[pos:])
        }
        value = value[:pos]
    return value.strip().lower(), options
````

This does not support werkzeug.parse_options_header's multiple=True, which I didn't see being used anywhere anyway (is that for accept-header or what?). This is written for Sanic, but if anyone cares to port it to Werkzeug, the code is there (sorry -- don't have time to do it myself). Adding support for multiple=True is a few lines of code (with proper handling of quoted commas), if deemed useful.

CPython cgi.parse_header tests as well as two others that I added based on testing real browsers all pass with this:

````python
import pytest

from sanic import headers


@pytest.mark.parametrize(
    "input, expected",
    [
        ("text/plain", ("text/plain", {})),
        ("text/vnd.just.made.this.up ; ", ("text/vnd.just.made.this.up", {})),
        ("text/plain;charset=us-ascii", ("text/plain", {"charset": "us-ascii"})),
        ('text/plain ; charset="us-ascii"', ("text/plain", {"charset": "us-ascii"})),
        (
            'text/plain ; charset="us-ascii"; another=opt',
            ("text/plain", {"charset": "us-ascii", "another": "opt"})
        ),
        (
            'attachment; filename="silly.txt"',
            ("attachment", {"filename": "silly.txt"})
        ),
        (
            'attachment; filename="strange;name"',
            ("attachment", {"filename": "strange;name"})
        ),
        (
            'attachment; filename="strange;name";size=123;',
            ("attachment", {"filename": "strange;name", "size": "123"})
        ),
        (
            'form-data; name="files"; filename="fo\\"o;bar\\"',
            ('form-data', {'name': 'files', 'filename': 'fo"o;bar\\'})
            # cgi.parse_header:
            # ('form-data', {'name': 'files', 'filename': 'fo"o;bar\\'})
            # werkzeug.parse_options_header:
            # ('form-data', {'name': 'files', 'filename': '"fo\\"o', 'bar\\"': None})
        ),
        # <input type=file name="foo&quot;;bar\"> with Unicode filename!
        (
            # Chrome:
            # Content-Disposition: form-data; name="foo%22;bar\"; filename="😀"
            'form-data; name="foo%22;bar\\"; filename="😀"',
            ('form-data', {'name': 'foo";bar\\', 'filename': '😀'})
            # cgi: ('form-data', {'name': 'foo%22;bar"; filename="😀'})
            # werkzeug: ('form-data', {'name': 'foo%22;bar"; filename='})
        ),
        (
            # Firefox:
            # Content-Disposition: form-data; name="foo\";bar\"; filename="😀"
            'form-data; name="foo\\";bar\\"; filename="😀"',
            ('form-data', {'name': 'foo";bar\\', 'filename': '😀'})
            # cgi: ('form-data', {'name': 'foo";bar"; filename="😀'})
            # werkzeug: ('form-data', {'name': 'foo";bar"; filename='})
        ),
    ]
)
def test_parse_headers(input, expected):
    assert headers.parse_content_header(input) == expected
````


Pull request description:
refactor `parse_options_header`
Based on the implementation @tronic shared in #1628, I've refactored `parse_options_header`. The new implementation is 2-3 times faster. The parser now does an initial pass for valid `; key=value` parts, then does two simpler checks for continuation and charset markers on each key.

This parses parameters that are valid according to RFC 9110. Some invalid values were previously accepted, such as `key`, `key=`, and `key = value` (spaces around equals). Invalid parts are ignored.

This updates the parsing of quoted strings to handle both `\"` escaping in HTTP headers and `%22` in multipart form data.

We already handled RFC 2231 continuation and charset markers, however I don't think either feature is sent by modern clients. Only UTF-8, ASCII, and ISO-8859-1 charsets are accepted, others are left quoted.

Removed the remnants of the `multiple=True` flag that was removed earlier.

fixes #1628 

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 328fa6f4..87148b10 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -32,6 +32,9 @@ Unreleased
 -   When using a Unix socket for the development server, the path can start with a dot.
     :issue:`2595`
 -   Increase default work factor for PBKDF2 to 600,000 iterations. :issue:`2611`
+-   ``parse_options_header`` is 2-3 times faster. It conforms to :rfc:`9110`, some
+    invalid parts that were previously accepted are now ignored. :issue:`1628`
+-   The ``is_filename`` parameter to ``unquote_header_value`` is deprecated. :pr:`2614`
 
 
 Version 2.2.3
diff --git a/src/werkzeug/http.py b/src/werkzeug/http.py
index 72dfeee8..207712bf 100644
--- a/src/werkzeug/http.py
+++ b/src/werkzeug/http.py
@@ -13,7 +13,7 @@ from enum import Enum
 from hashlib import sha1
 from time import mktime
 from time import struct_time
-from urllib.parse import unquote_to_bytes as _unquote
+from urllib.parse import unquote
 from urllib.request import parse_http_list as _parse_list_header
 
 from ._internal import _cookie_quote
@@ -50,37 +50,6 @@ _token_chars = frozenset(
     "!#$%&'*+-.0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ^_`abcdefghijklmnopqrstuvwxyz|~"
 )
 _etag_re = re.compile(r'([Ww]/)?(?:"(.*?)"|(.*?))(?:\s*,\s*|$)')
-_option_header_piece_re = re.compile(
-    r"""
-    ;\s*,?\s*  # newlines were replaced with commas
-    (?P<key>
-        "[^"\\]*(?:\\.[^"\\]*)*"  # quoted string
-    |
-        [^\s;,=*]+  # token
-    )
-    (?:\*(?P<count>\d+))?  # *1, optional continuation index
-    \s*
-    (?:  # optionally followed by =value
-        (?:  # equals sign, possibly with encoding
-            \*\s*=\s*  # * indicates extended notation
-            (?:  # optional encoding
-                (?P<encoding>[^\s]+?)
-                '(?P<language>[^\s]*?)'
-            )?
-        |
-            =\s*  # basic notation
-        )
-        (?P<value>
-            "[^"\\]*(?:\\.[^"\\]*)*"  # quoted string
-        |
-            [^;,]+  # token
-        )?
-    )?
-    \s*
-    """,
-    flags=re.VERBOSE,
-)
-_option_header_start_mime_type = re.compile(r",\s*([^;,\s]+)([;,]\s*.+)?")
 _entity_headers = frozenset(
     [
         "allow",
@@ -222,30 +191,28 @@ def quote_header_value(
     return f'"{value}"'
 
 
-def unquote_header_value(value: str, is_filename: bool = False) -> str:
-    r"""Unquotes a header value.  (Reversal of :func:`quote_header_value`).
-    This does not use the real unquoting but what browsers are actually
-    using for quoting.
-
-    .. versionadded:: 0.5
+def unquote_header_value(value: str, is_filename: t.Optional[bool] = None) -> str:
+    """Unquote a header value. Reverse of :func:`quote_header_value`.
 
     :param value: the header value to unquote.
-    :param is_filename: The value represents a filename or path.
+
+    .. versionchanged:: 2.3
+        The ``is_filename`` parameter is deprecated and will be removed in Werkzeug 2.4.
     """
+    if is_filename is not None:
+        warnings.warn(
+            "The 'is_filename' parameter is deprecated and will be"
+            " removed in Werkzeug 2.4.",
+            DeprecationWarning,
+            stacklevel=2,
+        )
+
     if value and value[0] == value[-1] == '"':
-        # this is not the real unquoting, but fixing this so that the
-        # RFC is met will result in bugs with internet explorer and
-        # probably some other browsers as well.  IE for example is
-        # uploading files with "C:\foo\bar.txt" as filename
         value = value[1:-1]
 
-        # if this is a filename and the starting characters look like
-        # a UNC path, then just return the value without quotes.  Using the
-        # replace sequence below on a UNC path has the effect of turning
-        # the leading double slash into a single slash and then
-        # _fix_ie_filename() doesn't work correctly.  See #458.
-        if not is_filename or value[:2] != "\\\\":
+        if not is_filename:
             return value.replace("\\\\", "\\").replace('\\"', '"')
+
     return value
 
 
@@ -387,17 +354,80 @@ def parse_dict_header(value: str, cls: t.Type[dict] = dict) -> t.Dict[str, str]:
     return result
 
 
+# https://httpwg.org/specs/rfc9110.html#parameter
+_parameter_re = re.compile(
+    r"""
+    # don't match multiple empty parts, that causes backtracking
+    \s*;\s*  # find the part delimiter
+    (?:
+        ([\w!#$%&'*+\-.^`|~]+)  # key, one or more token chars
+        =  # equals, with no space on either side
+        (  # value, token or quoted string
+            [\w!#$%&'*+\-.^`|~]+  # one or more token chars
+        |
+            "(?:\\\\|\\"|.)*?"  # quoted string, consuming slash escapes
+        )
+    )?  # optionally match key=value, to account for empty parts
+    """,
+    re.ASCII | re.VERBOSE,
+)
+# https://www.rfc-editor.org/rfc/rfc2231#section-4
+_charset_value_re = re.compile(
+    r"""
+    ([\w!#$%&*+\-.^`|~]*)'  # charset part, could be empty
+    [\w!#$%&*+\-.^`|~]*'  # don't care about language part, usually empty
+    ([\w!#$%&'*+\-.^`|~]+)  # one or more token chars with percent encoding
+    """,
+    re.ASCII | re.VERBOSE,
+)
+# https://www.rfc-editor.org/rfc/rfc2231#section-3
+_continuation_re = re.compile(r"\*(\d+)$", re.ASCII)
+
+
 def parse_options_header(value: t.Optional[str]) -> t.Tuple[str, t.Dict[str, str]]:
-    """Parse a ``Content-Type``-like header into a tuple with the
-    value and any options:
+    """Parse a header that consists of a value with ``key=value`` parameters separated
+    by semicolons ``;``. For example, the ``Content-Type`` header.
+
+    .. code-block:: python
+
+        parse_options_header("text/html; charset=UTF-8")
+        ('text/html', {'charset': 'UTF-8'})
+
+        parse_options_header("")
+        ("", {})
 
-    >>> parse_options_header('text/html; charset=utf8')
-    ('text/html', {'charset': 'utf8'})
+    This parses valid parameter parts as described in
+    `RFC 9110 <https://httpwg.org/specs/rfc9110.html#parameter>`__. Invalid parts are
+    skipped.
 
-    This should is not for ``Cache-Control``-like headers, which use a
-    different format. For those, use :func:`parse_dict_header`.
+    This handles continuations and charsets as described in
+    `RFC 2231 <https://www.rfc-editor.org/rfc/rfc2231#section-3>`__, although not as
+    strictly as the RFC. Only ASCII, UTF-8, and ISO-8859-1 charsets are accepted,
+    otherwise the value remains quoted. These features should not be sent by modern
+    clients.
+
+    Clients may not be consistent in how they handle a quote character within a quoted
+    value. The `HTML Standard <https://html.spec.whatwg.org/#multipart-form-data>`__
+    replaces it with ``%22`` in multipart form data.
+    `RFC 9110 <https://httpwg.org/specs/rfc9110.html#quoted.strings>`__ uses backslash
+    escapes in HTTP headers. Both are decoded to the ``"`` character.
+
+    Clients may not be consistent in how they handle non-ASCII characters. HTML
+    documents must declare ``<meta charset=UTF-8>``, otherwise browsers may replace with
+    HTML character references, which can be decoded using :func:`html.unescape`.
 
     :param value: The header value to parse.
+    :return: ``(value, options)``, where ``options`` is a dict
+
+    .. versionchanged:: 2.3
+        Invalid parts, such as keys with no value, quoted keys, and incorrectly quoted
+        values, are discarded instead of treating as ``None``.
+
+    .. versionchanged:: 2.3
+        Only ASCII, UTF-8, and ISO-8859-1 are accepted for charset values.
+
+    .. versionchanged:: 2.3
+        Escaped quotes in quoted values, like ``%22`` and ``\\"``, are handled.
 
     .. versionchanged:: 2.2
         Option names are always converted to lowercase.
@@ -410,59 +440,71 @@ def parse_options_header(value: t.Optional[str]) -> t.Tuple[str, t.Dict[str, str
 
     .. versionadded:: 0.5
     """
-    if not value:
+    if value is None:
         return "", {}
 
-    result: t.List[t.Any] = []
+    value, _, rest = value.partition(";")
+    value = value.strip()
+    rest = rest.strip()
 
-    value = "," + value.replace("\n", ",")
-    while value:
-        match = _option_header_start_mime_type.match(value)
-        if not match:
-            break
-        result.append(match.group(1))  # mimetype
-        options: t.Dict[str, str] = {}
-        # Parse options
-        rest = match.group(2)
-        encoding: t.Optional[str]
-        continued_encoding: t.Optional[str] = None
-        while rest:
-            optmatch = _option_header_piece_re.match(rest)
-            if not optmatch:
-                break
-            option, count, encoding, language, option_value = optmatch.groups()
-            # Continuations don't have to supply the encoding after the
-            # first line. If we're in a continuation, track the current
-            # encoding to use for subsequent lines. Reset it when the
-            # continuation ends.
-            if not count:
-                continued_encoding = None
-            else:
-                if not encoding:
-                    encoding = continued_encoding
-                continued_encoding = encoding
-            option = unquote_header_value(option).lower()
+    if not value or not rest:
+        # empty (invalid) value, or value without options
+        return value, {}
 
-            if option_value is not None:
-                option_value = unquote_header_value(option_value, option == "filename")
+    rest = f";{rest}"
+    options: t.Dict[str, str] = {}
+    encoding: t.Optional[str] = None
+    continued_encoding: t.Optional[str] = None
 
-                if encoding is not None:
-                    option_value = _unquote(option_value).decode(encoding)
+    for pk, pv in _parameter_re.findall(rest):
+        if not pk:
+            # empty or invalid part
+            continue
 
-            if count:
-                # Continuations append to the existing value. For
-                # simplicity, this ignores the possibility of
-                # out-of-order indices, which shouldn't happen anyway.
-                if option_value is not None:
-                    options[option] = options.get(option, "") + option_value
-            else:
-                options[option] = option_value  # type: ignore[assignment]
+        pk = pk.lower()
+
+        if pk[-1] == "*":
+            # key*=charset''value becomes key=value, where value is percent encoded
+            pk = pk[:-1]
+            match = _charset_value_re.match(pv)
+
+            if match:
+                # If there is a valid charset marker in the value, split it off.
+                encoding, pv = match.groups()
+                # This might be the empty string, handled next.
+                encoding = encoding.lower()
+
+            # No charset marker, or marker with empty charset value.
+            if not encoding:
+                encoding = continued_encoding
+
+            # A safe list of encodings. Modern clients should only send ASCII or UTF-8.
+            # This list will not be extended further. An invalid encoding will leave the
+            # value quoted.
+            if encoding in {"ascii", "us-ascii", "utf-8", "iso-8859-1"}:
+                # Continuation parts don't require their own charset marker. This is
+                # looser than the RFC, it will persist across different keys and allows
+                # changing the charset during a continuation. But this implementation is
+                # much simpler than tracking the full state.
+                continued_encoding = encoding
+                # invalid bytes are replaced during unquoting
+                pv = unquote(pv, encoding=encoding)
+
+        # Remove quotes. At this point the value cannot be empty or a single quote.
+        if pv[0] == pv[-1] == '"':
+            # HTTP headers use slash, multipart form data uses percent
+            pv = pv[1:-1].replace("\\\\", "\\").replace('\\"', '"').replace("%22", '"')
 
-            rest = rest[optmatch.end() :]
-        result.append(options)
-        return tuple(result)  # type: ignore[return-value]
+        match = _continuation_re.search(pk)
+
+        if match:
+            # key*0=a; key*1=b becomes key=ab
+            pk = pk[: match.start()]
+            options[pk] = options.get(pk, "") + pv
+        else:
+            options[pk] = pv
 
-    return tuple(result) if result else ("", {})  # type: ignore[return-value]
+    return value, options
 
 
 _TAnyAccept = t.TypeVar("_TAnyAccept", bound="ds.Accept")


Test patch:
diff --git a/tests/test_http.py b/tests/test_http.py
index 61940a17..9c6ff6d2 100644
--- a/tests/test_http.py
+++ b/tests/test_http.py
@@ -274,68 +274,63 @@ class TestHTTPUtility:
         http.remove_hop_by_hop_headers(headers2)
         assert headers2 == datastructures.Headers([("Foo", "bar")])
 
-    def test_parse_options_header(self):
-        assert http.parse_options_header(None) == ("", {})
-        assert http.parse_options_header("") == ("", {})
-        assert http.parse_options_header(r'something; foo="other\"thing"') == (
-            "something",
-            {"foo": 'other"thing'},
-        )
-        assert http.parse_options_header(r'something; foo="other\"thing"; meh=42') == (
-            "something",
-            {"foo": 'other"thing', "meh": "42"},
-        )
-        assert http.parse_options_header(
-            r'something; foo="other\"thing"; meh=42; bleh'
-        ) == ("something", {"foo": 'other"thing', "meh": "42", "bleh": None})
-        assert http.parse_options_header(
-            'something; foo="other;thing"; meh=42; bleh'
-        ) == ("something", {"foo": "other;thing", "meh": "42", "bleh": None})
-        assert http.parse_options_header('something; foo="otherthing"; meh=; bleh') == (
-            "something",
-            {"foo": "otherthing", "meh": None, "bleh": None},
-        )
-        # Issue #404
-        assert http.parse_options_header(
-            'multipart/form-data; name="foo bar"; filename="bar foo"'
-        ) == ("multipart/form-data", {"name": "foo bar", "filename": "bar foo"})
-        # Examples from RFC
-        assert http.parse_options_header("audio/*; q=0.2, audio/basic") == (
-            "audio/*",
-            {"q": "0.2"},
-        )
-
-        assert http.parse_options_header(
-            "text/plain; q=0.5, text/html\n        text/x-dvi; q=0.8, text/x-c"
-        ) == ("text/plain", {"q": "0.5"})
-        # Issue #932
-        assert http.parse_options_header(
-            "form-data; name=\"a_file\"; filename*=UTF-8''"
-            '"%c2%a3%20and%20%e2%82%ac%20rates"'
-        ) == ("form-data", {"name": "a_file", "filename": "\xa3 and \u20ac rates"})
-        assert http.parse_options_header(
-            "form-data; name*=UTF-8''\"%C5%AAn%C4%ADc%C5%8Dde%CC%BD\"; "
-            'filename="some_file.txt"'
-        ) == (
-            "form-data",
-            {"name": "\u016an\u012dc\u014dde\u033d", "filename": "some_file.txt"},
-        )
+    @pytest.mark.parametrize(
+        ("value", "expect"),
+        [
+            (None, ""),
+            ("", ""),
+            (";a=b", ""),
+            ("v", "v"),
+            ("v;", "v"),
+        ],
+    )
+    def test_parse_options_header_empty(self, value, expect):
+        assert http.parse_options_header(value) == (expect, {})
 
-    def test_parse_options_header_value_with_quotes(self):
-        assert http.parse_options_header(
-            'form-data; name="file"; filename="t\'es\'t.txt"'
-        ) == ("form-data", {"name": "file", "filename": "t'es't.txt"})
-        assert http.parse_options_header(
-            "form-data; name=\"file\"; filename*=UTF-8''\"'🐍'.txt\""
-        ) == ("form-data", {"name": "file", "filename": "'🐍'.txt"})
+    @pytest.mark.parametrize(
+        ("value", "expect"),
+        [
+            ("v;a=b;c=d;", {"a": "b", "c": "d"}),
+            ("v;  ; a=b ; ", {"a": "b"}),
+            ("v;a", {}),
+            ("v;a=", {}),
+            ("v;=b", {}),
+            ('v;a="b"', {"a": "b"}),
+            ("v;a=µ", {}),
+            ('v;a="\';\'";b="µ";', {"a": "';'", "b": "µ"}),
+            ('v;a="b c"', {"a": "b c"}),
+            # HTTP headers use \" for internal "
+            ('v;a="b\\"c";d=e', {"a": 'b"c', "d": "e"}),
+            # HTTP headers use \\ for internal \
+            ('v;a="c:\\\\"', {"a": "c:\\"}),
+            # Invalid trailing slash in quoted part is left as-is.
+            ('v;a="c:\\"', {"a": "c:\\"}),
+            ('v;a="b\\\\\\"c"', {"a": 'b\\"c'}),
+            # multipart form data uses %22 for internal "
+            ('v;a="b%22c"', {"a": 'b"c'}),
+            ("v;a*=b", {"a": "b"}),
+            ("v;a*=ASCII'en'b", {"a": "b"}),
+            ("v;a*=US-ASCII''%62", {"a": "b"}),
+            ("v;a*=UTF-8''%C2%B5", {"a": "µ"}),
+            ("v;a*=US-ASCII''%C2%B5", {"a": "��"}),
+            ("v;a*=BAD''%62", {"a": "%62"}),
+            ("v;a*=UTF-8'''%F0%9F%90%8D'.txt", {"a": "'🐍'.txt"}),
+            ('v;a="🐍.txt"', {"a": "🐍.txt"}),
+            ("v;a*0=b;a*1=c;d=e", {"a": "bc", "d": "e"}),
+            ("v;a*0*=b", {"a": "b"}),
+            ("v;a*0*=UTF-8''b;a*1=c;a*2*=%C2%B5", {"a": "bcµ"}),
+        ],
+    )
+    def test_parse_options_header(self, value, expect) -> None:
+        assert http.parse_options_header(value) == ("v", expect)
 
     def test_parse_options_header_broken_values(self):
         # Issue #995
         assert http.parse_options_header(" ") == ("", {})
-        assert http.parse_options_header(" , ") == ("", {})
+        assert http.parse_options_header(" , ") == (",", {})
         assert http.parse_options_header(" ; ") == ("", {})
-        assert http.parse_options_header(" ,; ") == ("", {})
-        assert http.parse_options_header(" , a ") == ("", {})
+        assert http.parse_options_header(" ,; ") == (",", {})
+        assert http.parse_options_header(" , a ") == (", a", {})
         assert http.parse_options_header(" ; a ") == ("", {})
 
     def test_parse_options_header_case_insensitive(self):