fix(oauth): store keyring tokens as one entry per provider

internal/oauth/flow.go

@@ -167,20 +167,25 @@ func Refresh(ctx context.Context, client *http.Client, cfg Config, current Token
 	if trimmed(cfg.TokenEndpoint) == "" {
 		return Token{}, errors.New("oauth: no token endpoint configured for refresh")
 	}
+	// Prefer the scopes the current token was issued with; fall back to the
+	// configured defaults only when the stored token has none. The same set is
+	// sent on the wire and kept as the base so a response that omits scope
+	// cannot report a different grant than the provider just processed.
+	scopes := current.Scopes
+	if len(scopes) == 0 {
+		scopes = cfg.Scopes
+	}
 	form := url.Values{}
 	form.Set("grant_type", "refresh_token")
 	form.Set("refresh_token", refresh)
 	form.Set("client_id", cfg.ClientID)
 	if secret := trimmed(cfg.ClientSecret); secret != "" {
 		form.Set("client_secret", secret)
 	}
-	if len(cfg.Scopes) > 0 {
-		form.Set("scope", strings.Join(cfg.Scopes, " "))
+	if len(scopes) > 0 {
+		form.Set("scope", strings.Join(scopes, " "))
 	}
-	// Carry the existing token_type forward: a refresh response commonly omits it,
-	// and PostToken only overwrites TokenType when the response supplies one, so
-	// without seeding it here the type would be silently lost across refreshes (L15).
-	base := Token{Scopes: current.Scopes, RefreshToken: refresh, Account: current.Account, IDToken: current.IDToken, TokenType: current.TokenType}
+	base := Token{Scopes: scopes, RefreshToken: refresh, Account: current.Account, IDToken: current.IDToken, TokenType: current.TokenType}
 	return PostToken(ctx, client, cfg.TokenEndpoint, form, base, now)
 }
 

internal/oauth/flow_test.go

@@ -299,3 +299,66 @@ func TestRefreshPreservesTokenTypeWhenOmitted(t *testing.T) {
 		t.Fatalf("refresh should carry the existing token_type forward, got %q", tok.TokenType)
 	}
 }
+
+func TestRefreshPreservesScopesWhenOmitted(t *testing.T) {
+	var gotScope string
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_ = r.ParseForm()
+		gotScope = r.FormValue("scope")
+		_, _ = w.Write([]byte(`{"access_token":"new-at","expires_in":3600}`)) // no scope in response
+	}))
+	defer server.Close()
+	cfg := Config{ClientID: "c", TokenEndpoint: server.URL, Scopes: []string{"fallback-scope"}}
+	tok, err := Refresh(context.Background(), server.Client(), cfg, Token{RefreshToken: "keep-me", Scopes: []string{"custom-scope"}}, nil)
+	if err != nil {
+		t.Fatalf("Refresh: %v", err)
+	}
+	if gotScope != "custom-scope" {
+		t.Fatalf("refresh form scope = %q, want current token scopes", gotScope)
+	}
+	if len(tok.Scopes) != 1 || tok.Scopes[0] != "custom-scope" {
+		t.Fatalf("refresh should carry existing scopes forward, got %v", tok.Scopes)
+	}
+}
+
+func TestRefreshUsesConfigScopesWhenTokenHasNone(t *testing.T) {
+	var gotScope string
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_ = r.ParseForm()
+		gotScope = r.FormValue("scope")
+		_, _ = w.Write([]byte(`{"access_token":"new-at","expires_in":3600}`))
+	}))
+	defer server.Close()
+	cfg := Config{ClientID: "c", TokenEndpoint: server.URL, Scopes: []string{"fallback-scope"}}
+	tok, err := Refresh(context.Background(), server.Client(), cfg, Token{RefreshToken: "keep-me"}, nil)
+	if err != nil {
+		t.Fatalf("Refresh: %v", err)
+	}
+	if gotScope != "fallback-scope" {
+		t.Fatalf("refresh form scope = %q, want cfg.Scopes fallback", gotScope)
+	}
+	if len(tok.Scopes) != 1 || tok.Scopes[0] != "fallback-scope" {
+		t.Fatalf("refresh should use cfg scopes when token has none, got %v", tok.Scopes)
+	}
+}
+
+func TestRefreshFailsOnHTTPError(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(http.StatusBadRequest)
+		_, _ = w.Write([]byte(`{"error":"invalid_grant","error_description":"token expired"}`))
+	}))
+	defer server.Close()
+	cfg := Config{ClientID: "c", TokenEndpoint: server.URL}
+	_, err := Refresh(context.Background(), server.Client(), cfg, Token{RefreshToken: "expired-rt"}, nil)
+	if err == nil {
+		t.Fatal("expected refresh failure on HTTP 400")
+	}
+}
+
+func TestRefreshRequiresRefreshToken(t *testing.T) {
+	cfg := Config{ClientID: "c", TokenEndpoint: "https://example.com/token"}
+	_, err := Refresh(context.Background(), nil, cfg, Token{AccessToken: "only-access-token"}, nil)
+	if err == nil {
+		t.Fatal("expected error when token has no refresh token")
+	}
+}

