diff --git a/CHANGELOG.md b/CHANGELOG.md index 7a6d8d8e..6620a5bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ ### Fixed +- **HTML and PDF artifacts saved into a chat session now preview inline again instead of failing to load.** Inline previews fetch the artifact through `api/media`, but session-backed artifacts (files outside the workspace roots, authorized by the per-session media token) were fetched without the `session_id`, so the request couldn't present the grant and the preview silently failed. The PDF and HTML inline loaders now thread the active `session_id` into the `api/media` fetch URL (and the download / open-full-page fallbacks), so a session-scoped artifact authorizes and renders — HTML still only renders inside the `sandbox="allow-scripts"` iframe, SVG stays a download, and the secret/state hard-deny still runs before any grant. Thanks @santastabber. (#5157) - **The Workspace → Todos panel and the sidebar task list now render identically.** Both surfaces showed the same todo data but through two separately-maintained renderers, so row markup could drift between them. They now share one set of helpers (`renderTodoRows` / `renderTodoRow` / `renderTodoEmptyState` + a frozen status-rendering map in `static/ui.js`), with the empty state standardized on the same `todos_no_active` string. No change to todo state, hydration, or persistence — purely a rendering-consistency de-dup, with all user-controlled fields escaped through the shared path. Thanks @Stacey2911. (#5103) - **The version badge and in-app updater now work on macOS when the WebUI is launched by launchd.** A launchd-started process inherits a minimal `PATH` that doesn't include git's location (e.g. Homebrew), so `api/updates.py` running a bare `git` failed — breaking both the displayed version and the update checker on those installs. Git is now resolved once in `_run_git()`: normal `PATH` lookup first (byte-identical on Linux/CI and every non-miss), falling back to `/usr/bin/git` only on macOS when `PATH` lookup misses; a `.git`-absent install keeps its existing `no_git` path and the missing-git diagnostic is preserved when no executable exists. Thanks @rodboev. (#5184, fixes #5175) - **Settings search now finds the busy-input-mode setting by its option text and description, not just its label.** The Settings search index was built from each field's *label* only, so searching for natural terms like "queue", "interrupt", "steer", "message", or "default" didn't surface the busy-input-mode control even though those words describe exactly what it does. The index now also collects each field's option text, description text, and optional per-field supplemental search terms (kept a strict superset of the old label-only match, so existing results are unaffected), and the busy-input field carries supplemental terms so it's discoverable by those words. Thanks @rodboev. (#5163, fixes #5148) diff --git a/api/routes.py b/api/routes.py index 1316653d..4642ac35 100644 --- a/api/routes.py +++ b/api/routes.py @@ -15111,13 +15111,13 @@ def _message_content_text(content) -> str: return str(content or "") -def _session_media_token_allows_image_path(sid: str, target: Path, image_mimes: set[str]) -> bool: - """Allow exact MEDIA:image paths already present in the requested session.""" +def _session_media_token_allows_path(sid: str, target: Path, allowed_mimes: set[str]) -> bool: + """Allow exact safe MEDIA: paths already present in the requested session.""" sid = str(sid or "").strip() if not sid: return False mime = MIME_MAP.get(target.suffix.lower(), "application/octet-stream") - if mime not in image_mimes: + if mime not in allowed_mimes: return False try: target_resolved = target.resolve() @@ -15152,6 +15152,11 @@ def _session_media_token_allows_image_path(sid: str, target: Path, image_mimes: return False +def _session_media_token_allows_image_path(sid: str, target: Path, image_mimes: set[str]) -> bool: + """Backward-compatible image-only wrapper for existing callers/tests.""" + return _session_media_token_allows_path(sid, target, image_mimes) + + def _path_is_within_root(child: Path, root: Path) -> bool: """Return True when ``child`` is inside ``root`` without crashing on Windows drives.""" try: @@ -15166,7 +15171,7 @@ def _handle_media(handler, parsed): Security: - Path must resolve to an allowed root (hermes home, /tmp, common dirs) - Auth-gated when auth is enabled - - Only image MIME types are served inline; all others force download + - Safe preview MIME types can render inline when requested; SVG always downloads - SVG always served as attachment (XSS risk) - No path traversal: resolved path must stay within an allowed root - Additional roots can be added via MEDIA_ALLOWED_ROOTS env var @@ -15240,10 +15245,17 @@ def _handle_media(handler, parsed): for root in allowed_roots if root.exists() ) - session_media_allowed = _session_media_token_allows_image_path( + _AUDIO_VIDEO_PDF_TYPES = { + "audio/mpeg", "audio/wav", "audio/x-wav", "audio/mp4", "audio/aac", + "audio/ogg", "audio/opus", "audio/flac", + "video/mp4", "video/quicktime", "video/webm", "video/ogg", + "application/pdf", + } + _SESSION_MEDIA_TOKEN_TYPES = _INLINE_IMAGE_TYPES | _AUDIO_VIDEO_PDF_TYPES | {"text/html"} + session_media_allowed = _session_media_token_allows_path( qs.get("session_id", [""])[0], target, - _INLINE_IMAGE_TYPES, + _SESSION_MEDIA_TOKEN_TYPES, ) # ── #3234: hard-deny Hermes's own state + secret/config files ──────────── @@ -15429,12 +15441,7 @@ def _handle_media(handler, parsed): # Only serve safe media/PDF types inline when explicitly requested. HTML is # allowed inline only with a CSP sandbox so "open full page" can work without # granting same-origin access to the WebUI. SVG is always a download (XSS risk). - _INLINE_PREVIEW_TYPES = _INLINE_IMAGE_TYPES | { - "audio/mpeg", "audio/wav", "audio/x-wav", "audio/mp4", "audio/aac", - "audio/ogg", "audio/opus", "audio/flac", - "video/mp4", "video/quicktime", "video/webm", "video/ogg", - "application/pdf", - } + _INLINE_PREVIEW_TYPES = _INLINE_IMAGE_TYPES | _AUDIO_VIDEO_PDF_TYPES _DOWNLOAD_TYPES = {"image/svg+xml"} # SVG: XSS risk, force download inline_preview = qs.get("inline", [""])[0] == "1" html_inline_ok = inline_preview and mime == "text/html" diff --git a/static/ui.js b/static/ui.js index 9c29b8ac..54296679 100644 --- a/static/ui.js +++ b/static/ui.js @@ -14808,19 +14808,23 @@ function loadPdfInline(container){ el.setAttribute('data-loaded','1'); const path=el.dataset.path; const fname=path.split('/').pop()||path; + const mediaSessionId=(typeof S!=='undefined'&&S&&S.session&&S.session.session_id)?String(S.session.session_id):''; + const publicMediaUrl='api/media?path='+encodeURIComponent(path); + const mediaUrl=publicMediaUrl+(mediaSessionId?'&session_id='+encodeURIComponent(mediaSessionId):''); const loadPdf=(pdfjsLib)=>{ - fetch('api/media?path='+encodeURIComponent(path)) + fetch(mediaUrl) .then(r=>{if(!r.ok) throw new Error(r.status); return r.arrayBuffer();}) .then(buf=>{ if(buf.byteLength>PDF_MAX_SIZE){ - el.outerHTML=`
📎 ${esc(fname)}
${t('pdf_too_large')}
`; + const dlUrl=publicMediaUrl+'&download=1'; + el.outerHTML=`
📎 ${esc(fname)}
${t('pdf_too_large')}
`; return; } return pdfjsLib.getDocument({data:buf, isEvalSupported:false}).promise; }) .then(pdf=>{ if(!pdf) return; - const dlUrl='api/media?path='+encodeURIComponent(path)+'&download=1'; + const dlUrl=publicMediaUrl+'&download=1'; const total=pdf.numPages; const pagesLabel=total>1?` · ${total} pages`:''; const wrap=document.createElement('div'); @@ -14859,7 +14863,7 @@ function loadPdfInline(container){ renderPage(1); }) .catch(()=>{ - const dlUrl='api/media?path='+encodeURIComponent(path)+'&download=1'; + const dlUrl=publicMediaUrl+'&download=1'; el.outerHTML=`
📎 ${esc(fname)}
${t('pdf_error')}
`; }); }; @@ -14879,7 +14883,7 @@ function loadPdfInline(container){ window.addEventListener('pdfjs-ready',()=>{ _pdfjsReady=true; loadPdf(window._pdfjsLib); },{once:true}); setTimeout(()=>{ if(!_pdfjsReady){ - const dlUrl='api/media?path='+encodeURIComponent(path)+'&download=1'; + const dlUrl=publicMediaUrl+'&download=1'; if(el.parentNode){ el.outerHTML=`
📎 ${esc(fname)}
${t('pdf_error')}
`; } @@ -14899,20 +14903,23 @@ function loadHtmlInline(container){ el.setAttribute('data-loaded','1'); const path=el.dataset.path; const fname=path.split('/').pop()||path; - fetch('api/media?path='+encodeURIComponent(path)) + const mediaSessionId=(typeof S!=='undefined'&&S&&S.session&&S.session.session_id)?String(S.session.session_id):''; + const publicMediaUrl='api/media?path='+encodeURIComponent(path); + const mediaUrl=publicMediaUrl+(mediaSessionId?'&session_id='+encodeURIComponent(mediaSessionId):''); + fetch(mediaUrl) .then(r=>{if(!r.ok) throw new Error(r.status); return r.text();}) .then(html=>{ if(html.length>HTML_MAX_SIZE){ - const openUrl='api/media?path='+encodeURIComponent(path)+'&inline=1'; + const openUrl=publicMediaUrl+'&inline=1'; el.outerHTML=`
📎 ${esc(fname)}
${t('html_too_large')}
`; return; } - const openUrl='api/media?path='+encodeURIComponent(path)+'&inline=1'; + const openUrl=publicMediaUrl+'&inline=1'; const safeHtml=html.replace(/&/g,'&').replace(/"/g,'"').replace(//g,'>'); el.outerHTML=`
${t('html_sandbox_label')}${t('html_open_full')} ↗
`; }) .catch(()=>{ - const dlUrl='api/media?path='+encodeURIComponent(path)+'&download=1'; + const dlUrl=publicMediaUrl+'&download=1'; el.outerHTML=`
📎 ${esc(fname)}
${t('html_error')}
`; }); }); diff --git a/tests/test_media_inline.py b/tests/test_media_inline.py index 34840111..2c69ed0c 100644 --- a/tests/test_media_inline.py +++ b/tests/test_media_inline.py @@ -292,11 +292,15 @@ class TestMediaEndpointUnit(unittest.TestCase): self.assertIn("image/svg+xml", routes_src, "SVG MIME type must be handled (forced download) in _handle_media") - def test_non_image_forces_download(self): - """Non-image files should be forced to download, not served inline.""" + def test_inline_preview_mime_whitelist_exists(self): + """Only the explicit safe preview whitelist should be eligible for inline display.""" routes_src = (REPO_ROOT / "api" / "routes.py").read_text(encoding="utf-8") self.assertIn("_INLINE_IMAGE_TYPES", routes_src, "_INLINE_IMAGE_TYPES whitelist must exist in _handle_media") + self.assertIn("_AUDIO_VIDEO_PDF_TYPES", routes_src, + "shared audio/video/PDF preview MIME whitelist must exist in _handle_media") + self.assertIn('{"text/html"}', routes_src, + "HTML must be added only to the session-token whitelist") def test_media_allowed_roots_env_var_referenced(self): """Handler must reference MEDIA_ALLOWED_ROOTS for configurable roots.""" @@ -638,6 +642,102 @@ class TestMediaEndpointUnit(unittest.TestCase): ) ) + def test_session_media_token_allows_exact_html_path_when_mime_is_safe(self): + from api import routes + + with tempfile.TemporaryDirectory() as tmpd: + html = pathlib.Path(tmpd) / "report.html" + html.write_text("

Report

", encoding="utf-8") + session = SimpleNamespace(messages=[{"role": "assistant", "content": f"MEDIA:{html}"}]) + with mock.patch.object(routes, "get_session", return_value=session): + self.assertTrue( + routes._session_media_token_allows_path( + "s-media", html, {"text/html"} + ) + ) + + def test_session_media_token_rejects_mentioned_html_when_mime_not_allowed(self): + from api import routes + + with tempfile.TemporaryDirectory() as tmpd: + html = pathlib.Path(tmpd) / "report.html" + html.write_text("

Report

", encoding="utf-8") + session = SimpleNamespace(messages=[{"role": "assistant", "content": f"MEDIA:{html}"}]) + with mock.patch.object(routes, "get_session", return_value=session): + self.assertFalse( + routes._session_media_token_allows_path( + "s-media", html, {"image/png"} + ) + ) + + def test_session_media_token_rejects_user_authored_html_path(self): + from api import routes + + with tempfile.TemporaryDirectory() as tmpd: + html = pathlib.Path(tmpd) / "report.html" + html.write_text("

Report

", encoding="utf-8") + session = SimpleNamespace(messages=[{"role": "user", "content": f"MEDIA:{html}"}]) + with mock.patch.object(routes, "get_session", return_value=session): + self.assertFalse( + routes._session_media_token_allows_path( + "s-media", html, {"text/html"} + ) + ) + + def test_handle_media_session_authorizes_html_artifact_outside_roots(self): + from api import routes + + class _Handler: + def __init__(self): + self.status = None + self.headers = {} + self.body = b"" + def send_response(self, code): + self.status = code + def send_header(self, k, v): + self.headers[k.lower()] = v + def end_headers(self): + pass + class _W: + def __init__(self, owner): + self.owner = owner + def write(self, b): + self.owner.body += b + def flush(self): + pass + @property + def wfile(self): + return self._W(self) + + with tempfile.TemporaryDirectory() as home, tempfile.TemporaryDirectory() as outside: + hermes_home = pathlib.Path(home) / ".hermes" + hermes_home.mkdir(parents=True) + ws = hermes_home / "workspace" + ws.mkdir() + html = pathlib.Path(outside) / "report.html" + html.write_text("

Report

", encoding="utf-8") + session = SimpleNamespace(messages=[{"role": "assistant", "content": f"MEDIA:{html}"}]) + with mock.patch.dict(os.environ, {"HERMES_HOME": str(hermes_home), "MEDIA_ALLOWED_ROOTS": ""}), \ + mock.patch.object(routes, "get_last_workspace", lambda: str(ws)), \ + mock.patch.object(routes, "get_session", return_value=session), \ + mock.patch("api.auth.is_auth_enabled", lambda: False): + handler = _Handler() + routes._handle_media( + handler, + SimpleNamespace( + query=( + f"path={urllib.parse.quote(str(html.resolve()))}" + "&session_id=s-media&inline=1" + ), + path="/api/media", + ), + ) + + self.assertEqual(handler.status, 200) + self.assertIn("text/html", handler.headers.get("content-type", "")) + self.assertIn("sandbox", handler.headers.get("content-security-policy", "")) + self.assertIn(b"Report", handler.body) + # ── Integration tests: live server on TEST_PORT ─────────────────────────────── # No collection-time skip guard — conftest.py starts the server via its diff --git a/tests/test_pdf_html_preview.py b/tests/test_pdf_html_preview.py index ab81fa69..51be6a2d 100644 --- a/tests/test_pdf_html_preview.py +++ b/tests/test_pdf_html_preview.py @@ -180,17 +180,37 @@ class TestLoadHtmlInlineFunction: """Must use srcdoc (not src) for HTML content to keep it same-origin sandboxed.""" ui = _read_js('ui.js') idx = ui.find('function loadHtmlInline') - body = ui[idx:idx + 1500] + body = ui[idx:idx + 2200] assert 'srcdoc=' in body, 'Must use srcdoc attribute for inline HTML rendering' def test_escapes_html_for_srcdoc(self): """HTML content must be escaped before embedding in srcdoc to prevent attribute injection.""" ui = _read_js('ui.js') idx = ui.find('function loadHtmlInline') - body = ui[idx:idx + 1500] + body = ui[idx:idx + 2200] # Must escape &, <, >, " to prevent breaking out of srcdoc attribute assert '&' in body or 'replace' in body, 'Must escape HTML entities for srcdoc' + def test_html_fetch_url_includes_session_id_for_session_media_artifacts(self): + ui = _read_js('ui.js') + idx = ui.find('function loadHtmlInline') + body = ui[idx:idx + 1200] + assert 'const mediaSessionId=' in body + assert "'&session_id='+encodeURIComponent(mediaSessionId)" in body + assert 'fetch(mediaUrl)' in body + assert "const publicMediaUrl='api/media?path='+encodeURIComponent(path);" in body + assert "const openUrl=publicMediaUrl+'&inline=1';" in body + + def test_pdf_fetch_url_includes_session_id_for_session_media_artifacts(self): + ui = _read_js('ui.js') + idx = ui.find('function loadPdfInline') + body = ui[idx:idx + 1200] + assert 'const mediaSessionId=' in body + assert "'&session_id='+encodeURIComponent(mediaSessionId)" in body + assert 'fetch(mediaUrl)' in body + assert "const publicMediaUrl='api/media?path='+encodeURIComponent(path);" in body + assert "const dlUrl=publicMediaUrl+'&download=1';" in body + # ── requestAnimationFrame integration ──────────────────────────────────────