fix(security): close a set of small auth and hardening gaps

backend/endpoints/user.py

@@ -12,6 +12,7 @@
 from endpoints.responses.identity import InviteLinkSchema, UserSchema
 from handler.auth import auth_handler
 from handler.auth.constants import Scope
+from handler.auth.middleware.redis_session_middleware import RedisSessionMiddleware
 from handler.database import db_user_handler
 from handler.filesystem import fs_asset_handler
 from handler.filesystem.assets_handler import (
@@ -201,6 +202,10 @@ def create_user_from_invite(
         UserSchema: Newly created user
     """
 
+    # Ahead of the "already exists" checks, which would otherwise enumerate
+    # accounts for an invalid token. Not consumed, so a retry keeps the invite.
+    auth_handler.assert_invite_link_token_valid(token)
+
     try:
         validate_username(username)
         validate_password(password)
@@ -471,18 +476,25 @@ async def update_user(
         cleaned_data["avatar_path"] = file_location
 
     if cleaned_data:
+        # Sessions are keyed by username, so the old one is what identifies
+        # them once the update has renamed the account.
+        previous_username = db_user.username
+
         db_user_handler.update_user(id, cleaned_data)
 
         # A role change alters the user's effective permissions; tell their UI.
         if "role" in cleaned_data:
             await emit_permissions_changed(id)
 
-        # Log out the current user if username or password changed
+        # The target's sessions, not the caller's: an admin resetting a
+        # compromised account has to lock its attacker out.
         creds_updated = cleaned_data.get("username") or cleaned_data.get(
             "hashed_password"
         )
-        if request.user.id == id and creds_updated:
-            request.session.clear()
+        if creds_updated:
+            await RedisSessionMiddleware.clear_user_sessions(previous_username)
+            if request.user.id == id:
+                request.session.clear()
 
     db_user = db_user_handler.get_user(id)
     if not db_user:
@@ -558,6 +570,10 @@ async def refresh_retro_achievements(
     ] = False,
 ) -> None:
     """Refresh RetroAchievements progression data for a user."""
+    # Admin users can refresh any user, while other users can only refresh self
+    if id != request.user.id and request.user.role != Role.ADMIN:
+        raise HTTPException(status_code=status.HTTP_403_FORBIDDEN, detail="Forbidden")
+
     user = db_user_handler.get_user(id)
     if not user or not user.ra_username:
         raise HTTPException(

backend/handler/auth/base_handler.py

@@ -74,6 +74,15 @@ def _romm_username(provided: str, fallback: str) -> str:
     return username
 
 
+def _invite_token_spent() -> HTTPException:
+    """The one response for a spent or unusable invite, so neither caller
+    distinguishes them."""
+    return HTTPException(
+        status_code=status.HTTP_400_BAD_REQUEST,
+        detail="Invite token has already been used or is invalid.",
+    )
+
+
 class AuthHandler:
     def __init__(self) -> None:
         self.pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto")
@@ -88,7 +97,12 @@ def hash_client_token(raw: str) -> str:
         return hashlib.sha256(raw.encode()).hexdigest()
 
     def verify_password(self, plain_password, hashed_password):
-        return self.pwd_context.verify(plain_password, hashed_password)
+        try:
+            return self.pwd_context.verify(plain_password, hashed_password)
+        except ValueError:
+            # OIDC-provisioned accounts hold a placeholder, not a bcrypt hash,
+            # and passlib raises on one it cannot identify.
+            return False
 
     def get_password_hash(self, password):
         return self.pwd_context.hash(password)
@@ -252,13 +266,24 @@ def generate_invite_link_token(
             to_encode,
             oct_key,
         )
-        invite_link = f"{ROMM_BASE_URL}/register?token={token}"
+        # The link is already in the response; the token registers an account on
+        # its own, so the log gets only its id.
         log.info(
-            f"Invite link created by {hl(user.username, color=CYAN)}: {hl(invite_link)}"
+            f"Invite link created by {hl(user.username, color=CYAN)} (jti: {hl(jti)})"
         )
         redis_client.setex(f"invite-jti:{jti}", expires_in, "valid")
         return token
 
+    def assert_invite_link_token_valid(self, token: str) -> None:
+        """Raise unless the invite link token is valid, leaving it unspent.
+
+        Args:
+            token (str): The token to check.
+        """
+        jti, _ = self._decode_invite_link_token(token)
+        if redis_client.get(f"invite-jti:{jti}") != b"valid":
+            raise _invite_token_spent()
+
     def consume_invite_link_token(self, token: str) -> str:
         """
         Verify and consume the invite link token, which invalidates the token to prevent reuse.
@@ -269,6 +294,17 @@ def consume_invite_link_token(self, token: str) -> str:
         Returns:
             str: The role associated with the token.
         """
+        jti, role = self._decode_invite_link_token(token)
+
+        # Read and invalidate in one operation, so two registrations racing on
+        # one invite cannot both see it as valid and both create an account.
+        if redis_client.getdel(f"invite-jti:{jti}") != b"valid":
+            raise _invite_token_spent()
+
+        return role
+
+    def _decode_invite_link_token(self, token: str) -> tuple[str, str]:
+        """Decode an invite link token and return its `(jti, role)`."""
         try:
             payload = jwt.decode(token, oct_key, algorithms=[ALGORITHM])
         except (BadSignatureError, DecodeError, ValueError) as exc:
@@ -282,16 +318,10 @@ def consume_invite_link_token(self, token: str) -> str:
 
         jti = payload.claims.get("jti")
         role = payload.claims.get("role", "USER").upper()
-        if not jti or redis_client.get(f"invite-jti:{jti}") != b"valid":
-            raise HTTPException(
-                status_code=status.HTTP_400_BAD_REQUEST,
-                detail="Invite token has already been used or is invalid.",
-            )
+        if not jti:
+            raise _invite_token_spent()
 
-        # Invalidate the token as soon as it's read
-        redis_client.delete(f"invite-jti:{jti}")
-
-        return role
+        return jti, role
 
 
 class OAuthHandler:

backend/handler/auth/middleware/csrf_middleware.py

@@ -15,6 +15,8 @@
 from starlette.responses import PlainTextResponse, Response
 from starlette.types import ASGIApp, Message, Receive, Scope, Send
 
+from handler.auth.constants import SESSION_COOKIE_NAME
+
 
 class CSRFMiddleware:
     def __init__(
@@ -33,6 +35,7 @@ def __init__(
         cookie_httponly: bool = False,
         cookie_samesite: str = "lax",
         header_name: str = "x-csrftoken",
+        session_cookie_name: str = SESSION_COOKIE_NAME,
     ) -> None:
         if safe_methods is None:
             safe_methods = {"GET", "HEAD", "OPTIONS", "TRACE"}
@@ -51,6 +54,7 @@ def __init__(
         self.cookie_httponly = cookie_httponly
         self.cookie_samesite = cookie_samesite
         self.header_name = header_name
+        self.session_cookie_name = session_cookie_name
 
     async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
         # Skip CSRF check if not an HTTP request, like websockets
@@ -60,9 +64,12 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
 
         request = Request(scope, receive)
 
-        # Skip CSRF check if Authorization header is present
+        # HybridAuthBackend resolves the session cookie ahead of this header, so
+        # a request carrying both authenticates as the cookie's owner.
         auth_scheme = request.headers.get("Authorization", "").split(" ", 1)[0].lower()
-        if auth_scheme == "bearer" or auth_scheme == "basic":
+        if auth_scheme in ("bearer", "basic") and not request.cookies.get(
+            self.session_cookie_name
+        ):
             await self.app(scope, receive, send)
             return None
 

backend/handler/auth/middleware/redis_session_middleware.py

@@ -31,10 +31,16 @@ async def clear_user_sessions(user_id: str) -> None:
         Clears all active sessions for a given user.
         """
         session_ids = await async_cache.smembers(f"user_sessions:{user_id}")
-        if session_ids:
-            for session_id in session_ids:
-                await async_cache.delete(f"session:{session_id}")
-            await async_cache.delete(f"user_sessions:{user_id}")
+        if not session_ids:
+            return
+
+        # A member arrives as bytes from a client that does not decode, and its
+        # repr in a key name would miss the session and leave it live.
+        keys = [
+            f"session:{sid.decode() if isinstance(sid, bytes) else sid}"
+            for sid in session_ids
+        ]
+        await async_cache.delete(*keys, f"user_sessions:{user_id}")
 
     async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
         if scope["type"] not in ("http", "websocket"):
@@ -66,19 +72,29 @@ async def send_wrapper(message: Message) -> None:
                 user_id = scope["session"].get("sub")
 
                 if scope["session"]:
-                    session_id = scope["session"].pop("session_id", None) or str(
-                        uuid.uuid4()
-                    )  # Retrieve or create session_id
+                    existing_id = scope["session"].pop("session_id", None)
+                    session_id = existing_id or str(uuid.uuid4())
                     session_data_json = json.dumps(scope["session"])
-                    await async_cache.set(
-                        f"session:{session_id}", session_data_json, ex=self.max_age
+                    # Refreshed only while its record is still there, so a
+                    # session revoked mid-request is not written back.
+                    stored = await async_cache.set(
+                        f"session:{session_id}",
+                        session_data_json,
+                        ex=self.max_age,
+                        xx=existing_id is not None,
                     )
 
-                    # Add session_id to user set of sessions
-                    if user_id:
-                        await async_cache.sadd(f"user_sessions:{user_id}", session_id)
+                    if stored:
+                        # Add session_id to user set of sessions
+                        if user_id:
+                            await async_cache.sadd(
+                                f"user_sessions:{user_id}", session_id
+                            )
+
+                        header_value = f"{self.session_cookie}={session_id}; path=/; Max-Age={self.max_age}; {self.security_flags}"
+                    else:
+                        header_value = f"{self.session_cookie}=null; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT; {self.security_flags}"
 
-                    header_value = f"{self.session_cookie}={session_id}; path=/; Max-Age={self.max_age}; {self.security_flags}"
                     headers.append("Set-Cookie", header_value)
                 elif session_id:
                     await async_cache.delete(f"session:{session_id}")