From 1b58ae2493933d76272cd6c8f53dfb94507273e4 Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Mon, 19 Dec 2022 19:07:06 +0100 Subject: [PATCH 1/4] misc: let django generate set-cookies headers (#72613) Django's HttpResponse cannot hold more than one value for an HTTP headers, so if multiple Set-Cookie are produced by a Quixote HttpResponse the first set-cookie headers will be overwritten by the last one. --- tests/form_pages/test_all.py | 4 ++-- tests/test_auth_pages.py | 2 +- tests/test_saml_auth.py | 2 +- wcs/compat.py | 32 ++++++++++++++++++++++++++++++++ wcs/middleware.py | 3 ++- wcs/qommon/http_response.py | 24 +----------------------- 6 files changed, 39 insertions(+), 28 deletions(-) diff --git a/tests/form_pages/test_all.py b/tests/form_pages/test_all.py index 7559b5e2f..bdc41c8dd 100644 --- a/tests/form_pages/test_all.py +++ b/tests/form_pages/test_all.py @@ -5711,13 +5711,13 @@ def test_session_cookie_flags(pub): create_formdef() app = get_app(pub) resp = app.get('/test/', status=200) - assert resp.headers['Set-Cookie'].startswith('sessionid-') + assert resp.headers['Set-Cookie'].strip().startswith('sessionid-') assert 'HttpOnly' in resp.headers['Set-Cookie'] assert 'Secure' not in resp.headers['Set-Cookie'] app = get_app(pub, https=True) resp = app.get('/test/', status=200) - assert resp.headers['Set-Cookie'].startswith('sessionid-') + assert resp.headers['Set-Cookie'].strip().startswith('sessionid-') assert 'HttpOnly' in resp.headers['Set-Cookie'] assert 'Secure' in resp.headers['Set-Cookie'] diff --git a/tests/test_auth_pages.py b/tests/test_auth_pages.py index 92e8dfb00..23a65eade 100644 --- a/tests/test_auth_pages.py +++ b/tests/test_auth_pages.py @@ -67,7 +67,7 @@ def test_login_cookie(pub): assert list(cookie_store.keys()) == [cookie_name] assert 'HttpOnly' in resp.headers['Set-Cookie'] assert 'SameSite=None' in resp.headers['Set-Cookie'] - assert 'path=/' in resp.headers['Set-Cookie'] + assert 'Path=/' in resp.headers['Set-Cookie'] def test_login_logout(pub): diff --git a/tests/test_saml_auth.py b/tests/test_saml_auth.py index 4c65df2e8..ca80813ad 100644 --- a/tests/test_saml_auth.py +++ b/tests/test_saml_auth.py @@ -607,7 +607,7 @@ def test_opened_session_cookie(pub): assert 'Secure' in resp.headers['Set-Cookie'] assert 'HttpOnly' in resp.headers['Set-Cookie'] assert 'SameSite=None' in resp.headers['Set-Cookie'] - assert 'path=/' in resp.headers['Set-Cookie'] + assert 'Path=/' in resp.headers['Set-Cookie'] assert resp.status_int == 302 assert ( resp.location diff --git a/wcs/compat.py b/wcs/compat.py index d0f48b4c5..1ed7246b1 100644 --- a/wcs/compat.py +++ b/wcs/compat.py @@ -33,6 +33,31 @@ from .qommon.http_request import HTTPRequest from .qommon.publisher import set_publisher_class +def transfer_cookies(quixote_response, django_response): + for name, attrs in quixote_response.cookies.items(): + value = str(attrs['value']) + if 'samesite' not in attrs: + attrs['samesite'] = 'None' + kwargs = {} + samesite_none = False + for attr, val in attrs.items(): + attr = attr.lower() + if val is None: + continue + if attr == 'comment': + continue + if attr == 'samesite' and val.lower() == 'none': + samesite_none = True + elif attr in ('expires', 'domain', 'path', 'max_age', 'samesite'): + kwargs[attr] = val + elif attr in ('httponly', 'secure') and val: + kwargs[attr] = True + django_response.set_cookie(name, value, **kwargs) + # work around absent support for None in django 2.2 + if samesite_none: + django_response.cookies[name]['samesite'] = 'None' + + class TemplateWithFallbackView(TemplateView): quixote_response = None @@ -70,6 +95,8 @@ class TemplateWithFallbackView(TemplateView): else: response = self.render_to_response(context) + transfer_cookies(self.quixote_response, response) + for name, value in self.quixote_response.generate_headers(): if name in ('Connection', 'Content-Length'): continue @@ -83,6 +110,7 @@ class TemplateWithFallbackView(TemplateView): if self.quixote_response and self.quixote_response.status_code != 200: django_response.status_code = self.quixote_response.status_code django_response.reason_phrase = self.quixote_response.reason_phrase + transfer_cookies(self.quixote_response, django_response) for name, value in self.quixote_response.generate_headers(): if name in ('Connection', 'Content-Length'): continue @@ -211,6 +239,8 @@ class CompatWcsPublisher(WcsPublisher): self.session_manager.finish_successful_request() request.ignore_session = True # no further changes + transfer_cookies(response, django_response) + for name, value in response.generate_headers(): if name in ('Connection', 'Content-Length'): continue @@ -264,6 +294,8 @@ class PublishErrorMiddleware(MiddlewareMixin): reason=request.response.reason_phrase, ) + transfer_cookies(request.response, django_response) + for name, value in request.response.generate_headers(): if name in ('Connection', 'Content-Length'): continue diff --git a/wcs/middleware.py b/wcs/middleware.py index 6a29ad53f..d0f23f36e 100644 --- a/wcs/middleware.py +++ b/wcs/middleware.py @@ -26,7 +26,7 @@ from django.utils.deprecation import MiddlewareMixin from quixote import get_publisher from quixote.errors import RequestError -from .compat import CompatHTTPRequest, CompatWcsPublisher +from .compat import CompatHTTPRequest, CompatWcsPublisher, transfer_cookies from .qommon.http_response import HTTPResponse from .qommon.publisher import ImmediateRedirectException @@ -87,6 +87,7 @@ class PublisherInitialisationMiddleware(MiddlewareMixin): if compat_request.form: new_query_string = '?' + urllib.parse.urlencode(compat_request.form) response = HttpResponseRedirect(compat_request.get_path() + new_query_string) + transfer_cookies(compat_request.response, response) for name, value in compat_request.response.generate_headers(): if name == 'Content-Length': continue diff --git a/wcs/qommon/http_response.py b/wcs/qommon/http_response.py index 83ef36f18..165c07aca 100644 --- a/wcs/qommon/http_response.py +++ b/wcs/qommon/http_response.py @@ -35,29 +35,7 @@ class HTTPResponse(quixote.http_response.HTTPResponse): self.charset = get_publisher().site_charset def _gen_cookie_headers(self): - cookie_headers = [] - for name, attrs in self.cookies.items(): - value = str(attrs['value']) - if '"' in value: - value = value.replace('"', '\\"') - chunks = ['%s="%s"' % (name, value)] - if 'samesite' not in attrs: - attrs['samesite'] = 'None' - for name, val in attrs.items(): - name = name.lower() - if val is None: - continue - if name in ('expires', 'domain', 'path', 'max_age', 'comment'): - name = name.replace('_', '-') - chunks.append('%s=%s' % (name, val)) - elif name == 'samesite': - chunks.append('SameSite=%s' % val) - elif name == 'secure' and val: - chunks.append('Secure') - elif name == 'httponly' and val: - chunks.append('HttpOnly') - cookie_headers.append(('Set-Cookie', '; '.join(chunks))) - return cookie_headers + return [] def reset_includes(self): self.javascript_scripts = None -- 2.37.2