fix(desktop): don't let stale .env token clobber desktop-injected session token - #76958
fix(desktop): don't let stale .env token clobber desktop-injected session token#76958andrexibiza wants to merge 8 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the desktop-to-gateway token path; current main still has the reported overwrite path.
Problems
hermes_cli/env_loader.py:494treats every inheritedHERMES_DASHBOARD_SESSION_TOKENas Electron-injected. That changes the documented loader rule that a profile.envoverrides stale shell exports (current mainhermes_cli/env_loader.py:470). Electron already supplies an authoritative discriminator: both spawn paths setHERMES_DESKTOP='1'beside the fresh token (apps/desktop/electron/main.ts:8199-8202,:8492-8495).
Suggested changes
- Preserve the token only when
HERMES_DESKTOP == '1'before the dotenv load. Add the marker to the positive cases and cover an unmarked pre-existing token being overridden by.env.
Automated hermes-sweeper review.
| # clobber the injected token, breaking the desktop↔gateway WebSocket | ||
| # auth handshake. Save/restore it around the .env load so the injected | ||
| # value (if present) always wins. | ||
| _desktop_session_token = os.environ.get("HERMES_DASHBOARD_SESSION_TOKEN") |
There was a problem hiding this comment.
This cannot distinguish the Electron spawn credential from an ordinary shell export. Current loader semantics explicitly let the profile .env override stale shell values (env_loader.py:470), while both Electron spawn paths set HERMES_DESKTOP='1' beside this token (apps/desktop/electron/main.ts:8199-8202, :8492-8495). Snapshot and restore only when that pre-load marker is "1", then add an unmarked-shell-token regression case.
There was a problem hiding this comment.
Addressed in 8476064508 — the desktop session-token save/restore is gated on HERMES_DESKTOP='1' (hermes_cli/env_loader.py:488-493), so unmarked shell exports keep the documented loader semantics and only the desktop-injected token path gets the preserve behavior. Verified at head 8476064508: 21/21 env_loader tests pass.
|
Agreed — the unconditional save/restore changed the documented loader rule for non-desktop runs. Fixed in 809e299aba. Change: Tests (
Validation: |
…sion token
The desktop app injects HERMES_DASHBOARD_SESSION_TOKEN into the spawned
hermes serve process so it can authenticate its WebSocket probe. The
user .env may contain a stale persisted value for the same key, and
load_hermes_dotenv() loads it with override=True, silently replacing
the injected token. The gateway then rejects the desktop's WS
handshake with 403 ("websocket connection failed").
Save/restore the injected token around the .env load so the spawn-time
value wins when present. Other keys keep existing override semantics.
Review follow-up: unconditionally preserving an inherited HERMES_DASHBOARD_SESSION_TOKEN changed the documented loader rule that a profile .env overrides stale shell exports. Electron pairs the injected token with HERMES_DESKTOP='1' on both spawn paths (apps/desktop/electron/main.ts), so gate the save/restore on that marker. An unmarked shell-exported token keeps the existing override semantics.
809e299 to
8476064
Compare
|
Review receipt — comment from 2026-08-02 on hermes_cli/env_loader.py (session-token preservation): Addressed in 8476064 — the HERMES_DASHBOARD_SESSION_TOKEN save/restore is now gated on HERMES_DESKTOP=='1' (both Electron spawn paths in apps/desktop/electron/main.ts set the marker beside the token), so an unmarked shell export keeps the documented '.env overrides stale shell exports' rule exactly as requested. Regression coverage added per the review: test_desktop_injected_session_token_survives_dotenv_load (marked path) + test_session_token_from_dotenv_when_not_injected (unmarked path). 21/21 env_loader tests pass. |
…paths Signed-off-by: andrexibiza <84248988+andrexibiza@users.noreply.github.com>
…atch Signed-off-by: andrexibiza <84248988+andrexibiza@users.noreply.github.com>
The provenance classifier labels the stale-.env clobber state 'injected' (HERMES_DESKTOP=1 survives the override while the token is replaced), so the old early-return suppressed the actionable hint on the exact lockout scenario it was built for. A loopback mismatch with an env-sourced server token IS the signature — a genuinely adopted injection would have matched and never reached the hint. Fire for injected+env alike; 'generated' keeps its restart wording. Signed-off-by: andrexibiza <84248988+andrexibiza@users.noreply.github.com>
Signed-off-by: andrexibiza <84248988+andrexibiza@users.noreply.github.com>
… on WS rejection Signed-off-by: andrexibiza <84248988+andrexibiza@users.noreply.github.com>
Signed-off-by: andrexibiza <84248988+andrexibiza@users.noreply.github.com>
|
Consolidated class close — validation receipt for the full 5-layer PR (head f435f8b, 8 commits): Sweeper point addressed: the original keep_open point (unconditional save/restore changed the documented .env-overrides-shell rule) is fixed at 8476064 — preservation is gated on HERMES_DESKTOP=='1', pinned by test_unmarked_inherited_token_is_overridden_by_dotenv (a shell-exported token WITHOUT the marker is still overridden by .env). Layer validation (all on this head):
git diff --check clean; check-windows-footguns clean; all commits DCO-signed with the exact noreply email. Related-PR coordination (comments posted): #66120 (env_loader hunk superseded here — credit to @PabloTheThinker in the body; haptic fix + drag-drop split are theirs to land), #54034 (composes; security-boundary decision + tmp-file race fixes listed), #74603 (disjoint main.ts regions; 4 fix points listed). |
|
Reproduced this on current main ( In my case it showed up through Desktop group bots rather than the primary boot screen. The same Hermes host is used locally and from another Mac over SSH. Three profile backends appeared in the room but stopped responding because each profile had a stale
I tested the same precedence fix locally. With a stale token in a temporary profile This confirms the bug also affects pooled per-profile backends and that switching between local Desktop and SSH-connected Desktop is a real way to hit it. |
SHA-gated, fork-local executor for rebuilding PR NousResearch#76958 on exact upstream main. Remove after publication.
Use the fork’s path-gated push executor to rebuild and verify the complete PR NousResearch#76958 product delta on exact upstream main.
Related #40680 #50737 #54034 #55790 #56974 #66120 #66223 #67305 #73599 #73628 #74563 #74603
What
Closes the entire bug class that locked Hermes Desktop out for over an hour: a stale
HERMES_DASHBOARD_SESSION_TOKENpersisted in the user.envsilently clobbers the fresh token the Electron app injects into the spawnedhermes serveprocess.env_loader.pyloads.envwithoverride=True, so the stale value wins beforeweb_server._SESSION_TOKENresolves → every WS handshake is rejected (token_mismatch→ HTTP 403) → the desktop retries the identical failing boot for hours with no actionable message.One PR, five layers, all verified against current main:
HERMES_DASHBOARD_SESSION_TOKENpreservation now lives in_load_dotenv_with_fallback()itself, gated onHERMES_DESKTOP == '1'— everyoverride=Trueload path (user env, project env, managed env, hot-reload re-entry) preserves the Electron-injected token. Unmarked shell exports keep the documented.env-overrides-shell rule; no injected token →.envvalue still loads.source=injected|env|generated length=N— never the value). A loopbacktoken_mismatchwith an env-sourced server token now WARNs naming the exact fix (staleHERMES_DASHBOARD_SESSION_TOKENin<home>/.env— remove the line or runhermes setup). The hint fires forinjectedtoo: under the desktop marker, a mismatch IS the clobber signature. Auth semantics untouched.serve404 (web UI disabled) is recognized byisHeadlessServeResponse()and returns the spawn token with an info note instead of a boot error — killing the misleadingcould not read served dashboard tokenline that masked the real failure on every launch.createTokenRejectionRetryGuard) — the silent hour-long retry loop stops and the existing failure overlay names the.envkey + resolved path + fix. Non-token failures keep the existing retry/repair behavior.hermes servesubprocess + tempHERMES_HOMEwith a stale.envtoken: injected token ACCEPTED, stale REJECTED. RED on origin/main (proven: fresh → 403, stale →gateway.ready), GREEN on this branch. The harness keeps the class closed on CI.Dedup note — supersedes the token-preservation portion of #66120 (authored by @PabloTheThinker): #66120 adds the same save/restore intent gated on
HERMES_DESKTOP=1, but (a) covers only the user-env load plus a re-apply — the project-envoverride=Truepath stays exposed; (b) ships zero regression tests (sweeper-flagged). This PR covers all load paths at the low-level loader and adds the missing tests. Credit for identifying the seam goes to @PabloTheThinker. If this merges first, #66120's env_loader hunk becomes redundant.Related PRs (same bug class / same files — read before merging)
_SESSION_TOKENresolve line inhermes_cli/web_server.pyin the opposite direction (persistence; sweeperkeep_open salvageability=mediumwith an unresolved security-boundary decision).os.environread that classifies whatever value lands in_SESSION_TOKEN(env-set → injected/env; file-only → generated), so it works identically with or without persistence. Both touchweb_server.py— whitespace-only merge-order concern.apps/desktop/electron/main.ts, but the probe path (~1648unwrapWindowsVenvHermesCommand, ~7599spawnPoolBackend, ~7856startHermes). This PR edits the failure-latch path (~8292/8628/8671/9742) plusbackend-start-failure.ts, which #74603 does not touch.web/src/lib/api.tsonly); this PR is the desktop-spawn/backend side.hermes_cli/web_server.py+tools/environments/local.py(WS capability isolation vs token diagnostics).Related issues: #40680 (this PR's
Fixestarget — stale-token poisoning across local/remote mode transitions; grouped facets #50737/#55790/#56974), #74563 (runtime resolution inconsistency behind #74603), #73599 (stale session token → silent dashboard reload), #66223 (dashboard loopback 401 with valid token).Reproduction (current vs expected)
Current (pre-fix) on main: with a stale
HERMES_DASHBOARD_SESSION_TOKEN=old-tokenline in~/.hermes/.env:hermes servewith a fresh injected token +HERMES_DESKTOP=1).env_loader.pyloads.envwithoverride=True→ the stale value replaces the injected token beforeweb_server._SESSION_TOKENresolves./api/wswith its fresh token →hmac.compare_digestfails → HTTP 403 → boot fails → retry loop repeats for ~1h.console auth rejected reason=token_mismatch mode=loopback; desktop.log:could not read served dashboard token (Hermes backend): 404: Headless backend...thenWebSocket (/api/ws) rejected the session token— repeated ~7×.Expected (post-fix): the desktop-injected token always wins when
HERMES_DESKTOP=1; the WS handshake succeeds; a stale.envline is inert. If a mismatch still occurs, the backend log says where its token came from, the boot loop stops after 3 attempts, and the failure names the exact file/line to fix.How to test
Platforms tested
scripts/run_tests.sh(CI-parity runner), web_server targeted suites green (10/10 new tests; the 34-runner baseline failures reproduce identically on pristine origin/main — proven by file-swap stash-test, zero overlap with this diff), vitest 30/30, tsc clean,git diff --checkclean,scripts/check-windows-footguns.pyclean on all changed files.os.environsave/restore; pure TS helper logic; harness spawns real subprocess).Why this matters to users
Before: any stale
HERMES_DASHBOARD_SESSION_TOKENleft in.env(e.g. from a prior remote-gateway setup, #40680, or an old update) silently poisoned desktop startup — the app retried the identical failure for hours with a generic WebSocket error, the log pointed at a misleading headless-404 line, and the only remedy was manually deleting a line from a file most users never open. After: the desktop-injected per-launch token always wins, a mismatch tells you exactly which file and key to remove (or just runhermes setup), the retry loop stops after 3 attempts instead of an hour, headless launches log cleanly, and the CI harness makes the whole class un-regressable.Fixes #40680
Part of #78914
Part of #78915