From 2f0b713a21329dc434d2b5866ee43a94fab713c3 Mon Sep 17 00:00:00 2001 From: Richie McIlroy <33632126+richiemcilroy@users.noreply.github.com> Date: Mon, 3 Aug 2026 01:56:30 +0100 Subject: [PATCH] fix(web): require an explicit share for org video downloads --- .../unit/video-download-permissions.test.ts | 114 ++++++++++++++++++ apps/web/actions/videos/download.ts | 1 - apps/web/app/s/[videoId]/page.tsx | 1 - apps/web/lib/video-download-permissions.ts | 38 +++--- 4 files changed, 136 insertions(+), 18 deletions(-) create mode 100644 apps/web/__tests__/unit/video-download-permissions.test.ts diff --git a/apps/web/__tests__/unit/video-download-permissions.test.ts b/apps/web/__tests__/unit/video-download-permissions.test.ts new file mode 100644 index 00000000000..5737566ef28 --- /dev/null +++ b/apps/web/__tests__/unit/video-download-permissions.test.ts @@ -0,0 +1,114 @@ +import type { Organisation, User, Video } from "@cap/web-domain"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const schema = { + organizationMembers: { table: "organizationMembers" }, + sharedVideos: { table: "sharedVideos" }, + spaceMembers: { table: "spaceMembers" }, + spaceVideos: { table: "spaceVideos" }, +}; + +vi.mock("@cap/database/schema", () => schema); + +// Each select() call resolves against the next queued result, and the table it +// read is recorded so a test can assert which lookups actually happened. +let queued: unknown[][] = []; +const tablesRead: string[] = []; + +const mockDb = { + select: () => mockDb, + from: (table: { table: string }) => { + tablesRead.push(table.table); + return mockDb; + }, + where: () => { + const rows = queued.shift() ?? []; + const result = Promise.resolve(rows) as Promise & { + limit: () => Promise; + }; + result.limit = () => Promise.resolve(rows); + return result; + }, +}; + +vi.mock("@cap/database", () => ({ db: () => mockDb })); + +const { canUserDownloadVideo } = await import( + "../../lib/video-download-permissions" +); + +const OWNER = "user-owner" as User.UserId; +const OTHER = "user-other" as User.UserId; +const VIDEO = "video-1" as Video.VideoId; +const VIDEO_ORG = "org-owning-the-video" as Organisation.OrganisationId; + +function call(userId: User.UserId) { + return canUserDownloadVideo({ userId, ownerId: OWNER, videoId: VIDEO }); +} + +describe("canUserDownloadVideo", () => { + beforeEach(() => { + queued = []; + tablesRead.length = 0; + }); + + it("allows the owner without querying shares", async () => { + expect(await call(OWNER)).toBe(true); + expect(tablesRead).toEqual([]); + }); + + // The video's own orgId must not grant download access: VideosPolicy.canView + // requires an explicit sharedVideos row, and no creation path writes one, so + // trusting orgId let org colleagues download videos they cannot open. + it("denies an org colleague when the video was never explicitly shared", async () => { + queued = [ + [], // sharedVideos: no explicit org share + [], // spaceVideos: no space share + ]; + + expect(await call(OTHER)).toBe(false); + expect(tablesRead).toContain("sharedVideos"); + expect(tablesRead).not.toContain("organizationMembers"); + }); + + it("allows a member of an org the video was explicitly shared with", async () => { + queued = [ + [{ organizationId: VIDEO_ORG }], // sharedVideos + [{ id: "membership-1" }], // organizationMembers + ]; + + expect(await call(OTHER)).toBe(true); + expect(tablesRead).toEqual(["sharedVideos", "organizationMembers"]); + }); + + it("denies a non-member even when the video is shared with some org", async () => { + queued = [ + [{ organizationId: VIDEO_ORG }], // sharedVideos + [], // organizationMembers: not a member + [], // spaceVideos + ]; + + expect(await call(OTHER)).toBe(false); + }); + + it("allows a member of a space the video was shared into", async () => { + queued = [ + [], // sharedVideos + [{ spaceId: "space-1" }], // spaceVideos + [{ id: "space-membership-1" }], // spaceMembers + ]; + + expect(await call(OTHER)).toBe(true); + expect(tablesRead).toContain("spaceMembers"); + }); + + it("denies a non-member of the space the video was shared into", async () => { + queued = [ + [], // sharedVideos + [{ spaceId: "space-1" }], // spaceVideos + [], // spaceMembers + ]; + + expect(await call(OTHER)).toBe(false); + }); +}); diff --git a/apps/web/actions/videos/download.ts b/apps/web/actions/videos/download.ts index 9a894f13415..5ced7ae8cf8 100644 --- a/apps/web/actions/videos/download.ts +++ b/apps/web/actions/videos/download.ts @@ -90,7 +90,6 @@ export async function getVideoDownloadInfo( userId: user.id, ownerId: video.ownerId, videoId, - orgId: video.orgId, }); if (!allowed) { diff --git a/apps/web/app/s/[videoId]/page.tsx b/apps/web/app/s/[videoId]/page.tsx index 668cf99b48d..a419164ed5c 100644 --- a/apps/web/app/s/[videoId]/page.tsx +++ b/apps/web/app/s/[videoId]/page.tsx @@ -801,7 +801,6 @@ async function AuthorizedContent({ userId, ownerId: video.owner.id, videoId, - orgId: video.orgId, }) : false; diff --git a/apps/web/lib/video-download-permissions.ts b/apps/web/lib/video-download-permissions.ts index 0fc626bf647..d7d3e10ab7a 100644 --- a/apps/web/lib/video-download-permissions.ts +++ b/apps/web/lib/video-download-permissions.ts @@ -5,19 +5,22 @@ import { spaceMembers, spaceVideos, } from "@cap/database/schema"; -import type { Organisation, User, Video } from "@cap/web-domain"; +import type { User, Video } from "@cap/web-domain"; import { and, eq, inArray } from "drizzle-orm"; +// Download access must not be broader than view access. VideosPolicy.canView +// grants org members access only through an explicit sharedVideos row (see +// OrganisationsRepo.membershipForVideo), and no video-creation path writes one, +// so trusting the video's own orgId here let colleagues download recordings +// they cannot open. export async function canUserDownloadVideo({ userId, ownerId, videoId, - orgId, }: { userId: User.UserId; ownerId: User.UserId; videoId: Video.VideoId; - orgId: Organisation.OrganisationId; }): Promise { if (userId === ownerId) return true; @@ -26,20 +29,23 @@ export async function canUserDownloadVideo({ .from(sharedVideos) .where(eq(sharedVideos.videoId, videoId)); - const orgIds = [orgId, ...sharedOrgs.map((org) => org.organizationId)]; - - const [orgMembership] = await db() - .select({ id: organizationMembers.id }) - .from(organizationMembers) - .where( - and( - eq(organizationMembers.userId, userId), - inArray(organizationMembers.organizationId, orgIds), - ), - ) - .limit(1); + if (sharedOrgs.length > 0) { + const [orgMembership] = await db() + .select({ id: organizationMembers.id }) + .from(organizationMembers) + .where( + and( + eq(organizationMembers.userId, userId), + inArray( + organizationMembers.organizationId, + sharedOrgs.map((org) => org.organizationId), + ), + ), + ) + .limit(1); - if (orgMembership) return true; + if (orgMembership) return true; + } const sharedSpaces = await db() .select({ spaceId: spaceVideos.spaceId })