From a6f86ec1f0b3e7f030123d596842239493b80695 Mon Sep 17 00:00:00 2001 From: Frederic Cabassut Date: Tue, 18 Aug 2026 09:56:56 +0200 Subject: [PATCH] fix(wu-002): refuse password resets that could never work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second slice of issue #77. PATCH /api/admin/users/[id] accepted { newPassword } for any account and returned 200, but resetPassword() writes only passwordHash while verifyPassword() matches only rows whose authProvider is 'password'. So for an OAuth-only account (the "OAuth-only" checkbox stores 'none') or anyone who has signed in via OAuth (authProvider becomes the provider name), the endpoint wrote a hash that could never be used to sign in, returned success, and fired a "your password was reset" email. It lied. The endpoint already guards both other self-affecting operations server-side — "Cannot change your own role" and "Cannot delete your own account" — but a newPassword targeting the requesting admin succeeded. Two early returns in the newPassword branch, ordered length -> self -> eligibility so the existing length 400 keeps precedence. Both are placed before the resetPassword() call and before the email block: that ordering is what guarantees no unusable hash is written and no misleading email is sent, and it is a property of control flow rather than of an assertion, because neither is observable from a test. passwordHash never leaves the server (toUser omits it) and isEmailConfigured() is false under e2e, so an assertion either way would pass for a broken implementation too. Eligibility comes from canResetPassword() rather than an inline comparison, so the endpoint and the settings UI cannot drift; the 400 message comes from passwordResetBlockedReason() so the API reason and the UI explanation are the same string. DoD items verified: - [x] 1. eligible user other than self still 200s, new password works - [x] 2. authProvider 'none' returns 400 naming the reason - [x] 3. positive control: an eligible user CAN log in after a reset - [x] 4. targeting own id returns 400 naming the reason - [x] 5. sub-8-char password still 400s for length, beating eligibility - [x] 6. both guards precede the write and the email (diff-verified) - [x] 7. role, revokeSessions and DELETE byte-unchanged (15+/0-) - [x] 8. eligibility imported from wu-001, not re-implemented - [x] 9. declarative Given/When/Then, distinct victim emails throughout Verified independently by the orchestrator: pnpm check 0 errors / 14 pre-existing warnings; pnpm test:unit 306 passed / 26 files; pnpm build && pnpm test:e2e admin-users.spec.ts 14 passed (7 auth-setup + 3 pre-existing + 4 new). The reviewer additionally reverted the endpoint to its pre-change form and confirmed both guard scenarios fail with "Expected: 400, Received: 200", so they are genuine regression tests rather than assertions of behaviour that already held. Reviewed-by: adversarial-review (PASS) Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RbqJcFofZSbhbGwUwZP3wU --- src/routes/api/admin/users/[id]/+server.ts | 15 +++ tests/e2e/admin-users.spec.ts | 135 ++++++++++++++++++++- 2 files changed, 149 insertions(+), 1 deletion(-) diff --git a/src/routes/api/admin/users/[id]/+server.ts b/src/routes/api/admin/users/[id]/+server.ts index d38779c..6de9780 100644 --- a/src/routes/api/admin/users/[id]/+server.ts +++ b/src/routes/api/admin/users/[id]/+server.ts @@ -3,6 +3,7 @@ import type { RequestHandler } from './$types.js'; import { requireAdmin } from '$lib/server/api-utils.js'; import { getUser, deleteUser, updateUserRole, resetPassword, revokeAllSessions } from '$lib/server/auth.js'; import { isEmailConfigured, sendPasswordResetEmail, sendRoleChangedEmail } from '$lib/server/email.js'; +import { canResetPassword, passwordResetBlockedReason } from '$lib/utils/auth-methods.js'; export const PATCH: RequestHandler = async ({ params, request, ...event }) => { const admin = requireAdmin(event); @@ -34,6 +35,20 @@ export const PATCH: RequestHandler = async ({ params, request, ...event }) => { if (body.newPassword.length < 8) { throw error(400, 'Password must be at least 8 characters'); } + // Prevent admin from resetting their own password — changing your own + // password goes through the profile flow, which verifies the current one. + if (userId === admin.id) { + throw error(400, 'Cannot reset your own password — change it from your profile instead'); + } + // Refuse accounts that do not sign in with a password. resetPassword() + // writes only passwordHash, and verifyPassword() matches only rows whose + // authProvider is 'password' — a hash written here could never be used to + // sign in, while the email below would claim the reset worked. The reason + // text comes from the same module as the check, so this matches what the + // UI explains; the ?? only satisfies error()'s string parameter. + if (!canResetPassword(user)) { + throw error(400, passwordResetBlockedReason(user) ?? 'This account does not use password login'); + } await resetPassword(userId, body.newPassword); // Notify user of password reset (fire-and-forget) diff --git a/tests/e2e/admin-users.spec.ts b/tests/e2e/admin-users.spec.ts index 6bf850e..46b44a6 100644 --- a/tests/e2e/admin-users.spec.ts +++ b/tests/e2e/admin-users.spec.ts @@ -1,4 +1,5 @@ -import { test, expect } from './helpers/fixtures.js'; +import { test, expect, TEST_EMAIL } from './helpers/fixtures.js'; +import { randomUUID } from 'crypto'; import { readFileSync } from 'fs'; import { fileURLToPath } from 'url'; import { dirname, join } from 'path'; @@ -23,6 +24,35 @@ async function createVictim( return user.id; } +/** + * Create a victim user with no password via the admin API, returning the user's id. + * Omitting the password is the OAuth-only path: createUser() stores authProvider + * 'none', which verifyPassword() never matches. + */ +async function createOAuthOnlyVictim( + page: import('@playwright/test').Page, + email: string, + displayName = email.split('@')[0] +): Promise { + const res = await page.request.post('/api/admin/users', { + data: { email, displayName, role: 'user' } + }); + expect(res.status()).toBe(201); + const user = await res.json(); + expect(user.authProvider).toBe('none'); + return user.id; +} + +/** The id of the admin account the authenticated page is signed in as. */ +async function currentAdminId(page: import('@playwright/test').Page): Promise { + const res = await page.request.get('/api/admin/users'); + expect(res.status()).toBe(200); + const users = (await res.json()) as Array<{ id: number; email: string }>; + const admin = users.find((u) => u.email === TEST_EMAIL); + if (!admin) throw new Error(`admin ${TEST_EMAIL} is missing from the users list`); + return admin.id; +} + test.describe.serial('Admin — User Deletion', () => { test('Scenario: Admin deletes a user from the users list', async ({ authenticatedPage: page @@ -151,3 +181,106 @@ test.describe.serial('Admin — User Deletion', () => { expect(loginRes.status()).toBe(401); }); }); + +test.describe.serial('Admin — Password Reset', () => { + test('Scenario: An account can sign in with a password the admin set', async ({ + authenticatedPage: page, + browser + }) => { + // Given a password-auth user exists + const email = 'reset-target@test.com'; + const victimId = await createVictim(page, email, 'Reset Target'); + + // When the admin sets a new password for that account + const newPassword = `reset-${randomUUID()}`; + const res = await page.request.patch(`/api/admin/users/${victimId}`, { + data: { newPassword } + }); + + // Then the reset succeeds + expect(res.status()).toBe(200); + + // And the account can sign in with the new password. A separate browser + // context keeps the resulting session cookie out of the admin's jar. + const ctx = await browser.newContext(); + const loginRes = await ctx.request.post('/api/auth/login', { + data: { email, password: newPassword } + }); + expect(loginRes.status()).toBe(200); + await ctx.close(); + + // Clean up + await page.request.delete(`/api/admin/users/${victimId}`); + }); + + test('Scenario: An account that signs in via OAuth cannot be given a password', async ({ + authenticatedPage: page, + browser + }) => { + // Given an OAuth-only user exists + const email = 'oauth-only@test.com'; + const victimId = await createOAuthOnlyVictim(page, email, 'OAuth Only'); + + // When the admin attempts to set a password for that account + const attemptedPassword = `attempt-${randomUUID()}`; + const res = await page.request.patch(`/api/admin/users/${victimId}`, { + data: { newPassword: attemptedPassword } + }); + + // Then the attempt is rejected and the response says why + expect(res.status()).toBe(400); + const body = await res.json(); + expect(body.message).toMatch(/OAuth|password login/i); + + // And the account still cannot sign in with the attempted password. + // This is the positive control for the rejection above: it confirms no + // usable credential was stored, not merely that the request 400'd. + const ctx = await browser.newContext(); + const loginRes = await ctx.request.post('/api/auth/login', { + data: { email, password: attemptedPassword } + }); + expect(loginRes.status()).toBe(401); + await ctx.close(); + + // Clean up + await page.request.delete(`/api/admin/users/${victimId}`); + }); + + test('Scenario: An admin cannot reset their own password', async ({ + authenticatedPage: page + }) => { + // Given the admin's own account + const adminId = await currentAdminId(page); + + // When the admin targets their own account for a password reset + const res = await page.request.patch(`/api/admin/users/${adminId}`, { + data: { newPassword: `self-${randomUUID()}` } + }); + + // Then the attempt is rejected and the response says why + expect(res.status()).toBe(400); + const body = await res.json(); + expect(body.message).toMatch(/your own password/i); + }); + + test('Scenario: A password below the minimum length is rejected for its length, whatever the account', async ({ + authenticatedPage: page + }) => { + // Given an OAuth-only user exists, who is also ineligible for a reset + const victimId = await createOAuthOnlyVictim(page, 'short-pw-oauth@test.com', 'Short Pw'); + + // When the admin attempts to set a password shorter than the minimum + const res = await page.request.patch(`/api/admin/users/${victimId}`, { + data: { newPassword: 'short' } + }); + + // Then the length rule is what the response reports, taking precedence + // over the eligibility rule + expect(res.status()).toBe(400); + const body = await res.json(); + expect(body.message).toMatch(/at least 8 characters/i); + + // Clean up + await page.request.delete(`/api/admin/users/${victimId}`); + }); +});