fix(dashboard): let a frame re-issue its own sandbox-doc GET (#6176)

[bolichen97]

Summary

Fixes #6176 — generated widgets and HTML artifacts render blank in the desktop app while working fine in a browser.

Revision 2 rewrites this PR. The first revision claimed two root causes and shipped a fix for each; one of them was wrong. Both blocking reviews were correct and the diff is now 3 files instead of 5.

Root cause

The sandbox-doc handler _stash.pop()'d the entry on the first GET, so the URL was permanently spent. A frame that re-issues the GET it just made — renderer recovery, restore from tray, back/forward navigation, all of which Electron does — got a 404. That 404 lands inside the frame, where nothing in the frontend observes it (the document is served with an opaque origin, so the parent cannot read whether it loaded), and the frame stays at opacity: 0 until onLoad fires. The result is a permanently blank area with no error and no retry affordance.

Changes

File	Change
src/kiro_crew/dashboard/handlers/sandbox_doc.py	The first successful GET collapses the entry's deadline to _IN_FLIGHT_GRACE_SECS instead of popping it — same lock, min() so a replay cannot push the deadline out. _evictable rewritten to the equivalent form that reads honestly for both a fresh mint and a served entry (same inequality, no behaviour change).
test/test_sandbox_doc_channel.py	Re-issue works; the deadline collapses; a replay does not extend it; the URL is dead past the window; source-level guard that the collapse cannot be silently reverted to a plain read.
website/src/hooks/useSandboxDoc.ts	Corrects the retry doc comment, which asserted the URL was single-use with no grace.

Responding to the blocking reviews

GPT 5.6 — "replayable token bypasses client binding behind shared proxies" (BLOCKING). Correct, and the first revision made it worse by deleting the paragraph that documented exactly this: request.remote is the proxy's address whenever the dashboard is reached through a tunnel or reverse proxy, so every client shares it and the binding is worth nothing precisely where a leaked URL is most reachable. A plain _stash.get() therefore left a 15-minute replay window for any co-tenant behind that proxy.

A bare pop() cannot be restored, because it is the cause of the bug this PR closes. Session-cookie auth is foreclosed by design — the module docstring records that a sandboxed frame in some engines does not send one, which is why the route is on the auth bypass list at all. So the resolution is to keep the spend and bound the window: serving collapses the deadline to seconds (_IN_FLIGHT_GRACE_SECS, 10s), long enough for a renderer to re-issue a GET that follows its predecessor by milliseconds, 90× shorter than the mint TTL the plain read left in place. The reasoning is restored to the docstring rather than removed, and pinned by a source-level test.

First Principles — "the frame-ancestors strip has no reachable cause" (BLOCK). Correct. Verified independently: the mint returns a relative url (sandbox_doc.py), all four consumers (WidgetFrame, ArtifactBody, ArtifactsPage, RemoteArtifactDetailPage) assign it straight to iframe src, and the desktop window loads the dashboard top-level at http://localhost:<port> (main.js BACKEND_URL). A relative src resolves against the parent's own origin, so frame and parent origins are identical by construction and frame-ancestors 'self' matches whatever the host spelling — there is no localhost vs 127.0.0.1 producer in this tree. website/electron/sandbox-doc-headers.js, its test file, and the onHeadersReceived block are all deleted, which is also what the failing Electron Shell Tests check was reporting (Missing from build.files: sandbox-doc-headers.js).

The now-false sentence in useSandboxDoc.ts is fixed as well.

Not in scope

First Principles' second Watch item — _evictable cap-evicting an entry ~10s after mint, 404ing a legitimate late re-fetch — is pre-existing on main and independent of this change. It is left alone here so this diff stays the size of the defect it closes.

Security

• Single use remains the load-bearing control; the replay window is bounded to seconds and is not extendable by fetching in a loop.
• Content-Security-Policy: sandbox on the response is untouched, and no CSP is now rewritten anywhere in the Electron shell — model-authored HTML still gets an opaque origin.
• Stash caps (_MAX_ENTRIES, _MAX_BYTES) unchanged; a served entry becomes evictable, so the first revision's added stash_full pressure is gone too.
• Every authorization decision stays SEL-audited.

Testing

• test/test_sandbox_doc_channel.py — 42 passed
• website/electron shell-contract.test.js — 5 passed (was failing on the deleted module)
• black ratchet, isort, flake8, mypy, tsc --noEmit, eslint — all clean
• Rebased onto main @ 24cb5e76c

[github-actions]

UX Review (Fable 5) — ✅ PASS

UX-level review of 330b1498640ba4bed4ada046df11353b48d8021a — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: PASS

Backend fix with no new UI surface; it removes a silent blank-frame failure, and the one string touched is a doc comment made accurate.

Watch

• The grace window is measured from the first serve, so a frame reloaded >10s later (e.g. restore from tray after minutes minimized) still 404s into a permanently blank area with no error or retry — the docstring itself confirms "the retry control still appears only when the MINT itself fails." Low frequency, but the failure is invisible and unrecoverable without a full page reload; worth confirming the Desktop app renders generated widgets/HTML artifacts blank (sandboxed iframe); fine in browser #6176 repro is the fast-re-issue case, and tracking a spent-load detection follow-up.

[UX-REVIEWED] 330b149

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 330b1498640ba4bed4ada046df11353b48d8021a — this comment is updated in place on each push.

Review details

Served entries within their grace window are still classified evictable, so gallery pressure can re-blank the exact frame this PR set out to fix.

FINDING — src/kiro_crew/dashboard/handlers/sandbox_doc.py:112 — a serve collapses the entry to exp = min(..., now + _IN_FLIGHT_GRACE_SECS) (≈now + 10), which satisfies if exp <= cutoff where cutoff = now + _TOKEN_TTL_SECS - _IN_FLIGHT_GRACE_SECS (≈now + 890), so _evictable() treats a still-within-grace served entry as evictable — under stash pressure (_over()) a new mint evicts it instead of refusing with 503, and the owning frame's re-issued GET then hits _stash.get(doc_id) None → 404 → permanently blank frame; the _evictable comment's claim that "one comparison covers them" is inverted (the comparison protects fresh mints, exp > cutoff, but exposes served entries, exp ≈ now + grace), contradicting the _prune docstring's "never touches an entry ... fetched within the last _IN_FLIGHT_GRACE_SECS" → Fix: also protect recently-served entries in _evictable, e.g. skip any entry with exp <= now + _IN_FLIGHT_GRACE_SECS in addition to keeping exp > cutoff, so only aged-unserved mints in the middle band remain evictable.

[OPUS-REVIEWED] 330b149

Verdict parsed from the review's SHA-scoped output markers for commit 330b1498640ba4bed4ada046df11353b48d8021a.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 330b1498640ba4bed4ada046df11353b48d8021a: <one-sentence reason>

[github-actions]

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 330b1498640ba4bed4ada046df11353b48d8021a.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/dashboard/handlers/sandbox_doc.py:230 -- replay grace makes the bearer URL reusable behind shared proxies
entry = _stash.get(doc_id, None)
Model HTML exfiltrates its URL -> attacker replays it through the same proxy within 10 seconds -> auth-bypassed GET returns the document.
Anchor: residual/security
Fix: Restore atomic _stash.pop(doc_id, None) consumption.
[BLOCK-MERGE] 330b149
[GPT-REVIEWED] 330b149
False positive or not applicable? A repository writer can comment:
/ai-review override gpt 330b1498640ba4bed4ada046df11353b48d8021a: <one-sentence reason>

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 330b1498640ba4bed4ada046df11353b48d8021a — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

The 10-second grace only rescues re-issues that follow within milliseconds; two of the three triggers the fix names can arrive minutes later and still blank silently.

Watch

• The docstring justifies the window with "a re-issued GET follows its predecessor by milliseconds, not minutes," but restore-from-tray and history navigation — both cited as causes in "renderer recovery, restore from tray, history navigation — all of which Electron does" — re-issue at restore/navigation time, which is unbounded relative to the first serve. Those paths still 404 inside the frame with no observability or retry affordance, so the reported symptom (permanently blank widget) survives for them; confirm which trigger Desktop app renders generated widgets/HTML artifacts blank (sandboxed iframe); fine in browser #6176 actually reproduced, or the bug reopens on the slow paths.
• The silent-failure mode itself (parent cannot observe an in-frame 404, iframe pinned at opacity: 0) is untouched and now guards a narrower defect; the deferred beacon/deadline is the eventual root fix and deserves a tracked follow-up rather than only a docstring note.

[DESIGN-REVIEWED] 330b149

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 330b1498640ba4bed4ada046df11353b48d8021a — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

First-Principles-Verdict: CONCERNS

The fix is derived and minimal, but the new comments promise served entries a grace-window eviction shield the unchanged inequality mathematically never provides.

What this change ships

Intent: stop desktop-app widgets/artifacts rendering permanently blank when a frame re-issues the GET for its already-spent sandbox-doc URL — a FIX (#6176).

1. A frame can re-load its own document for ~10s after first load — justified (reported defect).
2. A served URL dies after the grace window instead of surviving its 15-min mint TTL — justified, derived from the proxy-replay boundary.
3. A served entry becomes evictable under stash pressure — declared, but contradicted by the diff's own comments (see Watch).
4. Frontend retry doc comment no longer claims the URL is strictly single-use — justified.
5. Source-level pin tests swap from "pop exists" to "collapse exists, under the lock" — justified; guards the load-bearing control.

No new config key, flag, or exported surface; the fix reuses _IN_FLIGHT_GRACE_SECS. Sibling count: grepped pop|single|once|spent in webapp_preview.py (the channel the docstring mirrors) — 0 single-use serve patterns, so 0 unfixed siblings.

Watch

The description says "a served entry becomes evictable", yet the shipped comments say the opposite: "Within this window an entry must not be evicted … and a URL already served may be served again" (_IN_FLIGHT_GRACE_SECS) and "fetched within the last _IN_FLIGHT_GRACE_SECS" (_prune). The inequality (exp <= now + _TOKEN_TTL_SECS - _IN_FLIGHT_GRACE_SECS) makes a served entry (exp ≈ now+10s vs cutoff now+890s) evictable at second zero — so in the gallery-pressure scenario the module itself calls "normal use, not an edge case", a mint inside the window evicts the just-served entry and the blank frame returns. One side is false; a human should pick which.

Subtractions

• Delete the false protection claims: "or that was fetched within the last _IN_FLIGHT_GRACE_SECS" in _prune's docstring, "or served within it" in _evictable's comment, and "a URL already served may be served again" from the _IN_FLIGHT_GRACE_SECS comment — the comparison protects fresh mints only, which is what the description's Security section already admits.

[FIRST-PRINCIPLES-REVIEWED] 330b149

[bolichen97]

🤖 Holding this PR rather than pushing a third revision. GPT 5.6's re-block is correct, and while checking whether a narrower design could satisfy it I falsified the causal chain in my own revision-2 description. Both halves of this PR are now unsupported as fixes for #6176.

1. The frame-ancestors half had no reachable trigger — already established, and deleted in revision 2. The mint returns a relative url, all four consumers assign it straight to iframe src, and the desktop window loads the dashboard top-level at http://localhost:<port>, so frame and parent origins are identical by construction.

2. The single-use half does not produce the reported symptom either. Revision 2 claimed a spent-URL re-fetch renders as a permanently blank area because the 404 lands inside the frame and the iframe stays at opacity: 0 until onLoad fires. The second clause is right — WidgetFrame.tsx and ArtifactBody.tsx both gate opacity on onLoad. The first is wrong:

>>> e = web.HTTPNotFound()
status: 404   content_type: text/plain   body: b'404: Not Found'

An iframe navigation that receives a 404 with a body is a completed navigation, so load fires, opacity goes to 1, and the frame renders the visible text 404: Not Found. A user hitting the spent-URL path would report a frame reading "404: Not Found", not a blank area. #6176 reports blank with no error, in both chat and the Artifacts tab.

3. There is no design that satisfies both constraints. GPT's threat model is that model-authored HTML reads its own URL, exfiltrates it, and a co-tenant replays it through the shared proxy address. Distinguishing "the same frame re-loading" from "another client holding the same URL" needs something the attacker lacks, and behind a proxy there is nothing: the client address is shared, session cookies are foreclosed (the module docstring records that a sandboxed frame in some engines does not send one — that is why this route is on the auth bypass list), and Sec-Fetch-* is forgeable. So tolerating a bare-URL re-fetch necessarily opens a replay window, and shrinking that window from 900s to 10s narrows the race without closing it. GPT is not asking for something unreasonable; single use really is the only control that survives a shared proxy.

Since the re-fetch tolerance buys no confirmed fix for #6176, paying a security relaxation for it is the wrong trade. Not overriding.

What #6176 actually needs: the root cause is still unidentified. The symptom — a frame that loads and paints nothing, no error text, on two independent surfaces, desktop-only — points away from the transport (both halves here) and toward the document rendering at zero effective size or with nothing painted. The next honest step is instrumentation on a desktop build (does onLoad fire? what is the frame's getBoundingClientRect()? does the response arrive with status 200 and the expected byte length?), not another speculative transport fix.

[bolichen97]

Closing this superseded implementation rather than resolving its conflict. The hold analysis established that the proposed replay window does not explain #6176 and weakens the single-use sandbox URL boundary. Safer document-load recovery work has since landed in #6461 and #6481. Keeping #6176 open for reporter verification and further desktop instrumentation; this branch is not deleted.