Skip to content

fix(tui_gateway): scrub credentials from shell.exec subprocess env - #60423

Open
Da7-Tech wants to merge 3 commits into
NousResearch:mainfrom
Da7-Tech:fix/tui-shell-exec-sanitize-env
Open

fix(tui_gateway): scrub credentials from shell.exec subprocess env#60423
Da7-Tech wants to merge 3 commits into
NousResearch:mainfrom
Da7-Tech:fix/tui-shell-exec-sanitize-env

Conversation

@Da7-Tech

@Da7-Tech Da7-Tech commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Prevents the TUI gateway shell.exec JSON-RPC handler from passing the gateway process's full environment to a user-requested shell command.

Before this change, the handler called subprocess.run(..., shell=True) without an explicit env, so the child inherited provider credentials, GitHub credentials, messaging tokens, infrastructure secrets, and Hermes-internal routing credentials from os.environ.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Tests

Changes Made

  • Passes the existing centralized hermes_subprocess_env(inherit_credentials=False) result to the shell.exec subprocess.
  • Applies both of the helper's existing stripping tiers:
    • always-stripped GitHub, messaging, gateway, and infrastructure credentials;
    • provider and tool credentials, because shell.exec does not need model credentials.
  • Preserves benign process variables required by ordinary commands, including PATH.
  • Adds handler-level regression tests that:
    • execute a real shell command and prove a provider key is unavailable;
    • exercise six representative credential classes through the real shell.exec handler, asserting on the captured child environment rather than the handler's 4000-character-truncated stdout;
    • demonstrate the truncation hazard itself: a secret rendered ahead of a large environment dump lands outside the stdout window (an output-based check would false-pass), while the environment assertion still catches it;
    • confirm a benign canary variable and PATH remain available;
    • inspect the subprocess.run call and prove an explicit sanitized environment is supplied.

The sibling quick-command shell=True route already used its own sanitized environment and is unchanged. The cli.exec route intentionally uses hermes_subprocess_env(inherit_credentials=True) because it can launch the model-driving Hermes CLI; Tier-1 and Hermes-internal secrets remain stripped there by the central helper.

How to Test

uv run --extra dev pytest -q \
  tests/tui_gateway/test_shell_exec_env_sanitized.py \
  tests/tui_gateway/test_protocol.py \
  tests/tools/test_hermes_subprocess_env.py \
  tests/tools/test_local_env_session_leak.py \
  tests/tools/test_local_env_windows_msys.py \
  tests/tools/test_local_env_blocklist.py \
  tests/tools/test_env_passthrough.py

uvx ruff check \
  tui_gateway/server.py \
  tests/tui_gateway/test_shell_exec_env_sanitized.py

python3 -m py_compile \
  tui_gateway/server.py \
  tests/tui_gateway/test_shell_exec_env_sanitized.py

python3 scripts/check-windows-footguns.py --diff origin/main

git diff --check

Verification Results

Three independent verification methods passed, with the final branch head at 8a01ebe4e:

  1. Behavioral and regression tests: all 4 focused handler tests passed on the final head. The 228-test relevant suite listed under "How to Test" — covering the TUI gateway, environment sanitization, session isolation, Windows environment handling, blocklists, and passthrough behavior — passed on the final head; the focused tests were rerun after the last unrelated upstream rebase.
  2. Static and structural audit: Ruff, Python compilation, the Windows-footgun checker, and whitespace validation passed. An AST inspection found exactly two shell=True calls in tui_gateway/server.py and confirmed that both pass an explicit sanitized env.
  3. Independent live canary: a standalone probe invoked the real shell.exec handler with synthetic provider, GitHub, messaging, infrastructure, auxiliary-model, and gateway-relay secrets. All six credential classes were absent from the child, while PATH and a benign canary variable were preserved.

The dangerous-command detector also remained active: the live verification used the allowed env command rather than bypassing or weakening command screening.

Scope

This PR deliberately reuses the repository's central subprocess-environment policy. It does not add another sanitizer, alter credential classification, change the quick-command route, or broaden shell execution permissions.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my own code
  • I have added tests that prove the fix is effective
  • Focused and expanded relevant tests pass
  • Static, structural, portability, and privacy checks pass

@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 sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P3 Low — cosmetic, nice to have labels Jul 7, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused hardening change. Current main confirms the gap: tui_gateway/server.py:14491-14494 runs shell.exec with shell=True and no explicit env, while ui-tui/src/app/useSubmission.ts:103 and :135 route user shell input through that RPC.

