From c7b612ab6a15ff3c304fa4b625fe6c510283e607 Mon Sep 17 00:00:00 2001 From: Edgar Arakelyan Date: Wed, 28 Oct 2026 14:06:39 -0700 Subject: [PATCH] fix(env): stage a loaded artifact without passing its names to the shell as code MCPServerEnv and WebsiteEnv staged a loaded environment artifact on a VM sandbox's host with bash lines that spliced the artifact's file name and the env's name in unquoted. A name holding shell syntax ran as commands on that host, which on the local provider is the operator's machine. - MCPServerEnv quotes its staging lines and removes the staged file through docker_cp's remove_source, which passes the paths as arguments. - WebsiteEnv quotes the compose exec, compose cp and rm lines. - Both refuse a file name that is not one plain name (a '/', a '\', '.', '..', empty, or a control character) before anything runs, since it builds host and container paths. Tests: a real LocalSandbox runs each env's staging lines with a stub docker, for file and env names with ';', $(), backticks, quotes and spaces; the gateway integration tests load a shell-named seed into an MCP server and a website and check it loads and runs nothing. Co-Authored-By: Claude Opus 5.5 --- src/agent_env/env/envs/mcp_server.py | 10 +- src/agent_env/env/envs/website.py | 11 +- src/agent_env/utils/paths.py | 6 ++ tst/integration/env/gateway/gateway_test.py | 31 +++++- tst/unit/env/envs/test_artifact_staging.py | 111 ++++++++++++++++++++ 5 files changed, 159 insertions(+), 10 deletions(-) create mode 100644 tst/unit/env/envs/test_artifact_staging.py diff --git a/src/agent_env/env/envs/mcp_server.py b/src/agent_env/env/envs/mcp_server.py index 46128d48e..3fd124765 100644 --- a/src/agent_env/env/envs/mcp_server.py +++ b/src/agent_env/env/envs/mcp_server.py @@ -5,6 +5,7 @@ import asyncio import logging import os +import shlex import uuid from importlib.metadata import version as pkg_version from typing import TYPE_CHECKING, Any, Callable, ClassVar, Optional @@ -24,6 +25,7 @@ from agent_env.env.gateway import GatewayMode from agent_env.providers.sandbox_providers.sandbox_provider import SANDBOX_MODE_CONTAINER from agent_env.attribution import Attribution +from agent_env.utils.paths import validate_file_name if TYPE_CHECKING: from agent_env.artifact import CliArtifact, EnvironmentArtifact, EnvironmentUniverseArtifact @@ -231,6 +233,7 @@ async def _staged_artifact_size(self, file_artifact) -> int | None: return metadata.size if metadata is not None else None async def _copy_artifact_into_container(self, file_artifact) -> str: + validate_file_name(file_artifact.filename) container_path = f"/data/{file_artifact.filename}" if self._sandbox.mode == SANDBOX_MODE_CONTAINER: await self._sandbox.write_file_from_object(file_artifact.object_url, container_path) @@ -241,12 +244,11 @@ async def _copy_artifact_into_container(self, file_artifact) -> str: from agent_env.providers.env_providers.constants import GATEWAY_APP_DIR stage_dir = f"{GATEWAY_APP_DIR}/_artifact_staging" vm_temp_path = f"{stage_dir}/{self.environment_name}-{uuid.uuid4().hex[:8]}-{file_artifact.filename}" - await self._sandbox.exec_script(f"mkdir -p {stage_dir}") + await self._sandbox.exec_script(f"mkdir -p {shlex.quote(stage_dir)}") await self._sandbox.load_object_file(file_artifact.object_url, vm_temp_path) container_id = await self._env_provider._get_container_id(self._sandbox, self.environment_name) - await self._sandbox.exec_script(f"docker exec {container_id} mkdir -p /data") - await self._sandbox.docker_cp(vm_temp_path, f"{container_id}:{container_path}") - await self._sandbox.exec_script(f"rm -f {vm_temp_path}") + await self._sandbox.exec_script(f"docker exec {shlex.quote(container_id)} mkdir -p /data") + await self._sandbox.docker_cp(vm_temp_path, f"{container_id}:{container_path}", remove_source=True) return container_path async def _load_environment_artifact_legacy(self, file_artifact) -> None: diff --git a/src/agent_env/env/envs/website.py b/src/agent_env/env/envs/website.py index a35dd30e7..c844a817b 100644 --- a/src/agent_env/env/envs/website.py +++ b/src/agent_env/env/envs/website.py @@ -5,6 +5,7 @@ import asyncio import logging import os +import shlex import uuid from importlib.metadata import version as pkg_version from typing import TYPE_CHECKING, Any, Callable, ClassVar, Optional @@ -22,6 +23,7 @@ ) from agent_env.env.gateway import GatewayMode from agent_env.attribution import Attribution +from agent_env.utils.paths import validate_file_name if TYPE_CHECKING: from agent_env.bundle.authoring import AuthoringContext from agent_env.entity.export import ExportContext @@ -187,17 +189,18 @@ async def load_file_artifact_universe(self, file_artifact_universe: "Any", desti async def _copy_artifact_into_container(self, file_artifact) -> str: from agent_env.providers.env_providers.constants import AGENT_ENV_WEBSITE_BACKEND_SUFFIX, DOCKER_COMPOSE_PATH filename = file_artifact.filename + validate_file_name(filename) backend_service = f"{self.environment_name}-{AGENT_ENV_WEBSITE_BACKEND_SUFFIX}" # Random suffix so concurrent loads on a shared sandbox (parallel /load-universe, # MCP+website sharing a service_name, repeated loads) don't race on a shared path. vm_path = f"/tmp/_artifact_{self.environment_name}-{uuid.uuid4().hex[:8]}-{filename}" container_path = f"/tmp/data/{filename}" - compose = f"docker compose -f {DOCKER_COMPOSE_PATH}" + compose = f"docker compose -f {shlex.quote(DOCKER_COMPOSE_PATH)}" await self._sandbox.load_object_file(file_artifact.object_url, vm_path) - await self._sandbox.exec_script(f"{compose} exec -T {backend_service} mkdir -p /tmp/data") - await self._sandbox.exec_script(f"{compose} cp {vm_path} {backend_service}:{container_path}") - await self._sandbox.exec_script(f"rm -f {vm_path}") + await self._sandbox.exec_script(f"{compose} exec -T {shlex.quote(backend_service)} mkdir -p /tmp/data") + await self._sandbox.exec_script(f"{compose} cp {shlex.quote(vm_path)} {shlex.quote(f'{backend_service}:{container_path}')}") + await self._sandbox.exec_script(f"rm -f {shlex.quote(vm_path)}") return container_path async def _load_environment_artifact_legacy(self, file_artifact) -> None: diff --git a/src/agent_env/utils/paths.py b/src/agent_env/utils/paths.py index f678f44d6..a0d3984ec 100644 --- a/src/agent_env/utils/paths.py +++ b/src/agent_env/utils/paths.py @@ -25,6 +25,12 @@ def validate_relative_filename(filename: str) -> None: raise ValueError(f"Filename {filename!r} must not contain '..' segments") +def validate_file_name(filename: str) -> None: + """Reject anything but one plain file name: an env stages a loaded artifact at host and container paths built from it.""" + if filename in ("", ".", "..") or "/" in filename or "\\" in filename or any(ord(c) < 32 or ord(c) == 127 for c in filename): + raise ValueError(f"Artifact filename {filename!r} must be a single file name, with no '/', '\\' or control characters") + + def os_reason(e: OSError) -> str: """What an OSError says went wrong, as a problem message names it.""" if e.errno == errno.ELOOP: diff --git a/tst/integration/env/gateway/gateway_test.py b/tst/integration/env/gateway/gateway_test.py index ec075584d..19d404c74 100644 --- a/tst/integration/env/gateway/gateway_test.py +++ b/tst/integration/env/gateway/gateway_test.py @@ -2076,7 +2076,7 @@ async def recording_send(self, request, *args, **kwargs): @pytest.mark.asyncio @pytest.mark.integration -async def test_gateway_with_agentenv_environment(agentenv_items_env): +async def test_gateway_with_agentenv_environment(agentenv_items_env, monkeypatch, tmp_path): """v1 data-plane (/v1/data:*) served by AgentEnvEnvironment + decorators. Counterpart to the legacy /api/reset coverage in test_gateway_with_single_mcp_server.""" @@ -2191,6 +2191,11 @@ async def assert_tool_sees_items(session, tools_result, tool_names): await env.load_environment_artifact(csv_artifact) assert (await protocol_v1.get_data(base)).parts[0].data == {"items": ["file:text/csv:a,b\n1,2"]} + # A file name with shell syntax in it is staged and loaded as it is, and runs nothing on the sandbox host. + await _load_shell_named_artifact(env, "items", monkeypatch, tmp_path) + assert (await protocol_v1.get_data(base)).parts[0].data == {"items": ["shell-named"]} + assert not (tmp_path / "pwned").exists() + # Environment Card extension end-to-end (urn:agentenv:set-errors/v1): a server-local # error-injection capability declared with @extension. Invoking it makes the named tool # start raising at the given rate; calls succeed before and produce errors after. @@ -2359,7 +2364,7 @@ async def test_verify_universe_roundtrip_export_all_v1_and_fallback(multi_env_it @pytest.mark.asyncio @pytest.mark.integration -async def test_gateway_with_website_env_v1(agentenv_website_env): +async def test_gateway_with_website_env_v1(agentenv_website_env, monkeypatch, tmp_path): """v1 data-plane branch of WebsiteEnv.load_environment_artifact: supports_v1 -> reset + add inline, served by a Starlette AgentEnvStarletteApplication backend behind the gateway.""" env = Env.get(agentenv_website_env.id, version=agentenv_website_env.version) @@ -2403,10 +2408,32 @@ async def test_gateway_with_website_env_v1(agentenv_website_env): ) await env.load_environment_artifact(csv_artifact) assert (await protocol_v1.get_data(base)).parts[0].data == {"items": ["file:text/csv:a,b\n1,2"]} + + await _load_shell_named_artifact(env, "webitems", monkeypatch, tmp_path) + assert (await protocol_v1.get_data(base)).parts[0].data == {"items": ["shell-named"]} + assert not (tmp_path / "pwned").exists() finally: await env.close() +async def _load_shell_named_artifact(env, environment_name: str, monkeypatch, tmp_path) -> None: + """Load a seed whose file name runs ``touch pwned`` if the shell parses it, from ``tmp_path`` so a run leaves it there.""" + artifact = EnvironmentArtifact.put( + id=f"test-{environment_name}-shell-named-service-data", + environment_name=environment_name, + file_artifact=FileArtifact.put_bytes( + id=f"test-{environment_name}-shell-named-data", + description=f"{environment_name} seed with shell syntax in its file name", + filename="$(touch pwned) `touch pwned`.json", + content=json.dumps({"items": ["shell-named"]}).encode(), + content_type="application/json", + ), + ) + with monkeypatch.context() as m: + m.chdir(tmp_path) + await env.load_environment_artifact(artifact) + + @pytest.mark.asyncio @pytest.mark.integration async def test_gateway_triggers_extension(sandbox_provider, mcp_server_envs, email_service_artifact): diff --git a/tst/unit/env/envs/test_artifact_staging.py b/tst/unit/env/envs/test_artifact_staging.py new file mode 100644 index 000000000..13cb6042e --- /dev/null +++ b/tst/unit/env/envs/test_artifact_staging.py @@ -0,0 +1,111 @@ +"""An env stages a loaded artifact's file through its sandbox's shell: a file or env name with shell syntax in it is +staged and copied as it is and runs nothing, and a file name that isn't one plain name is refused before anything runs. +A real LocalSandbox runs the scripts; docker is a stub that logs its arguments.""" + +from __future__ import annotations + +import os +import shlex +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock + +import pytest + +from agent_env.env.envs.mcp_server import MCPServerEnv +from agent_env.env.envs.website import WebsiteEnv +from agent_env.providers.sandbox_providers.local_sandbox import LocalSandbox + +_SHELL_NAMES = [ + ("email", "x;touch pwned;#.json"), + ("email", "$(touch pwned).json"), + ("email", "`touch pwned`.json"), + ("email", "x' ; touch pwned ; '.json"), + ("email", "seed data.json"), + ("e;touch pwned;#", "seed.json"), +] +_NOT_ONE_NAME = ["../seed.json", "a/b.json", "a\\b.json", "..", ".", "", "seed\n.json", "seed\x00.json"] + + +@pytest.fixture +def host(tmp_path, monkeypatch): + log = tmp_path / "docker.log" + stub = tmp_path / "bin" / "docker" + stub.parent.mkdir() + stub.write_text(f'#!/bin/sh\nfor a in "$@"; do printf "%s\\n" "$a"; done >> {shlex.quote(str(log))}\necho --- >> {shlex.quote(str(log))}\n') + stub.chmod(0o755) + monkeypatch.setenv("PATH", f"{stub.parent}{os.pathsep}{os.environ['PATH']}") + monkeypatch.chdir(tmp_path) + sandbox = LocalSandbox(work_dir=tmp_path / "app") + sandbox.work_dir.mkdir() + + async def load_object_file(object_url, path): + Path(sandbox._rewrite_app_arg(path)).write_bytes(b"{}") + + sandbox.load_object_file = load_object_file + + def docker_calls() -> list[list[str]]: + return [call.strip("\n").split("\n") for call in log.read_text().split("---\n")[:-1]] if log.exists() else [] + + return SimpleNamespace(sandbox=sandbox, root=tmp_path, docker_calls=docker_calls) + + +@pytest.mark.asyncio +@pytest.mark.parametrize(("environment_name", "filename"), _SHELL_NAMES) +async def test_an_mcp_server_stages_a_shell_looking_name_as_it_is(host, environment_name, filename): + env = _mcp(host, environment_name) + + assert await env._copy_artifact_into_container(_artifact(filename)) == f"/data/{filename}" + + exec_mkdir, cp = host.docker_calls() + assert exec_mkdir == ["exec", "cid", "mkdir", "-p", "/data"] + assert cp[0] == "cp" and cp[2] == f"cid:/data/{filename}" + staged = Path(cp[1]) + assert staged.parent == host.sandbox.work_dir / "_artifact_staging" and staged.name.endswith(filename) + assert not (host.root / "pwned").exists() + assert not staged.exists() + + +@pytest.mark.asyncio +@pytest.mark.parametrize(("environment_name", "filename"), _SHELL_NAMES) +async def test_a_website_stages_a_shell_looking_name_as_it_is(host, environment_name, filename): + env = _website(host, environment_name) + + assert await env._copy_artifact_into_container(_artifact(filename)) == f"/tmp/data/{filename}" + + exec_mkdir, cp = host.docker_calls() + service = f"{environment_name}-website-backend" + compose = ["compose", "-f", str(host.sandbox.work_dir / "docker-compose.yml")] + assert exec_mkdir == [*compose, "exec", "-T", service, "mkdir", "-p", "/tmp/data"] + assert cp[:4] == [*compose, "cp"] and cp[5] == f"{service}:/tmp/data/{filename}" + assert not (host.root / "pwned").exists() + assert cp[4].endswith(filename) and not Path(cp[4]).exists() + + +@pytest.mark.asyncio +@pytest.mark.parametrize("make_env", [lambda h: _mcp(h, "email"), lambda h: _website(h, "email")], ids=["mcp", "website"]) +@pytest.mark.parametrize("filename", _NOT_ONE_NAME) +async def test_a_name_that_is_not_one_plain_file_name_is_refused_before_anything_runs(host, make_env, filename): + with pytest.raises(ValueError, match="must be a single file name"): + await make_env(host)._copy_artifact_into_container(_artifact(filename)) + + assert host.docker_calls() == [] and list(host.sandbox.work_dir.iterdir()) == [] + + +def _mcp(host, environment_name: str) -> MCPServerEnv: + env = MCPServerEnv(id="mcp-email", version=1, docker_image_artifact=None, environment_name=environment_name) + env._sandbox = host.sandbox + env._env_provider = SimpleNamespace(_get_container_id=AsyncMock(return_value="cid")) + return env + + +def _website(host, environment_name: str) -> WebsiteEnv: + image = SimpleNamespace(id="img", version=1, type="docker_image", image_name="img:1") + env = WebsiteEnv(id="w", version=1, backend_docker_image_artifact=image, frontend_docker_image_artifact=image, + environment_name=environment_name) + env._sandbox = host.sandbox + return env + + +def _artifact(filename: str) -> SimpleNamespace: + return SimpleNamespace(filename=filename, object_url="file:///store/seed")