Skip to content

fix(terminal): create session snapshot owner-only to stop secret leak - #41278

Closed
agentswe wants to merge 1 commit into
NousResearch:mainfrom
agentswe:fix/terminal-snapshot-permissions
Closed

fix(terminal): create session snapshot owner-only to stop secret leak#41278
agentswe wants to merge 1 commit into
NousResearch:mainfrom
agentswe:fix/terminal-snapshot-permissions

Conversation

@agentswe

@agentswe agentswe commented Jun 7, 2026

Copy link
Copy Markdown

What does this PR do?

The local terminal backend captures a login-shell snapshot via
export -p > /tmp/hermes-snap-<id>.sh so env vars persist across the
spawn-per-call model. export -p dumps every exported variable, and the
snapshot's bash subprocess inherits the host environment: the provider
blocklist strips model API keys, but SUDO_PASSWORD is not blocklisted and
the general AWS credential chain (AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY
/ AWS_SESSION_TOKEN) is intentionally passed through, plus any user-set
secrets. The file was created with the shell's inherited default umask
(typically 022 -> mode 0644), so on a shared/multi-user host any other
local user could read it and harvest those secrets for the whole
(potentially long-lived gateway) session. The same dump is rewritten after
every command, and the file only goes away at cleanup().

I went with umask 077 rather than a Python-side chmod because the
snapshot is written by the shell on the remote side for the container/ssh
backends too — chmod in Python only reaches the local backend, whereas the
umask travels with the bootstrap script and fixes every backend uniformly.
Setting it once at the top of the init bootstrap covers first creation; the
per-command re-dump is wrapped in a (umask 077; ...) subshell so a
snapshot recreated mid-session can't regress to world-readable while
keeping the umask change off the user's own command. The cwd marker file
gets the same treatment for consistency.

Related Issue

N/A

Type of Change

  • 🔒 Security fix

Changes Made

  • tools/environments/base.py: prepend umask 077 to the init_session
    bootstrap so the snapshot and cwd files are created mode 0600; wrap the
    per-command snapshot/cwd re-dump in _wrap_command in a (umask 077; ...)
    subshell so the restriction survives mid-session recreation without
    affecting the user's command.
  • tests/tools/test_snapshot_permissions.py: new tests asserting the
    wrapper string is umask-protected (and that the umask does not leak into
    the user command), plus end-to-end checks that the local backend's
    snapshot lands owner-only and stays that way after a command.
  • scripts/release.py: add my noreply email to AUTHOR_MAP.

How to Test

  1. pytest tests/tools/test_snapshot_permissions.py tests/tools/test_base_environment.py -q
  2. To see the original exposure, revert the umask 077 additions in
    base.py and re-run: the file-mode tests fail with the snapshot at
    0o644 (group/other readable). With the fix they pass at 0o600.
  3. Manual: run a terminal command on the local backend and check
    stat -f '%Lp' /tmp/hermes-snap-*.sh (macOS) / stat -c '%a' (Linux)
    reports 600.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix
  • I've run the relevant tests and they pass
  • I've added tests for my changes
  • I've tested on my platform: macOS 15 (Darwin 25.5)

Documentation & Housekeeping

  • I've updated relevant documentation (docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture — N/A
  • I've considered cross-platform impact (Windows, macOS) — the fix is a
    POSIX umask in the shell bootstrap; harmless no-op on the Windows/Git Bash
    path, and the on-disk mode tests are skipped on Windows
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

## What does this PR do?

The local terminal backend captures a login-shell snapshot via
`export -p > /tmp/hermes-snap-<id>.sh` so env vars persist across the
spawn-per-call model. `export -p` dumps every exported variable, and the
snapshot's bash subprocess inherits the host environment: the provider
blocklist strips model API keys, but SUDO_PASSWORD is not blocklisted and
the general AWS credential chain (AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY
/ AWS_SESSION_TOKEN) is intentionally passed through, plus any user-set
secrets. The file was created with the shell's inherited default umask
(typically 022 -> mode 0644), so on a shared/multi-user host any other
local user could read it and harvest those secrets for the whole
(potentially long-lived gateway) session. The same dump is rewritten after
every command, and the file only goes away at cleanup().

I went with `umask 077` rather than a Python-side chmod because the
snapshot is written by the shell on the remote side for the container/ssh
backends too — chmod in Python only reaches the local backend, whereas the
umask travels with the bootstrap script and fixes every backend uniformly.
Setting it once at the top of the init bootstrap covers first creation; the
per-command re-dump is wrapped in a `(umask 077; ...)` subshell so a
snapshot recreated mid-session can't regress to world-readable while
keeping the umask change off the user's own command. The cwd marker file
gets the same treatment for consistency.

## Related Issue

N/A

## Type of Change

- [x] 🔒 Security fix

## Changes Made

- `tools/environments/base.py`: prepend `umask 077` to the `init_session`
  bootstrap so the snapshot and cwd files are created mode 0600; wrap the
  per-command snapshot/cwd re-dump in `_wrap_command` in a `(umask 077; ...)`
  subshell so the restriction survives mid-session recreation without
  affecting the user's command.
- `tests/tools/test_snapshot_permissions.py`: new tests asserting the
  wrapper string is umask-protected (and that the umask does not leak into
  the user command), plus end-to-end checks that the local backend's
  snapshot lands owner-only and stays that way after a command.
- `scripts/release.py`: add my noreply email to `AUTHOR_MAP`.

## How to Test