internal/oauth/lock.go

@@ -1,7 +1,6 @@
 package oauth
 
 import (
-	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -11,27 +10,47 @@ import (
 	"github.com/Gitlawb/zero/internal/lockutil"
 )
 
-const (
-	fileLockTimeout    = 5 * time.Second
+// Lock timing knobs (vars so tests can shorten absolute ceilings without
+// changing production defaults).
+var (
+	// fileLockTimeout is how long acquisition waits after the last sign of a
+	// healthy holder (or when the lock path cannot be stated). A multi-entry
+	// keyring pass can legitimately run several 10s OS commands while refreshing
+	// the lease; contenders must not give up while that lease stays healthy.
+	// While the holder's mtime stays within fileLockStaleAfter, this idle
+	// deadline is extended so a fixed 5s window cannot fail a healthy peer.
+	fileLockTimeout = 5 * time.Second
+	// fileLockStaleAfter is how old a lock file's mtime must be before a waiter
+	// may reclaim it as abandoned. Must stay above one keyring command timeout
+	// plus lease refresh slack (holders refresh every fileLockRefreshInterval).
 	fileLockStaleAfter = 30 * time.Second
 )
 
 var lockSeq atomic.Uint64
 
 // acquireFileLock takes a cross-process exclusive lock by creating lockPath with
-// O_EXCL. It retries with a short backoff until a timeout, breaking a lock whose
-// file is older than fileLockStaleAfter (so a crashed holder cannot deadlock the
-// store). Release is ownership-aware: it removes the lock only if it still holds
-// our token, so a stale-broken holder cannot delete a newer holder's lock.
-func acquireFileLock(lockPath string, now func() time.Time) (func(), error) {
+// O_EXCL. It retries with a short backoff while a live holder's lease remains
+// healthy (mtime refreshed within fileLockStaleAfter), reclaiming only a lock
+// older than that threshold so a crashed holder cannot deadlock the store.
+// Release is ownership-aware: it removes the lock only if it still holds our
+// token, so a stale-broken holder cannot delete a newer holder's lock.
+// The returned token is the contents written into the lock file; lease refresh
+// must re-check it before touching mtime so a replaced holder cannot keep a
+// thief's lock alive.
+//
+// Timing always uses the real wall clock, never the now parameter: now is
+// StoreOptions.Now, which callers may legitimately fix (e.g. a test or an
+// embedded clock). Measuring the deadline with that clock would either never
+// fire (fixed clock) or diverge from the mtime lease stamps (wall-clock).
+func acquireFileLock(lockPath string, now func() time.Time) (unlock func() error, token string, err error) {
 	if now == nil {
 		now = time.Now
 	}
 	if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil {
-		return nil, err
+		return nil, "", err
 	}
-	token := fmt.Sprintf("%d-%d-%d", os.Getpid(), now().UnixNano(), lockSeq.Add(1))
-	deadline := now().Add(fileLockTimeout)
+	token = fmt.Sprintf("%d-%d-%d", os.Getpid(), now().UnixNano(), lockSeq.Add(1))
+	idleDeadline := time.Now().Add(fileLockTimeout)
 	for {
 		f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600)
 		if err == nil {
@@ -40,57 +59,93 @@ func acquireFileLock(lockPath string, now func() time.Time) (func(), error) {
 			// other processes. Fail closed: remove the file and surface the error.
 			if _, werr := f.WriteString(token); werr != nil {
 				_ = f.Close()
-				_ = lockutil.RemoveLockFile(lockPath)
-				return nil, fmt.Errorf("oauth: write token lock: %w", werr)
+				if rerr := lockutil.RemoveLockFile(lockPath); rerr != nil {
+					return nil, "", fmt.Errorf("oauth: write token lock: %w; cleanup: %v", werr, rerr)
+				}
+				return nil, "", fmt.Errorf("oauth: write token lock: %w", werr)
 			}
 			if cerr := f.Close(); cerr != nil {
-				_ = lockutil.RemoveLockFile(lockPath)
-				return nil, fmt.Errorf("oauth: close token lock: %w", cerr)
+				if rerr := lockutil.RemoveLockFile(lockPath); rerr != nil {
+					return nil, "", fmt.Errorf("oauth: close token lock: %w; cleanup: %v", cerr, rerr)
+				}
+				return nil, "", fmt.Errorf("oauth: close token lock: %w", cerr)
 			}
 			var released bool
-			return func() {
+			return func() error {
 				if released {
-					return
+					return nil
 				}
 				released = true
 				if data, rerr := os.ReadFile(lockPath); rerr == nil && string(data) == token {
-					_ = lockutil.RemoveLockFile(lockPath)
+					if err := lockutil.RemoveLockFile(lockPath); err != nil {
+						return fmt.Errorf("oauth: release token lock %s: %w", filepath.Base(lockPath), err)
+					}
 				}
-			}, nil
+				return nil
+			}, token, nil
 		}
 		// On Windows a concurrent holder's os.Remove leaves the lock file in a
 		// "delete pending" state, so an O_EXCL create races it with
-		// ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist. Treat that
-		// as contention and retry, exactly like ErrExist — otherwise the lock
-		// spuriously fails under concurrency on Windows.
-		if !errors.Is(err, os.ErrExist) && !errors.Is(err, os.ErrPermission) {
-			return nil, fmt.Errorf("oauth: acquire token lock: %w", err)
+		// ERROR_ACCESS_DENIED (os.ErrPermission) rather than ErrExist. The
+		// platform predicate recognizes that as contention (and returns
+		// unrelated permission errors immediately) instead of failing the lock
+		// spuriously under concurrency.
+		if !isLockCreateContention(err) {
+			return nil, "", fmt.Errorf("oauth: acquire token lock: %w", err)
 		}
 		// Reclaim a stale lock left by a crashed holder — atomically (H3). A blind
 		// Remove lets two racers both reclaim + recreate and so both hold the lock;
 		// reclaimStaleLock renames the file aside (only one rename wins) and restores
 		// it if it turns out fresh, so a live lock is never deleted out from under it.
-		if info, statErr := os.Stat(lockPath); statErr == nil && time.Since(info.ModTime()) > fileLockStaleAfter {
-			cleared, rerr := lockutil.ReclaimStaleLock(lockPath, token, func(reclaimedPath string) bool {
-				info, err := os.Stat(reclaimedPath)
-				return err == nil && time.Since(info.ModTime()) <= fileLockStaleAfter
-			})
-			if rerr != nil {
-				// Reclaim hit a hard failure: the rename aside failed outright, or a
-				// live holder's lock could not be put back (the lock path may be
-				// missing, so re-acquiring would break mutual exclusion). Fail closed
-				// instead of spinning to the deadline.
-				return nil, fmt.Errorf("oauth: reclaim stale token lock: %w", rerr)
-			}
-			if cleared {
-				continue
+		if info, statErr := os.Stat(lockPath); statErr == nil {
+			age := time.Since(info.ModTime())
+			// Future mtimes (clock skew, hostile Chtimes) are not healthy leases:
+			// age is negative, so neither the reclaim branch nor the deadline
+			// extension below treats them as live. Contenders time out instead of
+			// waiting forever on a never-stale lock.
+			if age > fileLockStaleAfter {
+				cleared, rerr := lockutil.ReclaimStaleLock(lockPath, token, func(reclaimedPath string) bool {
+					info, err := os.Stat(reclaimedPath)
+					if err != nil {
+						return false
+					}
+					reclaimedAge := time.Since(info.ModTime())
+					return reclaimedAge >= 0 && reclaimedAge <= fileLockStaleAfter
+				})
+				if rerr != nil {
+					// Reclaim hit a hard failure: the rename aside failed outright, or a
+					// live holder's lock could not be put back (the lock path may be
+					// missing, so re-acquiring would break mutual exclusion). Fail closed
+					// instead of spinning to the deadline.
+					return nil, "", fmt.Errorf("oauth: reclaim stale token lock: %w", rerr)
+				}
+				if cleared {
+					continue
+				}
+				// Lost the reclaim race, or isLive reported a still-fresh holder
+				// (callback true → ReclaimStaleLock restores and returns false).
+				// Refresh the idle deadline so reclaim work that overran the prior
+				// window does not immediately time out a healthy peer.
+				idleDeadline = time.Now().Add(fileLockTimeout)
+			} else if age >= 0 {
+				// Holder looks healthy (lease refreshed recently, mtime not in the
+				// future). Keep waiting for the critical section to finish rather
+				// than timing out after a fixed window shorter than a legitimate
+				// multi-entry keyring pass.
+				idleDeadline = time.Now().Add(fileLockTimeout)
 			}
-			// Lost the reclaim race (or it was actually fresh) — fall through to the
-			// bounded wait rather than hot-spinning on a reclaim that never wins.
 		}
-		if now().After(deadline) {
-			return nil, fmt.Errorf("oauth: timed out acquiring token lock %s", filepath.Base(lockPath))
+		if time.Now().After(idleDeadline) {
+			return nil, "", fmt.Errorf("oauth: timed out acquiring token lock %s", filepath.Base(lockPath))
 		}
 		time.Sleep(10 * time.Millisecond)
 	}
 }
+
+// ownLockFile reports whether path still holds token. Used by lease refresh so
+// a holder that was reclaimed after a long pause cannot Chtimes a replacement
+// lock and keep two critical sections alive.
+func ownLockFile(path, token string) bool {
+	data, err := os.ReadFile(path)
+	return err == nil && string(data) == token
+}

internal/oauth/lock_owner_unix.go

@@ -0,0 +1,41 @@
+//go:build !windows
+
+package oauth
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"syscall"
+)
+
+// isLockCreateContention reports whether a failed O_EXCL lock create should be
+// treated as contention (another holder's lock exists) rather than a hard
+// error. On non-Windows platforms the only contention errno is EEXIST
+// (os.ErrExist); EACCES (os.ErrPermission) is a genuine permission failure and
+// must surface immediately rather than spin to the lock timeout.
+func isLockCreateContention(err error) bool {
+	return errors.Is(err, os.ErrExist)
+}
+
+// checkOAuthLockDirOwner rejects a fallback lock directory not owned by the
+// current user: on a shared temp root another user could have pre-created the
+// path and would then control its lifetime (deletion/renaming), permanently
+// denying OAuth keyring operations.
+func checkOAuthLockDirOwner(info os.FileInfo) error {
+	stat, ok := info.Sys().(*syscall.Stat_t)
+	if !ok {
+		return errors.New("oauth lock fallback directory ownership metadata unavailable")
+	}
+	if int(stat.Uid) != os.Geteuid() {
+		return fmt.Errorf("oauth lock fallback directory is owned by uid %d, not the current user", stat.Uid)
+	}
+	return nil
+}
+
+// identityLockRoot is never called on non-Windows: keyringFallbackLockDir
+// resolves the uid-anchored home and the fixed /tmp root directly. It exists so
+// the shared fallback can name one helper without a build-tagged call site.
+func identityLockRoot() (string, error) {
+	return "", fmt.Errorf("oauth: identityLockRoot is windows-only")
+}

internal/oauth/lock_owner_windows.go

@@ -0,0 +1,54 @@
+//go:build windows
+
+package oauth
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"path/filepath"
+
+	"golang.org/x/sys/windows"
+)
+
+// isLockCreateContention reports whether a failed O_EXCL lock create should be
+// treated as contention (another holder's lock exists) rather than a hard
+// error. On Windows, a concurrent holder's os.Remove leaves the lock file in a
+// "delete pending" state, so the O_EXCL create races it with
+// ERROR_ACCESS_DENIED (os.ErrPermission) rather than os.ErrExist; both are
+// contention here.
+func isLockCreateContention(err error) bool {
+	return errors.Is(err, os.ErrExist) || errors.Is(err, os.ErrPermission)
+}
+
+// checkOAuthLockDirOwner is a no-op on Windows: identityLockRoot resolves a
+// per-user location from the process token rather than a shared root, so there
+// is no co-tenant directory to validate ownership of.
+func checkOAuthLockDirOwner(os.FileInfo) error {
+	return nil
+}
+
+// identityLockRoot resolves the last-resort keyring lock directory from the
+// caller's own identity instead of the environment. os.TempDir() is not usable
+// here for the same reason the Unix branch refuses it: it resolves %TMP%, then
+// %TEMP%, then %USERPROFILE%, and the first two are launcher-controlled, so two
+// processes of one user can compute different lock paths while writing the same
+// fixed keyring account and race it. Being per-user by default is not the
+// property that matters; being stable for that user is.
+//
+// SHGetKnownFolderPath reads the user's profile location through the process
+// token, so it ignores %LOCALAPPDATA% and every other temp override. Failing
+// closed is deliberate: without a stable identity there is no lock path two
+// processes are guaranteed to agree on, and a guessed one silently reintroduces
+// the race it is meant to prevent.
+func identityLockRoot() (string, error) {
+	base, err := windows.KnownFolderPath(windows.FOLDERID_LocalAppData, 0)
+	if err != nil {
+		return "", fmt.Errorf("resolve LocalAppData for keyring lock: %w", err)
+	}
+	dir := filepath.Join(base, "zero", "oauth-locks")
+	if err := os.MkdirAll(dir, 0o700); err != nil {
+		return "", err
+	}
+	return dir, nil
+}