From a75abe14544eb1bcc05ead001d64f7d2013b0e12 Mon Sep 17 00:00:00 2001 From: seavor Date: Mon, 20 Apr 2026 21:24:43 -0500 Subject: [PATCH] fix login and known hosts --- webclient/src/__test-utils__/storeFixtures.ts | 1 + .../src/api/response/SessionResponseImpl.ts | 4 +- .../src/components/KnownHosts/KnownHosts.tsx | 6 +- .../KnownHosts/useKnownHostsComponent.ts | 34 +++-- webclient/src/containers/Login/Login.spec.tsx | 100 +++++++++++- webclient/src/containers/Login/useLogin.ts | 15 +- .../src/forms/LoginForm/LoginForm.spec.tsx | 91 ++++++++++- webclient/src/forms/LoginForm/LoginForm.tsx | 75 ++++++--- webclient/src/forms/LoginForm/useLoginForm.ts | 22 ++- .../src/store/server/server.actions.spec.ts | 5 +- webclient/src/store/server/server.actions.ts | 2 - .../src/store/server/server.dispatch.spec.ts | 6 +- webclient/src/store/server/server.dispatch.ts | 7 +- .../src/store/server/server.interfaces.ts | 4 + webclient/src/store/server/server.reducer.ts | 17 +++ .../src/store/server/server.selectors.ts | 2 + webclient/src/store/server/server.types.ts | 1 + webclient/src/types/server.ts | 4 + webclient/src/websocket/WebClient.spec.ts | 63 +++++++- webclient/src/websocket/WebClient.ts | 75 ++++++--- .../events/session/serverIdentification.ts | 7 +- .../session-login-flow.integration.spec.ts | 143 ++++++++++++++++++ .../websocket/services/WebSocketService.ts | 7 +- .../src/websocket/types/WebClientResponse.ts | 2 +- webclient/src/websocket/utils/index.ts | 1 + .../websocket/utils/passwordHasher.spec.ts | 20 ++- 26 files changed, 618 insertions(+), 96 deletions(-) create mode 100644 webclient/src/websocket/events/session/session-login-flow.integration.spec.ts diff --git a/webclient/src/__test-utils__/storeFixtures.ts b/webclient/src/__test-utils__/storeFixtures.ts index e4026c9f0..0f0c3efd7 100644 --- a/webclient/src/__test-utils__/storeFixtures.ts +++ b/webclient/src/__test-utils__/storeFixtures.ts @@ -27,6 +27,7 @@ function makeUser(overrides: Partial = {}): Data.ServerInf export const disconnectedState: Partial = { server: { initialized: false, + testConnectionStatus: null, buddyList: {}, ignoreList: {}, status: { diff --git a/webclient/src/api/response/SessionResponseImpl.ts b/webclient/src/api/response/SessionResponseImpl.ts index 261541ad9..ade27a36f 100644 --- a/webclient/src/api/response/SessionResponseImpl.ts +++ b/webclient/src/api/response/SessionResponseImpl.ts @@ -30,8 +30,8 @@ export class SessionResponseImpl implements WebsocketTypes.ISessionResponse { ServerDispatch.connectionFailed(); } - testConnectionSuccessful(): void { - ServerDispatch.testConnectionSuccessful(); + testConnectionSuccessful(serverOptions: number): void { + ServerDispatch.testConnectionSuccessful(serverOptions); } testConnectionFailed(): void { diff --git a/webclient/src/components/KnownHosts/KnownHosts.tsx b/webclient/src/components/KnownHosts/KnownHosts.tsx index 0717db31c..57e0621a3 100644 --- a/webclient/src/components/KnownHosts/KnownHosts.tsx +++ b/webclient/src/components/KnownHosts/KnownHosts.tsx @@ -61,7 +61,7 @@ const KnownHosts = ({ input, meta, disabled }: KnownHostsProps) => { const { hosts, selectedHost, - testingConnection, + testConnectionStatus, dialogState, onPick, openAddKnownHostDialog, @@ -119,8 +119,8 @@ const KnownHosts = ({ input, meta, disabled }: KnownHostsProps) => {
-
- {testingConnection === TestConnection.FAILED ? ( +
+ {testConnectionStatus === TestConnection.FAILED ? ( ) : ( diff --git a/webclient/src/components/KnownHosts/useKnownHostsComponent.ts b/webclient/src/components/KnownHosts/useKnownHostsComponent.ts index 26508a5a8..be6afa056 100644 --- a/webclient/src/components/KnownHosts/useKnownHostsComponent.ts +++ b/webclient/src/components/KnownHosts/useKnownHostsComponent.ts @@ -4,8 +4,9 @@ import { useTranslation } from 'react-i18next'; import { useToast } from '@app/components'; import { LoadingState, useKnownHosts, useReduxEffect, useWebClient } from '@app/hooks'; import { getHostPort, HostDTO } from '@app/services'; -import { ServerTypes } from '@app/store'; +import { ServerDispatch, ServerSelectors, ServerTypes, useAppSelector } from '@app/store'; import { App } from '@app/types'; +import { passwordSaltSupported } from '@app/websocket'; export enum TestConnection { TESTING = 'testing', @@ -16,7 +17,7 @@ export enum TestConnection { export interface KnownHostsComponent { hosts: App.Host[]; selectedHost: App.Host | undefined; - testingConnection: TestConnection | null; + testConnectionStatus: TestConnection | null; dialogState: { open: boolean; edit: HostDTO | null }; onPick: (id: number) => Promise; openAddKnownHostDialog: () => void; @@ -55,9 +56,13 @@ export function useKnownHostsComponent({ edit: null, }); - const [testingConnection, setTestingConnection] = useState(null); - // Tracks the host currently awaiting a testConnection response. If null when a - // response arrives, the caller has moved on — ignore the stale reply. + // UI status lives in redux (see ServerSelectors.getTestConnectionStatus) so + // the LoginForm can gate its submit button + hashing-capability UI on the + // same signal. Tracks-the-host-pending lives in a ref — redux doesn't know + // the Host.id, and we need it to persist `supportsHashedPassword` on success. + const testConnectionStatus = useAppSelector(ServerSelectors.getTestConnectionStatus) as + | TestConnection + | null; const pendingTestRef = useRef(null); const selectedHost = @@ -66,7 +71,7 @@ export function useKnownHostsComponent({ const testConnection = (host: HostDTO) => { pendingTestRef.current = host; - setTestingConnection(TestConnection.TESTING); + ServerDispatch.testConnectionStarted(); webClient.request.authentication.testConnection({ ...getHostPort(host) }); }; @@ -78,19 +83,20 @@ export function useKnownHostsComponent({ testConnection(selectedHost); }, [selectedHost]); - useReduxEffect(() => { - if (!pendingTestRef.current) { + useReduxEffect<{ serverOptions: number }>(({ payload: { serverOptions } }) => { + const host = pendingTestRef.current; + if (!host) { return; } - setTestingConnection(TestConnection.SUCCESS); pendingTestRef.current = null; + + const supportsHashedPassword = passwordSaltSupported(serverOptions); + if (host.id != null && host.supportsHashedPassword !== supportsHashedPassword) { + void knownHosts.update(host.id, { supportsHashedPassword }); + } }, ServerTypes.TEST_CONNECTION_SUCCESSFUL, []); useReduxEffect(() => { - if (!pendingTestRef.current) { - return; - } - setTestingConnection(TestConnection.FAILED); pendingTestRef.current = null; }, ServerTypes.TEST_CONNECTION_FAILED, []); @@ -163,7 +169,7 @@ export function useKnownHostsComponent({ return { hosts, selectedHost, - testingConnection, + testConnectionStatus, dialogState, onPick, openAddKnownHostDialog, diff --git a/webclient/src/containers/Login/Login.spec.tsx b/webclient/src/containers/Login/Login.spec.tsx index 3ba49352d..33b57263c 100644 --- a/webclient/src/containers/Login/Login.spec.tsx +++ b/webclient/src/containers/Login/Login.spec.tsx @@ -172,8 +172,13 @@ describe('Login — logout cycle (same JS session)', () => { await flushEffects(); first.unmount(); + // Submit button stays disabled until testConnectionStatus resolves to 'success'; + // preload it so the click actually dispatches. const { getByRole, queryByText } = renderWithProviders(, { - preloadedState: disconnectedState, + preloadedState: { + ...disconnectedState, + server: { ...(disconnectedState.server as any), testConnectionStatus: 'success' }, + }, }); await flushEffects(); @@ -199,3 +204,96 @@ describe('Login — refresh cycle', () => { }); }); }); + +// End-to-end regression: the symptom reported on this branch was "save-password +// checkbox shows, login succeeds, but HostDTO.hashedPassword stays empty in +// Dexie". These tests wire the full chain — auto-login fires onSubmitLogin +// (capturing the form in rememberLoginRef), then a store.dispatch of +// LOGIN_SUCCESSFUL drives the useReduxEffect that calls knownHosts.update. +describe('Login — LOGIN_SUCCESSFUL → knownHosts persistence', () => { + const armWithUpdate = () => { + const update = vi.fn().mockResolvedValue(undefined); + const host = makeHost({ + id: 7, + remember: true, + userName: 'alice', + hashedPassword: 'stored-hash', + supportsHashedPassword: true, + lastSelected: true, + }); + hoisted.useSettings.mockReturnValue( + makeSettingsHook({ + status: LoadingState.READY, + value: makeSettings({ autoConnect: true }), + update: vi.fn().mockResolvedValue(undefined), + }) + ); + hoisted.useKnownHosts.mockReturnValue( + makeKnownHostsHook({ + status: LoadingState.READY, + value: { hosts: [host], selectedHost: host }, + update, + }) + ); + hoisted.getSettings.mockResolvedValue(makeSettings({ autoConnect: true })); + hoisted.getKnownHosts.mockResolvedValue({ hosts: [host], selectedHost: host }); + return { update, hostId: host.id! }; + }; + + test('persists userName + hashedPassword when loginSuccessful carries a real hash', async () => { + const { update, hostId } = armWithUpdate(); + + const { store } = renderWithProviders(, { + preloadedState: { + ...disconnectedState, + server: { ...(disconnectedState.server as any), testConnectionStatus: 'success' }, + }, + }); + await waitFor(() => { + expect(hoisted.mockWebClient.request.authentication.login).toHaveBeenCalledTimes(1); + }); + + store.dispatch({ + type: 'server/loginSuccessful', + payload: { options: { hashedPassword: 'real-hash-xyz' } }, + }); + await flushEffects(); + + expect(update).toHaveBeenCalledWith(hostId, { + remember: true, + userName: 'alice', + hashedPassword: 'real-hash-xyz', + }); + }); + + test('does not persist credentials when hashedPassword is empty (empty-salt fallback)', async () => { + const { update, hostId } = armWithUpdate(); + + const { store } = renderWithProviders(, { + preloadedState: { + ...disconnectedState, + server: { ...(disconnectedState.server as any), testConnectionStatus: 'success' }, + }, + }); + await waitFor(() => { + expect(hoisted.mockWebClient.request.authentication.login).toHaveBeenCalledTimes(1); + }); + + // Empty-salt fallback path: login went through as plain password, so the + // response layer has no hash to carry forward. + store.dispatch({ + type: 'server/loginSuccessful', + payload: { options: { hashedPassword: undefined } }, + }); + await flushEffects(); + + // Guard in useLogin.updateHost clears remember+credentials so next load + // reflects that save-password wasn't honoured — no stale "checked" checkbox + // sitting against a null hash. + expect(update).toHaveBeenCalledWith(hostId, { + remember: false, + userName: null, + hashedPassword: null, + }); + }); +}); diff --git a/webclient/src/containers/Login/useLogin.ts b/webclient/src/containers/Login/useLogin.ts index 4baded8ad..724c00dcf 100644 --- a/webclient/src/containers/Login/useLogin.ts +++ b/webclient/src/containers/Login/useLogin.ts @@ -148,16 +148,23 @@ export function useLogin(): Login { }, [ServerTypes.CONNECTION_FAILED, ServerTypes.LOGIN_FAILED], []); const updateHost = ( - hashedPassword: string, + hashedPassword: string | undefined, { selectedHost, remember, userName }: LoginFormValues, ) => { if (selectedHost.id == null) { return; } + // Only honour the Remember checkbox when we actually received a hash to + // store. A server that advertises SupportsPasswordHash but returns an empty + // salt falls through to a plain-password login (no hash produced); writing + // `remember: true` with a null hash would leave the checkbox visibly + // checked on next load while the stored-password flow silently couldn't + // activate. Resetting to the unchecked state makes the failure visible. + const persistCredentials = remember && Boolean(hashedPassword); knownHosts.update(selectedHost.id, { - remember, - userName: remember ? userName : null, - hashedPassword: remember ? hashedPassword : null, + remember: persistCredentials, + userName: persistCredentials ? userName : null, + hashedPassword: persistCredentials ? hashedPassword : null, }); }; diff --git a/webclient/src/forms/LoginForm/LoginForm.spec.tsx b/webclient/src/forms/LoginForm/LoginForm.spec.tsx index 0dcc8f071..121af8b46 100644 --- a/webclient/src/forms/LoginForm/LoginForm.spec.tsx +++ b/webclient/src/forms/LoginForm/LoginForm.spec.tsx @@ -28,7 +28,7 @@ beforeAll(() => { }); describe('LoginForm — regression: settings.autoConnect is not clobbered by host state', () => { - test('selecting a host with remember=false does NOT call settings.update', () => { + test('selecting a hashed-capable host with remember=false does NOT call settings.update', () => { const update = vi.fn().mockResolvedValue(undefined); hoisted.mockUseSettings.mockReturnValue( @@ -44,6 +44,7 @@ describe('LoginForm — regression: settings.autoConnect is not clobbered by hos remember: false, userName: undefined, hashedPassword: undefined, + supportsHashedPassword: true, lastSelected: true, }); hoisted.mockUseKnownHosts.mockReturnValue( @@ -78,6 +79,7 @@ describe('LoginForm — regression: settings.autoConnect is not clobbered by hos remember: true, userName: 'joe', hashedPassword: 'abc', + supportsHashedPassword: true, lastSelected: true, }); hoisted.mockUseKnownHosts.mockReturnValue( @@ -95,3 +97,90 @@ describe('LoginForm — regression: settings.autoConnect is not clobbered by hos expect(onSubmit).not.toHaveBeenCalled(); }); }); + +describe('LoginForm — hashed-password gating', () => { + // The gate requires BOTH a successful test-connection AND host.supportsHashedPassword=true; + // preload testConnectionStatus='success' so the host flag is the only lever under test. + const testedState = { + ...disconnectedState, + server: { ...(disconnectedState.server as any), testConnectionStatus: 'success' as const }, + }; + + const renderWith = (host: ReturnType) => { + hoisted.mockUseSettings.mockReturnValue( + makeSettingsHook({ + status: LoadingState.READY, + value: makeSettings({ autoConnect: false }), + update: vi.fn().mockResolvedValue(undefined), + }) + ); + hoisted.mockUseKnownHosts.mockReturnValue( + makeKnownHostsHook({ + status: LoadingState.READY, + value: { hosts: [host], selectedHost: host }, + }) + ); + return renderWithProviders( + , + { preloadedState: testedState } + ); + }; + + const hasRemember = (root: HTMLElement) => Boolean(root.querySelector('input[name="remember"]')); + const hasAutoConnect = (root: HTMLElement) => + Boolean(Array.from(root.querySelectorAll('.MuiFormControlLabel-root')).find((n) => + n.textContent?.includes('LoginForm.label.autoConnect'), + )); + + test('hides Remember + Auto Connect when the host does not support hashed passwords', () => { + const { container } = renderWith( + makeHost({ id: 1, supportsHashedPassword: false, lastSelected: true }) + ); + expect(hasRemember(container)).toBe(false); + expect(hasAutoConnect(container)).toBe(false); + }); + + test('hides Remember + Auto Connect when the host has never been test-connected', () => { + const { container } = renderWith( + makeHost({ id: 1, supportsHashedPassword: undefined, lastSelected: true }) + ); + expect(hasRemember(container)).toBe(false); + expect(hasAutoConnect(container)).toBe(false); + }); + + test('shows Remember + Auto Connect when the host supports hashed passwords', () => { + const { container } = renderWith( + makeHost({ id: 1, supportsHashedPassword: true, lastSelected: true }) + ); + expect(hasRemember(container)).toBe(true); + expect(hasAutoConnect(container)).toBe(true); + }); + + test('clears settings.autoConnect when selecting a naked-password host with it previously on', () => { + const update = vi.fn().mockResolvedValue(undefined); + hoisted.mockUseSettings.mockReturnValue( + makeSettingsHook({ + status: LoadingState.READY, + value: makeSettings({ autoConnect: true }), + update, + }) + ); + const host = makeHost({ + id: 1, + supportsHashedPassword: false, + remember: true, + lastSelected: true, + }); + hoisted.mockUseKnownHosts.mockReturnValue( + makeKnownHostsHook({ + status: LoadingState.READY, + value: { hosts: [host], selectedHost: host }, + }) + ); + renderWithProviders( + , + { preloadedState: disconnectedState } + ); + expect(update).toHaveBeenCalledWith({ autoConnect: false }); + }); +}); diff --git a/webclient/src/forms/LoginForm/LoginForm.tsx b/webclient/src/forms/LoginForm/LoginForm.tsx index b301c2730..1bd3fb4ee 100644 --- a/webclient/src/forms/LoginForm/LoginForm.tsx +++ b/webclient/src/forms/LoginForm/LoginForm.tsx @@ -12,11 +12,22 @@ import { CheckboxField, InputField, KnownHosts } from '@app/components'; import type { FormErrors } from '@app/forms'; import { LoadingState, useKnownHosts, useSettings } from '@app/hooks'; import { HostDTO } from '@app/services'; +import { ServerSelectors, TestConnectionStatus, useAppSelector } from '@app/store'; import { useLoginFormBody } from './useLoginForm'; import './LoginForm.css'; +// Remember Password and Auto Connect both require server-side password hashing +// to be useful (no hash to save = nothing to resume with). Test-connection +// captures capability from ServerIdentification before the user ever logs in, +// so we can afford a strict "hidden until a completed test proves supported" gate. +const hostSupportsHashedPassword = ( + host: HostDTO | undefined, + testConnectionStatus: TestConnectionStatus, +): boolean => + testConnectionStatus === 'success' && host?.supportsHashedPassword === true; + export interface LoginFormValues { userName: string; password: string; @@ -47,6 +58,7 @@ const LoginFormBody = ({ const STORED_PASSWORD_LABEL = t('LoginForm.label.savedPassword'); const { + selectedHost, useStoredPasswordLabel, setUseStoredPasswordLabel, onSelectedHostChange, @@ -56,6 +68,13 @@ const LoginFormBody = ({ passwordFieldBlur, } = useLoginFormBody(form); + const testConnectionStatus = useAppSelector(ServerSelectors.getTestConnectionStatus); + const showHashingGatedOptions = hostSupportsHashedPassword(selectedHost, testConnectionStatus); + // Login is only meaningful once we know the host is reachable + speaks the + // Cockatrice protocol. Keep the button disabled until test-connection resolves + // to 'success'; re-disable on any subsequent re-test. + const loginDisabled = disableSubmitButton || testConnectionStatus !== 'success'; + return (
@@ -80,12 +99,16 @@ const LoginFormBody = ({ />
- - {onRememberChange} + {showHashingGatedOptions && ( + <> + + {onRememberChange} + + )}
-
- - {({ input }) => ( - onUserToggleAutoConnect(checked, input.onChange)} - color="primary" - /> - } - /> - )} - -
+ {showHashingGatedOptions && ( +
+ + {({ input }) => ( + onUserToggleAutoConnect(checked, input.onChange)} + color="primary" + /> + } + /> + )} + +
+ )}
diff --git a/webclient/src/forms/LoginForm/useLoginForm.ts b/webclient/src/forms/LoginForm/useLoginForm.ts index 4cc16dae6..d08eeb256 100644 --- a/webclient/src/forms/LoginForm/useLoginForm.ts +++ b/webclient/src/forms/LoginForm/useLoginForm.ts @@ -5,6 +5,7 @@ import { LoadingState, useKnownHosts, useSettings } from '@app/hooks'; import { HostDTO } from '@app/services'; export interface LoginFormBody { + selectedHost: HostDTO | undefined; useStoredPasswordLabel: boolean; setUseStoredPasswordLabel: (v: boolean) => void; onSelectedHostChange: (host: HostDTO | undefined) => void; @@ -35,16 +36,30 @@ export function useLoginFormBody(form: MinimalFormApi): LoginFormBody { const togglePasswordLabel = (on: boolean) => setUseStoredPasswordLabel(on); - // @critical Host-sync must not touch autoConnect — app-level setting, not per-host. + // @critical Host-sync normally must not touch autoConnect — it's an app-level + // setting, not per-host. The single exception is switching to a host that is + // *proven* naked (supportsHashedPassword === false): the Remember/AutoConnect + // UI is hidden there, so we also clear the persisted setting to prevent a + // stale `true` from surviving silently. `undefined` (fresh host, test not + // yet complete) leaves the preference alone — test-connection will resolve + // capability in milliseconds. const onSelectedHostChange = (host: HostDTO | undefined) => { if (!host) { return; } + const nakedServer = host.supportsHashedPassword === false; form.change('userName', host.userName ?? ''); form.change('password', ''); - form.change('remember', Boolean(host.remember)); + form.change('remember', !nakedServer && Boolean(host.remember)); setStoredHashInvalidated(false); - togglePasswordLabel(Boolean(host.remember && host.hashedPassword)); + togglePasswordLabel(!nakedServer && Boolean(host.remember && host.hashedPassword)); + + if (nakedServer) { + form.change('autoConnect', false); + if (settings.status === LoadingState.READY && settings.value?.autoConnect) { + void settings.update({ autoConnect: false }); + } + } }; const onUserNameChange = (userName: string | undefined) => { @@ -81,6 +96,7 @@ export function useLoginFormBody(form: MinimalFormApi): LoginFormBody { togglePasswordLabel(canUseStoredPassword(values.remember, values.password)); return { + selectedHost, useStoredPasswordLabel, setUseStoredPasswordLabel, onSelectedHostChange, diff --git a/webclient/src/store/server/server.actions.spec.ts b/webclient/src/store/server/server.actions.spec.ts index f741086c5..1b5865369 100644 --- a/webclient/src/store/server/server.actions.spec.ts +++ b/webclient/src/store/server/server.actions.spec.ts @@ -42,7 +42,10 @@ describe('Actions', () => { }); it('testConnectionSuccessful', () => { - expect(Actions.testConnectionSuccessful()).toEqual({ type: Types.TEST_CONNECTION_SUCCESSFUL, payload: undefined }); + expect(Actions.testConnectionSuccessful({ serverOptions: 1 })).toEqual({ + type: Types.TEST_CONNECTION_SUCCESSFUL, + payload: { serverOptions: 1 }, + }); }); it('testConnectionFailed', () => { diff --git a/webclient/src/store/server/server.actions.ts b/webclient/src/store/server/server.actions.ts index 1b16afd01..cd8cb1e50 100644 --- a/webclient/src/store/server/server.actions.ts +++ b/webclient/src/store/server/server.actions.ts @@ -10,8 +10,6 @@ const SignalActions = { loginSuccessful: createAction<{ options: WebsocketTypes.LoginSuccessContext }>('server/loginSuccessful'), loginFailed: createAction('server/loginFailed'), connectionFailed: createAction('server/connectionFailed'), - testConnectionSuccessful: createAction('server/testConnectionSuccessful'), - testConnectionFailed: createAction('server/testConnectionFailed'), registrationRequiresEmail: createAction('server/registrationRequiresEmail'), registrationSuccess: createAction('server/registrationSuccess'), registrationEmailError: createAction<{ error: string }>('server/registrationEmailError'), diff --git a/webclient/src/store/server/server.dispatch.spec.ts b/webclient/src/store/server/server.dispatch.spec.ts index bab47faa7..caa061010 100644 --- a/webclient/src/store/server/server.dispatch.spec.ts +++ b/webclient/src/store/server/server.dispatch.spec.ts @@ -56,8 +56,10 @@ describe('Dispatch', () => { }); it('testConnectionSuccessful dispatches Actions.testConnectionSuccessful()', () => { - Dispatch.testConnectionSuccessful(); - expect(mockDispatch).toHaveBeenCalledWith(Actions.testConnectionSuccessful()); + Dispatch.testConnectionSuccessful(3); + expect(mockDispatch).toHaveBeenCalledWith( + Actions.testConnectionSuccessful({ serverOptions: 3 }), + ); }); it('testConnectionFailed dispatches Actions.testConnectionFailed()', () => { diff --git a/webclient/src/store/server/server.dispatch.ts b/webclient/src/store/server/server.dispatch.ts index 823085e0b..e6e74c96b 100644 --- a/webclient/src/store/server/server.dispatch.ts +++ b/webclient/src/store/server/server.dispatch.ts @@ -22,8 +22,11 @@ export const Dispatch = { connectionFailed: () => { store.dispatch(Actions.connectionFailed()); }, - testConnectionSuccessful: () => { - store.dispatch(Actions.testConnectionSuccessful()); + testConnectionStarted: () => { + store.dispatch(Actions.testConnectionStarted()); + }, + testConnectionSuccessful: (serverOptions: number) => { + store.dispatch(Actions.testConnectionSuccessful({ serverOptions })); }, testConnectionFailed: () => { store.dispatch(Actions.testConnectionFailed()); diff --git a/webclient/src/store/server/server.interfaces.ts b/webclient/src/store/server/server.interfaces.ts index a086f1f4d..fde65a97f 100644 --- a/webclient/src/store/server/server.interfaces.ts +++ b/webclient/src/store/server/server.interfaces.ts @@ -1,8 +1,12 @@ import { App, Data, Enriched } from '@app/types'; import { WebsocketTypes } from '@app/websocket/types'; +export type TestConnectionStatus = 'testing' | 'success' | 'failed' | null; + export interface ServerState { initialized: boolean; + /** Lifecycle of the most recent test connection — drives Login button + hashed-password UI gates. */ + testConnectionStatus: TestConnectionStatus; /** Buddies keyed by username for O(1) lookup. Use `getSortedBuddyList` for display. */ buddyList: { [userName: string]: Data.ServerInfo_User }; /** Ignored users keyed by username for O(1) lookup. Use `getSortedIgnoreList` for display. */ diff --git a/webclient/src/store/server/server.reducer.ts b/webclient/src/store/server/server.reducer.ts index 140c89969..ef928433e 100644 --- a/webclient/src/store/server/server.reducer.ts +++ b/webclient/src/store/server/server.reducer.ts @@ -69,6 +69,7 @@ function removeByPath(folder: Data.ServerInfo_DeckStorage_Folder, pathSegments: const initialState: ServerState = { initialized: false, + testConnectionStatus: null, buddyList: {}, ignoreList: {}, @@ -124,6 +125,22 @@ export const serverSlice = createSlice({ state.status.connectionAttemptMade = true; }, + testConnectionStarted: (state) => { + state.testConnectionStatus = 'testing'; + }, + + // `serverOptions` is typed on the action so `useReduxEffect` subscribers + // (see useKnownHostsComponent) can read it from the dispatched action — + // it's deliberately not stored in state since only the lifecycle matters + // here; the capability bitmask is persisted per-host to Dexie. + testConnectionSuccessful: (state, _action: PayloadAction<{ serverOptions: number }>) => { + state.testConnectionStatus = 'success'; + }, + + testConnectionFailed: (state) => { + state.testConnectionStatus = 'failed'; + }, + clearStore: (state) => ({ ...initialState, status: { ...state.status }, diff --git a/webclient/src/store/server/server.selectors.ts b/webclient/src/store/server/server.selectors.ts index 4794f54c4..2b56daeb0 100644 --- a/webclient/src/store/server/server.selectors.ts +++ b/webclient/src/store/server/server.selectors.ts @@ -19,6 +19,8 @@ export const Selectors = { getDescription: ({ server }: State) => server.status.description, getState: ({ server }: State) => server.status.state, getConnectionAttemptMade: ({ server }: State) => server.status.connectionAttemptMade, + /** Lifecycle status of the latest test connection. `null` until the first test fires. */ + getTestConnectionStatus: ({ server }: State) => server.testConnectionStatus, getUser: ({ server }: State) => server.user, /** True when the server status has reached LOGGED_IN. */ diff --git a/webclient/src/store/server/server.types.ts b/webclient/src/store/server/server.types.ts index d6224d6ac..8462ff1fd 100644 --- a/webclient/src/store/server/server.types.ts +++ b/webclient/src/store/server/server.types.ts @@ -9,6 +9,7 @@ export const Types = { LOGIN_SUCCESSFUL: a.loginSuccessful.type, LOGIN_FAILED: a.loginFailed.type, CONNECTION_FAILED: a.connectionFailed.type, + TEST_CONNECTION_STARTED: a.testConnectionStarted.type, TEST_CONNECTION_SUCCESSFUL: a.testConnectionSuccessful.type, TEST_CONNECTION_FAILED: a.testConnectionFailed.type, SERVER_MESSAGE: a.serverMessage.type, diff --git a/webclient/src/types/server.ts b/webclient/src/types/server.ts index 4b23cc02d..e280e258a 100644 --- a/webclient/src/types/server.ts +++ b/webclient/src/types/server.ts @@ -10,4 +10,8 @@ export class Host { userName?: string; hashedPassword?: string; remember?: boolean; + // Captured from Event_ServerIdentification.serverOptions during test connection. + // `undefined` = never tested; `true`/`false` = confirmed. UI gates the + // Remember Password and Auto Connect checkboxes on this. + supportsHashedPassword?: boolean; } diff --git a/webclient/src/websocket/WebClient.spec.ts b/webclient/src/websocket/WebClient.spec.ts index 7bb855a67..98870e7ac 100644 --- a/webclient/src/websocket/WebClient.spec.ts +++ b/webclient/src/websocket/WebClient.spec.ts @@ -38,6 +38,35 @@ import type { IWebClientResponse } from './types/WebClientResponse'; import type { IWebClientRequest } from './types/WebClientRequest'; import type { ConnectTarget } from './types/WebClientConfig'; import { installMockWebSocket } from './__mocks__/helpers'; +import { create, setExtension, toBinary } from '@bufbuild/protobuf'; +import { + Event_ServerIdentification_ext, + Event_ServerIdentification_ServerOptions, + Event_ServerIdentificationSchema, + ServerMessageSchema, + ServerMessage_MessageType, + SessionEventSchema, +} from '@app/generated'; +import { PROTOCOL_VERSION } from './config'; + +function buildServerIdentificationMessage({ + protocolVersion = PROTOCOL_VERSION, + serverOptions = 0, +}: { protocolVersion?: number; serverOptions?: number } = {}): Uint8Array { + const ident = create(Event_ServerIdentificationSchema, { + serverName: 'TestServer', + serverVersion: '2.8.0', + protocolVersion, + serverOptions, + }); + const sessionEvent = create(SessionEventSchema); + setExtension(sessionEvent, Event_ServerIdentification_ext, ident); + const server = create(ServerMessageSchema, { + messageType: ServerMessage_MessageType.SESSION_EVENT, + sessionEvent, + }); + return toBinary(ServerMessageSchema, server); +} function makeMockResponse(): IWebClientResponse { return { @@ -171,23 +200,49 @@ describe('WebClient', () => { expect(MockWS).toHaveBeenCalledWith(expect.stringContaining('://h:1')); }); - it('calls testConnectionSuccessful and closes on open', () => { + it('routes path-bearing hosts through the default TLS port (nginx proxy)', () => { + client.testConnect({ host: 'server.example.com/servatrice', port: '4748' }); + expect(MockWS).toHaveBeenCalledWith(expect.stringMatching(/:\/\/server\.example\.com\/servatrice$/)); + }); + + it('dispatches testConnectionSuccessful with serverOptions on ServerIdentification', () => { client.testConnect(target); - wsMockInstance.onopen(); - expect(mockResponse.session.testConnectionSuccessful).toHaveBeenCalled(); + const data = buildServerIdentificationMessage({ + serverOptions: Event_ServerIdentification_ServerOptions.SupportsPasswordHash, + }); + wsMockInstance.onmessage({ data: data.buffer }); + expect(mockResponse.session.testConnectionSuccessful).toHaveBeenCalledWith( + Event_ServerIdentification_ServerOptions.SupportsPasswordHash, + ); expect(wsMockInstance.close).toHaveBeenCalled(); }); + it('reports success with serverOptions=0 for naked-password servers', () => { + client.testConnect(target); + const data = buildServerIdentificationMessage({ serverOptions: 0 }); + wsMockInstance.onmessage({ data: data.buffer }); + expect(mockResponse.session.testConnectionSuccessful).toHaveBeenCalledWith(0); + }); + + it('fails on protocol-version mismatch instead of reporting success', () => { + client.testConnect(target); + const data = buildServerIdentificationMessage({ protocolVersion: PROTOCOL_VERSION + 1 }); + wsMockInstance.onmessage({ data: data.buffer }); + expect(mockResponse.session.testConnectionFailed).toHaveBeenCalled(); + expect(mockResponse.session.testConnectionSuccessful).not.toHaveBeenCalled(); + }); + it('calls testConnectionFailed on error', () => { client.testConnect(target); wsMockInstance.onerror(); expect(mockResponse.session.testConnectionFailed).toHaveBeenCalled(); }); - it('closes socket after keepalive timeout', () => { + it('fires testConnectionFailed when ServerIdentification never arrives before the keepalive timeout', () => { client.testConnect(target); vi.advanceTimersByTime(5000); expect(wsMockInstance.close).toHaveBeenCalled(); + expect(mockResponse.session.testConnectionFailed).toHaveBeenCalled(); }); it('closes the prior in-flight socket on rapid re-click', () => { diff --git a/webclient/src/websocket/WebClient.ts b/webclient/src/websocket/WebClient.ts index 8088753b2..f9a217fbe 100644 --- a/webclient/src/websocket/WebClient.ts +++ b/webclient/src/websocket/WebClient.ts @@ -1,5 +1,13 @@ +import { fromBinary, getExtension, hasExtension } from '@bufbuild/protobuf'; + +import { + Event_ServerIdentification_ext, + ServerMessageSchema, + ServerMessage_MessageType, +} from '@app/generated'; + import { ping } from './commands/session'; -import { CLIENT_OPTIONS } from './config'; +import { CLIENT_OPTIONS, PROTOCOL_VERSION } from './config'; import { GameEvents } from './events/game'; import { RoomEvents } from './events/room'; import { SessionEvents } from './events/session'; @@ -9,6 +17,7 @@ import type { IWebClientResponse } from './types/WebClientResponse'; import { StatusEnum } from './types/StatusEnum'; import { ProtobufService } from './services/ProtobufService'; import { WebSocketService } from './services/WebSocketService'; +import { buildWebSocketUrl } from './utils/buildWebSocketUrl'; export class WebClient { private static _instance: WebClient | null = null; @@ -82,34 +91,58 @@ export class WebClient { } const protocol = window.location.hostname === 'localhost' ? 'ws' : 'wss'; - const { host, port } = target; - const socket = new WebSocket(`${protocol}://${host}:${port}`); + const socket = new WebSocket(buildWebSocketUrl(protocol, target.host, target.port)); socket.binaryType = 'arraybuffer'; this.testSocket = socket; - const timeout = setTimeout(() => socket.close(), CLIENT_OPTIONS.keepalive); - - const clearIfActive = () => { + // "Green" means reachable AND speaking a compatible Cockatrice protocol. + // Waiting for Event_ServerIdentification lets us carry serverOptions back + // to the UI so naked-password hosts can be distinguished without a login. + let resolved = false; + const resolve = (ok: boolean, serverOptions = 0): void => { + if (resolved) { + return; + } + resolved = true; + clearTimeout(timeout); + // Suppress dispatches from a superseded socket — a newer test has + // already taken over and we'd race a stale result into its pending-ref. if (this.testSocket === socket) { + if (ok) { + this.response.session.testConnectionSuccessful(serverOptions); + } else { + this.response.session.testConnectionFailed(); + } this.testSocket = null; } + socket.close(); + }; + + const timeout = setTimeout(() => resolve(false), CLIENT_OPTIONS.keepalive); + + socket.onmessage = (event: MessageEvent) => { + try { + const msg = fromBinary(ServerMessageSchema, new Uint8Array(event.data)); + if (msg.messageType !== ServerMessage_MessageType.SESSION_EVENT) { + return; + } + const sessionEvent = msg.sessionEvent; + if (!sessionEvent || !hasExtension(sessionEvent, Event_ServerIdentification_ext)) { + return; + } + const ident = getExtension(sessionEvent, Event_ServerIdentification_ext); + if (ident.protocolVersion !== PROTOCOL_VERSION) { + resolve(false); + return; + } + resolve(true, ident.serverOptions); + } catch { + resolve(false); + } }; - socket.onopen = () => { - clearTimeout(timeout); - this.response.session.testConnectionSuccessful(); - socket.close(); - clearIfActive(); - }; - - socket.onerror = () => { - this.response.session.testConnectionFailed(); - clearIfActive(); - }; - - socket.onclose = () => { - clearIfActive(); - }; + socket.onerror = () => resolve(false); + socket.onclose = () => resolve(false); } public disconnect(): void { diff --git a/webclient/src/websocket/events/session/serverIdentification.ts b/webclient/src/websocket/events/session/serverIdentification.ts index 57e999032..84a21522a 100644 --- a/webclient/src/websocket/events/session/serverIdentification.ts +++ b/webclient/src/websocket/events/session/serverIdentification.ts @@ -32,7 +32,12 @@ export function serverIdentification(info: Event_ServerIdentification): void { SessionCommands.updateStatus(StatusEnum.LOGGING_IN, 'Logging In...'); if (getPasswordSalt) { SessionCommands.requestPasswordSalt(rest, - (salt) => SessionCommands.login(rest, password, salt), + // Empty salt means the server advertised SupportsPasswordHash but + // can't actually produce one. Treat it as effectively unsupported — + // fall through to a plain-password login rather than failing. + (salt) => salt + ? SessionCommands.login(rest, password, salt) + : SessionCommands.login(rest, password), () => { response.session.loginFailed(); SessionCommands.disconnect(); }, diff --git a/webclient/src/websocket/events/session/session-login-flow.integration.spec.ts b/webclient/src/websocket/events/session/session-login-flow.integration.spec.ts new file mode 100644 index 000000000..ead48704a --- /dev/null +++ b/webclient/src/websocket/events/session/session-login-flow.integration.spec.ts @@ -0,0 +1,143 @@ +// Integration coverage of the full login chain through the real session +// event handler + real session commands. Exercises: +// serverIdentification(info) +// → requestPasswordSalt (real) +// → login (real) +// → WebClient.instance.response.session.loginSuccessful(...) +// +// Only the transport (sendSessionCommand), the connection-state shim, and the +// password-hash bitmask helper are mocked — everything in between is the +// production code path. This triangulates the "loginSuccessful payload is +// empty" symptom reported in plans/when-the-login-quirky-crane.md. + +vi.mock('../../WebClient'); + +vi.mock('../../config', () => ({ + CLIENT_CONFIG: { clientver: 'test-client', clientfeatures: [] }, + CLIENT_OPTIONS: { autojoinrooms: false, keepalive: 5000 }, + PROTOCOL_VERSION: 14, +})); + +vi.mock('../../utils/connectionState', () => ({ + consumePendingOptions: vi.fn().mockReturnValue(null), +})); + +// Only the bitmask check is mocked; hashPassword stays real so the test +// asserts against a genuinely computed hash (not a sentinel string). +vi.mock('../../utils', async (importOriginal) => ({ + ...(await importOriginal()), + passwordSaltSupported: vi.fn().mockReturnValue(0), +})); + +import { create } from '@bufbuild/protobuf'; +import { Mock } from 'vitest'; +import { Event_ServerIdentificationSchema } from '@app/generated'; + +import { WebClient } from '../../WebClient'; +import { consumePendingOptions } from '../../utils/connectionState'; +import { passwordSaltSupported, hashPassword } from '../../utils'; +import { WebSocketConnectReason } from '../../types/ConnectOptions'; +import { makeCallbackHelpers } from '../../__mocks__/callbackHelpers'; +import { serverIdentification } from './serverIdentification'; + +const { invokeOnSuccess } = makeCallbackHelpers( + WebClient.instance.protobuf.sendSessionCommand as Mock, + 2, +); + +const makeLoginOptions = () => ({ + host: 'h', + port: '1', + userName: 'alice', + password: 'mypass', + reason: WebSocketConnectReason.LOGIN as const, +}); + +const makeInfo = (overrides: Record = {}) => + create(Event_ServerIdentificationSchema, { + serverName: 'TestServer', + serverVersion: '1.0', + protocolVersion: 14, + serverOptions: 1, + ...overrides, + }); + +beforeEach(() => { + (WebClient.instance.protobuf.sendSessionCommand as Mock).mockClear(); + (WebClient.instance.response.session.loginSuccessful as Mock).mockClear(); + (WebClient.instance.response.session.loginFailed as Mock).mockClear(); + (WebClient.instance.response.session.updateStatus as Mock).mockClear(); + (consumePendingOptions as Mock).mockReset(); + (passwordSaltSupported as Mock).mockReset(); +}); + +describe('integration: fresh login on a hashed-password server', () => { + it('loginSuccessful carries a non-empty hashedPassword matching hashPassword(salt, password)', () => { + (consumePendingOptions as Mock).mockReturnValue(makeLoginOptions()); + (passwordSaltSupported as Mock).mockReturnValue(1); + + // 1. Server identifies with the hashed-password bit set. + serverIdentification(makeInfo({ serverOptions: 1 })); + + // 2. RequestPasswordSalt command was sent; resolve it with a salt. + invokeOnSuccess({ passwordSalt: 'xyz' }); + + // 3. Login command was sent; resolve it with a minimal success response. + invokeOnSuccess({ buddyList: [], ignoreList: [], userInfo: { name: 'alice' } }); + + // 4. The response layer must receive the SAME hash the client computed. + const expected = hashPassword('xyz', 'mypass'); + expect(WebClient.instance.response.session.loginSuccessful).toHaveBeenCalledTimes(1); + expect(WebClient.instance.response.session.loginSuccessful).toHaveBeenCalledWith({ + hashedPassword: expected, + }); + expect(expected.length).toBeGreaterThan(16); + }); + + it('Command_Login carries hashedPassword and no plaintext password value', () => { + (consumePendingOptions as Mock).mockReturnValue(makeLoginOptions()); + (passwordSaltSupported as Mock).mockReturnValue(1); + + serverIdentification(makeInfo({ serverOptions: 1 })); + invokeOnSuccess({ passwordSalt: 'xyz' }); + + // Second sendSessionCommand call is the Command_Login. Protobuf always + // materializes the `password` field (proto-default ""), so we assert on + // the VALUE, not the presence: hashedPassword holds the real hash and + // plaintext password is empty. + const calls = (WebClient.instance.protobuf.sendSessionCommand as Mock).mock.calls; + const loginPayload = calls[1]?.[1]; + expect(loginPayload.hashedPassword).toBe(hashPassword('xyz', 'mypass')); + expect(loginPayload.password).toBe(''); + }); +}); + +describe('integration: server returns empty passwordSalt (effectively unsupported)', () => { + it('falls back to a plain-password Command_Login', () => { + (consumePendingOptions as Mock).mockReturnValue(makeLoginOptions()); + (passwordSaltSupported as Mock).mockReturnValue(1); + + serverIdentification(makeInfo({ serverOptions: 1 })); + // Server's Response_PasswordSalt has the proto default "" — simulate that. + invokeOnSuccess({ passwordSalt: '' }); + + const calls = (WebClient.instance.protobuf.sendSessionCommand as Mock).mock.calls; + // Two commands: RequestPasswordSalt, then the fallback plain Command_Login. + expect(calls.length).toBeGreaterThanOrEqual(2); + const loginPayload = calls[1][1]; + expect(loginPayload.password).toBe('mypass'); + expect(loginPayload.hashedPassword).toBe(''); + }); + + it('does not surface as a login failure', () => { + (consumePendingOptions as Mock).mockReturnValue(makeLoginOptions()); + (passwordSaltSupported as Mock).mockReturnValue(1); + + serverIdentification(makeInfo({ serverOptions: 1 })); + invokeOnSuccess({ passwordSalt: '' }); + + // Missing salt is treated as "unsupported", not a failure — the client + // should let the plain login proceed without dispatching loginFailed. + expect(WebClient.instance.response.session.loginFailed).not.toHaveBeenCalled(); + }); +}); diff --git a/webclient/src/websocket/services/WebSocketService.ts b/webclient/src/websocket/services/WebSocketService.ts index 86e81a9cb..585abc7ca 100644 --- a/webclient/src/websocket/services/WebSocketService.ts +++ b/webclient/src/websocket/services/WebSocketService.ts @@ -4,6 +4,7 @@ import { StatusEnum } from '../types/StatusEnum'; import { KeepAliveService } from './KeepAliveService'; import { CLIENT_OPTIONS } from '../config'; import type { ConnectTarget } from '../types/WebClientConfig'; +import { buildWebSocketUrl } from '../utils/buildWebSocketUrl'; export interface ReconnectConfig { maxAttempts: number; @@ -79,7 +80,7 @@ export class WebSocketService { this.keepalive = CLIENT_OPTIONS.keepalive; const { host, port } = target; - this.socket = this.createWebSocket(`${protocol}://${host}:${port}`); + this.socket = this.createWebSocket(buildWebSocketUrl(protocol as 'ws' | 'wss', host, port)); } public disconnect(): void { @@ -204,7 +205,9 @@ export class WebSocketService { return; } const { host, port } = this.lastTarget; - this.socket = this.createWebSocket(`${this.lastProtocol}://${host}:${port}`); + this.socket = this.createWebSocket( + buildWebSocketUrl(this.lastProtocol as 'ws' | 'wss', host, port), + ); }, delay); } diff --git a/webclient/src/websocket/types/WebClientResponse.ts b/webclient/src/websocket/types/WebClientResponse.ts index a9b57b56c..beedf6d79 100644 --- a/webclient/src/websocket/types/WebClientResponse.ts +++ b/webclient/src/websocket/types/WebClientResponse.ts @@ -55,7 +55,7 @@ export interface ISessionResponse { loginSuccessful(options: LoginSuccessContext): void; loginFailed(): void; connectionFailed(): void; - testConnectionSuccessful(): void; + testConnectionSuccessful(serverOptions: number): void; testConnectionFailed(): void; updateBuddyList(buddyList: ServerInfo_User[]): void; addToBuddyList(user: ServerInfo_User): void; diff --git a/webclient/src/websocket/utils/index.ts b/webclient/src/websocket/utils/index.ts index 4147f4af8..3821f61c1 100644 --- a/webclient/src/websocket/utils/index.ts +++ b/webclient/src/websocket/utils/index.ts @@ -1,3 +1,4 @@ +export * from './buildWebSocketUrl'; export * from './guid.util'; export * from './sanitizeHtml.util'; export * from './passwordHasher'; diff --git a/webclient/src/websocket/utils/passwordHasher.spec.ts b/webclient/src/websocket/utils/passwordHasher.spec.ts index 0d2a22142..61840c68c 100644 --- a/webclient/src/websocket/utils/passwordHasher.spec.ts +++ b/webclient/src/websocket/utils/passwordHasher.spec.ts @@ -1,6 +1,6 @@ vi.mock('../../generated/proto/event_server_identification_pb', async (importOriginal) => ({ ...(await importOriginal()), - Event_ServerIdentification_ServerOptions: { SupportsPasswordHash: 2 }, + Event_ServerIdentification_ServerOptions: { SupportsPasswordHash: 1 }, })); import { hashPassword, generateSalt, passwordSaltSupported } from './passwordHasher'; @@ -40,13 +40,19 @@ describe('generateSalt', () => { }); describe('passwordSaltSupported', () => { - it('returns non-zero when SupportsPasswordHash bit is set', () => { - // SupportsPasswordHash = 2 from mock; 2 & 2 = 2 - expect(passwordSaltSupported(2)).toBeTruthy(); + it('returns false for NoOptions (0)', () => { + expect(passwordSaltSupported(0)).toBeFalsy(); }); - it('returns zero when SupportsPasswordHash bit is not set', () => { - // 1 & 2 = 0 - expect(passwordSaltSupported(1)).toBeFalsy(); + it('returns true when the SupportsPasswordHash bit (1) is set', () => { + expect(passwordSaltSupported(1)).toBeTruthy(); + }); + + it('returns false when an unrelated bit is set (2)', () => { + expect(passwordSaltSupported(2)).toBeFalsy(); + }); + + it('returns true when bit 0 is set alongside other bits (3)', () => { + expect(passwordSaltSupported(3)).toBeTruthy(); }); });