Verify external assets without forwarding Gitea credentials
This commit is contained in:
@@ -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.
|
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.
|
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
|
Attachment downloads and redirects carry the Gitea authorization token only
|
||||||
their scheme, host, and port match the configured Gitea service. Cross-origin
|
when their scheme, host, and port match the configured service. Cross-origin
|
||||||
URLs and redirects are left unrequested and reported as unverifiable.
|
content is downloaded without that token and verified by its digest.
|
||||||
|
|
||||||
The upload operation accepts only an inspection result without conflicts or
|
The upload operation accepts only an inspection result without conflicts or
|
||||||
unverifiable assets. It uploads missing binaries in filename order and then the
|
unverifiable assets. It uploads missing binaries in filename order and then the
|
||||||
|
|||||||
@@ -259,6 +259,8 @@ repository parents and confirm clean worktrees. Affected suites pass.
|
|||||||
|
|
||||||
### R7 — Medium: Attachment origin restrictions exceed credential isolation
|
### R7 — Medium: Attachment origin restrictions exceed credential isolation
|
||||||
|
|
||||||
|
Status: Resolved.
|
||||||
|
|
||||||
Category: Inadvertently strengthened prohibition.
|
Category: Inadvertently strengthened prohibition.
|
||||||
|
|
||||||
**Evidence:** [downloadHash](../../internal/gitea/assets.go), around lines 164
|
**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
|
conflicting bytes rejected. Direct and redirected external requests never
|
||||||
carry the Gitea token; same-origin authenticated downloads continue to work.
|
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
|
### R8 — Medium: The release wrapper collapses documented exit codes
|
||||||
|
|
||||||
Category: Executable integration and documentation correctness.
|
Category: Executable integration and documentation correctness.
|
||||||
|
|||||||
@@ -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"))
|
return "", 0, failure(FailureUnverifiable, "download release asset", 0, errors.New("attachment has no download URL"))
|
||||||
}
|
}
|
||||||
assetURL, err := base.Parse(rawURL)
|
assetURL, err := base.Parse(rawURL)
|
||||||
if err != nil || !sameOrigin(base, assetURL) || assetURL.User != nil {
|
if err != nil || assetURL.User != nil {
|
||||||
return "", 0, failure(FailureUnverifiable, "download release asset", 0, errors.New("asset URL is outside the configured service origin"))
|
return "", 0, failure(FailureUnverifiable, "download release asset", 0, errors.New("invalid attachment URL"))
|
||||||
}
|
}
|
||||||
assetURL.Fragment = ""
|
assetURL.Fragment = ""
|
||||||
visited := make(map[string]struct{})
|
visited := make(map[string]struct{})
|
||||||
@@ -176,7 +176,9 @@ func (c Client) downloadHash(ctx context.Context, client *http.Client, base *url
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return "", 0, failure(FailureUnverifiable, "download release asset", 0, err)
|
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")
|
request.Header.Set("Accept", "application/octet-stream")
|
||||||
response, err := client.Do(request)
|
response, err := client.Do(request)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -185,8 +187,8 @@ func (c Client) downloadHash(ctx context.Context, client *http.Client, base *url
|
|||||||
if isRedirect(response.StatusCode) {
|
if isRedirect(response.StatusCode) {
|
||||||
location, locationErr := response.Location()
|
location, locationErr := response.Location()
|
||||||
_ = response.Body.Close()
|
_ = response.Body.Close()
|
||||||
if locationErr != nil || !sameOrigin(base, location) || location.User != nil {
|
if locationErr != nil || location.User != nil {
|
||||||
return "", 0, failure(FailureUnverifiable, "redirect release asset", response.StatusCode, errors.New("asset redirect is outside the configured service origin"))
|
return "", 0, failure(FailureUnverifiable, "redirect release asset", response.StatusCode, errors.New("invalid attachment redirect"))
|
||||||
}
|
}
|
||||||
location.Fragment = ""
|
location.Fragment = ""
|
||||||
assetURL = location
|
assetURL = location
|
||||||
|
|||||||
@@ -249,26 +249,48 @@ func TestInspectAssetsFollowsSameOriginRedirectWithAuthorization(t *testing.T) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestInspectAssetsNeverSendsTokenToCrossOriginRedirect(t *testing.T) {
|
func TestInspectAssetsVerifiesExternalContentWithoutGiteaToken(t *testing.T) {
|
||||||
var externalCalled atomic.Bool
|
for _, mode := range []string{"direct", "redirect", "conflicting"} {
|
||||||
external := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { externalCalled.Store(true) }))
|
t.Run(mode, func(t *testing.T) {
|
||||||
defer external.Close()
|
expected := assetFor("app.bin", []byte("matching bytes"))
|
||||||
expected := assetFor("app.bin", []byte("alpha"))
|
externalGets := 0
|
||||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
external := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
assertAssetGET(t, r, "")
|
externalGets++
|
||||||
if r.URL.Path == assetListPath {
|
if r.Header.Get("Authorization") != "" {
|
||||||
writeAssetList(t, w, []assetResponse{{ID: 1, Name: expected.Filename, Size: expected.Size, BrowserDownloadURL: "/files/redirect"}})
|
t.Error("Gitea token reached external host")
|
||||||
return
|
}
|
||||||
}
|
content := []byte("matching bytes")
|
||||||
http.Redirect(w, r, external.URL+"/private", http.StatusFound)
|
if mode == "conflicting" {
|
||||||
}))
|
content = []byte("different bytes")
|
||||||
defer server.Close()
|
}
|
||||||
inspection, err := testClient(server).InspectAssets(context.Background(), testDestination(server.URL), 10, []ExpectedAsset{expected})
|
w.Write(content)
|
||||||
if err != nil {
|
}))
|
||||||
t.Fatal(err)
|
defer external.Close()
|
||||||
}
|
origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
if inspection.Assets[0].State != AssetUnverifiable || externalCalled.Load() {
|
assertAssetGET(t, r, "")
|
||||||
t.Fatalf("inspection = %#v, cross-origin called = %t", inspection, externalCalled.Load())
|
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)
|
||||||
|
}
|
||||||
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user