fix(executor): never give the archive token back after a redirect left its origin

commit: be35c9fd5a4c0688511eb260bb9f75d915e31a4f
author: brice · 8d ago · 2026-10-14 03:01 UTC
parent: 547d930
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

2 files · +76 -5

M internal/executor/actions_download.go +19 -5


@@ -13,6 +13,7 @@ import (
13	13		"net/url"
14	14		"os"
15	15		"path/filepath"
	16	+	"slices"
16	17		"strings"
17	18		"time"
18	19

@@ -74,12 +75,25 @@ func (j job) archiveClient() http.Client {
74	75		if len(via) >= 10 {
75	76		return errors.New("stopped after 10 redirects")
76	77		}
77		-	prev := via[len(via)-1]
78		-	if prev.URL.Scheme == "https" && req.URL.Scheme != "https" {
79		-	return fmt.Errorf("refusing to follow a redirect from https to %s", req.URL.Scheme)
	78	+	// Go builds every redirected request from the headers of the first one
	79	+	// (copying Authorization when the host matches), so the header must be
	80	+	// removed again on every hop that is not on the origin the token was meant
	81	+	// for: the first request's. Once the chain left that origin, it never gets
	82	+	// the header back, even if a later hop returns to it (A -> B -> A would
	83	+	// otherwise be re-authorised by the copy).
	84	+	origin := originOf(via[0].URL.String())
	85	+	chain := slices.Concat(via[1:], []*http.Request{req}) // not append: via belongs to the client
	86	+	for i, r := range chain {
	87	+	prev := via[i]
	88	+	if prev.URL.Scheme == "https" && r.URL.Scheme != "https" {
	89	+	return fmt.Errorf("refusing to follow a redirect from https to %s", r.URL.Scheme)
	90	+	}
80	91		}
81		-	if originOf(prev.URL.String()) != originOf(req.URL.String()) {
82		-	req.Header.Del("Authorization")
	92	+	for _, r := range chain {
	93	+	if originOf(r.URL.String()) != origin {
	94	+	req.Header.Del("Authorization")
	95	+	break
	96	+	}
83	97		}
84	98		return nil
85	99		}

 	"net/url"
 	"os"
 	"path/filepath"
+	"slices"
 	"strings"
 	"time"
 		if len(via) >= 10 {
 			return errors.New("stopped after 10 redirects")
 		}
-		prev := via[len(via)-1]
+		// Go builds every redirected request from the headers of the first one
-		if prev.URL.Scheme == "https" && req.URL.Scheme != "https" {
+		// (copying Authorization when the host matches), so the header must be
-			return fmt.Errorf("refusing to follow a redirect from https to %s", req.URL.Scheme)
+		// removed again on every hop that is not on the origin the token was meant
+		// for: the first request's. Once the chain left that origin, it never gets
+		// the header back, even if a later hop returns to it (A -> B -> A would
+		// otherwise be re-authorised by the copy).
+		origin := originOf(via[0].URL.String())
+		chain := slices.Concat(via[1:], []*http.Request{req}) // not append: via belongs to the client
+		for i, r := range chain {
+			prev := via[i]
+			if prev.URL.Scheme == "https" && r.URL.Scheme != "https" {
+				return fmt.Errorf("refusing to follow a redirect from https to %s", r.URL.Scheme)
+			}
 		}
-		if originOf(prev.URL.String()) != originOf(req.URL.String()) {
+		for _, r := range chain {
-			req.Header.Del("Authorization")
+			if originOf(r.URL.String()) != origin {
+				req.Header.Del("Authorization")
+				break
+			}
 		}
 		return nil
 	}

M internal/executor/actions_test.go +57 -0


@@ -12,6 +12,7 @@ import (
12	12		"os"
13	13		"path/filepath"
14	14		"strings"
	15	+	"sync"
15	16		"sync/atomic"
16	17		"testing"
17	18		"time"

@@ -758,3 +759,59 @@ func TestArchiveRedirectPolicy(t *testing.T) {
758	759		t.Error("redirect loops must stop")
759	760		}
760	761		}
	762	+
	763	+	// GO-008: Go rebuilds the headers of a redirected request from the first
	764	+	// request, so a chain that leaves the origin and stays on the new one, or comes
	765	+	// back, would get the Authorization header again on the third request. Real
	766	+	// servers, real redirects.
	767	+	func TestArchiveRedirectChainNeverReauthorises(t *testing.T) {
	768	+	type hit struct{ path, auth string }
	769	+	var mu sync.Mutex
	770	+	var hits []hit
	771	+	record := func(r *http.Request) {
	772	+	mu.Lock()
	773	+	hits = append(hits, hit{r.URL.Path, r.Header.Get("Authorization")})
	774	+	mu.Unlock()
	775	+	}
	776	+	var a, b *httptest.Server
	777	+	// a (the authorised origin) and b are two servers on the same host and
	778	+	// different ports: the case of Go's host-only comparison.
	779	+	a = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	780	+	record(r)
	781	+	switch r.URL.Path {
	782	+	case "/start":
	783	+	http.Redirect(w, r, b.URL+"/first", http.StatusFound)
	784	+	case "/back":
	785	+	_, _ = w.Write(tarOf(t, "o-x-1", files{"action.yml": nodeAction, "main.sh": "echo ok\n"}))
	786	+	}
	787	+	}))
	788	+	defer a.Close()
	789	+	b = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
	790	+	record(r)
	791	+	switch r.URL.Path {
	792	+	case "/first":
	793	+	http.Redirect(w, r, b.URL+"/second", http.StatusFound)
	794	+	case "/second":
	795	+	http.Redirect(w, r, a.URL+"/back", http.StatusFound) // and back to the origin
	796	+	}
	797	+	}))
	798	+	defer b.Close()
	799	+	res := func(context.Context, []string) ([]ResolvedAction, error) {
	800	+	return []ResolvedAction{{Ref: "o/x@v1", NameWithOwner: "o/x", ResolvedSHA: strings.Repeat("e", 40), TarballURL: a.URL + "/start", Token: "tok", AuthOrigin: originOf(a.URL)}}, nil
	801	+	}
	802	+	_, r := actionRun(t, files{}, " - uses: o/x@v1\n", func(c *Config) { c.Actions.Resolver = res })
	803	+	if r.Conclusion != ConclusionSuccess {
	804	+	t.Fatalf("%+v", r)
	805	+	}
	806	+	want := []hit{{"/start", "set"}, {"/first", ""}, {"/second", ""}, {"/back", ""}}
	807	+	mu.Lock()
	808	+	defer mu.Unlock()
	809	+	if len(hits) != len(want) {
	810	+	t.Fatalf("hits %v", hits)
	811	+	}
	812	+	for i, h := range hits {
	813	+	if h.path != want[i].path \|\| (h.auth != "") != (want[i].auth != "") {
	814	+	t.Errorf("hit %d: %+v, want %+v (auth %q)", i, h, want[i], h.auth)
	815	+	}
	816	+	}
	817	+	}


