From dec32c86cf8a6a5cadba0619271895f2942ca12c Mon Sep 17 00:00:00 2001 From: Mike Horgan Date: Tue, 30 Jun 2026 11:46:13 -0400 Subject: [PATCH] improve spt update token fallback handling --- cli/cmd/update.go | 83 +++++++++-- cli/cmd/update_feedback_test.go | 216 +++++++++++++++++++++++++++-- cli/internal/update/github.go | 63 ++++++++- cli/internal/update/github_test.go | 19 +++ 4 files changed, 352 insertions(+), 29 deletions(-) diff --git a/cli/cmd/update.go b/cli/cmd/update.go index cca7373e..c7ea7046 100644 --- a/cli/cmd/update.go +++ b/cli/cmd/update.go @@ -29,6 +29,20 @@ type updateOptions struct { token string } +type updateTokenSource string + +const ( + updateTokenSourceNone updateTokenSource = "" + updateTokenSourceFlag updateTokenSource = "flag" + updateTokenSourceEnv updateTokenSource = "env" +) + +type updateToken struct { + Value string + Name string + Src updateTokenSource +} + var ( updateCurrentVersion = func() string { return buildinfo.Version } updateExecutable = os.Executable @@ -97,12 +111,8 @@ func runUpdateCommand(cmd *cobra.Command, opts *updateOptions) error { return updateUserError(cmd, "--timeout must be positive") } - ctx, cancel := context.WithTimeout(cmd.Context(), opts.timeout) - defer cancel() - token := resolveUpdateToken(opts.token) - client := updateNewGitHubClient(opts.timeout, token) - releases, err := client.ListReleases(ctx) + releases, client, err := listUpdateReleases(cmd.Context(), cmd, opts.timeout, token) if err != nil { return updateUserError(cmd, "failed to list GitHub releases: %v", err) } @@ -174,11 +184,13 @@ func runUpdateCommand(cmd *cobra.Command, opts *updateOptions) error { return updateUserError(cmd, "%v", err) } - assetBytes, err := client.DownloadAsset(ctx, asset) + dlCtx, dlCancel := context.WithTimeout(cmd.Context(), opts.timeout) + defer dlCancel() + assetBytes, err := client.DownloadAsset(dlCtx, asset) if err != nil { return updateUserError(cmd, "failed to download %s: %v", asset.Name, err) } - checksumBytes, err := client.DownloadAsset(ctx, checksumAsset) + checksumBytes, err := client.DownloadAsset(dlCtx, checksumAsset) if err != nil { return updateUserError(cmd, "failed to download %s: %v", checksumAsset.Name, err) } @@ -207,14 +219,61 @@ func runUpdateCommand(cmd *cobra.Command, opts *updateOptions) error { return nil } -func resolveUpdateToken(flagToken string) string { - if strings.TrimSpace(flagToken) != "" { - return strings.TrimSpace(flagToken) +func resolveUpdateToken(flagToken string) updateToken { + if v := strings.TrimSpace(flagToken); v != "" { + return updateToken{Value: v, Name: "--token", Src: updateTokenSourceFlag} } if v := strings.TrimSpace(os.Getenv(constants.EnvSptGitHubToken)); v != "" { - return v + return updateToken{Value: v, Name: constants.EnvSptGitHubToken, Src: updateTokenSourceEnv} + } + if v := strings.TrimSpace(os.Getenv("GITHUB_TOKEN")); v != "" { + return updateToken{Value: v, Name: "GITHUB_TOKEN", Src: updateTokenSourceEnv} + } + return updateToken{Src: updateTokenSourceNone} +} + +// listUpdateReleases resolves the release list, choosing how to apply any token. +// +// An explicit --token expresses deliberate intent, so it is sent on the first +// request and an invalid value fails clearly. An ambient env/dotenv token is +// treated as best-effort: release lookup is attempted unauthenticated first so a +// stale token cannot break public-repo access, and the token is only used to +// retry when the unauthenticated attempt is rate-limited. +func listUpdateReleases(ctx context.Context, cmd *cobra.Command, timeout time.Duration, token updateToken) ([]updater.Release, updater.GitHubClient, error) { + listOnce := func(client updater.GitHubClient) ([]updater.Release, error) { + attemptCtx, cancel := context.WithTimeout(ctx, timeout) + defer cancel() + return client.ListReleases(attemptCtx) + } + + if token.Src == updateTokenSourceFlag { + client := updateNewGitHubClient(timeout, token.Value) + releases, err := listOnce(client) + if err != nil && updater.IsBadCredentialsError(err) { + return nil, client, fmt.Errorf("GitHub token from --token appears invalid: %w", err) + } + return releases, client, err + } + + unauthClient := updateNewGitHubClient(timeout, "") + releases, err := listOnce(unauthClient) + if err == nil || token.Value == "" || !updater.IsRateLimitError(err) { + return releases, unauthClient, err } - return strings.TrimSpace(os.Getenv("GITHUB_TOKEN")) + + authClient := updateNewGitHubClient(timeout, token.Value) + authReleases, authErr := listOnce(authClient) + if authErr == nil { + return authReleases, authClient, nil + } + if updater.IsBadCredentialsError(authErr) { + // The ambient token is invalid. Surface the original rate-limit error + // (an unauthenticated retry would hit the same limit) and tell the user + // the token is being ignored so they can clean it up. + _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "warning: %s appears invalid; ignoring it for GitHub release lookup\n", token.Name) + return nil, unauthClient, err + } + return nil, authClient, authErr } func warnImageOverride(cmd *cobra.Command) { diff --git a/cli/cmd/update_feedback_test.go b/cli/cmd/update_feedback_test.go index 3fec5815..739e897c 100644 --- a/cli/cmd/update_feedback_test.go +++ b/cli/cmd/update_feedback_test.go @@ -18,6 +18,7 @@ import ( "testing" "time" + "github.com/dell/storage-performance-tool/cli/internal/constants" updater "github.com/dell/storage-performance-tool/cli/internal/update" ) @@ -93,6 +94,175 @@ func TestUpdateAlreadyUpToDateDoesNotDownload(t *testing.T) { }) } +func TestUpdateCheckIgnoresEnvTokenWhenUnauthenticatedLookupSucceeds(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "stale-token") + withCustomUpdateServer(t, customUpdateServerOptions{currentVersion: "5.10.4"}, func(s *customUpdateServer) { + cmd := newUpdateCommand() + cmd.SetArgs([]string{"--check"}) + var out, errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + err := cmd.Execute() + var exitErr *ExitCodeError + if !errors.As(err, &exitErr) || exitErr.Code != 10 { + t.Fatalf("Execute() error = %v, want exit 10", err) + } + if got := s.releaseAuthHeaders; len(got) != 1 || got[0] != "" { + t.Fatalf("release auth headers = %#v, want unauthenticated request", got) + } + if errOut.String() != "" { + t.Fatalf("stderr = %q", errOut.String()) + } + if !strings.Contains(out.String(), "available=true") { + t.Fatalf("stdout = %q", out.String()) + } + }) +} + +func TestUpdateCheckRetriesWithEnvTokenOnRateLimit(t *testing.T) { + t.Setenv("SPT_GITHUB_TOKEN", "good-token") + withCustomUpdateServer(t, customUpdateServerOptions{currentVersion: "5.10.4", rateLimitUnauth: true, acceptedToken: "good-token"}, func(s *customUpdateServer) { + cmd := newUpdateCommand() + cmd.SetArgs([]string{"--check"}) + var out, errOut bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errOut) + err := cmd.Execute() + var exitErr *ExitCodeError + if !errors.As(err, &exitErr) || exitErr.Code != 10 { + t.Fatalf("Execute() error = %v, want exit 10", err) + } + want := []string{"", "Bearer good-token"} + if got := s.releaseAuthHeaders; len(got) != len(want) || got[0] != want[0] || got[1] != want[1] { + t.Fatalf("release auth headers = %#v, want %#v", got, want) + } + if errOut.String() != "" { + t.Fatalf("stderr = %q", errOut.String()) + } + if !strings.Contains(out.String(), "available=true") { + t.Fatalf("stdout = %q", out.String()) + } + }) +} + +// Explicit --token expresses intent, so it must be sent (and validated) on the +// first request even when an unauthenticated lookup would have succeeded. +func TestUpdateCheckReportsExplicitBadTokenEvenWhenUnauthWouldSucceed(t *testing.T) { + withCustomUpdateServer(t, customUpdateServerOptions{currentVersion: "5.10.4", acceptedToken: "good-token"}, func(s *customUpdateServer) { + cmd := newUpdateCommand() + cmd.SetArgs([]string{"--check", "--token", "bad-token"}) + var errOut bytes.Buffer + cmd.SetErr(&errOut) + err := cmd.Execute() + if err == nil { + t.Fatal("Execute() accepted bad explicit token") + } + for _, want := range []string{"--token", "invalid"} { + if !strings.Contains(errOut.String(), want) { + t.Fatalf("stderr = %q missing %q", errOut.String(), want) + } + } + if got := s.releaseAuthHeaders; len(got) != 1 || got[0] != "Bearer bad-token" { + t.Fatalf("release auth headers = %#v, want a single authenticated request", got) + } + }) +} + +// A bad ambient token must not trigger a doomed unauthenticated retry: the +// original rate-limit error is surfaced and the token is reported as ignored. +func TestUpdateCheckWarnsAndReturnsRateLimitWhenEnvTokenIsBad(t *testing.T) { + t.Setenv("GITHUB_TOKEN", "bad-token") + withCustomUpdateServer(t, customUpdateServerOptions{currentVersion: "5.10.4", rateLimitUnauth: true, acceptedToken: "good-token"}, func(s *customUpdateServer) { + cmd := newUpdateCommand() + cmd.SetArgs([]string{"--check"}) + var errOut bytes.Buffer + cmd.SetErr(&errOut) + err := cmd.Execute() + if err == nil { + t.Fatal("Execute() accepted rate-limited unauthenticated lookup with bad env token") + } + wantHeaders := []string{"", "Bearer bad-token"} + if got := s.releaseAuthHeaders; len(got) != len(wantHeaders) || got[0] != wantHeaders[0] || got[1] != wantHeaders[1] { + t.Fatalf("release auth headers = %#v, want %#v (no doomed third request)", got, wantHeaders) + } + for _, want := range []string{"warning: GITHUB_TOKEN appears invalid", "rate limit"} { + if !strings.Contains(errOut.String(), want) { + t.Fatalf("stderr = %q missing %q", errOut.String(), want) + } + } + }) +} + +// The client returned from the rate-limit retry must be the authenticated one, +// and it must carry the token through to the asset download. +func TestUpdateDownloadsWithAuthClientAfterRateLimitRetry(t *testing.T) { + t.Setenv("SPT_GITHUB_TOKEN", "good-token") + withCustomUpdateServer(t, customUpdateServerOptions{currentVersion: "dev", rateLimitUnauth: true, acceptedToken: "good-token", assetUsesAPIURL: true}, func(s *customUpdateServer) { + outPath := filepath.Join(t.TempDir(), "spt-release") + cmd := newUpdateCommand() + cmd.SetArgs([]string{"--output", outPath}) + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&bytes.Buffer{}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute() error = %v", err) + } + got, err := os.ReadFile(outPath) + if err != nil { + t.Fatalf("ReadFile output: %v", err) + } + if string(got) != "release binary" { + t.Fatalf("output file = %q", got) + } + wantReleaseHeaders := []string{"", "Bearer good-token"} + if h := s.releaseAuthHeaders; len(h) != len(wantReleaseHeaders) || h[0] != wantReleaseHeaders[0] || h[1] != wantReleaseHeaders[1] { + t.Fatalf("release auth headers = %#v, want %#v", h, wantReleaseHeaders) + } + if len(s.assetAuthHeaders) == 0 { + t.Fatal("no asset downloads recorded") + } + for i, h := range s.assetAuthHeaders { + if h != "Bearer good-token" { + t.Fatalf("asset auth header[%d] = %q, want authenticated download", i, h) + } + } + }) +} + +func TestResolveUpdateTokenPrecedence(t *testing.T) { + cases := []struct { + name string + flag string + sptEnv string + githubEnv string + wantValue string + wantName string + wantSrc updateTokenSource + }{ + {name: "flag wins over env", flag: "flag-token", sptEnv: "spt-token", githubEnv: "gh-token", wantValue: "flag-token", wantName: "--token", wantSrc: updateTokenSourceFlag}, + {name: "spt env wins over github env", sptEnv: "spt-token", githubEnv: "gh-token", wantValue: "spt-token", wantName: constants.EnvSptGitHubToken, wantSrc: updateTokenSourceEnv}, + {name: "github env used when others unset", githubEnv: "gh-token", wantValue: "gh-token", wantName: "GITHUB_TOKEN", wantSrc: updateTokenSourceEnv}, + {name: "none when all unset", wantValue: "", wantSrc: updateTokenSourceNone}, + {name: "whitespace flag is ignored", flag: " ", githubEnv: "gh-token", wantValue: "gh-token", wantName: "GITHUB_TOKEN", wantSrc: updateTokenSourceEnv}, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + t.Setenv(constants.EnvSptGitHubToken, tt.sptEnv) + t.Setenv("GITHUB_TOKEN", tt.githubEnv) + got := resolveUpdateToken(tt.flag) + if got.Value != tt.wantValue { + t.Fatalf("Value = %q, want %q", got.Value, tt.wantValue) + } + if got.Src != tt.wantSrc { + t.Fatalf("Src = %q, want %q", got.Src, tt.wantSrc) + } + if tt.wantName != "" && got.Name != tt.wantName { + t.Fatalf("Name = %q, want %q", got.Name, tt.wantName) + } + }) + } +} + func TestUpdateChecksumMismatchDoesNotReplace(t *testing.T) { withCustomUpdateServer(t, customUpdateServerOptions{currentVersion: "5.10.4", badChecksum: true}, func(_ *customUpdateServer) { target := filepath.Join(t.TempDir(), "spt") @@ -215,16 +385,21 @@ func TestConfirmUpdateIfNeededReadsLine(t *testing.T) { } type customUpdateServerOptions struct { - currentVersion string - releaseAssets []updater.Asset - badChecksum bool - goos string - goarch string + currentVersion string + releaseAssets []updater.Asset + badChecksum bool + goos string + goarch string + rateLimitUnauth bool + acceptedToken string + assetUsesAPIURL bool } type customUpdateServer struct { - server *httptest.Server - assetDownloads int + server *httptest.Server + assetDownloads int + releaseAuthHeaders []string + assetAuthHeaders []string } func withCustomUpdateServer(t *testing.T, opts customUpdateServerOptions, fn func(*customUpdateServer)) { @@ -246,16 +421,37 @@ func withCustomUpdateServer(t *testing.T, opts customUpdateServerOptions, fn fun {Name: "spt-5.10.5-linux-amd64.gz", BrowserDownloadURL: server.URL + "/assets/spt.gz", Size: int64(len(archive))}, {Name: updater.ChecksumAssetName, BrowserDownloadURL: server.URL + "/assets/SHA256SUMS", Size: int64(len(checksumLine))}, } + if opts.assetUsesAPIURL { + // Setting the asset API URL makes the client authenticate the + // download, so we can observe which client performed the fetch. + assets[0].URL = server.URL + "/assets/spt.gz" + assets[1].URL = server.URL + "/assets/SHA256SUMS" + } } - mux.HandleFunc("/repos/dell/storage-performance-tool/releases", func(w http.ResponseWriter, _ *http.Request) { + mux.HandleFunc("/repos/dell/storage-performance-tool/releases", func(w http.ResponseWriter, r *http.Request) { + auth := r.Header.Get("Authorization") + state.releaseAuthHeaders = append(state.releaseAuthHeaders, auth) + if opts.rateLimitUnauth && auth == "" { + w.Header().Set("X-RateLimit-Remaining", "0") + w.Header().Set("X-RateLimit-Reset", "1782420000") + http.Error(w, "rate limited", http.StatusForbidden) + return + } + if opts.acceptedToken != "" && auth != "Bearer "+opts.acceptedToken { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"message":"Bad credentials","status":"401"}`)) + return + } writeFeedbackJSON(t, w, []updater.Release{{TagName: "v5.10.5", HTMLURL: "https://github.com/dell/storage-performance-tool/releases/tag/v5.10.5", Assets: assets}}) }) - mux.HandleFunc("/assets/spt.gz", func(w http.ResponseWriter, _ *http.Request) { + mux.HandleFunc("/assets/spt.gz", func(w http.ResponseWriter, r *http.Request) { state.assetDownloads++ + state.assetAuthHeaders = append(state.assetAuthHeaders, r.Header.Get("Authorization")) _, _ = w.Write(archive) }) - mux.HandleFunc("/assets/SHA256SUMS", func(w http.ResponseWriter, _ *http.Request) { + mux.HandleFunc("/assets/SHA256SUMS", func(w http.ResponseWriter, r *http.Request) { state.assetDownloads++ + state.assetAuthHeaders = append(state.assetAuthHeaders, r.Header.Get("Authorization")) _, _ = w.Write([]byte(checksumLine)) }) defer server.Close() diff --git a/cli/internal/update/github.go b/cli/internal/update/github.go index a65da870..94c31c3d 100644 --- a/cli/internal/update/github.go +++ b/cli/internal/update/github.go @@ -3,6 +3,7 @@ package update import ( "context" "encoding/json" + "errors" "fmt" "io" "net" @@ -46,6 +47,47 @@ func (e *PageLimitError) Error() string { return fmt.Sprintf("GitHub release pagination exceeded %d pages", e.Limit) } +// RateLimitError reports a GitHub API rate-limit response (primary or secondary). +type RateLimitError struct { + Reset string + RetryAfter string +} + +func (e *RateLimitError) Error() string { + when := e.Reset + label := "X-RateLimit-Reset" + if when == "" && e.RetryAfter != "" { + when = e.RetryAfter + label = "Retry-After" + } + if when == "" { + when = "unknown" + } + return fmt.Sprintf("GitHub API rate limit exceeded; set SPT_GITHUB_TOKEN or GITHUB_TOKEN to raise the limit and retry (%s=%s)", label, when) +} + +// IsRateLimitError reports whether err is a GitHub API rate-limit response. +func IsRateLimitError(err error) bool { + var rateErr *RateLimitError + return errors.As(err, &rateErr) +} + +// BadCredentialsError reports a GitHub API authentication failure. +type BadCredentialsError struct { + Status string + Body string +} + +func (e *BadCredentialsError) Error() string { + return fmt.Sprintf("GitHub API request failed: %s: %s", e.Status, e.Body) +} + +// IsBadCredentialsError reports whether err is a GitHub API authentication failure. +func IsBadCredentialsError(err error) bool { + var authErr *BadCredentialsError + return errors.As(err, &authErr) +} + // NewGitHubClient returns a GitHub client with default SPT repository settings. func NewGitHubClient(timeout time.Duration, token string) GitHubClient { return GitHubClient{ @@ -116,10 +158,18 @@ func (c GitHubClient) getJSON(ctx context.Context, client *http.Client, rawURL s defer func() { _ = resp.Body.Close() }() if resp.StatusCode < 200 || resp.StatusCode >= 300 { body, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) - if (resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusTooManyRequests) && resp.Header.Get("X-RateLimit-Remaining") == "0" { - return nil, rateLimitError(resp) + bodyText := strings.TrimSpace(string(body)) + if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusTooManyRequests { + // Primary rate limits report X-RateLimit-Remaining: 0; secondary + // (abuse) limits report Retry-After without that header. + if resp.Header.Get("X-RateLimit-Remaining") == "0" || resp.Header.Get("Retry-After") != "" { + return nil, rateLimitError(resp) + } + } + if resp.StatusCode == http.StatusUnauthorized { + return nil, &BadCredentialsError{Status: resp.Status, Body: bodyText} } - return nil, fmt.Errorf("GitHub API request failed: %s: %s", resp.Status, strings.TrimSpace(string(body))) + return nil, fmt.Errorf("GitHub API request failed: %s: %s", resp.Status, bodyText) } if err := json.NewDecoder(resp.Body).Decode(out); err != nil { return nil, err @@ -287,9 +337,8 @@ func sameHost(a, b *url.URL) bool { } func rateLimitError(resp *http.Response) error { - reset := resp.Header.Get("X-RateLimit-Reset") - if reset == "" { - reset = "unknown" + return &RateLimitError{ + Reset: resp.Header.Get("X-RateLimit-Reset"), + RetryAfter: resp.Header.Get("Retry-After"), } - return fmt.Errorf("GitHub API rate limit exceeded; set SPT_GITHUB_TOKEN or GITHUB_TOKEN to raise the limit and retry (X-RateLimit-Reset=%s)", reset) } diff --git a/cli/internal/update/github_test.go b/cli/internal/update/github_test.go index 7a9a6971..fa5f154f 100644 --- a/cli/internal/update/github_test.go +++ b/cli/internal/update/github_test.go @@ -71,6 +71,25 @@ func TestGitHubClientListReleasesRateLimitError(t *testing.T) { } } +func TestGitHubClientListReleasesSecondaryRateLimitError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // Secondary/abuse limits return 403 with Retry-After and no + // X-RateLimit-Remaining: 0. + w.Header().Set("Retry-After", "60") + http.Error(w, "secondary rate limit", http.StatusForbidden) + })) + defer server.Close() + + client := GitHubClient{HTTPClient: server.Client(), BaseURL: server.URL, Owner: "dell", Repo: "storage-performance-tool", AllowInsecureLocalHTTP: true} + _, err := client.ListReleases(context.Background()) + if err == nil { + t.Fatal("ListReleases accepted secondary rate-limit response") + } + if !IsRateLimitError(err) { + t.Fatalf("err = %v, want RateLimitError for secondary rate limit", err) + } +} + func serverURL(t *testing.T, r *http.Request) string { t.Helper() return "http://" + r.Host + r.URL.Path