fix(housekeeping): close final authorization gaps

This commit is contained in:
Simo committed 2026-08-30 21:01:32 +02:00
1 parent 2b8f73a91d
commit 222535e116
9 files changed
+219 -44

No files matched your search

+17 -22
View File
@@ -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 () => {
+3 -2
View File
@@ -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" };
@@ -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<ContentMutationFailure>);
expect(fsMocks.unlink).not.toHaveBeenCalled();
},
);
it("rejects declared MIME, filename extension, and actual bytes that disagree", async () => {
const disguisedSvg = new File(
['<svg xmlns="http://www.w3.org/2000/svg"></svg>'],
@@ -247,6 +247,7 @@ async function mediaUpload(input: unknown): Promise<ContentMutationSnapshot> {
async function mediaDelete(input: unknown): Promise<ContentMutationSnapshot> {
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);
@@ -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 () =>
@@ -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,
@@ -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",
@@ -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<void> {
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<void> {
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<PeopleMutationSnapshot> {
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 =
+2 -2
View File
@@ -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", () => {