Repository: falconry/falcon (python)

Issue:
fix: Special-case Response.append_header to handle Set-Cookie
When setting cookies directly (e.g., when proxying an upstream response with cookie headers), we must special-case Set-Cookie since the usual method of joining multiple header values with a comma is incorrect in that case.

I've got a prototype fix for this already, so I'll go ahead and work on a PR.

Pull request description:
feat: Provide a way to set raw cookie headers
Previously, setting raw cookies was not supported without having to subclass Response and override several methods. This patch adds official support for this functionality, while also explicitly disallowing the use of Set-Cookie with certain generic header manipulation methods that have semantics that are incompatible with Set-Cookie.

Fixes #1265

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 4fd165c..b22c3a1 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -4,6 +4,16 @@
 Breaking Changes
 ----------------
 
+- Previously, several methods in the ``falcon.Response`` class
+  could be used to attempt to set raw cookie headers. However,
+  due to the Set-Cookie header values not being combinable
+  as a comma-delimited list, this resulted in an
+  incorrect response being constructed for the user agent in
+  the case that more than one cookie was being set. Therefore,
+  the following methods of ``falcon.Response`` now raise an
+  instance of ``ValueError`` if an attempt is made to use them
+  for Set-Cookie: ``set_header()``, ``delete_header()``,
+  ``get_header()``, ``set_headers()``.
 - ``testing.Result.json`` now returns ``None`` when the response body is
   empty, rather than raising an error.
 - ``Request.get_param_as_bool()`` now defaults to treating valueless
@@ -83,7 +93,7 @@ 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.
--
+- ``append_header()`` now supports appending raw Set-Cookie header values.
 
 Fixed
 -----
diff --git a/docs/api/cookies.rst b/docs/api/cookies.rst
index faf95a9..dbfc918 100644
--- a/docs/api/cookies.rst
+++ b/docs/api/cookies.rst
@@ -33,15 +33,13 @@ read-only to avoid unintended side-effects.
 Setting Cookies
 ~~~~~~~~~~~~~~~
 
-Setting cookies on a response is done via :py:meth:`~.Response.set_cookie`.
-
-The :py:meth:`~.Response.set_cookie` method should be used instead of
-:py:meth:`~.Response.set_header` or :py:meth:`~.Response.append_header`.
-With :py:meth:`~.Response.set_header` you cannot set multiple headers
-with the same name (which is how multiple cookies are sent to the client).
-Furthermore, :py:meth:`~.Response.append_header` appends multiple values
-to the same header field in a way that is not compatible with the special
-format required by the `Set-Cookie` header.
+Setting cookies on a response may be done either via
+:py:meth:`~.Response.set_cookie` or :py:meth:`~.Response.append_header`.
+
+One of these methods should be used instead of
+:py:meth:`~.Response.set_header`. With :py:meth:`~.Response.set_header` you
+cannot set multiple headers with the same name (which is how multiple cookies
+are sent to the client).
 
 Simple example:
 
diff --git a/docs/changes/2.0.0.rst b/docs/changes/2.0.0.rst
index 3e7749f..1b1f608 100644
--- a/docs/changes/2.0.0.rst
+++ b/docs/changes/2.0.0.rst
@@ -4,6 +4,17 @@ Changelog for Falcon 2.0.0
 Breaking Changes
 ----------------
 
+- Previously, several methods in the :class:`~.Response` class
+  could be used to attempt to set raw cookie headers. However,
+  due to the Set-Cookie header values not being combinable
+  as a comma-delimited list, this resulted in an
+  incorrect response being constructed for the user agent in
+  the case that more than one cookie was being set. Therefore,
+  the following methods of ``falcon.Response`` now raise an
+  instance of ``ValueError`` if an attempt is made to use them
+  for Set-Cookie: :meth:`~.Response.set_header`,
+  :meth:`~.Response.delete_header`, :meth:`~.Response.get_header`,
+  :meth:`~.Response.set_headers`.
 - :attr:`falcon.testing.Result.json` now returns ``None`` when the response body is
   empty, rather than raising an error.
 - :meth:`~.Request.get_param_as_bool` now defaults to treating valueless
@@ -92,6 +103,7 @@ 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.
+- :meth:`~.Response.append_header` now supports appending raw Set-Cookie header values.
 
 Fixed
 -----
diff --git a/falcon/errors.py b/falcon/errors.py
index a6528d1..df21645 100644
--- a/falcon/errors.py
+++ b/falcon/errors.py
@@ -12,7 +12,7 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-"""HTTP error classes.
+"""HTTP error classes and other Falcon-specific errors.
 
 This module implements a collection of `falcon.HTTPError`
 specializations that can be raised to generate a 4xx or 5xx HTTP
@@ -43,6 +43,10 @@ from falcon.http_error import HTTPError, NoRepresentation, \
 import falcon.status_codes as status
 
 
+class HeaderNotSupported(ValueError):
+    """The specified header is not supported by this method."""
+
+
 class HTTPBadRequest(HTTPError):
     """400 Bad Request.
 
diff --git a/falcon/response.py b/falcon/response.py
index f36fa0d..f1cf5e2 100644
--- a/falcon/response.py
+++ b/falcon/response.py
@@ -18,6 +18,7 @@ import mimetypes
 
 
 from falcon import DEFAULT_MEDIA_TYPE
+from falcon.errors import HeaderNotSupported
 from falcon.media import Handlers
 from falcon.response_helpers import (
     format_content_disposition,
@@ -157,6 +158,7 @@ class Response(object):
         'stream',
         '_cookies',
         '_data',
+        '_extra_headers',
         '_headers',
         '_media',
         '__dict__',
@@ -169,6 +171,14 @@ class Response(object):
         self.status = '200 OK'
         self._headers = {}
 
+        # NOTE(kgriffs): Collection of additional headers as a list of raw
+        #   tuples, to use in cases where we need more control over setting
+        #   headers and duplicates are allowable or even necessary.
+        #
+        # PERF(kgriffs): Save some CPU cycles and a few bytes of RAM by
+        #   only instantiating the list object later on IFF it is needed.
+        self._extra_headers = None
+
         self.options = options if options else ResponseOptions()
 
         # NOTE(tbug): will be set to a SimpleCookie object
@@ -449,6 +459,11 @@ class Response(object):
     def get_header(self, name, default=None):
         """Retrieve the raw string value for the given header.
 
+        Normally, when a header has multiple values, they will be
+        returned as a single, comma-delimited string. However, the
+        Set-Cookie header does not support this format, and so
+        attempting to retrieve it will raise an error.
+
         Args:
             name (str): Header name, case-insensitive. Must be of type ``str``
                 or ``StringType``, and only character values 0x00 through 0xFF
@@ -457,20 +472,33 @@ class Response(object):
             default: Value to return if the header
                 is not found (default ``None``).
 
+        Raises:
+            ValueError: The value of the 'Set-Cookie' header(s) was requested.
+
         Returns:
             str: The value of the specified header if set, or
             the default value if not set.
         """
-        return self._headers.get(name.lower(), default)
+
+        # NOTE(kgriffs): normalize name by lowercasing it
+        name = name.lower()
+
+        if name == 'set-cookie':
+            raise HeaderNotSupported('Getting Set-Cookie is not currently supported.')
+
+        return self._headers.get(name, default)
 
     def set_header(self, name, value):
         """Set a header for this response to a given value.
 
         Warning:
-            Calling this method overwrites the existing value, if any.
+            Calling this method overwrites any values already set for this
+            header. To append an additional value for this header, use
+            :meth:`~.append_header` instead.
 
         Warning:
-            For setting cookies, see instead :meth:`~.set_cookie`
+            This method cannot be used to set cookies; instead, use
+            :meth:`~.append_header` or :meth:`~.set_cookie`.
 
         Args:
             name (str): Header name (case-insensitive). The restrictions
@@ -480,6 +508,9 @@ class Response(object):
                 ``StringType``. Strings must contain only US-ASCII characters.
                 Under Python 2.x, the ``unicode`` type is also accepted,
                 although such strings are also limited to US-ASCII.
+
+        Raises:
+            ValueError: `name` cannot be ``'Set-Cookie'``.
         """
 
         # NOTE(kgriffs): uwsgi fails with a TypeError if any header
@@ -490,40 +521,61 @@ class Response(object):
         value = str(value)
 
         # NOTE(kgriffs): normalize name by lowercasing it
-        self._headers[name.lower()] = value
+        name = name.lower()
+
+        if name == 'set-cookie':
+            raise HeaderNotSupported('This method cannot be used to set cookies')
+
+        self._headers[name] = value
 
     def delete_header(self, name):
         """Delete a header that was previously set for this response.
 
         If the header was not previously set, nothing is done (no error is
-        raised).
+        raised). Otherwise, all values set for the header will be removed
+        from the response.
 
         Note that calling this method is equivalent to setting the
-        corresponding header property (when said property is available) to ``None``. For
-        example::
+        corresponding header property (when said property is available) to
+        ``None``. For example::
 
             resp.etag = None
 
+        Warning:
+            This method cannot be used with the Set-Cookie header. Instead,
+            use :meth:`~.unset_cookie` to remove a cookie and ensure that the
+            user agent expires its own copy of the data as well.
+
         Args:
             name (str): Header name (case-insensitive).  Must be of type
                 ``str`` or ``StringType`` and contain only US-ASCII characters.
                 Under Python 2.x, the ``unicode`` type is also accepted,
                 although such strings are also limited to US-ASCII.
+
+        Raises:
+            ValueError: `name` cannot be ``'Set-Cookie'``.
         """
 
         # NOTE(kgriffs): normalize name by lowercasing it
-        self._headers.pop(name.lower(), None)
+        name = name.lower()
+
+        if name == 'set-cookie':
+            raise HeaderNotSupported('This method cannot be used to remove cookies')
+
+        self._headers.pop(name, None)
 
     def append_header(self, name, value):
         """Set or append a header for this response.
 
