From 45e015e7dc67c75294b054b1395404957358bf2f Mon Sep 17 00:00:00 2001 From: bircni Date: Sun, 18 Oct 2026 17:14:46 +0200 Subject: [PATCH] fix(auth): make logout reject only the credential tea stored Signed-off-by: bircni --- internal/app/auth.go | 12 ++++++------ internal/app/auth_test.go | 2 +- internal/auth/manager.go | 14 ++++++++++---- internal/auth/manager_test.go | 14 ++++++++++---- internal/git/credentials.go | 20 +++++++++----------- internal/git/credentials_test.go | 16 ++++++++++++++-- 6 files changed, 50 insertions(+), 28 deletions(-) diff --git a/internal/app/auth.go b/internal/app/auth.go index f15f26af..2bb08076 100644 --- a/internal/app/auth.go +++ b/internal/app/auth.go @@ -709,7 +709,7 @@ instance, such as a copy of its token that a keychain kept after a push.`, if err != nil { return err } - account, exists := t.host.Users[t.User] + _, exists := t.host.Users[t.User] switch { case len(t.host.Users) == 0: return clierr.Wrap(clierr.Usage, fmt.Errorf("not logged in to %s", t.Host)) @@ -725,14 +725,14 @@ instance, such as a copy of its token that a keychain kept after a push.`, if err != nil { return err } - if err := manager.Logout(t.Host, t.User); err != nil { + removed, err := manager.Logout(t.Host, t.User) + if err != nil { return err } a.done(cmd, "Logged out of %s (%s)", t.Host, t.User) - if !account.Tokenless() { - if err := git.RejectCredentials(cmd.Context(), a.gitProbe(), a.options.cwd, t.Host, t.User); err != nil { - a.warn(cmd, "Could not ask git's credential helpers to forget %s on %s", t.User, t.Host) - } + // Helpers erase only the copies that hold the removed token, not the user's own credentials. + if err := git.RejectCredentials(cmd.Context(), a.gitProbe(), a.options.cwd, t.Host, t.User, removed.AccessToken); err != nil { + a.warn(cmd, "Could not ask git's credential helpers to forget %s on %s", t.User, t.Host) } if s, err := a.settings(); err == nil { if active := s.Hosts.Hosts[t.Host].User; active != "" && active != t.host.User { diff --git a/internal/app/auth_test.go b/internal/app/auth_test.go index c41201f4..8ef198fe 100644 --- a/internal/app/auth_test.go +++ b/internal/app/auth_test.go @@ -777,7 +777,7 @@ func TestLogoutAsksGitToForgetTheAccount(t *testing.T) { deps.Git = runner configureAccounts(t, deps, instance, "alice", "alice", "bob") require.NoError(t, Execute(context.Background(), deps, []string{"auth", "logout", "--yes"})) - require.Equal(t, []string{"credential reject\nprotocol=https\nhost=example.org:3000\nusername=alice\n\n"}, runner.inputs) + require.Equal(t, []string{"credential reject\nprotocol=https\nhost=example.org:3000\nusername=alice\npassword=alice-token\n\n"}, runner.inputs) require.Equal(t, "āœ“ Logged out of "+instance+" (alice)\nāœ“ Switched active account for "+instance+" to bob\n", withoutANSI(diagnostics.String())) err := Execute(context.Background(), deps, []string{"auth", "logout", "carol", "--yes"}) require.EqualError(t, err, `account "carol" is not logged in on `+instance+" (accounts: bob)") diff --git a/internal/auth/manager.go b/internal/auth/manager.go index 67e574c2..f34cb1a1 100644 --- a/internal/auth/manager.go +++ b/internal/auth/manager.go @@ -98,10 +98,11 @@ func (m Manager) register(instance, user string, account settings.Account, optio }) } -// Logout removes one account and restores its credential if publication fails. -// An unreadable credential does not prevent removing the account. -func (m Manager) Logout(instance, user string) error { - return m.settings.Transaction(settings.TargetHosts, func(s *settings.Snapshot) (func() error, error) { +// Logout removes one account, restores its credential if publication fails and returns the +// removed credential. An unreadable credential does not prevent removing the account. +func (m Manager) Logout(instance, user string) (Credential, error) { + var removed Credential + err := m.settings.Transaction(settings.TargetHosts, func(s *settings.Snapshot) (func() error, error) { host := s.Hosts.Hosts[instance] account, exists := host.Users[user] if !exists { @@ -111,6 +112,7 @@ func (m Manager) Logout(instance, user string) error { if !account.Tokenless() { credential, err := m.credentials.Load(instance, user, account.CredentialStore) if err == nil { + removed = credential // Preserve a compensating copy until the account file is successfully published. restore = func() error { return m.credentials.Save(instance, user, account.CredentialStore, credential) } } @@ -132,4 +134,8 @@ func (m Manager) Logout(instance, user string) error { s.Hosts.Hosts[instance] = host return restore, nil }) + if err != nil { + return Credential{}, err + } + return removed, nil } diff --git a/internal/auth/manager_test.go b/internal/auth/manager_test.go index 8301330e..030e3900 100644 --- a/internal/auth/manager_test.go +++ b/internal/auth/manager_test.go @@ -70,7 +70,9 @@ func TestRegisterAndLogoutIsolateAccounts(t *testing.T) { for _, user := range []string{"alice", "bob", "charlie"} { require.NoError(t, manager.Register("https://example.org", user, "keyring", "token", HostOptions{}, Credential{AccessToken: "secret-" + user})) } - require.NoError(t, manager.Logout("https://example.org", "charlie")) + removed, err := manager.Logout("https://example.org", "charlie") + require.NoError(t, err) + require.Equal(t, "secret-charlie", removed.AccessToken) s, err := config.Load() require.NoError(t, err) require.Empty(t, s.Hosts.Hosts["https://example.org"].User) @@ -154,14 +156,17 @@ func TestLogoutRemovesAccountWithMissingCredential(t *testing.T) { manager := newTestManager(t, config, &Store{Dir: config.Dir, Keyring: keyring}) require.NoError(t, manager.Register("https://example.org", "alice", settings.StoreKeyring, settings.AuthToken, HostOptions{}, Credential{AccessToken: "secret"})) clear(keyring.values) - require.NoError(t, manager.Logout("https://example.org", "alice")) + removed, err := manager.Logout("https://example.org", "alice") + require.NoError(t, err) + require.Empty(t, removed.AccessToken) s, err := config.Load() require.NoError(t, err) require.Empty(t, s.Hosts.Hosts["https://example.org"].Users) require.NoError(t, manager.Register("https://example.org", "alice", settings.StoreKeyring, settings.AuthToken, HostOptions{}, Credential{AccessToken: "secret"})) // An unavailable keyring keeps the account rather than orphaning its token. keyring.fail = true - require.Error(t, manager.Logout("https://example.org", "alice")) + _, err = manager.Logout("https://example.org", "alice") + require.Error(t, err) s, err = config.Load() require.NoError(t, err) require.Contains(t, s.Hosts.Hosts["https://example.org"].Users, "alice") @@ -175,7 +180,8 @@ func TestRegisterHTTPSignRejectsTokenAccounts(t *testing.T) { s, err := config.Load() require.NoError(t, err) require.Equal(t, settings.Account{AuthMethod: settings.AuthSSH, SSHKey: "/keys/id"}, s.Hosts.Hosts["https://example.org"].Users["alice"]) - require.NoError(t, manager.Logout("https://example.org", "alice")) + _, err = manager.Logout("https://example.org", "alice") + require.NoError(t, err) } func TestRegisterReplacesAnAccountOnlyWhenAsked(t *testing.T) { diff --git a/internal/git/credentials.go b/internal/git/credentials.go index ef4594a4..be067b17 100644 --- a/internal/git/credentials.go +++ b/internal/git/credentials.go @@ -153,25 +153,23 @@ func (s System) Input(ctx context.Context, dir, input string, args ...string) (s return s.Output(ctx, dir, true, args...) } -// RejectCredentials asks Git's credential helpers to erase the credentials they stored for user on -// instance, such as a copy of tea's token that another helper kept after a push. Runners that cannot -// send input reject nothing. -func RejectCredentials(ctx context.Context, runner Runner, dir, instance, user string) error { +// RejectCredentials asks Git's credential helpers to erase the copies of token they stored for user on +// instance, such as one that a keychain kept after a push. Helpers keep entries with another password, +// such as the user's own. Runners that cannot send input reject nothing. +func RejectCredentials(ctx context.Context, runner Runner, dir, instance, user, token string) error { input, ok := runner.(InputRunner) - if !ok { + if !ok || user == "" || token == "" { return nil } u, err := url.Parse(instance) if err != nil || u.Host == "" { return fmt.Errorf("invalid instance URL %q", instance) } - if strings.ContainsAny(user, "\x00\r\n") { - return errors.New("invalid account name") - } - request := "protocol=" + u.Scheme + "\nhost=" + u.Host + "\n" - if user != "" { - request += "username=" + user + "\n" + if strings.ContainsAny(user+token, "\x00\r\n") { + return errors.New("invalid account name or token") } + // No path: copies are stored per repository when useHttpPath is set, and a path must match exactly. + request := "protocol=" + u.Scheme + "\nhost=" + u.Host + "\nusername=" + user + "\npassword=" + token + "\n" _, err = input.Input(ctx, dir, request+"\n", "credential", "reject") return err } diff --git a/internal/git/credentials_test.go b/internal/git/credentials_test.go index fdcc9afc..2395d105 100644 --- a/internal/git/credentials_test.go +++ b/internal/git/credentials_test.go @@ -102,11 +102,23 @@ func TestRejectCredentialsAsksHelpersToEraseTheAccount(t *testing.T) { runner := System{} _, err := runner.Output(ctx, home, false, "config", "--global", "credential.helper", "!f() { echo \"op=$1\" >> '"+log+"'; cat >> '"+log+"'; }; f") require.NoError(t, err) + store := filepath.Join(home, "git-credentials") + _, err = runner.Output(ctx, home, false, "config", "--global", "--add", "credential.helper", "store --file="+store) + require.NoError(t, err) + own := "http://alice:own-password@localhost:3001\n" + require.NoError(t, os.WriteFile(store, []byte(own+"http://alice:tea-token@localhost:3001/gitea/alice/project.git\n"), 0o600)) require.Empty(t, HelperScope(ctx, runner, home, "http://localhost:3001")) - require.NoError(t, RejectCredentials(ctx, runner, home, "http://localhost:3001/gitea", "alice")) + require.NoError(t, RejectCredentials(ctx, runner, home, "http://localhost:3001/gitea", "alice", "tea-token")) data, err := os.ReadFile(log) require.NoError(t, err) - require.Equal(t, "op=erase\nprotocol=http\nhost=localhost:3001\nusername=alice\n", string(data)) + require.Equal(t, "op=erase\nprotocol=http\nhost=localhost:3001\nusername=alice\npassword=tea-token\n", string(data)) + data, err = os.ReadFile(store) + require.NoError(t, err) + require.Equal(t, own, string(data), "only the copy of tea's token is erased") + require.NoError(t, RejectCredentials(ctx, runner, home, "http://localhost:3001", "alice", "")) + data, err = os.ReadFile(store) + require.NoError(t, err) + require.Equal(t, own, string(data), "without a token nothing is erased") executable, err := os.Executable() require.NoError(t, err) require.NoError(t, SetupCredentials(ctx, runner, CredentialSetup{Directory: home, Instance: "http://localhost:3001", Executable: executable}))