Skip to content

Commit e6dfcd3

Browse files
babakksBagToad
babakks
and
BagToad
committed
fix: use separate http client for non-github hosts
Signed-off-by: Babak K. Shandiz <babakks@github.com> Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com>
1 parent 6bbaae0 commit e6dfcd3

25 files changed

Lines changed: 294 additions & 143 deletions

File tree

‎api/http_client.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,50 @@ func NewHTTPClient(opts HTTPClientOptions) (*http.Client, error) {
8686
return client, nil
8787
}
8888

89+
// ExternalHTTPClientOptions holds options for creating an external HTTP client.
90+
type ExternalHTTPClientOptions struct {
91+
AppVersion string
92+
Log io.Writer
93+
LogColorize bool
94+
Transport http.RoundTripper
95+
}
96+
97+
// NewExternalHTTPClient creates an HTTP client for talking to non-GitHub hosts.
98+
// It includes debug logging and a User-Agent header but does not attach any
99+
// authentication tokens or GitHub-specific headers.
100+
func NewExternalHTTPClient(opts ExternalHTTPClientOptions) (*http.Client, error) {
101+
clientOpts := ghAPI.ClientOptions{
102+
Host: "none",
103+
AuthToken: "none",
104+
LogIgnoreEnv: true,
105+
SkipDefaultHeaders: true,
106+
Transport: opts.Transport,
107+
}
108+
109+
debugEnabled, debugValue := utils.IsDebugEnabled()
110+
logVerboseHTTP := false
111+
if strings.Contains(debugValue, "api") {
112+
logVerboseHTTP = true
113+
}
114+
115+
if logVerboseHTTP || debugEnabled {
116+
clientOpts.Log = opts.Log
117+
clientOpts.LogColorize = opts.LogColorize
118+
clientOpts.LogVerboseHTTP = logVerboseHTTP
119+
}
120+
121+
clientOpts.Headers = map[string]string{
122+
userAgent: fmt.Sprintf("GitHub CLI %s", opts.AppVersion),
123+
}
124+
125+
client, err := ghAPI.NewHTTPClient(clientOpts)
126+
if err != nil {
127+
return nil, err
128+
}
129+
130+
return client, nil
131+
}
132+
89133
func NewCachedHTTPClient(httpClient *http.Client, ttl time.Duration) *http.Client {
90134
newClient := *httpClient
91135
newClient.Transport = AddCacheTTLHeader(httpClient.Transport, ttl)

‎api/http_client_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -381,6 +381,55 @@ func TestNewHTTPClientWithoutTelemetryDisabler(t *testing.T) {
381381
assert.Equal(t, 204, res.StatusCode)
382382
}
383383

384+
func TestNewExternalHTTPClient(t *testing.T) {
385+
tests := []struct {
386+
name string
387+
url string
388+
}{
389+
{
390+
name: "third-party host",
391+
url: "https://example.com/path",
392+
},
393+
{
394+
// Even when talking to GitHub, the external client must not set
395+
// authorization or any GitHub-specific headers.
396+
name: "github.com host",
397+
url: "https://api.github.com/repos/cli/cli",
398+
},
399+
}
400+
401+
for _, tt := range tests {
402+
t.Run(tt.name, func(t *testing.T) {
403+
var gotReq *http.Request
404+
transport := &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) {
405+
gotReq = req
406+
return &http.Response{StatusCode: 204, Body: io.NopCloser(strings.NewReader(""))}, nil
407+
}}
408+
409+
client, err := NewExternalHTTPClient(ExternalHTTPClientOptions{
410+
AppVersion: "v1.2.3",
411+
Transport: transport,
412+
})
413+
require.NoError(t, err)
414+
415+
req, err := http.NewRequest("GET", tt.url, nil)
416+
require.NoError(t, err)
417+
418+
res, err := client.Do(req)
419+
require.NoError(t, err)
420+
assert.Equal(t, 204, res.StatusCode)
421+
422+
// No headers should be set by default, except for User-Agent which should include the app version.
423+
assert.Equal(t, []string{"GitHub CLI v1.2.3"}, gotReq.Header.Values("user-agent"))
424+
assert.Empty(t, gotReq.Header.Values("authorization"))
425+
assert.Empty(t, gotReq.Header.Values("x-github-api-version"))
426+
assert.Empty(t, gotReq.Header.Values("accept"))
427+
assert.Empty(t, gotReq.Header.Values("content-type"))
428+
assert.Empty(t, gotReq.Header.Values("time-zone"))
429+
})
430+
}
431+
}
432+
384433
type fakeTelemetryDisabler struct {
385434
disabled bool
386435
}

