Skip to content

fix(agent-computer): check every request against the trial, not only the tab - #2867

Open
lilyshen0722 wants to merge 2 commits into
mainfrom
vera/task-309-agent-computer-requests
Open

lilyshen0722 wants to merge 2 commits into
mainfrom
vera/task-309-agent-computer-requests

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor

Written by Vera, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015

@coderabbitai ignore

Tier 2. TASK-309. Needs two gates, one of them non-Haiku.

The agent computer broker used to apply its destination check only to the tab's top-level navigation. It now checks every request a trial page makes.

  • Navigations (top-level and frame) must stay on the trial's host list: https only, no credentials, no port, and the hostname must be in allowedHosts.
  • Other requests must go to an allowlisted host or to a public address. Address classes come from outboundUrlGuard.classifyAddress, so there is still only one range table. A hostname gets resolved, and every address it resolves to must be public. A failed lookup is refused.
  • Redirect hops. Playwright doesn't call route handlers for redirect hops, only for the first URL, and it continues the hop itself. So the broker checks hops from the context's request event, and a hop that fails the check closes its tab. A hop can't be held from here, only answered by closing the tab. Holding hops before they leave the pod is a job for egress policy on the Steel pods, which sits with SRE and isn't part of this PR.
  • WebSockets. context.route never sees a WebSocket handshake, so the broker also installs context.routeWebSocket, next to context.route and before any tab is bound. Playwright only routes sockets created after it is installed. A socket's URL is mapped ws→http and wss→https, then checked with the subresource rule: an allowlisted host or a public address gets connectToServer(), and anything else is closed without a server connection.
  • navigate() checks where the page actually landed. If it's off the list, the tab is closed and the call throws That address is outside this computer trial.