The proposed hermes_subprocess_env(inherit_credentials=False) call matches the central helper's documented behavior in tools/environments/local.py:471-522: it strips Tier-1 secrets and the provider/tool credential blocklist while retaining ordinary process variables. The other TUI shell=True route, quick commands, already supplies a sanitized environment at tui_gateway/server.py:11934-11948, so no equivalent unaddressed sibling path was found. The helper import is already present at tui_gateway/server.py:28, and the one-line hunk remains directly applicable despite line movement.

This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@Da7-Tech
Da7-Tech force-pushed the fix/tui-shell-exec-sanitize-env branch from 355fba9 to 801ab30 Compare July 16, 2026 06:05
@Da7-Tech

Copy link
Copy Markdown
Contributor Author

Rebased and revalidated on current main in 801ab3096. The review did not identify an additional code change, so the focused two-file scope is unchanged.

Validation completed:

  • 206 TUI gateway and subprocess-environment tests passed.
  • ruff, py_compile, the Windows-footgun checker, and git diff --check passed.
  • AST review found exactly two shell=True routes in tui_gateway/server.py; both now pass an explicit sanitized env.
  • An independent live canary checked six credential classes through the real shell.exec handler: all were absent in the child while PATH remained available.

GitHub reports the PR mergeable.

@Da7-Tech
Da7-Tech force-pushed the fix/tui-shell-exec-sanitize-env branch from 801ab30 to 6da95b9 Compare July 16, 2026 07:19
@Da7-Tech

Copy link
Copy Markdown
Contributor Author

Follow-up hardening is complete in 6da95b9d8. I corrected the test terminology, expanded the handler-level regression to cover provider, GitHub, messaging, infrastructure, auxiliary-model, and gateway-relay credential classes, and updated the PR description so it matches the actual two-file diff.\n\nRevalidation passed: 3 focused tests, 227 expanded relevant tests, Ruff, Python compilation, the Windows-footgun checker, git diff --check, an AST audit of both shell=True routes, an independent live shell.exec canary, and a privacy scan. All six synthetic credential classes were scrubbed while PATH and a benign variable remained available.

@Da7-Tech
Da7-Tech force-pushed the fix/tui-shell-exec-sanitize-env branch from 6da95b9 to 0e3e219 Compare July 16, 2026 08:19
@Da7-Tech

Da7-Tech commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Final follow-up is complete in 0e3e21991. The regression terminology now distinguishes the helper's two stripping tiers, and handler-level coverage exercises provider, GitHub, messaging, infrastructure, auxiliary-model, and gateway-relay credentials while proving benign variables and PATH survive. The PR description now matches the actual two-file diff and existing central helper.

Validation includes 3 focused tests on the final head, a 227-test relevant suite, Ruff, Python compilation, Windows-footgun and whitespace checks, an AST audit of both shell=True routes, an independent live shell.exec canary, and a privacy scan.

The shell.exec JSON-RPC handler ran subprocess.run(shell=True) without an
explicit env, so the spawned shell inherited the full gateway process
environment, including provider credentials and always-stripped
infrastructure or messaging secrets. Pass
hermes_subprocess_env(inherit_credentials=False) so both stripping tiers
apply while benign variables such as PATH remain available.

Add handler-level regression coverage for a provider key, a captured
explicit env, and a live env probe spanning provider, GitHub, messaging,
infrastructure, auxiliary-model, and gateway-relay secret classes.

Assert the multi-class coverage on the actual env dict passed to
subprocess.run rather than the handler's stdout, which is truncated to the
last 4000 characters: on a large-environment host a benign trailing variable
falls outside that window (false failure), and a leaked secret rendered
before it would pass unnoticed (false pass). A dedicated regression
constructs the leaked dump, shows the secret lands outside the 4000-char
window while stdout looks clean, then proves the env-dict check catches it;
the capture helper fails loudly if the handler ever stops passing an
explicit env.
@andrexibiza

Copy link
Copy Markdown
Contributor

Bound to the child-process credential-inheritance class under #83565 (#83565) — same bug class, different surface. tui_gateway shell.exec scrub; Wave D — merge lane established via #78036; currently dirty. The EPIC carries the live class table, dedup adjudication, and the dependency-driven merge order.

…nitize-env

Co-authored-by: Cursor <cursoragent@cursor.com>

# Conflicts:
#	tui_gateway/server.py
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:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

4 participants