From 3cf9d15ad5a3e68c240529f63adf1483730b7f80 Mon Sep 17 00:00:00 2001 From: Val Alexander Date: Fri, 5 Jun 2026 16:58:30 -0700 Subject: [PATCH 1/2] fix(onboarding): restrict bootstrap admin promotion --- .../__tests__/onboarding-security.test.ts | 199 ++++++++++++++++++ .../src/lib/server/functions/onboarding.ts | 99 ++++++--- 2 files changed, 265 insertions(+), 33 deletions(-) create mode 100644 apps/web/src/lib/server/functions/__tests__/onboarding-security.test.ts diff --git a/apps/web/src/lib/server/functions/__tests__/onboarding-security.test.ts b/apps/web/src/lib/server/functions/__tests__/onboarding-security.test.ts new file mode 100644 index 000000000..978624abf --- /dev/null +++ b/apps/web/src/lib/server/functions/__tests__/onboarding-security.test.ts @@ -0,0 +1,199 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +type AnyHandler = (args: { data: Record }) => Promise + +vi.mock('@tanstack/react-start', () => ({ + createServerFn: () => ({ + inputValidator: () => ({ + handler: (fn: AnyHandler) => fn, + }), + handler: (fn: AnyHandler) => fn, + }), +})) + +const hoisted = vi.hoisted(() => ({ + mockGetSession: vi.fn(), + mockGetSettings: vi.fn(), + mockAssertNotManaged: vi.fn(), + mockInvalidateSettingsCache: vi.fn(), + mockPrincipalFindFirst: vi.fn(), + mockTxExecute: vi.fn(), + mockUpdateSet: vi.fn(), + mockUpdateWhere: vi.fn(), + mockInsertValues: vi.fn(), + mockTransaction: vi.fn(), + mockEq: vi.fn((column: string, value: unknown) => ({ op: 'eq', column, value })), + mockAnd: vi.fn((...conditions: unknown[]) => ({ op: 'and', conditions })), + mockNe: vi.fn((column: string, value: unknown) => ({ op: 'ne', column, value })), + mockGenerateId: vi.fn(), +})) + +vi.mock('@/lib/server/auth/session', () => ({ + getSession: hoisted.mockGetSession, +})) + +vi.mock('../workspace', () => ({ + getSettings: hoisted.mockGetSettings, +})) + +vi.mock('@/lib/server/config-file/managed-guard', () => ({ + assertNotManaged: hoisted.mockAssertNotManaged, +})) + +vi.mock('@/lib/server/domains/settings/settings.helpers', () => ({ + invalidateSettingsCache: hoisted.mockInvalidateSettingsCache, +})) + +vi.mock('@opencoven-feedback/ids', () => ({ + generateId: hoisted.mockGenerateId, +})) + +vi.mock('@/lib/server/domains/principals/principal.service', () => ({ + syncPrincipalProfile: vi.fn(), +})) + +vi.mock('@/lib/server/domains/boards/board.service', () => ({ + listBoards: vi.fn(), +})) + +vi.mock('@/lib/server/domains/settings', () => ({ + DEFAULT_AUTH_CONFIG: {}, + DEFAULT_PORTAL_CONFIG: {}, +})) + +vi.mock('@/lib/server/config-file/managed-paths', () => ({ + isPathManaged: vi.fn(() => false), +})) + +vi.mock('@/lib/server/db', () => ({ + USE_CASE_TYPES: ['saas', 'consumer', 'marketplace', 'internal'], + DEFAULT_STATUSES: [], + settings: { id: 'settings.id' }, + principal: { + id: 'principal.id', + userId: 'principal.userId', + role: 'principal.role', + type: 'principal.type', + }, + user: { id: 'user.id' }, + postStatuses: {}, + eq: hoisted.mockEq, + and: hoisted.mockAnd, + ne: hoisted.mockNe, + sql: (strings: TemplateStringsArray, ...values: unknown[]) => ({ strings, values }), + db: { + query: { + principal: { + findFirst: hoisted.mockPrincipalFindFirst, + }, + postStatuses: { + findFirst: vi.fn(), + }, + }, + update: vi.fn(() => ({ + set: hoisted.mockUpdateSet.mockImplementation(() => ({ + where: hoisted.mockUpdateWhere.mockResolvedValue(undefined), + returning: vi.fn().mockResolvedValue([]), + })), + })), + insert: vi.fn(() => ({ + values: hoisted.mockInsertValues.mockResolvedValue(undefined), + })), + transaction: hoisted.mockTransaction.mockImplementation(async (fn: (tx: unknown) => unknown) => + fn({ + execute: hoisted.mockTxExecute.mockResolvedValue(undefined), + query: { + principal: { + findFirst: hoisted.mockPrincipalFindFirst, + }, + }, + update: vi.fn(() => ({ + set: hoisted.mockUpdateSet.mockImplementation(() => ({ + where: hoisted.mockUpdateWhere.mockResolvedValue(undefined), + })), + })), + insert: vi.fn(() => ({ + values: hoisted.mockInsertValues.mockResolvedValue(undefined), + })), + }) + ), + }, +})) + +const { saveUseCaseFn } = await import('../onboarding') + +const incompleteSettings = { + id: 'workspace_existing', + setupState: JSON.stringify({ + version: 1, + steps: { core: true, workspace: false, boards: false }, + }), +} + +describe('saveUseCaseFn onboarding admin promotion', () => { + beforeEach(() => { + vi.clearAllMocks() + hoisted.mockGetSession.mockResolvedValue({ user: { id: 'user_attacker' } }) + hoisted.mockGetSettings.mockResolvedValue(incompleteSettings) + hoisted.mockAssertNotManaged.mockResolvedValue(undefined) + hoisted.mockInvalidateSettingsCache.mockResolvedValue(undefined) + hoisted.mockGenerateId.mockReturnValue('principal_new_admin') + }) + + it('does not promote an existing non-admin principal when a human admin already exists', async () => { + hoisted.mockPrincipalFindFirst + .mockResolvedValueOnce({ id: 'principal_attacker', userId: 'user_attacker', role: 'user' }) + .mockResolvedValueOnce({ + id: 'principal_admin', + userId: 'user_admin', + role: 'admin', + type: 'user', + }) + + await expect(saveUseCaseFn({ data: { useCase: 'saas' } })).rejects.toThrow( + 'Only admin can complete setup' + ) + + expect(hoisted.mockUpdateSet).not.toHaveBeenCalled() + expect(hoisted.mockInsertValues).not.toHaveBeenCalled() + }) + + it('does not promote a non-admin principal when another human principal already exists', async () => { + hoisted.mockPrincipalFindFirst + .mockResolvedValueOnce({ id: 'principal_attacker', userId: 'user_attacker', role: 'user' }) + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ id: 'principal_other', userId: 'user_other', role: 'user' }) + + await expect(saveUseCaseFn({ data: { useCase: 'saas' } })).rejects.toThrow( + 'Only admin can complete setup' + ) + + expect(hoisted.mockUpdateSet).not.toHaveBeenCalled() + expect(hoisted.mockInsertValues).not.toHaveBeenCalled() + }) + + it('still bootstraps an admin principal when no other human principal exists', async () => { + hoisted.mockPrincipalFindFirst + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(null) + .mockResolvedValueOnce(null) + + await saveUseCaseFn({ data: { useCase: 'saas' } }) + + expect(hoisted.mockInsertValues).toHaveBeenCalledWith( + expect.objectContaining({ + id: 'principal_new_admin', + userId: 'user_attacker', + role: 'admin', + }) + ) + expect(hoisted.mockUpdateSet).toHaveBeenCalledWith({ + setupState: JSON.stringify({ + version: 1, + steps: { core: true, workspace: false, boards: false }, + useCase: 'saas', + }), + }) + expect(hoisted.mockInvalidateSettingsCache).toHaveBeenCalledOnce() + }) +}) diff --git a/apps/web/src/lib/server/functions/onboarding.ts b/apps/web/src/lib/server/functions/onboarding.ts index 5038b2c28..f463f5591 100644 --- a/apps/web/src/lib/server/functions/onboarding.ts +++ b/apps/web/src/lib/server/functions/onboarding.ts @@ -14,8 +14,10 @@ import { principal, user, postStatuses, - and, eq, + and, + ne, + sql, DEFAULT_STATUSES, } from '@/lib/server/db' import { invalidateSettingsCache } from '@/lib/server/domains/settings/settings.helpers' @@ -25,36 +27,53 @@ import { isPathManaged } from '@/lib/server/config-file/managed-paths' import { slugify } from '@/lib/shared/utils' import { getSetupState } from '@/lib/shared/db-types' -async function ensureOnboardingAdminPrincipal(userId: UserId, logLabel: string): Promise { - const existing = await db.query.principal.findFirst({ - where: eq(principal.userId, userId), - }) - - if (existing && isAdmin(existing.role)) { - return - } - - const existingAdmin = await db.query.principal.findFirst({ - where: and(eq(principal.role, 'admin'), eq(principal.type, 'user')), - columns: { id: true }, - }) +/** Onboarding promotes the acting user to admin during bootstrap only. + * Once another human principal exists, onboarding endpoints must not + * promote a different non-admin principal. */ +async function ensureAdminPrincipal(userId: UserId, logLabel: string): Promise { + await db.transaction(async (tx) => { + await tx.execute( + sql`SELECT pg_advisory_xact_lock(hashtext('quackback:onboarding_bootstrap_admin'))` + ) + + const existing = await tx.query.principal.findFirst({ + where: eq(principal.userId, userId), + }) + if (existing && isAdmin(existing.role)) { + return + } - if (existingAdmin) { - throw new Error('Only admin can complete setup') - } + const existingAdmin = await tx.query.principal.findFirst({ + where: and(eq(principal.role, 'admin'), eq(principal.type, 'user')), + columns: { id: true }, + }) + if (existingAdmin) { + throw new Error('Only admin can complete setup') + } - if (!existing) { - console.log(`[fn:onboarding] ${logLabel}: creating admin principal for first onboarding user`) - await db.insert(principal).values({ - id: generateId('principal'), - userId, - role: 'admin', - createdAt: new Date(), + const existingHumanPrincipal = await tx.query.principal.findFirst({ + where: existing + ? and(eq(principal.type, 'user'), ne(principal.id, existing.id)) + : eq(principal.type, 'user'), + columns: { id: true }, }) - } else { - console.log(`[fn:onboarding] ${logLabel}: upgrading first onboarding user to admin`) - await db.update(principal).set({ role: 'admin' }).where(eq(principal.userId, userId)) - } + if (existingHumanPrincipal) { + throw new Error('Only admin can complete setup') + } + + if (!existing) { + console.log(`[fn:onboarding] ${logLabel}: creating admin principal for first onboarding user`) + await tx.insert(principal).values({ + id: generateId('principal'), + userId, + role: 'admin', + createdAt: new Date(), + }) + } else { + console.log(`[fn:onboarding] ${logLabel}: upgrading first onboarding user to admin`) + await tx.update(principal).set({ role: 'admin' }).where(eq(principal.userId, userId)) + } + }) } /** @@ -138,9 +157,21 @@ export const setupWorkspaceFn = createServerFn({ method: 'POST' }) let setupState: SetupState | null = getSetupState(existingSettings?.setupState ?? null) - // First authenticated user may bootstrap an install, but once a human admin - // exists, onboarding mutations require the acting user to already be admin. - await ensureOnboardingAdminPrincipal(session.user.id as UserId, 'setupWorkspaceFn') + // Fresh install (no settings): first authenticated user becomes admin. + // Settings exist + workspace step done: require existing admin. + // Settings exist + workspace step not done: ensure user becomes admin. + if (!existingSettings) { + await ensureAdminPrincipal(session.user.id as UserId, 'setupWorkspaceFn') + } else if (setupState?.steps?.workspace) { + const principalRecord = await db.query.principal.findFirst({ + where: eq(principal.userId, session.user.id as UserId), + }) + if (!principalRecord || !isAdmin(principalRecord.role)) { + throw new Error('Only admin can complete setup') + } + } else { + await ensureAdminPrincipal(session.user.id as UserId, 'setupWorkspaceFn') + } // Check if onboarding is already complete if (setupState?.steps?.core && setupState?.steps?.workspace && setupState?.steps?.boards) { @@ -354,7 +385,9 @@ export const saveUseCaseFn = createServerFn({ method: 'POST' }) steps: { core: true, workspace: false, boards: false }, } - await ensureOnboardingAdminPrincipal(session.user.id as UserId, 'saveUseCaseFn') + if (!setupState.steps.workspace) { + await ensureAdminPrincipal(session.user.id as UserId, 'saveUseCaseFn') + } const updatedState: SetupState = { ...setupState, useCase: data.useCase } @@ -379,7 +412,7 @@ export const saveUseCaseFn = createServerFn({ method: 'POST' }) useCase: data.useCase, } - await ensureOnboardingAdminPrincipal(session.user.id as UserId, 'saveUseCaseFn') + await ensureAdminPrincipal(session.user.id as UserId, 'saveUseCaseFn') await db.insert(settings).values({ id: generateId('workspace'), From db2033c3a23fa42cc6240fa1fc9f339903e4eadd Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 6 Jun 2026 13:47:19 +0000 Subject: [PATCH 2/2] fix(onboarding): serialize bootstrap admin checks and gate fresh use-case insert --- .../__tests__/onboarding-security.test.ts | 14 +++++++ .../functions/__tests__/onboarding.test.ts | 38 +++++++++++++------ .../src/lib/server/functions/onboarding.ts | 2 + 3 files changed, 42 insertions(+), 12 deletions(-) diff --git a/apps/web/src/lib/server/functions/__tests__/onboarding-security.test.ts b/apps/web/src/lib/server/functions/__tests__/onboarding-security.test.ts index 978624abf..00f9bcff9 100644 --- a/apps/web/src/lib/server/functions/__tests__/onboarding-security.test.ts +++ b/apps/web/src/lib/server/functions/__tests__/onboarding-security.test.ts @@ -196,4 +196,18 @@ describe('saveUseCaseFn onboarding admin promotion', () => { }) expect(hoisted.mockInvalidateSettingsCache).toHaveBeenCalledOnce() }) + + it('rejects before inserting settings on fresh install when another human principal exists', async () => { + hoisted.mockGetSettings.mockResolvedValue(null) + hoisted.mockPrincipalFindFirst + .mockResolvedValueOnce({ id: 'principal_attacker', userId: 'user_attacker', role: 'user' }) + .mockResolvedValueOnce(null) + .mockResolvedValueOnce({ id: 'principal_other', userId: 'user_other', role: 'user' }) + + await expect(saveUseCaseFn({ data: { useCase: 'saas' } })).rejects.toThrow( + 'Only admin can complete setup' + ) + + expect(hoisted.mockInsertValues).not.toHaveBeenCalled() + }) }) diff --git a/apps/web/src/lib/server/functions/__tests__/onboarding.test.ts b/apps/web/src/lib/server/functions/__tests__/onboarding.test.ts index f00567f6a..a6cc8a51e 100644 --- a/apps/web/src/lib/server/functions/__tests__/onboarding.test.ts +++ b/apps/web/src/lib/server/functions/__tests__/onboarding.test.ts @@ -24,6 +24,7 @@ const { state, principalTable, settingsTable, postStatusesTable, userTable } = v statusesExist: true, }, principalTable: { + id: 'principal.id', userId: 'principal.userId', role: 'principal.role', type: 'principal.type', @@ -87,23 +88,20 @@ function matchesWhere(row: Record, where: unknown): boolean { if (clause.op === 'and') { return clause.conditions?.every((condition) => matchesWhere(row, condition)) ?? false } + if (clause.op === 'ne') { + if (clause.col === principalTable.id) return row.id !== clause.value + return false + } if (clause.col === principalTable.userId) return row.userId === clause.value + if (clause.col === principalTable.id) return row.id === clause.value if (clause.col === principalTable.role) return row.role === clause.value if (clause.col === principalTable.type) return row.type === clause.value if (clause.col === settingsTable.id) return row.id === clause.value return false } -vi.mock('@/lib/server/db', () => ({ - USE_CASE_TYPES: ['saas', 'consumer', 'marketplace', 'internal'], - DEFAULT_STATUSES: [], - principal: principalTable, - settings: settingsTable, - postStatuses: postStatusesTable, - user: userTable, - eq: (col: string, value: unknown) => ({ op: 'eq', col, value }), - and: (...conditions: unknown[]) => ({ op: 'and', conditions }), - db: { +vi.mock('@/lib/server/db', () => { + const dbMock = { query: { principal: { findFirst: async ({ where }: { where: unknown }) => @@ -152,8 +150,24 @@ vi.mock('@/lib/server/db', () => ({ }, }), }), - }, -})) + execute: async () => undefined, + transaction: async (fn: (tx: unknown) => unknown) => fn(dbMock), + } + + return { + USE_CASE_TYPES: ['saas', 'consumer', 'marketplace', 'internal'], + DEFAULT_STATUSES: [], + principal: principalTable, + settings: settingsTable, + postStatuses: postStatusesTable, + user: userTable, + eq: (col: string, value: unknown) => ({ op: 'eq', col, value }), + and: (...conditions: unknown[]) => ({ op: 'and', conditions }), + ne: (col: string, value: unknown) => ({ op: 'ne', col, value }), + sql: (strings: TemplateStringsArray, ...values: unknown[]) => ({ strings, values }), + db: dbMock, + } +}) beforeEach(() => { state.sessionUserId = 'user_attacker' diff --git a/apps/web/src/lib/server/functions/onboarding.ts b/apps/web/src/lib/server/functions/onboarding.ts index f463f5591..f4689b299 100644 --- a/apps/web/src/lib/server/functions/onboarding.ts +++ b/apps/web/src/lib/server/functions/onboarding.ts @@ -406,6 +406,8 @@ export const saveUseCaseFn = createServerFn({ method: 'POST' }) // (same rationale as setupWorkspaceFn): no settings row yet to // read managedFieldPaths from. The reconciler will overwrite on // its next tick if the file owns these fields. + await ensureAdminPrincipal(session.user.id as UserId, 'saveUseCaseFn') + const setupState: SetupState = { version: 1, steps: { core: true, workspace: false, boards: false },