Skip to content

Skill scripts inherit the full os.environ, while execute_python and run_command scrub it #2

Description

@saxman

The inconsistency

As of 0.24.0 there are three subprocess surfaces a model can drive, and they take two different positions on the parent environment.

Surface Child environment
execute_python fresh dict from _EXECUTE_PYTHON_ENV_ALLOWLIST (aimu/tools/builtin.py)
run_command fresh dict from _COMMAND_ENV_ALLOWLIST, plus names a host opts into via make_command_tool(env_passthrough=...)
{skill}__{stem} script tools the full os.environ

The third is aimu/skills/mcp.py:96:

resolved_env = None if env is None else {**os.environ, **env}

Both branches hand the child everything. When env is None, Popen inherits the parent environment wholesale; when it is not, the host's values are merged over os.environ rather than replacing it. run_script_file's docstring says so explicitly, so this is a deliberate choice rather than an oversight.

Why it is worth revisiting now

0.24.0 took a position while adding run_command: a subprocess the model can drive should not see the process environment, because a model that can run env or printenv can lift a credential into its own context, and from there anything else it holds can carry it off. That is why the allowlist exists and why the escape hatch is an explicit, host-named passthrough rather than a denylist.

That position is now held on two of the three surfaces. The third is arguably the easiest one to reach: authoring a script is gated in a typical host's approval list, but running an already-installed {skill}__{stem} tool generally is not, so the gate fires once at install time and never again.

Not filing this as a vulnerability

Running a script that is on disk is code the operator chose to install, which is the same disclosure execute_python and run_command both make about themselves. Nothing here is exploitable without that step. What seems worth resolving is the asymmetry: a library whose two newest subprocess surfaces scrub the environment, and whose third does not, is hard to reason about from the outside, and the difference is not stated anywhere a caller comparing them would look.

Options, roughly in increasing order of disruption

  1. Document the asymmetry where run_script_file already documents the merge, and stop there. Cheapest, and leaves callers to discover the difference by reading two files.
  2. Add an opt-in parameter to run_script_file / build_skills_server, defaulting to today's inherit, letting a host ask for an allowlist. No behavior change unless asked for.
  3. Flip the default to an allowlist plus passthrough, matching make_command_tool. Cleanest end state and a breaking change: a script relying on an ambient variable stops seeing it.

Worth noting for option 3 that the env parameter already exists precisely to carry host-provided context into a script, so the ambient inherit may be doing less work than it appears. A host that needs a specific variable in a script already has a supported way to put it there without inheriting everything.

Context

Found while reviewing the Kokua side of the 0.24.0 consumption, where the same question came up for the host's own config: the equivalent setting there is locked against the assistant's own writes for exactly this reason.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions