Skip to content

Commit 5bef954

Browse files
Deeven-Serugemini-code-assist[bot]Yuan325
authored
fix(http)!: sanitize non-2xx error output (#2654)
## Summary - sanitize non-2xx response errors by default (no upstream body) - add returnFullError opt-in for raw body in error - log truncated body at debug level when sanitized - add regression tests + docs ## Breaking change - non-2xx errors now return sanitized messages unless returnFullError is enabled ## Testing - go test ./internal/sources/http Fixes #2617 --------- Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com>
1 parent 4564efe commit 5bef954

5 files changed

Lines changed: 129 additions & 6 deletions

File tree

‎docs/en/resources/sources/http.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ headers:
3232
queryParams:
3333
param1: value1
3434
param2: value2
35+
# returnFullError: false
3536
# disableSslVerification: false
3637
```
3738

@@ -49,6 +50,7 @@ instead of hardcoding your secrets into the configuration file.
4950
| timeout | string | false | The timeout for HTTP requests (e.g., "5s", "1m", refer to [ParseDuration][parse-duration-doc] for more examples). Defaults to 30s. |
5051
| headers | map[string]string | false | Default headers to include in the HTTP requests. |
5152
| queryParams | map[string]string | false | Default query parameters to include in the HTTP requests. |
53+
| returnFullError | bool | false | Include raw upstream response bodies in error messages for non-2xx responses. Defaults to `false`. |
5254
| disableSslVerification | bool | false | Disable SSL certificate verification. This should only be used for local development. Defaults to `false`. |
5355

5456
[parse-duration-doc]: https://pkg.go.dev/time#ParseDuration

‎internal/sources/http/http.go‎

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ import (
3030
)
3131

3232
const SourceType string = "http"
33+
const maxErrorBodyLogBytes = 1024
3334

3435
// validate interface
3536
var _ sources.SourceConfig = Config{}
@@ -55,6 +56,7 @@ type Config struct {
5556
Timeout string `yaml:"timeout"`
5657
DefaultHeaders map[string]string `yaml:"headers"`
5758
QueryParams map[string]string `yaml:"queryParams"`
59+
ReturnFullError bool `yaml:"returnFullError"`
5860
DisableSslVerification bool `yaml:"disableSslVerification"`
5961
}
6062

@@ -146,7 +148,7 @@ func (s *Source) Client() *http.Client {
146148
return s.client
147149
}
148150

149-
func (s *Source) RunRequest(req *http.Request) (any, error) {
151+
func (s *Source) RunRequest(ctx context.Context, req *http.Request) (any, error) {
150152
// Make request and fetch response
151153
resp, err := s.Client().Do(req)
152154
if err != nil {
@@ -160,7 +162,21 @@ func (s *Source) RunRequest(req *http.Request) (any, error) {
160162
return nil, err
161163
}
162164
if resp.StatusCode < 200 || resp.StatusCode > 299 {
163-
return nil, fmt.Errorf("unexpected status code: %d, response body: %s", resp.StatusCode, string(body))
165+
if s.ReturnFullError {
166+
return nil, fmt.Errorf("unexpected status code: %d, response body: %s", resp.StatusCode, string(body))
167+
}
168+
169+
logger, err := util.LoggerFromContext(ctx)
170+
if err != nil {
171+
return nil, fmt.Errorf("unable to get logger from ctx: %s", err)
172+
}
173+
logger.DebugContext(ctx, "http source upstream error", "status", resp.StatusCode, "body", truncateForLog(body, maxErrorBodyLogBytes))
174+
175+
statusText := http.StatusText(resp.StatusCode)
176+
if statusText != "" {
177+
return nil, fmt.Errorf("unexpected status code: %d (%s)", resp.StatusCode, statusText)
178+
}
179+
return nil, fmt.Errorf("unexpected status code: %d", resp.StatusCode)
164180
}
165181

166182
var data any
@@ -170,3 +186,13 @@ func (s *Source) RunRequest(req *http.Request) (any, error) {
170186
}
171187
return data, nil
172188
}
189+
190+
func truncateForLog(body []byte, limit int) string {
191+
if limit <= 0 || len(body) == 0 {
192+
return ""
193+
}
194+
if len(body) <= limit {
195+
return string(body)
196+
}
197+
return fmt.Sprintf("%s...(%d bytes truncated)", string(body[:limit]), len(body)-limit)
198+
}

‎internal/sources/http/http_test.go‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,20 @@
1515
package http_test
1616

1717
import (
18+
"bytes"
1819
"context"
20+
nethttp "net/http"
21+
"net/http/httptest"
22+
"strings"
1923
"testing"
2024

2125
"github.com/google/go-cmp/cmp"
26+
"github.com/googleapis/genai-toolbox/internal/log"
2227
"github.com/googleapis/genai-toolbox/internal/server"
2328
"github.com/googleapis/genai-toolbox/internal/sources"
2429
"github.com/googleapis/genai-toolbox/internal/sources/http"
2530
"github.com/googleapis/genai-toolbox/internal/testutils"
31+
"github.com/googleapis/genai-toolbox/internal/util"
2632
)
2733

2834
func TestParseFromYamlHttp(t *testing.T) {
@@ -63,6 +69,7 @@ func TestParseFromYamlHttp(t *testing.T) {
6369
queryParams:
6470
api-key: test_api_key
6571
param: param-value
72+
returnFullError: true
6673
disableSslVerification: true
6774
`,
6875
want: map[string]sources.SourceConfig{
@@ -73,6 +80,7 @@ func TestParseFromYamlHttp(t *testing.T) {
7380
Timeout: "10s",
7481
DefaultHeaders: map[string]string{"Authorization": "test_header", "Custom-Header": "custom"},
7582
QueryParams: map[string]string{"api-key": "test_api_key", "param": "param-value"},
83+
ReturnFullError: true,
7684
DisableSslVerification: true,
7785
},
7886
},
@@ -136,3 +144,85 @@ func TestFailParseFromYaml(t *testing.T) {
136144
})
137145
}
138146
}
147+
148+
func TestRunRequestSanitizesErrorBodyByDefault(t *testing.T) {
149+
server := httptest.NewServer(nethttp.HandlerFunc(func(w nethttp.ResponseWriter, r *nethttp.Request) {
150+
w.WriteHeader(nethttp.StatusBadRequest)
151+
_, _ = w.Write([]byte("sensitive details"))
152+
}))
153+
defer server.Close()
154+
155+
logger, err := log.NewLogger("standard", log.Debug, &bytes.Buffer{}, &bytes.Buffer{})
156+
if err != nil {
157+
t.Fatalf("failed to create logger: %v", err)
158+
}
159+
ctx := util.WithLogger(context.Background(), logger)
160+
161+
sourceConfig := http.Config{
162+
Name: "test-http",
163+
Type: http.SourceType,
164+
BaseURL: server.URL,
165+
Timeout: "30s",
166+
}
167+
initialized, err := sourceConfig.Initialize(ctx, nil)
168+
if err != nil {
169+
t.Fatalf("failed to initialize source: %v", err)
170+
}
171+
source := initialized.(*http.Source)
172+
173+
req, err := nethttp.NewRequestWithContext(ctx, nethttp.MethodGet, server.URL, nil)
174+
if err != nil {
175+
t.Fatalf("failed to build request: %v", err)
176+
}
177+
178+
_, err = source.RunRequest(ctx, req)
179+
if err == nil {
180+
t.Fatalf("expected error for non-2xx response")
181+
}
182+
if strings.Contains(err.Error(), "sensitive details") {
183+
t.Fatalf("expected sanitized error message, got %q", err.Error())
184+
}
185+
if !strings.Contains(err.Error(), "unexpected status code: 400") {
186+
t.Fatalf("expected status code in error message, got %q", err.Error())
187+
}
188+
}
189+
190+
func TestRunRequestIncludesErrorBodyWhenEnabled(t *testing.T) {
191+
server := httptest.NewServer(nethttp.HandlerFunc(func(w nethttp.ResponseWriter, r *nethttp.Request) {
192+
w.WriteHeader(nethttp.StatusInternalServerError)
193+
_, _ = w.Write([]byte("sensitive details"))
194+
}))
195+
defer server.Close()
196+
197+
logger, err := log.NewLogger("standard", log.Debug, &bytes.Buffer{}, &bytes.Buffer{})
198+
if err != nil {
199+
t.Fatalf("failed to create logger: %v", err)
200+
}
201+
ctx := util.WithLogger(context.Background(), logger)
202+
203+
sourceConfig := http.Config{
204+
Name: "test-http",
205+
Type: http.SourceType,
206+
BaseURL: server.URL,
207+
Timeout: "30s",
208+
ReturnFullError: true,
209+
}
210+
initialized, err := sourceConfig.Initialize(ctx, nil)
211+
if err != nil {
212+
t.Fatalf("failed to initialize source: %v", err)
213+
}
214+
source := initialized.(*http.Source)
215+
216+
req, err := nethttp.NewRequestWithContext(ctx, nethttp.MethodGet, server.URL, nil)
217+
if err != nil {
218+
t.Fatalf("failed to build request: %v", err)
219+
}
220+
221+
_, err = source.RunRequest(ctx, req)
222+
if err == nil {
223+
t.Fatalf("expected error for non-2xx response")
224+
}
225+
if !strings.Contains(err.Error(), "response body: sensitive details") {
226+
t.Fatalf("expected response body in error message, got %q", err.Error())
227+
}
228+
}

