Sfoglia il codice sorgente

fix(backup): require settings:update to restore the settings category (#2656)

The restore endpoint was gated on `github:restore` alone, and the settings
category rewrites arbitrary non-auth `Settings` rows. Backup and Settings are
separate permission groups, so a role holding only Backup could change
settings it cannot change through `PUT /api/v1/settings/`, the endpoint that
owns them.

The inconsistency is ours rather than an inference: this module already makes
exactly this argument — it is why the four protected auth keys are refused
outright — and `library.py` sets the precedent of elevating a route to
`settings:update` for the same reason.

Gated per-category rather than by demoting `github:restore` wholesale, so it
stays narrow and doesn't presume the answer for `spools:*`, `archives:*` and
`kprofiles`. That broader permission-model question goes to the maintainer in
the PR reply.

`current_user is None` only means auth is disabled — `github:restore` is in
`_APIKEY_DENIED_PERMISSIONS`, so an API key never reaches the route body.

No frontend change: `request()` puts the 403's `detail` on the Error, and the
modal already renders `restoreMutation.error.message` in its red block, so
the user sees the missing permission named.

Tests: 1 regression (a Backup-only role gets 403 and `run_restore` is never
awaited) + 3 controls (the same role still restores the other three
categories; a role holding both permissions still restores settings; auth
disabled is unaffected). The regression confirmed failing against the pre-fix
route. `_create_config` gained an optional token so it works under auth.
jmoore-skild 1 mese fa
parent
commit
8efa9c2945

+ 23 - 1
backend/app/api/routes/github_backup.py

@@ -26,6 +26,7 @@ from backend.app.schemas.github_backup import (
     GitHubRestoreResponse,
     GitHubRestoreResponse,
     GitHubTestConnectionResponse,
     GitHubTestConnectionResponse,
     ProviderType,
     ProviderType,
+    RestoreCategory,
 )
 )
 from backend.app.services.github_backup import github_backup_service
 from backend.app.services.github_backup import github_backup_service
 from backend.app.services.github_restore import github_restore_service
 from backend.app.services.github_restore import github_restore_service
