fix: prevent http.DefaultClient mutation in search tools#151
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical race condition in the tavily and traversaal search tools where http.DefaultClient.Transport was being mutated directly, affecting all HTTP requests process-wide. The fix creates new http.Client instances when a proxy is configured, following the established pattern used consistently in other tools (duckduckgo, perplexity, sploitus, searxng, google, browser).
Changes:
- Fixed
tavily.goandtraversaal.goto create new HTTP client instances instead of mutating the global singleton - Added comprehensive test coverage for both tools including mutation guards, availability checks, and response parsing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| backend/pkg/tools/tavily.go | Creates new http.Client with custom transport when proxy is configured, preventing global state mutation |
| backend/pkg/tools/traversaal.go | Creates new http.Client with custom transport when proxy is configured, preventing global state mutation |
| backend/pkg/tools/tavily_test.go | Adds tests for mutation guard, IsAvailable, and parseHTTPResponse with multiple error conditions |
| backend/pkg/tools/traversaal_test.go | Adds tests for mutation guard, IsAvailable, and parseHTTPResponse with success and error paths |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
backend/pkg/tools/tavily_test.go
Outdated
| if http.DefaultClient.Transport != nil { | ||
| t.Error("http.DefaultClient.Transport should remain nil when no proxy is configured") |
There was a problem hiding this comment.
This assertion is incorrect. The test should verify that http.DefaultClient.Transport remains unchanged (comparing to originalTransport captured before the test), not that it remains nil. Other tests or code in the same process might have already set DefaultClient.Transport to a non-nil value. The TestTavilySearchDoesNotMutateDefaultClient test correctly captures the original value and compares against it - this test should follow the same pattern.
backend/pkg/tools/traversaal_test.go
Outdated
| if http.DefaultClient.Transport != nil { | ||
| t.Error("http.DefaultClient.Transport should remain nil when no proxy is configured") |
There was a problem hiding this comment.
This assertion is incorrect. The test should verify that http.DefaultClient.Transport remains unchanged (comparing to originalTransport captured before the test), not that it remains nil. Other tests or code in the same process might have already set DefaultClient.Transport to a non-nil value. The TestTraversaalSearchDoesNotMutateDefaultClient test correctly captures the original value and compares against it - this test should follow the same pattern.
backend/pkg/tools/tavily_test.go
Outdated
| ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| resp := tavilySearchResult{ | ||
| Answer: "test answer", | ||
| Query: "test", | ||
| Results: []tavilyResult{ | ||
| {Title: "Result 1", URL: "https://example.com/1", Content: "content 1", Score: 0.95}, | ||
| }, | ||
| } | ||
| w.Header().Set("Content-Type", "application/json") | ||
| json.NewEncoder(w).Encode(resp) | ||
| })) | ||
| defer ts.Close() |
There was a problem hiding this comment.
The test server created here is never used since proxyURL is empty. Consider removing the test server setup (lines 46-57) to avoid confusion, or save and restore the original transport value like in TestTavilySearchDoesNotMutateDefaultClient to make the assertion more robust.
| if parseErr == nil { | ||
| t.Fatal("parseHTTPResponse() expected error, got nil") | ||
| } | ||
| }) |
There was a problem hiding this comment.
The errContain field is defined in the test cases but never validated in the test logic. The test should check that the error message contains the expected string. Add validation like: if !strings.Contains(parseErr.Error(), tt.errContain) { t.Errorf("error message %q should contain %q", parseErr.Error(), tt.errContain) }
Both tavily.go and traversaal.go assigned http.DefaultClient to a local variable and then mutated its Transport field when configuring a proxy. Since DefaultClient is a process-wide singleton, this creates a data race: concurrent requests overwrite each other's proxy configuration, and all other HTTP callers in the process are affected. Create a new http.Client instance when proxy is configured instead of mutating the global. When no proxy is set, DefaultClient is used read-only (no mutation). This matches the pattern in browser.go. Signed-off-by: mason5052 <ehehwnwjs5052@gmail.com>
d280e8e to
e36f47f
Compare
- Remove unused httptest.Server in TestTavilySearchWithoutProxy (tavily.search() targets hardcoded tavilyURL, not the test server) - Remove unused encoding/json imports from both test files Signed-off-by: mason5052 <ehehwnwjs5052@gmail.com>
|
thank you for the PR! |
Description
Problem
Both
tavily.goandtraversaal.goassignhttp.DefaultClientto a local variable and then mutate itsTransportfield when configuring a proxy URL:Since
http.DefaultClientis a process-wide singleton, this creates a data race: concurrent requests from different tool instances overwrite each other's proxy configuration, and all other HTTP callers in the process inherit the mutated transport.Solution
Create a new
http.Clientinstance when proxy is configured instead of mutating the global. When no proxy is set,DefaultClientis used read-only (no mutation). This matches the correct pattern already used inbrowser.go:callScraper().Type of Change
Areas Affected
Testing
Configuration
Steps to Reproduce the Bug
http.DefaultClient.Transportis overwritten by whichever goroutine runs lastTest Results
tavily_test.go: Tests thathttp.DefaultClient.Transportis NOT mutated aftersearch()with proxy configured. Also coversIsAvailable()andparseHTTPResponse()error paths.traversaal_test.go: Same mutation guard test, plusIsAvailable()andparseHTTPResponse()success/error paths.Security Considerations
Checklist