diff --git a/internal/gitprovider/github/app_auth.go b/internal/gitprovider/github/app_auth.go index c2bd1b9..d29056d 100644 --- a/internal/gitprovider/github/app_auth.go +++ b/internal/gitprovider/github/app_auth.go @@ -70,7 +70,7 @@ func newGitHubAppFromConfig(ctx context.Context, profile string, store credentia } httpClient := opts.HTTPClient if httpClient == nil { - httpClient = http.DefaultClient + httpClient = defaultBoundedHTTPClient() } now := opts.Now if now == nil { diff --git a/internal/gitprovider/github/client.go b/internal/gitprovider/github/client.go index 586fe6b..06a9be6 100644 --- a/internal/gitprovider/github/client.go +++ b/internal/gitprovider/github/client.go @@ -20,6 +20,11 @@ const ( credentialTypePAT = "pat" credentialTypeGitHubApp = "github_app" // #nosec G101 -- credential type label, not a secret. gitHubAppInitTimeout = 30 * time.Second + // defaultHTTPTimeout bounds every REST/GraphQL request made with the + // default client. All responses here are JSON payloads, so two minutes is + // generous — the bound exists because an unbounded client parks the whole + // run indefinitely on a connection the server's edge silently abandoned. + defaultHTTPTimeout = 2 * time.Minute gitHubAppRefreshSkew = 5 * time.Minute gitHubAppJWTBackdate = 60 * time.Second gitHubAppJWTLifetime = 9 * time.Minute @@ -135,7 +140,7 @@ func New(opts Options) (*Client, error) { } httpClient := opts.HTTPClient if httpClient == nil { - httpClient = http.DefaultClient + httpClient = defaultBoundedHTTPClient() } return &Client{ host: normalizedHost, @@ -146,6 +151,14 @@ func New(opts Options) (*Client, error) { }, nil } +// defaultBoundedHTTPClient is the fallback for construction paths where the +// caller supplies no HTTPClient. Never http.DefaultClient: it has no timeout, +// so a connection the server's edge silently abandons would park the request +// — and the whole run — indefinitely. +func defaultBoundedHTTPClient() *http.Client { + return &http.Client{Timeout: defaultHTTPTimeout} +} + // Host returns the normalized host this client is bound to. func (c *Client) Host() string { if c == nil { diff --git a/internal/gitprovider/github/client_test.go b/internal/gitprovider/github/client_test.go index 44e2b25..5bf29f1 100644 --- a/internal/gitprovider/github/client_test.go +++ b/internal/gitprovider/github/client_test.go @@ -31,6 +31,23 @@ func TestCapabilities(t *testing.T) { } } +func TestDefaultHTTPClientIsBounded(t *testing.T) { + // A stalled connection must never park a run indefinitely: the fallback + // client (used by both the PAT and GitHub-App construction paths whenever + // the caller doesn't supply one) needs a timeout. + fallback := defaultBoundedHTTPClient() + if fallback.Timeout <= 0 { + t.Fatalf("default HTTP client has no timeout; a silently dropped connection would hang forever") + } + if fallback == http.DefaultClient { + t.Fatalf("default HTTP client must not be http.DefaultClient (shared, unbounded)") + } + client := mustClient(t, Options{Token: "token"}) + if client.httpClient.Timeout <= 0 { + t.Fatalf("NewClient without an explicit HTTPClient must use the bounded fallback") + } +} + func TestNewFromGitConfigBuildsPATClientAndCredential(t *testing.T) { store := tokenStore{"work": {credentials.GitTokenKey: "token"}} client, credential, err := NewFromGitConfig(config.GitConfig{