fix(cli): keep each sign-in stamped past the one it replaces

The stamp in the keychain entry is what decides that secret against one still
sitting in the token file, and it came straight off the wall clock. A clock
that stepped backwards between two logins therefore handed the win to the
older of them: a login the keychain took but the token file could not be
pointed at was resolved back to the credential it replaced, and the fresh one
was erased from the keychain on the way past.

save_cli_token now reads the stamp already on disk and pins the new sign-in
just above it, so the ordering never depends on the clock having moved
forwards. On a clock that did, this changes nothing.
This commit is contained in:
Mateo Edgeton 2026-08-20 04:53:09 -07:00
parent b9ce630b0e
commit a329dfbb45
2 changed files with 62 additions and 4 deletions

View file

@ -12,6 +12,7 @@ first time it reads one.
This module has no dependencies on proxy code and can be safely imported at the SDK level.
"""
import math
import time
from dataclasses import dataclass
from pathlib import Path
@ -144,14 +145,29 @@ 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.
"""
staged: Final = _stage_token_file(_without_secret(record))
stamped: Final = _stamped_past_the_login_on_disk(record)
staged: Final = _stage_token_file(_without_secret(stamped))
if isinstance(staged, CredentialNotSaved):
return staged
outcome: Final = SecretStored() if record.key is None else vault.write(_encode_secret(record, record.key))
outcome: Final = SecretStored() if stamped.key is None else vault.write(_encode_secret(stamped, stamped.key))
if isinstance(outcome, SecretStored):
return outcome if _commit_token_file(staged) else CredentialNotRecorded()
discard_staged_json(staged)
return _keep_the_secret_in_the_file(record, outcome)
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.
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.
"""
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)}))
def _keep_the_secret_in_the_file(record: CliTokenRecord, outcome: SecretWrite) -> SecretSave:
@ -331,7 +347,9 @@ def _apply_vault_secret(record: CliTokenRecord, blob: str, vault: SecretVault) -
The sign-in each secret came from decides it, because either store can be the stale one. A
secret is usually left on disk by a keychain that would not take it, which makes the file the
fresher of the two. It is the older one when a login the keychain did take could not replace
the file afterwards, and serving that one would put a superseded credential back in use.
the file afterwards, and serving that one would put a superseded credential back in use. Equal
stamps are one login sitting in both stores, left by a migration whose scrub was refused, so
that branch retries the migration rather than trading one credential for another.
A scrub the file refuses leaves that superseded secret where it lies, which is the state the
login already named when it could not replace the file, and which `lite logout` reports rather

View file

@ -488,6 +488,46 @@ class TestSaveCliToken:
assert path.read_text() == before
assert list(path.parent.glob(".tmp-*")) == []
def test_a_login_is_stamped_past_the_one_it_replaces_even_on_a_clock_that_went_back(
self, isolated_home, secret_vault_factory
):
"""The stamp is what decides the keychain secret against the one on disk, so a login that
carries an earlier wall clock than the login before it must not be filed as the older of
the two."""
_write_legacy_file(isolated_home, key="sk-old", timestamp=2000.0)
vault = secret_vault_factory()
save_cli_token(CliTokenRecord(base_url=SERVER, key="sk-new", timestamp=1000.0), vault=vault)
assert json.loads(vault.blob)["timestamp"] > 2000.0
def test_a_clock_that_went_back_does_not_hand_the_win_to_the_superseded_login(
self, isolated_home, secret_vault_factory
):
"""The disk state a login reports as CredentialNotRecorded: the keychain took the new
secret and the file still holds the previous one. Reading it back has to produce the login
that was just made, and an earlier wall clock is no reason to serve the one it replaced."""
_write_legacy_file(isolated_home, key="sk-superseded", timestamp=2000.0)
vault = secret_vault_factory()
save_cli_token(CliTokenRecord(base_url=SERVER, key="sk-fresh", timestamp=1000.0), vault=vault)
_write_legacy_file(isolated_home, key="sk-superseded", timestamp=2000.0)
assert load_cli_token(vault=vault).key == "sk-fresh"
def test_a_login_on_a_clock_that_moved_forwards_keeps_its_own_time(
self, isolated_home, secret_vault_factory
):
"""Pinning the stamp above the previous login is only ever a floor. The ordinary case has
to record when the user actually signed in, because that is what decides expiry."""
_write_legacy_file(isolated_home, key="sk-old", timestamp=1000.0)
vault = secret_vault_factory()
save_cli_token(CliTokenRecord(base_url=SERVER, key="sk-new", timestamp=2000.0), vault=vault)
assert json.loads(vault.blob)["timestamp"] == 2000.0
assert json.loads(_token_file(isolated_home).read_text())["timestamp"] == 2000.0
class TestScrubFailure:
"""A keychain that took the secret while the file kept it is the worst of both stores: the