Skip to content

Harden review frame: drop popup/download rights, validate external hrefs - #37

Open
dacoldest wants to merge 1 commit into
petergyang:mainfrom
dacoldest:harden/frame-sandbox-and-external-scheme
Open

Harden review frame: drop popup/download rights, validate external hrefs#37
dacoldest wants to merge 1 commit into
petergyang:mainfrom
dacoldest:harden/frame-sandbox-and-external-scheme

Conversation

@dacoldest

Copy link
Copy Markdown

Two small hardening changes to the review chrome. Both turned up while I was reading through the frame policy before installing the skill, and both looked worth sending back rather than just patching locally.

Frame sandbox drops allow-popups and allow-downloads

The iframe renders artifacts that are untrusted by definition, and its sandbox granted popups and downloads. Neither appears to be needed.

allow-popups in particular is redundant with the design that is already there: sdk.js does not open external links itself, it posts eh:external to the chrome, and the chrome calls window.open. The frame already delegates popup-opening upward, so granting it the capability directly just widens what a hostile artifact can do to the reviewer. Nothing in the tree references downloads at all.

Remaining sandbox is allow-scripts allow-forms allow-modals, plus allow-same-origin for localhost reviews exactly as before.

eh:external validates the href before window.open

The handler passed msg.href straight through. The origin check is the only thing in front of it, so a compromised frame or a misconfigured policy could land a javascript: URL there, and it would execute on the chrome's origin next to the session token.

Rather than add a second URL-safety helper, this routes the href through the existing normalizeHref in editing.js. It already allowlists http/https/mailto/tel and already has coverage for javascript:, data:, vbscript:, and tab-obfuscated variants like java\tscript:. Reusing it keeps one place where scheme policy lives.

Tests

92/92 pass. Added one regression test asserting no review grants the frame popups or downloads, and confirmed it fails against the previous constant before keeping it, so a revert cannot pass silently.

Not changed

framePolicy still returns targetOrigin: "*" for file and Markdown reviews. Tightening that to "null" for the opaque-origin case might be worth considering, but it changes an existing asserted value and felt like your call rather than mine.

Happy to adjust or split this if you would prefer it in separate PRs.

The review iframe renders artifacts that are untrusted by definition, but
its sandbox granted allow-popups and allow-downloads. Neither is needed:
the SDK posts external links to the chrome over eh:external and the chrome
opens them, and nothing downloads. Removing both narrows what a malicious
artifact can do to the reviewer.

The eh:external handler also called window.open on an unvalidated href.
The origin check is the only thing in front of it, so a compromised frame
or a misconfigured policy could land a javascript: URL there, executing on
the chrome's origin alongside the session token. Route it through the
existing normalizeHref, which already allowlists http/https/mailto/tel and
is tested against javascript:, data:, vbscript: and tab-obfuscated
variants, rather than adding a second URL-safety helper to diverge from.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
petergyang added a commit that referenced this pull request Sep 2, 2026
petergyang added a commit that referenced this pull request Sep 3, 2026
* Wait for Send with one background poll instead of timed re-polls

The skill told agents to run poll --timeout 600 in the foreground and
re-run it on every timeout. Now it starts one open-ended poll in the
background and ends the turn; the harness wakes the agent when the
command exits, which only happens on Send or when the review is closed.

To make an open-ended wait safe, the CLI reconnects (restarting the
server if needed) when the connection drops instead of giving up after
three tries, and treats a socket close without end as a drop so the
promise cannot be left unsettled.

* Retry the atomic rename when Windows briefly locks the temp file

From community PR #40 by @rzilem.

* Harden review frame: drop popup/download rights, validate external hrefs

From community PR #37 by @dacoldest.

* Don't submit a comment on an IME composition Enter

From community PR #30 by @ulysses0604.

* Use the document filename as the browser tab title

From community PR #36 by @NuclearManatee.

* setup: treat any node_modules bin as transient; fix caret, details, heading-label bugs

- Port community PR #7 by @noammigdali-hio onto the invocation(run) signature:
  a bin under any node_modules segment is transient, not just npm's _npx cache.
- Issue #12: clicks inside the compose textarea place the caret instead of
  being swallowed by the card's focus handler.
- Issue #25: <summary> keeps its native toggle so collapsed content stays reachable.
- Issue #26: a heading labels itself instead of being named after the heading before it.

* Server: make every wait terminate, persist delivery, queue second sends, one server per state dir

- A tab that unloads tells the server; unless it reconnects within a short
  grace the review ends with reason window_closed and the waiting poll is
  released. A tab that silently vanishes ends the review after the 30-minute
  TTL instead of being deleted with the poll left hanging.
- A poll with no review open waits a short grace for one, then closes with
  reason no_review_open. Closed answers carry the unsent counts.
- A new poll for a target supersedes an older one, so stale background waits
  cannot double-apply a batch.
