mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-08-07 21:16:44 +00:00
fix(server): tidy auth quirks after the DI fold
Verified defects the relocation carried byte-for-byte, each pinned by a regression test (AUTH-DB-089..093): - getTravelStats kept dropping lat/lng of exactly 0 (equator / prime meridian) through its falsy check — now explicit != null. - registerUser's INSERT user -> invite used_count bump -> joinTripAsMember ran statement-by-statement; the whole signup now runs in db.transaction() so a mid-sequence throw rolls everything back. - verifyMfaLogin's backup-code burn and last_login/login_count bump now commit as one atomic pair. - deleteMcpToken's revokeUserSessions call is best-effort (try/catch), matching the changePassword/resetPassword revocations. - updateApiKeys' current! non-null assertions became current?. ?? null — a user row deleted mid-request degrades to a 0-row UPDATE, not a TypeError/500. Deliberately preserved (documented in migration-graph.md): the two divergent "Password authentication is disabled" strings and enableMfa's 15-min pending-secret TTL after a wrong code.
This commit is contained in:
jubnl
+28
-19
@@ -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)
|
||||
|
||||
|
||||
@@ -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<Pick<User, 'maps_api_key' | 'openweather_api_key' | 'unsplash_api_key'>>('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<string, unknown>;
|
||||
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 };
|
||||
}
|
||||
|
||||
|
||||
@@ -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)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user