diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 599ca96a2..4ca442c54 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,9 @@ Change history for XBlock Unreleased ---------- +* Fixed ``webob_to_django_response`` dropping all but the last ``Set-Cookie`` + header; they are now loaded into the Django response's cookie jar so every + cookie is sent to the client. * Removed ``threading.RLock`` from ``Mixologist``'s class cache to make it safe for ASGI/async deployments. ``dict.setdefault()`` atomicity (via CPython's GIL) provides equivalent thread safety without blocking the event loop. diff --git a/xblock/django/request.py b/xblock/django/request.py index 067bdb80c..20375c741 100644 --- a/xblock/django/request.py +++ b/xblock/django/request.py @@ -1,9 +1,10 @@ """Helpers for WebOb requests and responses.""" +from http.cookies import SimpleCookie from itertools import chain, repeat from lazy import lazy import webob -from django.http import StreamingHttpResponse, HttpResponse +from django.http import BadHeaderError, StreamingHttpResponse, HttpResponse from webob.multidict import MultiDict, NestedMultiDict, NoVars @@ -22,7 +23,19 @@ def webob_to_django_response(webob_response, streaming=False): status=webob_response.status_code, ) for name, value in webob_response.headerlist: - django_response[name] = value + if name.lower() == 'set-cookie': + # Django headers can't hold multiple values with the same name, + # so Set-Cookie headers go through the response's cookie jar. + # SimpleCookie parses with the request Cookie grammar, so a header + # that does not parse to exactly one cookie would be changed or + # dropped; fail loudly instead. + cookie = SimpleCookie() + cookie.load(value) + if len(cookie) != 1: + raise BadHeaderError("Set-Cookie header did not parse to exactly one cookie") + django_response.cookies.update(cookie) + else: + django_response[name] = value return django_response diff --git a/xblock/test/django/test_request.py b/xblock/test/django/test_request.py index 0ca88dd22..ca3dbb34e 100644 --- a/xblock/test/django/test_request.py +++ b/xblock/test/django/test_request.py @@ -7,6 +7,7 @@ from unittest import TestCase try: + from django.http import BadHeaderError from django.test.client import RequestFactory HAS_DJANGO = True except ImportError: @@ -70,6 +71,23 @@ def test_headers(self): self.assertIn('X-Foo', self._as_django(headerlist=[('X-Foo', 'bar')])) self.assertEqual(self._as_django(headerlist=[('X-Foo', 'bar')])['X-Foo'], 'bar') + def test_multiple_set_cookie_headers(self): + response = self._as_django(headerlist=[ + ('Set-Cookie', 'yummy_cookie=choco; Path=/; HttpOnly'), + ('Set-Cookie', 'tasty_cookie=strawberry; SameSite=Lax'), + ]) + self.assertEqual(response.cookies['yummy_cookie'].value, 'choco') + self.assertEqual(response.cookies['yummy_cookie']['path'], '/') + self.assertTrue(response.cookies['yummy_cookie']['httponly']) + self.assertEqual(response.cookies['tasty_cookie'].value, 'strawberry') + self.assertEqual(response.cookies['tasty_cookie']['samesite'], 'Lax') + + def test_set_cookie_header_that_is_not_one_cookie(self): + # Each of these would change or drop a cookie if loaded into the response's cookie jar. + for value in ('a=1; Path=/; Priority=High; HttpOnly', 'pref=x; sessionid=evil; Path=/', 'a b=1'): + with self.subTest(value=value), self.assertRaises(BadHeaderError): + self._as_django(headerlist=[('Set-Cookie', value)]) + def test_content_types(self): # JSON content type (no charset should be returned) self.assertEqual( @@ -105,6 +123,14 @@ def test_headers(self): self.assertIn('X-Foo', self._as_django(headerlist=[('X-Foo', 'bar')])) self.assertEqual(self._as_django(headerlist=[('X-Foo', 'bar')])['X-Foo'], 'bar') + def test_multiple_set_cookie_headers(self): + response = self._as_django(headerlist=[ + ('Set-Cookie', 'yummy_cookie=choco'), + ('Set-Cookie', 'tasty_cookie=strawberry'), + ]) + self.assertEqual(response.cookies['yummy_cookie'].value, 'choco') + self.assertEqual(response.cookies['tasty_cookie'].value, 'strawberry') + def test_content_types(self): # JSON content type (no charset should be returned) self.assertEqual(