avoid setting unused oauth state cookies on API requests

get_login_url may raise 403 to short-circuit redirects,
so don't create the oauth state cookie until _after_ that has happened

jupyterhub/services/auth.py

@@ -1079,19 +1079,30 @@ class HubAuthenticated:
     def hub_auth(self, auth):
         self._hub_auth = auth
 
+    _hub_login_url = None
+
     def get_login_url(self):
         """Return the Hub's login URL"""
-        login_url = self.hub_auth.login_url
-        if isinstance(self.hub_auth, HubOAuth):
-            # add state argument to OAuth url
-            state = self.hub_auth.set_state_cookie(self, next_url=self.request.uri)
-            login_url = url_concat(login_url, {'state': state})
+        if self._hub_login_url is not None:
+            # cached value, don't call this more than once per handler
+            return self._hub_login_url
         # temporary override at setting level,
         # to allow any subclass overrides of get_login_url to preserve their effect
         # for example, APIHandler raises 403 to prevent redirects
-        with mock.patch.dict(self.application.settings, {"login_url": login_url}):
-            app_log.debug("Redirecting to login url: %s", login_url)
-            return super().get_login_url()
+        with mock.patch.dict(
+            self.application.settings, {"login_url": self.hub_auth.login_url}
+        ):
+            login_url = super().get_login_url()
+        app_log.debug("Redirecting to login url: %s", login_url)
+
+        if isinstance(self.hub_auth, HubOAuth):
+            # add state argument to OAuth url
+            # must do this _after_ allowing get_login_url to raise
+            # so we don't set unused cookies
+            state = self.hub_auth.set_state_cookie(self, next_url=self.request.uri)
+            login_url = url_concat(login_url, {'state': state})
+        self._hub_login_url = login_url
+        return login_url
 
     def check_hub_user(self, model):
         """Check whether Hub-authenticated user or service should be allowed.

jupyterhub/singleuser/extension.py

@@ -130,22 +130,30 @@ class JupyterHubIdentityProvider(IdentityProvider):
     def _patch_get_login_url(self, handler):
         original_get_login_url = handler.get_login_url
 
+        _hub_login_url = None
+
         def get_login_url():
             """Return the Hub's login URL, to begin login redirect"""
-            login_url = self.hub_auth.login_url
-            # add state argument to OAuth url
-            state = self.hub_auth.set_state_cookie(
-                handler, next_url=handler.request.uri
-            )
-            login_url = url_concat(login_url, {'state': state})
-            # temporary override at setting level,
+            nonlocal _hub_login_url
+            if _hub_login_url is not None:
+                # cached value, don't call this more than once per handler
+                return _hub_login_url
+            # temporary override at settings level,
             # to allow any subclass overrides of get_login_url to preserve their effect;
             # for example, APIHandler raises 403 to prevent redirects
             with mock.patch.dict(
-                handler.application.settings, {"login_url": login_url}
+                handler.application.settings, {"login_url": self.hub_auth.login_url}
             ):
-                self.log.debug("Redirecting to login url: %s", login_url)
-                return original_get_login_url()
+                login_url = original_get_login_url()
+            self.log.debug("Redirecting to login url: %s", login_url)
+            # add state argument to OAuth url
+            # must do this _after_ allowing get_login_url to raise
+            # so we don't set unused cookies
+            state = self.hub_auth.set_state_cookie(
+                handler, next_url=handler.request.uri
+            )
+            _hub_login_url = url_concat(login_url, {'state': state})
+            return _hub_login_url
 
         handler.get_login_url = get_login_url
 

jupyterhub/tests/test_singleuser.py

@@ -400,3 +400,16 @@ async def test_token_url_cookie(app, user, full_spawn):
     assert r.status_code == 200
 
     await user.stop()
+
+
+async def test_api_403_no_cookie(app, user, full_spawn):
+    """unused oauth cookies don't get set for failed requests to API handlers"""
+    await user.spawn()
+    await app.proxy.add_user(user)
+    url = url_path_join(public_url(app, user), "/api/contents/")
+    r = await async_requests.get(url, allow_redirects=False)
+    # 403, not redirect
+    assert r.status_code == 403
+    # no state cookie set
+    assert not r.cookies
+    await user.stop()