From 72b836d205a865e0355b3efd767ab4c0708e9b03 Mon Sep 17 00:00:00 2001 From: alpha Date: Wed, 19 Aug 2026 12:51:47 -0400 Subject: [PATCH] fix(auth): never report a completed password change as failed The remember-me write sat inside the same try as the request, so an electron-store failure (disk full, permissions, a corrupted file) returned `success: false` from a change the server had already made. The new password was live and the user was told it failed - and the dialog then asks them to retry with a current password that is no longer current, so the retry fails too. Nothing on screen ever says the change went through. The write now has its own try and logs on failure. `resetPassword` was fixed for this in #103 and `login.tsx` draws the same line ("Persisting the remember-me choice must never block the sign-in attempt"); this path was left out of that PR as out of scope. `test/change-password.test.mjs` covers the write in both directions, the rejected-change case, and the regression itself: `updateConfig` throws and the change must still report success, which fails against the old shape. Closes #104 Co-Authored-By: Claude Opus 5 --- src/main/services/auth.service.ts | 14 ++++++- test/change-password.test.mjs | 70 +++++++++++++++++++++++++++++++ test/run.mjs | 1 + 3 files changed, 83 insertions(+), 2 deletions(-) create mode 100644 test/change-password.test.mjs diff --git a/src/main/services/auth.service.ts b/src/main/services/auth.service.ts index 951d84d..bf9a9bd 100644 --- a/src/main/services/auth.service.ts +++ b/src/main/services/auth.service.ts @@ -176,8 +176,18 @@ export class AuthService { // Only persist when the user opted in - login/logout leave the store empty otherwise, // and writing here would put credentials back behind their back. - if (configStore.getConfig().rememberMe) { - configStore.updateConfig({ password: newPassword }); + // + // Guarded separately from the request, because by this line the password has already + // changed on the server. Letting a disk write decide the return value reports a failure + // for a change that happened, and the dialog then asks the user to try again with a + // current password that is no longer current, so the retry cannot work either. Same + // line `resetPassword` draws, and `login.tsx` before it. + try { + if (configStore.getConfig().rememberMe) { + configStore.updateConfig({ password: newPassword }); + } + } catch (err) { + console.error('Failed to store the new password after change:', err); } return { success: true }; diff --git a/test/change-password.test.mjs b/test/change-password.test.mjs new file mode 100644 index 0000000..faca256 --- /dev/null +++ b/test/change-password.test.mjs @@ -0,0 +1,70 @@ +/** + * The remember-me write in `changePassword`, and the one way it can misreport. + * + * By the time that write runs the password has already changed on the server. If a disk + * failure is allowed to decide the return value, the user is told the change failed while it + * actually went through, and the dialog then asks them to retry with a "current password" that + * is no longer current - so the retry fails too, and nothing on screen ever says the change + * succeeded. `login.tsx` draws the same line for the same reason, and `resetPassword` was + * fixed for it in #103. + */ +import { createChecker, loadMain } from './helpers.mjs'; + +export async function run() { + const { check, failures } = createChecker('change-password'); + + const { configStore } = await loadMain('store/config.store.js'); + const { authService } = await loadMain('services/auth.service.js'); + + // Stand in for AuthApi. `private` is erased at runtime, so the singleton's client is swappable. + let response = {}; + authService.client = { + changePassword: async () => response, + }; + + response = {}; + + configStore.updateConfig({ rememberMe: false, email: '', password: '' }); + check( + 'changePassword succeeds without rememberMe', + (await authService.changePassword('old', 'brand-new')).success === true + ); + check('no password is written when the user did not opt in', configStore.getConfig().password === ''); + + configStore.updateConfig({ rememberMe: true, email: 'a@b.c', password: 'stale' }); + await authService.changePassword('old', 'brand-new'); + check( + 'the stored password is replaced when rememberMe is on', + configStore.getConfig().password === 'brand-new' + ); + + // A rejected change must not touch the store: the old password is still the live one. + configStore.updateConfig({ rememberMe: true, email: 'a@b.c', password: 'still-valid' }); + response = { error: { code: 'BAD_REQUEST', message: 'Current password is incorrect' } }; + const rejected = await authService.changePassword('wrong', 'never-set'); + check('a rejected change reports failure', rejected.success === false); + check( + 'a rejected change leaves the stored password alone', + configStore.getConfig().password === 'still-valid' + ); + + // The regression this file exists for. + configStore.updateConfig({ rememberMe: true, email: 'a@b.c', password: 'stale' }); + response = {}; + const realUpdate = configStore.updateConfig.bind(configStore); + configStore.updateConfig = () => { + throw new Error('disk full'); + }; + let survived; + try { + survived = await authService.changePassword('old', 'brand-new'); + } finally { + configStore.updateConfig = realUpdate; + } + check('a change still succeeds when the store write throws', survived.success === true); + check('and reports no error', survived.error === undefined); + + configStore.updateConfig({ rememberMe: false, email: '', password: '' }); + + return failures; +} diff --git a/test/run.mjs b/test/run.mjs index 2069c35..153386f 100644 --- a/test/run.mjs +++ b/test/run.mjs @@ -29,6 +29,7 @@ for (const module of [ './suggestion-sentinel.test.mjs', './suggestion-emphasis.test.mjs', './mac-update-util.test.mjs', + './change-password.test.mjs', './password-reset.test.mjs', ]) { const { run } = await import(module);