diff --git a/docs/integrations/gitea.md b/docs/integrations/gitea.md index 7a32c6a..35eee31 100644 --- a/docs/integrations/gitea.md +++ b/docs/integrations/gitea.md @@ -47,9 +47,9 @@ not missing. A failed read-only inspection reports `not_attempted`; if this invocation already delivered the page, its outcome retains `partial` progress. Unknown-write outcomes are reserved for unresolved writes. Unrelated attachments are returned separately and are preserved. -Attachment downloads and same-origin redirects carry authorization only when -their scheme, host, and port match the configured Gitea service. Cross-origin -URLs and redirects are left unrequested and reported as unverifiable. +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. The upload operation accepts only an inspection result without conflicts or unverifiable assets. It uploads missing binaries in filename order and then the diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 89d1138..f86485b 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -259,6 +259,8 @@ repository parents and confirm clean worktrees. Affected suites pass. ### R7 — Medium: Attachment origin restrictions exceed credential isolation +Status: Resolved. + Category: Inadvertently strengthened prohibition. **Evidence:** [downloadHash](../../internal/gitea/assets.go), around lines 164 @@ -281,6 +283,12 @@ network-policy framework. conflicting bytes rejected. Direct and redirected external requests never carry the Gitea token; same-origin authenticated downloads continue to work. +**Resolution:** Attachment URLs and redirects may cross origins; the client +sets the Gitea token only on requests to its configured origin. HTTP tests +verify direct and redirected external matches, conflicting external bytes, +absence of authorization on external requests, and continued same-origin +authentication. The Gitea suite passes. + ### R8 — Medium: The release wrapper collapses documented exit codes Category: Executable integration and documentation correctness. diff --git a/internal/gitea/assets.go b/internal/gitea/assets.go index 381d8b6..f4c1e1c 100644 --- a/internal/gitea/assets.go +++ b/internal/gitea/assets.go @@ -161,8 +161,8 @@ func (c Client) downloadHash(ctx context.Context, client *http.Client, base *url return "", 0, failure(FailureUnverifiable, "download release asset", 0, errors.New("attachment has no download URL")) } assetURL, err := base.Parse(rawURL) - if err != nil || !sameOrigin(base, assetURL) || assetURL.User != nil { - return "", 0, failure(FailureUnverifiable, "download release asset", 0, errors.New("asset URL is outside the configured service origin")) + if err != nil || assetURL.User != nil { + return "", 0, failure(FailureUnverifiable, "download release asset", 0, errors.New("invalid attachment URL")) } assetURL.Fragment = "" visited := make(map[string]struct{}) @@ -176,7 +176,9 @@ func (c Client) downloadHash(ctx context.Context, client *http.Client, base *url if err != nil { return "", 0, failure(FailureUnverifiable, "download release asset", 0, err) } - request.Header.Set("Authorization", "token "+token) + if sameOrigin(base, assetURL) { + request.Header.Set("Authorization", "token "+token) + } request.Header.Set("Accept", "application/octet-stream") response, err := client.Do(request) if err != nil { @@ -185,8 +187,8 @@ func (c Client) downloadHash(ctx context.Context, client *http.Client, base *url if isRedirect(response.StatusCode) { location, locationErr := response.Location() _ = response.Body.Close() - if locationErr != nil || !sameOrigin(base, location) || location.User != nil { - return "", 0, failure(FailureUnverifiable, "redirect release asset", response.StatusCode, errors.New("asset redirect is outside the configured service origin")) + if locationErr != nil || location.User != nil { + return "", 0, failure(FailureUnverifiable, "redirect release asset", response.StatusCode, errors.New("invalid attachment redirect")) } location.Fragment = "" assetURL = location diff --git a/internal/gitea/assets_test.go b/internal/gitea/assets_test.go index 3dd85d0..b162f3a 100644 --- a/internal/gitea/assets_test.go +++ b/internal/gitea/assets_test.go @@ -249,26 +249,48 @@ func TestInspectAssetsFollowsSameOriginRedirectWithAuthorization(t *testing.T) { } } -func TestInspectAssetsNeverSendsTokenToCrossOriginRedirect(t *testing.T) { - var externalCalled atomic.Bool - external := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { externalCalled.Store(true) })) - defer external.Close() - expected := assetFor("app.bin", []byte("alpha")) - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assertAssetGET(t, r, "") - if r.URL.Path == assetListPath { - writeAssetList(t, w, []assetResponse{{ID: 1, Name: expected.Filename, Size: expected.Size, BrowserDownloadURL: "/files/redirect"}}) - return - } - http.Redirect(w, r, external.URL+"/private", http.StatusFound) - })) - defer server.Close() - inspection, err := testClient(server).InspectAssets(context.Background(), testDestination(server.URL), 10, []ExpectedAsset{expected}) - if err != nil { - t.Fatal(err) - } - if inspection.Assets[0].State != AssetUnverifiable || externalCalled.Load() { - t.Fatalf("inspection = %#v, cross-origin called = %t", inspection, externalCalled.Load()) +func TestInspectAssetsVerifiesExternalContentWithoutGiteaToken(t *testing.T) { + for _, mode := range []string{"direct", "redirect", "conflicting"} { + t.Run(mode, func(t *testing.T) { + expected := assetFor("app.bin", []byte("matching bytes")) + externalGets := 0 + external := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + externalGets++ + if r.Header.Get("Authorization") != "" { + t.Error("Gitea token reached external host") + } + content := []byte("matching bytes") + if mode == "conflicting" { + content = []byte("different bytes") + } + w.Write(content) + })) + defer external.Close() + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assertAssetGET(t, r, "") + if r.URL.Path == assetListPath { + download := external.URL + "/content" + if mode == "redirect" { + download = "/redirect" + } + writeAssetList(t, w, []assetResponse{{ID: 1, Name: expected.Filename, Size: expected.Size, BrowserDownloadURL: download}}) + return + } + http.Redirect(w, r, external.URL+"/content", http.StatusFound) + })) + defer origin.Close() + inspection, err := testClient(origin).InspectAssets(context.Background(), testDestination(origin.URL), 10, []ExpectedAsset{expected}) + if err != nil { + t.Fatal(err) + } + want := AssetMatching + if mode == "conflicting" { + want = AssetConflicting + } + if inspection.Assets[0].State != want || externalGets != 1 { + t.Fatalf("inspection=%#v downloads=%d", inspection, externalGets) + } + }) } }