fix(media): resolve MEDIA: paths containing spaces (one grammar, JS + Python)

api/routes.py

@@ -2848,6 +2848,9 @@ def _rebuild_stale_session_list_cache():
     redact_session_data,
     _redact_text,
     _CLIENT_DISCONNECT_ERRORS,
+    media_token_pattern,
+    unquote_media_ref,
+    media_token_exceeds_max_length,
 )
 from api.agent_health import build_agent_health_payload
 from api.gateway_chat import gateway_chat_config_status
@@ -18806,7 +18809,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(media_token_pattern())
 
 
 def _message_content_text(content) -> str:
@@ -18852,6 +18855,19 @@ def _session_media_token_allows_path(sid: str, target: Path, allowed_mimes: set[
         if "MEDIA:" not in text:
             continue
         for ref in _MEDIA_TOKEN_RE.findall(text):
+            # An over-ceiling capture is not a legal MEDIA token (shared lexical
+            # contract; see MEDIA_TOKEN_MAX_LENGTH in api/helpers.py). Neither
+            # renderer will turn it into a media node, so it must not mint an
+            # allow-list entry either — otherwise a single oversized token could
+            # authorize a path that nothing on the page can actually request.
+            if media_token_exceeds_max_length(ref):
+                continue
+            # A quoted ref (the unambiguous spelling for spaced paths) is
+            # captured WITH its quotes so the matched span covers the whole
+            # token; strip them before touching the filesystem, or the
+            # allow-list entry would carry a literal quote and never match the
+            # path the renderer actually requests.
+            ref = unquote_media_ref(ref)
             if "://" in ref:
                 continue
             try:

api/share_refs.py

@@ -0,0 +1,223 @@
+"""
+Hermes Web UI -- ONE fail-closed public-reference decision for share snapshots.
+
+``api/shares.py`` used to apply its privacy guard only while substituting
+canonical ``MEDIA:`` tokens, so every OTHER URL-bearing shape in message
+content reached the public share page unexamined. The share page loads
+``static/ui.js`` and ``static/share.js`` calls ``renderMd()``, and that
+renderer has several sinks that turn message text into a LIVE URL:
+
+* ``_inlineMediaHtmlForRef()`` -- restores a stashed ``MEDIA:`` token. A local
+  path becomes ``api/media?path=...``; a loopback http(s) ref is retargeted at
+  ``document.baseURI``.
+* ``_mdImageHtml()`` -- the outer AND inline ``![alt](url)`` passes. A
+  ``file://`` target is routed through ``_inlineMediaHtmlForRef()`` (so it too
+  becomes ``api/media?path=...``); an http(s) target becomes a direct
+  ``<img src>``.
+* ``_markdownHref()`` -- the outer AND inline ``[label](url)`` passes. A
+  ``file://`` target becomes ``api/media?path=...&inline=1``.
+* the autolink pass -- a bare ``http(s)://`` run in prose becomes ``<a href>``.
+* ``_tag()`` -- a raw ``<img src="api/media?...">`` survives sanitisation,
+  because ``_isSafeUrl()`` allows an ``api/`` relative src for images.
+
+Each of those makes an ANONYMOUS viewer's browser issue an authenticated
+same-origin request against our own ``/api/media`` route, or round-trips a host
+filesystem path into a public snapshot. So the decision cannot live inside the
+``MEDIA:`` substitution: it has to cover every token the client will turn into
+a URL, and it has to be taken BEFORE publication.
+
+Two properties this module exists to preserve:
+
+1. **One pass, whole tokens.** :data:`SHARE_REFERENCE_RE` is a single
+   alternation and every branch matches a COMPLETE parser-equivalent token.
+   Callers replace the whole matched span or preserve it byte-for-byte, never
+   part of it. ``re.sub`` then resumes AFTER the span, so the scanner can never
+   restart inside a token it already refused -- the bug that made an external
+   URL's tail rewritable when the old pattern rejected by non-match.
+2. **Fail closed.** An unparseable value, an empty host, a value still
+   percent-decoding at the bound, or a nested absolute/relative URL start in
+   the path, query, or fragment all mean "placeholder the token".
+"""
+
+from __future__ import annotations
+
+import re
+from urllib.parse import unquote, urlsplit
+
+from api.helpers import (
+    _LOCAL_TARGET_MARKERS,
+    external_media_url_hides_local_target,
+    media_token_pattern,
+)
+
+# Number of percent-decode rounds a probe gets before we stop and judge it.
+# A single decode misses `%254d` (`%4d` after one pass, `M` after two), and an
+# unbounded loop is a DoS on crafted input.
+_DECODE_ROUNDS = 3
+
+_MEDIA_KEYWORD = "MEDIA:"
+
+
+def decode_probe_bounded(value: str, *, rounds: int = _DECODE_ROUNDS) -> tuple[str, bool]:
+    """Percent-decode *value* up to *rounds* times; report whether it settled.
+
+    Returns ``(decoded, still_changing)``. ``still_changing`` is ``True`` when
+    the value was STILL mutating when the bound was reached.
+
+    ``api.helpers._decode_url_component_bounded`` returns only the decoded
+    string, so a value that never settles is indistinguishable there from one
+    that decoded cleanly to something harmless -- and the caller then accepts
+    it. At a publication boundary that is a hole, so this variant hands the
+    verdict back to the caller instead of hiding it.
+    """
+    current = str(value or "")
+    for _ in range(max(0, rounds)):
+        nxt = unquote(current)
+        if nxt == current:
+            return current, False
+        current = nxt
+    return current, unquote(current) != current
+
+
+# Nested URL starts that mean "this value leads back to a local or
+# authenticated target". A superset of the marker tuple in api/helpers.py,
+# imported rather than restated so the two can never drift:
+#
+# * ``http://`` / ``https://`` -- a SECOND absolute URL inside the path, query,
+#   or fragment of the first. The token we are judging already carries its own
+#   scheme, so another one is smuggled, not structural.
+# * ``api/media`` -- the slash-less spelling of our own route, which is exactly
+#   the form ``_isSafeUrl()`` accepts as an image src.
+#
+# The netloc is deliberately NOT probed, so an ordinary public CDN host is never
+# refused for its name alone.
+_NESTED_START_MARKERS: tuple[str, ...] = tuple(
+    dict.fromkeys(_LOCAL_TARGET_MARKERS + ("http://", "https://", "api/media"))
+)
+
+_FILE_SCHEME_RE = re.compile(r"(?i)^file://")
+_HTTP_SCHEME_RE = re.compile(r"(?i)^https?://")
+# Our own media route as a RELATIVE URL. A `?` is required so the words
+# "api/media" in ordinary prose are not mistaken for a URL; the dangerous shape
+# always carries a query (`api/media?path=...`).
+_API_MEDIA_RE = re.compile(r"(?i)^/?api/media\?")
+
+
+def public_reference_hides_local_target(value: str) -> bool:
+    """True when *value* must NOT be published into a public share snapshot.
+
+    Covers every shape :data:`SHARE_REFERENCE_RE` can match, and answers for
+    the WHOLE value across path, query, and fragment.
+
+    Refused:
+
+    * ``file://`` -- absolute and un-scoped, so it can name anything on the
+      host. Never embeddable either (``_resolve_against_roots`` rejects it), so
+      preserving it would only disclose a path.
+    * a relative ``api/media?…`` / ``/api/media?…`` -- our authenticated route.
+    * an http(s) URL that :func:`api.helpers.external_media_url_hides_local_target`
+      already refuses (private host, empty host, unparseable, or a marker in
+      the decoded path/query/fragment).
+    * an http(s) URL whose probe is still percent-decoding at the bound.
+    * an http(s) URL carrying a nested absolute or relative URL start.
+    * anything else -- a shape we cannot reason about is refused, not accepted.
+
+    Preserved: an ordinary public http(s) URL, including one with a harmless
+    query string. Over-blocking a genuine public asset is also a failure.
+    """
+    v = str(value or "").strip()
+    if not v:
+        return True
+    if _FILE_SCHEME_RE.match(v) or _API_MEDIA_RE.match(v):
+        return True
+    if not _HTTP_SCHEME_RE.match(v):
+        # Not a shape this module classifies -- fail closed.
+        return True
+    # The host/marker half of the decision, reused verbatim so the server guard
+    # and its client twin in static/ui.js keep agreeing on those rows.
+    if external_media_url_hides_local_target(v):
+        return True
+    try:
+        parts = urlsplit(v)
+    except ValueError:
+        return True
+    probe = "".join((
+        parts.path or "",
+        "?" + parts.query if parts.query else "",
+        "#" + parts.fragment if parts.fragment else "",
+    ))
+    decoded, still_changing = decode_probe_bounded(probe)
+    if still_changing:
+        return True
+    lowered = decoded.lower()
+    return any(marker in lowered for marker in _NESTED_START_MARKERS)
+
+
+# ── The tokenizer ────────────────────────────────────────────────────────────
+# One alternation over every shape the share renderer turns into a live URL.
+# Each branch matches a COMPLETE token, so a caller's replace-or-preserve
+# decision always covers the whole span.
+#
+# Character classes mirror the client regexes so the server and the renderer
+# tokenize the same text the same way:
+#   _URL_RUN      -> the autolink class in renderMd()
+#   _MD_IMAGE_URL -> the `[^)]+` target class of the ![alt](url) passes
+#   _MD_LINK_URL  -> the `[^\s)]+` target class of the [label](url) passes
+# `\n` is additionally excluded from the Markdown target classes: a newline
+# never appears inside a real target and excluding it keeps one refusal from
+# swallowing the following line.
+_URL_RUN = r"[^\s<>\"')\]]+"
+_MD_IMAGE_URL = r"[^)\n]+"
+_MD_LINK_URL = r"[^\s)\n]+"
+# Only the schemes whose Markdown target a client sink turns into a local or
+# authenticated URL. `data:image/` is validated separately by
+# _isSafeDataImageUri() and carries no host path; `workspace://`, `session://`,
+# `mailto:`, `tel:` and `message:` produce no fetchable local file URL.
+_MD_SCHEMES = r"(?i:https?://|file://)"
+# Same, plus the relative spelling of our own media route, so a Markdown
+# image/link whose target is `/api/media?…` is refused as ONE whole token
+# instead of leaving `![x](` and `)` behind as orphaned prose.
+_MD_TARGET_STARTS = r"(?i:https?://|file://|/?api/media\?)"
+
+SHARE_REFERENCE_PATTERN = (
+    # 1. Canonical MEDIA: token. First so `MEDIA:https://…/MEDIA:/etc/x.png`
+    #    stays ONE token instead of being re-entered at the nested keyword.
+    r"(?P<media>" + media_token_pattern(extra_exclude=">") + r")"
+    # 2. Markdown image, outer and inline passes.
+    r"|(?P<mdimg>!\[[^\]]*\]\((?P<imgurl>" + _MD_TARGET_STARTS + _MD_IMAGE_URL + r")\))"
+    # 3. Markdown link, outer and inline passes.
+    r"|(?P<mdlink>\[[^\]]+\]\((?P<linkurl>" + _MD_TARGET_STARTS + _MD_LINK_URL + r")\))"
+    # 4. Bare absolute URL in prose (autolink sink, and path disclosure for
+    #    file://, which no sink linkifies but which still names a host file).
+    r"|(?P<bare>" + _MD_SCHEMES + _URL_RUN + r")"
+    # 5. Relative reference to our own authenticated media route. Reachable as
+    #    a live image src through _isSafeUrl()/_tag().
+    r"|(?P<apimedia>/?(?i:api/media)\?" + _URL_RUN + r")"
+)
+
+SHARE_REFERENCE_RE = re.compile(SHARE_REFERENCE_PATTERN)
+
+
+def media_ref_of_match(match: re.Match) -> str:
+    """Return the raw (still quoted) ref of a ``media`` branch match.
+
+    The ``media`` branch wraps :func:`api.helpers.media_token_pattern`, whose
+    own capture group is unnamed and whose index therefore depends on that
+    helper's internals. Slice the matched TEXT instead: it always begins with
+    the ``MEDIA:`` keyword and everything after it is the capture.
+    """
+    span = match.group("media") or ""
+    return span[len(_MEDIA_KEYWORD):] if span.startswith(_MEDIA_KEYWORD) else span
+
+
+def url_of_match(match: re.Match) -> str:
+    """Return the URL a non-``media`` branch match carries.
+
+    For the Markdown branches that is the target inside the parentheses; for
+    the bare and relative branches the whole span IS the URL.
+    """
+    for name in ("imgurl", "linkurl"):
+        value = match.group(name)
+        if value is not None:
+            return value
+    return match.group(0)

api/shares.py

@@ -24,7 +24,18 @@
 from pathlib import Path
 
 from api.config import STATE_DIR
-from api.helpers import redact_session_data
+from api.helpers import (
+    redact_session_data,
+    unquote_media_ref,
+    is_external_media_url,
+    media_token_exceeds_max_length,
+)
+from api.share_refs import (
+    SHARE_REFERENCE_RE,
+    media_ref_of_match,
+    public_reference_hides_local_target,
+    url_of_match,
+)
 # _redact_fn_cached is the ALWAYS-ON credential redactor (agent redactor with
 # force=True + local fallback regex). Unlike redact_session_data it does NOT
 # consult the user-toggleable api_redact_enabled setting — a public share is a
@@ -115,14 +126,23 @@ def _redact_share_paths(text: str, extra_paths) -> str:
     return text
 
 
-# Regex matching local MEDIA:<path> references — same pattern that
-# _inlineMediaHtmlForRef() in ui.js handles when rendering messages.
-# Excludes MEDIA: followed by http/https URLs so external images pass
-# through unchanged.  file:// references are NOT matched here — they are
-# always rejected at the public-share boundary (absolute, un-scoped).
-_SHARE_MEDIA_RE = re.compile(
-    r"MEDIA:(?!https?://)([^\s\)\]>]+)"
-)
+# Regex matching EVERY URL-bearing token the share renderer can turn into a
+# live URL — the canonical `MEDIA:` token plus Markdown images/links, bare
+# absolute URLs, and relative `api/media?…` references. Shape and rationale
+# live in api/share_refs.py.
+#
+# NOTE: the `MEDIA:` branch deliberately does NOT pass exclude_urls=True. That
+# option is a negative lookahead at the current start position, so an external
+# URL is rejected by NOT MATCHING — and `re.sub` then resumes scanning one
+# character later, inside the very token it just refused. For
+# `MEDIA:https://cdn.test/img/MEDIA:/etc/passwd.png` the outer token is skipped
+# and the nested `MEDIA:` matches as a fresh LOCAL token, so a legitimate
+# external URL gets its tail rewritten (and local paths get probed from share
+# text). Match the complete canonical token here and classify it in
+# _replace_ref(): match -> classify -> decide, which cannot restart mid-token.
+# Every other branch obeys the same rule, so one refusal always consumes its
+# whole token.
+_SHARE_MEDIA_RE = SHARE_REFERENCE_RE
 
 # Max size (in bytes) for files we'll embed as base64 in a share snapshot.
 _SHARE_EMBED_MAX_BYTES = 512 * 1024  # 512 KiB
@@ -290,9 +310,52 @@ def _resolve_against_roots(raw: str) -> Path | None:
         return None
 
     def _replace_ref(m: re.Match) -> str:
-        raw = (m.group(1) or "").strip()
+        # Non-`MEDIA:` branches: a Markdown image/link target, a bare absolute
+        # URL, or a relative reference to our own media route. Each of these is
+        # a live-URL sink on the share page (see api/share_refs.py for the sink
+        # inventory), so ONE fail-closed decision covers the WHOLE matched span
+        # here, exactly as the `MEDIA:` branch below does. Never embedded: the
+        # allowed-roots embed path exists for canonical `MEDIA:` tokens, and a
+        # Markdown target that survives is preserved byte-for-byte instead.
+        if m.group("media") is None:
+            if public_reference_hides_local_target(url_of_match(m)):
+                return _PLACEHOLDER
+            return m.group(0)
+
+        # Quoted refs are captured with their quotes so the replaced span covers
+        # the whole token; strip them before any path resolution, or a spaced
+        # path that the renderer displays would be placeholdered here.
+        capture = media_ref_of_match(m)
+        raw = unquote_media_ref(capture)
         if not raw:
             return m.group(0)
+        # An over-ceiling capture is not a legal MEDIA token (shared lexical
+        # contract; see MEDIA_TOKEN_MAX_LENGTH in api/helpers.py). Neither
+        # renderer turns it into a media node, so the snapshot must leave the
+        # span exactly as prose rather than embed or placeholder it — otherwise
+        # the share and the live view disagree about one token.
+        if media_token_exceeds_max_length(capture):
+            return m.group(0)
+        # Classification happens HERE, on a token that was actually matched, and
+        # is the only thing that decides external vs local. The pattern no longer
+        # rejects external URLs by non-match, so the scanner can never resume
+        # inside a refused token and rewrite the tail of an external URL that
+        # itself contains `MEDIA:`. Return the exact original span so an exempt
+        # external token is preserved byte-for-byte.
+        #
+        # But "carries an http(s) scheme" is NOT the same as "safe to publish".
+        # The share renderer restores a preserved token into an image URL, so a
+        # URL whose own path/query/fragment names a local file, our
+        # authenticated /api/media route, or a loopback/private host would
+        # either round-trip a host path into the anonymous snapshot or make the
+        # viewer's browser issue a same-origin authenticated request. Decide the
+        # WHOLE token: preserve it byte-for-byte, or placeholder all of it. A
+        # partial rewrite is exactly the bug that let the scanner resume inside
+        # a token it had refused.
+        if is_external_media_url(raw):
+            if public_reference_hides_local_target(raw):
+                return _PLACEHOLDER
+            return m.group(0)
 
         # --- Resolve and validate against allowed roots -----------------------
         p = _resolve_against_roots(raw)