Security hardening (maziggy/bambuddy-security #18)

backend/app/services/github_backup.py

@@ -21,6 +21,7 @@ from backend.app.models.spool import Spool
 from backend.app.models.spool_usage_history import SpoolUsageHistory
 from backend.app.models.user import User
 from backend.app.services.git_providers.factory import get_provider_backend
+from backend.app.services.github_restore import is_restorable_setting_key
 from backend.app.services.printer_manager import printer_manager
 from backend.app.services.secret_fields import SECRET_PLACEHOLDER, is_secret_setting_key, mask_preset
 
@@ -750,11 +751,14 @@ class GitHubBackupService:
         # unset, as it does for the restore), so the restore can still tell
         # whether a switch had one. A URL with a login in it is
         # left out: masked, it would be restored over a working one.
+        # Anything else a restore would not write is left out: keys earlier
+        # versions left behind (some hold a credential under a name no rule
+        # recognises), runtime state and Bambuddy's own flags.
         settings_data = {}
         for s in settings:
             if is_secret_setting_key(s.key):
                 settings_data[s.key] = SECRET_PLACEHOLDER if (s.value or "").strip() else s.value
-            elif not URL_CREDENTIALS_PATTERN.search(s.value or ""):
+            elif is_restorable_setting_key(s.key) and not URL_CREDENTIALS_PATTERN.search(s.value or ""):
                 settings_data[s.key] = s.value
 
         files["settings/app_settings.json"] = {

backend/app/services/github_restore.py

@@ -259,6 +259,17 @@ _MANYFOLD_CLIENT_ID_KEY = "manyfold_client_id"
 _READS_A_SAVED_NULL = frozenset({"ams_temp_alarm", "default_printer_id", "open_in_slicer"})
 
 
+def is_restorable_setting_key(key: str) -> bool:
+    """Whether a restore writes this settings key at all (with a value its
+    editor accepts). The Git backup holds only these, so nothing a restore
+    would refuse as unknown leaves the instance."""
+    return (
+        key in AppSettingsUpdate.model_fields
+        or key in _EXTRA_RESTORABLE_KEYS
+        or key in (_MANYFOLD_URL_KEY, _MANYFOLD_CLIENT_ID_KEY)
+    )
+
+
 def _stored_form(value: object) -> str:
     """A checked value as its editor stores it."""
     if isinstance(value, bool):

backend/tests/unit/test_github_backup_settings_secrets.py

@@ -68,7 +68,7 @@ async def test_a_backed_up_secret_is_refused_on_restore(db_session):
 
 @pytest.mark.asyncio
 async def test_the_backup_hides_exactly_what_the_restore_refuses(db_session):
-    rows = {**SECRETS, "currency": "EUR", "mqtt_enabled": "true", "theme_api": "x", "ldap_server_url": "ldap://h"}
+    rows = {**SECRETS, "currency": "EUR", "mqtt_enabled": "true", "ldap_server_url": "ldap://h"}
     collected = await _collect(db_session, rows)
 
     for key in rows:
@@ -104,3 +104,34 @@ async def test_a_switch_whose_credential_stayed_behind_is_still_left_off_on_rest
 
     assert plan.companion == ("mqtt_enabled",)
     assert "mqtt_password" in plan.blocked
+
+
+LEGACY = {
+    "virtual_printer_tailscale_auth_key": "value-legacy-one",
+    "vapid_private_key": "value-legacy-two",
+    "dark_theme": "true",
+    "_backfill_1425_pipeline_permissions_done": "true",
+    "debug_logging_enabled": "true",
+}
+
+
+@pytest.mark.asyncio
+async def test_a_key_a_restore_does_not_write_is_not_backed_up(db_session):
+    """Keys earlier versions left behind, runtime state and Bambuddy's own
+    flags: a restore refuses them, so the backup leaves them out, secrets
+    named in no recognisable way among them."""
+    collected = await _collect(db_session, {**LEGACY, "currency": "EUR", "smtp_host": "mail.local"})
+
+    assert not set(LEGACY) & set(collected)
+    assert "value-legacy" not in json.dumps(collected)
+    assert collected["currency"] == "EUR" and collected["smtp_host"] == "mail.local"
+
+
+@pytest.mark.asyncio
+async def test_everything_backed_up_is_something_a_restore_knows(db_session):
+    rows = {**SECRETS, **LEGACY, "currency": "EUR", "ldap_server_url": "ldap://h", "manyfold_client_id": "id"}
+    collected = await _collect(db_session, rows)
+
+    plan = await GitHubRestoreService()._plan_settings(db_session, collected)
+
+    assert plan.unknown == () and plan.internal == ()

backend/tests/unit/test_github_restore_validation.py

@@ -275,3 +275,14 @@ def test_every_address_key_is_one_a_restore_would_otherwise_write():
     from backend.app.services.github_restore import _checked_setting
 
     assert set(_ADDRESS_KEYS) == {key for key in _ADDRESS_KEYS if _checked_setting(key, "")[0] is not None}
+
+
+def test_the_backup_and_the_restore_agree_on_which_keys_are_settings():
+    """The backup keeps exactly the keys a restore writes; the two must not drift."""
+    from backend.app.schemas.settings import AppSettingsUpdate
+    from backend.app.services.github_restore import _EXTRA_RESTORABLE_KEYS, _checked_setting, is_restorable_setting_key
+
+    keys = {*AppSettingsUpdate.model_fields, *_EXTRA_RESTORABLE_KEYS, "manyfold_url", "manyfold_client_id"}
+    keys |= {"vapid_private_key", "dark_theme", "_backfill_x_done", "user_1_email_2fa_enabled", "auth_enabled"}
+    for key in keys:
+        assert is_restorable_setting_key(key) is (_checked_setting(key, "")[0] is not None), key