Always clear the session cookie on logout

airflow-core/src/airflow/api_fastapi/auth/tokens.py

@@ -353,13 +353,27 @@ async def avalidated_claims(
         return claims
 
     def revoke_token(self, token: str) -> None:
-        """Validate the token, extract jti and exp, and revoke it in the database."""
+        """
+        Validate the token, extract jti and exp, and revoke it in the database.
+
+        A token that does not validate is not revoked: it is already unusable, so this is logged as a
+        warning. A valid token whose revocation cannot be recorded is still usable until it expires,
+        so that failure is logged as an error rather than being reported the same way.
+        """
         try:
             claims = self.validated_claims(token)
-            if (jti := claims.get("jti")) and (exp := claims.get("exp")):
-                RevokedToken.revoke(jti, datetime.fromtimestamp(exp, tz=timezone.utc))
-        except (jwt.InvalidTokenError, Exception):
-            log.warning("Failed to revoke token", exc_info=True)
+        except Exception:
+            log.warning("Not revoking a token that does not validate", exc_info=True)
+            return
+        if not ((jti := claims.get("jti")) and (exp := claims.get("exp"))):
+            log.warning("Not revoking a token that carries no jti or exp claim")
+            return
+        try:
+            RevokedToken.revoke(jti, datetime.fromtimestamp(exp, tz=timezone.utc))
+        except Exception:
+            log.exception(
+                "Failed to record the revocation of token %s; it remains valid until it expires", jti
+            )
 
     def status(self):
         if self.jwks:

airflow-core/src/airflow/api_fastapi/core_api/routes/public/auth.py

@@ -78,13 +78,12 @@ def logout(
     for token_str in collect_request_tokens(request, bearer_credentials):
         auth_manager.revoke_token(token_str)
 
-    logout_url = auth_manager.get_url_logout()
-    if logout_url:
-        return RedirectResponse(logout_url)
-
+    # The local session cookie is cleared on every path, including the redirect to an external
+    # logout URL: revocation above is recorded server side and can fail, so it must not be the
+    # only thing that ends the browser's session.
     secure = request.base_url.scheme == "https" or bool(conf.get("api", "ssl_cert", fallback=""))
     cookie_path = get_cookie_path()
-    response = RedirectResponse(auth_manager.get_url_login())
+    response = RedirectResponse(auth_manager.get_url_logout() or auth_manager.get_url_login())
     response.delete_cookie(
         key=COOKIE_NAME_JWT_TOKEN,
         path=cookie_path,

airflow-core/tests/unit/api_fastapi/auth/test_tokens.py

@@ -424,6 +424,49 @@ def test_revoke_token_with_db_error_does_not_raise(self):
         ):
             validator.revoke_token(token)
 
+    def test_revoke_token_db_error_is_logged_as_error(self):
+        """A valid token that cannot be revoked stays usable, so the failure is logged as an error."""
+        import time
+        from unittest.mock import patch
+
+        from sqlalchemy.exc import SQLAlchemyError
+
+        now = int(time.time())
+        payload = {
+            "sub": "user",
+            "jti": "db-error-jti",
+            "exp": now + 3600,
+            "iat": now,
+            "nbf": now,
+            "aud": "test",
+        }
+        token = jwt.encode(payload, "secret", algorithm="HS256")
+        validator = JWTValidator(
+            secret_key="secret", audience="test", algorithm=["HS256"], leeway=0, issuer=None
+        )
+        with (
+            patch("airflow.api_fastapi.auth.tokens.log") as mock_log,
+            patch("airflow.models.revoked_token.RevokedToken.revoke", side_effect=SQLAlchemyError("db down")),
+        ):
+            validator.revoke_token(token)
+
+        mock_log.exception.assert_called_once()
+        assert "db-error-jti" in mock_log.exception.call_args.args
+
+    def test_revoke_token_invalid_token_is_not_logged_as_error(self):
+        """A token that does not validate is already unusable, so it is only a warning."""
+        from unittest.mock import patch
+
+        validator = JWTValidator(
+            secret_key="secret", audience="test", algorithm=["HS256"], leeway=0, issuer=None
+        )
+        with patch("airflow.api_fastapi.auth.tokens.log") as mock_log:
+            validator.revoke_token("invalid-token")
+
+        mock_log.exception.assert_not_called()
+        mock_log.error.assert_not_called()
+        mock_log.warning.assert_called_once()
+
 
 @pytest.fixture(scope="session")
 def rsa_private_key():

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_auth.py

@@ -17,7 +17,7 @@
 from __future__ import annotations
 
 import time
-from unittest.mock import MagicMock, patch
+from unittest.mock import AsyncMock, MagicMock, patch
 from urllib.parse import parse_qs, urlencode
 
 import jwt
@@ -351,3 +351,46 @@ def test_logout_revokes_token_when_logout_url_redirects(self, logout_client):
         assert response.status_code == 307
         assert response.headers["location"] == "http://external/logout"
         assert RevokedToken.is_revoked("test-jti-redirect-456") is True
+
+    def test_logout_clears_cookie_when_logout_url_redirects(self, logout_client):
+        """The local session cookie is cleared on the external-logout redirect too, not only revoked."""
+        auth_manager = logout_client.app.state.auth_manager
+        token_str = self._mint(auth_manager, "test-jti-redirect-cookie")
+
+        logout_client.cookies.set(COOKIE_NAME_JWT_TOKEN, token_str)
+        # The refresh middleware is stubbed out so that only the logout route can clear the cookie.
+        with (
+            patch(
+                "airflow.api_fastapi.auth.middlewares.refresh_token.JWTRefreshMiddleware._refresh_user",
+                new=AsyncMock(return_value=(None, None)),
+            ),
+            patch.object(auth_manager, "get_url_logout", return_value="http://external/logout"),
+        ):
+            response = logout_client.get("/auth/logout", follow_redirects=False)
+
+        assert response.status_code == 307
+        assert response.headers["location"] == "http://external/logout"
+        cookies = response.headers.get_list("set-cookie")
+        assert any(c.startswith(f"{COOKIE_NAME_JWT_TOKEN}=") and "Max-Age=0" in c for c in cookies)
+
+    def test_logout_clears_cookie_when_revocation_cannot_be_recorded(self, logout_client):
+        """A failed revocation write does not leave the browser session in place."""
+        from sqlalchemy.exc import SQLAlchemyError
+
+        auth_manager = logout_client.app.state.auth_manager
+        token_str = self._mint(auth_manager, "test-jti-revoke-fails")
+
+        logout_client.cookies.set(COOKIE_NAME_JWT_TOKEN, token_str)
+        with (
+            patch(
+                "airflow.api_fastapi.auth.middlewares.refresh_token.JWTRefreshMiddleware._refresh_user",
+                new=AsyncMock(return_value=(None, None)),
+            ),
+            patch.object(auth_manager, "get_url_logout", return_value="http://external/logout"),
+            patch("airflow.models.revoked_token.RevokedToken.revoke", side_effect=SQLAlchemyError("db down")),
+        ):
+            response = logout_client.get("/auth/logout", follow_redirects=False)
+
+        assert response.status_code == 307
+        cookies = response.headers.get_list("set-cookie")
+        assert any(c.startswith(f"{COOKIE_NAME_JWT_TOKEN}=") and "Max-Age=0" in c for c in cookies)