‎internal/codespaces/api/api.go‎

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -60,10 +60,11 @@ const (
6060

6161
// API is the interface to the codespace service.
6262
type API struct {
63-
client func() (*http.Client, error)
64-
githubAPI string
65-
githubServer string
66-
retryBackoff time.Duration
63+
client func() (*http.Client, error)
64+
externalClient func() (*http.Client, error)
65+
githubAPI string
66+
githubServer string
67+
retryBackoff time.Duration
6768
}
6869

6970
// New creates a new API client connecting to the configured endpoints with the HTTP client.
@@ -93,10 +94,11 @@ func New(f *cmdutil.Factory) *API {
9394
}
9495

9596
return &API{
96-
client: f.HttpClient,
97-
githubAPI: strings.TrimSuffix(apiURL, "/"),
98-
githubServer: strings.TrimSuffix(serverURL, "/"),
99-
retryBackoff: 100 * time.Millisecond,
97+
client: f.HttpClient,
98+
externalClient: f.ExternalHttpClient,
99+
githubAPI: strings.TrimSuffix(apiURL, "/"),
100+
githubServer: strings.TrimSuffix(serverURL, "/"),
101+
retryBackoff: 100 * time.Millisecond,
100102
}
101103
}
102104

@@ -1214,12 +1216,8 @@ func (a *API) withRetry(f func() (*http.Response, error)) (*http.Response, error
12141216
}, backoff.WithMaxRetries(bo, 3))
12151217
}
12161218

1217-
// HTTPClient returns the HTTP client used to make requests to the API.
1218-
func (a *API) HTTPClient() (*http.Client, error) {
1219-
httpClient, err := a.client()
1220-
if err != nil {
1221-
return nil, err
1222-
}
1223-
1224-
return httpClient, nil
1219+
// ExternalHTTPClient returns an HTTP client for requests to non-GitHub hosts.
1220+
// It must not carry GitHub authentication credentials.
1221+
func (a *API) ExternalHTTPClient() (*http.Client, error) {
1222+
return a.externalClient()
12251223
}

‎internal/codespaces/codespaces.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ func connectionReady(codespace *api.Codespace) bool {
3939
type apiClient interface {
4040
GetCodespace(ctx context.Context, name string, includeConnection bool) (*api.Codespace, error)
4141
StartCodespace(ctx context.Context, name string) error
42-
HTTPClient() (*http.Client, error)
42+
ExternalHTTPClient() (*http.Client, error)
4343
}
4444

4545
type progressIndicator interface {
@@ -66,12 +66,12 @@ func GetCodespaceConnection(ctx context.Context, progress progressIndicator, api
6666
progress.StartProgressIndicatorWithLabel("Connecting to codespace")
6767
defer progress.StopProgressIndicator()
6868

69-
httpClient, err := apiClient.HTTPClient()
69+
externalHttpClient, err := apiClient.ExternalHTTPClient()
7070
if err != nil {
7171
return nil, fmt.Errorf("error getting http client: %w", err)
7272
}
7373

74-
return connection.NewCodespaceConnection(ctx, codespace, httpClient)
74+
return connection.NewCodespaceConnection(ctx, codespace, externalHttpClient)
7575
}
7676

7777
// waitUntilCodespaceConnectionReady waits for a Codespace to be running and is able to be connected to.

‎internal/codespaces/codespaces_test.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -202,8 +202,8 @@ func (m *mockApiClient) GetCodespace(ctx context.Context, name string, includeCo
202202
return m.onGetCodespace()
203203
}
204204

205-
func (m *mockApiClient) HTTPClient() (*http.Client, error) {
206-
panic("Not implemented")
205+
func (m *mockApiClient) ExternalHTTPClient() (*http.Client, error) {
206+
return nil, nil
207207
}
208208

209209
type mockProgressIndicator struct{}

‎pkg/cmd/attestation/api/client.go‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"io"
77
"net/http"
8+
neturl "net/url"
89
"strings"
910
"time"
1011

@@ -67,18 +68,18 @@ type Client interface {
6768
}
6869

6970
type LiveClient struct {
70-
githubAPI githubApiClient
71-
httpClient httpClient
72-
host string
73-
logger *ioconfig.Handler
71+
githubAPI githubApiClient
72+
externalHttpClient httpClient
73+
host string
74+
logger *ioconfig.Handler
7475
}
7576

76-
func NewLiveClient(hc *http.Client, host string, l *ioconfig.Handler) *LiveClient {
77+
func NewLiveClient(hc *http.Client, externalClient *http.Client, host string, l *ioconfig.Handler) *LiveClient {
7778
return &LiveClient{
78-
githubAPI: api.NewClientFromHTTP(hc),
79-
host: strings.TrimSuffix(host, "/"),
80-
httpClient: hc,
81-
logger: l,
79+
githubAPI: api.NewClientFromHTTP(hc),
80+
host: strings.TrimSuffix(host, "/"),
81+
externalHttpClient: externalClient,
82+
logger: l,
8283
}
8384
}
8485

@@ -121,7 +122,7 @@ func (c *LiveClient) buildRequestURL(params FetchParams) (string, error) {
121122
// ref: https://github.com/cli/go-gh/blob/d32c104a9a25c9de3d7c7b07a43ae0091441c858/example_gh_test.go#L96
122123
url = fmt.Sprintf("%s?per_page=%d", url, perPage)
123124
if params.PredicateType != "" {
124-
url = fmt.Sprintf("%s&predicate_type=%s", url, params.PredicateType)
125+
url = fmt.Sprintf("%s&predicate_type=%s", url, neturl.QueryEscape(params.PredicateType))
125126
}
126127
return url, nil
127128
}
@@ -225,7 +226,7 @@ func (c *LiveClient) getBundle(url string) (*bundle.Bundle, error) {
225226
var sgBundle *bundle.Bundle
226227
bo := backoff.NewConstantBackOff(getAttestationRetryInterval)
227228
err := backoff.Retry(func() error {
228-
resp, err := c.httpClient.Get(url)
229+
resp, err := c.externalHttpClient.Get(url)
229230
if err != nil {
230231
return fmt.Errorf("request to fetch bundle from URL failed: %w", err)
231232
}

‎pkg/cmd/attestation/api/client_test.go‎

Lines changed: 26 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,17 @@ func NewClientWithMockGHClient(hasNextPage bool) Client {
2828
githubAPI: mockAPIClient{
2929
OnRESTWithNext: fetcher.OnRESTSuccessWithNextPage,
3030
},
31-
httpClient: httpClient,
32-
logger: l,
31+
externalHttpClient: httpClient,
32+
logger: l,
3333
}
3434
}
3535

3636
return &LiveClient{
3737
githubAPI: mockAPIClient{
3838
OnRESTWithNext: fetcher.OnRESTSuccess,
3939
},
40-
httpClient: httpClient,
41-
logger: l,
40+
externalHttpClient: httpClient,
41+
logger: l,
4242
}
4343
}
4444

@@ -137,8 +137,8 @@ func TestGetByDigest_NoAttestationsFound(t *testing.T) {
137137
githubAPI: mockAPIClient{
138138
OnRESTWithNext: fetcher.OnRESTWithNextNoAttestations,
139139
},
140-
httpClient: httpClient,
141-
logger: io.NewTestHandler(),
140+
externalHttpClient: httpClient,
141+
logger: io.NewTestHandler(),
142142
}
143143

144144
attestations, err := c.GetByDigest(testFetchParamsWithRepo)
@@ -167,8 +167,8 @@ func TestGetByDigest_Error(t *testing.T) {
167167
func TestFetchBundleFromAttestations_BundleURL(t *testing.T) {
168168
httpClient := &mockHttpClient{}
169169
client := LiveClient{
170-
httpClient: httpClient,
171-
logger: io.NewTestHandler(),
170+
externalHttpClient: httpClient,
171+
logger: io.NewTestHandler(),
172172
}
173173

174174
att1 := makeTestAttestation()
@@ -184,8 +184,8 @@ func TestFetchBundleFromAttestations_BundleURL(t *testing.T) {
184184
func TestFetchBundleFromAttestations_MissingBundleAndBundleURLFields(t *testing.T) {
185185
httpClient := &mockHttpClient{}
186186
client := LiveClient{
187-
httpClient: httpClient,
188-
logger: io.NewTestHandler(),
187+
externalHttpClient: httpClient,
188+
logger: io.NewTestHandler(),
189189
}
190190

191191
// If both the BundleURL and Bundle fields are empty, the function should
@@ -207,8 +207,8 @@ func TestFetchBundleFromAttestations_FailOnTheSecondAttestation(t *testing.T) {
207207
}
208208

209209
c := &LiveClient{
210-
httpClient: mockHTTPClient,
211-
logger: io.NewTestHandler(),
210+
externalHttpClient: mockHTTPClient,
211+
logger: io.NewTestHandler(),
212212
}
213213

214214
att1 := makeTestAttestation()
@@ -223,8 +223,8 @@ func TestFetchBundleFromAttestations_FailAfterRetrying(t *testing.T) {
223223
mockHTTPClient := &reqFailHttpClient{}
224224

225225
c := &LiveClient{
226-
httpClient: mockHTTPClient,
227-
logger: io.NewTestHandler(),
226+
externalHttpClient: mockHTTPClient,
227+
logger: io.NewTestHandler(),
228228
}
229229

230230
a := makeTestAttestation()
@@ -239,8 +239,8 @@ func TestFetchBundleFromAttestations_FallbackToBundleField(t *testing.T) {
239239
mockHTTPClient := &mockHttpClient{}
240240

241241
c := &LiveClient{
242-
httpClient: mockHTTPClient,
243-
logger: io.NewTestHandler(),
242+
externalHttpClient: mockHTTPClient,
243+
logger: io.NewTestHandler(),
244244
}
245245

246246
// If the bundle URL is empty, the code will fallback to the bundle field
@@ -257,8 +257,8 @@ func TestGetBundle(t *testing.T) {
257257
mockHTTPClient := &mockHttpClient{}
258258

259259
c := &LiveClient{
260-
httpClient: mockHTTPClient,
261-
logger: io.NewTestHandler(),
260+
externalHttpClient: mockHTTPClient,
261+
logger: io.NewTestHandler(),
262262
}
263263

264264
b, err := c.getBundle("https://mybundleurl.com")
@@ -276,8 +276,8 @@ func TestGetBundle_SuccessfulRetry(t *testing.T) {
276276
}
277277

278278
c := &LiveClient{
279-
httpClient: mockHTTPClient,
280-
logger: io.NewTestHandler(),
279+
externalHttpClient: mockHTTPClient,
280+
logger: io.NewTestHandler(),
281281
}
282282

283283
b, err := c.getBundle("mybundleurl")
@@ -290,8 +290,8 @@ func TestGetBundle_SuccessfulRetry(t *testing.T) {
290290
func TestGetBundle_PermanentBackoffFail(t *testing.T) {
291291
mockHTTPClient := &invalidBundleClient{}
292292
c := &LiveClient{
293-
httpClient: mockHTTPClient,
294-
logger: io.NewTestHandler(),
293+
externalHttpClient: mockHTTPClient,
294+
logger: io.NewTestHandler(),
295295
}
296296

297297
b, err := c.getBundle("mybundleurl")
@@ -307,8 +307,8 @@ func TestGetBundle_RequestFail(t *testing.T) {
307307
mockHTTPClient := &reqFailHttpClient{}
308308

309309
c := &LiveClient{
310-
httpClient: mockHTTPClient,
311-
logger: io.NewTestHandler(),
310+
externalHttpClient: mockHTTPClient,
311+
logger: io.NewTestHandler(),
312312
}
313313

314314
b, err := c.getBundle("mybundleurl")
@@ -360,8 +360,8 @@ func TestGetAttestationsRetries(t *testing.T) {
360360
githubAPI: mockAPIClient{
361361
OnRESTWithNext: fetcher.FlakyOnRESTSuccessWithNextPageHandler(),
362362
},
363-
httpClient: &mockHttpClient{},
364-
logger: io.NewTestHandler(),
363+
externalHttpClient: &mockHttpClient{},
364+
logger: io.NewTestHandler(),
365365
}
366366

367367
testFetchParamsWithRepo.Limit = 30

0 commit comments

Comments
 (0)