From e573a6d9a6c43af766a7a8fcee66751b97bdf96c Mon Sep 17 00:00:00 2001 From: danielhanchen Date: Wed, 29 Jul 2026 07:45:36 +0000 Subject: [PATCH] Tighten the bootstrap newline comments --- studio/backend/auth/storage.py | 48 +++++++++---------- studio/backend/tests/test_desktop_auth.py | 24 ++++------ unsloth_cli/commands/studio.py | 2 +- .../tests/test_studio_password_prompt.py | 2 +- 4 files changed, 33 insertions(+), 43 deletions(-) diff --git a/studio/backend/auth/storage.py b/studio/backend/auth/storage.py index 2b4e26f19d..9702827725 100644 --- a/studio/backend/auth/storage.py +++ b/studio/backend/auth/storage.py @@ -32,20 +32,18 @@ _bootstrap_password: Optional[str] = None def _bootstrap_file_bytes(password: str) -> bytes: - """The exact on-disk form: the secret plus one LF. + """Exact on-disk form: the secret plus one LF. - Encoded rather than written as text because text mode translates "\\n" to - CRLF on Windows, and `$(cat ...)` strips the LF but leaves the CR attached - to the credential. + Bytes, not text: text mode writes CRLF on Windows, and `$(cat ...)` strips + the LF but leaves the CR attached to the credential. """ return (password + "\n").encode("utf-8") def _persist_bootstrap_password(password: str) -> None: - """Write the bootstrap password 0600, with a trailing LF on every OS. + """Atomically write the bootstrap password 0600, LF terminated on every OS. - Atomic: this can rewrite a live file, and a partial write would destroy the - only plaintext copy of the recovery credential. + A partial write would destroy the only plaintext recovery credential. """ fd, tmp_name = tempfile.mkstemp( prefix = f".{_BOOTSTRAP_PW_PATH.name}.", dir = _BOOTSTRAP_PW_PATH.parent @@ -69,22 +67,20 @@ def _persist_bootstrap_password(password: str) -> None: def _normalise_bootstrap_file(raw: bytes, password: str) -> None: """Append the LF a pre-newline release left off. - Append-only, and only to a file that is exactly the credential. Rewriting - could restore revoked plaintext, because clear_bootstrap_password() may - unlink or (when unlink fails, notably on Windows while this descriptor is - open) truncate the file through another descriptor at any point after we - read it. Appending cannot: the worst case is a lone "\\n" over a cleared - file, which strips to empty and reads back as no bootstrap password. - - Releases before the newline wrote the password with no terminator at all, - so that is the only shape in the wild. Anything else is left alone and - keeps working, since every reader strips. + Append-only, and only when the file is exactly the credential: + clear_bootstrap_password() may unlink or (when unlink fails, notably on + Windows while this descriptor is open) truncate through another descriptor + after we read, so a rewrite could restore revoked plaintext. An append + cannot: worst case is a lone "\\n" over a cleared file, which strips back to + no bootstrap password. Pre-newline releases wrote no terminator at all, so + that is the only shape in the wild; anything else reads fine, since every + reader strips, and is left alone. """ if raw != password.encode("utf-8"): return - # O_BINARY or Windows opens the descriptor in text mode and turns the LF - # straight back into CRLF, which is the bug this exists to fix. + # O_BINARY: without it Windows opens in text mode and turns the LF straight + # back into CRLF, the bug being fixed. fd = os.open( _BOOTSTRAP_PW_PATH, os.O_WRONLY | os.O_APPEND | getattr(os, "O_BINARY", 0), @@ -106,8 +102,8 @@ def _read_persisted_bootstrap_password() -> Optional[str]: return None # No caller handles a raise, so an unreadable file has to mean "no bootstrap - # password", not a dead backend. We write UTF-8, so bytes that will not - # decode are damage whose plaintext is worthless anyway. + # password", not a dead backend. We write UTF-8, so undecodable bytes are + # damage whose plaintext is worthless anyway. try: raw = _BOOTSTRAP_PW_PATH.read_bytes() password = raw.decode("utf-8").strip() @@ -116,8 +112,8 @@ def _read_persisted_bootstrap_password() -> Optional[str]: if not password: return None - # Older releases wrote no terminator, so upgrades kept the `cat` problem. - # Best-effort: a read-only auth dir must not fail startup. + # Older releases wrote no terminator; best-effort, a read-only auth dir must + # not fail startup. if raw != _bootstrap_file_bytes(password): try: _normalise_bootstrap_file(raw, password) @@ -166,9 +162,9 @@ def get_bootstrap_password() -> Optional[str]: def _load_bootstrap_password() -> Optional[str]: """Load an existing bootstrap password without creating one. - This, not generate_bootstrap_password(), is the path an upgraded install - takes (ensure_default_admin short-circuits once the admin row exists), so - the normalisation has to happen here too. + Upgrades take this path, not generate_bootstrap_password() + (ensure_default_admin short-circuits once the admin row exists), so it has + to normalise too. """ global _bootstrap_password _bootstrap_password = _read_persisted_bootstrap_password() diff --git a/studio/backend/tests/test_desktop_auth.py b/studio/backend/tests/test_desktop_auth.py index 0b873e7ef7..cbffe9568d 100644 --- a/studio/backend/tests/test_desktop_auth.py +++ b/studio/backend/tests/test_desktop_auth.py @@ -135,10 +135,10 @@ def test_ensure_default_admin_loads_existing_bootstrap_after_restart(monkeypatch def test_bootstrap_password_file_ends_with_a_newline(): - # Otherwise `cat` welds the passphrase to the shell prompt and both get copied. + # Otherwise `cat` welds the passphrase onto the shell prompt. storage.ensure_default_admin() - # Bytes, not read_text: that decodes CRLF back to "\n" and hides a CR. + # Bytes: read_text would decode CRLF back to "\n" and hide a CR. raw = storage._BOOTSTRAP_PW_PATH.read_bytes() assert raw == storage.get_bootstrap_password().encode("utf-8") + b"\n" @@ -154,8 +154,7 @@ def test_bootstrap_password_round_trips_across_a_restart_with_the_newline(): def test_upgrade_normalises_the_bootstrap_file(): - # The upgrade path is ensure_default_admin() on an install that already has - # the admin row, which never reaches generate_bootstrap_password(). + # Upgrade path: the admin row exists, so generate_bootstrap_password() never runs. seed_user() storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") @@ -174,8 +173,7 @@ def test_upgrade_normalises_the_bootstrap_file(): ], ) def test_only_an_exactly_unterminated_bootstrap_file_is_touched(other): - # Appending is safe precisely because it is restricted to the one shape - # released code produced. Everything else reads fine and is left alone. + # Appending is safe only because it is restricted to the one released shape. seed_user() storage._BOOTSTRAP_PW_PATH.write_bytes(other) @@ -222,8 +220,7 @@ def test_migration_failure_does_not_break_startup(monkeypatch): def test_normalising_never_recreates_a_cleared_bootstrap_file(monkeypatch): - # A rename would resurrect the file if the password changed after the read, - # leaving revoked plaintext for a later auth.db reset to re-seed. + # A rename would resurrect revoked plaintext if the password changed after the read. seed_user() storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") @@ -263,8 +260,7 @@ def test_normalising_does_not_overwrite_a_rotated_bootstrap_file(monkeypatch): def test_leading_whitespace_bootstrap_file_is_left_alone(monkeypatch): - # Only trailing whitespace is rewritten in place: without a rename there is - # no atomicity, and a partial rewrite here could mis-strip. + # An in-place rewrite is not atomic, so only the exact unterminated shape is touched. seed_user() storage._BOOTSTRAP_PW_PATH.write_bytes(b" legacy-bootstrap-secret ") @@ -275,8 +271,7 @@ def test_leading_whitespace_bootstrap_file_is_left_alone(monkeypatch): def test_normalising_opens_the_file_in_binary_mode(monkeypatch): - # Without O_BINARY, Windows opens the descriptor in text mode and os.write - # turns the LF back into CRLF, reintroducing the bug being fixed. + # Without O_BINARY, Windows text mode turns the written LF back into CRLF. seed_user() storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") monkeypatch.setattr(storage.os, "O_BINARY", 0x8000, raising = False) @@ -296,9 +291,8 @@ def test_normalising_opens_the_file_in_binary_mode(monkeypatch): def test_clearing_by_truncation_mid_normalisation_is_not_undone(monkeypatch): - # clear_bootstrap_password() truncates through its own descriptor when the - # unlink fails, which is what happens on Windows while ours is open. The - # append must not put the revoked plaintext back. + # clear_bootstrap_password() truncates through its own descriptor when the unlink + # fails (Windows, while ours is open); the append must not restore the plaintext. seed_user() storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") diff --git a/unsloth_cli/commands/studio.py b/unsloth_cli/commands/studio.py index 9185b0aa51..bfd748ae00 100644 --- a/unsloth_cli/commands/studio.py +++ b/unsloth_cli/commands/studio.py @@ -483,7 +483,7 @@ def _write_auth_secret(path: Path, secret: str) -> None: os.chmod(tmp_path, 0o600) except OSError: pass - # newline pins LF: text mode would write CRLF on Windows, and `$(cat ...)` + # newline pins LF: text mode writes CRLF on Windows, and `$(cat ...)` # strips the LF but leaves the CR glued to the credential. with os.fdopen(fd, "w", encoding = "utf-8", newline = "\n") as f: fd = -1 diff --git a/unsloth_cli/tests/test_studio_password_prompt.py b/unsloth_cli/tests/test_studio_password_prompt.py index 9be3320e4e..f45b228c84 100644 --- a/unsloth_cli/tests/test_studio_password_prompt.py +++ b/unsloth_cli/tests/test_studio_password_prompt.py @@ -1211,7 +1211,7 @@ def test_write_auth_secret_terminates_the_file_with_a_newline(monkeypatch, tmp_p studio_mod._write_auth_secret(path, "desktop-abc123") - # Bytes, not read_text: that decodes CRLF back to "\n" and hides a CR. + # Bytes: read_text would decode CRLF back to "\n" and hide a CR. assert path.read_bytes() == b"desktop-abc123\n"