- delivered is persisted, so a server replaced between delivery and --ack
  honors the ack instead of re-shipping the batch. A batch written by an
  overlapping server is read from disk.
- Sending while the agent is applying a batch queues only the new items and
  carries the old cleanup forward; the agent's next --ack clears both.
- edits_saved and markdown flags per page; edit text capped at 200k with an
  explicit truncated marker instead of a silent 4k cut; Markdown and
  self-rendering pages keep unsent edit rows when the file changes on disk.
- Artifact URLs carry a per-run secret segment; a page key derived from the
  file path is no longer enough to read sibling files.
- Localhost reviews proxy the app's own requests (API calls, prefetches,
  srcset images) to the app origin instead of 404ing.
- One server per state directory: a lock file plus a health-checked takeover
  of an older-protocol server. Protocol bumped to 9.
- New routes: DELETE edit (undo), POST mode (dynamic), POST discard, POST away.
- Comment rewording updates an undelivered batch in place, or takes a fresh
  id after delivery so the rewrite ships next time.

* Browser and SDK: tab-close beacon, session re-bootstrap, undo, leftover banner, multi-block edits

- The chrome tells the server when the tab unloads (keepalive fetch on
  pagehide) and re-opens a session when the server has forgotten it, instead
  of turning into a dead tab that still looks alive.
- Undo for the two irreversible gestures: a toast with an Undo button after a
  block delete or move restores the block and drops its edit row.
- Feedback left over from a review that ended without a Send shows a
  Keep/Discard banner on the next open, noting that HTML text edits are
  already in the file either way.
- A second Send while the agent works shows as queued, not as nobody
  listening. Comments get an Edit button and an edited badge; rewording
  after delivery explains it ships with the next Send.
- Typing over a selection that spans blocks reports every block it touched,
  including a deleted row for one that vanished; a paragraph turned into a
  list keeps its label and captured original.
- CLI: an open-ended wait ends after 12 hours with a timeout status that
  says to check status and start again; superseded polls just print.
- SKILL.md: closed/superseded handling, edits_saved, deleted and truncated
  rows, rendered quotes with anchors, staged assets before ack, note-only
  batches. Codex block updated to match.
- srcset URLs are absolutized for localhost reviews.

* Fix phantom sessions after a review ends; pin edit-list ordinals to load order

- After the server ended a session, the tab's event stream reconnected, got
  a 404, and the new re-bootstrap path opened a fresh session with no tab
  behind it, which kept the review open and the poll waiting. The chrome
  now never re-bootstraps once the review ended or the tab is unloading,
  and the server drops a session no browser connected to within a minute.
- Label ordinals ("p 3") come from sibling order at load, so deleting a
  paragraph does not hand two blocks the same label within one review.
- A list command now retracts the row the fresh <li> emitted about itself
  and reports the change under the paragraph's own label with its original
  text.

* Make the reconnect test fail fast instead of stalling CI; per-test timeout

On a slow Windows runner the poll's reconnect could spawn its own server
before the test's replacement announced itself, and then wait forever for a
batch sent elsewhere. The first reconnect now waits 3s, the test races the
poll against a 30s timer, and every test gets a 60s timeout so a hang fails
the file instead of the whole job.

* agent-loop: bound the reconnect test's setup and instrument it for the Windows stall

* agent-loop: bound child shutdown waits; log cleanup state

* agent-loop: a signal-killed child counts as stopped; drop the debug markers

* SKILL: background wait is Claude Code only; Codex and others stay on the poll in the foreground

Codex returns a detached session id and nothing wakes the agent when that
session finishes, so a Send sat unread until the user nudged. The skill and
the AGENTS.md block now say which harness gets which wait, and how to cope
with a shell tool that caps command duration.

* SKILL: stay on the foreground poll through incoming messages; state the ended-turn limit plainly

* setup: own the AGENTS.md block with markers and replace a legacy one

A project set up months ago kept the instructions from that day forever,
because setup skipped any AGENTS.md that already mentioned human-review.
The block is now written between marker comments and rewritten on every
setup; the unmarked block an older setup wrote is recognized by its heading
and replaced; a project that wrote its own human-review guidance is left
alone.

* v0.7.0

* state: keep each page's pristine copy in its own file

Every edit flush rewrote state.json in full, and it embedded the complete
HTML of every page reviewed in the last month. The copy now lives in
pristine/<key>.html, is written only when it changes, and is removed with
its page. A state file that still embeds copies loads as before and moves
them out on its next save.

* Editor smoke tests under jsdom; fix ordinal snapshot missing the body

Three tests boot the SDK against a jsdom document: block delete with undo,
typing over a selection that spans blocks, and stable labels after a
deletion. The last one caught that the load-order snapshot skipped the body
element, so top-level paragraphs still renumbered.
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