From 222535e116f5cdcaee89fb106f48688df17fab0d Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Sun, 30 Aug 2026 21:01:32 +0200 Subject: [PATCH] fix(housekeeping): close final authorization gaps --- src/actions/content-legacy-parity.test.ts | 39 ++++---- src/actions/save-logo.ts | 5 +- .../mutation-runtime-external.test.ts | 13 +++ .../services/mutation-runtime-external.ts | 1 + .../people/commands/user-commands.test.ts | 33 ++++++- .../domains/people/commands/user-commands.ts | 8 +- .../mutations-production-workflows.test.ts | 97 ++++++++++++++++++- .../domains/people/services/mutations.ts | 63 ++++++++++-- src/lib/admin-operations-contract.test.ts | 4 +- 9 files changed, 219 insertions(+), 44 deletions(-) diff --git a/src/actions/content-legacy-parity.test.ts b/src/actions/content-legacy-parity.test.ts index d0e09022..8846d727 100644 --- a/src/actions/content-legacy-parity.test.ts +++ b/src/actions/content-legacy-parity.test.ts @@ -4,6 +4,7 @@ import { redirect } from "next/navigation"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { getHousekeepingCapabilityContext } from "@/features/housekeeping/foundation/server-capability-context"; import { requirePermission, requireStaff } from "@/lib/admin/guard"; +import { PERMS } from "@/lib/permission-slugs"; import { createAd } from "./admin-ads"; import { createArticle } from "./admin-articles"; import { uploadMedia } from "./admin-media"; @@ -192,7 +193,7 @@ describe("Content compatibility wrappers", () => { expect(executeLegacyBrandAssetMutation).not.toHaveBeenCalled(); }); - it("preserves the legacy favicon page gate and establishes a staff logo floor", async () => { + it("preserves the favicon page gate and requires settings edit for logo mutation", async () => { vi.clearAllMocks(); const file = new File(["bytes"], "image.png", { type: "image/png" }); await saveFavicon(form({ file }) as FormData); @@ -200,8 +201,8 @@ describe("Content compatibility wrappers", () => { await saveLogo(form({ file }) as FormData); expect(requirePermission).toHaveBeenNthCalledWith(1, "settings.view"); expect(requirePermission).toHaveBeenNthCalledWith(2, "settings.view"); - expect(requirePermission).not.toHaveBeenCalledWith("settings.edit"); - expect(requireStaff).toHaveBeenCalledOnce(); + expect(requirePermission).toHaveBeenNthCalledWith(3, PERMS.SETTINGS_EDIT); + expect(requireStaff).not.toHaveBeenCalled(); expect( auditedBrandExecute.mock.calls.map(([operation]) => operation), ).toEqual(["favicon.save", "favicon.delete", "logo.save"]); @@ -219,7 +220,9 @@ describe("Content compatibility wrappers", () => { expect(executeLegacyBrandAssetMutation).not.toHaveBeenCalled(); expect(auditedBrandExecute).not.toHaveBeenCalled(); - vi.mocked(requireStaff).mockRejectedValueOnce(new Error("logo denied")); + vi.mocked(requirePermission).mockRejectedValueOnce( + new Error("logo denied"), + ); await expect(saveLogo(form({ file }) as FormData)).rejects.toThrow( "logo denied", ); @@ -227,26 +230,18 @@ describe("Content compatibility wrappers", () => { expect(auditedBrandExecute).not.toHaveBeenCalled(); }); - it("lets a requireStaff-approved actor without an additional ACL use the audited logo boundary", async () => { + it("does not let a staff-only actor bypass the logo settings ACL", async () => { vi.clearAllMocks(); - vi.mocked(requireStaff).mockResolvedValue(staff as never); - const file = new File(["bytes"], "logo.png", { type: "image/png" }); - await expect(saveLogo(form({ file }) as FormData)).resolves.toEqual({ - success: true, - url: "/api/media/x", - }); - expect(requirePermission).not.toHaveBeenCalled(); - expect(requireStaff).toHaveBeenCalledOnce(); - expect(auditedBrandExecute).toHaveBeenCalledWith( - "logo.save", - { file }, - expect.objectContaining({ - capability: expect.objectContaining({ - actor: expect.objectContaining({ id: 42 }), - }), - legacy: true, - }), + vi.mocked(requirePermission).mockRejectedValueOnce( + new Error("settings edit denied"), ); + const file = new File(["bytes"], "logo.png", { type: "image/png" }); + await expect(saveLogo(form({ file }) as FormData)).rejects.toThrow( + "settings edit denied", + ); + expect(requirePermission).toHaveBeenCalledWith(PERMS.SETTINGS_EDIT); + expect(requireStaff).not.toHaveBeenCalled(); + expect(auditedBrandExecute).not.toHaveBeenCalled(); }); it("refuses a brand mutation when the rehydrated actor changes after the legacy guard", async () => { diff --git a/src/actions/save-logo.ts b/src/actions/save-logo.ts index e127dd87..b85bdaa5 100644 --- a/src/actions/save-logo.ts +++ b/src/actions/save-logo.ts @@ -3,7 +3,8 @@ import { revalidatePath } from "next/cache"; import type { ContentMutationSnapshot } from "@/features/housekeeping/domains/content/services/mutations"; import { createCorrelationId } from "@/features/housekeeping/foundation/contracts"; -import { requireStaff, type StaffUser } from "@/lib/admin/guard"; +import { requirePermission, type StaffUser } from "@/lib/admin/guard"; +import { PERMS } from "@/lib/permission-slugs"; const PARTIAL_ERROR = "Logo change completed partially; verify storage and audit state"; @@ -34,7 +35,7 @@ async function executeAuditedLogoMutation( export async function saveLogo( formData: FormData, ): Promise<{ success: boolean; url?: string; error?: string }> { - const staff = await requireStaff(); + const staff = await requirePermission(PERMS.SETTINGS_EDIT); try { const file = formData.get("file") as File | null; if (!file) return { success: false, error: "No file provided" }; diff --git a/src/features/housekeeping/domains/content/services/mutation-runtime-external.test.ts b/src/features/housekeeping/domains/content/services/mutation-runtime-external.test.ts index f6ff73c4..33a7efe4 100644 --- a/src/features/housekeeping/domains/content/services/mutation-runtime-external.test.ts +++ b/src/features/housekeeping/domains/content/services/mutation-runtime-external.test.ts @@ -147,6 +147,19 @@ describe("Content external mutation runtime", () => { expect(fsMocks.unlink).not.toHaveBeenCalled(); }); + it.each(["favicon/brand.ico", "logo/brand.png", "favicon\\brand.ico"])( + "rejects nested media deletion outside the generic upload namespace: %s", + async (filename) => { + await expect( + executeContentExternalMutation("media.delete", { filename }, context), + ).rejects.toMatchObject({ + code: "VALIDATION", + messageKey: "errors.housekeeping.validation", + } satisfies Partial); + expect(fsMocks.unlink).not.toHaveBeenCalled(); + }, + ); + it("rejects declared MIME, filename extension, and actual bytes that disagree", async () => { const disguisedSvg = new File( [''], diff --git a/src/features/housekeeping/domains/content/services/mutation-runtime-external.ts b/src/features/housekeeping/domains/content/services/mutation-runtime-external.ts index 220351f2..7fe7e025 100644 --- a/src/features/housekeeping/domains/content/services/mutation-runtime-external.ts +++ b/src/features/housekeeping/domains/content/services/mutation-runtime-external.ts @@ -247,6 +247,7 @@ async function mediaUpload(input: unknown): Promise { async function mediaDelete(input: unknown): Promise { const name = text(record(input).filename ?? record(input).name, 255, true); + if (name.includes("/") || name.includes("\\")) throw validation(); let filePath: string; try { filePath = resolveMediaPath(name); diff --git a/src/features/housekeeping/domains/people/commands/user-commands.test.ts b/src/features/housekeeping/domains/people/commands/user-commands.test.ts index a104b472..cd6b18d4 100644 --- a/src/features/housekeeping/domains/people/commands/user-commands.test.ts +++ b/src/features/housekeeping/domains/people/commands/user-commands.test.ts @@ -79,8 +79,8 @@ describe("People user commands", () => { "people.user.reset-password": [PERMS.USERS_RESET_PASSWORD], "people.user.send-currency": [PERMS.USERS_EDIT], "people.user.trade-lock": [PERMS.USERS_EDIT], - "people.users.bulk-ban": [PERMS.USERS_EDIT], - "people.users.bulk-unban": [PERMS.USERS_EDIT], + "people.users.bulk-ban": [PERMS.USERS_BAN], + "people.users.bulk-unban": [PERMS.USERS_BAN], "people.users.bulk-currency": [PERMS.USERS_EDIT], "people.users.bulk-badge": [PERMS.USERS_EDIT], }); @@ -127,6 +127,15 @@ describe("People user commands", () => { }, ); + it("accepts configured rank identifiers above the historical rank 7 ceiling", () => { + const command = commands.find((entry) => entry.id === "people.user.update"); + if (!command) throw new Error("command missing"); + + expect( + command.input.safeParse({ userId: 7, fields: { rank: 8 } }).success, + ).toBe(true); + }); + it("delegates parsed input and correlation to the redirect-free service", async () => { const execute = vi.fn(async (context, operation, input) => ({ ok: true as const, @@ -163,6 +172,26 @@ describe("People user commands", () => { }); describe("People mutation service boundary", () => { + it("requires the ban capability for bulk ban operations", async () => { + const execute = vi.fn(async () => ({ before: null, after: null })); + const service = createPeopleMutationService({ execute }, async () => + capabilityContext([PERMS.USERS_EDIT]), + ); + + const result = await service.execute( + { expectedActorId: 42, correlationId: "denied-bulk-ban" }, + "users.bulk-ban", + { userIds: [7], reason: "abuse", duration: 0 }, + ); + + expect(result).toMatchObject({ + ok: false, + error: { code: "FORBIDDEN" }, + correlationId: "denied-bulk-ban", + }); + expect(execute).not.toHaveBeenCalled(); + }); + it("fails closed before the adapter when the exact capability is absent", async () => { const execute = vi.fn(async () => ({ before: null, after: null })); const service = createPeopleMutationService({ execute }, async () => diff --git a/src/features/housekeeping/domains/people/commands/user-commands.ts b/src/features/housekeeping/domains/people/commands/user-commands.ts index 5c6989e6..7be5fefb 100644 --- a/src/features/housekeeping/domains/people/commands/user-commands.ts +++ b/src/features/housekeeping/domains/people/commands/user-commands.ts @@ -68,7 +68,7 @@ const userIds = z.array(positiveId).min(1).max(100); const optionalUpdateFields = { username: z.string().min(3).max(20).optional(), mail: z.email().optional(), - rank: z.number().int().min(1).max(7).optional(), + rank: z.number().int().min(1).optional(), motto: z.string().max(127).optional(), credits: z.number().int().min(0).max(2_147_483_647).optional(), pixels: z.number().int().min(0).max(2_147_483_647).optional(), @@ -78,7 +78,7 @@ const optionalUpdateFields = { const updateFields = z.union([ z.object({ ...optionalUpdateFields, username: z.string().min(3).max(20) }), z.object({ ...optionalUpdateFields, mail: z.email() }), - z.object({ ...optionalUpdateFields, rank: z.number().int().min(1).max(7) }), + z.object({ ...optionalUpdateFields, rank: z.number().int().min(1) }), z.object({ ...optionalUpdateFields, motto: z.string().max(127) }), z.object({ ...optionalUpdateFields, @@ -198,7 +198,7 @@ export function createUserCommands( userCommand(service, { id: "people.users.bulk-ban", operation: "users.bulk-ban", - capability: PERMS.USERS_EDIT, + capability: PERMS.USERS_BAN, input: z.object({ userIds, reason: requiredText(500), @@ -210,7 +210,7 @@ export function createUserCommands( userCommand(service, { id: "people.users.bulk-unban", operation: "users.bulk-unban", - capability: PERMS.USERS_EDIT, + capability: PERMS.USERS_BAN, input: z.object({ userIds }), requiresReason: true, attempts: 3, diff --git a/src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts b/src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts index 144495fe..6eeac817 100644 --- a/src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts +++ b/src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts @@ -5,6 +5,7 @@ import type { HousekeepingCapabilityContext } from "../../../foundation/contract const mocks = vi.hoisted(() => ({ audit: vi.fn(), deleteWhere: vi.fn(), + execute: vi.fn(), hashPassword: vi.fn(), invalidateLoginCache: vi.fn(), insertValues: vi.fn(), @@ -56,6 +57,7 @@ function selectResult() { function databaseFacade() { const facade = { + execute: mocks.execute, select: vi.fn(() => ({ from: vi.fn(() => ({ where: vi.fn(selectResult), @@ -164,14 +166,98 @@ beforeEach(() => { mocks.selectQueue.length = 0; mocks.resolveServerContext.mockResolvedValue(context()); mocks.audit.mockResolvedValue(undefined); + mocks.execute.mockResolvedValue([[{ id: 8 }], []]); mocks.hashPassword.mockResolvedValue("hashed-password"); mocks.reloadSettings.mockResolvedValue(undefined); for (const call of Object.values(mocks.rcon)) call.mockResolvedValue(true); }); describe("People production workflow adapter", () => { + it("does not treat a high numeric rank as a super-admin hierarchy bypass", async () => { + mocks.resolveServerContext.mockResolvedValue({ + ...context(), + actor: { id: 42, username: "operator", rank: 8 }, + isSuperAdmin: false, + }); + mocks.selectQueue.push([target({ rank: 8 })]); + + const result = await peopleMutationService.execute( + invocation, + "user.update", + { userId: 7, fields: { motto: "Denied" } }, + ); + + expect(result).toMatchObject({ ok: false, error: { code: "FORBIDDEN" } }); + expect(mocks.transaction).not.toHaveBeenCalled(); + }); + + it("prevents a non-super-admin from assigning their own configured rank", async () => { + mocks.resolveServerContext.mockResolvedValue({ + ...context(), + actor: { id: 42, username: "operator", rank: 8 }, + isSuperAdmin: false, + }); + mocks.selectQueue.push([target({ rank: 2 })]); + + const result = await peopleMutationService.execute( + invocation, + "user.update", + { userId: 7, fields: { rank: 8 } }, + ); + + expect(result).toMatchObject({ ok: false, error: { code: "FORBIDDEN" } }); + expect(mocks.transaction).not.toHaveBeenCalled(); + }); + + it("rejects a rank assignment when the configured rank does not exist", async () => { + mocks.resolveServerContext.mockResolvedValue({ + ...context(), + actor: { id: 42, username: "operator", rank: 8 }, + isSuperAdmin: true, + }); + mocks.selectQueue.push([target({ rank: 2 })]); + mocks.execute.mockResolvedValueOnce([[], []]); + + const result = await peopleMutationService.execute( + invocation, + "user.update", + { userId: 7, fields: { rank: 8 } }, + ); + + expect(result).toMatchObject({ + ok: false, + error: { + code: "VALIDATION", + fieldErrors: { rank: ["errors.validation.invalid"] }, + }, + }); + expect(mocks.transaction).not.toHaveBeenCalled(); + }); + + it("checks every bulk target hierarchy before starting a mutation", async () => { + mocks.resolveServerContext.mockResolvedValue({ + ...context(), + actor: { id: 42, username: "operator", rank: 8 }, + isSuperAdmin: false, + }); + mocks.selectQueue.push([{ id: 7, rank: 8 }]); + + const result = await peopleMutationService.execute( + invocation, + "users.bulk-ban", + { userIds: [7], duration: 3600, reason: "Denied" }, + ); + + expect(result).toMatchObject({ ok: false, error: { code: "FORBIDDEN" } }); + expect(mocks.transaction).not.toHaveBeenCalled(); + }); + it("preserves legacy bulk order, duplicates, totals, and StaffActivities", async () => { const ids = Array.from({ length: 101 }, (_, index) => (index % 2) + 1); + mocks.selectQueue.push([ + { id: 1, rank: 2 }, + { id: 2, rank: 2 }, + ]); const result = await peopleMutationService.execute( invocation, "users.bulk-unban", @@ -694,6 +780,10 @@ describe("People production workflow adapter", () => { ); it("executes bulk ban, currency, and badge with database, RCON, and legacy activity outcomes", async () => { + mocks.selectQueue.push([ + { id: 7, rank: 2 }, + { id: 8, rank: 2 }, + ]); await expect( peopleMutationService.execute(invocation, "users.bulk-ban", { userIds: [7, 8], @@ -706,6 +796,7 @@ describe("People production workflow adapter", () => { }); expect(mocks.insertValues).toHaveBeenCalledTimes(2); + mocks.selectQueue.push([{ id: 7, rank: 2 }]); await expect( peopleMutationService.execute(invocation, "users.bulk-currency", { userIds: [7], @@ -718,7 +809,7 @@ describe("People production workflow adapter", () => { }); expect(mocks.rcon.giveCredits).toHaveBeenCalledWith(7, 50); - mocks.selectQueue.push([], [{ maxSlot: 2 }]); + mocks.selectQueue.push([{ id: 7, rank: 2 }], [], [{ maxSlot: 2 }]); await expect( peopleMutationService.execute(invocation, "users.bulk-badge", { userIds: [7], @@ -743,6 +834,7 @@ describe("People production workflow adapter", () => { it.each(["false", "throw"] as const)( "reports bulk currency database completion separately when RCON reports %s", async (failureMode) => { + mocks.selectQueue.push([{ id: 7, rank: 2 }]); if (failureMode === "false") { mocks.rcon.giveCredits.mockResolvedValue(false); } else { @@ -790,7 +882,7 @@ describe("People production workflow adapter", () => { it.each(["false", "throw"] as const)( "reports bulk badge database completion separately when RCON reports %s", async (failureMode) => { - mocks.selectQueue.push([], [{ maxSlot: 2 }]); + mocks.selectQueue.push([{ id: 7, rank: 2 }], [], [{ maxSlot: 2 }]); if (failureMode === "false") { mocks.rcon.giveBadge.mockResolvedValue(false); } else { @@ -895,6 +987,7 @@ describe("People production workflow adapter", () => { mocks.resolveServerContext.mockResolvedValue(context()); mocks.audit.mockResolvedValue(undefined); mocks.rcon.giveCredits.mockResolvedValue(false); + mocks.selectQueue.push([{ id: 7, rank: 2 }]); const legacyBulk = await peopleMutationService.execute( invocation, "users.bulk-currency", diff --git a/src/features/housekeeping/domains/people/services/mutations.ts b/src/features/housekeeping/domains/people/services/mutations.ts index 87517a2d..43521051 100644 --- a/src/features/housekeeping/domains/people/services/mutations.ts +++ b/src/features/housekeeping/domains/people/services/mutations.ts @@ -149,6 +149,8 @@ function operationCapability(operation: PeopleMutationOperation) { if ( operation === "user.ban" || operation === "user.unban" || + operation === "users.bulk-ban" || + operation === "users.bulk-unban" || operation === "ban.create" || operation === "ban.lift" || operation === "help-ticket.unban" @@ -400,7 +402,7 @@ async function loadTarget( if ( guardHierarchy && target.rank >= context.capability.actor.rank && - context.capability.actor.rank < 7 + !context.capability.isSuperAdmin ) { throw new PeopleMutationFailure( "FORBIDDEN", @@ -410,6 +412,43 @@ async function loadTarget( return target; } +async function assertBulkTargetHierarchy( + userIds: readonly number[], + context: PeopleMutationContext, +): Promise { + const uniqueIds = [...new Set(userIds)]; + const targets = await db + .select({ id: User.id, rank: User.rank }) + .from(User) + .where(inArray(User.id, uniqueIds)); + if (targets.length !== uniqueIds.length) { + throw new PeopleMutationFailure( + "NOT_FOUND", + "errors.housekeeping.notFound", + ); + } + if ( + !context.capability.isSuperAdmin && + targets.some((target) => target.rank >= context.capability.actor.rank) + ) { + throw new PeopleMutationFailure( + "FORBIDDEN", + "errors.housekeeping.forbidden", + ); + } +} + +async function assertConfiguredRankExists(rank: number): Promise { + const [rows] = (await db.execute( + sql`SELECT id FROM permission_ranks WHERE id = ${rank} LIMIT 1`, + )) as unknown as [unknown[], unknown]; + if (!Array.isArray(rows) || rows.length === 0) { + throw new PeopleMutationFailure("VALIDATION", "errors.validation.invalid", { + rank: ["errors.validation.invalid"], + }); + } +} + function targetSnapshot(target: TargetUser) { return { id: target.id, @@ -592,15 +631,18 @@ async function executeUserMutation( if (operation === "user.update") { const fields = record(data.fields); const nextRank = fields.rank; - if ( - nextRank !== undefined && - positiveInteger(nextRank) >= context.capability.actor.rank && - context.capability.actor.rank < 7 - ) { - throw new PeopleMutationFailure( - "FORBIDDEN", - "errors.housekeeping.forbidden", - ); + if (nextRank !== undefined) { + const rank = positiveInteger(nextRank); + if ( + rank >= context.capability.actor.rank && + !context.capability.isSuperAdmin + ) { + throw new PeopleMutationFailure( + "FORBIDDEN", + "errors.housekeeping.forbidden", + ); + } + await assertConfiguredRankExists(rank); } const userPatch = Object.fromEntries( ["username", "mail", "rank", "motto", "credits", "pixels"].flatMap( @@ -1072,6 +1114,7 @@ async function executeBulkMutation( ): Promise { const data = record(input); const ids = userIds(data.userIds, context.legacy); + await assertBulkTargetHierarchy(ids, context); const failures: Array<{ userId: number; reason: string }> = []; const externalSyncFailures: Array<{ userId: number; reason: string }> = []; const externalSyncFailureReason = diff --git a/src/lib/admin-operations-contract.test.ts b/src/lib/admin-operations-contract.test.ts index f602fada..2a8e5e0c 100644 --- a/src/lib/admin-operations-contract.test.ts +++ b/src/lib/admin-operations-contract.test.ts @@ -38,7 +38,7 @@ describe("administration backend contract", () => { expect(source).not.toMatch(/await requireStaffRateLimited\(\)/); }); - it("keeps requireStaff only for the approved legacy logo security floor", () => { + it("requires explicit permissions for every administration action", () => { const dir = "src/actions"; const offenders: string[] = []; for (const name of readdirSync(dir)) { @@ -48,7 +48,7 @@ describe("administration backend contract", () => { offenders.push(name); } } - expect(offenders).toEqual(["save-logo.ts"]); + expect(offenders).toEqual([]); }); it("keeps the shared ops health probe behind the API", () => {