fix(backup): stop overwrite writing a spool's other tag key (#2656)

Four of the review's smaller items.

    E6, the substantive one. tag_uid and tray_uuid are both in the overwrite setattr
    loop, so a spool matched on one key got 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 .scalars().first() is non-deterministic and an AMS tag lookup
    resolves to an arbitrary one of the two spools. The same loop could also clear a
    tag the user had scanned since the backup was taken, when the backup entry held
    None.

    _find_spool now reports which key matched, and _guard_tag_overwrite drops a tag
    column from the write when the incoming value is empty and the local row has one
    (the backup predates the scan, so the local tag is the newer fact) or when
    another local spool already holds it. Announced in the tally the way the archive
    un-delete case already announces itself, rather than done silently — the
    spoolTagKept locale key landed with the rest of the i18n block last commit.

    E5. The Restore button is hidden without github:restore. All three endpoints are
    gated on it server-side, so the modal 403s on its first preview; offering the
    button is offering an action that cannot work. Button only — the card stays
    visible, since configuring backups is a separate permission — and hasPermission
    returns true with auth off, so a single-user instance is unaffected.

    E3. models/github_backup.py: the trigger comment said manual/scheduled; this PR
    added a third value.

    E4. ha_token_from_env: recommending no change, with the reasoning recorded as a
    test rather than left in a review thread. It is built only in the settings GET
    response, is absent from AppSettingsUpdate, and is therefore never a Settings
    row — it cannot reach a backup, so an allowlist entry would be dead code. Worse,
    a name-shaped exception to a belt-and-braces denylist is a live hole: an
    attacker-authored settings/app_settings.json could get a *token*-named row
    written by choosing that name.

    4 unit tests and 1 frontend test that fail against this commit's parent, plus 4
    controls: a free tag is still written, an unchanged tag is not reported as kept,
    an insert is unaffected, and the button still shows with auth disabled.

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)

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,

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 = {

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(<GitHubBackupSettings />);
+
+    // 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(<GitHubBackupSettings />);
+
+    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(<GitHubBackupSettings />);
+
+    await waitFor(() =>
+      expect(screen.getByRole('button', { name: /Restore from Git/i })).toBeInTheDocument(),
+    );
+  });
+});

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')}
                         </Button>
                         {/* Restore from the backup repo (#2656) */}
-                        <Button
-                          variant="secondary"
-                          size="sm"
-                          onClick={() => setShowGitRestore(true)}
-                          disabled={status.restore_running}
-                        >
-                          <RotateCcw className="w-4 h-4" />
-                          {t('backup.restoreFromGit.button')}
-                        </Button>
+                        {canRestoreFromGit && (
+                          <Button
+                            variant="secondary"
+                            size="sm"
+                            onClick={() => setShowGitRestore(true)}
+                            disabled={status.restore_running}
+                          >
+                            <RotateCcw className="w-4 h-4" />
+                            {t('backup.restoreFromGit.button')}
+                          </Button>
+                        )}
                       </>
                     )}
                   </>