@@ -12,6 +12,7 @@ import (
12	"os"	12		"os"
13	"path/filepath"	13		"path/filepath"
14	"strings"	14		"strings"
		15	+	"sync"
15	"sync/atomic"	16		"sync/atomic"
16	"testing"	17		"testing"
17	"time"	18		"time"

@@ -758,3 +759,59 @@ func TestArchiveRedirectPolicy(t *testing.T) {
758	t.Error("redirect loops must stop")	759		t.Error("redirect loops must stop")
759	}	760		}
760	}	761		}
		762	+
		763	+	// GO-008: Go rebuilds the headers of a redirected request from the first
		764	+	// request, so a chain that leaves the origin and stays on the new one, or comes
		765	+	// back, would get the Authorization header again on the third request. Real
		766	+	// servers, real redirects.
		767	+	func TestArchiveRedirectChainNeverReauthorises(t *testing.T) {
		768	+	type hit struct{ path, auth string }
		769	+	var mu sync.Mutex
		770	+	var hits []hit
		771	+	record := func(r *http.Request) {
		772	+	mu.Lock()
		773	+	hits = append(hits, hit{r.URL.Path, r.Header.Get("Authorization")})
		774	+	mu.Unlock()
		775	+	}
		776	+	var a, b *httptest.Server
		777	+	// a (the authorised origin) and b are two servers on the same host and
		778	+	// different ports: the case of Go's host-only comparison.
		779	+	a = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		780	+	record(r)
		781	+	switch r.URL.Path {
		782	+	case "/start":
		783	+	http.Redirect(w, r, b.URL+"/first", http.StatusFound)
		784	+	case "/back":
		785	+	_, _ = w.Write(tarOf(t, "o-x-1", files{"action.yml": nodeAction, "main.sh": "echo ok\n"}))
		786	+	}
		787	+	}))
		788	+	defer a.Close()
		789	+	b = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		790	+	record(r)
		791	+	switch r.URL.Path {
		792	+	case "/first":
		793	+	http.Redirect(w, r, b.URL+"/second", http.StatusFound)
		794	+	case "/second":
		795	+	http.Redirect(w, r, a.URL+"/back", http.StatusFound) // and back to the origin
		796	+	}
		797	+	}))
		798	+	defer b.Close()
		799	+	res := func(context.Context, []string) ([]ResolvedAction, error) {
		800	+	return []ResolvedAction{{Ref: "o/x@v1", NameWithOwner: "o/x", ResolvedSHA: strings.Repeat("e", 40), TarballURL: a.URL + "/start", Token: "tok", AuthOrigin: originOf(a.URL)}}, nil
		801	+	}
		802	+	_, r := actionRun(t, files{}, " - uses: o/x@v1\n", func(c *Config) { c.Actions.Resolver = res })
		803	+	if r.Conclusion != ConclusionSuccess {
		804	+	t.Fatalf("%+v", r)
		805	+	}
		806	+	want := []hit{{"/start", "set"}, {"/first", ""}, {"/second", ""}, {"/back", ""}}
		807	+	mu.Lock()
		808	+	defer mu.Unlock()
		809	+	if len(hits) != len(want) {
		810	+	t.Fatalf("hits %v", hits)
		811	+	}
		812	+	for i, h := range hits {
		813	+	if h.path != want[i].path \|\| (h.auth != "") != (want[i].auth != "") {
		814	+	t.Errorf("hit %d: %+v, want %+v (auth %q)", i, h, want[i], h.auth)
		815	+	}
		816	+	}
		817	+	}