Skip to content

fix(desktop): resolve hermes CLI via login shell on Linux - #43834

Closed
VrtxOmega wants to merge 2 commits into
NousResearch:mainfrom
VrtxOmega:fix/desktop-nix-login-shell-hermes-42923
Closed

VrtxOmega wants to merge 2 commits into
NousResearch:mainfrom
VrtxOmega:fix/desktop-nix-login-shell-hermes-42923

Conversation

@VrtxOmega

Copy link
Copy Markdown

Summary

On Linux/NixOS, Hermes Desktop can inherit a stripped PATH that omits nix profile shims where hermes is installed after hermes setup. The resolver then falls through to first-launch bootstrap even though the CLI works in the user's shell.

Fix

When findOnPath('hermes') misses on non-Windows hosts, fall back to sh -lc 'command -v hermes' before bootstrap.

Fixes #42923

Notes

This addresses PATH visibility for packaged Desktop launches. The separate "pick existing folder" validation for ~/.hermes without a checkout tree may still need follow-up if that UI path remains broken on Nix.

Verification

cd apps/desktop && node --test electron/backend-probes.test.cjs

Packaged Desktop on Linux/NixOS often inherits a stripped PATH without
the nix profile shims where `hermes` is installed after `hermes setup`.
Fall back to `sh -lc 'command -v hermes'` before triggering bootstrap.

Fixes NousResearch#42923
Reorder backend-probes docs/functions and restrict login-shell PATH
resolution to an allowlisted command name so the sh -lc probe cannot
be abused for shell injection.
@liuhao1024

Copy link
Copy Markdown

Verification comment — security-reviewed by scheduled code review bot

Reviewed the login-shell PATH probe implementation. The approach is solid:

  1. Allowlist gate — LOGIN_SHELL_COMMAND_ALLOWLIST restricts findCommandOnLoginShell to only hermes, preventing future callers from accidentally passing user-controlled input to sh -lc.
  2. Defense-in-depth test — the test suite includes hermes; rm -rf / in the rejection set, confirming the allowlist blocks shell metachar injection even if someone modifies the allowlist later.
  3. Platform guard — returns null on Windows, avoiding sh unavailability.
  4. Fallback chain — findOnPath → findCommandOnLoginShell is the right ordering; fast PATH probe first, slower login-shell probe only as fallback.

One minor observation: findCommandOnLoginShell takes the last line of command -v output (.split('\n').pop()), which handles edge cases where the login shell prints preamble text before the resolved path. This is correct behavior.

No issues found. Clean security fix for NixOS/Linux PATH resolution.

@alt-glitch alt-glitch added type/bug Something isn't working area/nix Nix flake, NixOS module, container packaging P3 Low — cosmetic, nice to have labels Jun 10, 2026

@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 tracing the NixOS packaged-PATH failure. The premise is still present on current main: apps/desktop/electron/main.ts:3416 only calls findOnPath('hermes'), so a CLI available solely through the login-shell profile is not considered.

Problems

  • This branch targets Electron .cjs files that no longer exist on main. Commit 39d09453f migrated the relevant code to apps/desktop/electron/main.ts and apps/desktop/electron/backend-probes.ts; GitHub consequently reports the PR as conflicting.
  • The new tests in apps/desktop/electron/backend-probes.test.cjs:84-101 cover only rejected/no-op inputs. They do not verify a successful login-shell lookup or the resolveHermesBackend fallback.

Suggested changes

  • Salvage the allowlisted probe into the current TypeScript files and insert it after findOnPath('hermes') at apps/desktop/electron/main.ts:3416, preserving the existing verifyHermesCli check at line 3442.
  • Add controlled success and resolver-fallback coverage rather than relying on a host-installed CLI.

Automated hermes-sweeper review.

assert.equal(verifyHermesCli('/definitely/not/a/real/binary/anywhere'), false)
})

test('findCommandOnLoginShell returns null for falsy command', () => {

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.

These tests cover only early-return guard paths. Please add controlled coverage that proves a successful login-shell lookup is used by the resolver after findOnPath('hermes') misses.

@VrtxOmega

Copy link
Copy Markdown
Author

Closing this older point fix as superseded by the merged #69696 (25851e6e5800401be885c79bbc8510ccf9bc248e). That change resolves the login-shell PATH before local backend resolution and spawning, carries the TypeScript integration, and explicitly credits this investigation. Thank you for carrying it through; no separate port of the removed CommonJS files is needed.

@VrtxOmega VrtxOmega closed this Sep 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/nix Nix flake, NixOS module, container packaging 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 type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Hermes Desktop ignores existing installation

4 participants