From 4e0bf598ed9a31c86ae5e0cc1d3287c4d7f61296 Mon Sep 17 00:00:00 2001 From: simoleo89 Date: Thu, 27 Aug 2026 17:44:53 +0200 Subject: [PATCH] fix(housekeeping): harden preference persistence --- src/actions/housekeeping-preferences.test.ts | 99 ++++++++++--------- src/actions/housekeeping-preferences.ts | 52 +++++----- .../foundation/preferences/repository.test.ts | 98 ++++++++++++++++++ .../foundation/preferences/repository.ts | 12 ++- .../foundation/preferences/schema.test.ts | 28 ++++++ .../foundation/preferences/schema.ts | 22 ++++- .../housekeeping-preferences-repository.ts | 43 ++++++++ 7 files changed, 272 insertions(+), 82 deletions(-) create mode 100644 src/lib/housekeeping-preferences-repository.ts diff --git a/src/actions/housekeeping-preferences.test.ts b/src/actions/housekeeping-preferences.test.ts index 976df44d28..767ad7532e 100644 --- a/src/actions/housekeeping-preferences.test.ts +++ b/src/actions/housekeeping-preferences.test.ts @@ -1,89 +1,90 @@ -import { describe, expect, it, vi } from "vitest"; +import { beforeEach, describe, expect, it, vi } from "vitest"; vi.mock("@/features/housekeeping/foundation/server-capability-context", () => ({ getHousekeepingCapabilityContext: vi.fn(), })); -import type { HousekeepingPreferencesRepository } from "@/features/housekeeping/foundation/preferences/repository"; +const preferenceRepository = vi.hoisted(() => ({ + read: vi.fn(), + upsert: vi.fn(), +})); + +vi.mock("@/lib/housekeeping-preferences-repository", () => ({ + housekeepingPreferencesRepository: preferenceRepository, +})); + import { defaultHousekeepingPreferences } from "@/features/housekeeping/foundation/preferences/schema"; -import type { HousekeepingRegistry } from "@/features/housekeeping/foundation/registry"; +import { getHousekeepingCapabilityContext } from "@/features/housekeeping/foundation/server-capability-context"; import { - type HousekeepingPreferencesActionDependencies, loadHousekeepingPreferences, saveHousekeepingPreferences, } from "./housekeeping-preferences"; -const registry: HousekeepingRegistry = { domains: [] }; const allowedContext = { actor: { id: 42, username: "operator", rank: 0 }, isSuperAdmin: false, - has: (slug: string) => slug === "admin.dashboard", - hasAny: (...slugs: string[]) => slugs.includes("admin.dashboard"), - hasAll: (...slugs: string[]) => - slugs.every((slug) => slug === "admin.dashboard"), + has: () => true, + hasAny: () => true, + hasAll: () => true, }; -function dependencies( - context = allowedContext, -): HousekeepingPreferencesActionDependencies & { - repository: HousekeepingPreferencesRepository; -} { - return { - repository: { - read: vi.fn().mockResolvedValue(defaultHousekeepingPreferences()), - upsert: vi.fn(), - }, - registry, - getContext: vi.fn().mockResolvedValue(context), - }; -} +beforeEach(() => { + vi.clearAllMocks(); + vi.mocked(getHousekeepingCapabilityContext).mockResolvedValue(allowedContext); + preferenceRepository.read.mockResolvedValue(defaultHousekeepingPreferences()); +}); describe("housekeeping preference actions", () => { - it("derives the read owner from request capability context", async () => { - const deps = dependencies(); - - const result = await loadHousekeepingPreferences(deps); - - expect(result.ok).toBe(true); - expect(deps.repository.read).toHaveBeenCalledWith(42); - expect(deps.getContext).toHaveBeenCalledOnce(); + it("does not expose dependency or user identity parameters to callers", () => { + expect(loadHousekeepingPreferences).toHaveLength(0); + expect(saveHousekeepingPreferences).toHaveLength(1); }); - it("rejects a denied operator without reading or writing another user preferences", async () => { - const deps = dependencies({ ...allowedContext, hasAny: () => false }); + it("derives the read owner from the server capability context", async () => { + const result = await loadHousekeepingPreferences(); + + expect(result.ok).toBe(true); + expect(preferenceRepository.read).toHaveBeenCalledWith(42); + }); + + it("rejects a denied operator without reading or writing preferences", async () => { + vi.mocked(getHousekeepingCapabilityContext).mockResolvedValueOnce({ + ...allowedContext, + hasAny: () => false, + }); const result = await saveHousekeepingPreferences( defaultHousekeepingPreferences(), - deps, ); expect(result).toMatchObject({ ok: false, error: { code: "FORBIDDEN" } }); - expect(deps.repository.read).not.toHaveBeenCalled(); - expect(deps.repository.upsert).not.toHaveBeenCalled(); + expect(preferenceRepository.read).not.toHaveBeenCalled(); + expect(preferenceRepository.upsert).not.toHaveBeenCalled(); }); - it("validates writes and persists them for the context actor only", async () => { - const deps = dependencies(); + it("reconciles input before persisting or returning it", async () => { const value = { ...defaultHousekeepingPreferences(), - pinnedRouteIds: ["people.users"], + pinnedRouteIds: ["removed.route"], + pinnedCommandIds: ["removed.command"], }; - const result = await saveHousekeepingPreferences(value, deps); + const result = await saveHousekeepingPreferences(value); - expect(result).toMatchObject({ ok: true, data: value }); - expect(deps.repository.upsert).toHaveBeenCalledWith(42, value); + expect(result).toMatchObject({ + ok: true, + data: { pinnedRouteIds: [], pinnedCommandIds: [] }, + }); + expect(preferenceRepository.upsert).toHaveBeenCalledWith( + 42, + expect.objectContaining({ pinnedRouteIds: [], pinnedCommandIds: [] }), + ); }); it("returns a validation result before an invalid payload reaches persistence", async () => { - const deps = dependencies(); - - const result = await saveHousekeepingPreferences( - { schemaVersion: 2 }, - deps, - ); + const result = await saveHousekeepingPreferences({ schemaVersion: 2 }); expect(result).toMatchObject({ ok: false, error: { code: "VALIDATION" } }); - expect(deps.repository.upsert).not.toHaveBeenCalled(); + expect(preferenceRepository.upsert).not.toHaveBeenCalled(); }); }); diff --git a/src/actions/housekeeping-preferences.ts b/src/actions/housekeeping-preferences.ts index adedaef0e3..87f97bf9de 100644 --- a/src/actions/housekeeping-preferences.ts +++ b/src/actions/housekeeping-preferences.ts @@ -1,42 +1,41 @@ +"use server"; + import { authorizeHousekeeping } from "@/features/housekeeping/foundation/authorization"; import { anyCapability, fail, - type HousekeepingCapabilityContext, type HousekeepingResult, ok, } from "@/features/housekeeping/foundation/contracts"; import { createCorrelationId } from "@/features/housekeeping/foundation/correlation"; import { reconcilePreferences } from "@/features/housekeeping/foundation/preferences/reconcile"; -import type { HousekeepingPreferencesRepository } from "@/features/housekeeping/foundation/preferences/repository"; import { type HousekeepingPreferences, housekeepingPreferencesSchema, } from "@/features/housekeeping/foundation/preferences/schema"; -import type { HousekeepingRegistry } from "@/features/housekeeping/foundation/registry"; +import { createHousekeepingRegistry } from "@/features/housekeeping/foundation/registry"; import { getHousekeepingCapabilityContext } from "@/features/housekeeping/foundation/server-capability-context"; +import { HOUSEKEEPING_MANIFESTS } from "@/features/housekeeping/manifests"; +import { housekeepingPreferencesRepository } from "@/lib/housekeeping-preferences-repository"; import { PERMS } from "@/lib/permission-slugs"; const preferencesCapability = anyCapability(PERMS.ADMIN_DASHBOARD); +const housekeepingRegistry = createHousekeepingRegistry(HOUSEKEEPING_MANIFESTS); -export interface HousekeepingPreferencesActionDependencies { - repository: HousekeepingPreferencesRepository; - registry: HousekeepingRegistry; - getContext?: () => Promise; -} - -export async function loadHousekeepingPreferences( - dependencies: HousekeepingPreferencesActionDependencies, -): Promise> { - const context = await resolveContext(dependencies); +export async function loadHousekeepingPreferences(): Promise< + HousekeepingResult +> { + const context = await getHousekeepingCapabilityContext(); const authorization = authorizeHousekeeping(context, preferencesCapability); if (!authorization.ok) return authorization; const correlationId = createCorrelationId(); try { - const stored = await dependencies.repository.read(context.actor.id); + const stored = await housekeepingPreferencesRepository.read( + context.actor.id, + ); return ok( - reconcilePreferences(stored, dependencies.registry, context), + reconcilePreferences(stored, housekeepingRegistry, context), correlationId, ); } catch { @@ -50,9 +49,8 @@ export async function loadHousekeepingPreferences( export async function saveHousekeepingPreferences( input: unknown, - dependencies: HousekeepingPreferencesActionDependencies, ): Promise> { - const context = await resolveContext(dependencies); + const context = await getHousekeepingCapabilityContext(); const authorization = authorizeHousekeeping(context, preferencesCapability); if (!authorization.ok) return authorization; @@ -66,9 +64,17 @@ export async function saveHousekeepingPreferences( ); } + const reconciled = reconcilePreferences( + parsed.data, + housekeepingRegistry, + context, + ); try { - await dependencies.repository.upsert(context.actor.id, parsed.data); - return ok(parsed.data, correlationId); + await housekeepingPreferencesRepository.upsert( + context.actor.id, + reconciled, + ); + return ok(reconciled, correlationId); } catch { return fail( "INTERNAL", @@ -77,11 +83,3 @@ export async function saveHousekeepingPreferences( ); } } - -async function resolveContext( - dependencies: HousekeepingPreferencesActionDependencies, -): Promise { - return dependencies.getContext - ? dependencies.getContext() - : getHousekeepingCapabilityContext(); -} diff --git a/src/features/housekeeping/foundation/preferences/repository.test.ts b/src/features/housekeeping/foundation/preferences/repository.test.ts index 4bfbdbdc98..c5fdf4acc7 100644 --- a/src/features/housekeeping/foundation/preferences/repository.test.ts +++ b/src/features/housekeeping/foundation/preferences/repository.test.ts @@ -1,4 +1,16 @@ import { describe, expect, it, vi } from "vitest"; + +const drizzleMocks = vi.hoisted(() => ({ + eq: vi.fn((column: unknown, value: unknown) => ({ column, value })), +})); + +vi.mock("drizzle-orm", async (importOriginal) => ({ + ...(await importOriginal()), + eq: drizzleMocks.eq, +})); + +import { HousekeepingUserPreferences } from "@/lib/db"; +import { createDrizzleHousekeepingPreferencesStorage } from "@/lib/housekeeping-preferences-repository"; import { createHousekeepingPreferencesRepository, type HousekeepingPreferencesStorage, @@ -39,4 +51,90 @@ describe("housekeeping preferences repository", () => { payload: JSON.stringify(value), }); }); + + it("returns a fresh default for malformed or incompatible stored values", async () => { + const storage: HousekeepingPreferencesStorage = { + findByUserId: vi + .fn() + .mockResolvedValueOnce({ schemaVersion: 1, payload: "{broken" }) + .mockResolvedValueOnce({ schemaVersion: 2, payload: "{}" }) + .mockResolvedValueOnce({ + schemaVersion: 1, + payload: JSON.stringify({ schemaVersion: 1 }), + }), + upsert: vi.fn(), + }; + const repository = createHousekeepingPreferencesRepository(storage); + + await expect(repository.read(42)).resolves.toEqual( + defaultHousekeepingPreferences(), + ); + await expect(repository.read(42)).resolves.toEqual( + defaultHousekeepingPreferences(), + ); + await expect(repository.read(42)).resolves.toEqual( + defaultHousekeepingPreferences(), + ); + }); + + it("preserves genuine storage failures instead of treating them as corrupt preferences", async () => { + const storage: HousekeepingPreferencesStorage = { + findByUserId: vi + .fn() + .mockRejectedValue(new Error("database unavailable")), + upsert: vi.fn(), + }; + + await expect( + createHousekeepingPreferencesRepository(storage).read(42), + ).rejects.toThrow("database unavailable"); + }); + + it("uses the preferences table and user ID for the typed Drizzle read and upsert", async () => { + const limit = vi.fn().mockResolvedValue([]); + const where = vi.fn().mockReturnValue({ limit }); + const from = vi.fn().mockReturnValue({ where }); + const select = vi.fn().mockReturnValue({ from }); + const onDuplicateKeyUpdate = vi.fn().mockResolvedValue(undefined); + const values = vi.fn().mockReturnValue({ onDuplicateKeyUpdate }); + const insert = vi.fn().mockReturnValue({ values }); + const storage = createDrizzleHousekeepingPreferencesStorage({ + select, + insert, + } as never); + const value = defaultHousekeepingPreferences(); + + await storage.findByUserId(42); + await storage.upsert({ + userId: 42, + schemaVersion: 1, + payload: JSON.stringify(value), + }); + + expect(select).toHaveBeenCalledWith({ + schemaVersion: HousekeepingUserPreferences.schemaVersion, + payload: HousekeepingUserPreferences.payload, + }); + expect(from).toHaveBeenCalledWith(HousekeepingUserPreferences); + expect(drizzleMocks.eq).toHaveBeenCalledWith( + HousekeepingUserPreferences.userId, + 42, + ); + expect(where).toHaveBeenCalledWith({ + column: HousekeepingUserPreferences.userId, + value: 42, + }); + expect(values).toHaveBeenCalledWith({ + userId: 42, + schemaVersion: 1, + payload: JSON.stringify(value), + }); + expect(onDuplicateKeyUpdate).toHaveBeenCalledWith({ + set: expect.objectContaining({ + schemaVersion: 1, + payload: JSON.stringify(value), + updatedAt: expect.any(Date), + }), + }); + }); }); diff --git a/src/features/housekeeping/foundation/preferences/repository.ts b/src/features/housekeeping/foundation/preferences/repository.ts index 592fa2fd36..6f43acb11c 100644 --- a/src/features/housekeeping/foundation/preferences/repository.ts +++ b/src/features/housekeeping/foundation/preferences/repository.ts @@ -32,11 +32,15 @@ export function createHousekeepingPreferencesRepository( async read(userId) { const row = await storage.findByUserId(userId); if (!row) return defaultHousekeepingPreferences(); - if (row.schemaVersion !== 1) { - throw new Error("unsupported housekeeping preferences schema version"); - } - return parseHousekeepingPreferences(row.payload); + try { + if (row.schemaVersion !== 1) { + return defaultHousekeepingPreferences(); + } + return parseHousekeepingPreferences(row.payload); + } catch { + return defaultHousekeepingPreferences(); + } }, async upsert(userId, value) { await storage.upsert({ diff --git a/src/features/housekeeping/foundation/preferences/schema.test.ts b/src/features/housekeeping/foundation/preferences/schema.test.ts index fe8abcf211..8067ce16a9 100644 --- a/src/features/housekeeping/foundation/preferences/schema.test.ts +++ b/src/features/housekeeping/foundation/preferences/schema.test.ts @@ -43,4 +43,32 @@ describe("housekeeping preferences schema", () => { expect(parseHousekeepingPreferences(JSON.stringify(value))).toEqual(value); }); + + it("rejects identifiers and lists that exceed the bounded presentation payload", () => { + const value = defaultHousekeepingPreferences(); + + expect( + housekeepingPreferencesSchema.safeParse({ + ...value, + pinnedRouteIds: ["a".repeat(129)], + }).success, + ).toBe(false); + expect( + housekeepingPreferencesSchema.safeParse({ + ...value, + pinnedRouteIds: Array.from( + { length: 65 }, + (_, index) => `route.${index}`, + ), + }).success, + ).toBe(false); + expect( + housekeepingPreferencesSchema.safeParse({ + ...value, + pinnedRouteIds: Array.from({ length: 33 }, (_, index) => + `${index}`.padEnd(128, "a"), + ), + }).success, + ).toBe(false); + }); }); diff --git a/src/features/housekeeping/foundation/preferences/schema.ts b/src/features/housekeeping/foundation/preferences/schema.ts index b5ee80d3e2..0bed8f8f65 100644 --- a/src/features/housekeeping/foundation/preferences/schema.ts +++ b/src/features/housekeeping/foundation/preferences/schema.ts @@ -1,5 +1,9 @@ import { z } from "zod"; +export const HOUSEKEEPING_PREFERENCE_MAX_IDENTIFIER_LENGTH = 128; +export const HOUSEKEEPING_PREFERENCE_MAX_ITEMS_PER_ARRAY = 64; +export const HOUSEKEEPING_PREFERENCE_MAX_SERIALIZED_BYTES = 4096; + export interface HousekeepingPreferences { schemaVersion: 1; pinnedRouteIds: string[]; @@ -10,7 +14,10 @@ export interface HousekeepingPreferences { } const uniqueIdentifiers = z - .array(z.string().trim().min(1)) + .array( + z.string().trim().min(1).max(HOUSEKEEPING_PREFERENCE_MAX_IDENTIFIER_LENGTH), + ) + .max(HOUSEKEEPING_PREFERENCE_MAX_ITEMS_PER_ARRAY) .superRefine((values, context) => { const seen = new Set(); for (const [index, value] of values.entries()) { @@ -35,7 +42,18 @@ export const housekeepingPreferencesSchema: z.ZodType = widgetOrder: uniqueIdentifiers, enabledOptionalWidgetIds: uniqueIdentifiers, }) - .strict(); + .strict() + .superRefine((preferences, context) => { + if ( + JSON.stringify(preferences).length > + HOUSEKEEPING_PREFERENCE_MAX_SERIALIZED_BYTES + ) { + context.addIssue({ + code: "custom", + message: "housekeeping preferences payload is too large", + }); + } + }); export function defaultHousekeepingPreferences(): HousekeepingPreferences { return { diff --git a/src/lib/housekeeping-preferences-repository.ts b/src/lib/housekeeping-preferences-repository.ts new file mode 100644 index 0000000000..237d460f90 --- /dev/null +++ b/src/lib/housekeeping-preferences-repository.ts @@ -0,0 +1,43 @@ +import { eq } from "drizzle-orm"; +import { + createHousekeepingPreferencesRepository, + type HousekeepingPreferencesStorage, +} from "@/features/housekeeping/foundation/preferences/repository"; +import { type Db, db, HousekeepingUserPreferences } from "@/lib/db"; + +type HousekeepingPreferencesDb = Pick; + +export function createDrizzleHousekeepingPreferencesStorage( + database: HousekeepingPreferencesDb, +): HousekeepingPreferencesStorage { + return { + async findByUserId(userId) { + const rows = await database + .select({ + schemaVersion: HousekeepingUserPreferences.schemaVersion, + payload: HousekeepingUserPreferences.payload, + }) + .from(HousekeepingUserPreferences) + .where(eq(HousekeepingUserPreferences.userId, userId)) + .limit(1); + return rows[0] ?? null; + }, + async upsert(row) { + await database + .insert(HousekeepingUserPreferences) + .values(row) + .onDuplicateKeyUpdate({ + set: { + schemaVersion: row.schemaVersion, + payload: row.payload, + updatedAt: new Date(), + }, + }); + }, + }; +} + +export const housekeepingPreferencesRepository = + createHousekeepingPreferencesRepository( + createDrizzleHousekeepingPreferencesStorage(db), + );