Skip to content

fix(desktop): scrub credentials from hermes serve spawn env - #70372

Open
zapabob wants to merge 3 commits into
NousResearch:mainfrom
zapabob:fix/desktop-serve-env-scrub
Open

fix(desktop): scrub credentials from hermes serve spawn env#70372
zapabob wants to merge 3 commits into
NousResearch:mainfrom
zapabob:fix/desktop-serve-env-scrub

Conversation

@zapabob

@zapabob zapabob commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Scrub credential-shaped keys from the environment passed to hermes serve / pool backend children and the serve --help capability probe.
  • Re-inject only intentional overrides (HERMES_HOME, minted HERMES_DASHBOARD_SESSION_TOKEN, HERMES_DESKTOP, web dist, ready file, PATH/PYTHONPATH overlays).

Why

Long-lived Desktop backends inherited the full parent Electron env, including messaging tokens and provider keys that belong in HERMES_HOME/.env. Capability probes also do not need secrets.

Test plan

  • npx tsx --test apps/desktop/electron/scrub-child-env.test.ts
  • Manual: start Desktop local backend and confirm serve still boots with a minted session token

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) labels Jul 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #70370 and #70373 create the same shared scrubber and main-process import while changing separate call sites. Please consolidate these sibling changes or rebase them in a defined order.

@teknium1 teknium1 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.

Thanks for targeting a real Desktop secret-forwarding path: current main still forwards process.env into the serve --help probe (apps/desktop/electron/main.ts:1903) and local backend spawns (apps/desktop/electron/main.ts:8096-8110, 8384-8403).

Problems

  • apps/desktop/electron/scrub-child-env.ts:30 classifies OPENROUTER_BASE_URL as a credential. This is a documented endpoint override (website/docs/reference/environment-variables.md:16) consumed by runtime resolution (hermes_cli/runtime_provider.py:1185), so scrubbing it breaks a supported process-environment configuration.
  • apps/desktop/electron/scrub-child-env.ts:40 is case-sensitive. Normalize names before matching so mixed-case credential variables are not forwarded on Windows-supported paths; existing Desktop code already accounts for Windows environment-key casing in apps/desktop/electron/backend-env.ts:25-31.
  • The member review correctly identifies that #70370 and #70373 add the same helper. Consolidate the shared implementation and the three sets of call sites rather than landing duplicate modules.

Suggested changes

  • Retain non-secret endpoint variables, normalize credential-name matching, and add mixed-case coverage.
  • Add a composed child-environment test for the backend spawn path after consolidating the sibling work.

Automated hermes-sweeper review.

'GEMINI_BASE_URL',
'OPENAI_BASE_URL',
'OPENROUTER_BASE_URL',
'OLLAMA_BASE_URL',

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.

OPENROUTER_BASE_URL is a documented, non-secret endpoint override and current provider resolution reads it from the process environment (website/docs/reference/environment-variables.md:16, hermes_cli/runtime_provider.py:1185). Removing it here changes supported Desktop backend configuration; please keep endpoint settings out of the credential denylist.

return false
}

