From 40e627b462c673198d2cd586c9b454a483df92f4 Mon Sep 17 00:00:00 2001 From: mohammadmseet-hue Date: Sun, 19 Apr 2026 05:06:18 +0200 Subject: [PATCH] security: reject cross-host redirects to prevent Authorization leak The client manually follows HTTP 301 redirects in two places by extracting the Location header (or the RedirectionError.Location field) and recursively issuing a new request through the client's auth transport. Because the new request reuses the same transport, any Authorization header configured via WithAuthToken is also sent to the redirect target. Go's net/http.Client strips Authorization headers on cross-domain redirects by default (see https://github.com/golang/go/issues/35104), but the custom redirect handling in roundTripWithOptionalFollowRedirect and bareDoUntilFound bypasses that protection. A compromised GitHub Enterprise Server, a MitM on an HTTP GHE link, or any API response that can inject a Location header can therefore redirect the client to an attacker-controlled host and capture the user's Bearer token. This change validates that the redirect target's host matches the client's configured BaseURL.Host before following the redirect. Same-host redirects (the normal rate-limit-redirection case) are unaffected; cross-host targets are rejected with a descriptive error. Relative Location values continue to resolve against BaseURL as before. Tests cover both the cross-host rejection and the same-host follow path in roundTripWithOptionalFollowRedirect, plus cross-host rejection in bareDoUntilFound. --- github/github.go | 30 ++++++++++++++++++ github/github_test.go | 74 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 insertions(+) diff --git a/github/github.go b/github/github.go index fc2db27de8c..6594412eb06 100644 --- a/github/github.go +++ b/github/github.go @@ -1090,6 +1090,12 @@ func (c *Client) bareDoUntilFound(ctx context.Context, req *http.Request, maxRed return nil, nil, errInvalidLocation } newURL := c.BaseURL.ResolveReference(rerr.Location) + // Refuse to follow a permanent redirect to a different host: + // req.Clone preserves Authorization headers added by the auth + // transport, so a cross-host target would leak credentials. + if newURL.Host != c.BaseURL.Host { + return nil, response, fmt.Errorf("refusing to follow cross-host redirect from %q to %q", c.BaseURL.Host, newURL.Host) + } newRequest := req.Clone(ctx) newRequest.URL = newURL return c.bareDoUntilFound(ctx, newRequest, maxRedirects-1) @@ -1846,11 +1852,35 @@ func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u stri if maxRedirects > 0 && resp.StatusCode == http.StatusMovedPermanently { _ = resp.Body.Close() u = resp.Header.Get("Location") + if err := c.checkRedirectHost(u); err != nil { + return nil, err + } resp, err = c.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects-1, opts...) } return resp, err } +// checkRedirectHost returns an error if the redirect target is on a different +// host than the client's configured BaseURL. This prevents credentials attached +// by the auth transport from being sent to an attacker-controlled host when a +// compromised or malicious API response returns a cross-origin Location header. +// An empty Location is also rejected. +func (c *Client) checkRedirectHost(location string) error { + if location == "" { + return errInvalidLocation + } + target, err := url.Parse(location) + if err != nil { + return fmt.Errorf("invalid redirect location %q: %w", location, err) + } + // Resolve relative locations against BaseURL so relative paths are allowed. + target = c.BaseURL.ResolveReference(target) + if target.Host != c.BaseURL.Host { + return fmt.Errorf("refusing to follow cross-host redirect from %q to %q", c.BaseURL.Host, target.Host) + } + return nil +} + // Ptr is a helper routine that allocates a new T value // to store v and returns a pointer to it. func Ptr[T any](v T) *T { diff --git a/github/github_test.go b/github/github_test.go index f1584b2794c..d659b594c78 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -2256,6 +2256,80 @@ func TestBareDoUntilFound_UnexpectedRedirection(t *testing.T) { } } +// TestBareDoUntilFound_RejectsCrossHostRedirect verifies that bareDoUntilFound +// refuses to follow a 301 redirect whose Location points to a different host, +// which would otherwise leak the Authorization header (added by the auth +// transport) to an attacker-controlled server. +func TestBareDoUntilFound_RejectsCrossHostRedirect(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", "https://evil.example.com/steal") + w.WriteHeader(http.StatusMovedPermanently) + }) + + req, _ := client.NewRequest("GET", ".", nil) + _, _, err := client.bareDoUntilFound(t.Context(), req, 1) + if err == nil { + t.Fatal("Expected cross-host redirect to be rejected, got nil error.") + } + if !strings.Contains(err.Error(), "cross-host redirect") { + t.Errorf("Expected cross-host redirect error, got: %v", err) + } +} + +// TestRoundTripWithOptionalFollowRedirect_RejectsCrossHostRedirect verifies +// that roundTripWithOptionalFollowRedirect refuses to follow a 301 redirect to +// a different host, preventing Authorization-header leakage to attacker- +// controlled servers via a malicious or compromised API response. +func TestRoundTripWithOptionalFollowRedirect_RejectsCrossHostRedirect(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", "https://evil.example.com/steal") + w.WriteHeader(http.StatusMovedPermanently) + }) + + _, err := client.roundTripWithOptionalFollowRedirect(t.Context(), ".", 1) + if err == nil { + t.Fatal("Expected cross-host redirect to be rejected, got nil error.") + } + if !strings.Contains(err.Error(), "cross-host redirect") { + t.Errorf("Expected cross-host redirect error, got: %v", err) + } +} + +// TestRoundTripWithOptionalFollowRedirect_AllowsSameHostRedirect ensures the +// cross-host check does not break legitimate same-host 301 follow behavior +// (the path that rate-limit redirection relies on). +func TestRoundTripWithOptionalFollowRedirect_AllowsSameHostRedirect(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + var followed atomic.Bool + mux.HandleFunc("/archive", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Location", baseURLPath+"/archive-target") + w.WriteHeader(http.StatusMovedPermanently) + }) + mux.HandleFunc("/archive-target", func(w http.ResponseWriter, _ *http.Request) { + followed.Store(true) + w.WriteHeader(http.StatusOK) + }) + + resp, err := client.roundTripWithOptionalFollowRedirect(t.Context(), "archive", 2) + if err != nil { + t.Fatalf("Unexpected error on same-host redirect: %v", err) + } + if resp != nil && resp.Body != nil { + resp.Body.Close() + } + if !followed.Load() { + t.Error("Expected same-host redirect to be followed.") + } +} + func TestSanitizeURL(t *testing.T) { t.Parallel() tests := []struct {