Skip to content

Commit 80e5c48

Browse files
toothbrushclaude
andcommitted
auth: persist refresh token before access token on login
RecordLoginContext wrote the access token before the paired refresh token, the same hazardous ordering fixed in contextTokenStore.SaveTokens. If the refresh write failed, the keyring could hold a fresh access JWT paired with a stale refresh token from an earlier login; on access-token expiry that dead refresh token forces a re-login. Write the refresh slot first (and the else-branch stale-token delete), so a failed refresh write aborts before the access token is touched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 245a777 commit 80e5c48

2 files changed

Lines changed: 51 additions & 5 deletions

File tree

cmd/entire/cli/auth/contexts.go

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,15 +70,17 @@ func RecordLoginContext(rawToken, refreshToken string, activate bool) (string, e
7070
}
7171
}
7272

73-
encoded := tokenstore.EncodeTokenWithExpiration(rawToken, expiresIn)
74-
if err := tokenstore.Set(keychainService, handle, encoded); err != nil {
75-
return "", fmt.Errorf("store login token in keyring: %w", err)
76-
}
77-
7873
// The refresh token lives in the paired "<service>:refresh" slot (raw,
7974
// no expiry suffix). Clear any prior one when this login carries none,
8075
// so a stale token from an earlier session can't later be replayed
8176
// against the server's single-use rotation and revoke the family.
77+
//
78+
// Write the refresh slot BEFORE the access token, matching
79+
// contextTokenStore.SaveTokens: a partial write must never leave a fresh
80+
// access token paired with a stale refresh token left over from an
81+
// earlier login. Refresh-first means a failed refresh write aborts before
82+
// the access token is touched (old pair preserved), rather than committing
83+
// a new access JWT against a dead refresh token.
8284
refreshSlot := tokenstore.RefreshService(keychainService)
8385
if refreshToken != "" {
8486
if err := tokenstore.Set(refreshSlot, handle, refreshToken); err != nil {
@@ -88,6 +90,11 @@ func RecordLoginContext(rawToken, refreshToken string, activate bool) (string, e
8890
_ = tokenstore.Delete(refreshSlot, handle) //nolint:errcheck // best-effort cleanup of a stale refresh token
8991
}
9092

93+
encoded := tokenstore.EncodeTokenWithExpiration(rawToken, expiresIn)
94+
if err := tokenstore.Set(keychainService, handle, encoded); err != nil {
95+
return "", fmt.Errorf("store login token in keyring: %w", err)
96+
}
97+
9198
var name string
9299
cfgDir := contexts.DefaultConfigDir()
93100
if modErr := contexts.Modify(cfgDir, func(f *contexts.File) (bool, error) {

cmd/entire/cli/auth/contexts_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,45 @@ func makeJWT(t *testing.T, payloadJSON string) string {
2525
return header + "." + payload + "." + enc.EncodeToString([]byte("sig"))
2626
}
2727

28+
// RecordLoginContext must persist the refresh token before the access token,
29+
// so a failed access write never commits a fresh access JWT against a stale
30+
// refresh token left over from an earlier login.
31+
func TestRecordLoginContext_RefreshFirstOrdering(t *testing.T) {
32+
cfgDir := t.TempDir()
33+
t.Setenv("ENTIRE_CONFIG_DIR", cfgDir)
34+
35+
const coreURL = "https://core.example.com"
36+
const handle = "alice"
37+
svc := tokenstore.CoreKeyringService(coreURL)
38+
path := filepath.Join(t.TempDir(), "tokens.json")
39+
40+
// A prior login left a stale refresh token in the slot.
41+
seedRestore := tokenstore.UseFileBackendForTesting(path)
42+
if err := tokenstore.Set(tokenstore.RefreshService(svc), handle, "entr_stale"); err != nil {
43+
t.Fatalf("seed stale refresh: %v", err)
44+
}
45+
seedRestore()
46+
47+
// Fail the access-token write only.
48+
failAccess := func(service, _ string) bool { return service == svc }
49+
restore := tokenstore.UseFailingBackendForTesting(path, failAccess)
50+
t.Cleanup(restore)
51+
52+
exp := time.Now().Add(2 * time.Hour).Unix()
53+
token := makeJWT(t, fmt.Sprintf(`{"iss":%q,"handle":%q,"exp":%d}`, coreURL, handle, exp))
54+
if _, err := RecordLoginContext(token, "entr_login_new", true); err == nil {
55+
t.Fatal("RecordLoginContext: want error when access write fails")
56+
}
57+
// The refresh token must already be the new one (written first), and no
58+
// access token may sit alongside the stale refresh token.
59+
if r, _ := tokenstore.Get(tokenstore.RefreshService(svc), handle); r != "entr_login_new" { //nolint:errcheck // read-back
60+
t.Fatalf("refresh slot = %q, want entr_login_new persisted before the access write", r)
61+
}
62+
if v, err := tokenstore.Get(svc, handle); !errors.Is(err, tokenstore.ErrNotFound) {
63+
t.Fatalf("access slot = %q (err=%v); a fresh access token must not be committed when its write failed", v, err)
64+
}
65+
}
66+
2867
func TestRecordLoginContext_WritesContextAndToken(t *testing.T) {
2968
// Sets ENTIRE_CONFIG_DIR and swaps the keyring backend — process-global
3069
// state, so this test cannot run in parallel.

0 commit comments

Comments
 (0)