fix: enforce unique usernames (#1628)

* fix: enforce unique usernames

Registration checked for a taken name, hashed the password, then
inserted, with nothing in the schema enforcing uniqueness. Concurrent
sign-ups could create two accounts with one name, after which login,
password reset, proxy login and admin lookups by username picked
between them arbitrarily.

Add a unique index on users.username for every dialect. Existing
duplicates are renamed (later rows get a -<id> suffix, the oldest keeps
the name) rather than deleted. Registration and admin user creation now
return the same 409 when the insert loses the race.

* fix: avoid username collisions during migration

src/backend/database/db/index.ts

@@ -925,6 +925,38 @@ const dropRetiredColumns = () => {
   }
 };
 
+// Registration checked for a taken username and then inserted with nothing
+// enforcing it, so concurrent sign-ups could create two accounts with one
+// name, and every lookup by username picked between them arbitrarily. Later
+// duplicates are renamed rather than deleted, so no account is lost.
+const enforceUniqueUsernames = () => {
+  try {
+    sqlite.exec(`
+      WITH ranked AS MATERIALIZED (
+        SELECT id, username,
+          ROW_NUMBER() OVER (ORDER BY registered_at, id) AS sequence,
+          ROW_NUMBER() OVER (PARTITION BY username ORDER BY registered_at, id) AS occurrence
+        FROM users
+      ), candidates AS (
+        SELECT 0 AS candidate UNION ALL SELECT sequence FROM ranked
+      ), available AS (
+        SELECT MIN(candidate) AS candidate FROM candidates
+        WHERE NOT EXISTS (
+          SELECT 1 FROM ranked WHERE username LIKE 'termix-recovered-' || candidate || '-' || '%'
+        )
+      )
+      UPDATE users SET username = 'termix-recovered-' || (SELECT candidate FROM available) || '-' || ranked.sequence
+      FROM ranked WHERE ranked.id = users.id AND ranked.occurrence > 1;
+      CREATE UNIQUE INDEX IF NOT EXISTS idx_users_username ON users (username);
+    `);
+  } catch (error) {
+    databaseLogger.warn("Failed to enforce unique usernames", {
+      operation: "schema_migration",
+      error: getErrorMessage(error),
+    });
+  }
+};
+
 const migrateSchema = () => {
   repairSnippetsColumns(sqlite);
   addColumnIfNotExists("user_preferences", "theme", "TEXT");
@@ -996,6 +1028,7 @@ const migrateSchema = () => {
       );
     }
   }
+  enforceUniqueUsernames();
   addColumnIfNotExists("sessions", "external_session_ref", "TEXT");
 
   addColumnIfNotExists("ssh_data", "name", "TEXT");

src/backend/database/db/schema.mysql.ts

@@ -21,13 +21,19 @@ import {
 } from "drizzle-orm/mysql-core";
 import { sql } from "drizzle-orm";
 
-export const users = mysqlTable("users", {
-  id: varchar("id", { length: 255 }).primaryKey(),
-  username: text("username").notNull(),
-  passwordHash: text("password_hash").notNull(),
-  isAdmin: boolean("is_admin").notNull().default(false),
-  registeredAt: text("registered_at").notNull().default(sql`(CURRENT_TIMESTAMP)`),
-});
+export const users = mysqlTable(
+  "users",
+  {
+    id: varchar("id", { length: 255 }).primaryKey(),
+    username: varchar("username", { length: 255 }).notNull(),
+    passwordHash: text("password_hash").notNull(),
+    isAdmin: boolean("is_admin").notNull().default(false),
+    registeredAt: text("registered_at")
+      .notNull()
+      .default(sql`(CURRENT_TIMESTAMP)`),
+  },
+  (table) => [uniqueIndex("idx_users_username").on(table.username)],
+);
 
 export const settings = mysqlTable("settings", {
   key: varchar("key", { length: 255 }).primaryKey(),

src/backend/database/db/schema.pg.ts

@@ -21,13 +21,19 @@ import {
 } from "drizzle-orm/pg-core";
 import { sql } from "drizzle-orm";
 
-export const users = pgTable("users", {
-  id: varchar("id", { length: 255 }).primaryKey(),
-  username: text("username").notNull(),
-  passwordHash: text("password_hash").notNull(),
-  isAdmin: boolean("is_admin").notNull().default(false),
-  registeredAt: text("registered_at").notNull().default(sql`CURRENT_TIMESTAMP`),
-});
+export const users = pgTable(
+  "users",
+  {
+    id: varchar("id", { length: 255 }).primaryKey(),
+    username: varchar("username", { length: 255 }).notNull(),
+    passwordHash: text("password_hash").notNull(),
+    isAdmin: boolean("is_admin").notNull().default(false),
+    registeredAt: text("registered_at")
+      .notNull()
+      .default(sql`CURRENT_TIMESTAMP`),
+  },
+  (table) => [uniqueIndex("idx_users_username").on(table.username)],
+);
 
 export const settings = pgTable("settings", {
   key: varchar("key", { length: 255 }).primaryKey(),

src/backend/database/db/schema.ts

@@ -9,13 +9,19 @@ import {
 } from "drizzle-orm/sqlite-core";
 import { sql } from "drizzle-orm";
 
-export const users = sqliteTable("users", {
-  id: text("id").primaryKey(),
-  username: text("username").notNull(),
-  passwordHash: text("password_hash").notNull(),
-  isAdmin: integer("is_admin", { mode: "boolean" }).notNull().default(false),
-  registeredAt: text("registered_at").notNull().default(sql`CURRENT_TIMESTAMP`),
-});
+export const users = sqliteTable(
+  "users",
+  {
+    id: text("id").primaryKey(),
+    username: text("username").notNull(),
+    passwordHash: text("password_hash").notNull(),
+    isAdmin: integer("is_admin", { mode: "boolean" }).notNull().default(false),
+    registeredAt: text("registered_at")
+      .notNull()
+      .default(sql`CURRENT_TIMESTAMP`),
+  },
+  (table) => [uniqueIndex("idx_users_username").on(table.username)],
+);
 
 export const settings = sqliteTable("settings", {
   key: text("key").primaryKey(),

src/backend/database/repositories/user-repository.ts

@@ -24,6 +24,27 @@ const userColumns = {
     ),
 };
 
+// The unique-violation codes of better-sqlite3, node-postgres and mysql2.
+const UNIQUE_VIOLATION_CODES = new Set([
+  "SQLITE_CONSTRAINT_UNIQUE",
+  "23505",
+  "ER_DUP_ENTRY",
+]);
+
+/**
+ * Whether creating a user failed because the username is taken. The id is
+ * generated, so the username index is the only one a new row can collide on.
+ * drizzle wraps driver errors, so the code may sit on a cause.
+ */
+export function isUsernameTaken(error: unknown): boolean {
+  for (let e = error; e instanceof Error; e = e.cause) {
+    if (UNIQUE_VIOLATION_CODES.has((e as { code?: string }).code ?? "")) {
+      return true;
+    }
+  }
+  return false;
+}
+
 export class UserRepository {
   constructor(
     private readonly context: DatabaseContext,

src/backend/database/routes/user-admin-routes.ts

@@ -17,9 +17,10 @@ import {
   listUserSecondFactors,
   resetUserSecondFactors,
 } from "../../auth/second-factor-admin.js";
-import type {
-  UserRecord,
-  UserRepository,
+import {
+  isUsernameTaken,
+  type UserRecord,
+  type UserRepository,
 } from "../repositories/user-repository.js";
 
 function isNonEmptyString(val: unknown): val is string {
@@ -541,6 +542,9 @@ export function registerUserAdminRoutes(
         toast: { type: "success", message: `User created: ${username}` },
       });
     } catch (err) {
+      if (isUsernameTaken(err)) {
+        return res.status(409).json({ error: "Username already exists" });
+      }
       authLogger.error("Failed to admin-create user", err);
       res.status(500).json({ error: "Failed to create user" });
     }

src/backend/database/routes/users.ts

@@ -41,7 +41,10 @@ import {
   createCurrentUserAuthRepository,
   createCurrentUserRepository,
 } from "../repositories/factory.js";
-import type { UserRecord } from "../repositories/user-repository.js";
+import {
+  isUsernameTaken,
+  type UserRecord,
+} from "../repositories/user-repository.js";
 import {
   getTrustedProxyAuthConfig,
   getTrustedProxySourceAddress,
@@ -265,6 +268,10 @@ router.post("/create", async (req, res) => {
       toast: { type: "success", message: `User created: ${username}` },
     });
   } catch (err) {
+    // Lost a race with a concurrent sign-up for the same name.
+    if (isUsernameTaken(err)) {
+      return res.status(409).json({ error: "Username already exists" });
+    }
     authLogger.error("Failed to create user", err);
     res.status(500).json({ error: "Failed to create user" });
   }

src/backend/tests/database/db/unique-username.test.ts

@@ -0,0 +1,109 @@
+import fs from "fs";
+import os from "os";
+import path from "path";
+import Database from "better-sqlite3";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+
+/**
+ * Registration checked for a taken username before inserting, with nothing in
+ * the schema to back it up, so two concurrent sign-ups could both get the same
+ * name. Startup renames the later duplicates instead of deleting anyone, then
+ * makes the name unique.
+ */
+describe("users.username uniqueness", () => {
+  let dataDir: string;
+
+  beforeEach(() => {
+    dataDir = fs.mkdtempSync(path.join(os.tmpdir(), "termix-unique-user-"));
+    vi.resetModules();
+    process.env.DATA_DIR = dataDir;
+    process.env.DB_FILE_ENCRYPTION = "false";
+    process.env.ALLOW_EMPTY_DATA_DIR = "true";
+  });
+
+  afterEach(() => {
+    delete process.env.DATA_DIR;
+    delete process.env.DB_FILE_ENCRYPTION;
+    delete process.env.ALLOW_EMPTY_DATA_DIR;
+    fs.rmSync(dataDir, { recursive: true, force: true });
+  });
+
+  function writeDatabaseWithDuplicates(): void {
+    const seed = new Database(":memory:");
+    seed.exec(`
+      CREATE TABLE users (
+        id TEXT PRIMARY KEY,
+        username TEXT NOT NULL,
+        password_hash TEXT NOT NULL,
+        is_admin INTEGER NOT NULL DEFAULT 0,
+        registered_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP
+      );
+
+      INSERT INTO users (id, username, password_hash, registered_at) VALUES
+        ('late', 'alice', 'hash', '2025-03-01 00:00:00'),
+        ('first', 'alice', 'hash', '2025-01-01 00:00:00'),
+        ('tie-b', 'bob', 'hash', '2025-01-01 00:00:00'),
+        ('tie-a', 'bob', 'hash', '2025-01-01 00:00:00'),
+        ('only', 'Alice', 'hash', '2025-02-01 00:00:00');
+    `);
+    fs.writeFileSync(path.join(dataDir, "db.sqlite"), seed.serialize());
+    seed.close();
+  }
+
+  async function bootAndGetSqlite(): Promise<Database.Database> {
+    const db = await import("../../../database/db/index.js");
+    await db.initializeDatabase();
+    return db.getSqlite();
+  }
+
+  it("keeps the oldest holder of a name and renames the rest", async () => {
+    writeDatabaseWithDuplicates();
+
+    const sqlite = await bootAndGetSqlite();
+
+    const names = Object.fromEntries(
+      sqlite
+        .prepare("SELECT id, username FROM users")
+        .raw()
+        .all() as [string, string][],
+    );
+    expect(names).toEqual({
+      first: "alice",
+      late: "termix-recovered-0-5",
+      "tie-a": "bob",
+      "tie-b": "termix-recovered-0-3",
+      only: "Alice",
+    });
+  });
+
+  it("rejects a second user with a taken name", async () => {
+    const sqlite = await bootAndGetSqlite();
+
+    const insert = sqlite.prepare(
+      "INSERT INTO users (id, username, password_hash) VALUES (?, ?, ?)",
+    );
+    insert.run("u1", "alice", "hash");
+
+    expect(() => insert.run("u2", "alice", "hash")).toThrow(/UNIQUE/i);
+  });
+});
+
+// Run the checked-in migration itself, including occupied recovery prefixes.
+it("migrates duplicate names without colliding with existing names", () => {
+  const db = new Database(":memory:");
+  try {
+    db.exec(`CREATE TABLE users (id TEXT PRIMARY KEY, username TEXT NOT NULL, registered_at TEXT NOT NULL);
+      INSERT INTO users VALUES
+        ('first', 'alice', '2025-01-01'),
+        ('late', 'alice', '2025-02-01'),
+        ('other', 'alice-late', '2025-01-01'),
+        ('occupied', 'termix-recovered-0-1', '2025-01-01');`);
+    db.exec(fs.readFileSync("drizzle/sqlite/0067_unique_username.sql", "utf8"));
+    const rows = db.prepare("SELECT username FROM users").pluck().all();
+    expect(new Set(rows).size).toBe(4);
+    expect(rows).toContain("alice");
+    expect(rows).toContain("alice-late");
+    expect(rows).toContain("termix-recovered-0-1");
+    expect(() => db.exec("INSERT INTO users VALUES ('new', 'alice', '2026-01-01')")).toThrow(/UNIQUE/i);
+  } finally { db.close(); }
+});

src/backend/tests/database/repositories/unique-username-migration.test.ts

@@ -0,0 +1,44 @@
+import fs from "node:fs";
+import { sql } from "drizzle-orm";
+import { expect, it } from "vitest";
+import { TestSqliteDatabase, testDialect } from "./test-support.js";
+
+it("migrates duplicate usernames without colliding with occupied recovery names", async () => {
+  const adapter = new TestSqliteDatabase();
+  await adapter.connect();
+  try {
+    await adapter.exec(`CREATE TABLE username_migration_fixture (
+      id varchar(255) PRIMARY KEY, username TEXT NOT NULL, registered_at varchar(255) NOT NULL
+    );
+    INSERT INTO username_migration_fixture VALUES
+      ('first', 'alice', '2025-01-01'),
+      ('late', 'alice', '2025-02-01'),
+      ('other', 'alice-late', '2025-01-01'),
+      ('occupied', 'termix-recovered-0-1', '2025-01-01');`);
+    const dialect = testDialect();
+    const number = dialect === "sqlite" ? "0067" : "0072";
+    const migration = fs
+      .readFileSync(`drizzle/${dialect}/${number}_unique_username.sql`, "utf8")
+      .replace(/\busers\b/g, "username_migration_fixture")
+      .replaceAll("idx_users_username", "idx_fixture_username");
+    for (const statement of migration.split("--> statement-breakpoint")) {
+      if (statement.trim()) await adapter.run(sql.raw(statement));
+    }
+    const rows = await adapter.query<{ username: string }>(
+      sql`SELECT username FROM username_migration_fixture`,
+    );
+    const names = rows.map((row) => row.username);
+    expect(new Set(names).size).toBe(4);
+    expect(names).toEqual(
+      expect.arrayContaining(["alice", "alice-late", "termix-recovered-0-1"]),
+    );
+    await expect(async () => {
+      await adapter.run(
+        sql`INSERT INTO username_migration_fixture VALUES ('new', 'alice', '2026-01-01')`,
+      );
+    }).rejects.toThrow();
+  } finally {
+    await adapter.exec("DROP TABLE IF EXISTS username_migration_fixture");
+    await adapter.close();
+  }
+});

src/backend/tests/database/repositories/user-repository.test.ts

@@ -1,6 +1,9 @@
 import { afterEach, describe, expect, it } from "vitest";
 import { TestSqliteDatabase } from "./test-support.js";
-import { UserRepository } from "../../../database/repositories/user-repository.js";
+import {
+  UserRepository,
+  isUsernameTaken,
+} from "../../../database/repositories/user-repository.js";
 
 describe("UserRepository.listPage", () => {
   let adapter: TestSqliteDatabase | null = null;
@@ -93,3 +96,27 @@ describe("UserRepository.listPage", () => {
     expect(page.total).toBe(5);
   });
 });
+
+describe("UserRepository.create", () => {
+  let adapter: TestSqliteDatabase | null = null;
+
+  afterEach(async () => {
+    if (adapter) {
+      await adapter.close();
+      adapter = null;
+    }
+  });
+
+  it("rejects a taken username in a way callers can recognise", async () => {
+    adapter = new TestSqliteDatabase();
+    const repo = new UserRepository(await adapter.connect());
+    await repo.create({ id: "u1", username: "alice", passwordHash: "hash" });
+
+    const error = await repo
+      .create({ id: "u2", username: "alice", passwordHash: "hash" })
+      .catch((e: unknown) => e);
+
+    expect(isUsernameTaken(error)).toBe(true);
+    expect(isUsernameTaken(new Error("unrelated"))).toBe(false);
+  });
+});