diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b315c3b6..b50ac1572 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ For more information about each release including git tags and artifacts, see [R - Scylla healthcheck probing gossip instead of CQL readiness ([#1041](https://github.com/roostorg/coop/pull/1041) by [@reitblatt](https://github.com/reitblatt)) - Podman Compose setup failing to resolve the backend ([#981](https://github.com/roostorg/coop/pull/981) by [@juanmrad](https://github.com/juanmrad)) - DB Migrator rejecting non-standard Scylla ports ([#878](https://github.com/roostorg/coop/pull/878) by [@jess-upscrolled](https://github.com/jess-upscrolled)) +- 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)) ### Security diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts new file mode 100644 index 000000000..70581e002 --- /dev/null +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -0,0 +1,539 @@ +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' | 'getExistingJobsForItem', + ResolverFn +>; +const Mutation = resolvers.Mutation as Record< + 'dequeueManualReviewJob', + ResolverFn +>; +const ManualReviewQueue = resolvers.ManualReviewQueue as Record< + | 'jobs' + | 'pendingJobCount' + | 'oldestJobCreatedAt' + | 'explicitlyAssignedReviewers' + | 'hiddenActionIds' + | 'clearReportsTriggerActionIds', + 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 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: () => + user == null + ? null + : { + id: user.id, + orgId: user.orgId, + getPermissions: () => user.permissions, + }, + services: { + ManualReviewToolService: { + getReviewableQueuesForUser, + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + getQueueForOrgAndDangerouslyBypassPermissioning, + getTotalPendingJobCountForQueues, + dequeueNextJob, + getAllJobsForQueue, + getJobsForQueue, + getExistingJobsForItem, + getPendingJobCount, + getOldestJobCreatedAt, + getUsersWhoCanSeeQueue, + getHiddenActionsForQueue, + getClearReportsTriggerActionsForQueue, + }, + }, + dataSources: { + userAPI: { getGraphQLUsersFromIds }, + }, + }; + + return { + ctx, + getReviewableQueuesForUser, + getAllQueuesForOrgAndDangerouslyBypassPermissioning, + getQueueForOrgAndDangerouslyBypassPermissioning, + getTotalPendingJobCountForQueues, + dequeueNextJob, + getAllJobsForQueue, + getJobsForQueue, + getExistingJobsForItem, + getPendingJobCount, + getOldestJobCreatedAt, + getUsersWhoCanSeeQueue, + getHiddenActionsForQueue, + getClearReportsTriggerActionsForQueue, + getGraphQLUsersFromIds, + }; +} + +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('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(); + }); + + it('shares the per-request reviewable-queue lookup with queue fields', async () => { + const { ctx, getReviewableQueuesForUser } = makeCtx({ + reviewableQueueIds: ['q-1'], + }); + + await Promise.all([ + Query.getExistingJobsForItem( + {}, + { itemId: 'item-1', itemTypeId: 'content' }, + ctx, + ), + ManualReviewQueue.pendingJobCount( + { orgId: 'org-1', id: 'q-1' }, + {}, + ctx, + ), + ]); + + expect(getReviewableQueuesForUser).toHaveBeenCalledTimes(1); + }); + }); + + 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 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(); + }); + + // A queue stays in users_and_favorite_mrt_queues after access is revoked, + // so `me { favoriteMRTQueues { jobs } }` hands this resolver a queue the + // caller can no longer review. + 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-revoked' }, + jobsArgs, + ctx, + ), + ).rejects.toThrow('User does not have access to this queue'); + expect(getAllJobsForQueue).not.toHaveBeenCalled(); + }); + + // RoutingRule.destinationQueue hands back a queue for EDIT_MRT_QUEUES + // holders, but getReviewableQueuesForUser returns nothing without + // VIEW_MRT -- so that combination must not reach jobs. + it('jobs refuses a caller with EDIT_MRT_QUEUES but no VIEW_MRT', async () => { + const { ctx, getAllJobsForQueue } = makeCtx({ + reviewableQueueIds: [], + 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), + ).rejects.toThrow('User does not have access to this queue'); + expect(getAllJobsForQueue).not.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(); + }); + + // The dashboard asks for these fields on every queue at once; the lookup is + // memoized per request so that stays one query rather than one per queue. + it('looks up reviewable queues once per request across fields and queues', 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); + }); + + // These fields re-check the parent queue just like jobs/pendingJobCount/ + // oldestJobCreatedAt: a revoked member keeps the queue in + // `me.favoriteMRTQueues`, so it can still reach these resolvers. + 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 f76fefe89..197b39360 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -14,6 +14,7 @@ import { getEndOfDayInTimezone, getStartOfDayInTimezone, } from '../../utils/time.js'; +import { type GraphQLUserParent } from '../datasources/userKyselyPersistence.js'; import { type GQLContentAppealManualReviewJobPayloadResolvers, type GQLContentManualReviewJobPayloadResolvers, @@ -34,6 +35,7 @@ import { type GQLUserAppealManualReviewJobPayloadResolvers, type GQLUserManualReviewJobPayloadResolvers, } from '../generated.js'; +import { type Context } from '../resolvers.js'; import { formatItemSubmissionForGQL } from '../types.js'; import { forbiddenError, @@ -1746,8 +1748,73 @@ const NcmecManualReviewJobPayload: GQLNcmecManualReviewJobPayloadResolvers = { }, }; +/** + * The queue-scoped fields below each resolve one queue at a time, but the MRT + * dashboard asks for them on every reviewable queue at once. The GraphQL + * context is a fresh object per request, so memoizing on it keeps the + * reviewability lookup to one query per request instead of one per queue. + */ +const reviewableQueueIdsByRequest = new WeakMap< + Context, + Promise> +>(); + +// The cache is populated before the first await, so concurrent field resolvers +// in the same request share one in-flight lookup rather than racing. +async function getReviewableQueueIds( + context: Context, + user: GraphQLUserParent, +) { + const cached = reviewableQueueIdsByRequest.get(context); + if (cached != null) { + return cached; + } + + const queueIds = + context.services.ManualReviewToolService.getReviewableQueuesForUser({ + invoker: { + userId: user.id, + permissions: user.getPermissions(), + orgId: user.orgId, + }, + }).then((queues) => new Set(queues.map((reviewable) => reviewable.id))); + + reviewableQueueIdsByRequest.set(context, queueIds); + return queueIds; +} + +/** + * Throws unless the caller may review `queue`. Mirrors + * `getReviewableQueuesForUser`: VIEW_MRT is required, and EDIT_MRT_QUEUES sees + * every queue in the org. Queue objects can reach these resolvers from paths + * that don't themselves check membership (e.g. `User.favoriteMRTQueues`, which + * keeps favorites after access is revoked, and `RoutingRule.destinationQueue`, + * which bypasses queue permissions for EDIT_MRT_QUEUES holders), so each field + * has to authorize rather than trust its parent. + */ +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 reviewableQueueIds = await getReviewableQueueIds(context, user); + if (!reviewableQueueIds.has(queue.id)) { + throw forbiddenError('User does not have access to this queue'); + } + return user; +} + const ManualReviewQueue: GQLManualReviewQueueResolvers = { async jobs(queue, { ids: jobIds, limit }, context) { + await assertQueueIsReviewable(queue, context); + const { orgId, id: queueId } = queue; if (jobIds == null) { @@ -1771,6 +1838,8 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { }); }, async pendingJobCount(queue, _, context) { + await assertQueueIsReviewable(queue, context); + const { orgId, id: queueId } = queue; return context.services.ManualReviewToolService.getPendingJobCount({ orgId, @@ -1778,6 +1847,8 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { }); }, async oldestJobCreatedAt(queue, _, context) { + await assertQueueIsReviewable(queue, context); + const { orgId, id: queueId } = queue; return context.services.ManualReviewToolService.getOldestJobCreatedAt({ orgId, @@ -1786,10 +1857,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 = ( @@ -1802,10 +1870,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; @@ -1815,10 +1880,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, @@ -2083,14 +2145,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 +2244,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(); @@ -2198,10 +2272,13 @@ const Query: GQLQueryResolvers = { throw unauthenticatedError('Authenticated user required'); } + const reviewableQueueIds = await getReviewableQueueIds(context, user); + return context.services.ManualReviewToolService.getExistingJobsForItem({ orgId: user.orgId, itemId: params.itemId, itemTypeId: params.itemTypeId, + queueIds: [...reviewableQueueIds], }); }, async getDecisionsTable(_, params, context) { @@ -2275,6 +2352,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/manualReviewToolService.ts b/server/services/manualReviewToolService/manualReviewToolService.ts index bea70a9ca..bf50b4ff9 100644 --- a/server/services/manualReviewToolService/manualReviewToolService.ts +++ b/server/services/manualReviewToolService/manualReviewToolService.ts @@ -1212,6 +1212,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 b0422f29c..fc36b029c 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -188,6 +188,130 @@ 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); + }, + ); + + // getExistingJobsForItem only scans the caller's queue set + // set, so the queue filter alone determines what is visible + // to prevent authenticated users from reading job payloads + // from queues they have no access to + 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]); + + // The same job must stay invisible when the caller's queue set does not + // include its queue. + 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([]); + + // A caller with no reviewable queues searches nothing rather than + // erroring on an empty `in ()`. + 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 bc515f5bd..f90df134e 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.ts @@ -1696,8 +1696,14 @@ export default class QueueOperations { orgId: string; itemId: string; itemTypeId: string; + queueIds: string[]; }) { - const { orgId, itemId, itemTypeId } = opts; + const { orgId, itemId, itemTypeId, queueIds } = opts; + // A caller with no reviewable queues searches nothing; Kysely would compile + // the filter below to `in ()`, which Postgres rejects. + 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 @@ -1707,6 +1713,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..58b09e90a 100644 --- a/server/test/fixtureHelpers/createMrtQueue.ts +++ b/server/test/fixtureHelpers/createMrtQueue.ts @@ -5,11 +5,13 @@ export default async function (opts: { orgId: string; mrtService: Dependencies['ManualReviewToolService']; userId: string; + /** Queue names are unique per org; pass one to make a second queue. */ + 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: [],