From fe11202c2df1a6373ed3230e22847a76e157cb0d Mon Sep 17 00:00:00 2001 From: mateo-berri <277851410+mateo-berri@users.noreply.github.com> Date: Thu, 20 Aug 2026 03:28:28 -0700 Subject: [PATCH] fix(cli): write the logout note again when the file holding it had to go When a full disk refuses the replacement file and a read-only token file refuses the rewrite in place, the only way left to get the secret off disk is to remove the file carrying it. That file was also the note saying the keychain went unchecked, so its absence made the next logout read a keychain that was never confirmed as one already known to be clean. Removing it is what frees the room the replacement was refused for, so the note is written again on the way out and the logout after this one still warns. --- litellm/litellm_core_utils/cli_token_utils.py | 32 ++++++++++-- .../test_cli_token_utils.py | 52 ++++++++++++++++++- 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/litellm/litellm_core_utils/cli_token_utils.py b/litellm/litellm_core_utils/cli_token_utils.py index 2f45742ce03..69a77a36882 100644 --- a/litellm/litellm_core_utils/cli_token_utils.py +++ b/litellm/litellm_core_utils/cli_token_utils.py @@ -173,7 +173,8 @@ def clear_cli_token(*, vault: SecretVault = SYSTEM_KEYRING) -> SecretClear: what lets a later run tell a machine with a credential it cannot reach apart from one that never had a login at all, and taking it away would leave the next logout answering the warning this one just issued with a false all-clear. The secret goes either way, and a file that will give up - neither its copy nor itself outranks whatever the keychain had to say. + neither its copy nor itself is removed rather than kept, with the note written again afterwards + so the warning still outlives this run. """ outcome: Final = vault.erase() record: Final = _read_token_file() @@ -183,6 +184,8 @@ def clear_cli_token(*, vault: SecretVault = SYSTEM_KEYRING) -> SecretClear: removal: Final = _remove_token_file() if removal is not None and record is not None and not _scrub_file_secret(record): return removal + if removal is None and record is not None and _the_keychain_went_unchecked(outcome): + _write_the_note_the_removal_took_with_it(record) return SecretErased() if settled else outcome @@ -194,6 +197,19 @@ def _remove_token_file() -> CredentialNotCleared | None: return None +def _write_the_note_the_removal_took_with_it(record: CliTokenRecord) -> None: + """Put the secret-free note back after the file carrying it had to go to get the secret off disk. + + Reaching here means neither rewrite would take, so the file went instead, and its absence is + what the next logout would read as a keychain already known to be clean. Removing it is also + what frees the room the rewrite was refused for, so the note usually lands on this second try. + When it does not, the warning this logout printed is the only one the user gets. + """ + staged: Final = _stage_scrubbed_file(record) + if staged is not None: + _commit_token_file(staged) + + def _keep_the_unchecked_keychain_on_record(outcome: SecretErase, record: CliTokenRecord | None) -> bool: """Whether the token file, stripped of its secret, is worth keeping as the note that says so. @@ -203,17 +219,27 @@ def _keep_the_unchecked_keychain_on_record(outcome: SecretErase, record: CliToke its secret neither to a staged replacement nor to an overwrite is not kept either, because the secret goes first. """ - if record is None or isinstance(outcome, SecretStranded): + if record is None or not _the_keychain_went_unchecked(outcome): return False return _scrub_file_secret(record) +def _the_keychain_went_unchecked(outcome: SecretErase) -> bool: + """Whether the keychain neither confirmed the erase nor answered that it still holds the secret""" + match outcome: + case SecretErased() | SecretStranded(): + return False + case KeyringDisabled() | KeyringNotInstalled() | KeyringUnreachable(): + return True + + def _nothing_left_behind(outcome: SecretErase, record: CliTokenRecord | None) -> bool: """Whether the keychain can be trusted to hold no credential of ours once the file is gone. A machine with no token file has no stored login to end, and `clear_cli_token` keeps one behind whenever the keychain is left unconfirmed, taking the secret out in place when it cannot stage a - replacement, so a missing file is real evidence rather than the absence of it. Past that, a + replacement and writing the note again when the file holding it had to go, so a missing file is + real evidence rather than the absence of it. Past that, a keychain that could not be reached is never trusted, whatever the file looks like. Even a file holding its own secret says only that the login which wrote it had no keychain to write to, and the login before it may well have had one: the entry that login left outlives both the diff --git a/tests/test_litellm/litellm_core_utils/test_cli_token_utils.py b/tests/test_litellm/litellm_core_utils/test_cli_token_utils.py index e3d14c6da46..b8fcf3618cf 100644 --- a/tests/test_litellm/litellm_core_utils/test_cli_token_utils.py +++ b/tests/test_litellm/litellm_core_utils/test_cli_token_utils.py @@ -1,7 +1,9 @@ +import errno import json import os import stat import sys +import tempfile import threading import time @@ -82,6 +84,25 @@ def _blob(base_url=SERVER, key="sk-vault", jwt_token=""): return json.dumps({"base_url": base_url, "key": key, "jwt_token": jwt_token}) +_REAL_MKSTEMP = tempfile.mkstemp + + +class _MkstempThatNeedsTheOldFileGone: + """A disk with exactly one token file's worth of room left on it. + + Staging a replacement needs room for a second file, which is what a full disk refuses. Removing + the file already there is what gives that room back. + """ + + def __init__(self, path): + self.path = path + + def __call__(self, *args, **kwargs): + if self.path.exists(): + raise OSError(errno.ENOSPC, "No space left on device") + return _REAL_MKSTEMP(*args, **kwargs) + + _REAL_REPLACE = os.replace @@ -553,7 +574,7 @@ class TestClearCliToken: assert load_cli_token(vault=vault) is None @pytest.mark.parametrize( - "failure", [KeyringDisabled(), KeyringUnreachable(), KeyringDiscardsWrites()] + "failure", [KeyringDisabled(), KeyringUnreachable(), KeyringNotInstalled()] ) def test_a_secret_in_the_file_is_no_evidence_about_a_keychain_that_exists( self, isolated_home, secret_vault_factory, failure @@ -561,7 +582,10 @@ class TestClearCliToken: """Store a secret in the keychain, sign in again while the keychain is unusable so the new secret lands in the file, then log out while it is still unusable. The file now carries its own secret and the first login's entry is still there, so reading the file as proof of a - clean keychain reports a logout that did not happen.""" + clean keychain reports a logout that did not happen. + + The three unusable states are the whole of what an erase can answer besides erased and + stranded; a backend that keeps nothing it is given is something only a write finds out.""" _write_legacy_file(isolated_home) vault = secret_vault_factory(available=False, failure=failure) @@ -705,6 +729,30 @@ class TestClearCliToken: assert json.loads(path.read_text()).get("key") is None + @pytest.mark.skipif(os.geteuid() == 0, reason="root ignores file permissions") + def test_a_note_the_logout_had_to_remove_is_written_again_for_the_next_one( + self, isolated_home, secret_vault_factory, monkeypatch + ): + """A full disk refuses the replacement file and a read-only token file refuses the rewrite + in place, so the only way left to get the secret off disk is to remove the file carrying it. + That file was also the note saying the keychain went unchecked, and its absence is what the + next logout would read as a keychain already known to be clean. + + Removing it is what frees the room the replacement was refused for, so the note is written + again on the way out and the logout after this one still warns.""" + path = _write_legacy_file(isolated_home) + path.chmod(0o400) + monkeypatch.setattr( + "litellm.litellm_core_utils.private_json.tempfile.mkstemp", + _MkstempThatNeedsTheOldFileGone(path), + ) + vault = secret_vault_factory(available=False, failure=KeyringUnreachable()) + + assert clear_cli_token(vault=vault) == KeyringUnreachable() + assert clear_cli_token(vault=vault) == KeyringUnreachable() + + assert json.loads(path.read_text()).get("key") is None + @pytest.mark.skipif(os.geteuid() == 0, reason="root ignores directory permissions") def test_a_metadata_file_that_will_not_go_is_not_worth_alarming_the_user_over( self, isolated_home, secret_vault_factory