From 84893d9c6595d93842d088516cd59532f35a2732 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 6 Jun 2026 00:00:54 -0400 Subject: [PATCH 1/3] fix(wfctl): bound release asset downloads --- cmd/wfctl/plugin_install.go | 12 +++++++++--- cmd/wfctl/plugin_install_test.go | 27 +++++++++++++++++++++++++++ cmd/wfctl/update.go | 6 +++++- 3 files changed, 41 insertions(+), 4 deletions(-) diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index 04d4d60dd..84f600c14 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -1112,6 +1112,9 @@ func parseGitHubReleaseDownloadURL(rawURL string) (owner, repo, tag, filename st // (github.com/.../releases/download/.../file) redirects to a signed S3 URL and // does not propagate the Authorization header correctly. func downloadGitHubReleaseAsset(owner, repo, tag, filename, token string) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), downloadTimeout) + defer cancel() + // Step 1: resolve the asset ID from the release metadata. releaseURL := fmt.Sprintf("%s/repos/%s/%s/releases/tags/%s", //nolint:gosec // G107 gitHubAPIBaseURL, @@ -1119,7 +1122,7 @@ func downloadGitHubReleaseAsset(owner, repo, tag, filename, token string) ([]byt neturl.PathEscape(repo), neturl.PathEscape(tag), ) - req, err := http.NewRequest(http.MethodGet, releaseURL, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, releaseURL, nil) if err != nil { return nil, err } @@ -1165,7 +1168,7 @@ func downloadGitHubReleaseAsset(owner, repo, tag, filename, token string) ([]byt neturl.PathEscape(repo), assetID, ) - req2, err := http.NewRequest(http.MethodGet, assetURL, nil) + req2, err := http.NewRequestWithContext(ctx, http.MethodGet, assetURL, nil) if err != nil { return nil, err } @@ -1203,7 +1206,10 @@ func downloadURL(rawURL string) ([]byte, error) { } // Public repos and non-release GitHub URLs: direct GET with optional Bearer. - req, err := http.NewRequest(http.MethodGet, rawURL, nil) //nolint:gosec // G107: URL comes from registry manifest + ctx, cancel := context.WithTimeout(context.Background(), downloadTimeout) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil) //nolint:gosec // G107: URL comes from registry manifest if err != nil { return nil, err } diff --git a/cmd/wfctl/plugin_install_test.go b/cmd/wfctl/plugin_install_test.go index f09fe90f5..315e559d5 100644 --- a/cmd/wfctl/plugin_install_test.go +++ b/cmd/wfctl/plugin_install_test.go @@ -9,6 +9,7 @@ import ( "encoding/json" "flag" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -115,6 +116,32 @@ func installTestClient(t *testing.T, ct *captureTransport) { t.Cleanup(func() { http.DefaultClient = orig }) } +func TestDownloadURL_DirectGetUsesBoundedRequestContext(t *testing.T) { + orig := http.DefaultClient + http.DefaultClient = &http.Client{ + Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { + if _, ok := req.Context().Deadline(); !ok { + return nil, fmt.Errorf("request has no deadline") + } + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader("ok")), + Header: make(http.Header), + Request: req, + }, nil + }), + } + t.Cleanup(func() { http.DefaultClient = orig }) + + got, err := downloadURL("https://example.com/plugin.tar.gz") + if err != nil { + t.Fatalf("downloadURL: %v", err) + } + if string(got) != "ok" { + t.Fatalf("downloadURL body = %q, want ok", got) + } +} + // TestDownloadURL_GitHubAuthHeader verifies that downloadURL injects a Bearer // Authorization header for non-release github.com URLs (direct-download path) // using the first non-empty token env var (RELEASES_TOKEN > GH_TOKEN > diff --git a/cmd/wfctl/update.go b/cmd/wfctl/update.go index 9d1b94635..712df164c 100644 --- a/cmd/wfctl/update.go +++ b/cmd/wfctl/update.go @@ -1,6 +1,7 @@ package main import ( + "context" "encoding/json" "flag" "fmt" @@ -353,8 +354,11 @@ func verifyAssetChecksum(checksumAsset *githubAsset, assetName string, data []by // downloadWithTimeout fetches a URL using an HTTP client with the given timeout. func downloadWithTimeout(url string, timeout time.Duration) ([]byte, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + client := &http.Client{Timeout: timeout} - req, err := http.NewRequest(http.MethodGet, url, nil) //nolint:noctx // timeout is set on the client + req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) if err != nil { return nil, err } From 77c93aea71aa58cf6865fea10b41cd92ce001861 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 6 Jun 2026 00:09:56 -0400 Subject: [PATCH 2/3] fix(wfctl): split private asset download timeout --- cmd/wfctl/plugin_install.go | 14 ++++--- cmd/wfctl/plugin_install_test.go | 70 ++++++++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 5 deletions(-) diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index 84f600c14..ad0682ed6 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -1054,6 +1054,8 @@ var gitHubAPIBaseURL = "https://api.github.com" // independently. A generous timeout covers large binary asset downloads. var gitHubAPIClient = &http.Client{Timeout: 10 * time.Minute} +const gitHubReleaseMetadataTimeout = 30 * time.Second + // gitHubToken returns the first non-empty GitHub token from the environment, // checking RELEASES_TOKEN, GH_TOKEN, and GITHUB_TOKEN in order. func gitHubToken() string { @@ -1112,9 +1114,6 @@ func parseGitHubReleaseDownloadURL(rawURL string) (owner, repo, tag, filename st // (github.com/.../releases/download/.../file) redirects to a signed S3 URL and // does not propagate the Authorization header correctly. func downloadGitHubReleaseAsset(owner, repo, tag, filename, token string) ([]byte, error) { - ctx, cancel := context.WithTimeout(context.Background(), downloadTimeout) - defer cancel() - // Step 1: resolve the asset ID from the release metadata. releaseURL := fmt.Sprintf("%s/repos/%s/%s/releases/tags/%s", //nolint:gosec // G107 gitHubAPIBaseURL, @@ -1122,7 +1121,9 @@ func downloadGitHubReleaseAsset(owner, repo, tag, filename, token string) ([]byt neturl.PathEscape(repo), neturl.PathEscape(tag), ) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, releaseURL, nil) + metadataCtx, metadataCancel := context.WithTimeout(context.Background(), gitHubReleaseMetadataTimeout) + defer metadataCancel() + req, err := http.NewRequestWithContext(metadataCtx, http.MethodGet, releaseURL, nil) if err != nil { return nil, err } @@ -1149,6 +1150,7 @@ func downloadGitHubReleaseAsset(owner, repo, tag, filename, token string) ([]byt if err := json.NewDecoder(resp.Body).Decode(&release); err != nil { return nil, fmt.Errorf("decode GitHub release response: %w", err) } + metadataCancel() var assetID int64 for _, a := range release.Assets { @@ -1168,7 +1170,9 @@ func downloadGitHubReleaseAsset(owner, repo, tag, filename, token string) ([]byt neturl.PathEscape(repo), assetID, ) - req2, err := http.NewRequestWithContext(ctx, http.MethodGet, assetURL, nil) + assetCtx, assetCancel := context.WithTimeout(context.Background(), downloadTimeout) + defer assetCancel() + req2, err := http.NewRequestWithContext(assetCtx, http.MethodGet, assetURL, nil) if err != nil { return nil, err } diff --git a/cmd/wfctl/plugin_install_test.go b/cmd/wfctl/plugin_install_test.go index 315e559d5..f84f9c3e4 100644 --- a/cmd/wfctl/plugin_install_test.go +++ b/cmd/wfctl/plugin_install_test.go @@ -19,6 +19,7 @@ import ( "strings" "sync" "testing" + "time" ) // captureTransport is a test http.RoundTripper that: @@ -417,6 +418,75 @@ func TestDownloadURL_PrivateReleaseAsset(t *testing.T) { } } +func TestDownloadURL_PrivateReleaseAssetUsesFreshAssetDownloadDeadline(t *testing.T) { + const ( + wantAssetID = int64(99) + wantFilename = "plugin-linux-amd64.tar.gz" + wantTag = "v1.0.0" + wantOwner = "GoCodeAlone" + wantRepo = "test-plugin" + wantToken = "test-secret-token" + ) + + var metadataDeadline, assetDeadline time.Time + rt := roundTripFunc(func(req *http.Request) (*http.Response, error) { + deadline, ok := req.Context().Deadline() + if !ok { + return nil, fmt.Errorf("%s has no request deadline", req.URL.Path) + } + + switch req.URL.Path { + case fmt.Sprintf("/repos/%s/%s/releases/tags/%s", wantOwner, wantRepo, wantTag): + metadataDeadline = deadline + time.Sleep(10 * time.Millisecond) + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader( + fmt.Sprintf(`{"assets":[{"id":%d,"name":%q}]}`, wantAssetID, wantFilename), + )), + Header: make(http.Header), + Request: req, + }, nil + case fmt.Sprintf("/repos/%s/%s/releases/assets/%d", wantOwner, wantRepo, wantAssetID): + assetDeadline = deadline + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader("fake tarball bytes")), + Header: make(http.Header), + Request: req, + }, nil + default: + return nil, fmt.Errorf("unexpected path %s", req.URL.Path) + } + }) + + origAPIBase := gitHubAPIBaseURL + origAPIClient := gitHubAPIClient + gitHubAPIBaseURL = "https://api.github.test" + gitHubAPIClient = &http.Client{Transport: rt} + t.Cleanup(func() { + gitHubAPIBaseURL = origAPIBase + gitHubAPIClient = origAPIClient + }) + + t.Setenv("RELEASES_TOKEN", wantToken) + for _, k := range []string{"GH_TOKEN", "GITHUB_TOKEN"} { + t.Setenv(k, "") + } + + rawURL := fmt.Sprintf("https://github.com/%s/%s/releases/download/%s/%s", + wantOwner, wantRepo, wantTag, wantFilename) + if _, err := downloadURL(rawURL); err != nil { + t.Fatalf("downloadURL: %v", err) + } + if metadataDeadline.IsZero() || assetDeadline.IsZero() { + t.Fatalf("missing recorded deadlines: metadata=%v asset=%v", metadataDeadline, assetDeadline) + } + if !assetDeadline.After(metadataDeadline) { + t.Fatalf("asset deadline = %v, want after metadata deadline %v", assetDeadline, metadataDeadline) + } +} + // TestDownloadURL_PublicReleaseNoToken verifies that when no token is set, // downloadURL falls back to a plain GET for release download URLs (public repos). func TestDownloadURL_PublicReleaseNoToken(t *testing.T) { From 72d99af01f49cc564b712ab3b4ecf057814583b3 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Sat, 6 Jun 2026 00:15:56 -0400 Subject: [PATCH 3/3] feat(wfctl): show download progress --- cmd/wfctl/download_progress.go | 74 ++++++++++++++++++++++++++++++++ cmd/wfctl/plugin_install.go | 4 +- cmd/wfctl/plugin_install_test.go | 27 ++++++++++++ cmd/wfctl/update.go | 2 +- 4 files changed, 104 insertions(+), 3 deletions(-) create mode 100644 cmd/wfctl/download_progress.go diff --git a/cmd/wfctl/download_progress.go b/cmd/wfctl/download_progress.go new file mode 100644 index 000000000..0dd04b3f6 --- /dev/null +++ b/cmd/wfctl/download_progress.go @@ -0,0 +1,74 @@ +package main + +import ( + "bytes" + "fmt" + "io" + "os" + "time" +) + +const downloadProgressInterval = 2 * time.Second + +func readDownloadBodyWithProgress(r io.Reader, total int64) ([]byte, error) { + var buf bytes.Buffer + tracker := newDownloadProgress(os.Stderr, total) + if _, err := io.Copy(io.MultiWriter(&buf, tracker), r); err != nil { + return nil, err + } + tracker.finish() + return buf.Bytes(), nil +} + +type downloadProgress struct { + w io.Writer + total int64 + read int64 + last time.Time +} + +func newDownloadProgress(w io.Writer, total int64) *downloadProgress { + p := &downloadProgress{w: w, total: total} + p.emit("Download progress") + return p +} + +func (p *downloadProgress) Write(data []byte) (int, error) { + n := len(data) + p.read += int64(n) + now := time.Now() + if p.last.IsZero() || now.Sub(p.last) >= downloadProgressInterval || (p.total > 0 && p.read >= p.total) { + p.emit("Download progress") + } + return n, nil +} + +func (p *downloadProgress) finish() { + p.emit("Download complete") +} + +func (p *downloadProgress) emit(prefix string) { + if p.w == nil { + return + } + p.last = time.Now() + if p.total > 0 { + percent := float64(p.read) / float64(p.total) * 100 + fmt.Fprintf(p.w, "%s: %s/%s (%.0f%%)\n", prefix, formatDownloadBytes(p.read), formatDownloadBytes(p.total), percent) + return + } + fmt.Fprintf(p.w, "%s: %s\n", prefix, formatDownloadBytes(p.read)) +} + +func formatDownloadBytes(n int64) string { + const unit = 1024 + if n < unit { + return fmt.Sprintf("%d B", n) + } + div, exp := int64(unit), 0 + for next := n / unit; next >= unit && exp < 4; next /= unit { + div *= unit + exp++ + } + return fmt.Sprintf("%.1f %ciB", float64(n)/float64(div), "KMGTPE"[exp]) +} diff --git a/cmd/wfctl/plugin_install.go b/cmd/wfctl/plugin_install.go index ad0682ed6..d5d5f3a15 100644 --- a/cmd/wfctl/plugin_install.go +++ b/cmd/wfctl/plugin_install.go @@ -1189,7 +1189,7 @@ func downloadGitHubReleaseAsset(owner, repo, tag, filename, token string) ([]byt if resp2.StatusCode != http.StatusOK { return nil, fmt.Errorf("GitHub asset download API: HTTP %d for asset %d", resp2.StatusCode, assetID) } - return io.ReadAll(resp2.Body) + return readDownloadBodyWithProgress(resp2.Body, resp2.ContentLength) } // downloadURL fetches a URL and returns the body bytes. @@ -1230,7 +1230,7 @@ func downloadURL(rawURL string) ([]byte, error) { if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, rawURL) } - return io.ReadAll(resp.Body) + return readDownloadBodyWithProgress(resp.Body, resp.ContentLength) } // verifyChecksum checks that data matches the expected SHA256 hex string. diff --git a/cmd/wfctl/plugin_install_test.go b/cmd/wfctl/plugin_install_test.go index f84f9c3e4..21e72af98 100644 --- a/cmd/wfctl/plugin_install_test.go +++ b/cmd/wfctl/plugin_install_test.go @@ -143,6 +143,33 @@ func TestDownloadURL_DirectGetUsesBoundedRequestContext(t *testing.T) { } } +func TestDownloadURL_LargeDirectDownloadEmitsProgress(t *testing.T) { + orig := http.DefaultClient + http.DefaultClient = &http.Client{ + Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + ContentLength: int64(len("fake tarball bytes")), + Body: io.NopCloser(strings.NewReader("fake tarball bytes")), + Header: make(http.Header), + Request: req, + }, nil + }), + } + t.Cleanup(func() { http.DefaultClient = orig }) + + stderr, err := captureStderr(t, func() error { + _, err := downloadURL("https://example.com/plugin.tar.gz") + return err + }) + if err != nil { + t.Fatalf("downloadURL: %v", err) + } + if !strings.Contains(stderr, "Download progress") || !strings.Contains(stderr, "Download complete") { + t.Fatalf("stderr = %q, want progress and completion indicators", stderr) + } +} + // TestDownloadURL_GitHubAuthHeader verifies that downloadURL injects a Bearer // Authorization header for non-release github.com URLs (direct-download path) // using the first non-empty token env var (RELEASES_TOKEN > GH_TOKEN > diff --git a/cmd/wfctl/update.go b/cmd/wfctl/update.go index 712df164c..9b94b9be6 100644 --- a/cmd/wfctl/update.go +++ b/cmd/wfctl/update.go @@ -371,7 +371,7 @@ func downloadWithTimeout(url string, timeout time.Duration) ([]byte, error) { if resp.StatusCode != http.StatusOK { return nil, fmt.Errorf("HTTP %d from %s", resp.StatusCode, url) } - return io.ReadAll(resp.Body) + return readDownloadBodyWithProgress(resp.Body, resp.ContentLength) } // replaceBinary writes newData to execPath atomically by writing to a temp file