diff --git a/contributors/emails/laithweinberger@gmail.com b/contributors/emails/laithweinberger@gmail.com new file mode 100644 index 0000000000000..4fe41d91a00a5 --- /dev/null +++ b/contributors/emails/laithweinberger@gmail.com @@ -0,0 +1 @@ +laithrw diff --git a/model_tools.py b/model_tools.py index 4c10995b71617..655ceb6490871 100644 --- a/model_tools.py +++ b/model_tools.py @@ -551,6 +551,21 @@ def _compute_tool_definitions( } break + # browser_exec (Browser Use mode) runs arbitrary Python on the host via + # the browser-use CLI subprocess. A session whose toolset selection + # excludes the terminal surface (e.g. a messaging platform configured + # without terminal access) must not regain host code execution through + # the browser toolset — that would silently widen the operator's chosen + # security posture. Session-level gate, NOT a check_fn: check_fn results + # are TTL-cached process-wide while one gateway process serves many + # sessions with different toolset configs. + if "browser_exec" in available_tool_names and "terminal" not in available_tool_names: + filtered_tools = [ + td for td in filtered_tools + if td.get("function", {}).get("name") != "browser_exec" + ] + available_tool_names.discard("browser_exec") + if not quiet_mode: if filtered_tools: tool_names = [t["function"]["name"] for t in filtered_tools] diff --git a/tests/tools/test_browser_use_cli.py b/tests/tools/test_browser_use_cli.py index 110ec2c15758a..a0167f3b74b02 100644 --- a/tests/tools/test_browser_use_cli.py +++ b/tests/tools/test_browser_use_cli.py @@ -87,6 +87,36 @@ class TestToolSurfaceSwap: assert "browser_exec" in TOOLSETS["browser"]["tools"] assert "browser_exec" in TOOLSETS["coding"]["tools"] + def test_browser_exec_stripped_without_terminal(self, monkeypatch): + """Sessions without the terminal surface must not regain host code + execution through browser_exec (arbitrary Python via the CLI).""" + monkeypatch.setattr(bu_cli, "is_browser_use_cli_mode", lambda: True) + from tools.registry import registry + + entry = registry.get_entry("browser_exec") + monkeypatch.setattr(entry, "check_fn", lambda: True) + import model_tools + + defs = model_tools.get_tool_definitions( + enabled_toolsets=["browser"], quiet_mode=False + ) + names = {t["function"]["name"] for t in defs} + assert "browser_exec" not in names + + def test_browser_exec_present_with_terminal(self, monkeypatch): + monkeypatch.setattr(bu_cli, "is_browser_use_cli_mode", lambda: True) + from tools.registry import registry + + entry = registry.get_entry("browser_exec") + monkeypatch.setattr(entry, "check_fn", lambda: True) + import model_tools + + defs = model_tools.get_tool_definitions( + enabled_toolsets=["browser", "terminal"], quiet_mode=False + ) + names = {t["function"]["name"] for t in defs} + assert "browser_exec" in names + class TestFindCli: def test_prefers_installed_binary(self, monkeypatch): @@ -287,10 +317,14 @@ class TestBrowserUseSlashCommand: assert saved["browser"]["backend"] == "browser-use" assert stub.session_resets == 1 - def test_use_off_removes_backend(self, monkeypatch): + def test_use_off_pins_backend_off(self, monkeypatch): + """`off` must be written explicitly (BACKEND_DISABLED), not removed: + with the key merely deleted, is_legacy_browser_use_cloud_config() + would re-activate CLI mode on the next start for anyone with + BROWSER_USE_API_KEY set, so /browser use off wouldn't stick.""" config = {"browser": {"backend": "browser-use"}} stub, saved = self._run("/browser use off", config, monkeypatch) - assert "backend" not in saved["browser"] + assert saved["browser"]["backend"] == bu_cli.BACKEND_DISABLED assert stub.session_resets == 1 def test_use_bad_arg_prints_usage_without_writing(self, monkeypatch): @@ -420,33 +454,32 @@ class TestHeaderVariants: class TestSkillTextDescription: - @pytest.fixture(autouse=True) - def _reset_skill_cache(self, monkeypatch): - monkeypatch.setattr(bu_cli, "_skill_text_cache", None) - monkeypatch.setattr(bu_cli, "_skill_text_fetched", False) - yield + """The schema description is fully pinned: header + _HELPERS_DIGEST. - def test_description_is_verbatim_cli_skill_text(self, tmp_path, monkeypatch): - cli = _fake_cli( - tmp_path, - 'if [ "$1" = "skill" ]; then echo "# Browser Use\nverbatim skill body"; fi\n', + The live ``browser-use skill`` fetch was removed after A/B benchmarking + showed the pinned digest matches the full skill dump on success rate + (36/36 vs 36/36, opus-4.8 + kimi-k3) — see tools/browser_use_cli.py. + """ + + def test_description_is_pinned_header_plus_digest(self, monkeypatch): + # Even with a CLI present, the description must NOT shell out. + monkeypatch.setattr( + bu_cli, "_find_cli", + lambda: (_ for _ in ()).throw(AssertionError("schema must not invoke the CLI")), ) - monkeypatch.setattr(bu_cli, "_find_cli", lambda: [cli]) overrides = bu_cli._dynamic_schema_overrides() - assert overrides["description"].startswith(bu_cli._DESCRIPTION_HEADER) - assert overrides["description"].endswith("# Browser Use\nverbatim skill body") + assert overrides["description"].startswith(bu_cli._HEADER_BASE) + assert overrides["description"].endswith(bu_cli._HELPERS_DIGEST) - def test_skill_text_cached_after_first_fetch(self, tmp_path, monkeypatch): - calls = [] - cli = _fake_cli(tmp_path, 'echo "skill text"\n') - monkeypatch.setattr(bu_cli, "_find_cli", lambda: (calls.append(1), [cli])[1]) - assert "skill text" in bu_cli._cli_skill_text() - assert "skill text" in bu_cli._cli_skill_text() - assert len(calls) == 1 + def test_digest_names_core_helpers(self): + for helper in ("new_tab(", "page_info()", "js(", "fill_input(", + "click_at_xy(", "capture_screenshot()", "cdp("): + assert helper in bu_cli._HELPERS_DIGEST - def test_no_override_when_cli_missing(self, monkeypatch): - monkeypatch.setattr(bu_cli, "_find_cli", lambda: None) - assert bu_cli._dynamic_schema_overrides() == {} + def test_static_fallback_carries_digest_and_install_hint(self): + desc = bu_cli.BROWSER_EXEC_SCHEMA["description"] + assert bu_cli._HELPERS_DIGEST in desc + assert "uv tool install browser-use" in desc class TestBrowserExec: diff --git a/tools/browser_use_cli.py b/tools/browser_use_cli.py index ff5479bac8d54..ddc241f222c9f 100644 --- a/tools/browser_use_cli.py +++ b/tools/browser_use_cli.py @@ -308,13 +308,8 @@ _HEADER_BASE = ( "Batch each sub-procedure (navigate, wait, extract, act) into one call " "— do not spend a call per action — but for long extractions prefer " "several medium calls that append to workspace files over one giant " - "call, so progress survives timeouts. js() takes a JS expression: " - "js('document.title') or js('(() => {...})()') — a bare '() => {...}' " - "returns the function itself, uncalled. The CLI's own documentation " - "follows and is complete (no need to read separate browser-use skill " - "files) — where it shows shell heredocs (browser-use <<'PY' … PY), pass " - "the Python as `code` instead; where it shows BU_NAME=, pass " - "session= instead." + "call, so progress survives timeouts. For a named cloud browser, pass " + "session= (never BU_NAME env syntax)." ) _HEADER_VISION = ( @@ -331,7 +326,13 @@ _HEADER_TEXT_ONLY = ( "clicks — skip the screenshot-driven workflow described below." ) -_DESCRIPTION_HEADER = _HEADER_BASE +_DESCRIPTION_HEADER = _HEADER_BASE # back-compat alias for external imports + +# NOTE: browser_exec is additionally gated at tool-definition time — sessions +# whose resolved toolsets do not include ``terminal`` never see it (see +# model_tools._compute_tool_definitions). The check_fn registered below only +# answers "is Browser Use mode configured"; surface policy lives with the +# session, not in the process-wide TTL-cached check_fn. def _description_header() -> str: @@ -348,32 +349,45 @@ def _description_header() -> str: _skill_text_cache: Optional[str] = None _skill_text_fetched = False +# Pinned quick-reference for the CLI's pre-imported helpers. Replaces the +# live ``browser-use skill`` fetch: embedding whatever text the installed CLI +# version prints would ship uncontrolled third-party content into every +# session's system-side schema (version drift across machines, supply-chain +# exposure, and a byte-unstable prompt). A/B benchmarked Aug 2026 (108 runs, +# opus-4.8 + kimi-k3, 6 multi-step tasks x 3 reps): header-only schema went +# 36/36 vs 36/36 for the full skill dump at ~equal tokens (-60% vs the +# legacy browser_* toolset either way). The pinned digest below keeps the +# first-call reliability of the helper names without the 7.7KB dump. +_HELPERS_DIGEST = ( + "\n\nHELPERS (pre-imported): new_tab(url) opens/navigates (use for the " + "FIRST navigation), goto_url(url) navigates the current tab, " + "wait_for_load() after navigation, page_info() summarizes the current " + "page state, js(expr) evaluates a JS expression and returns its value " + "(js('document.title'); wrap function bodies as js('(() => {...})()') — " + "a bare '() => {...}' returns the function itself, uncalled), " + "fill_input(selector, text) types into inputs, click_at_xy(x, y) clicks " + "viewport coordinates, capture_screenshot() saves and prints a " + "screenshot path, cdp('Domain.method', **kwargs) is raw CDP — " + "cdp('Accessibility.getFullAXTree')['nodes'] lists every element's " + "role/name/backendDOMNodeId (filter in Python before printing; it is " + "thousands of nodes), then cdp('DOM.getBoxModel', backendNodeId=n) gives " + "click coordinates. ensure_real_tab() recovers from a stale/internal " + "tab. Login walls: stop and ask the user; never guess credentials." +) + def _cli_skill_text() -> str: - """Return the installed CLI's skill""" - global _skill_text_cache, _skill_text_fetched - if _skill_text_fetched: - return _skill_text_cache or "" - _skill_text_fetched = True - cmd = _find_cli() - if not cmd: - return "" - try: - proc = subprocess.run( - [*cmd, "skill"], capture_output=True, text=True, timeout=30 - ) - if proc.returncode == 0 and proc.stdout.strip(): - _skill_text_cache = proc.stdout.strip() - except Exception as e: - logger.debug("Could not fetch browser-use skill text: %s", e) + """Deprecated: always returns "" — the schema uses the pinned header. + + Kept so tests and any external callers keep importing a stable symbol; + see _HELPERS_DIGEST for the rationale (benchmark-backed removal of the + live ``browser-use skill`` fetch). + """ return _skill_text_cache or "" def _dynamic_schema_overrides() -> dict: - skill = _cli_skill_text() - if not skill: - return {} - return {"description": _description_header() + "\n\n---\n\n" + skill} + return {"description": _description_header() + _HELPERS_DIGEST} BROWSER_EXEC_SCHEMA = { @@ -381,8 +395,8 @@ BROWSER_EXEC_SCHEMA = { # Static fallback, used only when the CLI (and uvx) is unavailable "description": ( _HEADER_BASE - + "\n\n(The browser-use CLI is not installed yet, so its full skill " - "documentation could not be loaded. Install it with " + + _HELPERS_DIGEST + + "\n\n(The browser-use CLI is not installed yet. Install it with " "`uv tool install browser-use`.)" ), "parameters": { diff --git a/website/docs/user-guide/features/browser.md b/website/docs/user-guide/features/browser.md index f5d29713873da..c254e5fcc753d 100644 --- a/website/docs/user-guide/features/browser.md +++ b/website/docs/user-guide/features/browser.md @@ -75,6 +75,13 @@ browser: Cloud browsers need `browser-use auth login` or `BROWSER_USE_API_KEY`. +:::note +Because Browser Use mode executes model-written Python on your machine, the +`browser_exec` tool is only offered to sessions that also have terminal +access. Platforms configured without the terminal toolset (e.g. a locked-down +messaging surface) keep the default browser tools instead. +::: + ### Firecrawl cloud mode To use Firecrawl as your cloud browser provider, add: