fix(media): MEDIA: token regex strips markdown backtick on 7 capture sites (#7359)

## Thinking Path When an assistant emits `` `MEDIA:/path` `` wrapped in inline-code markdown, the shared character class ``[^\s\)\]]+`` across all seven MEDIA capture sites greedy-matched the closing backtick into the path. The resulting ``/api/media?path=...%60`` URL 404'd on the legitimate file (issue #7359). The same drift between frontend linkification and backend allowlist means even if the URL is constructed with the trailing backtick, the session allowlist rejects the backtick-polluted path — the user gets a broken download and a 404 body saved as ``.json``. Verified the issue's repro: 6 sites share ``[^\s\)\]]+`` (the preview-flattener at ui.js:9142 is ``[^\s]+`` and is a 7th site, also covered). One additional site, ``messages.js:4896`` (combined walk), was missing from the original 6-site list in the issue body — the suggested fix shape applies to all seven. The fix is a one-character-set widening: add `` ` `` to the excluded class on every site so the capture stops at the markdown delimiter instead of including it. ## What Changed All seven sites: ``[^\s\)\]]+`` → ``[^\s\)\]\`]+`` (JS) and the Python equivalent ``MEDIA:([^\s\)\]\`]+)``: 1. ``static/ui.js:7695`` — linkifier regex (the front-door capture). 2. ``static/ui.js:9142`` — preview-text flattener (also drops ``)`` and ``]`` for symmetric handling; was ``[^\s]+`` which would still pull the backtick into the preview). 3. ``static/messages.js:4848`` — streaming chunk finalize. 4. ``static/messages.js:4896`` — streaming combined walk (the issue-missing 7th site). 5. ``static/messages.js:4922`` — streaming partial tail. 6. ``api/routes.py:20453`` — ``_MEDIA_TOKEN_RE`` (the session allowlist). 7. ``api/media_snapshots.py:507`` — ``media_re`` (settle-time snapshot capture). ## Why It Matters A user clicking an inline-code-wrapped MEDIA token gets a 404 body saved as ``.json`` instead of the file they asked for. On Android Chrome (and desktop Chrome), the 404 silently downloads as ``skills.zip.json`` with no UI signal that the original was missing. The backend allowlist and the frontend linkifier must agree, so the fix widens the character class on both sides. ## Verification - ``tests/test_issue7359_media_backtick_strip.py``: 17 passed (new). - 7 site-shape assertions (parametrized over the seven sites). - ``_MEDIA_TOKEN_RE`` behaviour: strips trailing backtick from inline-code payload, preserves bare-token capture, preserves trailing-``)`` exclusion. - ``media_re`` behaviour: same three. - pre-fix class round-trip: ``\`MEDIA:/a.zip\``` → ``["/a.zip\`"]`` (the reported bug); post-fix → ``["/a.zip"]``. - ``node --check`` on the two modified JS files. - ``pytest tests/ -k 'media and not 7359'``: 291 passed, 1 skipped, 0 failed — no existing media test regresses. - ``ruff check tests/test_issue7359_media_backtick_strip.py``: clean. - ``scripts/ruff_lint.py --diff origin/master`` curated gate: 0 new findings on added/modified lines. ## Contract Change No API contract change. UI rendering may now display ``somefile.zip`` (a file) where it previously displayed ``somefile.zip\``` (literal backtick in the preview); both already displayed correctly in the bare-token case, and the inline-code path was the broken one. ## Risks - Sites that depended on the backtick being included in the path would regress, but no consumer in this repo treats a trailing `` ` `` as part of a MEDIA path — the original six-site fix shape was maintainer-recommended and the 7th site matches the same pattern. - The preview flattener at ``ui.js:9142`` previously used a wider class (``[^\s]+`` only, no ``)``/``]``) so it would have matched a path containing a literal ``)``. The fix tightens that to match the other six sites; if anyone relied on the looser class the rendering would now use ``a file`` for the surrounding paragraph. Verified no test in the suite exercises a MEDIA token containing ``)``. ## Model Used Hermes Agent (step-5-preview) with pytest, ruff, and node. Tests are executed against the live compiled regexes (for the two Python sites) and against the literal source text (for the five JS sites), so a revert on any of the seven sites surfaces as a source-shape failure. ## Related - #7359 — original issue (KimSpecht, 2026-08-29) with the repro and six-site list. - #7334 — pre-existing PR (angelusbr, 2026-08-27) that covers three sites; this PR supersedes it by covering all seven and following the maintainer-recommended shape from the 9/20 comment. - Maintainer's 9/20 comment on #7359 named #7334 as "the correct existing implementation lane"; if #7334 lands first, this PR is closeable as duplicate and the maintainer's ``sprint-candidate`` label can move. Credit goes to angelusbr for the early character-class idea regardless of which PR merges.
2026-09-22 17:24:29 +08:00
1 parent a1805abfb2
commit 2330faa0e4
@@ -504,7 +504,7 @@ def annotate_media_snapshots(
        resolve_ref = resolve_media_ref
    if allowed_predicate is None:
        allowed_predicate = media_capture_allowed
-    media_re = _re.compile(r"MEDIA:([^\s\)\]]+)")
+    media_re = _re.compile(r"MEDIA:([^\s\)\]\`]+)")
    captured = 0
    for msg in messages or []:
        if not isinstance(msg, dict) or msg.get("role") != "assistant":
@@ -20450,7 +20450,7 @@ def _serve_inline_html_preview(handler, target: Path, cache_control: str, *, csp
    return True


-_MEDIA_TOKEN_RE = re.compile(r"MEDIA:([^\s\)\]]+)")
+_MEDIA_TOKEN_RE = re.compile(r"MEDIA:([^\s\)\]\`]+)")


 def _message_content_text(content) -> str:
@@ -4845,7 +4845,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){
  function _smdMediaTailFlushEntry(entry){
    const chunk=_smdMediaTailEntryChunk(entry);
    if(!chunk) return;
-    const m=/^MEDIA:([^\s\)\]]+)$/.exec(String(chunk));
+    const m=/^MEDIA:([^\s\)\]\u0060]+)$/.exec(String(chunk));
    const emitted=!!(m && entry && entry.parent && _smdAppendMediaNode(entry.parent, m[1]));
    if(!emitted && entry) _smdMediaWriteText(entry.parent, entry.data, entry.baseAddText, entry.writeText, chunk);
  }
