From 56b97430eb1f57b9077e35d623c52bcd4e56967e Mon Sep 17 00:00:00 2001 From: Bai Li Date: Thu, 20 Aug 2026 18:05:40 -0700 Subject: [PATCH] fix(docker): expand ~ and $VAR in extra_mounts destinations The source side of an extra_mounts spec is normalized with expandvars(expanduser(...)) so authors can write portable specs; the destination was only checked for a leading "/" and never expanded. That asymmetry makes one common mount impossible to write portably. env_passthrough forwards HOME with the HOST value on purpose, so any container-side path that must line up with $HOME -- $HOME/.uipath for the uip CLI's saved login state, for instance -- has a different literal value on every host. The only way to express it was to hardcode one host's home directory, which then mounts to the wrong place everywhere else. A login state the CLI cannot see fails tasks as a capability problem rather than a config one, so the misconfiguration is close to invisible: it cost 26% of the rows in an ad-hoc Maestro run before it was spotted. Expand the destination the same way, before the absolute-path check, so `~/.uipath:$HOME/.uipath:rw` resolves. Two details worth keeping: - expandvars leaves an unset variable verbatim, so a typo'd name still fails the absolute-path check. The message now shows the raw and the expanded form, otherwise it reads as a puzzle. - the framework-owned-mount check runs on the expanded destination, since a variable could itself expand to /work or / and the raw form would sail past the gate. Co-Authored-By: Claude Opus 5 (1M context) --- src/coder_eval/isolation/docker_runner.py | 36 ++++++++++++++++++----- tests/test_docker_runner_mounts.py | 28 ++++++++++++++++++ 2 files changed, 56 insertions(+), 8 deletions(-) diff --git a/src/coder_eval/isolation/docker_runner.py b/src/coder_eval/isolation/docker_runner.py index 2331e4ec..0b97aeda 100644 --- a/src/coder_eval/isolation/docker_runner.py +++ b/src/coder_eval/isolation/docker_runner.py @@ -290,9 +290,19 @@ def _validate_extra_mount(spec: str) -> str: Defends against typos that would silently expose the host fs to the container, and against mount specs that shadow framework-owned mounts. - Normalizes the source side by expanding ``~`` and ``$VAR`` so authors - can write portable specs. Returns the (possibly rewritten) spec to - feed back into argv. + Normalizes BOTH sides by expanding ``~`` and ``$VAR`` so authors can + write portable specs. Returns the (possibly rewritten) spec to feed + back into argv. + + The destination is expanded for the same reason the source is, and it + matters more than it looks: ``env_passthrough`` forwards ``HOME`` with + the HOST value on purpose, so a container-side path that must line up + with ``$HOME`` (``$HOME/.uipath`` for the uip CLI's login state, say) + has a different literal value on every host. Without expansion the only + way to write that mount is to hardcode one host's home directory, which + then silently mounts to the wrong place everywhere else -- and a login + state the CLI cannot see reads as a capability failure, not a config + error. Notes: - Mode is REQUIRED. Forgetting ``:ro`` is the single most common way @@ -312,26 +322,36 @@ def _validate_extra_mount(spec: str) -> str: parts = body.split(":") if len(parts) < 2 or len(parts) > 3: raise ValueError(f"Invalid extra_mounts entry {spec!r}: expected `src:dst[:ro|rw]`.") - src, dst = head + parts[0], parts[1] + src, raw_dst = head + parts[0], parts[1] # Default to read-only when mode is omitted. Mounting host paths RW # by default is the wrong sandbox stance: the few RW use-cases are # better stated explicitly than implied by silence. mode = parts[2] if len(parts) == 3 else "ro" if not src: raise ValueError(f"Invalid extra_mounts entry {spec!r}: empty source path.") - if not dst: + if not raw_dst: raise ValueError(f"Invalid extra_mounts entry {spec!r}: empty destination path.") + # Expand ~ and $VAR on BOTH sides so authors can write portable specs. + # Destination expansion happens BEFORE the absolute-path check, since the + # whole point is to let `$HOME/...` resolve to an absolute path. + expanded_src = os.path.expandvars(os.path.expanduser(src)) + dst = os.path.expandvars(os.path.expanduser(raw_dst)) if not dst.startswith("/"): - raise ValueError(f"Invalid extra_mounts entry {spec!r}: destination must be an absolute path.") + # An unset variable is left verbatim by expandvars, so a typo'd name + # lands here. Show both forms or the message is a puzzle. + detail = f"{raw_dst!r}" if dst == raw_dst else f"{raw_dst!r} (expanded to {dst!r})" + raise ValueError( + f"Invalid extra_mounts entry {spec!r}: destination must be an absolute path, got {detail}." + ) if mode not in ("ro", "rw"): raise ValueError(f"Invalid extra_mounts entry {spec!r}: mode must be 'ro' or 'rw'.") - # Expand ~ and $VAR in the source so authors can write portable specs. - expanded_src = os.path.expandvars(os.path.expanduser(src)) if not Path(expanded_src).exists(): raise ValueError(f"Invalid extra_mounts entry {spec!r}: source path does not exist on host.") # Reject destinations that shadow framework-owned mounts inside the # container. ``/work`` substrings are caught too -- /work/foo would # land underneath our staging dir and shadow the input/output tree. + # Checked on the EXPANDED destination: `$HOME` could itself expand to a + # reserved path, and the raw form would sail past this gate. dst_norm = dst.rstrip("/") or "/" if dst_norm in _RESERVED_MOUNT_DESTS or dst_norm.startswith(CONTAINER_WORK_DIR + "/"): raise ValueError( diff --git a/tests/test_docker_runner_mounts.py b/tests/test_docker_runner_mounts.py index 0459b8e0..d5f857eb 100644 --- a/tests/test_docker_runner_mounts.py +++ b/tests/test_docker_runner_mounts.py @@ -108,6 +108,34 @@ def test_var_expansion_in_source(self, real_dir, monkeypatch): result = _validate_extra_mount("$MYDIR:/mnt/x") assert result.startswith(real_dir + ":") + def test_var_expansion_in_destination(self, real_dir, monkeypatch): + """``$VAR`` in the destination expands too. + + ``env_passthrough`` forwards ``HOME`` with the host value, so a mount + that has to line up with the container's ``$HOME`` would otherwise have + to hardcode one host's home directory. + """ + monkeypatch.setenv("HOME", "/home/someuser") + result = _validate_extra_mount(f"{real_dir}:$HOME/.uipath:rw") + assert result == f"{real_dir}:/home/someuser/.uipath:rw" + + def test_home_expansion_in_destination(self, real_dir, monkeypatch): + """``~`` in the destination expands the same way the source's does.""" + monkeypatch.setenv("HOME", "/home/someuser") + result = _validate_extra_mount(f"{real_dir}:~/.uipath:ro") + assert result == f"{real_dir}:/home/someuser/.uipath:ro" + + def test_unset_var_destination_rejected_with_both_forms(self, real_dir): + """An unset var is left verbatim, so it must fail loudly, not mount blind.""" + with pytest.raises(ValueError, match="destination must be an absolute path"): + _validate_extra_mount(f"{real_dir}:$NO_SUCH_VAR_HERE/x:ro") + + def test_destination_var_expanding_to_reserved_is_rejected(self, real_dir, monkeypatch): + """The shadow check runs on the EXPANDED destination, not the raw one.""" + monkeypatch.setenv("SNEAKY", "/work") + with pytest.raises(ValueError, match="shadows a framework-owned mount"): + _validate_extra_mount(f"{real_dir}:$SNEAKY:ro") + def test_malformed_no_colon(self): with pytest.raises(ValueError, match="expected `src:dst"): _validate_extra_mount("just-one-token")