From 561551ef34cddfb8d966ce6985aaa7b11de7384d Mon Sep 17 00:00:00 2001 From: zhenghuizli Date: Sat, 18 Apr 2026 13:52:18 +0800 Subject: [PATCH] fix(tool/browser): address review feedback for CDP browser tool Critical fixes: - Fix screenshot file lifecycle: don't delete tmpFile after MediaStore.Store (MediaStore only records a mapping, removing the file breaks the media ref) - Add stateMu for concurrent-safe access to history/mediaStore/cdp state - Fix mutex ordering race: executeClose now acquires cdpMu before stateMu - Harden SSRF: block numeric hostnames (0x7f000001, 127.1, 2130706433), CGNAT (100.64.0.0/10), benchmark (198.18.0.0/15) ranges - Document DNS rebinding limitation inherent to CDP architecture Other fixes: - Fix scrollIntoView behavior: 'instant' -> 'auto' (Chrome 125+ TypeError) - Handle json.Unmarshal errors in executeType/executeFill (was silently ignored) - Thread context through CDP SendCtx for proper timeout/cancellation - Clear stale data-pcw-idx attributes in executeState to prevent stale clicks - Fix env var naming: PICOCLAW_TOOLS_BROWSER_TIMEOUT -> _TIMEOUT_SECONDS - Fix stub Parameters() schema: add "required" field for consistency - Fix NewBrowserTool doc comment: connection is lazy, not eager - Fix screenshot integration test assertion to match no-MediaStore path - Split browser.go (990 lines) into browser.go + browser_actions.go (<600 each) - Add 8 new SSRF bypass test cases Co-Authored-By: Claude Opus 4.6 (1M context) --- pkg/config/config.go | 2 +- pkg/tools/browser.go | 653 +++++--------------------- pkg/tools/browser_actions.go | 549 ++++++++++++++++++++++ pkg/tools/browser_cdp.go | 26 +- pkg/tools/browser_integration_test.go | 7 +- pkg/tools/browser_stub.go | 3 + pkg/tools/browser_test.go | 10 + 7 files changed, 694 insertions(+), 556 deletions(-) create mode 100644 pkg/tools/browser_actions.go diff --git a/pkg/config/config.go b/pkg/config/config.go index 571e6a21b..9be2f6e8f 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -901,7 +901,7 @@ type SearchCacheConfig struct { type BrowserToolConfig struct { ToolConfig `envPrefix:"PICOCLAW_TOOLS_BROWSER_"` CDPEndpoint string `json:"cdp_endpoint" yaml:"cdp_endpoint,omitempty" env:"PICOCLAW_TOOLS_BROWSER_CDP_ENDPOINT"` - Timeout int `json:"timeout_seconds" yaml:"timeout_seconds,omitempty" env:"PICOCLAW_TOOLS_BROWSER_TIMEOUT"` + Timeout int `json:"timeout_seconds" yaml:"timeout_seconds,omitempty" env:"PICOCLAW_TOOLS_BROWSER_TIMEOUT_SECONDS"` Stealth bool `json:"stealth" yaml:"stealth,omitempty" env:"PICOCLAW_TOOLS_BROWSER_STEALTH"` AllowEval bool `json:"allow_evaluate" yaml:"allow_evaluate,omitempty" env:"PICOCLAW_TOOLS_BROWSER_ALLOW_EVALUATE"` } diff --git a/pkg/tools/browser.go b/pkg/tools/browser.go index 4703bd804..b75401aed 100644 --- a/pkg/tools/browser.go +++ b/pkg/tools/browser.go @@ -4,13 +4,9 @@ package tools import ( "context" - "encoding/base64" - "encoding/json" "fmt" "net" "net/url" - "os" - "path/filepath" "strings" "sync" "time" @@ -31,10 +27,13 @@ const maxHistorySize = 50 // keep last 50 pages to bound memory // BrowserTool provides browser automation capabilities via Chrome DevTools Protocol. // It enables AI agents to navigate web pages, interact with elements, and extract data. +// +// Lock ordering: cdpMu must always be acquired before stateMu to avoid deadlock. type BrowserTool struct { cfg config.BrowserToolConfig cdp *CDPClient - cdpMu sync.Mutex // guards lazy cdp connection + cdpMu sync.Mutex // guards lazy cdp connection and cdp pointer + stateMu sync.Mutex // guards mutable session state: history, mediaStore chromePath string stealthJS string mediaStore media.MediaStore @@ -42,8 +41,8 @@ type BrowserTool struct { } // NewBrowserTool creates a new BrowserTool. It verifies that Chrome is available -// and attempts to connect to the CDP endpoint. Returns an error if Chrome is -// not found or the connection fails. +// on the system. The actual CDP connection is deferred to the first Execute call +// (lazy connect via connectIfNeeded). Returns an error if Chrome is not found. func NewBrowserTool(cfg config.BrowserToolConfig) (*BrowserTool, error) { chromePath, err := FindChromePath() if err != nil { @@ -209,532 +208,19 @@ func (t *BrowserTool) Execute(ctx context.Context, args map[string]any) *ToolRes } } -// --- Action implementations --- - -func (t *BrowserTool) executeNavigate(ctx context.Context, args map[string]any) *ToolResult { - urlStr, _ := args["url"].(string) - if urlStr == "" { - return ErrorResult("url is required for navigate action") - } - - // Validate URL - if err := validateBrowserURL(urlStr); err != nil { - return ErrorResult(err.Error()) - } - - if err := t.cdp.Navigate(ctx, urlStr); err != nil { - return ErrorResult(fmt.Sprintf("navigation failed: %v", err)) - } - - // Record URL in history (title will be updated when state is called) - t.recordVisit(urlStr, "") - - return SilentResult(fmt.Sprintf("Navigated to %s. Run 'state' to inspect page elements.", urlStr)) -} - -func (t *BrowserTool) executeState(ctx context.Context) *ToolResult { - // JavaScript that extracts interactive elements with [N] indices - js := `(function() { - var selectors = 'a, button, input, select, textarea, [role="button"], [role="link"], [role="tab"], [onclick], [tabindex]:not([tabindex="-1"])'; - var elements = document.querySelectorAll(selectors); - var result = []; - var idx = 0; - for (var i = 0; i < elements.length; i++) { - var el = elements[i]; - var rect = el.getBoundingClientRect(); - if (rect.width === 0 && rect.height === 0) continue; - if (getComputedStyle(el).visibility === 'hidden') continue; - if (getComputedStyle(el).display === 'none') continue; - var tag = el.tagName.toLowerCase(); - var info = { - i: idx, - tag: tag, - text: (el.textContent || '').trim().slice(0, 80).replace(/\s+/g, ' ') - }; - if (el.getAttribute('role')) info.role = el.getAttribute('role'); - if (el.getAttribute('type')) info.type = el.getAttribute('type'); - if (el.getAttribute('name')) info.name = el.getAttribute('name'); - if (el.getAttribute('placeholder')) info.placeholder = el.getAttribute('placeholder'); - if (el.value !== undefined && el.value !== '') info.value = String(el.value).slice(0, 80); - if (tag === 'a' && el.getAttribute('href')) info.href = el.getAttribute('href').slice(0, 120); - if (el.getAttribute('aria-label')) info.label = el.getAttribute('aria-label').slice(0, 80); - if (el.disabled) info.disabled = true; - // Store selector path for interaction - el.setAttribute('data-pcw-idx', String(idx)); - result.push(info); - idx++; - } - return JSON.stringify({ - title: document.title, - url: location.href, - elements: result, - count: idx - }); -})();` - - raw, err := t.cdp.Evaluate(ctx, js) - if err != nil { - return ErrorResult(fmt.Sprintf("failed to extract page state: %v", err)) - } - - // Parse the JSON string returned by JS - var stateStr string - if err := json.Unmarshal(raw, &stateStr); err != nil { - return ErrorResult(fmt.Sprintf("failed to parse state JSON: %v", err)) - } - - var state struct { - Title string `json:"title"` - URL string `json:"url"` - Elements []map[string]any `json:"elements"` - Count int `json:"count"` - } - if err := json.Unmarshal([]byte(stateStr), &state); err != nil { - return ErrorResult(fmt.Sprintf("failed to parse state: %v", err)) - } - - // Update title in history if it changed (SPA navigations) - t.updateCurrentTitle(state.Title) - - // Format output for LLM consumption - var sb strings.Builder - - // Include browsing history summary for context - if summary := t.historySummary(); summary != "" { - sb.WriteString(summary) - sb.WriteByte('\n') - } - - sb.WriteString(fmt.Sprintf("Current page: %s\nURL: %s\n\n", state.Title, state.URL)) - sb.WriteString(fmt.Sprintf("Interactive elements (%d):\n", state.Count)) - - for _, el := range state.Elements { - idxVal, ok := el["i"].(float64) - if !ok { - continue - } - idx := int(idxVal) - tag, _ := el["tag"].(string) - text, _ := el["text"].(string) - - sb.WriteString(fmt.Sprintf("[%d] %s", idx, tag)) - if role, ok := el["role"].(string); ok { - sb.WriteString(fmt.Sprintf(" role=%q", role)) - } - if typ, ok := el["type"].(string); ok { - sb.WriteString(fmt.Sprintf(" type=%q", typ)) - } - if name, ok := el["name"].(string); ok { - sb.WriteString(fmt.Sprintf(" name=%q", name)) - } - if placeholder, ok := el["placeholder"].(string); ok { - sb.WriteString(fmt.Sprintf(" placeholder=%q", placeholder)) - } - if value, ok := el["value"].(string); ok { - sb.WriteString(fmt.Sprintf(" value=%q", value)) - } - if href, ok := el["href"].(string); ok { - sb.WriteString(fmt.Sprintf(" href=%q", href)) - } - if label, ok := el["label"].(string); ok { - sb.WriteString(fmt.Sprintf(" aria-label=%q", label)) - } - if disabled, ok := el["disabled"].(bool); ok && disabled { - sb.WriteString(" [disabled]") - } - if text != "" { - sb.WriteString(fmt.Sprintf(" %q", text)) - } - sb.WriteByte('\n') - } - - return SilentResult(sb.String()) -} - -func (t *BrowserTool) executeClick(ctx context.Context, args map[string]any) *ToolResult { - index, ok := getIntArg(args, "index") - if !ok { - return ErrorResult("index is required for click action (use [N] from state output)") - } - - // Click using the data-pcw-idx attribute we set during state - js := fmt.Sprintf(`(function() { - var el = document.querySelector('[data-pcw-idx="%d"]'); - if (!el) return JSON.stringify({error: 'Element [%d] not found. Run state to refresh indices.'}); - el.scrollIntoView({block: 'center', behavior: 'instant'}); - el.click(); - return JSON.stringify({ok: true, tag: el.tagName.toLowerCase(), text: (el.textContent || '').trim().slice(0, 40)}); -})();`, index, index) - - raw, err := t.cdp.Evaluate(ctx, js) - if err != nil { - return ErrorResult(fmt.Sprintf("click failed: %v", err)) - } - - var resultStr string - if err := json.Unmarshal(raw, &resultStr); err != nil { - return ErrorResult(fmt.Sprintf("failed to parse click result: %v", err)) - } - - var result struct { - OK bool `json:"ok"` - Error string `json:"error"` - Tag string `json:"tag"` - Text string `json:"text"` - } - if err := json.Unmarshal([]byte(resultStr), &result); err != nil { - return ErrorResult(fmt.Sprintf("failed to parse click result: %v", err)) - } - - if result.Error != "" { - return ErrorResult(result.Error) - } - - return SilentResult(fmt.Sprintf("Clicked [%d] <%s> %q. Run 'state' to see updated page.", index, result.Tag, result.Text)) -} - -func (t *BrowserTool) executeType(ctx context.Context, args map[string]any) *ToolResult { - index, ok := getIntArg(args, "index") - if !ok { - return ErrorResult("index is required for type action") - } - text, _ := args["text"].(string) - if text == "" { - return ErrorResult("text is required for type action") - } - - // Focus the element - focusJS := fmt.Sprintf(`(function() { - var el = document.querySelector('[data-pcw-idx="%d"]'); - if (!el) return JSON.stringify({error: 'Element [%d] not found. Run state to refresh indices.'}); - el.scrollIntoView({block: 'center', behavior: 'instant'}); - el.focus(); - return JSON.stringify({ok: true}); -})();`, index, index) - - raw, err := t.cdp.Evaluate(ctx, focusJS) - if err != nil { - return ErrorResult(fmt.Sprintf("focus failed: %v", err)) - } - - var focusStr string - if err := json.Unmarshal(raw, &focusStr); err != nil { - return ErrorResult(fmt.Sprintf("failed to parse focus result: %v", err)) - } - if strings.Contains(focusStr, "error") { - var result struct{ Error string `json:"error"` } - json.Unmarshal([]byte(focusStr), &result) - if result.Error != "" { - return ErrorResult(result.Error) - } - } - - // Type using CDP Input.insertText - if err := t.cdp.InsertText(text); err != nil { - return ErrorResult(fmt.Sprintf("type failed: %v", err)) - } - - return SilentResult(fmt.Sprintf("Typed %q into [%d].", text, index)) -} - -func (t *BrowserTool) executeFill(ctx context.Context, args map[string]any) *ToolResult { - index, ok := getIntArg(args, "index") - if !ok { - return ErrorResult("index is required for fill action") - } - text, _ := args["text"].(string) - if text == "" { - return ErrorResult("text is required for fill action") - } - - // Focus, select all existing text, then type new text - clearJS := fmt.Sprintf(`(function() { - var el = document.querySelector('[data-pcw-idx="%d"]'); - if (!el) return JSON.stringify({error: 'Element [%d] not found.'}); - el.scrollIntoView({block: 'center', behavior: 'instant'}); - el.focus(); - el.value = ''; - el.dispatchEvent(new Event('input', {bubbles: true})); - return JSON.stringify({ok: true}); -})();`, index, index) - - raw, err := t.cdp.Evaluate(ctx, clearJS) - if err != nil { - return ErrorResult(fmt.Sprintf("fill failed (clear step): %v", err)) - } - - var clearStr string - if err := json.Unmarshal(raw, &clearStr); err != nil { - return ErrorResult(fmt.Sprintf("failed to parse fill result: %v", err)) - } - if strings.Contains(clearStr, "error") { - var result struct{ Error string `json:"error"` } - json.Unmarshal([]byte(clearStr), &result) - if result.Error != "" { - return ErrorResult(result.Error) - } - } - - // Type new value - if err := t.cdp.InsertText(text); err != nil { - return ErrorResult(fmt.Sprintf("fill failed (type step): %v", err)) - } - - return SilentResult(fmt.Sprintf("Filled [%d] with %q.", index, text)) -} - -func (t *BrowserTool) executeSelect(ctx context.Context, args map[string]any) *ToolResult { - index, ok := getIntArg(args, "index") - if !ok { - return ErrorResult("index is required for select action") - } - text, _ := args["text"].(string) - if text == "" { - return ErrorResult("text (option value) is required for select action") - } - - textJSON, _ := json.Marshal(text) - js := fmt.Sprintf(`(function() { - var el = document.querySelector('[data-pcw-idx="%d"]'); - if (!el) return JSON.stringify({error: 'Element [%d] not found.'}); - if (el.tagName.toLowerCase() !== 'select') return JSON.stringify({error: 'Element [%d] is not a '}); + var target = %s; + var opts = el.options; + for (var i = 0; i < opts.length; i++) { + if (opts[i].value === target || opts[i].text.trim() === target) { + el.value = opts[i].value; + el.dispatchEvent(new Event('change', {bubbles: true})); + return JSON.stringify({ok: true, selected: opts[i].text.trim()}); + } + } + var available = []; + for (var j = 0; j < opts.length; j++) available.push(opts[j].text.trim()); + return JSON.stringify({error: 'Option not found. Available: ' + available.join(', ')}); +})();`, index, index, index, string(textJSON)) + + raw, err := t.cdp.Evaluate(ctx, js) + if err != nil { + return ErrorResult(fmt.Sprintf("select failed: %v", err)) + } + + var resultStr string + if err := json.Unmarshal(raw, &resultStr); err != nil { + return ErrorResult(fmt.Sprintf("failed to parse select result: %v", err)) + } + var result struct { + OK bool `json:"ok"` + Error string `json:"error"` + Selected string `json:"selected"` + } + if err := json.Unmarshal([]byte(resultStr), &result); err != nil { + return ErrorResult(fmt.Sprintf("failed to parse select result: %v", err)) + } + + if result.Error != "" { + return ErrorResult(result.Error) + } + return SilentResult(fmt.Sprintf("Selected %q in [%d].", result.Selected, index)) +} + +func (t *BrowserTool) executeScreenshot(ctx context.Context) *ToolResult { + data, err := t.cdp.CaptureScreenshot("png", 0) + if err != nil { + return ErrorResult(fmt.Sprintf("screenshot failed: %v", err)) + } + + // Decode base64 PNG data + pngBytes, err := base64.StdEncoding.DecodeString(data) + if err != nil { + return ErrorResult(fmt.Sprintf("failed to decode screenshot: %v", err)) + } + + // Write to temporary file + tmpDir := os.TempDir() + tmpFile := filepath.Join(tmpDir, fmt.Sprintf("screenshot-%d.png", time.Now().UnixNano())) + if err := os.WriteFile(tmpFile, pngBytes, 0600); err != nil { + return ErrorResult(fmt.Sprintf("failed to save screenshot: %v", err)) + } + + // Build scope from channel and chatID context + channel := ToolChannel(ctx) + chatID := ToolChatID(ctx) + scope := fmt.Sprintf("tool:browser:screenshot:%s:%s", channel, chatID) + + // Store in media store if available; otherwise return inline info to LLM + if t.mediaStore != nil { + ref, err := t.mediaStore.Store(tmpFile, media.MediaMeta{ + Filename: "screenshot.png", + ContentType: "image/png", + Source: "tool:browser.screenshot", + }, scope) + if err != nil { + logger.WarnCF("tool", "Failed to store screenshot", + map[string]any{"error": err.Error()}) + // Fall through to inline path + } else { + // Note: do NOT remove tmpFile here. MediaStore.Store records a + // path mapping without copying the file. Removing it would make + // the media reference unresolvable. Let MediaStore manage the + // file lifecycle via its CleanupPolicy. + return &ToolResult{ + ForLLM: "Screenshot captured and sent to user (PNG).", + ForUser: "Screenshot captured", + Media: []string{ref}, + ResponseHandled: true, + } + } + } + + // No MediaStore or store failed — save to disk and return path as artifact + // so LLM can reference it and user can access it via send_file if needed + return &ToolResult{ + ForLLM: fmt.Sprintf("Screenshot saved to %s (%d KB). Use send_file to deliver to user if needed.", tmpFile, len(pngBytes)/1024), + ArtifactTags: []string{fmt.Sprintf("[file:%s]", tmpFile)}, + } +} + +func (t *BrowserTool) executeGetText(ctx context.Context, args map[string]any) *ToolResult { + index, ok := getIntArg(args, "index") + if !ok { + return ErrorResult("index is required for get_text action") + } + + js := fmt.Sprintf(`(function() { + var el = document.querySelector('[data-pcw-idx="%d"]'); + if (!el) return JSON.stringify({error: 'Element [%d] not found.'}); + var text = el.value !== undefined && el.value !== '' ? el.value : el.textContent; + return JSON.stringify({text: (text || '').trim()}); +})();`, index, index) + + raw, err := t.cdp.Evaluate(ctx, js) + if err != nil { + return ErrorResult(fmt.Sprintf("get_text failed: %v", err)) + } + + var resultStr string + if err := json.Unmarshal(raw, &resultStr); err != nil { + return ErrorResult(fmt.Sprintf("failed to parse get_text result: %v", err)) + } + var result struct { + Text string `json:"text"` + Error string `json:"error"` + } + if err := json.Unmarshal([]byte(resultStr), &result); err != nil { + return ErrorResult(fmt.Sprintf("failed to parse get_text result: %v", err)) + } + + if result.Error != "" { + return ErrorResult(result.Error) + } + return SilentResult(fmt.Sprintf("[%d] text: %q", index, result.Text)) +} + +func (t *BrowserTool) executeScroll(ctx context.Context, args map[string]any) *ToolResult { + direction, _ := args["direction"].(string) + if direction == "" { + // Also accept from "text" parameter for convenience + direction, _ = args["text"].(string) + } + + switch direction { + case "up": + // ok + case "down": + // ok + default: + return ErrorResult("direction must be 'up' or 'down'") + } + + amount := 500 // pixels + if direction == "up" { + amount = -500 + } + + js := fmt.Sprintf(`window.scrollBy(0, %d); JSON.stringify({scrollY: window.scrollY});`, amount) + _, err := t.cdp.Evaluate(ctx, js) + if err != nil { + return ErrorResult(fmt.Sprintf("scroll failed: %v", err)) + } + + return SilentResult(fmt.Sprintf("Scrolled %s. Run 'state' to see updated elements.", direction)) +} + +func (t *BrowserTool) executeKeys(_ context.Context, args map[string]any) *ToolResult { + text, _ := args["text"].(string) + if text == "" { + return ErrorResult("text (key name) is required for keys action. Examples: Enter, Tab, Escape, ArrowDown") + } + + // Map common key names + key := text + switch strings.ToLower(text) { + case "enter", "return": + key = "Enter" + case "tab": + key = "Tab" + case "escape", "esc": + key = "Escape" + case "backspace": + key = "Backspace" + case "delete": + key = "Delete" + case "arrowup", "up": + key = "ArrowUp" + case "arrowdown", "down": + key = "ArrowDown" + case "arrowleft", "left": + key = "ArrowLeft" + case "arrowright", "right": + key = "ArrowRight" + } + + if err := t.cdp.DispatchKeyEvent("keyDown", key, 0); err != nil { + return ErrorResult(fmt.Sprintf("keyDown failed: %v", err)) + } + if err := t.cdp.DispatchKeyEvent("keyUp", key, 0); err != nil { + return ErrorResult(fmt.Sprintf("keyUp failed: %v", err)) + } + + return SilentResult(fmt.Sprintf("Pressed key: %s", key)) +} + +func (t *BrowserTool) executeEvaluate(ctx context.Context, args map[string]any) *ToolResult { + if !t.cfg.AllowEval { + return ErrorResult("evaluate action is disabled. Set tools.browser.allow_evaluate=true in config to enable JavaScript execution.") + } + + code, _ := args["code"].(string) + if code == "" { + return ErrorResult("code is required for evaluate action") + } + + raw, err := t.cdp.Evaluate(ctx, code) + if err != nil { + errMsg := err.Error() + // Provide actionable guidance based on error type + var hint string + if strings.Contains(errMsg, "not a function") || strings.Contains(errMsg, "is not iterable") { + hint = " Hint: the variable type may differ from expected. Use typeof/Array.isArray to check, or try document.querySelectorAll instead." + } else if strings.Contains(errMsg, "already been declared") { + hint = " Hint: wrap your code in an IIFE: (function(){ ... })()" + } else if strings.Contains(errMsg, "Failed to fetch") || strings.Contains(errMsg, "NetworkError") { + hint = " Hint: cross-origin fetch may be blocked. Try extracting data from the DOM directly instead of calling APIs." + } else if strings.Contains(errMsg, "not defined") { + hint = " Hint: the variable/function does not exist on this page. Use 'state' to see available elements." + } + return ErrorResult(fmt.Sprintf("evaluate failed: %v.%s", err, hint)) + } + + if raw == nil { + return SilentResult("null") + } + result, _ := json.MarshalIndent(json.RawMessage(raw), "", " ") + return SilentResult(string(result)) +} diff --git a/pkg/tools/browser_cdp.go b/pkg/tools/browser_cdp.go index e075a0134..612353b63 100644 --- a/pkg/tools/browser_cdp.go +++ b/pkg/tools/browser_cdp.go @@ -46,8 +46,7 @@ type cdpErrorPayload struct { // cdpPending tracks an in-flight CDP request. type cdpPending struct { - ch chan cdpResponse - timer *time.Timer + ch chan cdpResponse } type cdpResponse struct { @@ -112,6 +111,15 @@ func (c *CDPClient) Send(method string, params map[string]any) (json.RawMessage, // SendWithTimeout sends a CDP command with a custom timeout. func (c *CDPClient) SendWithTimeout(method string, params map[string]any, timeout time.Duration) (json.RawMessage, error) { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + return c.SendCtx(ctx, method, params) +} + +// SendCtx sends a CDP command and waits for the response, respecting context +// cancellation and deadline. This ensures tool-level timeouts properly propagate +// to CDP operations. +func (c *CDPClient) SendCtx(ctx context.Context, method string, params map[string]any) (json.RawMessage, error) { if c.closed.Load() { return nil, fmt.Errorf("CDP connection closed") } @@ -134,8 +142,7 @@ func (c *CDPClient) SendWithTimeout(method string, params map[string]any, timeou // Register pending request p := &cdpPending{ - ch: make(chan cdpResponse, 1), - timer: time.NewTimer(timeout), + ch: make(chan cdpResponse, 1), } c.mu.Lock() @@ -144,7 +151,6 @@ func (c *CDPClient) SendWithTimeout(method string, params map[string]any, timeou // Clean up on return defer func() { - p.timer.Stop() c.mu.Lock() delete(c.pending, id) c.mu.Unlock() @@ -158,12 +164,12 @@ func (c *CDPClient) SendWithTimeout(method string, params map[string]any, timeou return nil, fmt.Errorf("failed to send CDP message: %w", err) } - // Wait for response or timeout + // Wait for response, context cancellation, or connection close select { case resp := <-p.ch: return resp.Result, resp.Err - case <-p.timer.C: - return nil, fmt.Errorf("CDP command %q timed out after %v", method, timeout) + case <-ctx.Done(): + return nil, fmt.Errorf("CDP command %q cancelled: %w", method, ctx.Err()) case <-c.done: return nil, fmt.Errorf("CDP connection closed while waiting for %q", method) } @@ -395,7 +401,7 @@ func (c *CDPClient) EnableDOM() error { // Evaluate executes JavaScript in the page context and returns the result value. func (c *CDPClient) Evaluate(ctx context.Context, expression string) (json.RawMessage, error) { - result, err := c.Send("Runtime.evaluate", map[string]any{ + result, err := c.SendCtx(ctx, "Runtime.evaluate", map[string]any{ "expression": expression, "returnByValue": true, "awaitPromise": true, @@ -443,7 +449,7 @@ func (c *CDPClient) Navigate(ctx context.Context, targetURL string) error { handlerID := c.On("Page.domContentEventFired", handler) defer c.Off("Page.domContentEventFired", handlerID) - _, err := c.Send("Page.navigate", map[string]any{ + _, err := c.SendCtx(ctx, "Page.navigate", map[string]any{ "url": targetURL, }) if err != nil { diff --git a/pkg/tools/browser_integration_test.go b/pkg/tools/browser_integration_test.go index ba4918ef3..8a736b010 100644 --- a/pkg/tools/browser_integration_test.go +++ b/pkg/tools/browser_integration_test.go @@ -98,9 +98,10 @@ func TestIntegration_Screenshot(t *testing.T) { t.Logf("Screenshot result: ForLLM=%s, ForUser=%s, Media=%v", result.ForLLM, result.ForUser, result.Media) - // Verify temp file was created (even without MediaStore it should have existed briefly) - if !strings.Contains(result.ForLLM, "Screenshot captured") { - t.Error("unexpected screenshot result") + // Verify screenshot result - without MediaStore, returns "Screenshot saved to..." path; + // with MediaStore, returns "Screenshot captured". + if !strings.Contains(result.ForLLM, "Screenshot") { + t.Errorf("unexpected screenshot result: %s", result.ForLLM) } tool.Execute(ctx, map[string]any{"action": "close"}) diff --git a/pkg/tools/browser_stub.go b/pkg/tools/browser_stub.go index 21dfd4ed1..b62666182 100644 --- a/pkg/tools/browser_stub.go +++ b/pkg/tools/browser_stub.go @@ -26,10 +26,13 @@ func (t *BrowserTool) Name() string { return "browser" } func (t *BrowserTool) Description() string { return "Browser automation (not compiled)" } // Parameters implements Tool interface (stub). +// Returns an explicit empty schema to keep schema generation consistent +// even when the tool isn't compiled in. func (t *BrowserTool) Parameters() map[string]any { return map[string]any{ "type": "object", "properties": map[string]any{}, + "required": []string{}, } } diff --git a/pkg/tools/browser_test.go b/pkg/tools/browser_test.go index 5b54a4d31..de4454912 100644 --- a/pkg/tools/browser_test.go +++ b/pkg/tools/browser_test.go @@ -56,6 +56,16 @@ func TestValidateBrowserURL(t *testing.T) { {"http://[::1]", true, "IPv6 loopback blocked"}, {"not-a-url", true, "invalid URL"}, {"", true, "empty URL"}, + // Numeric hostname bypass vectors + {"http://0/", true, "numeric zero hostname blocked"}, + {"http://127.1/", true, "numeric short loopback blocked"}, + {"http://2130706433/", true, "decimal IP blocked"}, + {"http://0x7f000001/", true, "hex IP blocked"}, + // Additional private ranges + {"http://100.64.0.1", true, "CGNAT range blocked"}, + {"http://100.127.255.254", true, "CGNAT range high end blocked"}, + {"http://198.18.0.1", true, "benchmark range blocked"}, + {"http://198.19.255.254", true, "benchmark range high end blocked"}, } for _, tt := range tests {