From 8811af5e6e1c62f3bced77ce345c62337cefc27a Mon Sep 17 00:00:00 2001 From: Meier Lukas Date: Fri, 30 Jan 2026 18:31:13 +0100 Subject: [PATCH] Merge commit from fork * fix(security): resolve ssrf issue with simple ping * refactor: improve testability of app-access-control and fix tests * fix: unnecessary space Co-authored-by: ajnart --------- Co-authored-by: ajnart --- packages/api/src/router/app.ts | 49 ++++++++---- .../api/src/router/app/app-access-control.ts | 67 +++++++++-------- packages/api/src/router/test/app.spec.ts | 12 ++- .../api/src/router/test/widgets/app.spec.ts | 35 +++++++-- packages/api/src/router/widgets/app.ts | 74 ++++++++++++++++--- packages/widgets/src/app/component.tsx | 2 +- .../widgets/src/app/ping/ping-indicator.tsx | 8 +- 7 files changed, 179 insertions(+), 68 deletions(-) diff --git a/packages/api/src/router/app.ts b/packages/api/src/router/app.ts index 32c607951..2f2445330 100644 --- a/packages/api/src/router/app.ts +++ b/packages/api/src/router/app.ts @@ -1,7 +1,9 @@ import { TRPCError } from "@trpc/server"; import { z } from "zod/v4"; +import type { Session } from "@homarr/auth"; import { createId } from "@homarr/common"; +import type { Database, InferSelectModel } from "@homarr/db"; import { asc, eq, inArray, like } from "@homarr/db"; import { apps } from "@homarr/db/schema"; import { selectAppSchema } from "@homarr/db/validationSchemas"; @@ -11,7 +13,7 @@ import { byIdSchema, paginatedSchema } from "@homarr/validation/common"; import { convertIntersectionToZodObject } from "../schema-merger"; import { createTRPCRouter, permissionRequiredProcedure, protectedProcedure, publicProcedure } from "../trpc"; -import { canUserSeeAppAsync } from "./app/app-access-control"; +import { AppAccessControl } from "./app/app-access-control"; const defaultIcon = "https://cdn.jsdelivr.net/gh/homarr-labs/dashboard-icons@master/svg/homarr.svg"; @@ -89,9 +91,8 @@ export const appRouter = createTRPCRouter({ .output(selectAppSchema) .meta({ openapi: { method: "GET", path: "/api/apps/{id}", tags: ["apps"], protect: true } }) .query(async ({ ctx, input }) => { - const app = await ctx.db.query.apps.findFirst({ - where: eq(apps.id, input.id), - }); + const repository = new AppRepository(ctx.db, ctx.session?.user ?? null); + const app = await repository.getByIdAsync(input.id); if (!app) { throw new TRPCError({ @@ -100,20 +101,11 @@ export const appRouter = createTRPCRouter({ }); } - const canUserSeeApp = await canUserSeeAppAsync(ctx.session?.user ?? null, app.id); - if (!canUserSeeApp) { - throw new TRPCError({ - code: "NOT_FOUND", - message: "App not found", - }); - } - return app; }), byIds: publicProcedure.input(z.array(z.string())).query(async ({ ctx, input }) => { - return await ctx.db.query.apps.findMany({ - where: inArray(apps.id, input), - }); + const repository = new AppRepository(ctx.db, ctx.session?.user ?? null); + return await repository.getByIdsAsync(input); }), create: permissionRequiredProcedure .requiresPermission("app-create") @@ -187,3 +179,30 @@ export const appRouter = createTRPCRouter({ await ctx.db.delete(apps).where(eq(apps.id, input.id)); }), }); + +type App = InferSelectModel; + +export class AppRepository { + private readonly accessControl: AppAccessControl; + + constructor( + private db: Database, + user: Session["user"] | null, + ) { + this.accessControl = new AppAccessControl(db, user); + } + + public async getByIdAsync(id: string): Promise { + const apps = await this.getByIdsAsync([id]); + return apps[0] ?? null; + } + + public async getByIdsAsync(ids: string[]): Promise { + const canUserSeeApps = await this.accessControl.canUserSeeAppsAsync(ids); + const dbApps = await this.db.query.apps.findMany({ + where: inArray(apps.id, ids), + }); + + return canUserSeeApps ? dbApps : []; + } +} diff --git a/packages/api/src/router/app/app-access-control.ts b/packages/api/src/router/app/app-access-control.ts index 0f5a0a478..10ee934cc 100644 --- a/packages/api/src/router/app/app-access-control.ts +++ b/packages/api/src/router/app/app-access-control.ts @@ -1,45 +1,52 @@ import SuperJSON from "superjson"; import type { Session } from "@homarr/auth"; -import { db, eq, or } from "@homarr/db"; +import type { Database } from "@homarr/db"; +import { eq, or } from "@homarr/db"; import { items } from "@homarr/db/schema"; import type { WidgetComponentProps } from "../../../../widgets/src"; -export const canUserSeeAppAsync = async (user: Session["user"] | null, appId: string) => { - return await canUserSeeAppsAsync(user, [appId]); -}; +export class AppAccessControl { + constructor( + private db: Database, + private user: Session["user"] | null, + ) {} -export const canUserSeeAppsAsync = async (user: Session["user"] | null, appIds: string[]) => { - if (user) return true; + async canUserSeeAppAsync(appId: string) { + return await this.canUserSeeAppsAsync([appId]); + } - const appIdsOnPublicBoards = await getAllAppIdsOnPublicBoardsAsync(); - return appIds.every((appId) => appIdsOnPublicBoards.includes(appId)); -}; + async canUserSeeAppsAsync(appIds: string[]) { + // Currently any user can see all apps if they are logged in + if (this.user) return true; -const getAllAppIdsOnPublicBoardsAsync = async () => { - const itemsWithApps = await db.query.items.findMany({ - where: or(eq(items.kind, "app"), eq(items.kind, "bookmarks")), - with: { - board: { - columns: { - isPublic: true, + const appIdsOnPublicBoards = await this.getAllAppIdsOnPublicBoardsAsync(); + return appIds.every((appId) => appIdsOnPublicBoards.includes(appId)); + } + + private async getAllAppIdsOnPublicBoardsAsync() { + const itemsWithApps = await this.db.query.items.findMany({ + where: or(eq(items.kind, "app"), eq(items.kind, "bookmarks")), + with: { + board: { + columns: { + isPublic: true, + }, }, }, - }, - }); + }); + + return itemsWithApps + .filter((item) => item.board.isPublic) + .flatMap((item) => { + if (item.kind === "app") { + const parsedOptions = SuperJSON.parse["options"]>(item.options); + return [parsedOptions.appId]; + } - return itemsWithApps - .filter((item) => item.board.isPublic) - .flatMap((item) => { - if (item.kind === "app") { - const parsedOptions = SuperJSON.parse["options"]>(item.options); - return [parsedOptions.appId]; - } else if (item.kind === "bookmarks") { const parsedOptions = SuperJSON.parse["options"]>(item.options); return parsedOptions.items; - } - - throw new Error("Failed to get app ids from board. Invalid item kind: 'test'"); - }); -}; + }); + } +} diff --git a/packages/api/src/router/test/app.spec.ts b/packages/api/src/router/test/app.spec.ts index f8877a403..3002ef1a9 100644 --- a/packages/api/src/router/test/app.spec.ts +++ b/packages/api/src/router/test/app.spec.ts @@ -13,6 +13,14 @@ import * as appAccessControl from "../app/app-access-control"; // Mock the auth module to return an empty session vi.mock("@homarr/auth", () => ({ auth: () => ({}) as Session })); +const createFakeAccessControl = (canAccess: boolean) => + vi.fn( + class { + canUserSeeAppAsync = async (_: string) => await Promise.resolve(canAccess); + canUserSeeAppsAsync = async (_: string[]) => await Promise.resolve(canAccess); + } as unknown as typeof appAccessControl.AppAccessControl, + ); + const createDefaultSession = (permissions: GroupPermissionKey[] = []): Session => ({ user: { id: createId(), permissions, colorScheme: "light" }, expires: new Date().toISOString(), @@ -77,7 +85,7 @@ describe("byId should return an app by id", () => { deviceType: undefined, session: null, }); - vi.spyOn(appAccessControl, "canUserSeeAppAsync").mockReturnValue(Promise.resolve(true)); + vi.spyOn(appAccessControl, "AppAccessControl").mockImplementation(createFakeAccessControl(true)); await db.insert(apps).values([ { @@ -118,7 +126,7 @@ describe("byId should return an app by id", () => { href: "https://mantine.dev", }, ]); - vi.spyOn(appAccessControl, "canUserSeeAppAsync").mockReturnValue(Promise.resolve(false)); + vi.spyOn(appAccessControl, "AppAccessControl").mockImplementation(createFakeAccessControl(false)); // Act const actAsync = async () => await caller.byId({ id: "2" }); diff --git a/packages/api/src/router/test/widgets/app.spec.ts b/packages/api/src/router/test/widgets/app.spec.ts index bd5265ff6..0a14c7c96 100644 --- a/packages/api/src/router/test/widgets/app.spec.ts +++ b/packages/api/src/router/test/widgets/app.spec.ts @@ -1,6 +1,9 @@ import { describe, expect, test, vi } from "vitest"; import type { Session } from "@homarr/auth"; +import { createId } from "@homarr/common"; +import type { InferInsertModel } from "@homarr/db"; +import { apps } from "@homarr/db/schema"; import { createDb } from "@homarr/db/test"; import * as ping from "@homarr/ping"; @@ -9,22 +12,31 @@ import { appRouter } from "../../widgets/app"; // Mock the auth module to return an empty session vi.mock("@homarr/auth", () => ({ auth: () => ({}) as Session })); vi.mock("@homarr/ping", () => ({ sendPingRequestAsync: async () => await Promise.resolve(null) })); +vi.mock("../../app/app-access-control", () => ({ + AppAccessControl: vi.fn( + class { + canUserSeeAppAsync = async (_: string) => await Promise.resolve(true); + canUserSeeAppsAsync = async (_: string[]) => await Promise.resolve(true); + }, + ), +})); describe("ping should call sendPingRequestAsync with url and return result", () => { test("ping with error response should return error and url", async () => { // Arrange - const spy = vi.spyOn(ping, "sendPingRequestAsync"); + vi.spyOn(ping, "sendPingRequestAsync").mockImplementation(() => Promise.resolve({ error: "error" })); const url = "http://localhost"; const db = createDb(); + const app = createApp({ url }); + await db.insert(apps).values(app); const caller = appRouter.createCaller({ db, deviceType: undefined, session: null, }); - spy.mockImplementation(() => Promise.resolve({ error: "error" })); // Act - const result = await caller.ping({ url }); + const result = await caller.ping({ id: app.id }); // Assert expect(result.url).toBe(url); @@ -33,21 +45,32 @@ describe("ping should call sendPingRequestAsync with url and return result", () test("ping with success response should return statusCode and url", async () => { // Arrange - const spy = vi.spyOn(ping, "sendPingRequestAsync"); + vi.spyOn(ping, "sendPingRequestAsync").mockImplementation(() => + Promise.resolve({ statusCode: 200, durationMs: 123 }), + ); const url = "http://localhost"; const db = createDb(); + const app = createApp({ url }); + await db.insert(apps).values(app); const caller = appRouter.createCaller({ db, deviceType: undefined, session: null, }); - spy.mockImplementation(() => Promise.resolve({ statusCode: 200, durationMs: 123 })); // Act - const result = await caller.ping({ url }); + const result = await caller.ping({ id: app.id }); // Assert expect(result.url).toBe(url); expect("statusCode" in result).toBe(true); }); }); + +const createApp = ({ url }: { url: string }) => + ({ + id: createId(), + iconUrl: "", + name: "Test App", + href: url, + }) satisfies InferInsertModel; diff --git a/packages/api/src/router/widgets/app.ts b/packages/api/src/router/widgets/app.ts index 7a65225a6..d07241725 100644 --- a/packages/api/src/router/widgets/app.ts +++ b/packages/api/src/router/widgets/app.ts @@ -1,43 +1,97 @@ +import { TRPCError } from "@trpc/server"; import { observable } from "@trpc/server/observable"; import { z } from "zod/v4"; +import { getServerSettingByKeyAsync } from "@homarr/db/queries"; import { sendPingRequestAsync } from "@homarr/ping"; import { pingChannel, pingUrlChannel } from "@homarr/redis"; import { createTRPCRouter, publicProcedure } from "../../trpc"; +import { AppRepository } from "../app"; export const appRouter = createTRPCRouter({ - ping: publicProcedure.input(z.object({ url: z.string() })).query(async ({ input }) => { - const pingResult = await sendPingRequestAsync(input.url); + ping: publicProcedure.input(z.object({ id: z.string() })).query(async ({ ctx, input }) => { + const boardSettings = await getServerSettingByKeyAsync(ctx.db, "board"); + if (boardSettings.forceDisableStatus) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Ping functionality is disabled by server settings", + }); + } + + const repository = new AppRepository(ctx.db, ctx.session?.user ?? null); + const app = await repository.getByIdAsync(input.id); + + if (!app) { + throw new TRPCError({ + code: "NOT_FOUND", + message: "App not found", + }); + } + + const pingUrl = app.pingUrl ?? app.href; + + if (!pingUrl) { + throw new TRPCError({ + code: "CONFLICT", + message: "No URL to ping configured for this app", + }); + } + + const pingResult = await sendPingRequestAsync(pingUrl); return { - url: input.url, + url: pingUrl, ...pingResult, }; }), updatedPing: publicProcedure .input( z.object({ - url: z.string(), + id: z.string(), }), ) - .subscription(async ({ input }) => { - await pingUrlChannel.addAsync(input.url); + .subscription(async ({ ctx, input }) => { + const boardSettings = await getServerSettingByKeyAsync(ctx.db, "board"); + if (boardSettings.forceDisableStatus) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Ping functionality is disabled by server settings", + }); + } - const pingResult = await sendPingRequestAsync(input.url); + const repository = new AppRepository(ctx.db, ctx.session?.user ?? null); + const app = await repository.getByIdAsync(input.id); + + if (!app) { + throw new TRPCError({ + code: "NOT_FOUND", + message: "App not found", + }); + } + + const pingUrl = app.pingUrl ?? app.href; + + if (!pingUrl) { + throw new TRPCError({ + code: "CONFLICT", + message: "No URL to ping configured for this app", + }); + } + + await pingUrlChannel.addAsync(pingUrl); return observable<{ url: string; statusCode: number; durationMs: number } | { url: string; error: string }>( (emit) => { - emit.next({ url: input.url, ...pingResult }); const unsubscribe = pingChannel.subscribe((message) => { // Only emit if same url - if (message.url !== input.url) return; + if (message.url !== pingUrl) return; emit.next(message); }); return () => { unsubscribe(); - void pingUrlChannel.removeAsync(input.url); + void pingUrlChannel.removeAsync(pingUrl); }; }, ); diff --git a/packages/widgets/src/app/component.tsx b/packages/widgets/src/app/component.tsx index 8511eb7b0..1caa1ce56 100644 --- a/packages/widgets/src/app/component.tsx +++ b/packages/widgets/src/app/component.tsx @@ -131,7 +131,7 @@ export default function AppWidget({ options, isEditMode, height, width }: Widget {options.pingEnabled && !settings.forceDisableStatus && !board.disableStatus && app.href ? ( }> - + ) : null} diff --git a/packages/widgets/src/app/ping/ping-indicator.tsx b/packages/widgets/src/app/ping/ping-indicator.tsx index a74a6c79f..a6299ed95 100644 --- a/packages/widgets/src/app/ping/ping-indicator.tsx +++ b/packages/widgets/src/app/ping/ping-indicator.tsx @@ -7,13 +7,13 @@ import { clientApi } from "@homarr/api/client"; import { PingDot } from "./ping-dot"; interface PingIndicatorProps { - href: string; + appId: string; } -export const PingIndicator = ({ href }: PingIndicatorProps) => { +export const PingIndicator = ({ appId }: PingIndicatorProps) => { const [ping] = clientApi.widget.app.ping.useSuspenseQuery( { - url: href, + id: appId, }, { refetchOnMount: false, @@ -24,7 +24,7 @@ export const PingIndicator = ({ href }: PingIndicatorProps) => { const [pingResult, setPingResult] = useState(ping); clientApi.widget.app.updatedPing.useSubscription( - { url: href }, + { id: appId }, { onData(data) { setPingResult(data);