Skip to content

Fix session media image rendering - #3064

Merged
1 commit merged into
nesquena:masterfrom
franksong2702:franksong2702/media-session-images
May 28, 2026
Merged

1 commit merged into
nesquena:masterfrom
franksong2702:franksong2702/media-session-images

Conversation

@franksong2702

Copy link
Copy Markdown
Contributor

Thinking Path

  • Hermes WebUI renders agent-produced artifacts in chat via MEDIA: tokens.
  • Local MEDIA:/absolute/path.png tokens are rewritten to /api/media?path=... so the browser can show thumbnails.
  • /api/media correctly blocks arbitrary local paths, but it had no session-scoped allowance for exact image paths already present in the transcript.
  • The result was a broken-thumbnail regression for generated images saved outside the current workspace.
  • This PR keeps the broad local-path block in place while allowing exact safe image paths referenced by the requested session.

What Changed

  • Adds the current session_id to local MEDIA: image URLs generated by renderMd().
  • Adds a server-side session transcript check for exact local MEDIA: image paths.
  • Keeps unmentioned paths and non-image files rejected.
  • Adds focused regression coverage for allowed exact image paths, rejected unmentioned paths, rejected non-image paths, and the frontend session_id URL parameter.
  • Updates the changelog.

Why It Matters

Agent-generated images can live in project artifact folders outside the active WebUI workspace. Those images should render when the transcript itself explicitly references them, without expanding /api/media into broad home-directory access.

Closes #3063.

Verification

  • /Users/xuefusong/hermes-webui/.venv/bin/python -m pytest tests/test_media_inline.py -q
    • 46 passed
  • /Users/xuefusong/hermes-webui/.venv/bin/python -m pytest tests/test_renderer_js_behaviour.py::TestRendererSanitization::test_media_token_image_uses_delegated_lightbox_not_inline_js -q
    • 1 passed
  • Manual local runtime verification on 8787:
    • before restart, the affected MEDIA: image request returned 403 Forbidden
    • after restart, the same URL returned 206 Partial Content with Content-Type: image/png
    • browser image dimensions for the four affected cards were 1080 x 1440

Risks / Follow-ups

  • This only authorizes safe image MIME types by exact transcript reference. Audio/video/PDF/HTML local paths outside existing allowed roots remain blocked.
  • The check scans persisted session messages for matching MEDIA: tokens; very large sessions could add a small amount of work only when a media request includes a path outside the normal allowed roots.

Model Used

OpenAI GPT-5.3 Codex, via Codex desktop. AI assisted with diagnosis, implementation, testing, live runtime verification, issue creation, and PR preparation.

@franksong2702
franksong2702 marked this pull request as ready for review May 28, 2026 10:12
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Summary

Read the full diff (4 files, +113/-6), the new helpers in api/routes.py:7543-7588, the frontend hook at static/ui.js:3419-3423, and the three new unit tests at tests/test_media_inline.py:264-303. The shape is right: a narrowly-scoped session-scoped allowance that keeps the broad local-path block in place for everything that isn't an exact MEDIA:-referenced image. Closes #3063 cleanly.

What I verified

