Repository: jupyterhub/oauthenticator

Issue:
Don't include next_url in state parameter in Login/Callback
<!-- Thank you for contributing. These HTML comments will not render in the issue, but you can delete them once you've read them if you prefer! -->

This comes up when navigating to some bookmarked URL for some file path, or using an extension like nbgitpuller. In this case, when the user is logged in, details such as the file name or repository name are encoded (*not encrypted*) in next_url. These can be considered sensitive identifiers, which you may not want to share with your Identity Provider (whether that's a third-party provider or just another team in your organization which manages your IdP but should not have access to this data).

### Proposed change
<!-- Use this section to describe the feature you'd like to be added. -->

Last time this code was touched:

[validate check state field in oauth callback](https://github.com/jupyterhub/oauthenticator/commit/4ef35fccf49808b9a6a39389dcf13976c0fddacf)

I think a reasonable thing to do is to simply NOT put the next_url in the state send to the IdP:

```
idp_state = base64({state_id})
cookie_state = base64({state_id, next_url})
```

We should still compare the `state_id` in the callback handler; it's important to maintain integrity of the flow by ensuring that the callback with the code was ultimately initiated by JupyterHub. But, we won't have to compare the full contents of the state cookie and the state value used in the token redirect flow.

### Who would use this feature?
<!-- Describe who would benefit from using this feature. -->

Ultimately this is a privacy/secrecy concern. I imagine there are other organizations for whom these identifiers (source code file names, repository names, etc.) are considered confidential, secret, or otherwise sensitive information. I also imagine there are other servers/plugins/extensions which encode different information in their url path that I'm not thinking of.


Discussion:

Patch:
diff --git a/oauthenticator/oauth2.py b/oauthenticator/oauth2.py
index ca4147f..bc09dff 100644
--- a/oauthenticator/oauth2.py
+++ b/oauthenticator/oauth2.py
@@ -70,13 +70,16 @@ class OAuthLoginHandler(OAuth2Mixin, BaseHandler):
     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 @@ class OAuthLoginHandler(OAuth2Mixin, BaseHandler):
             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 @@ class OAuthCallbackHandler(BaseHandler):
             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 @@ class OAuthCallbackHandler(BaseHandler):
 
     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:


Test patch:
diff --git a/oauthenticator/tests/mocks.py b/oauthenticator/tests/mocks.py
index b20af39..83909eb 100644
--- a/oauthenticator/tests/mocks.py
+++ b/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)
diff --git a/oauthenticator/tests/test_oauth2.py b/oauthenticator/tests/test_oauth2.py
index 06600e1..4386e92 100644
--- a/oauthenticator/tests/test_oauth2.py
+++ b/oauthenticator/tests/test_oauth2.py
@@ -2,17 +2,20 @@ import re
 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()