From 5b7f4fb3ec43844b64f1b4b4e205f0123c7a68a5 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 18:19:16 -0600 Subject: [PATCH 1/3] feat(py): environment allowlist and isolated launch for the worker A subprocess inherits its parent's environment by default, and the parent holds API keys, session tokens and database URLs that model-written code has no business reading. The worker starts from an allowlist instead: PATH, LANG, LC_* and LD_LIBRARY_PATH, with HOME and TMPDIR pointed at the scratch directory so that code with no sandbox has nowhere interesting to go. An allowlist alone does not close it. site imports usercustomize from the user site directory, and that code runs before the worker and can write straight back into os.environ. -I is what stops it, which is why it sits with the allowlist rather than among optional hardening. The test plants a usercustomize.py and asserts both halves: that it does restore the variable without -I, and does not with it. Without that control the test would pass while proving nothing. Isolated mode drops the user site directory and not the global one, so a .pth file in a shared installation still runs first. interpreter_warning() says so, and stays quiet for a virtual environment that excludes system packages or for any interpreter inside a container image, where the only person who can write to site-packages is the image author. pyvenv.cfg is read from the unresolved executable path on purpose: a venv's bin/python is usually a symlink to the interpreter it was built from, and following it reports on that installation instead. --- pkg-py/src/commons/_execution/__init__.py | 1 + pkg-py/src/commons/_execution/_env.py | 99 ++++++++++++ pkg-py/tests/test_execution_env.py | 176 ++++++++++++++++++++++ 3 files changed, 276 insertions(+) create mode 100644 pkg-py/src/commons/_execution/__init__.py create mode 100644 pkg-py/src/commons/_execution/_env.py create mode 100644 pkg-py/tests/test_execution_env.py diff --git a/pkg-py/src/commons/_execution/__init__.py b/pkg-py/src/commons/_execution/__init__.py new file mode 100644 index 00000000..48df49b2 --- /dev/null +++ b/pkg-py/src/commons/_execution/__init__.py @@ -0,0 +1 @@ +"""Running model-written code in a worker process.""" diff --git a/pkg-py/src/commons/_execution/_env.py b/pkg-py/src/commons/_execution/_env.py new file mode 100644 index 00000000..026c0f89 --- /dev/null +++ b/pkg-py/src/commons/_execution/_env.py @@ -0,0 +1,99 @@ +"""The environment the worker process is given. + +A subprocess inherits its parent's environment by default, and the parent +holds API keys, session tokens, and database URLs that model-written code has +no business reading. The worker therefore starts from an allowlist rather than +from what happens to be set. +""" + +from __future__ import annotations + +import os +import sys +from collections.abc import Sequence +from pathlib import Path + +__all__ = ["interpreter_warning", "worker_command", "worker_env"] + +# Enough to find an interpreter and produce readable text, and nothing else. +_KEEP = ("PATH", "LANG", "LD_LIBRARY_PATH") + + +def worker_env(scratch_dir: str) -> dict[str, str]: + """Build the worker's environment from an allowlist of the parent's.""" + env = {name: os.environ[name] for name in _KEEP if name in os.environ} + env.update( + {name: value for name, value in os.environ.items() if name.startswith("LC_")} + ) + # Point the worker's idea of home and scratch space at a directory it is + # allowed to have: with no sandbox engaged these are all that keep it out + # of the user's dot files. + env["HOME"] = scratch_dir + env["TMPDIR"] = scratch_dir + return env + + +def worker_command( + script: str, *args: str, executable: str = sys.executable +) -> Sequence[str]: + """The argv that launches the worker. + + ``-I`` is not a hardening extra to be traded off; it belongs with the + allowlist. Without it ``site`` imports ``usercustomize`` from the user site + directory, which runs before the worker and can write excluded variables + back into ``os.environ``. ``-u`` keeps the protocol channel unbuffered. + + ``executable`` defaults to the interpreter running commons, which is what + makes the worker's installed packages match the host's. + """ + return [executable, "-I", "-u", script, *args] + + +# Files a container runtime leaves behind, used to tell "this system Python is +# the image author's" from "this system Python is shared with other people". +_CONTAINER_MARKERS = ("/.dockerenv", "/run/.containerenv") + + +def _venv_includes_system_site(executable: str) -> bool | None: + """Read ``pyvenv.cfg`` for ``executable``; ``None`` if it is not a venv.""" + # Deliberately not resolved: a virtual environment's bin/python is usually + # a symlink to the interpreter it was built from, and following it lands on + # that installation rather than on the environment being asked about. + config = Path(executable).absolute().parent.parent / "pyvenv.cfg" + try: + text = config.read_text() + except OSError: + return None + for line in text.splitlines(): + key, sep, value = line.partition("=") + if sep and key.strip() == "include-system-site-packages": + return value.strip().lower() == "true" + return False + + +def interpreter_warning(executable: str = sys.executable) -> str | None: + """Say why this interpreter's startup hooks are not known, or ``None``. + + Isolated mode drops the user site directory but not the global one, so a + ``.pth`` file in a shared installation's site-packages still runs before + the worker does. Whoever can write there can therefore run code inside it. + That is fine when the only person who can write there is the image author, + and not fine on a machine shared with other people. + """ + includes_system_site = _venv_includes_system_site(executable) + if includes_system_site is False: + return None + if any(os.path.exists(marker) for marker in _CONTAINER_MARKERS): + return None + reason = ( + "is a virtual environment built with --system-site-packages" + if includes_system_site + else "is not a virtual environment" + ) + return ( + f"the code execution worker would run on {executable}, which {reason}, " + "so commons cannot tell what runs at its startup. Anyone who can write " + "to its site-packages can run code inside the worker. Use a virtual " + "environment that excludes system packages, or a container image whose " + "contents you control." + ) diff --git a/pkg-py/tests/test_execution_env.py b/pkg-py/tests/test_execution_env.py new file mode 100644 index 00000000..b94a1334 --- /dev/null +++ b/pkg-py/tests/test_execution_env.py @@ -0,0 +1,176 @@ +"""What the worker process is allowed to inherit, and what it must not. + +These launch real interpreters. The property under test is what a child +process can actually see, and an assertion about the contents of a dictionary +would not have caught the failure this guards against. +""" + +from __future__ import annotations + +import os +import pathlib +import subprocess +import sys + +import pytest + +from commons._execution._env import ( + interpreter_warning, + worker_command, + worker_env, +) + +READ_SECRET = "import os; print(os.environ.get('COMMONS_TEST_SECRET'))" + + +def test_a_variable_outside_the_allowlist_does_not_reach_the_worker( + tmp_path, monkeypatch +) -> None: + monkeypatch.setenv("COMMONS_TEST_SECRET", "sk-not-a-real-key") + + result = subprocess.run( + [sys.executable, "-c", READ_SECRET], + env=worker_env(str(tmp_path)), + capture_output=True, + text=True, + check=True, + ) + + assert result.stdout.strip() == "None" + + +def test_home_and_tmpdir_point_at_the_scratch_directory(tmp_path) -> None: + # Left pointing at the real home, the worker could read the user's dot + # files and write anywhere they can write. Both are the scratch directory + # so that code with no sandbox still has nowhere interesting to go. + result = subprocess.run( + [ + sys.executable, + "-c", + "import os; print(os.environ['HOME'], os.environ['TMPDIR'])", + ], + env=worker_env(str(tmp_path)), + capture_output=True, + text=True, + check=True, + ) + + assert result.stdout.split() == [str(tmp_path), str(tmp_path)] + + +def _plant_usercustomize(executable: str, env: dict[str, str]) -> bool: + """Put code in the interpreter's user site directory that restores the secret. + + Returns whether the interpreter would import it at all, so the test can + say it was skipped rather than pass without having tried anything. + """ + probe = subprocess.run( + [ + executable, + "-c", + "import site; print(site.ENABLE_USER_SITE); print(site.getusersitepackages())", + ], + env=env, + capture_output=True, + text=True, + check=True, + ) + enabled, _, directory = probe.stdout.partition("\n") + if enabled.strip() != "True": + return False + target = pathlib.Path(directory.strip()) + target.mkdir(parents=True, exist_ok=True) + (target / "usercustomize.py").write_text( + "import os\nos.environ['COMMONS_TEST_SECRET'] = 'restored'\n" + ) + return True + + +def test_startup_hooks_cannot_put_an_excluded_variable_back( + tmp_path, monkeypatch +) -> None: + # An allowlist alone is not enough. `site` imports `usercustomize` from the + # user site directory before the worker runs, and that code can write + # straight back into os.environ. Isolated mode is what closes it. + monkeypatch.setenv("COMMONS_TEST_SECRET", "sk-not-a-real-key") + # A virtual environment turns the user site directory off, so the hole + # only opens on an interpreter like the one this venv was built from. + # That is exactly the interpreter the note warns about running on. + executable = getattr(sys, "_base_executable", None) + if executable is None: + pytest.skip("no non-virtual-environment interpreter to test against") + env = worker_env(str(tmp_path)) + script = tmp_path / "worker.py" + script.write_text(READ_SECRET + "\n") + if not _plant_usercustomize(executable, env): + pytest.skip("this interpreter does not import usercustomize") + + unguarded = subprocess.run( + [executable, str(script)], env=env, capture_output=True, text=True, check=True + ) + guarded = subprocess.run( + worker_command(str(script), executable=executable), + env=env, + capture_output=True, + text=True, + check=True, + ) + + # The control matters: without it a passing test proves nothing about -I. + assert unguarded.stdout.strip() == "restored" + assert guarded.stdout.strip() == "None" + + +def test_a_virtual_environment_without_system_packages_is_accepted() -> None: + # The suite runs in exactly the kind of interpreter the design asks for. + assert interpreter_warning() is None + + +def test_a_virtual_environment_that_includes_system_packages_is_flagged( + tmp_path, +) -> None: + # Isolated mode drops the user site directory, not the global one, so a + # venv wired to the system site-packages is still exposed to anything + # installed there. + venv = tmp_path / "shared" + subprocess.run( + [ + sys.executable, + "-m", + "venv", + "--system-site-packages", + "--without-pip", + str(venv), + ], + check=True, + capture_output=True, + ) + + warning = interpreter_warning(str(venv / "bin" / "python")) + + assert warning is not None + assert "system" in warning + + +def test_an_interpreter_outside_any_virtual_environment_is_flagged() -> None: + # The interpreter this virtual environment was built from, which is not + # itself inside one. + executable = getattr(sys, "_base_executable", None) + if executable is None: + pytest.skip("no non-virtual-environment interpreter to test against") + + warning = interpreter_warning(executable) + + assert warning is not None + assert executable in warning + + +def test_the_worker_keeps_what_it_needs_to_run(tmp_path, monkeypatch) -> None: + # An allowlist that is too narrow fails differently but just as badly: the + # worker cannot find an interpreter or mangles non-ASCII output. + monkeypatch.setenv("LC_ALL", "en_US.UTF-8") + + env = worker_env(str(tmp_path)) + + assert env["PATH"] == os.environ["PATH"] + assert env["LC_ALL"] == "en_US.UTF-8" From 6e30500b9a133d52fcba2a1ae9cddb2ee4e1c740 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Tue, 1 Sep 2026 18:21:18 -0600 Subject: [PATCH 2/3] fix(py): make the container check explicit rather than ambient The two tests asserting that an interpreter is flagged would have failed inside Docker or Podman, where interpreter_warning() deliberately stays quiet. They now say which case they are testing. interpreter_warning() takes containerised explicitly, still probing for the marker files when it is not given. A caller that knows how it is deployed should not have to let commons guess, and the suppression branch now has a test of its own instead of only firing where nobody runs the suite. Found by review of 5b7f4fb. --- pkg-py/src/commons/_execution/_env.py | 16 ++++++++++++---- pkg-py/tests/test_execution_env.py | 13 +++++++++++-- 2 files changed, 23 insertions(+), 6 deletions(-) diff --git a/pkg-py/src/commons/_execution/_env.py b/pkg-py/src/commons/_execution/_env.py index 026c0f89..7078de6b 100644 --- a/pkg-py/src/commons/_execution/_env.py +++ b/pkg-py/src/commons/_execution/_env.py @@ -13,7 +13,7 @@ from collections.abc import Sequence from pathlib import Path -__all__ = ["interpreter_warning", "worker_command", "worker_env"] +__all__ = ["in_container", "interpreter_warning", "worker_command", "worker_env"] # Enough to find an interpreter and produce readable text, and nothing else. _KEEP = ("PATH", "LANG", "LD_LIBRARY_PATH") @@ -71,19 +71,27 @@ def _venv_includes_system_site(executable: str) -> bool | None: return False -def interpreter_warning(executable: str = sys.executable) -> str | None: +def in_container() -> bool: + """Whether this process looks like it is running inside an image.""" + return any(os.path.exists(marker) for marker in _CONTAINER_MARKERS) + + +def interpreter_warning( + executable: str = sys.executable, *, containerised: bool | None = None +) -> str | None: """Say why this interpreter's startup hooks are not known, or ``None``. Isolated mode drops the user site directory but not the global one, so a ``.pth`` file in a shared installation's site-packages still runs before the worker does. Whoever can write there can therefore run code inside it. That is fine when the only person who can write there is the image author, - and not fine on a machine shared with other people. + and not fine on a machine shared with other people. Pass ``containerised`` + to state which case this is rather than let it be inferred. """ includes_system_site = _venv_includes_system_site(executable) if includes_system_site is False: return None - if any(os.path.exists(marker) for marker in _CONTAINER_MARKERS): + if in_container() if containerised is None else containerised: return None reason = ( "is a virtual environment built with --system-site-packages" diff --git a/pkg-py/tests/test_execution_env.py b/pkg-py/tests/test_execution_env.py index b94a1334..6efe454d 100644 --- a/pkg-py/tests/test_execution_env.py +++ b/pkg-py/tests/test_execution_env.py @@ -146,7 +146,7 @@ def test_a_virtual_environment_that_includes_system_packages_is_flagged( capture_output=True, ) - warning = interpreter_warning(str(venv / "bin" / "python")) + warning = interpreter_warning(str(venv / "bin" / "python"), containerised=False) assert warning is not None assert "system" in warning @@ -159,7 +159,7 @@ def test_an_interpreter_outside_any_virtual_environment_is_flagged() -> None: if executable is None: pytest.skip("no non-virtual-environment interpreter to test against") - warning = interpreter_warning(executable) + warning = interpreter_warning(executable, containerised=False) assert warning is not None assert executable in warning @@ -174,3 +174,12 @@ def test_the_worker_keeps_what_it_needs_to_run(tmp_path, monkeypatch) -> None: assert env["PATH"] == os.environ["PATH"] assert env["LC_ALL"] == "en_US.UTF-8" + + +def test_a_container_image_is_accepted_even_outside_a_virtual_environment() -> None: + # In an image, the only person who can write to site-packages is whoever + # built it, so a bare system interpreter is the expected arrangement + # rather than a shared machine's. + executable = getattr(sys, "_base_executable", None) or sys.executable + + assert interpreter_warning(executable, containerised=True) is None From 2807bbf10958a7533b713c5505b6cfd80cda67e0 Mon Sep 17 00:00:00 2001 From: Josh Taillon Date: Mon, 7 Sep 2026 16:58:44 -0600 Subject: [PATCH 3/3] fix(py): harden pyvenv.cfg reading and pin down the env contract Review fixes: - _venv_includes_system_site catches UnicodeDecodeError alongside OSError, so a corrupted or locale-encoded pyvenv.cfg degrades to "unknown" instead of raising out of an advisory check. - A bare interpreter name is resolved through PATH, so the warning inspects the same interpreter a launch would find rather than one relative to the caller's working directory. - Tests now pin worker_command's argv (isolated, unbuffered, argument order) in a test that cannot skip, cover in_container and the containerised inference path in both directions, assert LANG and LD_LIBRARY_PATH survive the allowlist, and exercise the pyvenv.cfg parser against synthetic configs instead of only real venv output. --- pkg-py/src/commons/_execution/_env.py | 12 ++- pkg-py/tests/test_execution_env.py | 108 ++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 1 deletion(-) diff --git a/pkg-py/src/commons/_execution/_env.py b/pkg-py/src/commons/_execution/_env.py index 7078de6b..86f7db47 100644 --- a/pkg-py/src/commons/_execution/_env.py +++ b/pkg-py/src/commons/_execution/_env.py @@ -9,6 +9,7 @@ from __future__ import annotations import os +import shutil import sys from collections.abc import Sequence from pathlib import Path @@ -56,13 +57,22 @@ def worker_command( def _venv_includes_system_site(executable: str) -> bool | None: """Read ``pyvenv.cfg`` for ``executable``; ``None`` if it is not a venv.""" + # A bare name is looked up on PATH, so the warning inspects the same + # interpreter a launch would find rather than one relative to the + # caller's working directory. + if not os.path.isabs(executable): + executable = shutil.which(executable) or executable + if not os.path.isabs(executable): + return None # Deliberately not resolved: a virtual environment's bin/python is usually # a symlink to the interpreter it was built from, and following it lands on # that installation rather than on the environment being asked about. config = Path(executable).absolute().parent.parent / "pyvenv.cfg" try: text = config.read_text() - except OSError: + except (OSError, UnicodeDecodeError): + # A config that cannot be read or decoded means the interpreter's + # startup hooks are unknown, not that this check should fail. return None for line in text.splitlines(): key, sep, value = line.partition("=") diff --git a/pkg-py/tests/test_execution_env.py b/pkg-py/tests/test_execution_env.py index 6efe454d..83dbcd2e 100644 --- a/pkg-py/tests/test_execution_env.py +++ b/pkg-py/tests/test_execution_env.py @@ -14,7 +14,9 @@ import pytest +from commons._execution import _env from commons._execution._env import ( + in_container, interpreter_warning, worker_command, worker_env, @@ -168,12 +170,16 @@ def test_an_interpreter_outside_any_virtual_environment_is_flagged() -> None: def test_the_worker_keeps_what_it_needs_to_run(tmp_path, monkeypatch) -> None: # An allowlist that is too narrow fails differently but just as badly: the # worker cannot find an interpreter or mangles non-ASCII output. + monkeypatch.setenv("LANG", "en_US.UTF-8") monkeypatch.setenv("LC_ALL", "en_US.UTF-8") + monkeypatch.setenv("LD_LIBRARY_PATH", "/opt/lib") env = worker_env(str(tmp_path)) assert env["PATH"] == os.environ["PATH"] + assert env["LANG"] == "en_US.UTF-8" assert env["LC_ALL"] == "en_US.UTF-8" + assert env["LD_LIBRARY_PATH"] == "/opt/lib" def test_a_container_image_is_accepted_even_outside_a_virtual_environment() -> None: @@ -183,3 +189,105 @@ def test_a_container_image_is_accepted_even_outside_a_virtual_environment() -> N executable = getattr(sys, "_base_executable", None) or sys.executable assert interpreter_warning(executable, containerised=True) is None + + +def test_the_launch_command_runs_isolated_and_unbuffered() -> None: + # The behavioural tests prove -I through a planted usercustomize, but they + # skip when no suitable interpreter exists, and nothing behavioural would + # catch a dropped -u or a reordered argv. This one cannot skip. + assert list(worker_command("worker.py", "one", "two")) == [ + sys.executable, + "-I", + "-u", + "worker.py", + "one", + "two", + ] + + +def test_the_launch_command_uses_the_given_interpreter() -> None: + assert list(worker_command("worker.py", executable="/opt/py/bin/python")) == [ + "/opt/py/bin/python", + "-I", + "-u", + "worker.py", + ] + + +def test_container_detection_follows_the_marker_files(tmp_path, monkeypatch) -> None: + marker = tmp_path / ".dockerenv" + monkeypatch.setattr(_env, "_CONTAINER_MARKERS", (str(marker),)) + + assert in_container() is False + marker.touch() + assert in_container() is True + + +def test_an_inferred_container_suppresses_the_warning(tmp_path, monkeypatch) -> None: + # With containerised left to inference, the marker files are what stand + # between a shared machine and a silenced warning, so both outcomes need + # to be reachable from the default call. + executable = getattr(sys, "_base_executable", None) + if executable is None: + pytest.skip("no non-virtual-environment interpreter to test against") + marker = tmp_path / ".containerenv" + monkeypatch.setattr(_env, "_CONTAINER_MARKERS", (str(marker),)) + + assert interpreter_warning(executable) is not None + marker.touch() + assert interpreter_warning(executable) is None + + +def test_a_pyvenv_cfg_that_is_not_text_means_unknown(tmp_path) -> None: + # A corrupted or locale-encoded config must degrade to "startup hooks + # unknown" — a warning — not raise out of an advisory check. + (tmp_path / "pyvenv.cfg").write_bytes(b"home = /usr\n\xff\xfe not text\n") + + warning = interpreter_warning(str(tmp_path / "bin" / "python"), containerised=False) + + assert warning is not None + + +@pytest.mark.parametrize( + ("content", "expected"), + [ + ("include-system-site-packages = true\n", True), + ("include-system-site-packages=true\n", True), + ("include-system-site-packages = TRUE\n", True), + ("include-system-site-packages = false\n", False), + ("include-system-site-packages = yes\n", False), + ("home = /usr/local\n", False), + ("a line without a separator\ninclude-system-site-packages = true\n", True), + ], +) +def test_pyvenv_cfg_parsing(tmp_path, content, expected) -> None: + # Real venvs are well-behaved; these pin the contract for configs written + # by hand or by other tools. + (tmp_path / "pyvenv.cfg").write_text(content) + + assert _env._venv_includes_system_site(str(tmp_path / "bin" / "python")) is expected + + +def test_a_missing_pyvenv_cfg_means_not_a_virtual_environment(tmp_path) -> None: + assert _env._venv_includes_system_site(str(tmp_path / "bin" / "python")) is None + + +def test_a_bare_interpreter_name_is_found_on_path(tmp_path, monkeypatch) -> None: + # The warning must inspect the interpreter a launch would actually find, + # not a path relative to the caller's working directory. + binary = tmp_path / "bin" / "python" + binary.parent.mkdir() + binary.touch() + binary.chmod(0o755) + (tmp_path / "pyvenv.cfg").write_text("include-system-site-packages = false\n") + monkeypatch.setenv("PATH", str(tmp_path / "bin")) + + assert interpreter_warning("python", containerised=False) is None + + +def test_an_unfindable_interpreter_is_treated_as_unknown(tmp_path, monkeypatch) -> None: + monkeypatch.setenv("PATH", str(tmp_path)) + + warning = interpreter_warning("no-such-interpreter", containerised=False) + + assert warning is not None