Merge pull request #671 from johnpmayer/feature/no-next-url-auth

Drop next_url from authorize_redirect state param

oauthenticator/oauth2.py

@@ -70,13 +70,16 @@ def _OAUTH_ACCESS_TOKEN_URL(self):
     def _OAUTH_USERINFO_URL(self):
         return self.authenticator.userdata_url
 
-    def set_state_cookie(self, state):
-        self._set_cookie(STATE_COOKIE_NAME, state, expires_days=1, httponly=True)
+    def set_state_cookie(self, state_cookie_value):
+        self._set_cookie(
+            STATE_COOKIE_NAME, state_cookie_value, expires_days=1, httponly=True
+        )
 
-    _state = None
+    def _generate_state_id(self):
+        return uuid.uuid4().hex
 
-    def get_state(self):
-        next_url = original_next_url = self.get_argument("next", None)
+    def _get_next_url(self):
+        next_url = self.get_argument("next", None)
         if next_url:
             # avoid browsers treating \ as /
             next_url = next_url.replace("\\", quote("\\"))
@@ -86,23 +89,22 @@ def get_state(self):
             next_url = urlinfo._replace(
                 scheme="", netloc="", path="/" + urlinfo.path.lstrip("/")
             ).geturl()
-            if next_url != original_next_url:
-                self.log.warning(
-                    f"Ignoring next_url {original_next_url}, using {next_url}"
-                )
-        if self._state is None:
-            self._state = _serialize_state(
-                {"state_id": uuid.uuid4().hex, "next_url": next_url}
-            )
-        return self._state
+            return next_url
 
     def get(self):
         redirect_uri = self.authenticator.get_callback_url(self)
         token_params = self.authenticator.extra_authorize_params.copy()
         self.log.info(f"OAuth redirect: {redirect_uri}")
-        state = self.get_state()
-        self.set_state_cookie(state)
-        token_params["state"] = state
+
+        state_id = self._generate_state_id()
+        next_url = self._get_next_url()
+
+        cookie_state = _serialize_state({"state_id": state_id, "next_url": next_url})
+        self.set_state_cookie(cookie_state)
+
+        authorize_state = _serialize_state({"state_id": state_id})
+        token_params["state"] = authorize_state
+
         self.authorize_redirect(
             redirect_uri=redirect_uri,
             client_id=self.authenticator.client_id,
@@ -147,8 +149,12 @@ def check_state(self):
             raise web.HTTPError(400, "OAuth state missing from cookies")
         if not url_state:
             raise web.HTTPError(400, "OAuth state missing from URL")
-        if cookie_state != url_state:
-            self.log.warning(f"OAuth state mismatch: {cookie_state} != {url_state}")
+        cookie_state_id = _deserialize_state(cookie_state).get('state_id')
+        url_state_id = _deserialize_state(url_state).get('state_id')
+        if cookie_state_id != url_state_id:
+            self.log.warning(
+                f"OAuth state mismatch: {cookie_state_id} != {url_state_id}"
+            )
             raise web.HTTPError(400, "OAuth state mismatch")
 
     def check_error(self):
@@ -187,7 +193,7 @@ def append_query_parameters(self, url, exclude=None):
 
     def get_next_url(self, user=None):
         """Get the redirect target from the state field"""
-        state = self.get_state_url()
+        state = self.get_state_cookie()
         if state:
             next_url = _deserialize_state(state).get("next_url")
             if next_url:

oauthenticator/tests/mocks.py

@@ -248,6 +248,10 @@ def mock_handler(Handler, uri='https://hub.example.com', method='GET', **setting
     return handler
 
 
+async def mock_login_user_coro():
+    return True
+
+
 async def no_code_test(authenticator):
     """Run a test to exercise no code in the request"""
     handler = Mock(spec=web.RequestHandler)

oauthenticator/tests/test_oauth2.py

@@ -2,17 +2,20 @@
 import uuid
 from unittest.mock import Mock, PropertyMock
 
-from pytest import mark
+from pytest import mark, raises
+from tornado.web import HTTPError
 from traitlets.config import Config
 
 from ..oauth2 import (
     STATE_COOKIE_NAME,
+    OAuthCallbackHandler,
     OAuthenticator,
+    OAuthLoginHandler,
     OAuthLogoutHandler,
     _deserialize_state,
     _serialize_state,
 )
-from .mocks import mock_handler
+from .mocks import mock_handler, mock_login_user_coro
 
 
 async def test_serialize_state():
@@ -26,6 +29,99 @@ async def test_serialize_state():
     assert state2 == state1
 
 
+TEST_STATE_ID = '123'
+TEST_NEXT_URL = '/ABC'
+
+
+async def test_login_states():
+    login_request_uri = f"http://myhost/login?next={TEST_NEXT_URL}"
+    authenticator = OAuthenticator()
+    login_handler = mock_handler(
+        OAuthLoginHandler,
+        uri=login_request_uri,
+        authenticator=authenticator,
+    )
+
+    login_handler._generate_state_id = Mock(return_value=TEST_STATE_ID)
+
+    login_handler.set_state_cookie = Mock()
+    login_handler.authorize_redirect = Mock()
+
+    login_handler.get()  # no await, we've mocked the authorizer_redirect to NOT be async
+
+    expected_cookie_value = _serialize_state(
+        {
+            'state_id': TEST_STATE_ID,
+            'next_url': TEST_NEXT_URL,
+        }
+    )
+
+    login_handler.set_state_cookie.assert_called_once_with(expected_cookie_value)
+
+    expected_state_param_value = _serialize_state(
+        {
+            'state_id': TEST_STATE_ID,
+        }
+    )
+
+    login_handler.authorize_redirect.assert_called_once()
+    assert (
+        login_handler.authorize_redirect.call_args.kwargs['extra_params']['state']
+        == expected_state_param_value
+    )
+
+
+async def test_callback_check_states_match(monkeypatch):
+    url_state = _serialize_state({'state_id': TEST_STATE_ID})
+    callback_request_uri = f"http://myhost/callback?code=123&state={url_state}"
+
+    cookie_state = _serialize_state(
+        {
+            'state_id': TEST_STATE_ID,
+            'next_url': TEST_NEXT_URL,
+        }
+    )
+
+    authenticator = OAuthenticator()
+    callback_handler = mock_handler(
+        OAuthCallbackHandler,
+        uri=callback_request_uri,
+        authenticator=authenticator,
+    )
+
+    callback_handler.get_secure_cookie = Mock(return_value=cookie_state.encode('utf8'))
+    callback_handler.login_user = Mock(return_value=mock_login_user_coro())
+    callback_handler.redirect = Mock()
+
+    await callback_handler.get()
+
+    callback_handler.redirect.assert_called_once_with('/ABC')
+
+
+async def test_callback_check_states_nomatch():
+    wrong_url_state = _serialize_state({'state_id': 'wr0ng'})
+    callback_request_uri = f"http://myhost/callback?code=123&state={wrong_url_state}"
+
+    cookie_state = _serialize_state(
+        {
+            'state_id': TEST_STATE_ID,
+            'next_url': TEST_NEXT_URL,
+        }
+    )
+
+    authenticator = OAuthenticator()
+    callback_handler = mock_handler(
+        OAuthCallbackHandler,
+        uri=callback_request_uri,
+        authenticator=authenticator,
+    )
+
+    callback_handler.get_secure_cookie = Mock(return_value=cookie_state.encode('utf8'))
+
+    with raises(HTTPError, match="OAuth state mismatch"):
+        await callback_handler.get()
+
+
 async def test_custom_logout(monkeypatch):
     login_url = "http://myhost/login"
     authenticator = OAuthenticator()