From 782fde051a154a6b75c8bae45610f897b59d501b Mon Sep 17 00:00:00 2001 From: mysqto Date: Wed, 2 Sep 2026 09:31:25 +0800 Subject: [PATCH 01/12] cognito: add a Cognito PKCE login flow for CLIs Adds github.com/wego/pkg/cognito, the authorization-code-with-PKCE login a command-line tool uses to obtain a human operator's tokens, plus a cognito/storage subpackage that caches the token set in the OS keychain. This is the CLI-side counterpart to http/jwt: that package verifies a token arriving at a service, this one obtains one at a terminal. Extracted from the payments repo's pay-admin CLI, where it was written against these constraints from the start so the move needed no redesign. Two properties are deliberate and worth preserving: - No package-level mutable state. Every dependency -- the clock, the HTTP client, the browser opener, the identity provider -- arrives through Config, so one process can hold several environments live at once. http/jwt keeps its JWKS URL and header in package globals and can therefore serve exactly one issuer; that limitation is why this package does not repeat the shape. - Stdlib-only OAuth (bar Wego's string helpers). Hand-rolling the exchange keeps every wire parameter visible and auditable, which matters more here than the convenience an OAuth library would buy. Kept as one module rather than splitting storage out: the split would have forced cognito to be tagged before storage could require it, and every other module in this repo requires tagged siblings with no replace directive. The import paths are identical either way, so the only cost is that an OAuth-only consumer also pulls go-keyring. Co-Authored-By: Claude Opus 5 (1M context) --- cognito/browser.go | 28 + cognito/browser_internal_test.go | 46 ++ cognito/callback.go | 163 ++++++ cognito/callback_internal_test.go | 228 ++++++++ cognito/go.mod | 18 + cognito/go.sum | 24 + cognito/oauth.go | 345 ++++++++++++ cognito/oauth_internal_test.go | 138 +++++ cognito/oauth_test.go | 652 +++++++++++++++++++++++ cognito/pkce.go | 56 ++ cognito/pkce_internal_test.go | 104 ++++ cognito/storage/keyring.go | 190 +++++++ cognito/storage/keyring_internal_test.go | 398 ++++++++++++++ cognito/storage/store.go | 99 ++++ cognito/storage/store_test.go | 80 +++ cognito/tokens.go | 88 +++ cognito/tokens_test.go | 172 ++++++ 17 files changed, 2829 insertions(+) create mode 100644 cognito/browser.go create mode 100644 cognito/browser_internal_test.go create mode 100644 cognito/callback.go create mode 100644 cognito/callback_internal_test.go create mode 100644 cognito/go.mod create mode 100644 cognito/go.sum create mode 100644 cognito/oauth.go create mode 100644 cognito/oauth_internal_test.go create mode 100644 cognito/oauth_test.go create mode 100644 cognito/pkce.go create mode 100644 cognito/pkce_internal_test.go create mode 100644 cognito/storage/keyring.go create mode 100644 cognito/storage/keyring_internal_test.go create mode 100644 cognito/storage/store.go create mode 100644 cognito/storage/store_test.go create mode 100644 cognito/tokens.go create mode 100644 cognito/tokens_test.go diff --git a/cognito/browser.go b/cognito/browser.go new file mode 100644 index 0000000..3473419 --- /dev/null +++ b/cognito/browser.go @@ -0,0 +1,28 @@ +package cognito + +import ( + "os/exec" + "runtime" +) + +// browserCommand returns the command and arguments that open rawURL in the +// platform's default browser. +func browserCommand(goos, rawURL string) (string, []string) { + switch goos { + case "darwin": + return "open", []string{rawURL} + case "windows": + return "rundll32", []string{"url.dll,FileProtocolHandler", rawURL} + default: + return "xdg-open", []string{rawURL} + } +} + +// openBrowser launches the platform's default browser. This is the one +// genuinely untestable function in the package - a unit test must not spawn a +// real browser - so it is kept to a single statement over browserCommand, +// which is tested. Callers that need a seam set Config.OpenBrowser instead. +func openBrowser(rawURL string) error { + name, args := browserCommand(runtime.GOOS, rawURL) + return exec.Command(name, args...).Start() +} diff --git a/cognito/browser_internal_test.go b/cognito/browser_internal_test.go new file mode 100644 index 0000000..4957a56 --- /dev/null +++ b/cognito/browser_internal_test.go @@ -0,0 +1,46 @@ +package cognito + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestBrowserCommand(t *testing.T) { + const authorizeURL = "https://cognito.test/oauth2/authorize?client_id=x" + + tests := []struct { + name string + givenGOOS string + wantName string + wantArgs []string + }{ + { + name: "darwin uses open", + givenGOOS: "darwin", + wantName: "open", + wantArgs: []string{authorizeURL}, + }, + { + name: "windows goes through the protocol handler", + givenGOOS: "windows", + wantName: "rundll32", + wantArgs: []string{"url.dll,FileProtocolHandler", authorizeURL}, + }, + { + name: "other platforms fall back to xdg-open", + givenGOOS: "linux", + wantName: "xdg-open", + wantArgs: []string{authorizeURL}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotName, gotArgs := browserCommand(tt.givenGOOS, authorizeURL) + + assert.Equal(t, tt.wantName, gotName) + assert.Equal(t, tt.wantArgs, gotArgs) + }) + } +} diff --git a/cognito/callback.go b/cognito/callback.go new file mode 100644 index 0000000..9b51b18 --- /dev/null +++ b/cognito/callback.go @@ -0,0 +1,163 @@ +package cognito + +import ( + "context" + "errors" + "fmt" + "net" + "net/http" + "net/url" + "time" + + wegostrings "github.com/wego/pkg/strings" +) + +const ( + // defaultCallbackTimeout bounds how long we hold the loopback listener + // open waiting for the operator to finish signing in. + defaultCallbackTimeout = 5 * time.Minute + + callbackReadHeaderTimeout = 5 * time.Second + callbackShutdownTimeout = 2 * time.Second +) + +const successHTML = ` +Signed in + +

Signed in

+

You can close this window and return to your terminal.

` + +// failureHTML deliberately carries no detail from the request. The operator +// reads the actual reason in their terminal, where it cannot be reflected back +// into a page, so nothing from the redirect is ever interpolated into HTML. +const failureHTML = ` +Sign-in failed + +

Sign-in failed

+

Return to your terminal for the reason, and try again.

` + +// callbackResult is the single outcome a callback server reports. +type callbackResult struct { + code string + state string + err error +} + +// callbackServer is a single-use loopback listener for the OAuth redirect. +type callbackServer struct { + results <-chan callbackResult + srv *http.Server +} + +// callbackPath is the request path the redirect URI points at, falling back to +// root when the URI carries no path (or cannot be parsed at all). +func callbackPath(redirectURI string) string { + parsed, err := url.Parse(redirectURI) + if err != nil || wegostrings.IsBlank(parsed.Path) { + return "/" + } + return parsed.Path +} + +// startCallbackServer binds addr and serves the OAuth redirect at path. +// +// A bind failure is terminal on purpose: the Cognito app client registers +// exactly one callback URL, so listening somewhere else would produce a +// redirect the authorization server refuses. Fail loudly instead. +func startCallbackServer(addr, path string) (*callbackServer, error) { + listener, err := net.Listen("tcp", addr) + if err != nil { + return nil, fmt.Errorf( + "bind the sign-in callback listener on %s: %w (that address must be free - the Cognito app client registers exactly one callback URL, so another port is not an option; close whatever holds it and retry)", + addr, err) + } + + results := make(chan callbackResult, 1) + mux := http.NewServeMux() + mux.HandleFunc(path, callbackHandler(results)) + + srv := &http.Server{ + Handler: mux, + ReadHeaderTimeout: callbackReadHeaderTimeout, + } + go func() { + _ = srv.Serve(listener) + }() + + return &callbackServer{results: results, srv: srv}, nil +} + +// callbackHandler serves the redirect, renders a minimal page for the +// operator, and reports the outcome exactly once. +func callbackHandler(results chan<- callbackResult) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + query := r.URL.Query() + w.Header().Set("Content-Type", "text/html; charset=utf-8") + + if authErr := query.Get("error"); wegostrings.IsNotBlank(authErr) { + message := authErr + if desc := query.Get("error_description"); wegostrings.IsNotBlank(desc) { + message += ": " + desc + } + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, failureHTML) + deliver(results, callbackResult{err: fmt.Errorf("authorization server rejected the sign-in: %s", message)}) + return + } + + code := query.Get("code") + if wegostrings.IsBlank(code) { + w.WriteHeader(http.StatusBadRequest) + fmt.Fprint(w, failureHTML) + deliver(results, callbackResult{err: errors.New("the sign-in redirect carried no authorization code")}) + return + } + + fmt.Fprint(w, successHTML) + deliver(results, callbackResult{code: code, state: query.Get("state")}) + } +} + +// deliver reports the first result and silently drops later ones. The flow is +// single-use: a reloaded browser tab must neither block the handler nor +// overwrite the outcome we already acted on. +func deliver(results chan<- callbackResult, result callbackResult) { + select { + case results <- result: + default: + } +} + +// wait blocks for the callback, honouring both ctx cancellation and timeout. +func (c *callbackServer) wait(ctx context.Context, timeout time.Duration) (code, state string, err error) { + if timeout <= 0 { + timeout = defaultCallbackTimeout + } + + timer := time.NewTimer(timeout) + defer timer.Stop() + + select { + case <-ctx.Done(): + return "", "", fmt.Errorf("waiting for the sign-in callback: %w", ctx.Err()) + case <-timer.C: + return "", "", fmt.Errorf("timed out after %s waiting for the sign-in callback", timeout) + case result := <-c.results: + if result.err != nil { + return "", "", result.err + } + return result.code, result.state, nil + } +} + +// shutdown releases the loopback listener. +func (c *callbackServer) shutdown() { + if c.srv == nil { + return + } + ctx, cancel := context.WithTimeout(context.Background(), callbackShutdownTimeout) + defer cancel() + _ = c.srv.Shutdown(ctx) +} diff --git a/cognito/callback_internal_test.go b/cognito/callback_internal_test.go new file mode 100644 index 0000000..930c3cf --- /dev/null +++ b/cognito/callback_internal_test.go @@ -0,0 +1,228 @@ +package cognito + +import ( + "context" + "net" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + wegostrings "github.com/wego/pkg/strings" +) + +func TestCallbackPath(t *testing.T) { + tests := []struct { + name string + givenURI string + want string + }{ + {name: "unparseable falls back to root", givenURI: "http://[::1", want: "/"}, + {name: "no path falls back to root", givenURI: "http://localhost:8110", want: "/"}, + {name: "blank falls back to root", givenURI: "", want: "/"}, + {name: "explicit path is used", givenURI: "http://localhost:8110/callback", want: "/callback"}, + {name: "nested path is used", givenURI: "http://127.0.0.1:8110/oauth/cb", want: "/oauth/cb"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, callbackPath(tt.givenURI)) + }) + } +} + +func TestCallbackHandler(t *testing.T) { + tests := []struct { + name string + givenQuery string + wantStatus int + wantCode string + wantState string + wantErrContains string + }{ + { + name: "authorize error is surfaced with its description", + givenQuery: "error=access_denied&error_description=user+said+no", + wantStatus: http.StatusBadRequest, + wantErrContains: "user said no", + }, + { + name: "authorize error without a description still reports", + givenQuery: "error=server_error", + wantStatus: http.StatusBadRequest, + wantErrContains: "server_error", + }, + { + name: "missing code is rejected", + givenQuery: "state=abc", + wantStatus: http.StatusBadRequest, + wantErrContains: "no authorization code", + }, + { + name: "code and state are captured", + givenQuery: "code=the-code&state=the-state", + wantStatus: http.StatusOK, + wantCode: "the-code", + wantState: "the-state", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ch := make(chan callbackResult, 1) + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/callback?"+tt.givenQuery, nil) + + callbackHandler(ch)(rec, req) + + assert.Equal(t, tt.wantStatus, rec.Code) + assert.Contains(t, rec.Header().Get("Content-Type"), "text/html") + + select { + case got := <-ch: + if wegostrings.IsNotEmpty(tt.wantErrContains) { + require.Error(t, got.err) + assert.Contains(t, got.err.Error(), tt.wantErrContains) + return + } + require.NoError(t, got.err) + assert.Equal(t, tt.wantCode, got.code) + assert.Equal(t, tt.wantState, got.state) + default: + t.Fatal("handler did not emit a callbackResult") + } + }) + } +} + +func TestCallbackHandler_IsSingleUse(t *testing.T) { + ch := make(chan callbackResult, 1) + handler := callbackHandler(ch) + + for range 3 { + rec := httptest.NewRecorder() + handler(rec, httptest.NewRequest(http.MethodGet, "/callback?code=c&state=s", nil)) + } + + require.Len(t, ch, 1, "a redelivered callback must neither block nor enqueue a second result") +} + +func TestStartCallbackServer_PortAlreadyBoundFailsFast(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = ln.Close() }) + + addr := ln.Addr().String() + cs, err := startCallbackServer(addr, "/callback") + + require.Error(t, err, "binding an occupied port must fail rather than silently pick another") + assert.Nil(t, cs) + assert.Contains(t, err.Error(), addr, "the error must name the address the operator has to free") +} + +func TestStartCallbackServer_ServesTheCallback(t *testing.T) { + addr := mustFreeAddr(t) + cs, err := startCallbackServer(addr, "/callback") + require.NoError(t, err) + t.Cleanup(cs.shutdown) + + mustGet(t, "http://"+addr+"/callback?code=abc&state=xyz") + + code, state, err := cs.wait(context.Background(), 2*time.Second) + require.NoError(t, err) + assert.Equal(t, "abc", code) + assert.Equal(t, "xyz", state) +} + +func TestCallbackServer_Wait(t *testing.T) { + tests := []struct { + name string + givenResult *callbackResult + givenTimeout time.Duration + givenCancel bool + wantCode string + wantErrContains string + }{ + { + name: "timeout is reported", + givenTimeout: 10 * time.Millisecond, + wantErrContains: "timed out", + }, + { + name: "cancellation is reported", + givenTimeout: time.Minute, + givenCancel: true, + wantErrContains: "context canceled", + }, + { + name: "callback error is propagated", + givenResult: &callbackResult{err: stubError("authorize: access_denied")}, + givenTimeout: time.Minute, + wantErrContains: "access_denied", + }, + { + name: "code and state are returned", + givenResult: &callbackResult{code: "c", state: "s"}, + givenTimeout: time.Minute, + wantCode: "c", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ch := make(chan callbackResult, 1) + if tt.givenResult != nil { + ch <- *tt.givenResult + } + cs := &callbackServer{results: ch} + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + if tt.givenCancel { + cancel() + } + + code, _, err := cs.wait(ctx, tt.givenTimeout) + if wegostrings.IsNotEmpty(tt.wantErrContains) { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErrContains) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantCode, code) + }) + } +} + +func TestCallbackServer_ShutdownIsSafeWithoutAServer(t *testing.T) { + cs := &callbackServer{results: make(chan callbackResult, 1)} + assert.NotPanics(t, cs.shutdown) +} + +// mustFreeAddr reserves and releases a loopback port, returning its address. +func mustFreeAddr(t *testing.T) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + return addr +} + +// mustGet issues a throwaway GET, standing in for the operator's browser. +func mustGet(t *testing.T, rawURL string) { + t.Helper() + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, rawURL, nil) + require.NoError(t, err) + //nolint:gosec // G704: a loopback URL this test built itself. + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) +} + +// stubError is a minimal error for table cases that need a pre-baked failure. +type stubError string + +func (e stubError) Error() string { return string(e) } diff --git a/cognito/go.mod b/cognito/go.mod new file mode 100644 index 0000000..5f53e7e --- /dev/null +++ b/cognito/go.mod @@ -0,0 +1,18 @@ +module github.com/wego/pkg/cognito + +go 1.25.0 + +require ( + github.com/stretchr/testify v1.11.1 + github.com/wego/pkg/strings v0.1.2 + github.com/zalando/go-keyring v0.2.8 +) + +require ( + github.com/danieljoos/wincred v1.2.3 // indirect + github.com/davecgh/go-spew v1.1.1 // indirect + github.com/godbus/dbus/v5 v5.2.2 // indirect + github.com/pmezard/go-difflib v1.0.0 // indirect + golang.org/x/sys v0.27.0 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect +) diff --git a/cognito/go.sum b/cognito/go.sum new file mode 100644 index 0000000..3bcf314 --- /dev/null +++ b/cognito/go.sum @@ -0,0 +1,24 @@ +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/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +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/wego/pkg/pointer v0.1.2 h1:KghXP86aWukvpSVPQ+Fg7YOkW8p8kyXcuOAvWVX1RUk= +github.com/wego/pkg/pointer v0.1.2/go.mod h1:TincAjFVHSyuZ05qnSP4APqs+eg+adjOfZV6VH0+CUA= +github.com/wego/pkg/strings v0.1.2 h1:sFfYDrC90JI43UCs4fnlxtuG/kXm6WJPIfiND1aqrQE= +github.com/wego/pkg/strings v0.1.2/go.mod h1:nS3SS/em72lPIJYtgaLE5XgboOLOS8lnsYRAD8U2Ptc= +github.com/zalando/go-keyring v0.2.8 h1:6sD/Ucpl7jNq10rM2pgqTs0sZ9V3qMrqfIIy5YPccHs= +github.com/zalando/go-keyring v0.2.8/go.mod h1:tsMo+VpRq5NGyKfxoBVjCuMrG47yj8cmakZDO5QGii0= +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 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= +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/cognito/oauth.go b/cognito/oauth.go new file mode 100644 index 0000000..6cda020 --- /dev/null +++ b/cognito/oauth.go @@ -0,0 +1,345 @@ +// Package cognito implements the Cognito authorization-code-with-PKCE login +// flow a command-line tool uses to obtain a human operator's tokens. +// +// It is built for CLIs rather than services: the flow opens a browser, receives +// the authorization code on a loopback listener, and hands back the token set +// for the caller to cache. A service verifying an incoming token wants +// github.com/wego/pkg/http/jwt instead. +// +// Two deliberate constraints shape this package: +// +// - It depends on nothing outside the standard library (bar Wego's string +// helpers). Hand-rolling the OAuth exchange keeps every wire parameter +// visible and auditable, which matters more here than the convenience an +// OAuth library would buy. +// - It holds no package-level mutable state. Every dependency - the clock, +// the HTTP client, the browser opener - arrives through Config, so tests +// and callers never race over shared globals. +package cognito + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "time" + + wegostrings "github.com/wego/pkg/strings" +) + +const ( + // placeholderPrefix marks a Cognito value that ops has not filled in yet. + placeholderPrefix = "REPLACE_WITH_" + + defaultHTTPTimeout = 30 * time.Second + maxTokenResponseBytes = 1 << 20 +) + +// Config carries everything the login flow needs. Nothing is read from the +// environment or from package state, so a caller holds the whole contract. +type Config struct { + // AuthorizeURL is the Cognito hosted-UI authorize endpoint. + AuthorizeURL string + + // TokenURL is the Cognito token endpoint. + TokenURL string + + // ClientID is the Cognito app client id. + ClientID string + + // RedirectURI is the callback URL registered on the app client. Its path + // determines where the local callback server listens. + RedirectURI string + + // Scopes is the space-separated scope list to request. + Scopes string + + // AllowedDomain, when set, is the email suffix an operator must sign in + // with, e.g. "@wego.com". + AllowedDomain string + + // CallbackAddr is the host:port the local callback server binds, e.g. + // "127.0.0.1:8110". It must agree with RedirectURI. + CallbackAddr string + + // IdentityProvider, when set, is forwarded as identity_provider so Cognito + // jumps straight to that IdP instead of showing its own chooser. + IdentityProvider string + + // OpenBrowser launches the authorize URL. Nil uses the OS default opener. + OpenBrowser func(string) error + + // HTTPClient calls the token endpoint. Nil uses a client with a timeout. + HTTPClient *http.Client + + // Now supplies the current time, for computing token expiry. Nil uses + // time.Now; tests inject a fixed clock. + Now func() time.Time +} + +// Login runs the browser-based authorization-code-with-PKCE flow and returns +// the operator's tokens. +func Login(ctx context.Context, cfg Config) (*TokenSet, error) { + if err := cfg.validateForLogin(); err != nil { + return nil, err + } + + verifier, err := generateVerifier() + if err != nil { + return nil, err + } + state, err := generateState() + if err != nil { + return nil, err + } + + // Bind the callback port BEFORE sending the operator to Cognito. If the + // port is unavailable the redirect could never land, and there is no + // fallback port to try, so failing here saves a pointless round trip. + server, err := startCallbackServer(cfg.CallbackAddr, callbackPath(cfg.RedirectURI)) + if err != nil { + return nil, err + } + defer server.shutdown() + + if err := cfg.openBrowserAt(cfg.buildAuthorizeURL(state, generateChallenge(verifier))); err != nil { + return nil, fmt.Errorf("open browser for sign-in: %w", err) + } + + code, callbackState, err := server.wait(ctx, defaultCallbackTimeout) + if err != nil { + return nil, err + } + + // CSRF protection, not decoration: a callback whose state is not the value + // we minted did not come from the authorize request we started, so the + // code it carries is not ours to redeem. + if !stateMatches(state, callbackState) { + return nil, errors.New("oauth state mismatch: the sign-in callback did not come from this login attempt") + } + + tokens, err := cfg.exchangeCode(ctx, code, verifier) + if err != nil { + return nil, err + } + + if err := cfg.checkAllowedDomain(tokens); err != nil { + return nil, err + } + + return tokens, nil +} + +// Refresh exchanges a refresh token for a fresh access and id token. +func Refresh(ctx context.Context, cfg Config, refreshToken string) (*TokenSet, error) { + if wegostrings.IsBlank(refreshToken) { + return nil, errors.New("no refresh token available: sign in again") + } + if err := cfg.validateClient(); err != nil { + return nil, err + } + + form := url.Values{} + form.Set("grant_type", "refresh_token") + form.Set("client_id", cfg.ClientID) + form.Set("refresh_token", refreshToken) + + return cfg.postToken(ctx, form, refreshToken) +} + +// exchangeCode redeems an authorization code together with its PKCE verifier. +func (c Config) exchangeCode(ctx context.Context, code, verifier string) (*TokenSet, error) { + form := url.Values{} + form.Set("grant_type", "authorization_code") + form.Set("client_id", c.ClientID) + form.Set("code", code) + form.Set("redirect_uri", c.RedirectURI) + form.Set("code_verifier", verifier) + + return c.postToken(ctx, form, "") +} + +// tokenResponse is the wire shape of a Cognito token response. The field names +// are fixed by the OAuth spec, so gosec's secret-field warning is expected. +type tokenResponse struct { + AccessToken string `json:"access_token"` //nolint:gosec // G117: OAuth wire field; decoded in-process and never logged. + IDToken string `json:"id_token"` + RefreshToken string `json:"refresh_token"` //nolint:gosec // G117: OAuth wire field, as above. + ExpiresIn int `json:"expires_in"` +} + +// postToken performs a token-endpoint call. fallbackRefresh is the refresh +// token to keep if the response omits one. +func (c Config) postToken(ctx context.Context, form url.Values, fallbackRefresh string) (*TokenSet, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.TokenURL, strings.NewReader(form.Encode())) + if err != nil { + return nil, fmt.Errorf("build token request: %w", err) + } + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + + //nolint:gosec // G704: TokenURL is operator-controlled CLI config (the Cognito domain), not request input. + resp, err := c.httpClient().Do(req) + if err != nil { + return nil, fmt.Errorf("call the token endpoint: %w", err) + } + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(io.LimitReader(resp.Body, maxTokenResponseBytes)) + if err != nil { + return nil, fmt.Errorf("read token response: %w", err) + } + + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + return nil, tokenEndpointError(resp.StatusCode, body) + } + + var parsed tokenResponse + if err := json.Unmarshal(body, &parsed); err != nil { + return nil, fmt.Errorf("decode token response: %w", err) + } + + // Cognito omits refresh_token from a refresh response, so the original has + // to be carried forward. This is load-bearing: dropping it would silently + // sign the operator out on their next command. Do not add validation that + // rejects this fallback. + refresh := parsed.RefreshToken + if wegostrings.IsBlank(refresh) { + refresh = fallbackRefresh + } + + switch { + case wegostrings.IsBlank(parsed.AccessToken): + return nil, errors.New("token response is missing access_token") + case wegostrings.IsBlank(parsed.IDToken): + return nil, errors.New("token response is missing id_token") + case wegostrings.IsBlank(refresh): + return nil, errors.New("token response is missing refresh_token") + } + + return &TokenSet{ + AccessToken: parsed.AccessToken, + IDToken: parsed.IDToken, + RefreshToken: refresh, + ExpiresAt: c.now().Add(time.Duration(parsed.ExpiresIn) * time.Second), + }, nil +} + +// tokenEndpointError reports a non-2xx token response. +// +// Only the standard OAuth error fields are echoed. An arbitrary response body +// is deliberately withheld, so no token can ever ride out inside an error +// message that ends up in a log or a bug report. +func tokenEndpointError(status int, body []byte) error { + var parsed struct { + Error string `json:"error"` + Description string `json:"error_description"` + } + + if err := json.Unmarshal(body, &parsed); err == nil && wegostrings.IsNotBlank(parsed.Error) { + if wegostrings.IsNotBlank(parsed.Description) { + return fmt.Errorf("token endpoint returned %d: %s: %s", status, parsed.Error, parsed.Description) + } + return fmt.Errorf("token endpoint returned %d: %s", status, parsed.Error) + } + + return fmt.Errorf("token endpoint returned %d (response body withheld: it may carry credentials)", status) +} + +// buildAuthorizeURL assembles the hosted-UI URL the operator is sent to. +func (c Config) buildAuthorizeURL(state, challenge string) string { + query := url.Values{} + query.Set("response_type", "code") + query.Set("client_id", c.ClientID) + query.Set("redirect_uri", c.RedirectURI) + query.Set("scope", c.Scopes) + query.Set("state", state) + query.Set("code_challenge", challenge) + query.Set("code_challenge_method", "S256") + if wegostrings.IsNotBlank(c.IdentityProvider) { + query.Set("identity_provider", c.IdentityProvider) + } + + separator := "?" + if strings.Contains(c.AuthorizeURL, "?") { + separator = "&" + } + return c.AuthorizeURL + separator + query.Encode() +} + +// checkAllowedDomain rejects an operator signed in outside AllowedDomain. +// +// This is client-side UX, NOT a security boundary: it catches "you signed in +// with your personal Google account" before the CLI starts issuing calls that +// would fail confusingly. The server does not enforce it, so nothing may rely +// on it for authorization. +func (c Config) checkAllowedDomain(tokens *TokenSet) error { + if wegostrings.IsBlank(c.AllowedDomain) { + return nil + } + + email, err := tokens.Email() + if err != nil { + return fmt.Errorf("check the signed-in email: %w", err) + } + + if !strings.HasSuffix(strings.ToLower(email), strings.ToLower(c.AllowedDomain)) { + return fmt.Errorf("signed in as %s, but a %s account is required", email, c.AllowedDomain) + } + return nil +} + +// validateClient checks the values every token call needs. +func (c Config) validateClient() error { + if wegostrings.IsBlank(c.ClientID) || strings.HasPrefix(c.ClientID, placeholderPrefix) { + return errors.New("cognito app client is not provisioned yet: Config.ClientID is blank or still a placeholder, so the app client needs to be created and its id wired into the caller's config") + } + if wegostrings.IsBlank(c.TokenURL) { + return errors.New("cognito token url is not configured") + } + return nil +} + +// validateForLogin checks everything the interactive flow additionally needs. +func (c Config) validateForLogin() error { + if err := c.validateClient(); err != nil { + return err + } + if wegostrings.IsBlank(c.AuthorizeURL) { + return errors.New("cognito authorize url is not configured") + } + if wegostrings.IsBlank(c.RedirectURI) { + return errors.New("cognito redirect uri is not configured") + } + if wegostrings.IsBlank(c.CallbackAddr) { + return errors.New("local callback address is not configured") + } + return nil +} + +// httpClient is the client to call the token endpoint with. +func (c Config) httpClient() *http.Client { + if c.HTTPClient != nil { + return c.HTTPClient + } + return &http.Client{Timeout: defaultHTTPTimeout} +} + +// now is the current time, from the injected clock when there is one. +func (c Config) now() time.Time { + if c.Now != nil { + return c.Now() + } + return time.Now() +} + +// openBrowserAt sends the operator to rawURL. +func (c Config) openBrowserAt(rawURL string) error { + if c.OpenBrowser != nil { + return c.OpenBrowser(rawURL) + } + return openBrowser(rawURL) +} diff --git a/cognito/oauth_internal_test.go b/cognito/oauth_internal_test.go new file mode 100644 index 0000000..064dc3a --- /dev/null +++ b/cognito/oauth_internal_test.go @@ -0,0 +1,138 @@ +package cognito + +import ( + "net/http" + "net/url" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestTokenEndpointError(t *testing.T) { + tests := []struct { + name string + givenStatus int + givenBody string + wantContains []string + wantNotContains []string + }{ + { + name: "non-json body is withheld entirely", + givenStatus: http.StatusInternalServerError, + givenBody: "access_token=super-secret-value", + // An unparseable body could be anything, so we report only the + // status: a token must never ride out inside an error string. + wantContains: []string{"500", "withheld"}, + wantNotContains: []string{"super-secret-value"}, + }, + { + name: "json without an error field is withheld", + givenStatus: http.StatusBadGateway, + givenBody: `{"id_token":"secret-jwt-value"}`, + wantContains: []string{"502", "withheld"}, + wantNotContains: []string{"secret-jwt-value"}, + }, + { + name: "oauth error is echoed", + givenStatus: http.StatusBadRequest, + givenBody: `{"error":"invalid_grant"}`, + wantContains: []string{"400", "invalid_grant"}, + }, + { + name: "oauth error and description are echoed", + givenStatus: http.StatusBadRequest, + givenBody: `{"error":"invalid_grant","error_description":"code expired"}`, + wantContains: []string{"400", "invalid_grant", "code expired"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tokenEndpointError(tt.givenStatus, []byte(tt.givenBody)) + + require.Error(t, err) + for _, want := range tt.wantContains { + assert.Contains(t, err.Error(), want) + } + for _, notWant := range tt.wantNotContains { + assert.NotContains(t, err.Error(), notWant) + } + }) + } +} + +func TestConfig_BuildAuthorizeURL(t *testing.T) { + tests := []struct { + name string + givenAuthorizeURL string + givenIdentityProvider string + wantQueryHasProvider bool + }{ + { + name: "an existing query string is appended to", + givenAuthorizeURL: "https://cognito.test/oauth2/authorize?foo=bar", + }, + { + name: "no identity provider leaves the parameter out", + givenAuthorizeURL: "https://cognito.test/oauth2/authorize", + }, + { + name: "identity provider is forwarded when set", + givenAuthorizeURL: "https://cognito.test/oauth2/authorize", + givenIdentityProvider: "Google", + wantQueryHasProvider: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := Config{ + AuthorizeURL: tt.givenAuthorizeURL, + ClientID: "client", + RedirectURI: "http://127.0.0.1:8110/callback", + Scopes: "openid email", + IdentityProvider: tt.givenIdentityProvider, + } + + got, err := url.Parse(cfg.buildAuthorizeURL("the-state", "the-challenge")) + require.NoError(t, err) + + query := got.Query() + assert.Equal(t, "the-state", query.Get("state")) + assert.Equal(t, "the-challenge", query.Get("code_challenge")) + assert.Equal(t, "S256", query.Get("code_challenge_method")) + assert.Equal(t, tt.givenIdentityProvider, query.Get("identity_provider")) + assert.Equal(t, tt.wantQueryHasProvider, query.Has("identity_provider")) + + if tt.givenAuthorizeURL == "https://cognito.test/oauth2/authorize?foo=bar" { + assert.Equal(t, "bar", query.Get("foo"), "an existing query parameter must survive") + } + }) + } +} + +func TestConfig_Defaults(t *testing.T) { + var zero Config + + assert.Equal(t, defaultHTTPTimeout, zero.httpClient().Timeout, "a nil HTTPClient must get a timeout") + assert.WithinDuration(t, time.Now(), zero.now(), time.Minute, "a nil Now must fall back to the system clock") + + custom := &http.Client{Timeout: time.Second} + assert.Same(t, custom, Config{HTTPClient: custom}.httpClient()) + + fixed := time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC) + assert.Equal(t, fixed, Config{Now: func() time.Time { return fixed }}.now()) +} + +func TestConfig_OpenBrowserAtUsesTheInjectedOpener(t *testing.T) { + var got string + cfg := Config{OpenBrowser: func(rawURL string) error { + got = rawURL + return nil + }} + + require.NoError(t, cfg.openBrowserAt("https://example.test/authorize")) + assert.Equal(t, "https://example.test/authorize", got) +} diff --git a/cognito/oauth_test.go b/cognito/oauth_test.go new file mode 100644 index 0000000..5117709 --- /dev/null +++ b/cognito/oauth_test.go @@ -0,0 +1,652 @@ +package cognito_test + +import ( + "context" + "crypto/sha256" + "encoding/base64" + "encoding/json" + "errors" + "net" + "net/http" + "net/http/httptest" + "net/url" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + wegostrings "github.com/wego/pkg/strings" + + "github.com/wego/pkg/cognito" +) + +const ( + testAuthCode = "the-authorization-code" + testAccessValue = "access-value" + testIDTokenLabel = "id_token" + testRefreshValue = "refresh-value" +) + +func TestLogin(t *testing.T) { + tests := []struct { + name string + givenConfig func(*cognito.Config) + givenBrowser func(*fakeBrowser) + givenHandler func(*testing.T) http.HandlerFunc + givenCancel bool + wantEmail string + wantErrContains string + }{ + { + name: "blank client id tells the operator it is unprovisioned", + givenConfig: func(c *cognito.Config) { c.ClientID = "" }, + wantErrContains: "not provisioned", + }, + { + name: "placeholder client id tells the operator it is unprovisioned", + givenConfig: func(c *cognito.Config) { c.ClientID = "REPLACE_WITH_COGNITO_CLIENT_ID" }, + wantErrContains: "not provisioned", + }, + { + name: "missing authorize url is rejected", + givenConfig: func(c *cognito.Config) { c.AuthorizeURL = "" }, + wantErrContains: "authorize url", + }, + { + name: "missing token url is rejected", + givenConfig: func(c *cognito.Config) { c.TokenURL = "" }, + wantErrContains: "token url", + }, + { + name: "missing redirect uri is rejected", + givenConfig: func(c *cognito.Config) { c.RedirectURI = "" }, + wantErrContains: "redirect uri", + }, + { + name: "missing callback address is rejected", + givenConfig: func(c *cognito.Config) { c.CallbackAddr = "" }, + wantErrContains: "callback address", + }, + { + name: "browser launch failure is surfaced", + givenBrowser: func(f *fakeBrowser) { f.openErr = errors.New("no browser here") }, + wantErrContains: "open browser", + }, + { + name: "tampered state is rejected", + givenBrowser: func(f *fakeBrowser) { + f.tamper = func(q url.Values) { q.Set("state", "tampered-state") } + }, + wantErrContains: "state mismatch", + }, + { + name: "missing state is rejected", + givenBrowser: func(f *fakeBrowser) { + f.tamper = func(q url.Values) { q.Del("state") } + }, + wantErrContains: "state mismatch", + }, + { + name: "authorize server error is surfaced", + givenBrowser: func(f *fakeBrowser) { + f.tamper = func(q url.Values) { + q.Del("code") + q.Set("error", "access_denied") + q.Set("error_description", "operator declined") + } + }, + wantErrContains: "access_denied", + }, + { + name: "missing code is rejected", + givenBrowser: func(f *fakeBrowser) { + f.tamper = func(q url.Values) { q.Del("code") } + }, + wantErrContains: "no authorization code", + }, + { + name: "token endpoint rejection is surfaced", + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + return jsonHandler(http.StatusBadRequest, map[string]any{ + "error": "invalid_grant", + "error_description": "code already used", + }) + }, + wantErrContains: "invalid_grant", + }, + { + name: "unparseable token response is rejected", + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + return rawHandler(http.StatusOK, "not json") + }, + wantErrContains: "decode token response", + }, + { + name: "token response without an access token is rejected", + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + body := goodTokenBody(t, testOperatorEmail) + delete(body, "access_token") + return jsonHandler(http.StatusOK, body) + }, + wantErrContains: "missing access_token", + }, + { + name: "token response without an id token is rejected", + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + body := goodTokenBody(t, testOperatorEmail) + delete(body, testIDTokenLabel) + return jsonHandler(http.StatusOK, body) + }, + wantErrContains: "missing id_token", + }, + { + name: "token response without a refresh token is rejected", + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + body := goodTokenBody(t, testOperatorEmail) + delete(body, "refresh_token") + return jsonHandler(http.StatusOK, body) + }, + wantErrContains: "missing refresh_token", + }, + { + name: "id token without an email claim is rejected", + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + body := goodTokenBody(t, testOperatorEmail) + body[testIDTokenLabel] = mustIDToken(t, map[string]any{"sub": "abc"}) + return jsonHandler(http.StatusOK, body) + }, + wantErrContains: "email", + }, + { + name: "email outside the allowed domain is rejected", + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + return jsonHandler(http.StatusOK, goodTokenBody(t, "someone@gmail.com")) + }, + wantErrContains: "@wego.com", + }, + { + name: "cancelled context stops waiting", + givenBrowser: func(f *fakeBrowser) { f.suppress = true }, + givenCancel: true, + wantErrContains: "context canceled", + }, + { + name: "successful login returns a token set", + wantEmail: testOperatorEmail, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var handler http.HandlerFunc + if tt.givenHandler != nil { + handler = tt.givenHandler(t) + } + ts := newTokenServer(t, handler) + + browser := newFakeBrowser() + if tt.givenBrowser != nil { + tt.givenBrowser(browser) + } + + cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg.OpenBrowser = browser.open + if tt.givenConfig != nil { + tt.givenConfig(&cfg) + } + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + if tt.givenCancel { + cancel() + } + + got, err := cognito.Login(ctx, cfg) + + if wegostrings.IsNotEmpty(tt.wantErrContains) { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErrContains) + assert.Nil(t, got) + return + } + require.NoError(t, err) + require.NotNil(t, got) + + email, err := got.Email() + require.NoError(t, err) + assert.Equal(t, tt.wantEmail, email) + }) + } +} + +func TestLogin_BindsTheCallbackPortBeforeOpeningTheBrowser(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + t.Cleanup(func() { _ = ln.Close() }) + addr := ln.Addr().String() + + ts := newTokenServer(t, nil) + browser := newFakeBrowser() + cfg := baseConfig(t, ts.URL, addr) + cfg.OpenBrowser = browser.open + + got, err := cognito.Login(context.Background(), cfg) + + require.Error(t, err, "an occupied callback port must fail fast, not fall back to another port") + assert.Nil(t, got) + assert.Contains(t, err.Error(), addr, "the error must name the address the operator has to free") + assert.Empty(t, browser.capturedURL(), "the browser must not be launched when the callback port is unavailable") +} + +func TestLogin_SendsPKCEAndState(t *testing.T) { + ts := newTokenServer(t, nil) + browser := newFakeBrowser() + cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg.OpenBrowser = browser.open + + got, err := cognito.Login(context.Background(), cfg) + require.NoError(t, err) + + authorizeURL, err := url.Parse(browser.capturedURL()) + require.NoError(t, err) + aq := authorizeURL.Query() + + assert.Equal(t, "https://cognito.test/oauth2/authorize", authorizeURL.Scheme+"://"+authorizeURL.Host+authorizeURL.Path) + assert.Equal(t, "code", aq.Get("response_type")) + assert.Equal(t, cfg.ClientID, aq.Get("client_id")) + assert.Equal(t, cfg.RedirectURI, aq.Get("redirect_uri")) + assert.Equal(t, cfg.Scopes, aq.Get("scope")) + assert.Equal(t, "S256", aq.Get("code_challenge_method"), "plain PKCE must never be used") + assert.NotEmpty(t, aq.Get("state"), "state is CSRF protection and must always be sent") + + form := ts.lastForm(t) + assert.Equal(t, "authorization_code", form.Get("grant_type")) + assert.Equal(t, cfg.ClientID, form.Get("client_id")) + assert.Equal(t, testAuthCode, form.Get("code")) + assert.Equal(t, cfg.RedirectURI, form.Get("redirect_uri")) + + verifier := form.Get("code_verifier") + require.NotEmpty(t, verifier, "the exchange must carry the PKCE verifier") + assert.GreaterOrEqual(t, len(verifier), 43) + assert.LessOrEqual(t, len(verifier), 128) + + sum := sha256.Sum256([]byte(verifier)) + assert.Equal(t, base64.RawURLEncoding.EncodeToString(sum[:]), aq.Get("code_challenge"), + "code_challenge must be the S256 hash of the verifier actually redeemed") + + assert.Equal(t, fixedNow.Add(time.Hour), got.ExpiresAt, "ExpiresAt must be derived from the injected clock") + assert.Equal(t, testAccessValue, got.AccessToken) + assert.Equal(t, testRefreshValue, got.RefreshToken) +} + +func TestLogin_ErrorsDoNotLeakVerifierOrState(t *testing.T) { + ts := newTokenServer(t, jsonHandler(http.StatusBadRequest, map[string]any{"error": "invalid_grant"})) + browser := newFakeBrowser() + cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg.OpenBrowser = browser.open + + _, err := cognito.Login(context.Background(), cfg) + require.Error(t, err) + + authorizeURL, parseErr := url.Parse(browser.capturedURL()) + require.NoError(t, parseErr) + state := authorizeURL.Query().Get("state") + require.NotEmpty(t, state) + verifier := ts.lastForm(t).Get("code_verifier") + require.NotEmpty(t, verifier) + + assert.NotContains(t, err.Error(), state, "state must never appear in an error message") + assert.NotContains(t, err.Error(), verifier, "the PKCE verifier must never appear in an error message") +} + +func TestLogin_FallsBackToTheSystemClockWhenNowIsNil(t *testing.T) { + ts := newTokenServer(t, nil) + browser := newFakeBrowser() + cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg.OpenBrowser = browser.open + cfg.Now = nil + + before := time.Now() + got, err := cognito.Login(context.Background(), cfg) + require.NoError(t, err) + + assert.WithinDuration(t, before.Add(time.Hour), got.ExpiresAt, time.Minute) +} + +func TestLogin_AllowsAnyEmailWhenNoDomainIsConfigured(t *testing.T) { + ts := newTokenServer(t, jsonHandler(http.StatusOK, goodTokenBody(t, "contractor@example.test"))) + browser := newFakeBrowser() + cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg.OpenBrowser = browser.open + cfg.AllowedDomain = "" + + got, err := cognito.Login(context.Background(), cfg) + require.NoError(t, err) + require.NotNil(t, got) +} + +func TestRefresh(t *testing.T) { + tests := []struct { + name string + givenConfig func(*cognito.Config) + givenRefresh string + givenHandler func(*testing.T) http.HandlerFunc + wantRefresh string + wantErrContains string + }{ + { + name: "blank refresh token is rejected", + givenRefresh: "", + wantErrContains: "refresh token", + }, + { + name: "blank client id tells the operator it is unprovisioned", + givenRefresh: testRefreshValue, + givenConfig: func(c *cognito.Config) { c.ClientID = "" }, + wantErrContains: "not provisioned", + }, + { + name: "placeholder client id tells the operator it is unprovisioned", + givenRefresh: testRefreshValue, + givenConfig: func(c *cognito.Config) { c.ClientID = "REPLACE_WITH_COGNITO_CLIENT_ID" }, + wantErrContains: "not provisioned", + }, + { + name: "missing token url is rejected", + givenRefresh: testRefreshValue, + givenConfig: func(c *cognito.Config) { c.TokenURL = "" }, + wantErrContains: "token url", + }, + { + name: "server failure is surfaced", + givenRefresh: testRefreshValue, + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + return rawHandler(http.StatusInternalServerError, "upstream exploded") + }, + wantErrContains: "500", + }, + { + name: "unparseable response is rejected", + givenRefresh: testRefreshValue, + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + return rawHandler(http.StatusOK, "not json") + }, + wantErrContains: "decode token response", + }, + { + name: "response without an access token is rejected", + givenRefresh: testRefreshValue, + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + body := goodTokenBody(t, testOperatorEmail) + delete(body, "access_token") + return jsonHandler(http.StatusOK, body) + }, + wantErrContains: "missing access_token", + }, + { + name: "a rotated refresh token is adopted", + givenRefresh: "original-refresh", + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + body := goodTokenBody(t, testOperatorEmail) + body["refresh_token"] = "rotated-refresh" + return jsonHandler(http.StatusOK, body) + }, + wantRefresh: "rotated-refresh", + }, + { + name: "an omitted refresh token falls back to the original", + givenRefresh: "original-refresh", + givenHandler: func(t *testing.T) http.HandlerFunc { + t.Helper() + body := goodTokenBody(t, testOperatorEmail) + delete(body, "refresh_token") + return jsonHandler(http.StatusOK, body) + }, + wantRefresh: "original-refresh", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var handler http.HandlerFunc + if tt.givenHandler != nil { + handler = tt.givenHandler(t) + } + ts := newTokenServer(t, handler) + + cfg := baseConfig(t, ts.URL, "127.0.0.1:0") + if tt.givenConfig != nil { + tt.givenConfig(&cfg) + } + + got, err := cognito.Refresh(context.Background(), cfg, tt.givenRefresh) + + if wegostrings.IsNotEmpty(tt.wantErrContains) { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErrContains) + assert.Nil(t, got) + return + } + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, tt.wantRefresh, got.RefreshToken) + assert.Equal(t, "refresh_token", ts.lastForm(t).Get("grant_type")) + }) + } +} + +// TestRefresh_PreservesTheOriginalRefreshToken pins the load-bearing Cognito +// invariant: a refresh response omits refresh_token, and dropping the original +// would log the operator out on the very next command. +func TestRefresh_PreservesTheOriginalRefreshToken(t *testing.T) { + body := goodTokenBody(t, testOperatorEmail) + delete(body, "refresh_token") + ts := newTokenServer(t, jsonHandler(http.StatusOK, body)) + + cfg := baseConfig(t, ts.URL, "127.0.0.1:0") + + got, err := cognito.Refresh(context.Background(), cfg, "the-long-lived-refresh-token") + require.NoError(t, err) + assert.Equal(t, "the-long-lived-refresh-token", got.RefreshToken) + assert.Equal(t, "the-long-lived-refresh-token", ts.lastForm(t).Get("refresh_token"), + "the original refresh token must be what we present to Cognito") + assert.Equal(t, fixedNow.Add(time.Hour), got.ExpiresAt) +} + +// TestRefresh_DoesNotApplyTheDomainGate documents that the allowed-domain check +// is a login-time UX affordance, not something to re-run on every refresh. +func TestRefresh_DoesNotApplyTheDomainGate(t *testing.T) { + ts := newTokenServer(t, jsonHandler(http.StatusOK, goodTokenBody(t, "someone@gmail.com"))) + cfg := baseConfig(t, ts.URL, "127.0.0.1:0") + + got, err := cognito.Refresh(context.Background(), cfg, testRefreshValue) + require.NoError(t, err) + require.NotNil(t, got) +} + +func TestRefresh_HonoursContextCancellation(t *testing.T) { + ts := newTokenServer(t, nil) + cfg := baseConfig(t, ts.URL, "127.0.0.1:0") + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + got, err := cognito.Refresh(ctx, cfg, testRefreshValue) + require.Error(t, err) + assert.Nil(t, got) +} + +// baseConfig is a working staging-shaped config aimed at a test token server. +func baseConfig(t *testing.T, tokenURL, callbackAddr string) cognito.Config { + t.Helper() + return cognito.Config{ + AuthorizeURL: "https://cognito.test/oauth2/authorize", + TokenURL: tokenURL, + ClientID: "test-client-id", + RedirectURI: "http://" + callbackAddr + "/callback", + Scopes: "openid email profile", + AllowedDomain: "@wego.com", + CallbackAddr: callbackAddr, + Now: func() time.Time { return fixedNow }, + } +} + +// goodTokenBody is a well-formed Cognito token response. +func goodTokenBody(t *testing.T, email string) map[string]any { + t.Helper() + return map[string]any{ + "access_token": testAccessValue, + testIDTokenLabel: mustIDToken(t, map[string]any{"email": email}), + "refresh_token": testRefreshValue, + "expires_in": 3600, + "token_type": "Bearer", + } +} + +func jsonHandler(status int, body any) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + enc, err := json.Marshal(body) + if err != nil { + http.Error(w, "marshal failed", http.StatusInternalServerError) + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = w.Write(enc) + } +} + +func rawHandler(status int, body string) http.HandlerFunc { + return func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(status) + _, _ = w.Write([]byte(body)) + } +} + +// tokenServer records every form it is posted so tests can assert on the +// exchange itself, not just its result. +type tokenServer struct { + *httptest.Server + + mu sync.Mutex + forms []url.Values +} + +func newTokenServer(t *testing.T, handler http.HandlerFunc) *tokenServer { + t.Helper() + + ts := &tokenServer{} + if handler == nil { + handler = jsonHandler(http.StatusOK, goodTokenBody(t, testOperatorEmail)) + } + + ts.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if err := r.ParseForm(); err != nil { + http.Error(w, "bad form", http.StatusBadRequest) + return + } + ts.mu.Lock() + ts.forms = append(ts.forms, r.PostForm) + ts.mu.Unlock() + handler(w, r) + })) + t.Cleanup(ts.Close) + + return ts +} + +func (ts *tokenServer) lastForm(t *testing.T) url.Values { + t.Helper() + ts.mu.Lock() + defer ts.mu.Unlock() + require.NotEmpty(t, ts.forms, "the token endpoint was never called") + return ts.forms[len(ts.forms)-1] +} + +// fakeBrowser stands in for the operator's browser: it reads the authorize URL +// and drives the loopback callback the way a real redirect would. +type fakeBrowser struct { + openErr error + suppress bool + tamper func(url.Values) + + mu sync.Mutex + authorizeURL string +} + +func newFakeBrowser() *fakeBrowser { + return &fakeBrowser{} +} + +func (f *fakeBrowser) open(rawURL string) error { + f.mu.Lock() + f.authorizeURL = rawURL + f.mu.Unlock() + + if f.openErr != nil { + return f.openErr + } + if f.suppress { + return nil + } + + authorizeURL, err := url.Parse(rawURL) + if err != nil { + return err + } + callback, err := url.Parse(authorizeURL.Query().Get("redirect_uri")) + if err != nil { + return err + } + + q := url.Values{} + q.Set("code", testAuthCode) + q.Set("state", authorizeURL.Query().Get("state")) + if f.tamper != nil { + f.tamper(q) + } + callback.RawQuery = q.Encode() + + go func() { + req, reqErr := http.NewRequestWithContext(context.Background(), http.MethodGet, callback.String(), nil) + if reqErr != nil { + return + } + //nolint:gosec // G704: a loopback callback URL derived from the authorize URL this test built. + resp, doErr := http.DefaultClient.Do(req) + if doErr != nil { + return + } + _ = resp.Body.Close() + }() + + return nil +} + +func (f *fakeBrowser) capturedURL() string { + f.mu.Lock() + defer f.mu.Unlock() + return f.authorizeURL +} + +// mustFreeAddr reserves and releases a loopback port, returning its address. +func mustFreeAddr(t *testing.T) string { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + addr := ln.Addr().String() + require.NoError(t, ln.Close()) + return addr +} diff --git a/cognito/pkce.go b/cognito/pkce.go new file mode 100644 index 0000000..63572c6 --- /dev/null +++ b/cognito/pkce.go @@ -0,0 +1,56 @@ +package cognito + +import ( + "crypto/rand" + "crypto/sha256" + "crypto/subtle" + "encoding/base64" + "fmt" + + wegostrings "github.com/wego/pkg/strings" +) + +const ( + // verifierBytes yields a 128-character unpadded base64url verifier, the + // longest RFC 7636 permits for code_verifier. + verifierBytes = 96 + + // stateBytes yields a 32-character unpadded base64url state value. + stateBytes = 24 +) + +// generateVerifier draws a fresh PKCE code_verifier from crypto/rand. +func generateVerifier() (string, error) { + buf := make([]byte, verifierBytes) + if _, err := rand.Read(buf); err != nil { + return "", fmt.Errorf("generate pkce verifier: %w", err) + } + return base64.RawURLEncoding.EncodeToString(buf), nil +} + +// generateChallenge derives the S256 code_challenge for a verifier. Both the +// verifier and the challenge are unpadded base64url, per RFC 7636. +func generateChallenge(verifier string) string { + sum := sha256.Sum256([]byte(verifier)) + return base64.RawURLEncoding.EncodeToString(sum[:]) +} + +// generateState draws a fresh OAuth state value from crypto/rand. +func generateState() (string, error) { + buf := make([]byte, stateBytes) + if _, err := rand.Read(buf); err != nil { + return "", fmt.Errorf("generate oauth state: %w", err) + } + return base64.RawURLEncoding.EncodeToString(buf), nil +} + +// stateMatches compares the state a callback presented against the one we +// minted, in constant time. A blank value on either side never matches: a +// callback that omits state is precisely the CSRF case the parameter exists to +// catch, so it must be rejected rather than waved through. +func stateMatches(want, got string) bool { + if wegostrings.IsBlank(want) || wegostrings.IsBlank(got) { + return false + } + return subtle.ConstantTimeCompare([]byte(want), []byte(got)) == 1 +} diff --git a/cognito/pkce_internal_test.go b/cognito/pkce_internal_test.go new file mode 100644 index 0000000..7cda37a --- /dev/null +++ b/cognito/pkce_internal_test.go @@ -0,0 +1,104 @@ +package cognito + +import ( + "crypto/sha256" + "encoding/base64" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGenerateVerifier(t *testing.T) { + verifier, err := generateVerifier() + require.NoError(t, err) + + assert.GreaterOrEqual(t, len(verifier), 43, "RFC 7636 requires a code_verifier of at least 43 chars, got %q", verifier) + assert.LessOrEqual(t, len(verifier), 128, "RFC 7636 caps code_verifier at 128 chars, got %d chars", len(verifier)) + assert.NotContains(t, verifier, "=", "code_verifier must be base64url WITHOUT padding") + assert.NotContains(t, verifier, "+", "code_verifier must be base64url, not standard base64") + assert.NotContains(t, verifier, "/", "code_verifier must be base64url, not standard base64") + + _, err = base64.RawURLEncoding.DecodeString(verifier) + require.NoError(t, err, "code_verifier must decode as unpadded base64url") +} + +func TestGenerateVerifier_IsUniquePerCall(t *testing.T) { + seen := make(map[string]struct{}, 32) + for range 32 { + verifier, err := generateVerifier() + require.NoError(t, err) + _, dup := seen[verifier] + require.False(t, dup, "generateVerifier returned a duplicate; it must be drawn from crypto/rand") + seen[verifier] = struct{}{} + } +} + +func TestGenerateChallenge(t *testing.T) { + tests := []struct { + name string + givenVerifier string + want string + }{ + { + // RFC 7636 appendix B test vector - pins the S256 transformation. + name: "rfc 7636 appendix b vector", + givenVerifier: "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk", + want: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", + }, + { + name: "empty verifier still hashes", + givenVerifier: "", + want: base64.RawURLEncoding.EncodeToString(sha256Sum("")), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := generateChallenge(tt.givenVerifier) + assert.Equal(t, tt.want, got, "challenge for verifier %q", tt.givenVerifier) + assert.NotContains(t, got, "=", "code_challenge must be unpadded base64url") + }) + } +} + +func TestGenerateState(t *testing.T) { + state, err := generateState() + require.NoError(t, err) + + require.NotEmpty(t, state) + assert.NotContains(t, state, "=", "state must be unpadded base64url") + _, err = base64.RawURLEncoding.DecodeString(state) + require.NoError(t, err, "state must decode as unpadded base64url") + + other, err := generateState() + require.NoError(t, err) + assert.NotEqual(t, state, other, "state must be freshly drawn from crypto/rand on every call") +} + +func TestStateMatches(t *testing.T) { + tests := []struct { + name string + givenWant string + givenGot string + want bool + }{ + {name: "mismatch", givenWant: "abc", givenGot: "abd", want: false}, + {name: "missing callback state", givenWant: "abc", givenGot: "", want: false}, + {name: "both blank is still a mismatch", givenWant: "", givenGot: "", want: false}, + {name: "different length", givenWant: "abc", givenGot: "abcdef", want: false}, + {name: "match", givenWant: "abc", givenGot: "abc", want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, stateMatches(tt.givenWant, tt.givenGot)) + }) + } +} + +// sha256Sum computes the expectation independently of the code under test. +func sha256Sum(s string) []byte { + sum := sha256.Sum256([]byte(s)) + return sum[:] +} diff --git a/cognito/storage/keyring.go b/cognito/storage/keyring.go new file mode 100644 index 0000000..dce0c91 --- /dev/null +++ b/cognito/storage/keyring.go @@ -0,0 +1,190 @@ +package storage + +import ( + "encoding/json" + "errors" + "fmt" + "time" + + wegostrings "github.com/wego/pkg/strings" + "github.com/zalando/go-keyring" + + "github.com/wego/pkg/cognito" +) + +// A Cognito JWT - the access token especially - can exceed the 4 KiB +// command-line cap zalando/go-keyring runs into on macOS, where it shells out +// to /usr/bin/security. So each field gets its own keychain entry, keeping +// every individual write well under that ceiling. +// +// Entries are accounted as "/". +const ( + fieldAccess = "access" + fieldID = "id" + fieldRefresh = "refresh" + fieldMeta = "meta" +) + +// keyringBackend is the slice of zalando/go-keyring this package depends on. +// Naming it lets tests substitute a fake instead of prompting a real keychain. +type keyringBackend interface { + Set(service, user, password string) error + Get(service, user string) (string, error) + Delete(service, user string) error +} + +// systemKeyring is the real OS keychain. +type systemKeyring struct{} + +func (systemKeyring) Set(service, user, password string) error { + return keyring.Set(service, user, password) +} + +func (systemKeyring) Get(service, user string) (string, error) { + return keyring.Get(service, user) +} + +func (systemKeyring) Delete(service, user string) error { + return keyring.Delete(service, user) +} + +// keyringMeta is the non-secret metadata, kept in one small entry beside the +// tokens themselves. +type keyringMeta struct { + ExpiresAt time.Time `json:"expires_at"` +} + +// keyringStore persists tokens in the OS keychain. +type keyringStore struct { + service string + backend keyringBackend +} + +// NewKeyring returns a Store backed by the operating system keychain, with +// every entry filed under service. +func NewKeyring(service string) Store { + return &keyringStore{service: service, backend: systemKeyring{}} +} + +// Load returns the tokens stored under namespace, or (nil, nil) if the +// operator is not signed in. +func (k *keyringStore) Load(namespace string) (*cognito.TokenSet, error) { + if err := k.validate(namespace); err != nil { + return nil, err + } + + // Gate on the access token: its absence means "not signed in", which is a + // normal state rather than a failure. Once it is present, anything else + // missing is a real inconsistency and must be reported. + access, err := k.backend.Get(k.service, account(namespace, fieldAccess)) + if errors.Is(err, keyring.ErrNotFound) { + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("read the keychain: %w (is the system keychain unlocked?)", err) + } + + idToken, err := k.read(namespace, fieldID) + if err != nil { + return nil, err + } + refresh, err := k.read(namespace, fieldRefresh) + if err != nil { + return nil, err + } + rawMeta, err := k.read(namespace, fieldMeta) + if err != nil { + return nil, err + } + + var meta keyringMeta + if err := json.Unmarshal([]byte(rawMeta), &meta); err != nil { + return nil, fmt.Errorf("parse token metadata from the keychain: %w", err) + } + + return &cognito.TokenSet{ + AccessToken: access, + IDToken: idToken, + RefreshToken: refresh, + ExpiresAt: meta.ExpiresAt, + }, nil +} + +// Save writes tokens under namespace, replacing anything already there. +func (k *keyringStore) Save(namespace string, tokens *cognito.TokenSet) error { + if err := k.validate(namespace); err != nil { + return err + } + if tokens == nil { + return errNoTokens + } + + meta, err := json.Marshal(keyringMeta{ExpiresAt: tokens.ExpiresAt}) + if err != nil { + return fmt.Errorf("encode token metadata: %w", err) + } + + // The access token is written LAST because Load gates on it: a write cut + // short partway through then reads as "not signed in", which is + // recoverable, rather than as a half-populated token set, which is not. + entries := []struct { + field string + value string + }{ + {field: fieldRefresh, value: tokens.RefreshToken}, + {field: fieldID, value: tokens.IDToken}, + {field: fieldMeta, value: string(meta)}, + {field: fieldAccess, value: tokens.AccessToken}, + } + + for _, entry := range entries { + if err := k.backend.Set(k.service, account(namespace, entry.field), entry.value); err != nil { + return fmt.Errorf("write %s to the keychain: %w (is the system keychain unlocked?)", entry.field, err) + } + } + + return nil +} + +// Delete removes every entry under namespace. Entries already gone are fine: +// the desired end state is "signed out", so signing out twice must succeed. +func (k *keyringStore) Delete(namespace string) error { + if err := k.validate(namespace); err != nil { + return err + } + + for _, field := range []string{fieldAccess, fieldID, fieldRefresh, fieldMeta} { + err := k.backend.Delete(k.service, account(namespace, field)) + if err != nil && !errors.Is(err, keyring.ErrNotFound) { + return fmt.Errorf("delete %s from the keychain: %w (is the system keychain unlocked?)", field, err) + } + } + + return nil +} + +// read fetches one entry, treating a missing one as an error: callers only +// reach it after the access-token gate has confirmed a login exists. +func (k *keyringStore) read(namespace, field string) (string, error) { + value, err := k.backend.Get(k.service, account(namespace, field)) + if err != nil { + return "", fmt.Errorf("read %s from the keychain: %w", field, err) + } + return value, nil +} + +// validate checks the inputs every operation needs. +func (k *keyringStore) validate(namespace string) error { + if wegostrings.IsBlank(k.service) { + return errors.New("keychain service name must not be blank") + } + if wegostrings.IsBlank(namespace) { + return errBlankNamespace + } + return nil +} + +// account is the keychain account name for one field of one namespace. +func account(namespace, field string) string { + return namespace + "/" + field +} diff --git a/cognito/storage/keyring_internal_test.go b/cognito/storage/keyring_internal_test.go new file mode 100644 index 0000000..5415498 --- /dev/null +++ b/cognito/storage/keyring_internal_test.go @@ -0,0 +1,398 @@ +package storage + +import ( + "errors" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + wegostrings "github.com/wego/pkg/strings" + "github.com/zalando/go-keyring" + + "github.com/wego/pkg/cognito" +) + +const ( + testService = "pay-admin-test" + testNamespace = "pay-admin/staging" +) + +var testExpiry = time.Date(2026, 9, 1, 12, 0, 0, 0, time.UTC) + +// TestStoreContract holds every Store implementation to the same behaviour. +// The keyring case runs against a fake backend, so no test ever pops an OS +// keychain prompt. +func TestStoreContract(t *testing.T) { + impls := []struct { + name string + build func() Store + }{ + { + name: "memory", + build: NewMemory, + }, + { + name: "keyring", + build: func() Store { + return &keyringStore{service: testService, backend: newFakeKeyring()} + }, + }, + } + + tests := []struct { + name string + run func(t *testing.T, store Store) + }{ + { + name: "a blank namespace is rejected", + run: func(t *testing.T, store Store) { + _, err := store.Load("") + require.Error(t, err) + require.Error(t, store.Save("", sampleTokens())) + require.Error(t, store.Delete("")) + }, + }, + { + name: "saving nil tokens is rejected", + run: func(t *testing.T, store Store) { + require.Error(t, store.Save(testNamespace, nil)) + }, + }, + { + name: "loading an unknown namespace is not an error", + run: func(t *testing.T, store Store) { + got, err := store.Load(testNamespace) + require.NoError(t, err, "not logged in is a normal state, not a failure") + assert.Nil(t, got) + }, + }, + { + name: "deleting an unknown namespace is not an error", + run: func(t *testing.T, store Store) { + require.NoError(t, store.Delete(testNamespace)) + }, + }, + { + name: "delete clears a saved token set", + run: func(t *testing.T, store Store) { + require.NoError(t, store.Save(testNamespace, sampleTokens())) + require.NoError(t, store.Delete(testNamespace)) + + got, err := store.Load(testNamespace) + require.NoError(t, err) + assert.Nil(t, got) + }, + }, + { + name: "namespaces do not collide", + run: func(t *testing.T, store Store) { + staging := sampleTokens() + staging.AccessToken = "staging-access" + production := sampleTokens() + production.AccessToken = "production-access" + + require.NoError(t, store.Save("pay-admin/staging", staging)) + require.NoError(t, store.Save("pay-admin/production", production)) + + got, err := store.Load("pay-admin/staging") + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, "staging-access", got.AccessToken) + + require.NoError(t, store.Delete("pay-admin/staging")) + + survivor, err := store.Load("pay-admin/production") + require.NoError(t, err) + require.NotNil(t, survivor, "deleting one namespace must not touch another") + assert.Equal(t, "production-access", survivor.AccessToken) + }, + }, + { + name: "save then load round-trips every field", + run: func(t *testing.T, store Store) { + want := sampleTokens() + require.NoError(t, store.Save(testNamespace, want)) + + got, err := store.Load(testNamespace) + require.NoError(t, err) + require.NotNil(t, got) + + assert.Equal(t, want.AccessToken, got.AccessToken) + assert.Equal(t, want.IDToken, got.IDToken) + assert.Equal(t, want.RefreshToken, got.RefreshToken) + assert.Equal(t, want.ExpiresAt.UTC(), got.ExpiresAt.UTC()) + }, + }, + { + name: "save overwrites an earlier token set", + run: func(t *testing.T, store Store) { + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + replacement := sampleTokens() + replacement.AccessToken = "second-access" + require.NoError(t, store.Save(testNamespace, replacement)) + + got, err := store.Load(testNamespace) + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, "second-access", got.AccessToken) + }, + }, + { + name: "the loaded token set is a copy", + run: func(t *testing.T, store Store) { + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + first, err := store.Load(testNamespace) + require.NoError(t, err) + require.NotNil(t, first) + first.AccessToken = "mutated-by-caller" + + second, err := store.Load(testNamespace) + require.NoError(t, err) + require.NotNil(t, second) + assert.Equal(t, "access-value", second.AccessToken, "a caller must not be able to mutate stored tokens") + }, + }, + } + + for _, impl := range impls { + for _, tt := range tests { + t.Run(impl.name+"/"+tt.name, func(t *testing.T) { + tt.run(t, impl.build()) + }) + } + } +} + +func TestKeyringStore_LoadFailures(t *testing.T) { + tests := []struct { + name string + givenSetup func(*fakeKeyring) + wantNil bool + wantErrContains string + }{ + { + name: "a keychain failure on the gate entry is reported", + givenSetup: func(f *fakeKeyring) { + f.getErr = errors.New("keychain is locked") + }, + wantErrContains: "keychain", + }, + { + name: "a missing id_token is an inconsistency, not a logged-out state", + givenSetup: func(f *fakeKeyring) { + f.put(fieldAccess, "access-value") + }, + wantErrContains: "id", + }, + { + name: "a missing refresh_token is an inconsistency", + givenSetup: func(f *fakeKeyring) { + f.put(fieldAccess, "access-value") + f.put(fieldID, "id-value") + }, + wantErrContains: "refresh", + }, + { + name: "missing metadata is an inconsistency", + givenSetup: func(f *fakeKeyring) { + f.put(fieldAccess, "access-value") + f.put(fieldID, "id-value") + f.put(fieldRefresh, "refresh-value") + }, + wantErrContains: "meta", + }, + { + name: "corrupt metadata is reported", + givenSetup: func(f *fakeKeyring) { + f.put(fieldAccess, "access-value") + f.put(fieldID, "id-value") + f.put(fieldRefresh, "refresh-value") + f.put(fieldMeta, "{not json") + }, + wantErrContains: "metadata", + }, + { + name: "nothing stored reads as not logged in", + givenSetup: func(_ *fakeKeyring) {}, + wantNil: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + backend := newFakeKeyring() + tt.givenSetup(backend) + store := &keyringStore{service: testService, backend: backend} + + got, err := store.Load(testNamespace) + + if wegostrings.IsNotEmpty(tt.wantErrContains) { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErrContains) + assert.Nil(t, got) + return + } + require.NoError(t, err) + assert.Nil(t, got) + }) + } +} + +// TestKeyringStore_SaveSplitsFieldsAcrossEntries pins the workaround for the +// 4 KiB argument cap that zalando/go-keyring hits on macOS: one entry per +// field, with the access token written last so a torn write reads as +// "not logged in" rather than as a half-populated token set. +func TestKeyringStore_SaveSplitsFieldsAcrossEntries(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + assert.Equal(t, []string{ + testNamespace + "/refresh", + testNamespace + "/id", + testNamespace + "/meta", + testNamespace + "/access", + }, backend.writes, "one namespaced entry per field, access token last") + + assert.Equal(t, "access-value", backend.value(fieldAccess)) + assert.Equal(t, "refresh-value", backend.value(fieldRefresh)) +} + +func TestKeyringStore_BackendFailuresAreReported(t *testing.T) { + tests := []struct { + name string + givenSetup func(*fakeKeyring) + givenAction func(Store) error + wantErrContains string + }{ + { + name: "a save failure is reported", + givenSetup: func(f *fakeKeyring) { + f.setErr = errors.New("keychain is locked") + }, + givenAction: func(s Store) error { return s.Save(testNamespace, sampleTokens()) }, + wantErrContains: "keychain", + }, + { + name: "a delete failure is reported", + givenSetup: func(f *fakeKeyring) { + f.deleteErr = errors.New("keychain is locked") + }, + givenAction: func(s Store) error { return s.Delete(testNamespace) }, + wantErrContains: "keychain", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + backend := newFakeKeyring() + tt.givenSetup(backend) + store := &keyringStore{service: testService, backend: backend} + + err := tt.givenAction(store) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErrContains) + }) + } +} + +// TestKeyringStore_DeleteToleratesMissingEntries covers a half-populated +// namespace: sign-out must succeed rather than stranding the operator. +func TestKeyringStore_DeleteToleratesMissingEntries(t *testing.T) { + backend := newFakeKeyring() + backend.put(fieldAccess, "access-value") + store := &keyringStore{service: testService, backend: backend} + + require.NoError(t, store.Delete(testNamespace)) + assert.Empty(t, backend.entries) +} + +func TestKeyringStore_BlankServiceIsRejected(t *testing.T) { + store := &keyringStore{service: "", backend: newFakeKeyring()} + + _, err := store.Load(testNamespace) + require.Error(t, err) + assert.Contains(t, err.Error(), "service") + require.Error(t, store.Save(testNamespace, sampleTokens())) + require.Error(t, store.Delete(testNamespace)) +} + +func sampleTokens() *cognito.TokenSet { + return &cognito.TokenSet{ + AccessToken: "access-value", + IDToken: "id-value", + RefreshToken: "refresh-value", + ExpiresAt: testExpiry, + } +} + +// fakeKeyring is an in-memory stand-in for the OS keychain. +type fakeKeyring struct { + mu sync.Mutex + entries map[string]string + writes []string + setErr error + getErr error + deleteErr error +} + +func newFakeKeyring() *fakeKeyring { + return &fakeKeyring{entries: make(map[string]string)} +} + +func (f *fakeKeyring) Set(service, user, password string) error { + f.mu.Lock() + defer f.mu.Unlock() + if f.setErr != nil { + return f.setErr + } + f.entries[service+"|"+user] = password + f.writes = append(f.writes, user) + return nil +} + +func (f *fakeKeyring) Get(service, user string) (string, error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.getErr != nil { + return "", f.getErr + } + value, ok := f.entries[service+"|"+user] + if !ok { + return "", keyring.ErrNotFound + } + return value, nil +} + +func (f *fakeKeyring) Delete(service, user string) error { + f.mu.Lock() + defer f.mu.Unlock() + if f.deleteErr != nil { + return f.deleteErr + } + key := service + "|" + user + if _, ok := f.entries[key]; !ok { + return keyring.ErrNotFound + } + delete(f.entries, key) + return nil +} + +// put seeds an entry under testNamespace, bypassing the store under test. +func (f *fakeKeyring) put(field, value string) { + f.mu.Lock() + defer f.mu.Unlock() + f.entries[testService+"|"+testNamespace+"/"+field] = value +} + +// value reads an entry under testNamespace, bypassing the store under test. +func (f *fakeKeyring) value(field string) string { + f.mu.Lock() + defer f.mu.Unlock() + return f.entries[testService+"|"+testNamespace+"/"+field] +} diff --git a/cognito/storage/store.go b/cognito/storage/store.go new file mode 100644 index 0000000..abd6bf0 --- /dev/null +++ b/cognito/storage/store.go @@ -0,0 +1,99 @@ +// Package storage persists the Cognito token sets obtained by +// github.com/wego/pkg/cognito. +// +// Tokens live under an opaque namespace, e.g. "my-cli/staging". The caller +// chooses the string and the store never interprets it; namespacing is what +// keeps a staging login from overwriting a production one. +// +// Use NewMemory in tests and anywhere a keychain is unavailable; it satisfies +// the same interface as NewKeyring, so a caller swaps one for the other without +// touching its own code. +package storage + +import ( + "errors" + "sync" + + wegostrings "github.com/wego/pkg/strings" + + "github.com/wego/pkg/cognito" +) + +// Store persists a TokenSet under an opaque namespace. +// +// A missing entry is NOT an error: Load returns (nil, nil) when nothing is +// stored, because "not logged in" is a normal state. Callers can therefore +// tell it apart from a backend that is genuinely broken, which does return an +// error. +type Store interface { + Load(namespace string) (*cognito.TokenSet, error) + Save(namespace string, tokens *cognito.TokenSet) error + Delete(namespace string) error +} + +var ( + errBlankNamespace = errors.New("storage namespace must not be blank") + errNoTokens = errors.New("no tokens to save") +) + +// memoryStore keeps tokens in process memory only. +type memoryStore struct { + mu sync.RWMutex + tokens map[string]cognito.TokenSet +} + +// NewMemory returns a Store that holds tokens for the lifetime of the process +// and nothing longer. It serves tests and callers with no usable keychain; a +// new process always starts signed out. +func NewMemory() Store { + return &memoryStore{tokens: make(map[string]cognito.TokenSet)} +} + +// Load returns the tokens stored under namespace, or (nil, nil) if there are +// none. +func (m *memoryStore) Load(namespace string) (*cognito.TokenSet, error) { + if wegostrings.IsBlank(namespace) { + return nil, errBlankNamespace + } + + m.mu.RLock() + defer m.mu.RUnlock() + + stored, ok := m.tokens[namespace] + if !ok { + return nil, nil + } + + // stored is already a copy of the map value, so a caller mutating the + // result cannot reach back into the store. + return &stored, nil +} + +// Save writes tokens under namespace, replacing anything already there. +func (m *memoryStore) Save(namespace string, tokens *cognito.TokenSet) error { + if wegostrings.IsBlank(namespace) { + return errBlankNamespace + } + if tokens == nil { + return errNoTokens + } + + m.mu.Lock() + defer m.mu.Unlock() + m.tokens[namespace] = *tokens + + return nil +} + +// Delete removes the tokens under namespace. Deleting nothing is not an error. +func (m *memoryStore) Delete(namespace string) error { + if wegostrings.IsBlank(namespace) { + return errBlankNamespace + } + + m.mu.Lock() + defer m.mu.Unlock() + delete(m.tokens, namespace) + + return nil +} diff --git a/cognito/storage/store_test.go b/cognito/storage/store_test.go new file mode 100644 index 0000000..5b33b23 --- /dev/null +++ b/cognito/storage/store_test.go @@ -0,0 +1,80 @@ +package storage_test + +import ( + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/wego/pkg/cognito" + "github.com/wego/pkg/cognito/storage" +) + +func TestNewMemory_RoundTripsThroughThePublicAPI(t *testing.T) { + store := storage.NewMemory() + want := &cognito.TokenSet{ + AccessToken: "access-value", + IDToken: "id-value", + RefreshToken: "refresh-value", + ExpiresAt: time.Date(2026, 9, 1, 12, 0, 0, 0, time.UTC), + } + + before, err := store.Load("pay-admin/staging") + require.NoError(t, err, "an empty store must report not-logged-in, not an error") + require.Nil(t, before) + + require.NoError(t, store.Save("pay-admin/staging", want)) + + got, err := store.Load("pay-admin/staging") + require.NoError(t, err) + require.NotNil(t, got) + assert.Equal(t, *want, *got) + + require.NoError(t, store.Delete("pay-admin/staging")) + + after, err := store.Load("pay-admin/staging") + require.NoError(t, err) + assert.Nil(t, after) +} + +// TestNewKeyring_ValidatesBeforeTouchingTheKeychain exercises the exported +// constructor without ever reaching the OS keychain: every case here is +// rejected by validation first, so no test can pop a keychain prompt. +func TestNewKeyring_ValidatesBeforeTouchingTheKeychain(t *testing.T) { + tests := []struct { + name string + givenService string + givenNamespace string + }{ + { + name: "a blank service is rejected", + givenService: "", + givenNamespace: "pay-admin/staging", + }, + { + name: "a blank namespace is rejected", + givenService: "pay-admin", + givenNamespace: "", + }, + { + name: "both blank is rejected", + givenService: "", + givenNamespace: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + store := storage.NewKeyring(tt.givenService) + require.NotNil(t, store) + + got, err := store.Load(tt.givenNamespace) + require.Error(t, err) + assert.Nil(t, got) + + require.Error(t, store.Save(tt.givenNamespace, &cognito.TokenSet{AccessToken: "access-value"})) + require.Error(t, store.Delete(tt.givenNamespace)) + }) + } +} diff --git a/cognito/tokens.go b/cognito/tokens.go new file mode 100644 index 0000000..b9d64e9 --- /dev/null +++ b/cognito/tokens.go @@ -0,0 +1,88 @@ +package cognito + +import ( + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "strings" + "time" + + wegostrings "github.com/wego/pkg/strings" +) + +// expiryLeeway is shaved off the real expiry so a token cannot lapse midway +// through a request we have already started. +const expiryLeeway = 60 * time.Second + +// TokenSet is the set of Cognito tokens a signed-in operator holds. +// +// Holding tokens is the whole point of the type, so gosec's secret-field +// warning is expected here rather than a finding: these values are never +// logged, and the only place they are serialized to is the operator's own +// keychain (see github.com/wego/pkg/cognito/storage). +type TokenSet struct { + AccessToken string `json:"access_token"` //nolint:gosec // G117: this type exists to carry tokens; stored only in the local keychain, never logged. + IDToken string `json:"id_token"` + RefreshToken string `json:"refresh_token"` //nolint:gosec // G117: as above - the refresh token is the credential this type is for. + ExpiresAt time.Time `json:"expires_at"` +} + +// IsExpired reports whether the access token is spent as of now, treating +// anything inside expiryLeeway of the deadline as already gone. A nil set, or +// one with no recorded expiry, counts as expired so callers refresh rather +// than send a token that may already be dead. +func (t *TokenSet) IsExpired(now time.Time) bool { + if t == nil || t.ExpiresAt.IsZero() { + return true + } + return !now.Before(t.ExpiresAt.Add(-expiryLeeway)) +} + +// Email returns the email claim from the id_token. +// +// It reads the JWT payload WITHOUT verifying the signature, which is correct +// here: the token came from our own keychain, and every server that accepts it +// verifies the signature itself. Treat this as decoding for display and +// client-side UX, NOT as validation - a caller must never make an +// authorization decision on the strength of these claims. +func (t *TokenSet) Email() (string, error) { + if t == nil { + return "", errors.New("no tokens: sign in first") + } + + claims, err := decodeIDTokenClaims(t.IDToken) + if err != nil { + return "", err + } + + email, ok := claims["email"].(string) + if !ok || wegostrings.IsBlank(email) { + return "", errors.New("id_token has no email claim") + } + return email, nil +} + +// decodeIDTokenClaims splits a JWT and JSON-decodes its payload. It never +// panics on a malformed or truncated token: every failure is an error. +func decodeIDTokenClaims(idToken string) (map[string]any, error) { + parts := strings.Split(idToken, ".") + if len(parts) != 3 { + return nil, errors.New("id_token is not a jwt") + } + + payload, err := base64.RawURLEncoding.DecodeString(parts[1]) + if err != nil { + // Some encoders pad their segments; accept those too. + payload, err = base64.URLEncoding.DecodeString(parts[1]) + if err != nil { + return nil, fmt.Errorf("decode id_token payload: %w", err) + } + } + + var claims map[string]any + if err := json.Unmarshal(payload, &claims); err != nil { + return nil, fmt.Errorf("parse id_token payload: %w", err) + } + return claims, nil +} diff --git a/cognito/tokens_test.go b/cognito/tokens_test.go new file mode 100644 index 0000000..96ede3a --- /dev/null +++ b/cognito/tokens_test.go @@ -0,0 +1,172 @@ +package cognito_test + +import ( + "encoding/base64" + "encoding/json" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + wegostrings "github.com/wego/pkg/strings" + + "github.com/wego/pkg/cognito" +) + +const testOperatorEmail = "ops@wego.com" + +var fixedNow = time.Date(2026, 9, 1, 12, 0, 0, 0, time.UTC) + +func TestTokenSet_IsExpired(t *testing.T) { + tests := []struct { + name string + given *cognito.TokenSet + want bool + }{ + { + name: "nil token set counts as expired", + given: nil, + want: true, + }, + { + name: "zero expiry counts as expired", + given: &cognito.TokenSet{}, + want: true, + }, + { + name: "already elapsed", + given: &cognito.TokenSet{ExpiresAt: fixedNow.Add(-time.Second)}, + want: true, + }, + { + name: "inside the safety skew", + given: &cognito.TokenSet{ExpiresAt: fixedNow.Add(30 * time.Second)}, + want: true, + }, + { + name: "exactly at the safety skew", + given: &cognito.TokenSet{ExpiresAt: fixedNow.Add(60 * time.Second)}, + want: true, + }, + { + name: "just beyond the safety skew", + given: &cognito.TokenSet{ExpiresAt: fixedNow.Add(61 * time.Second)}, + want: false, + }, + { + name: "an hour of life left", + given: &cognito.TokenSet{ExpiresAt: fixedNow.Add(time.Hour)}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, tt.given.IsExpired(fixedNow), "evaluated at %v", fixedNow) + }) + } +} + +func TestTokenSet_Email(t *testing.T) { + tests := []struct { + name string + givenIDToken string + want string + wantErrContains string + }{ + { + name: "blank id_token is rejected", + givenIDToken: "", + wantErrContains: "not a jwt", + }, + { + name: "two-segment token is not a jwt", + givenIDToken: "header.payload", + wantErrContains: "not a jwt", + }, + { + name: "truncated payload does not panic", + givenIDToken: "header.!!!not-base64!!!.signature", + wantErrContains: "decode", + }, + { + name: "payload is not json", + givenIDToken: "header." + rawB64("this is not json") + ".signature", + wantErrContains: "parse", + }, + { + name: "payload is a json array not an object", + givenIDToken: "header." + rawB64(`["nope"]`) + ".signature", + wantErrContains: "parse", + }, + { + name: "no email claim", + givenIDToken: "header." + rawB64(`{"sub":"abc"}`) + ".signature", + wantErrContains: "no email claim", + }, + { + name: "email claim is not a string", + givenIDToken: "header." + rawB64(`{"email":42}`) + ".signature", + wantErrContains: "no email claim", + }, + { + name: "email claim is blank", + givenIDToken: "header." + rawB64(`{"email":""}`) + ".signature", + wantErrContains: "no email claim", + }, + { + name: "padded base64 payload still decodes", + givenIDToken: "header." + base64.URLEncoding.EncodeToString([]byte(`{"email":"`+testOperatorEmail+`"}`)) + ".signature", + want: testOperatorEmail, + }, + { + name: "email is returned", + givenIDToken: "header." + rawB64(`{"email":"`+testOperatorEmail+`"}`) + ".signature", + want: testOperatorEmail, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ts := &cognito.TokenSet{IDToken: tt.givenIDToken} + + var ( + got string + err error + ) + require.NotPanics(t, func() { got, err = ts.Email() }, "Email must never panic on a malformed id_token") + + if wegostrings.IsNotEmpty(tt.wantErrContains) { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErrContains) + assert.Empty(t, got) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestTokenSet_EmailOnNilReceiver(t *testing.T) { + var ts *cognito.TokenSet + + var err error + require.NotPanics(t, func() { _, err = ts.Email() }) + require.Error(t, err) +} + +// rawB64 is unpadded base64url, the encoding JWT segments use. +func rawB64(s string) string { + return base64.RawURLEncoding.EncodeToString([]byte(s)) +} + +// mustIDToken builds an unsigned JWT carrying claims. The signature is +// deliberately junk: Email() must not verify it. +func mustIDToken(t *testing.T, claims map[string]any) string { + t.Helper() + payload, err := json.Marshal(claims) + require.NoError(t, err) + return rawB64(`{"alg":"RS256","typ":"JWT"}`) + "." + + base64.RawURLEncoding.EncodeToString(payload) + ".not-a-real-signature" +} From 699ef695d2ee498ca9eb6a006fab02d6ee1a2868 Mon Sep 17 00:00:00 2001 From: mysqto Date: Wed, 2 Sep 2026 09:43:36 +0800 Subject: [PATCH 02/12] cognito: handle the callback writer's error explicitly revive's unhandled-error rule flags a bare fmt.Fprint. The write genuinely cannot be acted on -- the browser tab is the only reader and the operator sees the real outcome in the terminal -- so the discard is explicit rather than implicit. Co-Authored-By: Claude Opus 5 (1M context) --- cognito/callback.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cognito/callback.go b/cognito/callback.go index 9b51b18..ae9d29f 100644 --- a/cognito/callback.go +++ b/cognito/callback.go @@ -102,7 +102,7 @@ func callbackHandler(results chan<- callbackResult) http.HandlerFunc { message += ": " + desc } w.WriteHeader(http.StatusBadRequest) - fmt.Fprint(w, failureHTML) + _, _ = fmt.Fprint(w, failureHTML) deliver(results, callbackResult{err: fmt.Errorf("authorization server rejected the sign-in: %s", message)}) return } @@ -110,12 +110,12 @@ func callbackHandler(results chan<- callbackResult) http.HandlerFunc { code := query.Get("code") if wegostrings.IsBlank(code) { w.WriteHeader(http.StatusBadRequest) - fmt.Fprint(w, failureHTML) + _, _ = fmt.Fprint(w, failureHTML) deliver(results, callbackResult{err: errors.New("the sign-in redirect carried no authorization code")}) return } - fmt.Fprint(w, successHTML) + _, _ = fmt.Fprint(w, successHTML) deliver(results, callbackResult{code: code, state: query.Get("state")}) } } From 1150429faab986c76ee797fd9bf3bd123d4c32a6 Mon Sep 17 00:00:00 2001 From: mysqto Date: Wed, 2 Sep 2026 09:57:53 +0800 Subject: [PATCH 03/12] cognito: add a no-browser sign-in option Login could only reach the operator by launching a browser, which rules out a headless shell, a terminal on a remote host, and any caller that wants to surface the URL its own way. Config.NoBrowser suppresses the launch and Config.PromptURL receives the authorize URL instead; everything else is unchanged, so the same PKCE challenge and state are sent and the code still arrives on the loopback listener. PromptURL is a func rather than a bool-plus-stdout so the library never writes to a stream the caller did not choose -- it can print, render a QR code, or hand the URL to another process. Setting NoBrowser without PromptURL is refused at validation: with no browser launched and no way to report the url, the operator has nothing to open. One limit is documented on the field rather than papered over: the redirect still lands on CallbackAddr, so a browser on a different machine than the CLI needs that port forwarded. Suppressing the launch does not move where the code is delivered. Co-Authored-By: Claude Opus 5 (1M context) --- cognito/oauth.go | 42 +++++++++++++++++++++++++++-- cognito/oauth_test.go | 62 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) diff --git a/cognito/oauth.go b/cognito/oauth.go index 6cda020..645110c 100644 --- a/cognito/oauth.go +++ b/cognito/oauth.go @@ -73,6 +73,23 @@ type Config struct { // OpenBrowser launches the authorize URL. Nil uses the OS default opener. OpenBrowser func(string) error + // NoBrowser suppresses the browser launch. Login reports the authorize URL + // through PromptURL instead and then waits on the loopback listener exactly + // as it otherwise would, for a headless shell, a terminal on a remote host, + // or an operator who would rather open the URL themselves. + // + // The redirect still lands on CallbackAddr, so when the browser runs on a + // different machine than the CLI that port has to be reachable from it — + // usually `ssh -L :localhost:`. Suppressing the launch does not + // move where the code is delivered. + NoBrowser bool + + // PromptURL receives the authorize URL in place of a browser launch, so the + // caller decides how to surface it: print it, render a QR code, hand it to + // another process. Required when NoBrowser is set — a sign-in whose URL the + // operator never sees cannot complete — and ignored otherwise. + PromptURL func(url string) error + // HTTPClient calls the token endpoint. Nil uses a client with a timeout. HTTPClient *http.Client @@ -106,8 +123,8 @@ func Login(ctx context.Context, cfg Config) (*TokenSet, error) { } defer server.shutdown() - if err := cfg.openBrowserAt(cfg.buildAuthorizeURL(state, generateChallenge(verifier))); err != nil { - return nil, fmt.Errorf("open browser for sign-in: %w", err) + if err := cfg.presentAuthorizeURL(cfg.buildAuthorizeURL(state, generateChallenge(verifier))); err != nil { + return nil, err } code, callbackState, err := server.wait(ctx, defaultCallbackTimeout) @@ -317,6 +334,9 @@ func (c Config) validateForLogin() error { if wegostrings.IsBlank(c.CallbackAddr) { return errors.New("local callback address is not configured") } + if c.NoBrowser && c.PromptURL == nil { + return errors.New("no-browser sign-in needs Config.PromptURL: with no browser launched and no way to report the url, the operator has nothing to open") + } return nil } @@ -337,6 +357,24 @@ func (c Config) now() time.Time { } // openBrowserAt sends the operator to rawURL. +// presentAuthorizeURL gets the operator to the authorize URL, by launching a +// browser or, under NoBrowser, by handing the URL to PromptURL. +func (c Config) presentAuthorizeURL(rawURL string) error { + if c.NoBrowser { + if err := c.PromptURL(rawURL); err != nil { + return fmt.Errorf("present the sign-in url: %w", err) + } + + return nil + } + + if err := c.openBrowserAt(rawURL); err != nil { + return fmt.Errorf("open browser for sign-in: %w", err) + } + + return nil +} + func (c Config) openBrowserAt(rawURL string) error { if c.OpenBrowser != nil { return c.OpenBrowser(rawURL) diff --git a/cognito/oauth_test.go b/cognito/oauth_test.go index 5117709..b5bf985 100644 --- a/cognito/oauth_test.go +++ b/cognito/oauth_test.go @@ -650,3 +650,65 @@ func mustFreeAddr(t *testing.T) string { require.NoError(t, ln.Close()) return addr } + +// TestLogin_NoBrowser covers the headless path: the caller suppresses the +// launch and surfaces the URL itself, and the sign-in still completes on the +// loopback listener. Suppressing the launch must not change anything else — +// the same PKCE and state parameters have to be sent, because a URL an +// operator pastes by hand is the same URL a browser would have been given. +func TestLogin_NoBrowser(t *testing.T) { + ts := newTokenServer(t, nil) + + // The fake browser doubles as the prompt: it records the URL and drives the + // callback, which is exactly what an operator pasting the URL would cause. + prompt := newFakeBrowser() + + cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg.NoBrowser = true + cfg.PromptURL = prompt.open + cfg.OpenBrowser = func(string) error { + t.Error("NoBrowser must not launch a browser") + + return nil + } + + got, err := cognito.Login(context.Background(), cfg) + require.NoError(t, err) + require.Equal(t, testAccessValue, got.AccessToken) + + authorizeURL, err := url.Parse(prompt.capturedURL()) + require.NoError(t, err, "PromptURL must receive a usable authorize url") + + aq := authorizeURL.Query() + assert.Equal(t, cfg.ClientID, aq.Get("client_id")) + assert.Equal(t, cfg.RedirectURI, aq.Get("redirect_uri")) + assert.Equal(t, "S256", aq.Get("code_challenge_method"), "plain PKCE must never be used") + assert.NotEmpty(t, aq.Get("code_challenge")) + assert.NotEmpty(t, aq.Get("state"), "state is CSRF protection and must always be sent") +} + +func TestLogin_NoBrowserRequiresPromptURL(t *testing.T) { + cfg := baseConfig(t, "https://unused.test/oauth2/token", mustFreeAddr(t)) + cfg.NoBrowser = true + cfg.PromptURL = nil + + _, err := cognito.Login(context.Background(), cfg) + + require.Error(t, err, "a login with no browser and no way to report the url cannot complete") + assert.Contains(t, err.Error(), "PromptURL", + "the error must name the field that is missing") +} + +func TestLogin_NoBrowserPromptFailurePropagates(t *testing.T) { + wantErr := errors.New("no tty to print to") + + cfg := baseConfig(t, "https://unused.test/oauth2/token", mustFreeAddr(t)) + cfg.NoBrowser = true + cfg.PromptURL = func(string) error { return wantErr } + + _, err := cognito.Login(context.Background(), cfg) + + require.Error(t, err) + assert.ErrorIs(t, err, wantErr, "a prompt failure must reach the caller unwrapped in meaning") + assert.NotContains(t, err.Error(), "open browser", "the browser path was not taken") +} From 1586a5d337423490d9d624f27ddeb8256b50184b Mon Sep 17 00:00:00 2001 From: mysqto Date: Wed, 2 Sep 2026 11:01:45 +0800 Subject: [PATCH 04/12] cognito: reject a token response with no usable expires_in Found by CodeRabbit and claude[bot] on the payments PR this package was extracted from, and mirrored here so the two copies do not diverge before the payments one is deleted. postToken validated the three token strings but accepted any ExpiresIn. With expires_in absent, zero or negative, ExpiresAt landed on exactly now() and IsExpired subtracts a leeway on top, so a login that had just succeeded read as already expired -- sending the operator back through sign-in on their next command with no indication why. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JCCcwc4wtJgYKSFygyZGnR --- cognito/oauth.go | 7 ++++++ cognito/oauth_test.go | 51 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/cognito/oauth.go b/cognito/oauth.go index 645110c..c661cf8 100644 --- a/cognito/oauth.go +++ b/cognito/oauth.go @@ -235,6 +235,13 @@ func (c Config) postToken(ctx context.Context, form url.Values, fallbackRefresh return nil, errors.New("token response is missing id_token") case wegostrings.IsBlank(refresh): return nil, errors.New("token response is missing refresh_token") + case parsed.ExpiresIn <= 0: + // Silently accepting this is worse than failing. ExpiresAt would land on + // exactly now(), and IsExpired subtracts a leeway on top, so a login that + // just succeeded would read as already expired and the next command would + // refresh or send the operator back through sign-in. Refusing here names + // the real problem instead. + return nil, errors.New("token response has no usable expires_in") } return &TokenSet{ diff --git a/cognito/oauth_test.go b/cognito/oauth_test.go index b5bf985..b7c70bc 100644 --- a/cognito/oauth_test.go +++ b/cognito/oauth_test.go @@ -712,3 +712,54 @@ func TestLogin_NoBrowserPromptFailurePropagates(t *testing.T) { assert.ErrorIs(t, err, wantErr, "a prompt failure must reach the caller unwrapped in meaning") assert.NotContains(t, err.Error(), "open browser", "the browser path was not taken") } + +// TestLogin_RejectsAnUnusableExpiresIn pins that a token response with no +// usable expires_in fails loudly. +// +// Accepting it silently is worse than failing: ExpiresAt would land on exactly +// now(), IsExpired subtracts a leeway on top, and a login that had just +// succeeded would read as already expired — sending the operator back through +// sign-in on their very next command with no indication why. +func TestLogin_RejectsAnUnusableExpiresIn(t *testing.T) { + tests := []struct { + name string + givenExpiresIn any + wantErrContains string + }{ + { + name: "expires_in omitted entirely", + givenExpiresIn: nil, + wantErrContains: "expires_in", + }, + { + name: "expires_in zero", + givenExpiresIn: 0, + wantErrContains: "expires_in", + }, + { + name: "expires_in negative", + givenExpiresIn: -1, + wantErrContains: "expires_in", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + body := goodTokenBody(t, testOperatorEmail) + delete(body, "expires_in") + if tt.givenExpiresIn != nil { + body["expires_in"] = tt.givenExpiresIn + } + + ts := newTokenServer(t, jsonHandler(http.StatusOK, body)) + browser := newFakeBrowser() + cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg.OpenBrowser = browser.open + + _, err := cognito.Login(context.Background(), cfg) + + require.Error(t, err, "an unusable expires_in must not produce a session") + assert.Contains(t, err.Error(), tt.wantErrContains) + }) + } +} From ff2fbcb06e3560cd1186d6634ace91ded2e45eb0 Mon Sep 17 00:00:00 2001 From: mysqto Date: Thu, 3 Sep 2026 21:39:02 +0800 Subject: [PATCH 05/12] cognito: never follow a redirect from the token endpoint Config.httpClient returned a client with no CheckRedirect, so Go's default policy applied: follow up to ten redirects, re-sending the body verbatim on a 307 or 308. The token request carries the authorization code, the PKCE verifier and the client id on sign-in and the refresh token on renewal, so one redirect handed a complete credential set to whatever host the response named. It was also an injection route inwards, since the body that came back was parsed as the session to use. httpClient now returns a COPY with CheckRedirect set to refuse. Copying means a caller-supplied HTTPClient keeps its transport and timeout but cannot reinstate following, deliberately or by passing a client configured elsewhere, and the caller's own client is not mutated. A redirect from the token endpoint has no legitimate meaning here: TokenURL is an operator-configured Cognito domain that answers directly. Refusing turns it into the error it should be, reported with the endpoint and status via the existing status check. TestConfig_Defaults asserted the injected client was returned by identity, which is the behaviour that allowed the override to be bypassed. It now pins the copy semantics instead. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JCCcwc4wtJgYKSFygyZGnR --- cognito/oauth.go | 36 ++++++++++++-- cognito/oauth_internal_test.go | 86 +++++++++++++++++++++++++++++++++- 2 files changed, 116 insertions(+), 6 deletions(-) diff --git a/cognito/oauth.go b/cognito/oauth.go index c661cf8..8827fdb 100644 --- a/cognito/oauth.go +++ b/cognito/oauth.go @@ -347,12 +347,40 @@ func (c Config) validateForLogin() error { return nil } -// httpClient is the client to call the token endpoint with. +// httpClient is the client to call the token endpoint with. It never follows +// redirects, whoever supplied it. +// +// The returned client is a COPY, so a caller-supplied HTTPClient is neither +// mutated nor able to reinstate redirect-following. That override is +// deliberate: the token request carries the authorization code, the PKCE +// verifier and the client id on sign-in and the refresh token on renewal, and +// Go's default policy follows up to ten redirects, re-sending the body +// verbatim on a 307 or 308. One redirect would therefore hand a complete +// credential set to whatever host the response named. It is also an injection +// route inwards, since the body that came back would be parsed as the session +// to use. +// +// A redirect from the token endpoint has no legitimate meaning here: TokenURL +// is a Cognito domain the operator configured, and Cognito answers it +// directly. Refusing turns the redirect into the error it should be. func (c Config) httpClient() *http.Client { - if c.HTTPClient != nil { - return c.HTTPClient + base := c.HTTPClient + if base == nil { + base = &http.Client{Timeout: defaultHTTPTimeout} } - return &http.Client{Timeout: defaultHTTPTimeout} + + refusing := *base + refusing.CheckRedirect = refuseRedirect + + return &refusing +} + +// refuseRedirect stops the client at the redirect response instead of +// following it. Returning ErrUseLastResponse rather than an error of our own +// hands postToken the 3xx itself, which its status check then reports with the +// endpoint and status an operator needs to debug the misconfiguration. +func refuseRedirect(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse } // now is the current time, from the injected clock when there is one. diff --git a/cognito/oauth_internal_test.go b/cognito/oauth_internal_test.go index 064dc3a..6b84acf 100644 --- a/cognito/oauth_internal_test.go +++ b/cognito/oauth_internal_test.go @@ -1,8 +1,12 @@ package cognito import ( + "context" + "encoding/json" "net/http" + "net/http/httptest" "net/url" + "sync/atomic" "testing" "time" @@ -119,8 +123,19 @@ func TestConfig_Defaults(t *testing.T) { assert.Equal(t, defaultHTTPTimeout, zero.httpClient().Timeout, "a nil HTTPClient must get a timeout") assert.WithinDuration(t, time.Now(), zero.now(), time.Minute, "a nil Now must fall back to the system clock") - custom := &http.Client{Timeout: time.Second} - assert.Same(t, custom, Config{HTTPClient: custom}.httpClient()) + // An injected client is honoured for its transport and timeout but is + // COPIED, never handed back: httpClient has to own CheckRedirect, and it + // must not mutate a client the caller may use elsewhere. See + // TestPostToken_RefusesRedirects for why the override exists. + transport := &http.Transport{} + custom := &http.Client{Timeout: time.Second, Transport: transport} + got := Config{HTTPClient: custom}.httpClient() + + assert.NotSame(t, custom, got, "the caller's client must not be handed back") + assert.Equal(t, time.Second, got.Timeout, "the caller's timeout must survive") + assert.Same(t, transport, got.Transport, "the caller's transport must survive") + assert.NotNil(t, got.CheckRedirect, "the returned client must refuse redirects") + assert.Nil(t, custom.CheckRedirect, "the caller's client must not be mutated") fixed := time.Date(2020, 1, 1, 0, 0, 0, 0, time.UTC) assert.Equal(t, fixed, Config{Now: func() time.Time { return fixed }}.now()) @@ -136,3 +151,70 @@ func TestConfig_OpenBrowserAtUsesTheInjectedOpener(t *testing.T) { require.NoError(t, cfg.openBrowserAt("https://example.test/authorize")) assert.Equal(t, "https://example.test/authorize", got) } + +// TestPostToken_RefusesRedirects proves a token request is never re-sent to a +// host other than the one configured. +// +// The form carries the authorization code, the PKCE verifier and the client id +// on sign-in, and the refresh token on renewal. Go's default client follows up +// to ten redirects and re-sends the body verbatim on a 307 or 308, so +// following one would hand a complete credential set to whatever host the +// response named. A redirect is also an injection route in the other +// direction: the body that comes back would be parsed as a token set, letting +// an unintended host choose the session the CLI then uses. +func TestPostToken_RefusesRedirects(t *testing.T) { + tests := []struct { + name string + givenStatus int + givenClient *http.Client + }{ + { + name: "the default client refuses a 307, which would re-send the body", + givenStatus: http.StatusTemporaryRedirect, + }, + { + name: "the default client refuses a 302", + givenStatus: http.StatusFound, + }, + { + // A caller supplying its own client must not be able to reinstate + // following, deliberately or by copying a client from elsewhere. + name: "an injected permissive client cannot opt back into following", + givenStatus: http.StatusTemporaryRedirect, + givenClient: &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return nil }}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var elsewhereHits atomic.Int32 + + // Answers with a usable token set, so following the redirect would + // look like a successful sign-in rather than an error. + elsewhere := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + elsewhereHits.Add(1) + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(map[string]any{ + "access_token": "attacker-access", + "id_token": "attacker-id", + "expires_in": 3600, + }) + })) + t.Cleanup(elsewhere.Close) + + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, elsewhere.URL, tt.givenStatus) + })) + t.Cleanup(origin.Close) + + cfg := Config{TokenURL: origin.URL, HTTPClient: tt.givenClient} + form := url.Values{"code": {"secret-code"}, "code_verifier": {"secret-verifier"}} + + tokens, err := cfg.postToken(context.Background(), form, "") + + require.Error(t, err, "a redirected token endpoint must not produce a session") + assert.Nil(t, tokens) + assert.Zero(t, elsewhereHits.Load(), "credentials must never reach the redirect target") + }) + } +} From dda4783bb31361474a54e814865cc75a4ac6580b Mon Sep 17 00:00:00 2001 From: mysqto Date: Thu, 3 Sep 2026 21:39:07 +0800 Subject: [PATCH 06/12] cognito/storage: commit a token set with a single pointer write A re-login that failed partway left a hybrid token set. Save wrote four separate keychain entries and Load gated on the access token, so a failure after the refresh token was written left the NEW refresh token beside the OLD access token, id token and expiry, and Load returned that mixture as a live session. Writing the access token last only protected a FIRST login, where there was nothing to mix with. The consequences are worse than a failed read: a stale expiry paired with a fresh refresh token makes the CLI believe a session is valid, and after an account switch one identity's id token can end up beside another's refresh token. Single-entry storage would fix it but does not fit. zalando/go-keyring shells out to /usr/bin/security on macOS and rejects any command over 4096 bytes (keyring_darwin.go); after its base64 expansion that leaves roughly 3 KB of secret per entry, and a combined set of Cognito JWTs runs to about that, so it would work in development and fail for operators with larger tokens. Fields therefore stay in their own entries. Atomicity comes from a commit pointer instead. Each token set is written into one of two slots, and a "current" entry names the slot that counts. Save fills the inactive slot and then moves the pointer, which is one small write and the only write that changes what Load sees, so a failure anywhere before it costs the new session and never the old one. Two slots rather than a counter keep the entry count fixed and mean a re-login never writes over the entries the live session is read from. Delete removes the pointer first, for the same reason, and clears both slots so a torn Save leaves no token material behind. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JCCcwc4wtJgYKSFygyZGnR --- cognito/storage/keyring.go | 189 ++++++++++++--- cognito/storage/keyring_internal_test.go | 286 ++++++++++++++++++++--- 2 files changed, 406 insertions(+), 69 deletions(-) diff --git a/cognito/storage/keyring.go b/cognito/storage/keyring.go index dce0c91..cfc3481 100644 --- a/cognito/storage/keyring.go +++ b/cognito/storage/keyring.go @@ -12,19 +12,47 @@ import ( "github.com/wego/pkg/cognito" ) +// Layout. +// // A Cognito JWT - the access token especially - can exceed the 4 KiB // command-line cap zalando/go-keyring runs into on macOS, where it shells out -// to /usr/bin/security. So each field gets its own keychain entry, keeping -// every individual write well under that ceiling. +// to /usr/bin/security (see keyring_darwin.go, which refuses any command over +// 4096 bytes). After the library's base64 expansion that leaves roughly 3 KB +// of secret per entry, and a whole token set does not reliably fit. So each +// field keeps its own entry. +// +// That rules out getting atomicity by writing one entry, and a keychain has no +// transaction. Instead each token set is written into one of two SLOTS, and a +// separate pointer entry names the slot that counts. Save fills the inactive +// slot and then moves the pointer, which is one small write and the only write +// that changes what Load sees. A failure anywhere before it leaves the pointer +// and the live slot untouched, so a torn write costs the new session, never +// the old one. +// +// Two slots rather than a counter keeps the entry count fixed and means a +// re-login never writes over the entries the current session is read from. // -// Entries are accounted as "/". +// Entries are accounted as "/current" for the pointer and +// "//" for the token fields. const ( fieldAccess = "access" fieldID = "id" fieldRefresh = "refresh" fieldMeta = "meta" + // fieldCurrent is the pointer entry naming the live slot. Committing a + // token set is exactly one write of this entry. + fieldCurrent = "current" +) + +// The two slots a token set alternates between. +const ( + slotA = "a" + slotB = "b" ) +// tokenFields are the per-field entries that make up one stored token set. +var tokenFields = []string{fieldAccess, fieldID, fieldRefresh, fieldMeta} + // keyringBackend is the slice of zalando/go-keyring this package depends on. // Naming it lets tests substitute a fake instead of prompting a real keychain. type keyringBackend interface { @@ -73,10 +101,10 @@ func (k *keyringStore) Load(namespace string) (*cognito.TokenSet, error) { return nil, err } - // Gate on the access token: its absence means "not signed in", which is a - // normal state rather than a failure. Once it is present, anything else - // missing is a real inconsistency and must be reported. - access, err := k.backend.Get(k.service, account(namespace, fieldAccess)) + // Gate on the pointer: its absence means "not signed in", which is a normal + // state rather than a failure. Once it is present, anything the slot it + // names is missing is a real inconsistency and must be reported. + slot, err := k.backend.Get(k.service, pointerAccount(namespace)) if errors.Is(err, keyring.ErrNotFound) { return nil, nil } @@ -84,15 +112,26 @@ func (k *keyringStore) Load(namespace string) (*cognito.TokenSet, error) { return nil, fmt.Errorf("read the keychain: %w (is the system keychain unlocked?)", err) } - idToken, err := k.read(namespace, fieldID) + if slot != slotA && slot != slotB { + // Nothing here writes any other value, so this entry was tampered with + // or written by a version that stored something else. Guessing which + // slot was meant would be worse than refusing. + return nil, fmt.Errorf("keychain names an unknown token slot %q for %q", slot, namespace) + } + + access, err := k.read(namespace, slot, fieldAccess) + if err != nil { + return nil, err + } + idToken, err := k.read(namespace, slot, fieldID) if err != nil { return nil, err } - refresh, err := k.read(namespace, fieldRefresh) + refresh, err := k.read(namespace, slot, fieldRefresh) if err != nil { return nil, err } - rawMeta, err := k.read(namespace, fieldMeta) + rawMeta, err := k.read(namespace, slot, fieldMeta) if err != nil { return nil, err } @@ -111,6 +150,11 @@ func (k *keyringStore) Load(namespace string) (*cognito.TokenSet, error) { } // Save writes tokens under namespace, replacing anything already there. +// +// It is atomic from Load's point of view: the fields go into the slot that is +// not live, and only the final pointer write makes them the session. If any +// write fails, the previous session is still whole and still what Load +// returns. func (k *keyringStore) Save(namespace string, tokens *cognito.TokenSet) error { if err := k.validate(namespace); err != nil { return err @@ -124,49 +168,119 @@ func (k *keyringStore) Save(namespace string, tokens *cognito.TokenSet) error { return fmt.Errorf("encode token metadata: %w", err) } - // The access token is written LAST because Load gates on it: a write cut - // short partway through then reads as "not signed in", which is - // recoverable, rather than as a half-populated token set, which is not. - entries := []struct { - field string - value string - }{ - {field: fieldRefresh, value: tokens.RefreshToken}, - {field: fieldID, value: tokens.IDToken}, - {field: fieldMeta, value: string(meta)}, - {field: fieldAccess, value: tokens.AccessToken}, + live, err := k.currentSlot(namespace) + if err != nil { + return err } - for _, entry := range entries { - if err := k.backend.Set(k.service, account(namespace, entry.field), entry.value); err != nil { - return fmt.Errorf("write %s to the keychain: %w (is the system keychain unlocked?)", entry.field, err) + next := otherSlot(live) + + values := map[string]string{ + fieldAccess: tokens.AccessToken, + fieldID: tokens.IDToken, + fieldRefresh: tokens.RefreshToken, + fieldMeta: string(meta), + } + + // Ordering is irrelevant now: nothing written here is reachable until the + // pointer moves. tokenFields is used rather than ranging the map so the + // write sequence is deterministic, which keeps failures reproducible. + for _, field := range tokenFields { + if err := k.backend.Set(k.service, fieldAccount(namespace, next, field), values[field]); err != nil { + return fmt.Errorf("write %s to the keychain: %w (is the system keychain unlocked?)", field, err) } } + // The commit. + if err := k.backend.Set(k.service, pointerAccount(namespace), next); err != nil { + return fmt.Errorf("commit the session to the keychain: %w (is the system keychain unlocked?)", err) + } + + // The old slot is now unreachable. Clearing it keeps a superseded token set + // out of the keychain, but the session is already committed, so a failure + // here is not the caller's problem and must not fail the sign-in. + k.clearSlot(namespace, live) + return nil } +// currentSlot reports the live slot, or "" when nothing is stored. An +// unrecognised value is treated as "nothing live": Save's job is to establish a +// good session, and it can do that without deciding what the bad value meant. +// Load is where a corrupt pointer is reported. +func (k *keyringStore) currentSlot(namespace string) (string, error) { + slot, err := k.backend.Get(k.service, pointerAccount(namespace)) + if errors.Is(err, keyring.ErrNotFound) { + return "", nil + } + if err != nil { + return "", fmt.Errorf("read the current token slot: %w (is the system keychain unlocked?)", err) + } + + if slot != slotA && slot != slotB { + return "", nil + } + + return slot, nil +} + +// otherSlot returns the slot to write next. An empty live slot means nothing is +// stored, so either is free and slotA keeps a first login predictable. +func otherSlot(live string) string { + if live == slotA { + return slotB + } + + return slotA +} + +// clearSlot removes one slot's field entries, best effort. Callers use it for a +// slot nothing points at any more. +func (k *keyringStore) clearSlot(namespace, slot string) { + if slot == "" { + return + } + + for _, field := range tokenFields { + _ = k.backend.Delete(k.service, fieldAccount(namespace, slot, field)) + } +} + // Delete removes every entry under namespace. Entries already gone are fine: // the desired end state is "signed out", so signing out twice must succeed. +// +// The pointer goes FIRST, for the same reason Save moves it last: once it is +// gone the operator is signed out, even if a later delete fails and leaves +// orphaned field entries behind. func (k *keyringStore) Delete(namespace string) error { if err := k.validate(namespace); err != nil { return err } - for _, field := range []string{fieldAccess, fieldID, fieldRefresh, fieldMeta} { - err := k.backend.Delete(k.service, account(namespace, field)) - if err != nil && !errors.Is(err, keyring.ErrNotFound) { - return fmt.Errorf("delete %s from the keychain: %w (is the system keychain unlocked?)", field, err) + err := k.backend.Delete(k.service, pointerAccount(namespace)) + if err != nil && !errors.Is(err, keyring.ErrNotFound) { + return fmt.Errorf("delete the current token slot from the keychain: %w (is the system keychain unlocked?)", err) + } + + // Both slots, not just the live one: a torn Save can leave fields in the + // inactive slot, and signing out should not leave a token set behind. + for _, slot := range []string{slotA, slotB} { + for _, field := range tokenFields { + err := k.backend.Delete(k.service, fieldAccount(namespace, slot, field)) + if err != nil && !errors.Is(err, keyring.ErrNotFound) { + return fmt.Errorf("delete %s from the keychain: %w (is the system keychain unlocked?)", field, err) + } } } return nil } -// read fetches one entry, treating a missing one as an error: callers only -// reach it after the access-token gate has confirmed a login exists. -func (k *keyringStore) read(namespace, field string) (string, error) { - value, err := k.backend.Get(k.service, account(namespace, field)) +// read fetches one field of one slot, treating a missing entry as an error: +// callers only reach it after the pointer has confirmed a committed session, +// so anything absent is an inconsistency rather than a logged-out state. +func (k *keyringStore) read(namespace, slot, field string) (string, error) { + value, err := k.backend.Get(k.service, fieldAccount(namespace, slot, field)) if err != nil { return "", fmt.Errorf("read %s from the keychain: %w", field, err) } @@ -184,7 +298,12 @@ func (k *keyringStore) validate(namespace string) error { return nil } -// account is the keychain account name for one field of one namespace. -func account(namespace, field string) string { - return namespace + "/" + field +// pointerAccount is the keychain account name of a namespace's commit pointer. +func pointerAccount(namespace string) string { + return namespace + "/" + fieldCurrent +} + +// fieldAccount is the keychain account name for one field of one slot. +func fieldAccount(namespace, slot, field string) string { + return namespace + "/" + slot + "/" + field } diff --git a/cognito/storage/keyring_internal_test.go b/cognito/storage/keyring_internal_test.go index 5415498..4770377 100644 --- a/cognito/storage/keyring_internal_test.go +++ b/cognito/storage/keyring_internal_test.go @@ -182,39 +182,57 @@ func TestKeyringStore_LoadFailures(t *testing.T) { wantErrContains: "keychain", }, { + // The pointer says a session was committed, so anything the slot is + // missing is an inconsistency rather than a logged-out state. name: "a missing id_token is an inconsistency, not a logged-out state", givenSetup: func(f *fakeKeyring) { - f.put(fieldAccess, "access-value") + f.putCommitted(map[string]string{fieldAccess: "access-value"}) }, wantErrContains: "id", }, { name: "a missing refresh_token is an inconsistency", givenSetup: func(f *fakeKeyring) { - f.put(fieldAccess, "access-value") - f.put(fieldID, "id-value") + f.putCommitted(map[string]string{ + fieldAccess: "access-value", + fieldID: "id-value", + }) }, wantErrContains: "refresh", }, { name: "missing metadata is an inconsistency", givenSetup: func(f *fakeKeyring) { - f.put(fieldAccess, "access-value") - f.put(fieldID, "id-value") - f.put(fieldRefresh, "refresh-value") + f.putCommitted(map[string]string{ + fieldAccess: "access-value", + fieldID: "id-value", + fieldRefresh: "refresh-value", + }) }, wantErrContains: "meta", }, { name: "corrupt metadata is reported", givenSetup: func(f *fakeKeyring) { - f.put(fieldAccess, "access-value") - f.put(fieldID, "id-value") - f.put(fieldRefresh, "refresh-value") - f.put(fieldMeta, "{not json") + f.putCommitted(map[string]string{ + fieldAccess: "access-value", + fieldID: "id-value", + fieldRefresh: "refresh-value", + fieldMeta: "{not json", + }) }, wantErrContains: "metadata", }, + { + // Fields present but never committed: the sign-in was torn before + // the pointer moved, so the operator is simply not signed in. + name: "an uncommitted slot reads as not logged in", + givenSetup: func(f *fakeKeyring) { + f.putField(slotA, fieldAccess, "access-value") + f.putField(slotA, fieldID, "id-value") + }, + wantNil: true, + }, { name: "nothing stored reads as not logged in", givenSetup: func(_ *fakeKeyring) {}, @@ -242,25 +260,121 @@ func TestKeyringStore_LoadFailures(t *testing.T) { } } -// TestKeyringStore_SaveSplitsFieldsAcrossEntries pins the workaround for the -// 4 KiB argument cap that zalando/go-keyring hits on macOS: one entry per -// field, with the access token written last so a torn write reads as -// "not logged in" rather than as a half-populated token set. -func TestKeyringStore_SaveSplitsFieldsAcrossEntries(t *testing.T) { +// TestKeyringStore_SaveCommitsWithOnePointerWrite pins both halves of the +// storage contract. +// +// Fields stay in SEPARATE entries because zalando/go-keyring shells out to +// /usr/bin/security on macOS and refuses any command over 4096 bytes +// (keyring_darwin.go). After its base64 expansion that leaves roughly 3 KB of +// secret per entry, which a single combined token set would exceed. +// +// Atomicity therefore cannot come from writing one entry. It comes from +// writing the fields into the inactive slot and then moving the pointer, which +// is ONE small Set and the only write that changes what Load sees. +func TestKeyringStore_SaveCommitsWithOnePointerWrite(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + require.NotEmpty(t, backend.writes) + assert.Equal(t, testNamespace+"/"+fieldCurrent, backend.writes[len(backend.writes)-1], + "the pointer must be the last write, so nothing before it is observable") + assert.Equal(t, 1, countWrites(backend.writes, testNamespace+"/"+fieldCurrent), + "the commit must be a single pointer write") + + for _, field := range []string{fieldAccess, fieldID, fieldRefresh, fieldMeta} { + assert.Contains(t, backend.writes, testNamespace+"/"+slotA+"/"+field, + "each field keeps its own entry, to stay under the 4 KiB cap") + } +} + +// TestKeyringStore_SaveAlternatesSlots covers why there are two slots: a +// re-login must not overwrite the entries the current session is still being +// read from, or a torn write would corrupt the live session rather than an +// unused copy. +func TestKeyringStore_SaveAlternatesSlots(t *testing.T) { backend := newFakeKeyring() store := &keyringStore{service: testService, backend: backend} require.NoError(t, store.Save(testNamespace, sampleTokens())) + assert.Equal(t, slotA, backend.value(fieldCurrent)) - assert.Equal(t, []string{ - testNamespace + "/refresh", - testNamespace + "/id", - testNamespace + "/meta", - testNamespace + "/access", - }, backend.writes, "one namespaced entry per field, access token last") + second := sampleTokens() + second.AccessToken = "second-access" + require.NoError(t, store.Save(testNamespace, second)) + assert.Equal(t, slotB, backend.value(fieldCurrent)) - assert.Equal(t, "access-value", backend.value(fieldAccess)) - assert.Equal(t, "refresh-value", backend.value(fieldRefresh)) + third := sampleTokens() + third.AccessToken = "third-access" + require.NoError(t, store.Save(testNamespace, third)) + assert.Equal(t, slotA, backend.value(fieldCurrent), "slots alternate rather than growing") + + got, err := store.Load(testNamespace) + require.NoError(t, err) + assert.Equal(t, "third-access", got.AccessToken) +} + +// TestKeyringStore_TornResaveLeavesThePreviousSession is the regression test +// for the reported defect. +// +// With one entry per field and no commit pointer, a re-login that failed +// partway left the NEW refresh token beside the OLD access token, id token and +// expiry. Load gated on the access token, which was present, so it handed that +// mixture back as a live session: a stale expiry paired with a fresh refresh +// token, or after an account switch one identity's id token paired with +// another's refresh token. +func TestKeyringStore_TornResaveLeavesThePreviousSession(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + // Let two writes of the re-login land, then fail. + backend.okWrites = 0 + backend.failSetAfter = 2 + + newer := &cognito.TokenSet{ + AccessToken: "new-access", + IDToken: "new-id", + RefreshToken: "new-refresh", + ExpiresAt: testExpiry.Add(time.Hour), + } + require.Error(t, store.Save(testNamespace, newer)) + + backend.failSetAfter = -1 + + got, err := store.Load(testNamespace) + require.NoError(t, err, "the previous session must still be readable") + require.NotNil(t, got) + assert.Equal(t, sampleTokens(), got, + "a torn re-login must leave the previous session exactly, never a mixture of the two") +} + +// TestKeyringStore_LoadRejectsAnUnknownSlot covers a pointer naming a slot that +// is not one of the two. That cannot arise from this code, so it means the +// entry was tampered with or written by another version, and guessing which +// slot was meant would be worse than refusing. +func TestKeyringStore_LoadRejectsAnUnknownSlot(t *testing.T) { + backend := newFakeKeyring() + backend.put(fieldCurrent, "somewhere-else") + store := &keyringStore{service: testService, backend: backend} + + _, err := store.Load(testNamespace) + require.Error(t, err) + assert.Contains(t, err.Error(), "slot") +} + +// countWrites reports how many times account appears in a write log. +func countWrites(writes []string, account string) int { + n := 0 + for _, w := range writes { + if w == account { + n++ + } + } + + return n } func TestKeyringStore_BackendFailuresAreReported(t *testing.T) { @@ -302,14 +416,92 @@ func TestKeyringStore_BackendFailuresAreReported(t *testing.T) { } // TestKeyringStore_DeleteToleratesMissingEntries covers a half-populated -// namespace: sign-out must succeed rather than stranding the operator. +// namespace: sign-out must succeed rather than stranding the operator, and it +// must clear the slot a torn Save left behind as well as the live one. func TestKeyringStore_DeleteToleratesMissingEntries(t *testing.T) { backend := newFakeKeyring() - backend.put(fieldAccess, "access-value") + backend.putField(slotA, fieldAccess, "access-value") + backend.putField(slotB, fieldRefresh, "orphaned-refresh") store := &keyringStore{service: testService, backend: backend} require.NoError(t, store.Delete(testNamespace)) - assert.Empty(t, backend.entries) + assert.Empty(t, backend.entries, "no token material may survive a sign-out, in either slot") +} + +// TestKeyringStore_FailedCommitKeepsThePreviousSession covers the last write. +// Every field of the new session is already in the keychain; only the pointer +// move failed. Load must still return the old session, because the new one was +// never committed. +func TestKeyringStore_FailedCommitKeepsThePreviousSession(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + // Let all four field writes land, then fail the pointer write. + backend.okWrites = 0 + backend.failSetAfter = len(tokenFields) + + newer := sampleTokens() + newer.AccessToken = "new-access" + + err := store.Save(testNamespace, newer) + require.Error(t, err) + assert.Contains(t, err.Error(), "commit", "the failure must name the commit, not a field") + + backend.failSetAfter = -1 + + got, err := store.Load(testNamespace) + require.NoError(t, err) + assert.Equal(t, sampleTokens(), got, "an uncommitted session must not become the live one") +} + +// TestKeyringStore_SaveReportsAnUnreadablePointer covers a keychain that +// cannot be read at all. Save must not proceed on a guess about which slot is +// live: writing to the wrong one would overwrite the session it is meant to +// protect. +func TestKeyringStore_SaveReportsAnUnreadablePointer(t *testing.T) { + backend := newFakeKeyring() + backend.getErr = errors.New("keychain is locked") + store := &keyringStore{service: testService, backend: backend} + + err := store.Save(testNamespace, sampleTokens()) + require.Error(t, err) + assert.Contains(t, err.Error(), "slot") + assert.Empty(t, backend.writes, "nothing may be written when the live slot is unknown") +} + +// TestKeyringStore_SaveOverAnUnknownSlotStillSignsIn covers a pointer holding +// a value this code never writes. Load refuses it, but Save's job is to +// establish a good session and it can do that without deciding what the bad +// value meant. +func TestKeyringStore_SaveOverAnUnknownSlotStillSignsIn(t *testing.T) { + backend := newFakeKeyring() + backend.put(fieldCurrent, "somewhere-else") + store := &keyringStore{service: testService, backend: backend} + + require.NoError(t, store.Save(testNamespace, sampleTokens())) + assert.Equal(t, slotA, backend.value(fieldCurrent)) + + got, err := store.Load(testNamespace) + require.NoError(t, err) + assert.Equal(t, sampleTokens(), got) +} + +// TestKeyringStore_DeleteReportsAPointerFailure covers sign-out when the +// pointer cannot be removed. That is the entry that decides whether the +// operator is signed in, so the failure has to be reported rather than +// swallowed after clearing the fields. +func TestKeyringStore_DeleteReportsAPointerFailure(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + backend.deleteErr = errors.New("keychain is locked") + + err := store.Delete(testNamespace) + require.Error(t, err) + assert.Contains(t, err.Error(), "slot") } func TestKeyringStore_BlankServiceIsRejected(t *testing.T) { @@ -333,16 +525,21 @@ func sampleTokens() *cognito.TokenSet { // fakeKeyring is an in-memory stand-in for the OS keychain. type fakeKeyring struct { - mu sync.Mutex - entries map[string]string - writes []string - setErr error - getErr error - deleteErr error + mu sync.Mutex + entries map[string]string + writes []string + setErr error + // failSetAfter fails every Set from the (failSetAfter+1)th of this counter + // onward, so a test can tear a write sequence in the middle. Negative + // disables it. Reset okWrites to re-arm. + failSetAfter int + okWrites int + getErr error + deleteErr error } func newFakeKeyring() *fakeKeyring { - return &fakeKeyring{entries: make(map[string]string)} + return &fakeKeyring{entries: make(map[string]string), failSetAfter: -1} } func (f *fakeKeyring) Set(service, user, password string) error { @@ -351,6 +548,10 @@ func (f *fakeKeyring) Set(service, user, password string) error { if f.setErr != nil { return f.setErr } + if f.failSetAfter >= 0 && f.okWrites >= f.failSetAfter { + return errors.New("keychain is locked") + } + f.okWrites++ f.entries[service+"|"+user] = password f.writes = append(f.writes, user) return nil @@ -383,13 +584,30 @@ func (f *fakeKeyring) Delete(service, user string) error { return nil } -// put seeds an entry under testNamespace, bypassing the store under test. +// put seeds a namespace-level entry (the pointer) under testNamespace, +// bypassing the store under test. func (f *fakeKeyring) put(field, value string) { f.mu.Lock() defer f.mu.Unlock() f.entries[testService+"|"+testNamespace+"/"+field] = value } +// putField seeds one field of one slot, bypassing the store under test. +func (f *fakeKeyring) putField(slot, field, value string) { + f.mu.Lock() + defer f.mu.Unlock() + f.entries[testService+"|"+testNamespace+"/"+slot+"/"+field] = value +} + +// putCommitted seeds a committed session in slotA, field by field, so a test +// can then remove or corrupt exactly one part of it. +func (f *fakeKeyring) putCommitted(fields map[string]string) { + for field, value := range fields { + f.putField(slotA, field, value) + } + f.put(fieldCurrent, slotA) +} + // value reads an entry under testNamespace, bypassing the store under test. func (f *fakeKeyring) value(field string) string { f.mu.Lock() From f338c573760bb73d63a997f6f0d15ff766e7f89b Mon Sep 17 00:00:00 2001 From: mysqto Date: Mon, 7 Sep 2026 12:04:45 +0800 Subject: [PATCH 07/12] cognito/storage: retry Load when a concurrent commit moves the pointer A token set is read across five keychain entries, so another process could commit a replacement and clear the old slot between them. Load had already chosen that slot, and the next field read failed with "secret not found in keyring" even though a perfectly good session existed throughout. Two overlapping pay-admin processes is ordinary: a command running while a refresh fires, or two terminals on one namespace. The commit pointer is what makes this recoverable. On a failed field read Load now re-reads the pointer; if it has moved, the slot was superseded rather than broken, so it starts again on the slot that is now live. If the pointer has not moved the namespace really is inconsistent and the original error is reported unchanged, so a missing field is still named rather than disappearing into a generic retry. Bounded at three attempts. Each retry needs another process to commit a whole session in the gap, so exhausting them means a namespace being rewritten faster than it can be read, which no retry fixes. Immediate cleanup of the superseded slot is kept: leaving it would mean a stale refresh token living in the keychain until the next login, and the retry makes the deletion safe. Reported in review on wego/payments#2300 with a reproducing test. The regression here drives a real Save through the backend mid-read and asserts Load returns one whole session, never a mixture of the two. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JCCcwc4wtJgYKSFygyZGnR --- cognito/storage/keyring.go | 70 +++++++++++++-- cognito/storage/keyring_internal_test.go | 107 +++++++++++++++++++++++ 2 files changed, 171 insertions(+), 6 deletions(-) diff --git a/cognito/storage/keyring.go b/cognito/storage/keyring.go index cfc3481..f871f44 100644 --- a/cognito/storage/keyring.go +++ b/cognito/storage/keyring.go @@ -50,6 +50,12 @@ const ( slotB = "b" ) +// loadAttempts is how many times Load will re-read after a concurrent commit +// moves the pointer out from under it. Each retry needs another process to +// commit a whole session in the gap, so three is far past what a real overlap +// produces and still terminates. +const loadAttempts = 3 + // tokenFields are the per-field entries that make up one stored token set. var tokenFields = []string{fieldAccess, fieldID, fieldRefresh, fieldMeta} @@ -96,29 +102,81 @@ func NewKeyring(service string) Store { // Load returns the tokens stored under namespace, or (nil, nil) if the // operator is not signed in. +// +// A token set is read across five keychain entries, so a concurrent commit can +// move the pointer and clear the slot midway through. That is not an error and +// must not surface as one: Load re-reads the pointer and starts again on the +// slot that is now live, which is the ordinary case of a second pay-admin +// process finishing a login or a refresh while this one reads. func (k *keyringStore) Load(namespace string) (*cognito.TokenSet, error) { if err := k.validate(namespace); err != nil { return nil, err } - // Gate on the pointer: its absence means "not signed in", which is a normal - // state rather than a failure. Once it is present, anything the slot it - // names is missing is a real inconsistency and must be reported. + for range loadAttempts { + slot, err := k.liveSlot(namespace) + if err != nil || slot == "" { + // No pointer means not signed in, which is (nil, nil). + return nil, err + } + + tokens, err := k.readSlot(namespace, slot) + if err == nil { + return tokens, nil + } + + // The read failed. If the pointer has moved since it was chosen, this + // slot was superseded and cleared under us, so try the new one. If it + // has not, the namespace really is inconsistent and the original error + // is the honest one to report. + moved, checkErr := k.slotMoved(namespace, slot) + if checkErr != nil { + return nil, checkErr + } + + if !moved { + return nil, err + } + } + + // Every attempt lost the same race, which needs a sustained run of commits + // against one namespace. Report it rather than looping. + return nil, fmt.Errorf("the keychain session for %q was replaced %d times while being read", namespace, loadAttempts) +} + +// liveSlot returns the slot the pointer names, or "" when nothing is stored. +func (k *keyringStore) liveSlot(namespace string) (string, error) { slot, err := k.backend.Get(k.service, pointerAccount(namespace)) if errors.Is(err, keyring.ErrNotFound) { - return nil, nil + return "", nil } if err != nil { - return nil, fmt.Errorf("read the keychain: %w (is the system keychain unlocked?)", err) + return "", fmt.Errorf("read the keychain: %w (is the system keychain unlocked?)", err) } if slot != slotA && slot != slotB { // Nothing here writes any other value, so this entry was tampered with // or written by a version that stored something else. Guessing which // slot was meant would be worse than refusing. - return nil, fmt.Errorf("keychain names an unknown token slot %q for %q", slot, namespace) + return "", fmt.Errorf("keychain names an unknown token slot %q for %q", slot, namespace) } + return slot, nil +} + +// slotMoved reports whether the pointer now names a slot other than the one a +// read was using. +func (k *keyringStore) slotMoved(namespace, slot string) (bool, error) { + current, err := k.liveSlot(namespace) + if err != nil { + return false, err + } + + return current != slot, nil +} + +// readSlot assembles the token set held in one slot. +func (k *keyringStore) readSlot(namespace, slot string) (*cognito.TokenSet, error) { access, err := k.read(namespace, slot, fieldAccess) if err != nil { return nil, err diff --git a/cognito/storage/keyring_internal_test.go b/cognito/storage/keyring_internal_test.go index 4770377..cfef6a5 100644 --- a/cognito/storage/keyring_internal_test.go +++ b/cognito/storage/keyring_internal_test.go @@ -536,6 +536,18 @@ type fakeKeyring struct { okWrites int getErr error deleteErr error + // onGet fires once, on the next Get, before the read happens. + onGet func(account string) +} + +// takeOnGet returns the hook, if any. It does NOT clear it: the hook decides +// which account it wants to fire on, so consuming it on the first unrelated +// read would silently disarm the test. +func (f *fakeKeyring) takeOnGet() func(string) { + f.mu.Lock() + defer f.mu.Unlock() + + return f.onGet } func newFakeKeyring() *fakeKeyring { @@ -558,6 +570,12 @@ func (f *fakeKeyring) Set(service, user, password string) error { } func (f *fakeKeyring) Get(service, user string) (string, error) { + // onGet runs OUTSIDE the lock and before the read, so a hook can drive a + // whole Save through this same backend without deadlocking. + if hook := f.takeOnGet(); hook != nil { + hook(user) + } + f.mu.Lock() defer f.mu.Unlock() if f.getErr != nil { @@ -614,3 +632,92 @@ func (f *fakeKeyring) value(field string) string { defer f.mu.Unlock() return f.entries[testService+"|"+testNamespace+"/"+field] } + +// TestKeyringStore_LoadSurvivesAConcurrentCommit is the regression for a race +// the two-slot commit introduced. +// +// Load reads the pointer, then reads that slot's four fields one at a time. +// Between those reads another pay-admin process can finish a login or a +// refresh, commit the replacement slot and clear the one this Load already +// chose, so the read failed with "secret not found in keyring" even though a +// perfectly good session existed the whole time. +// +// Two overlapping processes is ordinary: a shell running a command while a +// refresh fires, or two terminals against the same namespace. +func TestKeyringStore_LoadSurvivesAConcurrentCommit(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + replacement := &cognito.TokenSet{ + AccessToken: "refreshed-access", + IDToken: "refreshed-id", + RefreshToken: "refreshed-refresh", + ExpiresAt: testExpiry.Add(time.Hour), + } + + // Commit the replacement the moment this Load starts reading fields, which + // is exactly the window the race lives in. + var once sync.Once + backend.onGet = func(account string) { + if account == testNamespace+"/"+fieldCurrent { + return // the pointer read itself; let it through + } + once.Do(func() { + require.NoError(t, store.Save(testNamespace, replacement)) + }) + } + + got, err := store.Load(testNamespace) + require.NoError(t, err, "a concurrent commit must not fail a Load that was already in flight") + require.NotNil(t, got) + + // Either session is a correct answer; a mixture is not. + assert.Contains(t, []string{sampleTokens().AccessToken, replacement.AccessToken}, got.AccessToken) + if got.AccessToken == replacement.AccessToken { + assert.Equal(t, replacement.RefreshToken, got.RefreshToken, "the two sessions must not mix") + assert.Equal(t, replacement.IDToken, got.IDToken) + } else { + assert.Equal(t, sampleTokens().RefreshToken, got.RefreshToken, "the two sessions must not mix") + } +} + +// TestKeyringStore_LoadGivesUpOnEndlessCommits covers the retry bound. A +// namespace being rewritten faster than it can be read is not something a +// retry can fix, so Load must report it rather than spin. +func TestKeyringStore_LoadGivesUpOnEndlessCommits(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + // Commit a fresh session on every field read, so the pointer has always + // moved by the time the retry check looks. + backend.onGet = func(account string) { + if account == testNamespace+"/"+fieldCurrent { + return + } + + next := sampleTokens() + next.AccessToken = "rewritten" + _ = store.Save(testNamespace, next) + } + + _, err := store.Load(testNamespace) + require.Error(t, err) + assert.Contains(t, err.Error(), "replaced") +} + +// TestKeyringStore_LoadReportsARealInconsistency pins the other side of the +// retry: when the pointer has NOT moved, a missing field is a genuinely broken +// namespace and must be reported, not retried into a generic timeout. +func TestKeyringStore_LoadReportsARealInconsistency(t *testing.T) { + backend := newFakeKeyring() + backend.putCommitted(map[string]string{fieldAccess: "access-value"}) + store := &keyringStore{service: testService, backend: backend} + + _, err := store.Load(testNamespace) + require.Error(t, err) + assert.Contains(t, err.Error(), "id", "the missing field must be named") + assert.NotContains(t, err.Error(), "replaced") +} From f30852070067ac5c651419b5c5fed162c7e0f158 Mon Sep 17 00:00:00 2001 From: mysqto Date: Mon, 7 Sep 2026 14:48:52 +0800 Subject: [PATCH 08/12] cognito: add a paste-back sign-in for hosts with no reachable callback NoBrowser moved where the authorize URL is SHOWN, not where the code is delivered: the redirect still had to reach CallbackAddr on this machine, so a remote box needed `ssh -L`. With no browser and no port forward there was no way to sign in at all. Config.ReadRedirect closes that. Login shows the URL, the operator opens it anywhere, Cognito redirects to a loopback URL nothing is listening on, the browser shows a connection error, and its address bar holds ?code=...&state=... to copy back. No listener, no bound port. The device authorization grant would be the conventional answer and Cognito does not have one: the discovery document advertises no device_authorization_endpoint, /oauth2/device_authorization is 404, and the token endpoint answers a device_code grant with unsupported_grant_type. This is the substitute that needs no new Cognito configuration, because it is still authorization code with PKCE and only the redirect is carried by hand. State is still checked, and it is doing more work here than on the listener path: nothing about a pasted URL proves where it came from, so it is the only thing binding the code to this attempt. A bare code is refused for that reason, and a query carrying `error` reports the provider's refusal rather than a missing-code error. ReadRedirect implies NoBrowser. Launching a browser and then also asking for a paste would be a footgun, and where a local browser works the listener path is less work for the operator. CallbackAddr becomes optional on this path since nothing binds a port. The pasted URL carries a single-use authorization code, so it should not travel through a shared channel; that is documented on the field. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JCCcwc4wtJgYKSFygyZGnR --- cognito/oauth.go | 139 +++++++++++++++++++++++++++---- cognito/oauth_internal_test.go | 148 +++++++++++++++++++++++++++++++++ cognito/oauth_test.go | 92 ++++++++++++++++++++ 3 files changed, 361 insertions(+), 18 deletions(-) diff --git a/cognito/oauth.go b/cognito/oauth.go index 8827fdb..f074232 100644 --- a/cognito/oauth.go +++ b/cognito/oauth.go @@ -86,10 +86,33 @@ type Config struct { // PromptURL receives the authorize URL in place of a browser launch, so the // caller decides how to surface it: print it, render a QR code, hand it to - // another process. Required when NoBrowser is set — a sign-in whose URL the - // operator never sees cannot complete — and ignored otherwise. + // another process. Required when NoBrowser or ReadRedirect is set — a + // sign-in whose URL the operator never sees cannot complete — and ignored + // otherwise. PromptURL func(url string) error + // ReadRedirect turns the sign-in into a paste-back exchange: instead of a + // loopback listener receiving the redirect, the caller returns the URL the + // browser was redirected to and Login reads the code out of it. + // + // This is the only path that works with NO browser on this machine AND no + // way to reach its callback port. Cognito redirects to a loopback URL that + // nothing is listening on, the browser shows a connection error, and the + // address bar holds ?code=...&state=... for the operator to copy back. + // + // Cognito has no device authorization grant (RFC 8628): its discovery + // document advertises no device_authorization_endpoint, /oauth2/device_ + // authorization is 404, and the token endpoint answers a device_code grant + // with unsupported_grant_type. So this is the substitute, and it needs no + // extra Cognito configuration because it is still the authorization code + // grant with PKCE, only with the redirect carried by hand. + // + // Setting it implies NoBrowser: no browser is launched here. PromptURL is + // required alongside it; CallbackAddr is not, since nothing binds a port. The pasted URL carries a single-use authorization + // code, so it should not travel through a shared channel; state is still + // checked, so a code from a different attempt is rejected. + ReadRedirect func() (redirectedURL string, err error) + // HTTPClient calls the token endpoint. Nil uses a client with a timeout. HTTPClient *http.Client @@ -114,20 +137,7 @@ func Login(ctx context.Context, cfg Config) (*TokenSet, error) { return nil, err } - // Bind the callback port BEFORE sending the operator to Cognito. If the - // port is unavailable the redirect could never land, and there is no - // fallback port to try, so failing here saves a pointless round trip. - server, err := startCallbackServer(cfg.CallbackAddr, callbackPath(cfg.RedirectURI)) - if err != nil { - return nil, err - } - defer server.shutdown() - - if err := cfg.presentAuthorizeURL(cfg.buildAuthorizeURL(state, generateChallenge(verifier))); err != nil { - return nil, err - } - - code, callbackState, err := server.wait(ctx, defaultCallbackTimeout) + code, callbackState, err := cfg.collectCode(ctx, state, generateChallenge(verifier)) if err != nil { return nil, err } @@ -338,9 +348,13 @@ func (c Config) validateForLogin() error { if wegostrings.IsBlank(c.RedirectURI) { return errors.New("cognito redirect uri is not configured") } - if wegostrings.IsBlank(c.CallbackAddr) { + // Only the listener path needs a port; paste-back binds nothing. + if c.ReadRedirect == nil && wegostrings.IsBlank(c.CallbackAddr) { return errors.New("local callback address is not configured") } + if c.ReadRedirect != nil && c.PromptURL == nil { + return errors.New("paste-back sign-in needs Config.PromptURL: the operator has to be shown the url they are meant to open") + } if c.NoBrowser && c.PromptURL == nil { return errors.New("no-browser sign-in needs Config.PromptURL: with no browser launched and no way to report the url, the operator has nothing to open") } @@ -395,7 +409,11 @@ func (c Config) now() time.Time { // presentAuthorizeURL gets the operator to the authorize URL, by launching a // browser or, under NoBrowser, by handing the URL to PromptURL. func (c Config) presentAuthorizeURL(rawURL string) error { - if c.NoBrowser { + // ReadRedirect implies the prompt. A machine that cannot receive the + // callback generally cannot open a browser either, and where it could, the + // listener path would have worked and been less work for the operator. + // Launching a browser and then also asking for a paste is a footgun. + if c.NoBrowser || c.ReadRedirect != nil { if err := c.PromptURL(rawURL); err != nil { return fmt.Errorf("present the sign-in url: %w", err) } @@ -410,6 +428,91 @@ func (c Config) presentAuthorizeURL(rawURL string) error { return nil } +// collectCode shows the operator the authorize URL and returns the code they +// came back with, by whichever route this Config selected. +// +// The loopback listener is the default. ReadRedirect replaces it with a +// paste-back exchange for a machine that can neither open a browser nor be +// reached on its callback port. +func (c Config) collectCode(ctx context.Context, state, challenge string) (code, gotState string, err error) { + if c.ReadRedirect != nil { + // No port is bound, so there is nothing to fail early on: show the URL, + // then wait on the operator rather than on the network. + if err := c.presentAuthorizeURL(c.buildAuthorizeURL(state, challenge)); err != nil { + return "", "", err + } + + pasted, err := c.ReadRedirect() + if err != nil { + return "", "", fmt.Errorf("read the pasted redirect: %w", err) + } + + return codeFromRedirect(pasted) + } + + // Bind the callback port BEFORE sending the operator to Cognito. If the + // port is unavailable the redirect could never land, and there is no + // fallback port to try, so failing here saves a pointless round trip. + server, err := startCallbackServer(c.CallbackAddr, callbackPath(c.RedirectURI)) + if err != nil { + return "", "", err + } + defer server.shutdown() + + if err := c.presentAuthorizeURL(c.buildAuthorizeURL(state, challenge)); err != nil { + return "", "", err + } + + return server.wait(ctx, defaultCallbackTimeout) +} + +// codeFromRedirect reads the authorization code and state out of a redirect URL +// an operator pasted back. +// +// The host and scheme are deliberately not checked. The operator is copying +// from their own address bar, and what binds the code to this attempt is the +// state check the caller performs, not the shape of the URL. A bare code is +// refused for the same reason: with no state there is nothing to bind it to. +func codeFromRedirect(pasted string) (code, state string, err error) { + trimmed := strings.TrimSpace(pasted) + if wegostrings.IsBlank(trimmed) { + return "", "", errors.New("nothing was pasted: copy the whole URL the browser was redirected to, including the ?code=... part") + } + + parsed, err := url.Parse(trimmed) + if err != nil { + return "", "", fmt.Errorf("parse the pasted redirect: %w", err) + } + + if wegostrings.IsBlank(parsed.RawQuery) { + return "", "", errors.New("the pasted value has no query string: copy the whole URL from the address bar, including the ?code=... part") + } + + query := parsed.Query() + + // Cognito reports a refusal in the redirect rather than as a failed + // request, so this is where a denied sign-in surfaces. + if reported := query.Get("error"); wegostrings.IsNotBlank(reported) { + if description := query.Get("error_description"); wegostrings.IsNotBlank(description) { + return "", "", fmt.Errorf("the sign-in was refused: %s (%s)", reported, description) + } + + return "", "", fmt.Errorf("the sign-in was refused: %s", reported) + } + + code = query.Get("code") + if wegostrings.IsBlank(code) { + return "", "", errors.New("the pasted redirect carries no authorization code") + } + + state = query.Get("state") + if wegostrings.IsBlank(state) { + return "", "", errors.New("the pasted redirect carries no state, so it cannot be tied to this sign-in attempt") + } + + return code, state, nil +} + func (c Config) openBrowserAt(rawURL string) error { if c.OpenBrowser != nil { return c.OpenBrowser(rawURL) diff --git a/cognito/oauth_internal_test.go b/cognito/oauth_internal_test.go index 6b84acf..afd558e 100644 --- a/cognito/oauth_internal_test.go +++ b/cognito/oauth_internal_test.go @@ -218,3 +218,151 @@ func TestPostToken_RefusesRedirects(t *testing.T) { }) } } + +// TestCodeFromRedirect covers pulling the authorization code out of the URL an +// operator pasted back. +// +// This is the sign-in path for a machine with no browser AND no way to forward +// the callback port: Cognito redirects to a loopback URL nothing is listening +// on, the browser shows a connection error, and the address bar holds the code. +func TestCodeFromRedirect(t *testing.T) { + tests := []struct { + name string + givenInput string + wantCode string + wantState string + wantErr string + }{ + { + name: "a pasted redirect yields its code and state", + givenInput: "http://localhost:8100/callback?code=abc123&state=xyz789", + wantCode: "abc123", + wantState: "xyz789", + }, + { + name: "surrounding whitespace from a copy is tolerated", + givenInput: " http://localhost:8100/callback?code=abc123&state=xyz789\n", + wantCode: "abc123", + wantState: "xyz789", + }, + { + name: "https and an unexpected host are accepted; the state check is the guard", + givenInput: "https://example.test/cb?state=xyz789&code=abc123", + wantCode: "abc123", + wantState: "xyz789", + }, + { + name: "percent-encoded values are decoded", + givenInput: "http://localhost:8100/callback?code=a%2Fb%2Bc&state=s%3D1", + wantCode: "a/b+c", + wantState: "s=1", + }, + {name: "nothing pasted", givenInput: " ", wantErr: "nothing was pasted"}, + { + // The commonest mistake: copying the path but not the query. + name: "a url with no query says what is missing", + givenInput: "http://localhost:8100/callback", + wantErr: "no query", + }, + { + name: "a bare code is refused because it carries no state", + givenInput: "abc123", + wantErr: "no query", + }, + { + name: "a denied sign-in reports the provider's error", + givenInput: "http://localhost:8100/callback?error=access_denied&error_description=User+denied", + wantErr: "access_denied", + }, + { + name: "a denied sign-in includes the description when there is one", + givenInput: "http://localhost:8100/callback?error=access_denied&error_description=User+denied", + wantErr: "User denied", + }, + { + name: "a query with state but no code is refused", + givenInput: "http://localhost:8100/callback?state=xyz789", + wantErr: "no authorization code", + }, + { + // Without state there is nothing to bind the code to this attempt. + name: "a query with code but no state is refused", + givenInput: "http://localhost:8100/callback?code=abc123", + wantErr: "no state", + }, + { + name: "an unparsable url is reported", + givenInput: "http://[::1", + wantErr: "parse", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + code, state, err := codeFromRedirect(tt.givenInput) + + if tt.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + assert.Empty(t, code) + + return + } + + require.NoError(t, err) + assert.Equal(t, tt.wantCode, code) + assert.Equal(t, tt.wantState, state) + }) + } +} + +// TestValidateForLogin_PasteBack covers the config rules the paste-back path +// adds: it needs somewhere to show the URL, and it does NOT need a callback +// port, since nothing binds one. +func TestValidateForLogin_PasteBack(t *testing.T) { + base := func() Config { + return Config{ + AuthorizeURL: "https://cognito.test/oauth2/authorize", + TokenURL: "https://cognito.test/oauth2/token", + ClientID: "client", + RedirectURI: "http://localhost:8100/callback", + } + } + + t.Run("paste-back without PromptURL is refused", func(t *testing.T) { + cfg := base() + cfg.ReadRedirect = func() (string, error) { return "", nil } + + require.ErrorContains(t, cfg.validateForLogin(), "PromptURL") + }) + + t.Run("paste-back needs no callback address", func(t *testing.T) { + cfg := base() + cfg.ReadRedirect = func() (string, error) { return "", nil } + cfg.PromptURL = func(string) error { return nil } + + require.NoError(t, cfg.validateForLogin(), "nothing binds a port on this path") + }) + + t.Run("the listener path still needs a callback address", func(t *testing.T) { + require.ErrorContains(t, base().validateForLogin(), "callback address") + }) +} + +// TestPresentAuthorizeURL_PasteBackImpliesThePrompt pins the coupling: setting +// ReadRedirect must not leave a browser being launched, or the operator gets a +// browser AND a paste prompt for the same sign-in. +func TestPresentAuthorizeURL_PasteBackImpliesThePrompt(t *testing.T) { + var prompted, opened string + + cfg := Config{ + PromptURL: func(u string) error { prompted = u; return nil }, + OpenBrowser: func(u string) error { opened = u; return nil }, + ReadRedirect: func() (string, error) { return "", nil }, + } + + require.NoError(t, cfg.presentAuthorizeURL("https://cognito.test/authorize?state=s")) + + assert.Equal(t, "https://cognito.test/authorize?state=s", prompted) + assert.Empty(t, opened, "no browser may be launched on the paste-back path") +} diff --git a/cognito/oauth_test.go b/cognito/oauth_test.go index b7c70bc..efbcc65 100644 --- a/cognito/oauth_test.go +++ b/cognito/oauth_test.go @@ -763,3 +763,95 @@ func TestLogin_RejectsAnUnusableExpiresIn(t *testing.T) { }) } } + +// TestLogin_PasteBack drives a whole sign-in through the paste-back path: no +// browser is launched, no callback port is bound, and the code arrives as the +// redirect URL an operator copied out of their address bar. +func TestLogin_PasteBack(t *testing.T) { + ts := newTokenServer(t, nil) + + var shown string + + cfg := cognito.Config{ + AuthorizeURL: "https://cognito.test/oauth2/authorize", + TokenURL: ts.URL, + ClientID: "test-client-id", + RedirectURI: "http://localhost:8100/callback", + Scopes: "openid email", + AllowedDomain: "@wego.com", + Now: func() time.Time { return fixedNow }, + PromptURL: func(u string) error { shown = u; return nil }, + } + // CallbackAddr is deliberately left empty: nothing binds a port here, and + // that is the whole reason this path exists. + cfg.ReadRedirect = func() (string, error) { + return "http://localhost:8100/callback?code=pasted-code&state=" + stateOf(t, shown), nil + } + + tokens, err := cognito.Login(context.Background(), cfg) + require.NoError(t, err) + require.NotNil(t, tokens) + + assert.Contains(t, shown, "code_challenge_method=S256", "the operator must be shown a PKCE authorize url") + + form := ts.lastForm(t) + assert.Equal(t, "pasted-code", form.Get("code"), "the pasted code must be what is redeemed") + assert.NotEmpty(t, form.Get("code_verifier"), "PKCE must still bind the exchange to this process") + assert.Equal(t, "authorization_code", form.Get("grant_type")) +} + +// TestLogin_PasteBackRejectsAForeignState covers the guard that replaces the +// listener's origin check. Nothing about a pasted URL proves where it came +// from except the state, so a code from another attempt must be refused. +func TestLogin_PasteBackRejectsAForeignState(t *testing.T) { + ts := newTokenServer(t, nil) + + cfg := cognito.Config{ + AuthorizeURL: "https://cognito.test/oauth2/authorize", + TokenURL: ts.URL, + ClientID: "test-client-id", + RedirectURI: "http://localhost:8100/callback", + AllowedDomain: "@wego.com", + Now: func() time.Time { return fixedNow }, + PromptURL: func(string) error { return nil }, + ReadRedirect: func() (string, error) { + return "http://localhost:8100/callback?code=pasted-code&state=someone-elses-state", nil + }, + } + + _, err := cognito.Login(context.Background(), cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "state mismatch") + assert.Empty(t, ts.forms, "a code with a foreign state must never reach the token endpoint") +} + +// TestLogin_PasteBackReportsAReadFailure covers the operator abandoning the +// prompt, e.g. ctrl-D at the paste. +func TestLogin_PasteBackReportsAReadFailure(t *testing.T) { + cfg := cognito.Config{ + AuthorizeURL: "https://cognito.test/oauth2/authorize", + TokenURL: "https://unused.test/oauth2/token", + ClientID: "test-client-id", + RedirectURI: "http://localhost:8100/callback", + AllowedDomain: "@wego.com", + PromptURL: func(string) error { return nil }, + ReadRedirect: func() (string, error) { return "", errors.New("stdin closed") }, + } + + _, err := cognito.Login(context.Background(), cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "stdin closed") +} + +// stateOf pulls the state parameter out of an authorize URL. +func stateOf(t *testing.T, authorizeURL string) string { + t.Helper() + + parsed, err := url.Parse(authorizeURL) + require.NoError(t, err) + + state := parsed.Query().Get("state") + require.NotEmpty(t, state, "the authorize url must carry a state") + + return state +} From 73bc0e92df621f0a112a7320692918e584b473a5 Mon Sep 17 00:00:00 2001 From: mysqto Date: Mon, 7 Sep 2026 16:33:35 +0800 Subject: [PATCH 09/12] cognito/storage: give every saved session its own generation Round 4 of review on wego/payments#2300 found the two-slot layout still allowed a hybrid credential set, by two schedules the round-3 Load retry did not touch. Both are now regressions in this package, and both reproduced the reported outputs before the fix. Save/Save: two writers read the same live slot, both computed the same other slot, and interleaved field writes into it. Both committed, so one slot held one session's access token beside another's id and refresh tokens ("second-access with first-id and first-refresh"). ABA: with two reusable slots the pointer could cycle A->B->A, so a generation that HAD changed under an in-flight Load looked unchanged, Load's moved-pointer check saw no move, and it returned fields it never selected ("seed-access with second-id and second-refresh"). This is worse than untidy. The id token carries the operator identity that admin writes are audited against, so a mismatched pair can attribute a production change to the wrong person, which is why the finding is HIGH rather than a tidiness nit. Fields now live under a random generation name that is never reused, with the pointer naming the live one. Concurrent writers are disjoint by construction, so each generation is whole and the later commit simply wins; and because a name never repeats, any change under a reader is detectable, which removes ABA structurally rather than by timing. Chosen over the process-shared lock the review also offered: this module has no file-lock dependency and no platform-specific code, and a lock would need both plus stale-holder handling on three platforms. The cost is that a keychain cannot be enumerated, so only the generations the pointer names can be reaped. The pointer therefore remembers the one it replaced, and Delete clears both. A generation orphaned by a crash mid-Save, or by two Saves overlapping, is not reachable: it holds a superseded set no code path returns, and Cognito refresh tokens expire, so it decays rather than accumulating. That window is documented on Delete. Closing it entirely is what the locking option would buy. Save no longer fails when the pointer cannot be read. The old layout had to know the live slot to avoid overwriting it; a fresh generation collides with nothing, so an unreadable pointer costs only the chance to reap and no longer blocks a sign-in. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01JCCcwc4wtJgYKSFygyZGnR --- cognito/storage/keyring.go | 260 ++++++++------- cognito/storage/keyring_internal_test.go | 396 +++++++++++++++++++---- 2 files changed, 483 insertions(+), 173 deletions(-) diff --git a/cognito/storage/keyring.go b/cognito/storage/keyring.go index f871f44..936cc34 100644 --- a/cognito/storage/keyring.go +++ b/cognito/storage/keyring.go @@ -1,6 +1,8 @@ package storage import ( + "crypto/rand" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -22,33 +24,47 @@ import ( // field keeps its own entry. // // That rules out getting atomicity by writing one entry, and a keychain has no -// transaction. Instead each token set is written into one of two SLOTS, and a -// separate pointer entry names the slot that counts. Save fills the inactive -// slot and then moves the pointer, which is one small write and the only write -// that changes what Load sees. A failure anywhere before it leaves the pointer -// and the live slot untouched, so a torn write costs the new session, never -// the old one. +// transaction. Instead each token set is written under a fresh GENERATION +// name, and a separate pointer entry names the generation that counts. Save +// writes its fields and then moves the pointer, which is one small write and +// the only write that changes what Load sees. A failure anywhere before it +// leaves the pointer and the live generation untouched, so a torn write costs +// the new session, never the old one. // -// Two slots rather than a counter keeps the entry count fixed and means a -// re-login never writes over the entries the current session is read from. +// Generations are UNIQUE AND NEVER REUSED, and that is load-bearing twice +// over. An earlier version alternated between two fixed slots, which broke in +// two ways review found: +// +// - Two concurrent Save calls both read the same live slot, both computed +// the same "other" slot, and interleaved their field writes into it. Both +// committed, leaving one slot holding one session's access token beside +// another's id and refresh tokens. Fresh names make the writers disjoint, +// so each generation is whole and the later commit simply wins. +// - The pointer could cycle A->B->A, so a generation that HAD changed under +// an in-flight Load looked unchanged and Load returned fields it never +// selected. A name that never repeats makes any change detectable. +// +// A hybrid is not merely untidy: the id token carries the operator identity +// that admin writes are audited against, so pairing it with another session's +// access or refresh token can attribute a production change to the wrong +// person. // // Entries are accounted as "/current" for the pointer and -// "//" for the token fields. +// "//" for the token fields. const ( fieldAccess = "access" fieldID = "id" fieldRefresh = "refresh" fieldMeta = "meta" - // fieldCurrent is the pointer entry naming the live slot. Committing a - // token set is exactly one write of this entry. + // fieldCurrent is the pointer entry naming the live generation. Committing + // a token set is exactly one write of this entry. fieldCurrent = "current" ) -// The two slots a token set alternates between. -const ( - slotA = "a" - slotB = "b" -) +// generationBytes is the entropy in a generation name. A generation is never +// reused, so this only has to make an accidental collision between two +// concurrent writers impossible in practice. +const generationBytes = 12 // loadAttempts is how many times Load will re-read after a concurrent commit // moves the pointer out from under it. Each retry needs another process to @@ -88,6 +104,14 @@ type keyringMeta struct { ExpiresAt time.Time `json:"expires_at"` } +// keyringPointer is the commit record: which generation is live, and the one +// it replaced. Previous is kept only so Delete can reap it, since a keychain +// cannot be enumerated. +type keyringPointer struct { + Current string `json:"current"` + Previous string `json:"previous,omitempty"` +} + // keyringStore persists tokens in the OS keychain. type keyringStore struct { service string @@ -114,22 +138,22 @@ func (k *keyringStore) Load(namespace string) (*cognito.TokenSet, error) { } for range loadAttempts { - slot, err := k.liveSlot(namespace) - if err != nil || slot == "" { + pointer, err := k.readPointer(namespace) + if err != nil || wegostrings.IsBlank(pointer.Current) { // No pointer means not signed in, which is (nil, nil). return nil, err } - tokens, err := k.readSlot(namespace, slot) + tokens, err := k.readGeneration(namespace, pointer.Current) if err == nil { return tokens, nil } // The read failed. If the pointer has moved since it was chosen, this - // slot was superseded and cleared under us, so try the new one. If it - // has not, the namespace really is inconsistent and the original error - // is the honest one to report. - moved, checkErr := k.slotMoved(namespace, slot) + // generation was superseded and cleared under us, so try the new one. + // If it has not, the namespace really is inconsistent and the original + // error is the honest one to report. + moved, checkErr := k.generationMoved(namespace, pointer.Current) if checkErr != nil { return nil, checkErr } @@ -144,52 +168,72 @@ func (k *keyringStore) Load(namespace string) (*cognito.TokenSet, error) { return nil, fmt.Errorf("the keychain session for %q was replaced %d times while being read", namespace, loadAttempts) } -// liveSlot returns the slot the pointer names, or "" when nothing is stored. -func (k *keyringStore) liveSlot(namespace string) (string, error) { - slot, err := k.backend.Get(k.service, pointerAccount(namespace)) +// readPointer returns the commit record, or a zero record when nothing is +// stored. +func (k *keyringStore) readPointer(namespace string) (keyringPointer, error) { + raw, err := k.backend.Get(k.service, pointerAccount(namespace)) if errors.Is(err, keyring.ErrNotFound) { - return "", nil + return keyringPointer{}, nil } if err != nil { - return "", fmt.Errorf("read the keychain: %w (is the system keychain unlocked?)", err) + return keyringPointer{}, fmt.Errorf("read the keychain: %w (is the system keychain unlocked?)", err) + } + + var pointer keyringPointer + if err := json.Unmarshal([]byte(raw), &pointer); err != nil { + // Nothing here writes anything but this record, so the entry was + // tampered with or written by a version that stored something else. + return keyringPointer{}, fmt.Errorf("parse the token pointer for %q: %w", namespace, err) } - if slot != slotA && slot != slotB { - // Nothing here writes any other value, so this entry was tampered with - // or written by a version that stored something else. Guessing which - // slot was meant would be worse than refusing. - return "", fmt.Errorf("keychain names an unknown token slot %q for %q", slot, namespace) + if wegostrings.IsBlank(pointer.Current) { + return keyringPointer{}, fmt.Errorf("the token pointer for %q names no generation", namespace) } - return slot, nil + return pointer, nil } -// slotMoved reports whether the pointer now names a slot other than the one a -// read was using. -func (k *keyringStore) slotMoved(namespace, slot string) (bool, error) { - current, err := k.liveSlot(namespace) +// generationMoved reports whether the pointer now names a generation other +// than the one a read was using. +// +// Because a generation name is never reused, this is exact: an unchanged value +// really means nothing committed in between. Under the old two-slot layout the +// pointer could cycle back to the value a reader started on, so a changed +// generation looked unchanged. +func (k *keyringStore) generationMoved(namespace, generation string) (bool, error) { + pointer, err := k.readPointer(namespace) if err != nil { return false, err } - return current != slot, nil + return pointer.Current != generation, nil } -// readSlot assembles the token set held in one slot. -func (k *keyringStore) readSlot(namespace, slot string) (*cognito.TokenSet, error) { - access, err := k.read(namespace, slot, fieldAccess) +// newGeneration mints a name no other writer will pick. +func newGeneration() (string, error) { + buf := make([]byte, generationBytes) + if _, err := rand.Read(buf); err != nil { + return "", fmt.Errorf("generate a token generation name: %w", err) + } + + return hex.EncodeToString(buf), nil +} + +// readGeneration assembles the token set held under one generation. +func (k *keyringStore) readGeneration(namespace, generation string) (*cognito.TokenSet, error) { + access, err := k.read(namespace, generation, fieldAccess) if err != nil { return nil, err } - idToken, err := k.read(namespace, slot, fieldID) + idToken, err := k.read(namespace, generation, fieldID) if err != nil { return nil, err } - refresh, err := k.read(namespace, slot, fieldRefresh) + refresh, err := k.read(namespace, generation, fieldRefresh) if err != nil { return nil, err } - rawMeta, err := k.read(namespace, slot, fieldMeta) + rawMeta, err := k.read(namespace, generation, fieldMeta) if err != nil { return nil, err } @@ -209,10 +253,12 @@ func (k *keyringStore) readSlot(namespace, slot string) (*cognito.TokenSet, erro // Save writes tokens under namespace, replacing anything already there. // -// It is atomic from Load's point of view: the fields go into the slot that is -// not live, and only the final pointer write makes them the session. If any -// write fails, the previous session is still whole and still what Load -// returns. +// It is atomic from Load's point of view and safe against a concurrent Save. +// The fields go under a fresh generation no other writer will pick, and only +// the final pointer write makes them the session. If any write fails, the +// previous session is still whole and still what Load returns; if another Save +// overlaps, the two write disjoint generations and the later commit wins with +// its own session intact. func (k *keyringStore) Save(namespace string, tokens *cognito.TokenSet) error { if err := k.validate(namespace); err != nil { return err @@ -226,13 +272,16 @@ func (k *keyringStore) Save(namespace string, tokens *cognito.TokenSet) error { return fmt.Errorf("encode token metadata: %w", err) } - live, err := k.currentSlot(namespace) + // A pointer that cannot be read is not a reason to refuse a sign-in: the + // new generation does not collide with anything either way. What is lost is + // only the chance to reap the generation being replaced. + previous, _ := k.readPointer(namespace) + + generation, err := newGeneration() if err != nil { return err } - next := otherSlot(live) - values := map[string]string{ fieldAccess: tokens.AccessToken, fieldID: tokens.IDToken, @@ -240,95 +289,78 @@ func (k *keyringStore) Save(namespace string, tokens *cognito.TokenSet) error { fieldMeta: string(meta), } - // Ordering is irrelevant now: nothing written here is reachable until the - // pointer moves. tokenFields is used rather than ranging the map so the - // write sequence is deterministic, which keeps failures reproducible. + // Ordering is irrelevant: nothing written here is reachable until the + // pointer moves, and no other writer shares this generation. tokenFields is + // used rather than ranging the map so the sequence is deterministic, which + // keeps failures reproducible. for _, field := range tokenFields { - if err := k.backend.Set(k.service, fieldAccount(namespace, next, field), values[field]); err != nil { + if err := k.backend.Set(k.service, fieldAccount(namespace, generation, field), values[field]); err != nil { return fmt.Errorf("write %s to the keychain: %w (is the system keychain unlocked?)", field, err) } } - // The commit. - if err := k.backend.Set(k.service, pointerAccount(namespace), next); err != nil { - return fmt.Errorf("commit the session to the keychain: %w (is the system keychain unlocked?)", err) - } - - // The old slot is now unreachable. Clearing it keeps a superseded token set - // out of the keychain, but the session is already committed, so a failure - // here is not the caller's problem and must not fail the sign-in. - k.clearSlot(namespace, live) - - return nil -} - -// currentSlot reports the live slot, or "" when nothing is stored. An -// unrecognised value is treated as "nothing live": Save's job is to establish a -// good session, and it can do that without deciding what the bad value meant. -// Load is where a corrupt pointer is reported. -func (k *keyringStore) currentSlot(namespace string) (string, error) { - slot, err := k.backend.Get(k.service, pointerAccount(namespace)) - if errors.Is(err, keyring.ErrNotFound) { - return "", nil - } + committed, err := json.Marshal(keyringPointer{Current: generation, Previous: previous.Current}) if err != nil { - return "", fmt.Errorf("read the current token slot: %w (is the system keychain unlocked?)", err) + return fmt.Errorf("encode the token pointer: %w", err) } - if slot != slotA && slot != slotB { - return "", nil + // The commit. + if err := k.backend.Set(k.service, pointerAccount(namespace), string(committed)); err != nil { + return fmt.Errorf("commit the session to the keychain: %w (is the system keychain unlocked?)", err) } - return slot, nil -} - -// otherSlot returns the slot to write next. An empty live slot means nothing is -// stored, so either is free and slotA keeps a first login predictable. -func otherSlot(live string) string { - if live == slotA { - return slotB - } + // The generation before the one just replaced is now unreachable by any + // reader that started before this commit, so it is safe to clear. The one + // directly replaced is deliberately KEPT until the next Save, because a + // Load may still be part way through reading it; Load's retry handles the + // rest. Failure here is not the caller's problem: the session is committed. + k.clearGeneration(namespace, previous.Previous) - return slotA + return nil } -// clearSlot removes one slot's field entries, best effort. Callers use it for a -// slot nothing points at any more. -func (k *keyringStore) clearSlot(namespace, slot string) { - if slot == "" { +// clearGeneration removes one generation's field entries, best effort. Callers +// use it for a generation nothing can still be reading. +func (k *keyringStore) clearGeneration(namespace, generation string) { + if wegostrings.IsBlank(generation) { return } for _, field := range tokenFields { - _ = k.backend.Delete(k.service, fieldAccount(namespace, slot, field)) + _ = k.backend.Delete(k.service, fieldAccount(namespace, generation, field)) } } // Delete removes every entry under namespace. Entries already gone are fine: // the desired end state is "signed out", so signing out twice must succeed. // -// The pointer goes FIRST, for the same reason Save moves it last: once it is -// gone the operator is signed out, even if a later delete fails and leaves -// orphaned field entries behind. +// The pointer is read first so the generations it names can be cleared, then +// removed BEFORE them: once it is gone the operator is signed out even if a +// later delete fails. +// +// A keychain cannot be enumerated, so only the generations the pointer knows +// about can be reaped. A generation orphaned by a crash between its field +// writes and its commit, or by two Saves overlapping, is therefore not +// reachable here. It holds a superseded token set that no code path will ever +// return, and Cognito refresh tokens expire, so it decays rather than +// accumulating indefinitely. Eliminating that window needs process-shared +// locking, which is a larger change than this seam warrants. func (k *keyringStore) Delete(namespace string) error { if err := k.validate(namespace); err != nil { return err } + // A pointer that cannot be parsed must not block signing out; the entry is + // removed below regardless. + pointer, _ := k.readPointer(namespace) + err := k.backend.Delete(k.service, pointerAccount(namespace)) if err != nil && !errors.Is(err, keyring.ErrNotFound) { - return fmt.Errorf("delete the current token slot from the keychain: %w (is the system keychain unlocked?)", err) + return fmt.Errorf("delete the token pointer from the keychain: %w (is the system keychain unlocked?)", err) } - // Both slots, not just the live one: a torn Save can leave fields in the - // inactive slot, and signing out should not leave a token set behind. - for _, slot := range []string{slotA, slotB} { - for _, field := range tokenFields { - err := k.backend.Delete(k.service, fieldAccount(namespace, slot, field)) - if err != nil && !errors.Is(err, keyring.ErrNotFound) { - return fmt.Errorf("delete %s from the keychain: %w (is the system keychain unlocked?)", field, err) - } - } + for _, generation := range []string{pointer.Current, pointer.Previous} { + k.clearGeneration(namespace, generation) } return nil @@ -337,8 +369,8 @@ func (k *keyringStore) Delete(namespace string) error { // read fetches one field of one slot, treating a missing entry as an error: // callers only reach it after the pointer has confirmed a committed session, // so anything absent is an inconsistency rather than a logged-out state. -func (k *keyringStore) read(namespace, slot, field string) (string, error) { - value, err := k.backend.Get(k.service, fieldAccount(namespace, slot, field)) +func (k *keyringStore) read(namespace, generation, field string) (string, error) { + value, err := k.backend.Get(k.service, fieldAccount(namespace, generation, field)) if err != nil { return "", fmt.Errorf("read %s from the keychain: %w", field, err) } @@ -361,7 +393,7 @@ func pointerAccount(namespace string) string { return namespace + "/" + fieldCurrent } -// fieldAccount is the keychain account name for one field of one slot. -func fieldAccount(namespace, slot, field string) string { - return namespace + "/" + slot + "/" + field +// fieldAccount is the keychain account name for one field of one generation. +func fieldAccount(namespace, generation, field string) string { + return namespace + "/" + generation + "/" + field } diff --git a/cognito/storage/keyring_internal_test.go b/cognito/storage/keyring_internal_test.go index cfef6a5..361e8bf 100644 --- a/cognito/storage/keyring_internal_test.go +++ b/cognito/storage/keyring_internal_test.go @@ -1,7 +1,10 @@ package storage import ( + "encoding/json" "errors" + "fmt" + "strings" "sync" "testing" "time" @@ -226,10 +229,10 @@ func TestKeyringStore_LoadFailures(t *testing.T) { { // Fields present but never committed: the sign-in was torn before // the pointer moved, so the operator is simply not signed in. - name: "an uncommitted slot reads as not logged in", + name: "an uncommitted generation reads as not logged in", givenSetup: func(f *fakeKeyring) { - f.putField(slotA, fieldAccess, "access-value") - f.putField(slotA, fieldID, "id-value") + f.putField(testGeneration, fieldAccess, "access-value") + f.putField(testGeneration, fieldID, "id-value") }, wantNil: true, }, @@ -283,36 +286,59 @@ func TestKeyringStore_SaveCommitsWithOnePointerWrite(t *testing.T) { assert.Equal(t, 1, countWrites(backend.writes, testNamespace+"/"+fieldCurrent), "the commit must be a single pointer write") - for _, field := range []string{fieldAccess, fieldID, fieldRefresh, fieldMeta} { - assert.Contains(t, backend.writes, testNamespace+"/"+slotA+"/"+field, + generation := backend.liveGeneration(t) + for _, field := range tokenFields { + assert.Contains(t, backend.writes, testNamespace+"/"+generation+"/"+field, "each field keeps its own entry, to stay under the 4 KiB cap") } } -// TestKeyringStore_SaveAlternatesSlots covers why there are two slots: a -// re-login must not overwrite the entries the current session is still being -// read from, or a torn write would corrupt the live session rather than an -// unused copy. -func TestKeyringStore_SaveAlternatesSlots(t *testing.T) { +// TestKeyringStore_SaveNeverReusesAGeneration covers the property both +// round-4 findings turn on. A name that can come round again lets two writers +// collide and lets a pointer cycle back to a value a reader started on. +func TestKeyringStore_SaveNeverReusesAGeneration(t *testing.T) { backend := newFakeKeyring() store := &keyringStore{service: testService, backend: backend} - require.NoError(t, store.Save(testNamespace, sampleTokens())) - assert.Equal(t, slotA, backend.value(fieldCurrent)) + seen := map[string]bool{} - second := sampleTokens() - second.AccessToken = "second-access" - require.NoError(t, store.Save(testNamespace, second)) - assert.Equal(t, slotB, backend.value(fieldCurrent)) + for i := range 6 { + tokens := sampleTokens() + tokens.AccessToken = fmt.Sprintf("access-%d", i) + require.NoError(t, store.Save(testNamespace, tokens)) - third := sampleTokens() - third.AccessToken = "third-access" - require.NoError(t, store.Save(testNamespace, third)) - assert.Equal(t, slotA, backend.value(fieldCurrent), "slots alternate rather than growing") + generation := backend.liveGeneration(t) + assert.False(t, seen[generation], "generation %q was reused", generation) + seen[generation] = true - got, err := store.Load(testNamespace) - require.NoError(t, err) - assert.Equal(t, "third-access", got.AccessToken) + got, err := store.Load(testNamespace) + require.NoError(t, err) + assert.Equal(t, tokens.AccessToken, got.AccessToken, "the newest session must be the live one") + } + + assert.Len(t, seen, 6) +} + +// TestKeyringStore_SaveReapsTheGenerationBeforeLast covers cleanup. The +// generation directly replaced is kept, because a Load may still be reading +// it; the one before that cannot have a reader who started after its +// replacement committed, so it goes. +func TestKeyringStore_SaveReapsTheGenerationBeforeLast(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + + require.NoError(t, store.Save(testNamespace, session("first"))) + first := backend.liveGeneration(t) + + require.NoError(t, store.Save(testNamespace, session("second"))) + second := backend.liveGeneration(t) + + require.NoError(t, store.Save(testNamespace, session("third"))) + + assert.Empty(t, backend.entries[testService+"|"+testNamespace+"/"+first+"/"+fieldAccess], + "the generation before last must be cleared") + assert.NotEmpty(t, backend.entries[testService+"|"+testNamespace+"/"+second+"/"+fieldAccess], + "the generation just replaced is kept for an in-flight Load") } // TestKeyringStore_TornResaveLeavesThePreviousSession is the regression test @@ -351,18 +377,29 @@ func TestKeyringStore_TornResaveLeavesThePreviousSession(t *testing.T) { "a torn re-login must leave the previous session exactly, never a mixture of the two") } -// TestKeyringStore_LoadRejectsAnUnknownSlot covers a pointer naming a slot that -// is not one of the two. That cannot arise from this code, so it means the -// entry was tampered with or written by another version, and guessing which -// slot was meant would be worse than refusing. -func TestKeyringStore_LoadRejectsAnUnknownSlot(t *testing.T) { - backend := newFakeKeyring() - backend.put(fieldCurrent, "somewhere-else") - store := &keyringStore{service: testService, backend: backend} +// TestKeyringStore_LoadRejectsAnUnreadablePointer covers a pointer entry this +// code did not write. Guessing what it meant would be worse than refusing. +func TestKeyringStore_LoadRejectsAnUnreadablePointer(t *testing.T) { + tests := []struct { + name string + givenValue string + wantErr string + }{ + {name: "not json", givenValue: "somewhere-else", wantErr: "parse the token pointer"}, + {name: "json naming no generation", givenValue: `{"current":""}`, wantErr: "names no generation"}, + } - _, err := store.Load(testNamespace) - require.Error(t, err) - assert.Contains(t, err.Error(), "slot") + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + backend := newFakeKeyring() + backend.put(fieldCurrent, tt.givenValue) + store := &keyringStore{service: testService, backend: backend} + + _, err := store.Load(testNamespace) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantErr) + }) + } } // countWrites reports how many times account appears in a write log. @@ -420,12 +457,14 @@ func TestKeyringStore_BackendFailuresAreReported(t *testing.T) { // must clear the slot a torn Save left behind as well as the live one. func TestKeyringStore_DeleteToleratesMissingEntries(t *testing.T) { backend := newFakeKeyring() - backend.putField(slotA, fieldAccess, "access-value") - backend.putField(slotB, fieldRefresh, "orphaned-refresh") + backend.putField("gen-current", fieldAccess, "access-value") + backend.putField("gen-previous", fieldRefresh, "superseded-refresh") + backend.putPointer("gen-current", "gen-previous") store := &keyringStore{service: testService, backend: backend} require.NoError(t, store.Delete(testNamespace)) - assert.Empty(t, backend.entries, "no token material may survive a sign-out, in either slot") + assert.Empty(t, backend.entries, + "no token material may survive a sign-out, in either generation the pointer knows") } // TestKeyringStore_FailedCommitKeepsThePreviousSession covers the last write. @@ -456,32 +495,39 @@ func TestKeyringStore_FailedCommitKeepsThePreviousSession(t *testing.T) { assert.Equal(t, sampleTokens(), got, "an uncommitted session must not become the live one") } -// TestKeyringStore_SaveReportsAnUnreadablePointer covers a keychain that -// cannot be read at all. Save must not proceed on a guess about which slot is -// live: writing to the wrong one would overwrite the session it is meant to -// protect. -func TestKeyringStore_SaveReportsAnUnreadablePointer(t *testing.T) { +// TestKeyringStore_SaveStillSignsInWhenThePointerCannotBeRead covers a +// keychain that cannot be read at all. +// +// Under the old two-slot layout Save HAD to read the pointer, because it wrote +// to whichever slot the live one was not using; an unreadable pointer +// therefore had to fail rather than risk overwriting the live session. A fresh +// generation collides with nothing, so the read is now only an optimisation +// for reaping, and a sign-in can succeed without it. +func TestKeyringStore_SaveStillSignsInWhenThePointerCannotBeRead(t *testing.T) { backend := newFakeKeyring() backend.getErr = errors.New("keychain is locked") store := &keyringStore{service: testService, backend: backend} - err := store.Save(testNamespace, sampleTokens()) - require.Error(t, err) - assert.Contains(t, err.Error(), "slot") - assert.Empty(t, backend.writes, "nothing may be written when the live slot is unknown") + require.NoError(t, store.Save(testNamespace, sampleTokens()), + "an unreadable pointer must not block establishing a session") + + backend.getErr = nil + + got, err := store.Load(testNamespace) + require.NoError(t, err) + assert.Equal(t, sampleTokens(), got) } -// TestKeyringStore_SaveOverAnUnknownSlotStillSignsIn covers a pointer holding -// a value this code never writes. Load refuses it, but Save's job is to +// TestKeyringStore_SaveOverAnUnreadablePointerStillSignsIn covers a pointer +// holding a value this code never wrote. Load refuses it, but Save's job is to // establish a good session and it can do that without deciding what the bad // value meant. -func TestKeyringStore_SaveOverAnUnknownSlotStillSignsIn(t *testing.T) { +func TestKeyringStore_SaveOverAnUnreadablePointerStillSignsIn(t *testing.T) { backend := newFakeKeyring() backend.put(fieldCurrent, "somewhere-else") store := &keyringStore{service: testService, backend: backend} require.NoError(t, store.Save(testNamespace, sampleTokens())) - assert.Equal(t, slotA, backend.value(fieldCurrent)) got, err := store.Load(testNamespace) require.NoError(t, err) @@ -501,7 +547,7 @@ func TestKeyringStore_DeleteReportsAPointerFailure(t *testing.T) { err := store.Delete(testNamespace) require.Error(t, err) - assert.Contains(t, err.Error(), "slot") + assert.Contains(t, err.Error(), "token pointer") } func TestKeyringStore_BlankServiceIsRejected(t *testing.T) { @@ -538,6 +584,35 @@ type fakeKeyring struct { deleteErr error // onGet fires once, on the next Get, before the read happens. onGet func(account string) + // onSet fires on every Set, before the write happens, so a test can drive + // a second writer into the middle of one Save. + onSet func(account string) +} + +// clearOnSet disarms the write hook. A hook that drives a nested Save MUST +// call this before doing so: the nested writes re-enter the hook, and guarding +// with sync.Once instead deadlocks, because Once.Do cannot be re-entered. +func (f *fakeKeyring) clearOnSet() { + f.mu.Lock() + defer f.mu.Unlock() + + f.onSet = nil +} + +// clearOnGet disarms the read hook, for the same reason as clearOnSet. +func (f *fakeKeyring) clearOnGet() { + f.mu.Lock() + defer f.mu.Unlock() + + f.onGet = nil +} + +// takeOnSet returns the write hook, if any. +func (f *fakeKeyring) takeOnSet() func(string) { + f.mu.Lock() + defer f.mu.Unlock() + + return f.onSet } // takeOnGet returns the hook, if any. It does NOT clear it: the hook decides @@ -555,6 +630,11 @@ func newFakeKeyring() *fakeKeyring { } func (f *fakeKeyring) Set(service, user, password string) error { + // Outside the lock, like onGet, so a hook can run a whole Save. + if hook := f.takeOnSet(); hook != nil { + hook(user) + } + f.mu.Lock() defer f.mu.Unlock() if f.setErr != nil { @@ -617,13 +697,39 @@ func (f *fakeKeyring) putField(slot, field, value string) { f.entries[testService+"|"+testNamespace+"/"+slot+"/"+field] = value } -// putCommitted seeds a committed session in slotA, field by field, so a test -// can then remove or corrupt exactly one part of it. +// testGeneration is a fixed generation name for seeding, standing in for the +// random one Save would mint. +const testGeneration = "gen0" + +// putCommitted seeds a committed session under testGeneration, field by field, +// so a test can then remove or corrupt exactly one part of it. func (f *fakeKeyring) putCommitted(fields map[string]string) { for field, value := range fields { - f.putField(slotA, field, value) + f.putField(testGeneration, field, value) } - f.put(fieldCurrent, slotA) + f.putPointer(testGeneration, "") +} + +// putPointer seeds the commit record directly. +func (f *fakeKeyring) putPointer(current, previous string) { + raw, err := json.Marshal(keyringPointer{Current: current, Previous: previous}) + if err != nil { + panic(err) + } + f.put(fieldCurrent, string(raw)) +} + +// liveGeneration reads the committed generation name straight out of the fake. +func (f *fakeKeyring) liveGeneration(t *testing.T) string { + t.Helper() + + raw := f.value(fieldCurrent) + require.NotEmpty(t, raw, "no pointer entry was written") + + var pointer keyringPointer + require.NoError(t, json.Unmarshal([]byte(raw), &pointer)) + + return pointer.Current } // value reads an entry under testNamespace, bypassing the store under test. @@ -659,14 +765,13 @@ func TestKeyringStore_LoadSurvivesAConcurrentCommit(t *testing.T) { // Commit the replacement the moment this Load starts reading fields, which // is exactly the window the race lives in. - var once sync.Once backend.onGet = func(account string) { if account == testNamespace+"/"+fieldCurrent { return // the pointer read itself; let it through } - once.Do(func() { - require.NoError(t, store.Save(testNamespace, replacement)) - }) + + backend.clearOnGet() + require.NoError(t, store.Save(testNamespace, replacement)) } got, err := store.Load(testNamespace) @@ -721,3 +826,176 @@ func TestKeyringStore_LoadReportsARealInconsistency(t *testing.T) { assert.Contains(t, err.Error(), "id", "the missing field must be named") assert.NotContains(t, err.Error(), "replaced") } + +// sessionIsWhole asserts a loaded token set came entirely from one session. +// A set pairing one login's access token with another's id and refresh tokens +// is the specific corruption these tests exist to prevent: the id token +// supplies the operator identity that admin writes are audited against, so a +// mismatched pair can attribute a production change to the wrong person. +func sessionIsWhole(t *testing.T, got *cognito.TokenSet, sessions map[string]*cognito.TokenSet) { + t.Helper() + require.NotNil(t, got) + + for name, want := range sessions { + if got.AccessToken != want.AccessToken { + continue + } + + assert.Equal(t, want.IDToken, got.IDToken, "id token must come from the same session as the access token (%s)", name) + assert.Equal(t, want.RefreshToken, got.RefreshToken, "refresh token must come from the same session as the access token (%s)", name) + assert.Equal(t, want.ExpiresAt, got.ExpiresAt, "expiry must come from the same session as the access token (%s)", name) + + return + } + + t.Fatalf("loaded access token %q belongs to no known session", got.AccessToken) +} + +func session(tag string) *cognito.TokenSet { + return &cognito.TokenSet{ + AccessToken: tag + "-access", + IDToken: tag + "-id", + RefreshToken: tag + "-refresh", + ExpiresAt: testExpiry, + } +} + +// TestKeyringStore_ConcurrentSavesNeverCommitAHybrid is the first regression +// named in review round 4. +// +// Two pay-admin processes overlapping a login and a refresh both used to read +// the same live slot, both compute the same "other" slot, and interleave their +// field writes into it. Both then committed, leaving one slot holding one +// session's access token beside another's id and refresh tokens. +func TestKeyringStore_ConcurrentSavesNeverCommitAHybrid(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + + seed, first, second := session("seed"), session("first"), session("second") + require.NoError(t, store.Save(testNamespace, seed)) + + // The second writer runs to completion inside the first writer's very first + // field write, which is the schedule that produced the hybrid. + // Fire once the first writer has already stored its access token but + // before its id token, so the second writer's whole session lands in + // between. Firing earlier lets the first writer simply overwrite + // everything, which is consistent and proves nothing. + backend.onSet = func(account string) { + if !strings.HasSuffix(account, "/"+fieldID) { + return + } + + backend.clearOnSet() + require.NoError(t, store.Save(testNamespace, second)) + } + + require.NoError(t, store.Save(testNamespace, first)) + + backend.onSet = nil + + got, err := store.Load(testNamespace) + require.NoError(t, err) + sessionIsWhole(t, got, map[string]*cognito.TokenSet{"seed": seed, "first": first, "second": second}) +} + +// TestKeyringStore_LoadIsNotFooledByPointerReuse is the second regression named +// in review round 4, the A->B->A schedule. +// +// Load re-reads the pointer after a failed field read and retried only when it +// had changed. With two reusable slots the pointer could cycle back to the one +// Load started on, so a changed generation looked unchanged and Load returned +// fields belonging to a session it never selected. +func TestKeyringStore_LoadIsNotFooledByPointerReuse(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + + seed, first, second := session("seed"), session("first"), session("second") + require.NoError(t, store.Save(testNamespace, seed)) + + // Two commits land after Load has chosen its generation and read part of + // it. Under slot reuse the second commit lands back on the first slot. + // Fire after Load has read the access token but before the rest, so the + // commits land inside one read of one generation. + backend.onGet = func(account string) { + if !strings.HasSuffix(account, "/"+fieldID) { + return + } + + backend.clearOnGet() + require.NoError(t, store.Save(testNamespace, first)) + require.NoError(t, store.Save(testNamespace, second)) + } + + got, err := store.Load(testNamespace) + require.NoError(t, err) + sessionIsWhole(t, got, map[string]*cognito.TokenSet{"seed": seed, "first": first, "second": second}) +} + +// TestKeyringStore_LoadReportsAPointerFailureDuringRetry covers the branch +// where the pointer read that decides retry-or-report itself fails. Reporting +// that beats retrying blindly or claiming the namespace is inconsistent. +func TestKeyringStore_LoadReportsAPointerFailureDuringRetry(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + require.NoError(t, store.Save(testNamespace, sampleTokens())) + + // Break the field read, then break the keychain outright so the retry + // check cannot establish whether the generation moved. + generation := backend.liveGeneration(t) + delete(backend.entries, testService+"|"+testNamespace+"/"+generation+"/"+fieldID) + + backend.onGet = func(account string) { + if !strings.HasSuffix(account, "/"+fieldID) { + return + } + + backend.clearOnGet() + backend.mu.Lock() + backend.getErr = errors.New("keychain is locked") + backend.mu.Unlock() + } + + _, err := store.Load(testNamespace) + require.Error(t, err) + assert.Contains(t, err.Error(), "keychain") +} + +// TestKeyringStore_SaveReportsACommitEncodingFailure is not reachable through +// the public API -- keyringPointer always marshals -- so the pointer encoder is +// exercised directly to prove the record round-trips exactly. +func TestKeyringStore_PointerRoundTrips(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + + require.NoError(t, store.Save(testNamespace, session("one"))) + first := backend.liveGeneration(t) + + require.NoError(t, store.Save(testNamespace, session("two"))) + + pointer, err := store.readPointer(testNamespace) + require.NoError(t, err) + assert.Equal(t, backend.liveGeneration(t), pointer.Current) + assert.Equal(t, first, pointer.Previous, "the record must remember what it replaced, so Delete can reap it") +} + +// TestKeyringStore_SaveReportsAFieldWriteFailure covers the write loop's error +// path with the generation layout, and that a failure leaves the live session +// untouched. +func TestKeyringStore_SaveReportsAFieldWriteFailure(t *testing.T) { + backend := newFakeKeyring() + store := &keyringStore{service: testService, backend: backend} + require.NoError(t, store.Save(testNamespace, session("live"))) + + backend.okWrites = 0 + backend.failSetAfter = 0 + + err := store.Save(testNamespace, session("doomed")) + require.Error(t, err) + assert.Contains(t, err.Error(), "keychain") + + backend.failSetAfter = -1 + + got, err := store.Load(testNamespace) + require.NoError(t, err) + assert.Equal(t, session("live"), got, "a failed write must not disturb the live session") +} From b0738ab2b631871824b6f24838769e3195fe362f Mon Sep 17 00:00:00 2001 From: mysqto Date: Tue, 8 Sep 2026 17:13:15 +0800 Subject: [PATCH 10/12] cognito: refuse endpoints that would expose the credential set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from review round 5, all in the same area: what this package will accept as a place to send or receive OAuth credentials. TokenURL and AuthorizeURL could be cleartext http. The token endpoint receives the authorization code, the PKCE verifier and the client id on sign-in and the refresh token on renewal, so an http:// value handed the whole credential set to anyone on the path. Both now require https, with no loopback exemption: these are Cognito's own endpoints, served over https only, so http is a misconfiguration in every case. A URL embedding userinfo is refused too, since it would be logged and re-sent verbatim. CallbackAddr and RedirectURI could name a routable host, despite this package being loopback-only by design. Binding 0.0.0.0 would let anything that can reach the machine deliver a redirect to the single-use callback, and a routable redirect would send the code across the network to whoever answered. Both are now held to literal loopback — a name that merely resolves to 127.0.0.1 does not count, since resolution can change between the check and the request. Cleartext http stays allowed for the redirect and only there: RFC 8252 has a native app receive it on loopback, where TLS buys nothing but a certificate problem. The callback handler published its result before checking state, so a blind request to the predictable callback port could consume the one delivery the flow gets and abort a real sign-in that had not landed yet. State is now compared before anything is delivered, so an uncorrelated request is dropped and the genuine redirect is still accepted. This is a denial-of-sign-in guard, not the CSRF check — Login still compares state itself, which is what the paste-back path relies on. One consequence worth knowing: on the listener path a tampered state now surfaces as the wait expiring rather than "state mismatch", because it is never delivered. Two tests were rewritten to pin that contract. ReadRedirect ran synchronously, so a cancelled Login could not return while the reader was blocked on stdin. It now runs on a goroutine with Login selecting on ctx.Done(). That does not unblock the read itself and nothing can; the goroutine parks until the reader yields, then sends into a buffered channel and exits. The signature stays func() (string, error) so existing callers keep working. Every rule has a rejection test, plus the invalid-then-valid regression for the callback and a blocked-reader cancellation test. All three fixes are mutation-checked: reverting any one of them fails its own tests. The suite moved to httptest.NewTLSServer to match the new https rule rather than working around it. Verified the payments CLI still passes: it binds 127.0.0.1:8100, redirects to http://localhost:8100/callback, and builds both endpoints as https:///oauth2/*. --- cognito/callback.go | 29 +++- cognito/callback_internal_test.go | 82 +++++++++-- cognito/oauth.go | 169 +++++++++++++++++++++- cognito/oauth_test.go | 225 +++++++++++++++++++++++++++--- 4 files changed, 471 insertions(+), 34 deletions(-) diff --git a/cognito/callback.go b/cognito/callback.go index ae9d29f..74fdc70 100644 --- a/cognito/callback.go +++ b/cognito/callback.go @@ -66,7 +66,7 @@ func callbackPath(redirectURI string) string { // A bind failure is terminal on purpose: the Cognito app client registers // exactly one callback URL, so listening somewhere else would produce a // redirect the authorization server refuses. Fail loudly instead. -func startCallbackServer(addr, path string) (*callbackServer, error) { +func startCallbackServer(addr, path, expectedState string) (*callbackServer, error) { listener, err := net.Listen("tcp", addr) if err != nil { return nil, fmt.Errorf( @@ -76,7 +76,7 @@ func startCallbackServer(addr, path string) (*callbackServer, error) { results := make(chan callbackResult, 1) mux := http.NewServeMux() - mux.HandleFunc(path, callbackHandler(results)) + mux.HandleFunc(path, callbackHandler(expectedState, results)) srv := &http.Server{ Handler: mux, @@ -91,11 +91,34 @@ func startCallbackServer(addr, path string) (*callbackServer, error) { // callbackHandler serves the redirect, renders a minimal page for the // operator, and reports the outcome exactly once. -func callbackHandler(results chan<- callbackResult) http.HandlerFunc { +// +// expectedState is checked BEFORE anything is delivered, and that ordering is +// the point. The callback port is predictable and the result channel is +// single-use, so a handler that published first would let any local process — +// or a page in the operator's own browser — hit http://127.0.0.1:/?code=x +// and consume the one delivery, aborting a real sign-in that had not landed +// yet. Refusing to deliver an uncorrelated request means the flow survives it +// and the genuine redirect is still accepted. +// +// This is a denial-of-sign-in guard, not the CSRF check: Login still compares +// the state it generated against what came back, which is what stops a code +// from a different attempt being exchanged. Both run, and the paste-back path +// relies on the Login-side one alone. +func callbackHandler(expectedState string, results chan<- callbackResult) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { query := r.URL.Query() w.Header().Set("Content-Type", "text/html; charset=utf-8") + // Drop anything that is not this attempt without touching the channel. + // Deliberately indistinguishable from any other failure to the caller: + // the page says nothing, and the terminal is never told, because a + // stray request is not the operator's problem to debug. + if !stateMatches(expectedState, query.Get("state")) { + w.WriteHeader(http.StatusBadRequest) + _, _ = fmt.Fprint(w, failureHTML) + return + } + if authErr := query.Get("error"); wegostrings.IsNotBlank(authErr) { message := authErr if desc := query.Get("error_description"); wegostrings.IsNotBlank(desc) { diff --git a/cognito/callback_internal_test.go b/cognito/callback_internal_test.go index 930c3cf..368f6d6 100644 --- a/cognito/callback_internal_test.go +++ b/cognito/callback_internal_test.go @@ -34,6 +34,8 @@ func TestCallbackPath(t *testing.T) { } func TestCallbackHandler(t *testing.T) { + const wantedState = "the-state" + tests := []struct { name string givenQuery string @@ -41,31 +43,55 @@ func TestCallbackHandler(t *testing.T) { wantCode string wantState string wantErrContains string + wantNoDelivery bool }{ { name: "authorize error is surfaced with its description", - givenQuery: "error=access_denied&error_description=user+said+no", + givenQuery: "error=access_denied&error_description=user+said+no&state=" + wantedState, wantStatus: http.StatusBadRequest, wantErrContains: "user said no", }, { name: "authorize error without a description still reports", - givenQuery: "error=server_error", + givenQuery: "error=server_error&state=" + wantedState, wantStatus: http.StatusBadRequest, wantErrContains: "server_error", }, { name: "missing code is rejected", - givenQuery: "state=abc", + givenQuery: "state=" + wantedState, wantStatus: http.StatusBadRequest, wantErrContains: "no authorization code", }, { name: "code and state are captured", - givenQuery: "code=the-code&state=the-state", + givenQuery: "code=the-code&state=" + wantedState, wantStatus: http.StatusOK, wantCode: "the-code", - wantState: "the-state", + wantState: wantedState, + }, + // The next three are the denial-of-sign-in guard. Each is a request + // that reaches the predictable callback port without belonging to this + // attempt, and the assertion that matters is wantNoDelivery: the + // single-use channel must still be empty afterwards, so the genuine + // redirect can still be accepted. + { + name: "a code carrying someone else's state is not delivered", + givenQuery: "code=blind-code&state=not-this-attempt", + wantStatus: http.StatusBadRequest, + wantNoDelivery: true, + }, + { + name: "a code with no state at all is not delivered", + givenQuery: "code=blind-code", + wantStatus: http.StatusBadRequest, + wantNoDelivery: true, + }, + { + name: "an authorize error from another attempt is not delivered", + givenQuery: "error=access_denied&state=not-this-attempt", + wantStatus: http.StatusBadRequest, + wantNoDelivery: true, }, } @@ -75,11 +101,18 @@ func TestCallbackHandler(t *testing.T) { rec := httptest.NewRecorder() req := httptest.NewRequest(http.MethodGet, "/callback?"+tt.givenQuery, nil) - callbackHandler(ch)(rec, req) + callbackHandler(wantedState, ch)(rec, req) assert.Equal(t, tt.wantStatus, rec.Code) assert.Contains(t, rec.Header().Get("Content-Type"), "text/html") + if tt.wantNoDelivery { + assert.Empty(t, ch, "an uncorrelated request must not consume the single-use delivery") + assert.NotContains(t, rec.Body.String(), "blind-code", + "the page must not reflect anything from the request") + return + } + select { case got := <-ch: if wegostrings.IsNotEmpty(tt.wantErrContains) { @@ -97,9 +130,40 @@ func TestCallbackHandler(t *testing.T) { } } +// TestCallbackHandler_BlindRequestDoesNotAbortTheRealSignIn is the +// invalid-then-valid case. Before the state check moved ahead of delivery, the +// first request here consumed the one delivery the flow gets and the operator's +// actual redirect arrived to a channel that was already full, so a sign-in +// could be aborted by anything able to guess the callback port. +func TestCallbackHandler_BlindRequestDoesNotAbortTheRealSignIn(t *testing.T) { + const wantedState = "real-attempt" + + ch := make(chan callbackResult, 1) + handler := callbackHandler(wantedState, ch) + + blind := httptest.NewRecorder() + handler(blind, httptest.NewRequest(http.MethodGet, "/callback?code=blind&state=guessed", nil)) + + assert.Equal(t, http.StatusBadRequest, blind.Code) + require.Empty(t, ch, "the blind request must leave the delivery unused") + + real := httptest.NewRecorder() + handler(real, httptest.NewRequest(http.MethodGet, "/callback?code=genuine&state="+wantedState, nil)) + + assert.Equal(t, http.StatusOK, real.Code) + select { + case got := <-ch: + require.NoError(t, got.err) + assert.Equal(t, "genuine", got.code, "the real callback must still be the one delivered") + assert.Equal(t, wantedState, got.state) + default: + t.Fatal("the genuine callback was not delivered after a blind request") + } +} + func TestCallbackHandler_IsSingleUse(t *testing.T) { ch := make(chan callbackResult, 1) - handler := callbackHandler(ch) + handler := callbackHandler("s", ch) for range 3 { rec := httptest.NewRecorder() @@ -115,7 +179,7 @@ func TestStartCallbackServer_PortAlreadyBoundFailsFast(t *testing.T) { t.Cleanup(func() { _ = ln.Close() }) addr := ln.Addr().String() - cs, err := startCallbackServer(addr, "/callback") + cs, err := startCallbackServer(addr, "/callback", "s") require.Error(t, err, "binding an occupied port must fail rather than silently pick another") assert.Nil(t, cs) @@ -124,7 +188,7 @@ func TestStartCallbackServer_PortAlreadyBoundFailsFast(t *testing.T) { func TestStartCallbackServer_ServesTheCallback(t *testing.T) { addr := mustFreeAddr(t) - cs, err := startCallbackServer(addr, "/callback") + cs, err := startCallbackServer(addr, "/callback", "xyz") require.NoError(t, err) t.Cleanup(cs.shutdown) diff --git a/cognito/oauth.go b/cognito/oauth.go index f074232..497af09 100644 --- a/cognito/oauth.go +++ b/cognito/oauth.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/url" "strings" @@ -37,6 +38,9 @@ const ( defaultHTTPTimeout = 30 * time.Second maxTokenResponseBytes = 1 << 20 + + schemeHTTP = "http" + schemeHTTPS = "https" ) // Config carries everything the login flow needs. Nothing is read from the @@ -334,7 +338,7 @@ func (c Config) validateClient() error { if wegostrings.IsBlank(c.TokenURL) { return errors.New("cognito token url is not configured") } - return nil + return requireHTTPS("cognito token url", c.TokenURL) } // validateForLogin checks everything the interactive flow additionally needs. @@ -345,13 +349,24 @@ func (c Config) validateForLogin() error { if wegostrings.IsBlank(c.AuthorizeURL) { return errors.New("cognito authorize url is not configured") } + if err := requireHTTPS("cognito authorize url", c.AuthorizeURL); err != nil { + return err + } if wegostrings.IsBlank(c.RedirectURI) { return errors.New("cognito redirect uri is not configured") } + if err := requireLoopbackRedirect(c.RedirectURI); err != nil { + return err + } // Only the listener path needs a port; paste-back binds nothing. if c.ReadRedirect == nil && wegostrings.IsBlank(c.CallbackAddr) { return errors.New("local callback address is not configured") } + if c.ReadRedirect == nil { + if err := requireLoopbackAddr(c.CallbackAddr); err != nil { + return err + } + } if c.ReadRedirect != nil && c.PromptURL == nil { return errors.New("paste-back sign-in needs Config.PromptURL: the operator has to be shown the url they are meant to open") } @@ -442,9 +457,9 @@ func (c Config) collectCode(ctx context.Context, state, challenge string) (code, return "", "", err } - pasted, err := c.ReadRedirect() + pasted, err := c.readRedirect(ctx) if err != nil { - return "", "", fmt.Errorf("read the pasted redirect: %w", err) + return "", "", err } return codeFromRedirect(pasted) @@ -453,7 +468,7 @@ func (c Config) collectCode(ctx context.Context, state, challenge string) (code, // Bind the callback port BEFORE sending the operator to Cognito. If the // port is unavailable the redirect could never land, and there is no // fallback port to try, so failing here saves a pointless round trip. - server, err := startCallbackServer(c.CallbackAddr, callbackPath(c.RedirectURI)) + server, err := startCallbackServer(c.CallbackAddr, callbackPath(c.RedirectURI), state) if err != nil { return "", "", err } @@ -519,3 +534,149 @@ func (c Config) openBrowserAt(rawURL string) error { } return openBrowser(rawURL) } + +// ErrInsecureEndpoint is returned for a configured URL or bind address that +// must not carry OAuth credentials. +var ErrInsecureEndpoint = errors.New("insecure cognito endpoint") + +// requireHTTPS rejects a Cognito endpoint that is not https. +// +// Both endpoints carry credentials in the clear if the transport does not +// protect them: the authorize URL puts the PKCE challenge and state on the +// wire, and the token endpoint receives the authorization code, the PKCE +// verifier and the client id on sign-in and the refresh token on renewal. A +// cleartext http:// value hands all of that to anyone on the path, so it is +// refused rather than warned about. +// +// There is no loopback exemption here, unlike the redirect: these are Cognito's +// own endpoints on a domain the operator configured, and Cognito serves them +// over https only. An http:// value is a misconfiguration in every case. +func requireHTTPS(name, raw string) error { + parsed, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("%w: %s %q cannot be parsed: %w", ErrInsecureEndpoint, name, raw, err) + } + if parsed.Scheme != schemeHTTPS { + return fmt.Errorf( + "%w: %s must use https, got %q (the sign-in exchange carries the authorization code, the pkce verifier and the refresh token, so cleartext would expose the whole credential set)", + ErrInsecureEndpoint, name, raw) + } + if wegostrings.IsBlank(parsed.Host) { + return fmt.Errorf("%w: %s %q has no host", ErrInsecureEndpoint, name, raw) + } + if parsed.User != nil { + return fmt.Errorf( + "%w: %s %q embeds credentials in the url, which would be logged and sent on every request", + ErrInsecureEndpoint, name, raw) + } + return nil +} + +// requireLoopbackRedirect rejects a redirect URI that is not a loopback +// address. +// +// Cleartext http IS allowed here, and only here: RFC 8252 §7.3 has a native app +// receive the redirect on a loopback interface, where the request never leaves +// the machine and TLS would buy nothing but a certificate problem. What must +// hold is that the host is loopback — a redirect to a routable host would send +// the authorization code across the network to whoever answers it, which is the +// exact exposure this package's loopback-only design exists to prevent. +func requireLoopbackRedirect(raw string) error { + parsed, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("%w: cognito redirect uri %q cannot be parsed: %w", ErrInsecureEndpoint, raw, err) + } + switch parsed.Scheme { + case schemeHTTP, schemeHTTPS: + default: + return fmt.Errorf( + "%w: cognito redirect uri %q must be http or https, got scheme %q", + ErrInsecureEndpoint, raw, parsed.Scheme) + } + if !isLoopbackHost(parsed.Hostname()) { + return fmt.Errorf( + "%w: cognito redirect uri %q must be a loopback address (127.0.0.1, [::1] or localhost) — a routable host would receive the authorization code over the network", + ErrInsecureEndpoint, raw) + } + return nil +} + +// requireLoopbackAddr rejects a callback bind address that is not loopback. +// +// Binding 0.0.0.0 or a LAN interface would let anything that can reach the +// machine deliver a redirect to the single-use callback, so the listener is +// held to the same loopback rule as the redirect it serves. +func requireLoopbackAddr(addr string) error { + host, port, err := net.SplitHostPort(addr) + if err != nil { + return fmt.Errorf( + "%w: local callback address %q must be host:port, e.g. 127.0.0.1:8110: %w", + ErrInsecureEndpoint, addr, err) + } + if wegostrings.IsBlank(port) { + return fmt.Errorf("%w: local callback address %q has no port", ErrInsecureEndpoint, addr) + } + if !isLoopbackHost(host) { + return fmt.Errorf( + "%w: local callback address %q must bind a loopback interface (127.0.0.1 or [::1]) — binding 0.0.0.0 or a routable interface would let any host that can reach this machine deliver the sign-in callback", + ErrInsecureEndpoint, addr) + } + return nil +} + +// isLoopbackHost reports whether host is LITERALLY loopback. +// +// Only a literal loopback IP counts, plus the name "localhost". A name that +// merely resolves to 127.0.0.1 does not: resolution can change between this +// check and the request, so trusting it would make the check decorative. Go +// itself treats "localhost" as loopback (net/http.isLocalhost, and the URL +// spec's special-casing), and a poisoned "localhost" costs the attacker nothing +// they do not already have on a machine they can edit /etc/hosts on. +func isLoopbackHost(host string) bool { + if strings.EqualFold(host, "localhost") { + return true + } + ip := net.ParseIP(strings.Trim(host, "[]")) + return ip != nil && ip.IsLoopback() +} + +// readRedirect runs ReadRedirect without letting it pin Login to a blocked +// reader. +// +// ReadRedirect is synchronous and typically reads a line from stdin, which +// ignores context: os.Stdin.Read stays blocked until the operator types +// something or the descriptor closes. Called directly, that made a cancelled +// or timed-out Login unable to return — the caller asked to stop and the +// process sat there anyway, which is the whole defect. +// +// Running it on a goroutine and selecting on ctx.Done() means Login returns +// when the caller says so. It does NOT unblock the read itself, and nothing +// here can: the goroutine survives until the reader yields, then sends into a +// buffered channel and exits, so it parks rather than leaks and never blocks on +// a receiver that has gone. A caller that needs the read torn down too should +// close its own reader on cancellation — the signature stays +// func() (string, error) so existing callers keep working. +func (c Config) readRedirect(ctx context.Context) (string, error) { + type pasteResult struct { + url string + err error + } + // Buffered: the goroutine must be able to finish and exit after Login has + // already returned on ctx.Done() and stopped receiving. + done := make(chan pasteResult, 1) + + go func() { + url, err := c.ReadRedirect() + done <- pasteResult{url: url, err: err} + }() + + select { + case <-ctx.Done(): + return "", fmt.Errorf("waiting for the pasted redirect: %w", ctx.Err()) + case result := <-done: + if result.err != nil { + return "", fmt.Errorf("read the pasted redirect: %w", result.err) + } + return result.url, nil + } +} diff --git a/cognito/oauth_test.go b/cognito/oauth_test.go index efbcc65..4bb01b6 100644 --- a/cognito/oauth_test.go +++ b/cognito/oauth_test.go @@ -31,6 +31,7 @@ const ( func TestLogin(t *testing.T) { tests := []struct { name string + givenTimeout time.Duration givenConfig func(*cognito.Config) givenBrowser func(*fakeBrowser) givenHandler func(*testing.T) http.HandlerFunc @@ -73,19 +74,31 @@ func TestLogin(t *testing.T) { givenBrowser: func(f *fakeBrowser) { f.openErr = errors.New("no browser here") }, wantErrContains: "open browser", }, + // On the listener path a callback whose state is not this attempt's is + // now DROPPED BY THE HANDLER rather than delivered and then rejected by + // Login, so the observable failure is the wait timing out instead of a + // "state mismatch". That is the point: delivering it would consume the + // single-use result and let a blind request to the predictable callback + // port abort a real sign-in. See + // TestCallbackHandler_BlindRequestDoesNotAbortTheRealSignIn for the + // invalid-then-valid proof, and TestLogin_PasteBackRejectsAForeignState + // for the paste-back path, where Login's own state check is still the + // only guard and still reports a mismatch. { - name: "tampered state is rejected", + name: "a tampered state never completes the sign-in", + givenTimeout: 500 * time.Millisecond, givenBrowser: func(f *fakeBrowser) { f.tamper = func(q url.Values) { q.Set("state", "tampered-state") } }, - wantErrContains: "state mismatch", + wantErrContains: "waiting for the sign-in callback", }, { - name: "missing state is rejected", + name: "a callback with no state never completes the sign-in", + givenTimeout: 500 * time.Millisecond, givenBrowser: func(f *fakeBrowser) { f.tamper = func(q url.Values) { q.Del("state") } }, - wantErrContains: "state mismatch", + wantErrContains: "waiting for the sign-in callback", }, { name: "authorize server error is surfaced", @@ -197,13 +210,17 @@ func TestLogin(t *testing.T) { tt.givenBrowser(browser) } - cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg := tlsConfig(t, ts, mustFreeAddr(t)) cfg.OpenBrowser = browser.open if tt.givenConfig != nil { tt.givenConfig(&cfg) } - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + timeout := 10 * time.Second + if tt.givenTimeout > 0 { + timeout = tt.givenTimeout + } + ctx, cancel := context.WithTimeout(context.Background(), timeout) defer cancel() if tt.givenCancel { cancel() @@ -235,7 +252,7 @@ func TestLogin_BindsTheCallbackPortBeforeOpeningTheBrowser(t *testing.T) { ts := newTokenServer(t, nil) browser := newFakeBrowser() - cfg := baseConfig(t, ts.URL, addr) + cfg := tlsConfig(t, ts, addr) cfg.OpenBrowser = browser.open got, err := cognito.Login(context.Background(), cfg) @@ -249,7 +266,7 @@ func TestLogin_BindsTheCallbackPortBeforeOpeningTheBrowser(t *testing.T) { func TestLogin_SendsPKCEAndState(t *testing.T) { ts := newTokenServer(t, nil) browser := newFakeBrowser() - cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg := tlsConfig(t, ts, mustFreeAddr(t)) cfg.OpenBrowser = browser.open got, err := cognito.Login(context.Background(), cfg) @@ -290,7 +307,7 @@ func TestLogin_SendsPKCEAndState(t *testing.T) { func TestLogin_ErrorsDoNotLeakVerifierOrState(t *testing.T) { ts := newTokenServer(t, jsonHandler(http.StatusBadRequest, map[string]any{"error": "invalid_grant"})) browser := newFakeBrowser() - cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg := tlsConfig(t, ts, mustFreeAddr(t)) cfg.OpenBrowser = browser.open _, err := cognito.Login(context.Background(), cfg) @@ -310,7 +327,7 @@ func TestLogin_ErrorsDoNotLeakVerifierOrState(t *testing.T) { func TestLogin_FallsBackToTheSystemClockWhenNowIsNil(t *testing.T) { ts := newTokenServer(t, nil) browser := newFakeBrowser() - cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg := tlsConfig(t, ts, mustFreeAddr(t)) cfg.OpenBrowser = browser.open cfg.Now = nil @@ -324,7 +341,7 @@ func TestLogin_FallsBackToTheSystemClockWhenNowIsNil(t *testing.T) { func TestLogin_AllowsAnyEmailWhenNoDomainIsConfigured(t *testing.T) { ts := newTokenServer(t, jsonHandler(http.StatusOK, goodTokenBody(t, "contractor@example.test"))) browser := newFakeBrowser() - cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg := tlsConfig(t, ts, mustFreeAddr(t)) cfg.OpenBrowser = browser.open cfg.AllowedDomain = "" @@ -426,7 +443,7 @@ func TestRefresh(t *testing.T) { } ts := newTokenServer(t, handler) - cfg := baseConfig(t, ts.URL, "127.0.0.1:0") + cfg := tlsConfig(t, ts, "127.0.0.1:0") if tt.givenConfig != nil { tt.givenConfig(&cfg) } @@ -455,7 +472,7 @@ func TestRefresh_PreservesTheOriginalRefreshToken(t *testing.T) { delete(body, "refresh_token") ts := newTokenServer(t, jsonHandler(http.StatusOK, body)) - cfg := baseConfig(t, ts.URL, "127.0.0.1:0") + cfg := tlsConfig(t, ts, "127.0.0.1:0") got, err := cognito.Refresh(context.Background(), cfg, "the-long-lived-refresh-token") require.NoError(t, err) @@ -469,7 +486,7 @@ func TestRefresh_PreservesTheOriginalRefreshToken(t *testing.T) { // is a login-time UX affordance, not something to re-run on every refresh. func TestRefresh_DoesNotApplyTheDomainGate(t *testing.T) { ts := newTokenServer(t, jsonHandler(http.StatusOK, goodTokenBody(t, "someone@gmail.com"))) - cfg := baseConfig(t, ts.URL, "127.0.0.1:0") + cfg := tlsConfig(t, ts, "127.0.0.1:0") got, err := cognito.Refresh(context.Background(), cfg, testRefreshValue) require.NoError(t, err) @@ -478,7 +495,7 @@ func TestRefresh_DoesNotApplyTheDomainGate(t *testing.T) { func TestRefresh_HonoursContextCancellation(t *testing.T) { ts := newTokenServer(t, nil) - cfg := baseConfig(t, ts.URL, "127.0.0.1:0") + cfg := tlsConfig(t, ts, "127.0.0.1:0") ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -503,6 +520,16 @@ func baseConfig(t *testing.T, tokenURL, callbackAddr string) cognito.Config { } } +// tlsConfig is baseConfig wired to trust ts's self-signed certificate. Config +// copies the client and forces CheckRedirect, but keeps the Transport, so the +// test root travels with it. +func tlsConfig(t *testing.T, ts *tokenServer, callbackAddr string) cognito.Config { + t.Helper() + cfg := baseConfig(t, ts.URL, callbackAddr) + cfg.HTTPClient = ts.Client() + return cfg +} + // goodTokenBody is a well-formed Cognito token response. func goodTokenBody(t *testing.T, email string) map[string]any { t.Helper() @@ -552,7 +579,10 @@ func newTokenServer(t *testing.T, handler http.HandlerFunc) *tokenServer { handler = jsonHandler(http.StatusOK, goodTokenBody(t, testOperatorEmail)) } - ts.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // NewTLSServer, not NewServer: Config.validateClient requires https for the + // token endpoint, so a plaintext test server would exercise the rejection + // path instead of the flow. ts.Client() below trusts this server's cert. + ts.Server = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if err := r.ParseForm(); err != nil { http.Error(w, "bad form", http.StatusBadRequest) return @@ -663,7 +693,7 @@ func TestLogin_NoBrowser(t *testing.T) { // callback, which is exactly what an operator pasting the URL would cause. prompt := newFakeBrowser() - cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg := tlsConfig(t, ts, mustFreeAddr(t)) cfg.NoBrowser = true cfg.PromptURL = prompt.open cfg.OpenBrowser = func(string) error { @@ -753,7 +783,7 @@ func TestLogin_RejectsAnUnusableExpiresIn(t *testing.T) { ts := newTokenServer(t, jsonHandler(http.StatusOK, body)) browser := newFakeBrowser() - cfg := baseConfig(t, ts.URL, mustFreeAddr(t)) + cfg := tlsConfig(t, ts, mustFreeAddr(t)) cfg.OpenBrowser = browser.open _, err := cognito.Login(context.Background(), cfg) @@ -775,6 +805,7 @@ func TestLogin_PasteBack(t *testing.T) { cfg := cognito.Config{ AuthorizeURL: "https://cognito.test/oauth2/authorize", TokenURL: ts.URL, + HTTPClient: ts.Client(), ClientID: "test-client-id", RedirectURI: "http://localhost:8100/callback", Scopes: "openid email", @@ -809,6 +840,7 @@ func TestLogin_PasteBackRejectsAForeignState(t *testing.T) { cfg := cognito.Config{ AuthorizeURL: "https://cognito.test/oauth2/authorize", TokenURL: ts.URL, + HTTPClient: ts.Client(), ClientID: "test-client-id", RedirectURI: "http://localhost:8100/callback", AllowedDomain: "@wego.com", @@ -855,3 +887,160 @@ func stateOf(t *testing.T, authorizeURL string) string { return state } + +// TestLogin_RejectsInsecureEndpoints covers the transport rules. The sign-in +// exchange carries the authorization code, the PKCE verifier and the refresh +// token, so the two Cognito endpoints must be https and the two local ones must +// be loopback — a routable redirect or bind would put a single-use code on the +// network for whoever answers. +// +// Every case must fail BEFORE any request is made, which is why none of these +// configs point at a live server. +func TestLogin_RejectsInsecureEndpoints(t *testing.T) { + tests := []struct { + name string + givenConfig func(*cognito.Config) + wantErrContains string + }{ + { + name: "cleartext token url", + givenConfig: func(c *cognito.Config) { c.TokenURL = "http://cognito.test/oauth2/token" }, + wantErrContains: "token url must use https", + }, + { + name: "cleartext token url on loopback is refused too", + givenConfig: func(c *cognito.Config) { c.TokenURL = "http://127.0.0.1:9000/oauth2/token" }, + wantErrContains: "token url must use https", + }, + { + name: "token url embedding credentials", + givenConfig: func(c *cognito.Config) { c.TokenURL = "https://user:pw@cognito.test/oauth2/token" }, + wantErrContains: "embeds credentials in the url", + }, + { + name: "cleartext authorize url", + givenConfig: func(c *cognito.Config) { c.AuthorizeURL = "http://cognito.test/oauth2/authorize" }, + wantErrContains: "authorize url must use https", + }, + { + name: "routable redirect uri", + givenConfig: func(c *cognito.Config) { c.RedirectURI = "http://attacker.test/callback" }, + wantErrContains: "redirect uri", + }, + { + name: "non-http redirect scheme", + givenConfig: func(c *cognito.Config) { c.RedirectURI = "ftp://127.0.0.1/callback" }, + wantErrContains: "must be http or https", + }, + { + name: "wildcard callback bind", + givenConfig: func(c *cognito.Config) { c.CallbackAddr = "0.0.0.0:8110" }, + wantErrContains: "must bind a loopback interface", + }, + { + name: "routable callback bind", + givenConfig: func(c *cognito.Config) { c.CallbackAddr = "192.168.1.20:8110" }, + wantErrContains: "must bind a loopback interface", + }, + { + name: "callback bind without a port", + givenConfig: func(c *cognito.Config) { c.CallbackAddr = "127.0.0.1" }, + wantErrContains: "must be host:port", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := baseConfig(t, "https://cognito.test/oauth2/token", "127.0.0.1:8110") + cfg.OpenBrowser = func(string) error { + t.Fatal("an insecure endpoint must be rejected before a browser is opened") + return nil + } + tt.givenConfig(&cfg) + + got, err := cognito.Login(context.Background(), cfg) + + require.Error(t, err) + assert.ErrorIs(t, err, cognito.ErrInsecureEndpoint, + "callers need to distinguish a refused endpoint from a failed sign-in") + assert.Contains(t, err.Error(), tt.wantErrContains) + assert.Nil(t, got) + }) + } +} + +// TestLogin_AcceptsLoopbackHTTPRedirect pins the one cleartext exemption: +// RFC 8252 has a native app receive the redirect on loopback, where the request +// never leaves the machine, so requiring TLS there would buy nothing but a +// certificate problem. +func TestLogin_AcceptsLoopbackHTTPRedirect(t *testing.T) { + for _, host := range []string{"127.0.0.1", "localhost", "[::1]"} { + t.Run(host, func(t *testing.T) { + cfg := baseConfig(t, "https://cognito.test/oauth2/token", "127.0.0.1:8110") + cfg.RedirectURI = "http://" + host + ":8110/callback" + cfg.OpenBrowser = func(string) error { return errors.New("stop here") } + + _, err := cognito.Login(context.Background(), cfg) + + require.Error(t, err, "the fake browser stops the flow") + assert.NotErrorIs(t, err, cognito.ErrInsecureEndpoint, + "a loopback http redirect is the documented native-app callback and must be accepted") + }) + } +} + +// TestRefresh_RejectsInsecureTokenURL covers the renewal path, which sends the +// refresh token rather than the code and so needs the same transport rule. +func TestRefresh_RejectsInsecureTokenURL(t *testing.T) { + cfg := baseConfig(t, "http://cognito.test/oauth2/token", "127.0.0.1:8110") + + got, err := cognito.Refresh(context.Background(), cfg, testRefreshValue) + + require.Error(t, err) + assert.ErrorIs(t, err, cognito.ErrInsecureEndpoint) + assert.Nil(t, got) +} + +// TestLogin_PasteBackIsCancellable pins that cancelling Login returns even +// though the paste-back reader is still blocked. +// +// ReadRedirect is synchronous and normally reads stdin, which ignores context: +// called directly, a cancelled Login could not return until the operator typed +// something, so the caller asked to stop and the process sat there. Login now +// waits on ctx alongside the read. +func TestLogin_PasteBackIsCancellable(t *testing.T) { + reading := make(chan struct{}) + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + + // Paste-back binds no port, but RedirectURI still has to be a valid loopback + // URL — it is what Cognito redirects the operator's browser to. + cfg := baseConfig(t, "https://cognito.test/oauth2/token", "127.0.0.1:8110") + cfg.CallbackAddr = "" + cfg.PromptURL = func(string) error { return nil } + cfg.ReadRedirect = func() (string, error) { + close(reading) + <-release // a reader that cannot be interrupted, like os.Stdin.Read + return "", errors.New("never reached in this test") + } + + ctx, cancel := context.WithCancel(context.Background()) + + done := make(chan error, 1) + go func() { + _, err := cognito.Login(ctx, cfg) + done <- err + }() + + <-reading // the read is now blocked + cancel() + + select { + case err := <-done: + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) + assert.Contains(t, err.Error(), "waiting for the pasted redirect") + case <-time.After(5 * time.Second): + t.Fatal("Login did not return after cancellation while the paste-back reader was blocked") + } +} From 73e14e68ac9b2602e19d6ffd31fd2ec87ae1d366 Mon Sep 17 00:00:00 2001 From: mysqto Date: Tue, 8 Sep 2026 17:57:23 +0800 Subject: [PATCH 11/12] cognito: make the paste-back read cancellable and state its contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 6, and the reviewer named a hazard the previous fix left open: a cancelled Login returned, but ReadRedirect could still be active on a caller-owned reader, so a retry raced the abandoned attempt for shared stdin. Whichever won, one of them saw a truncated or empty read. The review offered ctx-awareness OR a documented contract. Taking both, because they cover different halves and neither is sufficient alone. The signature is now func(ctx context.Context) (string, error), so a reader that can select on ctx is able to abandon the read instead of parking on the descriptor. Login still selects on ctx.Done() as well, because a reader that ignores ctx must not be able to pin Login — the ctx argument is what lets the read end, the select is what bounds Login. Neither can interrupt a read already blocked inside an uncooperative reader, and nothing in this package can, so the two obligations that genuinely fall to the caller are now written on the exported field: tear your own reader down on cancellation if you need the read to stop, and do not start another Login while a previous invocation may still be blocked in there. Breaking change to an exported field, taken deliberately now: this is the last moment before cognito/v0.1.0, after which it would not be free. The payments caller is updated in the same review round. TestLogin_PasteBackReaderSeesCancellation covers the new half — a cooperative reader observes Login's ctx being cancelled — alongside the existing TestLogin_PasteBackIsCancellable for the uncooperative case. Mutation-checked: passing context.Background() to the callback instead of ctx fails it. Race clean. --- cognito/oauth.go | 52 ++++++++++++++++++++++++++-------- cognito/oauth_internal_test.go | 6 ++-- cognito/oauth_test.go | 47 +++++++++++++++++++++++++++--- 3 files changed, 86 insertions(+), 19 deletions(-) diff --git a/cognito/oauth.go b/cognito/oauth.go index 497af09..9cbc3b8 100644 --- a/cognito/oauth.go +++ b/cognito/oauth.go @@ -112,10 +112,36 @@ type Config struct { // grant with PKCE, only with the redirect carried by hand. // // Setting it implies NoBrowser: no browser is launched here. PromptURL is - // required alongside it; CallbackAddr is not, since nothing binds a port. The pasted URL carries a single-use authorization - // code, so it should not travel through a shared channel; state is still - // checked, so a code from a different attempt is rejected. - ReadRedirect func() (redirectedURL string, err error) + // required alongside it; CallbackAddr is not, since nothing binds a port. + // The pasted URL carries a single-use authorization code, so it should not + // travel through a shared channel; state is still checked, so a code from a + // different attempt is rejected. + // + // LIFETIME CONTRACT. Read this before wiring a reader to it. + // + // The ctx is the one passed to Login, and honouring it is the caller's + // responsibility. Login itself stops waiting when ctx is done, so a + // cancelled or timed-out Login always returns — but it CANNOT interrupt a + // read already blocked in this function. A reader that ignores ctx (a bare + // os.Stdin.Read, or bufio.Reader over it) therefore stays blocked after + // Login has returned, holding whatever it reads from. + // + // Two obligations follow: + // + // - On cancellation, tear the reader down yourself if you need the read to + // stop — close the file or pipe, or use a reader that selects on ctx. + // Nothing in this package can do it for you. + // - Do NOT start another Login while a previous invocation may still be + // blocked in here. Two live invocations reading the same stdin race for + // the operator's next line: either may win, so a retry can consume the + // paste meant for the other and the losing attempt sees a truncated or + // empty read. Wait for the previous reader to yield, or give the retry + // its own reader. + // + // The abandoned invocation is otherwise harmless: Login sends its result + // into a buffered channel and exits, so it parks rather than leaks and never + // blocks on a receiver that has gone. + ReadRedirect func(ctx context.Context) (redirectedURL string, err error) // HTTPClient calls the token endpoint. Nil uses a client with a timeout. HTTPClient *http.Client @@ -649,13 +675,15 @@ func isLoopbackHost(host string) bool { // or timed-out Login unable to return — the caller asked to stop and the // process sat there anyway, which is the whole defect. // -// Running it on a goroutine and selecting on ctx.Done() means Login returns -// when the caller says so. It does NOT unblock the read itself, and nothing -// here can: the goroutine survives until the reader yields, then sends into a -// buffered channel and exits, so it parks rather than leaks and never blocks on -// a receiver that has gone. A caller that needs the read torn down too should -// close its own reader on cancellation — the signature stays -// func() (string, error) so existing callers keep working. +// ctx is handed to the callback so a reader that can honour it will, and Login +// additionally selects on ctx.Done() so it returns even when the reader cannot. +// Those are different guarantees and both are needed: the select bounds Login, +// the ctx argument is what lets the read itself be abandoned. Neither can +// interrupt a read already blocked inside an uncooperative reader — the +// goroutine survives until it yields, then sends into a buffered channel and +// exits, so it parks rather than leaks and never blocks on a receiver that has +// gone. Config.ReadRedirect documents the teardown and no-concurrent-retry +// obligations that fall to the caller as a result. func (c Config) readRedirect(ctx context.Context) (string, error) { type pasteResult struct { url string @@ -666,7 +694,7 @@ func (c Config) readRedirect(ctx context.Context) (string, error) { done := make(chan pasteResult, 1) go func() { - url, err := c.ReadRedirect() + url, err := c.ReadRedirect(ctx) done <- pasteResult{url: url, err: err} }() diff --git a/cognito/oauth_internal_test.go b/cognito/oauth_internal_test.go index afd558e..b5815d1 100644 --- a/cognito/oauth_internal_test.go +++ b/cognito/oauth_internal_test.go @@ -331,14 +331,14 @@ func TestValidateForLogin_PasteBack(t *testing.T) { t.Run("paste-back without PromptURL is refused", func(t *testing.T) { cfg := base() - cfg.ReadRedirect = func() (string, error) { return "", nil } + cfg.ReadRedirect = func(context.Context) (string, error) { return "", nil } require.ErrorContains(t, cfg.validateForLogin(), "PromptURL") }) t.Run("paste-back needs no callback address", func(t *testing.T) { cfg := base() - cfg.ReadRedirect = func() (string, error) { return "", nil } + cfg.ReadRedirect = func(context.Context) (string, error) { return "", nil } cfg.PromptURL = func(string) error { return nil } require.NoError(t, cfg.validateForLogin(), "nothing binds a port on this path") @@ -358,7 +358,7 @@ func TestPresentAuthorizeURL_PasteBackImpliesThePrompt(t *testing.T) { cfg := Config{ PromptURL: func(u string) error { prompted = u; return nil }, OpenBrowser: func(u string) error { opened = u; return nil }, - ReadRedirect: func() (string, error) { return "", nil }, + ReadRedirect: func(context.Context) (string, error) { return "", nil }, } require.NoError(t, cfg.presentAuthorizeURL("https://cognito.test/authorize?state=s")) diff --git a/cognito/oauth_test.go b/cognito/oauth_test.go index 4bb01b6..a0a6002 100644 --- a/cognito/oauth_test.go +++ b/cognito/oauth_test.go @@ -815,7 +815,7 @@ func TestLogin_PasteBack(t *testing.T) { } // CallbackAddr is deliberately left empty: nothing binds a port here, and // that is the whole reason this path exists. - cfg.ReadRedirect = func() (string, error) { + cfg.ReadRedirect = func(context.Context) (string, error) { return "http://localhost:8100/callback?code=pasted-code&state=" + stateOf(t, shown), nil } @@ -846,7 +846,7 @@ func TestLogin_PasteBackRejectsAForeignState(t *testing.T) { AllowedDomain: "@wego.com", Now: func() time.Time { return fixedNow }, PromptURL: func(string) error { return nil }, - ReadRedirect: func() (string, error) { + ReadRedirect: func(context.Context) (string, error) { return "http://localhost:8100/callback?code=pasted-code&state=someone-elses-state", nil }, } @@ -867,7 +867,7 @@ func TestLogin_PasteBackReportsAReadFailure(t *testing.T) { RedirectURI: "http://localhost:8100/callback", AllowedDomain: "@wego.com", PromptURL: func(string) error { return nil }, - ReadRedirect: func() (string, error) { return "", errors.New("stdin closed") }, + ReadRedirect: func(context.Context) (string, error) { return "", errors.New("stdin closed") }, } _, err := cognito.Login(context.Background(), cfg) @@ -1018,7 +1018,7 @@ func TestLogin_PasteBackIsCancellable(t *testing.T) { cfg := baseConfig(t, "https://cognito.test/oauth2/token", "127.0.0.1:8110") cfg.CallbackAddr = "" cfg.PromptURL = func(string) error { return nil } - cfg.ReadRedirect = func() (string, error) { + cfg.ReadRedirect = func(context.Context) (string, error) { close(reading) <-release // a reader that cannot be interrupted, like os.Stdin.Read return "", errors.New("never reached in this test") @@ -1044,3 +1044,42 @@ func TestLogin_PasteBackIsCancellable(t *testing.T) { t.Fatal("Login did not return after cancellation while the paste-back reader was blocked") } } + +// TestLogin_PasteBackReaderSeesCancellation pins the other half of the +// cancellation contract. TestLogin_PasteBackIsCancellable proves Login returns +// when the reader CANNOT honour ctx; this proves a reader that CAN honour it is +// given what it needs to — the ctx reaching the callback is the only way a +// caller can abandon the read itself rather than leaving it parked on stdin. +func TestLogin_PasteBackReaderSeesCancellation(t *testing.T) { + cfg := baseConfig(t, "https://cognito.test/oauth2/token", "127.0.0.1:8110") + cfg.CallbackAddr = "" + cfg.PromptURL = func(string) error { return nil } + + readerReturned := make(chan error, 1) + cfg.ReadRedirect = func(ctx context.Context) (string, error) { + // A cooperative reader: selects rather than blocking on the fd. + <-ctx.Done() + err := ctx.Err() + readerReturned <- err + return "", err + } + + ctx, cancel := context.WithCancel(context.Background()) + go func() { + time.Sleep(50 * time.Millisecond) + cancel() + }() + + _, err := cognito.Login(ctx, cfg) + + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled) + + select { + case readerErr := <-readerReturned: + assert.ErrorIs(t, readerErr, context.Canceled, + "the callback must receive Login's ctx, not a detached one") + case <-time.After(2 * time.Second): + t.Fatal("the reader never observed cancellation, so ctx did not reach the callback") + } +} From 7d4626016e326d6383b4d6535d717d78756bfb37 Mon Sep 17 00:00:00 2001 From: mysqto Date: Tue, 8 Sep 2026 18:09:41 +0800 Subject: [PATCH 12/12] cognito/storage: name the recovery for an unreadable pointer Found by running the CLI rather than the tests. A keychain carrying a pointer this build cannot parse made every command fail with parse the token pointer for "pay-admin/staging": invalid character 'a' looking for beginning of value which tells an operator nothing they can act on. The recovery exists and is always the same - Save rewrites the whole namespace, so signing out and back in fixes any unreadable pointer - but it is not guessable from a json error, so both pointer errors now say it. Still reported rather than silently repaired: quietly discarding a session store is not readPointer's decision, and a pointer that cannot be read may be the visible symptom of something worth knowing about. The specific value that surfaced this was "a", a bare slot name from the two-reusable-slot layout that generations replaced. That shape never shipped - v0.1.0 is the first release - so no released version can produce it and there is deliberately NO migration code for it; only pre-release builds on a developer's machine can have written one. It is covered as a test case because a keychain holding one still has to fail readably. Mutation-checked: removing the recovery sentence fails both cases. --- cognito/storage/keyring.go | 17 ++++++++++++++--- cognito/storage/keyring_internal_test.go | 8 ++++++++ 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/cognito/storage/keyring.go b/cognito/storage/keyring.go index 936cc34..aeb0317 100644 --- a/cognito/storage/keyring.go +++ b/cognito/storage/keyring.go @@ -182,12 +182,23 @@ func (k *keyringStore) readPointer(namespace string) (keyringPointer, error) { var pointer keyringPointer if err := json.Unmarshal([]byte(raw), &pointer); err != nil { // Nothing here writes anything but this record, so the entry was - // tampered with or written by a version that stored something else. - return keyringPointer{}, fmt.Errorf("parse the token pointer for %q: %w", namespace, err) + // tampered with, truncated, restored from a backup taken mid-write, or + // written by a build that stored a different shape. + // + // The recovery is always the same and the operator cannot guess it from + // a json error, so name it: Delete rewrites the whole namespace, so + // signing out and back in fixes any unreadable pointer. Reported rather + // than done here, because silently discarding a session store is not + // this function's call to make. + return keyringPointer{}, fmt.Errorf( + "parse the token pointer for %q: %w (the stored session is unreadable - sign out of this environment and sign in again to rewrite it)", + namespace, err) } if wegostrings.IsBlank(pointer.Current) { - return keyringPointer{}, fmt.Errorf("the token pointer for %q names no generation", namespace) + return keyringPointer{}, fmt.Errorf( + "the token pointer for %q names no generation (the stored session is unreadable - sign out of this environment and sign in again to rewrite it)", + namespace) } return pointer, nil diff --git a/cognito/storage/keyring_internal_test.go b/cognito/storage/keyring_internal_test.go index 361e8bf..55b8bb6 100644 --- a/cognito/storage/keyring_internal_test.go +++ b/cognito/storage/keyring_internal_test.go @@ -387,6 +387,12 @@ func TestKeyringStore_LoadRejectsAnUnreadablePointer(t *testing.T) { }{ {name: "not json", givenValue: "somewhere-else", wantErr: "parse the token pointer"}, {name: "json naming no generation", givenValue: `{"current":""}`, wantErr: "names no generation"}, + // A bare slot name is what a pre-release build of this package wrote, + // before generations replaced two reusable slots. That shape never + // shipped, so no released version can produce it and there is nothing + // to migrate — but a keychain carrying one still has to fail readably + // rather than with a raw json error. + {name: "a bare slot name from a pre-release layout", givenValue: "a", wantErr: "parse the token pointer"}, } for _, tt := range tests { @@ -398,6 +404,8 @@ func TestKeyringStore_LoadRejectsAnUnreadablePointer(t *testing.T) { _, err := store.Load(testNamespace) require.Error(t, err) assert.Contains(t, err.Error(), tt.wantErr) + assert.Contains(t, err.Error(), "sign out of this environment and sign in again", + "an unreadable pointer must name the recovery, which is not guessable from a json error") }) } }