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

@@ -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():

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">
     <div class="flex items-start gap-3">
       <i :class="resultSuccess ? 'fas fa-check-circle text-green-600' : 'fas fa-times-circle text-red-600'" aria-hidden="true"></i>
       <div>

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."""
+        """When ENABLE_FACTORY_RESET=False, accessing the page redirects away."""
-        # Simulate admin session
         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
+        # Redirect to /settings (302) when disabled, or to login (302/307) when unauthenticated
-        assert response.status_code in (302, 307, 200)
+        assert response.status_code in (302, 307)
 
-    def test_view_renders_when_enabled(self, client):
+    def test_view_requires_auth(self, client):
-        """When enabled and admin, renders the reset page."""
+        """Unauthenticated users are redirected away from the page."""
-        # Set session cookie with admin user
         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", follow_redirects=False)
-            response = client.get("/admin/system-reset")
+        # Should redirect to login since there's no active session
-
+        assert response.status_code in (302, 307)
-        # Will get redirect to login if not authenticated, which is expected
-        assert response.status_code in (200, 302, 307)