diff --git a/server/src/nest/auth/passkey.service.ts b/server/src/nest/auth/passkey.service.ts index 669172c2d..5dadc95d6 100644 --- a/server/src/nest/auth/passkey.service.ts +++ b/server/src/nest/auth/passkey.service.ts @@ -18,8 +18,9 @@ import type { WebauthnCredentialsRepository } from '../../db/repositories/Webaut import { WebauthnChallenges } from '../../db/entities/WebauthnChallenges.entity'; import type { WebauthnChallengesRepository } from '../../db/repositories/WebauthnChallenges.repository'; import { Users } from '../../db/entities/Users.entity'; -import type { UsersRepository } from '../../db/repositories/Users.repository'; +import type { UsersRepository, UserRow } from '../../db/repositories/Users.repository'; import type { User } from '../../types'; +import { toRowId } from '../common/row-id'; // --------------------------------------------------------------------------- // Constants @@ -85,6 +86,38 @@ function defaultCredentialName(deviceType: string | undefined): string { return deviceType === 'multiDevice' ? 'Passkey (synced)' : 'Passkey'; } +/** + * `UsersRepository.findById` returns the repository's full-row shape + * (`UserRow` — every `users` column, `role: string`, several `T | null` + * columns); the client-payload helper `stripUserForClient` takes the + * narrower `User` contract type (`role: 'admin' | 'user'`, those same + * columns `T | undefined`). This is the one place in this file that reads a + * user through the repository and hands it to that helper, so the mapping + * lives here rather than widening `stripUserForClient`'s parameter for + * every other caller (Plan 3b Task 3 review, F5). + * + * A spread, not a field-by-field reconstruction: the legacy raw-SQL + * equivalent (`this.db.get('SELECT * FROM users WHERE id = ?', ...)`, + * `daef15be7:passkey.service.ts:417`) handed `stripUserForClient` the + * *actual* full row at runtime despite its `User`-typed generic — every + * column `SELECT *` returns, not just the ones `User` declares. Narrowing + * to an explicit field list here would silently drop columns the legacy + * response carried (parity is law); only the columns whose *type* actually + * conflicts (`role`'s string vs. the union, and the `T | null` columns + * `User` declares as `T | undefined`) are overridden below — every other + * column passes through unchanged, exactly as it did before. + */ +function toClientUser(row: UserRow): User { + return { + ...row, + role: row.role === 'admin' ? 'admin' : 'user', + mfa_enabled: row.mfa_enabled ?? undefined, + must_change_password: row.must_change_password ?? undefined, + created_at: row.created_at ?? undefined, + updated_at: row.updated_at ?? undefined, + }; +} + /** * WebAuthn (passkey) registration, discoverable-credential login and * credential management. No instance state — the challenge store is DB-backed @@ -405,7 +438,7 @@ export class PasskeyService { // (device possession + biometric/PIN), so it mints the real session directly // — the SAME path as password and OIDC login (no new token shape). const token = await this.auth.generateToken(user); - const userSafe = stripUserForClient(user as unknown as User) as Record; + const userSafe = stripUserForClient(toClientUser(user)) as Record; return { token, user: { ...userSafe, avatar_url: avatarUrl(user) }, auditUserId: Number(user.id) }; } @@ -421,8 +454,20 @@ export class PasskeyService { async renamePasskey(userId: number, id: string, name: unknown): Promise<{ error?: string; status?: number; success?: boolean }> { const cleanName = sanitizeName(name); if (!cleanName) return { error: 'Name is required', status: 400 }; + // Convert, VALIDATE, and answer the legacy not-found before the + // repository call (program rule 15): the legacy `UPDATE ... WHERE id = ? + // AND user_id = ?` bound `Number(id)` as a plain parameter — a + // non-numeric route id produced `NaN`, which SQLite compared against the + // INTEGER `id` column and never matched (`NaN` is never `=` to + // anything), so `changes === 0` and this returned its ordinary 404. A + // typed MikroORM filter has no such leniency: it renders a JS `NaN` as + // the bare, unquoted token `NaN`, which SQLite parses as a column + // reference and throws — a 500 where the legacy 404'd (Plan 3b Task 3 + // review, F1). + const rowId = toRowId(id); + if (rowId === null) return { error: 'Passkey not found', status: 404 }; // Ownership enforced in SQL (404 on miss, never a 403 that leaks existence). - const changes = await this.webauthnCredentials.renameOwned(Number(id), userId, cleanName); + const changes = await this.webauthnCredentials.renameOwned(rowId, userId, cleanName); if (changes === 0) return { error: 'Passkey not found', status: 404 }; return { success: true }; } @@ -440,16 +485,29 @@ export class PasskeyService { if (!passwordHash || !password || !bcrypt.compareSync(password, passwordHash)) { return { error: 'Incorrect password', status: 401 }; } - const changes = await this.webauthnCredentials.deleteOwned(Number(id), userId); + // Same guard as `renamePasskey` (F1) — placed after the password check + // to match the legacy statement order exactly (a wrong password still + // answers 401 before a bad id is ever considered). + const rowId = toRowId(id); + if (rowId === null) return { error: 'Passkey not found', status: 404 }; + const changes = await this.webauthnCredentials.deleteOwned(rowId, userId); if (changes === 0) return { error: 'Passkey not found', status: 404 }; return { success: true }; } /** Admin: clear all of a user's passkeys (e.g. on suspected compromise). */ async adminResetPasskeys(targetUserId: number): Promise<{ error?: string; status?: number; success?: boolean; deleted?: number; email?: string }> { - const target = await this.users.findIdAndEmail(targetUserId); + // `AdminService.resetUserPasskeys` converts the route param with a bare + // `Number(id)` before calling in — a non-numeric id arrives here as + // `NaN`, still typed `number` at the JS level. Same F1 guard: validate + // before the repository call and answer the legacy 404 (the raw + // `SELECT id, email FROM users WHERE id = ?` bound `Number(id)` too and + // simply matched no row). + const rowId = toRowId(targetUserId); + if (rowId === null) return { error: 'User not found', status: 404 }; + const target = await this.users.findIdAndEmail(rowId); if (!target) return { error: 'User not found', status: 404 }; - const deleted = await this.webauthnCredentials.deleteAllForUser(targetUserId); + const deleted = await this.webauthnCredentials.deleteAllForUser(rowId); return { success: true, deleted, email: target.email }; } } diff --git a/server/src/nest/auth/registration-invites.service.ts b/server/src/nest/auth/registration-invites.service.ts index 425eb85ba..3ae137064 100644 --- a/server/src/nest/auth/registration-invites.service.ts +++ b/server/src/nest/auth/registration-invites.service.ts @@ -4,6 +4,7 @@ import crypto from 'crypto'; import { DatabaseService } from '../database/database.service'; import { InviteTokens } from '../../db/entities/InviteTokens.entity'; import type { InviteTokensRepository } from '../../db/repositories/InviteTokens.repository'; +import { toRowId } from '../common/row-id'; /** * Registration invites: the tokens an admin hands out so someone can create an @@ -90,9 +91,16 @@ export class RegistrationInvitesService { // statement tolerated a non-numeric string bind and simply matched no // row, so the 404 below reproduces that same observable outcome without // routing an invalid value into the query layer). - const numericId = Number(id); + // + // `toRowId`, not a bare `Number.isInteger(Number(id))` guard: the plain + // guard accepted prefixed numeric literals JS understands and SQLite's + // INTEGER affinity does not (`'0x10'` → `16`, `'0b100'` → `4`), which is + // a genuine parity break, not just stricter validation (Plan 3b Task 3 + // review, F2) — `toRowId` requires the digits-only shape the legacy + // raw-string bind actually matched. + const numericId = toRowId(id); // RI6 — the 404 check. - if (!Number.isInteger(numericId) || (await this.inviteTokens.findIdById(numericId)) === null) { + if (numericId === null || (await this.inviteTokens.findIdById(numericId)) === null) { return { error: 'Invite not found', status: 404 }; } // RI7. diff --git a/server/tests/integration/passkey.test.ts b/server/tests/integration/passkey.test.ts new file mode 100644 index 000000000..5af013829 --- /dev/null +++ b/server/tests/integration/passkey.test.ts @@ -0,0 +1,141 @@ +/** + * Passkey management + admin invite deletion — non-numeric/prefixed id + * regression coverage for Plan 3b Task 3's fix round. + * + * F1 (HIGH): `renamePasskey`/`deletePasskey`/`adminResetPasskeys` used to + * hand a bare `Number(id)` to a MikroORM filter — a non-numeric route id + * became `NaN`, which the ORM renders as the unquoted token `NaN` in the + * generated SQL, and SQLite throws `InvalidFieldNameException: no such + * column: NaN` (a 500). The legacy raw-SQL route always answered its + * ordinary 404 for the same input. These cases pin the legacy status AND + * body through a real HTTP request, per the fix round's task brief. + * + * F2 (LOW): `RegistrationInvitesService.deleteInvite`'s `Number(id)` + + * `Number.isInteger` guard accepted prefixed numeric literals ('0x10') that + * the legacy raw-string bind never matched — `toRowId` closes that gap. + * + * A new file, not an addition to `admin.test.ts`/`auth.test.ts`: another + * implementer owns in-flight, unstaged changes in both files at the time + * this fix round runs (Task 4, OAuth). + */ +import { describe, it, expect, vi, beforeAll, beforeEach, afterAll } from 'vitest'; +import request from 'supertest'; +import type { Application } from 'express'; +import type { INestApplication } from '@nestjs/common'; + +vi.mock('../../src/db/database', async () => { + const { createSnapshotTestDb, buildDbMock } = await import('../helpers/db-mock'); + return buildDbMock(createSnapshotTestDb()); +}); +vi.mock('../../src/config', () => ({ + JWT_SECRET: 'test-jwt-secret-for-trek-testing-only', + ENCRYPTION_KEY: 'a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2a3b4c5d6a7b8c9d0e1f2', + updateJwtSecret: () => {}, + SESSION_DURATION: '24h', + SESSION_DURATION_MS: 86400000, + SESSION_DURATION_SECONDS: 86400, + DEFAULT_LANGUAGE: 'en', +})); +vi.mock('../../src/websocket', () => ({ broadcast: vi.fn(), broadcastToUser: vi.fn() })); + +import { db as testDb } from '../../src/db/database'; +import { buildApp } from '../../src/bootstrap'; +import { resetTestDb, resetRateLimits } from '../helpers/test-db'; +import { createUser, createAdmin } from '../helpers/factories'; +import { authCookie } from '../helpers/auth'; + +let nestApp: INestApplication; +let app: Application; + +beforeAll(async () => { + nestApp = await buildApp(); + app = nestApp.getHttpAdapter().getInstance(); +}); + +beforeEach(() => { + resetTestDb(testDb); + resetRateLimits(nestApp); +}); + +afterAll(async () => { + await nestApp.close(); + testDb.close(); +}); + +describe('Passkey management — non-numeric id parity (Plan 3b Task 3 review, F1)', () => { + it('PASSKEY-INT-001 — PATCH /auth/passkey/credentials/abc (non-numeric id) returns the legacy 404, not a 500', async () => { + const { user } = createUser(testDb); + + const res = await request(app) + .patch('/api/auth/passkey/credentials/abc') + .set('Cookie', authCookie(user.id)) + .send({ name: 'Ghost' }); + + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Passkey not found' }); + }); + + it('PASSKEY-INT-002 — PATCH /auth/passkey/credentials/1abc (leading-digit non-numeric id) returns the legacy 404, not a 500', async () => { + const { user } = createUser(testDb); + + const res = await request(app) + .patch('/api/auth/passkey/credentials/1abc') + .set('Cookie', authCookie(user.id)) + .send({ name: 'Ghost' }); + + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Passkey not found' }); + }); + + it('PASSKEY-INT-003 — PATCH /auth/passkey/credentials/0x10 (hex-literal id) returns the legacy 404, not a 500', async () => { + const { user } = createUser(testDb); + + const res = await request(app) + .patch('/api/auth/passkey/credentials/0x10') + .set('Cookie', authCookie(user.id)) + .send({ name: 'Ghost' }); + + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Passkey not found' }); + }); + + it('PASSKEY-INT-004 — DELETE /auth/passkey/credentials/abc (non-numeric id, correct password) returns the legacy 404, not a 500', async () => { + const { user, password } = createUser(testDb); + + const res = await request(app) + .delete('/api/auth/passkey/credentials/abc') + .set('Cookie', authCookie(user.id)) + .send({ password }); + + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Passkey not found' }); + }); + + it('PASSKEY-INT-005 — DELETE /admin/users/abc/passkeys (non-numeric id) returns the legacy 404, not a 500', async () => { + const { user: admin } = createAdmin(testDb); + + const res = await request(app) + .delete('/api/admin/users/abc/passkeys') + .set('Cookie', authCookie(admin.id)); + + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'User not found' }); + }); +}); + +describe('Registration invite deletion — prefixed numeric literal parity (Plan 3b Task 3 review, F2)', () => { + it('INVITE-INT-001 — DELETE /admin/invites/0x10 (hex literal) returns the legacy 404 and leaves invite id 16 untouched', async () => { + const { user: admin } = createAdmin(testDb); + testDb + .prepare('INSERT INTO invite_tokens (id, token, max_uses, used_count, expires_at, created_by) VALUES (16, ?, 1, 0, NULL, ?)') + .run('hex-literal-survivor', admin.id); + + const res = await request(app) + .delete('/api/admin/invites/0x10') + .set('Cookie', authCookie(admin.id)); + + expect(res.status).toBe(404); + expect(res.body).toEqual({ error: 'Invite not found' }); + expect(testDb.prepare('SELECT id FROM invite_tokens WHERE id = 16').get()).toBeDefined(); + }); +}); diff --git a/server/tests/unit/db/repositories/InviteTokens.repository.test.ts b/server/tests/unit/db/repositories/InviteTokens.repository.test.ts index 29bd1af07..f10f082b4 100644 --- a/server/tests/unit/db/repositories/InviteTokens.repository.test.ts +++ b/server/tests/unit/db/repositories/InviteTokens.repository.test.ts @@ -1,4 +1,4 @@ -import { afterAll, beforeAll, beforeEach, describe, expect, it } from 'vitest'; +import { afterAll, beforeAll, beforeEach, describe, expect, it, vi } from 'vitest'; import { createSnapshotTestDb } from '../../../helpers/db-mock'; import { resetTestDb } from '../../../helpers/test-db'; import { createTestOrm, type TestOrm } from '../../../helpers/test-orm'; @@ -21,6 +21,26 @@ function rawInvite(id: number): unknown { return testDb.prepare('SELECT * FROM invite_tokens WHERE id = ?').get(id); } +/** + * Spies on the ORM connection to capture the raw generated SQL of the one + * query `fn` runs, the same idiom `Categories.repository.test.ts`'s + * `withQueryCount` uses for a call-count assertion — here for the query + * text itself, since `listWithCreatorAndTrip`/`findWithCreatorAndTrip` run + * with `mapResults: false` and return driver rows, not a QueryBuilder a + * test could inspect directly (Plan 3b Task 3 review, F4). + */ +async function captureSql(fn: () => Promise): Promise<{ value: T; sql: string }> { + const connection = t.orm.em.getConnection(); + const spy = vi.spyOn(connection, 'execute'); + try { + const value = await fn(); + const call = spy.mock.calls.find((c) => typeof c[0] === 'string' && c[0].includes('invite_tokens')); + return { value, sql: (call?.[0] as string | undefined) ?? '' }; + } finally { + spy.mockRestore(); + } +} + describe('InviteTokensRepository', () => { describe('findByToken', () => { it('INVREPO-001: a row read through findByToken is the SELECT * row, key for key', async () => { @@ -138,6 +158,17 @@ describe('InviteTokensRepository', () => { it('INVREPO-012: findWithCreatorAndTrip is null for an unknown id', async () => { expect(await invites.findWithCreatorAndTrip(999_999)).toBeNull(); }); + + it('INVREPO-015: the generated SQL joins users with an INNER JOIN and trips with a LEFT JOIN — pins RI1\'s join TYPE (Plan 3b Task 3 review, F4; a `leftJoin` rewrite of the users join must fail this)', async () => { + const { user: admin } = createUser(testDb, { username: 'join-type-check' }); + createInviteToken(testDb, { token: 'join-type-check', created_by: admin.id }); + + const { sql } = await captureSql(() => invites.listWithCreatorAndTrip()); + + expect(sql).toMatch(/inner join `?users`?/i); + expect(sql).toMatch(/left join `?trips`?/i); + expect(sql).not.toMatch(/left join `?users`?/i); + }); }); // RI6/RI7 — deleteInvite's 404 check and the delete itself. diff --git a/server/tests/unit/nest/passkey.service.test.ts b/server/tests/unit/nest/passkey.service.test.ts index eaed0baa3..acf33aa06 100644 --- a/server/tests/unit/nest/passkey.service.test.ts +++ b/server/tests/unit/nest/passkey.service.test.ts @@ -616,13 +616,19 @@ describe('passkeyLoginVerify', () => { it('PASSKEY-SVC-024: success mints a real session, strips the user and bumps the bookkeeping', async () => { const { user } = createUser(testDb); + // A `users` column outside the `User` contract type (Plan 3b Task 3 + // review, F5) — the legacy `stripUserForClient(user)` ran on the FULL + // `SELECT *` row at runtime regardless of its `User`-typed generic, so + // this must still survive the repository-backed path's `toClientUser` + // spread, not be silently dropped by a narrower field-by-field mapping. + testDb.prepare("UPDATE users SET display_name = 'Legacy Passthrough' WHERE id = ?").run(user.id); const cred = insertCredential(user.id, { credential_id: 'good', counter: 5 }); seedChallenge('a9', null, 'authentication'); swMock.verifyAuthenticationResponse.mockResolvedValue({ verified: true, authenticationInfo: { newCounter: 6 } }); const result = await svc.passkeyLoginVerify({ assertionResponse: ceremonyResponse('a9', { id: 'good' }) }); expect(result.auditUserId).toBe(user.id); - expect(result.user).toMatchObject({ id: user.id, email: user.email }); + expect(result.user).toMatchObject({ id: user.id, email: user.email, display_name: 'Legacy Passthrough' }); expect(result.user).toHaveProperty('avatar_url'); expect(result.user).not.toHaveProperty('password_hash'); // The token is the SAME session shape password login mints ({ id, pv }). @@ -719,6 +725,13 @@ describe('renamePasskey', () => { expect(await svc.renamePasskey(other.id, String(cred.id), 'Steal')).toEqual({ error: 'Passkey not found', status: 404 }); expect(await svc.renamePasskey(user.id, '999999', 'Ghost')).toEqual({ error: 'Passkey not found', status: 404 }); }); + + it('PASSKEY-SVC-042: 404s (not a 500) on a non-numeric or prefixed-numeric id — Plan 3b Task 3 review, F1', async () => { + const { user } = createUser(testDb); + expect(await svc.renamePasskey(user.id, 'abc', 'Ghost')).toEqual({ error: 'Passkey not found', status: 404 }); + expect(await svc.renamePasskey(user.id, '1abc', 'Ghost')).toEqual({ error: 'Passkey not found', status: 404 }); + expect(await svc.renamePasskey(user.id, '0x10', 'Ghost')).toEqual({ error: 'Passkey not found', status: 404 }); + }); }); // ── deletePasskey ───────────────────────────────────────────────────────────── @@ -742,6 +755,17 @@ describe('deletePasskey', () => { expect(await svc.deletePasskey(user.id, String(cred.id), password)).toEqual({ success: true }); expect(testDb.prepare('SELECT COUNT(*) AS n FROM webauthn_credentials').get()).toEqual({ n: 0 }); }); + + it('PASSKEY-SVC-043: 404s (not a 500) on a non-numeric id once the password check passes — Plan 3b Task 3 review, F1', async () => { + const { user, password } = createUser(testDb); + expect(await svc.deletePasskey(user.id, 'abc', password)).toEqual({ error: 'Passkey not found', status: 404 }); + expect(await svc.deletePasskey(user.id, '0x10', password)).toEqual({ error: 'Passkey not found', status: 404 }); + }); + + it('PASSKEY-SVC-044: a wrong password still wins over a non-numeric id — 401, not 404 (legacy statement order)', async () => { + const { user, password } = createUser(testDb); + expect(await svc.deletePasskey(user.id, 'abc', `${password}x`)).toEqual({ error: 'Incorrect password', status: 401 }); + }); }); // ── adminResetPasskeys ──────────────────────────────────────────────────────── @@ -760,4 +784,8 @@ describe('adminResetPasskeys', () => { expect(await svc.adminResetPasskeys(user.id)).toEqual({ success: true, deleted: 0, email: user.email }); expect(testDb.prepare('SELECT id FROM webauthn_credentials').all()).toEqual([{ id: kept.id }]); }); + + it('PASSKEY-SVC-045: 404s (not a 500) when the caller hands in NaN — AdminService.resetUserPasskeys converts a non-numeric route id with a bare Number(id) before calling in (Plan 3b Task 3 review, F1)', async () => { + expect(await svc.adminResetPasskeys(Number('abc'))).toEqual({ error: 'User not found', status: 404 }); + }); }); diff --git a/server/tests/unit/nest/registration-invites.service.test.ts b/server/tests/unit/nest/registration-invites.service.test.ts index 1ac94d60f..e5a1ca720 100644 --- a/server/tests/unit/nest/registration-invites.service.test.ts +++ b/server/tests/unit/nest/registration-invites.service.test.ts @@ -74,6 +74,16 @@ describe('Invites', () => { const result = await svc.deleteInvite('99999') as any; expect(result.status).toBe(404); }); + + it('ADMIN-SVC-029 — deleteInvite(\'0x10\') is the legacy 404, not a hex-literal delete of invite id 16 (Plan 3b Task 3 review, F2)', async () => { + const { user: admin } = createAdmin(testDb); + testDb.prepare('INSERT INTO invite_tokens (id, token, max_uses, used_count, expires_at, created_by) VALUES (16, ?, 1, 0, NULL, ?)').run('hex-survivor', admin.id); + + const result = await svc.deleteInvite('0x10'); + + expect(result).toEqual({ error: 'Invite not found', status: 404 }); + expect(testDb.prepare('SELECT id FROM invite_tokens WHERE id = 16').get()).toBeDefined(); + }); }); describe('Invites — trip binding', () => {