feat(py): environment allowlist and isolated launch for the worker - #251
Merged
jat255 merged 5 commits intoSep 10, 2026
Merged
Conversation
jat255
marked this pull request as draft
September 2, 2026 00:49
jat255
force-pushed
the
jat255/m6-vmqw-env-allowlist
branch
from
September 10, 2026 20:46
94d102f to
bab7dfd
Compare
jat255
added this pull request to stack #349
September 10, 2026 20:48
jat255
marked this pull request as ready for review
September 10, 2026 20:48
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.
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.
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.
- Keep DYLD_LIBRARY_PATH alongside LD_LIBRARY_PATH; they are the macOS and Linux spellings of the same dynamic-linker search path, and the omission was accidental. - Refuse a non-absolute scratch_dir in worker_env() rather than letting it silently weaken the HOME/TMPDIR guarantee. - Say "or its pyvenv.cfg cannot be read" when the venv probe comes back unknown, instead of claiming the interpreter is not a virtual environment. - State in worker_command() that -I does not reach the global site-packages, and point at interpreter_warning() for that residual. - Pin the LC_* prefix match with a second variable, cover the empty parent environment, parametrize the container-marker tests over which marker is present, and test the scratch_dir refusal.
jat255
force-pushed
the
jat255/m6-vmqw-env-allowlist
branch
from
September 10, 2026 20:48
e479cb6 to
09ed120
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The environment the code execution worker is allowed to inherit, and the launch flag that stops the host putting back what the allowlist removed.
Summary
worker_env()builds the worker's environment from an allowlist (PATH,LANG,LC_*,LD_LIBRARY_PATH) rather than from whatever the parent happens to hold, since the parent holds API keys, session tokens, and database URLs.HOMEandTMPDIRpoint at the scratch directory, so code running before any sandbox exists still has nowhere interesting to go.worker_command()adds-I. That is not optional hardening:siteimportsusercustomizefrom the user site directory, and that code runs before the worker and can write straight back intoos.environ.interpreter_warning()covers what-Idoes not. Isolated mode drops the user site directory but not the global one, so a.pthfile in a shared installation still runs first. It stays quiet for a virtual environment that excludes system packages, and for a container image where the only person who can write to site-packages is whoever built it.Nothing consumes any of this yet. The worker and the driver are separate units of work.
Review notes
The
usercustomizetest asserts both halves: that the planted module does restore the variable without-I, and does not with it. Without that control it would pass while proving nothing, which is the failure mode worth guarding against here.Two details that look like mistakes and are not, both explained in comments at the code.
pyvenv.cfgis read from the unresolved executable path, because a virtual environment'sbin/pythonis usually a symlink to the interpreter it was built from and following it reports on the wrong installation. The tests reach forsys._base_executablebecause a virtual environment turns the user site directory off, so the hole only opens on the kind of interpreter the design warns about running on.Testing
uv run pytest(108 passed),uv run ruff check, anduv run pyrefly checkare clean inpkg-py/. Both commits were reviewed by roborev. One finding came back, that the two warning assertions would fail inside a container, and the fix went slightly wider than suggested: the container check became an explicit parameter, which also gave the suppression branch a test.