From 7dffdc05542cbfe37a91f86b661790038195fef5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 16 Mar 2026 22:38:24 +0000 Subject: [PATCH] fix(system-reset): address code review feedback - Add path traversal guard in reimport file copy loop - Improve error log message context for table wipe failures - Use conditional role=alert/status on result banner for accessibility - Make test assertions more specific (exact status codes) - Rename ambiguous view test Co-authored-by: christianlouis <361235+christianlouis@users.noreply.github.com> --- app/utils/system_reset.py | 8 +++++++- frontend/templates/system_reset.html | 2 +- tests/test_system_reset.py | 30 ++++++++++++++-------------- 3 files changed, 23 insertions(+), 17 deletions(-) diff --git a/app/utils/system_reset.py b/app/utils/system_reset.py index 508931ee..dfeaa5b5 100644 --- a/app/utils/system_reset.py +++ b/app/utils/system_reset.py @@ -168,7 +168,7 @@ def _wipe_database(db: Session) -> dict[str, int]: result[table_name] = count logger.info("Wiped %d rows from %s", count, table_name) except Exception: - logger.exception("Failed to wipe table %s", table_name) + logger.exception("Failed to wipe table %s during system reset", table_name) db.rollback() raise @@ -220,6 +220,12 @@ def perform_reset_and_reimport(db: Session) -> dict: if original_dir.is_dir(): for entry in original_dir.iterdir(): if entry.is_file(): + # Validate the resolved path stays within original_dir (path traversal guard) + try: + entry.resolve().relative_to(original_dir.resolve()) + except ValueError: + logger.warning("Skipping file outside original dir: %s", entry) + continue dest = reimport_dir / entry.name # Avoid overwriting: append counter if name clash if dest.exists(): diff --git a/frontend/templates/system_reset.html b/frontend/templates/system_reset.html index bbe0dfcb..452a8142 100644 --- a/frontend/templates/system_reset.html +++ b/frontend/templates/system_reset.html @@ -165,7 +165,7 @@ class="mt-8 rounded-lg p-4" :class="resultSuccess ? 'bg-green-50 dark:bg-green-900/20 border border-green-300 dark:border-green-700' : 'bg-red-50 dark:bg-red-900/20 border border-red-300 dark:border-red-700'" - role="status" aria-live="polite"> + :role="resultSuccess ? 'status' : 'alert'" aria-live="polite">
diff --git a/tests/test_system_reset.py b/tests/test_system_reset.py index 68f60150..570ebd19 100644 --- a/tests/test_system_reset.py +++ b/tests/test_system_reset.py @@ -257,14 +257,19 @@ class TestSystemResetApi: def test_full_reset_requires_feature_flag(self, client): """Returns 404 when ENABLE_FACTORY_RESET is false.""" - with patch("app.api.system_reset._require_admin", return_value={"is_admin": True}): + from app.api.system_reset import _require_admin + + client.app.dependency_overrides[_require_admin] = lambda: {"is_admin": True} + try: with patch("app.api.system_reset.settings") as mock_s: mock_s.enable_factory_reset = False response = client.post( "/api/admin/system-reset/full", json={"confirmation": "DELETE"}, ) - assert response.status_code in (403, 404) + finally: + client.app.dependency_overrides.pop(_require_admin, None) + assert response.status_code == 404 def test_full_reset_requires_confirmation(self, client): """Wrong confirmation string gets 400.""" @@ -369,25 +374,20 @@ class TestSystemResetView: """Tests for the /admin/system-reset view.""" def test_view_redirects_when_disabled(self, client): - """When ENABLE_FACTORY_RESET=False, redirects to /settings.""" - # Simulate admin session + """When ENABLE_FACTORY_RESET=False, accessing the page redirects away.""" with client: client.cookies.set("session", "test") with patch("app.views.system_reset.settings") as mock_s: mock_s.enable_factory_reset = False - # Session admin mock response = client.get("/admin/system-reset", follow_redirects=False) - # Should redirect (302) or require login - assert response.status_code in (302, 307, 200) + # Redirect to /settings (302) when disabled, or to login (302/307) when unauthenticated + assert response.status_code in (302, 307) - def test_view_renders_when_enabled(self, client): - """When enabled and admin, renders the reset page.""" - # Set session cookie with admin user + def test_view_requires_auth(self, client): + """Unauthenticated users are redirected away from the page.""" with patch("app.views.system_reset.settings") as mock_s: mock_s.enable_factory_reset = True mock_s.factory_reset_on_startup = False - # Force session - response = client.get("/admin/system-reset") - - # Will get redirect to login if not authenticated, which is expected - assert response.status_code in (200, 302, 307) + response = client.get("/admin/system-reset", follow_redirects=False) + # Should redirect to login since there's no active session + assert response.status_code in (302, 307)