diff --git a/cmd/cli/cmd/mcp.go b/cmd/cli/cmd/mcp.go index f29233e..11807d5 100644 --- a/cmd/cli/cmd/mcp.go +++ b/cmd/cli/cmd/mcp.go @@ -59,7 +59,10 @@ var mcpServeCmd = &cobra.Command{ tokenProvider := func() (string, error) { return lib.GetNullifyToken(ctx, authCtx.Host, nullifyToken, githubToken) } - httpClient, clientErr := client.NewRefreshingHTTPClient(authCtx.Host, tokenProvider) + refreshProvider := func() (string, error) { + return lib.RefreshNullifyToken(ctx, authCtx.Host, nullifyToken, githubToken) + } + httpClient, clientErr := client.NewRefreshingHTTPClient(authCtx.Host, tokenProvider, refreshProvider) if clientErr != nil { return fmt.Errorf("failed to create client: %w", clientErr) } diff --git a/internal/auth/login.go b/internal/auth/login.go index 0d06b5d..aadcdbb 100644 --- a/internal/auth/login.go +++ b/internal/auth/login.go @@ -3,6 +3,7 @@ package auth import ( "context" "encoding/json" + "errors" "fmt" "io" "net" @@ -18,6 +19,10 @@ import ( var httpClient = &http.Client{Timeout: 30 * time.Second} +// authBaseURL builds the API origin for the auth endpoints. It is a var so +// tests can point the auth calls at an httptest server. +var authBaseURL = func(host string) string { return "https://" + apiHost(host) } + type cliSessionResponse struct { SessionID string `json:"session_id"` AuthURL string `json:"auth_url"` @@ -198,8 +203,35 @@ func GetValidToken(ctx context.Context, host string) (string, error) { return hostCreds.AccessToken, nil } +// ErrNotRefreshable reports that the stored credentials for a host carry no +// refresh token, so no amount of retrying will produce a new access token. +var ErrNotRefreshable = errors.New("no refresh token stored") + +// ForceRefreshToken exchanges the stored refresh token for a new access token +// regardless of the recorded expiry. GetValidToken only refreshes once +// ExpiresAt has elapsed, which leaves callers no way to recover from a token +// the server rejects while the CLI still believes it is valid -- revocation, a +// killed session, or a local clock running behind the server's. +func ForceRefreshToken(ctx context.Context, host string) (string, error) { + creds, err := LoadCredentials() + if err != nil { + return "", fmt.Errorf("not authenticated - run 'nullify auth login'") + } + + hostCreds, ok := creds[CredentialKey(host)] + if !ok { + return "", fmt.Errorf("not authenticated for %s - run 'nullify auth login --host %s'", host, host) + } + if hostCreds.RefreshToken == "" { + return "", ErrNotRefreshable + } + + logger.L(ctx).Debug("forcing access token refresh") + return refreshToken(ctx, host, hostCreds.RefreshToken) +} + func createCLISession(ctx context.Context, host string, port int) (*cliSessionResponse, error) { - url := fmt.Sprintf("https://%s/auth/cli/session", apiHost(host)) + url := authBaseURL(host) + "/auth/cli/session" bodyData, err := json.Marshal(map[string]int{"port": port}) if err != nil { @@ -233,7 +265,7 @@ func createCLISession(ctx context.Context, host string, port int) (*cliSessionRe } func fetchCLIToken(ctx context.Context, host string, sessionID string) (*cliTokenResponse, error) { - url := fmt.Sprintf("https://%s/auth/cli/token", apiHost(host)) + url := authBaseURL(host) + "/auth/cli/token" bodyData, err := json.Marshal(map[string]string{"session_id": sessionID}) if err != nil { @@ -267,7 +299,7 @@ func fetchCLIToken(ctx context.Context, host string, sessionID string) (*cliToke } func refreshToken(ctx context.Context, host string, refreshTok string) (string, error) { - refreshURL := fmt.Sprintf("https://%s/auth/refresh_token", apiHost(host)) + refreshURL := authBaseURL(host) + "/auth/refresh_token" req, err := http.NewRequestWithContext(ctx, http.MethodGet, refreshURL, nil) if err != nil { @@ -296,17 +328,21 @@ func refreshToken(ctx context.Context, host string, refreshTok string) (string, return "", err } - // The endpoint delivers the new access token as a Set-Cookie header - // rather than in the JSON body; fall back to it when the body has none. + // GET /auth/refresh_token carries no token in its JSON body -- see + // RefreshTokenOutput in auth/pkg/endpoints/refresh_token.go, which has no + // token field at all. The access token arrives only as a Set-Cookie header, + // so this is the sole source on every refresh, not a fallback. The body + // check is kept in case the endpoint ever starts returning one. if result.AccessToken == "" { for _, c := range resp.Cookies() { - if c.Name == "access_token" { - result.AccessToken = c.Value - if result.ExpiresIn == 0 && c.MaxAge > 0 { - result.ExpiresIn = c.MaxAge - } - break + if c.Name != "access_token" || c.Value == "" { + continue + } + result.AccessToken = c.Value + if result.ExpiresIn == 0 { + result.ExpiresIn = cookieLifetime(c) } + break } } @@ -329,6 +365,24 @@ func refreshToken(ctx context.Context, host string, refreshTok string) (string, return result.AccessToken, nil } +// cookieLifetime returns a cookie's remaining lifetime in seconds, preferring +// Max-Age and falling back to Expires. Returns 0 when the cookie carries +// neither, which refreshToken stores as an already-elapsed ExpiresAt: an +// unknown lifetime forces a refresh on next use rather than being trusted. +// Storing 0 instead would read as "never expires" in GetValidToken, which +// only checks an expiry it was actually given. +func cookieLifetime(c *http.Cookie) int { + if c.MaxAge > 0 { + return c.MaxAge + } + if !c.Expires.IsZero() { + if secs := int(time.Until(c.Expires).Seconds()); secs > 0 { + return secs + } + } + return 0 +} + // apiHost returns the API hostname, prepending "api." if not already present. func apiHost(host string) string { if strings.HasPrefix(host, "api.") { diff --git a/internal/auth/login_test.go b/internal/auth/login_test.go new file mode 100644 index 0000000..9c5b3e2 --- /dev/null +++ b/internal/auth/login_test.go @@ -0,0 +1,222 @@ +package auth + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + "time" +) + +// newAuthServer points the package's auth calls at h and isolates the +// credential store, which SaveHostCredentials writes under os.UserHomeDir(). +func newAuthServer(t *testing.T, h http.HandlerFunc) *httptest.Server { + t.Helper() + srv := httptest.NewServer(h) + oldBase, oldClient := authBaseURL, httpClient + authBaseURL = func(string) string { return srv.URL } + httpClient = srv.Client() + t.Cleanup(func() { + authBaseURL, httpClient = oldBase, oldClient + srv.Close() + }) + t.Setenv("HOME", t.TempDir()) + return srv +} + +// refreshHandler mirrors what GET /auth/refresh_token actually returns: the +// access token only as a Set-Cookie header, with the metadata in the body. +// See setCookies and RefreshTokenOutput in auth/pkg/endpoints. +func refreshHandler(w http.ResponseWriter, _ *http.Request) { + http.SetCookie(w, &http.Cookie{ + Name: "access_token", + Value: "new-id-token", + MaxAge: 3600, + Path: "/", + Secure: true, + HttpOnly: true, + SameSite: http.SameSiteNoneMode, + }) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"expiresIn":3600,"installationIds":[],"expiresAt":1,` + + `"queryParameters":{"githubOwnerId":"42"},"isTriaged":false,"isNullifyDev":false}`)) +} + +func TestRefreshTokenReadsSetCookie(t *testing.T) { + newAuthServer(t, refreshHandler) + + got, err := refreshToken(context.Background(), "example.nullify.ai", "stored-refresh") + if err != nil { + t.Fatalf("refreshToken: %v", err) + } + if got != "new-id-token" { + t.Fatalf("token = %q, want new-id-token", got) + } + + creds, err := LoadCredentials() + if err != nil { + t.Fatalf("LoadCredentials: %v", err) + } + saved := creds[CredentialKey("example.nullify.ai")] + if saved.AccessToken != "new-id-token" { + t.Errorf("saved AccessToken = %q, want new-id-token", saved.AccessToken) + } + // The endpoint does not rotate the refresh cookie, so the stored one must + // survive or every later refresh fails. + if saved.RefreshToken != "stored-refresh" { + t.Errorf("saved RefreshToken = %q, want stored-refresh", saved.RefreshToken) + } + if d := saved.ExpiresAt - time.Now().Unix(); d < 3500 || d > 3600 { + t.Errorf("ExpiresAt delta = %ds, want ~3600", d) + } + if saved.QueryParameters["githubOwnerId"] != "42" { + t.Errorf("saved QueryParameters = %v, want githubOwnerId=42", saved.QueryParameters) + } +} + +func TestRefreshTokenBodyTokenWins(t *testing.T) { + newAuthServer(t, func(w http.ResponseWriter, _ *http.Request) { + http.SetCookie(w, &http.Cookie{Name: "access_token", Value: "cookie-token", MaxAge: 3600}) + _, _ = w.Write([]byte(`{"accessToken":"body-token","expiresIn":60}`)) + }) + + got, err := refreshToken(context.Background(), "example.nullify.ai", "rt") + if err != nil { + t.Fatalf("refreshToken: %v", err) + } + if got != "body-token" { + t.Errorf("token = %q, want body-token to take precedence", got) + } +} + +func TestRefreshTokenIgnoresEmptyCookie(t *testing.T) { + newAuthServer(t, func(w http.ResponseWriter, _ *http.Request) { + // A cleared cookie ahead of the live one must not win. + http.SetCookie(w, &http.Cookie{Name: "access_token", Value: "", MaxAge: -1}) + http.SetCookie(w, &http.Cookie{Name: "access_token", Value: "real-token", MaxAge: 3600}) + _, _ = w.Write([]byte(`{"expiresIn":3600}`)) + }) + + got, err := refreshToken(context.Background(), "example.nullify.ai", "rt") + if err != nil { + t.Fatalf("refreshToken: %v", err) + } + if got != "real-token" { + t.Errorf("token = %q, want real-token", got) + } +} + +func TestRefreshTokenExpiresFallback(t *testing.T) { + newAuthServer(t, func(w http.ResponseWriter, _ *http.Request) { + // Max-Age absent: the lifetime must come from Expires rather than + // collapsing to an already-expired credential. + http.SetCookie(w, &http.Cookie{ + Name: "access_token", + Value: "expires-token", + Expires: time.Now().Add(time.Hour), + }) + _, _ = w.Write([]byte(`{}`)) + }) + + if _, err := refreshToken(context.Background(), "example.nullify.ai", "rt"); err != nil { + t.Fatalf("refreshToken: %v", err) + } + + creds, err := LoadCredentials() + if err != nil { + t.Fatalf("LoadCredentials: %v", err) + } + if d := creds[CredentialKey("example.nullify.ai")].ExpiresAt - time.Now().Unix(); d < 3500 { + t.Errorf("ExpiresAt delta = %ds, want ~3600 from the Expires attribute", d) + } +} + +func TestRefreshTokenNoTokenAnywhere(t *testing.T) { + newAuthServer(t, func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte(`{"expiresIn":3600}`)) + }) + + _, err := refreshToken(context.Background(), "example.nullify.ai", "rt") + if err == nil { + t.Fatal("want an error when neither body nor cookie carries a token") + } +} + +func TestRefreshTokenNon200(t *testing.T) { + newAuthServer(t, func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + }) + + _, err := refreshToken(context.Background(), "example.nullify.ai", "rt") + if err == nil { + t.Fatal("want an error on a non-200 refresh") + } +} + +func TestGetValidTokenReturnsStoredTokenWhenUnexpired(t *testing.T) { + newAuthServer(t, func(http.ResponseWriter, *http.Request) { + t.Error("refresh must not be attempted while the token is still valid") + }) + seed(t, "example.nullify.ai", "live-token", "rt", time.Now().Add(time.Hour).Unix()) + + got, err := GetValidToken(context.Background(), "example.nullify.ai") + if err != nil { + t.Fatalf("GetValidToken: %v", err) + } + if got != "live-token" { + t.Errorf("token = %q, want live-token", got) + } +} + +func TestGetValidTokenRefreshesWhenExpired(t *testing.T) { + newAuthServer(t, refreshHandler) + seed(t, "example.nullify.ai", "old-token", "rt", time.Now().Add(-time.Hour).Unix()) + + got, err := GetValidToken(context.Background(), "example.nullify.ai") + if err != nil { + t.Fatalf("GetValidToken: %v", err) + } + if got != "new-id-token" { + t.Errorf("token = %q, want the refreshed new-id-token", got) + } +} + +// ForceRefreshToken exists so a caller holding a token the server rejects can +// get a new one even though the stored expiry still looks valid. +func TestForceRefreshTokenIgnoresUnexpiredCredential(t *testing.T) { + newAuthServer(t, refreshHandler) + seed(t, "example.nullify.ai", "revoked-token", "rt", time.Now().Add(time.Hour).Unix()) + + got, err := ForceRefreshToken(context.Background(), "example.nullify.ai") + if err != nil { + t.Fatalf("ForceRefreshToken: %v", err) + } + if got != "new-id-token" { + t.Errorf("token = %q, want new-id-token", got) + } +} + +func TestForceRefreshTokenWithoutRefreshToken(t *testing.T) { + newAuthServer(t, func(http.ResponseWriter, *http.Request) { + t.Error("refresh must not be attempted without a stored refresh token") + }) + seed(t, "example.nullify.ai", "tok", "", time.Now().Add(time.Hour).Unix()) + + _, err := ForceRefreshToken(context.Background(), "example.nullify.ai") + if !errors.Is(err, ErrNotRefreshable) { + t.Fatalf("err = %v, want ErrNotRefreshable", err) + } +} + +func seed(t *testing.T, host, access, refresh string, expiresAt int64) { + t.Helper() + err := SaveHostCredentials(host, HostCredentials{ + AccessToken: access, + RefreshToken: refresh, + ExpiresAt: expiresAt, + }) + if err != nil { + t.Fatalf("seed credentials: %v", err) + } +} diff --git a/internal/client/refreshing_http_client_test.go b/internal/client/refreshing_http_client_test.go index 8462599..98733c8 100644 --- a/internal/client/refreshing_http_client_test.go +++ b/internal/client/refreshing_http_client_test.go @@ -12,10 +12,11 @@ import ( func TestNewRefreshingHTTPClientFetchesInitialToken(t *testing.T) { calls := 0 - hc, err := NewRefreshingHTTPClient("acme.nullify.ai", func() (string, error) { + provider := func() (string, error) { calls++ return "tok", nil - }) + } + hc, err := NewRefreshingHTTPClient("acme.nullify.ai", provider, provider) if err != nil { t.Fatalf("unexpected err: %v", err) } @@ -28,9 +29,10 @@ func TestNewRefreshingHTTPClientFetchesInitialToken(t *testing.T) { } func TestNewRefreshingHTTPClientFailsFastOnTokenError(t *testing.T) { - _, err := NewRefreshingHTTPClient("acme.nullify.ai", func() (string, error) { + provider := func() (string, error) { return "", errors.New("no creds") - }) + } + _, err := NewRefreshingHTTPClient("acme.nullify.ai", provider, provider) if err == nil { t.Fatal("expected error when initial token fetch fails") } @@ -65,12 +67,13 @@ func TestRefreshingAuthTransportRefreshesAfterTTL(t *testing.T) { } stub := &stubTransport{} tr := &refreshingAuthTransport{ - nullifyHost: "acme.nullify.ai", - tokenProvider: provider, - transport: stub, - cachedToken: initialToken, - cachedAt: time.Now(), - cacheTTL: 10 * time.Millisecond, + nullifyHost: "acme.nullify.ai", + tokenProvider: provider, + refreshProvider: provider, + transport: stub, + cachedToken: initialToken, + cachedAt: time.Now(), + cacheTTL: 10 * time.Millisecond, } req1, _ := http.NewRequest("GET", "https://api.acme.nullify.ai/", nil) @@ -110,12 +113,13 @@ func TestRefreshingAuthTransportFallsBackOnRefreshError(t *testing.T) { } stub := &stubTransport{} tr := &refreshingAuthTransport{ - nullifyHost: "acme.nullify.ai", - tokenProvider: failing, - transport: stub, - cachedToken: "good-token", - cachedAt: time.Now().Add(-time.Hour), - cacheTTL: time.Second, + nullifyHost: "acme.nullify.ai", + tokenProvider: failing, + refreshProvider: failing, + transport: stub, + cachedToken: "good-token", + cachedAt: time.Now().Add(-time.Hour), + cacheTTL: time.Second, } req, _ := http.NewRequest("GET", "https://api.acme.nullify.ai/", nil) if _, err := tr.RoundTrip(req); err != nil { diff --git a/internal/client/refreshing_transport.go b/internal/client/refreshing_transport.go index c0237e0..d69bcd6 100644 --- a/internal/client/refreshing_transport.go +++ b/internal/client/refreshing_transport.go @@ -3,6 +3,7 @@ package client import ( "bytes" "context" + "errors" "io" "net/http" "strings" @@ -15,23 +16,39 @@ import ( // TokenProvider is a function that returns a valid token. type TokenProvider func() (string, error) +// ErrTokenNotRefreshable reports that the active token source yields a fixed +// string, so retrying after a 401 cannot produce a different token. +var ErrTokenNotRefreshable = errors.New("token source cannot be refreshed") + // refreshingAuthTransport wraps authTransport and refreshes the token proactively. type refreshingAuthTransport struct { nullifyHost string tokenProvider TokenProvider - transport http.RoundTripper + // refreshProvider mints a genuinely new token, ignoring any cached expiry. + // Distinct from tokenProvider, which returns the stored token untouched + // while it still looks valid. + refreshProvider TokenProvider + transport http.RoundTripper mu sync.RWMutex cachedToken string cachedAt time.Time - cacheTTL time.Duration + // providerRetryAfter and refreshRetryAfter are separate clocks because + // tokenProvider and refreshProvider are separate functions: one failing + // says nothing about the other. + providerRetryAfter time.Time + refreshRetryAfter time.Time + cacheTTL time.Duration + failureBackoff time.Duration } // NewRefreshingHTTPClient returns an *http.Client whose transport injects a // Nullify bearer token (refreshed on a TTL) and retries transient failures. It // is suitable for driving the generated api.Client in long-running processes // like the MCP server, where a token fetched at startup would otherwise expire. -func NewRefreshingHTTPClient(nullifyHost string, tokenProvider TokenProvider) (*http.Client, error) { +// refreshProvider is used to recover from a 401 and must bypass any cached +// expiry; passing tokenProvider for both makes the 401 retry a no-op. +func NewRefreshingHTTPClient(nullifyHost string, tokenProvider, refreshProvider TokenProvider) (*http.Client, error) { // Fetch an initial token so startup fails fast on auth problems. token, err := tokenProvider() if err != nil { @@ -39,12 +56,14 @@ func NewRefreshingHTTPClient(nullifyHost string, tokenProvider TokenProvider) (* } t := &refreshingAuthTransport{ - nullifyHost: nullifyHost, - tokenProvider: tokenProvider, - transport: http.DefaultTransport, - cachedToken: token, - cachedAt: time.Now(), - cacheTTL: 5 * time.Minute, + nullifyHost: nullifyHost, + tokenProvider: tokenProvider, + refreshProvider: refreshProvider, + transport: http.DefaultTransport, + cachedToken: token, + cachedAt: time.Now(), + cacheTTL: 5 * time.Minute, + failureBackoff: 30 * time.Second, } return &http.Client{ @@ -55,8 +74,8 @@ func NewRefreshingHTTPClient(nullifyHost string, tokenProvider TokenProvider) (* // NewRefreshingNullifyClient creates a NullifyClient that automatically refreshes // its auth token, suitable for long-running processes like MCP servers. -func NewRefreshingNullifyClient(nullifyHost string, tokenProvider TokenProvider) (*NullifyClient, error) { - httpClient, err := NewRefreshingHTTPClient(nullifyHost, tokenProvider) +func NewRefreshingNullifyClient(nullifyHost string, tokenProvider, refreshProvider TokenProvider) (*NullifyClient, error) { + httpClient, err := NewRefreshingHTTPClient(nullifyHost, tokenProvider, refreshProvider) if err != nil { return nil, err } @@ -74,9 +93,16 @@ func NewRefreshingNullifyClient(nullifyHost string, tokenProvider TokenProvider) }, nil } +// fresh reports whether the cached token can be served without consulting the +// provider: either it is still within its TTL, or a recent provider failure is +// still inside its backoff window. Callers must hold t.mu. +func (t *refreshingAuthTransport) fresh() bool { + return time.Since(t.cachedAt) < t.cacheTTL || time.Now().Before(t.providerRetryAfter) +} + func (t *refreshingAuthTransport) getToken(ctx context.Context) string { t.mu.RLock() - if time.Since(t.cachedAt) < t.cacheTTL { + if t.fresh() { token := t.cachedToken t.mu.RUnlock() return token @@ -86,37 +112,66 @@ func (t *refreshingAuthTransport) getToken(ctx context.Context) string { // Double-checked locking t.mu.Lock() defer t.mu.Unlock() - if time.Since(t.cachedAt) < t.cacheTTL { + if t.fresh() { return t.cachedToken } newToken, err := t.tokenProvider() if err != nil { - // Fall back to cached token; log so the user can diagnose 401s + // Fall back to cached token; log so the user can diagnose 401s. Hold + // off on the next attempt so a persistently failing provider does not + // put a blocking refresh in front of every single request. + t.providerRetryAfter = time.Now().Add(t.failureBackoff) logger.L(ctx).Warn("token refresh failed, using cached token", logger.Err(err)) return t.cachedToken } + t.providerRetryAfter = time.Time{} t.cachedToken = newToken t.cachedAt = time.Now() return t.cachedToken } -// forceRefresh fetches a new token regardless of the cache TTL. It is storm-safe: -// if another goroutine already replaced staleToken (e.g. several concurrent -// requests hit a 401 at once), the already-refreshed token is reused instead of -// fetching again. +// forceRefresh fetches a new token regardless of the cache TTL, returning "" when +// it cannot produce one the caller has not already tried. It is storm-safe: if +// another goroutine already replaced staleToken (e.g. several concurrent requests +// hit a 401 at once), the already-refreshed token is reused instead of fetching +// again. A refresh that makes no progress arms a backoff window, so a session +// whose credentials the server keeps rejecting does not put a blocking refresh in +// front of every request for the rest of its life. func (t *refreshingAuthTransport) forceRefresh(ctx context.Context, staleToken string) string { t.mu.Lock() defer t.mu.Unlock() if t.cachedToken != staleToken { return t.cachedToken } - newToken, err := t.tokenProvider() + if time.Now().Before(t.refreshRetryAfter) { + return "" + } + + newToken, err := t.refreshProvider() if err != nil { - logger.L(ctx).Warn("forced token refresh failed after 401", logger.Err(err)) + t.refreshRetryAfter = time.Now().Add(t.failureBackoff) + // A fixed token source has nothing to re-fetch, so the 401 is final + // rather than a failure worth warning about. + if errors.Is(err, ErrTokenNotRefreshable) { + logger.L(ctx).Debug("token source cannot be refreshed, surfacing 401") + } else { + logger.L(ctx).Warn("forced token refresh failed after 401", logger.Err(err)) + } return "" } + // Neither an empty token nor the one the server just rejected is progress. + // Leave the cache alone rather than replacing a working token with junk or + // re-fetching the same rejected string on every subsequent request. + if newToken == "" || newToken == staleToken { + t.refreshRetryAfter = time.Now().Add(t.failureBackoff) + logger.L(ctx).Warn("forced token refresh returned no new token, surfacing 401") + return "" + } + + t.providerRetryAfter = time.Time{} + t.refreshRetryAfter = time.Time{} t.cachedToken = newToken t.cachedAt = time.Now() return newToken diff --git a/internal/client/refreshing_transport_test.go b/internal/client/refreshing_transport_test.go index b7cff43..d301d77 100644 --- a/internal/client/refreshing_transport_test.go +++ b/internal/client/refreshing_transport_test.go @@ -2,6 +2,7 @@ package client import ( "context" + "errors" "io" "net/http" "strings" @@ -11,13 +12,19 @@ import ( ) func newRefreshingTransport(initial string, tp TokenProvider, inner http.RoundTripper) *refreshingAuthTransport { + return newRefreshingTransportWithRefresher(initial, tp, tp, inner) +} + +func newRefreshingTransportWithRefresher(initial string, tp, rp TokenProvider, inner http.RoundTripper) *refreshingAuthTransport { return &refreshingAuthTransport{ - nullifyHost: "acme.nullify.ai", - tokenProvider: tp, - transport: inner, - cachedToken: initial, - cachedAt: time.Now(), - cacheTTL: time.Hour, // keep getToken from refreshing on TTL during 401 tests + nullifyHost: "acme.nullify.ai", + tokenProvider: tp, + refreshProvider: rp, + transport: inner, + cachedToken: initial, + cachedAt: time.Now(), + cacheTTL: time.Hour, // keep getToken from refreshing on TTL during 401 tests + failureBackoff: 30 * time.Second, } } @@ -178,3 +185,152 @@ func TestGetTokenRefreshesOnTTLExpiry(t *testing.T) { t.Errorf("refreshCalls = %d, want 1", refreshCalls) } } + +// A token the server has revoked still looks valid to the CLI, so the plain +// token provider hands back the same string. The 401 retry must consult the +// refresh provider instead, or it can never recover. +func TestRefreshOn401UsesRefreshProviderWhenTokenLooksValid(t *testing.T) { + tokenCalls, refreshCalls := 0, 0 + tp := func() (string, error) { tokenCalls++; return "revoked", nil } + rp := func() (string, error) { refreshCalls++; return "fresh", nil } + inner, _ := bearerRouter(map[string]int{"revoked": 401, "fresh": 200}) + tr := newRefreshingTransportWithRefresher("revoked", tp, rp, inner) + + req, _ := http.NewRequest(http.MethodGet, "http://x", nil) + resp, err := tr.RoundTrip(req) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if resp.StatusCode != 200 { + t.Errorf("status = %d, want 200 after a forced refresh", resp.StatusCode) + } + if refreshCalls != 1 { + t.Errorf("refreshProvider calls = %d, want 1", refreshCalls) + } + if tokenCalls != 0 { + t.Errorf("tokenProvider calls = %d, want 0 (cache is within TTL)", tokenCalls) + } +} + +// A fixed token source (--nullify-token / NULLIFY_TOKEN) has nothing to +// re-fetch, so the 401 must surface instead of being retried. +func TestRefreshOn401GivesUpWhenNotRefreshable(t *testing.T) { + rp := func() (string, error) { return "", ErrTokenNotRefreshable } + inner, calls := bearerRouter(map[string]int{"fixed": 401}) + tr := newRefreshingTransportWithRefresher("fixed", func() (string, error) { return "fixed", nil }, rp, inner) + + req, _ := http.NewRequest(http.MethodGet, "http://x", nil) + resp, err := tr.RoundTrip(req) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + if resp.StatusCode != 401 { + t.Errorf("status = %d, want the original 401", resp.StatusCode) + } + if got := calls(); got != 1 { + t.Errorf("upstream requests = %d, want 1 (no retry)", got) + } +} + +// Once the TTL has elapsed and the provider is failing, the transport must not +// put a fresh blocking refresh in front of every request. +func TestFailingProviderIsBackedOff(t *testing.T) { + providerCalls := 0 + tp := func() (string, error) { providerCalls++; return "", errors.New("provider down") } + inner, _ := bearerRouter(map[string]int{"cached": 200}) + tr := newRefreshingTransportWithRefresher("cached", tp, tp, inner) + tr.cacheTTL = time.Millisecond + tr.cachedAt = time.Now().Add(-time.Hour) + + for i := 0; i < 5; i++ { + req, _ := http.NewRequest(http.MethodGet, "http://x", nil) + resp, err := tr.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip %d: %v", i, err) + } + if resp.StatusCode != 200 { + t.Fatalf("RoundTrip %d: status = %d, want 200 on the cached token", i, resp.StatusCode) + } + } + + if providerCalls != 1 { + t.Errorf("tokenProvider calls = %d, want 1 across 5 requests inside the backoff window", providerCalls) + } +} + +// A refresh provider that keeps failing must not be consulted again on every +// subsequent 401 -- each call is a blocking round trip taken under the write +// lock, and a session whose refresh token the server has rejected never recovers. +func TestForceRefreshIsBackedOffAfterFailure(t *testing.T) { + refreshCalls := 0 + rp := func() (string, error) { refreshCalls++; return "", io.ErrUnexpectedEOF } + inner, calls := bearerRouter(map[string]int{"revoked": 401}) + tr := newRefreshingTransportWithRefresher("revoked", func() (string, error) { return "revoked", nil }, rp, inner) + + for i := range 5 { + req, _ := http.NewRequest(http.MethodGet, "http://x", nil) + resp, err := tr.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip %d: %v", i, err) + } + if resp.StatusCode != 401 { + t.Fatalf("RoundTrip %d: status = %d, want the original 401", i, resp.StatusCode) + } + resp.Body.Close() + } + + if refreshCalls != 1 { + t.Errorf("refreshProvider calls = %d, want 1 across 5 requests inside the backoff window", refreshCalls) + } + if got := calls(); got != 5 { + t.Errorf("upstream requests = %d, want 5 (one per request, no retries)", got) + } +} + +// A refresh that hands back the token the server just rejected is not progress: +// RoundTrip cannot retry with it, so re-fetching it on the next 401 only costs +// another round trip. The cache must also be left alone. +func TestForceRefreshTreatsUnchangedTokenAsFailure(t *testing.T) { + refreshCalls := 0 + rp := func() (string, error) { refreshCalls++; return "revoked", nil } + inner, calls := bearerRouter(map[string]int{"revoked": 401}) + tr := newRefreshingTransportWithRefresher("revoked", func() (string, error) { return "revoked", nil }, rp, inner) + + for i := range 2 { + req, _ := http.NewRequest(http.MethodGet, "http://x", nil) + resp, err := tr.RoundTrip(req) + if err != nil { + t.Fatalf("RoundTrip %d: %v", i, err) + } + if resp.StatusCode != 401 { + t.Fatalf("RoundTrip %d: status = %d, want the original 401", i, resp.StatusCode) + } + resp.Body.Close() + } + + if refreshCalls != 1 { + t.Errorf("refreshProvider calls = %d, want 1 (the second request is inside the backoff window)", refreshCalls) + } + if got := calls(); got != 2 { + t.Errorf("upstream requests = %d, want 2 (no retry with the rejected token)", got) + } +} + +// An empty token from a provider that reported no error must not replace a +// cached one; the transport would then send "Bearer " on every later request. +func TestForceRefreshKeepsCacheOnEmptyToken(t *testing.T) { + rp := func() (string, error) { return "", nil } + inner, _ := bearerRouter(map[string]int{"revoked": 401}) + tr := newRefreshingTransportWithRefresher("revoked", func() (string, error) { return "revoked", nil }, rp, inner) + + req, _ := http.NewRequest(http.MethodGet, "http://x", nil) + resp, err := tr.RoundTrip(req) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } + resp.Body.Close() + + if tr.cachedToken != "revoked" { + t.Errorf("cachedToken = %q, want the previous token kept", tr.cachedToken) + } +} diff --git a/internal/lib/get_token.go b/internal/lib/get_token.go index d5f66fe..7138b99 100644 --- a/internal/lib/get_token.go +++ b/internal/lib/get_token.go @@ -22,14 +22,42 @@ type githubToken struct { Token string `json:"accessToken"` } +// GetNullifyToken resolves a token from the first source that provides one: +// the --nullify-token flag, NULLIFY_TOKEN, a GitHub Actions token exchange, or +// the stored credentials. func GetNullifyToken( ctx context.Context, nullifyHost string, nullifyTokenFlag string, githubTokenFlag string, +) (string, error) { + return resolveNullifyToken(ctx, nullifyHost, nullifyTokenFlag, githubTokenFlag, false) +} + +// RefreshNullifyToken resolves a token the same way as GetNullifyToken but +// insists on a freshly minted one, bypassing the stored expiry. Sources that +// yield a fixed string return ErrTokenNotRefreshable. +func RefreshNullifyToken( + ctx context.Context, + nullifyHost string, + nullifyTokenFlag string, + githubTokenFlag string, +) (string, error) { + return resolveNullifyToken(ctx, nullifyHost, nullifyTokenFlag, githubTokenFlag, true) +} + +func resolveNullifyToken( + ctx context.Context, + nullifyHost string, + nullifyTokenFlag string, + githubTokenFlag string, + force bool, ) (string, error) { // 1. Command-line flag if nullifyTokenFlag != "" { + if force { + return "", client.ErrTokenNotRefreshable + } logger.L(ctx).Debug("using token from flag") return nullifyTokenFlag, nil } @@ -37,6 +65,9 @@ func GetNullifyToken( // 2. Environment variable token := os.Getenv("NULLIFY_TOKEN") if token != "" { + if force { + return "", client.ErrTokenNotRefreshable + } logger.L(ctx).Debug("using token from env") return token, nil } @@ -100,7 +131,14 @@ func GetNullifyToken( } // 4. Stored credentials from ~/.nullify/credentials.json - storedToken, err := auth.GetValidToken(ctx, nullifyHost) + getStored := auth.GetValidToken + if force { + getStored = auth.ForceRefreshToken + } + storedToken, err := getStored(ctx, nullifyHost) + if force && errors.Is(err, auth.ErrNotRefreshable) { + return "", client.ErrTokenNotRefreshable + } if err == nil && storedToken != "" { logger.L(ctx).Debug("using token from stored credentials") return storedToken, nil diff --git a/internal/lib/get_token_test.go b/internal/lib/get_token_test.go index a22c0d1..38462f0 100644 --- a/internal/lib/get_token_test.go +++ b/internal/lib/get_token_test.go @@ -7,6 +7,7 @@ import ( "time" "github.com/nullify-platform/cli/internal/auth" + "github.com/nullify-platform/cli/internal/client" "github.com/nullify-platform/cli/internal/logger" "github.com/stretchr/testify/require" ) @@ -32,3 +33,57 @@ func TestGetNullifyTokenPreservesStoredCredentialErrors(t *testing.T) { "should preserve stored credential error, not return generic ErrNoToken") require.Contains(t, err.Error(), "stored credentials") } + +// A 401 retry is only worth making when the token source can produce a +// different token. The three fixed sources must say so with +// client.ErrTokenNotRefreshable rather than handing back the rejected value. +func TestRefreshNullifyTokenRejectsFixedSources(t *testing.T) { + ctx, err := logger.ConfigureDevelopmentLogger(context.Background(), "error") + require.NoError(t, err) + + tests := []struct { + name string + tokenFlag string + setup func(t *testing.T) + }{ + { + name: "nullify token flag", + tokenFlag: "flag-token", + }, + { + name: "NULLIFY_TOKEN env", + setup: func(t *testing.T) { + t.Setenv("NULLIFY_TOKEN", "env-token") + }, + }, + { + name: "stored credentials without a refresh token", + setup: func(t *testing.T) { + require.NoError(t, auth.SaveHostCredentials("acme.nullify.ai", auth.HostCredentials{ + AccessToken: "stored-token", + ExpiresAt: time.Now().Add(time.Hour).Unix(), + })) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("NULLIFY_TOKEN", "") + t.Setenv("GITHUB_ACTIONS", "") + if tt.setup != nil { + tt.setup(t) + } + + // The same inputs still resolve a token normally: forcing a refresh + // is what changes the outcome, not the precedence chain. + got, err := GetNullifyToken(ctx, "acme.nullify.ai", tt.tokenFlag, "") + require.NoError(t, err) + require.NotEmpty(t, got) + + _, err = RefreshNullifyToken(ctx, "acme.nullify.ai", tt.tokenFlag, "") + require.ErrorIs(t, err, client.ErrTokenNotRefreshable) + }) + } +}