diff --git a/synapse/handlers/oidc_handler.py b/synapse/handlers/oidc_handler.py index 3adc75fa4ab4..8a5c7a4df903 100644 --- a/synapse/handlers/oidc_handler.py +++ b/synapse/handlers/oidc_handler.py @@ -161,13 +161,9 @@ async def handle_oidc_callback(self, request: SynapseRequest) -> None: # Remove the cookie. There is a good chance that if the callback failed # once, it will fail next time and the code will already be exchanged. # Removing it early avoids spamming the provider with token requests. - request.addCookie( - SESSION_COOKIE_NAME, - b"", - path="/_synapse/oidc", - expires="Thu, Jan 01 1970 00:00:00 UTC", - httpOnly=True, - sameSite="lax", + request.cookies.append( + b"%s=; Path=/_synapse/client/oidc; Expires=Thu, Jan 01 1970 00:00:00 UTC; " + b"HttpOnly; SameSite=None; Secure" % (SESSION_COOKIE_NAME,) ) # Check for the state query parameter @@ -693,13 +689,17 @@ async def handle_redirect_request( ui_auth_session_id=ui_auth_session_id, ), ) - request.addCookie( - SESSION_COOKIE_NAME, - cookie, - path="/_synapse/client/oidc", - max_age="3600", - httpOnly=True, - sameSite="lax", + + # we set SameSite=None to ensure that the cookie is included in any POST + # requests to our callback (as is used with `response_mode=form_post`). + # + # Secure is necessary for SameSite=None + # + # we have to build the cookie by hand rather than calling request.addCookie + # to work around https://twistedmatrix.com/trac/ticket/10088 + request.cookies.append( + b"%s=%s; Path=/_synapse/client/oidc; Max-Age=3600; HttpOnly; " + b"SameSite=None; Secure" % (SESSION_COOKIE_NAME, cookie.encode("utf-8")) ) metadata = await self.load_metadata() diff --git a/tests/handlers/test_oidc.py b/tests/handlers/test_oidc.py index ad20400b1dbc..a09f0a40b994 100644 --- a/tests/handlers/test_oidc.py +++ b/tests/handlers/test_oidc.py @@ -308,7 +308,9 @@ def test_skip_verification(self): def test_redirect_request(self): """The redirect request has the right arguments & generates a valid session cookie.""" - req = Mock(spec=["addCookie"]) + req = Mock(spec=["cookies"]) + req.cookies = [] + url = self.get_success( self.provider.handle_redirect_request(req, b"http://client/redirect") ) @@ -328,18 +330,15 @@ def test_redirect_request(self): self.assertEqual(len(params["nonce"]), 1) # Check what is in the cookie - # note: python3.5 mock does not have the .called_once() method - calls = req.addCookie.call_args_list - self.assertEqual(len(calls), 1) # called once - # For some reason, call.args does not work with python3.5 - args = calls[0][0] - kwargs = calls[0][1] + self.assertEqual(len(req.cookies), 1) # one cookie + cookie_header = req.cookies[0] # The cookie name and path don't really matter, just that it has to be coherent # between the callback & redirect handlers. - self.assertEqual(args[0], b"oidc_session") - self.assertEqual(kwargs["path"], "/_synapse/client/oidc") - cookie = args[1] + parts = [p.strip() for p in cookie_header.split(b";")] + self.assertIn(b"Path=/_synapse/client/oidc", parts) + name, cookie = parts[0].split(b"=") + self.assertEqual(name, b"oidc_session") macaroon = pymacaroons.Macaroon.deserialize(cookie) state = self.handler._token_generator._get_value_from_macaroon( @@ -470,7 +469,7 @@ def test_callback(self): def test_callback_session(self): """The callback verifies the session presence and validity""" - request = Mock(spec=["args", "getCookie", "addCookie"]) + request = Mock(spec=["args", "getCookie", "cookies"]) # Missing cookie request.args = {} @@ -910,13 +909,14 @@ def _build_callback_request( spec=[ "args", "getCookie", - "addCookie", + "cookies", "requestHeaders", "getClientIP", "getHeader", ] ) + request.cookies = [] request.getCookie.return_value = session request.args = {} request.args[b"code"] = [code.encode("utf-8")]