feat(s5-4c): VM sandbox method gates — alert/confirm/prompt + window.open + top-nav 2-flag#447
Conversation
…ndowOpenDisposition, VM alert/confirm/prompt/open natives + back-channel queues - elidex-plugin: ALLOW_TOP_NAVIGATION_BY_USER_ACTIVATION bit + token parse, modals_allowed + top_navigation_allowed(flags, activation) predicates - elidex-script-session: OpenTabRequest / NamedFrameNavigation channel types, WindowOpenDisposition + pure window_open_disposition target dispatch (HTML §7.3.1.7 / §7.4.2.4); HostDriver gains modals_allowed + take_pending_open_tabs + take_pending_frame_navigations - elidex-js VM: marshal-only alert/confirm/prompt (§8.9.1 cannot-show gate, permanent step-4 opt-in) + window.open natives; NavigationState FIFO queues; HostDriver impl drains Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…aux-nav gate, link top-nav 2-flag re-key - boa JsRuntime: engine-agnostic take_pending_open_tabs / take_pending_frame_navigations wrappers (aux_nav_allowed=true by construction — entry gate already passed); modals + top-nav sites re-keyed onto elidex_plugin::sandbox predicates (One-issue-one-way) - shell content drains (navigation.rs + event_loop.rs) consume the trait surface, not the boa bridge; named-MISS OpenNewTab promotion now gated on the snapshot aux_nav_allowed (closes the ungated sandbox-bypass); route_frame_navigations extracted for test - shell link top-nav gate re-keyed onto top_navigation_allowed(flags, true) (click=activation) - app mode drains-and-drops both channels (no new-tab/iframe facility inline) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…axis + memo §5.3 landing note Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(4 sites), coerce_string_arg dedup, canonical-home doc fix - extract resolve_url_or_syntax_error in location.rs; window.open + 3 location natives share the resolve-or-throw-SyntaxError idiom (one-issue-one-way) - coerce_string_arg dedups window.open url/target arg coercion - sandbox.rs module doc: all delegation sites re-keyed (no remaining open-coded) - drain-site infallible-reparse comments (guards are defensive, not error-swallowing) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ethod removed, window.rs split, drain dedup
/code-review + /elidex-review findings (all CONFIRMED, fixed through the design lens):
- F1 (spec bug in new code, §7.2.2.1 step 16.1): window.open("","_self") was
navigating the existing navigable to about:blank; now empty url = null urlRecord
threaded as Option → no-op for existing navigables (_self/_top/_parent/named-HIT),
about:blank only for new ones (_blank/named-MISS, step 15.3). NamedFrameNavigation.url
→ Option<String>. Regression tests + whitespace-url spec-divergence pin added.
- F2 (won't-fix, VM is spec-correct): whitespace-only url URL-parses to the document
URL (JS-empty check per step 4), NOT boa's non-spec trim()-to-about:blank; pinned.
- F3 (dead code): HostDriver::modals_allowed had zero consumers (gate is engine-internal);
removed from trait + VM impl, kept HostData accessor — matches scripts_allowed precedent.
- F4 (dup): open-tab drain parse+notify deduped into notify_open_new_tabs (both pumps).
- Axis 4 MIN: alert step 2→1 citation fix. Axis 5 MIN: window.rs crossed 1000 → split
dialogs/open natives into vm/host/window_dialogs.rs (touch-time cohesion seam).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…tifact Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4276d607c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…pen channel (E4) Codex P2 (IMPORTANT, correctness): run_event_loop drained take_pending_open_tabs but not take_pending_frame_navigations, so a named-target window.open from a pure-async turn (timer/postMessage with no later user input) stranded its NamedFrameNavigation in the queue forever. Completes E4 drain symmetry: both back-channels drain at both pumps (audited all 3 sites). Factored the async-pump drain into the testable drain_async_window_open_channels helper + regression test that fails pre-fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc7b7957c8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… ordered queue
Codex P2 (IMPORTANT, correctness/ordering): the two separate window.open queues
(pending_open_tabs + pending_frame_navigations) lost cross-call FIFO — a task mixing a
named MISS and a _blank popup routed the _blank tab before the earlier named MISS,
reversing call-order tab creation. Root-check (>=2 rounds same area: R1 was the async
pump draining only one queue) found the two-queue split IS the defect. Collapsed to ONE
ordered queue: pending_window_open: VecDeque<WindowOpenIntent> {Popup|NamedFrame},
HostDriver::take_pending_window_opens, shell route_window_opens (one routing home). Call
order preserved by construction; one drain method makes R1's 'drain only one queue'
unrepresentable. boa adapter concatenates its two bridge queues (best-effort, matches
boa's prior popups-then-named effective order; deletion-bound). Ordering regression tests
at VM (intent queue) + shell (OpenNewTab order) levels. Cumulative fix-delta design
re-gate: CLEAN (0 CRIT/IMP; 3 MIN fixed — double-send, clamp doc, stale test doc).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42facbb333
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…_OPENS) Adds coverage for the unified pending_window_open queue's spam clamp: past the 1024 bound the NEW intent is dropped (not the oldest), so a runaway window.open loop stops adding work and the earliest calls survive in order. The design re-gate flagged this behavior as documented-but-untested; this closes that gap (and forces a genuine fresh Codex review for the 2nd dry-round confirmation on a changed head, since Codex dedups unchanged heads). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b57ed2e567
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…the landed note Codex P3 (MINOR, doc consistency): the §5.3 LANDED banner still named the pre-R2 seams (route_frame_navigations / take_pending_open_tabs / take_pending_frame_navigations / notify_open_new_tabs) that the R2 unification removed. Since the memo is the S5-4 SSoT, stale names would misdirect S5-6 follow-up. Rewrote the refinement bullets to the final delivered API (take_pending_window_opens / route_window_opens / pending_window_open), fixed the §4.3.2 supersede pointer (above, not below) to explicitly name the dead seams, and folded in the R2 single-queue + overflow-clamp deliverables. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 952f473528
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…turn + scheme filter
3 P2 findings on the window.open routing (all real, all fixed at the root):
- F1 (return semantics): process_pending_actions returned true on ANY non-empty
window.open queue, suppressing a co-located link's <a href> default nav even when
the open was a no-op — or, more broadly, even when a real _blank popup opened
(a browser opens the popup AND navigates the link). Root: the return conflated
'window.open effect' with 'own-context action'. Now returns true ONLY for an
own-context navigation (location.*/window.open _self/_top / history); window.open
tab/iframe opens are applied but never suppress the link default.
- F2 (drain order): pending_navigation (own-context, replaces pipeline) was drained
BEFORE pending_window_open, so window.open('/p','_blank'); window.open('/n','_self')
lost the popup. Now window.open opens (non-destructive) drain+apply FIRST, then the
pipeline-replacing navigation. The two channels stay distinct effect classes
(own-context last-wins vs other-context FIFO) — the bug was drain ORDER, not the
split, so no queue unification needed.
- F3 (security): the OpenNewTab / iframe-nav paths bypassed resolve_nav_url's
javascript:/vbscript: scheme block. All window.open tab/iframe URLs now route
through resolve_nav_url. Same-pattern audit: all OpenNewTab producers filtered.
route_window_opens returns WindowOpenOutcome { navigated_iframe, any_effect }.
Regression tests: no-op reports no effect; javascript: blocked; popup survives a
same-turn _self pipeline-replace; _blank popup does not suppress own-context.
Adversarial fix-delta re-check: CLEAN (0 CRIT/IMP/MIN).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fca7b0648
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…e-drain to S5-8 Codex R6 (4th window.open round) surfaced 2 P2 findings adjudicated OUT-OF-SCOPE by an adversarial cumulative design re-gate (delivered parent-window.open surface confirmed sound): - T2: named target resolved at drain time not call time (frame created post-call flips MISS→HIT). Un-fixable in-scope — the VM native can't reach the shell frame tree at call time without the S5-8 navigable model. Exactly the E3-documented NAME-axis deviation; non-security (aux-nav verdict snapshotted, rides the payload). - T3: iframe-originated window.open opens strand — process_pending_actions drains only the parent pipeline. PRE-EXISTING whole-iframe-intent-drain gap (iframe location/history/ <a target=_blank> all strand today), NOT an S5-4c regression; clean fix = uniform iframe-pipeline draining = the iframe browsing-context integration. Both FOLD into existing slot #11-browsing-context-model-window-open-postmessage (memo §8 D1). Genuine cross-PR boundaries needing the deferred model, not defer-as-edge. Threads reply-cited + resolved. No code change (delivered surface converged over R1/R2/R5). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…e stale VM queue name The §4.2 structural-guarantee table (:555) still named the pre-R2 VM field `pending_open_tabs`; the R2 two-queue→one-queue unification renamed it to `pending_window_open`. The §5.3 POST-LANDING DESIGN CORRECTION supersede blanket scopes only to the §4.3.2 two-queue design text, not the §4.2 table, so this cell read as a live present-tense reference to a queue field that no longer exists in the delivered code. The stated invariant (a blocked popup never enters the queue) holds unchanged under the unified queue — name-only drift, zero behavioral impact. Found by the external-converge TERMINAL cumulative fix-delta /elidex-review (Axis 5, 1 MIN; all other axes 0/0/0). Codex R7+R8 = 2-dry TERMINAL on the code. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
S5-4c — VM sandbox method gates + modals + window.open
Fourth slice of the S5-4 sandbox/security enforcement cluster (FLIP-precondition under the S5 umbrella). Lands the sandbox-method enforcement edges that only bite once the VM drives real shell traffic, so S5-6 swaps engines onto an already-gated surface. SoT =
docs/plans/2026-07-s5-4-sandbox-enforcement.md§5.3.Closes slot
#11-vm-sandbox-method-gates-and-modals.What lands
elidex-plugin::sandbox— newALLOW_TOP_NAVIGATION_BY_USER_ACTIVATIONbit + token,modals_allowedand the 2-flagtop_navigation_allowed(flags, activation)predicates (HTML §7.1.5 / §7.4.2.4), with full truth-table unit tests.elidex-script-session::navigation—OpenTabRequest/NamedFrameNavigationback-channel types + the pure engine-independentwindow_open_dispositiontarget dispatch (HTML §7.3.1.7).HostDrivergainstake_pending_open_tabs/take_pending_frame_navigations.vm/host/window_dialogs.rs) — marshal-onlyalert/confirm/prompt(HTML §8.9.1 cannot show simple dialogs — permanent step-4 opt-in, headless, boa-parity + spec-conformant) andwindow.open(§7.2.2.1 — spec-shaped dispatch, null-returning; WindowProxy = S5-8).NavigationStateFIFO back-channel queues.aux_nav_allowedsnapshot (§7.3.1.7 step 3) — closing a pre-existing ungated sandbox bypass. Link-target_top/_parentgate re-keyed ontotop_navigation_allowed(flags, true)(click = user activation → the 2-flag fidelity).modals_allowed/ top-nav sites re-keyed onto the canonicalelidex_plugin::sandboxpredicates (one-issue-one-way dedup); engine-agnosticJsRuntimedrain wrappers. No feature reshaping.Security by structure
Every gate is a chokepoint: a blocked popup never enters a queue (enqueue-time gating); the modal gate is a real chokepoint a future shell modal surface can only attach behind; the named-MISS aux-nav verdict is snapshotted at call time and consulted at drain, never re-reading live flags.
Spec-fidelity notes (webref-verified)
window.open("", "_self")(and_top/_parent/named-HIT) is a no-op — an empty url leaves urlRecord null and §7.2.2.1 step 16.1 navigates an existing navigable only for a non-null urlRecord; only new navigables (_blank/named-MISS) default to about:blank (step 15.3). A whitespace-only url is NOT empty (URL-parses to the document URL, a deliberate divergence from boa's non-spectrim()guard).Deferred carves
#11-transient-activation-tracking(D2) — real HTML user-activation model; the interim per-call-site static truth (script=false, link-click=true) is fail-closed.#11-browsing-context-model-window-open-postmessage(S5-8/B1).Pre-push gate
cargo fmt→mise run ci(green) →/simplify→/code-review high(4 confirmed findings, all fixed — incl. the empty-url spec bug caught in new code) →/review→/elidex-review(5-axis: 0 CRIT/IMP, 2 MIN fixed) + an adversarial re-verification of the empty-url fix. Umbrella §7 navigation-axis matrix updated.🤖 Generated with Claude Code