fix(cli): stamp each sign-in past the keychain as well as the file

A login the keychain took but the token file could not record leaves the
keychain naming a later sign-in than the file does. Reading only the file
then stamps the next login below that keychain entry, and a clock that
went back far enough puts the superseded credential back in use.
This commit is contained in:
mateo-berri 2026-08-20 05:07:21 -07:00
parent 8a8e8dc8ed
commit fb69fcf765
2 changed files with 45 additions and 8 deletions

View file

@ -145,7 +145,7 @@ def save_cli_token(record: CliTokenRecord, *, vault: SecretVault = SYSTEM_KEYRIN
keychain has already taken the new secret, and it reports itself as such rather than claiming
the previous login survived.
"""
stamped: Final = _stamped_past_the_login_on_disk(record)
stamped: Final = _stamped_past_every_stored_login(record, vault)
staged: Final = _stage_token_file(_without_secret(stamped))
if isinstance(staged, CredentialNotSaved):
return staged
@ -156,18 +156,42 @@ def save_cli_token(record: CliTokenRecord, *, vault: SecretVault = SYSTEM_KEYRIN
return _keep_the_secret_in_the_file(stamped, outcome)
def _stamped_past_the_login_on_disk(record: CliTokenRecord) -> CliTokenRecord:
"""Keep a sign-in's stamp ahead of the one it replaces, whatever the clock did in between.
def _stamped_past_every_stored_login(record: CliTokenRecord, vault: SecretVault) -> CliTokenRecord:
"""Keep a sign-in's stamp ahead of every login already stored, whatever the clock did in between.
The stamp is what decides a keychain secret against one still on disk, so a clock that stepped
backwards between two logins would hand the older of them the win and put a superseded
credential back in use. The file already names the login being replaced, and pinning the new
stamp just past it costs one read that changes nothing on a clock that only moves forwards.
credential back in use. Pinning the new stamp just past the highest one either store holds costs
one read each and changes nothing on a clock that only moves forwards.
"""
highest: Final = _highest_stamp_already_stored(record.base_url, vault)
if highest < record.timestamp:
return record
return record.model_copy(update=MappingProxyType({"timestamp": math.nextafter(highest, math.inf)}))
def _highest_stamp_already_stored(base_url: str, vault: SecretVault) -> float:
"""When the latest login either store still holds was made, or minus infinity when neither has one.
Both are asked because the file names the login being replaced only while the two agree. A login
the keychain took but the file could not record afterwards leaves the keychain holding the later
of the two, and reading only the file would stamp the next sign-in below it.
"""
previous: Final = _read_token_file()
if previous is None or previous.timestamp < record.timestamp:
return record
return record.model_copy(update=MappingProxyType({"timestamp": math.nextafter(previous.timestamp, math.inf)}))
secret: Final = _stored_secret(base_url, vault)
return max(
-math.inf if previous is None else previous.timestamp,
-math.inf if secret is None else secret.timestamp,
)
def _stored_secret(base_url: str, vault: SecretVault) -> CliTokenSecret | None:
"""The keychain's secret for this server, when it holds one this login may be compared against"""
match vault.read():
case SecretFound(blob=blob):
return _decode_secret(blob, base_url)
case SecretMissing() | KeyringNotInstalled() | KeyringDisabled() | KeyringUnreachable():
return None
def _keep_the_secret_in_the_file(record: CliTokenRecord, outcome: SecretWrite) -> SecretSave:

View file

@ -528,6 +528,19 @@ class TestSaveCliToken:
assert json.loads(vault.blob)["timestamp"] == 2000.0
assert json.loads(_token_file(isolated_home).read_text())["timestamp"] == 2000.0
def test_a_login_is_stamped_past_the_keychain_the_file_could_not_keep_up_with(
self, isolated_home, secret_vault_factory
):
"""A login reported as CredentialNotRecorded leaves the keychain holding a later sign-in
than the file names, so the file alone is no longer the floor. A later login on a clock
that went back past that keychain entry still has to be the one served."""
_write_legacy_file(isolated_home, key="sk-superseded", timestamp=1000.0)
vault = secret_vault_factory(blob=_blob(key="sk-recorded", timestamp=2000.0), writable=False)
save_cli_token(CliTokenRecord(base_url=SERVER, key="sk-fresh", timestamp=1500.0), vault=vault)
assert load_cli_token(vault=vault).key == "sk-fresh"
class TestScrubFailure:
"""A keychain that took the secret while the file kept it is the worst of both stores: the