fix(desktop): gate OAuth remote connect on AT-or-RT, not access token alone - #39464
Conversation
… alone The desktop OAuth remote-gateway path gated connectivity on hasOauthSessionCookie(), which checks only the access-token cookie (hermes_session_at, ~15 min TTL). The moment that cookie's Max-Age lapsed, Electron's cookie jar dropped it and both resolveRemoteBackend() and sanitizeDesktopConnectionConfig() reported "not signed in" — forcing a full IDP re-login every ~15 min — even though a valid 24h refresh-token cookie (hermes_session_rt) was sitting in the same jar. The desktop OAuth code (2026-06-04) was written against the obsolete "contract v1 issues no refresh token" model, two days after #37247 re-introduced server-side transparent refresh: Portal now issues a 24h rotating, reuse-detected refresh token, and the gateway middleware (_attempt_refresh) rotates a fresh AT from the RT on the next authenticated request. So an expired-AT/live-RT session is fully connectable — the desktop just never let the request through. Fix: - connection-config.cjs: add RT_COOKIE_VARIANTS + cookiesHaveLiveSession() (true when EITHER a live AT or RT cookie is present). Keep cookiesHaveSession() AT-only for callers that need that specific signal. - main.cjs: add hasLiveOauthSession(); resolveRemoteBackend()'s oauth branch now early-outs only when NEITHER cookie is present, otherwise uses the ws-ticket mint as the authoritative liveness probe (that POST carries the RT cookie and triggers the server-side AT rotation). A real 401 still surfaces as needsOauthLogin. Settings indicator + oauth-logout report against the same AT-or-RT notion. - Remove the stale "contract v1 / NO refresh token" docstrings in cookies.py and the verify_session comments in the Nous provider that contradicted #37247. Tests: +57 lines in connection-config.test.cjs covering the RT-only "still connectable" case. node --test: 32/32. dashboard-auth + nous-provider Python suites: 223/223. Note: server-side files (hermes_cli/dashboard_auth/, plugins/dashboard_auth/) are comment/docstring-only here, but this touches outside apps/desktop/ so it needs Teknium review.
🔎 Lint report:
|
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Bug fix: desktop app OAuth connectivity checks now look for EITHER a live access-token cookie OR a (longer-lived) refresh-token cookie. Previously gating only on the AT cookie caused spurious "not signed in" states every ~15 min even when a valid 24h RT was present. The fix adds cookiesHaveLiveSession, hasLiveOauthSession, and updates resolveRemoteBackend accordingly.
Design
- The two-function distinction is clear: cookiesHaveSession (AT-only) answers "is there an unexpired AT right now?"; cookiesHaveLiveSession (AT-or-RT) answers "is the user signed in at all?" for display/early-out purposes.
- The authoritative liveness check remains the ws-ticket mint at connect time (which triggers server-side AT rotation). This is the right architecture.
- Comment in connection-config.cjs documents the Portal NAS #293 / hermes #37247 contract change clearly.
- Dashboard auth cookies.py updated to reflect the 24h rotating RT (updating the old "30 days + deprecated" comments).
Tests
- Extensive new tests for cookiesHaveLiveSession covering AT-only, RT-only, both, empty values, unrelated cookies.
No concerns
Reviewed by Hermes Agent
Problem
The desktop OAuth remote-gateway path gated connectivity on
hasOauthSessionCookie(), which checks only the access-token cookie (hermes_session_at, ~15 min TTL). The instant that cookie's Max-Age lapsed, Electron's cookie jar dropped it and bothresolveRemoteBackend()andsanitizeDesktopConnectionConfig()reported "not signed in" — forcing a full IDP re-login every ~15 minutes — even though a valid 24h refresh-token cookie (hermes_session_rt) was sitting in the same jar.Impact case
Hits every desktop user on an OAuth (gated/hosted) remote gateway — the common path for the headline "connect to a hosted gateway" feature, not an exotic config. Symptom: you get bounced to the IDP sign-in screen roughly every 15 minutes of use, despite holding a 24h credential the server is fully prepared to honor.
Root cause
The desktop OAuth code was written against the obsolete "contract v1 issues no refresh token" model — and landed two days after #37247 re-introduced server-side transparent refresh. Since #37247, Nous Portal issues a 24h rotating, reuse-detected refresh token, and the gateway middleware (
_attempt_refresh) transparently rotates a fresh AT from the RT on the next authenticated request. So an expired-AT/live-RT session is fully connectable — the desktop just never let the request through; the AT-only gate short-circuited before the ws-ticket mint (which would have triggered the server refresh).Fix
connection-config.cjs: addRT_COOKIE_VARIANTS+cookiesHaveLiveSession()(true when either a live AT or RT cookie is present).cookiesHaveSession()stays AT-only for callers that need that specific signal.main.cjs: addhasLiveOauthSession().resolveRemoteBackend()'s oauth branch now early-outs only when neither cookie is present; otherwise it uses the ws-ticket mint as the authoritative liveness probe (that POST carries the RT cookie and triggers the server-side AT rotation). A real 401 still surfaces asneedsOauthLogin. Settings indicator + oauth-logout report against the same AT-or-RT notion.hermes_cli/dashboard_auth/cookies.pyand theverify_sessioncomments inplugins/dashboard_auth/nous/__init__.pythat contradicted feat(dashboard-auth): rotate dashboard sessions via refresh token #37247. (Comment/docstring-only — no behavior change server-side.)Tests
node --test(desktop): 32/32 — +57 lines covering the RT-only "still connectable" case inconnection-config.test.cjs.Manual E2E (live, against a real gated gateway)
Validated against a local OAuth gateway (
localhost:9119) by waiting out a natural AT expiry:Set-Cookiewas captured (closing the reuse-detection risk: a dropped rotated cookie would have replayed a stale RT and revoked the session).Review note
Touches
hermes_cli/dashboard_auth/andplugins/dashboard_auth/(comment/docstring-only) in addition toapps/desktop/, so this is outside the pure-Docker lane and needs @teknium1 review.