feat(py): environment allowlist and isolated launch for the worker - #251
Draft
jat255 wants to merge 2 commits into
Draft
feat(py): environment allowlist and isolated launch for the worker#251jat255 wants to merge 2 commits into
jat255 wants to merge 2 commits into
Conversation
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.
jat255
marked this pull request as draft
September 2, 2026 00:49
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
1 participant
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.