Merge a79f417 into 3d68e56

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
@@ -76,7 +86,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
 -----

docs/api/cookies.rst

@@ -39,15 +39,13 @@ The :py:attr:`~.Request.cookies` attribute is a regular
 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:
 

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
@@ -85,6 +96,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
 -----

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 @@ def on_get(self, req, resp):
 import falcon.status_codes as status
 
 
+class HeaderNotSupported(ValueError):
+    """The specified header is not supported by this method."""
+
+
 class HTTPBadRequest(HTTPError):
     """400 Bad Request.
 

falcon/response.py

@@ -18,6 +18,7 @@
 
 
 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 @@ def __init__(self, options=None):
         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 @@ def unset_cookie(self, name):
     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 @@ def get_header(self, name, default=None):
             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 @@ def set_header(self, name, value):
                 ``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 @@ def set_header(self, name, value):
         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 @@ def append_header(self, name, value):
         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 @@ def set_headers(self, headers):
             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 @@ def _wsgi_headers(self, media_type=None, py2=compat.PY2):
         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

tests/test_headers.py

@@ -181,6 +181,11 @@ def on_head(self, req, resp):
     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 @@ def test_response_append_header(self, client):
 
         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(['*']))