Fix session-backed HTML media artifact previews - #5157
santastabber wants to merge 1 commit into
Conversation
|
| Filename | Overview |
|---|---|
| api/routes.py | Generalizes session MEDIA-token helper; adds shared _AUDIO_VIDEO_PDF_TYPES set; deny-list and role checks are preserved; logic is correct. |
| static/ui.js | loadHtmlInline and loadPdfInline now correctly attach session_id to fetch calls; download/open links use publicMediaUrl without the token; no new token-leakage surface introduced. |
| tests/test_media_inline.py | New unit tests cover HTML authorization, user-role rejection, MIME enforcement, and end-to-end _handle_media path for session-only artifacts; existing tests updated to match renamed symbols. |
| tests/test_pdf_html_preview.py | Adds structural tests verifying that session_id appears in fetch URLs and publicMediaUrl is used for open/download links; window-size bump for loadHtmlInline body scan is justified by new code. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant UI as Browser (ui.js)
participant API as /api/media (routes.py)
participant FS as Filesystem
UI->>UI: loadHtmlInline / loadPdfInline triggered
UI->>UI: read S.session.session_id → mediaSessionId
UI->>API: fetch(mediaUrl) [path + session_id]
API->>API: parse session_id from QS
API->>API: _session_media_token_allows_path(sid, target, _SESSION_MEDIA_TOKEN_TYPES)
API->>API: get_session(sid) → scan assistant/tool messages for MEDIA: token
alt token matches target and MIME allowed
API->>FS: read file
FS-->>API: bytes
API-->>UI: 200 OK
else no match
API-->>UI: 403 Forbidden
end
UI->>UI: embed content via srcdoc (HTML) or pdfjsLib (PDF)
Note over UI: openUrl/dlUrl use publicMediaUrl (no session_id)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant UI as Browser (ui.js)
participant API as /api/media (routes.py)
participant FS as Filesystem
UI->>UI: loadHtmlInline / loadPdfInline triggered
UI->>UI: read S.session.session_id → mediaSessionId
UI->>API: fetch(mediaUrl) [path + session_id]
API->>API: parse session_id from QS
API->>API: _session_media_token_allows_path(sid, target, _SESSION_MEDIA_TOKEN_TYPES)
API->>API: get_session(sid) → scan assistant/tool messages for MEDIA: token
alt token matches target and MIME allowed
API->>FS: read file
FS-->>API: bytes
API-->>UI: 200 OK
else no match
API-->>UI: 403 Forbidden
end
UI->>UI: embed content via srcdoc (HTML) or pdfjsLib (PDF)
Note over UI: openUrl/dlUrl use publicMediaUrl (no session_id)
Reviews (2): Last reviewed commit: "fix: authorize session html media artifa..." | Re-trigger Greptile
|
Triage review — focused on whether widening the session-token MIME allowlist keeps the SummaryReading Code referenceThe token matcher still refuses user-authored MEDIA tokens, so a user can't mint access to an arbitrary path ( role = str(message.get("role") or "").strip().lower()
if role == "user":
continue
...
for ref in _MEDIA_TOKEN_RE.findall(text):
if "://" in ref:
continue
if Path(ref).expanduser().resolve() == target_resolved:
return TrueCritically, the if any(_within_ci(target, d) for d in _deny_dirs):
return bad(handler, "Path not in allowed location", 403)
...
if not within_allowed and not session_media_allowed:
return bad(handler, "Path not in allowed location", 403)So even though a session token can now authorize HTML inline pathThe inline-disposition logic at html_inline_ok = inline_preview and mime == "text/html"
...
csp = "sandbox allow-scripts" if html_inline_ok else None
return _serve_file_bytes(handler, target, mime, disposition, "private, max-age=3600", csp=csp)
One nit
CI is green across 3.11/3.12/3.13 incl. |
🔬 Gate certification — GREEN ✅ (one low-severity hardening follow-up, non-blocking)Certified head: What I ran (isolated worktree
|
| Gate | Result |
|---|---|
| Full pytest suite | 10994 passed, 10 skipped, 1 xfailed, 2 xpassed, 2 failed (test_skills_stats_cache, test_git_stage_unstage_discard_and_commit) — both in files this PR does not touch, both pass in isolation = pre-existing order-dependent flakes, not a regression. CI is authoritative: 11/11 shards + browser-smoke + lint all green. |
| Codex (reproduce) | "SHIP ONLY WITH FIXES" — flagged session_id reaching the "Open full page" URL. Reproduced as real but low-severity (see adjudication). |
| Opus (full review) | "✅ APPROVE — correct, safe, ready to merge." session_id is an identifier not a credential; Codex's proposed fix would break the out-of-roots open-full-page path. |
| Live HTTP boundary test (running server, artifact genuinely outside all roots) | PASS 4/4: assistant-emitted HTML + session_id + inline → 200 text/html with Content-Security-Policy: sandbox allow-scripts; no session_id → 403; user-authored MEDIA token → 403; secret path (state.db) → 403 even with a valid session grant. |
node -c static/ui.js |
PARSE OK |
Findings
✅ Auth boundary fails closed (verified at code + live HTTP). _session_media_token_allows_path returns False on every branch: empty sid, mime-not-in-allowlist, resolve exception, get_session exception, role=="user" messages skipped (user-authored tokens cannot mint a grant), :// refs skipped, and exact resolved-path equality (Path(ref).resolve() == target_resolved) — no prefix/startswith/commonpath, so no traversal escape. The #3234 secret/state hard-deny runs before the session-grant decision, so a grant can never reach sessions//memories//secrets/state.db (live-confirmed: case D → 403).
✅ XSS-safe on both render paths. (a) Chat inline = <iframe srcdoc=… sandbox="allow-scripts"> (no allow-same-origin → opaque origin, can't touch parent DOM/cookies/localStorage); srcdoc escaping ordered &→"→<→> so no srcdoc="…" breakout. (b) Direct serve = Content-Security-Policy: sandbox allow-scripts + X-Content-Type-Options: nosniff. Invariant holds: text/html serves inline iff the sandbox CSP is set — there is no code path that serves HTML inline without it (live-confirmed: case A carried the CSP header).
✅ No new cross-session/cross-profile exposure. Single-user auth principal; session_id only selects which session's emitted tokens to match. The widening newly reaches only assistant/tool-emitted audio/video/pdf/html artifacts outside the standard roots — the PR's intent.
openUrl = mediaUrl + '&inline=1', where mediaUrl carries &session_id=<sid> (static/ui.js:14702, 14706). Clicking it navigates a tab to a sandbox allow-scripts (opaque-origin, scriptable) HTML page whose window.location.search contains the session_id, readable by the served (tool-authored) HTML.
- Why it is not ship-blocking: session_id is the conversation identifier, not a credential — it already appears in multiple client URLs, and every endpoint is gated by the independent auth layer (
is_auth_enabled/cookie/CSRF), so a leaked session_id authenticates nothing. The only capability it confers is requesting that same session's own already-known assistant-emitted artifacts (exact-path grant) — and the user was already viewing that session to click the link. No cross-session/cross-profile escalation. The default inline preview isabout:srcdocand carries no session_id; the leak requires an explicit click on attacker-authored HTML. - Do NOT apply Codex's literal fix (strip session_id from the open-full-page URL): that URL needs the session grant to serve an out-of-roots artifact, so stripping it breaks the exact feature this PR adds (Opus flagged this correctly). A safe future hardening, if desired: mint a short-lived opaque one-file token for the open-full-page navigation instead of reusing the session_id query param — separate follow-up, not this PR.
ℹ️ Cosmetic (non-blocking): _SESSION_MEDIA_TOKEN_TYPES duplicates the audio/video/pdf members of _INLINE_PREVIEW_TYPES (could be one constant); the two new JS tests are static source-string assertions (consistent with this repo's convention).
Recommendation to the next agent (release)
Ready to merge. Full gate is green: auth fails closed, render is sandboxed on both paths, full suite passes (the 2 local failures are pre-existing unrelated flakes; CI is 11/11 green), and the live HTTP boundary test passes 4/4. The single security item raised by Codex is low-severity and non-blocking (identifier, not credential; behind an explicit click; no escalation) and its literal fix would break the feature — Opus's APPROVE is the correct call on impact. Optionally file a follow-up for the opaque-token hardening of the open-full-page link; do not hold this PR for it. No code changes needed before merge.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy/close — that's the release agent's call. This certification is valid only while the head stays at 17a93388.
17a9338 to
a6fb355
Compare
a6fb355 to
055e57f
Compare
🔬 Gate certification — GREEN ✅Certified head:
What I ran (isolated worktree
|
| Gate | Result |
|---|---|
| Codex (reproduce, on clean rebase) | SAFE TO SHIP — see stale-base note |
| Opus (full review) | COMMENT — no blocking issues, 2 follow-up notes |
Full pytest suite (-p no:xdist, to completion) |
11073 passed, 10 skipped, 1 xfailed, 2 xpassed, 16 subtests passed, 0 failed (418s) |
Live HTTP boundary test (4+ cases, out-of-roots /var/tmp artifact) |
all correct — see below |
| In-process grant boundary | 5/5 — assistant-path+safe-mime → allow; wrong-session / wrong-mime / secret-path / SVG → deny |
⚠️ Important gate note — stale-base artifact (resolved)
A first Codex pass against current origin/master returned KICK BACK TO AUTHOR with 5 CORE/SILENT findings (streaming.py partial-failure handling, profile default workspace, profile/model repair, extension_settings.js, gateway restart). All five are stale-base artifacts, not defects in this PR — they describe code from other PRs (incl. #5181) that merged to master after this PR's base. Codex itself diagnosed the cause ("This head is stale against origin/master"). I verified by rebasing the branch onto current master (c35835d8) locally in my throwaway worktree only — the rebase is clean (rc=0) and the post-rebase diff vs master is exactly the 4 media-auth files (api/routes.py, static/ui.js, tests/test_media_inline.py, tests/test_pdf_html_preview.py). Re-running Codex on the clean rebase flipped it to SAFE TO SHIP. (Also confirms the PR is cleanly mergeable on current master.)
Live HTTP boundary results (real server, artifact genuinely outside all roots)
| Case | Result | |
|---|---|---|
assistant-authored HTML + valid session_id + inline=1 |
200, Content-Type: text/html, Content-Security-Policy: sandbox allow-scripts |
✅ correct sandboxed inline grant |
same HTML, no session_id |
403 | ✅ token required |
same HTML, wrong session_id |
403 | ✅ denied |
/etc/passwd + valid session_id |
403 | ✅ secret/out-of-session path denied |
SVG + valid session_id + inline=1 |
403 (never inline) | ✅ XSS-risk type blocked from inline |
HTML without inline=1 |
200, Content-Disposition: attachment |
✅ forces download, no inline render |
Findings
- ✅ Boundary is exact-path + assistant-authored + safe-mime + session-scoped.
_session_media_token_allows_pathgrants only a path the assistant emitted as aMEDIA:reference in that session, with a mime in the allowed set. User-authored paths, wrong-mime, and cross-session paths are all rejected (unit + live verified). State/secret deny runs before the allow decision. - ✅ HTML never gets same-origin. Inline HTML is served only with
Content-Security-Policy: sandbox allow-scripts; SVG is always an attachment (XSS). The sandboxed iframe carries thesession_idon its fetch; the user-navigable "open full page" + download links deliberately omit it. - ✅ Prior advisor finding addressed without breaking the feature. The earlier Codex note (session_id reaching a navigable script-capable page) is resolved by scoping the token to the sandboxed fetch — the supported working surface — rather than the self-defeating "strip it everywhere" fix.
⚠️ Non-blocking follow-up (Opus + Codex agree): the "open full page" / download / large-file fallback links 403 for out-of-roots artifacts because they omitsession_id. This is an intentional tradeoff (the sandboxed inline preview is the supported surface; the prior shipped path didn't authorize these previews at all), not a regression. A future hardening could mint a scoped token for those links.⚠️ Non-blocking: the PR description states open/download links carrysession_id; the code (correctly) does not. Worth a one-line PR-body correction.
Recommendation to the next agent
Ready to merge — cert fresh for sha:055e57fac173, no further review needed. The security boundary is intact and verified at the HTTP layer (Codex SAFE on clean rebase, Opus no-blockers, 11073/0 suite, 6/6 live boundary cases, 5/5 in-process grant). The branch rebases cleanly onto current master with a 4-file diff. The two follow-up notes are non-blocking and can be a separate hardening PR. This cert is valid only while the head stays at sha:055e57fac173.
Gate-certifier layer (warm-up → gate → release). I do not merge/tag/deploy/close — that's the release agent's call. Cert valid only at sha:055e57fac173; a new push invalidates it.
, @santastabber) Release v0.51.739 — session-backed HTML/PDF media previews (#5157, @santastabber)
|
Shipped in v0.51.739 (via #5191). Thanks @santastabber — session-backed HTML/PDF artifacts now thread the active session_id into the api/media fetch so they authorize and preview inline again (HTML still sandboxed, SVG stays a download, secret/state hard-deny unchanged). Gate-certified green + maintainer-approved after live browser testing of the inline preview. |
Thinking Path
/api/media.MEDIA:authorization only covered exact image paths, so assistant-emitted HTML/PDF artifacts outside the active workspace could fail even though they were present in the transcript./api/mediaURLs without carrying the activesession_id, so the backend could not prove the artifact was session-emitted.What Changed
text/html, PDF, audio, and video preview MIME types to the session-emitted artifact allowlist.session_idfor fetch, open, and fallback download links.MEDIA:paths outside the workspace;MEDIA:paths staying unauthorized;session_id.Why It Matters
HTML artifacts generated by an assistant can now open consistently from chat even when they are not under the active workspace, while still preserving the important security boundary:
MEDIA:paths cannot mint access;Verification
./scripts/test.sh tests/test_media_inline.py tests/test_pdf_html_preview.py tests/test_issue1800_file_html_interactions.py -q115 passednode --check static/ui.jspython3 -m py_compile api/routes.pypython3 scripts/ruff_lint.py --diff origin/mastergit diff --check origin/master...HEADRisks / Follow-ups
MEDIA:token for the requested session.MEDIA:artifact preview authorization.Model Used
OpenAI GPT-5.5 via OpenRouter, with Hermes Agent tool use for local implementation and verification.