From e79d9dcddcd3760736934294c0804b772deb19ae Mon Sep 17 00:00:00 2001 From: Ashley Sommer Date: Fri, 28 Jun 2024 17:53:54 +1000 Subject: [PATCH 1/4] Add ability to delete a non-secure cookie. Also re-use existing cookie properties where possible if deleting a cookie that is already knowin in the response headers. Sanity-check and correct deletion cookie's secure_prefix and host_prefix based on the value of secure bool. --- sanic/cookies/response.py | 59 +++++++++++++++++++++++++++++------- tests/test_cookies.py | 63 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 11 deletions(-) diff --git a/sanic/cookies/response.py b/sanic/cookies/response.py index a0e18d19c6..69646a3e31 100644 --- a/sanic/cookies/response.py +++ b/sanic/cookies/response.py @@ -363,6 +363,7 @@ def delete_cookie( *, path: str = "/", domain: Optional[str] = None, + secure: bool = True, host_prefix: bool = False, secure_prefix: bool = False, ) -> None: @@ -381,6 +382,8 @@ def delete_cookie( :type path: Optional[str], optional :param domain: Domain of the cookie, defaults to None :type domain: Optional[str], optional + :param secure: Whether to delete a secure cookie. Defaults to True. + :param secure: bool :param host_prefix: Whether to add __Host- as a prefix to the key. This requires that path="/", domain=None, and secure=True, defaults to False @@ -389,8 +392,20 @@ def delete_cookie( This requires that secure=True, defaults to False :type secure_prefix: bool """ + + # Host prefix can only be used on a cookie where + # path=="/", domain==None, and secure==True + host_prefix = ( + False + if not (secure and path == "/" and domain is None) + else host_prefix + ) + # Secure prefix can only be used on a secure cookie + secure_prefix = False if not secure else secure_prefix + # remove it from header cookies: List[Cookie] = self.headers.popall(self.HEADER_KEY, []) + existing_cookie = None for cookie in cookies: if ( cookie.key != Cookie.make_key(key, host_prefix, secure_prefix) @@ -398,23 +413,45 @@ def delete_cookie( or cookie.domain != domain ): self.headers.add(self.HEADER_KEY, cookie) - + else: + # Keep a snapshot of the cookie-to-be-deleted + # for reference when creating the deletion cookie + if existing_cookie is None: + existing_cookie = cookie # This should be removed in v24.3 try: super().__delitem__(key) except KeyError: ... - self.add_cookie( - key=key, - value="", - path=path, - domain=domain, - max_age=0, - samesite=None, - host_prefix=host_prefix, - secure_prefix=secure_prefix, - ) + if existing_cookie is not None: + # Use all the same values as the cookie to be deleted + # except value="" and max_age=0 + self.add_cookie( + key=key, + value="", + path=existing_cookie.path, + domain=existing_cookie.domain, + secure=existing_cookie.secure, + max_age=0, + httponly=existing_cookie.httponly, + partitioned=existing_cookie.partitioned, + samesite=existing_cookie.samesite, + host_prefix=host_prefix, + secure_prefix=secure_prefix, + ) + else: + self.add_cookie( + key=key, + value="", + path=path, + domain=domain, + secure=secure, + max_age=0, + samesite=None, + host_prefix=host_prefix, + secure_prefix=secure_prefix, + ) # In v24.3, we should remove this as being a subclass of dict diff --git a/tests/test_cookies.py b/tests/test_cookies.py index 599a24efd0..4603685c20 100644 --- a/tests/test_cookies.py +++ b/tests/test_cookies.py @@ -380,6 +380,69 @@ def test_cookie_jar_delete_cookie_encode(): ] +def test_cookie_jar_delete_nonsecure_cookie(): + headers = Header() + jar = CookieJar(headers) + jar.delete_cookie("foo", domain="example.com", secure=False) + + encoded = [cookie.encode("ascii") for cookie in jar.cookies] + assert encoded == [ + b'foo=""; Path=/; Domain=example.com; Max-Age=0', + ] + + +def test_cookie_jar_delete_existing_cookie(): + headers = Header() + jar = CookieJar(headers) + jar.add_cookie( + "foo", "test", secure=True, domain="example.com", samesite="Strict" + ) + jar.delete_cookie("foo", domain="example.com", secure=True) + + encoded = [cookie.encode("ascii") for cookie in jar.cookies] + # deletion cookie contains samesite=Strict as was in original cookie + assert encoded == [ + b'foo=""; Path=/; Domain=example.com; Max-Age=0; SameSite=Strict; Secure', + ] + + +def test_cookie_jar_delete_existing_nonsecure_cookie(): + headers = Header() + jar = CookieJar(headers) + jar.add_cookie( + "foo", "test", secure=False, domain="example.com", samesite="Strict" + ) + jar.delete_cookie("foo", domain="example.com", secure=False) + + encoded = [cookie.encode("ascii") for cookie in jar.cookies] + # deletion cookie contains samesite=Strict as was in original cookie + assert encoded == [ + b'foo=""; Path=/; Domain=example.com; Max-Age=0; SameSite=Strict', + ] + + +def test_cookie_jar_delete_existing_nonsecure_cookie_bad_prefix(): + headers = Header() + jar = CookieJar(headers) + jar.add_cookie( + "foo", "test", secure=False, domain="example.com", samesite="Strict" + ) + jar.delete_cookie( + "foo", + domain="example.com", + secure=False, + secure_prefix=True, + host_prefix=True, + ) # noqa + + encoded = [cookie.encode("ascii") for cookie in jar.cookies] + # Deletion cookie has secure_prefix and host_prefix, but original cookie + # is not secure so ignore those invalid options on delete cookie. + assert encoded == [ + b'foo=""; Path=/; Domain=example.com; Max-Age=0; SameSite=Strict', + ] + + def test_cookie_jar_old_school_delete_encode(): headers = Header() jar = CookieJar(headers) From d1bf38d3ea01662fe867d2977e8a271cf5cba302 Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Sun, 30 Jun 2024 09:22:29 +0300 Subject: [PATCH 2/4] Fail explicitly --- sanic/cookies/response.py | 22 ++++++++++++---------- tests/test_cookies.py | 26 ++++++++++++-------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/sanic/cookies/response.py b/sanic/cookies/response.py index 69646a3e31..0921de5d43 100644 --- a/sanic/cookies/response.py +++ b/sanic/cookies/response.py @@ -395,13 +395,16 @@ def delete_cookie( # Host prefix can only be used on a cookie where # path=="/", domain==None, and secure==True - host_prefix = ( - False - if not (secure and path == "/" and domain is None) - else host_prefix - ) - # Secure prefix can only be used on a secure cookie - secure_prefix = False if not secure else secure_prefix + + if host_prefix and not (secure and path == "/" and domain is None): + raise ServerError( + "Cannot set host_prefix on a cookie without " + "path='/', domain=None, and secure=True" + ) + if secure_prefix and not secure: + raise ServerError( + "Cannot set secure_prefix on a cookie without secure=True" + ) # remove it from header cookies: List[Cookie] = self.headers.popall(self.HEADER_KEY, []) @@ -413,11 +416,10 @@ def delete_cookie( or cookie.domain != domain ): self.headers.add(self.HEADER_KEY, cookie) - else: + elif existing_cookie is None: # Keep a snapshot of the cookie-to-be-deleted # for reference when creating the deletion cookie - if existing_cookie is None: - existing_cookie = cookie + existing_cookie = cookie # This should be removed in v24.3 try: super().__delitem__(key) diff --git a/tests/test_cookies.py b/tests/test_cookies.py index 4603685c20..18c01a3a0a 100644 --- a/tests/test_cookies.py +++ b/tests/test_cookies.py @@ -427,20 +427,18 @@ def test_cookie_jar_delete_existing_nonsecure_cookie_bad_prefix(): jar.add_cookie( "foo", "test", secure=False, domain="example.com", samesite="Strict" ) - jar.delete_cookie( - "foo", - domain="example.com", - secure=False, - secure_prefix=True, - host_prefix=True, - ) # noqa - - encoded = [cookie.encode("ascii") for cookie in jar.cookies] - # Deletion cookie has secure_prefix and host_prefix, but original cookie - # is not secure so ignore those invalid options on delete cookie. - assert encoded == [ - b'foo=""; Path=/; Domain=example.com; Max-Age=0; SameSite=Strict', - ] + message = ( + "Cannot set host_prefix on a cookie without " + "path='/', domain=None, and secure=True" + ) + with pytest.raises(ServerError, match=message): + jar.delete_cookie( + "foo", + domain="example.com", + secure=False, + secure_prefix=True, + host_prefix=True, + ) def test_cookie_jar_old_school_delete_encode(): From 2a6550041e0a428afb7ab05cdb98d7994a10b88f Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Sun, 30 Jun 2024 09:23:53 +0300 Subject: [PATCH 3/4] squash --- sanic/cookies/response.py | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/sanic/cookies/response.py b/sanic/cookies/response.py index 0921de5d43..8d718990b2 100644 --- a/sanic/cookies/response.py +++ b/sanic/cookies/response.py @@ -392,10 +392,6 @@ def delete_cookie( This requires that secure=True, defaults to False :type secure_prefix: bool """ - - # Host prefix can only be used on a cookie where - # path=="/", domain==None, and secure==True - if host_prefix and not (secure and path == "/" and domain is None): raise ServerError( "Cannot set host_prefix on a cookie without " @@ -406,7 +402,6 @@ def delete_cookie( "Cannot set secure_prefix on a cookie without secure=True" ) - # remove it from header cookies: List[Cookie] = self.headers.popall(self.HEADER_KEY, []) existing_cookie = None for cookie in cookies: @@ -417,8 +412,6 @@ def delete_cookie( ): self.headers.add(self.HEADER_KEY, cookie) elif existing_cookie is None: - # Keep a snapshot of the cookie-to-be-deleted - # for reference when creating the deletion cookie existing_cookie = cookie # This should be removed in v24.3 try: @@ -541,9 +534,7 @@ def __init__( "Cannot set host_prefix on a cookie without secure=True" ) if path != "/": - raise ServerError( - "Cannot set host_prefix on a cookie unless path='/'" - ) + raise ServerError("Cannot set host_prefix on a cookie unless path='/'") if domain: raise ServerError( "Cannot set host_prefix on a cookie with a defined domain" @@ -645,9 +636,7 @@ def __str__(self): """Format as a Set-Cookie header value.""" output = ["%s=%s" % (self.key, _quote(self.value))] key_index = list(self._keys) - for key, value in sorted( - self.items(), key=lambda x: key_index.index(x[0]) - ): + for key, value in sorted(self.items(), key=lambda x: key_index.index(x[0])): if value is not None and value is not False: if key == "max-age": try: From cb4dc55ae8bdddb2fadb241527f701d736282a0f Mon Sep 17 00:00:00 2001 From: Adam Hopkins Date: Sun, 30 Jun 2024 09:24:02 +0300 Subject: [PATCH 4/4] Cleanup some comments --- sanic/cookies/response.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/sanic/cookies/response.py b/sanic/cookies/response.py index 8d718990b2..e84a24d820 100644 --- a/sanic/cookies/response.py +++ b/sanic/cookies/response.py @@ -534,7 +534,9 @@ def __init__( "Cannot set host_prefix on a cookie without secure=True" ) if path != "/": - raise ServerError("Cannot set host_prefix on a cookie unless path='/'") + raise ServerError( + "Cannot set host_prefix on a cookie unless path='/'" + ) if domain: raise ServerError( "Cannot set host_prefix on a cookie with a defined domain" @@ -636,7 +638,9 @@ def __str__(self): """Format as a Set-Cookie header value.""" output = ["%s=%s" % (self.key, _quote(self.value))] key_index = list(self._keys) - for key, value in sorted(self.items(), key=lambda x: key_index.index(x[0])): + for key, value in sorted( + self.items(), key=lambda x: key_index.index(x[0]) + ): if value is not None and value is not False: if key == "max-age": try: