diff --git a/backend/endpoints/user.py b/backend/endpoints/user.py index 246df940c9..4e963e0a74 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,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( diff --git a/backend/handler/auth/base_handler.py b/backend/handler/auth/base_handler.py index 6f8f1cd94d..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") @@ -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: diff --git a/backend/handler/auth/middleware/csrf_middleware.py b/backend/handler/auth/middleware/csrf_middleware.py index 7102675c61..89f8c769a6 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,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 diff --git a/backend/handler/auth/middleware/redis_session_middleware.py b/backend/handler/auth/middleware/redis_session_middleware.py index d575d2de27..5dad199217 100644 --- a/backend/handler/auth/middleware/redis_session_middleware.py +++ b/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}") diff --git a/backend/tests/endpoints/test_identity.py b/backend/tests/endpoints/test_identity.py index 4cf6374c45..c4bc1c27d0 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", @@ -222,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", [ @@ -260,6 +343,49 @@ 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 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"}, + 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/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/tests/handler/auth/test_redis_session_middleware.py b/backend/tests/handler/auth/test_redis_session_middleware.py new file mode 100644 index 0000000000..49e3180134 --- /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.constants import SESSION_COOKIE_NAME +from handler.auth.middleware.redis_session_middleware import RedisSessionMiddleware + +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_NAME, + 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_NAME, "not-a-real-session") + + response = client.post("/login") + + assert response.cookies[SESSION_COOKIE_NAME] != "not-a-real-session" 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( diff --git a/backend/utils/archives.py b/backend/utils/archives.py index a652342978..c34333de9a 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 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] 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..5e4865815b 100755 --- a/docker/init_scripts/init +++ b/docker/init_scripts/init @@ -133,12 +133,14 @@ start_bin_gunicorn() { export PYTHONUNBUFFERED=1 export PYTHONDONTWRITEBYTECODE=1 + # 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 \ --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..33f71827a6 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 # 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