Skip to content

feat(py): environment allowlist and isolated launch for the worker - #251

Merged
jat255 merged 5 commits into
jat255/m6-f11e-exec-backendfrom
jat255/m6-vmqw-env-allowlist
Sep 10, 2026
Merged

jat255 merged 5 commits into
jat255/m6-f11e-exec-backendfrom
jat255/m6-vmqw-env-allowlist

Conversation

@jat255

@jat255 jat255 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

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. HOME and TMPDIR point 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: site imports usercustomize from the user site directory, and that code runs before the worker and can write straight back into os.environ.

interpreter_warning() covers what -I does not. Isolated mode drops the user site directory but not the global one, so a .pth file 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 usercustomize test 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.cfg is read from the unresolved executable path, because a virtual environment's bin/python is usually a symlink to the interpreter it was built from and following it reports on the wrong installation. The tests reach for sys._base_executable because 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, and uv run pyrefly check are clean in pkg-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.

@jat255
jat255 marked this pull request as draft September 2, 2026 00:49
@jat255 jat255 added this to the py-M6: code execution milestone Sep 2, 2026
@jat255 jat255 added py Affects the Python implementation needs-manual-review Agent-created work that needs a human review labels Sep 2, 2026
@jat255
jat255 force-pushed the jat255/m6-vmqw-env-allowlist branch from 94d102f to bab7dfd Compare September 10, 2026 20:46
@jat255
jat255 changed the base branch from main to jat255/m6-f11e-exec-backend September 10, 2026 20:46
@jat255
jat255 added this pull request to stack #349 September 10, 2026 20:48
@jat255
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
jat255 force-pushed the jat255/m6-vmqw-env-allowlist branch from e479cb6 to 09ed120 Compare September 10, 2026 20:48
@jat255
jat255 merged commit f131e7a into main Sep 10, 2026
8 checks passed
@jat255
jat255 deleted the jat255/m6-vmqw-env-allowlist branch September 10, 2026 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-manual-review Agent-created work that needs a human review py Affects the Python implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant