From 22c2245ac4cb2f920e9ec24cd350b5b8873b9cbe Mon Sep 17 00:00:00 2001 From: Daniel Maksymilian Syrnicki Date: Mon, 28 Sep 2026 13:11:42 +0200 Subject: [PATCH] fix(updater,auth): atomic symlink flip, roll back on any failure, fail closed on a damaged users.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updater: - /opt/furtka/current is swapped via rename() instead of unlink+symlink, so a power cut can't leave the box without a current version - OSError after the flip (disk full writing the Caddyfile) now rolls back too; rollback restores the previous Caddyfile and reloads caddy + daemon-reload - the health check keeps polling on read timeouts / RemoteDisconnected instead of escaping and skipping the rollback - run_update writes an 'error' stage for any failure (download, sha, disk full); the settings page shows it instead of spinning on 'Downloading…' auth: - users.json that exists but is empty/corrupt/unreadable raises instead of reading as 'no admin yet' — before, a truncated file made /login an open create-admin form for the whole LAN. /login answers 503 with SSH recovery steps instead - save_users fsyncs before the rename - first-run admin creation is serialised (threaded server) Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Wq1hzUp4ZmCKHDuLKCnu73 --- assets/www/settings/index.html | 6 ++- furtka/api.py | 23 ++++++++- furtka/auth.py | 32 ++++++++---- furtka/updater.py | 93 ++++++++++++++++++++++++---------- tests/test_api.py | 22 ++++++++ tests/test_auth.py | 17 ++++--- tests/test_updater.py | 76 +++++++++++++++++++++++++++ 7 files changed, 221 insertions(+), 48 deletions(-) diff --git a/assets/www/settings/index.html b/assets/www/settings/index.html index 3966a2c..e403c2e 100644 --- a/assets/www/settings/index.html +++ b/assets/www/settings/index.html @@ -171,6 +171,7 @@ done: 'Update complete — reloading…', rolled_back: 'Update failed, rolled back to the previous version', rolled_back_manual: 'Rolled back manually', + error: 'Update failed — nothing was changed', }; let pollHandle = null; @@ -354,12 +355,13 @@ if (!r.ok) return; const s = await r.json(); const label = STAGE_LABELS[s.stage] || `Stage: ${s.stage}`; - setStatus(label, s.stage === 'rolled_back'); + const failed = s.stage === 'rolled_back' || s.stage === 'error'; + setStatus(label, failed); if (s.stage === 'done') { clearInterval(pollHandle); clearTimeout(fallbackReloadHandle); setTimeout(() => location.reload(), 5000); - } else if (s.stage === 'rolled_back') { + } else if (failed) { clearInterval(pollHandle); clearTimeout(fallbackReloadHandle); if (s.reason) { diff --git a/furtka/api.py b/furtka/api.py index 1c2d20a..417700a 100644 --- a/furtka/api.py +++ b/furtka/api.py @@ -1257,6 +1257,14 @@ MAX_BODY_BYTES = 1024 * 1024 # a slow request (image pull, reconcile) runs. Requests that change state # still run one at a time, as they did on the single-threaded server. _MUTATION_LOCK = threading.Lock() +# First-run admin creation is check-then-write; two setup POSTs racing on +# the threaded server must not both create an admin. +_SETUP_LOCK = threading.Lock() + +_USERS_FILE_BROKEN = ( + "The login file on this box (/var/lib/furtka/users.json) is damaged. " + "Log in over SSH, delete that file, then reload this page to set a new admin password." +) def _valid_app_name(name) -> bool: @@ -1376,6 +1384,14 @@ class _Handler(BaseHTTPRequestHandler): return self._json(400, {"error": "password is required"}) username = username.strip() + try: + with _SETUP_LOCK: + return self._login_or_setup(payload, username, password) + except auth.UsersFileError: + traceback.print_exc() + return self._json(503, {"error": _USERS_FILE_BROKEN}) + + def _login_or_setup(self, payload, username, password): if auth.setup_needed(): # First-run setup path — create the admin account, then log # in. Require password2 so a typo doesn't lock the user out @@ -1449,7 +1465,12 @@ class _Handler(BaseHTTPRequestHandler): # Already authed? Skip straight to the app list. if self._current_session() is not None: return self._redirect("/apps") - return self._html(200, _render_login_html(auth.setup_needed())) + try: + setup = auth.setup_needed() + except auth.UsersFileError: + traceback.print_exc() + return self._html(503, f"