@@ -4893,7 +4893,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){
    // Prose runs go through the owning text writer. MEDIA tokens go through
    // the single-token DOMParser helper only after a delimiter or
    // reliable filename suffix proves the ref is complete.
-    const re=/MEDIA:([^\s\)\]]+)/g;
+    const re=/MEDIA:([^\s\)\]\u0060]+)/g;
    let last=0, m;
    let unmatchedTail=null;
    while((m=re.exec(combined))){
@@ -4919,7 +4919,7 @@ function attachLiveStream(activeSid, streamId, uploaded=[], options={}){
    // MEDIA prefix; flush any prose before the partial MEDIA suffix.
    const rest = combined.slice(last);
    if(rest){
-      const tailMatch = /MEDIA:[^\s\)\]]*$/.exec(rest);
+      const tailMatch = /MEDIA:[^\s\)\]\u0060]*$/.exec(rest);
      const prefixTail = tailMatch ? '' : _smdMediaPrefixTail(rest);
      const tailValue = tailMatch ? tailMatch[0] : prefixTail;
      if(tailValue && rest.length < _MEDIA_TAIL_MAX){
@@ -7692,7 +7692,7 @@ function renderMd(raw){
  // generated images) and replace them with inline <img> or download links.
  // Stashed so the path/URL is never processed as markdown.
  const media_stash=[];
-  s=s.replace(/MEDIA:([^\s\)\]]+)/g,(_,raw_ref)=>{
+  s=s.replace(/MEDIA:([^\s\)\]\`]+)/g,(_,raw_ref)=>{
    media_stash.push(raw_ref);
    return '\x00D'+(media_stash.length-1)+'\x00';
  });
@@ -9139,7 +9139,7 @@ function _stripForTTS(text){
  // Strip links, keep text
  text=text.replace(/\[([^\]]+)\]\([^)]+\)/g,'$1');
  // Replace MEDIA: paths with a simple label
-  text=text.replace(/MEDIA:[^\s]+/g,'a file');
+  text=text.replace(/MEDIA:[^\s\)\]\`]+/g,'a file');
  // Strip emoji and emoticons
  text=text.replace(/[\u{1F600}-\u{1F64F}\u{1F300}-\u{1F5FF}\u{1F680}-\u{1F6FF}\u{1F1E0}-\u{1F1FF}\u{2600}-\u{26FF}\u{2700}-\u{27BF}\u{FE00}-\u{FE0F}\u{1F900}-\u{1F9FF}\u{1FA00}-\u{1FA6F}\u{1FA70}-\u{1FAFF}\u{200D}]/gu,'');
  // Strip HTML tags that may leak through markdown
@@ -0,0 +1,214 @@
+"""Regression tests for issue #7359 — MEDIA: token regex must not capture trailing backtick.
+
+When an assistant emits a MEDIA token wrapped in markdown inline code
+(`` `MEDIA:/path/file.zip` ``), every capture regex in the frontend
+linkifier, preview flattener, streaming finalizers and backend allowlists
+greedily included the closing backtick in the captured path. The
+generated URL/allowlist entry ended with a stray backtick, so the file
+download link broke and the backend media allowlist rejected the
+legitimate path.
+
+The fix adds `` ` `` to the excluded character class on all seven capture
+sites:
+
+  - ``static/ui.js:7695``     linkifier
+  - ``static/ui.js:9142``     preview flattener
+  - ``static/messages.js:4848``  streaming chunk finalize
+  - ``static/messages.js:4896``  streaming combined walk
+  - ``static/messages.js:4922``  streaming partial tail
+  - ``api/routes.py:_MEDIA_TOKEN_RE``   backend session allowlist
+  - ``api/media_snapshots.py:media_re`` settle-time snapshot capture
+
+These tests verify the source shape on every site (so a site silently
+reverting back to the old class fails the suite) and exercise the two
+Python capture sites directly. The JS regexes are sibling enough to the
+Python ones that a backtick-safe Python regex plus a source-shape
+assertion is sufficient: if the JS regexes are not backtick-safe the
+end-to-end repro is a known one-line Markdown payload, and a follow-up
+node-driver test can be added when the maintainer wants it.
+"""
+from __future__ import annotations
+
+import re
+from pathlib import Path
+
+import pytest
+
+REPO_ROOT = Path(__file__).resolve().parents[1]
+
+# Source-shape: each site's expected backtick-safe regex literal. Listed
+# alongside the file so a missing/regressed site shows up as a clear diff.
+JS_SITES: dict[str, list[str]] = {
+    # path -> list of expected backtick-safe regex literals
+    "static/ui.js": [
+        r"/MEDIA:([^\s\)\]\`]+)/g",  # linkifier
+        r"/MEDIA:[^\s\)\]\`]+/g",    # preview flattener (single, no group)
+    ],
+    "static/messages.js": [
+        r"/^MEDIA:([^\s\)\]\u0060]+)$/",  # streaming chunk finalize
+        r"/MEDIA:([^\s\)\]\u0060]+)/g",  # streaming combined walk
+        r"/MEDIA:[^\s\)\]\u0060]*$/",    # streaming partial tail
+    ],
+}
+
+PY_SITES: dict[str, str] = {
+    "api/routes.py": r"MEDIA:([^\s\)\]\`]+)",
+    "api/media_snapshots.py": r"MEDIA:([^\s\)\]\`]+)",
+}
+
+
+def _read(rel: str) -> str:
+    return (REPO_ROOT / rel).read_text(encoding="utf-8")
+
+
+# ---------------------------------------------------------------------------
+# Source-shape: every site must carry the backtick-safe class
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.parametrize(
+    ("relpath", "needle"),
+    [
+        (relpath, needle)
+        for relpath, needles in JS_SITES.items()
+        for needle in needles
+    ],
+    ids=lambda v: v if isinstance(v, str) else "/".join(v),
+)
+def test_js_site_excludes_backtick(relpath: str, needle: str) -> None:
+    """Each JS capture site has `` ` `` inside its character class."""
+    text = _read(relpath)
+    assert needle in text, (
+        f"{relpath} is missing the backtick-safe capture literal: {needle!r}"
+    )
+
+
+@pytest.mark.parametrize(
+    ("relpath", "needle"), list(PY_SITES.items()), ids=lambda v: v
+)
+def test_py_site_excludes_backtick(relpath: str, needle: str) -> None:
+    """Each Python capture site has `` ` `` inside its character class."""
+    text = _read(relpath)
+    assert needle in text, (
+        f"{relpath} is missing the backtick-safe capture literal: {needle!r}"
+    )
+
+
+def test_all_seven_sites_covered() -> None:
+    """Guard against silently adding/removing capture sites."""
+    expected = 2 + 3 + 2  # ui.js 2 + messages.js 3 + py 2
+    actual = sum(len(v) for v in JS_SITES.values()) + len(PY_SITES)
+    assert actual == expected, f"site count drifted: {actual} != {expected}"
+
+
+# ---------------------------------------------------------------------------
+# Python behavioural: compile the actual regexes and exercise them
+# ---------------------------------------------------------------------------
+
+
+@pytest.fixture(scope="module")
+def _media_token_re() -> re.Pattern[str]:
+    """Compile the live ``_MEDIA_TOKEN_RE`` from api/routes.py."""
+    routes_text = _read("api/routes.py")
+    m = re.search(
+        r'_MEDIA_TOKEN_RE = re\.compile\(r"([^"]+)"\)', routes_text, re.M
+    )
+    assert m, "_MEDIA_TOKEN_RE compile() not found in api/routes.py"
+    return re.compile(m.group(1))
+
+
+@pytest.fixture(scope="module")
+def _media_snap_re() -> re.Pattern[str]:
+    """Compile the live ``media_re`` from api/media_snapshots.py."""
+    snap_text = _read("api/media_snapshots.py")
+    m = re.search(
+        r'media_re = _re\.compile\(r"([^"]+)"\)', snap_text, re.M
+    )
+    assert m, "media_re compile() not found in api/media_snapshots.py"
+    return re.compile(m.group(1))
+
+
+def test_backend_allowlist_strips_backtick(_media_token_re: re.Pattern[str]) -> None:
+    """``_MEDIA_TOKEN_RE`` must not include the closing backtick in the path."""
+    # Inline-code wrapper: backtick before MEDIA:, backtick after the path.
+    captured = _media_token_re.findall("get `MEDIA:/home/kim/skills.zip` now")
+    assert captured == ["/home/kim/skills.zip"], captured
+    assert all(not c.endswith("`") for c in captured)
+
+
+def test_backend_allowlist_bare_token_still_captures(
+    _media_token_re: re.Pattern[str],
+) -> None:
+    """Backward-compat: bare ``MEDIA:/path`` (no backtick) still matches."""
+    captured = _media_token_re.findall(
+        "MEDIA:/home/kim/a.zip and MEDIA:/home/kim/b.tar"
+    )
+    assert captured == ["/home/kim/a.zip", "/home/kim/b.tar"], captured
+
+
+def test_backend_allowlist_rejects_trailing_paren(
+    _media_token_re: re.Pattern[str],
+) -> None:
+    """Pre-existing behaviour: ``)`` is still excluded."""
+    # Trailing ')' is part of markdown link syntax and must NOT be captured.
+    captured = _media_token_re.findall("see [file](MEDIA:/home/kim/a.zip) here")
+    # Either nothing (paren excluded) or the path without ')'.
+    assert all(not c.endswith(")") for c in captured), captured
+
+
+def test_snapshot_capture_strips_backtick(_media_snap_re: re.Pattern[str]) -> None:
+    """``media_re`` must not include the closing backtick in the path."""
+    captured = _media_snap_re.findall(
+        "assistant: `MEDIA:/home/kim/shot.png` and MEDIA:/home/kim/a.zip"
+    )
+    assert captured == ["/home/kim/shot.png", "/home/kim/a.zip"], captured
+    assert all(not c.endswith("`") for c in captured)
+
+
+def test_snapshot_capture_bare_token_still_captures(
+    _media_snap_re: re.Pattern[str],
+) -> None:
+    """Backward-compat: bare ``MEDIA:/path`` still matches."""
+    captured = _media_snap_re.findall(
+        "MEDIA:/home/kim/a.zip and MEDIA:/home/kim/b.zip"
+    )
+    assert captured == ["/home/kim/a.zip", "/home/kim/b.zip"], captured
+
+
+# ---------------------------------------------------------------------------
+# Revert-sensitivity: the pre-fix class really would have captured backtick
+# ---------------------------------------------------------------------------
+
+
+def test_pre_fix_class_would_have_captured_backtick() -> None:
+    """The buggy (pre-fix) class greedily included the trailing backtick."""
+    buggy = re.compile(r"MEDIA:([^\s\)\]]+)")
+    assert buggy.findall("`MEDIA:/home/kim/a.zip`") == ["/home/kim/a.zip`"]
+
+
+def test_post_fix_class_drops_backtick() -> None:
+    """The fixed class strips the closing backtick, as the source does."""
+    fixed = re.compile(r"MEDIA:([^\s\)\]\`]+)")
+    assert fixed.findall("`MEDIA:/home/kim/a.zip`") == ["/home/kim/a.zip"]
+
+
+# ---------------------------------------------------------------------------
+# node --check (so a JS syntax error in the modified source fails the suite)
+# ---------------------------------------------------------------------------
+
+
+import subprocess  # noqa: E402  (kept near the helper that uses it)
+
+
+@pytest.mark.parametrize("relpath", sorted(JS_SITES.keys()))
+def test_js_file_parses(relpath: str) -> None:
+    """``node --check`` on every modified JS file."""
+    proc = subprocess.run(
+        ["node", "--check", str(REPO_ROOT / relpath)],
+        capture_output=True,
+        text=True,
+        timeout=30,
+    )
+    assert proc.returncode == 0, (
+        f"{relpath} failed `node --check`:\n{proc.stderr}"
+    )