From a12fa53fbc96cce1c8b7f8d9d880cef51cb4128e Mon Sep 17 00:00:00 2001 From: GodsBoy Date: Wed, 27 May 2026 15:03:22 +0200 Subject: [PATCH 1/2] fix(gateway): diagnosable MEDIA delivery + canonical cache roots (#31733) Builds on the direction in #31764 with a broader cut at the same bug surface that surfaces in operator reports as "MEDIA tag did not attach": * MEDIA_DELIVERY_SAFE_ROOTS now lists the canonical ~/.hermes/cache/{images,audio,videos,documents,screenshots} explicitly alongside the legacy ~/.hermes/{type}_cache/ entries. get_hermes_dir returns the legacy path when it exists, so IMAGE_CACHE_DIR alone drops the canonical layout on co-existing installs (root cause of #31733). * validate_media_delivery_path now delegates to a private _validate_media_delivery_path_with_reason helper that returns (resolved_or_None, kebab_reason). filter_media_delivery_paths and filter_local_delivery_paths log the reason plus a redacted path (_redact_path_for_log), so operators chasing rejected MEDIA tags can tell allowlist-miss from stale-mtime from denied-prefix from does-not-resolve without rebuilding the bot with debug logging. * _redact_path_for_log neutralises control characters (newline, NUL, carriage return) so a model-emitted path containing an embedded newline cannot forge a second log line. * Hoisted _FINAL_RESPONSE_IMAGE_EXTS / _FINAL_RESPONSE_VIDEO_EXTS to module scope so the regression test imports the same partition the final-response dispatch actually uses, instead of snapshotting a local copy that could drift. * Regression coverage added in tests/gateway/test_platform_base.py: - default safe-roots tuple contains both legacy and canonical layouts - legacy and canonical image caches co-exist (the strengthened failure-shape test #31764 review asked for) - PDF deliverable from canonical ~/.hermes/cache/documents and legacy ~/.hermes/document_cache - filter_*_delivery_paths logs the redacted path and the kebab reason tag (using the imported _MEDIA_REASON_* constants, not raw strings) for stale-mtime, no-recency, does-not-resolve, denied-prefix, and newline-injection cases - validate_media_delivery_path delegation invariant: public API matches the resolved path returned by the reason-bearing helper - send_message tool and final-response paths share extract_media + filter_media_delivery_paths (asserted via source introspection so a future split surfaces here) - PDF MEDIA tag routes to send_document via the production extension partition (no image / video misroute) Scope: gateway media delivery only. Does not duplicate #32472's extension-derivation refactor or #31561 / #33206's markdown attachment recognition work; if any of those land first, the diagnosable-logging contract here still applies and the canonical-root entries become a clean merge against #31764. Deferred follow-up (out of scope for this PR): - Surfacing the rejection reason to the calling agent (send_message tool currently returns {success: true} regardless of dropped attachments) - Unifying the three dispatch-site extension sets across _process_message_background, _deliver_media_from_response, and the send_message tool path - Pre-existing residual: symlink-swap of a cache root before image_generate writes; TOCTOU between resolve() and upload; /var/tmp / /var/cache not in denylist --- gateway/platforms/base.py | 207 +++++++++++---- tests/gateway/test_platform_base.py | 385 ++++++++++++++++++++++++++++ 2 files changed, 548 insertions(+), 44 deletions(-) diff --git a/gateway/platforms/base.py b/gateway/platforms/base.py index 766f3541aa59..cb67598e0ccd 100644 --- a/gateway/platforms/base.py +++ b/gateway/platforms/base.py @@ -476,7 +476,7 @@ def is_host_excluded_by_no_proxy(hostname: str, no_proxy_value: str | None = Non from dataclasses import dataclass, field from datetime import datetime from pathlib import Path -from typing import Dict, List, Optional, Any, Callable, Awaitable, Tuple, Union +from typing import Any, Awaitable, Callable, Dict, Iterable, List, Optional, Tuple, Union from enum import Enum from pathlib import Path as _Path @@ -838,18 +838,44 @@ def cache_video_from_bytes(data: bytes, ext: str = ".mp4") -> str: # user should set this to true. MEDIA_DELIVERY_STRICT_ENV = "HERMES_MEDIA_DELIVERY_STRICT" MEDIA_DELIVERY_SAFE_ROOTS = ( + # ``*_CACHE_DIR`` resolves to legacy-or-canonical via ``get_hermes_dir`` + # (returns the legacy path when it exists on disk, otherwise the canonical + # one). The explicit legacy + canonical entries below ensure BOTH layouts + # are always deliverable regardless of which one ``get_hermes_dir`` picks + # — without them, generated artifacts under ``~/.hermes/cache/images/`` + # fail MEDIA delivery on hosts that still have the legacy + # ``~/.hermes/image_cache/`` directory from older installs (#31733). IMAGE_CACHE_DIR, AUDIO_CACHE_DIR, VIDEO_CACHE_DIR, DOCUMENT_CACHE_DIR, SCREENSHOT_CACHE_DIR, + # Legacy paths (preserved for backward compat with existing installs). _HERMES_HOME / "image_cache", _HERMES_HOME / "audio_cache", _HERMES_HOME / "video_cache", _HERMES_HOME / "document_cache", _HERMES_HOME / "browser_screenshots", + # Canonical new paths — where ``image_generate`` / document tools write by + # default when the legacy dirs are absent. Listed explicitly so co-existing + # legacy installs do not silently drop these files (#31733). + _HERMES_HOME / "cache" / "images", + _HERMES_HOME / "cache" / "audio", + _HERMES_HOME / "cache" / "videos", + _HERMES_HOME / "cache" / "documents", + _HERMES_HOME / "cache" / "screenshots", ) + +# Final-response dispatch extension partitions, hoisted to module scope so the +# regression test in ``tests/gateway/test_platform_base.py`` can import them +# instead of snapshotting the values locally (which would silently drift if +# the production sets gain a new extension). ``send_message`` tool and the +# scheduler keep their own local copies for now; unifying all dispatch sites +# is tracked as follow-up. +_FINAL_RESPONSE_IMAGE_EXTS = frozenset({".jpg", ".jpeg", ".png", ".webp", ".gif"}) +_FINAL_RESPONSE_VIDEO_EXTS = frozenset({".mp4", ".mov", ".avi", ".mkv", ".webm", ".3gp"}) + # Default recency window for trusting freshly-produced files (seconds). # The agent's actual work generally completes well inside 10 minutes; legitimate # build artifacts (PDFs from pandoc, plots from matplotlib, etc.) almost always @@ -992,47 +1018,88 @@ def _path_is_within(path: Path, root: Path) -> bool: return False -def validate_media_delivery_path(path: str) -> Optional[str]: - """Return a safe absolute file path for native media delivery, else None. +_REDACT_PATH_CONTROL_CHARS = re.compile(r"[\x00-\x1f\x7f]") - Default mode (single-user / private gateway): accept any existing regular - file that isn't under the credential / system-path denylist - (``_MEDIA_DELIVERY_DENIED_PREFIXES`` + ``~/.ssh``, ``~/.aws``, etc.). - This matches the symmetry of inbound delivery — Telegram/Discord/Slack - will hand the agent any file the user uploads, and the agent can hand - back any file that isn't a credential. - Strict mode (opt-in via ``gateway.strict`` in ``config.yaml`` or - ``HERMES_MEDIA_DELIVERY_STRICT=1``): the file MUST live under a - Hermes-managed cache, under an operator-allowlisted root - (``HERMES_MEDIA_ALLOW_DIRS``), or be freshly produced inside the - configured recency window. Suitable for public-facing bots where - prompt injection from one user shouldn't be able to exfiltrate the - host's secrets to that same user. +def _redact_path_for_log(path: str) -> str: + """Return a path representation safe for shared logs. + + Shows the topmost directory and the filename, eliding intermediate + components so operators have enough context to diagnose a "MEDIA + didn't attach" report without the full filesystem path leaking into + log storage. Example:: + + /tmp/pool_evidence/Quote_PFA30971.pdf -> /tmp/.../Quote_PFA30971.pdf + ~/.hermes/cache/images/foo.png -> ~/.hermes/.../foo.png - Symlinks are resolved before any containment / denylist check. + Short absolute paths (3 or fewer ``parts`` -- e.g. ``/etc/passwd``, + ``/tmp/leaked.pdf``) have no intermediate components to elide and are + returned with only the control-character pass applied. Control + characters (newline, carriage return, NUL, etc.) are replaced with + ``?`` regardless of length, so a model-emitted path containing + embedded newlines cannot forge a fake second log line. """ if not path: - return None + return "" + try: + raw_parts = Path(path).parts + except (TypeError, ValueError): + return "" + if len(raw_parts) <= 3: + rendered = path + else: + rendered = str(Path(raw_parts[0], raw_parts[1], "...", raw_parts[-1])) + return _REDACT_PATH_CONTROL_CHARS.sub("?", rendered) + + +# Reason tags returned by ``_validate_media_delivery_path_with_reason``. +# Kebab-string form is intentional: keeps grep / log-aggregator queries +# unambiguous and avoids enum import cycles in callers. +_MEDIA_REASON_EMPTY = "empty" +_MEDIA_REASON_NOT_ABSOLUTE = "not-absolute" +_MEDIA_REASON_DOES_NOT_RESOLVE = "does-not-resolve" +_MEDIA_REASON_NOT_A_FILE = "not-a-file" +_MEDIA_REASON_OUTSIDE_NO_RECENCY = "outside-allowlist-no-recency" +_MEDIA_REASON_OUTSIDE_DENIED_PREFIX = "outside-allowlist-denied-prefix" +_MEDIA_REASON_OUTSIDE_STALE_MTIME = "outside-allowlist-stale-mtime" +_MEDIA_REASON_ALLOWLISTED = "allowlisted" +_MEDIA_REASON_RECENCY_TRUSTED = "recency-trusted" +# Non-strict (default) accept: file resolved, is a regular file, and is not +# under the credential / system-path denylist. +_MEDIA_REASON_DENYLIST_CLEARED = "denylist-cleared" + + +def _validate_media_delivery_path_with_reason(path: str) -> Tuple[Optional[str], str]: + """Validate a candidate media path, returning ``(resolved, reason)``. + + The reason is a short kebab-string suitable for log fields. Callers that + just need the resolved-or-``None`` result should use + :func:`validate_media_delivery_path`. Callers that need diagnosability + (e.g. ``filter_media_delivery_paths``) use this directly so they can + log *why* a path was rejected — silent failures here have historically + been very hard to diagnose from production logs (#31733). + """ + if not path: + return None, _MEDIA_REASON_EMPTY candidate = str(path).strip() if len(candidate) >= 2 and candidate[0] == candidate[-1] and candidate[0] in "`\"'": candidate = candidate[1:-1].strip() candidate = candidate.lstrip("`\"'").rstrip("`\"',.;:)}]") if not candidate: - return None + return None, _MEDIA_REASON_EMPTY expanded = Path(os.path.expanduser(candidate)) if not expanded.is_absolute(): - return None + return None, _MEDIA_REASON_NOT_ABSOLUTE try: resolved = expanded.resolve(strict=True) except (OSError, RuntimeError, ValueError): - return None + return None, _MEDIA_REASON_DOES_NOT_RESOLVE if not resolved.is_file(): - return None + return None, _MEDIA_REASON_NOT_A_FILE # Cache / operator allowlist is always honored — these are unconditionally # trusted regardless of mode. @@ -1042,7 +1109,7 @@ def validate_media_delivery_path(path: str) -> Optional[str]: except (OSError, RuntimeError, ValueError): continue if _path_is_within(resolved, resolved_root): - return str(resolved) + return str(resolved), _MEDIA_REASON_ALLOWLISTED # Non-strict mode (default): accept anything not on the denylist. # The denylist still blocks /etc, /proc, ~/.ssh, ~/.aws, ~/.hermes/.env, @@ -1050,8 +1117,8 @@ def validate_media_delivery_path(path: str) -> Optional[str]: # (``MEDIA:/etc/passwd``, ``MEDIA:~/.ssh/id_rsa``) remain rejected. if not _media_delivery_strict_mode(): if _path_under_denied_prefix(resolved): - return None - return str(resolved) + return None, _MEDIA_REASON_OUTSIDE_DENIED_PREFIX + return str(resolved), _MEDIA_REASON_DENYLIST_CLEARED # Strict mode: fall back to recency-based trust for freshly-produced # files (e.g. ``pandoc -o /tmp/report.pdf`` or @@ -1059,11 +1126,39 @@ def validate_media_delivery_path(path: str) -> Optional[str]: # credential locations remain blocked even when "recent" — see # ``_MEDIA_DELIVERY_DENIED_PREFIXES`` for the denylist. window = _media_delivery_recency_seconds() - if window > 0 and not _path_under_denied_prefix(resolved): - if _file_is_recently_produced(resolved, window): - return str(resolved) + if window <= 0: + return None, _MEDIA_REASON_OUTSIDE_NO_RECENCY + if _path_under_denied_prefix(resolved): + return None, _MEDIA_REASON_OUTSIDE_DENIED_PREFIX + if _file_is_recently_produced(resolved, window): + return str(resolved), _MEDIA_REASON_RECENCY_TRUSTED + return None, _MEDIA_REASON_OUTSIDE_STALE_MTIME - return None + +def validate_media_delivery_path(path: str) -> Optional[str]: + """Return a safe absolute file path for native media delivery, else None. + + Default mode (single-user / private gateway): accept any existing regular + file that is not under the credential / system-path denylist + (``_MEDIA_DELIVERY_DENIED_PREFIXES`` plus ``~/.ssh``, ``~/.aws``, etc.). + This matches the symmetry of inbound delivery: the chat platform hands the + agent any file the user uploads, and the agent can hand back any file that + is not a credential. + + Strict mode (opt-in via ``gateway.strict`` in ``config.yaml`` or + ``HERMES_MEDIA_DELIVERY_STRICT=1``): the file MUST live under a + Hermes-managed cache, under an operator-allowlisted root + (``HERMES_MEDIA_ALLOW_DIRS``), or be freshly produced inside the configured + recency window. Suitable for public-facing bots where prompt injection from + one user should not be able to exfiltrate the host's secrets to that user. + + Symlinks are resolved before any containment / denylist check. Diagnosable + rejection reasons are surfaced through the internal + :func:`_validate_media_delivery_path_with_reason` helper, which the filter + functions use for log output. + """ + resolved, _reason = _validate_media_delivery_path_with_reason(path) + return resolved SUPPORTED_DOCUMENT_TYPES = { @@ -2434,27 +2529,48 @@ def validate_media_delivery_path(path: str) -> Optional[str]: return validate_media_delivery_path(path) @staticmethod - def filter_media_delivery_paths(media_files) -> List[Tuple[str, bool]]: - """Drop unsafe MEDIA paths and normalize accepted paths.""" + def filter_media_delivery_paths( + media_files: Optional[Iterable[Tuple[Any, Any]]], + ) -> List[Tuple[str, bool]]: + """Drop unsafe MEDIA paths and normalize accepted paths. + + Each rejection logs the redacted path plus a kebab-string reason + (e.g. ``outside-allowlist-stale-mtime``) so "MEDIA didn't attach" + operator reports are diagnosable without source-diving (#31733). + """ safe_media: List[Tuple[str, bool]] = [] for media_path, is_voice in media_files or []: - safe_path = validate_media_delivery_path(str(media_path)) - if safe_path: - safe_media.append((safe_path, bool(is_voice))) + raw = str(media_path) + resolved, reason = _validate_media_delivery_path_with_reason(raw) + if resolved: + safe_media.append((resolved, bool(is_voice))) else: - logger.warning("Skipping unsafe MEDIA directive path outside allowed roots") + logger.warning( + "Skipping unsafe MEDIA directive path %s, reason=%s", + _redact_path_for_log(raw), + reason, + ) return safe_media @staticmethod - def filter_local_delivery_paths(file_paths) -> List[str]: - """Drop unsafe bare local file paths and normalize accepted paths.""" + def filter_local_delivery_paths(file_paths: Optional[Iterable[Any]]) -> List[str]: + """Drop unsafe bare local file paths and normalize accepted paths. + + Same diagnosable-rejection contract as + :py:meth:`filter_media_delivery_paths`. + """ safe_paths: List[str] = [] for file_path in file_paths or []: - safe_path = validate_media_delivery_path(str(file_path)) - if safe_path: - safe_paths.append(safe_path) + raw = str(file_path) + resolved, reason = _validate_media_delivery_path_with_reason(raw) + if resolved: + safe_paths.append(resolved) else: - logger.warning("Skipping unsafe local file path outside allowed roots") + logger.warning( + "Skipping unsafe local file path %s, reason=%s", + _redact_path_for_log(raw), + reason, + ) return safe_paths @staticmethod @@ -3794,9 +3910,12 @@ async def _stop_typing_task() -> None: logger.warning("[%s] Error batching images: %s", self.name, batch_err, exc_info=True) - # Send extracted media files — route by file type - _VIDEO_EXTS = {'.mp4', '.mov', '.avi', '.mkv', '.webm', '.3gp'} - _IMAGE_EXTS = {'.jpg', '.jpeg', '.png', '.webp', '.gif'} + # Send extracted media files — route by file type. Use the + # module-scope partitions so the regression tests in + # ``tests/gateway/test_platform_base.py`` stay in sync with + # what production actually dispatches. + _VIDEO_EXTS = _FINAL_RESPONSE_VIDEO_EXTS + _IMAGE_EXTS = _FINAL_RESPONSE_IMAGE_EXTS # Partition images out of media_files + local_files so they # can be sent as a single batch (Signal RPC). When diff --git a/tests/gateway/test_platform_base.py b/tests/gateway/test_platform_base.py index 34d94c06f587..f06815c864b0 100644 --- a/tests/gateway/test_platform_base.py +++ b/tests/gateway/test_platform_base.py @@ -2,6 +2,7 @@ import os import time +from pathlib import Path from unittest.mock import patch import pytest @@ -12,7 +13,14 @@ MessageEvent, safe_url_for_log, utf16_len, + _FINAL_RESPONSE_IMAGE_EXTS, + _FINAL_RESPONSE_VIDEO_EXTS, + _MEDIA_REASON_OUTSIDE_DENIED_PREFIX, + _MEDIA_REASON_OUTSIDE_NO_RECENCY, + _MEDIA_REASON_OUTSIDE_STALE_MTIME, + _MEDIA_REASON_DOES_NOT_RESOLVE, _prefix_within_utf16_limit, + _validate_media_delivery_path_with_reason, ) @@ -683,6 +691,383 @@ def test_filter_passes_default_files_through(self, tmp_path, monkeypatch): assert out == [str(notes.resolve())] +# --------------------------------------------------------------------------- +# MEDIA delivery — broader-than-images regression coverage (#31733 follow-up) +# +# These tests complement #31764 by covering: +# - The actual ``MEDIA_DELIVERY_SAFE_ROOTS`` constant (no monkeypatch), +# so canonical+legacy regressions surface against the real config. +# - The legacy-and-canonical-co-exist failure shape the original bug +# report calls out (legacy dir exists on disk; ``get_hermes_dir`` picks +# legacy via *_CACHE_DIR; canonical files would otherwise be dropped). +# - PDF / document attachments (the existing recency-trust tests cover +# bare ``/tmp/report.pdf`` flow; these add explicit cache-root coverage). +# - Diagnosable rejection reasons in logs — the previous generic +# "Skipping unsafe MEDIA directive path outside allowed roots" warning +# gave operators no way to tell allowlist-miss apart from stale-mtime +# apart from denied-prefix. +# - Final-response vs ``send_message`` extraction parity, since both +# paths share ``extract_media`` + ``filter_media_delivery_paths`` by +# design — a regression here silently splits behaviour. +# --------------------------------------------------------------------------- + + +class TestSafeRootsCoverage: + """Assertions against the actual ``MEDIA_DELIVERY_SAFE_ROOTS`` constant.""" + + def test_default_safe_roots_include_legacy_and_canonical_subdirs(self): + """Both legacy ``image_cache`` and canonical ``cache/images`` are listed. + + Without explicit entries for both, the ``get_hermes_dir`` resolution + (returns the legacy path when it exists on disk) silently drops the + canonical one, which is exactly the failure shape reported in + #31733 when ``image_generate`` writes to ``cache/images`` on a host + that still has ``image_cache``. + """ + from gateway.platforms.base import MEDIA_DELIVERY_SAFE_ROOTS + + # Compare on the last 2 parts (legacy) or last 3 parts (canonical) + # of each Path. Direct tuple comparison reads more clearly than + # string suffix matching and is host-path agnostic. + legacy_tails = {tuple(p.parts[-2:]) for p in MEDIA_DELIVERY_SAFE_ROOTS if len(p.parts) >= 2} + canonical_tails = {tuple(p.parts[-3:]) for p in MEDIA_DELIVERY_SAFE_ROOTS if len(p.parts) >= 3} + + expected_legacy = { + (".hermes", "image_cache"), + (".hermes", "audio_cache"), + (".hermes", "video_cache"), + (".hermes", "document_cache"), + (".hermes", "browser_screenshots"), + } + expected_canonical = { + (".hermes", "cache", "images"), + (".hermes", "cache", "audio"), + (".hermes", "cache", "videos"), + (".hermes", "cache", "documents"), + (".hermes", "cache", "screenshots"), + } + missing_legacy = expected_legacy - legacy_tails + missing_canonical = expected_canonical - canonical_tails + assert not missing_legacy, f"Missing legacy roots: {sorted(missing_legacy)}" + assert not missing_canonical, f"Missing canonical roots: {sorted(missing_canonical)}" + + def test_legacy_and_canonical_image_caches_coexist(self, tmp_path, monkeypatch): + """Files under canonical ``cache/images`` are deliverable when the legacy + ``image_cache`` dir also exists on disk. + + This is the exact failure shape from #31733: with the legacy dir + present, ``get_hermes_dir`` resolves ``IMAGE_CACHE_DIR`` to the legacy + path, and the canonical path used to fall out of the safe roots tuple. + The fix is to always list the canonical paths explicitly. + """ + fake_home = tmp_path / "fake_home" + legacy_dir = fake_home / ".hermes" / "image_cache" + canonical_dir = fake_home / ".hermes" / "cache" / "images" + legacy_dir.mkdir(parents=True) + canonical_dir.mkdir(parents=True) + + legacy_file = legacy_dir / "legacy.png" + canonical_file = canonical_dir / "generated.png" + legacy_file.write_bytes(b"\x89PNG\r\n\x1a\n") + canonical_file.write_bytes(b"\x89PNG\r\n\x1a\n") + + # Point MEDIA_DELIVERY_SAFE_ROOTS at both, exactly the way main does + # for ``_HERMES_HOME / "image_cache"`` and ``_HERMES_HOME / "cache" / "images"``. + monkeypatch.setattr( + "gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", + (legacy_dir, canonical_dir), + ) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + assert BasePlatformAdapter.validate_media_delivery_path(str(canonical_file)) == str(canonical_file.resolve()) + assert BasePlatformAdapter.validate_media_delivery_path(str(legacy_file)) == str(legacy_file.resolve()) + + def test_pdf_under_canonical_documents_dir_is_accepted(self, tmp_path, monkeypatch): + """A PDF in ``~/.hermes/cache/documents`` is deliverable. + + Document delivery has been less covered than image delivery; this + guards against future "canonical roots for images but not documents" + regressions. + """ + canonical_docs = tmp_path / ".hermes" / "cache" / "documents" + canonical_docs.mkdir(parents=True) + pdf = canonical_docs / "report.pdf" + pdf.write_bytes(b"%PDF-1.4\n%mock") + + monkeypatch.setattr( + "gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", + (canonical_docs,), + ) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + assert BasePlatformAdapter.validate_media_delivery_path(str(pdf)) == str(pdf.resolve()) + + def test_pdf_under_legacy_document_cache_is_accepted(self, tmp_path, monkeypatch): + """A PDF in the legacy ``~/.hermes/document_cache`` is deliverable. + + Mirrors the canonical-documents test for hosts running older installs. + """ + legacy_docs = tmp_path / ".hermes" / "document_cache" + legacy_docs.mkdir(parents=True) + pdf = legacy_docs / "report.pdf" + pdf.write_bytes(b"%PDF-1.4\n%mock") + + monkeypatch.setattr( + "gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", + (legacy_docs,), + ) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + assert BasePlatformAdapter.validate_media_delivery_path(str(pdf)) == str(pdf.resolve()) + + +class TestMediaDeliveryRejectionLogging: + """``filter_*`` callers should log *why* a path was rejected. + + Before this work the warning was just ``"Skipping unsafe MEDIA directive + path outside allowed roots"``, so operators chasing "the MEDIA tag didn't + attach" had no way to tell allowlist-miss from stale-mtime from + denied-prefix without rebuilding the bot with debug logging. + + Reason-tag assertions import the production constants rather than raw + strings so a future rename of ``_MEDIA_REASON_*`` cannot silently drift + these tests out of sync with what production actually emits. + """ + + def _patch_roots_empty(self, monkeypatch): + monkeypatch.setattr( + "gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", + tuple(), + ) + + def test_filter_media_logs_redacted_path_and_reason_on_stale_mtime(self, tmp_path, monkeypatch, caplog): + self._patch_roots_empty(monkeypatch) + monkeypatch.delenv("HERMES_MEDIA_ALLOW_DIRS", raising=False) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "1") + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_SECONDS", "60") + + stale = tmp_path / "nested" / "subdir" / "report.pdf" + stale.parent.mkdir(parents=True) + stale.write_bytes(b"%PDF-1.4") + old_mtime = time.time() - 7200 + os.utime(stale, (old_mtime, old_mtime)) + + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_media_delivery_paths([(str(stale), False)]) + + assert out == [] + joined = "\n".join(r.getMessage() for r in caplog.records) + # Reason tag is present (imported constant, not raw string). + assert _MEDIA_REASON_OUTSIDE_STALE_MTIME in joined, joined + # Redacted path keeps filename so operator can map back to the artifact. + assert "report.pdf" in joined, joined + # Intermediate "nested" / "subdir" path components are elided. + assert "nested" not in joined, joined + assert "subdir" not in joined, joined + + def test_filter_local_logs_reason_no_recency(self, tmp_path, monkeypatch, caplog): + self._patch_roots_empty(monkeypatch) + monkeypatch.delenv("HERMES_MEDIA_ALLOW_DIRS", raising=False) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + fresh = tmp_path / "report.pdf" + fresh.write_bytes(b"%PDF-1.4") + + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_local_delivery_paths([str(fresh)]) + + assert out == [] + joined = "\n".join(r.getMessage() for r in caplog.records) + assert _MEDIA_REASON_OUTSIDE_NO_RECENCY in joined, joined + assert "report.pdf" in joined, joined + + def test_filter_media_logs_reason_does_not_resolve(self, tmp_path, monkeypatch, caplog): + """A nonexistent path is the most common false report. Log it usefully.""" + self._patch_roots_empty(monkeypatch) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "1") + + ghost = tmp_path / "this-was-never-written.pdf" + + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_media_delivery_paths([(str(ghost), False)]) + + assert out == [] + joined = "\n".join(r.getMessage() for r in caplog.records) + assert _MEDIA_REASON_DOES_NOT_RESOLVE in joined, joined + + def test_filter_media_logs_reason_under_denied_prefix(self, tmp_path, monkeypatch, caplog): + """A freshly-touched file under a denied prefix logs the denylist reason. + + Belt-and-braces with ``test_recency_trust_denies_system_paths_even_when_fresh``: + that test asserts the *return value* is None; this asserts the + *log message* carries the specific reason tag so operators can grep + for denylist-rejected attempts. + """ + self._patch_roots_empty(monkeypatch) + monkeypatch.delenv("HERMES_MEDIA_ALLOW_DIRS", raising=False) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "1") + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_SECONDS", "600") + + fake_home = tmp_path / "home" + ssh_dir = fake_home / ".ssh" + ssh_dir.mkdir(parents=True) + secret = ssh_dir / "id_rsa.txt" + secret.write_bytes(b"-----BEGIN ...") + monkeypatch.setenv("HOME", str(fake_home)) + + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_media_delivery_paths([(str(secret), False)]) + + assert out == [] + joined = "\n".join(r.getMessage() for r in caplog.records) + assert _MEDIA_REASON_OUTSIDE_DENIED_PREFIX in joined, joined + + def test_filter_media_log_neutralises_newlines_in_path(self, tmp_path, monkeypatch, caplog): + """A path containing a newline must not produce two log records. + + Without sanitisation an attacker emitting ``MEDIA:/tmp/foo\\nFAKE`` + could forge a second log line that looks operator-emitted. + """ + self._patch_roots_empty(monkeypatch) + monkeypatch.delenv("HERMES_MEDIA_ALLOW_DIRS", raising=False) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + evil = "/tmp/foo\nFAKE LOG ENTRY: granted access" + + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_media_delivery_paths([(evil, False)]) + + assert out == [] + # Exactly one record was produced. + assert len(caplog.records) == 1 + # The newline is replaced with a placeholder so the message stays + # on a single physical line in the operator log. + rendered = caplog.records[0].getMessage() + assert "\n" not in rendered, rendered + assert "FAKE LOG ENTRY" in rendered # body still visible, just neutralised + + +class TestValidateMediaDeliveryPathDelegation: + """``validate_media_delivery_path`` must agree with the reason-bearing helper. + + The public ``validate_media_delivery_path`` discards the reason and + returns just the resolved path. If those two surfaces ever diverge, + callers seeing ``None`` would log one reason while callers seeing the + resolved path would assume a different one. + """ + + def _patch_roots(self, monkeypatch, *roots): + monkeypatch.setattr( + "gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", + tuple(roots), + ) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + def test_public_and_reason_helper_agree_on_resolved_path(self, tmp_path, monkeypatch): + root = tmp_path / "cache" + root.mkdir() + accepted = root / "ok.pdf" + accepted.write_bytes(b"%PDF-1.4") + rejected = tmp_path / "outside.pdf" + rejected.write_bytes(b"%PDF-1.4") + self._patch_roots(monkeypatch, root) + + for candidate in (str(accepted), str(rejected), "", "relative.pdf", "/does/not/exist.pdf"): + public = BasePlatformAdapter.validate_media_delivery_path(candidate) + internal_resolved, _reason = _validate_media_delivery_path_with_reason(candidate) + assert public == internal_resolved, ( + f"Public API drifted from reason-bearing helper for input {candidate!r}: " + f"public={public!r} internal={internal_resolved!r}" + ) + + +class TestFinalResponseSendMessageParity: + """Final-response and ``send_message`` tool paths share extraction/validation. + + Both call ``extract_media`` + ``filter_media_delivery_paths`` on the + response body, by design, so a MEDIA tag that survives one path must + survive the other. A regression here would silently split behaviour + (e.g. final response stops attaching while send_message keeps working, + which is exactly the operator pain reported alongside #31733). + """ + + def test_send_message_tool_imports_the_same_extraction_surface(self): + """``send_message`` tool must reach for ``BasePlatformAdapter`` and the + same ``extract_media`` / ``filter_media_delivery_paths`` staticmethods. + + If a future refactor splits the tool onto a separate extraction chain, + this test catches it at import time before behaviour can drift. + """ + from tools import send_message_tool + import inspect + + src = inspect.getsource(send_message_tool) + assert "from gateway.platforms.base import BasePlatformAdapter" in src + assert "BasePlatformAdapter.extract_media" in src + assert "BasePlatformAdapter.filter_media_delivery_paths" in src + + def test_pdf_media_tag_extracted_identically_for_both_paths(self, tmp_path, monkeypatch): + """Same input -> same extracted media_files whether the caller comes + through the final-response dispatch or the ``send_message`` tool. + + Parity is structural (both sites call ``BasePlatformAdapter.extract_media`` + + ``filter_media_delivery_paths`` on the response body, asserted by + ``test_send_message_tool_imports_the_same_extraction_surface`` above). + This test then drives that shared extraction with a representative + PDF MEDIA tag and confirms a clean resolve, ensuring the staticmethod + surface still produces what both callers expect. + """ + canonical_docs = tmp_path / ".hermes" / "cache" / "documents" + canonical_docs.mkdir(parents=True) + pdf = canonical_docs / "quote.pdf" + pdf.write_bytes(b"%PDF-1.4") + + monkeypatch.setattr( + "gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", + (canonical_docs,), + ) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + body = f"Here is your quote.\n\nMEDIA:{pdf}\n" + + media, cleaned = BasePlatformAdapter.extract_media(body) + filtered = BasePlatformAdapter.filter_media_delivery_paths(media) + + assert filtered == [(str(pdf.resolve()), False)] + assert "MEDIA:" not in cleaned + + def test_pdf_extension_routes_to_document_not_image_or_video(self, tmp_path, monkeypatch): + """A PDF must land on the document/send_document branch, not image or video. + + Guards against future regressions in the dispatch partition in + ``_process_message_background``, which uses + ``_FINAL_RESPONSE_IMAGE_EXTS`` / ``_FINAL_RESPONSE_VIDEO_EXTS`` + (imported from production) to decide between ``send_multiple_images`` / + ``send_video`` / ``send_document``. + """ + canonical_docs = tmp_path / ".hermes" / "cache" / "documents" + canonical_docs.mkdir(parents=True) + pdf = canonical_docs / "report.pdf" + pdf.write_bytes(b"%PDF-1.4") + + monkeypatch.setattr( + "gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", + (canonical_docs,), + ) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + media, _ = BasePlatformAdapter.extract_media(f"MEDIA:{pdf}") + filtered = BasePlatformAdapter.filter_media_delivery_paths(media) + assert filtered == [(str(pdf.resolve()), False)] + + ext = Path(filtered[0][0]).suffix.lower() + assert ext == ".pdf" + # Use the production constants so a future addition like ``.heic`` + # to the image set would surface here instead of drifting silently. + assert ext not in _FINAL_RESPONSE_IMAGE_EXTS, "PDF must NOT route through send_multiple_images" + assert ext not in _FINAL_RESPONSE_VIDEO_EXTS, "PDF must NOT route through send_video" + + # --------------------------------------------------------------------------- # should_send_media_as_audio # --------------------------------------------------------------------------- From 7e3b47ae24ba35893f848e622fb8f105e40a5e42 Mon Sep 17 00:00:00 2001 From: GodsBoy Date: Fri, 29 May 2026 08:20:06 +0200 Subject: [PATCH 2/2] fix(gateway): close MEDIA log-forging gap and batch-abort cascade Review follow-ups on the diagnosable-MEDIA-delivery change: - _redact_path_for_log now neutralizes Unicode line separators (NEL U+0085, LS U+2028, PS U+2029) in addition to the C0/DEL range. The previous regex only covered control chars, so a model-emitted path with an embedded U+2028 still split into a second log line and defeated the stated 'cannot forge a fake second log line' guarantee. - _validate_media_delivery_path_with_reason wraps os.path.expanduser so a '~\x00...' path returns does-not-resolve instead of raising ValueError. The filter loops also gain a per-item guard so one unprocessable path can no longer abort the batch and silently drop every other attachment in the response. - Inlined the _FINAL_RESPONSE_*_EXTS module constants at the dispatch sites, removing the pointless local aliases, and dropped a redundant str() coercion in the validator. Tests (tests/gateway/test_platform_base.py, 116 -> 128): - Direct unit coverage for _redact_path_for_log (empty, short no-elide, long elision, C0 neutralization, Unicode line separators, unprintable fallback). - Batch-isolation regression: NUL-after-tilde path and a synthetic validator raise both drop only the offending item; the good attachment survives. - Delegation parity test now reaches the recency-trusted ACCEPT and not-a-file branches that were previously unreachable. - Reason-tag log coverage for not-absolute and not-a-file. - De-overclaimed the 'identical for both paths' parity test name and docstring (it exercises the shared surface once; structural parity is asserted separately by the import test). --- gateway/platforms/base.py | 60 +++++--- tests/gateway/test_platform_base.py | 224 ++++++++++++++++++++++++++-- 2 files changed, 255 insertions(+), 29 deletions(-) diff --git a/gateway/platforms/base.py b/gateway/platforms/base.py index cb67598e0ccd..9fd0c7b5046b 100644 --- a/gateway/platforms/base.py +++ b/gateway/platforms/base.py @@ -1018,7 +1018,12 @@ def _path_is_within(path: Path, root: Path) -> bool: return False -_REDACT_PATH_CONTROL_CHARS = re.compile(r"[\x00-\x1f\x7f]") +# Neutralised before logging so a model-emitted path cannot forge a second +# log line. Covers C0/DEL control chars (newline, CR, NUL, ESC) AND the +# Unicode line/paragraph separators (NEL U+0085, LS U+2028, PS U+2029) that +# ``str.splitlines()`` and most log aggregators (Loki, Datadog) treat as line +# breaks. The C0 range alone is not enough to keep the record on one line. +_REDACT_PATH_CONTROL_CHARS = re.compile(r"[\x00-\x1f\x7f\x85\u2028\u2029]") def _redact_path_for_log(path: str) -> str: @@ -1035,9 +1040,10 @@ def _redact_path_for_log(path: str) -> str: Short absolute paths (3 or fewer ``parts`` -- e.g. ``/etc/passwd``, ``/tmp/leaked.pdf``) have no intermediate components to elide and are returned with only the control-character pass applied. Control - characters (newline, carriage return, NUL, etc.) are replaced with - ``?`` regardless of length, so a model-emitted path containing - embedded newlines cannot forge a fake second log line. + characters and Unicode line separators (newline, carriage return, NUL, + NEL U+0085, LS U+2028, PS U+2029, etc.) are replaced with ``?`` + regardless of length, so a model-emitted path containing an embedded + line break cannot forge a fake second log line. """ if not path: return "" @@ -1067,6 +1073,10 @@ def _redact_path_for_log(path: str) -> str: # Non-strict (default) accept: file resolved, is a regular file, and is not # under the credential / system-path denylist. _MEDIA_REASON_DENYLIST_CLEARED = "denylist-cleared" +# Defensive catch-all for the filter loops: a path that makes the validator +# itself raise. Should be unreachable now that expanduser is guarded, but the +# loops log this and continue rather than dropping the whole attachment batch. +_MEDIA_REASON_VALIDATION_ERROR = "validation-error" def _validate_media_delivery_path_with_reason(path: str) -> Tuple[Optional[str], str]: @@ -1082,14 +1092,20 @@ def _validate_media_delivery_path_with_reason(path: str) -> Tuple[Optional[str], if not path: return None, _MEDIA_REASON_EMPTY - candidate = str(path).strip() + candidate = path.strip() if len(candidate) >= 2 and candidate[0] == candidate[-1] and candidate[0] in "`\"'": candidate = candidate[1:-1].strip() candidate = candidate.lstrip("`\"'").rstrip("`\"',.;:)}]") if not candidate: return None, _MEDIA_REASON_EMPTY - expanded = Path(os.path.expanduser(candidate)) + try: + expanded = Path(os.path.expanduser(candidate)) + except (OSError, RuntimeError, ValueError): + # os.path.expanduser raises ValueError("embedded null byte") for a + # ``~\x00...`` path before .resolve() is ever reached. Treat it as an + # unresolvable path instead of letting it crash the caller's loop. + return None, _MEDIA_REASON_DOES_NOT_RESOLVE if not expanded.is_absolute(): return None, _MEDIA_REASON_NOT_ABSOLUTE @@ -2541,7 +2557,12 @@ def filter_media_delivery_paths( safe_media: List[Tuple[str, bool]] = [] for media_path, is_voice in media_files or []: raw = str(media_path) - resolved, reason = _validate_media_delivery_path_with_reason(raw) + try: + resolved, reason = _validate_media_delivery_path_with_reason(raw) + except Exception: + # Never let one unprocessable path abort the whole batch and + # silently drop every other attachment in the response. + resolved, reason = None, _MEDIA_REASON_VALIDATION_ERROR if resolved: safe_media.append((resolved, bool(is_voice))) else: @@ -2562,7 +2583,11 @@ def filter_local_delivery_paths(file_paths: Optional[Iterable[Any]]) -> List[str safe_paths: List[str] = [] for file_path in file_paths or []: raw = str(file_path) - resolved, reason = _validate_media_delivery_path_with_reason(raw) + try: + resolved, reason = _validate_media_delivery_path_with_reason(raw) + except Exception: + # Never let one unprocessable path abort the whole batch. + resolved, reason = None, _MEDIA_REASON_VALIDATION_ERROR if resolved: safe_paths.append(resolved) else: @@ -3910,12 +3935,11 @@ async def _stop_typing_task() -> None: logger.warning("[%s] Error batching images: %s", self.name, batch_err, exc_info=True) - # Send extracted media files — route by file type. Use the - # module-scope partitions so the regression tests in - # ``tests/gateway/test_platform_base.py`` stay in sync with - # what production actually dispatches. - _VIDEO_EXTS = _FINAL_RESPONSE_VIDEO_EXTS - _IMAGE_EXTS = _FINAL_RESPONSE_IMAGE_EXTS + # Send extracted media files, route by file type using the + # module-scope partitions (_FINAL_RESPONSE_IMAGE_EXTS / + # _FINAL_RESPONSE_VIDEO_EXTS) directly, so the regression tests + # in ``tests/gateway/test_platform_base.py`` import the same + # constants production dispatches on. # Partition images out of media_files + local_files so they # can be sent as a single batch (Signal RPC). When @@ -3928,7 +3952,7 @@ async def _stop_typing_task() -> None: _non_image_media: list = [] for media_path, is_voice in media_files: _ext = Path(media_path).suffix.lower() - if (_ext in _IMAGE_EXTS + if (_ext in _FINAL_RESPONSE_IMAGE_EXTS and not is_voice and not force_document_attachments): _image_paths.append(media_path) @@ -3936,7 +3960,7 @@ async def _stop_typing_task() -> None: _non_image_media.append((media_path, is_voice)) _non_image_local: list = [] for file_path in local_files: - if (Path(file_path).suffix.lower() in _IMAGE_EXTS + if (Path(file_path).suffix.lower() in _FINAL_RESPONSE_IMAGE_EXTS and not force_document_attachments): _image_paths.append(file_path) else: @@ -3965,7 +3989,7 @@ async def _stop_typing_task() -> None: audio_path=media_path, metadata=_thread_metadata, ) - elif ext in _VIDEO_EXTS: + elif ext in _FINAL_RESPONSE_VIDEO_EXTS: media_result = await self.send_video( chat_id=event.source.chat_id, video_path=media_path, @@ -3989,7 +4013,7 @@ async def _stop_typing_task() -> None: await asyncio.sleep(human_delay) try: ext = Path(file_path).suffix.lower() - if ext in _VIDEO_EXTS: + if ext in _FINAL_RESPONSE_VIDEO_EXTS: await self.send_video( chat_id=event.source.chat_id, video_path=file_path, diff --git a/tests/gateway/test_platform_base.py b/tests/gateway/test_platform_base.py index f06815c864b0..3b27a1c03332 100644 --- a/tests/gateway/test_platform_base.py +++ b/tests/gateway/test_platform_base.py @@ -15,11 +15,17 @@ utf16_len, _FINAL_RESPONSE_IMAGE_EXTS, _FINAL_RESPONSE_VIDEO_EXTS, + _MEDIA_REASON_ALLOWLISTED, + _MEDIA_REASON_NOT_ABSOLUTE, + _MEDIA_REASON_NOT_A_FILE, _MEDIA_REASON_OUTSIDE_DENIED_PREFIX, _MEDIA_REASON_OUTSIDE_NO_RECENCY, _MEDIA_REASON_OUTSIDE_STALE_MTIME, _MEDIA_REASON_DOES_NOT_RESOLVE, + _MEDIA_REASON_RECENCY_TRUSTED, + _MEDIA_REASON_VALIDATION_ERROR, _prefix_within_utf16_limit, + _redact_path_for_log, _validate_media_delivery_path_with_reason, ) @@ -839,6 +845,11 @@ def _patch_roots_empty(self, monkeypatch): "gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", tuple(), ) + # The allowlist/recency rejection taxonomy this class asserts only + # applies in strict mode. Since #34022 the default is denylist-only, + # where an outside-allowlist file is accepted (denylist-cleared) + # instead of being rejected for stale-mtime / no-recency. + monkeypatch.setenv("HERMES_MEDIA_DELIVERY_STRICT", "1") def test_filter_media_logs_redacted_path_and_reason_on_stale_mtime(self, tmp_path, monkeypatch, caplog): self._patch_roots_empty(monkeypatch) @@ -947,6 +958,170 @@ def test_filter_media_log_neutralises_newlines_in_path(self, tmp_path, monkeypat assert "FAKE LOG ENTRY" in rendered # body still visible, just neutralised +class TestRedactPathForLog: + """Direct unit coverage for ``_redact_path_for_log``. + + Previously this helper was only exercised indirectly through + ``filter_media_delivery_paths``. The elision boundary and the + control-character / line-separator neutralisation are the log-injection + defense, so they get their own assertions here. + """ + + def test_empty_returns_empty_string(self): + assert _redact_path_for_log("") == "" + + def test_short_path_is_not_elided(self): + # 3 or fewer parts ('/', 'etc', 'passwd') -> nothing to elide. + assert _redact_path_for_log("/etc/passwd") == "/etc/passwd" + + def test_long_path_elides_intermediate_components(self): + out = _redact_path_for_log("/home/user/projects/sub/report.pdf") + assert "report.pdf" in out + assert "..." in out + assert "projects" not in out + assert "sub" not in out + + def test_c0_control_chars_replaced(self): + out = _redact_path_for_log("/tmp/a/b/c/deep\n\r\x00\x1b.pdf") + for ch in ("\n", "\r", "\x00", "\x1b"): + assert ch not in out + assert len(out.splitlines()) == 1, out + + def test_unicode_line_separators_replaced(self): + # NEL / LS / PS are treated as line breaks by str.splitlines() and most + # log aggregators, so the redactor must neutralise them too, not just + # the C0 range. + for sep in ("\u0085", "\u2028", "\u2029"): + evil = f"/home/user/deep/dir/evil{sep}INJECTED.pdf" + out = _redact_path_for_log(evil) + assert sep not in out, (sep, out) + assert len(out.splitlines()) == 1, (sep, out) + assert "INJECTED.pdf" in out # filename still visible, just neutralised + + def test_unprintable_fallback_for_unconstructable_input(self): + class _Boom: + def __fspath__(self): + raise ValueError("boom") + + # Path(_Boom()).parts raises -> the guarded fallback returns a marker. + assert _redact_path_for_log(_Boom()) == "" + + +class TestMediaDeliveryBatchIsolation: + """One unprocessable MEDIA path must not drop the whole attachment batch. + + Regression for the ``~\\x00...`` cascade: ``os.path.expanduser`` raised + ``ValueError: embedded null byte`` before ``.resolve()`` was reached, and + the filter loop had no per-item guard, so a single crafted path aborted the + batch and silently discarded every other (legitimate) attachment. + """ + + def _patch_roots(self, monkeypatch, *roots): + monkeypatch.setattr( + "gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", + tuple(roots), + ) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + def test_helper_does_not_raise_on_nul_after_tilde(self): + resolved, reason = _validate_media_delivery_path_with_reason("~\x00/evil.pdf") + assert resolved is None + assert reason == _MEDIA_REASON_DOES_NOT_RESOLVE + + def test_nul_after_tilde_path_does_not_abort_batch(self, tmp_path, monkeypatch, caplog): + good_dir = tmp_path / "cache" + good_dir.mkdir() + good = good_dir / "good.pdf" + good.write_bytes(b"%PDF-1.4") + self._patch_roots(monkeypatch, good_dir) + + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_media_delivery_paths( + [("~\x00/evil.pdf", False), (str(good), False)] + ) + + # The good file survives; the crafted path is dropped, not raised. + assert out == [(str(good.resolve()), False)] + joined = "\n".join(r.getMessage() for r in caplog.records) + assert _MEDIA_REASON_DOES_NOT_RESOLVE in joined, joined + + def test_validator_exception_is_isolated_per_item(self, tmp_path, monkeypatch, caplog): + """Even an unexpected raise inside the validator drops only that item. + + Exercises the defensive per-item ``try/except`` in the filter loop + independently of the specific NUL vector (now handled upstream). + """ + good_dir = tmp_path / "cache" + good_dir.mkdir() + good = good_dir / "good.pdf" + good.write_bytes(b"%PDF-1.4") + self._patch_roots(monkeypatch, good_dir) + + real = _validate_media_delivery_path_with_reason + + def _boom(path): + if "boom" in path: + raise RuntimeError("synthetic validator failure") + return real(path) + + monkeypatch.setattr( + "gateway.platforms.base._validate_media_delivery_path_with_reason", + _boom, + ) + + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_media_delivery_paths( + [("/tmp/boom.pdf", False), (str(good), False)] + ) + + assert out == [(str(good.resolve()), False)] + joined = "\n".join(r.getMessage() for r in caplog.records) + assert _MEDIA_REASON_VALIDATION_ERROR in joined, joined + + +class TestMediaDeliveryRejectionReasons: + """Log-reason coverage for reject branches the original suite skipped.""" + + def _patch_roots_empty(self, monkeypatch): + monkeypatch.setattr("gateway.platforms.base.MEDIA_DELIVERY_SAFE_ROOTS", tuple()) + monkeypatch.delenv("HERMES_MEDIA_ALLOW_DIRS", raising=False) + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "0") + + def test_filter_media_logs_reason_not_absolute(self, monkeypatch, caplog): + self._patch_roots_empty(monkeypatch) + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_media_delivery_paths([("relative/report.pdf", False)]) + assert out == [] + joined = "\n".join(r.getMessage() for r in caplog.records) + assert _MEDIA_REASON_NOT_ABSOLUTE in joined, joined + + def test_filter_media_logs_reason_not_a_file(self, tmp_path, monkeypatch, caplog): + self._patch_roots_empty(monkeypatch) + a_dir = tmp_path / "subdir" + a_dir.mkdir() + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_media_delivery_paths([(str(a_dir), False)]) + assert out == [] + joined = "\n".join(r.getMessage() for r in caplog.records) + assert _MEDIA_REASON_NOT_A_FILE in joined, joined + + def test_filter_media_log_neutralises_unicode_line_separator(self, monkeypatch, caplog): + """A path containing U+2028 must still produce exactly one log record. + + Companion to ``test_filter_media_log_neutralises_newlines_in_path`` for + the Unicode line-separator vector that the C0-only regex used to miss. + """ + self._patch_roots_empty(monkeypatch) + evil = "/tmp/foo\u2028FAKE LOG ENTRY: granted access" + with caplog.at_level("WARNING", logger="gateway.platforms.base"): + out = BasePlatformAdapter.filter_media_delivery_paths([(evil, False)]) + assert out == [] + assert len(caplog.records) == 1 + rendered = caplog.records[0].getMessage() + assert len(rendered.splitlines()) == 1, rendered + assert "\u2028" not in rendered + + class TestValidateMediaDeliveryPathDelegation: """``validate_media_delivery_path`` must agree with the reason-bearing helper. @@ -972,7 +1147,17 @@ def test_public_and_reason_helper_agree_on_resolved_path(self, tmp_path, monkeyp rejected.write_bytes(b"%PDF-1.4") self._patch_roots(monkeypatch, root) - for candidate in (str(accepted), str(rejected), "", "relative.pdf", "/does/not/exist.pdf"): + # Recency disabled (set by _patch_roots): covers the allowlisted ACCEPT + # plus the reject branches empty / not-absolute / does-not-resolve / + # not-a-file (str(root) resolves but is a directory). + for candidate in ( + str(accepted), # allowlisted ACCEPT + str(root), # resolves but is a directory -> not-a-file + str(rejected), # outside allowlist, recency off -> reject + "", # empty + "relative.pdf", # not-absolute + "/does/not/exist.pdf", # does-not-resolve + ): public = BasePlatformAdapter.validate_media_delivery_path(candidate) internal_resolved, _reason = _validate_media_delivery_path_with_reason(candidate) assert public == internal_resolved, ( @@ -980,6 +1165,22 @@ def test_public_and_reason_helper_agree_on_resolved_path(self, tmp_path, monkeyp f"public={public!r} internal={internal_resolved!r}" ) + # Recency-trusted ACCEPT path: an outside-allowlist file fresh enough to + # pass recency trust must also agree between the two surfaces. This case + # was previously unreachable here because recency was disabled + # unconditionally, leaving the ACCEPT-via-recency branch unverified. + # Recency only gates delivery in strict mode (the default is + # denylist-only since #34022), so enable strict mode to reach it. + monkeypatch.setenv("HERMES_MEDIA_DELIVERY_STRICT", "1") + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_FILES", "1") + monkeypatch.setenv("HERMES_MEDIA_TRUST_RECENT_SECONDS", "600") + fresh_outside = tmp_path / "fresh_report.pdf" + fresh_outside.write_bytes(b"%PDF-1.4") + public = BasePlatformAdapter.validate_media_delivery_path(str(fresh_outside)) + internal_resolved, reason = _validate_media_delivery_path_with_reason(str(fresh_outside)) + assert public == internal_resolved == str(fresh_outside.resolve()) + assert reason == _MEDIA_REASON_RECENCY_TRUSTED + class TestFinalResponseSendMessageParity: """Final-response and ``send_message`` tool paths share extraction/validation. @@ -1006,16 +1207,17 @@ def test_send_message_tool_imports_the_same_extraction_surface(self): assert "BasePlatformAdapter.extract_media" in src assert "BasePlatformAdapter.filter_media_delivery_paths" in src - def test_pdf_media_tag_extracted_identically_for_both_paths(self, tmp_path, monkeypatch): - """Same input -> same extracted media_files whether the caller comes - through the final-response dispatch or the ``send_message`` tool. - - Parity is structural (both sites call ``BasePlatformAdapter.extract_media`` - + ``filter_media_delivery_paths`` on the response body, asserted by - ``test_send_message_tool_imports_the_same_extraction_surface`` above). - This test then drives that shared extraction with a representative - PDF MEDIA tag and confirms a clean resolve, ensuring the staticmethod - surface still produces what both callers expect. + def test_pdf_media_tag_resolves_through_shared_extraction_surface(self, tmp_path, monkeypatch): + """Drive the shared ``extract_media`` + ``filter_media_delivery_paths`` + surface with a representative PDF MEDIA tag and confirm a clean resolve. + + This does NOT prove behavioural parity on its own: it exercises the + shared staticmethods once, it does not invoke ``send_message_tool``. + The parity guarantee is structural and is asserted separately by + ``test_send_message_tool_imports_the_same_extraction_surface`` (both + call sites reach the same ``BasePlatformAdapter`` staticmethods). This + test guards that those staticmethods still produce what both callers + expect for the canonical-documents PDF case. """ canonical_docs = tmp_path / ".hermes" / "cache" / "documents" canonical_docs.mkdir(parents=True)