From 0e76511eee0d6d771c4ab02cd292fa72df2a9fc0 Mon Sep 17 00:00:00 2001 From: Dan Date: Sat, 13 Jun 2026 20:47:55 +0100 Subject: [PATCH] fix(server): detect Postgres unique violations across driver wrapping Add a real-Postgres integration tier for the Drizzle stores and fix a bug it immediately surfaced. Bug: isUniqueViolation only checked the top-level error's `code === "23505"`, but Drizzle wraps the driver error in `.cause` and Bun's SQL driver exposes the SQLSTATE as `errno` (with `code` set to the generic ERR_POSTGRES_SERVER_ERROR). So a duplicate username threw a raw error that surfaced as 503 AUTH_UNAVAILABLE instead of a clean 409 USERNAME_TAKEN. Now walk the cause chain and check code, errno, and the message at each level. The fake-double unit tests could not catch this because they never hit a real driver. Tests / CI: - apps/server/src/db/integration.test.ts exercises DrizzleAuthStore, DrizzleFriendStore, and DrizzlePersistenceStore against real Postgres: column mappings, the username unique constraint, token-version revocation, canonical friendships listed both directions, the no-self-friend CHECK, and room/session persistence. Opt-in via RUN_DB_TESTS=1 so the default `bun test` stays database-free. - ci.yml: add a postgres:16 service, RUN_DB_TESTS=1, and a db:migrate step before the test run (which also validates migrations, including 0008). - Add a test:integration script. Co-Authored-By: Claude Opus 4.8 (1M context) --- .github/workflows/ci.yml | 20 ++++ apps/server/src/auth/auth.ts | 33 +++++-- apps/server/src/db/integration.test.ts | 129 +++++++++++++++++++++++++ package.json | 1 + 4 files changed, 176 insertions(+), 7 deletions(-) create mode 100644 apps/server/src/db/integration.test.ts diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9446a31..d2bd0c9 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -14,6 +14,23 @@ jobs: runs-on: ubuntu-latest env: COVERAGE_THRESHOLD: "80" + DATABASE_URL: postgres://postgres:postgres@localhost:5432/tilezo + RUN_DB_TESTS: "1" + + services: + postgres: + image: postgres:16-alpine + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: tilezo + ports: + - 5432:5432 + options: >- + --health-cmd pg_isready + --health-interval 10s + --health-timeout 5s + --health-retries 5 steps: - name: Checkout @@ -33,6 +50,9 @@ jobs: - name: Typecheck run: bun run typecheck + - name: Run database migrations + run: bun run db:migrate + - name: Test with coverage run: bun run test:coverage diff --git a/apps/server/src/auth/auth.ts b/apps/server/src/auth/auth.ts index 0d15a81..4022881 100644 --- a/apps/server/src/auth/auth.ts +++ b/apps/server/src/auth/auth.ts @@ -443,17 +443,36 @@ export class DrizzleAuthStore implements AuthStore { } } +// Detects a Postgres unique-violation (SQLSTATE 23505) across driver shapes. Drizzle wraps +// the driver error in `.cause`, and Bun's SQL driver exposes the SQLSTATE as `errno` while +// `code` is the generic "ERR_POSTGRES_SERVER_ERROR" — so we walk the cause chain and check +// code, errno, and the message at each level. function isUniqueViolation(error: unknown): boolean { - if (!error || typeof error !== "object") { - return false; - } + let current: unknown = error; + + for (let depth = 0; current && typeof current === "object" && depth < 5; depth += 1) { + const candidate = current as { + code?: unknown; + errno?: unknown; + message?: unknown; + cause?: unknown; + }; + + if (candidate.code === "23505" || candidate.errno === "23505") { + return true; + } + + if ( + typeof candidate.message === "string" && + /unique constraint|duplicate key/i.test(candidate.message) + ) { + return true; + } - if ((error as { code?: unknown }).code === "23505") { - return true; + current = candidate.cause; } - const message = (error as { message?: unknown }).message; - return typeof message === "string" && /unique|duplicate key/i.test(message); + return false; } export class AuthError extends Error { diff --git a/apps/server/src/db/integration.test.ts b/apps/server/src/db/integration.test.ts new file mode 100644 index 0000000..99f41f7 --- /dev/null +++ b/apps/server/src/db/integration.test.ts @@ -0,0 +1,129 @@ +import { beforeEach, describe, expect, test } from "bun:test"; +import { createRectRoomLayout } from "@tilezo/engine"; +import { DEFAULT_AVATAR_APPEARANCE } from "@tilezo/protocol"; +import { sql } from "drizzle-orm"; +import { DrizzleAuthStore, UsernameTakenError } from "../auth/auth"; +import { DrizzleFriendStore } from "../friends/friends"; +import { createDatabase } from "./db"; +import { DrizzlePersistenceStore } from "./persistence"; + +// Real-Postgres integration coverage for the Drizzle stores: exercises the actual column +// mappings, unique/check constraints, onConflict targets, and the bidirectional friend +// join that the in-memory/fake-double unit tests cannot. Opt-in via RUN_DB_TESTS=1 against +// a migrated Postgres (CI sets it and provides the service); the default `bun test` run +// skips it so it never needs a database — even though `.env` defines DATABASE_URL. +const dbTestsEnabled = process.env.RUN_DB_TESTS === "1" && Boolean(process.env.DATABASE_URL); +const db = dbTestsEnabled ? createDatabase(process.env.DATABASE_URL) : undefined; + +describe("database integration", () => { + const database = db; + + if (!database) { + test.skip("requires RUN_DB_TESTS=1 and a migrated Postgres", () => {}); + return; + } + + const authStore = new DrizzleAuthStore(database); + const friendStore = new DrizzleFriendStore(database); + const persistence = new DrizzlePersistenceStore(database); + + beforeEach(async () => { + await database.execute( + sql`TRUNCATE TABLE users, rooms, friendships, user_room_sessions, room_items RESTART IDENTITY CASCADE`, + ); + }); + + function seedUser(username: string) { + return authStore.createUser({ + appearance: DEFAULT_AVATAR_APPEARANCE, + username, + usernameKey: username.toLowerCase(), + passwordHash: `hash-${username}`, + }); + } + + test("round-trips users and enforces uniqueness and token revocation", async () => { + const user = await seedUser("Dan"); + expect(user.tokenVersion).toBe(0); + + expect(await authStore.findUserByUsernameKey("dan")).toMatchObject({ + id: user.id, + username: "Dan", + }); + expect(await authStore.findUserById(user.id)).toMatchObject({ id: user.id }); + + const updated = await authStore.updateUserAppearance(user.id, { + ...DEFAULT_AVATAR_APPEARANCE, + hair: "bob", + }); + expect(updated?.appearance.hair).toBe("bob"); + + await authStore.incrementTokenVersion(user.id); + expect((await authStore.findUserById(user.id))?.tokenVersion).toBe(1); + + // The DB unique constraint on username_key drives USERNAME_TAKEN, not a TOCTOU check. + await expect( + authStore.createUser({ + appearance: DEFAULT_AVATAR_APPEARANCE, + username: "DAN", + usernameKey: "dan", + passwordHash: "hash", + }), + ).rejects.toBeInstanceOf(UsernameTakenError); + }); + + test("stores canonical friendships and lists them from either direction", async () => { + const dan = await seedUser("Dan"); + const kai = await seedUser("Kai"); + + await friendStore.addFriend(dan.id, kai.id); + // Adding the reverse pair is idempotent (canonical ordering + onConflictDoNothing). + await friendStore.addFriend(kai.id, dan.id); + + expect(await friendStore.countFriends(dan.id)).toBe(1); + expect((await friendStore.listFriends(dan.id)).map((friend) => friend.id)).toEqual([kai.id]); + expect((await friendStore.listFriends(kai.id)).map((friend) => friend.id)).toEqual([dan.id]); + + await friendStore.removeFriend(kai.id, dan.id); + expect(await friendStore.countFriends(dan.id)).toBe(0); + }); + + test("rejects self-friendship at the database level", async () => { + const dan = await seedUser("Dan"); + await expect(friendStore.addFriend(dan.id, dan.id)).rejects.toThrow(); + }); + + test("seeds rooms, lists by visibility and owner, and tracks the last room", async () => { + const owner = await seedUser("Dan"); + const publicLayout = createRectRoomLayout("lobby", "Lobby", 3, 3, { x: 1, y: 1 }); + const privateLayout = createRectRoomLayout("home_dan", "Dan's Room", 4, 4, { x: 1, y: 1 }); + + await persistence.seedRoom(publicLayout); + await persistence.seedRoom(privateLayout, { + ownerUserId: owner.id, + visibility: "private", + access: "knock", + description: "cozy", + capacity: 10, + }); + + expect(await persistence.getRoom("lobby")).toMatchObject({ id: "lobby" }); + expect((await persistence.listPublicRooms()).map((layout) => layout.id)).toEqual(["lobby"]); + expect((await persistence.listOwnedRooms(owner.id)).map((room) => room.layout.id)).toEqual([ + "home_dan", + ]); + + const stored = await persistence.listRooms(); + expect(stored.find((room) => room.layout.id === "home_dan")).toMatchObject({ + visibility: "private", + access: "knock", + description: "cozy", + capacity: 10, + }); + + await persistence.saveLastRoomIdForUser(owner.id, "lobby"); + expect(await persistence.getLastRoomIdForUser(owner.id)).toBe("lobby"); + await persistence.clearLastRoomIdForUser(owner.id); + expect(await persistence.getLastRoomIdForUser(owner.id)).toBeUndefined(); + }); +}); diff --git a/package.json b/package.json index ebf6c64..bae9bfd 100644 --- a/package.json +++ b/package.json @@ -26,6 +26,7 @@ "stress:auth": "bun run scripts/stress-test.ts --scenario auth", "stress:room": "bun run scripts/stress-test.ts --scenario full --preseed-users", "test": "bun test", + "test:integration": "RUN_DB_TESTS=1 bun test apps/server/src/db/integration.test.ts", "test:coverage": "bun test --coverage --coverage-reporter=text --coverage-reporter=lcov --coverage-dir=coverage", "coverage:check": "bun run scripts/check-coverage.ts", "typecheck": "bun run --filter '*' typecheck",