Skip to content

fix(security): route tui_gateway shell.exec through the sanitized env builder - #78036

Open
andrexibiza wants to merge 2 commits into
NousResearch:mainfrom
andrexibiza:fix/security-tui-shell-exec-env
Open

fix(security): route tui_gateway shell.exec through the sanitized env builder#78036
andrexibiza wants to merge 2 commits into
NousResearch:mainfrom
andrexibiza:fix/security-tui-shell-exec-env

Conversation

@andrexibiza

@andrexibiza andrexibiza commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Related #60423

What changed and why

tui_gateway's registered shell.exec handler (@method("shell.exec") in tui_gateway/methods_tools.py) ran every approved quick command with subprocess.run(cmd, shell=True, ...) and no env= — the spawned shell inherited the gateway process's full os.environ, including provider API keys, gateway tokens, and *_PASSWORD values. The sibling cli.exec path in the same module already sanitizes via build_subprocess_env(); shell.exec now does the same, so a command run through it cannot read Hermes-managed credentials.

This covers the registered methods_tools handler; open PR #60423 addresses the duplicate shell.exec copy in tui_gateway/server.py — the two are complementary (different seams, same fix pattern).

How to test

  • python -m pytest tests/tui_gateway/test_shell_exec_sanitized_env.py -q → 1 passed
  • New regression test test_shell_exec_spawns_with_sanitized_env: plants EMAIL_PASSWORD in the parent env, runs the real shell.exec handler, asserts the child env is scrubbed.
  • Full tests/tui_gateway/ suite: 338 passed; the 3 failures are proven pre-existing on clean origin/main (identical on this Windows/MSYS box: signal.SIGPIPE-on-Windows and two timing tests).
  • git diff --check clean; check-windows-footguns.py clean on both changed files.

Platforms tested

Windows 11 (git-bash), Python 3.11, direct pytest (scripts/run_tests.sh is POSIX-only on native Windows).

Why this matters to users

Every command you approve and run through the desktop app's quick-command surface used to carry the gateway's API keys and database passwords into the shell. After this change, that shell gets the same sanitized environment as the agent's other exec paths — your secrets stay out of processes that never asked for them.

Part of #78207
Part of #78630

Part of #83565 — tui_gateway shell.exec through the sanitized builder; Wave D, merge lane via #60423.

… builder

The registered shell.exec handler (@method('shell.exec') in
methods_tools.py) spawned every approved quick command with
subprocess.run(cmd, shell=True) and no env=, inheriting the gateway's full
os.environ — provider keys, gateway tokens, *_PASSWORD values. The sibling
cli.exec path already sanitizes via build_subprocess_env(); shell.exec now
does the same. (The duplicate shell.exec copy in tui_gateway/server.py is
covered by open PR NousResearch#60423.)

Regression test: real shell.exec handler run with EMAIL_PASSWORD planted in
the parent env asserts the child env is scrubbed. Full tests/tui_gateway/
suite: 338 passed; the 3 failures reproduce identically on clean
origin/main on this Windows/MSYS box. diff-check and Windows-footgun lint
clean.

Signed-off-by: Andrex Ibiza, MBA <84248988+andrexibiza@users.noreply.github.com>
@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/tui Terminal UI (ui-tui/ + tui_gateway/) area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Aug 3, 2026
Signed-off-by: andrexibiza <84248988+andrexibiza@users.noreply.github.com>
@egilewski

Copy link
Copy Markdown
Contributor

looks mergeable

shell.exec now supplies a sanitized child environment through the shared builder, removing Hermes-managed credentials while preserving ordinary command variables. The current-main inheritance leak is closed, and no residual bypass was found in the reviewed TUI shell or quick-command paths.

Security evidence:

  • trust boundary: The TUI JSON-RPC shell.exec command runs in the gateway process, so child-process environment handling is the credential boundary; existing command-safety gates remain before execution.
  • source/sink/invariant: The handler passes the shared sanitized environment to its shell subprocess; the builder removes Hermes-managed secrets, applies profile/session environment controls, and preserves ordinary variables.
  • current-main reproduction: On current main, the shell subprocess inherited the gateway environment; the patched path no longer exposes the planted credential.
  • PR-head or patch-replay validation: The updated handler removes planted Hermes credentials while retaining PATH, and the focused regression and protocol checks pass.
  • positive/negative cases: Harmless commands still execute, while empty and dangerous commands remain rejected before the subprocess sink.
  • residual bypass search: Reviewed all related TUI shell-execution, quick-command, CLI-exec, and subprocess paths; no unpatched sibling TUI sink was found.
  • reviewer validation: Focused regression, protocol, Windows-wiring, and subprocess-environment tests passed.

Not checked:

  • broader TUI gateway suite
  • native Windows runtime

Signed: GPT-5.6-luna-max in Codex

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

Labels

area/auth Authentication, OAuth, credential pools comp/tui Terminal UI (ui-tui/ + tui_gateway/) P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants