feat(acp): surface safe browser tool metadata - #1000
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (5)
Included review availability: 3 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. WalkthroughThe ACP translation layer now identifies supported browser tools, generates sanitized titles, and stores browser descriptors in ACP ChangesBrowser ACP updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized change adds optional, sanitized browser metadata across ACP paths without introducing an actionable merge-blocking risk; it is merge-ready after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The URL redaction here is genuinely good and I want it landed. Requesting changes on the title rewrite, which drops disclosure the approver needs.
What works, and it is more than the body claims
Driven through permissionToolCall, head against base:
https://alice:hunter2@intranet.corp/path head "browser open https://intranet.corp"
base "browser_open https://alice:hunter2@intranet.corp/path"
https://ok.test/p?token=SECRET123#f=... head "browser open https://ok.test"
base "browser_open https://ok.test/p?token=SECRET123#f=SECRET456"
Credentials, query and fragment all gone. The path goes too, which the body does not claim, and that matters because secrets live in paths as often as in queries. https://www.paypal.com@evil.example/ renders as the true host rather than the deceptive spelling, and a URL with an embedded CR no longer puts a raw control byte in the title. Those are real improvements.
One correction for the body: it says this stops typed text and CDP endpoints reaching titles. Base never leaked those. primaryArgHint's key list is path/file_path/pattern/query/command/url/cwd, which never matched text, target, key or selector, so browser_type and browser_connect already rendered bare. The real leak closed is browser_open's URL. Worth rewording so the next reader does not over-trust it.
Blocking: every browser_action renders as the same six characters
cmd=eval head "browser action" base "browser_action eval"
cmd=keyboard_insert_text head "browser action" base "browser_action keyboard_insert_text"
browserToolTitle does if command != "open" { return "browser " + command }, and command is the tool-name suffix "action", so the args["command"] enum never reaches the title.
Judging it on its own terms rather than against base: RequestPermissionParams is {SessionID, ToolCall, Options}, so Title plus rawInput is the whole of what the editor user gets. browser_action is prompt-gated and permissionSupportsPersistentDecision is false for it, so the user is asked every single call with that string as the decision label, and eval is indistinguishable from get_title. The TUI still discloses it, which puts ACP below the terminal on the same decision, against the invariant permission.go:66-68 states for itself.
The redaction rationale does not apply here either: command is a closed enum validated against browserActionSpecs, so it can never carry attacker text. Put it back.
Bare-host browser_open loses the destination entirely
url="evil.example.test/pay" deriveBrowserHostScope="evil.example.test" title="browser open"
url="example.com" deriveBrowserHostScope="example.com" title="browser open"
Three places derive a host from that one argument and only two agree. browserOpenURLArg prepends https:// when there is no scheme, and the schema documents it. deriveBrowserHostScope does the same. browserToolTitle bails on an empty scheme and prints nothing, so the approver sees the bare words "browser open" while the browser navigates to https://evil.example.test/pay. Normalize the way the tool already does before parsing.
The title can name an origin the tool will not open
browserToolTitle decodes into a struct with a json:"url" tag, and encoding/json matches tags case-insensitively with last-key-wins, while the tool reads the map key exactly.
{"URL":"https://evil.com/x"} title "browser open https://evil.com"
browserOpenURLArg -> err "url is required", never navigates
So the prompt names a host that will not be visited. And on the start path, where raw JSON key order survives:
{"url":"https://a.com/x","URL":"https://b.com/x"} -> title "browser open https://b.com"
{"URL":"https://b.com/x","url":"https://a.com/x"} -> title "browser open https://a.com"
The title follows whichever key came last; the tool always opens the url one.
Being precise about reach, because I checked both paths: on the permission path this collision is currently harmless, because marshalArgs sorts keys so url always lands after URL. It is the start path that diverges. The URL-only case is wrong on both. Using the same exact-key lookup the tool uses fixes all of it and removes the dependence on map ordering, which is not a property worth relying on.
Titles are now unbounded
The browser branch returns before truncateHint:
hostlen 100 head title 126 base 76
hostlen 10000 head title 10026 base 76
hostlen 100000 head title 100026 base 76
Linear, no bound, and it reproduces on the permission surface too. read_file with a 4000-character path is still 73 bytes on head, so this is specific to the new branch. Arguments are model-generated, so it is reachable from injected page content, and the title is emitted before execution so a host that will never resolve still ships in full.
The same bypass breaks the UTF-8 invariant: {"url":"https://%FF%FE.com/"} yields an invalid-UTF-8 title that marshals as browser open https://��.com, a host nobody sent. TestToolTitleTruncateHintRuneSafe pins both invariants but only for read_file, so it passes.
Nits
The nine-command switch in browserToolDetails is a fourth copy of the browser-tool inventory, plus a fifth in prose in the doc comment. All nine agree today, and internal/acp already imports internal/tools, so a contract test would be cheap and would replace the hardcoded names in the new tests.
Not tested and genuinely a question rather than a finding: ACP vendor extensions normally live under _meta, and this adds a bare top-level browser key. There is no _meta usage anywhere in internal/acp and I had no strict-schema client to test against, so I am not claiming anything breaks.
Checked and correct
For any scheme:// input the title's host cannot diverge from the host the tool opens, since both call url.Parse on the same string. Percent-encoded hosts fail closed on both sides. [::1]:8443 and backslash-userinfo forms behave. The descriptor itself is sound: {Version, Command} with Command from a fixed allowlist that never touches request data, the nine names match NewLocalBrowserTools exactly, MCP collision is structurally impossible since registryToolName always returns the mcp_ prefix, and browser_screenshot/browser_pdf correctly get no descriptor. All three ToolCallUpdate constructors set it and a fresh struct is allocated per call. Both new tests are load-bearing: replacing the title body fails the presentation-safety test, and replacing the allowlist fails the MCP test.
One thought rather than a finding: promoting the host to be the only thing in the title makes it read more like a verified origin than base's raw spelling did. Homograph hosts render the same on both sides, so it is not a regression, but the framing invites more trust than the string earns.
Build, vet, gofmt and go test ./internal/acp/ all clean on head, linux and darwin included.
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving at 0b9c996e. All four are fixed, and the way you fixed the third is better than what I asked for.
Re-ran the same probes:
browser_action eval "browser action eval"
browser_action keyboard_insert_text "browser action keyboard_insert_text"
url="evil.example.test/pay" "browser open https://evil.example.test"
{"url":"a.com/x","URL":"b.com/x"} "browser open https://a.com" (matches the tool)
{"URL":"https://evil.com/x"} "browser open" (names nothing; tool refuses)
300-char host truncated at 76 with the ellipsis
I asked you to use the same exact-key lookup the tool uses. Extracting NormalizedBrowserActionCommand and NormalizeBrowserOpenURL and having ACP call them is the better answer, because it removes the second representation instead of adding a matching copy of it. The bare-host case now agrees with deriveBrowserHostScope and browserOpenURLArg by construction rather than by three implementations happening to agree.
The refactor is behaviour-preserving on the tool side: browserActionArgs gets the same command and spec it did before, and go test ./internal/tools/ -run Browser passes.
The fix is pinned rather than incidental. Breaking the command lookup fails TestBrowserToolUpdatesAreStructuredAndPresentationSafe.
And the redaction I liked in the first place still holds: credentials, query, fragment and path all stay out of the title, and https://www.paypal.com@evil.example/ still renders the true host.
Two things I raised that are not code and are still worth doing, neither blocking:
The PR body says this stops typed text and CDP endpoints reaching titles. Base never leaked those, since primaryArgHint's key list never matched text, target, key or selector. The real leak closed is browser_open's URL, and the body reads stronger than that.
The nine-command switch in browserToolDetails is still a separate copy of the browser-tool inventory. internal/acp already imports internal/tools, and you have just exported the normalizer, so a contract test asserting the switch against NewLocalBrowserTools is cheap now.
Build, vet, gofmt and go test ./internal/acp/ all clean.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Put the browser extension under ACP
_meta
internal/acp/types.go:217
ZERO negotiates ACP v1, whose extensibility contract reserves custom data for_metaand says implementations must not add custom fields at the root of protocol types. This PR instead addsjson:"browser"directly toToolCallUpdate, so every recognized browser call emits a non-protocol root member fromtoolCallStart,toolCallResult, andpermissionToolCall. Generated ACP tool-call models have no such member: an SDK-based client or relay drops it while decoding and re-encoding the payload, and a strict consumer may reject the payload. Only a bespoke raw-JSON decoder that knows ZERO's private shape would retain it.The root cause is that the standard ACP wire type and ZERO-specific extension data are modeled as one flat Go struct. The new serialization test does not exercise the boundary: it unmarshals into another local struct that already declares
json:"browser", so it proves only that ZERO can read the same private field it wrote. Model ACP's extension channel explicitly instead—for example, attach the existing{version, command}descriptor beneath a stable ZERO-owned key in_meta—and use one helper to populate that extension for start, result, and permission payloads so those paths cannot drift.Add a regression test that serializes each of those three payloads, passes it through an ACP protocol-shaped/generated decoder and encoder, and verifies that the namespaced descriptor survives. The same coverage should verify that non-browser tools omit the extension and that the existing privacy guarantees remain intact: no typed text, CDP target, URL path, query, fragment, credentials, or other request data may enter the descriptor.
9abf27e
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found one issue that needs to be addressed before this is ready.
Findings
-
[P2] Keep decoded Unicode controls out of browser permission titles
internal/acp/translate.go:121
The new title path parses the normalized URL and rebuilds the displayed origin fromu.Host.net/urlhas already percent-decoded valid UTF-8 escapes at that point, but the only presentation check isutf8.ValidString, which verifies encoding validity rather than whether the string is safe to render. Abrowser_openURL containingsafe.example%E2%80%AEevil.testtherefore passes the shared pre-permission URL validation and produces a title containing an actual U+202E bidi override. U+2066 (bidi isolate), U+0085 (C1 control), and U+2028 (line separator) reach the title through the same path. After ACP serialization and decoding, clients receive those actual runes in the approval label, where they can reorder or split the displayed origin; the browser execution path still receives the normalized URL with its percent escapes preserved. Base did not have this behavior becauseprimaryArgHintleft the%XXspelling literal, so the regression is introduced by the new parse-and-reconstruct display path.Please address the root cause at the presentation boundary: derive the human-facing origin through one display-safety helper that rejects/falls back or visibly escapes Unicode control, format/bidi, and line/paragraph-separator characters after URL parsing has decoded the host. Do not mutate the normalized URL passed to browser execution or broaden the browser's URL policy as part of this fix, and preserve ordinary safe host display, exact-key matching, userinfo/path/query/fragment redaction, and the existing title-length bound. Add table-driven regressions for the encoded examples above through both
toolCallStartandpermissionToolCall, including an ACP marshal/unmarshal round trip that asserts the decoded title contains no disallowed presentation runes and a companion assertion thatNormalizeBrowserOpenURLstill returns the unchanged execution URL. This keeps the correction scoped to the defect while preventing the start, permission, and wire representations from drifting again.
3 participants
Summary
Verification
make fmt-checkgo vet ./...go build ./...go test -race ./internal/acpgo test ./...make lint-staticmake vulncheckgo run ./cmd/zero-release buildgo run ./cmd/zero-release smokezero acpinitialize handshakeNo dependencies or lockfiles changed. No full mutation sweep was run due to local storage-safety constraints.
Summary by CodeRabbit
New Features
Bug Fixes