From 5313b0dccc80e83ba51e0c3ea71bb7e24fd804d4 Mon Sep 17 00:00:00 2001 From: Roland Date: Tue, 2 Jun 2026 13:37:51 -0600 Subject: [PATCH] Controller can unmount foreign mounts; protect the primary drive. The /api/disks/unmount endpoint only removed entries from fstab.yaml and asked the reconciler to unmount, but the reconciler deliberately never unmounts paths it has not claimed. Foreign mounts (udisks2 desktop automounts, manual mounts) therefore could not be unmounted from the UI, failing with a confusing "check journalctl" error. - Fall back to a direct umount(8) when a reconcile leaves the requested mount point live. - Reject unmounting the primary WROLPi drive (/media/wrolpi) with a clear 400, normalized to catch path traversal. - Require the normalized mount point to be strictly under /media/ before any direct umount, since the fallback runs as root. - Show "System" instead of an Unmount button for /media/wrolpi in both the React UI and the Controller's simplified UI. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/src/components/admin/ControllerPage.js | 2 +- controller/api/disks.py | 50 +++++++++++++++++---- controller/templates/index.html | 2 +- controller/test/test_disks_api.py | 51 +++++++++++++++++++++- 4 files changed, 93 insertions(+), 12 deletions(-) diff --git a/app/src/components/admin/ControllerPage.js b/app/src/components/admin/ControllerPage.js index b68711b0..dc7d60b2 100644 --- a/app/src/components/admin/ControllerPage.js +++ b/app/src/components/admin/ControllerPage.js @@ -633,7 +633,7 @@ function SmartDetailsModal({drive, open, onClose}) { // Protected mount points that should not be unmounted -const PROTECTED_MOUNTS = ['/', '/boot', '/boot/firmware']; +const PROTECTED_MOUNTS = ['/', '/boot', '/boot/firmware', '/media/wrolpi']; // Helper to format disk size const formatSize = (size) => { diff --git a/controller/api/disks.py b/controller/api/disks.py index b8c28162..90292c4e 100644 --- a/controller/api/disks.py +++ b/controller/api/disks.py @@ -2,14 +2,21 @@ Disk management API endpoints for WROLPi Controller. The Controller defines what should be mounted (writes fstab.yaml on the -WROLPi drive) but never calls mount(8) or umount(8) itself. After every -write to fstab.yaml the Controller triggers wrolpi-mounts.service via +WROLPi drive) but never calls mount(8) itself. After every write to +fstab.yaml the Controller triggers wrolpi-mounts.service via ``systemctl restart``; the service reconciles live state to match. The Controller verifies the outcome by reading /proc/mounts through the same MountExecutor abstraction the service uses. + +One exception: the reconciler deliberately never unmounts paths it has +not claimed, so foreign mounts (udisks2 desktop automounts, manual +mounts) under /media/ would otherwise be impossible to remove from the +UI. When a reconcile leaves the requested mount point live, /unmount +falls back to a direct umount(8) for non-reserved /media/ paths. """ import logging +import os import subprocess from typing import Optional @@ -33,6 +40,7 @@ from controller.lib.mount_executor import ( MountExecutor, SubprocessMountExecutor, ) +from controller.lib.reconciler import RESERVED_MOUNT_POINTS from controller.lib.smart import ( get_all_smart_status, is_smart_available, @@ -222,7 +230,12 @@ async def disk_mount(request: MountRequest): @router.post("/unmount") async def disk_unmount(request: UnmountRequest): - """Remove a mount from fstab.yaml and reconcile.""" + """Remove a mount from fstab.yaml and reconcile. + + Falls back to a direct umount when the mount survives the reconcile — + a foreign mount (udisks2 automount, manual mount) the reconciler never + claimed, or a managed mount the reconciler could not release. + """ _check_native_mode() try: @@ -235,6 +248,22 @@ async def disk_unmount(request: UnmountRequest): except ValueError as e: raise HTTPException(status_code=400, detail=str(e)) + # The direct-umount fallback below runs as root, so be strict about + # what we will pass to umount(8): a normalized path strictly under + # /media/ that is not the primary drive. + normalized = os.path.normpath(request.mount_point) + if normalized in RESERVED_MOUNT_POINTS: + raise HTTPException( + status_code=400, + detail=f"Cannot unmount {request.mount_point}: it is the primary " + f"WROLPi drive.", + ) + if not normalized.startswith("/media/"): + raise HTTPException( + status_code=400, + detail=f"Refusing to unmount {normalized}: not under /media/.", + ) + data = load_fstab() removed = data.remove_by_mount_point(request.mount_point) save_fstab(data) @@ -244,12 +273,15 @@ async def disk_unmount(request: UnmountRequest): raise HTTPException(status_code=500, detail=f"Failed to reconcile mounts: {err}") - if request.mount_point in _executor.current_mount_points(): - raise HTTPException( - status_code=500, - detail=f"Reconciler ran but {request.mount_point} is still mounted. " - f"Check journalctl -u wrolpi-mounts.service.", - ) + if normalized in _executor.current_mount_points(): + # The reconciler did not release it — either a foreign mount it + # never claimed, or the umount failed. Try directly. + result = _executor.unmount(normalized) + if not result.ok: + raise HTTPException( + status_code=500, + detail=f"{normalized} is still mounted: {result.error}", + ) return { "success": True, diff --git a/controller/templates/index.html b/controller/templates/index.html index 53073664..aa7a5807 100644 --- a/controller/templates/index.html +++ b/controller/templates/index.html @@ -1604,7 +1604,7 @@ `; - const protectedMounts = ['/', '/boot', '/boot/firmware']; + const protectedMounts = ['/', '/boot', '/boot/firmware', '/media/wrolpi']; const isProtected = protectedMounts.includes(disk.mountpoint); // Check if this disk has a persistent entry (by mount point or UUID) diff --git a/controller/test/test_disks_api.py b/controller/test/test_disks_api.py index 1ba09ccb..ebb53bdf 100644 --- a/controller/test/test_disks_api.py +++ b/controller/test/test_disks_api.py @@ -219,7 +219,8 @@ class TestUnmountEndpoint: assert fstab_yaml.load(disks_env["fstab_path"]).mounts == [] def test_unmount_returns_500_when_still_mounted(self, test_client, disks_env): - # Reconciler refuses the unmount — surface the failure. + # Reconciler refuses the unmount (e.g. busy) — the direct fallback + # fails the same way; surface the failure. disks_env["fake"]._live.add("/media/wrolpi/usb") disks_env["fake"].fail_for_unmount = frozenset({"/media/wrolpi/usb"}) (disks_env["fstab_path"].parent / "managed").write_text( @@ -232,6 +233,54 @@ class TestUnmountEndpoint: assert response.status_code == 500 assert "still mounted" in response.json()["detail"] + def test_unmount_foreign_mount_falls_back_to_direct_umount( + self, test_client, disks_env, + ): + # A udisks2 desktop automount: live, but never in fstab.yaml and + # never claimed by the reconciler. The reconciler will not touch + # it, so the endpoint must unmount it directly. + foreign = "/media/wrolpi/31a8ecf1-c00b-4691-b26f-b0966073659f" + disks_env["fake"]._live.add(foreign) + + response = test_client.post( + "/api/disks/unmount", + json={"mount_point": foreign}, + ) + assert response.status_code == 200 + body = response.json() + assert body["success"] is True + assert body["removed_from_fstab"] is False + assert disks_env["fake"].current_mount_points() == set() + + @pytest.mark.parametrize("mount_point", [ + "/media", # not strictly under /media/ + "/media_evil", # prefix-collides with /media + "/media/wrolpi/../wrolpi", # normalizes to the reserved primary + ]) + def test_unmount_rejects_paths_outside_media( + self, test_client, disks_env, mount_point, + ): + response = test_client.post( + "/api/disks/unmount", + json={"mount_point": mount_point}, + ) + assert response.status_code == 400 + assert disks_env["fake"].unmount_calls == [] + + def test_unmount_reserved_mount_point_rejected(self, test_client, disks_env): + # The primary WROLPi drive must never be unmountable from the API. + disks_env["fake"]._live.add("/media/wrolpi") + + response = test_client.post( + "/api/disks/unmount", + json={"mount_point": "/media/wrolpi"}, + ) + assert response.status_code == 400 + assert "primary" in response.json()["detail"].lower() + # No umount was attempted, directly or via the reconciler. + assert disks_env["fake"].unmount_calls == [] + assert disks_env["fake"].current_mount_points() == {"/media/wrolpi"} + # --- /api/disks/fstab read endpoint ---------------------------------------