diff --git a/internal/acp/permission.go b/internal/acp/permission.go index 90c0c960c..e243f7806 100644 --- a/internal/acp/permission.go +++ b/internal/acp/permission.go @@ -142,13 +142,15 @@ func actionOffered(optionID string, offered []PermissionOption) bool { // session/request_permission request from a ZERO permission request. func permissionToolCall(req agent.PermissionRequest) ToolCallUpdate { args := marshalArgs(req.Args) - return ToolCallUpdate{ + upd := ToolCallUpdate{ ToolCallID: req.ToolCallID, Title: toolTitle(req.ToolName, string(args)), Kind: toolKindFor(req.ToolName), Status: ToolStatusPending, RawInput: rawInputBytes(args), } + attachBrowserToolDetails(&upd, req.ToolName) + return upd } func marshalArgs(args map[string]any) []byte { diff --git a/internal/acp/permission_test.go b/internal/acp/permission_test.go index d17ab3384..e2211e741 100644 --- a/internal/acp/permission_test.go +++ b/internal/acp/permission_test.go @@ -82,3 +82,17 @@ func TestPermissionToolCall(t *testing.T) { t.Error("expected rawInput from args") } } + +func TestPermissionToolCallKeepsTheBrowserDescriptor(t *testing.T) { + call := permissionToolCall(agent.PermissionRequest{ + ToolCallID: "browser-1", + ToolName: "browser_connect", + Args: map[string]any{"target": "127.0.0.1:9222"}, + }) + if got := browserDescriptor(t, call); got != (BrowserToolDetails{Version: 1, Command: "connect"}) { + t.Fatalf("browser descriptor = %#v", got) + } + if call.Title != "browser connect" { + t.Fatalf("title = %q", call.Title) + } +} diff --git a/internal/acp/translate.go b/internal/acp/translate.go index 565174904..7f1f461fa 100644 --- a/internal/acp/translate.go +++ b/internal/acp/translate.go @@ -2,7 +2,9 @@ package acp import ( "encoding/json" + "net/url" "strings" + "unicode" "unicode/utf8" "github.com/Gitlawb/zero/internal/agent" @@ -44,12 +46,125 @@ func toolKindFor(name string) string { // toolTitle builds a concise human title, e.g. "read_file src/main.go". func toolTitle(name, rawArgs string) string { + if browser, ok := browserToolDetails(name); ok { + return browserToolTitle(browser.Command, rawArgs) + } if hint := primaryArgHint(rawArgs); hint != "" { return name + " " + hint } return name } +// browserToolDetails identifies ZERO's local browser helpers without treating +// similarly named MCP tools as browser automation. The descriptor intentionally +// contains no request data: ACP tool input is already protocol-visible, but a +// durable UI must not need to retain text, local CDP targets, or full URLs just +// to recognise the browser operation. +func browserToolDetails(name string) (*BrowserToolDetails, bool) { + const prefix = "browser_" + command, ok := strings.CutPrefix(name, prefix) + if !ok { + return nil, false + } + switch command { + case "install", "launch", "connect", "open", "snapshot", "click", "type", "press", "action": + return &BrowserToolDetails{Version: 1, Command: command}, true + default: + return nil, false + } +} + +const zeroBrowserMetaKey = "github.com/Gitlawb/zero/browser" + +// attachBrowserToolDetails stores ZERO's browser descriptor in ACP's reserved +// extension channel. Keeping this in one helper prevents start, result, and +// permission payloads from drifting onto different wire shapes. +func attachBrowserToolDetails(update *ToolCallUpdate, name string) { + browser, ok := browserToolDetails(name) + if !ok { + return + } + raw, err := json.Marshal(browser) + if err != nil { + return + } + update.Meta = map[string]json.RawMessage{zeroBrowserMetaKey: raw} +} + +// browserToolTitle avoids putting browser_type text, an attached DevTools +// endpoint, or a URL query/fragment in a tool-card title. Those values can +// carry credentials or session data; the UI only needs the operation and, for +// navigation, a human-recognisable origin. +func browserToolTitle(command, rawArgs string) string { + switch command { + case "action": + action, ok := exactJSONStringArg(rawArgs, "command") + if !ok { + return "browser action" + } + if action, ok := tools.NormalizedBrowserActionCommand(action); ok { + return "browser action " + action + } + return "browser action" + case "open": + rawURL, ok := exactJSONStringArg(rawArgs, "url") + if !ok { + return "browser open" + } + normalized, err := tools.NormalizeBrowserOpenURL(rawURL) + if err != nil { + return "browser open" + } + u, err := url.Parse(normalized) + if err != nil || u.Scheme == "" || u.Host == "" { + return "browser open" + } + origin := u.Scheme + "://" + u.Host + if !browserTitleTextSafe(origin) { + return "browser open" + } + return "browser open " + truncateHint(origin) + default: + return "browser " + command + } +} + +// browserTitleTextSafe validates text after URL parsing has decoded escaped +// UTF-8 in the host. Valid UTF-8 alone is not presentation-safe: control, +// format/bidi, and line/paragraph separator runes can reorder or split the +// permission label shown to a user. The execution URL remains unchanged. +func browserTitleTextSafe(text string) bool { + if !utf8.ValidString(text) { + return false + } + for _, r := range text { + if unicode.IsControl(r) || unicode.Is(unicode.Cf, r) || unicode.Is(unicode.Zl, r) || unicode.Is(unicode.Zp, r) { + return false + } + } + return true +} + +// exactJSONStringArg mirrors ZERO's map-based tool argument decoding: only the +// exact JSON key is considered, and a non-string value is invalid. In +// particular, an incidental "URL" key must not change a permission title when +// browser_open will only read "url". +func exactJSONStringArg(rawArgs, key string) (string, bool) { + var args map[string]json.RawMessage + if json.Unmarshal([]byte(rawArgs), &args) != nil { + return "", false + } + raw, ok := args[key] + if !ok { + return "", false + } + var value string + if json.Unmarshal(raw, &value) != nil { + return "", false + } + return value, true +} + // primaryArgHint extracts the most relevant argument (path/pattern/command) from // raw JSON arguments. Best-effort; returns "" when it can't parse. func primaryArgHint(rawArgs string) string { @@ -89,7 +204,7 @@ func rawInput(args string) json.RawMessage { // toolCallStart maps an advertised ZERO tool call to the initial ACP "tool_call" // update (status in_progress — ZERO executes immediately after advertising). func toolCallStart(call agent.ToolCall) ToolCallUpdate { - return ToolCallUpdate{ + upd := ToolCallUpdate{ SessionUpdate: UpdateToolCall, ToolCallID: call.ID, Title: toolTitle(call.Name, call.Arguments), @@ -97,6 +212,8 @@ func toolCallStart(call agent.ToolCall) ToolCallUpdate { Status: ToolStatusInProgress, RawInput: rawInput(call.Arguments), } + attachBrowserToolDetails(&upd, call.Name) + return upd } // toolCallResult maps a finished ZERO tool result to a "tool_call_update". @@ -116,6 +233,7 @@ func toolCallResult(result agent.ToolResult) ToolCallUpdate { if locs := toolResultLocations(result); len(locs) > 0 { upd.Locations = locs } + attachBrowserToolDetails(&upd, result.Name) return upd } diff --git a/internal/acp/translate_test.go b/internal/acp/translate_test.go index 4a9adc16d..89dbcc73a 100644 --- a/internal/acp/translate_test.go +++ b/internal/acp/translate_test.go @@ -1,14 +1,29 @@ package acp import ( + "encoding/json" "strings" "testing" + "unicode" "unicode/utf8" "github.com/Gitlawb/zero/internal/agent" "github.com/Gitlawb/zero/internal/tools" ) +func browserDescriptor(t *testing.T, update ToolCallUpdate) BrowserToolDetails { + t.Helper() + raw, ok := update.Meta[zeroBrowserMetaKey] + if !ok { + t.Fatalf("browser metadata = %#v, want %q", update.Meta, zeroBrowserMetaKey) + } + var details BrowserToolDetails + if err := json.Unmarshal(raw, &details); err != nil { + t.Fatalf("decode browser metadata: %v", err) + } + return details +} + func TestAgentMessageAndThoughtChunks(t *testing.T) { m := agentMessageChunk("hello") if m.SessionUpdate != UpdateAgentMessageChunk || m.Content.Type != "text" || m.Content.Text != "hello" { @@ -56,6 +71,217 @@ func TestToolTitleAndHint(t *testing.T) { } } +func TestBrowserToolUpdatesAreStructuredAndPresentationSafe(t *testing.T) { + start := toolCallStart(agent.ToolCall{ + ID: "browser-1", + Name: "browser_open", + Arguments: `{"url":"https://example.com/settings?token=not-for-a-title#account"}`, + }) + if got := browserDescriptor(t, start); got != (BrowserToolDetails{Version: 1, Command: "open"}) { + t.Fatalf("browser descriptor = %#v, want open", got) + } + if start.Title != "browser open https://example.com" { + t.Fatalf("browser title = %q", start.Title) + } + if strings.Contains(start.Title, "token=") || strings.Contains(start.Title, "#account") { + t.Fatalf("browser title leaked URL-sensitive data: %q", start.Title) + } + encoded, err := json.Marshal(start) + if err != nil { + t.Fatal(err) + } + var wire struct { + Meta map[string]json.RawMessage `json:"_meta"` + } + if err := json.Unmarshal(encoded, &wire); err != nil { + t.Fatal(err) + } + if _, ok := wire.Meta[zeroBrowserMetaKey]; !ok { + t.Fatalf("browser wire metadata = %#v", wire.Meta) + } + + typed := toolCallStart(agent.ToolCall{ + ID: "browser-2", + Name: "browser_type", + Arguments: `{"ref":"email","text":"secret@example.test"}`, + }) + if got := browserDescriptor(t, typed); got.Command != "type" { + t.Fatalf("browser type descriptor = %#v", got) + } + if typed.Title != "browser type" || strings.Contains(typed.Title, "secret@example.test") { + t.Fatalf("browser type title = %q", typed.Title) + } + + action := toolCallStart(agent.ToolCall{ + ID: "browser-3", + Name: "browser_action", + Arguments: `{"command":"keyboard_insert_text","args":["secret@example.test"]}`, + }) + if action.Title != "browser action keyboard_insert_text" { + t.Fatalf("browser action title = %q", action.Title) + } + + result := toolCallResult(agent.ToolResult{ + ToolCallID: "browser-2", + Name: "browser_type", + Status: tools.StatusOK, + }) + if got := browserDescriptor(t, result); got.Command != "type" { + t.Fatalf("browser result descriptor = %#v", got) + } +} + +func TestBrowserDescriptorSurvivesProtocolShapedRoundTrip(t *testing.T) { + updates := []ToolCallUpdate{ + toolCallStart(agent.ToolCall{ + ID: "start", + Name: "browser_open", + Arguments: `{"url":"https://user:password@example.test/private?token=secret#fragment"}`, + }), + toolCallResult(agent.ToolResult{ + ToolCallID: "result", + Name: "browser_type", + Status: tools.StatusOK, + }), + permissionToolCall(agent.PermissionRequest{ + ToolCallID: "permission", + ToolName: "browser_connect", + Args: map[string]any{"target": "127.0.0.1:9222"}, + }), + } + + type protocolToolCallUpdate struct { + SessionUpdate string `json:"sessionUpdate,omitempty"` + ToolCallID string `json:"toolCallId"` + Title string `json:"title,omitempty"` + Kind string `json:"kind,omitempty"` + Status string `json:"status,omitempty"` + RawInput json.RawMessage `json:"rawInput,omitempty"` + Content []ToolCallContent `json:"content,omitempty"` + Locations []ToolCallLocation `json:"locations,omitempty"` + Meta map[string]json.RawMessage `json:"_meta,omitempty"` + } + + for _, update := range updates { + encoded, err := json.Marshal(update) + if err != nil { + t.Fatal(err) + } + var root map[string]json.RawMessage + if err := json.Unmarshal(encoded, &root); err != nil { + t.Fatal(err) + } + if _, ok := root["browser"]; ok { + t.Fatalf("browser descriptor escaped ACP _meta: %s", encoded) + } + + var protocol protocolToolCallUpdate + if err := json.Unmarshal(encoded, &protocol); err != nil { + t.Fatal(err) + } + forwarded, err := json.Marshal(protocol) + if err != nil { + t.Fatal(err) + } + var roundTripped ToolCallUpdate + if err := json.Unmarshal(forwarded, &roundTripped); err != nil { + t.Fatal(err) + } + details := browserDescriptor(t, roundTripped) + if details.Version != 1 || details.Command == "" { + t.Fatalf("round-tripped browser descriptor = %#v", details) + } + descriptorJSON := string(roundTripped.Meta[zeroBrowserMetaKey]) + for _, secret := range []string{"password", "private", "token", "fragment", "127.0.0.1", "9222"} { + if strings.Contains(descriptorJSON, secret) { + t.Fatalf("browser descriptor leaked %q: %s", secret, descriptorJSON) + } + } + } +} + +func TestBrowserPermissionTitlesMirrorSafeToolArguments(t *testing.T) { + if got := browserToolTitle("open", `{"url":"evil.example.test/pay?token=hidden#fragment"}`); got != "browser open https://evil.example.test" { + t.Fatalf("bare-host title = %q", got) + } + if got := browserToolTitle("open", `{"URL":"https://different.example.test"}`); got != "browser open" { + t.Fatalf("case-variant URL title = %q", got) + } + if got := browserToolTitle("open", `{"URL":"https://different.example.test","url":"https://actual.example.test/path"}`); got != "browser open https://actual.example.test" { + t.Fatalf("exact URL key title = %q", got) + } + if got := browserToolTitle("action", `{"command":"not an action"}`); got != "browser action" { + t.Fatalf("unknown browser action title = %q", got) + } + + longHost := "https://" + strings.Repeat("a", 200) + ".example.test/path?token=hidden" + title := browserToolTitle("open", `{"url":"`+longHost+`"}`) + if !utf8.ValidString(title) || utf8.RuneCountInString(title) > len("browser open ")+61 || strings.Contains(title, "token=") { + t.Fatalf("bounded browser origin title = %q", title) + } +} + +func TestBrowserOpenTitlesRejectDecodedUnicodePresentationControls(t *testing.T) { + for _, rawURL := range []string{ + "https://safe.example%E2%80%AEevil.test/path", + "https://safe.example%E2%81%A6evil.test/path", + "https://safe.example%C2%85evil.test/path", + "https://safe.example%E2%80%A8evil.test/path", + "https://safe.example%E2%80%A9evil.test/path", + } { + t.Run(rawURL, func(t *testing.T) { + normalized, err := tools.NormalizeBrowserOpenURL(rawURL) + if err != nil { + t.Fatalf("execution URL rejected: %v", err) + } + if normalized != rawURL { + t.Fatalf("execution URL = %q, want unchanged %q", normalized, rawURL) + } + + args, err := json.Marshal(map[string]any{"url": rawURL}) + if err != nil { + t.Fatal(err) + } + updates := []ToolCallUpdate{ + toolCallStart(agent.ToolCall{ID: "start", Name: "browser_open", Arguments: string(args)}), + permissionToolCall(agent.PermissionRequest{ToolCallID: "permission", ToolName: "browser_open", Args: map[string]any{"url": rawURL}}), + } + for _, update := range updates { + encoded, err := json.Marshal(update) + if err != nil { + t.Fatal(err) + } + var decoded ToolCallUpdate + if err := json.Unmarshal(encoded, &decoded); err != nil { + t.Fatal(err) + } + if decoded.Title != "browser open" { + t.Fatalf("unsafe browser title survived wire round trip: %q", decoded.Title) + } + for _, r := range decoded.Title { + if unicode.IsControl(r) || unicode.Is(unicode.Cf, r) || unicode.Is(unicode.Zl, r) || unicode.Is(unicode.Zp, r) { + t.Fatalf("browser title contains unsafe presentation rune %U: %q", r, decoded.Title) + } + } + } + }) + } +} + +func TestBrowserDescriptorDoesNotClaimSimilarlyNamedMCPTools(t *testing.T) { + start := toolCallStart(agent.ToolCall{ID: "mcp-1", Name: "browser_plugin_open", Arguments: `{}`}) + if len(start.Meta) != 0 { + t.Fatalf("MCP-like tool received built-in browser metadata: %#v", start.Meta) + } + encoded, err := json.Marshal(start) + if err != nil { + t.Fatal(err) + } + if strings.Contains(string(encoded), `"browser"`) { + t.Fatalf("non-browser tool encoded browser field: %s", encoded) + } +} + func TestToolCallStart(t *testing.T) { upd := toolCallStart(agent.ToolCall{ID: "tc1", Name: "read_file", Arguments: `{"path":"a.go"}`}) if upd.SessionUpdate != UpdateToolCall { diff --git a/internal/acp/types.go b/internal/acp/types.go index b00bf672a..680476aec 100644 --- a/internal/acp/types.go +++ b/internal/acp/types.go @@ -210,6 +210,21 @@ type ToolCallUpdate struct { RawInput json.RawMessage `json:"rawInput,omitempty"` Content []ToolCallContent `json:"content,omitempty"` Locations []ToolCallLocation `json:"locations,omitempty"` + // Meta is ACP's extension channel. ZERO-owned values must remain beneath a + // namespaced key so protocol-shaped clients can preserve them while decoding + // and re-encoding a tool call. + Meta map[string]json.RawMessage `json:"_meta,omitempty"` +} + +// BrowserToolDetails identifies the browser helper operation behind a tool +// call. Version is the schema version for this optional ZERO extension; +// Command is one of install, launch, connect, open, snapshot, click, type, +// press, or action. Future fields must remain display-safe and must not +// include browser profile data, cookies, typed text, URL paths/queries, or +// DevTools endpoints. +type BrowserToolDetails struct { + Version int `json:"version"` + Command string `json:"command"` } // ToolCallContent is a tool call's rendered output. ZERO emits "content" (a diff --git a/internal/tools/local_browser.go b/internal/tools/local_browser.go index 44cd6d524..672057e84 100644 --- a/internal/tools/local_browser.go +++ b/internal/tools/local_browser.go @@ -548,11 +548,11 @@ func browserActionArgs(args map[string]any) (string, []string, error) { if err != nil { return "", nil, err } - command = strings.ToLower(strings.TrimSpace(command)) - spec, ok := browserActionSpecs[command] + command, ok := NormalizedBrowserActionCommand(command) if !ok { return "", nil, fmt.Errorf("command must be one of: %s", strings.Join(browserActionCommandNames(), ", ")) } + spec := browserActionSpecs[command] values, err := stringArrayArg(args, "args") if err != nil { return "", nil, err @@ -570,6 +570,16 @@ func browserActionArgs(args map[string]any) (string, []string, error) { return command, commandArgs, nil } +// NormalizedBrowserActionCommand returns the exact action that browser_action +// will execute after normalizing its command argument. ACP uses it only for a +// permission title, so an unrecognised command is never reflected as text that +// the tool would reject. +func NormalizedBrowserActionCommand(command string) (string, bool) { + command = strings.ToLower(strings.TrimSpace(command)) + _, ok := browserActionSpecs[command] + return command, ok +} + func browserActionCommandArgs(command string, spec browserActionSpec, values []string) ([]string, error) { switch command { case "connect": @@ -657,6 +667,13 @@ func browserOpenURLArg(args map[string]any) (string, error) { if err != nil { return "", err } + return NormalizeBrowserOpenURL(rawURL) +} + +// NormalizeBrowserOpenURL applies the browser_open URL rules before execution. +// Keeping this exported within the internal package lets permission displays +// describe the same destination the browser helper will open. +func NormalizeBrowserOpenURL(rawURL string) (string, error) { normalized := strings.TrimSpace(rawURL) if !strings.Contains(normalized, "://") { normalized = "https://" + normalized