From aad12c7e7180a09e8c62212234ed08a14340a2f8 Mon Sep 17 00:00:00 2001 From: Vito Meznaric Date: Tue, 29 Sep 2026 12:44:33 +0200 Subject: [PATCH] Fix a login race condition --- go.mod | 5 ++- go.sum | 12 ++++++ internal/auth/auth_test.go | 50 +++++++++++++++++++++++- internal/auth/login.go | 79 +++++++++++++++++++++++--------------- internal/auth/store.go | 41 +++++++++++++++++--- internal/cmd/root.go | 2 +- 6 files changed, 150 insertions(+), 39 deletions(-) diff --git a/go.mod b/go.mod index 6343a30..bcf9f99 100644 --- a/go.mod +++ b/go.mod @@ -3,8 +3,11 @@ module github.com/webshare-proxy/webshare-cli go 1.23.0 require ( + github.com/gofrs/flock v0.12.1 github.com/spf13/cobra v1.10.2 github.com/webshare-proxy/webshare-go v0.0.0-20260728085640-4f665a81a4e7 + github.com/zalando/go-keyring v0.2.8 + golang.org/x/oauth2 v0.30.0 ) require ( @@ -12,7 +15,5 @@ require ( github.com/godbus/dbus/v5 v5.2.2 // indirect github.com/inconshreveable/mousetrap v1.1.0 // indirect github.com/spf13/pflag v1.0.9 // indirect - github.com/zalando/go-keyring v0.2.8 // indirect - golang.org/x/oauth2 v0.30.0 // indirect golang.org/x/sys v0.27.0 // indirect ) diff --git a/go.sum b/go.sum index 1ab544f..6fcbf2b 100644 --- a/go.sum +++ b/go.sum @@ -1,15 +1,25 @@ github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= github.com/danieljoos/wincred v1.2.3 h1:v7dZC2x32Ut3nEfRH+vhoZGvN72+dQ/snVXo/vMFLdQ= github.com/danieljoos/wincred v1.2.3/go.mod h1:6qqX0WNrS4RzPZ1tnroDzq9kY3fu1KwE7MRLQK4X0bs= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/godbus/dbus/v5 v5.2.2 h1:TUR3TgtSVDmjiXOgAAyaZbYmIeP3DPkld3jgKGV8mXQ= github.com/godbus/dbus/v5 v5.2.2/go.mod h1:3AAv2+hPq5rdnr5txxxRwiGjPXamgoIHgz9FPBfOp3c= +github.com/gofrs/flock v0.12.1 h1:MTLVXXHf8ekldpJk3AKicLij9MdwOWkZ+a/jHHZby9E= +github.com/gofrs/flock v0.12.1/go.mod h1:9zxTsyu5xtJ9DK+1tFZyibEV7y3uwDxPPfbxeeHCoD0= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM= github.com/spf13/cobra v1.10.2 h1:DMTTonx5m65Ic0GOoRY2c16WCbHxOOw6xxezuLaBpcU= github.com/spf13/cobra v1.10.2/go.mod h1:7C1pvHqHw5A4vrJfjNwvOdzYu0Gml16OCs2GRiTUUS4= github.com/spf13/pflag v1.0.9 h1:9exaQaMOCwffKiiiYk6/BndUBv+iRViNW+4lEMi0PvY= github.com/spf13/pflag v1.0.9/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg= +github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= +github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= +github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= +github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= github.com/webshare-proxy/webshare-go v0.0.0-20260728085640-4f665a81a4e7 h1:6mmoTlwFIOGvQXf3AQ7jXrcolz9QaeX6RJ/Ikv0e3vY= github.com/webshare-proxy/webshare-go v0.0.0-20260728085640-4f665a81a4e7/go.mod h1:WrMcbXqIzvTEWoacXEIpnALevumOg6rNaYuzp9tpdHA= github.com/zalando/go-keyring v0.2.8 h1:6sD/Ucpl7jNq10rM2pgqTs0sZ9V3qMrqfIIy5YPccHs= @@ -20,3 +30,5 @@ golang.org/x/oauth2 v0.30.0/go.mod h1:B++QgG3ZKulg6sRPGD/mqlHQs5rB3Ml9erfeDY7xKl golang.org/x/sys v0.27.0 h1:wBqf8DvsY9Y/2P8gAfPDEYNuS30J4lPHJxXSb/nJZ+s= golang.org/x/sys v0.27.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index dd605c2..dbd68bf 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" "time" @@ -88,6 +89,11 @@ func newFakeAS(t *testing.T) *fakeAS { } as.grantType = r.PostForm.Get("grant_type") if as.grantType == "refresh_token" { + if r.PostForm.Get("refresh_token") != "refresh-1" || as.refreshed > 0 { + w.WriteHeader(http.StatusBadRequest) + writeJSON(w, map[string]any{"error": "invalid_grant"}) + return + } as.refreshed++ writeJSON(w, map[string]any{ "access_token": "access-2", "refresh_token": "refresh-2", @@ -220,7 +226,7 @@ func TestExpiredAccessTokenIsRefreshedAndTheRotatedPairIsStored(t *testing.T) { t.Fatalf("Save: %v", err) } - source := NewTokenSource(context.Background(), as.config(t), expired) + source := NewTokenSource(as.config(t), expired) token, err := source.Token(context.Background()) if err != nil { t.Fatalf("Token: %v", err) @@ -238,6 +244,48 @@ func TestExpiredAccessTokenIsRefreshedAndTheRotatedPairIsStored(t *testing.T) { } } +func TestCommandsRunningSideBySideRefreshOnlyOnce(t *testing.T) { + isolate(t) + // The file store, because it is what separate processes share; the mock + // keyring lives in this process's memory. + keyring.MockInitWithError(keyring.ErrUnsupportedPlatform) + as := newFakeAS(t) + expired := Credentials{ + AccessToken: "access-1", RefreshToken: "refresh-1", + Expiry: time.Now().Add(-time.Hour), Issuer: as.issuer, + TokenEndpoint: as.server.URL + "/oauth/token/", + } + if err := Save(as.server.URL, &expired); err != nil { + t.Fatalf("Save: %v", err) + } + + // Each source stands in for a separate command: its own copy of what was + // stored when it started, and nothing in memory shared with the other. + tokens := make([]string, 2) + failures := make([]error, 2) + var wg sync.WaitGroup + for i := range tokens { + credentials := expired + source := NewTokenSource(as.config(t), &credentials) + wg.Add(1) + go func() { + defer wg.Done() + token, err := source.Token(context.Background()) + tokens[i], failures[i] = token.Value, err + }() + } + wg.Wait() + + for i, err := range failures { + if err != nil { + t.Errorf("command %d: %v", i, err) + } + } + if as.refreshed != 1 || tokens[0] != "access-2" || tokens[1] != "access-2" { + t.Errorf("refreshed %d times, tokens = %v; want one refresh whose token both commands use", as.refreshed, tokens) + } +} + func TestLogoutRevokesBothTokensBeforeClearingThem(t *testing.T) { isolate(t) as := newFakeAS(t) diff --git a/internal/auth/login.go b/internal/auth/login.go index 9d7dfb4..952f0f0 100644 --- a/internal/auth/login.go +++ b/internal/auth/login.go @@ -343,22 +343,15 @@ func revoke(ctx context.Context, cfg Config, endpoint, token string) error { } // NewTokenSource presents the stored access token to the API and refreshes it -// when it expires. The context it is built with is the one a refresh uses, -// because golang.org/x/oauth2 takes no context per call. -func NewTokenSource(ctx context.Context, cfg Config, credentials *Credentials) webshare.TokenSource { - flow := &oauth2.Config{ - ClientID: cfg.ClientID, - Endpoint: oauth2.Endpoint{TokenURL: credentials.TokenEndpoint, AuthStyle: oauth2.AuthStyleInParams}, - } +// when it expires. +func NewTokenSource(cfg Config, credentials *Credentials) webshare.TokenSource { return &persistingSource{ - baseURL: cfg.BaseURL, - stored: credentials, - source: flow.TokenSource(cfg.oauthContext(ctx), &oauth2.Token{ - AccessToken: credentials.AccessToken, - RefreshToken: credentials.RefreshToken, - Expiry: credentials.Expiry, - TokenType: "Bearer", - }), + cfg: cfg, + flow: &oauth2.Config{ + ClientID: cfg.ClientID, + Endpoint: oauth2.Endpoint{TokenURL: credentials.TokenEndpoint, AuthStyle: oauth2.AuthStyleInParams}, + }, + stored: credentials, } } @@ -366,31 +359,57 @@ func NewTokenSource(ctx context.Context, cfg Config, credentials *Credentials) w // rotates both tokens on every refresh and refuses a refresh token twice, so // a rotation that is not stored leaves the next command with nothing usable. type persistingSource struct { - baseURL string - // mutex guards stored: oauth2's own source is safe for concurrent use and - // this wrapper has to be too, or two refreshes race and the loser spends a - // refresh token the server will never accept again. + cfg Config + flow *oauth2.Config + // mutex guards stored: the client may ask for a token from several + // goroutines, and only one of them may refresh. mutex sync.Mutex stored *Credentials - source oauth2.TokenSource } -func (p *persistingSource) Token(context.Context) (webshare.Token, error) { +func (p *persistingSource) Token(ctx context.Context) (webshare.Token, error) { p.mutex.Lock() defer p.mutex.Unlock() - token, err := p.source.Token() + if fresh(p.stored) { + return bearer(p.stored), nil + } + // Commands run side by side each start with the same refresh token, so + // they take turns: whoever gets the lock first refreshes, and the rest + // pick up the pair it stored instead of refreshing again. + unlock, err := lockRefresh(ctx) + if err != nil { + return webshare.Token{}, err + } + defer unlock() + latest, err := Load(p.cfg.BaseURL) if err != nil { return webshare.Token{}, fmt.Errorf("refreshing the stored login (run `webshare login` again): %w", err) } - if token.AccessToken != p.stored.AccessToken { - p.stored.AccessToken = token.AccessToken - p.stored.RefreshToken = token.RefreshToken - p.stored.Expiry = token.Expiry - if err := Save(p.baseURL, p.stored); err != nil { - return webshare.Token{}, err - } + p.stored = latest + if fresh(latest) { + return bearer(latest), nil } - return webshare.Token{Value: token.AccessToken, Scheme: "Bearer"}, nil + token, err := p.flow.TokenSource(p.cfg.oauthContext(ctx), &oauth2.Token{RefreshToken: latest.RefreshToken}).Token() + if err != nil { + return webshare.Token{}, fmt.Errorf("refreshing the stored login (run `webshare login` again): %w", err) + } + latest.AccessToken = token.AccessToken + latest.RefreshToken = token.RefreshToken + latest.Expiry = token.Expiry + if err := Save(p.cfg.BaseURL, latest); err != nil { + return webshare.Token{}, err + } + return bearer(latest), nil +} + +// fresh reports whether the access token can still be sent, with the same +// margin before expiry that golang.org/x/oauth2 allows. +func fresh(credentials *Credentials) bool { + return (&oauth2.Token{AccessToken: credentials.AccessToken, Expiry: credentials.Expiry}).Valid() +} + +func bearer(credentials *Credentials) webshare.Token { + return webshare.Token{Value: credentials.AccessToken, Scheme: "Bearer"} } func randomState() (string, error) { diff --git a/internal/auth/store.go b/internal/auth/store.go index 8663d74..7af019c 100644 --- a/internal/auth/store.go +++ b/internal/auth/store.go @@ -2,6 +2,7 @@ package auth import ( + "context" "encoding/json" "errors" "fmt" @@ -10,6 +11,7 @@ import ( "path/filepath" "time" + "github.com/gofrs/flock" "github.com/zalando/go-keyring" ) @@ -41,15 +43,44 @@ func accountFor(baseURL string) string { return parsed.Host } -// fallbackPath is where the tokens go when the operating system has no -// keyring to put them in. It never guesses: a relative path would drop a live -// token into whatever directory the command was run from. -func fallbackPath() (string, error) { +// configDir is where the CLI keeps what it has to keep on disk. It never +// guesses: a relative path would drop a live token into whatever directory the +// command was run from. +func configDir() (string, error) { dir, err := os.UserConfigDir() if err != nil { return "", fmt.Errorf("finding somewhere to keep the login: %w", err) } - return filepath.Join(dir, "webshare", "credentials.json"), nil + return filepath.Join(dir, "webshare"), nil +} + +// fallbackPath is where the tokens go when the operating system has no +// keyring to put them in. +func fallbackPath() (string, error) { + dir, err := configDir() + if err != nil { + return "", err + } + return filepath.Join(dir, "credentials.json"), nil +} + +// lockRefresh waits until no other webshare command is refreshing the login +// and returns the function that lets the next one in. The operating system +// releases the lock when its holder exits, so a command that crashes midway +// leaves nobody waiting on it. +func lockRefresh(ctx context.Context) (unlock func(), err error) { + dir, err := configDir() + if err != nil { + return nil, err + } + if err := os.MkdirAll(dir, 0o700); err != nil { + return nil, fmt.Errorf("creating %s: %w", dir, err) + } + lock := flock.New(filepath.Join(dir, "refresh.lock")) + if _, err := lock.TryLockContext(ctx, 50*time.Millisecond); err != nil { + return nil, fmt.Errorf("waiting for another webshare command to refresh the login: %w", err) + } + return func() { _ = lock.Unlock() }, nil } // Save stores the credentials in the OS keyring, falling back to a file only diff --git a/internal/cmd/root.go b/internal/cmd/root.go index fa515fa..c4b2eda 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -202,7 +202,7 @@ func newClient(ctx context.Context, flags *rootFlags) (*webshare.Client, error) return nil, err } opts = append(opts, webshare.WithTokenSource( - auth.NewTokenSource(ctx, authConfig(flags, credentials.Scopes), credentials))) + auth.NewTokenSource(authConfig(flags, credentials.Scopes), credentials))) } return webshare.NewClient(opts...) }