diff --git a/.changeset/account-settings-flash-messages.md b/.changeset/account-settings-flash-messages.md new file mode 100644 index 00000000..ed76ab31 --- /dev/null +++ b/.changeset/account-settings-flash-messages.md @@ -0,0 +1,9 @@ +--- +'ePDS': patch +--- + +Account Settings now confirms every action with a visible banner. + +**Affects:** End users + +**End users:** when you added a backup email, removed one, changed your handle, revoked a session, or hit a validation error on any of those, the page silently bounced back to the same form with no indication that anything had changed (or what went wrong). The page now shows a green "Backup email added" / "Handle updated" / etc. banner on success, and a red "That handle is not available" / "We couldn't send the verification email" / etc. banner on error — so you know whether your action took effect. diff --git a/packages/auth-service/src/__tests__/account-settings-flash.test.ts b/packages/auth-service/src/__tests__/account-settings-flash.test.ts new file mode 100644 index 00000000..20a76986 --- /dev/null +++ b/packages/auth-service/src/__tests__/account-settings-flash.test.ts @@ -0,0 +1,145 @@ +import { describe, it, expect } from 'vitest' +import { + FLASH_SUCCESS_MESSAGES, + FLASH_ERROR_MESSAGES, + resolveAccountFlashFromQuery, + renderSettingsPage, +} from '../routes/account-settings.js' + +describe('account-settings flash messages', () => { + // Every code referenced in a redirect from a POST handler needs an + // entry in the matching lookup, otherwise the user lands on the + // settings page with no acknowledgement of the action they took. + // These tests guard against drift — adding a new code without a + // corresponding entry would mean the redirect silently dropped. + + it('has a success message for each code redirected to from a POST handler', () => { + const expectedSuccessCodes = [ + 'backup_added', + 'backup_verified', + 'backup_removed', + 'handle_updated', + 'session_revoked', + ] + for (const code of expectedSuccessCodes) { + expect( + FLASH_SUCCESS_MESSAGES[code], + `missing FLASH_SUCCESS_MESSAGES["${code}"]`, + ).toBeTruthy() + } + }) + + it('has an error message for each code redirected to from a POST handler', () => { + const expectedErrorCodes = [ + 'invalid_email', + 'already_primary', + 'account_not_found', + 'send_failed', + 'backup_remove_failed', + 'revoke_failed', + 'verify_failed', + 'invalid_handle', + 'handle_failed', + 'handle_taken', + 'delete_failed', + 'confirm_delete', + ] + for (const code of expectedErrorCodes) { + expect( + FLASH_ERROR_MESSAGES[code], + `missing FLASH_ERROR_MESSAGES["${code}"]`, + ).toBeTruthy() + } + }) + + it('returns undefined for unknown codes — the GET handler treats this as "no banner"', () => { + expect(FLASH_SUCCESS_MESSAGES['attacker_injected_text']).toBeUndefined() + expect(FLASH_ERROR_MESSAGES['']).toBeUndefined() + }) + + it('all messages are plain text, no HTML', () => { + // The renderer escapeHtml's the lookup result anyway (defence in + // depth) but the values themselves shouldn't carry markup. + for (const msg of Object.values(FLASH_SUCCESS_MESSAGES)) { + expect(msg).not.toMatch(/<[a-z]/i) + } + for (const msg of Object.values(FLASH_ERROR_MESSAGES)) { + expect(msg).not.toMatch(/<[a-z]/i) + } + }) +}) + +describe('account-settings flash rendering', () => { + const renderPage = (messages: { + successMessage?: string + errorMessage?: string + }) => + renderSettingsPage({ + did: 'did:plc:test', + email: 'primary@example.com', + handleDomain: 'example.com', + currentHandle: null, + backupEmails: [], + sessions: [], + currentSessionToken: 'current-session', + csrfToken: 'csrf-token', + ...messages, + }) + + it('renders successful actions as an accessible status', () => { + const html = renderPage({ successMessage: 'Backup email removed.' }) + expect(html).toContain( + '
Backup email removed.
', + ) + }) + + it('renders escaped failures as an alert', () => { + const html = renderPage({ errorMessage: '' }) + expect(html).toContain( + '', + ) + expect(html).not.toContain('') + }) +}) + +describe('resolveAccountFlashFromQuery', () => { + it('returns the success message when the success code is known', () => { + const result = resolveAccountFlashFromQuery({ success: 'backup_added' }) + expect(result.successMessage).toBe(FLASH_SUCCESS_MESSAGES.backup_added) + expect(result.errorMessage).toBeNull() + }) + + it('returns the error message when the error code is known', () => { + const result = resolveAccountFlashFromQuery({ + success: '', + error: 'invalid_handle', + }) + expect(result.successMessage).toBeNull() + expect(result.errorMessage).toBe(FLASH_ERROR_MESSAGES.invalid_handle) + }) + + it('returns null on both sides when the query is empty', () => { + expect(resolveAccountFlashFromQuery({})).toEqual({ + successMessage: null, + errorMessage: null, + }) + }) + + it('returns null on both sides for unknown codes (safety against URL-injection of attacker text)', () => { + expect( + resolveAccountFlashFromQuery({ + success: 'attacker_chosen_text', + error: '', + }), + ).toEqual({ successMessage: null, errorMessage: null }) + }) + + it('ignores non-string query values gracefully (e.g. ?success=foo&success=bar arrays)', () => { + expect( + resolveAccountFlashFromQuery({ + success: ['backup_added', 'backup_removed'], + error: 12345, + }), + ).toEqual({ successMessage: null, errorMessage: null }) + }) +}) diff --git a/packages/auth-service/src/routes/account-settings.ts b/packages/auth-service/src/routes/account-settings.ts index 8923039a..aae88716 100644 --- a/packages/auth-service/src/routes/account-settings.ts +++ b/packages/auth-service/src/routes/account-settings.ts @@ -16,6 +16,71 @@ import { POWERED_BY_CSS, POWERED_BY_HTML } from '../lib/page-helpers.js' const logger = createLogger('auth:account-settings') +/** + * User-visible flash-message lookup tables for the GET /account + * page. The POST handlers redirect back with `?success=` or + * `?error=` and the GET handler renders the matching message. + * + * Whitelisted lookup (not `?success=Some+raw+text`) so attackers + * can't inject arbitrary text into the settings page via a crafted + * URL. + * + * Exported for unit testing. + */ +export const FLASH_SUCCESS_MESSAGES: Record = { + backup_added: + 'Backup email added. Click the verification link we sent to confirm.', + backup_verified: 'Backup email verified.', + backup_removed: 'Backup email removed.', + handle_updated: 'Handle updated.', + session_revoked: 'Session revoked.', +} + +export const FLASH_ERROR_MESSAGES: Record = { + invalid_email: 'That email address is not valid.', + already_primary: + 'That email is already your primary — you can sign in with it directly.', + account_not_found: "We couldn't find an account for your sign-in.", + send_failed: + "We couldn't send the verification email. Please try again in a moment.", + backup_remove_failed: + "We couldn't remove that backup email. Please try again in a moment.", + revoke_failed: + "We couldn't revoke that session. Please try again in a moment.", + verify_failed: + 'That verification link is no longer valid. Add the backup email again to receive a fresh link.', + invalid_handle: + "That handle isn't valid. Use 5–20 characters: letters, numbers, or hyphens.", + handle_failed: + "We couldn't change your handle. It may be reserved or already taken.", + delete_failed: + "We couldn't delete your account right now. Please try again in a moment.", + handle_taken: 'That handle is not available — please choose another.', + confirm_delete: + 'Type DELETE in the confirmation box to permanently delete your account.', +} + +/** + * Resolve the success / error message pair from a /account request's + * query params. Whitelist the keys via the FLASH_*_MESSAGES tables so + * an attacker can't craft a URL like + * `/account?error=Some+raw+text` to inject arbitrary copy. + * + * Exported so the route handler can stay declarative AND be unit- + * tested without needing a fake express request. + */ +export function resolveAccountFlashFromQuery(query: { + success?: unknown + error?: unknown +}): { successMessage: string | null; errorMessage: string | null } { + const successCode = typeof query.success === 'string' ? query.success : '' + const errorCode = typeof query.error === 'string' ? query.error : '' + return { + successMessage: FLASH_SUCCESS_MESSAGES[successCode] ?? null, + errorMessage: FLASH_ERROR_MESSAGES[errorCode] ?? null, + } +} + /** * Middleware that validates a better-auth session and injects it into res.locals. * If not authenticated, redirects to /account/login. @@ -80,6 +145,14 @@ export function createAccountSettingsRouter( logger.warn({ err }, 'Failed to list sessions') } + // The POST handlers below redirect back to /account with a + // ?success=… or ?error=… query param to confirm the action. + // resolveAccountFlashFromQuery whitelists the recognised codes + // so an attacker can't craft a URL that injects arbitrary text. + const { successMessage, errorMessage } = resolveAccountFlashFromQuery( + req.query as { success?: unknown; error?: unknown }, + ) + res.type('html').send( renderSettingsPage({ did: did ?? '(unknown)', @@ -90,6 +163,8 @@ export function createAccountSettingsRouter( sessions, currentSessionToken: session.session.token, csrfToken: res.locals.csrfToken, + successMessage, + errorMessage, }), ) }) @@ -209,15 +284,26 @@ export function createAccountSettingsRouter( async (req: Request, res: Response) => { const session = res.locals.betterAuthSession const email = ((req.body.email as string) || '').trim().toLowerCase() + if (!email) { + res.redirect(303, '/account?error=invalid_email') + return + } const did = await getDidByEmail( session.user.email, pdsUrl, internalSecret, ) - if (did && email) { + if (!did) { + res.redirect(303, '/account?error=account_not_found') + return + } + try { ctx.db.removeBackupEmail(did, email) + res.redirect(303, '/account?success=backup_removed') + } catch (err) { + logger.error({ err }, 'Failed to remove backup email') + res.redirect(303, '/account?error=backup_remove_failed') } - res.redirect(303, '/account') }, ) @@ -227,17 +313,20 @@ export function createAccountSettingsRouter( requireAuth, async (req: Request, res: Response) => { const tokenToRevoke = req.body.session_token as string - if (tokenToRevoke) { - try { - await auth.api.revokeSession({ - body: { token: tokenToRevoke }, - headers: fromNodeHeaders(req.headers), - }) - } catch (err) { - logger.warn({ err }, 'Failed to revoke session') - } + if (!tokenToRevoke) { + res.redirect(303, '/account?error=revoke_failed') + return + } + try { + await auth.api.revokeSession({ + body: { token: tokenToRevoke }, + headers: fromNodeHeaders(req.headers), + }) + res.redirect(303, '/account?success=session_revoked') + } catch (err) { + logger.warn({ err }, 'Failed to revoke session') + res.redirect(303, '/account?error=revoke_failed') } - res.redirect(303, '/account') }, ) @@ -413,7 +502,7 @@ export function createAccountSettingsRouter( return router } -function renderSettingsPage(opts: { +export function renderSettingsPage(opts: { did: string email: string handleDomain: string @@ -427,7 +516,21 @@ function renderSettingsPage(opts: { }> currentSessionToken: string csrfToken: string + successMessage?: string | null + errorMessage?: string | null }): string { + // Prepend any flash banners INSIDE the existing page-wrap so they + // sit above the settings content. Both go through escapeHtml even + // though the source dictionary is whitelisted — defence-in-depth + // against a future maintainer accidentally widening the lookup. + const flashHtml = [ + opts.successMessage + ? `
${escapeHtml(opts.successMessage)}
` + : '', + opts.errorMessage + ? `` + : '', + ].join('') const backupRows = opts.backupEmails .map( (be) => ` @@ -489,6 +592,8 @@ function renderSettingsPage(opts: { + ${flashHtml} +

Account

DID: ${escapeHtml(opts.did)}
@@ -589,6 +694,9 @@ const SETTINGS_CSS = ` .header { display: flex; justify-content: space-between; align-items: center; margin-bottom: 24px; border-bottom: 1px solid #eee; padding-bottom: 16px; } h1 { font-size: 24px; color: #111; } h2 { font-size: 18px; color: #333; margin-bottom: 12px; } + .flash { padding: 12px 16px; border-radius: 8px; margin-bottom: 20px; font-size: 14px; } + .flash-success { background: #f0fff4; color: #28a745; border: 1px solid #c3e6cb; } + .flash-error { background: #fdf0f0; color: #dc3545; border: 1px solid #f5c6cb; } .section { margin-bottom: 28px; padding-bottom: 20px; border-bottom: 1px solid #f0f0f0; } .section:last-child { border-bottom: none; margin-bottom: 0; } .setting-row { display: flex; justify-content: space-between; align-items: center; padding: 8px 0; font-size: 14px; color: #333; } diff --git a/vitest.config.ts b/vitest.config.ts index 954c8956..652b7384 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -24,7 +24,7 @@ export default defineConfig({ // See AGENTS.md for the ratcheting policy. thresholds: { statements: 58, - branches: 57, + branches: 58, functions: 71, lines: 57, },