Skip to content

fix(lsp): strip credentials from language server env - #55256

Open
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/lsp-subprocess-env-strip
Open

fix(lsp): strip credentials from language server env#55256
necoweb3 wants to merge 1 commit into
NousResearch:mainfrom
necoweb3:fix/lsp-subprocess-env-strip

Conversation

@necoweb3

Copy link
Copy Markdown
Contributor

Summary

This prevents Hermes credentials from being inherited by LSP language-server subprocesses.

agent/lsp/client.py previously spawned each language server with dict(os.environ), then applied explicit LSP env overrides. That copied provider keys, GitHub tokens, bot tokens, dashboard tokens, and other Hermes process credentials into workspace-scoped language server processes.

Why

Language servers are external subprocesses running in the active project workspace. They may come from project tooling or package managers, and they do not need Hermes model/provider credentials by default. Passing the full parent environment crosses the same child-process credential boundary as other subprocess env leaks.

Changes

  • Build the default LSP subprocess environment with hermes_subprocess_env(inherit_credentials=False).
  • Preserve explicit env={...} overrides passed to LSPClient, so intentionally configured language-server variables still work.
  • Add regression tests proving parent credentials are stripped and explicit LSP env overrides are preserved.

Tests

python -m pytest tests/agent/lsp/test_client_env.py -q --timeout-method=thread
2 passed

python -m pytest tests/agent/lsp/test_client_e2e.py -q --timeout-method=thread
6 passed

@alt-glitch alt-glitch added type/security Security vulnerability or hardening comp/lsp Language Server Protocol integration (P2 policy) area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jun 29, 2026

@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

Verdict: LGTM

Good security fix — strips credentials from the language server environment by using hermes_subprocess_env(inherit_credentials=False) instead of raw os.environ. The test is well-designed: it captures the env dict via a monkeypatched subprocess creator and verifies credentials are not leaked.

Security: Prevents LSP child processes from inheriting API keys and tokens.
Testing: Dedicated test file with clear assertion on credential stripping.


Reviewed by Hermes Agent

@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

Verdict: LGTM

Security fix: prevents Hermes credentials from being inherited by LSP language-server subprocesses. Uses hermes_subprocess_env(inherit_credentials=False) instead of dict(os.environ).

  • Clean security hardening
  • Dedicated test for env sanitization
  • Well-scoped: 2 files, 78 additions

Reviewed 2 files, 78 additions. Approved.

@egilewski

Copy link
Copy Markdown
Contributor

suggesting changes

Security evidence:

  • trust boundary: the LSP feature launches external language tooling and package-manager helpers from Hermes into workspace-controlled projects.
  • source/sink/invariant: LSP-related child subprocess environments should not inherit Hermes provider, GitHub, bot, or dashboard credentials unless the user explicitly configured a language-server override.
  • current-main reproduction: probe_lsp_env.py imported /home/mac/hermes/hermes-agent/worktrees/hermes-review-pool/worktrees/main-55256-20260708T200029Z/agent/lsp/client.py and captured OPENAI_API_KEY, ANTHROPIC_API_KEY, GH_TOKEN, TELEGRAM_BOT_TOKEN, and HERMES_DASHBOARD_SESSION_TOKEN in the language-server child environment.
  • PR-head or patch-replay validation: after current GitHub main advanced to 31e39dec84cbce925fb99cef9dccb739e2f92474, the same two-file GitHub patch replayed cleanly and imported /home/mac/hermes/hermes-agent/worktrees/hermes-review-pool/worktrees/pr-55256-20260708T200029Z-replay2/agent/lsp/client.py; the same probe showed those parent credentials stripped from LSPClient._spawn, while explicit env={...} overrides were still preserved.
  • positive/negative cases: tests/agent/lsp/test_client_env.py tests/agent/lsp/test_client_e2e.py tests/tools/test_hermes_subprocess_env.py -q passed as 26 passed; the PR's local --timeout-method=thread flag was not available in this review environment.
  • residual bypass search: agent/lsp/install.py still invokes npm install without an explicit sanitized env and builds go install from dict(os.environ) before setting GOBIN; probe_lsp_install_env.py on the replay captured synthetic OPENAI_API_KEY, ANTHROPIC_API_KEY, GH_TOKEN, and HERMES_DASHBOARD_SESSION_TOKEN reaching those LSP auto-install subprocesses.
  • reviewer validation: local source/probe review found this first blocker, so review stopped at the LSP installer credential path.

The LSPClient spawn path is fixed, but the LSP auto-install path can still leak the same parent credentials to package-manager subprocesses before the language server is launched. Please route the npm/go install environments through the same sanitized subprocess-env policy, adding only the install-specific variables such as GOBIN or the staging path, and add a regression test that proves installer subprocesses do not inherit Hermes credentials.

Because the submitted branch has stale/unrelated-history merge setup, I reviewed its meaningful two-file GitHub patch replayed onto current GitHub main at 31e39dec84cbce925fb99cef9dccb739e2f92474; that replay applied cleanly for source review and tests, but it does not by itself prove the submitted branch merges cleanly.

Signed: GPT-5.5-xhigh in Codex

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for addressing the direct LSP server credential boundary. The proposed LSPClient._spawn change is aligned with the centralized policy in tools/environments/local.py:471-547: it strips credentials by default and retains explicitly configured LSP overrides.

Problems

  • The LSP auto-install path remains outside that policy. agent/lsp/install.py:264-271 runs npm install without env=, so it inherits the parent environment. agent/lsp/install.py:308-319 explicitly copies os.environ for go install.
  • These are reachable before server startup when a binary is missing (agent/lsp/servers.py:237-239, 277-282), so the same credentials can still reach package-manager subprocesses.
  • The added test covers only LSPClient._spawn; it does not cover either installer environment.

Suggested changes

  • Route npm and Go install environments through hermes_subprocess_env(inherit_credentials=False); retain only install-specific values such as GOBIN.
  • Add regression tests capturing installer subprocess environments and asserting credentials are absent while required explicit variables remain.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 15, 2026
@andrexibiza

Copy link
Copy Markdown
Contributor

Bound to the child-process credential-inheritance class under #83565 (#83565) — same bug class, different surface. LSP server env strip; Wave D — shares agent/lsp/client.py with #77528; consumer of the sanitized builder. The EPIC carries the live class table, dedup adjudication, and the dependency-driven merge order.

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/lsp Language Server Protocol integration (P2 policy) P2 Medium — degraded but workaround exists sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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.

6 participants