From 8efa9c2945e685084f2bcbdc8e8d46fc90ccb32b Mon Sep 17 00:00:00 2001 From: jmoore-skild Date: Fri, 31 Jul 2026 16:37:18 -0400 Subject: [PATCH] fix(backup): require settings:update to restore the settings category (#2656) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/app/api/routes/github_backup.py | 24 +++- .../integration/test_github_restore_api.py | 121 +++++++++++++++++- 2 files changed, 143 insertions(+), 2 deletions(-) diff --git a/backend/app/api/routes/github_backup.py b/backend/app/api/routes/github_backup.py index 9ed7aa4e7..631f47345 100644 --- a/backend/app/api/routes/github_backup.py +++ b/backend/app/api/routes/github_backup.py @@ -26,6 +26,7 @@ from backend.app.schemas.github_backup import ( GitHubRestoreResponse, GitHubTestConnectionResponse, ProviderType, + RestoreCategory, ) from backend.app.services.github_backup import github_backup_service from backend.app.services.github_restore import github_restore_service @@ -440,14 +441,35 @@ async def preview_restore( async def restore_backup( request: GitHubRestoreRequest, 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. 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 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)) config = result.scalar_one_or_none() diff --git a/backend/tests/integration/test_github_restore_api.py b/backend/tests/integration/test_github_restore_api.py index 673b3edc9..657afa6e8 100644 --- a/backend/tests/integration/test_github_restore_api.py +++ b/backend/tests/integration/test_github_restore_api.py @@ -27,9 +27,10 @@ def _mock_private_repo_check(): 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( "/api/v1/github-backup/config", + headers={"Authorization": f"Bearer {token}"} if token else {}, json={ "repository_url": "https://github.com/test/repo", "access_token": "ghp_testtoken123", @@ -436,3 +437,121 @@ class TestRestoreDoesNotOpenTheMetricsEndpoint: authorised = await async_client.get("/api/v1/metrics", headers={"Authorization": "Bearer local-token"}) assert authorised.status_code == 200 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