Skip to content

fix(desktop): scrub credentials from updater and bootstrap children - #70373

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

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

Conversation

@zapabob

@zapabob zapabob commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Scrub credential-shaped environment variables from Desktop updater hand-off, streamed update, and bootstrap install.ps1/bash child processes.
  • Keep HERMES_HOME and rebuilt PATH entries required for the updater/bootstrap to run.

Why

Detached updater and first-launch bootstrap scripts previously inherited the full Electron parent env, including provider and messaging secrets they do not need.

Salvages the Desktop half of the subprocess scrub pattern used in #70342 / #70351 / #70357.

Test plan

  • npx tsx --test apps/desktop/electron/scrub-child-env.test.ts
  • Manual: trigger update hand-off path on Windows and confirm updater still launches

Co-authored-by: Cursor <cursoragent@cursor.com>
@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
Contributor

This was generated by AI during triage.

Related: #70370 and #70372 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.

Platforms CI failed with no Vitest suite because the test used node:test.
Lint failed on perfectionist/sort-imports for scrub-child-env.

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

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for addressing a real child-process credential boundary: current main still forwards process.env to the updater (apps/desktop/electron/main.ts:2963), streamed updater (apps/desktop/electron/main.ts:3091), and bootstrap scripts (apps/desktop/electron/bootstrap-runner.ts:469, :567).

Problems

  • apps/desktop/electron/scrub-child-env.ts:20 does not classify FAL_KEY as a credential. It matches neither the listed names nor a suffix, so it remains in updater/bootstrap child environments. FAL_KEY is a documented FAL API key (hermes_cli/config_defaults.py:3572) and is treated as a credential by the repository test filter (tests/conftest.py:121).
  • The shared helper duplicates the implementations in open #70370 and #70372, matching the member's consolidation request. This PR is currently reported as conflicting.

Suggested changes

  • Consolidate the three sibling changes around one helper and all intended call sites.
  • Add FAL_KEY to the explicit credential set and cover it in scrub-child-env.test.ts.

Automated hermes-sweeper review.

'_OAUTH_TOKEN'
])

const CREDENTIAL_NAMES = new Set([

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FAL_KEY is a documented FAL API credential (hermes_cli/config_defaults.py:3572) but is absent here and does not match any suffix below, so this scrubber forwards it. Please add it to the explicit credential names and assert its removal in this helper's test.

@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 area/install-update Installer, updater, packaging, wheels, doctor 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 updater and bootstrap children env; Wave F — shares main.ts + scrub-child-env.ts with #70370/#70372: 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

Consolidation coordination: this updater/bootstrap scope is being folded into survivor PR #70372 together with #70370, following the Wave F interlock in #83565. The re-port targets current main rather than reviving the obsolete runStreamedUpdate path: it covers the live Windows repo hand-off, staged-updater fallback, Windows bootstrap recovery, POSIX hand-off, bootstrap PowerShell/Bash children, and current uninstall --gui-summary child. FAL_KEY coverage is retained. The current branch remains unchanged; no close or branch rewrite has been performed.

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

Re-reviewed this against current main and the Wave F consolidation under #83565.

The original vulnerability is valid, but I do not think #70373 should be repaired or merged independently anymore. Its topology is substantially stale: current Desktop update handling has moved beyond the old runStreamedUpdate path, while the live credential-inheritance surface now includes the Windows repo hand-off, staged-updater fallback, Windows bootstrap recovery, POSIX hand-off, bootstrap PowerShell/Bash children, and the uninstall --gui-summary probe.

There are also policy defects in the helper that should not be carried forward unchanged:

  • credential-name matching is case-sensitive;
  • several documented non-secret *_BASE_URL endpoint overrides are classified as credentials;
  • the generic overrides map is applied after scrubbing and can therefore reintroduce an arbitrary credential;
  • the policy does not close effective-name tunnelling through APPTAINERENV_ / SINGULARITYENV_;
  • the Desktop policy should converge with the broader child-process boundary semantics rather than maintain a weaker parallel denylist.

FAL_KEY itself is fixed at this head, so the old inline finding is no longer the important blocker.

The right disposition is the one already recorded in the coordination comments: fold #70370 + #70373 into survivor #70372, re-port against current main, use one canonical case-insensitive policy, scrub every input map, allow exceptional secrets only through narrow purpose-built constructors, and cover every live Desktop child-process sink with composed regression tests.

I would close #70373 once that consolidated implementation actually exists on #70372. Until then, keep it as provenance for the updater/bootstrap portion of the class rather than treating this stale branch as independently mergeable.

@alt-glitch alt-glitch removed the sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data label Aug 18, 2026
@zapabob

zapabob commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Closing this sibling in favor of the requested consolidated survivor, #70372.

The updater/bootstrap work from this branch is preserved in #70372 and updated for the current live hand-off topology, alongside the terminal and backend child-process boundaries. The consolidated branch is rebased onto official main 2d92793045432be06eedde29ff64743ead6ed240; its current head is 8bb2c17aa3d0fb7d0828e96cf99a6bab858287ac. Focused tests, Desktop typecheck, changed-file lint/format, and diff checks passed as recorded there.

@zapabob zapabob closed this Aug 20, 2026
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: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-platform-windows Sweeper risk: may break or behave differently on native Windows type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants