From 0e2a78e0656dcbc2dd5687de962adab53400b60e Mon Sep 17 00:00:00 2001 From: mwiegand Date: Thu, 14 May 2026 22:24:19 +0200 Subject: [PATCH] secure(l4d2web): block non-admin writes on system overlays; last-admin guard on deactivate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _load_files_overlay docs already promised "owner or admin" for mutations, but the check only filtered by overlay.type — system overlays (user_id IS NULL) were writable by any logged-in user. Add the explicit 403 for non-admins; read-only routes remain open across all overlay types. Mirror the delete-route last-admin guard on /admin/users//deactivate so a future auth-model change (service accounts bypassing require_admin, etc.) can't accidentally lock out the system. --- l4d2web/routes/files_routes.py | 7 ++- l4d2web/routes/page_routes.py | 13 +++++ l4d2web/tests/test_admin_users.py | 15 +++++ l4d2web/tests/test_overlay_files_routes.py | 64 ++++++++++++++++++++++ 4 files changed, 97 insertions(+), 2 deletions(-) diff --git a/l4d2web/routes/files_routes.py b/l4d2web/routes/files_routes.py index e97145a..75291cc 100644 --- a/l4d2web/routes/files_routes.py +++ b/l4d2web/routes/files_routes.py @@ -88,13 +88,16 @@ def _load_overlay_for_user(overlay_id: int, user) -> Overlay | Response: def _load_files_overlay(overlay_id: int, user) -> Overlay | Response: """Like `_load_overlay_for_user`, but additionally 404s for any overlay - whose type isn't `files`. Mutating endpoints use this; read-only ones - keep working across all types.""" + whose type isn't `files`, and 403s when a non-admin tries to mutate a + system overlay (`user_id IS NULL`). Read-only endpoints keep working + across all overlay types for everyone.""" result = _load_overlay_for_user(overlay_id, user) if isinstance(result, Response): return result if result.type != "files": return Response(status=404) + if result.user_id is None and not user.admin: + return Response(status=403) return result diff --git a/l4d2web/routes/page_routes.py b/l4d2web/routes/page_routes.py index bddf70d..32fa1e2 100644 --- a/l4d2web/routes/page_routes.py +++ b/l4d2web/routes/page_routes.py @@ -69,6 +69,19 @@ def admin_users_deactivate(user_id: int) -> Response: target = db.scalar(select(User).where(User.id == user_id)) if target is None: return Response(status=404) + # Defense-in-depth: mirror the last-admin guard from delete. Through + # normal flow this is unreachable (self-deactivate refused above), but + # it survives future auth-model changes like service accounts. + if target.admin and target.active: + other_active_admins = db.scalar( + select(func.count(User.id)).where( + User.admin.is_(True), + User.active.is_(True), + User.id != user_id, + ) + ) or 0 + if other_active_admins == 0: + return Response("cannot deactivate the last active admin", status=409) target.active = False return redirect("/admin/users") diff --git a/l4d2web/tests/test_admin_users.py b/l4d2web/tests/test_admin_users.py index 823c64d..813e34a 100644 --- a/l4d2web/tests/test_admin_users.py +++ b/l4d2web/tests/test_admin_users.py @@ -78,6 +78,21 @@ def test_deactivate_flips_active_false(admin_client): assert _user_active(target) is False +def test_deactivate_other_admin_succeeds_when_more_than_one_admin(admin_client): + """Defense-in-depth mirror of the delete-admin test. The fixture creates + two admins; admin can deactivate admin2 because at least one admin (the + actor) remains.""" + client, _ = admin_client + with session_scope() as db: + admin2 = db.scalar(select(User).where(User.username == "admin2")) + admin2_id = admin2.id + + response = _post(client, f"/admin/users/{admin2_id}/deactivate") + + assert response.status_code == 302 + assert _user_active(admin2_id) is False + + def test_activate_flips_active_true(admin_client): client, _ = admin_client target = _add_user("bob", active=False) diff --git a/l4d2web/tests/test_overlay_files_routes.py b/l4d2web/tests/test_overlay_files_routes.py index b7d50be..b17c967 100644 --- a/l4d2web/tests/test_overlay_files_routes.py +++ b/l4d2web/tests/test_overlay_files_routes.py @@ -64,6 +64,70 @@ def _make_overlay(left4me_root: Path, *, user_id: int | None, name: str) -> int: return overlay_id +def _make_files_overlay(left4me_root: Path, *, user_id: int | None, name: str) -> int: + with session_scope() as s: + overlay = Overlay(name=name, path="", type="files", user_id=user_id) + s.add(overlay) + s.flush() + overlay.path = str(overlay.id) + overlay_id = overlay.id + (left4me_root / "overlays" / str(overlay_id)).mkdir(parents=True) + return overlay_id + + +def test_non_admin_cannot_mkdir_in_system_overlay(app, left4me_root: Path) -> None: + """System overlays (user_id IS NULL) are readable by everyone but only + writable by admins. A regular user attempting to mutate gets 403.""" + user_id = _make_user(username="bob", admin=False) + overlay_id = _make_files_overlay(left4me_root, user_id=None, name="system") + + client = _client_for(app, user_id) + response = client.post( + f"/overlays/{overlay_id}/files/mkdir", + json={"path": "newdir"}, + headers={"X-CSRF-Token": "test-token"}, + ) + assert response.status_code == 403 + + +def test_non_admin_can_still_read_system_overlay(app, left4me_root: Path) -> None: + user_id = _make_user(username="bob", admin=False) + overlay_id = _make_files_overlay(left4me_root, user_id=None, name="system") + (left4me_root / "overlays" / str(overlay_id) / "config.cfg").write_text("ok") + + client = _client_for(app, user_id) + response = client.get(f"/overlays/{overlay_id}/files") + assert response.status_code == 200 + assert b"config.cfg" in response.data + + +def test_admin_can_mkdir_in_system_overlay(app, left4me_root: Path) -> None: + admin_id = _make_user(username="admin", admin=True) + overlay_id = _make_files_overlay(left4me_root, user_id=None, name="system") + + client = _client_for(app, admin_id) + response = client.post( + f"/overlays/{overlay_id}/files/mkdir", + json={"path": "newdir"}, + headers={"X-CSRF-Token": "test-token"}, + ) + assert response.status_code in (200, 201, 204) + assert (left4me_root / "overlays" / str(overlay_id) / "newdir").is_dir() + + +def test_owner_can_mkdir_in_own_overlay(app, left4me_root: Path) -> None: + user_id = _make_user(username="bob", admin=False) + overlay_id = _make_files_overlay(left4me_root, user_id=user_id, name="mine") + + client = _client_for(app, user_id) + response = client.post( + f"/overlays/{overlay_id}/files/mkdir", + json={"path": "newdir"}, + headers={"X-CSRF-Token": "test-token"}, + ) + assert response.status_code in (200, 201, 204) + + def test_files_fragment_lists_root_directory(app, left4me_root: Path) -> None: user_id = _make_user() overlay_id = _make_overlay(left4me_root, user_id=user_id, name="my")