From 76676ee5a6492f49ff8ce819236382fcc6cd344b Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 2 Nov 2026 03:28:24 +0000 Subject: [PATCH] Limit release asset redirects --- docs/integrations/gitea.md | 3 +- docs/roadmap/implementation.md | 2 +- internal/gitea/assets.go | 6 +++ internal/gitea/assets_test.go | 89 ++++++++++++++++++++++++++++++++++ 4 files changed, 98 insertions(+), 2 deletions(-) diff --git a/docs/integrations/gitea.md b/docs/integrations/gitea.md index 3955907..03a4606 100644 --- a/docs/integrations/gitea.md +++ b/docs/integrations/gitea.md @@ -87,7 +87,8 @@ Unknown-write outcomes are reserved for unresolved writes. Unrelated attachments Attachment downloads and redirects carry the Gitea authorization token only when their scheme, host, and port match the configured service. Cross-origin -content is downloaded without that token and verified by its digest. +content is downloaded without that token and verified by its digest. Downloads +follow at most ten redirects; an additional redirect is unverifiable. Shared delivery uploads only after inspection finds no conflicts or unverifiable assets. It uploads missing binaries in filename order and then the diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 71ded96..01c0db8 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -47,7 +47,7 @@ the agent policy; an intermediate compilation dependency is recorded accurately. | 9 | A16 | Complete — shared declaration, receipt, delivery, prebuilt, and source-only regressions passed. | | 10 | A07: allocation and temporary metadata | Complete — allocation resolution, joined cleanup causes, tag evidence, and receipt installation checks passed. | | 11 | A07: build and delivery outcomes | Complete — managed and native cleanup tests cover success, failure, cancellation, hosted completion, and CLI reporting. | -| 12 | A08 | Pending | +| 12 | A08 | Complete — redirect boundary, origin authorization, cycle, and delivery outcome tests passed. | | 13 | A09 | Pending | | 14 | A10 | Pending | | 15 | A13 | Pending | diff --git a/internal/gitea/assets.go b/internal/gitea/assets.go index 445720e..62ed6ec 100644 --- a/internal/gitea/assets.go +++ b/internal/gitea/assets.go @@ -56,6 +56,7 @@ func (c Client) openAsset(ctx context.Context, client *http.Client, base *url.UR } assetURL.Fragment = "" visited := make(map[string]struct{}) + followed := 0 for { key := assetURL.String() if _, exists := visited[key]; exists { @@ -75,6 +76,10 @@ func (c Client) openAsset(ctx context.Context, client *http.Client, base *url.UR return nil, failure(FailureUnverifiable, "download release asset", 0, err) } if isRedirect(response.StatusCode) { + if followed == 10 { + _ = response.Body.Close() + return nil, failure(FailureUnverifiable, "redirect release asset: redirect limit exceeded", response.StatusCode, errors.New("at most 10 redirects are allowed")) + } location, locationErr := response.Location() _ = response.Body.Close() if locationErr != nil || location.User != nil { @@ -82,6 +87,7 @@ func (c Client) openAsset(ctx context.Context, client *http.Client, base *url.UR } location.Fragment = "" assetURL = location + followed++ continue } if response.StatusCode < 200 || response.StatusCode >= 300 { diff --git a/internal/gitea/assets_test.go b/internal/gitea/assets_test.go index 08d06bd..3d7816c 100644 --- a/internal/gitea/assets_test.go +++ b/internal/gitea/assets_test.go @@ -4,15 +4,104 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" + "strconv" + "strings" "sync/atomic" "testing" "go.maximumdirect.net/releasekit/internal/forge" ) +func TestBackendOpenAssetBoundsRedirects(t *testing.T) { + for _, hops := range []int{0, 10, 11} { + t.Run(strconv.Itoa(hops), func(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + if got := r.Header.Get("Authorization"); got != "token fixture-secret-token" { + t.Errorf("authorization = %q", got) + } + step, err := strconv.Atoi(strings.TrimPrefix(r.URL.Path, "/")) + if err != nil { + t.Errorf("invalid path %q", r.URL.Path) + return + } + if step < hops { + http.Redirect(w, r, fmt.Sprintf("/%d", step+1), http.StatusFound) + return + } + _, _ = io.WriteString(w, "asset") + })) + defer server.Close() + backend := NewBackend(testDestination(server.URL), server.Client()) + stream, err := backend.OpenAsset(t.Context(), assetHandle{downloadURL: "/0"}) + if hops <= 10 { + if err != nil { + t.Fatal(err) + } + body, readErr := io.ReadAll(stream) + closeErr := stream.Close() + if readErr != nil || closeErr != nil || string(body) != "asset" || requests.Load() != int32(hops+1) { + t.Fatalf("body = %q, read = %v, close = %v, requests = %d", body, readErr, closeErr, requests.Load()) + } + return + } + var failure *forge.Failure + if !errors.As(err, &failure) || failure.Kind != forge.FailureUnverifiable || !strings.Contains(err.Error(), "redirect limit") || requests.Load() != 11 || stream != nil { + t.Fatalf("stream = %v, error = %v, requests = %d", stream, err, requests.Load()) + } + }) + } +} + +func TestBackendOpenAssetRedirectReturnsToOrigin(t *testing.T) { + var origin *httptest.Server + external := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Authorization"); got != "" { + t.Errorf("cross-origin authorization = %q", got) + } + http.Redirect(w, r, origin.URL+"/final", http.StatusFound) + })) + defer external.Close() + origin = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Authorization"); got != "token fixture-secret-token" { + t.Errorf("origin authorization = %q", got) + } + if r.URL.Path == "/start" { + http.Redirect(w, r, external.URL+"/away", http.StatusFound) + return + } + _, _ = io.WriteString(w, "asset") + })) + defer origin.Close() + stream, err := NewBackend(testDestination(origin.URL), origin.Client()).OpenAsset(t.Context(), assetHandle{downloadURL: "/start"}) + if err != nil { + t.Fatal(err) + } + defer stream.Close() + if body, err := io.ReadAll(stream); err != nil || string(body) != "asset" { + t.Fatalf("body = %q, error = %v", body, err) + } +} + +func TestBackendOpenAssetRejectsRedirectCycle(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + http.Redirect(w, r, "/loop#different-fragment", http.StatusFound) + })) + defer server.Close() + stream, err := NewBackend(testDestination(server.URL), server.Client()).OpenAsset(t.Context(), assetHandle{downloadURL: "/loop"}) + var failure *forge.Failure + if !errors.As(err, &failure) || failure.Kind != forge.FailureUnverifiable || !strings.Contains(err.Error(), "loop") || requests.Load() != 1 || stream != nil { + t.Fatalf("stream = %v, error = %v, requests = %d", stream, err, requests.Load()) + } +} + func TestBackendListAssetsRejectsMalformedAttachmentList(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet || r.URL.Path != "/api/v1/repos/owner/project/releases/10/assets" {