feat: platform browser multi-tab, navigation allowlist, close cleanup, egress filter, allowlist worker wiring
- Multi-tab: browser_new_page / browser_list_pages / browser_switch_page / browser_close_page in playwright_mcp.py; each new page gets a fresh Playwright context, so cookies and storage are isolated between pages. Single-page tools act on the current page; switching to a missing index is an error, not a crash; closing the last page is an error (use browser_close). - Navigation allowlist: BROWSER_ALLOWED_HOSTS (comma list, leading-dot suffix match, case/port-insensitive) checked in _check_allowed_url before launching Chromium; unset (or empty) means no check, the old behaviour. file:// always allowed; localhost/127.0.0.1 have no special treatment. - Close cleanup: browser_close closes all pages and best-effort removes cwd browser-live.png / browser-live.json; custom screenshot files are untouched. list_pages needs no launch. - Egress filter: 10 programming-source Tinyproxy BASIC rules (python.org, nodejs.org, go.dev, pkg.go.dev, rust-lang.org, developer.mozilla.org, crates.io, stackoverflow.com, cve.mitre.org, nvd.nist.gov); already-present github.com / pypi.org / npmjs.org not duplicated; searx.diante.me stays exact-only, no diante.me wildcard. - Allowlist worker wiring: _docker_task_context now passes a set, non-empty BROWSER_ALLOWED_HOSTS from the worker env into [mcp_servers.playwright.env] in the tenant config, so the MCP process actually sees it (container -e alone does not reach MCP subprocesses, 2026-10-30). Unset/empty writes nothing and never overwrites; the value never goes into logs; it is not added to the websearch env block or the container env. ensure_playwright_mcp_env now returns False without rewriting when the result would be identical (canonical join: one blank line before the block), so a second prep leaves an unchanged tenant config alone; other sections untouched. - Skill + docs: browser-automation SKILL.md multi-tab/allowlist section; docs/platform-browser.md comparison doc (new file). - Tests (mock/fake only, no network, no real Chromium, no real credentials): cloud.tests.test_playwright + cloud.tests.test_websearch_worker, 48 tests OK, including 6 new wiring tests (switch off passes no variable, switch on reaches MCP env verbatim, empty never written, old-tenant merge idempotent + no-rewrite, ensure no-op when identical, allowlist enforces only after wiring). - NOT done (see PR body): no persistent profile, no credential vault, no approval card, no CAPTCHA bypass, in-page link navigation does not pass the allowlist, no real-Chromium live verification, host tinyproxy filter not reloaded.
This commit is contained in:
1 parent
27f48fe1ed
commit
da7a9950fb
8 files changed
+1050
-12
No files matched your search
+253
-11
@@ -1,7 +1,10 @@
|
||||
"""Headless Chromium automation for Codex tasks, via Playwright.
|
||||
|
||||
One Browser object owns one page for the lifetime of an MCP
|
||||
process. It is deliberately small: navigate, snapshot (page text),
|
||||
One Browser object owns a list of pages for the lifetime of an
|
||||
MCP process (multi-tab: new/list/switch/close tools; the
|
||||
single-page tools act on the current page, and each new_page
|
||||
gets a fresh Playwright context, so cookies are isolated
|
||||
between pages). It is deliberately small: navigate, snapshot (page text),
|
||||
click, fill, evaluate, screenshot, close. Anything fancier the
|
||||
agent can do with the shell tool, which has the same sandbox
|
||||
privileges as this browser (tenant Docker container, egress
|
||||
@@ -48,9 +51,65 @@ def _validate_basename(name, what):
|
||||
return name
|
||||
|
||||
|
||||
def _allowed_hosts():
|
||||
"""BROWSER_ALLOWED_HOSTS, or None when unset/empty.
|
||||
|
||||
Comma list, case-insensitive. An entry with a leading dot
|
||||
(e.g. .python.org) matches the bare domain and any subdomain;
|
||||
a plain entry matches exactly. localhost/127.0.0.1 are NOT
|
||||
special-cased: they are allowed only when listed explicitly.
|
||||
Unset (or empty) means no allowlist check at all -- the old
|
||||
behaviour, where only the egress proxy whitelist applies.
|
||||
Read on every navigate so tests and env changes take effect
|
||||
without recreating the Browser."""
|
||||
raw = os.environ.get("BROWSER_ALLOWED_HOSTS")
|
||||
if raw is None:
|
||||
return None
|
||||
hosts = [entry.strip().lower() for entry in raw.split(",")]
|
||||
hosts = [entry for entry in hosts if entry]
|
||||
return hosts or None
|
||||
|
||||
|
||||
def _check_allowed_url(url):
|
||||
"""Raise BrowserError before launch when the allowlist is set
|
||||
and the URL is not allowed. file:// is always allowed; every
|
||||
other scheme except http(s) is blocked when the allowlist is
|
||||
set, because it has no host to match."""
|
||||
allowed = _allowed_hosts()
|
||||
if allowed is None:
|
||||
return
|
||||
from urllib.parse import urlparse
|
||||
parsed = urlparse(str(url))
|
||||
scheme = (parsed.scheme or "").lower()
|
||||
if scheme == "file":
|
||||
return
|
||||
if scheme not in ("http", "https"):
|
||||
raise BrowserError(
|
||||
"host allowlist blocks non-http(s) URL %r when "
|
||||
"BROWSER_ALLOWED_HOSTS is set" % (str(url),))
|
||||
host = (parsed.hostname or "").lower()
|
||||
if not host:
|
||||
raise BrowserError(
|
||||
"host allowlist blocks URL with no host %r" % (str(url),))
|
||||
for entry in allowed:
|
||||
if entry.startswith("."):
|
||||
if host == entry[1:] or host.endswith(entry):
|
||||
return
|
||||
elif host == entry:
|
||||
return
|
||||
raise BrowserError(
|
||||
"host %r is not in BROWSER_ALLOWED_HOSTS (%s); "
|
||||
"add it (or a leading-dot suffix entry) to allow this "
|
||||
"domain" % (host, ", ".join(allowed)))
|
||||
|
||||
|
||||
class Browser:
|
||||
"""A single Chromium page. ``browser_factory`` is injectable:
|
||||
tests pass a fake returning a fake sync_playwright object."""
|
||||
"""Chromium pages (multi-tab). ``browser_factory`` is
|
||||
injectable: tests pass a fake returning a fake
|
||||
sync_playwright object. Single-page methods act on the
|
||||
current page; new_page/switch_page/close_page manage the
|
||||
page list, and each new_page gets a fresh Playwright
|
||||
context, so cookies are isolated between pages."""
|
||||
|
||||
def __init__(self, cwd=None, proxy=None, browser_factory=None):
|
||||
self.cwd = os.path.abspath(cwd or os.getcwd())
|
||||
@@ -60,6 +119,8 @@ class Browser:
|
||||
self._pw = None
|
||||
self._browser = None
|
||||
self._page = None
|
||||
self._pages = []
|
||||
self._current_index = 0
|
||||
self._current_url = ""
|
||||
self._last_title = ""
|
||||
# ref snapshot state (playwright-mcp style): ref -> css path
|
||||
@@ -110,6 +171,8 @@ class Browser:
|
||||
self._pw = pw
|
||||
self._browser = browser
|
||||
self._page = page
|
||||
self._pages = [page]
|
||||
self._current_index = 0
|
||||
|
||||
def _info(self):
|
||||
try:
|
||||
@@ -128,6 +191,9 @@ class Browser:
|
||||
def navigate(self, url):
|
||||
if not url or not str(url).strip():
|
||||
raise BrowserError("url is required")
|
||||
# Before _ensure on purpose: a blocked host must fail
|
||||
# without launching Chromium at all.
|
||||
_check_allowed_url(url)
|
||||
self._ensure()
|
||||
try:
|
||||
self._page.goto(str(url), timeout=NAV_TIMEOUT_MS,
|
||||
@@ -141,6 +207,146 @@ class Browser:
|
||||
result["ok"] = True
|
||||
return result
|
||||
|
||||
def _parse_page_index(self, index):
|
||||
"""Shared switch/close index validation. Raises
|
||||
BrowserError with the page count, never IndexError."""
|
||||
if isinstance(index, bool):
|
||||
raise BrowserError(
|
||||
"page index must be an integer, got %r" % (index,))
|
||||
try:
|
||||
idx = int(index)
|
||||
except (TypeError, ValueError):
|
||||
raise BrowserError(
|
||||
"page index must be an integer, got %r" % (index,))
|
||||
if isinstance(index, float) and idx != index:
|
||||
raise BrowserError(
|
||||
"page index must be an integer, got %r" % (index,))
|
||||
if not self._pages:
|
||||
raise BrowserError(
|
||||
"no pages open: call browser_new_page or navigate "
|
||||
"first")
|
||||
if not 0 <= idx < len(self._pages):
|
||||
raise BrowserError(
|
||||
"no such page: index %d (%d page(s) open, valid "
|
||||
"0-%d)" % (idx, len(self._pages),
|
||||
len(self._pages) - 1))
|
||||
return idx
|
||||
|
||||
def _stop_screencast(self):
|
||||
if self._cdp is not None:
|
||||
try:
|
||||
self._cdp.send("Page.stopScreencast")
|
||||
except Exception: # noqa: BLE001 - best-effort
|
||||
pass
|
||||
self._cdp = None
|
||||
self._cdp_page = None
|
||||
|
||||
def _sync_current_url(self):
|
||||
try:
|
||||
self._current_url = self._page.url or ""
|
||||
except Exception: # noqa: BLE001 - url is best-effort
|
||||
self._current_url = ""
|
||||
|
||||
def new_page(self):
|
||||
"""Open a new page and make it current. Every new_page
|
||||
gets a fresh Playwright context (browser.new_page()),
|
||||
so cookies and storage are isolated from other pages.
|
||||
Single-page tools then act on this page until a switch."""
|
||||
self._ensure()
|
||||
try:
|
||||
page = self._browser.new_page(
|
||||
viewport={"width": 1280, "height": 800})
|
||||
except Exception as exc: # noqa: BLE001
|
||||
raise BrowserError("new_page failed: %s" % exc)
|
||||
self._stop_screencast()
|
||||
self._pages.append(page)
|
||||
self._current_index = len(self._pages) - 1
|
||||
self._page = page
|
||||
self._ref_paths = {}
|
||||
self._sync_current_url()
|
||||
result = self._info()
|
||||
result["ok"] = True
|
||||
result["index"] = self._current_index
|
||||
result["count"] = len(self._pages)
|
||||
return result
|
||||
|
||||
def list_pages(self):
|
||||
"""List all open pages without launching a browser when
|
||||
none is open. Titles/URLs are best-effort per page."""
|
||||
if not self._pages:
|
||||
return {"pages": [], "current": None, "count": 0}
|
||||
pages = []
|
||||
for i, page in enumerate(self._pages):
|
||||
try:
|
||||
url = page.url or ""
|
||||
except Exception: # noqa: BLE001 - best-effort
|
||||
url = ""
|
||||
try:
|
||||
title = page.title() or ""
|
||||
except Exception: # noqa: BLE001 - best-effort
|
||||
title = ""
|
||||
pages.append({"index": i, "url": url, "title": title,
|
||||
"current": i == self._current_index})
|
||||
return {"pages": pages, "current": self._current_index,
|
||||
"count": len(pages)}
|
||||
|
||||
def switch_page(self, index):
|
||||
"""Make page ``index`` current. Single-page tools then
|
||||
act on it; refs from the previous page are dropped,
|
||||
because they map to that page's DOM."""
|
||||
idx = self._parse_page_index(index)
|
||||
if idx != self._current_index:
|
||||
self._stop_screencast()
|
||||
self._current_index = idx
|
||||
self._page = self._pages[idx]
|
||||
self._ref_paths = {}
|
||||
self._sync_current_url()
|
||||
result = self._info()
|
||||
result["ok"] = True
|
||||
result["index"] = self._current_index
|
||||
result["count"] = len(self._pages)
|
||||
return result
|
||||
|
||||
def close_page(self, index=None):
|
||||
"""Close one page. Closing the last page is an error --
|
||||
use browser_close for the whole browser. Closing the
|
||||
current page switches to the previous page (index-1, or
|
||||
0 when the first page was closed); closing a page before
|
||||
the current one keeps the same page current (its index
|
||||
shifts down by one)."""
|
||||
if not self._pages:
|
||||
raise BrowserError(
|
||||
"no pages open: call browser_new_page or navigate "
|
||||
"first")
|
||||
if len(self._pages) == 1:
|
||||
raise BrowserError(
|
||||
"cannot close the last page (index 0): use "
|
||||
"browser_close to close the whole browser")
|
||||
idx = (self._current_index if index is None
|
||||
else self._parse_page_index(index))
|
||||
page = self._pages[idx]
|
||||
try:
|
||||
page.close()
|
||||
except Exception as exc: # noqa: BLE001
|
||||
raise BrowserError("close_page failed: %s" % exc)
|
||||
del self._pages[idx]
|
||||
if self._cdp_page is page:
|
||||
self._cdp = None
|
||||
self._cdp_page = None
|
||||
if idx < self._current_index:
|
||||
self._current_index -= 1
|
||||
elif idx == self._current_index:
|
||||
self._current_index = idx - 1 if idx > 0 else 0
|
||||
self._page = self._pages[self._current_index]
|
||||
self._ref_paths = {}
|
||||
self._sync_current_url()
|
||||
result = self._info()
|
||||
result["ok"] = True
|
||||
result["closed_index"] = idx
|
||||
result["index"] = self._current_index
|
||||
result["count"] = len(self._pages)
|
||||
return result
|
||||
|
||||
def snapshot(self):
|
||||
self._ensure()
|
||||
try:
|
||||
@@ -482,8 +688,17 @@ class Browser:
|
||||
pass
|
||||
self._cdp = None
|
||||
self._cdp_page = None
|
||||
for closer in (lambda: self._page and self._page.close(),
|
||||
lambda: self._browser and self._browser.close(),
|
||||
for page in list(self._pages):
|
||||
try:
|
||||
page.close()
|
||||
except Exception as exc: # noqa: BLE001 - close is best-effort
|
||||
errors.append(str(exc))
|
||||
if self._page is not None and self._page not in self._pages:
|
||||
try:
|
||||
self._page.close()
|
||||
except Exception as exc: # noqa: BLE001 - close is best-effort
|
||||
errors.append(str(exc))
|
||||
for closer in (lambda: self._browser and self._browser.close(),
|
||||
lambda: self._pw and self._pw.stop()):
|
||||
try:
|
||||
closer()
|
||||
@@ -492,6 +707,19 @@ class Browser:
|
||||
self._page = None
|
||||
self._browser = None
|
||||
self._pw = None
|
||||
self._pages = []
|
||||
self._current_index = 0
|
||||
self._ref_paths = {}
|
||||
# Live-view files are per-browser, not per-shot: closing
|
||||
# the browser must not leave a stale browser-live.png /
|
||||
# browser-live.json for a watcher to mistake for a live
|
||||
# page. Failures are ignored on purpose (close is
|
||||
# best-effort; a missing file is the normal case).
|
||||
for name in ("browser-live.png", "browser-live.json"):
|
||||
try:
|
||||
os.remove(os.path.join(self.cwd, name))
|
||||
except OSError:
|
||||
pass
|
||||
return {"closed": True, "errors": errors}
|
||||
|
||||
|
||||
@@ -576,7 +804,9 @@ def ensure_playwright_mcp_env(codex_home_dir, env):
|
||||
proxy vars and PLAYWRIGHT_BROWSERS_PATH must live in
|
||||
config.toml. Idempotent in-place replacement of the env
|
||||
subsection; other sections untouched. Returns True when the
|
||||
file was written.
|
||||
file was written, False without rewriting when the resulting
|
||||
file would be identical to the existing one (so a second
|
||||
task prep for an unchanged tenant leaves the file alone).
|
||||
"""
|
||||
if not env:
|
||||
return False
|
||||
@@ -589,6 +819,7 @@ def ensure_playwright_mcp_env(codex_home_dir, env):
|
||||
existing = fh.read()
|
||||
except OSError:
|
||||
return False
|
||||
original = existing
|
||||
start = existing.find("[mcp_servers.playwright.env]")
|
||||
if start >= 0:
|
||||
end = existing.find("\n[", start + 1)
|
||||
@@ -598,12 +829,23 @@ def ensure_playwright_mcp_env(codex_home_dir, env):
|
||||
lines = ["\n[mcp_servers.playwright.env]\n"]
|
||||
for key in sorted(env):
|
||||
lines.append("%s = %s\n" % (key, json.dumps(str(env[key]))))
|
||||
# Canonical join: the old write (stripped body + lines[0]'s
|
||||
# leading blank line) grew one blank line per call, so a
|
||||
# second prep never produced identical bytes. Normalise
|
||||
# instead: body without trailing newlines, exactly one blank
|
||||
# line, then the block; an empty body keeps a single leading
|
||||
# blank line. This converges in one rewrite and is stable
|
||||
# afterwards, which the no-op check below relies on.
|
||||
block = "".join(lines).lstrip("\n")
|
||||
if existing.strip():
|
||||
new_text = existing.rstrip("\n") + "\n\n" + block
|
||||
else:
|
||||
new_text = "\n" + block
|
||||
if new_text == original:
|
||||
return False
|
||||
try:
|
||||
with open(config_path, "w", encoding="utf-8") as fh:
|
||||
fh.write(existing)
|
||||
if existing and not existing.endswith("\n"):
|
||||
fh.write("\n")
|
||||
fh.writelines(lines)
|
||||
fh.write(new_text)
|
||||
except OSError:
|
||||
return False
|
||||
return True
|
||||
Reference in new issue
Block a user