Skip to content

Unsafe concurrent modification of http.DefaultClient #1173

@ktravis

Description

@ktravis

When the RepositoriesService.DownloadReleaseAsset method is called, the http.Client pointed to by github.Client is modified in order to temporarily override http.Client.CheckRedirect for the span of a single request. A sync.Mutex is used on the github.Client struct to prevent multiple concurrent modifications by the same instance of github.Client - however, if multiple instances refer to the same http.Client and each call this method, it becomes unsafe.

The intent may be that each github.Client receives its own http.Client to mitigate this type of issue, but the fact that github.NewClient specifically defaults to using http.DefaultClient when one is not provided suggests otherwise. This not only affects multiple concurrent github.Client instances, it will also (potentially) impact any other code in the program making use of http.DefaultClient with no indication.

This behavior presented itself in a case where multiple clients were created to download release assets from different sources. The underlying http.Client passed to NewClient was nil in the case that authentication was not required, which led to the case above with http.DefaultClient being shared by multiple github clients. The ultimate effect was that redirect URLs returned by different method calls were mismatched (i.e. the request to download repo A release asset 1 instead returned a URL for downloading repo B's release asset 2), leading to data corruption.

Additionally, I did notice this method which performs a similar modification of the underlying http.Client without locking the client's mutex.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions