diff --git a/.github/workflows/fleet-e2e.yaml b/.github/workflows/fleet-e2e.yaml index 649bcf0..b3b92b0 100644 --- a/.github/workflows/fleet-e2e.yaml +++ b/.github/workflows/fleet-e2e.yaml @@ -148,7 +148,10 @@ jobs: # A sha can carry more than one candidate tag; pick the highest by # version sort so selection is deterministic regardless of API # ordering. Accept rc and dryrun tags (see the resolve gate). - VERSION=$(gh api "repos/${GITHUB_REPOSITORY}/tags" \ + # --paginate is required: the tags endpoint serves 30 per page, so + # without it the sha match only ever scans the first page and the + # fallback silently resolves empty once the repo outgrows 30 tags. + VERSION=$(gh api "repos/${GITHUB_REPOSITORY}/tags" --paginate \ --jq ".[] | select(.commit.sha == \"$WR_HEAD_SHA\") | .name" \ | grep -E -- '-(rc|dryrun)\.' | sort -V -r | head -n 1 || true) else diff --git a/.github/workflows/suite-bootstrap-pin.yaml b/.github/workflows/suite-bootstrap-pin.yaml index 8d5cbd2..6beca68 100644 --- a/.github/workflows/suite-bootstrap-pin.yaml +++ b/.github/workflows/suite-bootstrap-pin.yaml @@ -82,7 +82,10 @@ jobs: # dispatch). Look up tags pointing at this commit and filter to final # tags. Accept the highest by semver sort (in case a sha carries # multiple tags) but reject any rc or dryrun. - VERSION=$(gh api "repos/${GITHUB_REPOSITORY}/tags" \ + # --paginate is required: the tags endpoint serves 30 per page, so + # without it the sha match only ever scans the first page and the + # fallback silently resolves empty once the repo outgrows 30 tags. + VERSION=$(gh api "repos/${GITHUB_REPOSITORY}/tags" --paginate \ --jq ".[] | select(.commit.sha == \"$WR_HEAD_SHA\") | .name" \ | grep -vE '-(rc|dryrun)\.' \ | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$' \ diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d71fc9..6683b11 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,25 @@ A `Migration` section is added to any release that bumps `schema_version`. ### Fixed +- **release:** The stale-draft reaper now sees every release instead of only the + 30 most recent. The release list is paginated by the GitHub API, and the + listing read a single response without requesting a page size or following the + `Link` header, so on a repository with more than 30 releases the reaper never + saw the superseded RC drafts it exists to delete. It was a no-op in exactly the + case that needs it, and because draft cleanup is best-effort the accumulation + was silent. The listing now requests 100 per page and follows `rel="next"` to + the end, with a page bound so a malformed link chain cannot spin, and any + failure mid-walk is reported instead of returning a truncated list. Draft + resolution by tag or SHA, which stopped at the first 100 releases, uses the + same paginated listing. A next-page link is followed only when its scheme and + host both match the configured API endpoint, since every request carries a + bearer token. + +- **fleet:** The `fleet-e2e` and `suite-bootstrap-pin` version fallbacks that + resolve a tag by commit SHA now pass `--paginate`. The tags endpoint serves 30 + per page, so the lookup scanned only the newest 30 of the repository's tags and + resolved an empty version once the repository grew past 30 tags. + - **generate:** A manifest-level `concurrency.group` is now namespaced per workflow instead of being emitted bare onto every cascade workflow. A GitHub concurrency group is matched repository-wide rather than per workflow, and a diff --git a/internal/release/list_pagination_test.go b/internal/release/list_pagination_test.go new file mode 100644 index 0000000..3aebed0 --- /dev/null +++ b/internal/release/list_pagination_test.go @@ -0,0 +1,266 @@ +package release + +import ( + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync/atomic" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// newPagedReleaseServer stands up a stub that paginates the release list the way +// api.github.com actually does: it honors per_page, slices the corpus by the +// page query parameter, and advertises the next page with a real +// `Link: <...>; rel="next"` header, omitting that header on the final page. +// +// Every pre-existing release stub encodes the whole corpus into a single +// response, which is precisely why unpaginated list calls read as correct under +// test. A stub that cannot paginate cannot catch a pagination bug. +func newPagedReleaseServer(t *testing.T, corpus []GitHubRelease, deleted *[]int) (*httptest.Server, *int32) { + t.Helper() + var pageRequests int32 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/repos/owner/repo/releases" { + atomic.AddInt32(&pageRequests, 1) + + // GitHub's default page size is 30 when per_page is absent. A caller + // that omits per_page therefore sees at most 30 items. + perPage := 30 + if v := r.URL.Query().Get("per_page"); v != "" { + parsed, err := strconv.Atoi(v) + require.NoError(t, err) + perPage = parsed + } + page := 1 + if v := r.URL.Query().Get("page"); v != "" { + parsed, err := strconv.Atoi(v) + require.NoError(t, err) + page = parsed + } + + start := (page - 1) * perPage + if start > len(corpus) { + start = len(corpus) + } + end := start + perPage + if end > len(corpus) { + end = len(corpus) + } + + // Advertise rel="next" only while a further page exists, exactly as + // the real API does. + if end < len(corpus) { + next := fmt.Sprintf("%s/repos/owner/repo/releases?per_page=%d&page=%d", + strings.TrimSuffix("http://"+r.Host, "/"), perPage, page+1) + w.Header().Set("Link", fmt.Sprintf(`<%s>; rel="next", <%s>; rel="last"`, next, next)) + } + _ = json.NewEncoder(w).Encode(corpus[start:end]) + return + } + if r.Method == http.MethodDelete && strings.Contains(r.URL.Path, "/releases/") { + id, err := strconv.Atoi(strings.TrimPrefix(r.URL.Path, "/repos/owner/repo/releases/")) + require.NoError(t, err) + *deleted = append(*deleted, id) + w.WriteHeader(http.StatusNoContent) + return + } + w.WriteHeader(http.StatusOK) + })) + t.Cleanup(server.Close) + + return server, &pageRequests +} + +// buildReleaseCorpus returns filler releases followed by the stale drafts under +// test, mirroring the real ordering: /releases is newest-first, so a draft that +// has been superseded for a while sinks past the first page on any repository +// with an ordinary release history. +func buildReleaseCorpus(fillerCount int, stale ...GitHubRelease) []GitHubRelease { + corpus := make([]GitHubRelease, 0, fillerCount+len(stale)) + for i := 0; i < fillerCount; i++ { + corpus = append(corpus, GitHubRelease{ + ID: int64(1000 + i), + TagName: fmt.Sprintf("v9.%d.0", i), + Name: fmt.Sprintf("v9.%d.0", i), + Draft: false, + }) + } + corpus = append(corpus, stale...) + return corpus +} + +// TestListDraftReleases_FollowsLinkPagination pins the contract that the release +// lister walks every page. The corpus is 250 releases, which spans nine pages at +// GitHub's 30-item default and three at the maximum per_page=100, so no single +// request can cover it regardless of the page size the lister picks. +func TestListDraftReleases_FollowsLinkPagination(t *testing.T) { + stale := []GitHubRelease{ + {ID: 1, TagName: "v1.2.0-rc.0", Name: "v1.2.0-rc.0", Draft: true}, + {ID: 2, TagName: "v1.2.0-rc.1", Name: "v1.2.0-rc.1", Draft: true}, + } + corpus := buildReleaseCorpus(248, stale...) + require.Len(t, corpus, 250) + require.Greater(t, len(corpus), listPageSize, + "corpus must exceed the maximum page size or the walk is untested") + + deleted := &[]int{} + server, pageRequests := newPagedReleaseServer(t, corpus, deleted) + mgr := NewManagerWithURL("owner/repo", "test-token", server.URL) + + drafts, err := mgr.listDraftReleases() + require.NoError(t, err) + + // The drafts live at the tail of the corpus, past the first page under any + // page size the lister could choose. + assert.Len(t, drafts, 2, "lister must see drafts beyond the first page") + assert.Greater(t, int(atomic.LoadInt32(pageRequests)), 1, + "lister must issue more than one request to span a multi-page corpus") +} + +// TestCleanupStaleDrafts_ReapsDraftsBeyondFirstPage is the consumer-level proof. +// The reaper is a no-op exactly where it is needed: on a repository with a long +// release history, the superseded drafts it exists to delete have sunk past the +// first page. +func TestCleanupStaleDrafts_ReapsDraftsBeyondFirstPage(t *testing.T) { + stale := []GitHubRelease{ + {ID: 1, TagName: "v1.2.0-rc.0", Name: "v1.2.0-rc.0", Draft: true}, + {ID: 2, TagName: "v1.2.0-rc.1", Name: "v1.2.0-rc.1", Draft: true}, + } + corpus := buildReleaseCorpus(248, stale...) + + deleted := &[]int{} + server, _ := newPagedReleaseServer(t, corpus, deleted) + mgr := NewManagerWithURL("owner/repo", "test-token", server.URL) + + require.NoError(t, mgr.cleanupStaleDrafts("test", "v1.2.0-rc.2")) + + assert.ElementsMatch(t, []int{1, 2}, *deleted, + "stale drafts past the first page must still be reaped") +} + +// TestListDraftReleases_FailsLoudlyMidPagination asserts that an error on a +// later page surfaces rather than returning the pages gathered so far. Silent +// truncation is the very defect this change removes; a partial list handed to +// the reaper would preserve drafts it was asked to delete while reporting +// success. +func TestListDraftReleases_FailsLoudlyMidPagination(t *testing.T) { + var pageRequests int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(&pageRequests, 1) + if n == 1 { + next := fmt.Sprintf("http://%s/repos/owner/repo/releases?per_page=100&page=2", r.Host) + w.Header().Set("Link", fmt.Sprintf(`<%s>; rel="next"`, next)) + _ = json.NewEncoder(w).Encode([]GitHubRelease{ + {ID: 1, TagName: "v1.2.0-rc.0", Name: "v1.2.0-rc.0", Draft: true}, + }) + return + } + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message":"boom"}`)) + })) + t.Cleanup(server.Close) + + mgr := NewManagerWithURL("owner/repo", "test-token", server.URL) + + drafts, err := mgr.listDraftReleases() + require.Error(t, err, "a mid-pagination failure must not be reported as success") + assert.Nil(t, drafts, "a failed listing must not return a truncated page set") +} + +// TestListDraftReleases_BoundsPathologicalLinkChain guards against a server (or +// proxy) whose rel="next" never terminates. Without a bound the lister would +// spin forever holding the release path hostage; the walk stops and reports the +// anomaly instead of hanging. +func TestListDraftReleases_BoundsPathologicalLinkChain(t *testing.T) { + var pageRequests int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt32(&pageRequests, 1) + // Always advertise a next page: a self-perpetuating Link chain. + next := fmt.Sprintf("http://%s/repos/owner/repo/releases?per_page=100&page=99", r.Host) + w.Header().Set("Link", fmt.Sprintf(`<%s>; rel="next"`, next)) + _ = json.NewEncoder(w).Encode([]GitHubRelease{ + {ID: 1, TagName: "v1.2.0-rc.0", Name: "v1.2.0-rc.0", Draft: true}, + }) + })) + t.Cleanup(server.Close) + + mgr := NewManagerWithURL("owner/repo", "test-token", server.URL) + + _, err := mgr.listDraftReleases() + require.Error(t, err, "an unterminated Link chain must be reported, not followed forever") + assert.LessOrEqual(t, int(atomic.LoadInt32(&pageRequests)), maxListPages+1, + "the walk must stop at the page bound") +} + +// TestNewPageRequest_RejectsSchemeDowngrade pins the token-confidentiality half +// of the Link check. An attacker who can set the response header could otherwise +// keep the host and drop to http, putting the Bearer token on the wire in +// cleartext. Pinning the host alone leaves the token exposed to exactly the +// attacker the host pin exists to stop. +func TestNewPageRequest_RejectsSchemeDowngrade(t *testing.T) { + m := NewManagerWithURL("owner/repo", "tok", "https://api.github.com") + + _, err := m.newPageRequest("http://api.github.com/repos/owner/repo/releases?page=2") + + require.Error(t, err, "a scheme downgrade on a matching host must be rejected") + assert.Contains(t, err.Error(), "scheme") +} + +// TestNewPageRequest_RejectsForeignHost is the companion: a link pointing off the +// configured API host must not receive the token, whatever its scheme. +func TestNewPageRequest_RejectsForeignHost(t *testing.T) { + m := NewManagerWithURL("owner/repo", "tok", "https://api.github.com") + + _, err := m.newPageRequest("https://evil.example.com/repos/owner/repo/releases?page=2") + + require.Error(t, err, "a link to a foreign host must be rejected") + assert.Contains(t, err.Error(), "host") +} + +// TestNewPageRequest_AllowsSchemeMatchingBase proves the scheme pin is relative +// to the configured base rather than a hardcoded https, so it cannot break a +// legitimate deployment. A GitHub Enterprise install reached over plain http via +// GITHUB_API_URL advertises http links of its own; those must still be followed. +// The https enterprise case is the ordinary one and is covered alongside it. +func TestNewPageRequest_AllowsSchemeMatchingBase(t *testing.T) { + tests := []struct { + name string + baseURL string + next string + }{ + { + name: "enterprise over https", + baseURL: "https://ghe.example.com/api/v3", + next: "https://ghe.example.com/api/v3/repos/owner/repo/releases?page=2", + }, + { + name: "enterprise over plain http", + baseURL: "http://ghe.internal/api/v3", + next: "http://ghe.internal/api/v3/repos/owner/repo/releases?page=2", + }, + { + name: "public github", + baseURL: "https://api.github.com", + next: "https://api.github.com/repos/owner/repo/releases?page=2", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + m := NewManagerWithURL("owner/repo", "tok", tt.baseURL) + + req, err := m.newPageRequest(tt.next) + + require.NoError(t, err, "a link matching the configured base must be followed") + assert.Equal(t, tt.next, req.URL.String()) + assert.Equal(t, "Bearer tok", req.Header.Get("Authorization")) + }) + } +} diff --git a/internal/release/release.go b/internal/release/release.go index 578bc49..15db168 100644 --- a/internal/release/release.go +++ b/internal/release/release.go @@ -8,6 +8,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "regexp" "strings" @@ -687,27 +688,146 @@ func (m *Manager) cleanupStaleDrafts(environment, currentTag string) error { return nil } -// listDraftReleases returns all draft releases in the repository -func (m *Manager) listDraftReleases() ([]GitHubRelease, error) { - req, err := m.newRequest("GET", "/releases", nil) +// listPageSize is the page size requested from the release list endpoint. 100 is +// the maximum the GitHub REST API accepts; anything larger is silently clamped. +// Without it the API serves 30 items per page, so a caller reading a single +// response sees only the 30 most recent releases. +const listPageSize = 100 + +// maxListPages bounds the pagination walk. At listPageSize this covers 5000 +// releases, far beyond any repository the release path serves, while ensuring a +// malformed or self-referential rel="next" chain terminates instead of spinning +// forever and holding up a release. +const maxListPages = 50 + +// parseNextLink extracts the rel="next" URL from a GitHub Link header. It +// returns an empty string when the header is absent or advertises no next page, +// which is how the final page is signalled. +func parseNextLink(header string) string { + for _, segment := range strings.Split(header, ",") { + parts := strings.Split(strings.TrimSpace(segment), ";") + if len(parts) < 2 { + continue + } + target := strings.TrimSpace(parts[0]) + if !strings.HasPrefix(target, "<") || !strings.HasSuffix(target, ">") { + continue + } + for _, param := range parts[1:] { + if strings.EqualFold(strings.TrimSpace(param), `rel="next"`) { + return target[1 : len(target)-1] + } + } + } + return "" +} + +// listAllReleases walks every page of the repository's release list, following +// the Link header's rel="next" until the API stops advertising one. +// +// Any error, including exhausting the page bound, fails the whole listing rather +// than returning the pages gathered so far. Callers treat the result as the +// complete set of releases and act destructively on it: the draft reaper deletes +// what it finds and preserves what it does not. Handing back a silently +// truncated list would make those callers report success while skipping the very +// releases they exist to act on. +func (m *Manager) listAllReleases() ([]GitHubRelease, error) { + endpoint := fmt.Sprintf("/releases?per_page=%d", listPageSize) + req, err := m.newRequest("GET", endpoint, nil) if err != nil { return nil, err } + var all []GitHubRelease + for page := 1; ; page++ { + if page > maxListPages { + return nil, fmt.Errorf("release listing exceeded %d pages; refusing to follow a further rel=\"next\"", maxListPages) + } + + items, next, err := m.fetchReleasePage(req) + if err != nil { + return nil, err + } + all = append(all, items...) + + if next == "" { + return all, nil + } + + req, err = m.newPageRequest(next) + if err != nil { + return nil, err + } + } +} + +// fetchReleasePage performs one release-list request, returning the decoded page +// and the rel="next" URL advertised by the response (empty on the last page). +func (m *Manager) fetchReleasePage(req *http.Request) ([]GitHubRelease, string, error) { resp, err := m.client.Do(req) if err != nil { - return nil, fmt.Errorf("API request failed: %w", err) + return nil, "", fmt.Errorf("API request failed: %w", err) } defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { body, _ := io.ReadAll(resp.Body) - return nil, fmt.Errorf("API error %d: %s", resp.StatusCode, string(body)) + return nil, "", fmt.Errorf("API error %d: %s", resp.StatusCode, string(body)) } var releases []GitHubRelease if err := json.NewDecoder(resp.Body).Decode(&releases); err != nil { - return nil, fmt.Errorf("decoding response: %w", err) + return nil, "", fmt.Errorf("decoding response: %w", err) + } + + return releases, parseNextLink(resp.Header.Get("Link")), nil +} + +// newPageRequest builds the request for a subsequent page from the URL the API +// advertised. Both the scheme and the host are pinned to the configured base +// URL: every request carries a Bearer token, so following a Link header to an +// arbitrary host would hand that token to whoever set the header, and following +// one that keeps the host but downgrades the scheme would put that token on the +// wire in cleartext. Pinning the host alone leaves the same token exposed to the +// same attacker, so both are checked. +// +// The comparison is relative to the configured base rather than a hardcoded +// https. A GitHub Enterprise deployment reached over plain http via +// GITHUB_API_URL advertises http links of its own and still matches, as does the +// http test server; only a scheme that disagrees with the base is rejected. +func (m *Manager) newPageRequest(next string) (*http.Request, error) { + nextURL, err := url.Parse(next) + if err != nil { + return nil, fmt.Errorf("parsing next page link %q: %w", next, err) + } + base, err := url.Parse(m.baseURL) + if err != nil { + return nil, fmt.Errorf("parsing base URL %q: %w", m.baseURL, err) + } + if nextURL.Host != base.Host { + return nil, fmt.Errorf("next page link host %q does not match API host %q", nextURL.Host, base.Host) + } + if nextURL.Scheme != base.Scheme { + return nil, fmt.Errorf("next page link scheme %q does not match API scheme %q", nextURL.Scheme, base.Scheme) + } + + req, err := http.NewRequest(http.MethodGet, nextURL.String(), nil) + if err != nil { + return nil, err + } + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("Authorization", "Bearer "+m.token) + req.Header.Set("X-GitHub-Api-Version", "2022-11-28") + + return req, nil +} + +// listDraftReleases returns all draft releases in the repository, across every +// page of the release list. +func (m *Manager) listDraftReleases() ([]GitHubRelease, error) { + releases, err := m.listAllReleases() + if err != nil { + return nil, err } // Filter to only draft releases @@ -1070,29 +1190,11 @@ func (m *Manager) findReleaseByTagOrSHA(tag, sha string) (*GitHubRelease, error) sleep(time.Duration(attempt) * listRetryBackoff) } - req, err := m.newRequest("GET", "/releases?per_page=100", nil) + releases, err := m.listAllReleases() if err != nil { return nil, err } - resp, err := m.client.Do(req) - if err != nil { - return nil, fmt.Errorf("API request failed: %w", err) - } - - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - _ = resp.Body.Close() - return nil, fmt.Errorf("API error %d: %s", resp.StatusCode, string(body)) - } - - var releases []GitHubRelease - if err := json.NewDecoder(resp.Body).Decode(&releases); err != nil { - _ = resp.Body.Close() - return nil, fmt.Errorf("decoding response: %w", err) - } - _ = resp.Body.Close() - // Resolve the best match in priority order so a stale draft sharing a // target_commitish cannot win over the intended release: // 1. a draft whose tag matches exactly (strongest signal),