Skip to content

security: scrub process-env secrets from shell-tool output#295

Merged
mbektas merged 2 commits into
plmbr:mainfrom
pjdoland:fix/scrub-env-from-shell-output
May 19, 2026
Merged

security: scrub process-env secrets from shell-tool output#295
mbektas merged 2 commits into
plmbr:mainfrom
pjdoland:fix/scrub-env-from-shell-output

Conversation

@pjdoland
Copy link
Copy Markdown
Collaborator

@pjdoland pjdoland commented May 18, 2026

Summary

execute_command and run_command_in_embedded_terminal return raw stdout+stderr to chat history. A verbose LLM-driven command like env, printenv, or a misbehaving binary that dumps env on error will surface the user's own GITHUB_TOKEN, ANTHROPIC_API_KEY, NBI_GH_ACCESS_TOKEN_PASSWORD, and similar into the chat transcript (and through to the LLM provider, audit log, and any client tabs).

run_command_in_jupyter_terminal returns the captured terminal output back through the same chat tool-result channel, so it has the same exposure.

Solution

New helper redact_env_secrets in util.py runs two passes over every shell-tool output before it reaches chat:

  1. Env-value scrub. Replace literal occurrences of values for env vars whose name matches sensitive substrings (TOKEN, PASSWORD, SECRET, API_KEY, PRIVATE_KEY, CREDENTIAL, AUTH, OAUTH, BEARER, COOKIE, JWT, ACCESS_KEY). Substring + case-insensitive; 8-char value floor avoids redacting incidental short strings.
  2. Known-prefix scrub. Replace any token starting with a well-known credential prefix (ghp_, gho_, ghs_, ghu_, github_pat_, sk-ant-, sk-, xoxb-, xoxp-, xoxa-, AKIA, ASIA). Catches base64/derived shapes where the literal env value never appears, plus tokens passed via CLI flag.

All three shell tools (execute_command, run_command_in_embedded_terminal, run_command_in_jupyter_terminal) call the helper before emitting output. execute_command scrubs the final joined string; run_command_in_embedded_terminal scrubs each streamed line and the final stderr block; run_command_in_jupyter_terminal scrubs the captured terminal capture echoed back to the tool result.

Second-pass review surfaced an ANSI-color-wrapped bypass: pass 2's (?<![A-Za-z0-9_-]) lookbehind is blocked by the alphanumeric CSI terminator (m, K, etc.) that immediately precedes a colour-wrapped token in shell-tool output. The pass-2 implementation now scrubs against a working copy with ANSI replaced by a single space (preserving word boundaries) so the lookbehind anchors correctly; literal redactions still apply to the original text.

Admin opt-out: NBI_DISABLE_OUTPUT_SCRUB=1 disables the redaction for sophisticated users debugging credential helpers. Default off.

The existing _claude_cli._scrub_env_overrides (which redacts explicitly-passed env_overrides) routes through a shared _replace_values primitive in util.py, so the redaction contract stays consistent across the two surfaces.

Testing

Pytest covers: sensitive-name pattern matching, length floor, case-insensitivity, non-sensitive names left alone, substring-pattern matching across the full pattern list, end-to-end scrubbing of stdout and stderr through execute_command, the admin opt-out env var, all six well-known credential prefixes, the short-lookalike-passes-through contract, ANSI-color-wrapped prefix tokens, multi-line output, opt-out disables both passes, and end-to-end Jupyter-terminal scrub through the UI command response.

Full suite green: pytest 790 pass, tsc clean, prettier clean, jest clean.

