Passing the http.DefaultClient to a New client can lead to breakdowns

[aoterolorenzo]

! Not sure if I'm getting into a bad practice of this should be an expected behaviour

Instantiating a client with the Default http Golang client (client := github.NewClient(http.DefaultClient) leads to the perversion of the http.DefaultClient content, which when used over the rest of the code, starts breaking when for example using a Bearer token as Authorization: It will return 201, appearing the header not being included in the response.

func NewMyController() *MyController {
	return &MyController{
		client:    http.DefaultClient,
	        baseURL: "myBaseUrl",
	}
}

If after making any Github calls with a client := github.NewClient(http.DefaultClient), we run this code

req, _ := http.NewRequest(http.MethodGet, "myUrl", nil)

req.Header.Set("Authorization", "Bearer myToken")
resp, err := http.DefaultClient.Do(req)

fmt.Println(resp.StatusCode)

The http.DefaultClient will seem perverted since it will appear the Authorization header is not being correctly sent, as the retrieved StatusCode will be 201.

Extrainfo: Using go 1.21.4

[gmlewis]

You are saying that the actual http.DefaultClient is being corrupted and values within it are being overwritten?

[aoterolorenzo]

@gmlewis yes, exactly. That's the scenario I've encountered.

After instantiating a new github client (client := github.NewClient(http.DefaultClient)) and having performed some operations (specifically calls to workflow and jobs listings), other controller using the http.DefaultClient started failing with 201 responses (Using a Bearer Token auth, through the Authorization header).

Changing the http.DefaultClient to a new &http.Client{} or even just using that controller before the github instance, worked successfully. Also changing it on the github.NewClient() call using &http.Client{}, and leaving the other controller with the default one, worked.

That made me infer that the http.DefaultClient was being corrupted. Of course this have been debugged from the ground: Checked token, auth, etc... This actually took me several hours to get into the real issue.

Note: #1173 can be related.

[aoterolorenzo]

The operations executed with the github client where (at least) the http.DefaultClient is being supposedly corrupted are:

runs, resp, err := c.client.Actions.ListWorkflowRunsByID(ctx, GHOrg, GHChartsRepo, GHChartsWorkflowID, &opts)

jobs, _, err := c.client.Actions.ListWorkflowJobs(ctx, GHOrg, GHChartsRepo, *(run.ID), &github.ListWorkflowJobsOptions{})

url, _, err := c.client.Actions.GetWorkflowJobLogs(ctx, GHOrg, GHChartsRepo, *(job.ID), 2)

[aoterolorenzo]

Oh, and the client is instantiated with:

client := github.NewClient(http.DefaultClient).WithAuthToken("mytoken")

Which could be also a point where the issue is happening.

Hope it helps.

[aoterolorenzo]

Just saw https://github.com/google/go-github/blob/master/github/github.go#L318

func NewClient(httpClient *http.Client) *Client {
	c := &Client{client: httpClient}
	c.initialize()
	return c
}

Since both the client field and the httpClient are pointers, I think c its being instantiated with a direct reference here c := &Client{client: httpClient}. Then, in the initialize() function seems to be modified. Probably c.Authorizations = (*AuthorizationsService)(&c.common) is the key of the bearer auth problem later on using the client.

Probably making a value-clone, kind of:

httpClientValue := &httpClient
c := &Client{client: *httpClientValue}

would solve the issue (but we would be also missing the point of using a pointer injection on func NewClient(httpClient *http.Client), so not sure if that's the best solution: and changing it would be a hell of a breaking change).

Other option without breaking changes would be having a NewClient() constructor that would instantiate an empty client inside, declaring this issue as an expected behavior. I just saw NewClient(nil) calls out there but IMHO they don't seem very nice to see, so probably a NewClient() would be more concise to any user.

[gmlewis]

So first off, to immediately get to the bottom of this, please run your program again under delve the Go debugger, set a watch on http.DefaultClient and see what modifies it.

Once you have clearly identified the file and line number in this repo that is modifying http.DefaultClient, please create a minimal example main.go program that reliably reproduces the problem and post it here. We can then work on a solution.

[gmlewis]

I'm hoping this will be fixed by #3011.

[aoterolorenzo]

Sorry for the waiting. Here's the snippet:

func main() {
	github.NewClient(http.DefaultClient).WithAuthToken("myToken")

	req, _ := http.NewRequest(http.MethodGet, "myapiEndpoint", nil)
	req.Header.Set("Authorization", "Bearer myToken")
	resp, _ := http.DefaultClient.Do(req)
}

´http.DefaultClient.Transport´ pass from nil to github.com/google/go-github/v57/github.(*Client).WithAuthToken.func1´ when watching its value after the github.NewClient(http.DefaultClient).WithAuthToken(...)` call, and in my API (internal) I receive a 201 Status Code (can't say exactly why, trying other API's it feels that an Authentication header is still being sent, since response is "bad token" instead "no token". But in some way the client is corrupted.

[aoterolorenzo]

Working succesfully after forcing go.mod from commit a354a6c

github.com/google/go-github/v57 v57.0.1-0.20231218013855-a354a6c89032

Thank you, @gmlewis