From 7e3ac76c990924555faade685f1f062e44392370 Mon Sep 17 00:00:00 2001 From: Shalabh Agarwal Date: Fri, 24 Jul 2026 01:45:38 +0530 Subject: [PATCH 1/7] Fix broken access control on MRT queue/job read & dequeue Any authenticated user, regardless of role or queue membership, could enumerate every MRT queue in an org and read or dequeue/lock every job in them -- including CSAM/NCMEC-designated queues -- because Org.mrtQueues, Query.manualReviewQueue, Query.getTotalPendingJobsCount, and Mutation.dequeueManualReviewJob all resolved queues via the *Dangerously*BypassPermissioning helpers with no permission or membership check. Switch those four call sites to getReviewableQueuesForUser, the existing membership-aware lookup already used correctly elsewhere (e.g. RoutingRule.destinationQueue, user.ts's reviewableQueues resolver). Add the missing auth check on ManualReviewQueue.jobs (defense-in-depth; not independently reachable today). The *Dangerously*BypassPermissioning primitives themselves are kept -- RoutingRule.destinationQueue uses the same bypass correctly, gated behind EDIT_MRT_QUEUES. Fixes #1150 Co-Authored-By: Claude --- .../modules/manualReviewTool.resolver.test.ts | 199 ++++++++++++++++++ server/graphql/modules/manualReviewTool.ts | 47 ++++- server/graphql/modules/org.resolver.test.ts | 75 +++++++ server/graphql/modules/org.ts | 8 +- .../modules/QueueOperations.test.ts | 57 +++++ 5 files changed, 375 insertions(+), 11 deletions(-) create mode 100644 server/graphql/modules/manualReviewTool.resolver.test.ts diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts new file mode 100644 index 000000000..5a8c5fdf7 --- /dev/null +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -0,0 +1,199 @@ +import { UserPermission } from '../../services/userManagementService/index.js'; +import { resolvers } from './manualReviewTool.js'; + +type ResolverFn = ( + parent: unknown, + args: unknown, + ctx: unknown, +) => Promise; + +const Query = resolvers.Query as Record< + 'getTotalPendingJobsCount' | 'manualReviewQueue', + ResolverFn +>; +const Mutation = resolvers.Mutation as Record< + 'dequeueManualReviewJob', + ResolverFn +>; +const ManualReviewQueue = resolvers.ManualReviewQueue as Record< + 'jobs', + ResolverFn +>; + +function makeCtx(opts: { + reviewableQueueIds: string[]; + user?: { + id: string; + orgId: string; + permissions: readonly UserPermission[]; + } | null; +}) { + const user = + opts.user === undefined + ? { id: 'user-1', orgId: 'org-1', permissions: [UserPermission.VIEW_MRT] } + : opts.user; + + const getReviewableQueuesForUser = jest.fn(async () => + opts.reviewableQueueIds.map((id) => ({ id, orgId: 'org-1', name: id })), + ); + const getAllQueuesForOrgAndDangerouslyBypassPermissioning = jest.fn( + async () => { + throw new Error('resolver must not bypass permissioning (#1150)'); + }, + ); + const getQueueForOrgAndDangerouslyBypassPermissioning = jest.fn(async () => { + throw new Error('resolver must not bypass permissioning (#1150)'); + }); + const getTotalPendingJobCountForQueues = jest.fn(async () => 7); + const dequeueNextJob = jest.fn(async () => null); + const getAllJobsForQueue = jest.fn(async () => []); + + const ctx = { + getUser: () => + user == null + ? null + : { + id: user.id, + orgId: user.orgId, + getPermissions: () => user.permissions, + }, + services: { + ManualReviewToolService: { + getReviewableQueuesForUser, + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + getQueueForOrgAndDangerouslyBypassPermissioning, + getTotalPendingJobCountForQueues, + dequeueNextJob, + getAllJobsForQueue, + }, + }, + }; + + return { + ctx, + getReviewableQueuesForUser, + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + getQueueForOrgAndDangerouslyBypassPermissioning, + getTotalPendingJobCountForQueues, + dequeueNextJob, + getAllJobsForQueue, + }; +} + +describe('MRT queue/job resolvers are membership-scoped', () => { + describe('Query.getTotalPendingJobsCount', () => { + it('counts only the queues the caller can review, never all org queues', async () => { + const { + ctx, + getReviewableQueuesForUser, + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + getTotalPendingJobCountForQueues, + } = makeCtx({ reviewableQueueIds: ['q-1', 'q-2'] }); + + await expect(Query.getTotalPendingJobsCount({}, {}, ctx)).resolves.toBe( + 7, + ); + + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'user-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + }); + expect(getTotalPendingJobCountForQueues).toHaveBeenCalledWith('org-1', [ + 'q-1', + 'q-2', + ]); + expect( + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + ).not.toHaveBeenCalled(); + }); + + it('throws when there is no authenticated user', async () => { + const { ctx, getReviewableQueuesForUser } = makeCtx({ + reviewableQueueIds: [], + user: null, + }); + await expect(Query.getTotalPendingJobsCount({}, {}, ctx)).rejects.toThrow( + 'Authenticated user required', + ); + expect(getReviewableQueuesForUser).not.toHaveBeenCalled(); + }); + }); + + describe('Query.manualReviewQueue', () => { + it('returns a queue the caller can review', async () => { + const { ctx } = makeCtx({ reviewableQueueIds: ['q-1', 'q-2'] }); + await expect( + Query.manualReviewQueue({}, { id: 'q-2' }, ctx), + ).resolves.toMatchObject({ id: 'q-2' }); + }); + + it('returns null for a queue the caller is not a member of', async () => { + const { ctx, getQueueForOrgAndDangerouslyBypassPermissioning } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + Query.manualReviewQueue({}, { id: 'q-forbidden' }, ctx), + ).resolves.toBeNull(); + expect( + getQueueForOrgAndDangerouslyBypassPermissioning, + ).not.toHaveBeenCalled(); + }); + }); + + describe('Mutation.dequeueManualReviewJob', () => { + it('rejects a dequeue against a queue the caller cannot review', async () => { + const { ctx, dequeueNextJob } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + Mutation.dequeueManualReviewJob({}, { queueId: 'q-forbidden' }, ctx), + ).rejects.toThrow('User does not have access to this queue'); + expect(dequeueNextJob).not.toHaveBeenCalled(); + }); + + it('allows a dequeue against a queue the caller can review', async () => { + const { ctx, dequeueNextJob } = makeCtx({ + reviewableQueueIds: ['q-1', 'q-2'], + }); + await expect( + Mutation.dequeueManualReviewJob({}, { queueId: 'q-2' }, ctx), + ).resolves.toBeNull(); + expect(dequeueNextJob).toHaveBeenCalledWith({ + orgId: 'org-1', + queueId: 'q-2', + userId: 'user-1', + }); + }); + + it('throws when there is no authenticated user', async () => { + const { ctx, getReviewableQueuesForUser } = makeCtx({ + reviewableQueueIds: [], + user: null, + }); + await expect( + Mutation.dequeueManualReviewJob({}, { queueId: 'q-1' }, ctx), + ).rejects.toThrow('User required.'); + expect(getReviewableQueuesForUser).not.toHaveBeenCalled(); + }); + }); + + describe('ManualReviewQueue.jobs', () => { + it('throws when there is no authenticated user', async () => { + const { ctx, getAllJobsForQueue } = makeCtx({ + reviewableQueueIds: [], + user: null, + }); + await expect( + ManualReviewQueue.jobs( + { orgId: 'org-1', id: 'q-1' }, + { ids: null, limit: null }, + ctx, + ), + ).rejects.toThrow('User required.'); + expect(getAllJobsForQueue).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index f76fefe89..38ab95edc 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -1748,6 +1748,11 @@ const NcmecManualReviewJobPayload: GQLNcmecManualReviewJobPayloadResolvers = { const ManualReviewQueue: GQLManualReviewQueueResolvers = { async jobs(queue, { ids: jobIds, limit }, context) { + const user = context.getUser(); + if (user == null) { + throw unauthenticatedError('User required.'); + } + const { orgId, id: queueId } = queue; if (jobIds == null) { @@ -2083,14 +2088,20 @@ const Query: GQLQueryResolvers = { if (user == null) { throw unauthenticatedError('Authenticated user required'); } - const allQueues = - await context.services.ManualReviewToolService.getAllQueuesForOrgAndDangerouslyBypassPermissioning( - { orgId: user.orgId }, + const reviewableQueues = + await context.services.ManualReviewToolService.getReviewableQueuesForUser( + { + invoker: { + userId: user.id, + permissions: user.getPermissions(), + orgId: user.orgId, + }, + }, ); return context.services.ManualReviewToolService.getTotalPendingJobCountForQueues( user.orgId, - allQueues.map((q) => q.id), + reviewableQueues.map((q) => q.id), ); }, @@ -2176,11 +2187,17 @@ const Query: GQLQueryResolvers = { throw unauthenticatedError('User required.'); } - const queue = - await context.services.ManualReviewToolService.getQueueForOrgAndDangerouslyBypassPermissioning( - { orgId: user.orgId, queueId: id }, + const reviewableQueues = + await context.services.ManualReviewToolService.getReviewableQueuesForUser( + { + invoker: { + userId: user.id, + permissions: user.getPermissions(), + orgId: user.orgId, + }, + }, ); - return queue ?? null; + return reviewableQueues.find((queue) => queue.id === id) ?? null; }, async getCommentsForJob(_: unknown, { jobId }, context) { const user = context.getUser(); @@ -2275,6 +2292,20 @@ const Mutation: GQLMutationResolvers = { throw unauthenticatedError('User required.'); } + const reviewableQueues = + await context.services.ManualReviewToolService.getReviewableQueuesForUser( + { + invoker: { + userId: user.id, + permissions: user.getPermissions(), + orgId: user.orgId, + }, + }, + ); + if (!reviewableQueues.some((queue) => queue.id === queueId)) { + throw forbiddenError('User does not have access to this queue'); + } + const { id: userId, orgId } = user; const nextJob = await context.services.ManualReviewToolService.dequeueNextJob({ diff --git a/server/graphql/modules/org.resolver.test.ts b/server/graphql/modules/org.resolver.test.ts index 9d1d0c8ca..923077c8c 100644 --- a/server/graphql/modules/org.resolver.test.ts +++ b/server/graphql/modules/org.resolver.test.ts @@ -246,4 +246,79 @@ describe('Org resolvers', () => { expect(getOrgUsersForGraphQL).not.toHaveBeenCalled(); }); }); + + describe('Org.mrtQueues is membership-scoped', () => { + function makeCtx(opts: { callerOrgId?: string | null }) { + const getReviewableQueuesForUser = jest.fn(async () => [ + { id: 'q-1', orgId: 'org-1', name: 'q-1' }, + ]); + const getAllQueuesForOrgAndDangerouslyBypassPermissioning = jest.fn( + async () => { + throw new Error('resolver must not bypass permissioning'); + }, + ); + const ctx = { + getUser: () => + opts.callerOrgId === null + ? null + : { + id: 'user-1', + orgId: opts.callerOrgId ?? 'org-1', + getPermissions: () => [UserPermission.VIEW_MRT], + }, + services: { + ManualReviewToolService: { + getReviewableQueuesForUser, + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + }, + }, + }; + return { + ctx, + getReviewableQueuesForUser, + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + }; + } + + const orgParent = { id: 'org-1' }; + const Org = resolvers.Org as Record< + 'mrtQueues', + ( + parent: typeof orgParent, + args: unknown, + ctx: unknown, + ) => Promise + >; + + it('delegates to getReviewableQueuesForUser, not the bypass helper', async () => { + const { + ctx, + getReviewableQueuesForUser, + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + } = makeCtx({}); + await expect(Org.mrtQueues(orgParent, {}, ctx)).resolves.toEqual([ + { id: 'q-1', orgId: 'org-1', name: 'q-1' }, + ]); + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'user-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + }); + expect( + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + ).not.toHaveBeenCalled(); + }); + + it('throws the IDOR guard when the caller is in a different org', async () => { + const { ctx, getReviewableQueuesForUser } = makeCtx({ + callerOrgId: 'other-org', + }); + await expect(Org.mrtQueues(orgParent, {}, ctx)).rejects.toThrow( + 'User required', + ); + expect(getReviewableQueuesForUser).not.toHaveBeenCalled(); + }); + }); }); diff --git a/server/graphql/modules/org.ts b/server/graphql/modules/org.ts index 73af75047..8f28413b4 100644 --- a/server/graphql/modules/org.ts +++ b/server/graphql/modules/org.ts @@ -400,11 +400,13 @@ const Org: GQLOrgResolvers = { if (!user || user.orgId !== org.id) { throw unauthenticatedError('User required'); } - return context.services.ManualReviewToolService.getAllQueuesForOrgAndDangerouslyBypassPermissioning( - { + return context.services.ManualReviewToolService.getReviewableQueuesForUser({ + invoker: { + userId: user.id, + permissions: user.getPermissions(), orgId: user.orgId, }, - ); + }); }, async apiKey(org, _, context) { const user = context.getUser(); diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index b0422f29c..1eb56c715 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -188,6 +188,63 @@ describe('QueueOperations', () => { }, ); + // Regression: #1150 -- MRT queue resolvers used to resolve + // queues via *Dangerously*BypassPermissioning helpers with no permission or + // membership check, so any authenticated user could read (and dequeue/lock) + // every queue in the org, including CSAM/NCMEC queues. They now call + // getReviewableQueuesForUser instead; these lock in its filtering. + const invoker = ( + userId: string, + permissions: UserPermission[], + orgId: string, + ) => ({ invoker: { userId, permissions, orgId } }); + + testWithQueueAndActions()( + 'getReviewableQueuesForUser excludes a queue the user is not a member of', + async ({ org, queue, mrtService, deps }) => { + const { user: outsider } = await createUser(deps.KyselyPg, org.id); + const reviewable = await mrtService.getReviewableQueuesForUser( + invoker(outsider.id, [UserPermission.VIEW_MRT], org.id), + ); + expect(reviewable.map((q) => q.id)).not.toContain(queue.id); + }, + ); + + testWithQueueAndActions()( + 'getReviewableQueuesForUser includes a queue the user is a member of', + async ({ org, queue, user, mrtService }) => { + const reviewable = await mrtService.getReviewableQueuesForUser( + invoker(user.id, [UserPermission.VIEW_MRT], org.id), + ); + expect(reviewable.map((q) => q.id)).toContain(queue.id); + }, + ); + + testWithQueueAndActions()( + 'getReviewableQueuesForUser returns nothing for a user without VIEW_MRT, even for a queue they are a member of', + async ({ org, user, mrtService }) => { + const reviewable = await mrtService.getReviewableQueuesForUser( + invoker(user.id, [], org.id), + ); + expect(reviewable).toEqual([]); + }, + ); + + testWithQueueAndActions()( + 'getReviewableQueuesForUser bypasses membership for EDIT_MRT_QUEUES holders', + async ({ org, queue, mrtService, deps }) => { + const { user: outsider } = await createUser(deps.KyselyPg, org.id); + const reviewable = await mrtService.getReviewableQueuesForUser( + invoker( + outsider.id, + [UserPermission.VIEW_MRT, UserPermission.EDIT_MRT_QUEUES], + org.id, + ), + ); + expect(reviewable.map((q) => q.id)).toContain(queue.id); + }, + ); + // Regression: `deleteAllJobsFromQueue` is irreversible and used to accept // EDIT_MRT_QUEUES (held by moderator managers) -- that gap accidentally // cleared a production queue. It now requires MANAGE_ORG. From 93eb059787b7160e1962d5a1086169c07a4029b9 Mon Sep 17 00:00:00 2001 From: Cassidy James Date: Tue, 15 Sep 2026 11:35:33 -0600 Subject: [PATCH 2/7] Apply batched suggestions from code review Co-authored-by: Cassidy James --- server/graphql/modules/manualReviewTool.resolver.test.ts | 2 +- .../manualReviewToolService/modules/QueueOperations.test.ts | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index 5a8c5fdf7..635d980b0 100644 --- a/server/graphql/modules/manualReviewTool.resolver.test.ts +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -42,7 +42,7 @@ function makeCtx(opts: { }, ); const getQueueForOrgAndDangerouslyBypassPermissioning = jest.fn(async () => { - throw new Error('resolver must not bypass permissioning (#1150)'); + throw new Error('resolver must not bypass permissioning'); }); const getTotalPendingJobCountForQueues = jest.fn(async () => 7); const dequeueNextJob = jest.fn(async () => null); diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index 1eb56c715..98882a0a9 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -188,7 +188,6 @@ describe('QueueOperations', () => { }, ); - // Regression: #1150 -- MRT queue resolvers used to resolve // queues via *Dangerously*BypassPermissioning helpers with no permission or // membership check, so any authenticated user could read (and dequeue/lock) // every queue in the org, including CSAM/NCMEC queues. They now call From a8e742779143595f2aa4b807d60bd34b5f63beab Mon Sep 17 00:00:00 2001 From: Cassidy James Blaede Date: Tue, 15 Sep 2026 17:13:04 -0600 Subject: [PATCH 3/7] fix whitespace --- server/graphql/modules/manualReviewTool.resolver.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index 635d980b0..c239fd9d5 100644 --- a/server/graphql/modules/manualReviewTool.resolver.test.ts +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -42,7 +42,7 @@ function makeCtx(opts: { }, ); const getQueueForOrgAndDangerouslyBypassPermissioning = jest.fn(async () => { - throw new Error('resolver must not bypass permissioning'); + throw new Error('resolver must not bypass permissioning'); }); const getTotalPendingJobCountForQueues = jest.fn(async () => 7); const dequeueNextJob = jest.fn(async () => null); From 126c8c0503bffed5d2b41f34d37a74c477931f09 Mon Sep 17 00:00:00 2001 From: Juan Mrad Date: Sun, 20 Sep 2026 21:28:54 -0500 Subject: [PATCH 4/7] improve security by ensuring queues are revieable and adding assertion everywhere --- .../modules/manualReviewTool.resolver.test.ts | 317 +++++++++++++++++- server/graphql/modules/manualReviewTool.ts | 67 +++- server/graphql/modules/org.resolver.test.ts | 10 + .../manualReviewToolService.ts | 2 + .../modules/QueueOperations.test.ts | 151 +++++++-- .../modules/QueueOperations.ts | 20 +- server/test/fixtureHelpers/createMrtQueue.ts | 5 +- 7 files changed, 515 insertions(+), 57 deletions(-) diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index c239fd9d5..d098f5beb 100644 --- a/server/graphql/modules/manualReviewTool.resolver.test.ts +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -8,7 +8,7 @@ type ResolverFn = ( ) => Promise; const Query = resolvers.Query as Record< - 'getTotalPendingJobsCount' | 'manualReviewQueue', + 'getTotalPendingJobsCount' | 'manualReviewQueue' | 'getExistingJobsForItem', ResolverFn >; const Mutation = resolvers.Mutation as Record< @@ -16,7 +16,12 @@ const Mutation = resolvers.Mutation as Record< ResolverFn >; const ManualReviewQueue = resolvers.ManualReviewQueue as Record< - 'jobs', + | 'jobs' + | 'pendingJobCount' + | 'oldestJobCreatedAt' + | 'explicitlyAssignedReviewers' + | 'hiddenActionIds' + | 'clearReportsTriggerActionIds', ResolverFn >; @@ -33,8 +38,11 @@ function makeCtx(opts: { ? { id: 'user-1', orgId: 'org-1', permissions: [UserPermission.VIEW_MRT] } : opts.user; - const getReviewableQueuesForUser = jest.fn(async () => - opts.reviewableQueueIds.map((id) => ({ id, orgId: 'org-1', name: id })), + const getReviewableQueuesForUser = jest.fn( + async ({ queueIds }: { queueIds?: readonly string[] }) => + opts.reviewableQueueIds + .filter((id) => queueIds == null || queueIds.includes(id)) + .map((id) => ({ id, orgId: 'org-1', name: id })), ); const getAllQueuesForOrgAndDangerouslyBypassPermissioning = jest.fn( async () => { @@ -47,6 +55,20 @@ function makeCtx(opts: { const getTotalPendingJobCountForQueues = jest.fn(async () => 7); const dequeueNextJob = jest.fn(async () => null); const getAllJobsForQueue = jest.fn(async () => []); + const getJobsForQueue = jest.fn(async () => []); + const getExistingJobsForItem = jest.fn(async () => []); + const getPendingJobCount = jest.fn(async () => 3); + const getOldestJobCreatedAt = jest.fn(async () => new Date(0)); + const getUsersWhoCanSeeQueue = jest.fn( + async (): Promise<{ userId: string }[]> => [], + ); + const getHiddenActionsForQueue = jest.fn(async (): Promise => [ + 'action-1', + ]); + const getClearReportsTriggerActionsForQueue = jest.fn( + async (): Promise => [], + ); + const getGraphQLUsersFromIds = jest.fn(async (): Promise => []); const ctx = { getUser: () => @@ -65,8 +87,18 @@ function makeCtx(opts: { getTotalPendingJobCountForQueues, dequeueNextJob, getAllJobsForQueue, + getJobsForQueue, + getExistingJobsForItem, + getPendingJobCount, + getOldestJobCreatedAt, + getUsersWhoCanSeeQueue, + getHiddenActionsForQueue, + getClearReportsTriggerActionsForQueue, }, }, + dataSources: { + userAPI: { getGraphQLUsersFromIds }, + }, }; return { @@ -77,6 +109,14 @@ function makeCtx(opts: { getTotalPendingJobCountForQueues, dequeueNextJob, getAllJobsForQueue, + getJobsForQueue, + getExistingJobsForItem, + getPendingJobCount, + getOldestJobCreatedAt, + getUsersWhoCanSeeQueue, + getHiddenActionsForQueue, + getClearReportsTriggerActionsForQueue, + getGraphQLUsersFromIds, }; } @@ -143,14 +183,92 @@ describe('MRT queue/job resolvers are membership-scoped', () => { }); }); + describe('Query.getExistingJobsForItem', () => { + it('searches only the queues the caller can review, never all org queues', async () => { + const { ctx, getReviewableQueuesForUser, getExistingJobsForItem } = + makeCtx({ reviewableQueueIds: ['q-1', 'q-2'] }); + + await expect( + Query.getExistingJobsForItem( + {}, + { itemId: 'item-1', itemTypeId: 'content' }, + ctx, + ), + ).resolves.toEqual([]); + + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'user-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + }); + expect(getExistingJobsForItem).toHaveBeenCalledWith({ + orgId: 'org-1', + itemId: 'item-1', + itemTypeId: 'content', + queueIds: ['q-1', 'q-2'], + }); + }); + + it('searches nothing for a caller with no reviewable queues', async () => { + const { ctx, getExistingJobsForItem } = makeCtx({ + reviewableQueueIds: [], + user: { + id: 'user-1', + orgId: 'org-1', + permissions: [], + }, + }); + + await expect( + Query.getExistingJobsForItem( + {}, + { itemId: 'item-1', itemTypeId: 'content' }, + ctx, + ), + ).resolves.toEqual([]); + + expect(getExistingJobsForItem).toHaveBeenCalledWith({ + orgId: 'org-1', + itemId: 'item-1', + itemTypeId: 'content', + queueIds: [], + }); + }); + + it('throws when there is no authenticated user', async () => { + const { ctx, getReviewableQueuesForUser } = makeCtx({ + reviewableQueueIds: [], + user: null, + }); + await expect( + Query.getExistingJobsForItem( + {}, + { itemId: 'item-1', itemTypeId: 'content' }, + ctx, + ), + ).rejects.toThrow('Authenticated user required'); + expect(getReviewableQueuesForUser).not.toHaveBeenCalled(); + }); + }); + describe('Mutation.dequeueManualReviewJob', () => { it('rejects a dequeue against a queue the caller cannot review', async () => { - const { ctx, dequeueNextJob } = makeCtx({ + const { ctx, getReviewableQueuesForUser, dequeueNextJob } = makeCtx({ reviewableQueueIds: ['q-1'], }); await expect( Mutation.dequeueManualReviewJob({}, { queueId: 'q-forbidden' }, ctx), ).rejects.toThrow('User does not have access to this queue'); + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'user-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + queueIds: ['q-forbidden'], + }); expect(dequeueNextJob).not.toHaveBeenCalled(); }); @@ -180,20 +298,199 @@ describe('MRT queue/job resolvers are membership-scoped', () => { }); }); - describe('ManualReviewQueue.jobs', () => { - it('throws when there is no authenticated user', async () => { + describe('ManualReviewQueue queue-scoped fields authorize their parent', () => { + const jobsArgs = { ids: null, limit: null }; + + it('jobs throws when there is no authenticated user', async () => { const { ctx, getAllJobsForQueue } = makeCtx({ reviewableQueueIds: [], user: null, }); + await expect( + ManualReviewQueue.jobs({ orgId: 'org-1', id: 'q-1' }, jobsArgs, ctx), + ).rejects.toThrow('User required.'); + expect(getAllJobsForQueue).not.toHaveBeenCalled(); + }); + + it('jobs returns jobs for a queue the caller can review', async () => { + const { ctx, getAllJobsForQueue } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + ManualReviewQueue.jobs({ orgId: 'org-1', id: 'q-1' }, jobsArgs, ctx), + ).resolves.toEqual([]); + expect(getAllJobsForQueue).toHaveBeenCalled(); + }); + + it('jobs refuses a queue reachable only through a stale favorite', async () => { + const { ctx, getAllJobsForQueue } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); await expect( ManualReviewQueue.jobs( - { orgId: 'org-1', id: 'q-1' }, - { ids: null, limit: null }, + { orgId: 'org-1', id: 'q-revoked' }, + jobsArgs, ctx, ), - ).rejects.toThrow('User required.'); + ).rejects.toThrow('User does not have access to this queue'); expect(getAllJobsForQueue).not.toHaveBeenCalled(); }); + + it('jobs allows a queue manager without a separate VIEW_MRT permission', async () => { + const { ctx, getAllJobsForQueue } = makeCtx({ + reviewableQueueIds: ['q-1'], + user: { + id: 'user-1', + orgId: 'org-1', + permissions: [ + UserPermission.EDIT_MRT_QUEUES, + UserPermission.MANAGE_ROUTING_RULES, + ], + }, + }); + await expect( + ManualReviewQueue.jobs({ orgId: 'org-1', id: 'q-1' }, jobsArgs, ctx), + ).resolves.toEqual([]); + expect(getAllJobsForQueue).toHaveBeenCalled(); + }); + + it('jobs refuses a queue belonging to another org', async () => { + const { ctx, getAllJobsForQueue, getReviewableQueuesForUser } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + ManualReviewQueue.jobs({ orgId: 'org-2', id: 'q-1' }, jobsArgs, ctx), + ).rejects.toThrow('User does not have access to this queue'); + expect(getAllJobsForQueue).not.toHaveBeenCalled(); + expect(getReviewableQueuesForUser).not.toHaveBeenCalled(); + }); + + it('pendingJobCount refuses a queue the caller cannot review', async () => { + const { ctx, getPendingJobCount } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + ManualReviewQueue.pendingJobCount( + { orgId: 'org-1', id: 'q-revoked' }, + {}, + ctx, + ), + ).rejects.toThrow('User does not have access to this queue'); + expect(getPendingJobCount).not.toHaveBeenCalled(); + }); + + it('oldestJobCreatedAt refuses a queue the caller cannot review', async () => { + const { ctx, getOldestJobCreatedAt } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + ManualReviewQueue.oldestJobCreatedAt( + { orgId: 'org-1', id: 'q-revoked' }, + {}, + ctx, + ), + ).rejects.toThrow('User does not have access to this queue'); + expect(getOldestJobCreatedAt).not.toHaveBeenCalled(); + }); + + it('explicitlyAssignedReviewers refuses a queue the caller cannot review', async () => { + const { ctx, getUsersWhoCanSeeQueue, getGraphQLUsersFromIds } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + ManualReviewQueue.explicitlyAssignedReviewers( + { orgId: 'org-1', id: 'q-revoked' }, + {}, + ctx, + ), + ).rejects.toThrow('User does not have access to this queue'); + expect(getUsersWhoCanSeeQueue).not.toHaveBeenCalled(); + expect(getGraphQLUsersFromIds).not.toHaveBeenCalled(); + }); + + it('explicitlyAssignedReviewers lists reviewers for a queue the caller can review', async () => { + const { + ctx, + getReviewableQueuesForUser, + getUsersWhoCanSeeQueue, + getGraphQLUsersFromIds, + } = makeCtx({ reviewableQueueIds: ['q-1', 'q-2'] }); + + getUsersWhoCanSeeQueue.mockResolvedValue([{ userId: 'user-2' }]); + getGraphQLUsersFromIds.mockResolvedValue([{ id: 'user-2' }]); + + await expect( + ManualReviewQueue.explicitlyAssignedReviewers( + { orgId: 'org-1', id: 'q-2' }, + {}, + ctx, + ), + ).resolves.toEqual([{ id: 'user-2' }]); + expect(getReviewableQueuesForUser).toHaveBeenCalledTimes(1); + }); + + it('hiddenActionIds refuses a queue the caller cannot review', async () => { + const { ctx, getHiddenActionsForQueue } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + ManualReviewQueue.hiddenActionIds( + { orgId: 'org-1', id: 'q-revoked' }, + {}, + ctx, + ), + ).rejects.toThrow('User does not have access to this queue'); + expect(getHiddenActionsForQueue).not.toHaveBeenCalled(); + }); + + it('hiddenActionIds returns actions for a queue the caller can review', async () => { + const { ctx, getHiddenActionsForQueue } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + ManualReviewQueue.hiddenActionIds( + { orgId: 'org-1', id: 'q-1' }, + {}, + ctx, + ), + ).resolves.toEqual(['action-1']); + expect(getHiddenActionsForQueue).toHaveBeenCalledWith({ + orgId: 'org-1', + queueId: 'q-1', + }); + }); + + it('clearReportsTriggerActionIds refuses a queue the caller cannot review', async () => { + const { ctx, getClearReportsTriggerActionsForQueue } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + ManualReviewQueue.clearReportsTriggerActionIds( + { orgId: 'org-1', id: 'q-revoked' }, + {}, + ctx, + ), + ).rejects.toThrow('User does not have access to this queue'); + expect(getClearReportsTriggerActionsForQueue).not.toHaveBeenCalled(); + }); + + it('clearReportsTriggerActionIds returns actions for a queue the caller can review', async () => { + const { + ctx, + getReviewableQueuesForUser, + getClearReportsTriggerActionsForQueue, + } = makeCtx({ reviewableQueueIds: ['q-1'] }); + + getClearReportsTriggerActionsForQueue.mockResolvedValue(['trigger-1']); + + await expect( + ManualReviewQueue.clearReportsTriggerActionIds( + { orgId: 'org-1', id: 'q-1' }, + {}, + ctx, + ), + ).resolves.toEqual(['trigger-1']); + expect(getReviewableQueuesForUser).toHaveBeenCalledTimes(1); + }); }); }); diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index 7b5290565..6540733a1 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -34,6 +34,7 @@ import { type GQLUserAppealManualReviewJobPayloadResolvers, type GQLUserManualReviewJobPayloadResolvers, } from '../generated.js'; +import { type Context } from '../resolvers.js'; import { formatItemSubmissionForGQL } from '../types.js'; import { forbiddenError, @@ -1756,12 +1757,36 @@ const NcmecManualReviewJobPayload: GQLNcmecManualReviewJobPayloadResolvers = { }, }; +async function assertQueueIsReviewable( + queue: { id: string; orgId: string }, + context: Context, +) { + const user = context.getUser(); + if (user == null) { + throw unauthenticatedError('User required.'); + } + if (user.orgId !== queue.orgId) { + throw forbiddenError('User does not have access to this queue'); + } + + const reviewableQueues = + await context.services.ManualReviewToolService.getReviewableQueuesForUser({ + invoker: { + userId: user.id, + permissions: user.getPermissions(), + orgId: user.orgId, + }, + queueIds: [queue.id], + }); + if (reviewableQueues.length === 0) { + throw forbiddenError('User does not have access to this queue'); + } + return user; +} + const ManualReviewQueue: GQLManualReviewQueueResolvers = { async jobs(queue, { ids: jobIds, limit, lockToken }, context) { - const user = context.getUser(); - if (user == null) { - throw unauthenticatedError('User required.'); - } + const user = await assertQueueIsReviewable(queue, context); const { orgId, id: queueId } = queue; @@ -1807,6 +1832,8 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { ); }, async pendingJobCount(queue, _, context) { + await assertQueueIsReviewable(queue, context); + const { orgId, id: queueId } = queue; return context.services.ManualReviewToolService.getPendingJobCount({ orgId, @@ -1814,6 +1841,8 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { }); }, async oldestJobCreatedAt(queue, _, context) { + await assertQueueIsReviewable(queue, context); + const { orgId, id: queueId } = queue; return context.services.ManualReviewToolService.getOldestJobCreatedAt({ orgId, @@ -1822,10 +1851,7 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { }); }, async explicitlyAssignedReviewers(queue, _, context) { - const user = context.getUser(); - if (user == null) { - throw unauthenticatedError('User required.'); - } + const user = await assertQueueIsReviewable(queue, context); const { id: userId, orgId } = user; const userIds = ( @@ -1838,10 +1864,7 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { return context.dataSources.userAPI.getGraphQLUsersFromIds(userIds); }, async hiddenActionIds(queue, _, context) { - const user = context.getUser(); - if (user == null) { - throw unauthenticatedError('User required.'); - } + const user = await assertQueueIsReviewable(queue, context); const { orgId } = user; const { id: queueId } = queue; @@ -1851,10 +1874,7 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { }); }, async clearReportsTriggerActionIds(queue, _, context) { - const user = context.getUser(); - if (user == null) { - throw unauthenticatedError('User required.'); - } + const user = await assertQueueIsReviewable(queue, context); return context.services.ManualReviewToolService.getClearReportsTriggerActionsForQueue( { orgId: user.orgId, @@ -2246,10 +2266,22 @@ const Query: GQLQueryResolvers = { throw unauthenticatedError('Authenticated user required'); } + const reviewableQueues = + await context.services.ManualReviewToolService.getReviewableQueuesForUser( + { + invoker: { + userId: user.id, + permissions: user.getPermissions(), + orgId: user.orgId, + }, + }, + ); + return context.services.ManualReviewToolService.getExistingJobsForItem({ orgId: user.orgId, itemId: params.itemId, itemTypeId: params.itemTypeId, + queueIds: reviewableQueues.map((queue) => queue.id), }); }, async getDecisionsTable(_, params, context) { @@ -2331,9 +2363,10 @@ const Mutation: GQLMutationResolvers = { permissions: user.getPermissions(), orgId: user.orgId, }, + queueIds: [queueId], }, ); - if (!reviewableQueues.some((queue) => queue.id === queueId)) { + if (reviewableQueues.length === 0) { throw forbiddenError('User does not have access to this queue'); } diff --git a/server/graphql/modules/org.resolver.test.ts b/server/graphql/modules/org.resolver.test.ts index 923077c8c..c28954ba4 100644 --- a/server/graphql/modules/org.resolver.test.ts +++ b/server/graphql/modules/org.resolver.test.ts @@ -320,5 +320,15 @@ describe('Org resolvers', () => { ); expect(getReviewableQueuesForUser).not.toHaveBeenCalled(); }); + + it('throws when there is no authenticated user', async () => { + const { ctx, getReviewableQueuesForUser } = makeCtx({ + callerOrgId: null, + }); + await expect(Org.mrtQueues(orgParent, {}, ctx)).rejects.toThrow( + 'User required', + ); + expect(getReviewableQueuesForUser).not.toHaveBeenCalled(); + }); }); }); diff --git a/server/services/manualReviewToolService/manualReviewToolService.ts b/server/services/manualReviewToolService/manualReviewToolService.ts index 36753b497..819344c51 100644 --- a/server/services/manualReviewToolService/manualReviewToolService.ts +++ b/server/services/manualReviewToolService/manualReviewToolService.ts @@ -1032,6 +1032,7 @@ export class ManualReviewToolService { */ async getReviewableQueuesForUser(opts: { invoker: Invoker; + queueIds?: readonly string[]; }): Promise { return this.queueOps.getReviewableQueuesForUser(opts); } @@ -1283,6 +1284,7 @@ export class ManualReviewToolService { orgId: string; itemId: string; itemTypeId: string; + queueIds: string[]; }) { return this.queueOps.getExistingJobsForItem(opts); } diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index 98882a0a9..cbb12c3e4 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -192,19 +192,17 @@ describe('QueueOperations', () => { // membership check, so any authenticated user could read (and dequeue/lock) // every queue in the org, including CSAM/NCMEC queues. They now call // getReviewableQueuesForUser instead; these lock in its filtering. - const invoker = ( - userId: string, - permissions: UserPermission[], - orgId: string, - ) => ({ invoker: { userId, permissions, orgId } }); - testWithQueueAndActions()( 'getReviewableQueuesForUser excludes a queue the user is not a member of', async ({ org, queue, mrtService, deps }) => { const { user: outsider } = await createUser(deps.KyselyPg, org.id); - const reviewable = await mrtService.getReviewableQueuesForUser( - invoker(outsider.id, [UserPermission.VIEW_MRT], org.id), - ); + const reviewable = await mrtService.getReviewableQueuesForUser({ + invoker: { + userId: outsider.id, + permissions: [UserPermission.VIEW_MRT], + orgId: org.id, + }, + }); expect(reviewable.map((q) => q.id)).not.toContain(queue.id); }, ); @@ -212,9 +210,13 @@ describe('QueueOperations', () => { testWithQueueAndActions()( 'getReviewableQueuesForUser includes a queue the user is a member of', async ({ org, queue, user, mrtService }) => { - const reviewable = await mrtService.getReviewableQueuesForUser( - invoker(user.id, [UserPermission.VIEW_MRT], org.id), - ); + const reviewable = await mrtService.getReviewableQueuesForUser({ + invoker: { + userId: user.id, + permissions: [UserPermission.VIEW_MRT], + orgId: org.id, + }, + }); expect(reviewable.map((q) => q.id)).toContain(queue.id); }, ); @@ -222,28 +224,131 @@ describe('QueueOperations', () => { testWithQueueAndActions()( 'getReviewableQueuesForUser returns nothing for a user without VIEW_MRT, even for a queue they are a member of', async ({ org, user, mrtService }) => { - const reviewable = await mrtService.getReviewableQueuesForUser( - invoker(user.id, [], org.id), - ); + const reviewable = await mrtService.getReviewableQueuesForUser({ + invoker: { userId: user.id, permissions: [], orgId: org.id }, + }); expect(reviewable).toEqual([]); }, ); testWithQueueAndActions()( - 'getReviewableQueuesForUser bypasses membership for EDIT_MRT_QUEUES holders', + 'getReviewableQueuesForUser lets EDIT_MRT_QUEUES holders view every queue', async ({ org, queue, mrtService, deps }) => { const { user: outsider } = await createUser(deps.KyselyPg, org.id); - const reviewable = await mrtService.getReviewableQueuesForUser( - invoker( - outsider.id, - [UserPermission.VIEW_MRT, UserPermission.EDIT_MRT_QUEUES], - org.id, - ), - ); + const reviewable = await mrtService.getReviewableQueuesForUser({ + invoker: { + userId: outsider.id, + permissions: [UserPermission.EDIT_MRT_QUEUES], + orgId: org.id, + }, + }); expect(reviewable.map((q) => q.id)).toContain(queue.id); }, ); + testWithQueueAndActions()( + 'getReviewableQueuesForUser filters requested queue IDs by reviewer access', + async ({ org, queue, user, mrtService, deps }) => { + const { user: outsider } = await createUser(deps.KyselyPg, org.id); + await expect( + mrtService.getReviewableQueuesForUser({ + invoker: { + userId: outsider.id, + permissions: [UserPermission.VIEW_MRT], + orgId: org.id, + }, + queueIds: [queue.id], + }), + ).resolves.toEqual([]); + await expect( + mrtService.getReviewableQueuesForUser({ + invoker: { + userId: user.id, + permissions: [UserPermission.VIEW_MRT], + orgId: org.id, + }, + queueIds: [queue.id], + }), + ).resolves.toEqual([expect.objectContaining({ id: queue.id })]); + }, + ); + + testWithQueueAndActions()( + 'getReviewableQueuesForUser lets a queue manager filter without membership', + async ({ org, queue, mrtService, deps }) => { + const { user: outsider } = await createUser(deps.KyselyPg, org.id); + await expect( + mrtService.getReviewableQueuesForUser({ + invoker: { + userId: outsider.id, + permissions: [UserPermission.EDIT_MRT_QUEUES], + orgId: org.id, + }, + queueIds: [queue.id], + }), + ).resolves.toEqual([expect.objectContaining({ id: queue.id })]); + }, + ); + + testWithQueueAndActions()( + 'getExistingJobsForItem is scoped to the given queue IDs', + async ({ org, queue, user, mrtService, kyselyPg }) => { + const jobPayload = makeDummyMrtJobPayload(); + await mrtService['queueOps']['addJob']({ + orgId: org.id, + queueId: queue.id, + enqueueSourceInfo: { kind: 'REPORT' }, + jobPayload, + }); + const itemId = jobPayload.payload.item.itemId; + const itemTypeId = jobPayload.payload.item.itemTypeIdentifier.id; + await kyselyPg + .insertInto('manual_review_tool.job_creations') + .values({ + id: bullJobIdtoExternalJobId( + itemIdToBullJobId({ id: itemId, typeId: itemTypeId }), + ), + org_id: org.id, + item_id: itemId, + item_type_id: itemTypeId, + queue_id: queue.id, + created_at: new Date(), + enqueue_source_info: {}, + }) + .execute(); + + const inQueue = await mrtService.getExistingJobsForItem({ + orgId: org.id, + itemId, + itemTypeId, + queueIds: [queue.id], + }); + expect(inQueue.map((it) => it.queueId)).toEqual([queue.id]); + + const { queue: otherQueue } = await createMrtQueue({ + orgId: org.id, + mrtService, + userId: user.id, + name: `other-queue-${uid()}`, + }); + const otherQueueOnly = await mrtService.getExistingJobsForItem({ + orgId: org.id, + itemId, + itemTypeId, + queueIds: [otherQueue.id], + }); + expect(otherQueueOnly).toEqual([]); + + const noQueues = await mrtService.getExistingJobsForItem({ + orgId: org.id, + itemId, + itemTypeId, + queueIds: [], + }); + expect(noQueues).toEqual([]); + }, + ); + // Regression: `deleteAllJobsFromQueue` is irreversible and used to accept // EDIT_MRT_QUEUES (held by moderator managers) -- that gap accidentally // cleared a production queue. It now requires MANAGE_ORG. diff --git a/server/services/manualReviewToolService/modules/QueueOperations.ts b/server/services/manualReviewToolService/modules/QueueOperations.ts index 640e7e205..de613c061 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.ts @@ -596,16 +596,20 @@ export default class QueueOperations { return queue.id; } - async getReviewableQueuesForUser(opts: { invoker: Invoker }) { - const { invoker } = opts; + async getReviewableQueuesForUser(opts: { + invoker: Invoker; + queueIds?: readonly string[]; + }) { + const { invoker, queueIds } = opts; const { userId, permissions, orgId } = invoker; - const canSeeQueues = permissions.includes(UserPermission.VIEW_MRT); const bypassQueuePermissions = permissions.includes( UserPermission.EDIT_MRT_QUEUES, ); + const canSeeQueues = + permissions.includes(UserPermission.VIEW_MRT) || bypassQueuePermissions; - if (!canSeeQueues) { + if (!canSeeQueues || queueIds?.length === 0) { return []; } @@ -613,6 +617,7 @@ export default class QueueOperations { .selectFrom('manual_review_tool.manual_review_queues') .select(PgQueueSelection) .where('org_id', '=', orgId) + .$if(queueIds != null, (query) => query.where('id', 'in', queueIds ?? [])) .$if(!bypassQueuePermissions, (query) => query.where( 'id', @@ -1723,8 +1728,12 @@ export default class QueueOperations { orgId: string; itemId: string; itemTypeId: string; + queueIds: string[]; }) { - const { orgId, itemId, itemTypeId } = opts; + const { orgId, itemId, itemTypeId, queueIds } = opts; + if (queueIds.length === 0) { + return []; + } // Check postgres for creations within the last 7 days so we don't have to // search every bull queue for every item. const recentJobCreationQueues = await this.pgQuery @@ -1734,6 +1743,7 @@ export default class QueueOperations { .where('item_id', '=', itemId) .where('item_type_id', '=', itemTypeId) .where('created_at', '>=', new Date(Date.now() - WEEK_MS)) + .where('queue_id', 'in', queueIds) .execute(); const jobsWithQueue = await Promise.all( recentJobCreationQueues.map(async (rows) => { diff --git a/server/test/fixtureHelpers/createMrtQueue.ts b/server/test/fixtureHelpers/createMrtQueue.ts index 551ca8167..0379165c5 100644 --- a/server/test/fixtureHelpers/createMrtQueue.ts +++ b/server/test/fixtureHelpers/createMrtQueue.ts @@ -5,11 +5,12 @@ export default async function (opts: { orgId: string; mrtService: Dependencies['ManualReviewToolService']; userId: string; + name?: string; }) { - const { orgId, mrtService, userId } = opts; + const { orgId, mrtService, userId, name = 'test-queue' } = opts; const queue = await mrtService.createManualReviewQueue({ - name: 'test-queue', + name, description: null, userIds: [userId], hiddenActionIds: [], From 54d91329609b929c242bcb0a116f5bc151a62fa1 Mon Sep 17 00:00:00 2001 From: Juan Mrad Date: Sun, 20 Sep 2026 21:47:59 -0500 Subject: [PATCH 5/7] address code review --- CHANGELOG.md | 1 + .../modules/manualReviewTool.resolver.test.ts | 99 ++++++++++++++++++- server/graphql/modules/manualReviewTool.ts | 63 +++++++++--- .../modules/QueueOperations.test.ts | 6 +- 4 files changed, 146 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40d847a20..7f3ddc412 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ For more information about each release including git tags and artifacts, see [R ### Security +- Review queue and job access control hardening ([#1151](https://github.com/roostorg/coop/pull/1151) by [@serendipty01](https://github.com/serendipty01) and [@cassidyjames](https://github.com/cassidyjames)) - Passwords are hashed with Argon2id instead of bcrypt at cost factor 5 ([#901](https://github.com/roostorg/coop/pull/901) by [@serendipty01](https://github.com/serendipty01), closes [#900](https://github.com/roostorg/coop/issues/900)) - Minimum password length raised to 15 and enforced server-side ([#1065](https://github.com/roostorg/coop/pull/1065), [#1094](https://github.com/roostorg/coop/pull/1094) by [@serendipty01](https://github.com/serendipty01)) diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index d098f5beb..b23da54f7 100644 --- a/server/graphql/modules/manualReviewTool.resolver.test.ts +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -164,19 +164,39 @@ describe('MRT queue/job resolvers are membership-scoped', () => { describe('Query.manualReviewQueue', () => { it('returns a queue the caller can review', async () => { - const { ctx } = makeCtx({ reviewableQueueIds: ['q-1', 'q-2'] }); + const { ctx, getReviewableQueuesForUser } = makeCtx({ + reviewableQueueIds: ['q-1', 'q-2'], + }); await expect( Query.manualReviewQueue({}, { id: 'q-2' }, ctx), ).resolves.toMatchObject({ id: 'q-2' }); + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'user-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + queueIds: ['q-2'], + }); }); it('returns null for a queue the caller is not a member of', async () => { - const { ctx, getQueueForOrgAndDangerouslyBypassPermissioning } = makeCtx({ - reviewableQueueIds: ['q-1'], - }); + const { + ctx, + getReviewableQueuesForUser, + getQueueForOrgAndDangerouslyBypassPermissioning, + } = makeCtx({ reviewableQueueIds: ['q-1'] }); await expect( Query.manualReviewQueue({}, { id: 'q-forbidden' }, ctx), ).resolves.toBeNull(); + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'user-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + queueIds: ['q-forbidden'], + }); expect( getQueueForOrgAndDangerouslyBypassPermissioning, ).not.toHaveBeenCalled(); @@ -393,6 +413,77 @@ describe('MRT queue/job resolvers are membership-scoped', () => { expect(getOldestJobCreatedAt).not.toHaveBeenCalled(); }); + it('batches queue authorization across concurrent fields', async () => { + const { ctx, getReviewableQueuesForUser } = makeCtx({ + reviewableQueueIds: ['q-1', 'q-2'], + }); + + await Promise.all([ + ManualReviewQueue.jobs({ orgId: 'org-1', id: 'q-1' }, jobsArgs, ctx), + ManualReviewQueue.pendingJobCount( + { orgId: 'org-1', id: 'q-1' }, + {}, + ctx, + ), + ManualReviewQueue.oldestJobCreatedAt( + { orgId: 'org-1', id: 'q-2' }, + {}, + ctx, + ), + ]); + + expect(getReviewableQueuesForUser).toHaveBeenCalledTimes(1); + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'user-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + queueIds: ['q-1', 'q-2'], + }); + }); + + it('preserves allowed and denied results within one authorization batch', async () => { + const { + ctx, + getReviewableQueuesForUser, + getPendingJobCount, + getOldestJobCreatedAt, + } = makeCtx({ reviewableQueueIds: ['q-allowed'] }); + + const results = await Promise.allSettled([ + ManualReviewQueue.pendingJobCount( + { orgId: 'org-1', id: 'q-allowed' }, + {}, + ctx, + ), + ManualReviewQueue.oldestJobCreatedAt( + { orgId: 'org-1', id: 'q-denied' }, + {}, + ctx, + ), + ]); + + expect(results[0]).toMatchObject({ status: 'fulfilled', value: 3 }); + expect(results[1]).toMatchObject({ + status: 'rejected', + reason: expect.objectContaining({ + message: 'User does not have access to this queue', + }), + }); + expect(getReviewableQueuesForUser).toHaveBeenCalledTimes(1); + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'user-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + queueIds: ['q-allowed', 'q-denied'], + }); + expect(getPendingJobCount).toHaveBeenCalled(); + expect(getOldestJobCreatedAt).not.toHaveBeenCalled(); + }); + it('explicitlyAssignedReviewers refuses a queue the caller cannot review', async () => { const { ctx, getUsersWhoCanSeeQueue, getGraphQLUsersFromIds } = makeCtx({ reviewableQueueIds: ['q-1'], diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index 6540733a1..93c11e8c5 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -1,4 +1,5 @@ /* eslint-disable max-lines */ +import DataLoader from 'dataloader'; import _ from 'lodash'; import { itemSubmissionWithTypeIdentifierToItemSubmission } from '../../services/itemProcessingService/index.js'; @@ -1757,6 +1758,47 @@ const NcmecManualReviewJobPayload: GQLNcmecManualReviewJobPayloadResolvers = { }, }; +const queueReviewabilityLoaders = new WeakMap< + Context, + DataLoader +>(); + +function getQueueReviewabilityLoader(context: Context) { + const existing = queueReviewabilityLoaders.get(context); + if (existing != null) { + return existing; + } + + const user = context.getUser(); + if (user == null) { + throw unauthenticatedError('User required.'); + } + + const loader = new DataLoader( + async (queueIds) => { + const uniqueQueueIds = [...new Set(queueIds)]; + const reviewableQueues = + await context.services.ManualReviewToolService.getReviewableQueuesForUser( + { + invoker: { + userId: user.id, + permissions: user.getPermissions(), + orgId: user.orgId, + }, + queueIds: uniqueQueueIds, + }, + ); + const reviewableQueueIds = new Set( + reviewableQueues.map((queue) => queue.id), + ); + return queueIds.map((queueId) => reviewableQueueIds.has(queueId)); + }, + { cache: false }, + ); + queueReviewabilityLoaders.set(context, loader); + return loader; +} + async function assertQueueIsReviewable( queue: { id: string; orgId: string }, context: Context, @@ -1765,20 +1807,10 @@ async function assertQueueIsReviewable( if (user == null) { throw unauthenticatedError('User required.'); } - if (user.orgId !== queue.orgId) { - throw forbiddenError('User does not have access to this queue'); - } - - const reviewableQueues = - await context.services.ManualReviewToolService.getReviewableQueuesForUser({ - invoker: { - userId: user.id, - permissions: user.getPermissions(), - orgId: user.orgId, - }, - queueIds: [queue.id], - }); - if (reviewableQueues.length === 0) { + if ( + user.orgId !== queue.orgId || + !(await getQueueReviewabilityLoader(context).load(queue.id)) + ) { throw forbiddenError('User does not have access to this queue'); } return user; @@ -2246,9 +2278,10 @@ const Query: GQLQueryResolvers = { permissions: user.getPermissions(), orgId: user.orgId, }, + queueIds: [id], }, ); - return reviewableQueues.find((queue) => queue.id === id) ?? null; + return reviewableQueues[0] ?? null; }, async getCommentsForJob(_: unknown, { jobId }, context) { const user = context.getUser(); diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index cbb12c3e4..7cf62fa0a 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -188,10 +188,8 @@ describe('QueueOperations', () => { }, ); - // queues via *Dangerously*BypassPermissioning helpers with no permission or - // membership check, so any authenticated user could read (and dequeue/lock) - // every queue in the org, including CSAM/NCMEC queues. They now call - // getReviewableQueuesForUser instead; these lock in its filtering. + // These operations previously bypassed queue permissions, allowing any + // authenticated user to read and dequeue jobs from every queue in the org. testWithQueueAndActions()( 'getReviewableQueuesForUser excludes a queue the user is not a member of', async ({ org, queue, mrtService, deps }) => { From 1bc40447498455c539ce964d5808a52bcc8df315 Mon Sep 17 00:00:00 2001 From: Juan Mrad Date: Sun, 20 Sep 2026 22:31:43 -0500 Subject: [PATCH 6/7] improved import. favorite bypass fix --- .../modules/manualReviewTool.resolver.test.ts | 21 ++- server/graphql/modules/manualReviewTool.ts | 63 +------- server/graphql/modules/user.resolver.test.ts | 136 ++++++++++++++++++ server/graphql/modules/user.ts | 45 ++++-- .../utils/manualReviewQueueAuthorization.ts | 62 ++++++++ 5 files changed, 258 insertions(+), 69 deletions(-) create mode 100644 server/graphql/utils/manualReviewQueueAuthorization.ts diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index b23da54f7..755919bc0 100644 --- a/server/graphql/modules/manualReviewTool.resolver.test.ts +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -12,7 +12,7 @@ const Query = resolvers.Query as Record< ResolverFn >; const Mutation = resolvers.Mutation as Record< - 'dequeueManualReviewJob', + 'dequeueManualReviewJob' | 'submitManualReviewDecision', ResolverFn >; const ManualReviewQueue = resolvers.ManualReviewQueue as Record< @@ -54,6 +54,7 @@ function makeCtx(opts: { }); const getTotalPendingJobCountForQueues = jest.fn(async () => 7); const dequeueNextJob = jest.fn(async () => null); + const submitDecision = jest.fn(async () => ({ warnings: [] })); const getAllJobsForQueue = jest.fn(async () => []); const getJobsForQueue = jest.fn(async () => []); const getExistingJobsForItem = jest.fn(async () => []); @@ -86,6 +87,7 @@ function makeCtx(opts: { getQueueForOrgAndDangerouslyBypassPermissioning, getTotalPendingJobCountForQueues, dequeueNextJob, + submitDecision, getAllJobsForQueue, getJobsForQueue, getExistingJobsForItem, @@ -108,6 +110,7 @@ function makeCtx(opts: { getQueueForOrgAndDangerouslyBypassPermissioning, getTotalPendingJobCountForQueues, dequeueNextJob, + submitDecision, getAllJobsForQueue, getJobsForQueue, getExistingJobsForItem, @@ -318,6 +321,22 @@ describe('MRT queue/job resolvers are membership-scoped', () => { }); }); + describe('Mutation.submitManualReviewDecision', () => { + it('rejects a decision after queue access is revoked', async () => { + const { ctx, submitDecision } = makeCtx({ + reviewableQueueIds: [], + }); + await expect( + Mutation.submitManualReviewDecision( + {}, + { input: { queueId: 'q-revoked' } }, + ctx, + ), + ).rejects.toThrow('User does not have access to this queue'); + expect(submitDecision).not.toHaveBeenCalled(); + }); + }); + describe('ManualReviewQueue queue-scoped fields authorize their parent', () => { const jobsArgs = { ids: null, limit: null }; diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index 93c11e8c5..75ceb6cf7 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -1,5 +1,4 @@ /* eslint-disable max-lines */ -import DataLoader from 'dataloader'; import _ from 'lodash'; import { itemSubmissionWithTypeIdentifierToItemSubmission } from '../../services/itemProcessingService/index.js'; @@ -35,7 +34,6 @@ import { type GQLUserAppealManualReviewJobPayloadResolvers, type GQLUserManualReviewJobPayloadResolvers, } from '../generated.js'; -import { type Context } from '../resolvers.js'; import { formatItemSubmissionForGQL } from '../types.js'; import { forbiddenError, @@ -44,6 +42,7 @@ import { } from '../utils/errors.js'; import { gqlErrorResult, gqlSuccessResult } from '../utils/gqlResult.js'; import { oneOfInputToTaggedUnion } from '../utils/inputHelpers.js'; +import { assertQueueIsReviewable } from '../utils/manualReviewQueueAuthorization.js'; const { omit, sumBy } = _; @@ -1758,64 +1757,6 @@ const NcmecManualReviewJobPayload: GQLNcmecManualReviewJobPayloadResolvers = { }, }; -const queueReviewabilityLoaders = new WeakMap< - Context, - DataLoader ->(); - -function getQueueReviewabilityLoader(context: Context) { - const existing = queueReviewabilityLoaders.get(context); - if (existing != null) { - return existing; - } - - const user = context.getUser(); - if (user == null) { - throw unauthenticatedError('User required.'); - } - - const loader = new DataLoader( - async (queueIds) => { - const uniqueQueueIds = [...new Set(queueIds)]; - const reviewableQueues = - await context.services.ManualReviewToolService.getReviewableQueuesForUser( - { - invoker: { - userId: user.id, - permissions: user.getPermissions(), - orgId: user.orgId, - }, - queueIds: uniqueQueueIds, - }, - ); - const reviewableQueueIds = new Set( - reviewableQueues.map((queue) => queue.id), - ); - return queueIds.map((queueId) => reviewableQueueIds.has(queueId)); - }, - { cache: false }, - ); - queueReviewabilityLoaders.set(context, loader); - return loader; -} - -async function assertQueueIsReviewable( - queue: { id: string; orgId: string }, - context: Context, -) { - const user = context.getUser(); - if (user == null) { - throw unauthenticatedError('User required.'); - } - if ( - user.orgId !== queue.orgId || - !(await getQueueReviewabilityLoader(context).load(queue.id)) - ) { - throw forbiddenError('User does not have access to this queue'); - } - return user; -} - const ManualReviewQueue: GQLManualReviewQueueResolvers = { async jobs(queue, { ids: jobIds, limit, lockToken }, context) { const user = await assertQueueIsReviewable(queue, context); @@ -2442,6 +2383,8 @@ const Mutation: GQLMutationResolvers = { reportHistory, } = params.input; + await assertQueueIsReviewable({ id: queueId, orgId }, context); + const decisionPayloads = reportedItemDecisionComponents.map( (reportedItemDecisionComponent) => { const decision = oneOfInputToTaggedUnion( diff --git a/server/graphql/modules/user.resolver.test.ts b/server/graphql/modules/user.resolver.test.ts index c108d9fa8..c002bba5f 100644 --- a/server/graphql/modules/user.resolver.test.ts +++ b/server/graphql/modules/user.resolver.test.ts @@ -161,4 +161,140 @@ describe('user resolvers', () => { expect(getPublicSigningKeyPem).not.toHaveBeenCalled(); }); }); + + describe('MRT favorites respect the caller queue access', () => { + const caller = { + id: 'caller-1', + orgId: 'org-1', + getPermissions: () => [UserPermission.VIEW_MRT], + }; + const favoriteQueues = [ + { id: 'q-allowed', orgId: 'org-1', name: 'Allowed' }, + { id: 'q-denied', orgId: 'org-1', name: 'Denied' }, + ]; + + function makeCtx(opts?: { reviewableQueueIds?: string[]; user?: null }) { + const reviewableQueueIds = opts?.reviewableQueueIds ?? ['q-allowed']; + const getFavoriteQueuesForUser = jest.fn(async () => favoriteQueues); + const getReviewableQueuesForUser = jest.fn( + async ({ queueIds }: { queueIds?: readonly string[] }) => + favoriteQueues.filter( + (queue) => + reviewableQueueIds.includes(queue.id) && + (queueIds == null || queueIds.includes(queue.id)), + ), + ); + const addFavoriteQueueForUser = jest.fn(async () => undefined); + const ctx = { + getUser: () => (opts?.user === null ? null : caller), + services: { + ManualReviewToolService: { + getFavoriteQueuesForUser, + getReviewableQueuesForUser, + addFavoriteQueueForUser, + }, + }, + }; + return { + ctx, + getFavoriteQueuesForUser, + getReviewableQueuesForUser, + addFavoriteQueueForUser, + }; + } + + const User = resolvers.User as { + favoriteMRTQueues: ( + parent: { id: string; orgId: string }, + args: unknown, + ctx: unknown, + ) => Promise; + reviewableQueues: ( + parent: unknown, + args: { queueIds?: string[] | null }, + ctx: unknown, + ) => Promise; + }; + const Mutation = resolvers.Mutation as { + addFavoriteMRTQueue: ( + parent: unknown, + args: { queueId: string }, + ctx: unknown, + ) => Promise; + }; + + it("filters the caller's stale favorites through their reviewable queues", async () => { + const { ctx, getFavoriteQueuesForUser, getReviewableQueuesForUser } = + makeCtx(); + + await expect( + User.favoriteMRTQueues({ id: 'caller-1', orgId: 'org-1' }, {}, ctx), + ).resolves.toEqual([favoriteQueues[0]]); + expect(getFavoriteQueuesForUser).toHaveBeenCalledWith({ + userId: 'caller-1', + orgId: 'org-1', + }); + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'caller-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + queueIds: ['q-allowed', 'q-denied'], + }); + }); + + it("rejects another user's favorites", async () => { + const { ctx, getFavoriteQueuesForUser } = makeCtx(); + await expect( + User.favoriteMRTQueues({ id: 'other-user', orgId: 'org-1' }, {}, ctx), + ).rejects.toThrow('User does not have access to these queues'); + expect(getFavoriteQueuesForUser).not.toHaveBeenCalled(); + }); + + it('rejects favorites from another organization', async () => { + const { ctx, getFavoriteQueuesForUser } = makeCtx(); + await expect( + User.favoriteMRTQueues({ id: 'other-user', orgId: 'org-2' }, {}, ctx), + ).rejects.toThrow('User does not have access to these queues'); + expect(getFavoriteQueuesForUser).not.toHaveBeenCalled(); + }); + + it('rejects favoriting a queue the caller cannot review', async () => { + const { ctx, addFavoriteQueueForUser } = makeCtx(); + await expect( + Mutation.addFavoriteMRTQueue({}, { queueId: 'q-denied' }, ctx), + ).rejects.toThrow('User does not have access to this queue'); + expect(addFavoriteQueueForUser).not.toHaveBeenCalled(); + }); + + it('allows favoriting a queue the caller can review', async () => { + const { ctx, addFavoriteQueueForUser } = makeCtx(); + await expect( + Mutation.addFavoriteMRTQueue({}, { queueId: 'q-allowed' }, ctx), + ).resolves.toBeDefined(); + expect(addFavoriteQueueForUser).toHaveBeenCalledWith({ + userId: 'caller-1', + orgId: 'org-1', + queueId: 'q-allowed', + }); + }); + + it('passes requested queue IDs into the reviewable queue lookup', async () => { + const { ctx, getReviewableQueuesForUser } = makeCtx({ + reviewableQueueIds: ['q-allowed', 'q-denied'], + }); + await expect( + User.reviewableQueues({}, { queueIds: ['q-allowed'] }, ctx), + ).resolves.toEqual([favoriteQueues[0]]); + expect(getReviewableQueuesForUser).toHaveBeenCalledWith({ + invoker: { + userId: 'caller-1', + permissions: [UserPermission.VIEW_MRT], + orgId: 'org-1', + }, + queueIds: ['q-allowed'], + }); + }); + }); }); diff --git a/server/graphql/modules/user.ts b/server/graphql/modules/user.ts index edd51e9d0..ab0c5ee89 100644 --- a/server/graphql/modules/user.ts +++ b/server/graphql/modules/user.ts @@ -10,6 +10,7 @@ import { } from '../generated.js'; import { forbiddenError, unauthenticatedError } from '../utils/errors.js'; import { gqlSuccessResult } from '../utils/gqlResult.js'; +import { assertQueueIsReviewable } from '../utils/manualReviewQueueAuthorization.js'; const typeDefs = /* GraphQL */ ` enum UserRole { @@ -264,6 +265,10 @@ const Mutation: GQLMutationResolvers = { if (user == null) { throw unauthenticatedError('User required.'); } + await assertQueueIsReviewable( + { id: params.queueId, orgId: user.orgId }, + context, + ); await context.services.ManualReviewToolService.addFavoriteQueueForUser({ userId: user.id, orgId: user.orgId, @@ -412,10 +417,37 @@ const User: GQLUserResolvers = { }; }, async favoriteMRTQueues(user, _, context) { - return context.services.ManualReviewToolService.getFavoriteQueuesForUser({ - userId: user.id, - orgId: user.orgId, - }); + const caller = context.getUser(); + if (caller == null) { + throw unauthenticatedError('User required.'); + } + if (caller.id !== user.id || caller.orgId !== user.orgId) { + throw forbiddenError('User does not have access to these queues'); + } + + const favorites = + await context.services.ManualReviewToolService.getFavoriteQueuesForUser({ + userId: user.id, + orgId: user.orgId, + }); + if (favorites.length === 0) { + return []; + } + const reviewableQueues = + await context.services.ManualReviewToolService.getReviewableQueuesForUser( + { + invoker: { + userId: caller.id, + permissions: caller.getPermissions(), + orgId: caller.orgId, + }, + queueIds: favorites.map((queue) => queue.id), + }, + ); + const reviewableQueueIds = new Set( + reviewableQueues.map((queue) => queue.id), + ); + return favorites.filter((queue) => reviewableQueueIds.has(queue.id)); }, async reviewableQueues(_, { queueIds }, context) { const user = context.getUser(); @@ -431,13 +463,10 @@ const User: GQLUserResolvers = { permissions: user.getPermissions(), orgId: user.orgId, }, + queueIds: queueIds ?? undefined, }, ); - if (queueIds) { - return queues.filter((it) => queueIds.includes(it.id)); - } - return queues; }, }; diff --git a/server/graphql/utils/manualReviewQueueAuthorization.ts b/server/graphql/utils/manualReviewQueueAuthorization.ts new file mode 100644 index 000000000..ce1a1d3ba --- /dev/null +++ b/server/graphql/utils/manualReviewQueueAuthorization.ts @@ -0,0 +1,62 @@ +import DataLoader from 'dataloader'; + +import { type Context } from '../resolvers.js'; +import { forbiddenError, unauthenticatedError } from './errors.js'; + +const queueReviewabilityLoaders = new WeakMap< + Context, + DataLoader +>(); + +function getQueueReviewabilityLoader(context: Context) { + const existing = queueReviewabilityLoaders.get(context); + if (existing != null) { + return existing; + } + + const user = context.getUser(); + if (user == null) { + throw unauthenticatedError('User required.'); + } + + const loader = new DataLoader( + async (queueIds) => { + const uniqueQueueIds = [...new Set(queueIds)]; + const reviewableQueues = + await context.services.ManualReviewToolService.getReviewableQueuesForUser( + { + invoker: { + userId: user.id, + permissions: user.getPermissions(), + orgId: user.orgId, + }, + queueIds: uniqueQueueIds, + }, + ); + const reviewableQueueIds = new Set( + reviewableQueues.map((queue) => queue.id), + ); + return queueIds.map((queueId) => reviewableQueueIds.has(queueId)); + }, + { cache: false }, + ); + queueReviewabilityLoaders.set(context, loader); + return loader; +} + +export async function assertQueueIsReviewable( + queue: { id: string; orgId: string }, + context: Context, +) { + const user = context.getUser(); + if (user == null) { + throw unauthenticatedError('User required.'); + } + if ( + user.orgId !== queue.orgId || + !(await getQueueReviewabilityLoader(context).load(queue.id)) + ) { + throw forbiddenError('User does not have access to this queue'); + } + return user; +} From 40cfa8c03b637e06c3dcc1ee9e76a5e5cdf1bcc5 Mon Sep 17 00:00:00 2001 From: Juan Mrad Date: Sun, 20 Sep 2026 22:47:27 -0500 Subject: [PATCH 7/7] address cubic review comments --- .../modules/manualReviewTool.resolver.test.ts | 52 +++++++++++++++++++ server/graphql/modules/user.resolver.test.ts | 24 +++++++++ 2 files changed, 76 insertions(+) diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index 755919bc0..16536e543 100644 --- a/server/graphql/modules/manualReviewTool.resolver.test.ts +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -31,6 +31,7 @@ function makeCtx(opts: { id: string; orgId: string; permissions: readonly UserPermission[]; + email?: string; } | null; }) { const user = @@ -78,6 +79,7 @@ function makeCtx(opts: { : { id: user.id, orgId: user.orgId, + email: user.email ?? 'user@example.com', getPermissions: () => user.permissions, }, services: { @@ -322,6 +324,17 @@ describe('MRT queue/job resolvers are membership-scoped', () => { }); describe('Mutation.submitManualReviewDecision', () => { + it('throws when there is no authenticated user', async () => { + const { ctx, submitDecision } = makeCtx({ + reviewableQueueIds: [], + user: null, + }); + await expect( + Mutation.submitManualReviewDecision({}, { input: {} }, ctx), + ).rejects.toThrow('User required.'); + expect(submitDecision).not.toHaveBeenCalled(); + }); + it('rejects a decision after queue access is revoked', async () => { const { ctx, submitDecision } = makeCtx({ reviewableQueueIds: [], @@ -335,6 +348,45 @@ describe('MRT queue/job resolvers are membership-scoped', () => { ).rejects.toThrow('User does not have access to this queue'); expect(submitDecision).not.toHaveBeenCalled(); }); + + it('submits a decision for a queue the caller can review', async () => { + const { ctx, submitDecision } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + await expect( + Mutation.submitManualReviewDecision( + {}, + { + input: { + queueId: 'q-1', + jobId: 'job-1', + lockToken: 'lock-1', + reportedItemDecisionComponents: [{ ignore: { _: true } }], + relatedItemActions: [], + reportHistory: [], + decisionReason: null, + }, + }, + ctx, + ), + ).resolves.toEqual({ + __typename: 'SubmitDecisionSuccessResponse', + success: true, + warnings: [], + }); + expect(submitDecision).toHaveBeenCalledWith({ + reportHistory: [], + queueId: 'q-1', + jobId: 'job-1', + lockToken: 'lock-1', + decisionComponents: [{ type: 'IGNORE' }], + relatedActions: [], + reviewerId: 'user-1', + reviewerEmail: 'user@example.com', + orgId: 'org-1', + decisionReason: undefined, + }); + }); }); describe('ManualReviewQueue queue-scoped fields authorize their parent', () => { diff --git a/server/graphql/modules/user.resolver.test.ts b/server/graphql/modules/user.resolver.test.ts index 39b3363e8..2497e0e41 100644 --- a/server/graphql/modules/user.resolver.test.ts +++ b/server/graphql/modules/user.resolver.test.ts @@ -113,6 +113,30 @@ describe('user resolvers', () => { ) => Promise; }; + it('rejects unauthenticated favorite queue reads', async () => { + const { ctx, getFavoriteQueuesForUser } = makeCtx({ user: null }); + await expect( + User.favoriteMRTQueues({ id: 'caller-1', orgId: 'org-1' }, {}, ctx), + ).rejects.toThrow('User required.'); + expect(getFavoriteQueuesForUser).not.toHaveBeenCalled(); + }); + + it('rejects unauthenticated favorite queue writes', async () => { + const { ctx, addFavoriteQueueForUser } = makeCtx({ user: null }); + await expect( + Mutation.addFavoriteMRTQueue({}, { queueId: 'q-allowed' }, ctx), + ).rejects.toThrow('User required.'); + expect(addFavoriteQueueForUser).not.toHaveBeenCalled(); + }); + + it('rejects unauthenticated reviewable queue reads', async () => { + const { ctx, getReviewableQueuesForUser } = makeCtx({ user: null }); + await expect( + User.reviewableQueues({}, { queueIds: null }, ctx), + ).rejects.toThrow('Authenticated user required'); + expect(getReviewableQueuesForUser).not.toHaveBeenCalled(); + }); + it("filters the caller's stale favorites through their reviewable queues", async () => { const { ctx, getFavoriteQueuesForUser, getReviewableQueuesForUser } = makeCtx();