Release v0.51.739 — session-backed HTML/PDF media previews (#5157, @santastabber) - #5191
Conversation
|
| Filename | Overview |
|---|---|
| api/routes.py | Generalizes _session_media_token_allows_image_path into _session_media_token_allows_path accepting any MIME set; widens the session-token allowlist to include HTML, PDF, audio, and video types. Server-side logic looks correct — hard-deny runs before the grant check, HTML is CSP-sandboxed, SVG stays download-only. |
| static/ui.js | Threads session_id into fetch() calls for PDF and HTML inline loaders — the core fix is correct. However, all browser-navigated href/download URLs (dlUrl, openUrl) are built from publicMediaUrl (no session_id), breaking download and "Open full page" links for session-backed artifacts outside workspace roots. |
| tests/test_media_inline.py | Adds unit tests for the new _session_media_token_allows_path API and an integration test verifying 200 + CSP sandbox for a session-authorized HTML artifact. |
| tests/test_pdf_html_preview.py | Adds snapshot-style tests for session_id threading; notably validates the current broken fallback behavior (asserts publicMediaUrl is used), so these would need updating alongside the fix. |
| CHANGELOG.md | Release changelog entry; no issues. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Browser
participant UI as ui.js
participant Server as api/routes.py
Browser->>UI: render session-backed artifact
UI->>UI: build mediaUrl with session_id
UI->>Server: fetch(mediaUrl) session_id present
Server->>Server: _session_media_token_allows_path checks MEDIA token
Server-->>UI: 200 file bytes
UI->>Browser: render inline PDF or sandboxed iframe
Browser->>UI: click Open full page or download
UI->>UI: build openUrl/dlUrl from publicMediaUrl (no session_id)
UI->>Server: GET without session_id
Server->>Server: "session_media_allowed=False within_allowed=False"
Server-->>Browser: 403
%%{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 Browser
participant UI as ui.js
participant Server as api/routes.py
Browser->>UI: render session-backed artifact
UI->>UI: build mediaUrl with session_id
UI->>Server: fetch(mediaUrl) session_id present
Server->>Server: _session_media_token_allows_path checks MEDIA token
Server-->>UI: 200 file bytes
UI->>Browser: render inline PDF or sandboxed iframe
Browser->>UI: click Open full page or download
UI->>UI: build openUrl/dlUrl from publicMediaUrl (no session_id)
UI->>Server: GET without session_id
Server->>Server: "session_media_allowed=False within_allowed=False"
Server-->>Browser: 403
Reviews (1): Last reviewed commit: "docs(changelog): authorize session-backe..." | Re-trigger Greptile
| const openUrl=publicMediaUrl+'&inline=1'; | ||
| const safeHtml=html.replace(/&/g,'&').replace(/"/g,'"').replace(/</g,'<').replace(/>/g,'>'); | ||
| el.outerHTML=`<div class="html-preview-wrap"><div class="html-preview-header"><span>${t('html_sandbox_label')}</span><a href="${openUrl}" target="_blank" rel="noopener" class="html-open-link">${t('html_open_full')} ↗</a></div><iframe srcdoc="${safeHtml}" sandbox="allow-scripts" class="html-preview-iframe" loading="lazy"></iframe></div>`; |
There was a problem hiding this comment.
"Open full page ↗" link omits
session_id for session-backed artifacts
openUrl is constructed from publicMediaUrl (no session_id), so when a user clicks the "Open full page ↗" link for a session-backed HTML artifact (the exact use case this PR fixes), the server resolves session_media_allowed = False and the within_allowed check also fails → 403. The PR description explicitly states this fallback was fixed, but it still uses publicMediaUrl instead of mediaUrl. Same issue applies to the "too large" branch on line 14913 and the catch branch's dlUrl on line 14922.
| const dlUrl=publicMediaUrl+'&download=1'; | ||
| el.outerHTML=`<div class="pdf-preview-fallback"><a class="msg-media-link" href="${dlUrl}" download="${esc(fname)}">📎 ${esc(fname)}</a><br><span style="color:var(--muted);font-size:12px">${t('pdf_too_large')}</span></div>`; |
There was a problem hiding this comment.
PDF download links omit
session_id for session-backed artifacts
Every dlUrl in loadPdfInline is built from publicMediaUrl (no session_id). For a session-backed PDF outside the workspace root, fetch(mediaUrl) succeeds and the preview renders, but clicking the download link (or any of the four fallback branches — too-large, success header, catch, and pdfjs-timeout) hits the server without a session_id, so session_media_allowed is False and the request 403s. dlUrl should be derived from mediaUrl (not publicMediaUrl) in all four occurrences.
Release v0.51.739 — session-backed HTML/PDF media previews (#5157, @santastabber)
Ships #5157 by @santastabber. Maintainer-approved after live browser testing (Nathan signed off on the inline preview rendering).
What it fixes
Inline PDF/HTML previews fetch the artifact through
api/media. Session-backed artifacts (files outside the workspace roots, authorized by the per-session media token) were fetched without thesession_id, so the request couldn't present the grant → the preview silently failed to load. The PDF and HTML inline loaders now thread the activesession_idinto theapi/mediafetch URL (and the download / open-full-page fallbacks). HTML still renders only inside thesandbox="allow-scripts"iframe, SVG stays a download, and the secret/state hard-deny runs before any grant. +159/-25, 4 files (api/routes.py,static/ui.js, 2 test files).Gate
055e57fac173(this PR's head): Codex SAFE TO SHIP on a clean rebase (round-1 kick-back was all stale-base artifacts from other shipped PRs, not this diff), Opus no-blockers, full suite 11073 passed, live HTTP boundary test 6/6 (assistant-path + matching session → 200 +Content-Security-Policy: sandbox allow-scripts; no-token / wrong-session //etc/passwd/ user-authored-token / SVG-inline all 403/attachment), in-process grant 5/5.Closes #5157.