diff --git a/github/github.go b/github/github.go index 233493fb1d1..419702207c7 100644 --- a/github/github.go +++ b/github/github.go @@ -167,6 +167,8 @@ var ErrPathForbidden = errors.New("path must not contain '..' due to auth vulner type Client struct { client *http.Client // HTTP client used to communicate with the API. clientIgnoreRedirects *http.Client // HTTP client used to communicate with the API on endpoints where we don't want to follow redirects. + transport http.RoundTripper + token *string // Base URL for API requests. Defaults to the public GitHub API, but can be // set to a domain endpoint to use with GitHub Enterprise. baseURL should @@ -255,8 +257,8 @@ type service struct { } // Client returns the http.Client used by this GitHub client. -// This should only be used for requests to the GitHub API because -// request headers will contain an authorization token. +// This should only be used for requests to the configured GitHub API or upload +// URLs because requests to those origins may contain an authorization token. func (c *Client) Client() *http.Client { clientCopy := *c.client return &clientCopy @@ -434,7 +436,8 @@ func WithEnvProxy() ClientOptionsFunc { } // WithAuthToken returns a ClientOptionsFunc that sets the authentication token -// for a Client. If not set, the client will make unauthenticated requests. +// for requests to the Client's configured API and upload URLs. If not set, the +// client will make unauthenticated requests. func WithAuthToken(token string) ClientOptionsFunc { return func(o *clientOptions) error { if token == "" { @@ -602,25 +605,6 @@ func newClient(opts clientOptions) (*Client, error) { c.client.Transport = t2 } - if opts.token != nil { - transport := c.client.Transport - if transport == nil { - transport = http.DefaultTransport - } - c.client.Transport = roundTripperFunc(func(req *http.Request) (*http.Response, error) { - req = req.Clone(req.Context()) - req.Header.Set("Authorization", fmt.Sprintf("Bearer %v", *opts.token)) - return transport.RoundTrip(req) - }) - } - - c.clientIgnoreRedirects = &http.Client{ - Transport: c.client.Transport, - Timeout: c.client.Timeout, - Jar: c.client.Jar, - CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, - } - if opts.apiVersionMin != nil { c.apiVersionMin = *opts.apiVersionMin } @@ -647,6 +631,33 @@ func newClient(opts clientOptions) (*Client, error) { c.uploadURL, _ = url.Parse(uploadBaseURL) } + c.transport = c.client.Transport + if opts.token != nil { + transport := c.client.Transport + if transport == nil { + transport = http.DefaultTransport + } + token := "Bearer " + *opts.token + baseURL := c.baseURL + uploadURL := c.uploadURL + c.token = Ptr(*opts.token) + c.transport = transport + c.client.Transport = roundTripperFunc(func(req *http.Request) (*http.Response, error) { + if shouldAuthorizeURL(req.URL, baseURL, uploadURL) { + req = req.Clone(req.Context()) + req.Header.Set("Authorization", token) + } + return transport.RoundTrip(req) + }) + } + + c.clientIgnoreRedirects = &http.Client{ + Transport: c.client.Transport, + Timeout: c.client.Timeout, + Jar: c.client.Jar, + CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }, + } + c.disableRateLimitCheck = opts.disableRateLimitCheck if !c.disableRateLimitCheck { @@ -705,6 +716,38 @@ func newClient(opts clientOptions) (*Client, error) { return c, nil } +func shouldAuthorizeURL(u, baseURL, uploadURL *url.URL) bool { + if u == nil { + return false + } + + return sameOrigin(u, baseURL) || sameOrigin(u, uploadURL) +} + +func sameOrigin(u, base *url.URL) bool { + if u == nil || base == nil { + return false + } + + return strings.EqualFold(u.Scheme, base.Scheme) && + strings.EqualFold(u.Hostname(), base.Hostname()) && + defaultPort(u) == defaultPort(base) +} + +func defaultPort(u *url.URL) string { + if port := u.Port(); port != "" { + return port + } + switch strings.ToLower(u.Scheme) { + case "http": + return "80" + case "https": + return "443" + default: + return "" + } +} + // UserAgent returns the User-Agent header value for the client. func (c *Client) UserAgent() string { return c.userAgent @@ -744,6 +787,7 @@ func (c *Client) Clone(opts ...ClientOptionsFunc) (*Client, error) { userAgent: &c.userAgent, baseURL: Ptr(*c.baseURL), uploadURL: Ptr(*c.uploadURL), + token: c.token, disableRateLimitCheck: c.disableRateLimitCheck, rateLimitRedirectionalEndpoints: c.rateLimitRedirectionalEndpoints, maxSecondaryRateLimitRetryAfterDuration: &c.maxSecondaryRateLimitRetryAfterDuration, @@ -760,8 +804,12 @@ func (c *Client) Clone(opts ...ClientOptionsFunc) (*Client, error) { } if o.httpClient == nil { + transport := c.client.Transport + if c.token != nil { + transport = c.transport + } o.httpClient = &http.Client{ - Transport: c.client.Transport, + Transport: transport, CheckRedirect: c.client.CheckRedirect, Jar: c.client.Jar, Timeout: c.client.Timeout, @@ -1366,11 +1414,11 @@ func (c *Client) bareDoUntilFound(req *http.Request, maxRedirects int) (*url.URL return nil, nil, errInvalidLocation } newURL := c.baseURL.ResolveReference(rerr.Location) - // Refuse to follow a permanent redirect to a different host: + // Refuse to follow a permanent redirect to a different origin: // 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) + // transport, so a cross-origin target would leak credentials. + if !sameOrigin(newURL, c.baseURL) { + return nil, response, fmt.Errorf("refusing to follow cross-origin redirect from %q to %q", c.baseURL.Host, newURL.Host) } newRequest := req.Clone(req.Context()) newRequest.URL = newURL @@ -2137,7 +2185,7 @@ 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 { + if err := c.checkRedirectOrigin(u); err != nil { return nil, err } resp, err = c.roundTripWithOptionalFollowRedirect(ctx, u, maxRedirects-1, opts...) @@ -2145,12 +2193,12 @@ func (c *Client) roundTripWithOptionalFollowRedirect(ctx context.Context, u stri 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 +// checkRedirectOrigin returns an error if the redirect target is on a different +// origin 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 { +func (c *Client) checkRedirectOrigin(location string) error { if location == "" { return errInvalidLocation } @@ -2160,8 +2208,8 @@ func (c *Client) checkRedirectHost(location string) error { } // 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) + if !sameOrigin(target, c.baseURL) { + return fmt.Errorf("refusing to follow cross-origin redirect from %q to %q", c.baseURL.Host, target.Host) } return nil } diff --git a/github/github_test.go b/github/github_test.go index 6d82a59bb1b..e5e300998f3 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -596,6 +596,168 @@ func TestWithAuthToken(t *testing.T) { }) } +func TestWithAuthTokenAuthorizesConfiguredHostsOnly(t *testing.T) { + t.Parallel() + + const token = "secret-token" + + trustedAuths := make(chan string, 2) + trusted := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + trustedAuths <- r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer trusted.Close() + + attackerAuths := make(chan string, 2) + attacker := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attackerAuths <- r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer attacker.Close() + + baseURL := trusted.URL + "/api/" + uploadURL := trusted.URL + "/upload/" + client := mustNewClient(t, WithURLs(&baseURL, &uploadURL), WithAuthToken(token)) + + req, err := client.NewRequest(t.Context(), "GET", "repos/o/r", nil) + if err != nil { + t.Fatalf("NewRequest returned unexpected error: %v", err) + } + resp, err := client.BareDo(req) + if err != nil { + t.Fatalf("BareDo returned unexpected error: %v", err) + } + resp.Body.Close() + + req, err = client.NewUploadRequest(t.Context(), "assets", strings.NewReader("x"), 1, "") + if err != nil { + t.Fatalf("NewUploadRequest returned unexpected error: %v", err) + } + resp, err = client.BareDo(req) + if err != nil { + t.Fatalf("BareDo returned unexpected error: %v", err) + } + resp.Body.Close() + + for range 2 { + if got, want := <-trustedAuths, "Bearer "+token; got != want { + t.Errorf("Authorization on configured host = %q, want %q", got, want) + } + } + + req, err = client.NewRequest(t.Context(), "GET", attacker.URL+"/repos/o/r", nil) + if err != nil { + t.Fatalf("NewRequest returned unexpected error: %v", err) + } + resp, err = client.BareDo(req) + if err != nil { + t.Fatalf("BareDo returned unexpected error: %v", err) + } + resp.Body.Close() + + req, err = client.NewUploadRequest(t.Context(), attacker.URL+"/assets", strings.NewReader("x"), 1, "") + if err != nil { + t.Fatalf("NewUploadRequest returned unexpected error: %v", err) + } + resp, err = client.BareDo(req) + if err != nil { + t.Fatalf("BareDo returned unexpected error: %v", err) + } + resp.Body.Close() + + for range 2 { + if got := <-attackerAuths; got != "" { + t.Errorf("Authorization on cross-origin request = %q, want empty", got) + } + } +} + +func TestWithAuthTokenCloneAuthorizesCloneHosts(t *testing.T) { + t.Parallel() + + const token = "clone-token" + + gotAuth := make(chan string, 1) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAuth <- r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{}`)) + })) + defer server.Close() + + client := mustNewClient(t, WithAuthToken(token)) + baseURL := server.URL + "/api/" + uploadURL := server.URL + "/upload/" + clone, err := client.Clone(WithURLs(&baseURL, &uploadURL)) + if err != nil { + t.Fatalf("Clone returned unexpected error: %v", err) + } + + req, err := clone.NewRequest(t.Context(), "GET", "repos/o/r", nil) + if err != nil { + t.Fatalf("NewRequest returned unexpected error: %v", err) + } + resp, err := clone.BareDo(req) + if err != nil { + t.Fatalf("BareDo returned unexpected error: %v", err) + } + resp.Body.Close() + + if got, want := <-gotAuth, "Bearer "+token; got != want { + t.Errorf("Authorization on clone configured host = %q, want %q", got, want) + } +} + +func TestShouldAuthorizeURL(t *testing.T) { + t.Parallel() + + baseURL := mustParseURL(t, "https://api.github.test:443/") + uploadURL := mustParseURL(t, "https://uploads.github.test/") + tests := []struct { + name string + url *url.URL + baseURL *url.URL + want bool + }{ + {name: "nil url"}, + {name: "base url", url: mustParseURL(t, "https://api.github.test/repos"), want: true}, + {name: "upload url", url: mustParseURL(t, "https://uploads.github.test/assets"), want: true}, + {name: "same origin case insensitive", url: mustParseURL(t, "HTTPS://API.GITHUB.TEST/repos"), want: true}, + { + name: "same http origin default port", + url: mustParseURL(t, "http://api.github.test/repos"), + baseURL: mustParseURL(t, "http://api.github.test:80/"), + want: true, + }, + { + name: "same non-http origin", + url: mustParseURL(t, "git://api.github.test/repos"), + baseURL: mustParseURL(t, "git://api.github.test/"), + want: true, + }, + {name: "different scheme", url: mustParseURL(t, "http://api.github.test/repos")}, + {name: "different host", url: mustParseURL(t, "https://example.test/repos")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + baseURL := baseURL + if tt.baseURL != nil { + baseURL = tt.baseURL + } + if got := shouldAuthorizeURL(tt.url, baseURL, uploadURL); got != tt.want { + t.Errorf("shouldAuthorizeURL() = %t, want %t", got, tt.want) + } + }) + } + + if shouldAuthorizeURL(mustParseURL(t, "https://api.github.test/repos"), nil, nil) { + t.Error("shouldAuthorizeURL() with nil configured origins = true, want false") + } +} + func TestWithURLs(t *testing.T) { t.Parallel() for _, tt := range []struct { @@ -3350,11 +3512,11 @@ func TestBareDoUntilFound_UnexpectedRedirection(t *testing.T) { } } -// TestBareDoUntilFound_RejectsCrossHostRedirect verifies that bareDoUntilFound -// refuses to follow a 301 redirect whose Location points to a different host, +// TestBareDoUntilFound_RejectsCrossOriginRedirect verifies that bareDoUntilFound +// refuses to follow a 301 redirect whose Location points to a different origin, // which would otherwise leak the Authorization header (added by the auth // transport) to an attacker-controlled server. -func TestBareDoUntilFound_RejectsCrossHostRedirect(t *testing.T) { +func TestBareDoUntilFound_RejectsCrossOriginRedirect(t *testing.T) { t.Parallel() client, mux, _ := setup(t) @@ -3366,18 +3528,18 @@ func TestBareDoUntilFound_RejectsCrossHostRedirect(t *testing.T) { req, _ := client.NewRequest(t.Context(), "GET", ".", nil) _, _, err := client.bareDoUntilFound(req, 1) if err == nil { - t.Fatal("Expected cross-host redirect to be rejected, got nil error.") + t.Fatal("Expected cross-origin 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) + if !strings.Contains(err.Error(), "cross-origin redirect") { + t.Errorf("Expected cross-origin redirect error, got: %v", err) } } -// TestRoundTripWithOptionalFollowRedirect_RejectsCrossHostRedirect verifies +// TestRoundTripWithOptionalFollowRedirect_RejectsCrossOriginRedirect verifies // that roundTripWithOptionalFollowRedirect refuses to follow a 301 redirect to -// a different host, preventing Authorization-header leakage to attacker- +// a different origin, preventing Authorization-header leakage to attacker- // controlled servers via a malicious or compromised API response. -func TestRoundTripWithOptionalFollowRedirect_RejectsCrossHostRedirect(t *testing.T) { +func TestRoundTripWithOptionalFollowRedirect_RejectsCrossOriginRedirect(t *testing.T) { t.Parallel() client, mux, _ := setup(t) @@ -3388,15 +3550,15 @@ func TestRoundTripWithOptionalFollowRedirect_RejectsCrossHostRedirect(t *testing _, err := client.roundTripWithOptionalFollowRedirect(t.Context(), ".", 1) if err == nil { - t.Fatal("Expected cross-host redirect to be rejected, got nil error.") + t.Fatal("Expected cross-origin 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) + if !strings.Contains(err.Error(), "cross-origin redirect") { + t.Errorf("Expected cross-origin redirect error, got: %v", err) } } // TestRoundTripWithOptionalFollowRedirect_AllowsSameHostRedirect ensures the -// cross-host check does not break legitimate same-host 301 follow behavior +// cross-origin check does not break legitimate same-origin 301 follow behavior // (the path that rate-limit redirection relies on). func TestRoundTripWithOptionalFollowRedirect_AllowsSameHostRedirect(t *testing.T) { t.Parallel()