feat(security): add host origin allowlist on start - #170
Conversation
|
Please review this one first. #169 (file upload) was branched from the same |
|
macOS CI failed on the runner SDK, not the allowlist: |
|
The macOS failure was the SDK probe, not the runner. |
Restrict agent navigation with repeatable `headless start --allow PATTERN`. The matcher is an extra conjunct in agentMayNavigate; ping reports the active list, and changing it on a running host requires stop first.
agentMayNavigate now references NavigationAllowlist. The macOS SDK selection typechecks Protocol.swift in isolation, so every SDK looked incompatible and CI reported a missing toolchain.
be5f0cd to
703bd93
Compare
|
Rebased onto current main. Mergeable. Keep this ahead of #169 if both land; they still overlap on protocol files. |
SarthakWade
left a comment
There was a problem hiding this comment.
The allowlist contract is not yet enforced across every claimed path, so this should not merge yet.
The main blocker is Linux navigation triggered by a non-anchor control. The isolated runtime only preflights HTML anchors. A form submit button or trusted click handler that assigns location is allowed to execute, then the existing Page.frameRequestedNavigation recovery runs asynchronously. That cannot make the click fail with UNSAFE_NAVIGATION and does not guarantee an off-list request was never sent. A trusted click can also open a new Chromium target, and the browser process does not auto-attach that target to the session enforcement path. Please enforce the policy before request dispatch or at target creation, then cover form submission, script-driven top-frame navigation, redirects, and new-window attempts in Linux E2E.
There are two launcher correctness gaps as well: the post-spawn polling loop returns the first successful ping without checking that it carries the requested allowlist, and list comparison is order-sensitive even though pattern order has no semantic meaning.
This PR touches the macOS host and adds macOS E2E assertions, but the WKWebView E2E job was skipped. Please run it before merge as required by the repository contract.
GitHub currently reports the PR as mergeable with no conflicts. All required branch checks are green, and I also ran pnpm test (60/60 protocol tests) plus pnpm test:runtime locally. The separate linux-arm64 Release check failed during an existing authentication-storage-state E2E with a closed DevTools pipe; it is not a required branch check, but should be rerun or confirmed as unrelated.
Fail off-list Document navigations at Fetch.requestPaused, close extra page targets, and revalidate every start ping as a set.
|
Addressed the CHANGES_REQUESTED review. P1 Linux is the real boundary: when the allowlist is restricted, Fetch is enabled for Document Request (merged with mock URL patterns, never dropped when mocks change). E2E on linux and macos now covers form submit, script navigation, P1 P2 allowlist order Also added the |
Target.attachedToTarget also fires for attachToTarget. Closing extra page targets in that path would kill the agent session when no allowlist is set.
waitForDebuggerOnStart pauses window.open targets. Closing that session without Runtime.runIfWaitingForDebugger wedged the DevTools pipe, so inspect after an off-list popup returned HOST_UNAVAILABLE.
|
Follow-up for the Linux E2E failure after |
SarthakWade
left a comment
There was a problem hiding this comment.
The allowlist enforcement fixes and stable session-state assertions have been re-reviewed. Protocol, runtime, Linux Docker E2E, and macOS WKWebView E2E all pass on the latest head.
Summary
Adds a host-enforced origin allowlist so a prompt-injected agent cannot leave the app under test.
headless start --allow PATTERNrestricts visit, top-frame navigation, and in-page clicks; omitting--allowkeeps today's unrestricted HTTP(S) behavior.Contract
--allow; comma-separated values in one flag also accepted.--allowcannot addfile:,javascript:, credentials, or blocked extensions.normalizedWebURLis unchanged; the allowlist is an extra conjunct inagentMayNavigate.--allowis a parse error.*alone, paths, schemes, credentials, non-ASCII, and unexpected characters fail closed. Cap 32 patterns; case-insensitive DNS; IPv4 literals as exact hosts.*.example.commatches subdomains, not the apex, and notexample.com.evil.test.localhostmatches any port;localhost:3000matches only that port.UNSAFE_NAVIGATION.status/ ping includesnavigationAllowlist(empty array means unrestricted).NAVIGATION_ALLOWLIST_CONFLICT— stop first). Matching list orstartwithout--allowagainst a running host is a no-op.HEADLESS_NAVIGATION_ALLOWLISTon the spawned host. The injected runtime gets a JSON-encoded copy as defense in depth; host policy is authoritative.Tests
*/ schemes / credentials / empty--allow/ too many patterns, matcher (wildcard non-match, apex vs subdomain, port exactness, unrestricted when empty), visit still validatesexample.comas a URL, host-side deny viaagentMayNavigate, ping reports the list.__headlessNavigationAllowlist.start --allow 127.0.0.1, fixture visit OK,https://example.comand off-list click failUNSAFE_NAVIGATION, mismatch requires stop, restore unrestricted host.pnpm test:runtimepassed. Localpnpm testcannot run Swift on this Linux box (exit 69); the Linux Docker builder ran 46/46 protocol tests and Linux P2 E2E passed, including the new allowlist block.Review
@SarthakWade please review before morning.
Closes #167