‎internal/tools/http/http.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ type compatibleSource interface {
5252
HttpDefaultHeaders() map[string]string
5353
HttpBaseURL() string
5454
HttpQueryParams() map[string]string
55-
RunRequest(*http.Request) (any, error)
55+
RunRequest(context.Context, *http.Request) (any, error)
5656
}
5757

5858
type Config struct {
@@ -275,7 +275,7 @@ func (t Tool) Invoke(ctx context.Context, resourceMgr tools.SourceProvider, para
275275
req.Header.Set(k, v)
276276
}
277277

278-
resp, err := source.RunRequest(req)
278+
resp, err := source.RunRequest(ctx, req)
279279
if err != nil {
280280
return nil, util.ProcessGeneralError(err)
281281
}

‎tests/http/http_integration_test.go‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -431,7 +431,7 @@ func runAdvancedHTTPInvokeTest(t *testing.T) {
431431
requestBody: func() io.Reader {
432432
return bytes.NewBuffer([]byte(`{"animalArray": ["rabbit", "ostrich", "whale"], "id": 4, "path": "tool3", "country": "US", "X-Other-Header": "test"}`))
433433
},
434-
want: "error processing request: unexpected status code: 400, response body: Bad Request: Incorrect query parameter: id, actual: [2 1 4]",
434+
want: "error processing request: unexpected status code: 400 (Bad Request)",
435435
isAgentErr: true,
436436
},
437437
}
@@ -509,6 +509,11 @@ func getHTTPToolsConfig(sourceConfig map[string]any, toolType string) map[string
509509
otherSourceConfig["headers"] = map[string]string{"X-Custom-Header": "unexpected", "Content-Type": "application/json"}
510510
otherSourceConfig["queryParams"] = map[string]any{"id": 1, "name": "Sid"}
511511

512+
clientID := tests.ClientId
513+
if clientID == "" {
514+
clientID = "test-client-id"
515+
}
516+
512517
toolsFile := map[string]any{
513518
"sources": map[string]any{
514519
"my-instance": sourceConfig,
@@ -517,7 +522,7 @@ func getHTTPToolsConfig(sourceConfig map[string]any, toolType string) map[string
517522
"authServices": map[string]any{
518523
"my-google-auth": map[string]any{
519524
"type": "google",
520-
"clientId": tests.ClientId,
525+
"clientId": clientID,
521526
},
522527
},
523528
"tools": map[string]any{

0 commit comments

Comments
 (0)