fix(browser): extend private-network guard to browser_console (salvage #54477) - #321
Conversation
|
Review Complete Files Reviewed: 2 By Severity:
PR #321 introduces SSRF protections for the browser tool but ships with a critical ImportError that completely breaks cloud browser URL safety checks, plus a JavaScript unicode escape bypass in the eval denylist, a missing scroll input guard, and a dict-key redaction gap. Files Reviewed (2 files) |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Risk: 🔴 Critical (90/100) — 1 critical finding, 1 high, 1 medium · 424 LOC across 2 files
Critical Findings
Missing sensitive_query_param_name in url_safety.py — browser_tool.py:117 imports sensitive_query_param_name from tools.url_safety but the function is never defined in the codebase (confirmed via full file read of 409 lines and repo-wide grep). The except Exception catch at line 119 rebinds ALL four imported names to fail-closed fallbacks, making _is_safe_url return False for every URL and _is_always_blocked_url return True for every URL. This completely breaks browser navigation for all cloud backend users. The new credential-query-param protection at line 2725 is dead code since the fallback always returns None.
JavaScript \u{NNNNN} unicode escape bypass — browser_tool.py:3431-3444 uses Python's unicode_escape codec to decode JS escapes before token-checking, but Python does not handle the ES2015+ \u{NNNNN} code point form. Expressions like document["co\u{006f}kie"] pass through unescaped, bypassing the denylist for 'cookie' and all other sensitive tokens (fetch, XMLHttpRequest, WebSocket, localStorage, etc.).
High Findings
browser_scroll missing private-page input guard — browser_tool.py:3134 — browser_click, browser_type, and browser_press all have _blocked_private_page_action guards added, but browser_scroll does not, leaving a gap in the SSRF interaction surface for cloud backends.
Medium Findings
Dictionary keys bypass redaction — browser_tool.py:2671 — _redact_browser_output recursively redacts string values but dictionary keys pass through unredacted. If a secret appears as a dict key (e.g., a token used as a localStorage key name), it leaks to the model.
| sensitive_query_param_name as _sensitive_query_param_name, | ||
| ) | ||
| except Exception: | ||
| _is_safe_url = lambda url: False # noqa: E731 — fail-closed: block all if safety module unavailable | ||
| _is_always_blocked_url = lambda url: True # noqa: E731 — fail-closed on the floor too | ||
| _normalize_url_for_request = lambda url: url # noqa: E731 — best-effort fallback | ||
| _sensitive_query_param_name = lambda url: None # noqa: E731 — best-effort fallback |
There was a problem hiding this comment.
🔴 Missing sensitive_query_param_name in url_safety.py cascades to break ALL browser URL safety checks (security)
The PR adds sensitive_query_param_name as _sensitive_query_param_name to the multi-name import from tools.url_safety at tools/browser_tool.py:117. This function is never defined anywhere in the codebase (confirmed via full read of tools/url_safety.py — 409 lines, no such function; grep across entire repo yields zero definitions). In Python, from X import (a, b, c, d) raises ImportError when any name in the tuple is missing, which the except Exception at line 119 catches. The except block rebinds ALL four names to their fail-closed fallbacks: _is_safe_url = lambda url: False (treats every URL as unsafe), _is_always_blocked_url = lambda url: True (treats every URL as always-blocked), _normalize_url_for_request = lambda url: url (identity), _sensitive_query_param_name = lambda url: None (never detects anything). This means: (a) every browser_navigate call is blocked at the always-blocked floor check (line 2743), completely breaking browser navigation for cloud backends; (b) the new credential-query-param protection at line 2725 is dead code since _sensitive_query_param_name always returns None; (c) _is_safe_url always returns False, blocking all URLs at the SSRF check (line 2755). For local backends (Camofox), the _is_local_backend() guard at line 2743 may mask this, but cloud browser users are completely broken.
💡 Suggestion: Either implement sensitive_query_param_name(url: str) -> Optional[str] in tools/url_safety.py that returns the name of a credential-like query parameter if present, or split the import into two separate try/except blocks so the new import's failure does not cascade to _is_safe_url, _is_always_blocked_url, and _normalize_url_for_request.
📋 Prompt for AI Agents
In tools/url_safety.py, add a function sensitive_query_param_name(url: str) -> Optional[str] that: (1) parses the URL via urllib.parse.urlparse, (2) extracts query parameter names via urllib.parse.parse_qs, (3) compares each lowercased key against a frozenset of sensitive names (access_token, refresh_token, id_token, token, api_key, apikey, client_secret, password, auth, jwt, session, secret, key, code, signature, x-amz-signature — mirroring agent/redact._SENSITIVE_QUERY_PARAMS), and (4) returns the first matching key name or None. Alternatively, as a stopgap, split the import in browser_tool.py lines 112-123 into two separate try/except blocks so the missing function does not cascade to break _is_safe_url and _is_always_blocked_url.
| def _decode_js_string_literal(literal: str) -> str: | ||
| """Best-effort decode of a JavaScript string literal for policy checks. | ||
|
|
||
| This is not a JS parser. It only normalizes common escaped property names | ||
| such as ``document["co\\x6fkie"]`` before the fail-closed sensitive-token | ||
| check below. | ||
| """ | ||
| if len(literal) < 2: | ||
| return literal | ||
| body = literal[1:-1] | ||
| try: | ||
| return bytes(body, "utf-8").decode("unicode_escape") | ||
| except Exception: | ||
| return body |
There was a problem hiding this comment.
🟠 JavaScript \u{NNNNN} unicode code point escape bypasses browser eval policy denylist (security)
The _decode_js_string_literal function (browser_tool.py:3431-3444) uses Python's unicode_escape codec to decode JavaScript escape sequences before checking for sensitive tokens. Python's unicode_escape does not handle the JavaScript \u{NNNNN} unicode code point escape form (ES2015+). An expression like document["co\u{006f}kie"] (where \u{006f} is a JS code point escape evaluating to 'o') reaches Python as co\u{006f}kie. The unicode_escape decode fails because { is not a valid hex digit after \u, the except handler returns the raw body unchanged, and the sensitive-token check for 'cookie' misses it because 'cookie' is not a substring of co\u{006f}kie. The same bypass applies to ALL tokens in _SENSITIVE_BROWSER_EVAL_TOKENS: 'fetch', 'XMLHttpRequest', 'WebSocket', 'localStorage', 'indexedDB', etc. This allows an attacker to read cookies, access storage, and make network requests through browser_console(expression=...) by encoding sensitive API names with \u{NNNNN} escapes.
💡 Suggestion: Add a pre-pass to convert JavaScript \u{NNNNN} code point escapes to \uNNNN form that Python's unicode_escape understands, before the decode call at line 3442.
📋 Prompt for AI Agents
In _decode_js_string_literal at tools/browser_tool.py line 3441, add a regex substitution before the bytes(body, 'utf-8').decode('unicode_escape') call: body = re.sub(r'\\u\{([0-9a-fA-F]{1,6})\}', lambda m: '\\u' + m.group(1).zfill(4), body). This converts e.g. \u{6f} to \u006f which unicode_escape correctly decodes to 'o'. Place it inside the try block, before the decode call.
| if isinstance(value, dict): | ||
| return {key: _redact_browser_output(item) for key, item in value.items()} | ||
| return value |
There was a problem hiding this comment.
🟡 Dictionary keys bypass redaction in browser output redactor (security)
The _redact_browser_output function at line 2655 recursively redacts string values in nested structures but does NOT redact dictionary keys at line 2672: return {key: _redact_browser_output(item) for key, item in value.items()}. The key is used raw without redaction. If browser eval results or localStorage/datastore dumps contain secrets as dictionary keys — e.g., a prefixed API token used as a localStorage key name — the key passes through to the model unredacted while the value is properly redacted.
💡 Suggestion: Pass dictionary keys through _redact_browser_output as well: {_redact_browser_output(key): _redact_browser_output(item) for key, item in value.items()}.
📋 Prompt for AI Agents
In _redact_browser_output at tools/browser_tool.py line 2672, change {key: _redact_browser_output(item) for key, item in value.items()} to {_redact_browser_output(key): _redact_browser_output(item) for key, item in value.items()}. This ensures dictionary keys receive the same redaction treatment as values.
1 participant
Summary
Seals the last read path the browser SSRF sweep missed:
browser_console()console-output mode now re-checks the current page URL before returning console logs and exception details, so it can't leak content from a private/internal page reached via eval navigation.Salvage of NousResearch#54477 by @necoweb3, cherry-picked onto current
mainwith authorship preserved.Root cause
browser_console()has two modes. Theexpression=...mode routes through_browser_eval, which is already guarded. The no-expressionconsole-output mode fetchedconsole/errorsand returned them without re-checking the page URL — unlike its sibling read tools (browser_snapshot,browser_vision,_browser_eval,browser_get_images), which all re-check after a possible eval-driven navigation to a private address.Changes
tools/browser_tool.py: add the_eval_ssrf_guard_active()+_current_page_private_url()check to console-output mode, before the command fetch runs, matching thebrowser_get_imagessibling pattern (error wording included). Blocking early means the private page is never queried at all.tests/tools/test_browser_console_ssrf.py: regression coverage — private-page block, public-page allow, local-backend bypass,allow_private_urlsbypass, failed-command path.Validation
test_browser_console_ssrf+ snapshot/vision/eval/get_images SSRF tests — 39/39 green.browser_consolewith isolatedHERMES_HOME: private page blocked with zero secret leakage, public page flows, local backend bypasses.Infographic
Nous Research
Mirror-of: NousResearch#56373
NousResearch#56373