Skip to content

fix(gateway): translate Docker container paths for media delivery - #42305

Closed
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/docker-media-delivery-path-translation
Closed

fix(gateway): translate Docker container paths for media delivery#42305
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:fix/docker-media-delivery-path-translation

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Adds Docker container-to-host path translation in validate_media_delivery_path() so that media delivery works when the agent runs in a Docker sandbox with configured volume mounts.

Related Issue

Fixes #42299

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • gateway/platforms/base.py: Added _translate_docker_container_path() helper that maps container paths (e.g. /output/report.pdf) to host paths using longest-prefix match against terminal.docker_volumes config entries. Translation runs before resolve() so the host file can be found; all existing validation (denylist, strict mode, recency window) applies unchanged after translation.
  • tests/gateway/test_platform_base.py: Added TestDockerContainerPathTranslation class with 11 tests covering: basic translation, nested paths, longest-prefix match, non-docker backend, no matching volume, empty volumes, config errors, exact mount point match, integration with validate_media_delivery_path, and denylist enforcement after translation.

How to Test

  1. Configure terminal.backend: docker with docker_volumes: ["/host/path:/output"]
  2. Have the agent produce a file inside the container at /output/report.pdf
  3. Verify the file is delivered successfully via MEDIA:/output/report.pdf
  4. Run tests: pytest tests/gateway/test_platform_base.py::TestDockerContainerPathTranslation -v

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Code Intelligence

  • Analyzed: validate_media_delivery_path() (module-level function, called by BasePlatformAdapter class method + gateway adapters)
  • Blast radius: LOW — additive change; no existing behavior modified; only activates when terminal.backend == docker
  • Related patterns: telegram.py::_missing_media_path_error() already had a hint about container paths; this fix resolves the underlying validation gap

When terminal.backend is docker, the agent produces files inside the
sandbox and references them by container path (e.g. /output/report.pdf).
The gateway runs on the host and validate_media_delivery_path() resolves
paths against the host filesystem, so container paths are always rejected.

Add _translate_docker_container_path() which maps container paths to host
paths using longest-prefix match against terminal.docker_volumes config
entries (host:container format). Translation runs before resolve(), and
all existing validation (denylist, strict mode, recency) still applies
after translation.

Fixes NousResearch#42299
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery backend/docker Docker container execution labels Jun 8, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting the host/container boundary; the premise is still present on current main: gateway/platforms/base.py:1302-1305 resolves MEDIA paths only on the host and rejects an unresolved container path.

Problems

  • Security: gateway/platforms/base.py:1074-1075 joins an unnormalized suffix onto the host mount with no containment check. With /host/export:/output, MEDIA:/output/../../home/user/file resolves outside /host/export. In default mode, current validation accepts existing paths that are not denylisted (gateway/platforms/base.py:1320-1330), so this can bypass the intended bind-mount boundary.
  • Docker syntax: gateway/platforms/base.py:1060 uses rpartition(':'), so a documented mount such as /host/data:/data:ro is parsed with ro as its container prefix. The documented syntax explicitly supports host_path:container_path[:options] (website/docs/user-guide/configuration.md:426-434).

Suggested changes

  • Resolve both the translated file and mapped host root, then require containment within that root; add a traversal regression test.
  • Parse optional Docker mount modes correctly and test a :ro mount.

Automated hermes-sweeper review.

Comment thread gateway/platforms/base.py
for vol in volumes:
if not isinstance(vol, str) or ":" not in vol:
continue
host_part, _, container_part = vol.rpartition(":")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This misparses supported host:container[:options] specs: /host/data:/data:ro produces container_part == "ro", so paths below /data never translate. Please parse the optional mode separately and add a :ro regression test.

Comment thread gateway/platforms/base.py
return None

suffix = candidate[best_match_len:]
return best_host_prefix + suffix

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suffix is unnormalized, so /output/../../home/user/file under /host/export:/output becomes /host/export/../../home/user/file and can resolve outside the configured bind mount. Resolve the host root and candidate and reject results not contained in that root before returning them.

@teknium1 teknium1 added sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/i18n Localization, locales, translations labels Jul 14, 2026
@teknium1

teknium1 commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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!

@teknium1 teknium1 closed this Aug 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/i18n Localization, locales, translations backend/docker Docker container execution comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Media delivery rejects docker_volumes-mapped container paths (e.g. /output) under terminal.backend: docker

3 participants