if (CREDENTIAL_NAMES.has(name)) {

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.

Normalize name before matching the exact-name set and suffixes. This check currently misses mixed/lower-case credential-shaped keys; the Desktop backend already has Windows-specific case handling for environment keys in backend-env.ts.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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 Jul 30, 2026
@andrexibiza

Copy link
Copy Markdown
Contributor

Bound to the child-process credential-inheritance class under #83565 (#83565) — same bug class, different surface. desktop serve spawn env; Wave F — shares main.ts + scrub-child-env.ts with #70370/#70373: consolidate or strictly sequence, never merge independently. The EPIC carries the live class table, dedup adjudication, and the dependency-driven merge order.

zapabob commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Desktop child-env consolidation status — 2026-08-18

I re-audited #70370, #70372, and #70373 against current main at 68a713385e3c85c1300b9c8e89a4f5d11fba1738 and the Wave F contract in #83565.

Decision: use #70372 as the survivor and consolidate the terminal, serve/backend, updater/bootstrap, and current uninstall child-process sinks into this PR. Strict sequencing would retain the same shared-file conflicts while requiring repeated rebases of main.ts, the helper, and its tests.

The rebuilt history is designed as three logical commits:

  1. canonical Desktop child-env policies and Vitest coverage;
  2. embedded terminal, serve --help, primary backend, and pool-backend wiring;
  3. current updater hand-offs, bootstrap PowerShell/Bash children, and uninstall --gui-summary wiring.

The consolidated policy preserves documented non-secret endpoint overrides and the repository's trusted general-AWS runtime contract, matches env names case-insensitively, closes APPTAINERENV_ / SINGULARITYENV_ tunnelling, blocks FAL_KEY and Hermes-internal dynamic secrets, scrubs every source map rather than trusting an override object, and permits only a freshly minted dashboard session token to be re-added through a dedicated serve-env constructor.

Repository state: no branch rewrite or code push has occurred. The currently connected GitHub integration can read and comment but its Git Data / Contents / Refs writes to zapabob/hermes-agent return 403 Resource not accessible by integration. The existing heads of #70370, #70372, and #70373 therefore remain unchanged.

Part of #83565.

zapabob commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: upstream main advanced during the re-audit from 68a713385e3c85c1300b9c8e89a4f5d11fba1738 to 66221397a13a43a01fc96d636f3bac4268efc5c3 (8 commits). I rechecked the live import ordering and every targeted Desktop sink at the newer SHA: terminalShellEnv, serve --help, both primary/pool serve spawns, all four updater hand-offs, both bootstrap script spawns, and uninstall --gui-summary remain present with compatible exact anchors. The prepared rebuild driver fetches live upstream main and aborts before push if either upstream main or the #70372 head moves during validation.

@alt-glitch alt-glitch removed the sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows label Aug 17, 2026
@zapabob
zapabob force-pushed the fix/desktop-serve-env-scrub branch from 760a371 to 94796a6 Compare August 20, 2026 10:43
@zapabob

zapabob commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Consolidated #70370 and #70373 into this survivor and refreshed it onto official main 5ab7473. Final head: 94796a63a88db9c3ad239fcbb671123d996cea3b. The implementation now follows the current Desktop topology: terminal-ipc.ts, serve --help, primary and pooled backends, four updater hand-offs, both bootstrap script spawns, and uninstall --gui-summary. The shared policy is case-insensitive, scrubs every input map, blocks Apptainer/Singularity effective-name tunnelling, removes FAL_KEY and dynamic Hermes/auxiliary/relay secrets, preserves documented endpoint overrides, the general AWS chain, CLAUDE_CODE_OAUTH_TOKEN, NPM_TOKEN, and empty non-secret values, and re-adds only the freshly minted dashboard token through the dedicated serve builder. Local validation on this rebased head: 17 focused Electron tests passed; Desktop typecheck passed; ESLint and Prettier passed on all five changed files; git diff --check passed. The broader Electron project run reached 1,479 passed / 27 failed / 4 skipped; all 27 failures were unrelated Windows-host assumptions in POSIX mode, symlink, SSH-path, WSL, and temp-directory suites, and none touched these files. Fresh GitHub checks have been triggered for the exact final SHA.

@zapabob
zapabob force-pushed the fix/desktop-serve-env-scrub branch from 94796a6 to e89fc35 Compare August 20, 2026 10:45
@zapabob

zapabob commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: official main advanced once more while the prior comment was being posted. I rebased the same validated three-commit series onto 6d85d79 and force-updated with the exact prior-head lease. The current final head is e89fc358714187a0a9320e1364a35239843d7183; the intervening official change only touched Python update files, so the five-file Desktop patch and validation results are unchanged. Fresh checks now target this SHA.

@zapabob
zapabob force-pushed the fix/desktop-serve-env-scrub branch from e89fc35 to 8bb2c17 Compare August 20, 2026 11:19
@zapabob

zapabob commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up: rebased the consolidated survivor again onto the current official main (2d92793045432be06eedde29ff64743ead6ed240).

New final head: 8bb2c17aa3d0fb7d0828e96cf99a6bab858287ac

The intervening official commits do not overlap the five Desktop files in this PR. After the final rebase:

  • focused Electron tests: 17 passed;
  • changed-file ESLint with zero warnings: passed;
  • Prettier check: passed;
  • git diff --check: passed.

The full Desktop TypeScript check had already passed on the byte-identical Desktop patch immediately before this ancestry-only rebase. Fresh GitHub checks are now running for the new exact head.

@alt-glitch alt-glitch added area/install-update Installer, updater, packaging, wheels, doctor and removed sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 20, 2026
@zapabob
zapabob force-pushed the fix/desktop-serve-env-scrub branch from 8bb2c17 to f239561 Compare August 22, 2026 09:51
@zapabob

zapabob commented Aug 22, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the frozen official main 8e475ed. Exact remote lease was verified before the update; final head is f2395613582c43de9cb6954b98c0919483952125. The rebase was intended to be semantic-preserving, and the parent ancestry and git diff --check validations passed. Existing focused-test and CI evidence remains in the earlier review comment; GitHub will run fresh checks for this final SHA.

@alt-glitch alt-glitch removed the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Aug 22, 2026
@zapabob
zapabob force-pushed the fix/desktop-serve-env-scrub branch from f239561 to d95b13a Compare August 23, 2026 01:51
@zapabob

zapabob commented Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Current-main refresh: rebased this PR onto official upstream/main 7a54ab2. Published head: d95b13a. In the isolated worktree, git merge-base --is-ancestor and git diff --check both pass. This comment records the rebase/whitespace gates only; required GitHub checks remain subject to the new head.

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

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/desktop Electron desktop app (apps/desktop/*) 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.

4 participants