browser: click-path allowlist, safe tenant script upgrade, allowlist activation plan
- click/click_ref: pre-click allowlist gate on resolvable anchor href
(same check as navigate; miss refuses before the click executes);
click/click_ref/click_at: post-click landing URL check.
click_at has no pre-click gate by design (coordinates name no
element); JS-driven navigation, evaluate, meta refresh and popups
are documented as unintercepable with the Tinyproxy 403 backstop.
- Tenant script upgrade is now safe: only proven-unmodified scripts
(manifest match, 2a569ac known-shipped SHA-256, missing, or
current) auto-merge; tenant-modified scripts are never overwritten,
are reported as skipped, and get one deduped audit record. The
manifest never learns a tenant hash (else the next upgrade would
mistake a modified file for untouched). Idempotent reruns.
- Activation candidate = deployed Tinyproxy programming set encoded
with leading-dot entries (plain entries would regress
docs.python.org); npmjs.org per the deployed filter, not npmjs.com.
Host activation itself is NOT performed this round: only the plan,
validation tests and rollback are delivered.
Tests: 28 new (click 13, upgrade 8, activation 7), all fake/no-network;
98 browser-related tests pass including prior suites.
(cherry picked from commit e4a93f5748ec815f163d0bc096ff4dc4b8c651f5)
This commit is contained in:
host committed
1 parent
1141d0d0f8
commit
125b436ab3
8 files changed
+1150
-33
No files matched your search
+260
-23
@@ -103,6 +103,23 @@ def _check_allowed_url(url):
|
||||
"domain" % (host, ", ".join(allowed)))
|
||||
|
||||
|
||||
def _check_navigated_url(url):
|
||||
"""Post-navigation / click-target check: same allowlist as
|
||||
_check_allowed_url, but only for http(s) URLs. Anything else
|
||||
is not a page navigation (file:// is always allowed anyway,
|
||||
about:blank / data: / mailto: / javascript: never navigate
|
||||
the page to a host), so it is skipped here instead of
|
||||
raising a misleading non-http(s) error. Unset allowlist
|
||||
means no check at all, same as _check_allowed_url."""
|
||||
if _allowed_hosts() is None:
|
||||
return
|
||||
from urllib.parse import urlparse
|
||||
scheme = (urlparse(str(url)).scheme or "").lower()
|
||||
if scheme not in ("http", "https"):
|
||||
return
|
||||
_check_allowed_url(url)
|
||||
|
||||
|
||||
class Browser:
|
||||
"""Chromium pages (multi-tab). ``browser_factory`` is
|
||||
injectable: tests pass a fake returning a fake
|
||||
@@ -361,15 +378,84 @@ class Browser:
|
||||
result["truncated"] = truncated
|
||||
return result
|
||||
|
||||
def _click_href(self, selector):
|
||||
"""Best-effort anchor href for a click target, or None.
|
||||
|
||||
Reads the first matching element's href (the same element
|
||||
page.click would click). Returns the href resolved
|
||||
against the current page URL, or None when the target has
|
||||
no href, is not an anchor, or the locator API cannot
|
||||
answer (fake pages, non-anchor selectors). A None here
|
||||
does NOT mean allowed: it means the target is unknown
|
||||
before the click, so only the post-click URL check can
|
||||
catch it. Fragment-only hrefs resolve to the current
|
||||
page and are returned as None for the same reason: they
|
||||
never navigate to another host."""
|
||||
try:
|
||||
locator = self._page.locator(str(selector))
|
||||
except Exception: # noqa: BLE001 - unknown target
|
||||
return None
|
||||
target = getattr(locator, "first", locator)
|
||||
get_attr = getattr(target, "get_attribute", None)
|
||||
if get_attr is None:
|
||||
return None
|
||||
try:
|
||||
href = get_attr("href")
|
||||
except Exception: # noqa: BLE001 - unknown target
|
||||
return None
|
||||
if not href or not isinstance(href, str):
|
||||
return None
|
||||
href = href.strip()
|
||||
if not href or href.startswith("#"):
|
||||
return None
|
||||
from urllib.parse import urljoin
|
||||
try:
|
||||
return urljoin(self._current_url or "", href)
|
||||
except Exception: # noqa: BLE001 - unknown target
|
||||
return None
|
||||
|
||||
def _check_click_target(self, selector):
|
||||
"""Pre-click allowlist gate for click/click_ref.
|
||||
|
||||
When the allowlist is unset this is a no-op, so click
|
||||
behaviour is byte-identical to before. When set and the
|
||||
target href resolves to an http(s) URL, the same check
|
||||
as navigate runs BEFORE the click: a miss raises and
|
||||
the click is never executed. Unknown targets (no href,
|
||||
button/JS handler) skip this gate and are caught, if at
|
||||
all, by _check_navigated_current after the click."""
|
||||
if _allowed_hosts() is None:
|
||||
return
|
||||
target = self._click_href(selector)
|
||||
if target is not None:
|
||||
_check_navigated_url(target)
|
||||
|
||||
def _check_navigated_current(self):
|
||||
"""Post-click allowlist detection (not prevention).
|
||||
|
||||
The click already happened, so this can only report a
|
||||
blocked landing host, never undo the navigation. It
|
||||
exists because some click targets cannot be resolved
|
||||
before the click (coordinates, JS handlers, forms): for
|
||||
those, a raised error here is still better than silently
|
||||
continuing on a blocked host, and the egress proxy 403
|
||||
is the layer that actually stops the page load when the
|
||||
allowlist equals the proxy whitelist."""
|
||||
if _allowed_hosts() is None or not self._current_url:
|
||||
return
|
||||
_check_navigated_url(self._current_url)
|
||||
|
||||
def click(self, selector):
|
||||
if not selector or not str(selector).strip():
|
||||
raise BrowserError("selector is required")
|
||||
self._ensure()
|
||||
self._check_click_target(selector)
|
||||
try:
|
||||
self._page.click(str(selector), timeout=10000)
|
||||
self._current_url = self._page.url
|
||||
except Exception as exc: # noqa: BLE001
|
||||
raise BrowserError("click failed: %s" % exc)
|
||||
self._check_navigated_current()
|
||||
result = self._info()
|
||||
result["ok"] = True
|
||||
return result
|
||||
@@ -499,11 +585,16 @@ class Browser:
|
||||
"unknown ref %r: run browser_snapshot_refs first" % (ref,))
|
||||
self._ensure()
|
||||
path = self._ref_paths[ref]
|
||||
# The ref path is a CSS path, so the same href gate as
|
||||
# click applies: an anchor ref to a blocked host is
|
||||
# refused before the click, never executed.
|
||||
self._check_click_target(path)
|
||||
try:
|
||||
self._page.locator(path).first.click(timeout=10000)
|
||||
self._current_url = self._page.url
|
||||
except Exception as exc: # noqa: BLE001
|
||||
raise BrowserError("click_ref failed: %s" % exc)
|
||||
self._check_navigated_current()
|
||||
result = self._info()
|
||||
result["ok"] = True
|
||||
result["ref"] = ref
|
||||
@@ -619,11 +710,21 @@ class Browser:
|
||||
raise BrowserError(
|
||||
"click_at out of viewport 1280x800: %s,%s" % (x, y))
|
||||
self._ensure()
|
||||
# Deliberately NO pre-click gate here: coordinates do not
|
||||
# identify an element, so there is no href to check
|
||||
# before the click. Probing elementFromPoint would be
|
||||
# viewport/scroll-dependent, would miss JS-driven
|
||||
# navigations (the common click_at case: buttons, canvas,
|
||||
# SPA controls), and would run page JS on every user
|
||||
# click. What click_at does get is the post-click URL
|
||||
# check below, plus the egress proxy 403 backstop for
|
||||
# any host outside the proxy whitelist.
|
||||
try:
|
||||
self._page.mouse.click(fx, fy)
|
||||
self._current_url = self._page.url
|
||||
except Exception as exc: # noqa: BLE001
|
||||
raise BrowserError("click_at failed: %s" % exc)
|
||||
self._check_navigated_current()
|
||||
result = self._info()
|
||||
result["ok"] = True
|
||||
result["x"] = fx
|
||||
@@ -725,49 +826,183 @@ class Browser:
|
||||
|
||||
# ------------------------------------------------------- codex-home glue
|
||||
|
||||
PLAYWRIGHT_SCRIPT_NAMES = ("playwright_browser.py", "playwright_mcp.py",
|
||||
"playwright_remote.py")
|
||||
|
||||
# SHA-256 of the three MCP scripts as shipped at origin/main
|
||||
# 2a569ac (PR #272), verified against the git blobs. A tenant
|
||||
# script matching one of these hashes is PROVEN unmodified
|
||||
# (it is exactly what a previous seeding wrote), so the safe
|
||||
# upgrade below may replace it even when no manifest exists
|
||||
# (legacy tenants seeded before manifests were written).
|
||||
# A tenant script matching neither a manifest entry nor a hash
|
||||
# here is treated as tenant-modified and is NEVER overwritten.
|
||||
KNOWN_SHIPPED_SCRIPT_HASHES = {
|
||||
"playwright_browser.py":
|
||||
"0c36a286da3f8733fef4fec2dd18dec003fa68f54385fd4dc8da4b38e7571828",
|
||||
"playwright_mcp.py":
|
||||
"8455e67c2eb2ec6eb07bc9645292e2323b587e01d5a75fd8a985bbcd04a11198",
|
||||
"playwright_remote.py":
|
||||
"15a8026a02dfed6c630ea1614f75d54764a851df9d977c2380e15d0ed4f8a173",
|
||||
}
|
||||
|
||||
_MANIFEST_NAME = "playwright_scripts_manifest.json"
|
||||
_AUDIT_NAME = "playwright_scripts_audit.jsonl"
|
||||
|
||||
|
||||
def _sha256_bytes(data):
|
||||
import hashlib
|
||||
return hashlib.sha256(data).hexdigest()
|
||||
|
||||
|
||||
def _read_script_manifest(home):
|
||||
"""name -> sha256 last written by seeding, or {}.
|
||||
|
||||
The manifest is this helper's own metadata file in the
|
||||
tenant codex-home (not tenant data): it records what the
|
||||
last seeding wrote, so a later upgrade can tell an
|
||||
untouched script from a tenant-modified one. Missing or
|
||||
corrupt manifests read as empty -- the caller then falls
|
||||
back to KNOWN_SHIPPED_SCRIPT_HASHES, never to overwriting
|
||||
an unknown file."""
|
||||
try:
|
||||
with open(os.path.join(home, _MANIFEST_NAME),
|
||||
encoding="utf-8") as fh:
|
||||
data = json.load(fh)
|
||||
except Exception: # noqa: BLE001 - missing/corrupt = no proof
|
||||
return {}
|
||||
if not isinstance(data, dict):
|
||||
return {}
|
||||
return {k: v for k, v in data.items()
|
||||
if isinstance(k, str) and isinstance(v, str)}
|
||||
|
||||
|
||||
def _write_script_manifest(home, hashes):
|
||||
"""Best-effort manifest write; failures never block seeding."""
|
||||
try:
|
||||
with open(os.path.join(home, _MANIFEST_NAME), "w",
|
||||
encoding="utf-8") as fh:
|
||||
json.dump({k: hashes[k] for k in sorted(hashes)},
|
||||
fh, ensure_ascii=False, sort_keys=True)
|
||||
fh.write("\n")
|
||||
except OSError:
|
||||
pass
|
||||
|
||||
|
||||
def _audit_script_skipped(home, name, dst_hash, src_hash):
|
||||
"""Append one audit record for a skipped modified script.
|
||||
|
||||
Idempotent: a record with the same script + tenant hash is
|
||||
never appended twice, so re-running an upgrade does not
|
||||
grow the log. The audit file is append-only metadata; the
|
||||
skipped script itself is left byte-identical."""
|
||||
path = os.path.join(home, _AUDIT_NAME)
|
||||
try:
|
||||
with open(path, encoding="utf-8") as fh:
|
||||
for line in fh:
|
||||
try:
|
||||
rec = json.loads(line)
|
||||
except ValueError:
|
||||
continue
|
||||
if (isinstance(rec, dict)
|
||||
and rec.get("script") == name
|
||||
and rec.get("dst_sha256") == dst_hash
|
||||
and rec.get("action") == "skipped_modified"):
|
||||
return path
|
||||
except OSError:
|
||||
pass
|
||||
record = {"ts": int(time.time()),
|
||||
"action": "skipped_modified",
|
||||
"script": name,
|
||||
"dst_sha256": dst_hash,
|
||||
"src_sha256": src_hash,
|
||||
"note": "tenant-modified script left untouched; "
|
||||
"manual review required before upgrade"}
|
||||
try:
|
||||
with open(path, "a", encoding="utf-8") as fh:
|
||||
fh.write(json.dumps(record, ensure_ascii=False) + "\n")
|
||||
except OSError:
|
||||
pass
|
||||
return path
|
||||
|
||||
|
||||
def ensure_codex_home_playwright(codex_home_dir, command_args=None,
|
||||
script_dir=None):
|
||||
"""Seed playwright MCP files + config into a tenant codex-home.
|
||||
|
||||
MCP scripts are SYSTEM files, not tenant data: a tenant never
|
||||
edits them (user files live in the worktree, not codex-home),
|
||||
so unlike the old never-overwrite rule, a differing script is
|
||||
REFRESHED to the deployed version and reported in
|
||||
``script_refreshed``. Without this, every MCP feature ships
|
||||
dead for existing projects: they keep the first-seeded script
|
||||
forever and only brand-new projects get the update.
|
||||
Appends ``[mcp_servers.playwright]`` only when absent. No
|
||||
secrets live here -- proxy vars arrive via
|
||||
``ensure_playwright_mcp_env`` from worker proxy_env, and the
|
||||
browsers path is a fixed image path.
|
||||
SAFE UPGRADE RULE (changed from the old refresh-anything
|
||||
rule): a tenant script is auto-merged to the deployed
|
||||
version ONLY when it is proven unmodified -- its bytes
|
||||
match the seeding manifest, or match a
|
||||
KNOWN_SHIPPED_SCRIPT_HASHES entry (legacy tenants), or
|
||||
the file is missing (fresh seed) or already current. A
|
||||
script that differs AND matches neither proof is
|
||||
tenant-modified: it is NEVER overwritten, it is reported
|
||||
in ``script_skipped_modified`` (alias ``script_differs``),
|
||||
and one deduped audit record is appended to
|
||||
``playwright_scripts_audit.jsonl`` for manual review.
|
||||
The upgrade never deletes a tenant file and never rewrites
|
||||
existing config semantics: ``[mcp_servers.playwright]``
|
||||
is appended only when absent, and the manifest/audit files
|
||||
are this helper's own additive metadata. No secrets live
|
||||
here -- proxy vars arrive via ``ensure_playwright_mcp_env``
|
||||
from worker proxy_env, and the browsers path is a fixed
|
||||
image path.
|
||||
"""
|
||||
import shutil
|
||||
|
||||
home = os.path.abspath(codex_home_dir)
|
||||
os.makedirs(home, exist_ok=True)
|
||||
here = os.path.abspath(script_dir or os.path.dirname(__file__))
|
||||
manifest = _read_script_manifest(home)
|
||||
script_changed = False
|
||||
script_differs = []
|
||||
script_refreshed = []
|
||||
for name in ("playwright_browser.py", "playwright_mcp.py",
|
||||
"playwright_remote.py"):
|
||||
script_skipped = []
|
||||
audit_path = os.path.join(home, _AUDIT_NAME)
|
||||
new_hashes = {}
|
||||
for name in PLAYWRIGHT_SCRIPT_NAMES:
|
||||
src = os.path.join(here, name)
|
||||
dst = os.path.join(home, name)
|
||||
if not os.path.exists(src):
|
||||
continue
|
||||
with open(src, "rb") as fh:
|
||||
src_bytes = fh.read()
|
||||
src_hash = _sha256_bytes(src_bytes)
|
||||
if not os.path.exists(dst):
|
||||
shutil.copyfile(src, dst)
|
||||
script_changed = True
|
||||
new_hashes[name] = src_hash
|
||||
continue
|
||||
with open(dst, "rb") as fh:
|
||||
dst_bytes = fh.read()
|
||||
dst_hash = _sha256_bytes(dst_bytes)
|
||||
if dst_bytes == src_bytes:
|
||||
new_hashes[name] = src_hash
|
||||
continue
|
||||
proven_unmodified = (
|
||||
manifest.get(name) == dst_hash
|
||||
or KNOWN_SHIPPED_SCRIPT_HASHES.get(name) == dst_hash)
|
||||
if proven_unmodified:
|
||||
shutil.copyfile(src, dst)
|
||||
script_refreshed.append(name)
|
||||
script_changed = True
|
||||
new_hashes[name] = src_hash
|
||||
else:
|
||||
with open(src, "rb") as fh:
|
||||
src_bytes = fh.read()
|
||||
with open(dst, "rb") as fh:
|
||||
dst_bytes = fh.read()
|
||||
if src_bytes != dst_bytes:
|
||||
shutil.copyfile(src, dst)
|
||||
script_refreshed.append(name)
|
||||
script_changed = True
|
||||
continue
|
||||
script_differs.append(name)
|
||||
script_skipped.append(name)
|
||||
_audit_script_skipped(home, name, dst_hash, src_hash)
|
||||
# NEVER record the tenant-modified hash as proven:
|
||||
# writing dst_hash into the manifest would make the
|
||||
# NEXT upgrade mistake this modified file for an
|
||||
# untouched one and overwrite it. Keep any older
|
||||
# proof entry (so a revert to that version can still
|
||||
# auto-merge later); otherwise drop the entry, which
|
||||
# fails closed to skip again.
|
||||
if name in manifest:
|
||||
new_hashes[name] = manifest[name]
|
||||
if new_hashes:
|
||||
_write_script_manifest(home, new_hashes)
|
||||
config_path = os.path.join(home, "config.toml")
|
||||
if command_args is None:
|
||||
command_args = ["python3",
|
||||
@@ -793,7 +1028,9 @@ def ensure_codex_home_playwright(codex_home_dir, command_args=None,
|
||||
"config_changed": config_changed,
|
||||
"script_changed": script_changed,
|
||||
"script_differs": script_differs,
|
||||
"script_refreshed": script_refreshed}
|
||||
"script_refreshed": script_refreshed,
|
||||
"script_skipped_modified": script_skipped,
|
||||
"audit_path": audit_path}
|
||||
|
||||
|
||||
def ensure_playwright_mcp_env(codex_home_dir, env):
|
||||
|
||||
Reference in new issue
Block a user