Repository navigation
Check origins on pages, let a task name its browser, fix date-form picks - #84
Merged
senamakel merged 22 commits intoOct 10, 2026
Merged
Conversation
A session's allowed origins were handed to agent-browser as its allowedDomains, which fails every request to an unlisted host. That broke the pages themselves and the browser around them: - A site's own CDN and APIs were refused, so pages loaded without their pictures and scripts, and widgets such as a destination box never listed suggestions (OYO loads 94 files from assets.oyoroomscdn.com). - agent-browser installs that filter on every new target before letting it run. When the install found no execution context yet, the target stayed paused (a blank tab), and the session-level install closed the browser, so every later step reported BROWSER_UNAVAILABLE. - agent-browser refuses a profile beside allowedDomains, so a persistent profile could never start while a list was set. The session now checks pages itself (origins/) and never hands the list to the engine: - a navigation outside the list, typed or a raw navigate, tab_new, or window_new, is refused before the browser is asked; - the page navigate, perform, snapshot, or read leaves the session on is checked, and a refused one is left (back, else about:blank) before the call reports BlockedByPolicy; - a task's surface checks the page before every observation, so a page it was taken to by any means is never read or acted on. With no list the check costs nothing. An entry is an origin, a dotted host for its subdomains, or `*` for any public host; private and local addresses stay refused under `*`. The list remains a guard rail, not a sandbox: a page on an allowed site can still reach other hosts on its own.
StartTask constraints gain browser_executable and browser_profile, so a host can give each task the Chrome binary and the profile folder its settings name, instead of the binary read once when the module loaded and a fresh profile every time. A profile kept between tasks keeps a site's sign-in. A relative profile folder or a blank binary is refused as INVALID_REQUEST. `*` among the origins admits any public host (see the previous commit). fill_then_approve refuses it with ORIGINS_REQUIRED: it names no site card details may be typed on. Both are optional fields and a new accepted value, so the contract moves to 2.10. Describe's StartTask schema, the skill schema, and the docs name the new fields.
Tiny Sweeper reviewThis pull request moves the origin allow-list out of the browser engine into the session itself, adds host-set browser binary/profile task constraints, and fixes date-picker form handling plus cover dismissal in the flow runtime. Review lanes (critique, security) report open findings covering module declaration ordering, error-classification for timeouts, origin-policy edge cases (external resource loads, DNS rebinding, permission grants, fail-open windows around navigation commits), and TOCTOU/authorization concerns in the surface observation path; the tests and description lanes found nothing new and consider earlier-round findings addressed. The contract version is deliberately bumped to (2, 10). State: Changes requested Review snapshot
Completeness: Complete What changedNo supported behavioral explanation was produced. Features
Tests
Findings
Resolved this pass
Before merge
How this fits togetherflowchart LR
n0["every_variant<br/>changed"]:::changed
n1["Error<br/>changed"]:::changed
n0 -->|uses| n1
classDef changed fill:#0d4429,stroke:#238636,color:#e6edf3
classDef impacted fill:#161b22,stroke:#6e7681,color:#c9d1d9
classDef flagged fill:#5a1e02,stroke:#d93f0b,color:#ffffff
classDef blocking fill:#67060c,stroke:#f85149,color:#ffffff
Agent review detailscritique
security
tests
commits
description
e2e
Evidence and run details
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Comment |
![tinysweeper[bot]](https://avatars.githubusercontent.com/in/4526585?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0158 · 1,260,334 in / 79,390 out · 139,511 cached (11%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0093 · 731,311 in / 45,170 out · 81,838 cached (11%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0055 · 424,997 in / 26,959 out · 55,689 cached (13%) · gpt-5.6-luna
tests: $0.0005 · 51,323 in / 4,839 out · 1,856 cached (4%) · glm-5.3-flash
description: $0.0002 · 25,332 in / 456 out · 64 cached (0%) · glm-5.3-flash
A month drawn as its weeks (four to six rows of up to seven days, as
React-style pickers draw it) is read as the run of its days, like a flat
grid. A day is described by its date unless the page's own label for it
names the month; a label that names none ("Sold out") follows the date.
A grid's title is never taken from inside a calendar found before it.
Live, a hotel site's open days showed a number and a fare in week rows:
none read as a date, and the date step paged a year past October.
Paging a calendar to a date now also stops when text on screen heads the
wanted month: only months, years, and day names ("October 2026", "Oct
2026 Mo Tu We ... November 2026"). A day number or any other word makes
it a date or a sentence, never a heading.
Days that show only a number and a fare name no month, so no day ever
matched and the calendar was paged to the 12-month limit, twice.
Of several controls whose label holds a slot's word, the opener is the one holding it soonest, then the one with the fewest words: a control wrapping others names them all. Live, for the slot "from city" a trip-type tab "Multi City" came first and was refused; the do loop then pressed the button wrapping the whole form at its centre, twice, and picked a return date that turned a one-way search into a round trip.
YellowSnnowmann
changed the title
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0090 · 695,133 in / 54,453 out · 62,799 cached (9%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0047 · 339,591 in / 28,244 out · 40,717 cached (12%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0029 · 206,343 in / 15,085 out · 21,826 cached (11%) · gpt-5.6-luna
tests: $0.0007 · 74,818 in / 6,663 out · 128 cached (0%) · glm-5.3-flash
description: $0.0003 · 35,767 in / 1,416 out · 64 cached (0%) · glm-5.3-flash
The session read a page's address with its own string rules, and judged hosts the browser never opened: `http://2130706433/`, `127.1`, hex and octal forms, percent-escaped or full-width digits, `http:\\127.0.0.1\`, and `https://evil.test\@example.com/` (read as example.com; Chrome opens evil.test). Addresses are now parsed with the `url` crate, which follows the WHATWG rules Chrome does, and hosts are compared canonically. Under `*`, IPv4 carried in IPv6 (compatible, NAT64, 6to4) is judged as itself, site-local fec0::/10 is non-global, and single-label names and names under internal and home.arpa are local. Only about:blank and about:srcdoc pass among about: pages (about:settings is a browser page), and a blob: page passes when the site that made it does.
A page that moved out of the allowed origins on its own (a timer, a redirect after a raw command) could still be acted on or read: `perform` sent its action before checking, `evaluate`, `screenshot`, and raw commands never checked, and the first page of a session was taken as is. Now every call that acts on or reads the page checks it first, and a raw command or script checks the page it leaves before returning anything. A raw command naming an address in `url`, `url1`, or `url2` is checked like a navigation, a raw `read` of an address (which follows redirects unseen) is refused under a list, and raw waits and lookups that neither act nor read go unchecked, so a wait runs while a navigation commits. A page whose address cannot be read around a call's own work lets it go ahead; the observation's check reads twice, fails closed, is bounded by READ_TIMEOUT, and runs only under a list. A navigation reads the page it reached afresh. A refused first page is left; an attached browser keeps the person's tab and opens one of the session's own. A page refused after the call ran is `LeftRefusedPage`: the same BLOCKED_BY_POLICY code, but its delivery is unknown, since the click that reached it may have paid. Docs say what the list covers, what it does not (names are never resolved), and where card details may be typed.
`browser_executable` and `browser_profile` are host settings, but the agent-facing skill schema offered them, so a host relaying a model's constraints could let page text pick the binary that launches. A task now starts only with a binary given as the absolute path of an existing file (no name looked up on PATH, no folder) and a profile given as an absolute folder, each taken exactly as written, and with neither beside `browser_endpoint`, which launches nothing; agent-browser refused that pair only at the first browser step. The skill schema and SKILL.md leave both out, Describe marks them the host's, and the docs say a host never takes them from a model and must cancel a task still holding a profile before starting another on it. The skill and Describe also say that `fill_then_approve` refuses `*`, and the versioning guide shows the current contract version.
The location grant stood back whenever a browser had a profile folder, taking it for a person's own. Since a task may now keep a profile between its runs, that browser is still one the module launched, and a headless task pressing a store's "use my current location" waited on a bubble no one could answer. The grant now stands back only for a person's running browser (an endpoint); it lasts while the browser runs and is not written into the profile.
A header naming two months gave them to the grids it titled in the order they were found, so a missed grid (a "Today" over its first day, a hidden week) gave the next month's days the month before's name. Week rows made that reachable, and a wrong date is worse than none: grids are now dated only when every month of their header found its grid, and stay calendars, undated, otherwise. A page's own label that names a month stands alone, never behind a computed date. A calendar found before a grid is passed over again, so a second grid of bare numbers still reaches its header, but a header beyond it titles the grid only while it has a month left. Week rows may be seven (a row of day names before six weeks), cheap tests run before reading cell text, and a month name is matched as a whole word.
Any month-and-year text anywhere on the page stopped the paging, so a month menu, a strip of month tabs, or a list grouped by month could strand a dated calendar on the wrong month. The heading now counts only when it sits within HEADING_REACH (8) nodes of the very "next month" arrow, is no choice of month (an option, a tab, a menu item), and at least BARE_DAYS (7) days on screen show bare numbers; a calendar whose days name their month is paged by them alone. The paging test now asserts the step's outcome as well: an undated day is never pressed on its number.
Ranking by the fewest words made any short control sharing a slot's word beat its box: "Date Change" for "departure date", "City Guides" for "from city". Openers now rank by being on screen, then holding more of the slot's words, then holding its first (most particular) word, then wrapping no other match, with document order deciding the rest, as the first match did before.
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0338 · 2,427,145 in / 215,207 out · 276,599 cached (11%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0209 · 1,521,158 in / 132,079 out · 192,054 cached (13%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0112 · 723,176 in / 75,993 out · 84,353 cached (12%) · gpt-5.6-luna
tests: $0.0006 · 58,697 in / 1,993 out · 0 cached (0%) · glm-5.3-flash
description: $0.0005 · 59,505 in / 1,680 out · 64 cached (0%) · glm-5.3-flash
`snapshot`, `read_page`, and `screenshot` checked only the page they left, so a read still ran on a page that had moved out of the allowed origins; they now check first too, as actions do, and a raw `title` (a read of what the page shows) no longer skips the check. Opening a session no longer hands one out on a refused page: an attached browser whose own tab cannot be opened fails the open, and a launched one whose first page cannot be left is closed, and the open fails BlockedByPolicy.
Any seven clickable numbers anywhere on the page (a results list's page numbers) counted as a calendar's bare days. Only those within DAYS_REACH (100) nodes of the arrow about to be pressed count now: two months' cells, their day names, and their headings.
Describe is what a model reads, and it still listed `browser_executable` and `browser_profile`, marked as the host's. They are left out now, as in the skill, and the parity test names them as the host's own settings kept out on purpose. The payment test also checks `*` alone, not only beside a named site, and the binary test says why any existing file passes.
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0132 · 999,174 in / 83,985 out · 110,476 cached (11%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0069 · 503,590 in / 43,106 out · 69,314 cached (14%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0048 · 302,601 in / 33,911 out · 40,906 cached (14%) · gpt-5.6-luna
tests: $0.0005 · 61,744 in / 2,595 out · 64 cached (0%) · glm-5.3-flash
description: $0.0005 · 63,596 in / 1,517 out · 64 cached (0%) · glm-5.3-flash
A check around a call's own work let the call go ahead on any failure to read the page's address, so an engine or protocol fault looked like a readable page. It now lets work go ahead only when the engine says the page is committing a navigation (its execution context was destroyed), and fails the call on anything else. `addscript` and `addstyle` put a caller's chosen file into the page, so their address is checked like a navigation's. Tests assert the exact failures, and an empty-list assert uses assert_eq!, as clippy 1.99 asks (CI's Rust job failed on it).
A binary is now refused unless it is an executable file, where files carry that mark, besides an absolute path to one on this machine, so a readable file that is no program fails at StartTask rather than at the first browser step. The browser-setting checks move into a test of their own.
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0128 · 984,473 in / 67,989 out · 93,145 cached (9%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0064 · 513,194 in / 34,068 out · 63,395 cached (12%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0043 · 273,894 in / 28,501 out · 26,806 cached (10%) · gpt-5.6-luna
tests: $0.0005 · 63,268 in / 1,199 out · 1,536 cached (2%) · glm-5.3-flash
description: $0.0005 · 65,250 in / 1,232 out · 1,408 cached (2%) · glm-5.3-flash
A day drawn as its number over a fare ("23 6757") that carries its date
only in one inner label ("October 23, 2026") now takes that label as its
date, when the label names a month and the day's number. A month's title
is looked for before the grid or any of its five nearest ancestors, and
the title's own block is a calendar too, so arrows drawn in it page the
month; it is added after the two-month pickers, so no block around a
title and its grid is taken for a picker, and a wizard's "Next" beside
the grid keeps its name.
Live, a flight site's react-calendar drew "‹ October 2026 – November
2026 ›" above both months, outside either, and its priced days carried
their dates only in inner labels: the date step never found a day.
A browser press still refused as covered now says what covers its target, as a person would (`button "Select Location"`, `an empty layer`), in the error's details, and whether that is an empty layer a press outside a popup closes. Covers are read through open shadow roots; a layer counts as empty only when it shows nothing where it covers the target, belongs to no component, is placed over the page (fixed or absolute), covers half the window, and lies over a control sight marked. The new defaulted `Surface::dismiss_cover` presses such a layer at one of 49 grid points where the layer itself is on top and nothing pressable lies beneath (no control, nothing showing a pointer, nothing of the target), with raw mouse events and the on-screen cursor gliding there. A control, dialog, text, picture, an unreadable cover, or a layer with no clear point is never pressed. The desktop refuses it, and the workspace routes it like `back`. The flow presses such a layer once after a closer or Escape left the target covered, looks again, and retries the same vetted target; it never does so behind a dialog the task opened. The history names the cover, and a step that fails after a press still refused ends its note with "its press of … was refused because … lies over it", so a rescue deals with the cover rather than redo the steps before it. A press the page refused (`NOT_ACTIONABLE`) never reached it, so it no longer counts as one that opened a layer in front, or answered one. The retried press after a cover centres its target at once: a page with `scroll-behavior: smooth` was still moving it when the press landed. Live, a store's search box left its suggestions open over the page, every control outside them read as covered, and Escape left them there. The basket press was refused, the refused press read as one that opened a dialog, and the failed step told a rescue to answer it: rescues re-added other sizes and pressed + again.
A browser the module launches that is not found, or will not start, now says what to set: "no Chrome or Chromium was found on this machine; give the path of the browser to use", or, for a binary the session was given, "the browser binary given could not be started (…); check that its path names Chrome or Chromium". A binary that is no browser (it exits before the browser's address shows, or that wait runs out) reads as a browser that could not be started rather than a timeout or a module failure. A browser attached to keeps the engine's own words. An `open` or `browse` step's failure note keeps the first line of the failure's own words after its code (up to `FAILURE_CHARS`), and a task step that failed because no browser could be started is never rescued: the task fails at once, not recoverable, with a hint to give the path of Chrome or Chromium (`browser_executable`). Live, a task failed with a bare `https://www.amazon.in could not be opened: BROWSER_UNAVAILABLE` on a machine whose Chrome was not where it is looked for, after four rescues that could not have helped (tinyhumansai/openhuman#7199).
A raw `boundingbox` can test whether a text shows on the page (a `text=` selector), and a raw `permissions` grant changes what pages may do, so neither is sent while the session shows a refused page any more: both now get the check before and after that every call acting on or reading the page gets. Only a wait for the page to load and a read of its address, which a refusal names anyway, still go unchecked, so a wait runs while a navigation commits. The raw command's doc now says exactly that.
The rescuer is told that a failure naming what lies over a refused press's control names that cover as the screen shows it, its words screen data: close it, or answer it as the task says when it is the page's own question the task needs answered, and never redo the steps before it, which did their work. The flow guide says that a cart button in a page's header shows only how many items the cart holds, never which, so a step checks an item went in by its stepper, never by waiting for the cart button to show it. Live, a plan waited for "the basket shows Maggi" over a header count that never could, and rescues of that and of a covered basket press added more sizes of the item.
There was a problem hiding this comment.
Requesting changes: 2 lane(s) blocking, worst finding is critical.
Fix or reply to the findings below and push. The next review clears this automatically once they are gone — you should not need to dismiss anything by hand.
$0.0368 · 2,755,507 in / 198,009 out · 216,786 cached (8%) · gpt-5.6-luna, glm-5.3-flash
critique: $0.0216 · 1,557,679 in / 123,485 out · 148,741 cached (10%) · gpt-5.6-luna, glm-5.3-flash
security: $0.0127 · 883,749 in / 66,622 out · 66,381 cached (8%) · gpt-5.6-luna
tests: $0.0008 · 99,978 in / 1,844 out · 1,536 cached (2%) · glm-5.3-flash
description: $0.0008 · 101,767 in / 2,063 out · 0 cached (0%) · glm-5.3-flash
Comment on lines
+70
to
75
| if lower.contains("devtools url") || lower.starts_with("chrome launch task failed") { | ||
| return Error::browser_unavailable(message); | ||
| } | ||
| if lower.contains("timed out") || lower.contains("timeout") { | ||
| return Error::timeout(message, 0); | ||
| } |
There was a problem hiding this comment.
Map the timeout error before constructing the browser error
The devtools url check runs before the timeout check, so a message such as Timeout waiting for Chrome DevTools URL is classified as BrowserUnavailable and never reaches Error::timeout. Callers that distinguish a retryable timeout from a failed browser launch will take the wrong recovery path. Check timeout messages first, then classify the remaining DevTools-launch failures as BrowserUnavailable.
Suggested change
| if lower.contains("devtools url") || lower.starts_with("chrome launch task failed") { | |
| return Error::browser_unavailable(message); | |
| } | |
| if lower.contains("timed out") || lower.contains("timeout") { | |
| return Error::timeout(message, 0); | |
| } | |
| if lower.contains("timed out") || lower.contains("timeout") { | |
| return Error::timeout(message, 0); | |
| } | |
| if lower.contains("devtools url") || lower.starts_with("chrome launch task failed") { | |
| return Error::browser_unavailable(message); | |
| } |
[RULE] error-classification ·
Comment on lines
+95
to
+96
| const pressable = (node) => !node || Boolean(nearest(node, CONTROL)) | ||
| || getComputedStyle(node).cursor === 'pointer' || within(node, target); |
There was a problem hiding this comment.
Treat an empty point beneath the cover as non-pressable
pressable is meant to identify things that would receive a meaningful press beneath the cover, but !node returns true. For a full-screen backdrop over an otherwise empty part of the page, topAt returns null, so every such spot is considered pressable and press_cover rejects the backdrop even though clicking that empty area is exactly the intended dismissal behavior. Make the predicate require a node before checking the pressable conditions.
Suggested change
| const pressable = (node) => !node || Boolean(nearest(node, CONTROL)) | |
| || getComputedStyle(node).cursor === 'pointer' || within(node, target); | |
| const pressable = (node) => Boolean(node) && (Boolean(nearest(node, CONTROL)) | |
| || getComputedStyle(node).cursor === 'pointer' || within(node, target)); |
[RULE] incorrect-nullability ·
| /// The refusal, and a failure to read the address other than a | ||
| /// navigation's. | ||
| async fn check_if_readable(&mut self, after: bool) -> Result<()> { | ||
| match self.check(after).await { |
There was a problem hiding this comment.
Do not bypass origin checks on navigation errors
A navigation can leave the page at a refused origin while the browser temporarily reports errors such as execution context was destroyed. This branch treats those errors as success and lets the caller perform its operation without knowing the current page; if the follow-up check sees the same transient error, the operation can also return data or perform actions on the refused page. A navigation-related read failure should be retried until the address is known, or the operation should fail without executing while the page is unreadable.
[RULE] fail-open-policy-check ·
| let session = self.session(id)?; | ||
| let mut session = session.lock().await; | ||
| session.run(command).await | ||
| if !URL_IS_NO_PAGE.contains(&action) { |
There was a problem hiding this comment.
Validate permission-grant origins before sending the command
The admission loop only examines url, url1, and url2. A raw permission-grant command can carry its target in an origin field, so a command such as {"action":"grant_permissions","origin":"https://evil.example","permissions":["geolocation"]} can be sent while the current page is admitted and the target origin is not. Validate the permission target with the same origin policy before calling session.run.
[RULE] origin-policy-bypass ·
| // front. Live, a refused press of a store's basket button counted as | ||
| // one, the controls that read as covered after it were taken for a | ||
| // dialog it opened, and the step's failure told a rescue to answer it. | ||
| if !refused(&reply) { |
There was a problem hiding this comment.
Preserve front state when a refused press can still land
refused treats every NOT_ACTIONABLE response as proof that no input reached the page, but the new comment explicitly documents an exception: a result card's cover can cause the final mouse event to fail in transit after the card's own control was pressed. That response skips front.act, so the flow keeps the pre-action front state and may interpret subsequent controls as covered or route the step to the wrong rescue. Distinguish a pre-input refusal from this partial-action case using response details, or otherwise preserve the front update for the case that can land.
[RULE] stale-state ·
|
|
||
| mod live_calendar_tests; | ||
| mod live_controls_tests; | ||
| mod live_cover_tests; |
There was a problem hiding this comment.
Add the declared test module source files
These module declarations have no corresponding live_cover_tests.rs or live_day_label_tests.rs in the repository, so Rust fails module resolution when compiling the test target. Add both source files (or remove the declarations) before merging.
Additional critique observation
Add the declared test modules before compiling
[RULE] missing-module
Neither live_cover_tests.rs nor live_day_label_tests.rs exists at this commit, so these declarations produce Rust module-not-found errors and break the build for every caller. Add both files to the commit, or remove the declarations until their implementations are present.
[RULE] missing-module-source ·
| self.perform("back", Action::Back) | ||
| } | ||
|
|
||
| fn dismiss_cover(&self, target: &Candidate) -> DesktopResponse { |
There was a problem hiding this comment.
Check refused pages before running every raw command
The new dismiss_cover operation directly invokes press_cover without checking the current page against allowed_origins. Callers can use this operation after navigation or a redirect to act on a refused page, bypassing the policy that observation now enforces.
[RULE] authorization-bypass ·
| let command = convert::action(&action, session.options.default_timeout_ms)?; | ||
| // The page may have moved on its own since the last call (a redirect, | ||
| // a timer): an action is never sent to a refused one. | ||
| session.check_if_readable(false).await?; |
There was a problem hiding this comment.
Block permission grants in persistent profiles
Checking that the current page is admitted does not prevent a raw permission command from permanently changing a persistent browser profile. A caller can therefore grant location or similar privileges through this escape hatch, and the grant survives beyond the current session. Reject permission-changing commands for persistent profiles, or require an explicit non-persistent profile before allowing them.
[RULE] permission-escalation ·
Comment on lines
+261
to
+263
| workspace.dismiss_cover(&Candidate::default()); | ||
| workspace.navigate("https://flights.test"); | ||
| workspace.dismiss_cover(&Candidate::default()); |
There was a problem hiding this comment.
Assert every routing operation succeeds
The new test discards the responses from all three operations, so it would still pass if an operation returned an error as long as the recorder observed the call. Assert each response is successful (and add an explicit failure assertion if failure behavior is part of the contract) so the test covers the intended behavior rather than only invocation routing.
Suggested change
| workspace.dismiss_cover(&Candidate::default()); | |
| workspace.navigate("https://flights.test"); | |
| workspace.dismiss_cover(&Candidate::default()); | |
| assert!(workspace.dismiss_cover(&Candidate::default()).ok); | |
| assert!(workspace.navigate("https://flights.test").ok); | |
| assert!(workspace.dismiss_cover(&Candidate::default()).ok); |
[RULE] unchecked-test-result ·
| let id = self | ||
| .ensure_session() | ||
| .map_err(|error| Box::new(failure("observe", &error)))?; | ||
| let checking = self.browser.check_page(&id); |
There was a problem hiding this comment.
Hold the session lock through the policy-protected operation
check_page validates the current page while briefly holding the session lock, but the lock is released before see or snapshot runs. A navigation can therefore replace the admitted page with a refused origin between the check and the read. Keep validation and the subsequent page operation under the same session lock, or provide an atomic checked-operation API.
[RULE] authorization-toctou ·
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OpenHuman always sends an allowed-websites list, and the browser session handed that list to agent-browser as
allowedDomains. agent-browser then fails every request to an unlisted host. In the app this broke browser tasks on every site we test:assets.oyoroomscdn.com, so its destination box never listed suggestions and the run failed with "Please select a destination from the list";BROWSER_UNAVAILABLE;The session now checks pages itself and never hands the list to the engine.
StartTaskalso gainsbrowser_executableandbrowser_profile, so a host can give each task the Chrome binary and profile folder its settings name.This PR also fixes two date-form bugs from live round 13. Both fixes are generic, with no site names in code:
doloop that followed pressed the button wrapping the whole form ("From DEL … To BLR … Departure … Return …") at its centre, twice. The first press opened the calendar, and the second picked a return date.choosepressed "Next Month" up to its 12-month limit, into 2027, twice.Review hardening. An end-to-end review of every commit, plus five tinysweeper passes (75 threads, each answered and resolved), led to a further round of fixes:
Round 14 fixes (ixigo, Agoda, BigBasket), and the browser that cannot start. Also generic, with no site names in code:
Priced calendar days were never dated (ixigo, Agoda). Each day shows its number and a fare ("23 6757") and carries its date only in one inner label ("October 23, 2026"), which sight ignored because the label does not contain the shown text. ixigo also draws "‹ October 2026 – November 2026 ›" above both months, outside either, so its arrows paged no calendar. Sight now takes that inner label as the day's date when it names a month and the day's number. A month's title is looked for up to five ancestors above its grid, and the title's own block counts as part of the calendar, so arrows drawn in it page the month. That block is registered after the two-month pickers, so a wizard's "Next" beside the grid keeps its name.
The basket stayed covered, and rescues filled the cart (BigBasket). The search box left its suggestions open over the page, and every control outside them read as covered. The basket press was refused, Escape left the list there, and the refused press was counted as one that opened a dialog. The step's failure then told the rescuer "a dialog the task opened is in front … its own controls are: … Add …", and rescues re-added Maggi in other sizes and pressed + again (the cart ended with two kinds, one at quantity 7). Now:
NOT_ACTIONABLE) never counts as one that opened anything in front;button "Select Location",an empty layer, …) in the history and in the step's failure note;scroll-behavior: smooth);A task whose Chrome could not start said only
BROWSER_UNAVAILABLE(Browser tasks ignore the persistent profile and only see the Chrome path the module loaded with openhuman#7199). A browser the module launches now says what to set:A binary that is no browser now reads that way too, where before it read as a timeout or a module failure. The step note keeps the error's first line, and such a step is never rescued: the task fails at once with a hint naming
browser_executable. Live, four rescues of it ran for minutes before giving up.Raw page checks tightened (fifth tinysweeper pass). A raw
boundingbox(whose selector can test whether a text shows) and a rawpermissionsgrant are now checked like any call. Onlywaitforloadstateandurlgo unchecked.Related issue
The module half of tinyhumansai/openhuman#7199; the host half follows once this is released. Related: tinyhumansai/openhuman#7000.
API or behavior changes
Contract 2.9 → 2.10 (minor):
TaskConstraints.browser_executableandbrowser_profile;*accepted among origins: any public host; private and local addresses and names refused.Rust API.
TaskConstraintsgains two public fields. A Rust host that builds it with a struct literal must add them, or use..TaskConstraints::default(). OpenHuman's re-pin does. Release this as minor (0.11.0).How a page is checked (only under a list; with no list, nothing new reaches the browser):
Addresses are parsed by the WHATWG rules Chrome uses (the
urlcrate, already in the lockfile through agent-browser), and hosts are compared canonically. Sohttp://2130706433/,127.1, hex and octal forms, and percent-escaped or full-width digits are judged as the addresses they open.http:\\host\is read as the browser reads it, and so ishttps://evil.test\@example.com/(Chrome opensevil.test).Under
*:fec0::/10, is refused, with IPv4 carried in IPv6 judged as itself;localhost,local,internalandhome.arpaare refused.Besides web pages, a list admits:
about:blank,about:srcdocand browser error pages;blob:page made by an admitted host.It admits no other
about:page (about:settingsis a browser page), nor any file,data:or browser-setting page.When a page is checked:
url,url1orurl2is checked the same way. A rawreadof an address is refused under any list, because it fetches from this process and follows redirects unseen.perform,evaluate,snapshot,read_page,screenshot, every raw command, a rawtitleincluded) checks the page first, so nothing is sent to a page that moved out of the list on its own.evaluatealso check the page they leave before returning anything.navigatereads the page it reached afresh, rather than trusting the address the engine reports.waitforloadstate) and a read of its address (url, which a refusal names anyway) are not checked, so a wait runs while a navigation commits. A rawboundingboxand apermissionsgrant are checked like any call.addscript/addstyleaddresses are checked like navigations. A check around a call's own work lets the call proceed only when the engine says the page is committing a navigation; any other failure to read the address fails the call, and the next check catches a refused page.READ_TIMEOUT.New error variant
Error::LeftRefusedPage. It covers a page refused after the call ran. It has the sameBLOCKED_BY_POLICYcode, but delivery isunknown, notnot_delivered. Scenario: a click on a listed merchant's "Pay" lands on an unlisted bank page. A host that trusted "not delivered" would retry and pay twice.Files a page loads are no longer filtered (its CDN, APIs and frames). A name is never resolved, so under
*a public name that leads to a local address is admitted. This is documented inSessionOptions::allowed_origins,sessions.md,tasks.mdandsafety-and-privacy.md.StartTaskrefusals:ORIGINS_REQUIREDforfill_then_approvewith*.INVALID_REQUESTfor:browser_endpoint, which agent-browser would only have refused at the first browser step.Paths are taken exactly as written.
Host-only settings.
browser_executableandbrowser_profile:Location. A task's own kept profile now gets the location grant when it presses "use my current location", as a throwaway profile already did. Only a person's running browser (an endpoint) keeps its own say.
Sight calendars:
Date paging:
Slot openers: ranked by being on screen, then holding more of the slot's words, then holding its first word, then wrapping no other match.
Calendar days with a fare: a day's one inner label dates it when it names a month and the day's number; a month title is looked for up to six ancestors above its grid, and the block holding both is a calendar (its arrows page it) unless it holds a form's fields.
Covered presses:
Surface::dismiss_coveris new, defaulted toACTION_NOT_SUPPORTED, so existing surfaces are unaffected. The browser presses only an empty layer: one placed over the page, part of no component, over at least half the window, and lying over a control sight marked. It presses at one of 49 grid points where the layer is on top and nothing pressable lies beneath. Covers are read through open shadow roots, and a control, dialog, text, picture or unreadable cover is never pressed;details.coveranddetails.empty_layer(inside the module only; no bus payload changes);Safety trade-off, @senamakel please confirm. A page on an allowed site can now load files from any host, as in a normal browser. Under
fill_then_approve, that includes a payment provider's frame on a listed page; the docs now say so. The agent still can't open, act on or read a page of an unlisted site. The list stays a guard rail, not a sandbox.Validation
cargo fmt --all -- --check: cleancargo +1.99.0 clippy --workspace --exclude tinycomputer-accessibility --all-targets --all-features -- -D warnings: clean with CI's toolchain (accessibility is excluded locally, as in earlier PRs; CI covers it)cargo test --workspace --exclude tinycomputer-accessibility --all-features --no-fail-fast: 1261 passed, 0 failedRUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features --workspace --exclude tinycomputer-accessibility: cleanTINYCOMPUTER_LIVE_BROWSER=1 cargo test -p tinycomputer-browser --features agent-browser --lib -- live_: 31 passed, on local fixture pages in Chrome for Testingboundingboxorpermissionsreaches a refused page.task_live, headed Chrome, no site list), module built from c8b39c6:Tests
origins/origins_tests.rs(11 tests):*;@hosts;about:,blob:and other schemes;sessions/sessions_tests/origins_tests.rs:LeftRefusedPage;url,url1orurl2, and raw fetches;title) is sent to a page that moved out;surface/surface_tests/origins_tests.rs:error/error_tests.rs:LeftRefusedPagedelivery and suggestion, plus every variant's published name.task_tests/start_tests.rs: the refusals (binaries, profiles, the endpoint pairs) and a valid start.operations_tests.rs: the location grant in a kept profile.flow_tests/enter_tests.rs: the opener ranking, including the "Date Change", "City Guides", "Check availability" and off-screen cases.flow_tests/picker_tests.rs:BareCalendarDayssimulator quirk.sight_tests/live_calendar_tests.rs(gated):sight_tests/live_day_label_tests.rs(gated):sight_tests/live_cover_tests.rs(gated):scroll-behavior: smooth.surface_tests/cover_tests.rs:flow_tests/do_loop_tests.rs:sessions_tests/lifecycle_tests.rs,reply_tests.rs,step_kinds_tests.rs,task_tests/rescue_tests.rs:sessions_tests/origins_tests.rs: a rawboundingboxandpermissionsare refused on a page that moved out;urlandwaitforloadstatestill run.workspace_tests.rs:dismiss_coverfollows the active side.Untested edge: under
*, a public name that resolves to a local address is admitted, by design (see above).Documentation
docs/crates/tinycomputer-browser/sessions.md,errors.mddocs/technical/specs/unified-agent.md,desktop-module-contract.md,browser-sight.mddocs/technical/tasks.md,architecture.md,decision-thresholds.md(MAX_MONTHS,HEADING_REACH,BARE_DAYS)docs/safety-and-privacy.md,docs/giving-it-a-task.mddocs/crates/tinycomputer-bus/*(versioning.mdnow shows 2.10),tinycomputer-engine/tasks.md,flow/step-kinds.md,flow/filling-forms.mddocs/crates/tinycomputer/configuration.md,tinycomputer-skills/walking-the-schemas.mdstart_task.schema.jsonandSKILL.mddocs/crates/tinycomputer-browser/interacting.md,sight.md,errors.mddocs/crates/tinycomputer-core/surfaces-and-screens.mddocs/crates/tinycomputer-engine/workspace.md,flow/the-do-loop.mddocs/technical/decision-loops.md(same length as before),decision-thresholds.md(FAILURE_CHARS,LAYER_COVERS),tasks.md,specs/task-rescue.mdcrates/tinycomputer-bus/src/flow/guide.md) and the rescuer's protocolKnown limits, not addressed here
.comor*.comas an origin admits every .com site and still passesfill_then_approve. This predates the PR.StartTask. This is documented: cancel the earlier task first.permissionscommand passes no origin. It is documented insurface/location.rs.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the description