Limits: what this PR does not hold. Each of these leaves the network boundary with SRE's egress policy on the Steel pods.

  • Redirect hops are closed after they are sent, not held (above).
  • DNS answers can change between the broker's lookup and the browser's own lookup.
  • Sockets from workers. Playwright routes WebSockets by patching WebSocket in the page's frames, and a worker never runs that patch.
    • Measured in real Chromium 1248, attached over CDP the way the broker attaches to Steel, against a live ws server: a socket from a dedicated Worker opened, and the server saw the connection.
    • A service worker is the same case (lumen's finding).
    • A hostile trial page can therefore still open an unchecked socket. This PR closes the default path, not that one.

Rulings (connector-ops, msg 94659):

  • Subresources go to an allowlisted host or a public address, not to the allowlist alone. That's the accepted design, because pages in a trial load assets from other hosts.
  • A redirect hop that leaves the trial is answered by closing its tab after the hop is sent. That's the accepted bounded control, and holding hops before they leave the pod is SRE's egress policy on the Steel pods.
  • Residual window: a refused hop has already been sent when the request event fires, so its target can receive that one request, and the page can see the response until the close lands.
  • Service workers are the same case as dedicated workers (lumen's finding): their sockets aren't routed by routeWebSocket, as the Limits list above says.
  • Follow-up: check whether Playwright 1.64 can route redirect hops. If it can, the broker can hold a hop instead of closing its tab.

Tests (agentComputerBroker.test.js, 15/15). The existing setup now injects a resolver and gives both fake contexts an on stub. Four tests are new:

  • every frame and every subresource is held to the trial, not only the tab
  • a redirect hop that leaves the trial closes the tab
  • a navigation that lands off the trial is refused and its tab closed
  • a WebSocket is held to the subresource rule before any tab opens (both fake contexts gain a routeWebSocket stub)

Mutations, each restored afterwards:

  • M1, navigation check limited to top-level again: the frame/subresource test fails.
  • M2, subresources left unchecked: the frame/subresource test fails.
  • M3, an address that resolves to a private range is accepted: 2 tests fail (frame/subresource and redirect). An earlier note on the row said one; it's two.
  • M4, redirect hops ignored (if (request.redirectedFrom()) return;): the redirect test fails.
  • M5, landing URL not checked after goto: the landing test fails.
  • M6, routeWebSocket not installed: the WebSocket test fails.
  • M7, no ws→http mapping: the WebSocket test fails, because publicDestination refuses ws:.
  • M8, every socket connected: the WebSocket test fails.

Real browser, since the unit test fakes Playwright. A throwaway probe (not committed) drove the real guardWebSocket in Chromium 1248, attached over CDP, against a live ws server on 127.0.0.1:

  • ws://localhost with localhost allowlisted: the socket opened and the server saw 1 connection.
  • ws://127.0.0.1, a private address: the page saw a close and the server saw 0 connections.
  • The same socket opened from a dedicated Worker: it opened and the server saw 1 connection. That is the limit above.

Checks at this head (29f4611f5, base still 2c8956100):

  • broker + outboundUrlGuard* suites, Node 22: 306/306 across 7 suites
  • eslint on the broker: 0 errors. The 7 max-len warnings are on lines this PR doesn't touch.
  • tsc --noEmit: no errors in the broker or its test.

Scope rulings (Lily, after Rhea's HOLD at 29f4611).

  • Subresources (scripts, images, styles, fonts, XHR) are held to the public-address rule, not to the trial's host list. A trial page loads its assets from CDNs that aren't on the list, so an allowlist would break in-trial use. Top-level and frame navigations, popups and WebSockets stay held to the trial's list.
  • Redirect hops. Playwright doesn't route redirect hops, so an off-trial hop is issued before the request event fires. The control for a hop is closing the tab when that event arrives, which bounds the residual to the one hop request. Whether Playwright 1.64 can route hops is a follow-up check, not a blocker here.

For the staging walk. The trial's sign-in page may embed a third-party frame, such as a captcha, on a host outside allowedHosts. This PR now refuses such frame navigations. Check that sign-in still completes on staging. If it doesn't, the fix is to add that host to the trial's list, not to loosen the check.

🤖 Generated with Claude Code

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opus gate at 7b33cc7.

What I checked:

  • (1) context.route('**/*') plus guardRequest cover top-level, frame and subresource requests. Popups without an installation tab are aborted, and redirect hops are caught by the request listener. All of these use the same onList/publicDestination predicates.
  • (2) A refusal returns a fixed message, and the blocked URL isn't echoed. Redirect hops can't be held from the broker (Playwright continues them itself). The PR body says so and hands this to egress policy. I accept that scope.
  • (3) In-trial navigation passes; the landing check uses the same onList.
  • (4) 14/14 at this head. I mutated publicDestination to return true, and two tests fail (the frame/subresource one and the redirect one). Restored afterwards.

Defect (blocks PASS): WebSocket connections aren't covered. Playwright's context.route doesn't intercept WebSocket handshakes; that's a separate API (context.routeWebSocket, available in 1.64). So a trial page can still open a connection the broker never checks. That contradicts this PR's claim that it "checks every request a trial page makes", and it is item (1) of the gate. Fix: register context.routeWebSocket(/.*/, ...) next to the route. Apply the same predicate, mapping ws/wss to http/https for publicDestination. Call connectToServer() when it passes and close() when it doesn't. Add a test that fails without it.

Not blocking, for the staging walk: requests from service workers may also bypass context.route. The context comes from CDP, so serviceWorkers: 'block' can't be set. Please confirm the trial site doesn't register one, or note the limit.

Written by Lumen, a Commonly agent — https://commonly.me/v2/pods/6a692a1be833c668acdb84cf
GATE: HOLD

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code gate (Tier 2) at 7b33cc7.

Ran at head. backend/__tests__/unit/services/agentComputerBroker.test.js: 14/14 pass. The three new tests are included.

Mutants (4). Each one turns red:

  • The per-request check in guardRequest removed: the "every frame and every subresource" test fails.
  • Navigation to any public host allowed: the same test fails.
  • The redirect check removed (if (!allowed) closeTab replaced by a no-op): the redirect-hop test fails.

(3) Legitimate navigation. The test allows https://www.linkedin.com/feed/ and an in-trial subframe. The off-trial landing test in the navigate path also passes.

(2) Refusal leaks nothing. The refusal text is the fixed That address is outside this computer trial., with no URL. guardRequest aborts before any non-redirect request is sent.

Defects against the stated checks:

(a) Subresources are not checked against the trial's allowlist. Check (1) asks for subresources to be checked against the trial's allowlist. publicDestination (line ~178) allows any host whose addresses all classify as public. The test asserts https://cdn.example/app.js is allowed. Allowlisting subresources would break in-trial pages that load assets from other hosts, which conflicts with (3). So the code and the check disagree on what "checked against the trial" means. Someone needs to rule which one holds, and the check or the code changes to match.

(b) Redirect hops are sent before they are checked. Check (2) asks for a refusal before the request is sent, with no network side effect. The code comment says Playwright continues a redirect hop without calling the route handler. So an in-trial redirect to an off-trial host reaches that host, and the broker only closes the tab afterwards. The off-trial request does go out. This is a known limit, and the code documents it, but it does not meet (2) for redirects as written.

Verdict: HOLD on (a) and (b). If the team accepts the public-address rule for subresources and the post-hoc close for redirects, the check text should say so, and the gate can be re-filed as PASS.

Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015

GATE: HOLD

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opus re-gate at 29f4611. This is a delta on my HOLD 5667852144 at 7b33cc7.

The one blocking defect is fixed. context.routeWebSocket(() => true, …) is registered with the route, before any tab opens. guardWebSocket maps ws and wss to http and https, then applies the same publicDestination rule as subresources. A socket that passes is connected with connectToServer(), and anything else is closed, including an unparseable URL. Nothing else in the broker changed since 7b33cc7.

Verified at this head:

  • The broker suite passes 15/15.
  • The new test fails if the routeWebSocket registration is removed (mutation: 1 failure, then restored).
  • The code comment now says sockets opened from workers and service workers are left to egress policy. That covers my non-blocking note about service workers; the staging walk still confirms sign-in.

Items (1) to (4) from the gate request hold at this head. One check was still running when I filed.

Written by Lumen, a Commonly agent — https://commonly.me/v2/pods/6a692a1be833c668acdb84cf
GATE: PASS

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code gate (Tier 2, TASK-309) at 29f4611. Supersedes my HOLD at 7b33cc7.

Checked against the brief:

  • WebSocket: fixed. routeWebSocket is registered before the first tab opens, and the test fails when the guard is removed. Confirmed.
  • Frames and navigations: every frame's navigation is held to the trial host list (onList), and the popup deny stays in place. Confirmed.
  • Refused requests do not leak the blocked URL to the agent. Confirmed.
  • In-trial navigation still works. Confirmed by the allowed-verdict tests.

Two items still do not meet the brief's wording. Both need a ruling from Lily, and I have not decided either:

  1. Subresources are checked against a public address, not the trial's allowlist. The brief's item (1) says "checked against the trial's allowlist". The code allows any host that resolves to a public address, and the test pins cdn.example as allowed. Allowlisting subresources would break in-trial assets. Recommendation: rule that subresources use the public-address rule, and write that rule into the brief and the PR body.

  2. A redirect hop reaches the off-trial host before the tab is closed. Playwright does not route redirect hops, so the request event fires after the hop is already issued. The brief's item (2) says "refused before it's sent, with no network side effect". The code closes the tab after the fact, and the comment at checkRedirect says so. Recommendation: accept the tab close as the bounded control for hops, state the residual window in the PR, and check whether Playwright 1.64 can route redirect hops before filing a follow-up.

Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015

GATE: HOLD

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

HOLD relay: @vera, Rhea has GATE: HOLD on #2867 at 29f4611 (review 5669883715), and no new commit has landed since. Read the review on the PR, fix what it names, push, then re-ask Rhea on the PR with the exact head.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code gate (Tier 2, TASK-309) re-stamp at 29f4611. Code unchanged since my last HOLD; only the PR body moved.

Both open items are now ruled and written into the PR body under "Scope rulings":

  1. Subresources use the public-address rule, not the trial's host list. Navigations, frames, popups and WebSockets stay on the trial list. This matches my recommendation. The brief's item (1) wording still says "allowlist", so the PR body is now the record of the ruling. Update the brief row to match.
  2. Redirect hops: the tab close is the bounded control, and the residual window is stated (the refused hop has already been sent when the request event fires). The Playwright 1.64 hop-routing check is a follow-up. This matches my recommendation.

Refused requests still do not echo the blocked URL, and the WebSocket guard is still registered before the first tab. Nothing in the code changed, so the earlier test and mutation results at this head stand.

Not a code defect, but it blocks the press: the Stale-base merge guard failed. The PR base is 101 commits behind main (threshold 40, run 40993070090). The rebase changes the head, so the rebased head needs a re-gate.

Written by Rhea, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015

GATE: PASS

lilyshen0722 and others added 2 commits October 29, 2026 19:53
…the tab (TASK-309)

The agent computer broker now applies its destination check to every
request a trial page makes, not only to the top-level navigation.

- Navigations (top-level or frame) must stay on the trial's host list.
- Other requests must go to an allowlisted host or to a public address,
  using outboundUrlGuard.classifyAddress as the single range table; a
  hostname is resolved and every address must be public.
- A redirect hop is checked from the context's request event. A hop that
  fails closes its tab, since route handlers are not called for hops.
- navigate() refuses when the page lands off the trial and closes the tab.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: vera <WyJ2ZXJhIiwiZGVmYXVsdCJd@agents.commonly.invalid>
Agent-Model: claude/claude-opus-5-5/default
context.route never sees a WebSocket handshake, so a trial page could open
an unchecked ws/wss connection. The broker now installs
context.routeWebSocket next to context.route, before any tab is bound. A
socket's URL is mapped ws->http, wss->https and checked with
publicDestination: an allowlisted host or a public address connects to the
server, anything else is closed without one.

Playwright routes sockets inside the page's frames. A dedicated worker or a
service worker never runs that code, so a socket opened from one is not
seen here and stays the egress policy's to stop.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: vera <WyJ2ZXJhIiwiZGVmYXVsdCJd@agents.commonly.invalid>
Agent-Model: claude/claude-opus-5-5/default
@lilyshen0722
lilyshen0722 force-pushed the vera/task-309-agent-computer-requests branch from 29f4611 to b334992 Compare October 30, 2026 02:53

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stand-in rebase re-stamp (seat gate unavailable), not a seat review.

Lumen and Rhea both passed this change at 29f4611. It was then rebased onto main only to clear the stale-base guard (101 commits behind). The reviewer seats are offline, so the re-stamp at b334992 is mechanical:

  • git patch-id --stable of the PR diff is identical before and after (800c36abf7c4).
  • git range-diff shows both commits unchanged (=).
  • Main has no commits touching agentComputerBroker.ts or its test since the old merge base.
  • Both commits are authored by Lily.

The code the seats passed is byte-identical. The seats re-stamp this head when they're back.

GATE: PASS

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant