diff --git a/backend/app/models/github_backup.py b/backend/app/models/github_backup.py index b6b2f21b5..26d5d32c9 100644 --- a/backend/app/models/github_backup.py +++ b/backend/app/models/github_backup.py @@ -59,7 +59,7 @@ class GitHubBackupLog(Base): started_at: Mapped[datetime] = mapped_column(DateTime, server_default=func.now()) completed_at: Mapped[datetime | None] = mapped_column(DateTime, nullable=True) status: Mapped[str] = mapped_column(String(20)) # running/success/failed/skipped - trigger: Mapped[str] = mapped_column(String(20)) # manual/scheduled + trigger: Mapped[str] = mapped_column(String(20)) # manual/scheduled/restore commit_sha: Mapped[str | None] = mapped_column(String(40), nullable=True) files_changed: Mapped[int] = mapped_column(Integer, default=0) diff --git a/backend/app/services/github_restore.py b/backend/app/services/github_restore.py index e067d63d0..f49450e28 100644 --- a/backend/app/services/github_restore.py +++ b/backend/app/services/github_restore.py @@ -990,6 +990,7 @@ class GitHubRestoreService: return spool_id_map: dict[int, int] = {} + tags_kept = 0 for entry in spools: if not isinstance(entry, dict): @@ -997,7 +998,7 @@ class GitHubRestoreService: continue old_id = entry.get("id") if isinstance(entry.get("id"), int) else None - existing = await self._find_spool(db, entry) + existing, matched_on = await self._find_spool(db, entry) fields = { "material": entry.get("material") or "PLA", @@ -1028,6 +1029,7 @@ class GitHubRestoreService: if not overwrite: tally.skipped += 1 continue + tags_kept += await self._guard_tag_overwrite(db, existing, fields, matched_on) for key, value in fields.items(): setattr(existing, key, value) tally.restored += 1 @@ -1047,32 +1049,45 @@ class GitHubRestoreService: spool_id_map[old_id] = row.id tally.restored += 1 + if tags_kept: + tally.note( + "spoolTagKept", + f"{tags_kept} spool tag(s) left as they are — the backup would have cleared a tag that " + "has since been scanned, or moved one onto a second spool.", + count=tags_kept, + ) + await self._restore_spool_usage(db, usage_payload, tally, spool_id_map, archive_id_map) - async def _find_spool(self, db: AsyncSession, entry: dict) -> Spool | None: - """Match a backed-up spool to a local row. + async def _find_spool(self, db: AsyncSession, entry: dict) -> tuple[Spool | None, str | None]: + """Match a backed-up spool to a local row, and say which key matched. Physical identity first (an RFID/Bambu tag is the spool), then a descriptive composite including ``created_at`` so two otherwise identical spools added at different times stay distinct. + + The second element names the column that matched — ``"tag_uid"``, + ``"tray_uuid"`` or ``None`` for the composite. ``_guard_tag_overwrite`` + needs it: the matched column holds the incoming value by definition, so + it is the *other* one that overwrite can corrupt. """ tag_uid = entry.get("tag_uid") if tag_uid: result = await db.execute(select(Spool).where(Spool.tag_uid == tag_uid)) row = result.scalars().first() if row is not None: - return row + return row, "tag_uid" tray_uuid = entry.get("tray_uuid") if tray_uuid: result = await db.execute(select(Spool).where(Spool.tray_uuid == tray_uuid)) row = result.scalars().first() if row is not None: - return row + return row, "tray_uuid" created_at = _parse_dt(entry.get("created_at")) if created_at is None: - return None + return None, None result = await db.execute( select(Spool).where( Spool.created_at == created_at, @@ -1082,7 +1097,56 @@ class GitHubRestoreService: Spool.color_name == entry.get("color_name"), ) ) - return result.scalars().first() + return result.scalars().first(), None + + @staticmethod + async def _guard_tag_overwrite(db: AsyncSession, existing: Spool, fields: dict, matched_on: str | None) -> int: + """Remove tag columns from ``fields`` that an overwrite would corrupt. + + ``tag_uid`` and ``tray_uuid`` are both in ``fields`` and overwrite is a + blanket ``setattr`` loop, so a spool matched on one key gets the backup's + *other* key written onto it. Neither column has a unique constraint + (``models/spool.py``, and no unique index in the migrations), so nothing + errors — a duplicate tag simply appears, after which ``_find_spool``'s + ``.first()`` is non-deterministic and an AMS tag lookup resolves to an + arbitrary one of the two spools. The same loop can also *clear* a tag the + user has scanned since the backup was taken, when the backup entry holds + ``None``. + + Two refusals, and the row is otherwise overwritten as normal: + + * the incoming value is empty and the local row has one — the backup + predates the scan, so the local tag is the newer fact; + * the incoming value is already held by a different local spool — writing + it would create the duplicate described above. + + Returns how many columns were left alone, so the caller can say so in the + tally rather than doing it silently. + """ + kept = 0 + for column in ("tag_uid", "tray_uuid"): + # The column we matched on already holds the incoming value. + if column == matched_on: + continue + + incoming = fields.get(column) + current = getattr(existing, column) + if incoming == current: + continue + + if not incoming: + if current: + fields.pop(column) + kept += 1 + continue + + clash = await db.execute( + select(Spool.id).where(getattr(Spool, column) == incoming, Spool.id != existing.id) + ) + if clash.scalars().first() is not None: + fields.pop(column) + kept += 1 + return kept async def _restore_spool_usage( self, diff --git a/backend/tests/unit/test_github_restore.py b/backend/tests/unit/test_github_restore.py index 60675b1e9..81bc342f2 100644 --- a/backend/tests/unit/test_github_restore.py +++ b/backend/tests/unit/test_github_restore.py @@ -105,6 +105,23 @@ class TestSettingKeyBlocklist: def test_protected_set_is_only_the_auth_policy_keys(self, key): assert _is_protected_setting_key(key) is False + def test_ha_token_from_env_is_deliberately_not_carved_out(self): + """Recorded so the review's question about it is not re-litigated. + + ``ha_token_from_env`` looks like a false positive for the ``token`` hint, + but it is only ever constructed in the settings GET response + (``get_homeassistant_settings``). It is absent from ``AppSettingsUpdate`` + and so is never a ``Settings`` row — it cannot reach a backup, which + makes an allowlist entry for it dead code. + + Carving it out would also be a live hole rather than a tidy-up: an + attacker-authored ``settings/app_settings.json`` could then get a + ``*token*``-named row written simply by choosing that name. This + blocklist's whole job is belt-and-braces, so a name-shaped exception to + it is exactly the wrong shape of fix. + """ + assert _is_blocked_setting_key("ha_token_from_env") is True + class TestCategoryTally: def test_a_note_carries_code_params_and_english(self): @@ -484,6 +501,117 @@ class TestCompanionCredentials: assert plan == _SettingsPlan() +class TestSpoolTagOverwrite: + """Overwrite must not write the backup's *other* tag key onto a matched spool. + + ``tag_uid`` and ``tray_uuid`` are both in the overwrite ``setattr`` loop, and + neither column has a unique constraint, so writing one onto a spool matched + by the other silently creates a duplicate tag rather than erroring. After + that ``_find_spool``'s ``.first()`` is non-deterministic and an AMS tag + lookup resolves to an arbitrary one of the two. The same loop can also clear + a tag the user has scanned since the backup was taken. + """ + + def _entry(self, **overrides): + entry = { + "id": 41, + "material": "PLA", + "brand": "Bambu Lab", + "created_at": "2026-01-05 12:00:00", + "tag_uid": "TAG-A", + "tray_uuid": None, + } + entry.update(overrides) + return entry + + async def _restore(self, db, entry, tally=None): + tally = tally or _CategoryTally() + await _service()._restore_spools(db, {"spools": [entry]}, None, True, tally, {}) + await db.commit() + return tally + + @pytest.mark.asyncio + async def test_an_empty_incoming_tag_does_not_clear_a_scanned_one(self, db_session): + """The backup predates the scan, so the local tag is the newer fact.""" + db_session.add(Spool(material="PLA", brand="Bambu Lab", tag_uid="TAG-A", tray_uuid="TRAY-LIVE")) + await db_session.commit() + + tally = await self._restore(db_session, self._entry(tray_uuid=None)) + + row = (await db_session.execute(select(Spool))).scalar_one() + assert row.tray_uuid == "TRAY-LIVE" + assert any(note["code"] == "spoolTagKept" for note in tally.notes) + + @pytest.mark.asyncio + async def test_a_tag_another_spool_already_holds_is_not_written(self, db_session): + db_session.add(Spool(material="PLA", brand="Bambu Lab", tag_uid="TAG-A")) + db_session.add(Spool(material="PETG", brand="Other", tray_uuid="TRAY-B")) + await db_session.commit() + + tally = await self._restore(db_session, self._entry(tray_uuid="TRAY-B")) + + holders = (await db_session.execute(select(Spool).where(Spool.tray_uuid == "TRAY-B"))).scalars().all() + assert len(holders) == 1, "a duplicate tray_uuid makes AMS lookups non-deterministic" + assert holders[0].material == "PETG" + assert any(note["code"] == "spoolTagKept" for note in tally.notes) + + @pytest.mark.asyncio + async def test_the_note_counts_every_column_it_kept(self, db_session): + db_session.add(Spool(material="PLA", brand="Bambu Lab", tag_uid="TAG-A", tray_uuid="TRAY-LIVE")) + db_session.add(Spool(material="PETG", brand="Other", tag_uid="TAG-CLASH")) + await db_session.commit() + + # Matched on tray_uuid, so the guard judges tag_uid: it clashes. + tally = await self._restore(db_session, self._entry(tag_uid="TAG-CLASH", tray_uuid="TRAY-LIVE")) + + row = (await db_session.execute(select(Spool).where(Spool.tray_uuid == "TRAY-LIVE"))).scalar_one() + assert row.tag_uid == "TAG-A" + note = next(n for n in tally.notes if n["code"] == "spoolTagKept") + assert note["params"] == {"count": 1} + + # --- Controls ---------------------------------------------------------- + + @pytest.mark.asyncio + async def test_a_free_tag_is_still_written(self, db_session): + """The point of overwrite: a spool that gained a tray_uuid gets it.""" + db_session.add(Spool(material="PLA", brand="Bambu Lab", tag_uid="TAG-A")) + await db_session.commit() + + tally = await self._restore(db_session, self._entry(tray_uuid="TRAY-NEW")) + + row = (await db_session.execute(select(Spool))).scalar_one() + assert row.tray_uuid == "TRAY-NEW" + assert not any(note["code"] == "spoolTagKept" for note in tally.notes) + + @pytest.mark.asyncio + async def test_an_unchanged_tag_is_not_reported_as_kept(self, db_session): + db_session.add(Spool(material="PLA", brand="Bambu Lab", tag_uid="TAG-A", tray_uuid="TRAY-A")) + await db_session.commit() + + tally = await self._restore(db_session, self._entry(tray_uuid="TRAY-A")) + + assert not any(note["code"] == "spoolTagKept" for note in tally.notes) + + @pytest.mark.asyncio + async def test_a_new_spool_keeps_both_tags_from_the_backup(self, db_session): + """The guard is an overwrite-only concern; an insert is unaffected.""" + await self._restore(db_session, self._entry(tag_uid="TAG-NEW", tray_uuid="TRAY-NEW")) + + row = (await db_session.execute(select(Spool))).scalar_one() + assert (row.tag_uid, row.tray_uuid) == ("TAG-NEW", "TRAY-NEW") + + @pytest.mark.asyncio + async def test_find_spool_reports_which_key_matched(self, db_session): + db_session.add(Spool(material="PLA", tag_uid="TAG-A")) + db_session.add(Spool(material="PETG", tray_uuid="TRAY-B")) + await db_session.commit() + service = _service() + + assert (await service._find_spool(db_session, {"tag_uid": "TAG-A"}))[1] == "tag_uid" + assert (await service._find_spool(db_session, {"tray_uuid": "TRAY-B"}))[1] == "tray_uuid" + assert await service._find_spool(db_session, {"tag_uid": "NOPE"}) == (None, None) + + class TestRestoreSpools: def _spool_entry(self, **overrides): entry = { diff --git a/frontend/src/__tests__/components/GitHubBackupSettingsPermissions.test.tsx b/frontend/src/__tests__/components/GitHubBackupSettingsPermissions.test.tsx new file mode 100644 index 000000000..37bfb4671 --- /dev/null +++ b/frontend/src/__tests__/components/GitHubBackupSettingsPermissions.test.tsx @@ -0,0 +1,115 @@ +/** + * The Git Restore button must respect github:restore client-side (#2656). + * + * All three restore endpoints are gated on GITHUB_RESTORE server-side, so a + * user without it gets a 403 the moment the modal opens its preview. Offering + * the button anyway is an action that cannot work. + * + * Scoped to the button on purpose: the backup card itself stays visible, + * because configuring backups is a separate permission. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { screen, waitFor } from '@testing-library/react'; +import { http, HttpResponse } from 'msw'; +import { render } from '../utils'; +import { server } from '../mocks/server'; +import { GitHubBackupSettings } from '../../components/GitHubBackupSettings'; +import { setAuthToken } from '../../api/client'; + +afterEach(() => { + server.resetHandlers(); + setAuthToken(null); +}); + +/** A configured backup, which is what makes the action row render at all. */ +function mockConfiguredBackup() { + server.use( + http.get('*/api/v1/github-backup/config', () => + HttpResponse.json({ + id: 1, + provider: 'github', + repository_url: 'https://github.com/test/repo', + branch: 'main', + enabled: true, + schedule_enabled: false, + schedule_type: 'daily', + schedule_time: '02:00', + backup_kprofiles: true, + backup_cloud_profiles: false, + backup_spools: true, + backup_archives: true, + backup_settings: true, + last_backup_at: null, + last_backup_status: null, + }), + ), + http.get('*/api/v1/github-backup/status', () => + HttpResponse.json({ + configured: true, + enabled: true, + is_running: false, + restore_running: false, + progress: null, + last_backup_at: null, + last_backup_status: null, + next_run: null, + }), + ), + http.get('*/api/v1/github-backup/logs', () => HttpResponse.json([])), + ); +} + +function mockUserWith(permissions: string[]) { + setAuthToken('test-token', 'session'); + server.use( + http.get('*/api/v1/auth/status', () => + HttpResponse.json({ auth_enabled: true, requires_setup: false }), + ), + http.get('*/api/v1/auth/me', () => + HttpResponse.json({ id: 1, username: 'operator', is_admin: false, permissions }), + ), + ); +} + +describe('GitHubBackupSettings - github:restore gate', () => { + it('hides the Restore from Git button without the permission', async () => { + mockConfiguredBackup(); + mockUserWith(['settings:read', 'settings:update']); + + render(); + + // Wait for the action row itself, so an absent button is a real absence + // rather than the card simply not having rendered yet. + await waitFor(() => expect(screen.getByRole('button', { name: /Backup Now/i })).toBeInTheDocument()); + expect(screen.queryByRole('button', { name: /Restore from Git/i })).not.toBeInTheDocument(); + }); + + it('shows it when the user has github:restore', async () => { + mockConfiguredBackup(); + mockUserWith(['settings:read', 'github:restore']); + + render(); + + await waitFor(() => + expect(screen.getByRole('button', { name: /Restore from Git/i })).toBeInTheDocument(), + ); + }); + + it('shows it when auth is disabled entirely', async () => { + // hasPermission returns true with auth off, and it must stay that way - + // a single-user instance has no permissions to grant. + mockConfiguredBackup(); + server.use( + http.get('*/api/v1/auth/status', () => + HttpResponse.json({ auth_enabled: false, requires_setup: false }), + ), + ); + + render(); + + await waitFor(() => + expect(screen.getByRole('button', { name: /Restore from Git/i })).toBeInTheDocument(), + ); + }); +}); diff --git a/frontend/src/components/GitHubBackupSettings.tsx b/frontend/src/components/GitHubBackupSettings.tsx index cf49c91b4..c003ceb10 100644 --- a/frontend/src/components/GitHubBackupSettings.tsx +++ b/frontend/src/components/GitHubBackupSettings.tsx @@ -36,6 +36,7 @@ import type { CloudAuthStatus, Printer, } from '../api/client'; +import { useAuth } from '../contexts/AuthContext'; import { Card, CardContent, CardHeader } from './Card'; import { Button } from './Button'; import { Toggle } from './Toggle'; @@ -134,6 +135,14 @@ export function GitHubBackupSettings() { const queryClient = useQueryClient(); const { showToast } = useToast(); const { t } = useTranslation(); + const { hasPermission } = useAuth(); + + // All three restore endpoints are gated on GITHUB_RESTORE server-side, so a + // user without it gets a 403 the moment the modal opens its preview. Hide the + // button rather than offer an action that cannot work. Deliberately scoped to + // the button: the card itself stays visible, since backup configuration is a + // separate permission. hasPermission returns true when auth is off. + const canRestoreFromGit = hasPermission('github:restore'); // Local state for form const [repoUrl, setRepoUrl] = useState(''); @@ -956,15 +965,17 @@ export function GitHubBackupSettings() { {t('backup.test')} {/* Restore from the backup repo (#2656) */} - + {canRestoreFromGit && ( + + )} )}