Risks / follow-ups

  • Known gaps the helper does NOT close (documented in the helper's docstring): partial-value leaks where output truncates a token (catches only known prefixes); encodings other than the known-prefix list (URL-encoded, hex-derived HMACs). Addressed at a higher layer.
  • MCP tool results are not scrubbed today. External MCP server text content flows straight to chat; an MCP server that shells out and leaks env hits the same threat surface as execute_command. Tracked as a separate follow-up; routing every MCP text result through redact_env_secrets is a one-line change but the test coverage matrix is broader.
  • Architectural follow-up: a future ProcessSandbox that runs subprocesses with a filtered env (drop sensitive names unless explicitly passed through) would remove the source rather than scrubbing the sink. Tracked separately.
  • The chat sidebar / inline chat surfaces aren't affected; the redactor only fires on shell-tool output.

@pjdoland pjdoland added the bug Something isn't working label May 18, 2026
Copy link
Copy Markdown
Collaborator

@mbektas mbektas left a comment

Choose a reason for hiding this comment

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

LGTM!

pjdoland added 2 commits May 18, 2026 16:50
execute_command and run_command_in_embedded_terminal return raw
stdout+stderr to chat history. A verbose LLM-driven command like
\`env\`, \`printenv\`, or a misbehaving binary that dumps env on error
will surface the user's own GITHUB_TOKEN, ANTHROPIC_API_KEY,
NBI_GH_ACCESS_TOKEN_PASSWORD, and similar into the chat transcript
(and through to the LLM provider, audit log, and any client tabs).

New helper redact_process_secrets in util.py replaces occurrences of
process-env values with <redacted>. Conservative shape: only env vars
whose NAME contains one of TOKEN / PASSWORD / SECRET / API_KEY /
PRIVATE_KEY / CREDENTIAL / AUTH have their values redacted (substring,
case-insensitive). Values shorter than 8 characters are skipped so a
short incidental string can't trigger spurious redaction. The variable
name itself is left intact; only the value is masked.

Both shell tools call the helper before emitting output:
  - execute_command scrubs the final joined string before returning.
  - run_command_in_embedded_terminal scrubs each streamed line and the
    final stderr block before sending through MarkdownPartData.

Tests cover name-pattern matching, length floor, case-insensitivity,
non-sensitive names left alone, and end-to-end scrubbing of stdout and
stderr through execute_command.
Six-agent review on the scrub-env work surfaced four convergent
must-fixes. Applied:

- Drop the backward-compat alias `redact_process_secrets`; it had no
  external callers and was introduced in the same diff, so keeping it
  would ossify a misnaming.
- Extract a shared `_replace_values` primitive in util.py; the existing
  `_claude_cli._scrub_env_overrides` was a near-duplicate of the literal-
  value pass. Both helpers now route through one consistent code path
  (literal-substring match, 8-char value floor, `<redacted>` placeholder).
- Add `NBI_DISABLE_OUTPUT_SCRUB` env var (`1`/`true`/`yes`/`on`) for
  sophisticated users who need raw shell output when debugging
  credential helpers. Default off so the redaction stays load-bearing
  in normal use.
- Document both the redaction behavior (`docs/troubleshooting.md`) and
  the admin opt-out (`docs/admin-guide.md`) so users see the rationale
  when their command output shows `<redacted>`.

Three new tests pin the opt-out env-var values (`1` / `true` / unset),
known-prefix matches across all six well-known credential prefixes, and
the short-lookalike-passes-through contract.
@pjdoland pjdoland force-pushed the fix/scrub-env-from-shell-output branch from bee8fcf to b669ebf Compare May 18, 2026 20:55
@pjdoland pjdoland added this to the 5.0.x milestone May 19, 2026
@mbektas mbektas merged commit 67cc227 into plmbr:main May 19, 2026
4 checks passed
pjdoland added a commit to pjdoland/notebook-intelligence that referenced this pull request May 22, 2026
Promotes the [Unreleased] CHANGELOG snapshot to [5.0.0] - 2026-05-22
and expands it to cover everything merged into upstream/main after
PR plmbr#287's docs refresh. Bumps package.json to 5.0.0.

CHANGELOG additions cover the post-plmbr#287 surface:

- Settings tabs: plugin marketplace picker (plmbr#284), plugin marketplace
  details + Update button (plmbr#303), per-workspace MCP disable (plmbr#286),
  JSON-paste path in Add MCP server (plmbr#285).
- Launchers: hide-with-policy (plmbr#288), brand icons for Codex / opencode
  (plmbr#325, plmbr#333), per-launch directory picker (plmbr#332).
- Chat sidebar and agentic UX: workspace @-mention in Claude mode
  (plmbr#327), reload-open-files-on-disk (plmbr#330), steered system prompt
  away from over-eager notebook creation (plmbr#336).
- Skills: multi-manifest support (plmbr#321), tracks-upstream for user-
  imported skills (plmbr#322), HTTP kill switch for the reconciler (plmbr#291).
- Accessibility: full sub-section covering plmbr#305-plmbr#320.
- Security: shell-tool sandbox (plmbr#290), Claude UI-bridge sandbox (plmbr#323),
  0o600 on encrypted token (plmbr#293), env-secret scrubbing (plmbr#295), MCP
  config shape validation (plmbr#299), XSS allowlist (plmbr#296), Copilot WS
  auth + origin (plmbr#301), GHE host detection (plmbr#292), fastmcp -> mcp SDK
  swap (plmbr#324).
- Fixed: session listing unification (plmbr#310), session preview unwrap
  (plmbr#331), down-area runtime throw (plmbr#330 follow-up), WS message-handler
  leak (plmbr#294).
- Removed: fastmcp dependency, history.jsonl session gate.

Adds a Migration note covering the five behavior changes operators
should review before upgrading from 4.x: fastmcp swap, path
sandboxes, history.jsonl gate removal, workspace @-mention pointer
shape, and the Copilot WebSocket auth/origin tightening.

Two reviewer rounds (six personas each) applied:
- Round 1 caught security overclaims (plmbr#293, plmbr#299, plmbr#323), the
  plmbr#284/plmbr#303 mis-attribution, missing migration note, 3 em dashes,
  and the stale `fastmcp==2.x.*` recommendation in the admin guide.
- Round 2 caught the missing plmbr#301 migration bullet, missing version-
  matrix 5.0.x row, missing README TOC entry, and a couple of style
  nits (sub-heading overpromise, orphan bullet).

Skipped (deferred to future PRs):
- README first-run tour mention.
- Admin guide HTTP kill-switch row in Failure-modes table.
- Terminal drag-drop trust-model precision update after plmbr#327.
- Cipher description nit in plmbr#293 (Fernet AES-128-CBC+HMAC, not
  AES-GCM).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants