mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-06-21 14:21:46 +00:00
fix(auth): trim username and email on all write paths
Self-registration stored values verbatim, so trailing whitespace could produce rows that lookup code (which trims input) silently misses. Trim username and email before validation and INSERT in registerUser, adminService.updateUser, and oidcService.findOrCreateUser. updateSettings and adminService.createUser already trimmed correctly. Adds a one-shot backfill migration (trimUserWhitespace) that trims existing dirty rows; collisions are resolved by appending __migrated_<id> to the value with a loud console.warn so operators can review affected accounts. 18 new tests covering registration trim, duplicate detection, admin update trim, trip-member lookup regression, and all migration branches.
This commit is contained in:
@@ -1,6 +1,68 @@
|
||||
import Database from 'better-sqlite3';
|
||||
import { encrypt_api_key } from '../services/apiKeyCrypto';
|
||||
|
||||
export function trimUserWhitespace(db: Database.Database): void {
|
||||
type DirtyRow = { id: number; username?: string; email?: string };
|
||||
|
||||
const dirtyUsernames = db.prepare(
|
||||
`SELECT id, username FROM users WHERE username != TRIM(username)`
|
||||
).all() as DirtyRow[];
|
||||
|
||||
for (const row of dirtyUsernames) {
|
||||
const trimmed = row.username!.trim();
|
||||
const collision = db.prepare(
|
||||
`SELECT id FROM users WHERE LOWER(username) = LOWER(?) AND id != ?`
|
||||
).get(trimmed, row.id) as { id: number } | undefined;
|
||||
|
||||
const final = collision ? `${trimmed}__migrated_${row.id}` : trimmed;
|
||||
if (collision) {
|
||||
console.warn(
|
||||
`[migration] WHITESPACE COLLISION username: user id=${row.id} ` +
|
||||
`original=${JSON.stringify(row.username)} trimmed="${trimmed}" ` +
|
||||
`collides with user id=${collision.id}. Renamed to "${final}". ` +
|
||||
`Manual review required.`
|
||||
);
|
||||
} else {
|
||||
console.warn(
|
||||
`[migration] Trimmed username for user id=${row.id}: ` +
|
||||
`${JSON.stringify(row.username)} → "${final}"`
|
||||
);
|
||||
}
|
||||
db.prepare(`UPDATE users SET username = ? WHERE id = ?`).run(final, row.id);
|
||||
}
|
||||
|
||||
const dirtyEmails = db.prepare(
|
||||
`SELECT id, email FROM users WHERE email != TRIM(email)`
|
||||
).all() as DirtyRow[];
|
||||
|
||||
for (const row of dirtyEmails) {
|
||||
const trimmed = row.email!.trim();
|
||||
const collision = db.prepare(
|
||||
`SELECT id FROM users WHERE LOWER(email) = LOWER(?) AND id != ?`
|
||||
).get(trimmed, row.id) as { id: number } | undefined;
|
||||
|
||||
let final = trimmed;
|
||||
if (collision) {
|
||||
const at = trimmed.lastIndexOf('@');
|
||||
final = at > 0
|
||||
? `${trimmed.slice(0, at)}__migrated_${row.id}${trimmed.slice(at)}`
|
||||
: `${trimmed}__migrated_${row.id}`;
|
||||
console.warn(
|
||||
`[migration] WHITESPACE COLLISION email: user id=${row.id} ` +
|
||||
`original=${JSON.stringify(row.email)} trimmed="${trimmed}" ` +
|
||||
`collides with user id=${collision.id}. Renamed to "${final}". ` +
|
||||
`User cannot sign in with this email until manually corrected.`
|
||||
);
|
||||
} else {
|
||||
console.warn(
|
||||
`[migration] Trimmed email for user id=${row.id}: ` +
|
||||
`${JSON.stringify(row.email)} → "${final}"`
|
||||
);
|
||||
}
|
||||
db.prepare(`UPDATE users SET email = ? WHERE id = ?`).run(final, row.id);
|
||||
}
|
||||
}
|
||||
|
||||
function runMigrations(db: Database.Database): void {
|
||||
db.exec('CREATE TABLE IF NOT EXISTS schema_version (version INTEGER NOT NULL)');
|
||||
const versionRow = db.prepare('SELECT version FROM schema_version').get() as { version: number } | undefined;
|
||||
@@ -2147,6 +2209,8 @@ function runMigrations(db: Database.Database): void {
|
||||
db.exec(`INSERT INTO migrations (timestamp, name) VALUES (1777810195344, 'InitialSchema1777810195344');`);
|
||||
db.exec(`INSERT INTO app_settings (key, value) VALUES ('app_version', '${process.env.APP_VERSION || '3.0.14'}')`);
|
||||
},
|
||||
// trim leading/trailing whitespace from stored usernames and emails
|
||||
() => trimUserWhitespace(db),
|
||||
];
|
||||
|
||||
if (currentVersion < migrations.length) {
|
||||
|
||||
@@ -112,7 +112,9 @@ export function createUser(data: { username: string; email: string; password: st
|
||||
}
|
||||
|
||||
export function updateUser(id: string, data: { username?: string; email?: string; role?: string; password?: string }) {
|
||||
const { username, email, role, password } = data;
|
||||
const username = typeof data.username === 'string' ? data.username.trim() : data.username;
|
||||
const email = typeof data.email === 'string' ? data.email.trim() : data.email;
|
||||
const { role, password } = data;
|
||||
const user = db.prepare('SELECT * FROM users WHERE id = ?').get(id) as User | undefined;
|
||||
|
||||
if (!user) return { error: 'User not found', status: 404 };
|
||||
|
||||
@@ -343,7 +343,9 @@ export function registerUser(body: {
|
||||
password?: string;
|
||||
invite_token?: string;
|
||||
}): { error?: string; status?: number; token?: string; user?: Record<string, unknown>; auditUserId?: number; auditDetails?: Record<string, unknown> } {
|
||||
const { username, email, password, invite_token } = body;
|
||||
const username = typeof body.username === 'string' ? body.username.trim() : '';
|
||||
const email = typeof body.email === 'string' ? body.email.trim() : '';
|
||||
const { password, invite_token } = body;
|
||||
|
||||
const userCount = (db.prepare('SELECT COUNT(*) as count FROM users').get() as { count: number }).count;
|
||||
|
||||
|
||||
@@ -350,7 +350,7 @@ export function findOrCreateUser(
|
||||
config: OidcConfig,
|
||||
inviteToken?: string,
|
||||
): { user: User } | { error: string } {
|
||||
const email = userInfo.email!.toLowerCase();
|
||||
const email = userInfo.email!.trim().toLowerCase();
|
||||
const name = userInfo.name || userInfo.preferred_username || email.split('@')[0];
|
||||
const sub = userInfo.sub;
|
||||
|
||||
|
||||
@@ -368,6 +368,53 @@ describe('Admin user management', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Admin user management — whitespace normalization
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('Admin user management — whitespace normalization', () => {
|
||||
it('ADMIN-UPDATE-TRIM-1 — PUT /admin/users/:id trims username before storing', async () => {
|
||||
const { user: admin } = createAdmin(testDb);
|
||||
const { user } = createUser(testDb);
|
||||
|
||||
const res = await request(app)
|
||||
.put(`/api/admin/users/${user.id}`)
|
||||
.set('Cookie', authCookie(admin.id))
|
||||
.send({ username: ' trimmedadmin ' });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
const row = testDb.prepare('SELECT username FROM users WHERE id = ?').get(user.id) as { username: string };
|
||||
expect(row.username).toBe('trimmedadmin');
|
||||
});
|
||||
|
||||
it('ADMIN-UPDATE-TRIM-2 — PUT /admin/users/:id trims email before storing', async () => {
|
||||
const { user: admin } = createAdmin(testDb);
|
||||
const { user } = createUser(testDb);
|
||||
|
||||
const res = await request(app)
|
||||
.put(`/api/admin/users/${user.id}`)
|
||||
.set('Cookie', authCookie(admin.id))
|
||||
.send({ email: ' newemail@example.com ' });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
const row = testDb.prepare('SELECT email FROM users WHERE id = ?').get(user.id) as { email: string };
|
||||
expect(row.email).toBe('newemail@example.com');
|
||||
});
|
||||
|
||||
it('ADMIN-UPDATE-TRIM-3 — PUT /admin/users/:id with whitespace-padded username that trims to existing returns 409', async () => {
|
||||
const { user: admin } = createAdmin(testDb);
|
||||
const { user: existing } = createUser(testDb, { username: 'carol' });
|
||||
const { user: target } = createUser(testDb);
|
||||
|
||||
const res = await request(app)
|
||||
.put(`/api/admin/users/${target.id}`)
|
||||
.set('Cookie', authCookie(admin.id))
|
||||
.send({ username: ` ${existing.username} ` });
|
||||
|
||||
expect(res.status).toBe(409);
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// System stats
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -218,6 +218,54 @@ describe('Registration', () => {
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Registration — whitespace normalization
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
describe('Registration — whitespace normalization', () => {
|
||||
it('AUTH-REG-TRIM-1 — username with surrounding whitespace is trimmed before storage', async () => {
|
||||
const res = await request(app).post('/api/auth/register').send({
|
||||
username: ' trimmeduser ',
|
||||
email: 'trimmed@example.com',
|
||||
password: 'Str0ng!Pass',
|
||||
});
|
||||
expect(res.status).toBe(201);
|
||||
const row = testDb.prepare('SELECT username FROM users WHERE email = ?').get('trimmed@example.com') as { username: string };
|
||||
expect(row.username).toBe('trimmeduser');
|
||||
});
|
||||
|
||||
it('AUTH-REG-TRIM-2 — email with surrounding whitespace is trimmed before storage', async () => {
|
||||
const res = await request(app).post('/api/auth/register').send({
|
||||
username: 'emailtrimuser',
|
||||
email: ' emailtrim@example.com ',
|
||||
password: 'Str0ng!Pass',
|
||||
});
|
||||
expect(res.status).toBe(201);
|
||||
const row = testDb.prepare('SELECT email FROM users WHERE username = ?').get('emailtrimuser') as { email: string };
|
||||
expect(row.email).toBe('emailtrim@example.com');
|
||||
});
|
||||
|
||||
it('AUTH-REG-TRIM-3 — whitespace-padded username that trims to existing username returns 409', async () => {
|
||||
createUser(testDb, { username: 'alice', email: 'alice@example.com' });
|
||||
const res = await request(app).post('/api/auth/register').send({
|
||||
username: ' alice ',
|
||||
email: 'alice2@example.com',
|
||||
password: 'Str0ng!Pass',
|
||||
});
|
||||
expect(res.status).toBe(409);
|
||||
});
|
||||
|
||||
it('AUTH-REG-TRIM-4 — whitespace-padded email that trims to existing email returns 409', async () => {
|
||||
createUser(testDb, { username: 'bob', email: 'bob@example.com' });
|
||||
const res = await request(app).post('/api/auth/register').send({
|
||||
username: 'bob2',
|
||||
email: ' bob@example.com ',
|
||||
password: 'Str0ng!Pass',
|
||||
});
|
||||
expect(res.status).toBe(409);
|
||||
});
|
||||
});
|
||||
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
// Session / Me
|
||||
// ─────────────────────────────────────────────────────────────────────────────
|
||||
|
||||
@@ -677,6 +677,20 @@ describe('Trip members', () => {
|
||||
expect(res.body.error).toMatch(/already/i);
|
||||
});
|
||||
|
||||
it('TRIP-013 — Adding a member by whitespace-padded username resolves correctly → 201', async () => {
|
||||
const { user: owner } = createUser(testDb);
|
||||
const { user: invitee } = createUser(testDb, { username: 'paddeduser' });
|
||||
const trip = createTrip(testDb, owner.id, { title: 'Padded Trip' });
|
||||
|
||||
const res = await request(app)
|
||||
.post(`/api/trips/${trip.id}/members`)
|
||||
.set('Cookie', authCookie(owner.id))
|
||||
.send({ identifier: ' paddeduser ' });
|
||||
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.member.id).toBe(invitee.id);
|
||||
});
|
||||
|
||||
it('TRIP-014 — DELETE /api/trips/:id/members/:userId removes a member → 200', async () => {
|
||||
const { user: owner } = createUser(testDb);
|
||||
const { user: member } = createUser(testDb);
|
||||
|
||||
@@ -0,0 +1,122 @@
|
||||
/**
|
||||
* Unit tests for trimUserWhitespace — the backfill migration that normalises
|
||||
* leading/trailing whitespace in stored usernames and emails.
|
||||
* Tests TRIM-MIG-001 through TRIM-MIG-010.
|
||||
*/
|
||||
import { describe, it, expect, vi, beforeEach } from 'vitest';
|
||||
import Database from 'better-sqlite3';
|
||||
import { trimUserWhitespace } from '../../../src/db/migrations';
|
||||
|
||||
function makeDb() {
|
||||
const db = new Database(':memory:');
|
||||
db.exec('PRAGMA foreign_keys = ON');
|
||||
db.exec(`
|
||||
CREATE TABLE users (
|
||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
username TEXT UNIQUE NOT NULL,
|
||||
email TEXT UNIQUE NOT NULL,
|
||||
password_hash TEXT NOT NULL DEFAULT 'x',
|
||||
role TEXT NOT NULL DEFAULT 'user'
|
||||
)
|
||||
`);
|
||||
return db;
|
||||
}
|
||||
|
||||
function insert(db: Database.Database, username: string, email: string): number {
|
||||
const r = db.prepare('INSERT INTO users (username, email) VALUES (?, ?)').run(username, email);
|
||||
return Number(r.lastInsertRowid);
|
||||
}
|
||||
|
||||
function row(db: Database.Database, id: number) {
|
||||
return db.prepare('SELECT username, email FROM users WHERE id = ?').get(id) as { username: string; email: string };
|
||||
}
|
||||
|
||||
describe('trimUserWhitespace — clean data (no-op)', () => {
|
||||
it('TRIM-MIG-001 — leaves already-clean rows untouched', () => {
|
||||
const db = makeDb();
|
||||
const id = insert(db, 'alice', 'alice@example.com');
|
||||
trimUserWhitespace(db);
|
||||
expect(row(db, id)).toEqual({ username: 'alice', email: 'alice@example.com' });
|
||||
});
|
||||
});
|
||||
|
||||
describe('trimUserWhitespace — non-colliding dirty rows', () => {
|
||||
it('TRIM-MIG-002 — trims trailing whitespace from username', () => {
|
||||
const db = makeDb();
|
||||
const id = insert(db, 'alice ', 'alice@example.com');
|
||||
trimUserWhitespace(db);
|
||||
expect(row(db, id).username).toBe('alice');
|
||||
});
|
||||
|
||||
it('TRIM-MIG-003 — trims leading whitespace from username', () => {
|
||||
const db = makeDb();
|
||||
const id = insert(db, ' alice', 'alice@example.com');
|
||||
trimUserWhitespace(db);
|
||||
expect(row(db, id).username).toBe('alice');
|
||||
});
|
||||
|
||||
it('TRIM-MIG-004 — trims surrounding whitespace from email', () => {
|
||||
const db = makeDb();
|
||||
const id = insert(db, 'alice', ' alice@example.com ');
|
||||
trimUserWhitespace(db);
|
||||
expect(row(db, id).email).toBe('alice@example.com');
|
||||
});
|
||||
|
||||
it('TRIM-MIG-005 — emits a console.warn for each trimmed row', () => {
|
||||
const db = makeDb();
|
||||
insert(db, 'bob ', 'bob@example.com');
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
|
||||
trimUserWhitespace(db);
|
||||
expect(warn).toHaveBeenCalledWith(expect.stringContaining('[migration] Trimmed username'));
|
||||
warn.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
describe('trimUserWhitespace — username collision handling', () => {
|
||||
it('TRIM-MIG-006 — renames the dirty row to <trimmed>__migrated_<id> on collision', () => {
|
||||
const db = makeDb();
|
||||
insert(db, 'carol', 'carol@example.com');
|
||||
const dirtyId = insert(db, 'carol ', 'carol2@example.com');
|
||||
trimUserWhitespace(db);
|
||||
expect(row(db, dirtyId).username).toBe(`carol__migrated_${dirtyId}`);
|
||||
});
|
||||
|
||||
it('TRIM-MIG-007 — emits a WHITESPACE COLLISION warning for username collision', () => {
|
||||
const db = makeDb();
|
||||
insert(db, 'dan', 'dan@example.com');
|
||||
insert(db, 'dan ', 'dan2@example.com');
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
|
||||
trimUserWhitespace(db);
|
||||
expect(warn).toHaveBeenCalledWith(expect.stringContaining('WHITESPACE COLLISION username'));
|
||||
warn.mockRestore();
|
||||
});
|
||||
|
||||
it('TRIM-MIG-008 — the renamed value does not conflict with the existing clean row', () => {
|
||||
const db = makeDb();
|
||||
const cleanId = insert(db, 'eve', 'eve@example.com');
|
||||
const dirtyId = insert(db, 'eve ', 'eve2@example.com');
|
||||
trimUserWhitespace(db);
|
||||
expect(row(db, cleanId).username).toBe('eve');
|
||||
expect(row(db, dirtyId).username).toBe(`eve__migrated_${dirtyId}`);
|
||||
});
|
||||
});
|
||||
|
||||
describe('trimUserWhitespace — email collision handling', () => {
|
||||
it('TRIM-MIG-009 — renames dirty email as <local>__migrated_<id>@<domain> on collision', () => {
|
||||
const db = makeDb();
|
||||
insert(db, 'frank', 'frank@example.com');
|
||||
const dirtyId = insert(db, 'frank2', ' frank@example.com ');
|
||||
trimUserWhitespace(db);
|
||||
expect(row(db, dirtyId).email).toBe(`frank__migrated_${dirtyId}@example.com`);
|
||||
});
|
||||
|
||||
it('TRIM-MIG-010 — emits a WHITESPACE COLLISION warning for email collision', () => {
|
||||
const db = makeDb();
|
||||
insert(db, 'grace', 'grace@example.com');
|
||||
insert(db, 'grace2', 'grace@example.com ');
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
|
||||
trimUserWhitespace(db);
|
||||
expect(warn).toHaveBeenCalledWith(expect.stringContaining('WHITESPACE COLLISION email'));
|
||||
warn.mockRestore();
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user