Login unavailable

{_USERS_FILE_BROKEN}

") + return self._html(200, _render_login_html(setup)) # --- Auth guard for everything below ------------------------- session = self._current_session() diff --git a/furtka/auth.py b/furtka/auth.py index 6dbfa35..49fc7ab 100644 --- a/furtka/auth.py +++ b/furtka/auth.py @@ -23,6 +23,7 @@ from __future__ import annotations import json import math +import os import secrets import threading from dataclasses import dataclass @@ -46,27 +47,33 @@ def verify_password(plain: str, hashed: str) -> bool: return _verify_password(plain, hashed) +class UsersFileError(RuntimeError): + """users.json exists but can't be read or parsed.""" + + def load_users() -> dict: - """Return the users dict, or {} if the file is missing or empty. + """Return the users dict, or {} if the file is missing. Missing-file is the expected state on first boot and on upgrades from pre-auth versions — callers treat empty-dict as "setup required". + + A file that exists but is unreadable, empty or corrupt raises + UsersFileError instead: treating it as "no admin yet" would turn /login + into an open "create admin" form for anyone on the network. """ path = users_file() - if not path.exists(): - return {} try: raw = path.read_text() - except OSError: - return {} - if not raw.strip(): + except FileNotFoundError: return {} + except (OSError, UnicodeDecodeError) as e: + raise UsersFileError(f"{path} is unreadable: {e}") from e try: data = json.loads(raw) - except json.JSONDecodeError: - return {} + except json.JSONDecodeError as e: + raise UsersFileError(f"{path} is corrupt: {e}") from e if not isinstance(data, dict): - return {} + raise UsersFileError(f"{path} is corrupt: not a JSON object") return data @@ -80,7 +87,12 @@ def save_users(users: dict) -> None: path = users_file() path.parent.mkdir(parents=True, exist_ok=True) tmp = path.with_suffix(path.suffix + ".tmp") - tmp.write_text(json.dumps(users, indent=2) + "\n") + with open(tmp, "w") as fh: + fh.write(json.dumps(users, indent=2) + "\n") + # A power cut right after rename() can otherwise leave a zero-length + # users.json, which load_users() refuses — locking the owner out. + fh.flush() + os.fsync(fh.fileno()) tmp.chmod(0o600) tmp.replace(path) diff --git a/furtka/updater.py b/furtka/updater.py index 96a3073..39455fd 100644 --- a/furtka/updater.py +++ b/furtka/updater.py @@ -29,6 +29,7 @@ the updater at a tmpdir. from __future__ import annotations import fcntl +import http.client import json import os import shutil @@ -265,9 +266,10 @@ def write_state(stage: str, **extra) -> None: def read_state() -> dict: try: - return json.loads(state_path().read_text()) - except (FileNotFoundError, json.JSONDecodeError): + data = json.loads(state_path().read_text()) + except (OSError, UnicodeDecodeError, json.JSONDecodeError): return {} + return data if isinstance(data, dict) else {} def acquire_lock(): @@ -318,13 +320,29 @@ def _health_check(url: str, deadline_s: float = 30.0) -> bool: # check. 5xx → genuinely broken, don't accept. if 400 <= e.code < 500: return True - except urllib.error.URLError: - # Connection refused / DNS / timeout → not up yet, retry. + except (OSError, http.client.HTTPException): + # Connection refused / reset / timeout (URLError is an OSError; + # a read timeout or RemoteDisconnected is not wrapped by urllib) + # → not up yet, retry. pass time.sleep(1) return False +def _swap_symlink(link: Path, target: str | Path) -> None: + """Point `link` at `target` atomically. + + unlink() + symlink_to() leaves a window with no /opt/furtka/current at + all — a power cut there boots a box with no services. rename() over the + old link is atomic. + """ + tmp = link.with_name(link.name + ".new") + if tmp.is_symlink() or tmp.exists(): + tmp.unlink() + tmp.symlink_to(target) + os.replace(tmp, link) + + def prepare_update(check: UpdateCheck, download_dir: Path | None = None) -> tuple[Path, str]: """Download + verify the tarball. Returns (tarball_path, version).""" if not check.tarball_url or not check.sha256_url: @@ -369,15 +387,11 @@ def apply_update(tarball: Path, version: str) -> None: target.chmod(0o755) write_state("swapping", latest=version) - previous = None - if current.is_symlink(): - previous = os.readlink(current) - current.unlink() + previous = os.readlink(current) if current.is_symlink() else None + old_caddyfile = _CADDYFILE_LIVE.read_text() if _CADDYFILE_LIVE.is_file() else None try: - current.symlink_to(target) + _swap_symlink(current, target) except OSError as e: - if previous: - current.symlink_to(previous) raise UpdateError(f"symlink swap failed: {e}") from e write_state("restarting", latest=version) @@ -393,27 +407,41 @@ def apply_update(tarball: Path, version: str) -> None: _run(["systemctl", "daemon-reload"]) _run(["systemctl", "try-restart", "furtka-reconcile.service"]) _run(["systemctl", "restart", "furtka-api.service"]) - except UpdateError as e: - _rollback(previous, version, f"service restart failed: {e}") - raise + except (UpdateError, OSError) as e: + # OSError: e.g. disk full or EACCES writing the Caddyfile — the new + # version is already live, so this must roll back too. + _rollback(previous, version, f"service restart failed: {e}", old_caddyfile) + raise UpdateError(f"service restart failed: {e} — rolled back") from e write_state("verifying", latest=version) ok = _health_check("http://127.0.0.1:7000/api/apps", deadline_s=30.0) if not ok: - _rollback(previous, version, "health check failed after restart") + _rollback(previous, version, "health check failed after restart", old_caddyfile) raise UpdateError("new version failed health check — rolled back") write_state("done", version=version) -def _rollback(previous_target: str | None, failed_version: str, reason: str) -> None: +def _rollback( + previous_target: str | None, + failed_version: str, + reason: str, + old_caddyfile: str | None = None, +) -> None: current = current_symlink() if previous_target: - if current.is_symlink(): - current.unlink() - current.symlink_to(previous_target) + _swap_symlink(current, previous_target) + # The new release may have shipped Caddy routes the old code + # doesn't serve; put the previous Caddyfile back before reloading. + if old_caddyfile is not None: + try: + _CADDYFILE_LIVE.write_text(old_caddyfile) + except OSError: + pass # Best-effort restart on the previous target — if it fails too the # box is in a hard state, but we can only surface the reason. + subprocess.run(["systemctl", "daemon-reload"], check=False) + subprocess.run(["systemctl", "reload", "caddy"], check=False) subprocess.run(["systemctl", "restart", "furtka-api.service"], check=False) write_state( "rolled_back", @@ -430,13 +458,25 @@ def run_update() -> UpdateCheck: UpdateError on any failure; the state file records the stage. """ with acquire_lock(): - check = check_update() - if not check.update_available: - write_state("done", version=check.current, note="already up to date") + try: + check = check_update() + if not check.update_available: + write_state("done", version=check.current, note="already up to date") + return check + tarball, version = prepare_update(check) + apply_update(tarball, version) return check - tarball, version = prepare_update(check) - apply_update(tarball, version) - return check + except Exception as e: + # Without a terminal stage the settings page keeps showing + # "Downloading…" forever. Rollback already wrote its own stage. + if read_state().get("stage") != "rolled_back": + try: + write_state("error", reason=str(e) or type(e).__name__) + except OSError: + pass + if isinstance(e, UpdateError): + raise + raise UpdateError(str(e) or type(e).__name__) from e def rollback() -> str: @@ -456,8 +496,7 @@ def rollback() -> str: raise UpdateError("no other version slots available to roll back to") target_name = candidates[0] target = versions_dir() / target_name - current.unlink() - current.symlink_to(target) + _swap_symlink(current, target) subprocess.run(["systemctl", "daemon-reload"], check=False) subprocess.run(["systemctl", "restart", "furtka-api.service"], check=False) write_state("rolled_back_manual", restored_to=target_name) diff --git a/tests/test_api.py b/tests/test_api.py index 39f3bb4..4953ae5 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -1502,3 +1502,25 @@ def test_http_unexpected_exception_returns_json_500(fake_dirs, admin_session, mo finally: server.shutdown() server.server_close() + + +def test_login_with_corrupt_users_file_is_503_not_setup(fake_dirs, tmp_path): + (tmp_path / "users.json").write_text("") + server, port = _start_server() + try: + with pytest.raises(urllib.error.HTTPError) as e: + urllib.request.urlopen(_request(port, "/login")) + assert e.value.code == 503 + req = _request( + port, + "/login", + method="POST", + body={"username": "mallory", "password": "longenough1", "password2": "longenough1"}, + ) + with pytest.raises(urllib.error.HTTPError) as e: + urllib.request.urlopen(req) + assert e.value.code == 503 + assert (tmp_path / "users.json").read_text() == "" + finally: + server.shutdown() + server.server_close() diff --git a/tests/test_auth.py b/tests/test_auth.py index c4c2fcb..ddfedf3 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -40,14 +40,15 @@ def test_load_users_returns_empty_when_missing(tmp_users_file): assert auth.load_users() == {} -def test_load_users_returns_empty_on_junk(tmp_users_file): - tmp_users_file.write_text("{not json") - assert auth.load_users() == {} - - -def test_load_users_returns_empty_on_non_dict(tmp_users_file): - tmp_users_file.write_text("[]") - assert auth.load_users() == {} +@pytest.mark.parametrize("content", ["{not json", "[]", "", "\x00\x00"]) +def test_load_users_fails_closed_on_corrupt_file(tmp_users_file, content): + """A damaged users.json must not read as "no admin yet" — that would + turn /login into an open create-admin form for anyone on the LAN.""" + tmp_users_file.write_text(content) + with pytest.raises(auth.UsersFileError): + auth.load_users() + with pytest.raises(auth.UsersFileError): + auth.setup_needed() def test_save_users_atomic_and_0600(tmp_users_file): diff --git a/tests/test_updater.py b/tests/test_updater.py index d78120d..13fddd9 100644 --- a/tests/test_updater.py +++ b/tests/test_updater.py @@ -463,3 +463,79 @@ def test_check_update_raises_when_no_releases_published(updater, monkeypatch): monkeypatch.setattr(updater, "_forgejo_api", lambda path: []) with pytest.raises(updater.UpdateError, match="no releases"): updater.check_update() + + +def test_apply_update_rolls_back_on_oserror_and_restores_caddyfile(tmp_path, updater, monkeypatch): + """A non-UpdateError after the symlink flip (disk full writing the + Caddyfile) used to leave the new version live with no rollback.""" + versions = updater.versions_dir() + versions.mkdir(parents=True) + (versions / "26.0-alpha").mkdir() + current = updater.current_symlink() + current.symlink_to(versions / "26.0-alpha") + caddyfile = tmp_path / "etc_caddy" / "Caddyfile" + caddyfile.parent.mkdir() + caddyfile.write_text("# old routes\n") + + tar = tmp_path / "t.tar.gz" + _make_tarball(tar, "26.1-alpha") + + def full_disk(_source): + caddyfile.write_text("# half-written new routes\n") + raise OSError(28, "No space left on device") + + monkeypatch.setattr(updater, "_refresh_caddyfile", full_disk) + monkeypatch.setattr(updater, "_run", lambda cmd: None) + import subprocess + + monkeypatch.setattr(subprocess, "run", lambda *a, **kw: None) + + with pytest.raises(updater.UpdateError, match="rolled back"): + updater.apply_update(tar, "26.1-alpha") + assert current.resolve() == (versions / "26.0-alpha").resolve() + assert caddyfile.read_text() == "# old routes\n" + assert updater.read_state()["stage"] == "rolled_back" + + +def test_swap_symlink_replaces_existing_link(tmp_path, updater): + a, b = tmp_path / "a", tmp_path / "b" + a.mkdir() + b.mkdir() + link = tmp_path / "current" + link.symlink_to(a) + updater._swap_symlink(link, b) + assert link.resolve() == b.resolve() + assert not (tmp_path / "current.new").exists() + + +def test_run_update_writes_error_stage_on_download_failure(updater, monkeypatch): + class _Check: + update_available = True + latest = "26.1-alpha" + current = "26.0-alpha" + tarball_url = "http://x/t.tar.gz" + sha256_url = "http://x/t.sha256" + + def fake_check(): + return _Check() + + monkeypatch.setattr(updater, "check_update", fake_check) + + def boom(url, dest): + raise OSError(28, "No space left on device") + + monkeypatch.setattr(updater, "_download", boom) + with pytest.raises(updater.UpdateError, match="No space left"): + updater.run_update() + state = updater.read_state() + assert state["stage"] == "error" + assert "No space left" in state["reason"] + + +def test_health_check_survives_read_timeout(updater, monkeypatch): + def slow(url, timeout): + raise TimeoutError("timed out") + + monkeypatch.setattr("urllib.request.urlopen", slow) + monkeypatch.setattr(updater.time, "sleep", lambda _s: None) + assert updater._health_check("http://127.0.0.1:7000/api/apps", deadline_s=0.05) is False