Repository: falconry/falcon

Issue:
Optimize request cookie parsing
In 1.3, the request cookie parser was changed to parse individual cookies so that malformed ones could be skipped, rather than giving up on parsing the entire header. However, this is significantly slower than parsing the entire header value in one shot, esp. since the SimpleCookie parser is rather inefficient.

We should be able to do better. We can start with the SimpleCookie code from the latest CPython 3.7 code base, optimize it, and make it compatible with Python 2.

See also:

[request.py#L894](https://github.com/falconry/falcon/blob/master/falcon/request.py#L894)

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=h1) Report
> Merging [#1447](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/40189ed55cd87062b2051c626e32775458b81e7d?src=pr&el=desc) will **decrease** coverage by `0.5%`.
> The diff coverage is `78.57%`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1447/graphs/tree.svg?width=650&token=zOtkfNQ3h6&height=150&src=pr)](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1447      +/-   ##
==========================================
- Coverage     100%   99.49%   -0.51%     
==========================================
  Files          39       39              
  Lines        2558     2580      +22     
  Branches      395      403       +8     
==========================================
+ Hits         2558     2567       +9     
- Misses          0        9       +9     
- Partials        0        4       +4
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/request.py](https://codecov.io/gh/falconry/falcon/pull/1447/diff?src=pr&el=tree#diff-ZmFsY29uL3JlcXVlc3QucHk=) | `98.63% <100%> (-1.37%)` | :arrow_down: |
| [falcon/request\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1447/diff?src=pr&el=tree#diff-ZmFsY29uL3JlcXVlc3RfaGVscGVycy5weQ==) | `90.76% <68.42%> (-9.24%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=footer). Last update [40189ed...84b9972](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

codecov[bot]: # [Codecov](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=h1) Report
> Merging [#1447](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=desc) into [master](https://codecov.io/gh/falconry/falcon/commit/40189ed55cd87062b2051c626e32775458b81e7d?src=pr&el=desc) will **decrease** coverage by `0.5%`.
> The diff coverage is `78.57%`.

[![Impacted file tree graph](https://codecov.io/gh/falconry/falcon/pull/1447/graphs/tree.svg?width=650&token=zOtkfNQ3h6&height=150&src=pr)](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1447      +/-   ##
==========================================
- Coverage     100%   99.49%   -0.51%     
==========================================
  Files          39       39              
  Lines        2558     2580      +22     
  Branches      395      403       +8     
==========================================
+ Hits         2558     2567       +9     
- Misses          0        9       +9     
- Partials        0        4       +4
```


| [Impacted Files](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [falcon/request.py](https://codecov.io/gh/falconry/falcon/pull/1447/diff?src=pr&el=tree#diff-ZmFsY29uL3JlcXVlc3QucHk=) | `98.63% <100%> (-1.37%)` | :arrow_down: |
| [falcon/request\_helpers.py](https://codecov.io/gh/falconry/falcon/pull/1447/diff?src=pr&el=tree#diff-ZmFsY29uL3JlcXVlc3RfaGVscGVycy5weQ==) | `90.76% <68.42%> (-9.24%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=footer). Last update [40189ed...84b9972](https://codecov.io/gh/falconry/falcon/pull/1447?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

vytas7: LGTM now!

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index bf705ab..54c3a28 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -61,6 +61,9 @@ Breaking Changes
   Falcon detects that it is running on the wsgiref server. If you
   need to normalize stream semantics between wsgiref and a production WSGI
   server, ``Request.bounded_stream`` may be used instead.
+- ``Request.cookies`` now gives precedence to the first value
+  encountered in the Cookie header for a given cookie name, rather than the
+  last.
 
 Changes to Supported Platforms
 ------------------------------
@@ -109,6 +112,11 @@ New & Improved
   ``dumps()`` and ``loads()`` functions. This enables support not only for
   using any of a number of third-party JSON libraries, but also for
   customizing the keyword arguments used when (de)serializing objects.
+- Added a new method, ``get_cookie_values()``, to the ``Request``
+  class. The new method supports getting all values provided for a given
+  cookie, and is now the preferred mechanism for reading request cookies.
+- Optimized request cookie parsing. It is now roughly an order of magnitude
+  faster.
 - ``append_header()`` now supports appending raw Set-Cookie header values.
 
 Fixed
diff --git a/docs/api/cookies.rst b/docs/api/cookies.rst
index dbfc918..828780c 100644
--- a/docs/api/cookies.rst
+++ b/docs/api/cookies.rst
@@ -3,15 +3,16 @@
 Cookies
 -------
 
-Cookie support is available in Falcon version 0.3 or later.
-
 .. _getting-cookies:
 
 Getting Cookies
 ~~~~~~~~~~~~~~~
 
-Cookies can be read from a request via the :py:attr:`~.Request.cookies`
-request attribute:
+Cookies can be read from a request either via the
+:py:meth:`~.Request.get_cookie_values` method or the :py:attr:`~.Request.cookies`
+attribute on the :py:class:`~.Request` object. Generally speaking, the
+:py:meth:`~.Request.get_cookie_values` method should be used unless you need a
+collection of all the cookies in the request.
 
 .. code:: python
 
@@ -20,13 +21,13 @@ request attribute:
 
             cookies = req.cookies
 
-            if 'my_cookie' in cookies:
-                my_cookie_value = cookies['my_cookie']
-            # ....
+            my_cookie_values = req.get_cookie_values('my_cookie')
+            if my_cookie_values:
+                # NOTE: If there are multiple values set for the cookie, you
+                # will need to choose how to handle the additional values.
+                v = my_cookie_values[0]
 
-The :py:attr:`~.Request.cookies` attribute is a regular
-:py:class:`dict` object. The returned object should be treated as
-read-only to avoid unintended side-effects.
+                # ...
 
 .. _setting-cookies:
 
diff --git a/docs/changes/2.0.0.rst b/docs/changes/2.0.0.rst
index b9f5c89..60effcd 100644
--- a/docs/changes/2.0.0.rst
+++ b/docs/changes/2.0.0.rst
@@ -64,6 +64,9 @@ Breaking Changes
   stream when Falcon detects that it is running on the wsgiref server. If you
   need to normalize stream semantics between wsgiref and a production WSGI
   server, :attr:`~.Request.bounded_stream` may be used instead.
+- :attr:`falcon.Request.cookies` now gives precedence to the first value
+  encountered in the Cookie header for a given cookie name, rather than the
+  last.
 
 Changes to Supported Platforms
 ------------------------------
@@ -120,6 +123,12 @@ New & Improved
   ``dumps()`` and ``loads()`` functions. This enables support not only for
   using any of a number of third-party JSON libraries, but also for
   customizing the keyword arguments used when (de)serializing objects.
+- Added a new method, :meth:`~.Request.get_cookie_values`, to the
+  :class:`~.Request` class. The new method supports getting all values
+  provided for a given cookie, and is now the preferred mechanism for
+  reading request cookies.
+- Optimized request cookie parsing. It is now roughly an order of magnitude
+  faster.
 - :meth:`~.Response.append_header` now supports appending raw Set-Cookie header values.
 
 Fixed
diff --git a/falcon/request.py b/falcon/request.py
index c3a69ed..4e6ae76 100644
--- a/falcon/request.py
+++ b/falcon/request.py
@@ -27,11 +27,6 @@ from falcon.util import json
 from falcon.util.uri import parse_host, parse_query_string
 from falcon.vendor import mimeparse
 
-# NOTE(tbug): In some cases, compat.http_cookies is not a module
-# but a dict-like structure. This fixes that issue.
-# See issue https://github.com/falconry/falcon/issues/556
-SimpleCookie = compat.http_cookies.SimpleCookie
-
 DEFAULT_ERROR_LOG_FORMAT = (u'{0:%Y-%m-%d %H:%M:%S} [FALCON] [ERROR]'
                             u' {1} {2}{3} => ')
 
@@ -256,8 +251,12 @@ class Request(object):
         client_accepts_xml (bool): ``True`` if the Accept header indicates that
             the client is willing to receive XML, otherwise ``False``.
         cookies (dict):
-            A dict of name/value cookie pairs. (See also:
-            :ref:`Getting Cookies <getting-cookies>`)
+            A dict of name/value cookie pairs. The returned object should be
+            treated as read-only to avoid unintended side-effects.
+            If a cookie appears more than once in the request, only the first
+            value encountered will be made available here.
+
+            See also: :meth:`~get_cookie_values`
         content_type (str): Value of the Content-Type header, or ``None`` if
             the header is missing.
         content_length (int): Value of the Content-Length header converted
@@ -394,7 +393,6 @@ class Request(object):
         '_cached_prefix',
         '_cached_relative_uri',
         '_cached_uri',
-        '_cookies',
         '_params',
         '_wsgierrors',
         'content_type',
@@ -409,6 +407,9 @@ class Request(object):
         '_media',
     )
 
+    _cookies = None
+    _cookies_collapsed = None
+
     # Child classes may override this
     context_type = type('RequestContext', (dict,), {})
 
@@ -456,8 +457,6 @@ class Request(object):
             else:
                 self._params = {}
 
-        self._cookies = None
-
         self._cached_access_route = None
         self._cached_forwarded = None
         self._cached_forwarded_prefix = None
@@ -825,24 +824,17 @@ class Request(object):
 
     @property
     def cookies(self):
-        if self._cookies is None:
-            # NOTE(tbug): We might want to look into parsing
-            # cookies ourselves. The SimpleCookie is doing a
-            # lot if stuff only required to SEND cookies.
-            cookie_header = self.get_header('Cookie', default='')
-            parser = SimpleCookie()
-            for cookie_part in cookie_header.split('; '):
-                try:
-                    parser.load(cookie_part)
-                except compat.http_cookies.CookieError:
-                    pass
-            cookies = {}
-            for morsel in parser.values():
-                cookies[morsel.key] = morsel.value
+        if self._cookies_collapsed is None:
+            if self._cookies is None:
+                header_value = self.get_header('Cookie')
+                if header_value:
+                    self._cookies = helpers.parse_cookie_header(header_value)
+                else:
+                    self._cookies = {}
 
-            self._cookies = cookies
+            self._cookies_collapsed = {n: v[0] for n, v in self._cookies.items()}
 
-        return self._cookies
+        return self._cookies_collapsed
 
     @property
     def access_route(self):
@@ -1077,6 +1069,37 @@ class Request(object):
                    'Section 7.1.1.1')
             raise errors.HTTPInvalidHeader(msg, header)
 
+    def get_cookie_values(self, name):
+        """Return all values provided in the Cookie header for the named cookie.
+
+        (See also: :ref:`Getting Cookies <getting-cookies>`)
+
+        Args:
+            name (str): Cookie name, case-sensitive.
+
+        Returns:
+            list: Ordered list of all values specified in the Cookie header for
+            the named cookie, or ``None`` if the cookie was not included in
+            the request. If the cookie is specified more than once in the
+            header, the returned list of values will preserve the ordering of
+            the individual ``cookie-pair``'s in the header.
+        """
+
+        if self._cookies is None:
+            # PERF(kgriffs): While this code isn't exactly DRY (the same code
+            # is duplicated by the cookies property) it does make things a bit
+            # more performant by removing the extra function call that would
+            # be required to factor this out. If we ever have to do this in a
+            # *third* place, we would probably want to factor it out at that
+            # point.
+            header_value = self.get_header('Cookie')
+            if header_value:
+                self._cookies = helpers.parse_cookie_header(header_value)
+            else:
+                self._cookies = {}
+
+        return self._cookies.get(name)
+
     def get_param(self, name, required=False, store=None, default=None):
         """Return the raw value of a query string parameter as a string.
 
diff --git a/falcon/request_helpers.py b/falcon/request_helpers.py
index 13b9a1e..421b20e 100644
--- a/falcon/request_helpers.py
+++ b/falcon/request_helpers.py
@@ -15,6 +15,78 @@
 """Utilities for the Request class."""
 
 import io
+import re
+
+from falcon.util.compat import http_cookies
+
+# https://tools.ietf.org/html/rfc6265#section-4.1.1
+#
+# NOTE(kgriffs): Fortunately we don't have to worry about code points in
+#   header strings outside the range 0x0000 - 0x00FF per PEP 3333
+#   (see also: https://www.python.org/dev/peps/pep-3333/#unicode-issues)
+#
+_COOKIE_NAME_RESERVED_CHARS = re.compile('[\x00-\x1F\x7F-\xFF()<>@,;:\\\\"/[\\]?={} \x09]')
+
+
+def parse_cookie_header(header_value):
+    """Parse a Cookie header value into a dict of named values.
+
+    (See also: RFC 6265, Section 5.4)
+
+    Args:
+        header_value (str): Value of a Cookie header
+
+    Returns:
+        dict: Map of cookie names to a list of all cookie values found in the
+        header for that name. If a cookie is specified more than once in the
+        header, the order of the values will be preserved.
+    """
+
+    # See also:
+    #
+    #   https://tools.ietf.org/html/rfc6265#section-5.4
+    #   https://tools.ietf.org/html/rfc6265#section-4.1.1
+    #
+
+    cookies = {}
+
+    for token in header_value.split(';'):
+        name, __, value = token.partition('=')
+
+        # NOTE(kgriffs): RFC6265 is more strict about whitespace, but we
+        # are more lenient here to better handle old user agents and to
+        # mirror Python's standard library cookie parsing behavior
+        name = name.strip()
+        value = value.strip()
+
+        # NOTE(kgriffs): Skip malformed cookie-pair
+        if not name:
+            continue
+
+        # NOTE(kgriffs): Skip cookies with invalid names
+        if _COOKIE_NAME_RESERVED_CHARS.search(name):
+            continue
+
+        # NOTE(kgriffs): To maximize compatibility, we mimic the support in the
+        # standard library for escaped characters within a double-quoted
+        # cookie value according to the obsolete RFC 2109. However, we do not
+        # expect to see this encoding used much in practice, since Base64 is
+        # the current de-facto standard, as recommended by RFC 6265.
+        #
+        # PERF(kgriffs): These checks have been hoisted from within _unquote()
+        # to avoid the extra function call in the majority of the cases when it
+        # is not needed.
+        if len(value) > 2 and value[0] == '"' and value[-1] == '"':
+            value = http_cookies._unquote(value)
+
+        # PERF(kgriffs): This is slightly more performant as
+        # compared to using dict.setdefault()
+        if name in cookies:
+            cookies[name].append(value)
+        else:
+            cookies[name] = [value]
+
+    return cookies
 
 
 def header_property(wsgi_name):


Test patch:
diff --git a/tests/test_cookies.py b/tests/test_cookies.py
index 9d34b46..e0cbfcf 100644
--- a/tests/test_cookies.py
+++ b/tests/test_cookies.py
@@ -241,7 +241,9 @@ def test_request_cookie_parsing():
             'Cookie',
             """
             logged_in=no;_gh_sess=eyJzZXXzaW9uX2lkIjoiN2;
-            tz=Europe/Berlin; _ga=GA1.2.332347814.1422308165;
+            tz=Europe/Berlin; _ga =GA1.2.332347814.1422308165;
+            tz2=Europe/Paris ; _ga2="line1\\012line2";
+            tz3=Europe/Madrid ;_ga3= GA3.2.332347814.1422308165;
             _gat=1;
             _octo=GH1.1.201722077.1422308165
             """
@@ -251,39 +253,75 @@ def test_request_cookie_parsing():
     environ = testing.create_environ(headers=headers)
     req = falcon.Request(environ)
 
-    assert req.cookies['logged_in'] == 'no'
-    assert req.cookies['tz'] == 'Europe/Berlin'
-    assert req.cookies['_octo'] == 'GH1.1.201722077.1422308165'
-
-    assert 'logged_in' in req.cookies
-    assert '_gh_sess' in req.cookies
-    assert 'tz' in req.cookies
-    assert '_ga' in req.cookies
-    assert '_gat' in req.cookies
-    assert '_octo' in req.cookies
+    # NOTE(kgriffs): Test case-sensitivity
+    assert req.get_cookie_values('TZ') is None
+    assert 'TZ' not in req.cookies
+    with pytest.raises(KeyError):
+        req.cookies['TZ']
+
+    for name, value in [
+        ('logged_in', 'no'),
+        ('_gh_sess', 'eyJzZXXzaW9uX2lkIjoiN2'),
+        ('tz', 'Europe/Berlin'),
+        ('tz2', 'Europe/Paris'),
+        ('tz3', 'Europe/Madrid'),
+        ('_ga', 'GA1.2.332347814.1422308165'),
+        ('_ga2', 'line1\nline2'),
+        ('_ga3', 'GA3.2.332347814.1422308165'),
+        ('_gat', '1'),
+        ('_octo', 'GH1.1.201722077.1422308165'),
+    ]:
+        assert name in req.cookies
+        assert req.cookies[name] == value
+        assert req.get_cookie_values(name) == [value]
 
 
 def test_invalid_cookies_are_ignored():
+    vals = [chr(i) for i in range(0x1F)]
+    vals += [chr(i) for i in range(0x7F, 0xFF)]
+    vals += '()<>@,;:\\"/[]?={} \x09'.split()
+
+    for c in vals:
+        headers = [
+            (
+                'Cookie',
+                'good_cookie=foo;bad' + c + 'cookie=bar'
+            ),
+        ]
+
+        environ = testing.create_environ(headers=headers)
+        req = falcon.Request(environ)
+
+        assert req.cookies['good_cookie'] == 'foo'
+        assert 'bad' + c + 'cookie' not in req.cookies
+
+
+def test_duplicate_cookie():
     headers = [
         (
             'Cookie',
-            """
-            good_cookie=foo;
-            bad{cookie=bar
-            """
+            'x=1;bad{cookie=bar; x=2;x=3 ; x=4;'
         ),
     ]
 
     environ = testing.create_environ(headers=headers)
     req = falcon.Request(environ)
 
-    assert req.cookies['good_cookie'] == 'foo'
-    assert 'bad{cookie' not in req.cookies
+    assert req.cookies['x'] == '1'
+    assert req.get_cookie_values('x') == ['1', '2', '3', '4']
 
 
 def test_cookie_header_is_missing():
     environ = testing.create_environ(headers={})
+
+    req = falcon.Request(environ)
+    assert req.cookies == {}
+    assert req.get_cookie_values('x') is None
+
+    # NOTE(kgriffs): Test again with a new object to cover calling in the
+    #   opposite order.
     req = falcon.Request(environ)
+    assert req.get_cookie_values('x') is None
     assert req.cookies == {}