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 <thomas@ajnart.fr>

---------

Co-authored-by: ajnart <thomas@ajnart.fr>

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<typeof apps>;
+
+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<App | null> {
+    const apps = await this.getByIdsAsync([id]);
+    return apps[0] ?? null;
+  }
+
+  public async getByIdsAsync(ids: string[]): Promise<App[]> {
+    const canUserSeeApps = await this.accessControl.canUserSeeAppsAsync(ids);
+    const dbApps = await this.db.query.apps.findMany({
+      where: inArray(apps.id, ids),
+    });
+
+    return canUserSeeApps ? dbApps : [];
+  }
+}

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<WidgetComponentProps<"app">["options"]>(item.options);
+          return [parsedOptions.appId];
+        }
 
-  return itemsWithApps
-    .filter((item) => item.board.isPublic)
-    .flatMap((item) => {
-      if (item.kind === "app") {
-        const parsedOptions = SuperJSON.parse<WidgetComponentProps<"app">["options"]>(item.options);
-        return [parsedOptions.appId];
-      } else if (item.kind === "bookmarks") {
         const parsedOptions = SuperJSON.parse<WidgetComponentProps<"bookmarks">["options"]>(item.options);
         return parsedOptions.items;
-      }
-
-      throw new Error("Failed to get app ids from board. Invalid item kind: 'test'");
-    });
-};
+      });
+  }
+}

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" });

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<typeof apps>;

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);
           };
         },
       );

packages/widgets/src/app/component.tsx

@@ -131,7 +131,7 @@ export default function AppWidget({ options, isEditMode, height, width }: Widget
       </Tooltip.Floating>
       {options.pingEnabled && !settings.forceDisableStatus && !board.disableStatus && app.href ? (
         <Suspense fallback={<PingDot icon={IconLoader} color="blue" tooltip={`${t("common.action.loading")}…`} />}>
-          <PingIndicator href={app.pingUrl ?? app.href} />
+          <PingIndicator appId={app.id} />
         </Suspense>
       ) : null}
     </AppLink>

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<RouterOutputs["widget"]["app"]["ping"]>(ping);
 
   clientApi.widget.app.updatedPing.useSubscription(
-    { url: href },
+    { id: appId },
     {
       onData(data) {
         setPingResult(data);