1. `pytest tests/tools/test_snapshot_permissions.py tests/tools/test_base_environment.py -q`
2. To see the original exposure, revert the `umask 077` additions in
   `base.py` and re-run: the file-mode tests fail with the snapshot at
   `0o644` (group/other readable). With the fix they pass at `0o600`.
3. Manual: run a terminal command on the local backend and check
   `stat -f '%Lp' /tmp/hermes-snap-*.sh` (macOS) / `stat -c '%a'` (Linux)
   reports `600`.

## Checklist

### Code

- [x] I've read the Contributing Guide
- [x] My commit messages follow Conventional Commits
- [x] I searched for existing PRs to make sure this isn't a duplicate
- [x] My PR contains only changes related to this fix
- [x] I've run the relevant tests and they pass
- [x] I've added tests for my changes
- [x] I've tested on my platform: macOS 15 (Darwin 25.5)

### Documentation & Housekeeping

- [x] I've updated relevant documentation (docstrings) — or N/A
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture — N/A
- [x] I've considered cross-platform impact (Windows, macOS) — the fix is a
  POSIX umask in the shell bootstrap; harmless no-op on the Windows/Git Bash
  path, and the on-disk mode tests are skipped on Windows
- [x] I've updated tool descriptions/schemas if I changed tool behavior — N/A

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary (PR #41278)

Verdict: Approved

Looks Good

  • Security fix is well-scoped: adds umask 077 before snapshot/cwd file creation to prevent world-readable env dumps leaking secrets (SUDO_PASSWORD, AWS creds, etc.)
  • Tests pin both the wrapper-string contract (_wrap_command produces (umask 077; ...) subshells) and the actual on-disk file mode via LocalEnvironment
  • Tests verify umask is scoped to the subshell and does not leak into user commands
  • Covers edge cases: snapshot re-dump mid-session, file mode after command execution
  • AUTHOR_MAP entry added for release script

Reviewed by Hermes Agent

@daimon-nous daimon-nous Bot added type/security Security vulnerability or hardening tool/terminal Terminal execution and process management P1 High — major feature broken, no workaround labels Jun 7, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Verification — clean security fix

Solid fix for a real secret-exposure vector:

  1. Problem: init_session() runs export -p > /tmp/hermes-snap-*.sh to capture the login shell environment. export -p dumps every exported env var — including secrets not in the provider blocklist (SUDO_PASSWORD, AWS credential chain, user-set secrets). With the default umask (022 → mode 0644) on a shared host, any local user can read the snapshot and harvest secrets for the entire gateway session lifetime.

  2. Fix: Forces umask 077 in three places:

    • Top of init_session() bootstrap (one-shot)
    • _wrap_command() re-dump subshell (umask 077; export -p > …) — scoped to subshell so user commands don't inherit restrictive umask
    • _wrap_command() cwd-file write (umask 077; pwd -P > …)
  3. Subshell isolation: The umask in _wrap_command is wrapped in () so it doesn't leak into the user's eval command above it — correct.

  4. Tests: TestWrapCommandUmask verifies the wrapper string contract (umask present in snapshot re-dump, absent from user command). TestSnapshotFileMode verifies actual on-disk file modes with a forced permissive umask (os.umask(0o022)) to ensure the fix is exercised.

No issues found. LGTM.

@egilewski egilewski left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: needs rework before merge.

I checked this against current GitHub main f8adefdebf082047527a5fe628c0a4c6f3906a57, PR base e2cc24e3311da5575f9e0df256e14e62ff39dab2, and PR head f0ea72e5cdc6467890bfff02de856bf04cdfe371.

The security change itself validates. With a permissive 0o022 umask and synthetic AWS credentials, current main created the terminal snapshot and cwd files as 0644 while the synthetic secret was present in the snapshot. Replaying the PR's tools/environments/base.py plus tests/tools/test_snapshot_permissions.py hunks onto current main produced 0600 snapshot/cwd files on initial creation and kept the recreated snapshot at 0600 after a command, while the user command's own umask output stayed unchanged.

Validation:

  • git apply --check --exclude=scripts/release.py pr.diff passed for the security files.
  • git apply --check pr.diff failed at scripts/release.py:46, and GitHub reports mergeable=false / mergeable_state=dirty.
  • git merge-tree --write-tree f8adefdebf082047527a5fe628c0a4c6f3906a57 refs/remotes/upstream/pr/41278 exited nonzero with broad stale-branch conflicts.
  • python -B -m pytest -o addopts='' -p no:cacheprovider tests/tools/test_snapshot_permissions.py tests/tools/test_base_environment.py -q passed on the current-main replay (22 passed).
  • python -B -m compileall -q tools/environments/base.py tests/tools/test_snapshot_permissions.py passed.
  • coderabbit review --plain --base f8adefdebf082047527a5fe628c0a4c6f3906a57 --type uncommitted returned No findings.

Please rebase and resolve/drop the stale scripts/release.py author-map hunk. The security patch looks good once replayed onto current main, but the submitted branch is not mergeable as-is.

Signed: GPT-5.5-xhigh in Codex

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jun 21, 2026
@teknium1

teknium1 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Thanks for the careful writeup and the tests, @agentswe — the analysis of the exposure (export -p dumping non-blocklisted secrets to a world-readable snapshot in a shared temp dir) is accurate.

We're going to pass on this one, though. Closing without merging.

Appreciate the contribution.

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

Labels

P1 High — major feature broken, no workaround sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/terminal Terminal execution and process management type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants