From 21f34794beb16d36d9fde14942a4e8cc00847a83 Mon Sep 17 00:00:00 2001 From: Victor Nogueira Date: Tue, 25 Aug 2026 11:05:25 +0800 Subject: [PATCH] fix(desktop): recover remote sessions after gateway restart --- apps/desktop/electron/main.ts | 40 +++++++------- apps/desktop/electron/native-oauth.test.ts | 52 ++++++++++++++++++ apps/desktop/electron/native-oauth.ts | 24 +++++++-- .../src/app/settings/gateway-settings.test.ts | 27 +++++++++- .../src/app/settings/gateway-settings.tsx | 26 +++++++-- .../components/boot-failure-overlay.test.tsx | 53 +++++++++++++++++-- .../src/components/boot-failure-overlay.tsx | 10 ++-- apps/desktop/src/global.d.ts | 2 +- contributors/emails/victornogu80@gmail.com | 1 + 9 files changed, 198 insertions(+), 37 deletions(-) create mode 100644 contributors/emails/victornogu80@gmail.com diff --git a/apps/desktop/electron/main.ts b/apps/desktop/electron/main.ts index 82fc5d1690..d33f4434ed 100644 --- a/apps/desktop/electron/main.ts +++ b/apps/desktop/electron/main.ts @@ -14058,14 +14058,17 @@ async function fetchJsonForBackend( ipcMain.handle('hermes:connection-config:probe', async (_event, rawUrl) => probeRemoteAuthMode(rawUrl)) ipcMain.handle('hermes:connection-config:oauth-login', async (_event, rawUrl) => { - // Capability-gated login (RFC 8252). Probe the gateway's public /api/status: - // - advertises "native_pkce" in auth_flows → run the system-browser + - // loopback + PKCE flow. No embedded webview, tokens held by the app - // (encrypted keychain), REST/WS authenticated by bearer — no cookies. - // - older gateway without native_pkce → fall back to the legacy embedded - // BrowserWindow cookie flow, preserving compatibility. - // This is the "observable ladder + compatibility fallback tied to an - // identified older runtime" the desktop guide requires. + // Capability-gated login (RFC 8252). Probe the gateway's public /api/status + // for supported auth_flows and /api/auth/providers for provider capabilities: + // - all providers support password → always use the embedded login window + // (password providers require the dashboard login form; native PKCE + // can never complete for that provider shape) + // - advertises "native_pkce" AND at least one non-password provider → + // run the system-browser + loopback + PKCE flow + // - older gateway with no provider metadata → fall back to the auth_flows + // check (existing compatibility) + // - a failed native login reports the error rather than auto-falling back + // to the embedded flow — one sign-in action opens at most one window. const baseUrl = normalizeRemoteBaseUrl(rawUrl) let statusBody: any = null @@ -14077,7 +14080,10 @@ ipcMain.handle('hermes:connection-config:oauth-login', async (_event, rawUrl) => // own error handling and works against any gated gateway. } - const strategy = resolveLoginStrategy(statusBody) + const authRequired = statusBody && authModeFromStatus(statusBody) === 'oauth' + const providers = authRequired ? await gatewayAuthProviders(baseUrl) : [] + + const strategy = resolveLoginStrategy(statusBody, { providers }) if (strategy === 'native') { try { @@ -14097,10 +14103,10 @@ ipcMain.handle('hermes:connection-config:oauth-login', async (_event, rawUrl) => rememberLog( `[native-oauth] native login failed (${ error instanceof Error ? error.message : String(error) - }); falling back to embedded flow` + })` ) - // Fall through to the embedded flow so a native-flow hiccup (blocked - // loopback, user closed the browser) still lets the user sign in. + + return { ok: false, error: error instanceof Error ? error.message : String(error), connected: false } } } @@ -14119,19 +14125,17 @@ ipcMain.handle('hermes:connection-config:oauth-login', async (_event, rawUrl) => return { ok: true, baseUrl, connected } }) ipcMain.handle('hermes:connection-config:oauth-logout', async (_event, rawUrl) => { - const baseUrl = rawUrl ? normalizeRemoteBaseUrl(rawUrl) : '' - await clearOauthSession(baseUrl || undefined) + const baseUrl = normalizeRemoteBaseUrl(rawUrl) + await clearOauthSession(baseUrl) // Also drop any native (RFC 8252) bearer tokens for this gateway so a // logout clears BOTH auth shapes. - if (baseUrl) { - _clearNativeTokens(baseUrl) - } + _clearNativeTokens(baseUrl) // Report against the SAME liveness notion the Settings indicator uses // (AT-or-RT cookie, or a native token) so a logout that left any session // behind is reflected as still-connected rather than silently signed-out. - const connected = baseUrl ? (await hasLiveOauthSession(baseUrl)) || hasNativeSession(baseUrl) : false + const connected = (await hasLiveOauthSession(baseUrl)) || hasNativeSession(baseUrl) return { ok: true, connected } }) diff --git a/apps/desktop/electron/native-oauth.test.ts b/apps/desktop/electron/native-oauth.test.ts index bc341f33ef..e1cd0a537b 100644 --- a/apps/desktop/electron/native-oauth.test.ts +++ b/apps/desktop/electron/native-oauth.test.ts @@ -85,6 +85,58 @@ test('resolveLoginStrategy picks native only when advertised and not forced', () assert.equal(resolveLoginStrategy(gated, { forceEmbedded: true }), 'embedded') }) +// --- provider-aware strategy --- + +test('resolveLoginStrategy returns embedded when every provider supports password', () => { + const statusBody = { auth_required: true, auth_flows: ['cookie', 'native_pkce'] } + const providers = [{ name: 'basic', supportsPassword: true }] + + assert.equal(resolveLoginStrategy(statusBody, { providers }), 'embedded') +}) + +test('resolveLoginStrategy returns embedded for all-password even without native_pkce in auth_flows', () => { + const statusBody = { auth_required: true, auth_flows: ['cookie'] } + const providers = [{ name: 'basic', supportsPassword: true }] + + assert.equal(resolveLoginStrategy(statusBody, { providers }), 'embedded') +}) + +test('resolveLoginStrategy returns native for native_pkce gateway with non-password provider', () => { + const statusBody = { auth_required: true, auth_flows: ['cookie', 'native_pkce'] } + const providers = [{ name: 'nous', displayName: 'Nous Research', supportsPassword: false }] + + assert.equal(resolveLoginStrategy(statusBody, { providers }), 'native') +}) + +test('resolveLoginStrategy returns native for a mixed provider deployment', () => { + const statusBody = { auth_required: true, auth_flows: ['cookie', 'native_pkce'] } + + const providers = [ + { name: 'basic', supportsPassword: true }, + { name: 'nous', supportsPassword: false } + ] + + assert.equal(resolveLoginStrategy(statusBody, { providers }), 'native') +}) + +test('resolveLoginStrategy preserves existing behavior when providers are empty or missing', () => { + const gated = { auth_required: true, auth_flows: ['cookie', 'native_pkce'] } + const legacy = { auth_required: true, auth_flows: ['cookie'] } + + assert.equal(resolveLoginStrategy(gated, {}), 'native') + assert.equal(resolveLoginStrategy(gated, { providers: [] }), 'native') + assert.equal(resolveLoginStrategy(legacy, {}), 'embedded') + assert.equal(resolveLoginStrategy(legacy, { providers: [] }), 'embedded') +}) + +test('resolveLoginStrategy ignores providers with no name', () => { + const statusBody = { auth_required: true, auth_flows: ['cookie', 'native_pkce'] } + const providers = [{ supportsPassword: true }] + + // The unnamed provider is filtered out — still falls through to auth_flows. + assert.equal(resolveLoginStrategy(statusBody, { providers }), 'native') +}) + // --- URL building --- test('buildNativeAuthorizeUrl encodes params and honours a path prefix', () => { diff --git a/apps/desktop/electron/native-oauth.ts b/apps/desktop/electron/native-oauth.ts index 16e2d960f8..5d3af1f6e2 100644 --- a/apps/desktop/electron/native-oauth.ts +++ b/apps/desktop/electron/native-oauth.ts @@ -28,6 +28,8 @@ import { createHash, randomBytes } from 'node:crypto' +import { type AdvertisedAuthProvider, oauthGuardMayHardFail } from './native-auth-decisions' + // The gateway status field that lists supported auth flows. See // hermes_cli/web_server.py status handler. const NATIVE_FLOW_ID = 'native_pkce' @@ -78,19 +80,33 @@ export function statusSupportsNativeFlow(statusBody: any): boolean { } /** - * Decide the login strategy for a gated gateway from its status body. - * Returns 'native' when the gateway can do RFC 8252 AND we're not forced to - * the legacy path; 'embedded' otherwise (older gateway ⇒ webview fallback). + * Decide the login strategy for a gated gateway from its status body and + * advertised provider capabilities. + * + * Returns 'native' when the gateway advertises native_pkce AND at least one + * non-password provider is available; 'embedded' when all providers are + * password-only, the gateway lacks native_pkce, or forceEmbedded is set. + * + * Provider metadata is discovered from /api/auth/providers (separate from + * /api/status). When absent (older gateway), the decision falls through to + * the auth_flows check — existing compatibility is preserved. * * `forceEmbedded` lets a user/setting or an env override pin the legacy flow * (e.g. a corporate proxy that blocks loopback). Precedence written down here, * in one place, as a pure function — per the desktop "observable ladder" rule. */ -export function resolveLoginStrategy(statusBody: any, opts: { forceEmbedded?: boolean } = {}): 'native' | 'embedded' { +export function resolveLoginStrategy( + statusBody: any, + opts: { forceEmbedded?: boolean; providers?: AdvertisedAuthProvider[] } = {} +): 'native' | 'embedded' { if (opts.forceEmbedded) { return 'embedded' } + if (!oauthGuardMayHardFail(opts.providers)) { + return 'embedded' + } + return statusSupportsNativeFlow(statusBody) ? 'native' : 'embedded' } diff --git a/apps/desktop/src/app/settings/gateway-settings.test.ts b/apps/desktop/src/app/settings/gateway-settings.test.ts index a221a4c5ac..3cedbb9da2 100644 --- a/apps/desktop/src/app/settings/gateway-settings.test.ts +++ b/apps/desktop/src/app/settings/gateway-settings.test.ts @@ -1,6 +1,31 @@ import { describe, expect, it } from 'vitest' -import { savedCloudConnectionUrl } from './gateway-settings' +import { normalizeGatewaySettingsState, savedCloudConnectionUrl } from './gateway-settings' + +describe('normalizeGatewaySettingsState', () => { + it('fills missing and undefined persisted fields with canonical defaults', () => { + const normalized = normalizeGatewaySettingsState({ + mode: 'remote', + remoteAuthMode: undefined, + remoteUrl: 'https://gateway.example' + }) + + expect(normalized.mode).toBe('remote') + expect(normalized.remoteAuthMode).toBe('token') + expect(normalized.remoteUrl).toBe('https://gateway.example') + expect(normalized.sshHost).toBe('') + expect(normalized.sshPort).toBeNull() + expect(normalized.secureTokenStorage).toBe(true) + }) + + it('returns an independent default state for invalid persisted data', () => { + const first = normalizeGatewaySettingsState(null) + const second = normalizeGatewaySettingsState(undefined) + + expect(first).toEqual(second) + expect(first).not.toBe(second) + }) +}) describe('savedCloudConnectionUrl', () => { it('normalizes the URL of a persisted cloud connection', () => { diff --git a/apps/desktop/src/app/settings/gateway-settings.tsx b/apps/desktop/src/app/settings/gateway-settings.tsx index 4a384d4eb1..89b533e556 100644 --- a/apps/desktop/src/app/settings/gateway-settings.tsx +++ b/apps/desktop/src/app/settings/gateway-settings.tsx @@ -37,7 +37,7 @@ type ProbeStatus = 'idle' | 'probing' | 'done' | 'error' // Hermes Cloud discovery lifecycle for the cloud-mode panel. type CloudDiscoverStatus = 'idle' | 'loading' | 'done' | 'error' -interface GatewaySettingsState { +export interface GatewaySettingsState { envOverride: boolean mode: Mode remoteAuthMode: AuthMode @@ -81,6 +81,18 @@ const EMPTY_STATE: GatewaySettingsState = { sshRemoteProfile: '' } +export function normalizeGatewaySettingsState( + config: Partial | null | undefined +): GatewaySettingsState { + if (!config || typeof config !== 'object') { + return { ...EMPTY_STATE } + } + + const defined = Object.fromEntries(Object.entries(config).filter(([, value]) => value != null)) + + return { ...EMPTY_STATE, ...defined } +} + export function savedCloudConnectionUrl(config: Pick): string { return config.mode === 'cloud' ? config.remoteUrl.trim().replace(/\/+$/, '').toLowerCase() : '' } @@ -199,8 +211,10 @@ export function GatewaySettings({ embedded = false }: { embedded?: boolean } = { } const acceptSavedConfig = (config: GatewaySettingsState) => { - setState(config) - setConnectedCloudUrl(savedCloudConnectionUrl(config)) + const normalized = normalizeGatewaySettingsState(config) + + setState(normalized) + setConnectedCloudUrl(savedCloudConnectionUrl(normalized)) } // When set, the plain-text opt-in dialog is open; `apply` remembers whether @@ -622,11 +636,15 @@ export function GatewaySettings({ embedded = false }: { embedded?: boolean } = { } const signOut = async () => { + if (!trimmedUrl) { + return + } + const seq = ++signingSeq.current setSigningIn(true) try { - await window.hermesDesktop.oauthLogoutConnectionConfig(trimmedUrl || undefined) + await window.hermesDesktop.oauthLogoutConnectionConfig(trimmedUrl) const refreshed = await window.hermesDesktop.getConnectionConfig(null) if (seq !== signingSeq.current) { diff --git a/apps/desktop/src/components/boot-failure-overlay.test.tsx b/apps/desktop/src/components/boot-failure-overlay.test.tsx index db03d6d046..6d5e8810dd 100644 --- a/apps/desktop/src/components/boot-failure-overlay.test.tsx +++ b/apps/desktop/src/components/boot-failure-overlay.test.tsx @@ -1,5 +1,5 @@ import { cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { $desktopBoot } from '@/store/boot' import { $desktopOnboarding } from '@/store/onboarding' @@ -25,11 +25,11 @@ function failBoot() { }) } -function stubDesktop(config: Record) { +function stubDesktop(config: Record, overrides: Record = {}) { const original = window.hermesDesktop Object.defineProperty(window, 'hermesDesktop', { configurable: true, - value: { getRecentLogs: async () => ({ lines: [] }), getConnectionConfig: async () => config } + value: { getRecentLogs: async () => ({ lines: [] }), getConnectionConfig: async () => config, ...overrides } }) return () => Object.defineProperty(window, 'hermesDesktop', { configurable: true, value: original }) @@ -99,6 +99,53 @@ describe('BootFailureOverlay', () => { } }) + it('opens gateway settings with a partial persisted remote config', async () => { + const restore = stubDesktop({ mode: 'remote', remoteAuthMode: undefined, remoteUrl: undefined }) + + try { + render() + fireEvent.click(screen.getByRole('button', { name: /gateway settings/i })) + + expect(await screen.findByRole('button', { name: /back/i })).toBeTruthy() + expect(screen.queryByRole('button', { name: /retry/i })).toBeNull() + } finally { + restore() + } + }) + + it('clears and signs in only the failed gateway once', async () => { + const gatewayUrl = 'http://100.116.104.53:9191' + const logout = vi.fn().mockResolvedValue({ ok: true, connected: false }) + const login = vi.fn().mockResolvedValue({ ok: true, connected: false }) + + const restore = stubDesktop( + { + ...remoteToken, + remoteAuthMode: 'oauth', + remoteOauthConnected: false, + remoteTokenSet: false, + remoteUrl: gatewayUrl + }, + { + oauthLoginConnectionConfig: login, + oauthLogoutConnectionConfig: logout, + probeConnectionConfig: vi.fn().mockResolvedValue({ providers: [{ id: 'basic', type: 'password' }] }) + } + ) + + try { + render() + fireEvent.click(await screen.findByRole('button', { name: /sign out & sign in/i })) + + await waitFor(() => expect(login).toHaveBeenCalledWith(gatewayUrl)) + expect(logout).toHaveBeenCalledTimes(1) + expect(logout).toHaveBeenCalledWith(gatewayUrl) + expect(login).toHaveBeenCalledTimes(1) + } finally { + restore() + } + }) + it('shows the Nous Cloud down recovery when the backend flags isCloudBackendDown', async () => { const restore = stubDesktop(remoteToken) $desktopBoot.set({ diff --git a/apps/desktop/src/components/boot-failure-overlay.tsx b/apps/desktop/src/components/boot-failure-overlay.tsx index 2d71eca10c..8c7e47f07d 100644 --- a/apps/desktop/src/components/boot-failure-overlay.tsx +++ b/apps/desktop/src/components/boot-failure-overlay.tsx @@ -163,12 +163,10 @@ export function BootFailureOverlay() { setBusy(null) } - // Clear the OAuth partition first, then open the gateway's login window - // (username/password form or OAuth redirect — the desktop drives both). A - // partition-wide sign-out drops stale gateway AND identity-provider cookies so - // an expired session can't silently bounce us back into the same state. On a + // Clear this gateway's stale auth first, then open its login window + // (username/password form or OAuth redirect — the desktop drives both). On a // successful sign-in the cookie is re-established; reload so boot mints a fresh - // ticket against a live session. + // ticket against a live session without disturbing other saved gateways. const signInRemote = async () => { if (!remoteReauth) { return @@ -177,7 +175,7 @@ export function BootFailureOverlay() { setBusy('signin') try { - await window.hermesDesktop?.oauthLogoutConnectionConfig?.() + await window.hermesDesktop?.oauthLogoutConnectionConfig?.(remoteReauth.url) const result = await window.hermesDesktop?.oauthLoginConnectionConfig(remoteReauth.url) if (result?.connected) { diff --git a/apps/desktop/src/global.d.ts b/apps/desktop/src/global.d.ts index f3e0d0381e..398eec1b7a 100644 --- a/apps/desktop/src/global.d.ts +++ b/apps/desktop/src/global.d.ts @@ -186,7 +186,7 @@ declare global { sshResolveHost: (host: string) => Promise probeConnectionConfig: (remoteUrl: string) => Promise oauthLoginConnectionConfig: (remoteUrl: string) => Promise - oauthLogoutConnectionConfig: (remoteUrl?: string) => Promise + oauthLogoutConnectionConfig: (remoteUrl: string) => Promise // Hermes Cloud: one portal login powers discovery + silent per-agent // sign-in (cloud-auto-discovery Phase 3). cloud: { diff --git a/contributors/emails/victornogu80@gmail.com b/contributors/emails/victornogu80@gmail.com new file mode 100644 index 0000000000..56177828ee --- /dev/null +++ b/contributors/emails/victornogu80@gmail.com @@ -0,0 +1 @@ +victorftrdba