diff --git a/studio/backend/auth/storage.py b/studio/backend/auth/storage.py index 44df7c16f4..2b4e26f19d 100644 --- a/studio/backend/auth/storage.py +++ b/studio/backend/auth/storage.py @@ -67,43 +67,34 @@ def _persist_bootstrap_password(password: str) -> None: def _normalise_bootstrap_file(raw: bytes, password: str) -> None: - """Give an existing malformed file its trailing LF, in place. + """Append the LF a pre-newline release left off. - Deliberately not the atomic replace above: a rename would recreate the file - if clear_bootstrap_password() or the CLI cleanup deleted it after we read - it, putting revoked plaintext back on disk for a later reset to re-seed. - Opening without O_CREAT cannot resurrect a deleted file, and the contents - are re-checked through the descriptor so an in-place truncation is not - undone either. + 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. """ - data = _bootstrap_file_bytes(password) - # Only a trailing-whitespace rewrite is safe without a rename: every partial - # state is then "\n" plus leftover whitespace, which still strips to - # the same credential. Anything else keeps working unnormalised. - if not raw.startswith(data[:-1]): + if raw != password.encode("utf-8"): return - # O_BINARY or Windows opens the descriptor in text mode and os.write turns - # the LF straight back into CRLF, which is the bug this path exists to fix. - fd = os.open(_BOOTSTRAP_PW_PATH, os.O_RDWR | getattr(os, "O_BINARY", 0)) + # 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. + fd = os.open( + _BOOTSTRAP_PW_PATH, + os.O_WRONLY | os.O_APPEND | getattr(os, "O_BINARY", 0), + ) try: - if os.read(fd, len(raw) + 1) != raw: - return - os.lseek(fd, 0, os.SEEK_SET) - # os.write may write fewer bytes than asked; truncating after a short - # write would NUL-fill the credential and lock the admin out. - written = 0 - while written < len(data): - n = os.write(fd, data[written:]) - if not n: - return - written += n - os.ftruncate(fd, len(data)) + os.write(fd, b"\n") try: os.fchmod(fd, 0o600) except (AttributeError, OSError): - # fchmod only reached Windows in 3.13. Staying on the descriptor - # matters more than re-applying a mode the writer already set. + # fchmod only reached Windows in 3.13. pass finally: os.close(fd) @@ -125,9 +116,8 @@ def _read_persisted_bootstrap_password() -> Optional[str]: if not password: return None - # Older releases wrote no terminator, and text mode wrote CRLF on Windows, - # so upgrades kept the `cat` problem. Best-effort: a read-only auth dir or a - # credential cleared underneath us must not fail startup. + # Older releases wrote no terminator, so upgrades kept the `cat` problem. + # Best-effort: a read-only auth dir must not fail startup. if raw != _bootstrap_file_bytes(password): try: _normalise_bootstrap_file(raw, password) diff --git a/studio/backend/tests/test_desktop_auth.py b/studio/backend/tests/test_desktop_auth.py index a4a5b280d0..bc3b407f19 100644 --- a/studio/backend/tests/test_desktop_auth.py +++ b/studio/backend/tests/test_desktop_auth.py @@ -153,18 +153,11 @@ def test_bootstrap_password_round_trips_across_a_restart_with_the_newline(): assert storage.generate_bootstrap_password() == original -@pytest.mark.parametrize( - "legacy", - [ - b"legacy-bootstrap-secret", # written before the newline existed - b"legacy-bootstrap-secret\r\n", # written by text mode on Windows - ], -) -def test_upgrade_normalises_the_bootstrap_file(legacy): +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(). seed_user() - storage._BOOTSTRAP_PW_PATH.write_bytes(legacy) + storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") storage.ensure_default_admin() @@ -172,6 +165,23 @@ def test_upgrade_normalises_the_bootstrap_file(legacy): assert storage.get_bootstrap_password() == "legacy-bootstrap-secret" +@pytest.mark.parametrize("other", [ + b"legacy-bootstrap-secret\r\n", # only an unreleased build wrote this + b"legacy-bootstrap-secret\r", + b"legacy-bootstrap-secret ", +]) +def test_only_an_exactly_untermimated_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. + seed_user() + storage._BOOTSTRAP_PW_PATH.write_bytes(other) + + storage.ensure_default_admin() + + assert storage.get_bootstrap_password() == "legacy-bootstrap-secret" + assert storage._BOOTSTRAP_PW_PATH.read_bytes() == other + + def test_upgrade_normalises_when_the_admin_row_is_missing(): storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") @@ -242,7 +252,11 @@ def test_normalising_does_not_overwrite_a_rotated_bootstrap_file(monkeypatch): storage._read_persisted_bootstrap_password() - assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"brand-new-secret\n" + # The append may add a second newline; the rotated credential must survive. + raw = storage._BOOTSTRAP_PW_PATH.read_bytes() + assert raw.strip() == b"brand-new-secret" + storage._bootstrap_password = None + assert storage._load_bootstrap_password() == "brand-new-secret" def test_leading_whitespace_bootstrap_file_is_left_alone(monkeypatch): @@ -278,21 +292,29 @@ def test_normalising_opens_the_file_in_binary_mode(monkeypatch): assert seen and all(f & 0x8000 for f in seen), seen -def test_normalising_survives_a_short_write(monkeypatch): - # A short write followed by ftruncate would NUL-fill the credential. +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. seed_user() storage._BOOTSTRAP_PW_PATH.write_bytes(b"legacy-bootstrap-secret") - real_write = storage.os.write - def one_byte_at_a_time(fd, data): - return real_write(fd, data[:1]) + real_open = storage.os.open - monkeypatch.setattr(storage.os, "write", one_byte_at_a_time) + def truncate_then_open(path, flags, *args, **kwargs): + fd = real_open(path, flags, *args, **kwargs) + if str(path) == str(storage._BOOTSTRAP_PW_PATH): + storage._BOOTSTRAP_PW_PATH.write_text("", encoding = "utf-8") + return fd - storage.ensure_default_admin() + monkeypatch.setattr(storage.os, "open", truncate_then_open) - assert storage._BOOTSTRAP_PW_PATH.read_bytes() == b"legacy-bootstrap-secret\n" - assert storage.get_bootstrap_password() == "legacy-bootstrap-secret" + storage._read_persisted_bootstrap_password() + + # A lone newline over a cleared file still reads back as no password. + assert storage._BOOTSTRAP_PW_PATH.read_bytes().strip() == b"" + storage._bootstrap_password = None + assert storage._load_bootstrap_password() is None def test_normalising_works_without_fchmod(monkeypatch):