diff --git a/docs/subsystems/providers-accounts.md b/docs/subsystems/providers-accounts.md index d5c5d7b8..9a4b52b8 100644 --- a/docs/subsystems/providers-accounts.md +++ b/docs/subsystems/providers-accounts.md @@ -44,7 +44,7 @@ keys and accounts per request is in [Gateway routing and fallback](gateway-routi - Each refresh token has exactly one holder. Vendors rotate tokens on refresh, so two copies of one token would sign each other out. `savedTokenMu` stops two requests refreshing one saved account at once. - A Claude subscription account is used only for Claude Code, through the genuine Claude Code binary (see [Claude subscription bridge](claude-subscription-bridge.md)). - An unreadable `providers.json` is an error, never an empty list. Saving over an empty list would lose every provider. -- When `logins.json` cannot be decoded as saved accounts, reads keep the accounts read before. Before saving a change, `keepUnreadLogins` copies the original file aside, including valid JSON with invalid account field types, so its credentials can be recovered. +- When `logins.json` cannot be decoded as saved accounts, reads keep the accounts read before. Before saving a change, `keepUnreadLogins` copies the original file aside, including valid JSON with invalid account field types, so its credentials can be recovered. If the existing file cannot be read or the backup fails, saving returns the error and leaves the original file unchanged. A missing file can be created normally. - A switched-off provider (`Off`, #163) stays saved. The gateway says it is off instead of saying the model is unknown. - A provider removed or switched off takes its models out of the catalog. [Agent wiring](agent-wiring.md)'s `Reseat` moves agents off them. - A gateway key held to some accounts (#905) is held to those of the providers its list names: a provider it names no account of, the key uses as it always did. The list keeps an account by its stable id — `logins.json`'s `id`, set once written and kept through renames; the agents that keep no logins (and plugins) are known by a hash of the account's name, so an account renamed there drops out of a key's list, holding it closer, never wider. Storing an id for every agent is a larger change, not this one's. diff --git a/internal/provider/logins.go b/internal/provider/logins.go index 53c8e6f7..c27fe782 100644 --- a/internal/provider/logins.go +++ b/internal/provider/logins.go @@ -186,23 +186,28 @@ func readLogins() []savedLogin { } // keepUnreadLogins copies a logins.json that doesn't parse aside before it -// is written over, so the accounts in it can still be got back. -func keepUnreadLogins(path string) { +// is written over, so the accounts in it can still be got back. A failure +// to read or keep an existing file stops the write. +func keepUnreadLogins(path string) error { b, err := os.ReadFile(path) + if errors.Is(err, fs.ErrNotExist) { + return nil + } if err != nil { - return + return err } // Match readLogins: valid JSON can still have unreadable field types. var ls []savedLogin if json.Unmarshal(b, &ls) == nil { - return + return nil } bad := path + ".bad-" + time.Now().Format("20060102-150405") if err := os.WriteFile(bad, b, 0o600); err != nil { log.Printf("logins.json doesn't parse and couldn't be kept: %v", err) - return + return err } log.Printf("logins.json didn't parse; it is kept as %s", filepath.Base(bad)) + return nil } func writeLogins(ls []savedLogin) error { @@ -231,7 +236,9 @@ func writeLogins(ls []savedLogin) error { return err } defer Changed() // an account added, switched or gone: All builds anew - keepUnreadLogins(loginsPath()) + if err := keepUnreadLogins(loginsPath()); err != nil { + return err + } if err := writePrivate(loginsPath(), append(b, '\n')); err != nil { return err } diff --git a/internal/provider/logins_unread_test.go b/internal/provider/logins_unread_test.go index dedd9017..b3431447 100644 --- a/internal/provider/logins_unread_test.go +++ b/internal/provider/logins_unread_test.go @@ -2,13 +2,75 @@ package provider import ( "encoding/json" + "errors" "os" "path/filepath" "slices" "strings" "testing" + "testing/synctest" + "time" ) +// A read or backup that fails must stop a new sign-in from replacing the +// original credentials, even when no accounts have been read before. +func TestUnreadLoginsWriteErrorsKeepCredentials(t *testing.T) { + for _, fault := range []string{"read", "backup"} { + t.Run(fault, func(t *testing.T) { + azureHome(t) + // Freeze the backup's timestamp so an existing directory blocks + // exactly its filename, without depending on the wall clock. + synctest.Test(t, func(t *testing.T) { + p := loginsPath() + original := `[{"agent":"gemini","user":"old@example.com","auth":{"refresh_token":"old-refresh-token"}}]` + if fault == "backup" { + original = strings.TrimSuffix(original, "]") // a file half written + } + if err := os.MkdirAll(filepath.Dir(p), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(p, []byte(original), 0o600); err != nil { + t.Fatal(err) + } + lastLoginsMu.Lock() + lastLogins = nil // a cold start: no cached credentials to recover + lastLoginsMu.Unlock() + blocked := p + if fault == "read" { + if err := os.Chmod(p, 0); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(p, 0o600) }) + if _, err := os.ReadFile(p); err == nil { + t.Skip("the filesystem does not enforce the unreadable mode") + } else if !errors.Is(err, os.ErrPermission) { + t.Fatal(err) + } + } else { + blocked = p + ".bad-" + time.Now().Format("20060102-150405") + if err := os.Mkdir(blocked, 0o700); err != nil { + t.Fatal(err) + } + } + err := addGoogleLogin("gemini", "new@example.com", "", googleAuth{RefreshToken: "new-refresh-token", Project: "p"}) + if chmodErr := os.Chmod(p, 0o600); chmodErr != nil { + t.Fatal(chmodErr) + } + b, readErr := os.ReadFile(p) + if readErr != nil { + t.Fatal(readErr) + } + if string(b) != original { + t.Fatalf("original credentials overwritten after %s failed: sign-in error = %v", fault, err) + } + if err == nil || !strings.Contains(err.Error(), blocked) { + t.Fatalf("sign-in error = %v, want an error naming %s", err, blocked) + } + }) + }) + } +} + // Valid JSON can still be unreadable as saved accounts. On a cold start, // adding an account must keep those original credentials in a backup. func TestUnreadLoginsFieldTypesKeptBeforeWrite(t *testing.T) {