feat(tools): add curl tool with domain whitelist and configurable limits (gh:1845)#2416
feat(tools): add curl tool with domain whitelist and configurable limits (gh:1845)#2416fakhriaunur wants to merge 1 commit intosipeed:mainfrom
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
Adds a dedicated curl tool to let agents perform outbound HTTP requests with configurable safety limits (domain allowlist, timeouts, size limits), intended to avoid needing shell exec for API calls.
Changes:
- Introduces
pkg/tools/CurlToolwith URL validation, domain allowlisting, redirect checks, headers/body/method support, and timeout/size limiting. - Adds unit tests covering common request paths and allowlist behavior.
- Extends
ToolsConfigwith a newcurlsection andIsToolEnabled("curl")support.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| pkg/tools/curl.go | New HTTP request tool implementation with allowlist/limits and formatted output. |
| pkg/tools/curl_test.go | New unit tests for curl tool behaviors. |
| pkg/config/config.go | Adds CurlConfig and enables IsToolEnabled("curl") lookup. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return t.SendTTS.Enabled | ||
| case "write_file": | ||
| return t.WriteFile.Enabled | ||
| case "curl": | ||
| return t.Curl.Enabled |
There was a problem hiding this comment.
This PR adds ToolsConfig.Curl and IsToolEnabled("curl"), but there is no corresponding tool registration/use of this config anywhere (e.g. no IsToolEnabled("curl") checks or NewCurlTool(...) calls in pkg/agent/*). As a result the tool will never be available at runtime even when enabled in config. Please wire CurlTool into the agent tool registry (likely in AgentLoop or NewAgentInstance) using this config.
| timeout := curlDefaultTimeout | ||
| if tSec, ok := args["timeout"].(float64); ok && tSec > 0 { | ||
| timeout = time.Duration(tSec) * time.Second | ||
| if timeout > 120*time.Second { | ||
| timeout = 120 * time.Second |
There was a problem hiding this comment.
The per-call timeout argument is enforced via context.WithTimeout, but the http.Client is also created with a fixed Timeout (from opts.TimeoutSeconds/default 30s). This prevents callers from increasing the timeout beyond the client-level timeout, even though the tool parameter schema allows up to 120s. Consider relying on context timeouts only (no http.Client.Timeout) or setting the client timeout to the maximum allowed and enforcing the per-call limit via context.
| body, err := io.ReadAll(io.LimitReader(resp.Body, t.maxBytes)) | ||
| if err != nil { | ||
| if err == io.ErrUnexpectedEOF || strings.Contains(err.Error(), "http: request body too large") { | ||
| return ErrorResult(fmt.Sprintf("response body exceeded %d bytes limit", t.maxBytes)) | ||
| } | ||
| return ErrorResult(fmt.Sprintf("failed to read response: %v", err)) |
There was a problem hiding this comment.
io.ReadAll(io.LimitReader(resp.Body, t.maxBytes)) will silently truncate the response if it exceeds t.maxBytes and will not return an oversize error, so the current oversize error handling path is ineffective. To detect oversize reliably (and set truncated accurately), read t.maxBytes+1 bytes and check whether an extra byte was present, then either error or truncate explicitly.
| "body": string(body), | ||
| "url": urlStr, | ||
| "method": method, | ||
| "truncated": int64(len(body)) >= t.maxBytes, | ||
| } |
There was a problem hiding this comment.
truncated is computed as len(body) >= t.maxBytes, which will mark responses of exactly t.maxBytes bytes as truncated even when they fit the limit. If you implement an oversize probe (read maxBytes+1), set truncated based on whether more than maxBytes bytes were available.
| "minimum": 1.0, | ||
| "maximum": 120.0, |
There was a problem hiding this comment.
The schema for timeout declares type: integer but uses floating-point values for minimum/maximum (1.0/120.0). Some JSON-schema consumers treat this as inconsistent. Use integer values for min/max to match the declared type.
| "minimum": 1.0, | |
| "maximum": 120.0, | |
| "minimum": 1, | |
| "maximum": 120, |
| client.CheckRedirect = func(req *http.Request, via []*http.Request) error { | ||
| if len(via) >= curlMaxRedirects { | ||
| return fmt.Errorf("stopped after %d redirects", curlMaxRedirects) | ||
| } | ||
| if !isDomainAllowed(req.URL.Hostname(), opts.AllowedDomains) { | ||
| return fmt.Errorf("redirect to disallowed domain %q", req.URL.Hostname()) | ||
| } |
There was a problem hiding this comment.
CheckRedirect validates redirects against opts.AllowedDomains (unnormalized) rather than the normalized domain list stored on the tool. This can incorrectly block redirects when config includes schemes, mixed case, or trailing slashes. Normalize once in NewCurlTool and use that normalized slice for both initial request and redirect validation.
| ToolConfig `json:"-" envPrefix:"PICOCLAW_TOOLS_CURL_"` | ||
| AllowedDomains []string `json:"allowed_domains,omitempty" env:"PICOCLAW_TOOLS_CURL_ALLOWED_DOMAINS"` | ||
| TimeoutSeconds int `json:"timeout_seconds" env:"PICOCLAW_TOOLS_CURL_TIMEOUT_SECONDS"` | ||
| MaxBytes int64 `json:"max_bytes" env:"PICOCLAW_TOOLS_CURL_MAX_BYTES"` | ||
| } |
There was a problem hiding this comment.
CurlConfig embeds ToolConfig with json:"-", which prevents curl.enabled from being set via the JSON config file (unlike other tool configs such as ExecConfig/WebToolsConfig). Remove the json:"-" tag so enabled can be configured normally while keeping envPrefix for env var support.
sipeed-bot
bot
added
type: enhancement
📝 Description
Add a dedicated
curltool that allows agents to make HTTP requests to external APIs with domain whitelisting for security. This enables skills like Clawdfeed that need to call external APIs without requiring shell command execution.🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
Fixes #1845
📚 Technical Context
Implementation:
pkg/tools/curl.go- New CurlTool with domain whitelist, method/header/body/timeout supportpkg/tools/curl_test.go- 13 tests covering all security gates and HTTP operationspkg/config/config.go- AddedCurlConfigstruct withallowed_domains,timeout_seconds,max_bytespkg/agent/loop.go- Tool registration wired to configSecurity features:
example.comallowsapi.example.com)🧪 Test Environment
☑️ Checklist