From ff08fe50fe291c32c7a952e9fa7e204392a8b2e4 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Sun, 1 Nov 2026 00:05:15 +0100 Subject: [PATCH] fix: say when an admin sign-out or password change couldn't be stored instead of answering ok or wrong password #1066 --- app/api/admin/auth/route.ts | 13 +++++++++- app/api/admin/change-password/route.ts | 3 ++- lib/__tests__/admin-session-shared.test.ts | 29 ++++++++++++++++++++++ lib/admin/password.ts | 22 ++++++++++++++-- lib/admin/session.ts | 7 +++--- 5 files changed, 67 insertions(+), 7 deletions(-) diff --git a/app/api/admin/auth/route.ts b/app/api/admin/auth/route.ts index 71f4165a0..df75ae5d1 100644 --- a/app/api/admin/auth/route.ts +++ b/app/api/admin/auth/route.ts @@ -283,7 +283,18 @@ export async function DELETE(request: NextRequest) { return NextResponse.json({ error: 'Cross-origin request rejected' }, { status: 403 }); } const ip = getClientIP(request); - await clearAdminSessionCookie(); + try { + await clearAdminSessionCookie(); + } catch (error) { + if (!isStoreUnavailable(error)) throw error; + // The cookie is gone from this browser, but the sign-out couldn't be + // stored, so a copy of the cookie isn't refused yet. + await auditLog('admin.logout', { stored: false }, ip); + return NextResponse.json( + { error: 'Signed out in this browser, but the sign-out could not be saved, so a copy of the session still works until it expires. Try signing out again in a moment.' }, + { status: 503, headers: { 'Retry-After': '10' } }, + ); + } await auditLog('admin.logout', {}, ip); return NextResponse.json({ ok: true }); } catch (error) { diff --git a/app/api/admin/change-password/route.ts b/app/api/admin/change-password/route.ts index c18bdabe7..05c16323e 100644 --- a/app/api/admin/change-password/route.ts +++ b/app/api/admin/change-password/route.ts @@ -1,5 +1,5 @@ import { NextRequest, NextResponse } from 'next/server'; -import { changeAdminPassword } from '@/lib/admin/password'; +import { changeAdminPassword, isAdminPasswordConflict } from '@/lib/admin/password'; import { requireAdminAuth, getClientIP, setAdminSessionCookie } from '@/lib/admin/session'; import { auditLog } from '@/lib/admin/audit'; import { logger } from '@/lib/logger'; @@ -35,6 +35,7 @@ export async function POST(request: NextRequest) { await auditLog('admin.change-password', {}, ip); return NextResponse.json({ ok: true }); } catch (error) { + if (isAdminPasswordConflict(error)) return NextResponse.json({ error: error.message }, { status: 409 }); const readOnly = configReadOnlyResponse(error); if (readOnly) return readOnly; logger.error('Admin change password error', { error: error instanceof Error ? error.message : 'Unknown error' }); diff --git a/lib/__tests__/admin-session-shared.test.ts b/lib/__tests__/admin-session-shared.test.ts index 8b9b22d92..8d6907e44 100644 --- a/lib/__tests__/admin-session-shared.test.ts +++ b/lib/__tests__/admin-session-shared.test.ts @@ -5,6 +5,7 @@ import { mkdtempSync, rmSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { NextRequest } from 'next/server'; // With a shared store (#1066) an admin signed out on one replica has to be // refused on every other one at once, a new admin password has to sign out @@ -201,4 +202,32 @@ describe('admin sessions with a shared store', () => { store.down = false; expect(await statusFor(token)).toBe(200); }); + + it('says so when a sign-out could not be stored', async () => { + await setInitialAdminPassword('first-password-1'); + const token = await signIn(); + store.down = true; + const { DELETE } = await import('@/app/api/admin/auth/route'); + const res = await DELETE(new NextRequest('http://localhost/api/admin/auth', { method: 'DELETE', headers: { 'sec-fetch-site': 'same-origin' } })); + expect(res.status).toBe(503); + expect((await res.json()).error).toMatch(/could not be saved/); + expect(jar.has(COOKIE)).toBe(false); + store.down = false; + // As said: the copy still works. + expect(await statusFor(token)).toBe(200); + }); + + it('refuses the second of two password changes at once with 409, not "incorrect"', async () => { + await setInitialAdminPassword('first-password-1'); + await signIn(); + const { POST } = await import('@/app/api/admin/change-password/route'); + const change = (newPassword: string) => POST(new NextRequest('http://localhost/api/admin/change-password', { + method: 'POST', + headers: { 'sec-fetch-site': 'same-origin', 'content-type': 'application/json' }, + body: JSON.stringify({ currentPassword: 'first-password-1', newPassword }), + })); + const statuses = (await Promise.all([change('second-password-2'), change('third-password-3')])).map((r) => r.status); + expect(statuses.sort()).toEqual([200, 409]); + expect([await verifyAdminPassword('second-password-2'), await verifyAdminPassword('third-password-3')].filter(Boolean)).toHaveLength(1); + }); }); diff --git a/lib/admin/password.ts b/lib/admin/password.ts index b0bd28290..592991bc9 100644 --- a/lib/admin/password.ts +++ b/lib/admin/password.ts @@ -330,7 +330,25 @@ export async function verifyAdminPassword(password: string): Promise { } /** - * Change the admin password. Returns true on success. + * Two password changes at once (on two replicas): the one that came second + * was checked against a password that no longer applies. + */ +export class AdminPasswordConflictError extends Error { + constructor() { + super('The admin password was changed somewhere else a moment ago. Sign in with the new password and try again.'); + this.name = 'AdminPasswordConflictError'; + } +} + +/** By name, as the class may come from another bundle's copy of this module. */ +export function isAdminPasswordConflict(error: unknown): error is AdminPasswordConflictError { + return error instanceof Error && error.name === 'AdminPasswordConflictError'; +} + +/** + * Change the admin password. Returns true on success, false when the + * current password is wrong. Throws AdminPasswordConflictError when another + * change landed between the check and the write (shared store). */ export async function changeAdminPassword(currentPassword: string, newPassword: string): Promise { const shared = isSharedMode(); @@ -347,7 +365,7 @@ export async function changeAdminPassword(currentPassword: string, newPassword: const hash = await hashPassword(newPassword); const next: AdminConfigData = { passwordHash: hash, passwordHashFile: undefined }; if (shared) { - if (!(await writeConfigData(next, { ifVersion: stored!.version })).ok) return false; + if (!(await writeConfigData(next, { ifVersion: stored!.version })).ok) throw new AdminPasswordConflictError(); } else { await writeConfigData(next); } diff --git a/lib/admin/session.ts b/lib/admin/session.ts index 8c876b6a5..b76c88d54 100644 --- a/lib/admin/session.ts +++ b/lib/admin/session.ts @@ -266,12 +266,13 @@ export async function clearAdminSessionCookie(): Promise { try { await revokeAdminSession(payload.sid, payload.exp); } catch (error) { - // Local mode keeps it in memory regardless; only a restart forgets it. - // A shared store that is down keeps every admin request out until it - // is back, but then the token works again until it expires. logger.warn('Failed to save the admin session sign-out', { error: error instanceof Error ? error.message : 'Unknown error', }); + // Local mode keeps it in memory regardless; only a restart forgets it. + // With a shared store nothing kept it: once the store is back a copy of + // the cookie would work again until it expires, so the caller is told. + if (isStoreUnavailable(error)) throw error; } }