fix(auth): non-numeric passkey and invite ids answer the legacy 404, never NaN; invite join type pinned

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<User>('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<string, unknown>;
+    const userSafe = stripUserForClient(toClientUser(user)) as Record<string, unknown>;
     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 };
   }
 }

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.

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();
+  });
+});

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<T>(fn: () => Promise<T>): 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.

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 });
+  });
 });

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', () => {