fix(gateway): serialize bootstrap setup with an advisory lock (#1430) #1431
@@ -8,7 +8,7 @@ import {
|
|||||||
Post,
|
Post,
|
||||||
} from '@nestjs/common';
|
} from '@nestjs/common';
|
||||||
import { randomBytes, createHash } from 'node:crypto';
|
import { randomBytes, createHash } from 'node:crypto';
|
||||||
import { count, eq, type Db, users as usersTable, adminTokens } from '@mosaicstack/db';
|
import { count, eq, sql, type Db, users as usersTable, adminTokens } from '@mosaicstack/db';
|
||||||
import type { Auth } from '@mosaicstack/auth';
|
import type { Auth } from '@mosaicstack/auth';
|
||||||
import { v4 as uuid } from 'uuid';
|
import { v4 as uuid } from 'uuid';
|
||||||
import { AUTH } from '../auth/auth.tokens.js';
|
import { AUTH } from '../auth/auth.tokens.js';
|
||||||
@@ -16,6 +16,12 @@ import { DB } from '../database/database.module.js';
|
|||||||
import { BootstrapSetupDto } from './bootstrap.dto.js';
|
import { BootstrapSetupDto } from './bootstrap.dto.js';
|
||||||
import type { BootstrapStatusDto, BootstrapResultDto } from './bootstrap.dto.js';
|
import type { BootstrapStatusDto, BootstrapResultDto } from './bootstrap.dto.js';
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Advisory lock key serializing bootstrap setup. Arbitrary constant; must only
|
||||||
|
* be unique among advisory lock keys used against this database.
|
||||||
|
*/
|
||||||
|
export const BOOTSTRAP_SETUP_LOCK_KEY = 0x626f6f74; // 'boot'
|
||||||
|
|
||||||
@Controller('api/bootstrap')
|
@Controller('api/bootstrap')
|
||||||
export class BootstrapController {
|
export class BootstrapController {
|
||||||
constructor(
|
constructor(
|
||||||
@@ -31,13 +37,23 @@ export class BootstrapController {
|
|||||||
|
|
||||||
@Post('setup')
|
@Post('setup')
|
||||||
async setup(@Body() dto: BootstrapSetupDto): Promise<BootstrapResultDto> {
|
async setup(@Body() dto: BootstrapSetupDto): Promise<BootstrapResultDto> {
|
||||||
|
// #1430: the zero-user check and the admin creation must be one critical
|
||||||
|
// section, or two concurrent setup calls can each create an admin. The
|
||||||
|
// transaction-scoped advisory lock serializes setup across all gateway
|
||||||
|
// instances sharing this database; the second caller blocks on the lock,
|
||||||
|
// then re-reads the count and gets 403.
|
||||||
|
return this.db.transaction(async (tx) => {
|
||||||
|
await tx.execute(sql`select pg_advisory_xact_lock(${BOOTSTRAP_SETUP_LOCK_KEY})`);
|
||||||
|
|
||||||
// Only allow setup when zero users exist
|
// Only allow setup when zero users exist
|
||||||
const [result] = await this.db.select({ total: count() }).from(usersTable);
|
const [result] = await tx.select({ total: count() }).from(usersTable);
|
||||||
if ((result?.total ?? 0) > 0) {
|
if ((result?.total ?? 0) > 0) {
|
||||||
throw new ForbiddenException('Setup already completed — users exist');
|
throw new ForbiddenException('Setup already completed — users exist');
|
||||||
}
|
}
|
||||||
|
|
||||||
// Create admin user via BetterAuth API
|
// Create admin user via BetterAuth API. BetterAuth writes on its own
|
||||||
|
// connection and commits independently of this transaction; the reads
|
||||||
|
// below still see the committed row (READ COMMITTED statement snapshot).
|
||||||
const authApi = this.auth.api as unknown as {
|
const authApi = this.auth.api as unknown as {
|
||||||
createUser: (opts: {
|
createUser: (opts: {
|
||||||
body: { name: string; email: string; password: string; role?: string };
|
body: { name: string; email: string; password: string; role?: string };
|
||||||
@@ -56,7 +72,7 @@ export class BootstrapController {
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Verify user was created
|
// Verify user was created
|
||||||
const [user] = await this.db
|
const [user] = await tx
|
||||||
.select()
|
.select()
|
||||||
.from(usersTable)
|
.from(usersTable)
|
||||||
.where(eq(usersTable.id, created.user.id))
|
.where(eq(usersTable.id, created.user.id))
|
||||||
@@ -66,7 +82,7 @@ export class BootstrapController {
|
|||||||
|
|
||||||
// Ensure role is admin (createUser may not set it via BetterAuth)
|
// Ensure role is admin (createUser may not set it via BetterAuth)
|
||||||
if (user.role !== 'admin') {
|
if (user.role !== 'admin') {
|
||||||
await this.db.update(usersTable).set({ role: 'admin' }).where(eq(usersTable.id, user.id));
|
await tx.update(usersTable).set({ role: 'admin' }).where(eq(usersTable.id, user.id));
|
||||||
}
|
}
|
||||||
|
|
||||||
// Generate admin API token
|
// Generate admin API token
|
||||||
@@ -74,7 +90,7 @@ export class BootstrapController {
|
|||||||
const tokenHash = createHash('sha256').update(plaintext).digest('hex');
|
const tokenHash = createHash('sha256').update(plaintext).digest('hex');
|
||||||
const tokenId = uuid();
|
const tokenId = uuid();
|
||||||
|
|
||||||
const [token] = await this.db
|
const [token] = await tx
|
||||||
.insert(adminTokens)
|
.insert(adminTokens)
|
||||||
.values({
|
.values({
|
||||||
id: tokenId,
|
id: tokenId,
|
||||||
@@ -98,5 +114,6 @@ export class BootstrapController {
|
|||||||
label: token!.label,
|
label: token!.label,
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
});
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -20,7 +20,7 @@
|
|||||||
*/
|
*/
|
||||||
|
|
||||||
import 'reflect-metadata';
|
import 'reflect-metadata';
|
||||||
import { describe, it, expect, afterAll, beforeAll } from 'vitest';
|
import { describe, it, expect, afterAll, beforeAll, vi } from 'vitest';
|
||||||
import { Test } from '@nestjs/testing';
|
import { Test } from '@nestjs/testing';
|
||||||
import { ValidationPipe, type INestApplication } from '@nestjs/common';
|
import { ValidationPipe, type INestApplication } from '@nestjs/common';
|
||||||
import { FastifyAdapter, type NestFastifyApplication } from '@nestjs/platform-fastify';
|
import { FastifyAdapter, type NestFastifyApplication } from '@nestjs/platform-fastify';
|
||||||
@@ -52,13 +52,22 @@ const mockAuth = {
|
|||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
// Override db.select() so the second query (verify user exists) returns a user.
|
// The controller runs setup inside db.transaction(tx) and first takes the
|
||||||
// The bootstrap controller calls select().from() twice:
|
// #1430 advisory lock via tx.execute(). Inside the transaction it calls
|
||||||
|
// select().from() twice:
|
||||||
// 1. count() to check zero users → returns [{total: 0}]
|
// 1. count() to check zero users → returns [{total: 0}]
|
||||||
// 2. select().where().limit() → returns [the created user]
|
// 2. select().where().limit() → returns [the created user]
|
||||||
|
// callLog records the tx call order so the lock-before-check invariant is
|
||||||
|
// testable.
|
||||||
let selectCallCount = 0;
|
let selectCallCount = 0;
|
||||||
const mockDbWithUser = {
|
const callLog: string[] = [];
|
||||||
|
const mockTx = {
|
||||||
|
execute: () => {
|
||||||
|
callLog.push('execute');
|
||||||
|
return Promise.resolve([]);
|
||||||
|
},
|
||||||
select: () => {
|
select: () => {
|
||||||
|
callLog.push('select');
|
||||||
selectCallCount++;
|
selectCallCount++;
|
||||||
return {
|
return {
|
||||||
from: () => {
|
from: () => {
|
||||||
@@ -100,6 +109,9 @@ const mockDbWithUser = {
|
|||||||
}),
|
}),
|
||||||
}),
|
}),
|
||||||
};
|
};
|
||||||
|
const mockDbWithUser = {
|
||||||
|
transaction: <T>(cb: (tx: typeof mockTx) => Promise<T>) => cb(mockTx),
|
||||||
|
};
|
||||||
|
|
||||||
// ─── Test suite ───────────────────────────────────────────────────────────────
|
// ─── Test suite ───────────────────────────────────────────────────────────────
|
||||||
|
|
||||||
@@ -153,6 +165,11 @@ describe('POST /api/bootstrap/setup — ValidationPipe DTO binding', () => {
|
|||||||
expect(body.user.email).toBe('[email protected]');
|
expect(body.user.email).toBe('[email protected]');
|
||||||
expect(body.token).toBeDefined();
|
expect(body.token).toBeDefined();
|
||||||
expect(body.token.plaintext).toBeDefined();
|
expect(body.token.plaintext).toBeDefined();
|
||||||
|
|
||||||
|
// #1430: the advisory lock must be taken before the zero-user count, or
|
||||||
|
// two concurrent setups can both pass the check.
|
||||||
|
expect(callLog[0]).toBe('execute');
|
||||||
|
expect(callLog[1]).toBe('select');
|
||||||
});
|
});
|
||||||
|
|
||||||
it('returns 400 when extra forbidden properties are sent', async () => {
|
it('returns 400 when extra forbidden properties are sent', async () => {
|
||||||
@@ -188,3 +205,55 @@ describe('POST /api/bootstrap/setup — ValidationPipe DTO binding', () => {
|
|||||||
expect(res.status).toBe(400);
|
expect(res.status).toBe(400);
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ─── #1430 regression: users-exist check runs inside the locked transaction ──
|
||||||
|
|
||||||
|
describe('POST /api/bootstrap/setup — setup already completed', () => {
|
||||||
|
let app: INestApplication;
|
||||||
|
const createUserSpy = vi.fn();
|
||||||
|
const lockedTx = {
|
||||||
|
execute: () => Promise.resolve([]),
|
||||||
|
select: () => ({
|
||||||
|
// count() sees an existing user — the locked re-check must reject.
|
||||||
|
from: () => Promise.resolve([{ total: 1 }]),
|
||||||
|
}),
|
||||||
|
};
|
||||||
|
const mockDbUsersExist = {
|
||||||
|
transaction: <T>(cb: (tx: typeof lockedTx) => Promise<T>) => cb(lockedTx),
|
||||||
|
};
|
||||||
|
|
||||||
|
beforeAll(async () => {
|
||||||
|
const moduleRef = await Test.createTestingModule({
|
||||||
|
controllers: [BootstrapController],
|
||||||
|
providers: [
|
||||||
|
{ provide: AUTH, useValue: { api: { createUser: createUserSpy } } },
|
||||||
|
{ provide: DB, useValue: mockDbUsersExist },
|
||||||
|
],
|
||||||
|
}).compile();
|
||||||
|
|
||||||
|
app = moduleRef.createNestApplication<NestFastifyApplication>(new FastifyAdapter());
|
||||||
|
app.useGlobalPipes(
|
||||||
|
new ValidationPipe({
|
||||||
|
whitelist: true,
|
||||||
|
forbidNonWhitelisted: true,
|
||||||
|
transform: true,
|
||||||
|
}),
|
||||||
|
);
|
||||||
|
await app.init();
|
||||||
|
await app.getHttpAdapter().getInstance().ready();
|
||||||
|
});
|
||||||
|
|
||||||
|
afterAll(async () => {
|
||||||
|
await app.close();
|
||||||
|
});
|
||||||
|
|
||||||
|
it('returns 403 and never calls createUser when users already exist', async () => {
|
||||||
|
const res = await request(app.getHttpServer())
|
||||||
|
.post('/api/bootstrap/setup')
|
||||||
|
.send({ name: 'Admin', email: '[email protected]', password: 'password123' })
|
||||||
|
.set('Content-Type', 'application/json');
|
||||||
|
|
||||||
|
expect(res.status).toBe(403);
|
||||||
|
expect(createUserSpy).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user