fix(gateway): translate Docker container paths to host before media delivery - #47716
fix(gateway): translate Docker container paths to host before media delivery#47716elphamale wants to merge 5 commits into
Conversation
42f96a6 to
72d69bf
Compare
ab37b31 to
facfca2
Compare
_guest_media_send() opened any local path the LLM turn emitted via MEDIA: <path> with no containment check. The delivery-constraint prompt tells the model to stage under HERMES_HOME/cache/<subdir>, but that was prompt-level guidance only, not enforced -- a guest-triggered turn coerced into requesting an arbitrary host path (credentials store, SSH keys, etc.) would have it staged to TELEGRAM_HOME_CHANNEL and made deliverable to the guest chat via deliver_<token>. Add _guest_media_root() (HERMES_HOME/cache) and reject any resolved local path outside it before open()/upload is ever attempted. Also noting a separate, pre-existing issue found while testing this: _guest_media_send calls self._translate_docker_path(), which is not defined anywhere on this branch -- it only exists locally because PR NousResearch#47716 (a separate, still-open PR) is applied to the local production checkout. This PR has an undeclared runtime dependency on NousResearch#47716; merging this before NousResearch#47716 lands will crash the first guest media delivery attempt with AttributeError.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real Docker delivery mismatch: current main still documents that host-side gateway delivery cannot read container-only paths (gateway/run.py:3161-3165) and the generic adapter flow still validates those paths on the host (gateway/platforms/base.py:4927-4947).
Problems
- Blocking — security: PR
gateway/platforms/base.py:3558-3568reads an arbitrary unmapped container path withdocker exec ... catbefore validation. Docker mounts declared credential files into the container (tools/environments/docker.py:709-738), while default host validation permits ordinary files outside its host denylist (gateway/platforms/base.py:1264-1342). Copying to/tmp/<basename>can therefore turn a container-visible credential into a deliverable host file. - Blocking — wrong container: PR
gateway/platforms/base.py:3510-3526returns the first active container and receives no task/session identity. Concurrent task containers can have different mounts, so this can translate and read from the wrong environment. - The patch misses the primary generic adapter path at
gateway/platforms/base.py:4927-4947, plus cron and other direct filter callers.
Suggested changes
- Use a task-bound artifact/export resolver that permits only managed output roots; do not exec-read arbitrary model-emitted paths.
- Centralize the guarded conversion at every host-side attachment-delivery choke point and add integration coverage for security rejection and multiple active containers.
Automated hermes-sweeper review.
| host_path = src + path[len(dest):] | ||
| break | ||
|
|
||
| if host_path == path and not Path(path).exists() and container_id: |
There was a problem hiding this comment.
Blocking: this branch docker exec cats an arbitrary container path before the existing host-side media validator runs. Docker mounts declared credential files into the container, and copying one to /tmp/<basename> can make it pass the default non-strict host validator. Restrict reads to a task-bound managed artifact/export root and validate before extraction.
| import json | ||
| import subprocess | ||
| from tools.terminal_tool import _active_environments # type: ignore[import] | ||
| for env in list(_active_environments.values()): |
There was a problem hiding this comment.
This returns the first active container with an id, but the delivery call has no task/session identity. Concurrent Docker tasks can have different mount tables, so a response can be translated against the wrong container. Thread the producing task/container through this API instead of scanning all active environments.
|
Pushed ff2dda849 addressing both blocking findings plus the coverage bullet: Arbitrary container-path read / wrong-container selection: Coverage ("plus cron and other direct filter callers"): wired translation into These four (plus Filed #64889 to thread a real |
1ac7780 to
58b8a9d
Compare
…validation When the terminal backend is Docker, agent commands run inside a container where /workspace is a bind-mount of a host directory. send_message runs on the HOST where /workspace doesn't exist, so validate_media_delivery_path silently drops MEDIA:/workspace/... files — the text caption is sent but the video/image never is. Add _translate_docker_workspace_paths() which looks up the active DockerEnvironment._workspace_dir from terminal_tool._active_environments and rewrites /workspace/... paths to their host equivalents before path validation runs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…+ /tmp fallback
The previous fix only handled /workspace/ paths. Agent downloads often land
in /tmp/ or /root/.hermes/cache/ inside the container, which are not bind-
mounted to /workspace but may be covered by other mounts (e.g. audio_cache).
New approach:
1. Run `docker inspect` to get the full mount table for the active container.
2. Walk mounts longest-first to translate any container path to its host
equivalent (covers /root, /root/.hermes/cache/audio, /mnt/hermes_home, etc.)
3. For paths in unmounted dirs like /tmp/, fall back to `docker cp` into a
temp file on the host so the file can still be delivered.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ry paths The previous change added container→host path translation for the send_message tool, but the gateway delivery loop (gateway/run.py) extracts and filters MEDIA paths directly, bypassing that fix — so agent-response media written inside a Docker terminal backend was dropped by the host-side path filter. Promote the translation to shared BasePlatformAdapter staticmethods so a single implementation serves both paths: - _get_docker_mount_table / _translate_one_docker_path / translate_docker_media_paths / translate_docker_local_paths on BasePlatformAdapter. The mount table is built once per call; unmounted paths (e.g. /tmp) fall back to docker cp. - send_message_tool.py now calls the shared method (logic unchanged; the two tool-local functions are removed). - gateway/run.py translates container paths at all three delivery sites before the host-side media/local path filters run. Adds TestDockerPathTranslation (7 cases): bind-mount mapping, longest-prefix precedence, exact-destination match, unmapped passthrough, local-path variant, and empty inputs. No regression in the existing base/send_message suites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ction docker cp fails against the overlayfs storage driver on some hosts, so the last-resort container-path extraction in _translate_one_docker_path silently no-ops there and the original unmapped container path is returned unchanged. docker exec + cat works regardless of storage driver. Also stop renaming extracted files to a hashed temp name (hermes_docker_media_<md5>.<ext>) and preserve the container path's original basename instead. Some platform adapters (e.g. Telegram) fall back to the local upload filename as a display title when no explicit title is passed, so a hashed name was surfacing as a garbled title to end users.
…credential-exfil gap hermes-sweeper flagged two blocking issues in the Docker container->host translation added here: the exec-cat fallback read any unmapped container path with no containment check (a symlinked/aliased credential could be extracted to a fresh host /tmp path and sail past the delivery denylist, which only recognizes known host secret locations), and the mount lookup picked "the first active environment" with no binding to which task actually produced the path. - _get_docker_mount_table/_translate_one_docker_path/translate_docker_* now take an optional task_id. When given (send_message_tool, yuanbao_tools' send_dm, and the background-task delivery path — the only three call sites where task_id is reliably available and correctly correlated, traced end to end), lookup resolves only that task's own container and the exec-cat extraction fallback is available. - Extraction (_extract_docker_path) is confined to the container's /workspace root, resolved via `docker exec realpath -e` before any read (so a symlink planted at /workspace/x -> /root/.hermes/.env is caught, not followed), size-gated via `docker exec stat` before the actual cat (config-overridable via gateway.max_docker_extraction_bytes, defaulting to the largest enabled platform's own upload ceiling), and lands in a private tempfile.mkdtemp() dir rather than the predictable /tmp/<basename> the old code used. - Callers that can't identify the producing task (gateway/platforms/base.py's generic handle_message, weixin.py's send(), gateway/run.py's _deliver_media_from_response, and cron/scheduler.py's two delivery sites — confirmed job["id"] is not actually the job's terminal task_id) now also get mount-table translation, but degraded: only when exactly one Docker environment is active, and never the extraction fallback. This closes the coverage gap the sweeper review called out (cron and other direct filter callers had no translation at all) without extending the unsafe unscoped-extraction behavior to them. - Replaced the silent `except Exception: pass` around extraction with logger.warning at each rejection point. Follow-up NousResearch#64889 tracks threading a real task_id through agent_result so the degraded paths above can eventually get the same task-bound treatment instead of the mount-only/single-env fallback. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CvMiTppKDazXfakuoLgQK
58b8a9d to
e3fd87a
Compare
|
Closing as resolved by #81746 (merge commit 238351a), which merged the same container→host mount translation approach salvaged from #37207 — with #27779 by @LEAFERx as the first submission for this bug. Your PR independently arrived at the same fix; credit to all four contributors in the cluster. Thanks! |
_guest_media_send() opened any local path the LLM turn emitted via MEDIA: <path> with no containment check. The delivery-constraint prompt tells the model to stage under HERMES_HOME/cache/<subdir>, but that was prompt-level guidance only, not enforced -- a guest-triggered turn coerced into requesting an arbitrary host path (credentials store, SSH keys, etc.) would have it staged to TELEGRAM_HOME_CHANNEL and made deliverable to the guest chat via deliver_<token>. Add _guest_media_root() (HERMES_HOME/cache) and reject any resolved local path outside it before open()/upload is ever attempted. Also noting a separate, pre-existing issue found while testing this: _guest_media_send calls self._translate_docker_path(), which is not defined anywhere on this branch -- it only exists locally because PR NousResearch#47716 (a separate, still-open PR) is applied to the local production checkout. This PR has an undeclared runtime dependency on NousResearch#47716; merging this before NousResearch#47716 lands will crash the first guest media delivery attempt with AttributeError.
hermes-sweeper flagged a blocking bug: _guest_media_send called self._translate_docker_path(local_path), a method that doesn't exist anywhere in this codebase — any local media staging raised AttributeError before the containment check or upload ever ran. The PR's own tests hid this by monkeypatching the (nonexistent) method with an identity lambda instead of exercising it. _guest_media_root()'s docstring described an earlier design (hardcode a container path like /cache/<subdir> and translate it) that this same PR already moved away from — the delivery-constraint prompt in _handle_guest_message_update was changed to tell the LLM to write to and cite the same HERMES_HOME-relative path the host uses directly, matching the generic "cite whatever absolute path you wrote to, let the delivery pipeline translate it" convention every other MEDIA: prompt already relies on (agent/prompt_builder.py) — but the corresponding cleanup of _guest_media_send's translation call was missed. Wired to tools.credential_files.from_agent_visible_cache_path(): already on main (not part of the unrelated NousResearch#47716 Docker-translation work), and already used in production for the identical shape of problem — tools/image_source.py's _permitted_host_read_target() uses the same translate-then-contain pattern for agent-visible cache paths. No-ops cleanly when TERMINAL_ENV != "docker" or the path isn't under a mounted cache dir; the existing hard containment check (relative_to guest_media_root()) still gates the final result either way. Also replaced the two Branch 2/3 tests that only asserted the mock recorded a hand-built API call the test made directly, without ever invoking _handle_guest_message_update — so token resolution, caller authorization, and dispatch were never actually exercised. They now call the real handler with a deliver_<token> guest update. Added a companion test confirming the caller-authorization gate applies to token redemption too (a leaked valid token must still be denied for an unauthorized caller), per the fail-closed guarantee already documented in the handler. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012CvMiTppKDazXfakuoLgQK
Problem
When the terminal backend is Docker, agent commands run inside a container where output paths (
/workspace,/output,/tmp, or any other bind-mount destination) only exist inside the container. The gateway runs on the host, wherevalidate_media_delivery_pathcallsPath.resolve(strict=True). A container-only path raisesOSError→ returnsNone→filter_media_delivery_pathssilently drops the file. The recipient gets only the text caption while the tool reports success (the text message still gets amessage_id).This affects several independent host-side delivery paths across
send_message_tool.py,gateway/run.py,gateway/platforms/base.py's generic response path,weixin.py,cron/scheduler.py, andyuanbao_tools.py.Fix
Docker container→host translation, shared on
BasePlatformAdapter, applied before every host-side path filter runs:_get_docker_mount_table(task_id=None)— reads bind mounts viadocker inspect, sorted longest-destination-first. Whentask_idis given, resolves only that task's own environment (_active_environments[task_id]). When omitted, only proceeds if exactly one Docker environment is active — with zero or multiple concurrent environments there's no way to guess correctly, so it no-ops rather than risk resolving against the wrong container._translate_one_docker_path()— maps a path through the longest matching mount prefix; for a path under no mount that doesn't exist on the host, falls back to extraction — but only whenallow_extraction=True(i.e.task_idwas given)._extract_docker_path()— the exec-cat fallback, now gated three ways before any container read: (1) lexically confined to the container's/workspace(POSIXnormpath, any..rejected), (2) symlink-safe — resolved inside the container viadocker exec realpath -efirst, with the resolved path re-checked against/workspace(so a symlink at/workspace/x→/root/.hermes/.envis caught, not followed), (3) size-gated viadocker exec statbefore the read (gateway.max_docker_extraction_bytes, defaulting to the largest enabled platform's own upload ceiling). Extraction lands in a privatetempfile.mkdtemp()dir, not the old predictable/tmp/<basename>. Failures nowlogger.warninginstead of a silentexcept: pass.translate_docker_media_paths()/translate_docker_local_paths()— batch wrappers; take an optionaltask_id.Call sites, split by whether
task_idis reliably available (traced end to end, not assumed):send_message_tool.py,yuanbao_tools.py::_handle_yb_send_dm(both registry-dispatched tools —task_idarrives via**kw, threaded fromagent/agent_runtime_helpers.py→model_tools.handle_function_call→tools/registry.py'sentry.handler(args, **kwargs)), andgateway/run.py::_run_background_task(explicittask_idparam, threaded intoagent.run_conversation(..., task_id=task_id)).gateway/platforms/base.py's generichandle_messagepath,weixin.py'ssend(),gateway/run.py::_deliver_media_from_response, and bothcron/scheduler.pydelivery sites. None of these currently have a task_id that's actually correlated with the terminal environment that produced the path — confirmed for cron specifically:job.get("id")is not the job's real task_id, sinceagent.run_conversation(prompt)atcron/scheduler.py:2861is called with notask_id, soagent/turn_context.py'seffective_task_id = task_id or str(uuid.uuid4())generates a fresh random UUID per run. Thread a real task_id through agent_result so generic media delivery can be task-bound #64889 tracks threading a real task_id throughagent_resultso these can eventually get the same task-bound treatment.If no Docker environment is active, or
docker inspect/realpath/statfails, paths pass through unchanged — non-Docker setups are unaffected.Tests
tests/gateway/test_platform_base.py::TestDockerPathTranslation(28 cases, all shelling-out stubbed): bind-mount mapping/longest-prefix/exact-match/passthrough, task-bound extraction (exec-cat not cp, basename preserved, cat-failure cleanup, oversize-refusal with no host file left behind), containment (out-of-root path and..traversal rejected before anydocker execcall, symlink-escape rejected afterrealpathbut beforestat/cat), task-bound container selection (two concurrent containers each resolve their own mount table), and the degraded path (single-environment mount-only resolution, refuses when ambiguous, never extracts even when a container_id is available).Test plan
..-traversal, and oversize-file all rejected before/without a container read; failure and success paths clean up correctlyNote vs. the original test plan: an arbitrary unmounted
/tmp/path is no longer extracted in any mode — extraction is confined to the container's/workspace. This is intentional (see the security discussion below); the fix for a genuinely large or oddly-located output is Docker'spersistent_filesystemmode, where the bind mount delivers it with zero copy and no size cap.Security
Addresses hermes-sweeper's blocking findings on this PR:
/procalias, or a tool's own in-container copy of a secret) could be extracted to a fresh host/tmppath that the existing delivery denylist has no way to recognize as sensitive, since the denylist only matches known host secret locations. Closed by confining extraction to/workspace, resolved viarealpath -einside the container before any read._get_docker_mount_tableused to grab "the first active environment" with no correlation to which task asked for delivery. Closed for the paths where task_id is reliably available; degraded to single-environment-only elsewhere, tracked to full resolution in Thread a real task_id through agent_result so generic media delivery can be task-bound #64889.