From 6a0681a5d34f3a4500e12eb528aa18b7965a1a68 Mon Sep 17 00:00:00 2001 From: Sam Dornan Date: Sat, 19 Sep 2026 12:03:30 +0000 Subject: [PATCH 1/7] fix(security): close a set of small auth and hardening gaps Findings from a security audit of the current tree. Each is independent and small; the larger findings (unauthenticated netplay socket, image-baked provider credentials, decompression bombs) are left for separate changes. - csrf: an Authorization header exempted a request on presence alone, but HybridAuthBackend resolves the session cookie first, so a cookie plus any header value authenticated as the cookie's owner with the check skipped. Exempt only when no session cookie is present. - users: revoke every session of the user a credential change was applied to, not just the caller's, so an admin resetting a compromised account can lock the attacker out. Sessions are keyed by username, so the name is captured before the update renames them. - users: gate POST /{id}/ra/refresh on ownership. It took an arbitrary id under me.write and wrote to that user's row. - users: verify the invite token before the uniqueness checks, which let an anonymous caller enumerate accounts with a token they never had. Split verify from consume so a rejected username does not burn the invite. - auth: fail password verification on a hash passlib cannot identify. OIDC-provisioned accounts hold a placeholder, which raised and turned a 401 into a 500 that identified them. - auth: log the invite token's jti rather than the link. The link goes back to the caller in the response, and the log has a wider audience. - archives: pass "--" before the 7-Zip member argument, matching the bsdtar branch, so an entry named "-x" or "@file" stays a member name. - init: trust X-Forwarded-For only from the bundled nginx. Trusting every hop makes uvicorn read the leftmost entry, which the caller supplies, and the per-IP rate limits read that address. FORWARDED_ALLOW_IPS overrides it for deployments behind a further proxy. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC --- backend/endpoints/user.py | 25 +++++++++-- backend/handler/auth/base_handler.py | 41 +++++++++++++++---- .../auth/middleware/csrf_middleware.py | 15 ++++++- backend/tests/endpoints/test_identity.py | 12 ++++++ backend/tests/handler/auth/test_auth.py | 5 +++ .../handler/auth/test_csrf_middleware.py | 10 +++++ backend/utils/archives.py | 16 ++++++-- docker/init_scripts/init | 6 ++- env.template | 1 + 9 files changed, 115 insertions(+), 16 deletions(-) diff --git a/backend/endpoints/user.py b/backend/endpoints/user.py index 246df940c9..677139d12a 100644 --- a/backend/endpoints/user.py +++ b/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,12 @@ def create_user_from_invite( UserSchema: Newly created user """ + # Checked before anything user-specific: the "already exists" errors below + # would otherwise let an anonymous caller enumerate accounts with a token + # they never had. Spent only once the account is about to be created, so a + # rejected username does not burn the invite. + auth_handler.verify_invite_link_token(token) + try: validate_username(username) validate_password(password) @@ -471,18 +478,26 @@ 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 + # Revoke every session of the user the change was applied to, not just + # the caller's: an admin resetting a compromised account has to be able + # to lock the attacker out of it. 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 +573,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( diff --git a/backend/handler/auth/base_handler.py b/backend/handler/auth/base_handler.py index 6f8f1cd94d..2e9147c843 100644 --- a/backend/handler/auth/base_handler.py +++ b/backend/handler/auth/base_handler.py @@ -88,7 +88,14 @@ 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 rather than a bcrypt + # hash, and passlib raises on one it cannot identify. Failing the + # check keeps that a 401 instead of a 500 that tells an anonymous + # caller which accounts came from the provider. + return False def get_password_hash(self, password): return self.pwd_context.hash(password) @@ -252,13 +259,27 @@ def generate_invite_link_token( to_encode, oct_key, ) - invite_link = f"{ROMM_BASE_URL}/register?token={token}" + # The link itself goes back to the caller in the response, so only the + # id is logged: the token registers an account on its own, and the log + # reaches a wider audience than the admin who asked for it. 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 verify_invite_link_token(self, token: str) -> str: + """Verify an invite link token without spending it. + + Args: + token (str): The token to verify. + + Returns: + str: The role associated with the token. + """ + _, role = self._decode_invite_link_token(token) + return role + 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 +290,15 @@ 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) + + # Invalidate the token as soon as it's read + redis_client.delete(f"invite-jti:{jti}") + + return role + + def _decode_invite_link_token(self, token: str) -> tuple[str, str]: + """Validate an invite link token and return its `(jti, role)`.""" try: payload = jwt.decode(token, oct_key, algorithms=[ALGORITHM]) except (BadSignatureError, DecodeError, ValueError) as exc: @@ -288,10 +318,7 @@ def consume_invite_link_token(self, token: str) -> str: detail="Invite token has already been used or is invalid.", ) - # Invalidate the token as soon as it's read - redis_client.delete(f"invite-jti:{jti}") - - return role + return jti, role class OAuthHandler: diff --git a/backend/handler/auth/middleware/csrf_middleware.py b/backend/handler/auth/middleware/csrf_middleware.py index 7102675c61..9d4fe8eb93 100644 --- a/backend/handler/auth/middleware/csrf_middleware.py +++ b/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,16 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: request = Request(scope, receive) - # Skip CSRF check if Authorization header is present + # An Authorization header carries its own credential, so a caller using + # one cannot be riding a cookie and has nothing to forge. It only + # exempts the request when no session cookie is present though: + # `HybridAuthBackend` resolves the session first, so a cookie plus any + # Authorization value at all would otherwise authenticate as the cookie's + # owner with the check skipped. 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 diff --git a/backend/tests/endpoints/test_identity.py b/backend/tests/endpoints/test_identity.py index 4cf6374c45..f599278afd 100644 --- a/backend/tests/endpoints/test_identity.py +++ b/backend/tests/endpoints/test_identity.py @@ -79,6 +79,18 @@ def test_get_user_avatar( assert response.headers["content-type"].startswith("image/") +def test_refresh_ra_for_another_user_is_forbidden( + client, viewer_access_token: str, admin_user: User +): + # Rejected on ownership before the user is looked up, so it does not depend + # on the target having a RetroAchievements username set. + response = client.post( + f"/api/users/{admin_user.id}/ra/refresh", + headers={"Authorization": f"Bearer {viewer_access_token}"}, + ) + assert response.status_code == status.HTTP_403_FORBIDDEN + + def test_get_user_avatar_none_set(client, access_token: str, admin_user: User): response = client.get( f"/api/users/{admin_user.id}/avatar", diff --git a/backend/tests/handler/auth/test_auth.py b/backend/tests/handler/auth/test_auth.py index 0dd1fe759e..063afba984 100644 --- a/backend/tests/handler/auth/test_auth.py +++ b/backend/tests/handler/auth/test_auth.py @@ -27,6 +27,11 @@ def test_verify_password(): assert not auth_handler.verify_password( "password", auth_handler.get_password_hash("notpassword") ) + # OIDC-provisioned accounts hold a placeholder instead of a bcrypt hash; + # it has to fail the check rather than raise. + assert not auth_handler.verify_password( + "password", "3f2b1c7e-0c1d-4f5a-9a2b-8e7d6c5b4a39" + ) def test_authenticate_user(admin_user: User): diff --git a/backend/tests/handler/auth/test_csrf_middleware.py b/backend/tests/handler/auth/test_csrf_middleware.py index 1d59e12b01..239db08c36 100644 --- a/backend/tests/handler/auth/test_csrf_middleware.py +++ b/backend/tests/handler/auth/test_csrf_middleware.py @@ -203,6 +203,16 @@ def test_bearer_auth_bypass(self) -> None: resp = client.post("/post", headers={"Authorization": "Bearer token"}) assert resp.status_code == 200 + def test_session_cookie_defeats_auth_header_bypass(self) -> None: + """A session cookie is authenticated ahead of the Authorization header, + so its presence must keep the CSRF check in force.""" + app = create_test_app() + client = TestClient(app) + client.cookies.set("romm_session", "session-value") + + resp = client.post("/post", headers={"Authorization": "Bearer anything"}) + assert resp.status_code == 403 + def test_non_http_scope_bypass(self) -> None: """WebSocket (or other non-HTTP) scopes should pass through.""" # Manual ASGI call; TestClient doesn't expose WebSocket easily diff --git a/backend/utils/archives.py b/backend/utils/archives.py index a652342978..421e720172 100644 --- a/backend/utils/archives.py +++ b/backend/utils/archives.py @@ -177,7 +177,16 @@ def _process_largest_7z_member( start_decompression_time = time.monotonic() with subprocess.Popen( - [SEVEN_ZIP_PATH, "e", str(file_path), largest_file, "-so", "-y", "-spd"], + [ + SEVEN_ZIP_PATH, + "e", + str(file_path), + "-so", + "-y", + "-spd", + "--", + largest_file, + ], stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, shell=False, # trunk-ignore(bandit/B603): 7z path is hardcoded, args are validated @@ -557,8 +566,9 @@ def _archive_member_command(file_path: Path, member: str) -> list[str]: ] # "-spd" disables wildcard matching so a member name containing "*" or "?" - # can't select (and concatenate) other members. - return [SEVEN_ZIP_PATH, "e", str(file_path), member, "-so", "-y", "-spd"] + # can't select (and concatenate) other members, and "--" stops switch + # parsing so one starting with "-" or "@" stays a member name. + return [SEVEN_ZIP_PATH, "e", str(file_path), "-so", "-y", "-spd", "--", member] def _list_archive_file_members(file_path: Path) -> list[tuple[str, int]]: diff --git a/docker/init_scripts/init b/docker/init_scripts/init index f0c8f12e11..00b17c0b43 100755 --- a/docker/init_scripts/init +++ b/docker/init_scripts/init @@ -133,12 +133,16 @@ start_bin_gunicorn() { export PYTHONUNBUFFERED=1 export PYTHONDONTWRITEBYTECODE=1 + # Only the bundled nginx (127.0.0.1) may speak for the client: trusting every + # hop makes uvicorn take the leftmost X-Forwarded-For entry, which the caller + # supplies, and every per-IP rate limit reads that address. Deployments behind + # a further reverse proxy set FORWARDED_ALLOW_IPS to its address. local -a wrap=() otel_prefix wrap api "${wrap[@]}" gunicorn \ --bind=0.0.0.0:"${DEV_PORT:-5000}" \ --pid=/tmp/gunicorn.pid \ - --forwarded-allow-ips="*" \ + --forwarded-allow-ips="${FORWARDED_ALLOW_IPS:-127.0.0.1}" \ --worker-class uvicorn_worker.UvicornWorker \ --workers "${WEB_SERVER_CONCURRENCY:-4}" \ --timeout "${WEB_SERVER_TIMEOUT:-300}" \ diff --git a/env.template b/env.template index 2d6e0fa300..7cad63ddd6 100644 --- a/env.template +++ b/env.template @@ -147,6 +147,7 @@ WEB_SERVER_MAX_REQUESTS=1000 # Maximum requests a worker processes before resta WEB_SERVER_MAX_REQUESTS_JITTER=100 # Random jitter added to max requests value WEB_SERVER_WORKER_CONNECTIONS=1000 # Maximum simultaneous clients per worker process WEB_SERVER_GUNICORN_WAIT_SECONDS=30 # Seconds to wait for Gunicorn to start before giving up +FORWARDED_ALLOW_IPS=127.0.0.1 # Hosts whose X-Forwarded-For is trusted; set to your reverse proxy's address when running behind one IPV4_ONLY=false # Bind only to IPv4 # Proxy From 0d653b160d4a749689b6a09a5a10a434dc4a9a67 Mon Sep 17 00:00:00 2001 From: Sam Dornan Date: Sat, 19 Sep 2026 15:58:43 +0000 Subject: [PATCH 2/7] refactor: tighten comments and name the invite-token check Review-polish pass over the previous commit. Comments cut to the one non-obvious fact each: the cross-middleware ordering, passlib's behaviour on an unidentifiable hash, and what the leftmost X-Forwarded-For entry means. The rest was restating the code or defending the choice. verify_invite_link_token renamed to assert_invite_link_token_valid. The adjacent verify_password_reset_token verifies *and* consumes, so two verify_* methods on one class would have read as the same thing while differing on whether the token survives; assert_* is the idiom already used for raising checks (assert_rom_visible, assert_session_owner). It returned a role nothing read, so it returns None. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC --- backend/endpoints/user.py | 13 ++++------- backend/handler/auth/base_handler.py | 23 +++++++------------ .../auth/middleware/csrf_middleware.py | 8 ++----- backend/utils/archives.py | 4 ++-- docker/init_scripts/init | 6 ++--- 5 files changed, 19 insertions(+), 35 deletions(-) diff --git a/backend/endpoints/user.py b/backend/endpoints/user.py index 677139d12a..4e963e0a74 100644 --- a/backend/endpoints/user.py +++ b/backend/endpoints/user.py @@ -202,11 +202,9 @@ def create_user_from_invite( UserSchema: Newly created user """ - # Checked before anything user-specific: the "already exists" errors below - # would otherwise let an anonymous caller enumerate accounts with a token - # they never had. Spent only once the account is about to be created, so a - # rejected username does not burn the invite. - auth_handler.verify_invite_link_token(token) + # 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) @@ -488,9 +486,8 @@ async def update_user( if "role" in cleaned_data: await emit_permissions_changed(id) - # Revoke every session of the user the change was applied to, not just - # the caller's: an admin resetting a compromised account has to be able - # to lock the attacker out of it. + # 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" ) diff --git a/backend/handler/auth/base_handler.py b/backend/handler/auth/base_handler.py index 2e9147c843..d44de85727 100644 --- a/backend/handler/auth/base_handler.py +++ b/backend/handler/auth/base_handler.py @@ -91,10 +91,8 @@ def verify_password(self, plain_password, hashed_password): try: return self.pwd_context.verify(plain_password, hashed_password) except ValueError: - # OIDC-provisioned accounts hold a placeholder rather than a bcrypt - # hash, and passlib raises on one it cannot identify. Failing the - # check keeps that a 401 instead of a 500 that tells an anonymous - # caller which accounts came from the provider. + # 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): @@ -259,26 +257,21 @@ def generate_invite_link_token( to_encode, oct_key, ) - # The link itself goes back to the caller in the response, so only the - # id is logged: the token registers an account on its own, and the log - # reaches a wider audience than the admin who asked for it. + # 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)} (jti: {hl(jti)})" ) redis_client.setex(f"invite-jti:{jti}", expires_in, "valid") return token - def verify_invite_link_token(self, token: str) -> str: - """Verify an invite link token without spending it. + 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 verify. - - Returns: - str: The role associated with the token. + token (str): The token to check. """ - _, role = self._decode_invite_link_token(token) - return role + self._decode_invite_link_token(token) def consume_invite_link_token(self, token: str) -> str: """ diff --git a/backend/handler/auth/middleware/csrf_middleware.py b/backend/handler/auth/middleware/csrf_middleware.py index 9d4fe8eb93..89f8c769a6 100644 --- a/backend/handler/auth/middleware/csrf_middleware.py +++ b/backend/handler/auth/middleware/csrf_middleware.py @@ -64,12 +64,8 @@ async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None: request = Request(scope, receive) - # An Authorization header carries its own credential, so a caller using - # one cannot be riding a cookie and has nothing to forge. It only - # exempts the request when no session cookie is present though: - # `HybridAuthBackend` resolves the session first, so a cookie plus any - # Authorization value at all would otherwise authenticate as the cookie's - # owner with the check skipped. + # 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 in ("bearer", "basic") and not request.cookies.get( self.session_cookie_name diff --git a/backend/utils/archives.py b/backend/utils/archives.py index 421e720172..c34333de9a 100644 --- a/backend/utils/archives.py +++ b/backend/utils/archives.py @@ -566,8 +566,8 @@ def _archive_member_command(file_path: Path, member: str) -> list[str]: ] # "-spd" disables wildcard matching so a member name containing "*" or "?" - # can't select (and concatenate) other members, and "--" stops switch - # parsing so one starting with "-" or "@" stays a member name. + # can't select other members; "--" stops switch parsing so one starting + # with "-" or "@" stays a member name. return [SEVEN_ZIP_PATH, "e", str(file_path), "-so", "-y", "-spd", "--", member] diff --git a/docker/init_scripts/init b/docker/init_scripts/init index 00b17c0b43..5e4865815b 100755 --- a/docker/init_scripts/init +++ b/docker/init_scripts/init @@ -133,10 +133,8 @@ start_bin_gunicorn() { export PYTHONUNBUFFERED=1 export PYTHONDONTWRITEBYTECODE=1 - # Only the bundled nginx (127.0.0.1) may speak for the client: trusting every - # hop makes uvicorn take the leftmost X-Forwarded-For entry, which the caller - # supplies, and every per-IP rate limit reads that address. Deployments behind - # a further reverse proxy set FORWARDED_ALLOW_IPS to its address. + # Trusting every hop makes uvicorn read the leftmost X-Forwarded-For entry, + # which the caller supplies; only the bundled nginx may speak for the client. local -a wrap=() otel_prefix wrap api "${wrap[@]}" gunicorn \ From 69426515c7419846e7e083e2fc25bf05e890d8e1 Mon Sep 17 00:00:00 2001 From: Sam Dornan Date: Sat, 19 Sep 2026 16:34:30 +0000 Subject: [PATCH 3/7] fix(security): stop a revoked session being written back, correct proxy guidance Both from Greptile's review of this PR. A session loaded before `clear_user_sessions` ran was re-persisted when its request responded: the middleware writes any non-empty session back unconditionally, so an attacker holding one in flight restored it and the credential change locked nobody out. An existing record is now refreshed only while it is still in Redis, and a request whose session went away mid-flight has its cookie expired instead. A new session still writes unconditionally, so login is unaffected. The FORWARDED_ALLOW_IPS note told deployments behind a further reverse proxy to set the variable to that proxy's address. Gunicorn's immediate peer is always the bundled nginx on 127.0.0.1, so replacing it makes uvicorn distrust nginx and ignore the forwarded chain entirely, leaving every request on the loopback address and in one rate-limit bucket. The upstream proxy is appended, not substituted. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC --- .../middleware/redis_session_middleware.py | 30 +++++--- .../auth/test_redis_session_middleware.py | 70 +++++++++++++++++++ env.template | 2 +- 3 files changed, 92 insertions(+), 10 deletions(-) create mode 100644 backend/tests/handler/auth/test_redis_session_middleware.py diff --git a/backend/handler/auth/middleware/redis_session_middleware.py b/backend/handler/auth/middleware/redis_session_middleware.py index d575d2de27..9aa4a2e668 100644 --- a/backend/handler/auth/middleware/redis_session_middleware.py +++ b/backend/handler/auth/middleware/redis_session_middleware.py @@ -66,19 +66,31 @@ 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 + # One already in Redis is refreshed only while its record is + # still there. A credential change revokes sessions through + # `clear_user_sessions`, and a request already in flight when + # that happens would otherwise write its copy back and undo it. + 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}") diff --git a/backend/tests/handler/auth/test_redis_session_middleware.py b/backend/tests/handler/auth/test_redis_session_middleware.py new file mode 100644 index 0000000000..f314b4e06d --- /dev/null +++ b/backend/tests/handler/auth/test_redis_session_middleware.py @@ -0,0 +1,70 @@ +"""Test suite for RedisSessionMiddleware's Redis-backed session persistence.""" + +from starlette.applications import Starlette +from starlette.middleware import Middleware +from starlette.requests import Request +from starlette.responses import JSONResponse +from starlette.routing import Route +from starlette.testclient import TestClient + +from handler.auth.middleware.redis_session_middleware import RedisSessionMiddleware + +SESSION_COOKIE = "romm_session" +USERNAME = "user_1" + + +def create_test_app() -> Starlette: + async def login(request: Request) -> JSONResponse: + request.session["iss"] = "romm:auth" + request.session["sub"] = USERNAME + return JSONResponse({"ok": True}) + + async def revoke(request: Request) -> JSONResponse: + """Revoke the caller's sessions while their own request is in flight.""" + await RedisSessionMiddleware.clear_user_sessions(USERNAME) + return JSONResponse({"ok": True}) + + async def whoami(request: Request) -> JSONResponse: + return JSONResponse({"sub": request.session.get("sub")}) + + return Starlette( + routes=[ + Route("/login", login, methods=["POST"]), + Route("/revoke", revoke, methods=["POST"]), + Route("/whoami", whoami, methods=["GET"]), + ], + middleware=[ + Middleware( + RedisSessionMiddleware, + session_cookie=SESSION_COOKIE, + same_site="strict", + https_only=False, + ) + ], + ) + + +class TestRedisSessionMiddleware: + def test_session_survives_across_requests(self) -> None: + client = TestClient(create_test_app()) + + assert client.post("/login").status_code == 200 + assert client.get("/whoami").json()["sub"] == USERNAME + + def test_revoked_session_is_not_restored_by_an_in_flight_request(self) -> None: + """A request holding a copy of a session revoked mid-flight must not + write it back, or a credential change could never lock anyone out.""" + client = TestClient(create_test_app()) + client.post("/login") + + client.post("/revoke") + + assert client.get("/whoami").json()["sub"] is None + + def test_a_cookie_for_an_unknown_session_gets_a_fresh_id(self) -> None: + client = TestClient(create_test_app()) + client.cookies.set(SESSION_COOKIE, "not-a-real-session") + + response = client.post("/login") + + assert response.cookies[SESSION_COOKIE] != "not-a-real-session" diff --git a/env.template b/env.template index 7cad63ddd6..33f71827a6 100644 --- a/env.template +++ b/env.template @@ -147,7 +147,7 @@ WEB_SERVER_MAX_REQUESTS=1000 # Maximum requests a worker processes before resta WEB_SERVER_MAX_REQUESTS_JITTER=100 # Random jitter added to max requests value WEB_SERVER_WORKER_CONNECTIONS=1000 # Maximum simultaneous clients per worker process WEB_SERVER_GUNICORN_WAIT_SECONDS=30 # Seconds to wait for Gunicorn to start before giving up -FORWARDED_ALLOW_IPS=127.0.0.1 # Hosts whose X-Forwarded-For is trusted; set to your reverse proxy's address when running behind one +FORWARDED_ALLOW_IPS=127.0.0.1 # Peers whose X-Forwarded-For is trusted; behind a further reverse proxy, append its address rather than replacing the bundled nginx's IPV4_ONLY=false # Bind only to IPv4 # Proxy From e993b2fe11b0e8b5704188d6d21d41185402ed18 Mon Sep 17 00:00:00 2001 From: Sam Dornan Date: Sat, 19 Sep 2026 16:37:57 +0000 Subject: [PATCH 4/7] fix(security): consume invites atomically, and cover the new boundaries From Copilot's review of this PR. `consume_invite_link_token` read the jti and deleted it as two operations, so two registrations racing on one invite could both see it valid and both create an account. It now reads and invalidates with a single GETDEL, matching how refresh tokens are already spent in this file. The non-consuming check keeps a plain read, and both raise the same response so neither distinguishes a spent invite from an unusable one. Tests for the boundaries the review noted were uncovered: - An admin resetting another user's password ends that user's sessions while leaving the admin's own usable. The existing test only changed the caller's own password, which the previous implementation already handled. - Registration rejects a bad token without disclosing whether an account exists, and a registration rejected on a duplicate username leaves the invite usable, then spent once it registers one. - The 7-Zip and bsdtar member commands put "--" before the member, for members shaped like "-x", "@listfile" and "-so". Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC --- backend/handler/auth/base_handler.py | 28 ++++-- backend/tests/endpoints/test_identity.py | 111 +++++++++++++++++++++++ backend/tests/utils/test_archives.py | 13 +++ 3 files changed, 143 insertions(+), 9 deletions(-) diff --git a/backend/handler/auth/base_handler.py b/backend/handler/auth/base_handler.py index d44de85727..d112f39ab4 100644 --- a/backend/handler/auth/base_handler.py +++ b/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") @@ -271,7 +280,9 @@ def assert_invite_link_token_valid(self, token: str) -> None: Args: token (str): The token to check. """ - self._decode_invite_link_token(token) + 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: """ @@ -285,13 +296,15 @@ def consume_invite_link_token(self, token: str) -> str: """ jti, role = self._decode_invite_link_token(token) - # Invalidate the token as soon as it's read - redis_client.delete(f"invite-jti:{jti}") + # 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]: - """Validate an invite link token and return its `(jti, role)`.""" + """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: @@ -305,11 +318,8 @@ def _decode_invite_link_token(self, token: str) -> tuple[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() return jti, role diff --git a/backend/tests/endpoints/test_identity.py b/backend/tests/endpoints/test_identity.py index f599278afd..7aa2236e1e 100644 --- a/backend/tests/endpoints/test_identity.py +++ b/backend/tests/endpoints/test_identity.py @@ -234,6 +234,77 @@ def test_update_user_accepts_png_avatar( assert response.json()["avatar_path"].endswith("avatar.png") +def _invite_token(client, access_token: str) -> str: + response = client.post( + "/api/users/invite-link", + params={"role": Role.USER.value}, + headers={"Authorization": f"Bearer {access_token}"}, + ) + assert response.status_code == HTTPStatus.CREATED + return response.json()["token"] + + +def test_register_with_a_bad_token_does_not_disclose_existing_accounts( + client, access_token: str, editor_user: User +): + """The token is checked first, so the duplicate-account errors below it + cannot be used to enumerate accounts without a valid invite.""" + response = client.post( + "/api/users/register", + json={ + "username": editor_user.username, + "email": "someone@example.com", + "password": "a-good-password", + "token": "not-a-real-token", + }, + ) + + assert response.status_code == HTTPStatus.BAD_REQUEST + assert editor_user.username not in response.json()["detail"] + + +def test_a_rejected_registration_leaves_the_invite_usable( + client, access_token: str, editor_user: User +): + token = _invite_token(client, access_token) + + # Rejected on the duplicate username, after the token was checked. + response = client.post( + "/api/users/register", + json={ + "username": editor_user.username, + "email": "someone@example.com", + "password": "a-good-password", + "token": token, + }, + ) + assert response.status_code == HTTPStatus.BAD_REQUEST + + # The invite was verified, not spent, so it still registers an account. + response = client.post( + "/api/users/register", + json={ + "username": "test_invitee", + "email": "invitee@example.com", + "password": "a-good-password", + "token": token, + }, + ) + assert response.status_code == HTTPStatus.CREATED + + # And now it is spent. + response = client.post( + "/api/users/register", + json={ + "username": "test_invitee_2", + "email": "invitee2@example.com", + "password": "a-good-password", + "token": token, + }, + ) + assert response.status_code == HTTPStatus.BAD_REQUEST + + @pytest.mark.parametrize( "base_url, expected_url", [ @@ -272,6 +343,46 @@ def test_delete_user(client, access_token: str, editor_user: User): assert response.status_code == HTTPStatus.OK +@pytest.mark.asyncio +async def test_admin_password_reset_invalidates_the_target_user_sessions( + client, access_token: str, editor_user: User +): + """The reason the revocation is not scoped to the caller: an admin resetting + a compromised account has to end that account's sessions, not their own.""" + basic_auth = base64.b64encode( + f"{editor_user.username}:test_editor_password".encode("ascii") + ).decode("ascii") + response = client.post( + "/api/login", headers={"Authorization": f"Basic {basic_auth}"} + ) + assert response.status_code == HTTPStatus.OK + target_session = response.cookies.get("romm_session") + assert target_session is not None + + target_cookie = {"Cookie": f"romm_session={target_session}"} + assert client.get("/api/users/me", headers=target_cookie).status_code == ( + HTTPStatus.OK + ) + + # The admin resets the other user's password over a bearer token, so the + # caller is never the target. + response = client.put( + f"/api/users/{editor_user.id}", + data={"password": "reset_by_admin_password"}, + headers={"Authorization": f"Bearer {access_token}"}, + ) + assert response.status_code == HTTPStatus.OK + + response = client.get("/api/users/me", headers=target_cookie) + assert response.status_code in (HTTPStatus.UNAUTHORIZED, HTTPStatus.FORBIDDEN) + + # The admin's own credentials still work. + response = client.get( + "/api/users", headers={"Authorization": f"Bearer {access_token}"} + ) + assert response.status_code == HTTPStatus.OK + + @pytest.mark.asyncio async def test_password_change_invalidates_sessions(client, admin_user: User): # Get the user's session cookie diff --git a/backend/tests/utils/test_archives.py b/backend/tests/utils/test_archives.py index a7051b3fbc..a7abc8a338 100644 --- a/backend/tests/utils/test_archives.py +++ b/backend/tests/utils/test_archives.py @@ -416,6 +416,19 @@ def test_extraction_command_is_chosen_by_extension(self): Path("/fake/game.7z"), "a.gba" ) assert seven_zip_command[0] == archives.SEVEN_ZIP_PATH + assert seven_zip_command[-2:] == ["--", "a.gba"] + + def test_extraction_command_terminates_switches_before_the_member(self): + """Member names come from the archive's own listing, so one shaped like + a switch has to reach 7-Zip as a name.""" + for member in ("-x", "@listfile", "-so"): + command = archives._archive_member_command(Path("/fake/game.7z"), member) + assert command[-2:] == ["--", member] + + rar_command = archives._archive_member_command( + Path("/fake/GAME.RAR"), member + ) + assert rar_command[-2:] == ["--", member] def test_read_rar_archive_files_streams_members_in_ascii_order(self): listing = MagicMock( From b902d9f5e5c9418ceade7a881b1a030708513503 Mon Sep 17 00:00:00 2001 From: Sam Dornan Date: Sat, 19 Sep 2026 16:43:11 +0000 Subject: [PATCH 5/7] fix: delete the right key when clearing a user's sessions The new middleware test failed in CI, and the cause was in clear_user_sessions rather than the test: `smembers` answers with bytes on a client that does not decode, and the member went straight into an f-string key name, so the delete targeted `session:b''` and the real session stayed live. The write-back guard then found the key still there and refreshed it, which is exactly what the test asserts must not happen. Production is unaffected: `async_cache` is built with `decode_responses=True` there, so members arrive as strings. Only the pytest client, a bare `FakeAsyncRedis`, returns bytes, which is why this surfaced the moment a test exercised the helper end to end. Members are now decoded when they arrive as bytes, so the helper is correct under either client, and the keys go in one delete call. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC --- .../auth/middleware/redis_session_middleware.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/backend/handler/auth/middleware/redis_session_middleware.py b/backend/handler/auth/middleware/redis_session_middleware.py index 9aa4a2e668..039a7e56b2 100644 --- a/backend/handler/auth/middleware/redis_session_middleware.py +++ b/backend/handler/auth/middleware/redis_session_middleware.py @@ -31,10 +31,17 @@ 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 comes back as bytes from a client that does not decode, and + # it goes straight into a key name, where its repr 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"): From a6d582c7c0fc06cc0dd4c7cc2b7223cc732c8ba5 Mon Sep 17 00:00:00 2001 From: Sam Dornan Date: Sat, 19 Sep 2026 16:44:45 +0000 Subject: [PATCH 6/7] style: trim two comments and reuse the session cookie constant From Greptile's review. The write-back and key-decoding comments had grown past the two lines the repo allows and were narrating the failure they prevent; they now state the invariant only. The middleware test imported SESSION_COOKIE_NAME rather than repeating its value, which would have drifted if the cookie were ever renamed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC --- .../auth/middleware/redis_session_middleware.py | 11 ++++------- .../handler/auth/test_redis_session_middleware.py | 8 ++++---- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/backend/handler/auth/middleware/redis_session_middleware.py b/backend/handler/auth/middleware/redis_session_middleware.py index 039a7e56b2..5dad199217 100644 --- a/backend/handler/auth/middleware/redis_session_middleware.py +++ b/backend/handler/auth/middleware/redis_session_middleware.py @@ -34,9 +34,8 @@ async def clear_user_sessions(user_id: str) -> None: if not session_ids: return - # A member comes back as bytes from a client that does not decode, and - # it goes straight into a key name, where its repr would miss the - # session and leave it live. + # 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 @@ -76,10 +75,8 @@ async def send_wrapper(message: Message) -> None: existing_id = scope["session"].pop("session_id", None) session_id = existing_id or str(uuid.uuid4()) session_data_json = json.dumps(scope["session"]) - # One already in Redis is refreshed only while its record is - # still there. A credential change revokes sessions through - # `clear_user_sessions`, and a request already in flight when - # that happens would otherwise write its copy back and undo it. + # 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, diff --git a/backend/tests/handler/auth/test_redis_session_middleware.py b/backend/tests/handler/auth/test_redis_session_middleware.py index f314b4e06d..49e3180134 100644 --- a/backend/tests/handler/auth/test_redis_session_middleware.py +++ b/backend/tests/handler/auth/test_redis_session_middleware.py @@ -7,9 +7,9 @@ from starlette.routing import Route from starlette.testclient import TestClient +from handler.auth.constants import SESSION_COOKIE_NAME from handler.auth.middleware.redis_session_middleware import RedisSessionMiddleware -SESSION_COOKIE = "romm_session" USERNAME = "user_1" @@ -36,7 +36,7 @@ async def whoami(request: Request) -> JSONResponse: middleware=[ Middleware( RedisSessionMiddleware, - session_cookie=SESSION_COOKIE, + session_cookie=SESSION_COOKIE_NAME, same_site="strict", https_only=False, ) @@ -63,8 +63,8 @@ def test_revoked_session_is_not_restored_by_an_in_flight_request(self) -> None: def test_a_cookie_for_an_unknown_session_gets_a_fresh_id(self) -> None: client = TestClient(create_test_app()) - client.cookies.set(SESSION_COOKIE, "not-a-real-session") + client.cookies.set(SESSION_COOKIE_NAME, "not-a-real-session") response = client.post("/login") - assert response.cookies[SESSION_COOKIE] != "not-a-real-session" + assert response.cookies[SESSION_COOKIE_NAME] != "not-a-real-session" From d15d9e74506797bf1cd7a487850c0bdd2f7a6f50 Mon Sep 17 00:00:00 2001 From: Sam Dornan Date: Sat, 19 Sep 2026 16:55:53 +0000 Subject: [PATCH 7/7] test: send only the bearer on the admin reset The client kept the target's session cookie from its Basic login, and HybridAuthBackend resolves a cookie ahead of the Authorization header, so the reset authenticated as the target and took the self-update path. The test passed without ever exercising an admin resetting another account. The jar is cleared before the reset, leaving the bearer as the only credential. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC --- backend/tests/endpoints/test_identity.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/backend/tests/endpoints/test_identity.py b/backend/tests/endpoints/test_identity.py index 7aa2236e1e..c4bc1c27d0 100644 --- a/backend/tests/endpoints/test_identity.py +++ b/backend/tests/endpoints/test_identity.py @@ -364,8 +364,11 @@ async def test_admin_password_reset_invalidates_the_target_user_sessions( HTTPStatus.OK ) - # The admin resets the other user's password over a bearer token, so the - # caller is never the target. + # The bearer has to be the only credential on the reset: HybridAuthBackend + # resolves a session cookie ahead of it, and the jar still holds the + # target's, which would make this a self-update. + client.cookies.clear() + response = client.put( f"/api/users/{editor_user.id}", data={"password": "reset_by_admin_password"},