From 2e174e39179e44b21233455e7cfc93855141cd39 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Fri, 14 Aug 2026 14:54:39 -0700 Subject: [PATCH] fix(http): bound leftover DefaultClient fetches Unauthenticated Gmail tracking, Photos downloads, Slides thumbnails, and Zoom API calls used http.DefaultClient and could hang forever on a silent peer. Reuse the existing 30s ResponseHeaderTimeout transport for those fetches, and give Zoom a 30s client timeout. Signed-off-by: Sebastien Tardif --- internal/cmd/gmail_track_opens.go | 4 +- internal/cmd/http_client.go | 9 ++ internal/cmd/http_client_test.go | 116 +++++++++++++++++++++++ internal/cmd/photos.go | 2 +- internal/cmd/slides_thumbnail.go | 2 +- internal/googleapi/client.go | 8 ++ internal/googleapi/client_more_test.go | 20 ++++ internal/googleapi/photos.go | 2 +- internal/googleapi/photos_client_test.go | 38 ++++++++ internal/googleapi/photos_picker.go | 2 +- internal/zoom/client.go | 2 +- internal/zoom/client_test.go | 20 ++++ 12 files changed, 218 insertions(+), 7 deletions(-) create mode 100644 internal/cmd/http_client.go create mode 100644 internal/cmd/http_client_test.go create mode 100644 internal/googleapi/photos_client_test.go diff --git a/internal/cmd/gmail_track_opens.go b/internal/cmd/gmail_track_opens.go index 50cf6c526..9eeb85bed 100644 --- a/internal/cmd/gmail_track_opens.go +++ b/internal/cmd/gmail_track_opens.go @@ -51,7 +51,7 @@ func (c *GmailTrackOpensCmd) queryByTrackingID(ctx context.Context, cfg *trackin return fmt.Errorf("build request: %w", err) } - resp, err := http.DefaultClient.Do(req) + resp, err := outboundHTTPClient.Do(req) if err != nil { return fmt.Errorf("query tracker: %w", err) } @@ -136,7 +136,7 @@ func (c *GmailTrackOpensCmd) queryAdmin(ctx context.Context, cfg *tracking.Confi req, _ := http.NewRequestWithContext(ctx, "GET", reqURL.String(), nil) req.Header.Set("Authorization", "Bearer "+cfg.AdminKey) - resp, err := http.DefaultClient.Do(req) + resp, err := outboundHTTPClient.Do(req) if err != nil { return fmt.Errorf("query tracker: %w", err) } diff --git a/internal/cmd/http_client.go b/internal/cmd/http_client.go new file mode 100644 index 000000000..d51fbd730 --- /dev/null +++ b/internal/cmd/http_client.go @@ -0,0 +1,9 @@ +package cmd + +import ( + "github.com/openclaw/gogcli/internal/googleapi" +) + +// outboundHTTPClient bounds response-header wait for unauthenticated +// fetches (tracking queries, media downloads, slide thumbnails). +var outboundHTTPClient = googleapi.NewBoundedHTTPClient() diff --git a/internal/cmd/http_client_test.go b/internal/cmd/http_client_test.go new file mode 100644 index 000000000..ff00fd4a4 --- /dev/null +++ b/internal/cmd/http_client_test.go @@ -0,0 +1,116 @@ +package cmd + +import ( + "context" + "errors" + "io" + "net/http" + "path/filepath" + "strings" + "testing" + + "github.com/openclaw/gogcli/internal/googleapi" + "github.com/openclaw/gogcli/internal/tracking" + "github.com/openclaw/gogcli/internal/ui" +) + +type stubRoundTripper struct { + fn func(*http.Request) (*http.Response, error) +} + +func (s stubRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + return s.fn(req) +} + +func swapOutboundHTTPClient(t *testing.T, client *http.Client) { + t.Helper() + old := outboundHTTPClient + outboundHTTPClient = client + t.Cleanup(func() { outboundHTTPClient = old }) +} + +func TestOutboundHTTPClientIsBounded(t *testing.T) { + if outboundHTTPClient == http.DefaultClient { + t.Fatal("outboundHTTPClient is http.DefaultClient") + } + tr, ok := outboundHTTPClient.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected *http.Transport, got %T", outboundHTTPClient.Transport) + } + if tr.ResponseHeaderTimeout == 0 { + t.Fatal("ResponseHeaderTimeout is 0") + } +} + +func TestQueryByTrackingIDUsesOutboundHTTPClient(t *testing.T) { + var saw string + swapOutboundHTTPClient(t, &http.Client{ + Transport: stubRoundTripper{fn: func(req *http.Request) (*http.Response, error) { + saw = req.URL.String() + return nil, errors.New("sentinel-outbound") + }}, + }) + + u, err := ui.New(ui.Options{Stdout: io.Discard, Stderr: io.Discard, Color: "never"}) + if err != nil { + t.Fatalf("ui.New: %v", err) + } + cmd := &GmailTrackOpensCmd{TrackingID: "tid-1"} + err = cmd.queryByTrackingID(context.Background(), &tracking.Config{WorkerURL: "http://tracker.example"}, u) + if err == nil || !strings.Contains(err.Error(), "sentinel-outbound") { + t.Fatalf("queryByTrackingID error = %v", err) + } + if saw != "http://tracker.example/q/tid-1" { + t.Fatalf("request URL = %q", saw) + } +} + +func TestQueryAdminUsesOutboundHTTPClient(t *testing.T) { + var sawAuth string + swapOutboundHTTPClient(t, &http.Client{ + Transport: stubRoundTripper{fn: func(req *http.Request) (*http.Response, error) { + sawAuth = req.Header.Get("Authorization") + return nil, errors.New("sentinel-outbound") + }}, + }) + + u, err := ui.New(ui.Options{Stdout: io.Discard, Stderr: io.Discard, Color: "never"}) + if err != nil { + t.Fatalf("ui.New: %v", err) + } + cmd := &GmailTrackOpensCmd{} + err = cmd.queryAdmin(context.Background(), &tracking.Config{WorkerURL: "http://tracker.example", AdminKey: "secret"}, u) + if err == nil || !strings.Contains(err.Error(), "sentinel-outbound") { + t.Fatalf("queryAdmin error = %v", err) + } + if sawAuth != "Bearer secret" { + t.Fatalf("Authorization = %q", sawAuth) + } +} + +func TestDownloadSlidesThumbnailUsesOutboundHTTPClient(t *testing.T) { + swapOutboundHTTPClient(t, &http.Client{ + Transport: stubRoundTripper{fn: func(req *http.Request) (*http.Response, error) { + return nil, errors.New("sentinel-outbound") + }}, + }) + + _, _, err := downloadSlidesThumbnail(context.Background(), "http://cdn.example/thumb.png", filepath.Join(t.TempDir(), "t.png"), true) + if err == nil || !strings.Contains(err.Error(), "sentinel-outbound") { + t.Fatalf("downloadSlidesThumbnail error = %v", err) + } +} + +func TestNewBoundedHTTPClientMatchesAuthenticatedTransportTimeout(t *testing.T) { + client := googleapi.NewBoundedHTTPClient() + if client == http.DefaultClient { + t.Fatal("NewBoundedHTTPClient returned DefaultClient") + } + tr, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected *http.Transport, got %T", client.Transport) + } + if tr.ResponseHeaderTimeout == 0 { + t.Fatal("ResponseHeaderTimeout is 0") + } +} diff --git a/internal/cmd/photos.go b/internal/cmd/photos.go index bd5da3683..f25f566df 100644 --- a/internal/cmd/photos.go +++ b/internal/cmd/photos.go @@ -176,7 +176,7 @@ func (c *PhotosDownloadCmd) Run(ctx context.Context, flags *RootFlags) error { if err != nil { return fmt.Errorf("build media download request: %w", err) } - resp, err := http.DefaultClient.Do(req) + resp, err := outboundHTTPClient.Do(req) if err != nil { return fmt.Errorf("download media item: %w", err) } diff --git a/internal/cmd/slides_thumbnail.go b/internal/cmd/slides_thumbnail.go index 10bcfaf27..deda7dbfa 100644 --- a/internal/cmd/slides_thumbnail.go +++ b/internal/cmd/slides_thumbnail.go @@ -156,7 +156,7 @@ func downloadSlidesThumbnail(ctx context.Context, url, outputPath string, overwr return 0, "", fmt.Errorf("build thumbnail download request: %w", err) } - resp, err := http.DefaultClient.Do(req) + resp, err := outboundHTTPClient.Do(req) if err != nil { return 0, "", fmt.Errorf("download thumbnail: %w", err) } diff --git a/internal/googleapi/client.go b/internal/googleapi/client.go index c95234fe7..ab9477d74 100644 --- a/internal/googleapi/client.go +++ b/internal/googleapi/client.go @@ -317,6 +317,14 @@ func newBaseTransport() *http.Transport { return transport } +// NewBoundedHTTPClient returns an unauthenticated client with the same +// ResponseHeaderTimeout used by authenticated Google clients. It does not +// set Client.Timeout so large downloads are not cut short after headers +// arrive. +func NewBoundedHTTPClient() *http.Client { + return &http.Client{Transport: newBaseTransport()} +} + // reauthFunctionFromContext builds a Reauth closure from the auth // dependencies stored in the context. Returns nil if the dependencies are // not available or the Reauth function is not configured, in which case diff --git a/internal/googleapi/client_more_test.go b/internal/googleapi/client_more_test.go index f1e5f785d..9e3717cf2 100644 --- a/internal/googleapi/client_more_test.go +++ b/internal/googleapi/client_more_test.go @@ -1202,6 +1202,26 @@ func TestNewBaseTransport_SetsResponseHeaderTimeout(t *testing.T) { } } +func TestNewBoundedHTTPClient_SetsResponseHeaderTimeout(t *testing.T) { + client := NewBoundedHTTPClient() + if client == http.DefaultClient { + t.Fatal("NewBoundedHTTPClient returned DefaultClient") + } + + transport, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected *http.Transport, got %T", client.Transport) + } + + if transport.ResponseHeaderTimeout != responseHeaderTimeout { + t.Fatalf("expected ResponseHeaderTimeout=%v, got %v", responseHeaderTimeout, transport.ResponseHeaderTimeout) + } + + if client.Timeout != 0 { + t.Fatalf("expected no Client.Timeout, got %v", client.Timeout) + } +} + func TestOptionsForAccountScopes_NoClientTimeout(t *testing.T) { opts, err := optionsForAccountScopes(testClientResolverContext(t), "svc", "a@b.com", []string{"s1"}) if err != nil { diff --git a/internal/googleapi/photos.go b/internal/googleapi/photos.go index d0904c0f7..d82652324 100644 --- a/internal/googleapi/photos.go +++ b/internal/googleapi/photos.go @@ -38,7 +38,7 @@ func WithPhotosBaseURL(baseURL string) PhotosClientOption { func NewPhotosClient(client *http.Client, opts ...PhotosClientOption) *PhotosClient { if client == nil { - client = http.DefaultClient + client = NewBoundedHTTPClient() } c := &PhotosClient{ diff --git a/internal/googleapi/photos_client_test.go b/internal/googleapi/photos_client_test.go new file mode 100644 index 000000000..3b5145ee3 --- /dev/null +++ b/internal/googleapi/photos_client_test.go @@ -0,0 +1,38 @@ +package googleapi + +import ( + "net/http" + "testing" +) + +func TestNewPhotosClientNilUsesBoundedClient(t *testing.T) { + client := NewPhotosClient(nil) + if client.client == http.DefaultClient { + t.Fatal("nil PhotosClient fell back to DefaultClient") + } + + tr, ok := client.client.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected *http.Transport, got %T", client.client.Transport) + } + + if tr.ResponseHeaderTimeout != responseHeaderTimeout { + t.Fatalf("ResponseHeaderTimeout = %v", tr.ResponseHeaderTimeout) + } +} + +func TestNewPhotosPickerClientNilUsesBoundedClient(t *testing.T) { + client := NewPhotosPickerClient(nil) + if client.client == http.DefaultClient { + t.Fatal("nil PhotosPickerClient fell back to DefaultClient") + } + + tr, ok := client.client.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected *http.Transport, got %T", client.client.Transport) + } + + if tr.ResponseHeaderTimeout != responseHeaderTimeout { + t.Fatalf("ResponseHeaderTimeout = %v", tr.ResponseHeaderTimeout) + } +} diff --git a/internal/googleapi/photos_picker.go b/internal/googleapi/photos_picker.go index 1259a80a6..278aeff28 100644 --- a/internal/googleapi/photos_picker.go +++ b/internal/googleapi/photos_picker.go @@ -44,7 +44,7 @@ func WithPhotosPickerBaseURL(baseURL string) PhotosPickerClientOption { func NewPhotosPickerClient(client *http.Client, opts ...PhotosPickerClientOption) *PhotosPickerClient { if client == nil { - client = http.DefaultClient + client = NewBoundedHTTPClient() } c := &PhotosPickerClient{ diff --git a/internal/zoom/client.go b/internal/zoom/client.go index 877b85e1c..0fb4fbfb4 100644 --- a/internal/zoom/client.go +++ b/internal/zoom/client.go @@ -76,7 +76,7 @@ func NewClient(alias string, credentials Credentials, tokens TokenStore, opts .. ClientSecret: strings.TrimSpace(credentials.ClientSecret), }, alias: NormalizeAlias(alias), - httpClient: http.DefaultClient, + httpClient: &http.Client{Timeout: 30 * time.Second}, now: time.Now, tokens: tokens, } diff --git a/internal/zoom/client_test.go b/internal/zoom/client_test.go index d77ccf2cf..44e9349ad 100644 --- a/internal/zoom/client_test.go +++ b/internal/zoom/client_test.go @@ -136,3 +136,23 @@ func TestRedactZoomURL(t *testing.T) { t.Fatalf("expected include passwords env") } } + +func TestNewClientDefaultHTTPClientIsBounded(t *testing.T) { + store, _ := newTestStore(t) + client, err := NewClient("work", Credentials{ + AccountID: "acct", + ClientID: "client", + ClientSecret: "secret", + }, store) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + if client.httpClient == http.DefaultClient { + t.Fatal("NewClient used http.DefaultClient") + } + if client.httpClient.Timeout == 0 { + if tr, ok := client.httpClient.Transport.(*http.Transport); !ok || tr.ResponseHeaderTimeout == 0 { + t.Fatal("default Zoom HTTP client has no timeout") + } + } +}