@@ -440,14 +441,35 @@ async def preview_restore(
 async def restore_backup(
 async def restore_backup(
     request: GitHubRestoreRequest,
     request: GitHubRestoreRequest,
     db: AsyncSession = Depends(get_db),
     db: AsyncSession = Depends(get_db),
-    _: User | None = RequirePermissionIfAuthEnabled(Permission.GITHUB_RESTORE),
+    current_user: User | None = RequirePermissionIfAuthEnabled(Permission.GITHUB_RESTORE),
 ):
 ):
     """Restore selected categories from one backup commit.
     """Restore selected categories from one backup commit.
 
 
     Note there is no private-repo gate here, unlike the config endpoints: that
     Note there is no private-repo gate here, unlike the config endpoints: that
     check exists to stop credentials leaving the instance, and this path only
     check exists to stop credentials leaving the instance, and this path only
     reads. A config can only be saved against a private repo anyway.
     reads. A config can only be saved against a private repo anyway.
+
+    The settings category needs ``settings:update`` as well — see the check
+    below.
     """
     """
+    if RestoreCategory.SETTINGS in request.categories and current_user is not None:
+        # The settings category rewrites arbitrary non-auth Settings rows, which
+        # is what PUT /api/v1/settings/ gates on settings:update. Backup and
+        # Settings are separate permission groups, so a role holding only Backup
+        # could otherwise change settings it cannot change through the endpoint
+        # that owns them. This module already makes that argument — it is why
+        # the four protected auth keys are refused outright — so the gap was an
+        # inconsistency in ours, not a new policy.
+        #
+        # current_user is None only when auth is disabled: github:restore is in
+        # _APIKEY_DENIED_PERMISSIONS, so an API key never gets past the
+        # dependency to reach this line.
+        if not current_user.has_all_permissions(Permission.SETTINGS_UPDATE.value):
+            raise HTTPException(
+                status_code=403,
+                detail=f"Missing required permissions: {Permission.SETTINGS_UPDATE.value}",
+            )
+
     result = await db.execute(select(GitHubBackupConfig).limit(1))
     result = await db.execute(select(GitHubBackupConfig).limit(1))
     config = result.scalar_one_or_none()
     config = result.scalar_one_or_none()
 
 

+ 120 - 1
backend/tests/integration/test_github_restore_api.py

@@ -27,9 +27,10 @@ def _mock_private_repo_check():
         yield m
         yield m
 
 
 
 
-async def _create_config(async_client: AsyncClient) -> dict:
+async def _create_config(async_client: AsyncClient, token: str | None = None) -> dict:
     response = await async_client.post(
     response = await async_client.post(
         "/api/v1/github-backup/config",
         "/api/v1/github-backup/config",
+        headers={"Authorization": f"Bearer {token}"} if token else {},
         json={
         json={
             "repository_url": "https://github.com/test/repo",
             "repository_url": "https://github.com/test/repo",
             "access_token": "ghp_testtoken123",
             "access_token": "ghp_testtoken123",
@@ -436,3 +437,121 @@ class TestRestoreDoesNotOpenTheMetricsEndpoint:
         authorised = await async_client.get("/api/v1/metrics", headers={"Authorization": "Bearer local-token"})
         authorised = await async_client.get("/api/v1/metrics", headers={"Authorization": "Bearer local-token"})
         assert authorised.status_code == 200
         assert authorised.status_code == 200
         assert "bambuddy_build_info" in authorised.text
         assert "bambuddy_build_info" in authorised.text
+
+
+class TestSettingsRestoreNeedsSettingsUpdate(TestOwnershipPermissionsSetup):
+    """A Backup-only role must not reach around the gate that owns settings (#2656).
+
+    The settings category rewrites arbitrary non-auth ``Settings`` rows, which is
+    exactly what ``PUT /api/v1/settings/`` gates on ``settings:update``. Backup
+    and Settings are separate permission groups, so gating the restore endpoint
+    on ``github:restore`` alone let a role holding only Backup change settings it
+    could not change through the endpoint that owns them. This module already
+    makes that argument — it is why the four protected auth keys are refused
+    outright — so the gap was an inconsistency in ours.
+    """
+
+    async def _token_for(self, async_client: AsyncClient, admin_token: str, name: str, permissions: list[str]) -> str:
+        headers = {"Authorization": f"Bearer {admin_token}"}
+        group = await async_client.post(
+            "/api/v1/groups/",
+            headers=headers,
+            json={"name": name, "permissions": permissions},
+        )
+        assert group.status_code == 201, group.text
+        created = await async_client.post(
+            "/api/v1/users/",
+            headers=headers,
+            json={"username": name, "password": "Restorepass1!", "group_ids": [group.json()["id"]]},
+        )
+        assert created.status_code in (200, 201), created.text
+        login = await async_client.post(
+            "/api/v1/auth/login",
+            json={"username": name, "password": "Restorepass1!"},
+        )
+        assert login.status_code == 200, login.text
+        return login.json()["access_token"]
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_backup_only_role_cannot_restore_settings(self, async_client: AsyncClient, auth_setup):
+        token = await self._token_for(
+            async_client, auth_setup["admin_token"], "backuponly", ["github:backup", "github:restore"]
+        )
+        await _create_config(async_client, auth_setup["admin_token"])
+
+        with patch(
+            "backend.app.services.github_restore.github_restore_service.run_restore",
+            new=AsyncMock(return_value={"success": True, "message": "", "log_id": 1, "ref": "aaa1111", "results": {}}),
+        ) as mock:
+            response = await async_client.post(
+                "/api/v1/github-backup/restore",
+                headers={"Authorization": f"Bearer {token}"},
+                json={"categories": ["settings"]},
+            )
+
+        assert response.status_code == 403
+        assert "settings:update" in response.json()["detail"]
+        mock.assert_not_awaited(), "the refusal has to happen before anything is written"
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_the_same_role_can_still_restore_the_other_categories(self, async_client: AsyncClient, auth_setup):
+        """Control: the gate is per-category, not a blanket demotion of github:restore."""
+        token = await self._token_for(
+            async_client, auth_setup["admin_token"], "backuponly2", ["github:backup", "github:restore"]
+        )
+        await _create_config(async_client, auth_setup["admin_token"])
+
+        with patch(
+            "backend.app.services.github_restore.github_restore_service.run_restore",
+            new=AsyncMock(return_value={"success": True, "message": "", "log_id": 1, "ref": "aaa1111", "results": {}}),
+        ):
+            response = await async_client.post(
+                "/api/v1/github-backup/restore",
+                headers={"Authorization": f"Bearer {token}"},
+                json={"categories": ["spools", "archives", "kprofiles"]},
+            )
+
+        assert response.status_code == 200
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_a_role_holding_both_can_restore_settings(self, async_client: AsyncClient, auth_setup):
+        """Control: the gate must not lock out a role that legitimately holds both."""
+        token = await self._token_for(
+            async_client,
+            auth_setup["admin_token"],
+            "backupandsettings",
+            ["github:backup", "github:restore", "settings:read", "settings:update"],
+        )
+        await _create_config(async_client, auth_setup["admin_token"])
+
+        with patch(
+            "backend.app.services.github_restore.github_restore_service.run_restore",
+            new=AsyncMock(return_value={"success": True, "message": "", "log_id": 1, "ref": "aaa1111", "results": {}}),
+        ):
+            response = await async_client.post(
+                "/api/v1/github-backup/restore",
+                headers={"Authorization": f"Bearer {token}"},
+                json={"categories": ["settings"]},
+            )
+
+        assert response.status_code == 200
+
+    @pytest.mark.asyncio
+    @pytest.mark.integration
+    async def test_auth_disabled_is_unaffected(self, async_client: AsyncClient):
+        """Control: with auth off there is no user to check, and the dep returns None."""
+        await _create_config(async_client)
+
+        with patch(
+            "backend.app.services.github_restore.github_restore_service.run_restore",
+            new=AsyncMock(return_value={"success": True, "message": "", "log_id": 1, "ref": "aaa1111", "results": {}}),
+        ):
+            response = await async_client.post(
+                "/api/v1/github-backup/restore",
+                json={"categories": ["settings"]},
+            )
+
+        assert response.status_code == 200