From 42ad860b7d28ab7f8cdb23c1e600489417882ab2 Mon Sep 17 00:00:00 2001 From: The Doom Lab Date: Fri, 9 Oct 2026 01:33:03 +0200 Subject: [PATCH] Stop task logs and contributor pages leaking to people who shouldn't see them (#539) Co-Authored-By: Claude Sonnet 5.5 --- src/contributors/hooks/useContributorData.ts | 2 +- .../queries/getContributor.test.ts | 78 +++++++++++++ src/contributors/queries/getContributor.ts | 40 ++++++- .../utils/getProjectAccess.ts | 24 ++++ src/tasklogs/queries/getTaskLogs.test.ts | 109 ++++++++++++++++++ src/tasklogs/queries/getTaskLogs.ts | 51 +++++++- 6 files changed, 296 insertions(+), 8 deletions(-) create mode 100644 src/contributors/queries/getContributor.test.ts create mode 100644 src/projectprivileges/utils/getProjectAccess.ts create mode 100644 src/tasklogs/queries/getTaskLogs.test.ts diff --git a/src/contributors/hooks/useContributorData.ts b/src/contributors/hooks/useContributorData.ts index 5bf41d48..3e4aaa7a 100644 --- a/src/contributors/hooks/useContributorData.ts +++ b/src/contributors/hooks/useContributorData.ts @@ -4,7 +4,7 @@ import getProjectPrivilege from "src/projectprivileges/queries/getProjectPrivile import getTeamNames from "src/teams/queries/getTeamNames" export const useContributorData = (contributorId: number, projectId: number) => { - const [contributor] = useQuery(getContributor, { contributorId: contributorId }) + const [contributor] = useQuery(getContributor, { contributorId, projectId }) const contributorUser = contributor?.users[0] const [contributorPrivilege] = useQuery(getProjectPrivilege, { diff --git a/src/contributors/queries/getContributor.test.ts b/src/contributors/queries/getContributor.test.ts new file mode 100644 index 00000000..9d59746f --- /dev/null +++ b/src/contributors/queries/getContributor.test.ts @@ -0,0 +1,78 @@ +import { beforeEach, describe, expect, test, vi } from "vitest" + +const privileges = vi.fn() +const memberFindFirst = vi.fn() +vi.mock("db", () => ({ + default: { + projectPrivilege: { findMany: (...args: unknown[]) => privileges(...args) }, + projectMember: { findFirst: (...args: unknown[]) => memberFindFirst(...args) }, + }, + MemberPrivileges: { PROJECT_MANAGER: "PROJECT_MANAGER", CONTRIBUTOR: "CONTRIBUTOR" }, +})) + +import getContributor from "./getContributor" + +const ME = 7 +const ctx: any = { session: { userId: ME, $isAuthorized: () => true, $authorize: () => undefined } } +const person = (userId: number) => ({ id: 41, projectId: 3, users: [{ id: userId }] }) + +describe("getContributor", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + test("someone outside the project can't look anyone up in it", async () => { + privileges.mockResolvedValue([{ projectId: 99, privilege: "PROJECT_MANAGER" }]) + await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).rejects.toThrow( + "Contributor not found" + ) + expect(memberFindFirst).not.toHaveBeenCalled() + }) + + test("the lookup is limited to the project in the request", async () => { + privileges.mockResolvedValue([{ projectId: 3, privilege: "PROJECT_MANAGER" }]) + memberFindFirst.mockResolvedValue(person(55)) + await getContributor({ contributorId: 41, projectId: 3 }, ctx) + expect(memberFindFirst.mock.calls[0]![0].where).toEqual({ id: 41, projectId: 3, name: null }) + }) + + test("a contributor who isn't in that project is not found", async () => { + privileges.mockResolvedValue([{ projectId: 3, privilege: "PROJECT_MANAGER" }]) + memberFindFirst.mockResolvedValue(null) + await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).rejects.toThrow( + "Contributor not found" + ) + }) + + test("project managers can open anyone in their project", async () => { + privileges.mockResolvedValue([{ projectId: 3, privilege: "PROJECT_MANAGER" }]) + memberFindFirst.mockResolvedValue(person(55)) + await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).resolves.toMatchObject({ + id: 41, + }) + }) + + test("a contributor can open their own page", async () => { + privileges.mockResolvedValue([{ projectId: 3, privilege: "CONTRIBUTOR" }]) + memberFindFirst.mockResolvedValue(person(ME)) + await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).resolves.toMatchObject({ + id: 41, + }) + }) + + test("a contributor can open a teammate's page", async () => { + privileges.mockResolvedValue([{ projectId: 3, privilege: "CONTRIBUTOR" }]) + memberFindFirst.mockResolvedValueOnce(person(55)).mockResolvedValueOnce({ id: 8 }) // a shared team + await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).resolves.toMatchObject({ + id: 41, + }) + }) + + test("a contributor can't open someone they share no team with", async () => { + privileges.mockResolvedValue([{ projectId: 3, privilege: "CONTRIBUTOR" }]) + memberFindFirst.mockResolvedValueOnce(person(55)).mockResolvedValueOnce(null) + await expect(getContributor({ contributorId: 41, projectId: 3 }, ctx)).rejects.toThrow( + "Contributor not found" + ) + }) +}) diff --git a/src/contributors/queries/getContributor.ts b/src/contributors/queries/getContributor.ts index 19f1de5a..492147e3 100644 --- a/src/contributors/queries/getContributor.ts +++ b/src/contributors/queries/getContributor.ts @@ -1,17 +1,31 @@ import { resolver } from "@blitzjs/rpc" +import { NotFoundError } from "blitz" import db from "db" import { ProjectMemberWithUsers } from "src/core/types" +import { getProjectAccess } from "src/projectprivileges/utils/getProjectAccess" interface GetContributorInput { contributorId: number + projectId: number } export default resolver.pipe( resolver.authorize(), - async ({ contributorId }: GetContributorInput): Promise => { - // Directly query the database for the specific contributor + async ( + { contributorId, projectId }: GetContributorInput, + ctx + ): Promise => { + const userId = ctx.session.userId as number + const access = await getProjectAccess(userId) + + // Not your project: reported as not found, so ids can't be probed + if (!access.memberProjectIds.includes(projectId)) { + throw new NotFoundError("Contributor not found") + } + + // Directly query the database for the specific contributor, within this project const contributor = await db.projectMember.findFirst({ - where: { id: contributorId, name: null }, // Ensures we're getting a contributor (where name is null) + where: { id: contributorId, projectId, name: null }, // name null means a person, not a team include: { users: true, // Include the related users }, @@ -19,7 +33,7 @@ export default resolver.pipe( // If no contributor is found, return null if (!contributor) { - throw new Error("Contributor not found") + throw new NotFoundError("Contributor not found") } // Check if the contributor has more than one user and throw an error @@ -27,6 +41,24 @@ export default resolver.pipe( throw new Error(`Contributor has ${contributor.users.length} users! Expected exactly 1.`) } + // Project managers may open anyone in their project. Everyone else may open themselves and + // the people on a team with them, which are the contributor pages the app links them to. + if (!access.managerProjectIds.includes(projectId)) { + const person = contributor.users[0]! + const isSelf = person.id === userId + const sharesATeam = + !isSelf && + (await db.projectMember.findFirst({ + where: { + projectId, + name: { not: null }, + AND: [{ users: { some: { id: userId } } }, { users: { some: { id: person.id } } }], + }, + select: { id: true }, + })) !== null + if (!isSelf && !sharesATeam) throw new NotFoundError("Contributor not found") + } + return contributor // This is automatically typed as ProjectMemberWithUsers } ) diff --git a/src/projectprivileges/utils/getProjectAccess.ts b/src/projectprivileges/utils/getProjectAccess.ts new file mode 100644 index 00000000..c7ecb302 --- /dev/null +++ b/src/projectprivileges/utils/getProjectAccess.ts @@ -0,0 +1,24 @@ +import db, { MemberPrivileges } from "db" + +export type ProjectAccess = { + /** Every project the user belongs to, in any role. */ + memberProjectIds: number[] + /** The projects where the user is a project manager. */ + managerProjectIds: number[] +} + +// What the signed-in user may reach on the server. Queries that take a filter from the +// browser use this to stay inside the user's own projects, since the browser can't be trusted +// to ask only for what it is allowed to see. +export async function getProjectAccess(userId: number): Promise { + const privileges = await db.projectPrivilege.findMany({ + where: { userId }, + select: { projectId: true, privilege: true }, + }) + return { + memberProjectIds: privileges.map((row) => row.projectId), + managerProjectIds: privileges + .filter((row) => row.privilege === MemberPrivileges.PROJECT_MANAGER) + .map((row) => row.projectId), + } +} diff --git a/src/tasklogs/queries/getTaskLogs.test.ts b/src/tasklogs/queries/getTaskLogs.test.ts new file mode 100644 index 00000000..82d966d1 --- /dev/null +++ b/src/tasklogs/queries/getTaskLogs.test.ts @@ -0,0 +1,109 @@ +import { beforeEach, describe, expect, test, vi } from "vitest" + +// vi.mock is hoisted above these, so the mock reaches them lazily at call time +const privileges = vi.fn() +const taskLogFindMany = vi.fn() +const taskLogCount = vi.fn() +vi.mock("db", () => ({ + default: { + projectPrivilege: { findMany: (...args: unknown[]) => privileges(...args) }, + taskLog: { + findMany: (...args: unknown[]) => taskLogFindMany(...args), + count: (...args: unknown[]) => taskLogCount(...args), + }, + }, + MemberPrivileges: { PROJECT_MANAGER: "PROJECT_MANAGER", CONTRIBUTOR: "CONTRIBUTOR" }, +})) + +import getTaskLogs from "./getTaskLogs" + +const ME = 7 +const ctx: any = { session: { userId: ME, $isAuthorized: () => true, $authorize: () => undefined } } + +// Two projects: I manage project 1 and am a contributor in project 2 (project 3 is not mine) +const MY_PRIVILEGES = [ + { projectId: 1, privilege: "PROJECT_MANAGER" }, + { projectId: 2, privilege: "CONTRIBUTOR" }, +] + +// What the owner lookup returns for each log id +const OWNERS: Record = { + 10: { projectId: 1, assignedUserIds: [99] }, // someone else's, in a project I manage + 20: { projectId: 2, assignedUserIds: [ME] }, // mine, in a project where I'm a contributor + 21: { projectId: 2, assignedUserIds: [99] }, // someone else's, in that project + 22: { projectId: 2, assignedUserIds: [ME, 99] }, // a team of mine +} + +const fetchedLogs = (...ids: number[]) => + ids.map((id) => ({ id, status: "COMPLETED", metadata: { answer: `secret ${id}` } })) + +function setUp(ids: number[]) { + privileges.mockResolvedValue(MY_PRIVILEGES) + taskLogCount.mockResolvedValue(ids.length) + taskLogFindMany.mockImplementation(async (args: any) => + args.select + ? args.where.id.in.map((id: number) => ({ + id, + task: { projectId: OWNERS[id]!.projectId }, + assignedTo: { users: OWNERS[id]!.assignedUserIds.map((userId) => ({ id: userId })) }, + })) + : fetchedLogs(...ids) + ) +} + +describe("getTaskLogs", () => { + beforeEach(() => { + vi.clearAllMocks() + }) + + test("the filter from the browser is narrowed to the user's own projects", async () => { + setUp([20]) + const requested = { assignedToId: 5 } + await getTaskLogs({ where: requested }, ctx) + const expected = { AND: [requested, { task: { projectId: { in: [1, 2] } } }] } + expect(taskLogFindMany.mock.calls[0]![0].where).toEqual(expected) + expect(taskLogCount.mock.calls[0]![0].where).toEqual(expected) + }) + + test("with no filter at all it still only covers the user's projects", async () => { + setUp([]) + await getTaskLogs({}, ctx) + expect(taskLogFindMany.mock.calls[0]![0].where).toEqual({ + task: { projectId: { in: [1, 2] } }, + }) + }) + + test("project managers keep every response in their projects", async () => { + setUp([10]) + const { taskLogs } = await getTaskLogs({}, ctx) + expect(taskLogs[0]!.metadata).toEqual({ answer: "secret 10" }) + }) + + test("others' responses are blanked, but my own and my teams' are kept, in a project where I'm a contributor", async () => { + setUp([20, 21, 22]) + const { taskLogs } = await getTaskLogs({}, ctx) + const byId = Object.fromEntries(taskLogs.map((log: any) => [log.id, log])) + expect(byId[20].metadata).toEqual({ answer: "secret 20" }) + expect(byId[21].metadata).toBeNull() + expect(byId[22].metadata).toEqual({ answer: "secret 22" }) + // everything else about the log is still there, so summaries and counts keep working + expect(byId[21].status).toBe("COMPLETED") + }) + + test("paged results are covered by the same rule", async () => { + setUp([20, 21]) + const { taskLogs } = await getTaskLogs({ take: 2 }, ctx) + expect(taskLogs.find((log: any) => log.id === 21)!.metadata).toBeNull() + expect(taskLogs.find((log: any) => log.id === 20)!.metadata).toEqual({ answer: "secret 20" }) + }) + + test("a user with no projects gets a filter that matches nothing", async () => { + privileges.mockResolvedValue([]) + taskLogFindMany.mockResolvedValue([]) + taskLogCount.mockResolvedValue(0) + await getTaskLogs({ where: { assignedToId: 5 } }, ctx) + expect(taskLogFindMany.mock.calls[0]![0].where).toEqual({ + AND: [{ assignedToId: 5 }, { task: { projectId: { in: [] } } }], + }) + }) +}) diff --git a/src/tasklogs/queries/getTaskLogs.ts b/src/tasklogs/queries/getTaskLogs.ts index f6e2ed82..bfc35834 100644 --- a/src/tasklogs/queries/getTaskLogs.ts +++ b/src/tasklogs/queries/getTaskLogs.ts @@ -1,16 +1,59 @@ import { resolver } from "@blitzjs/rpc" import db, { Prisma } from "db" import { paginate } from "blitz" +import { getProjectAccess, ProjectAccess } from "src/projectprivileges/utils/getProjectAccess" // Define input types for the query interface GetTaskLogsInput extends Pick {} +// Form responses are only for the people they concern: project managers see them for their +// projects, and everyone else only for logs assigned to them or to a team they are on. Other +// logs are still returned (status, approval and dates feed summaries) with the response blanked. +async function hideOthersResponses( + taskLogs: T[], + userId: number, + access: ProjectAccess +): Promise { + if (taskLogs.length === 0) return taskLogs + const owners = await db.taskLog.findMany({ + where: { id: { in: taskLogs.map((log) => log.id) } }, + select: { + id: true, + task: { select: { projectId: true } }, + assignedTo: { select: { users: { select: { id: true } } } }, + }, + }) + const mayRead = new Set( + owners + .filter( + (log) => + access.managerProjectIds.includes(log.task.projectId) || + log.assignedTo.users.some((user) => user.id === userId) + ) + .map((log) => log.id) + ) + return taskLogs.map((log) => + mayRead.has(log.id) || !("metadata" in log) ? log : { ...log, metadata: null } + ) +} + export default resolver.pipe( resolver.authorize(), // Automatically handles authorization - async ({ where, orderBy, include, skip = 0, take }: GetTaskLogsInput) => { + async ({ where: requestedWhere, orderBy, include, skip = 0, take }: GetTaskLogsInput, ctx) => { + const userId = ctx.session.userId as number + const access = await getProjectAccess(userId) + + // The filter comes from the browser, so it is always narrowed to the user's own projects + const inMyProjects: Prisma.TaskLogWhereInput = { + task: { projectId: { in: access.memberProjectIds } }, + } + const where: Prisma.TaskLogWhereInput = requestedWhere + ? { AND: [requestedWhere, inMyProjects] } + : inMyProjects + if (typeof take !== "number") { - const [taskLogs, count] = await Promise.all([ + const [foundLogs, count] = await Promise.all([ db.taskLog.findMany({ where, orderBy, @@ -20,6 +63,7 @@ export default resolver.pipe( db.taskLog.count({ where }), ]) + const taskLogs = await hideOthersResponses(foundLogs, userId, access) return { taskLogs, nextPage: null, @@ -29,7 +73,7 @@ export default resolver.pipe( } const { - items: taskLogs, + items: foundLogs, hasMore, nextPage, count, @@ -46,6 +90,7 @@ export default resolver.pipe( }), }) + const taskLogs = await hideOthersResponses(foundLogs, userId, access) return { taskLogs, nextPage,