From 7e3ac76c990924555faade685f1f062e44392370 Mon Sep 17 00:00:00 2001 From: Shalabh Agarwal Date: Fri, 24 Jul 2026 01:45:38 +0530 Subject: [PATCH 01/12] 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 43d507373aa851700b97dba808feec3788688a1a Mon Sep 17 00:00:00 2001 From: Cassidy James Blaede Date: Wed, 9 Sep 2026 16:51:11 -0600 Subject: [PATCH 02/12] Authorize queue-scoped MRT fields instead of only authenticating ManualReviewQueue.jobs checked that someone was logged in but never that the caller could review the queue it was handed, and pendingJobCount and oldestJobCreatedAt had no check at all. Queue objects reach these resolvers from parents that don't establish membership themselves, so two traversals still leaked jobs and queue metadata after the previous commit: - User.favoriteMRTQueues keeps a favorite after access is revoked, and Org.users is readable by any authenticated org member, so `org { users { favoriteMRTQueues { jobs } } }` needed no permissions at all. - RoutingRule.destinationQueue bypasses queue permissions for EDIT_MRT_QUEUES holders, while getReviewableQueuesForUser returns nothing without VIEW_MRT, so that permission combination could reach a queue it had no queues for. Add assertQueueIsReviewable and apply it to all three fields. It mirrors getReviewableQueuesForUser exactly, requiring VIEW_MRT and letting EDIT_MRT_QUEUES see the whole org, plus a cross-org guard. The reviewable-queue lookup is memoized on the GraphQL context rather than called per field. The MRT dashboard asks for pendingJobCount and oldestJobCreatedAt on every reviewable queue at once, so calling the lookup inline would turn one dashboard load into two full queue fetches per queue. Apollo builds a fresh context per request and the cache is populated before the first await, so concurrent resolvers share one in-flight query. --- .../modules/manualReviewTool.resolver.test.ts | 130 +++++++++++++++++- server/graphql/modules/manualReviewTool.ts | 73 +++++++++- 2 files changed, 193 insertions(+), 10 deletions(-) diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index 5a8c5fdf7..b94f1bee7 100644 --- a/server/graphql/modules/manualReviewTool.resolver.test.ts +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -16,7 +16,7 @@ const Mutation = resolvers.Mutation as Record< ResolverFn >; const ManualReviewQueue = resolvers.ManualReviewQueue as Record< - 'jobs', + 'jobs' | 'pendingJobCount' | 'oldestJobCreatedAt', ResolverFn >; @@ -47,6 +47,9 @@ 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 getPendingJobCount = jest.fn(async () => 3); + const getOldestJobCreatedAt = jest.fn(async () => new Date(0)); const ctx = { getUser: () => @@ -65,6 +68,9 @@ function makeCtx(opts: { getTotalPendingJobCountForQueues, dequeueNextJob, getAllJobsForQueue, + getJobsForQueue, + getPendingJobCount, + getOldestJobCreatedAt, }, }, }; @@ -77,6 +83,9 @@ function makeCtx(opts: { getTotalPendingJobCountForQueues, dequeueNextJob, getAllJobsForQueue, + getJobsForQueue, + getPendingJobCount, + getOldestJobCreatedAt, }; } @@ -180,20 +189,129 @@ 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(); + }); + + // 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-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(); + }); + + // 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); }); }); }); diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index 38ab95edc..22901a5fe 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,12 +1748,71 @@ 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'); + } +} + const ManualReviewQueue: GQLManualReviewQueueResolvers = { async jobs(queue, { ids: jobIds, limit }, context) { - const user = context.getUser(); - if (user == null) { - throw unauthenticatedError('User required.'); - } + await assertQueueIsReviewable(queue, context); const { orgId, id: queueId } = queue; @@ -1776,6 +1837,8 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { }); }, async pendingJobCount(queue, _, context) { + await assertQueueIsReviewable(queue, context); + const { orgId, id: queueId } = queue; return context.services.ManualReviewToolService.getPendingJobCount({ orgId, @@ -1783,6 +1846,8 @@ const ManualReviewQueue: GQLManualReviewQueueResolvers = { }); }, async oldestJobCreatedAt(queue, _, context) { + await assertQueueIsReviewable(queue, context); + const { orgId, id: queueId } = queue; return context.services.ManualReviewToolService.getOldestJobCreatedAt({ orgId, From fb36e94d77a50dab59c00e70c210b274a6669d54 Mon Sep 17 00:00:00 2001 From: Cassidy James Blaede Date: Wed, 9 Sep 2026 16:51:27 -0600 Subject: [PATCH 03/12] CHANGELOG: note the queue and job access control fix --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6b315c3b6..855807556 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)) +- Manual review queues and jobs being readable by users without access to that queue ([#1151](https://github.com/roostorg/coop/pull/1151) by [@serendipty01](https://github.com/serendipty01), closes [#1150](https://github.com/roostorg/coop/issues/1150)) ### Security From 1972a3fadadc62749161783798409ccf53790e05 Mon Sep 17 00:00:00 2001 From: Cassidy James Blaede Date: Wed, 9 Sep 2026 17:21:50 -0600 Subject: [PATCH 04/12] MRT: scope getExistingJobsForItem to the caller's reviewable queues The resolver handed the service only an org id, so getExistingJobsForItem scanned every queue in the org and returned full pending job payloads for any item, regardless of the caller's access to those queues -- the same bypass class as #1150. It now resolves the caller's reviewable queues up front and passes their IDs down, and the service filters the job_creations lookup to that set. --- .../modules/manualReviewTool.resolver.test.ts | 75 ++++++++++++++++++- server/graphql/modules/manualReviewTool.ts | 12 +++ .../manualReviewToolService.ts | 1 + .../modules/QueueOperations.test.ts | 56 ++++++++++++++ .../modules/QueueOperations.ts | 4 +- 5 files changed, 146 insertions(+), 2 deletions(-) diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index b94f1bee7..374346ae6 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< @@ -48,6 +48,7 @@ function makeCtx(opts: { 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)); @@ -69,6 +70,7 @@ function makeCtx(opts: { dequeueNextJob, getAllJobsForQueue, getJobsForQueue, + getExistingJobsForItem, getPendingJobCount, getOldestJobCreatedAt, }, @@ -84,6 +86,7 @@ function makeCtx(opts: { dequeueNextJob, getAllJobsForQueue, getJobsForQueue, + getExistingJobsForItem, getPendingJobCount, getOldestJobCreatedAt, }; @@ -152,6 +155,76 @@ 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({ diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index 22901a5fe..2d95eb269 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -2280,10 +2280,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) { 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 1eb56c715..20fa1aedc 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -245,6 +245,62 @@ describe('QueueOperations', () => { }, ); + // Regression: #1150 -- getExistingJobsForItem used to scan every org queue + // for a job on the item, so any authenticated user could read job payloads + // from queues they had no access to. It now only scans the caller's queue + // set, so the queue filter alone determines what is visible. + 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, + }); + const otherQueueOnly = await mrtService.getExistingJobsForItem({ + orgId: org.id, + itemId, + itemTypeId, + queueIds: [otherQueue.id], + }); + expect(otherQueueOnly).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..ac1e03ef8 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.ts @@ -1696,8 +1696,9 @@ export default class QueueOperations { orgId: string; itemId: string; itemTypeId: string; + queueIds: string[]; }) { - const { orgId, itemId, itemTypeId } = opts; + const { orgId, itemId, itemTypeId, queueIds } = opts; // 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 +1708,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) => { From 7da7865b8fc42e6b254937d7ac900f947a1cc270 Mon Sep 17 00:00:00 2001 From: Cassidy James Blaede Date: Wed, 9 Sep 2026 17:24:17 -0600 Subject: [PATCH 05/12] MRT: authorize the remaining queue-scoped ManualReviewQueue fields explicitlyAssignedReviewers, hiddenActionIds, and clearReportsTriggerActionIds still gated on authentication alone, so a revoked member (whose queue stays in favoriteMRTQueues) or an EDIT_MRT_QUEUES holder reaching the queue through RoutingRule.destinationQueue could read queue membership and moderation config. Extend assertQueueIsReviewable to all three, matching jobs/pendingJobCount/ oldestJobCreatedAt, and have the helper return the caller so the resolvers stop re-fetching it. --- .../modules/manualReviewTool.resolver.test.ts | 130 +++++++++++++++++- server/graphql/modules/manualReviewTool.ts | 16 +-- 2 files changed, 133 insertions(+), 13 deletions(-) diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index 374346ae6..b7248b875 100644 --- a/server/graphql/modules/manualReviewTool.resolver.test.ts +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -16,7 +16,12 @@ const Mutation = resolvers.Mutation as Record< ResolverFn >; const ManualReviewQueue = resolvers.ManualReviewQueue as Record< - 'jobs' | 'pendingJobCount' | 'oldestJobCreatedAt', + | 'jobs' + | 'pendingJobCount' + | 'oldestJobCreatedAt' + | 'explicitlyAssignedReviewers' + | 'hiddenActionIds' + | 'clearReportsTriggerActionIds', ResolverFn >; @@ -51,6 +56,16 @@ function makeCtx(opts: { 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: () => @@ -73,8 +88,14 @@ function makeCtx(opts: { getExistingJobsForItem, getPendingJobCount, getOldestJobCreatedAt, + getUsersWhoCanSeeQueue, + getHiddenActionsForQueue, + getClearReportsTriggerActionsForQueue, }, }, + dataSources: { + userAPI: { getGraphQLUsersFromIds }, + }, }; return { @@ -89,6 +110,10 @@ function makeCtx(opts: { getExistingJobsForItem, getPendingJobCount, getOldestJobCreatedAt, + getUsersWhoCanSeeQueue, + getHiddenActionsForQueue, + getClearReportsTriggerActionsForQueue, + getGraphQLUsersFromIds, }; } @@ -386,5 +411,108 @@ describe('MRT queue/job resolvers are membership-scoped', () => { 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 2d95eb269..ec7e6f1bc 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -1808,6 +1808,7 @@ async function assertQueueIsReviewable( if (!reviewableQueueIds.has(queue.id)) { throw forbiddenError('User does not have access to this queue'); } + return user; } const ManualReviewQueue: GQLManualReviewQueueResolvers = { @@ -1856,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 = ( @@ -1872,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; @@ -1885,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, From 61ef824d15d6a50cec5931a622d3d0985b7d3957 Mon Sep 17 00:00:00 2001 From: Cassidy James Date: Wed, 9 Sep 2026 18:47:14 -0600 Subject: [PATCH 06/12] chore: update QueueOperations.test comment for clarity --- .../modules/QueueOperations.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index 20fa1aedc..ca5ce6d18 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -245,10 +245,10 @@ describe('QueueOperations', () => { }, ); - // Regression: #1150 -- getExistingJobsForItem used to scan every org queue - // for a job on the item, so any authenticated user could read job payloads - // from queues they had no access to. It now only scans the caller's queue - // set, so the queue filter alone determines what is visible. + // 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 }) => { From d63717d2f23c44f23d45f15d3f64849ac8db8bd2 Mon Sep 17 00:00:00 2001 From: Cassidy James Date: Wed, 9 Sep 2026 18:49:01 -0600 Subject: [PATCH 07/12] docs: update CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 855807556..cf907d527 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +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)) -- Manual review queues and jobs being readable by users without access to that queue ([#1151](https://github.com/roostorg/coop/pull/1151) by [@serendipty01](https://github.com/serendipty01), closes [#1150](https://github.com/roostorg/coop/issues/1150)) +- Manual 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 From cbba8df9bd2a69f5e5eb21d3a9f4356890b66358 Mon Sep 17 00:00:00 2001 From: Cassidy James Date: Wed, 9 Sep 2026 18:49:25 -0600 Subject: [PATCH 08/12] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cf907d527..bb3e2d4f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +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)) -- Manual 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) +- 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 From 10f7fb3c2941b1b74b08bdb35c793807896e6da4 Mon Sep 17 00:00:00 2001 From: Cassidy James Blaede Date: Wed, 9 Sep 2026 18:55:19 -0600 Subject: [PATCH 09/12] createMrtQueue: allow a custom queue name Queue names are unique per org and the fixture hardcoded `test-queue`, so the new getExistingJobsForItem scoping test failed with ManualReviewQueueNameExistsError as soon as it created a second queue in an org the fixture had already set up. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WcuVs559fcXVPycArcjJBS --- .../manualReviewToolService/modules/QueueOperations.test.ts | 1 + server/test/fixtureHelpers/createMrtQueue.ts | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index ca5ce6d18..b1e19a794 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -290,6 +290,7 @@ describe('QueueOperations', () => { orgId: org.id, mrtService, userId: user.id, + name: `other-queue-${uid()}`, }); const otherQueueOnly = await mrtService.getExistingJobsForItem({ orgId: org.id, 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: [], From 9a2eeb109ec7b52a71d187b7aa7f6e182ec6a7b8 Mon Sep 17 00:00:00 2001 From: Cassidy James Blaede Date: Wed, 9 Sep 2026 19:00:04 -0600 Subject: [PATCH 10/12] MRT: return no existing jobs for a caller with no reviewable queues Scoping getExistingJobsForItem to the caller's queues left an empty queue set reaching the query builder, and Kysely compiles that filter to `in ()`, which Postgres rejects outright. Any caller without VIEW_MRT -- or with it but no queue memberships -- got a 500 where the correct answer is an empty list. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WcuVs559fcXVPycArcjJBS --- .../modules/QueueOperations.test.ts | 10 ++++++++++ .../manualReviewToolService/modules/QueueOperations.ts | 5 +++++ 2 files changed, 15 insertions(+) diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index b1e19a794..fc36b029c 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -299,6 +299,16 @@ describe('QueueOperations', () => { 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([]); }, ); diff --git a/server/services/manualReviewToolService/modules/QueueOperations.ts b/server/services/manualReviewToolService/modules/QueueOperations.ts index ac1e03ef8..f90df134e 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.ts @@ -1699,6 +1699,11 @@ export default class QueueOperations { queueIds: string[]; }) { 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 From a09a911ac20af1b732603345f117769f72d4dc22 Mon Sep 17 00:00:00 2001 From: Cassidy James Blaede Date: Wed, 9 Sep 2026 19:02:08 -0600 Subject: [PATCH 11/12] MRT: reuse the per-request queue lookup in getExistingJobsForItem The resolver called getReviewableQueuesForUser directly, so a query that also asked for queue-scoped fields paid for two full queue fetches. It only needs the IDs, so the memoized lookup fits. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01WcuVs559fcXVPycArcjJBS --- .../modules/manualReviewTool.resolver.test.ts | 21 +++++++++++++++++++ server/graphql/modules/manualReviewTool.ts | 13 ++---------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/server/graphql/modules/manualReviewTool.resolver.test.ts b/server/graphql/modules/manualReviewTool.resolver.test.ts index b7248b875..70581e002 100644 --- a/server/graphql/modules/manualReviewTool.resolver.test.ts +++ b/server/graphql/modules/manualReviewTool.resolver.test.ts @@ -248,6 +248,27 @@ describe('MRT queue/job resolvers are membership-scoped', () => { ).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', () => { diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index ec7e6f1bc..197b39360 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -2272,22 +2272,13 @@ 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, - }, - }, - ); + const reviewableQueueIds = await getReviewableQueueIds(context, user); return context.services.ManualReviewToolService.getExistingJobsForItem({ orgId: user.orgId, itemId: params.itemId, itemTypeId: params.itemTypeId, - queueIds: reviewableQueues.map((queue) => queue.id), + queueIds: [...reviewableQueueIds], }); }, async getDecisionsTable(_, params, context) { From b4755b4b6cb291eb38488be3d892a697656541d5 Mon Sep 17 00:00:00 2001 From: Cassidy James Date: Thu, 10 Sep 2026 15:06:29 -0600 Subject: [PATCH 12/12] docs: close parenthesis in CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bb3e2d4f1..b50ac1572 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,7 +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) +- 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