Skip to content

Commit

Permalink
fix: Preserve HTTP Response in URL Errors (#3369)
Browse files Browse the repository at this point in the history
Fixes: #3362.
  • Loading branch information
silasalberti authored Dec 8, 2024
1 parent cea0bba commit f0bfdaf
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 5 deletions.
13 changes: 8 additions & 5 deletions github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -841,28 +841,31 @@ func (c *Client) BareDo(ctx context.Context, req *http.Request) (*Response, erro
}

resp, err := c.client.Do(req)
var response *Response
if resp != nil {
response = newResponse(resp)
}

if err != nil {
// If we got an error, and the context has been canceled,
// the context's error is probably more useful.
select {
case <-ctx.Done():
return nil, ctx.Err()
return response, ctx.Err()
default:
}

// If the error type is *url.Error, sanitize its URL before returning.
if e, ok := err.(*url.Error); ok {
if url, err := url.Parse(e.URL); err == nil {
e.URL = sanitizeURL(url).String()
return nil, e
return response, e
}
}

return nil, err
return response, err
}

response := newResponse(resp)

// Don't update the rate limits if this was a cached response.
// X-From-Cache is set by https://github.com/gregjones/httpcache
if response.Header.Get("X-From-Cache") == "" {
Expand Down
45 changes: 45 additions & 0 deletions github/github_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1106,6 +1106,51 @@ func TestDo_redirectLoop(t *testing.T) {
}
}

func TestDo_preservesResponseInHTTPError(t *testing.T) {
t.Parallel()
client, mux, _ := setup(t)

mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusNotFound)
fmt.Fprintf(w, `{
"message": "Resource not found",
"documentation_url": "https://docs.github.com/rest/reference/repos#get-a-repository"
}`)
})

req, _ := client.NewRequest("GET", ".", nil)
var resp *Response
var data interface{}
resp, err := client.Do(context.Background(), req, &data)

if err == nil {
t.Fatal("Expected error response")
}

// Verify error type and access to status code
errResp, ok := err.(*ErrorResponse)
if !ok {
t.Fatalf("Expected *ErrorResponse error, got %T", err)
}

// Verify status code is accessible from both Response and ErrorResponse
if resp == nil {
t.Fatal("Expected response to be returned even with error")
}
if got, want := resp.StatusCode, http.StatusNotFound; got != want {
t.Errorf("Response status = %d, want %d", got, want)
}
if got, want := errResp.Response.StatusCode, http.StatusNotFound; got != want {
t.Errorf("Error response status = %d, want %d", got, want)
}

// Verify error contains proper message
if !strings.Contains(errResp.Message, "Resource not found") {
t.Errorf("Error message = %q, want to contain 'Resource not found'", errResp.Message)
}
}

// Test that an error caused by the internal http client's Do() function
// does not leak the client secret.
func TestDo_sanitizeURL(t *testing.T) {
Expand Down

0 comments on commit f0bfdaf

Please sign in to comment.