fix(auth): make logout revoke, and keep the JWT out of the URL

CLAUDE.md

@@ -132,7 +132,8 @@ Key additions for production: `ENVIRONMENT=production`, `ADMIN_PASSWORD`, `CORS_
 - **Coolify `--project-directory`**: Coolify sets `--project-directory` to the repo root, not the compose file location. Relative paths in the compose (`context`, `volumes`) must be relative to the repo root (`./apps/api`, not `../../apps/api`).
 - **Coolify domain persistence**: Domains set in the Coolify UI may be cleared on redeploy/reload. Verify after each deploy. If persistent issues, add Traefik labels directly in the compose.
 - **`.dockerignore` vs `pyproject.toml`**: `apps/api/.dockerignore` excludes `*.md` but `pyproject.toml` references `readme = "README.md"` — `!README.md` exception is required in `.dockerignore` or `uv sync` fails.
-- **OAuth callback dual flow**: `GET /api/v1/auth/github/callback` accepts both OAuth login (with `state` CSRF param) and GitHub App installation redirect (with `installation_id`, no `state`). The `state` parameter is optional.
+- **OAuth callback dual flow**: `GET /api/v1/auth/github/callback` accepts both OAuth login (with `state` CSRF param) and GitHub App installation redirect (with `installation_id`, no `state`). The `state` parameter is optional. It redirects with **no token in the URL** — it sets the httpOnly refresh cookie, and the frontend trades that for an access token via `POST /auth/refresh` (`refreshToken()` in `shared/api/client.ts`).
+- **Refresh tokens are revocable**: each carries a `ver` claim matching `GitHubUser.token_version`; logout bumps the column, which invalidates every outstanding token for that user. `POST /auth/logout` is authenticated because revocation needs to know whose tokens to kill.
 - **GitHub App PEM key**: must be stored as raw PEM (multi-line) in Coolify env vars, NOT base64-encoded. The code passes it directly to `jwt.encode()`.
 - **OAuth tokens must be single-line**: Claude OAuth tokens pasted with line breaks cause `Invalid bearer token` errors. Frontend should strip whitespace/newlines from token input.
 

apps/api/alembic/versions/5b500a24f018_add_token_version_to_github_users.py

@@ -0,0 +1,35 @@
+"""Add token_version to github_users.
+
+Refresh tokens are JWTs, so they cannot be withdrawn once issued. Each one
+now carries the version it was minted under, and logout bumps this column --
+invalidating every outstanding token for that user without storing anything
+per token or needing a job to expire it.
+
+Existing rows default to 0. Tokens issued before this migration carry no
+``ver`` claim and are rejected on their next refresh, logging current users
+out once. That is the intent rather than a side effect: the point of the
+change is that tokens minted under the old rules stop being honoured.
+
+Revision ID: 5b500a24f018
+Revises: c4d5e6f7a0b1
+Create Date: 2026-08-01
+
+"""
+
+from collections.abc import Sequence
+
+import sqlalchemy as sa
+from alembic import op
+
+revision: str = "5b500a24f018"
+down_revision: str | None = "c4d5e6f7a0b1"
+branch_labels: str | Sequence[str] | None = None
+depends_on: str | Sequence[str] | None = None
+
+
+def upgrade() -> None:
+    op.add_column("github_users", sa.Column("token_version", sa.Integer(), server_default="0", nullable=False))
+
+
+def downgrade() -> None:
+    op.drop_column("github_users", "token_version")

apps/api/src/helprs/modules/identity/models.py

@@ -1,6 +1,6 @@
 """User identity ORM models."""
 
-from sqlalchemy import BigInteger, String
+from sqlalchemy import BigInteger, Integer, String
 from sqlalchemy.orm import Mapped, mapped_column
 
 from helprs.core.database import Base
@@ -16,3 +16,8 @@ class GitHubUser(Base):
     email: Mapped[str | None] = mapped_column(String(255), nullable=True)
     avatar_url: Mapped[str | None] = mapped_column(String(512), nullable=True)
     github_access_token_enc: Mapped[str] = mapped_column(String(512), nullable=False)
+    # Bumped on logout to invalidate outstanding refresh tokens. A JWT cannot
+    # be withdrawn once issued, so the token carries the version it was minted
+    # under and refresh compares the two. Cheaper than a denylist: nothing to
+    # store per token and nothing to expire.
+    token_version: Mapped[int] = mapped_column(Integer, nullable=False, default=0, server_default="0")

apps/api/src/helprs/modules/identity/router.py

@@ -15,7 +15,12 @@
 from helprs.core.exceptions import UnauthorizedError
 from helprs.core.middleware import limiter
 from helprs.modules.identity.schemas import TokenResponse, UserResponse, UserStatsResponse
-from helprs.modules.identity.service import authenticate_with_code, get_user_stats, refresh_tokens
+from helprs.modules.identity.service import (
+    authenticate_with_code,
+    get_user_stats,
+    refresh_tokens,
+    revoke_refresh_tokens,
+)
 
 router = APIRouter(prefix="/auth", tags=["auth"])
 
@@ -94,7 +99,12 @@ async def github_callback(
 
     _, tokens = await authenticate_with_code(session, code, settings)
 
-    response = RedirectResponse(url=f"{settings.APP_BASE_URL}/auth/callback?access_token={tokens.access_token}")
+    # No token in the URL. A redirect target lands in browser history, in the
+    # Referer of whatever the page loads next, and in every proxy log on the
+    # way -- for a credential that authenticates the whole API. The frontend
+    # trades the httpOnly refresh cookie set below for an access token by
+    # calling POST /auth/refresh, which puts it in a response body instead.
+    response = RedirectResponse(url=f"{settings.APP_BASE_URL}/auth/callback")
     _set_refresh_cookie(response, tokens.refresh_token, settings)
     response.delete_cookie(_OAUTH_STATE_COOKIE)
     return response
@@ -139,8 +149,21 @@ async def get_me(request: Request, user: CurrentUser) -> UserResponse:
 
 @router.post("/logout")
 @limiter.limit("30/minute")
-async def logout(request: Request, settings: GetSettings) -> Response:
-    """Clear the refresh cookie."""
+async def logout(
+    request: Request,
+    session: DbSession,
+    settings: GetSettings,
+    user: CurrentUser,
+) -> Response:
+    """Revoke the user's refresh tokens and clear the cookie.
+
+    Authenticated on purpose: revocation needs to know whose tokens to
+    invalidate. Clearing the cookie alone left a copied refresh token valid
+    for its full lifetime, which made this endpoint a no-op against anyone
+    who had actually taken one.
+    """
+    await revoke_refresh_tokens(session, user)
+
     response = Response(content='{"status":"ok"}', media_type="application/json")
     # Attributes must match those used to set it, or the browser keeps it.
     response.delete_cookie(

apps/api/src/helprs/modules/identity/service.py

@@ -91,7 +91,9 @@ def create_token_pair(user: GitHubUser, settings: Settings) -> TokenPair:
             settings.SECRET_KEY.get_secret_value(),
         ),
         refresh_token=create_access_token(
-            {"sub": str(user.id), "type": "refresh"},
+            # ver pins the token to the user's current version, so logout can
+            # invalidate every outstanding one by bumping it.
+            {"sub": str(user.id), "type": "refresh", "ver": user.token_version},
             settings.SECRET_KEY.get_secret_value(),
             REFRESH_TOKEN_LIFETIME,
         ),
@@ -112,9 +114,29 @@ async def refresh_tokens(refresh_token: str, session: AsyncSession, settings: Se
     if not user:
         raise UnauthorizedError("User not found")
 
+    if payload.get("ver") != user.token_version:
+        # Issued before a logout. Tokens minted before this field existed have
+        # no "ver" claim and are rejected the same way, which is the intent:
+        # the point of adding it is that older tokens stop being honoured.
+        raise UnauthorizedError("Refresh token has been revoked")
+
     return create_token_pair(user, settings)
 
 
+async def revoke_refresh_tokens(session: AsyncSession, user: GitHubUser) -> None:
+    """Invalidate every refresh token outstanding for this user.
+
+    Logout previously only cleared the cookie, so a token copied out of the
+    browser stayed valid for its full lifetime -- a "log out" that logged
+    nobody out. Bumping the version invalidates all of the user's sessions,
+    not just this browser's: for a tool that holds GitHub and Claude
+    credentials, logging out everywhere is the behaviour worth having, and it
+    is what someone clicking logout after losing a laptop expects.
+    """
+    user.token_version += 1
+    await session.flush()
+
+
 def _parse_subject(subject: object) -> uuid_mod.UUID:
     """Read the ``sub`` claim as a UUID, rejecting anything else."""
     if not isinstance(subject, str):

apps/api/tests/modules/identity/test_router.py

@@ -114,7 +114,12 @@ def _client(*args, **kwargs):
             resp = await client.get(f"/api/v1/auth/github/callback?code=test_code&state={state}")
 
         assert resp.status_code == 307
-        assert "access_token=" in resp.headers["location"]
+        # The token is deliberately NOT in the URL: a redirect target lands in
+        # browser history, in the next request's Referer, and in proxy logs.
+        assert "access_token=" not in resp.headers["location"]
+        assert resp.headers["location"].endswith("/auth/callback")
+        # It is reachable instead by trading the httpOnly cookie set here.
+        assert "refresh_token=" in resp.headers.get("set-cookie", "")
         assert "refresh_token" in resp.headers.get("set-cookie", "")
 
     async def test_invalid_state(self, app_with_db):
@@ -160,7 +165,7 @@ async def test_valid_refresh(self, authed_client, app_with_db):
         settings = get_settings()
 
         refresh_token = create_access_token(
-            {"sub": str(user_id), "type": "refresh"},
+            {"sub": str(user_id), "type": "refresh", "ver": 0},
             settings.SECRET_KEY.get_secret_value(),
             timedelta(days=7),
         )
@@ -185,11 +190,23 @@ async def test_missing_refresh_cookie(self, app_with_db):
 
 
 class TestLogout:
-    async def test_clears_cookie(self, app_with_db):
+    async def test_requires_authentication(self, app_with_db):
+        """Revocation has to know whose tokens to invalidate."""
         async with AsyncClient(
             transport=ASGITransport(app=app_with_db),
             base_url="http://test",
         ) as client:
             resp = await client.post("/api/v1/auth/logout")
-            assert resp.status_code == 200
-            assert resp.json() == {"status": "ok"}
+            assert resp.status_code == 401
+
+    async def test_clears_cookie_and_revokes(self, app_with_db, authed_client):
+        client, user_id = authed_client
+
+        resp = await client.post("/api/v1/auth/logout")
+
+        assert resp.status_code == 200
+        assert resp.json() == {"status": "ok"}
+        async with app_with_db.state.session_factory() as session:
+            refreshed = await session.get(GitHubUser, user_id)
+        # Every refresh token minted under version 0 is now dead.
+        assert refreshed.token_version == 1

apps/api/tests/modules/identity/test_service.py

@@ -20,17 +20,19 @@
     create_token_pair,
     get_decrypted_github_token,
     refresh_tokens,
+    revoke_refresh_tokens,
     sync_user,
 )
 
 
 class StoredUser:
     """Stand-in for a GitHubUser row where no database is needed."""
 
-    def __init__(self, *, encrypted_token: str = "", user_id: uuid.UUID | None = None) -> None:
+    def __init__(self, *, encrypted_token: str = "", user_id: uuid.UUID | None = None, token_version: int = 0) -> None:
         self.id = user_id or uuid.uuid4()
         self.github_login = "testuser"
         self.github_access_token_enc = encrypted_token
+        self.token_version = token_version
 
 
 def _serve_oauth(monkeypatch, *, profile_id: int = 99999999, login: str = "newuser") -> None:
@@ -124,13 +126,15 @@ def test_access_and_refresh_differ_and_carry_the_right_claims(self, settings):
         assert access_claims["github_login"] == "testuser"
         assert "type" not in access_claims
         assert refresh_claims["type"] == "refresh"
+        # Pins the token to the user's version so logout can revoke it.
+        assert refresh_claims["ver"] == user.token_version
 
 
 class TestRefreshTokens:
     async def test_valid_refresh_returns_a_new_pair(self, db_session, settings, test_user):
         user, _ = test_user
         refresh_token = create_access_token(
-            {"sub": str(user.id), "type": "refresh"},
+            {"sub": str(user.id), "type": "refresh", "ver": user.token_version},
             settings.SECRET_KEY.get_secret_value(),
             timedelta(days=7),
         )
@@ -140,6 +144,44 @@ async def test_valid_refresh_returns_a_new_pair(self, db_session, settings, test
         assert isinstance(pair, TokenPair)
         assert decode_access_token(pair.access_token, settings.SECRET_KEY.get_secret_value())["sub"] == str(user.id)
 
+    async def test_a_token_issued_before_logout_is_rejected(self, db_session, settings, test_user):
+        """A refresh token cannot be withdrawn once issued, so logout bumps
+        the version it was minted under. Before this, clearing the cookie was
+        the whole of "logout" and a copied token stayed valid for a week."""
+        user, _ = test_user
+        refresh_token = create_access_token(
+            {"sub": str(user.id), "type": "refresh", "ver": user.token_version},
+            settings.SECRET_KEY.get_secret_value(),
+            timedelta(days=7),
+        )
+
+        await revoke_refresh_tokens(db_session, user)
+
+        with pytest.raises(UnauthorizedError, match="revoked"):
+            await refresh_tokens(refresh_token, db_session, settings)
+
+    async def test_a_token_with_no_version_claim_is_rejected(self, db_session, settings, test_user):
+        """Tokens minted before the field existed. Rejecting them logs current
+        users out once, which is the point of adding it."""
+        user, _ = test_user
+        legacy = create_access_token(
+            {"sub": str(user.id), "type": "refresh"},
+            settings.SECRET_KEY.get_secret_value(),
+            timedelta(days=7),
+        )
+
+        with pytest.raises(UnauthorizedError, match="revoked"):
+            await refresh_tokens(legacy, db_session, settings)
+
+    async def test_a_fresh_token_still_works_after_revocation(self, db_session, settings, test_user):
+        user, _ = test_user
+        await revoke_refresh_tokens(db_session, user)
+
+        reissued = create_token_pair(user, settings)
+
+        pair = await refresh_tokens(reissued.refresh_token, db_session, settings)
+        assert isinstance(pair, TokenPair)
+
     async def test_invalid_refresh_token(self, db_session, settings):
         with pytest.raises(UnauthorizedError, match="Invalid or expired"):
             await refresh_tokens("invalid_token", db_session, settings)