diff --git a/migration-graph.md b/migration-graph.md index 5625bb50c..86fa4a88d 100644 --- a/migration-graph.md +++ b/migration-graph.md @@ -652,27 +652,36 @@ Wave-2 `permissions` + `auditLog` pair were the first frontier picks — all don branch coverage below 80% until the fold added AUTH-DB-050…088 — the gate polices relocations, not just new code. -## Quirks preserved in the auth fold (trailing `fix(server)` candidates) +## Quirks fixed after the auth fold (the trailing `fix(server)` commit) -The relocation was byte-identical; these verified oddities were carried as-is and await -their own fix commit (atlas/collections precedent — parity first, fixes separate): +The relocation itself was byte-identical; these were then fixed on top, each with a +regression test (`AUTH-DB-089…093`), so the parity diff and the behaviour change stayed +in separate commits: -1. **`getTravelStats` drops 0-coordinates.** `if (p.lat && p.lng)` skips a place on the - equator or prime meridian (the exact `|| null` class the atlas trailing commit fixed - in bucket items). -2. **`registerUser` multi-writes aren't transactional.** INSERT user → UPDATE - invite_tokens → `joinTripAsMember` run statement-by-statement; a mid-sequence throw - leaves a user without the invite bookkeeping. -3. **`verifyMfaLogin` splices the backup code outside a transaction.** The backup-code - UPDATE and the last_login UPDATE are separate statements. -4. **`enableMfa` bypasses `getPendingMfaSecret`'s TTL cleanup on failure** — a wrong code - leaves the pending secret live for the full 15 min (arguably by design). -5. **The two "Password authentication is disabled" strings differ** between login - (`… Please sign in with SSO.`) and changePassword (bare) — parity says both stay. -6. **`deleteMcpToken` calls `revokeUserSessions` unguarded** while changePassword/ - resetPassword wrap the same call in try/catch — an inconsistency, not yet a defect. -7. **`updateApiKeys` re-selects with `current!` non-null assertions** — a deleted-user - race throws instead of 404ing (pre-existing). +1. **`getTravelStats` dropped 0-coordinates.** `if (p.lat && p.lng)` skipped a place on + the equator or prime meridian (the exact falsy class the atlas trailing commit fixed + in bucket items). Now explicit `!= null` checks (AUTH-DB-089). +2. **`registerUser`'s multi-writes weren't transactional.** INSERT user → UPDATE + invite_tokens → `joinTripAsMember` ran statement-by-statement; a mid-sequence throw + left a half-registered user. The whole signup now runs in `db.transaction()` — a + throw rolls back the user row and the invite bookkeeping together (AUTH-DB-090). +3. **`verifyMfaLogin` spliced the backup code outside a transaction.** The backup-code + UPDATE and the last_login UPDATE now commit as one atomic pair — the code must not + burn without the login landing, or vice versa (AUTH-DB-091). +4. **`deleteMcpToken` called `revokeUserSessions` unguarded** while changePassword/ + resetPassword wrap the same call in try/catch — a session-sweep failure turned a + successful token delete into a 500. Now best-effort like the others (AUTH-DB-092). +5. **`updateApiKeys` re-selected with `current!` non-null assertions** — a user row + deleted mid-request threw a TypeError (→ 500) instead of degrading to a 0-row + UPDATE. Now `current?.… ?? null` (AUTH-DB-093). + +**Quirks deliberately preserved** (parity, not oversights): the two divergent +"Password authentication is disabled" strings (login's `… Please sign in with SSO.` +vs changePassword's bare form — both client-visible contracts); `enableMfa` leaving +the pending secret live for the full 15-min TTL after a wrong code (re-entry by +design); the import-time `DUMMY_PASSWORD_HASH` bcrypt and `avatarDir` mkdir (documented +side-effect exceptions); and the module-scoped per-email reset throttle with its +unref'd cleanup interval (shared across the bridge and container instances on purpose). ## Quirks fixed after the atlas fold (the trailing `fix(server)` commit) diff --git a/server/src/nest/auth/auth.service.ts b/server/src/nest/auth/auth.service.ts index daf3bfddb..83263d862 100644 --- a/server/src/nest/auth/auth.service.ts +++ b/server/src/nest/auth/auth.service.ts @@ -361,35 +361,39 @@ export class AuthService { const role = isFirstUser ? 'admin' : 'user'; try { - const result = this.db.run( - 'INSERT INTO users (username, email, password_hash, role, first_seen_version, login_count) VALUES (?, ?, ?, ?, ?, 0)', - username, email, password_hash, role, readEnv().app.appVersion || '0.0.0' - ); - - const user = { id: result.lastInsertRowid, username, email, role, avatar: null, mfa_enabled: false }; - const token = this.generateToken(user); - - if (validInvite) { - const updated = this.db.get( - 'UPDATE invite_tokens SET used_count = used_count + 1 WHERE id = ? AND (max_uses = 0 OR used_count < max_uses) RETURNING used_count', - validInvite.id + // One transaction for the whole signup: a mid-sequence throw (invite + // bookkeeping, trip auto-join) must not leave a half-registered user. + return this.db.transaction(() => { + const result = this.db.run( + 'INSERT INTO users (username, email, password_hash, role, first_seen_version, login_count) VALUES (?, ?, ?, ?, ?, 0)', + username, email, password_hash, role, readEnv().app.appVersion || '0.0.0' ); - if (!updated) { - console.warn(`[Auth] Invite token ${validInvite.token.slice(0, 8)}... exceeded max_uses due to race condition`); - } - // Trip-bound invite (#1402): auto-add the freshly registered user to the - // trip. Idempotent + owner-safe; no-ops if the bound trip was since deleted. - if (validInvite.trip_id) { - joinTripAsMember(Number(validInvite.trip_id), Number(result.lastInsertRowid), validInvite.created_by ?? null); - } - } - return { - token, - user: { ...user, avatar_url: null }, - auditUserId: Number(result.lastInsertRowid), - auditDetails: { username, email, role }, - }; + const user = { id: result.lastInsertRowid, username, email, role, avatar: null, mfa_enabled: false }; + const token = this.generateToken(user); + + if (validInvite) { + const updated = this.db.get( + 'UPDATE invite_tokens SET used_count = used_count + 1 WHERE id = ? AND (max_uses = 0 OR used_count < max_uses) RETURNING used_count', + validInvite.id + ); + if (!updated) { + console.warn(`[Auth] Invite token ${validInvite.token.slice(0, 8)}... exceeded max_uses due to race condition`); + } + // Trip-bound invite (#1402): auto-add the freshly registered user to the + // trip. Idempotent + owner-safe; no-ops if the bound trip was since deleted. + if (validInvite.trip_id) { + joinTripAsMember(Number(validInvite.trip_id), Number(result.lastInsertRowid), validInvite.created_by ?? null); + } + } + + return { + token, + user: { ...user, avatar_url: null }, + auditUserId: Number(result.lastInsertRowid), + auditDetails: { username, email, role }, + }; + }); } catch { return { error: 'Error creating user', status: 500 }; } @@ -570,11 +574,13 @@ export class AuthService { const body = rawBody as { maps_api_key?: string; openweather_api_key?: string; unsplash_api_key?: string }; const current = this.db.get>('SELECT maps_api_key, openweather_api_key, unsplash_api_key FROM users WHERE id = ?', userId); + // `?? null` instead of the former non-null assertions: a user row deleted + // mid-request must degrade to a 0-row UPDATE, not a TypeError/500. this.db.run( 'UPDATE users SET maps_api_key = ?, openweather_api_key = ?, unsplash_api_key = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', - body.maps_api_key !== undefined ? maybe_encrypt_api_key(body.maps_api_key) : current!.maps_api_key, - body.openweather_api_key !== undefined ? maybe_encrypt_api_key(body.openweather_api_key) : current!.openweather_api_key, - body.unsplash_api_key !== undefined ? maybe_encrypt_api_key(body.unsplash_api_key) : current!.unsplash_api_key, + body.maps_api_key !== undefined ? maybe_encrypt_api_key(body.maps_api_key) : current?.maps_api_key ?? null, + body.openweather_api_key !== undefined ? maybe_encrypt_api_key(body.openweather_api_key) : current?.openweather_api_key ?? null, + body.unsplash_api_key !== undefined ? maybe_encrypt_api_key(body.unsplash_api_key) : current?.unsplash_api_key ?? null, userId ); @@ -918,7 +924,9 @@ export class AuthService { const coords: { lat: number; lng: number }[] = []; places.forEach(p => { - if (p.lat && p.lng) coords.push({ lat: p.lat, lng: p.lng }); + // Explicit null checks: lat/lng of exactly 0 (equator / prime meridian) + // are valid coordinates the former falsy check silently dropped. + if (p.lat != null && p.lng != null) coords.push({ lat: p.lat, lng: p.lng }); if (p.address) { const parts = p.address.split(',').map(s => s.trim().replace(/\d{3,}/g, '').trim()); const cityPart = parts.find(s => !KNOWN_COUNTRIES.has(s) && /^[A-Za-z\u00C0-\u00FF\s-]{2,}$/.test(s)); @@ -1101,12 +1109,18 @@ export class AuthService { return { error: 'Invalid verification code', status: 401 }; } hashes.splice(idx, 1); - this.db.run('UPDATE users SET mfa_backup_codes = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', - JSON.stringify(hashes), - user.id - ); + // Consume the backup code and record the login atomically — the code + // must not burn without the login landing (or vice versa). + this.db.transaction(() => { + this.db.run('UPDATE users SET mfa_backup_codes = ?, updated_at = CURRENT_TIMESTAMP WHERE id = ?', + JSON.stringify(hashes), + user.id + ); + this.db.run('UPDATE users SET last_login = CURRENT_TIMESTAMP, login_count = login_count + 1 WHERE id = ?', user.id); + }); + } else { + this.db.run('UPDATE users SET last_login = CURRENT_TIMESTAMP, login_count = login_count + 1 WHERE id = ?', user.id); } - this.db.run('UPDATE users SET last_login = CURRENT_TIMESTAMP, login_count = login_count + 1 WHERE id = ?', user.id); const sessionToken = this.generateToken(user, remember); const userSafe = stripUserForClient(user) as Record; return { @@ -1333,7 +1347,9 @@ export class AuthService { const token = this.db.get('SELECT id FROM mcp_tokens WHERE id = ? AND user_id = ?', tokenId, userId); if (!token) return { error: 'Token not found', status: 404 }; this.db.run('DELETE FROM mcp_tokens WHERE id = ?', tokenId); - revokeUserSessions(userId); + // Best-effort, like the changePassword/resetPassword revocations: a session + // sweep failure must not turn a successful token delete into a 500. + try { revokeUserSessions?.(userId); } catch { /* best-effort */ } return { success: true }; } diff --git a/server/tests/unit/nest/auth.service.test.ts b/server/tests/unit/nest/auth.service.test.ts index 224dfcf93..c3145af94 100644 --- a/server/tests/unit/nest/auth.service.test.ts +++ b/server/tests/unit/nest/auth.service.test.ts @@ -55,6 +55,7 @@ vi.mock('../../../src/services/apiKeyCrypto', () => ({ })); vi.mock('../../../src/services/ephemeralTokens', () => ({ createEphemeralToken: vi.fn() })); vi.mock('../../../src/services/notifications', () => ({ sendPasswordResetEmail: vi.fn() })); +vi.mock('../../../src/services/tripMembership', () => ({ joinTripAsMember: vi.fn() })); vi.mock('../../../src/mcp/sessionManager', () => ({ revokeUserSessions: vi.fn() })); vi.mock('../../../src/scheduler', () => ({ startTripReminders: vi.fn(), @@ -81,6 +82,8 @@ import { verifyJwtAndLoadUser } from '../../../src/middleware/auth'; import { authenticator } from 'otplib'; import { hashBackupCode } from '../../../src/nest/auth/auth.helpers'; import { createEphemeralToken } from '../../../src/services/ephemeralTokens'; +import { joinTripAsMember } from '../../../src/services/tripMembership'; +import { revokeUserSessions } from '../../../src/mcp/sessionManager'; const svc = new AuthService( new DatabaseService(testDb), @@ -1191,6 +1194,69 @@ describe('ephemeral + demo helpers', () => { }); }); + +// --------------------------------------------------------------------------- +// Quirk fixes after the DI fold (trailing fix(server) commit): AUTH-DB-089+. +// The relocation carried these verbatim; the fixes land on top with a pin each. +// --------------------------------------------------------------------------- + +describe('auth quirk fixes', () => { + it('AUTH-DB-089: getTravelStats keeps a place at lat 0 / lng 0 (equator, prime meridian)', () => { + const { user } = createUser(testDb); + const trip = createTrip(testDb, user.id, { title: 'Null Island' }); + testDb.prepare('INSERT INTO places (trip_id, name, lat, lng) VALUES (?, ?, ?, ?)').run(trip.id, 'Null Island', 0, 0); + const stats = svc.getTravelStats(user.id); + expect(stats.coords).toContainEqual({ lat: 0, lng: 0 }); + }); + + it('AUTH-DB-090: a throw mid-registration rolls the whole signup back (user + invite bookkeeping)', () => { + const { user: owner } = createUser(testDb); + const trip = createTrip(testDb, owner.id); + const invite = createInviteToken(testDb, { max_uses: 5 }); + testDb.prepare('UPDATE invite_tokens SET trip_id = ? WHERE id = ?').run(trip.id, invite.id); + vi.mocked(joinTripAsMember).mockImplementationOnce(() => { throw new Error('boom'); }); + + const result = svc.registerUser({ username: 'rollback', email: 'rollback@x.com', password: 'Secure123!', invite_token: invite.token }); + + expect(result).toEqual({ error: 'Error creating user', status: 500 }); + expect(testDb.prepare("SELECT id FROM users WHERE email = 'rollback@x.com'").get()).toBeUndefined(); + const { used_count } = testDb.prepare('SELECT used_count FROM invite_tokens WHERE id = ?').get(invite.id) as { used_count: number }; + expect(used_count).toBe(0); + }); + + it('AUTH-DB-091: a backup-code login burns the code and records the login as one atomic pair', () => { + const { user, password } = createUser(testDb); + const secret = authenticator.generateSecret(); + testDb.prepare('UPDATE users SET mfa_enabled = 1, mfa_secret = ?, mfa_backup_codes = ? WHERE id = ?') + .run('enc:' + secret, JSON.stringify([hashBackupCode('DDDD-4444')]), user.id); + const interstitial = svc.loginUser({ email: user.email, password }); + + const result = svc.verifyMfaLogin({ mfa_token: interstitial.mfa_token, code: 'DDDD-4444' }); + + expect(typeof result.token).toBe('string'); + const row = testDb.prepare('SELECT mfa_backup_codes, login_count, last_login FROM users WHERE id = ?').get(user.id) as { mfa_backup_codes: string; login_count: number; last_login: string | null }; + expect(JSON.parse(row.mfa_backup_codes)).toHaveLength(0); + expect(row.login_count).toBe(1); + expect(row.last_login).not.toBeNull(); + }); + + it('AUTH-DB-092: deleteMcpToken succeeds even when the session sweep throws (best-effort)', () => { + const { user } = createUser(testDb); + const created = svc.createMcpToken(user.id, 'sweep-down'); + const tokenId = String((created.token as { id: number }).id); + vi.mocked(revokeUserSessions).mockImplementationOnce(() => { throw new Error('sweep down'); }); + + expect(svc.deleteMcpToken(user.id, tokenId)).toEqual({ success: true }); + expect(testDb.prepare('SELECT id FROM mcp_tokens WHERE id = ?').get(tokenId)).toBeUndefined(); + }); + + it('AUTH-DB-093: updateApiKeys degrades gracefully when the user row is gone (no TypeError/500)', () => { + expect(() => svc.updateApiKeys(999999, { maps_api_key: 'k' })).not.toThrow(); + const result = svc.updateApiKeys(999999, { openweather_api_key: 'w' }); + expect(result.success).toBe(true); + }); +}); + // --------------------------------------------------------------------------- // auth.bridge.ts delegation (coverage gate: one case per bridge export) // ---------------------------------------------------------------------------