diff --git a/src/coder_eval/isolation/docker_runner.py b/src/coder_eval/isolation/docker_runner.py index 2331e4ec..31683d75 100644 --- a/src/coder_eval/isolation/docker_runner.py +++ b/src/coder_eval/isolation/docker_runner.py @@ -290,11 +290,14 @@ 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. Notes: + - Destinations are expanded too. A container path that has to match a + host-valued var (``$SKILLS_REPO_PATH``) would otherwise have to be + hardcoded per machine. - Mode is REQUIRED. Forgetting ``:ro`` is the single most common way to accidentally hand the container RW access to a host directory, so we make the author write it explicitly. @@ -312,26 +315,35 @@ 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.") + # Expanded before the absolute-path check: that is the point. + expanded_src = os.path.expandvars(os.path.expanduser(src)) + dst = os.path.expandvars(os.path.expanduser(raw_dst)) + # A variable whose value carries a ':' would add fields to the spec rebuilt + # at the bottom, silently moving the destination or widening the mode. + # The drive prefix is excluded: its colon is legitimate and already split off. + if ":" in dst or ":" in expanded_src[len(head) :]: + raise ValueError(f"Invalid extra_mounts entry {spec!r}: expansion introduced a ':' into a path.") if not dst.startswith("/"): - raise ValueError(f"Invalid extra_mounts entry {spec!r}: destination must be an absolute path.") + # expandvars leaves an unset var verbatim, so typos land here. + 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. + # Expanded form: a var could itself expand to a reserved path. 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..f6d65e72 100644 --- a/tests/test_docker_runner_mounts.py +++ b/tests/test_docker_runner_mounts.py @@ -108,6 +108,41 @@ 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.""" + 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 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.""" + 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.""" + monkeypatch.setenv("SNEAKY", "/work") + with pytest.raises(ValueError, match="shadows a framework-owned mount"): + _validate_extra_mount(f"{real_dir}:$SNEAKY:ro") + + def test_destination_var_carrying_a_colon_is_rejected(self, real_dir, monkeypatch): + """A ':' in an expanded value would add fields to the rebuilt spec.""" + monkeypatch.setenv("SNEAKY", "/mnt/x:rw") + with pytest.raises(ValueError, match="introduced a ':'"): + _validate_extra_mount(f"{real_dir}:$SNEAKY:ro") + + def test_source_var_carrying_a_colon_is_rejected(self, monkeypatch): + """Same guard on the source side, which is rebuilt the same way.""" + monkeypatch.setenv("SNEAKY", "/mnt/x:rw") + with pytest.raises(ValueError, match="introduced a ':'"): + _validate_extra_mount("$SNEAKY:/mnt/y:ro") + def test_malformed_no_colon(self): with pytest.raises(ValueError, match="expected `src:dst"): _validate_extra_mount("just-one-token")