diff --git a/CHANGELOG.md b/CHANGELOG.md index ca7fe4b..cb0f614 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Unreleased +- Client: reject responses larger than 1 MiB without decoding truncated data, and report unhandled HTTP redirects as API errors instead of successful results. - Homebrew: install through a Formula (`brew install openclaw/tap/goplaces`) instead of the retired Cask; quarantine is preserved. ## 0.4.11 - 2026-09-13 diff --git a/docs/client-reference.md b/docs/client-reference.md index 69935ed..02ccf06 100644 --- a/docs/client-reference.md +++ b/docs/client-reference.md @@ -16,3 +16,5 @@ These request details are easy to miss when moving between the CLI and library: Field masks are defined with each request implementation so calls ask Google only for the fields represented by that workflow. Redirects may stay within the original origin (scheme, hostname, and port); redirects to another origin are rejected to keep the API key scoped to the configured endpoint. Configure an endpoint override directly when using a different host. A custom `HTTPClient.CheckRedirect` can still stop redirects. API keys echoed in upstream or transport diagnostics are redacted; error causes remain available through `errors.Is` and `errors.As`. + +Only final 2xx responses are decoded as results. Other HTTP statuses return an `APIError`, including redirects stopped with `http.ErrUseLastResponse` or returned without a `Location` header. Responses are limited to 1 MiB; larger bodies return an explicit size error instead of decoding a truncated payload. Oversized HTTP error responses retain their status in `APIError` and replace the body with a size diagnostic. The CLI exits with code 1 for these response failures. diff --git a/internal/places/client.go b/internal/places/client.go index 12a33ab..54fd8ff 100644 --- a/internal/places/client.go +++ b/internal/places/client.go @@ -16,6 +16,11 @@ import ( // DefaultBaseURL is the default endpoint for the Places API (New). const DefaultBaseURL = "https://places.googleapis.com/v1" +const ( + maxResponseBytes = 1 << 20 + responseTooLargeMessage = "response exceeds 1 MiB limit" +) + // Client wraps access to the Google Places API. type Client struct { apiKey string @@ -111,16 +116,24 @@ func (c *Client) doRequest( _ = response.Body.Close() }() - // Hard-cap payload size to avoid runaway error bodies. - payload, err := io.ReadAll(io.LimitReader(response.Body, 1<<20)) + // Read one extra byte to distinguish an exact-size response from truncation. + payload, err := io.ReadAll(io.LimitReader(response.Body, maxResponseBytes+1)) if err != nil { return nil, c.requestError("read response", err) } - if response.StatusCode >= http.StatusBadRequest { - apiErr := &APIError{StatusCode: response.StatusCode, Body: c.redactAPIKey(strings.TrimSpace(string(payload)))} + tooLarge := len(payload) > maxResponseBytes + if response.StatusCode < http.StatusOK || response.StatusCode >= http.StatusMultipleChoices { + body := responseTooLargeMessage + if !tooLarge { + body = c.redactAPIKey(strings.TrimSpace(string(payload))) + } + apiErr := &APIError{StatusCode: response.StatusCode, Body: body} return nil, apiErr } + if tooLarge { + return nil, errors.New("goplaces: " + responseTooLargeMessage) + } if len(payload) == 0 { return nil, errors.New("goplaces: empty response") diff --git a/internal/places/client_response_test.go b/internal/places/client_response_test.go new file mode 100644 index 0000000..1ffade9 --- /dev/null +++ b/internal/places/client_response_test.go @@ -0,0 +1,93 @@ +package places + +import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestClientResponseSizeBoundary(t *testing.T) { + const limit = 1 << 20 + for _, tt := range []struct { + name string + size int + wantErr bool + }{ + {"below limit", limit - 1, false}, + {"at limit", limit, false}, + {"over limit", limit + 1, true}, + } { + t.Run(tt.name, func(t *testing.T) { + prefix := `{"id":"fixture"}` + body := prefix + strings.Repeat(" ", tt.size-len(prefix)) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // Flush headers so the check also covers unknown content lengths. + w.(http.Flusher).Flush() + _, _ = fmt.Fprint(w, body) + })) + defer server.Close() + client := NewClient(Options{APIKey: "test-key", BaseURL: server.URL}) + place, err := client.Details(t.Context(), "fixture") + if tt.wantErr { + if err == nil || !strings.Contains(err.Error(), "response exceeds") || place.PlaceID != "" { + t.Fatalf("expected size error without a result, got place=%+v err=%v", place, err) + } + return + } + if err != nil || place.PlaceID != "fixture" { + t.Fatalf("place=%+v err=%v", place, err) + } + }) + } +} + +func TestClientRejectsTrailingDataBeyondResponseLimit(t *testing.T) { + body := `{"id":"fixture"}` + body += strings.Repeat(" ", (1<<20)-len(body)) + `{"id":"trailing"}` + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprint(w, body) + })) + defer server.Close() + client := NewClient(Options{APIKey: "test-key", BaseURL: server.URL}) + if place, err := client.Details(t.Context(), "fixture"); err == nil { + t.Fatalf("accepted a truncated response: %+v", place) + } +} + +func TestClientRejectsNonSuccessStatus(t *testing.T) { + for _, statusCode := range []int{http.StatusMultipleChoices, http.StatusFound, http.StatusNotModified} { + t.Run(http.StatusText(statusCode), func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(statusCode) + _, _ = fmt.Fprint(w, `{"id":"unexpected"}`) + })) + defer server.Close() + client := NewClient(Options{APIKey: "test-key", BaseURL: server.URL}) + place, err := client.Details(t.Context(), "fixture") + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != statusCode || place.PlaceID != "" { + t.Fatalf("expected APIError for %d without a result, got place=%+v err=%v", statusCode, place, err) + } + }) + } +} + +func TestClientOversizedAPIError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusBadGateway) + _, _ = fmt.Fprint(w, strings.Repeat("x", 1<<20)+securityTestKey) + })) + defer server.Close() + client := NewClient(Options{APIKey: securityTestKey, BaseURL: server.URL}) + _, err := client.Details(t.Context(), "fixture") + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusBadGateway { + t.Fatalf("expected APIError with upstream status, got %v", err) + } + if !strings.Contains(apiErr.Body, "response exceeds") || len(apiErr.Body) > 100 { + t.Fatal("expected a concise size diagnostic instead of a truncated upstream body") + } +} diff --git a/internal/places/client_security_test.go b/internal/places/client_security_test.go index eb101be..5aadb10 100644 --- a/internal/places/client_security_test.go +++ b/internal/places/client_security_test.go @@ -76,7 +76,8 @@ func TestClientPreservesCustomRedirectPolicy(t *testing.T) { client := NewClient(Options{APIKey: securityTestKey, BaseURL: server.URL, HTTPClient: httpClient}) for range 2 { place, err := client.Details(t.Context(), "fixture") - if err != nil || place.PlaceID != "stopped" { + var apiErr *APIError + if !errors.As(err, &apiErr) || apiErr.StatusCode != http.StatusFound || place.PlaceID != "" { t.Fatalf("place=%+v err=%v", place, err) } }