mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-10-03 12:46:49 +02:00
fix(backup): never restore the auth policy settings from a backup (#2656)
_collect_settings exports every Settings row minus two credential keys, so
auth_enabled / advanced_auth_enabled / local_login_enabled / setup_completed
all travel in a backup, and none of them are credential-shaped enough for
_SECRET_KEY_HINTS to catch. Writing them back was the one part of a settings
restore that changed who can reach the instance rather than how it behaves:
* auth_enabled=false — from any backup taken before auth was turned on —
disabled authentication. core.auth caches only the enabled=True result, on
a 30 s TTL, precisely so staleness fails closed; set_auth_enabled pairs its
write with invalidate_auth_enabled_cache(). The restore did neither, so it
left the stored value the open one.
* local_login_enabled=false walked straight past the #1589 refusals in
update_settings (no enabled OIDC provider / no OIDC link on the caller),
which exist to stop exactly that lockout.
* /github-backup/restore is gated on GITHUB_RESTORE alone, so honouring these
keys made that permission a way to rewrite auth config without
SETTINGS_UPDATE.
Flipping an existing row needed overwrite_existing, so the odds were lower
than the severity. Both refusals now share _is_skipped_setting_key so the
preview's item count still matches what a restore writes, and the skipped
keys get their own note pointing at the auth UI rather than being folded in
with the credential ones.
This commit is contained in:
@@ -70,6 +70,32 @@ _SENSITIVE_SETTING_KEYS = {"bambu_cloud_token", "auth_secret_key"}
|
||||
# skipped even if it isn't in the explicit denylist above.
|
||||
_SECRET_KEY_HINTS = ("token", "secret", "password", "access_code", "api_key", "passphrase")
|
||||
|
||||
# Keys that decide *who can reach the instance* rather than how it behaves. The
|
||||
# backup collector writes them like any other Settings row, so a backup taken
|
||||
# before auth was turned on carries auth_enabled=false — and a restore reaches
|
||||
# the table directly, so honouring them would:
|
||||
#
|
||||
# * disable authentication outright. ``set_auth_enabled`` pairs its write with
|
||||
# ``invalidate_auth_enabled_cache()``; we cannot, so the 30 s TTL in
|
||||
# core.auth is the only thing between the write and an open instance. That
|
||||
# cache is built to fail closed — writing the stored value behind its back
|
||||
# is what would make it fail open.
|
||||
# * bypass the lockout refusals ``update_settings`` enforces (a
|
||||
# ``local_login_enabled=false`` with no enabled OIDC provider, or with no
|
||||
# OIDC link on the caller, is a 400 there — #1589).
|
||||
# * cross a permission boundary: /github-backup/restore is gated on
|
||||
# GITHUB_RESTORE alone, so this would be a way to rewrite auth config
|
||||
# without SETTINGS_UPDATE.
|
||||
#
|
||||
# Auth is reconfigured through the auth UI, which has the guards. Restoring it
|
||||
# from a snapshot has no safe reading.
|
||||
_PROTECTED_SETTING_KEYS = {
|
||||
"auth_enabled",
|
||||
"advanced_auth_enabled",
|
||||
"local_login_enabled",
|
||||
"setup_completed",
|
||||
}
|
||||
|
||||
# Nozzle diameters the backup collector iterates. A path outside this set means
|
||||
# the backup was written by a newer version, so accept it rather than dropping
|
||||
# data, but keep the list for validation messages.
|
||||
@@ -91,6 +117,15 @@ def _is_blocked_setting_key(key: str) -> bool:
|
||||
return key in _SENSITIVE_SETTING_KEYS or any(hint in lowered for hint in _SECRET_KEY_HINTS)
|
||||
|
||||
|
||||
def _is_protected_setting_key(key: str) -> bool:
|
||||
return key in _PROTECTED_SETTING_KEYS
|
||||
|
||||
|
||||
def _is_skipped_setting_key(key: str) -> bool:
|
||||
"""True for any key ``_restore_settings`` refuses to write, for either reason."""
|
||||
return _is_blocked_setting_key(key) or _is_protected_setting_key(key)
|
||||
|
||||
|
||||
class _CategoryTally:
|
||||
"""Mutable accumulator matching ``GitHubRestoreCategoryResult``."""
|
||||
|
||||
@@ -276,9 +311,14 @@ class GitHubRestoreService:
|
||||
values = payload.get("settings") if isinstance(payload, dict) else None
|
||||
if not isinstance(values, dict):
|
||||
return 0, "No settings in payload"
|
||||
# Both refusals are counted the same way here so the preview's item
|
||||
# count matches what the restore actually writes; the wording only
|
||||
# calls out the credential ones, which are what a user might expect
|
||||
# to come back.
|
||||
blocked = sum(1 for key in values if _is_blocked_setting_key(key))
|
||||
skipped = sum(1 for key in values if _is_skipped_setting_key(key))
|
||||
detail = f"{blocked} credential-like keys will be skipped" if blocked else None
|
||||
return len(values) - blocked, detail
|
||||
return len(values) - skipped, detail
|
||||
|
||||
if category == RestoreCategory.SPOOLS:
|
||||
payload = parsed.get(SPOOLS_PATH)
|
||||
@@ -825,6 +865,7 @@ class GitHubRestoreService:
|
||||
return
|
||||
|
||||
blocked = 0
|
||||
protected = 0
|
||||
for key, value in values.items():
|
||||
if not isinstance(key, str) or not key:
|
||||
tally.failed += 1
|
||||
@@ -833,6 +874,10 @@ class GitHubRestoreService:
|
||||
blocked += 1
|
||||
tally.skipped += 1
|
||||
continue
|
||||
if _is_protected_setting_key(key):
|
||||
protected += 1
|
||||
tally.skipped += 1
|
||||
continue
|
||||
if value is None:
|
||||
tally.skipped += 1
|
||||
continue
|
||||
@@ -852,6 +897,11 @@ class GitHubRestoreService:
|
||||
|
||||
if blocked:
|
||||
tally.note(f"{blocked} credential-like key(s) skipped — re-enter secrets manually")
|
||||
if protected:
|
||||
tally.note(
|
||||
f"{protected} authentication setting(s) skipped — change those in Settings > "
|
||||
"Authentication so the lockout checks still run"
|
||||
)
|
||||
|
||||
async def _restore_kprofiles(self, db: AsyncSession, payload: dict, tally: _CategoryTally) -> None:
|
||||
by_serial: dict[str, list[tuple[str, dict]]] = {}
|
||||
|
||||
@@ -26,6 +26,8 @@ from backend.app.services.github_restore import (
|
||||
GitHubRestoreService,
|
||||
_CategoryTally,
|
||||
_is_blocked_setting_key,
|
||||
_is_protected_setting_key,
|
||||
_is_skipped_setting_key,
|
||||
_parse_dt,
|
||||
)
|
||||
|
||||
@@ -71,6 +73,20 @@ class TestSettingKeyBlocklist:
|
||||
def test_ordinary_keys_are_allowed(self, key):
|
||||
assert _is_blocked_setting_key(key) is False
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"key",
|
||||
["auth_enabled", "advanced_auth_enabled", "local_login_enabled", "setup_completed"],
|
||||
)
|
||||
def test_auth_policy_keys_are_protected(self, key):
|
||||
# Not credential-shaped, so the secret hints never catch them.
|
||||
assert _is_blocked_setting_key(key) is False
|
||||
assert _is_protected_setting_key(key) is True
|
||||
assert _is_skipped_setting_key(key) is True
|
||||
|
||||
@pytest.mark.parametrize("key", ["currency", "ldap_enabled", "auth_secret_key"])
|
||||
def test_protected_set_is_only_the_auth_policy_keys(self, key):
|
||||
assert _is_protected_setting_key(key) is False
|
||||
|
||||
|
||||
class TestCategoryTally:
|
||||
def test_notes_are_deduplicated(self):
|
||||
@@ -164,6 +180,36 @@ class TestRestoreSettings:
|
||||
assert tally.skipped == 2
|
||||
assert any("credential-like" in note for note in tally.notes)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_auth_settings_are_never_restored(self, db_session):
|
||||
"""Restoring auth_enabled=false would disable auth behind the cache's back."""
|
||||
db_session.add(Settings(key="auth_enabled", value="true"))
|
||||
db_session.add(Settings(key="local_login_enabled", value="true"))
|
||||
await db_session.commit()
|
||||
tally = _CategoryTally()
|
||||
payload = {
|
||||
"settings": {
|
||||
"currency": "EUR",
|
||||
"auth_enabled": "false",
|
||||
"advanced_auth_enabled": "false",
|
||||
"local_login_enabled": "false",
|
||||
"setup_completed": "false",
|
||||
}
|
||||
}
|
||||
|
||||
await _service()._restore_settings(db_session, payload, overwrite=True, tally=tally)
|
||||
await db_session.commit()
|
||||
|
||||
rows = {s.key: s.value for s in (await db_session.execute(select(Settings))).scalars().all()}
|
||||
assert rows["auth_enabled"] == "true"
|
||||
assert rows["local_login_enabled"] == "true"
|
||||
assert "advanced_auth_enabled" not in rows
|
||||
assert "setup_completed" not in rows
|
||||
assert rows["currency"] == "EUR"
|
||||
assert tally.restored == 1
|
||||
assert tally.skipped == 4
|
||||
assert any("authentication setting" in note for note in tally.notes)
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_missing_payload_is_noted_not_fatal(self, db_session):
|
||||
tally = _CategoryTally()
|
||||
|
||||
Reference in New Issue
Block a user