-        Warning:
-            If the header already exists, the new value will be appended
-            to it, delimited by a comma. Most header specifications support
-            this format, Set-Cookie being the notable exceptions.
+        If the header already exists, the new value will normally be appended
+        to it, delimited by a comma. The notable exception to this rule is
+        Set-Cookie, in which case a separate header line for each value will be
+        included in the response.
 
-        Warning:
-            For setting cookies, see :py:meth:`~.set_cookie`
+        Note:
+            While this method can be used to efficiently append raw
+            Set-Cookie headers to the response, you may find
+            :py:meth:`~.set_cookie` to be more convenient.
 
         Args:
             name (str): Header name (case-insensitive). The restrictions
@@ -542,17 +594,36 @@ class Response(object):
         name = str(name)
         value = str(value)
 
+        # NOTE(kgriffs): normalize name by lowercasing it
         name = name.lower()
-        if name in self._headers:
-            value = self._headers[name] + ', ' + value
 
-        self._headers[name] = value
+        if name == 'set-cookie':
+            if not self._extra_headers:
+                self._extra_headers = [(name, value)]
+            else:
+                self._extra_headers.append((name, value))
+        else:
+            if name in self._headers:
+                value = self._headers[name] + ', ' + value
+
+            self._headers[name] = value
 
     def set_headers(self, headers):
         """Set several headers at once.
 
+        This method can be used to set a collection of raw header names and
+        values all at once.
+
         Warning:
-            Calling this method overwrites existing values, if any.
+            Calling this method overwrites any existing values for the given
+            header. If a list containing multiple instances of the same header
+            is provided, only the last value will be used. To add multiple
+            values to the response for a given header, see
+            :meth:`~.append_header`.
+
+        Warning:
+            This method cannot be used to set cookies; instead, use
+            :meth:`~.append_header` or :meth:`~.set_cookie`.
 
         Args:
             headers (dict or list): A dictionary of header names and values
@@ -586,7 +657,12 @@ class Response(object):
             name = str(name)
             value = str(value)
 
-            _headers[name.lower()] = value
+            name = name.lower()
+
+            if name == 'set-cookie':
+                raise HeaderNotSupported('This method cannot be used to set cookies')
+
+            _headers[name] = value
 
     def add_link(self, target, rel, title=None, title_star=None,
                  anchor=None, hreflang=None, type_hint=None):
@@ -909,6 +985,12 @@ class Response(object):
         else:
             items = list(headers.items())
 
+        if self._extra_headers:
+            items += self._extra_headers
+
+        # NOTE(kgriffs): It is important to append these after self._extra_headers
+        #   in case the latter contains Set-Cookie headers that should be
+        #   overridden by a call to unset_cookie().
         if self._cookies is not None:
             # PERF(tbug):
             # The below implementation is ~23% faster than


Test patch:
diff --git a/tests/test_headers.py b/tests/test_headers.py
index 9581ed9..215334f 100644
--- a/tests/test_headers.py
+++ b/tests/test_headers.py
@@ -181,6 +181,11 @@ class AppendHeaderResource(object):
     def on_post(self, req, resp):
         resp.append_header('X-Things', 'thing-1')
 
+        c1 = 'ut_existing_user=1; expires=Mon, 14-Jan-2019 21:20:08 GMT; Max-Age=600; path=/'
+        resp.append_header('Set-Cookie', c1)
+        c2 = 'partner_source=deleted; expires=Thu, 01-Jan-1970 00:00:01 GMT; Max-Age=0'
+        resp.append_header('seT-cookie', c2)
+
 
 class RemoveHeaderResource(object):
     def __init__(self, with_double_quotes):
@@ -502,6 +507,27 @@ class TestHeaders(object):
 
         result = client.simulate_request(method='POST')
         assert result.headers['x-things'] == 'thing-1'
+        assert result.cookies['ut_existing_user'].max_age == 600
+        assert result.cookies['partner_source'].max_age == 0
+
+    @pytest.mark.parametrize('header_name', ['Set-Cookie', 'set-cookie', 'seT-cookie'])
+    @pytest.mark.parametrize('error_type', [ValueError, falcon.HeaderNotSupported])
+    def test_set_cookie_disallowed(self, client, header_name, error_type):
+        resp = falcon.Response()
+
+        cookie = 'ut_existing_user=1; expires=Mon, 14-Jan-2019 21:20:08 GMT; Max-Age=600; path=/'
+
+        with pytest.raises(error_type):
+            resp.set_header(header_name, cookie)
+
+        with pytest.raises(error_type):
+            resp.set_headers([(header_name, cookie)])
+
+        with pytest.raises(error_type):
+            resp.get_header(header_name)
+
+        with pytest.raises(error_type):
+            resp.delete_header(header_name)
 
     def test_vary_star(self, client):
         client.app.add_route('/', VaryHeaderResource(['*']))