From bd8fc6cc51de2d8f187ea63a8a7bebb8c4417088 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tao=20Bojl=C3=A9n?= Date: Tue, 30 Jun 2026 18:30:31 +0100 Subject: [PATCH] fix(mrt): scope queue mutations to the correct org (#872) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mrt): org-scope accessible-queue mutations (GHSA-mf74-gf5j-hxr9) addAccessibleQueuesForUser / removeAccessibleQueuesForUser accepted an arbitrary queueId / userId with no org-scoping, so an authenticated user in Org A could grant (or revoke) access to queues and users belonging to Org B. Thread the caller's orgId through the resolver, service wrapper, and QueueOperations write layer, and reject (403 AccessibleQueueNotInOrgError) if any target queue or user does not belong to the caller's org. Regression tests cover both IDOR dimensions (cross-org queueId and cross-org userId) for both add and remove, plus a same-org happy path. Co-Authored-By: Claude Opus 4.8 (1M context) * refactor(mrt): use transactional test harness, trim comments - Switch the accessible-queue IDOR tests to makeTransactionalTestWithFixture (per-org rollback, no manual cleanup). - Drop GHSA references and "what was" narration from code comments; keep only minimal present-tense notes. Co-Authored-By: Claude Opus 4.8 (1M context) * test(mrt): assert original user keeps access after cross-org revoke attempt Strengthen the GHSA-mf74 regression test for removeAccessibleQueuesForUser: after a rejected cross-org revoke, also assert the attacker still sees their own queue — confirming the rejected call left the original access intact. Co-Authored-By: pi * fix(mrt): org-scope create/update queue userIds (GHSA-mf74-gf5j-hxr9) createManualReviewQueue and updateManualReviewQueue accepted un-scoped userIds and wrote them straight into users_and_accessible_queues, the same un-scoped-input shape fixed in add/removeAccessibleQueuesForUser. create always creates the queue in the caller's org (orgId from the invoker) but granted access to users in other orgs; update's queue-row write was org-scoped but the join-table writes ran un-scoped inside the transaction. Refactor assertUsersAndQueuesInOrg into composable assertQueuesInOrg + assertUsersInOrg (add/remove callers unchanged), then guard create with assertUsersInOrg and update with assertUsersAndQueuesInOrg before any write, rejecting AccessibleQueueNotInOrgError for cross-org targets. Defense-in-depth/integrity fix: reads (getReviewableQueuesForUser, getQueueForOrg) and the dequeue path (checkQueueExists) are already org-scoped by the invoker, so a cross-org user could not actually see or act on the queue -- no confidentiality impact. Closes the same un-scoped-input shape the advisory fixed elsewhere. Regression tests cover cross-org userId for both create and update, asserting the mutation rejects and that update leaves the victim user absent from the queue's viewers. Co-Authored-By: pi * test(mrt): use a real org user in JobRouting queue fixtures The org-scoping guard added to createManualReviewQueue rejects userIds that are not members of the caller's org. JobRouting.test.ts invented userId = uid() and never created a matching public.users row, so every queue fixture in that suite threw AccessibleQueueNotInOrgError. Create the user via the createUser fixture helper (real org member) and wire its cleanup, matching QueueOperations.test.ts and CommentOperations.test.ts. Co-Authored-By: pi --------- Co-authored-by: Claude Opus 4.8 (1M context) --- server/graphql/modules/manualReviewTool.ts | 16 +- .../manualReviewToolService/dbTypes.ts | 3 + .../manualReviewToolService.ts | 30 ++- .../modules/JobRouting.test.ts | 8 +- .../modules/QueueOperations.test.ts | 176 ++++++++++++++++++ .../modules/QueueOperations.ts | 124 +++++++++++- 6 files changed, 331 insertions(+), 26 deletions(-) diff --git a/server/graphql/modules/manualReviewTool.ts b/server/graphql/modules/manualReviewTool.ts index 1d3a0a4..4d14efa 100644 --- a/server/graphql/modules/manualReviewTool.ts +++ b/server/graphql/modules/manualReviewTool.ts @@ -2460,10 +2460,11 @@ const Mutation: GQLMutationResolvers = { throw forbiddenError('User does not have permission to edit MRT queues'); } - await context.services.ManualReviewToolService.addAccessibleQueuesForUser( - params.input.userId, - params.input.queueIds, - ); + await context.services.ManualReviewToolService.addAccessibleQueuesForUser({ + orgId: user.orgId, + userId: params.input.userId, + queueIds: params.input.queueIds, + }); // TODO: try/catch and return failure cases return gqlSuccessResult( @@ -2481,8 +2482,11 @@ const Mutation: GQLMutationResolvers = { } await context.services.ManualReviewToolService.removeAccessibleQueuesForUser( - params.input.userId, - params.input.queueIds, + { + orgId: user.orgId, + userId: params.input.userId, + queueIds: params.input.queueIds, + }, ); // TODO: try/catch and return failure cases diff --git a/server/services/manualReviewToolService/dbTypes.ts b/server/services/manualReviewToolService/dbTypes.ts index b80aa16..3bccd11 100644 --- a/server/services/manualReviewToolService/dbTypes.ts +++ b/server/services/manualReviewToolService/dbTypes.ts @@ -6,6 +6,7 @@ import { } from '../../storage/dataWarehouse/warehouseDateTypes.js'; import { type JsonOf } from '../../utils/encoding.js'; import { type ConditionSetWithResultAsLogged } from '../analyticsLoggers/ruleExecutionLoggingUtils.js'; +import { type CoreAppTablesPg } from '../coreAppTables.js'; import { type NormalizedItemData } from '../itemProcessingService/toNormalizedItemDataOrErrors.js'; import { type ConditionSet, @@ -74,6 +75,8 @@ export type RoutingRuleExecutionsRow = { ); export type ManualReviewToolServicePg = { + // Shared with CoreAppTablesPg so org-scoping checks can query public.users. + 'public.users': CoreAppTablesPg['public.users']; 'manual_review_tool.manual_review_queues': { id: string; name: string; diff --git a/server/services/manualReviewToolService/manualReviewToolService.ts b/server/services/manualReviewToolService/manualReviewToolService.ts index 0c4dae0..2f09e0a 100644 --- a/server/services/manualReviewToolService/manualReviewToolService.ts +++ b/server/services/manualReviewToolService/manualReviewToolService.ts @@ -838,18 +838,28 @@ export class ManualReviewToolService { }); } - async addAccessibleQueuesForUser( - userId: string, - queueIds: readonly string[], - ) { - return this.queueOps.addAccessibleQueuesForUser([userId], queueIds); + async addAccessibleQueuesForUser(opts: { + orgId: string; + userId: string; + queueIds: readonly string[]; + }) { + return this.queueOps.addAccessibleQueuesForUser({ + orgId: opts.orgId, + userIds: [opts.userId], + queueIds: opts.queueIds, + }); } - async removeAccessibleQueuesForUser( - userId: string, - queueIds: readonly string[], - ) { - return this.queueOps.removeAccessibleQueuesForUser(userId, queueIds); + async removeAccessibleQueuesForUser(opts: { + orgId: string; + userId: string; + queueIds: readonly string[]; + }) { + return this.queueOps.removeAccessibleQueuesForUser({ + orgId: opts.orgId, + userId: opts.userId, + queueIds: opts.queueIds, + }); } /** diff --git a/server/services/manualReviewToolService/modules/JobRouting.test.ts b/server/services/manualReviewToolService/modules/JobRouting.test.ts index 828d97b..30c01d0 100644 --- a/server/services/manualReviewToolService/modules/JobRouting.test.ts +++ b/server/services/manualReviewToolService/modules/JobRouting.test.ts @@ -5,6 +5,7 @@ import { uid } from 'uid'; import getBottle from '../../../iocContainer/index.js'; import createContentItemTypes from '../../../test/fixtureHelpers/createContentItemTypes.js'; import createOrg from '../../../test/fixtureHelpers/createOrg.js'; +import createUser from '../../../test/fixtureHelpers/createUser.js'; import { makeTestWithFixture } from '../../../test/utils.js'; import { toCorrelationId } from '../../../utils/correlationIds.js'; import { jsonStringify } from '../../../utils/encoding.js'; @@ -30,7 +31,11 @@ describe('JobRouting tests', () => { }, uid(), ); - const userId = uid(); + const { user, cleanup: userCleanup } = await createUser( + container.KyselyPg, + org.id, + ); + const userId = user.id; const { itemTypes, cleanup: itemTypesCleanup } = await createContentItemTypes({ moderationConfigService: container.ModerationConfigService, @@ -249,6 +254,7 @@ describe('JobRouting tests', () => { noPolicyQueue.id, ); await itemTypesCleanup(); + await userCleanup(); await orgCleanup(); await container.closeSharedResourcesForShutdown(); }, diff --git a/server/services/manualReviewToolService/modules/QueueOperations.test.ts b/server/services/manualReviewToolService/modules/QueueOperations.test.ts index f3d2713..24dd887 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.test.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.test.ts @@ -7,6 +7,7 @@ import createContentItemTypes from '../../../test/fixtureHelpers/createContentIt import createMrtQueue from '../../../test/fixtureHelpers/createMrtQueue.js'; import createOrg from '../../../test/fixtureHelpers/createOrg.js'; import createUser from '../../../test/fixtureHelpers/createUser.js'; +import { makeTransactionalTestWithFixture } from '../../../test/harness/transactionalTest.js'; import { makeTestWithFixture } from '../../../test/utils.js'; import { UserPermission } from '../../userManagementService/index.js'; import { @@ -231,4 +232,179 @@ describe('QueueOperations', () => { ).resolves.toBeUndefined(); }, ); + + const testWithTwoOrgs = () => + makeTransactionalTestWithFixture(async ({ deps }) => { + const buildOrg = async () => { + const { org } = await createOrg( + { + KyselyPg: deps.KyselyPg, + ModerationConfigService: deps.ModerationConfigService, + ApiKeyService: deps.ApiKeyService, + }, + uid(), + ); + const { user } = await createUser(deps.KyselyPg, org.id); + const { queue } = await createMrtQueue({ + orgId: org.id, + mrtService: deps.ManualReviewToolService, + userId: user.id, + }); + return { org, user, queue }; + }; + + return { + attacker: await buildOrg(), + victim: await buildOrg(), + mrtService: deps.ManualReviewToolService, + }; + }); + + testWithTwoOrgs()( + 'addAccessibleQueuesForUser must not grant access to a queue in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.addAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: attacker.user.id, + queueIds: [victim.queue.id], + }), + ).rejects.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: victim.org.id, + queueId: victim.queue.id, + userId: attacker.user.id, + }); + expect(viewers.map((v) => v.userId)).not.toContain(attacker.user.id); + }, + ); + + testWithTwoOrgs()( + 'addAccessibleQueuesForUser must not grant access for a user in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.addAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: victim.user.id, + queueIds: [attacker.queue.id], + }), + ).rejects.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userId: victim.user.id, + }); + expect(viewers.map((v) => v.userId)).not.toContain(victim.user.id); + }, + ); + + testWithTwoOrgs()( + 'removeAccessibleQueuesForUser must not revoke access for a queue in a different org', + async ({ attacker, victim, mrtService }) => { + await mrtService.addAccessibleQueuesForUser({ + orgId: victim.org.id, + userId: victim.user.id, + queueIds: [victim.queue.id], + }); + + await expect( + mrtService.removeAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: attacker.user.id, + queueIds: [victim.queue.id], + }), + ).rejects.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: victim.org.id, + queueId: victim.queue.id, + userId: victim.user.id, + }); + expect(viewers.map((v) => v.userId)).toContain(victim.user.id); + }, + ); + + testWithTwoOrgs()( + 'removeAccessibleQueuesForUser must not revoke access for a user in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.removeAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: victim.user.id, + queueIds: [attacker.queue.id], + }), + ).rejects.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userId: attacker.user.id, + }); + expect(viewers.map((v) => v.userId)).toContain(attacker.user.id); + }, + ); + + testWithTwoOrgs()( + 'addAccessibleQueuesForUser grants access within the same org', + async ({ attacker, mrtService }) => { + await expect( + mrtService.addAccessibleQueuesForUser({ + orgId: attacker.org.id, + userId: attacker.user.id, + queueIds: [attacker.queue.id], + }), + ).resolves.toBeDefined(); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userId: attacker.user.id, + }); + expect(viewers.map((v) => v.userId)).toContain(attacker.user.id); + }, + ); + + testWithTwoOrgs()( + 'createManualReviewQueue must not grant access to a user in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.createManualReviewQueue({ + name: 'attacker-queue', + description: null, + userIds: [victim.user.id], + hiddenActionIds: [], + isAppealsQueue: false, + invokedBy: { + userId: attacker.user.id, + permissions: [UserPermission.EDIT_MRT_QUEUES], + orgId: attacker.org.id, + }, + }), + ).rejects.toMatchObject({ name: 'AccessibleQueueNotInOrgError' }); + }, + ); + + testWithTwoOrgs()( + 'updateManualReviewQueue must not grant access to a user in a different org', + async ({ attacker, victim, mrtService }) => { + await expect( + mrtService.updateManualReviewQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userIds: [attacker.user.id, victim.user.id], + actionIdsToHide: [], + actionIdsToUnhide: [], + }), + ).rejects.toMatchObject({ name: 'AccessibleQueueNotInOrgError' }); + + const viewers = await mrtService.getUsersWhoCanSeeQueue({ + orgId: attacker.org.id, + queueId: attacker.queue.id, + userId: attacker.user.id, + }); + expect(viewers.map((v) => v.userId)).not.toContain(victim.user.id); + }, + ); }); diff --git a/server/services/manualReviewToolService/modules/QueueOperations.ts b/server/services/manualReviewToolService/modules/QueueOperations.ts index 77b8ab4..b92c654 100644 --- a/server/services/manualReviewToolService/modules/QueueOperations.ts +++ b/server/services/manualReviewToolService/modules/QueueOperations.ts @@ -104,7 +104,8 @@ export type ManualReviewQueueErrorType = 'ManualReviewQueueNameExistsError'; export type QueueOperationsErrorType = | 'DeleteAllJobsUnauthorizedError' | 'QueueDoesNotExistError' - | 'UnableToDeleteDefaultQueueError'; + | 'UnableToDeleteDefaultQueueError' + | 'AccessibleQueueNotInOrgError'; // Compound identifier for a queue. orgId is needed for security, but also // because queues are/will be actually sharded across redis instances for @@ -266,6 +267,11 @@ export default class QueueOperations { ); } + // Defense-in-depth org-scoping: every user granted access to the new + // queue must belong to the caller's org. The queue itself is always + // created in the caller's org (orgId from the invoker). + await assertUsersInOrg(this.pgQuery, { orgId, userIds }); + try { return await this.transactionWithRetry(async (transaction) => { // In newer versions of kysely, this is greatly simplified with @@ -358,6 +364,16 @@ export default class QueueOperations { clearReportsTriggerActionIds, } = input; + // Defense-in-depth org-scoping: the target queue and every user granted + // access must belong to the caller's org. Reject before any write so a + // cross-org queueId or userId can't mutate users_and_accessible_queues + // rows belonging to another org. + await assertUsersAndQueuesInOrg(this.pgQuery, { + orgId, + userIds, + queueIds: [queueId], + }); + return this.transactionWithRetry(async (transaction) => { const [updatedQueue, _, __] = await Promise.all([ transaction @@ -655,10 +671,18 @@ export default class QueueOperations { .execute(); } - async addAccessibleQueuesForUser( - userIds: string[], - queueIds: readonly string[], - ) { + async addAccessibleQueuesForUser(opts: { + orgId: string; + userIds: readonly string[]; + queueIds: readonly string[]; + }) { + const { orgId, userIds, queueIds } = opts; + await assertUsersAndQueuesInOrg(this.pgQuery, { + orgId, + userIds, + queueIds, + }); + return this.pgQuery .insertInto('manual_review_tool.users_and_accessible_queues') .values( @@ -670,10 +694,18 @@ export default class QueueOperations { .execute(); } - async removeAccessibleQueuesForUser( - userId: string, - queueIds: readonly string[], - ) { + async removeAccessibleQueuesForUser(opts: { + orgId: string; + userId: string; + queueIds: readonly string[]; + }) { + const { orgId, userId, queueIds } = opts; + await assertUsersAndQueuesInOrg(this.pgQuery, { + orgId, + userIds: [userId], + queueIds, + }); + return this.pgQuery .deleteFrom('manual_review_tool.users_and_accessible_queues') .where('user_id', '=', userId) @@ -1883,6 +1915,80 @@ const makeQueueDoesNotExistError = (data: ErrorInstanceData) => { }); }; +/** + * Thrown when a target queue or user does not belong to the caller's org. + */ +const makeAccessibleQueueNotInOrgError = (data: ErrorInstanceData) => + new CoopError({ + status: 403, + type: [ErrorType.Unauthorized], + title: "Queue or user does not belong to the caller's organization", + name: 'AccessibleQueueNotInOrgError', + ...data, + }); + +/** + * Rejects before any write if a target queue does not belong to the caller's + * org. Defense-in-depth org-scoping for accessible-queue mutations. + */ +async function assertQueuesInOrg( + db: Kysely, + opts: { orgId: string; queueIds: readonly string[] }, +) { + const { orgId, queueIds } = opts; + const inOrgQueues = await db + .selectFrom('manual_review_tool.manual_review_queues') + .select('id') + .where('org_id', '=', orgId) + .where('id', 'in', queueIds) + .execute(); + if (inOrgQueues.length !== new Set(queueIds).size) { + throw makeAccessibleQueueNotInOrgError({ shouldErrorSpan: true }); + } +} + +/** + * Rejects before any write if a target user does not belong to the caller's + * org. Defense-in-depth org-scoping for accessible-queue mutations. + */ +async function assertUsersInOrg( + db: Kysely, + opts: { orgId: string; userIds: readonly string[] }, +) { + const { orgId, userIds } = opts; + const inOrgUsers = await db + .selectFrom('public.users') + .select('id') + .where('org_id', '=', orgId) + .where('id', 'in', userIds) + .execute(); + if (inOrgUsers.length !== new Set(userIds).size) { + throw makeAccessibleQueueNotInOrgError({ shouldErrorSpan: true }); + } +} + +/** + * Rejects before any write if a target queue or user does not belong to the + * caller's org. + */ +async function assertUsersAndQueuesInOrg( + db: Kysely, + opts: { + orgId: string; + userIds: readonly string[]; + queueIds: readonly string[]; + }, +) { + await assertQueuesInOrg(db, { + orgId: opts.orgId, + queueIds: opts.queueIds, + }); + await assertUsersInOrg(db, { + orgId: opts.orgId, + userIds: opts.userIds, + }); +} + export const makeUnableToDeleteDefaultQueueError = ( data: ErrorInstanceData, ) => { -- 2.51.2