Skip to content

Improve spt update token fallback handling #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mikehorgan-dell merged 1 commit into from
Jun 30, 2026
Merged

Improve spt update token fallback handling #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
83 changes: 71 additions & 12 deletions cli/cmd/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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) {
Expand Down
216 changes: 206 additions & 10 deletions cli/cmd/update_feedback_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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)) {
Expand All @@ -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()
Expand Down
Loading
Loading