The contract chain holds end-to-end:

  1. Frontend opt-in is conditional and local-only. renderMd() at static/ui.js:3419-3423 appends &session_id=... only on the local-path branch (the http(s):// branch above returns before reaching this code), so remote URLs don't leak the session id. The change preserves the existing encodeURIComponent(ref) for the path:

    const mediaSessionId=(typeof S!=='undefined'&&S&&S.session&&S.session.session_id)?String(S.session.session_id):'';
    const apiUrl='api/media?path='+encodeURIComponent(ref)+(mediaSessionId?'&session_id='+encodeURIComponent(mediaSessionId):'');
  2. Server short-circuits 403 in the right order. _handle_media computes within_allowed first (lines 7660-7665, scanning the existing allowed-roots), then consults _session_media_token_allows_image_path() only as a fallback. The not within_allowed and not session_media_allowed guard at line 7676 means the helper runs only when the request fails the normal allowed-roots check — no performance penalty for the common case.

  3. Helper validates three conditions before returning True. _session_media_token_allows_image_path at api/routes.py:7558-7588:

    • non-empty sid (rejects ambient/unscoped requests)
    • MIME maps to one of {png, jpeg, gif, webp, x-icon, bmp} — explicitly not audio, video, PDF, HTML, or SVG (XSS risk on SVG is consistent with the existing _INLINE_PREVIEW_TYPES treatment)
    • target.resolve() == Path(ref).expanduser().resolve() against tokens scanned from the requested session's persisted messages

    That triple constraint matches the boundary the issue spelled out — and resolving both sides handles symlink equivalence correctly.

Two concerns

Resolve-time check assumes message provenance

_session_media_token_allows_image_path trusts that persisted assistant messages contain agent-emitted MEDIA: paths, not request-controlled input. Practically that's true today — session.messages is written by the agent path, not by anonymous POSTs. But if any future code path lets a user inject arbitrary text into a session transcript (e.g. an editable user-message draft that gets persisted as part of a session compaction), the trust model would shift. Worth a one-line comment near _MEDIA_TOKEN_RE documenting that the security boundary depends on persisted-message provenance, so a future refactor doesn't quietly undermine it.

Test coverage gap: list-of-parts content

_message_content_text at lines 7546-7555 explicitly handles isinstance(content, list) with dict parts that carry .text, but no test exercises that branch. Codex/Anthropic assistant turns sometimes serialize as list-of-parts depending on which provider tagged the turn, so the branch is real, not theoretical. A two-line test message like:

session = SimpleNamespace(messages=[{
    "role": "assistant",
    "content": [{"type": "text", "text": f"MEDIA:{image}"}],
}])

would close it. Not a blocker.

On the existing test shape

The three new tests at tests/test_media_inline.py:264-303 cover happy path, unmentioned-path rejection, and non-image rejection. mock.patch.object(routes, "get_session", return_value=session) is the right seam (the helper has only one external dependency) and SimpleNamespace with messages=[{...}] matches what Session.compact() exposes. The string-content branch is well-exercised; only the list-of-parts branch above is missing.

Verdict

Approved. The frontend/server pair is well-scoped, the security boundary holds, the regex in _MEDIA_TOKEN_RE matches the frontend's /MEDIA:([^\s\)\]]+)/g at static/ui.js:2968, and CI is green on 3.11/3.12/3.13. The list-of-parts test case can land separately if the contributor wants.

nesquena-hermes pushed a commit that referenced this pull request May 28, 2026
Per Opus advisor on stage-batch36: skip role='user' messages in
_session_media_token_allows_image_path so a user-injected MEDIA: token
cannot mint an allow-list entry for the user's own request. Preserves
the original use case (assistant/tool emitted artifacts outside the
active workspace) while making the implicit threat model explicit.

Defense-in-depth — the single-user WebUI scope means same-origin user
input already had the same effective access, but multi-user / shared
WebUI deployments would benefit from the restriction.
@nesquena-hermes nesquena-hermes closed this pull request by merging all changes into nesquena:master in 6267716 May 28, 2026
@nesquena-hermes

Copy link
Copy Markdown
Collaborator

Shipped in v0.51.154 / Release DZ (stage-batch36, commit 6267716). Thanks for the contribution!

ai-ag2026 pushed a commit to ai-ag2026/hermes-webui that referenced this pull request May 28, 2026
# Conflicts:
#	CHANGELOG.md
ai-ag2026 pushed a commit to ai-ag2026/hermes-webui that referenced this pull request May 28, 2026
9-PR medium-risk cleanup:
- nesquena#3037 routes.py: argv-style prefill hook + env-var override for notes drawer
- nesquena#3046 models.py: compression parent not repaired as stale interrupted turn
- nesquena#3048 session_discoverability.py: --repair-safe CLI with default dry-run
- nesquena#3053 ui.js: streaming KaTeX guard for parser-owned equations
- nesquena#3059 models.py: empty partial activity rows excluded from sidebar recency
- nesquena#3060 profiles.py: API key writes to .env (chmod 600), not config.yaml
- nesquena#3064 routes.py: MEDIA: image tokens allow exact session-referenced paths
- nesquena#3069 models.py: cron sessions with project_id surface via Cron Jobs chip
- nesquena#3077 gateway_chat.py: HTTP 401 maps to gateway_auth_error event
SysAdminDoc pushed a commit to SysAdminDoc/hermes-webui that referenced this pull request Jun 26, 2026
# Conflicts:
#	CHANGELOG.md
SysAdminDoc pushed a commit to SysAdminDoc/hermes-webui that referenced this pull request Jun 26, 2026
9-PR medium-risk cleanup:
- nesquena#3037 routes.py: argv-style prefill hook + env-var override for notes drawer
- nesquena#3046 models.py: compression parent not repaired as stale interrupted turn
- nesquena#3048 session_discoverability.py: --repair-safe CLI with default dry-run
- nesquena#3053 ui.js: streaming KaTeX guard for parser-owned equations
- nesquena#3059 models.py: empty partial activity rows excluded from sidebar recency
- nesquena#3060 profiles.py: API key writes to .env (chmod 600), not config.yaml
- nesquena#3064 routes.py: MEDIA: image tokens allow exact session-referenced paths
- nesquena#3069 models.py: cron sessions with project_id surface via Cron Jobs chip
- nesquena#3077 gateway_chat.py: HTTP 401 maps to gateway_auth_error event
SysAdminDoc pushed a commit to SysAdminDoc/hermes-webui that referenced this pull request Jun 26, 2026
…le messages

Per Opus advisor on stage-batch36: skip role='user' messages in
_session_media_token_allows_image_path so a user-injected MEDIA: token
cannot mint an allow-list entry for the user's own request. Preserves
the original use case (assistant/tool emitted artifacts outside the
active workspace) while making the implicit threat model explicit.

Defense-in-depth — the single-user WebUI scope means same-origin user
input already had the same effective access, but multi-user / shared
WebUI deployments would benefit from the restriction.
bernyforce pushed a commit to bernyforce/hermes-webui that referenced this pull request Jul 29, 2026
# Conflicts:
#	CHANGELOG.md
bernyforce pushed a commit to bernyforce/hermes-webui that referenced this pull request Jul 29, 2026
9-PR medium-risk cleanup:
- nesquena#3037 routes.py: argv-style prefill hook + env-var override for notes drawer
- nesquena#3046 models.py: compression parent not repaired as stale interrupted turn
- nesquena#3048 session_discoverability.py: --repair-safe CLI with default dry-run
- nesquena#3053 ui.js: streaming KaTeX guard for parser-owned equations
- nesquena#3059 models.py: empty partial activity rows excluded from sidebar recency
- nesquena#3060 profiles.py: API key writes to .env (chmod 600), not config.yaml
- nesquena#3064 routes.py: MEDIA: image tokens allow exact session-referenced paths
- nesquena#3069 models.py: cron sessions with project_id surface via Cron Jobs chip
- nesquena#3077 gateway_chat.py: HTTP 401 maps to gateway_auth_error event
bernyforce pushed a commit to bernyforce/hermes-webui that referenced this pull request Jul 29, 2026
…le messages

Per Opus advisor on stage-batch36: skip role='user' messages in
_session_media_token_allows_image_path so a user-injected MEDIA: token
cannot mint an allow-list entry for the user's own request. Preserves
the original use case (assistant/tool emitted artifacts outside the
active workspace) while making the implicit threat model explicit.

Defense-in-depth — the single-user WebUI scope means same-origin user
input already had the same effective access, but multi-user / shared
WebUI deployments would benefit from the restriction.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Local MEDIA image tokens can render as broken thumbnails outside workspace

2 participants