From e4f89b81055009da1f6b00537e3138a2bcb214aa Mon Sep 17 00:00:00 2001 From: Archit Adish Gupta Date: Sun, 26 Jul 2026 00:58:55 +0530 Subject: [PATCH] fix(maintainer): scope flagged-accounts query at database level (closes #755) --- src/app/actions/maintainer.test.ts | 159 ++++++++++++++++++ .../actions/maintainer/flagged-accounts.ts | 85 +++++----- 2 files changed, 204 insertions(+), 40 deletions(-) diff --git a/src/app/actions/maintainer.test.ts b/src/app/actions/maintainer.test.ts index 9ad90e9c..df747278 100644 --- a/src/app/actions/maintainer.test.ts +++ b/src/app/actions/maintainer.test.ts @@ -863,6 +863,165 @@ describe('maintainer actions', () => { expect(res.data).toHaveLength(0); } }); + + // Regression tests for issue #755: query-level scoping + // The flagged_accounts query must now be constrained by `.in('user_id', …)` + // derived from users with activity in the maintainer's repos — no global + // unbounded load followed by JavaScript filtering. + + it('scopes flagged_accounts query at the database level to active user ids (regression #755)', async () => { + // activeUserIds from PR + recommendations: only user-active-pr + user-active-rec + const prs = [{ author_user_id: 'user-active-pr' }]; + const recs = [{ user_id: 'user-active-rec' }]; + + let flaggedAccountsCallCount = 0; + let lastInArgs: any[] = []; + + mockFrom.mockImplementation((table) => { + if (table === 'pull_requests') return chain(prs); + if (table === 'recommendations') return chain(recs); + if (table === 'flagged_accounts') { + flaggedAccountsCallCount += 1; + // Return a chain that records the `.in(` column + values + const c = chain([ + { + id: 1, + user_id: 'user-active-pr', + reason: 'daily_xp_event_spike', + severity: 'medium', + evidence: { items: [{ repo: 'my-org/my-repo', xpDelta: 10 }] }, + detected_at: '2026-05-18T00:00:00Z', + }, + { + id: 2, + user_id: 'user-active-rec', + reason: 'rapid_merge_spike', + severity: 'high', + evidence: { items: [{ repoFullName: 'my-org/my-repo' }] }, + detected_at: '2026-05-18T01:00:00Z', + }, + ]); + const origIn = c.in as any; + c.in = vi.fn((col: string, values: any[]) => { + if (col === 'user_id') lastInArgs = [col, values]; + return c; + }); + return c; + } + if (table === 'profiles') { + return chain([ + { id: 'user-active-pr', github_handle: 'active-pr-user', xp: 100, level: 2 }, + { id: 'user-active-rec', github_handle: 'active-rec-user', xp: 200, level: 3 }, + ]); + } + return chain([]); + }); + + const res = await getFlaggedAccounts({ installationId: 1 }); + expect(res.ok).toBe(true); + expect(flaggedAccountsCallCount).toBe(1); + // The `.in('user_id', …)` filter must be applied to the flagged_accounts query + expect(lastInArgs[0]).toBe('user_id'); + expect(lastInArgs[1]).toEqual(expect.arrayContaining(['user-active-pr', 'user-active-rec'])); + // user-inactive (no activity in maintainer's repos) must not be in the filter + expect(lastInArgs[1]).not.toContain('user-inactive'); + if (res.ok) { + expect(res.data).toHaveLength(2); + } + }); + + it('returns empty array without querying flagged_accounts when no users have activity in repos (#755)', async () => { + // No PRs and no recommendations → activeUserIds is empty + const prs: any[] = []; + const recs: any[] = []; + + let flaggedAccountsCallCount = 0; + + mockFrom.mockImplementation((table) => { + if (table === 'pull_requests') return chain(prs); + if (table === 'recommendations') return chain(recs); + if (table === 'flagged_accounts') { + flaggedAccountsCallCount += 1; + return chain([ + { + id: 99, + user_id: 'someone-else', + reason: 'rapid_merge_spike', + severity: 'high', + evidence: { items: [{ repo: 'other-org/other-repo' }] }, + detected_at: '2026-05-18T02:00:00Z', + }, + ]); + } + return chain([]); + }); + + const res = await getFlaggedAccounts({ installationId: 1 }); + expect(res.ok).toBe(true); + // The fix must short-circuit before touching flagged_accounts so no + // global rows are ever loaded for an attacker who controls zero repos + // with user activity. + expect(flaggedAccountsCallCount).toBe(0); + if (res.ok) { + expect(res.data).toHaveLength(0); + } + }); + + it('propagates query error when pull_requests activity lookup fails (#755)', async () => { + mockFrom.mockImplementation((table) => { + if (table === 'pull_requests') return chain(null, { message: 'pr lookup broke' }); + return chain([]); + }); + + const res = await getFlaggedAccounts({ installationId: 1 }); + expect(res.ok).toBe(false); + if (!res.ok) expect(res.error.code).toBe('query_failed'); + }); + + it('propagates query error when recommendations activity lookup fails (#755)', async () => { + mockFrom.mockImplementation((table) => { + if (table === 'pull_requests') return chain([{ author_user_id: 'u1' }]); + if (table === 'recommendations') return chain(null, { message: 'rec lookup broke' }); + return chain([]); + }); + + const res = await getFlaggedAccounts({ installationId: 1 }); + expect(res.ok).toBe(false); + if (!res.ok) expect(res.error.code).toBe('query_failed'); + }); + + it('still drops flags whose evidence.items repo is outside maintainer scope (#755)', async () => { + // Even when the user has activity in the maintainer's repos, a flag whose + // evidence solely references an unrelated repo must be filtered out. + const prs = [{ author_user_id: 'user-cross' }]; + const recs: any[] = []; + + mockFrom.mockImplementation((table) => { + if (table === 'pull_requests') return chain(prs); + if (table === 'recommendations') return chain(recs); + if (table === 'flagged_accounts') { + return chain([ + { + id: 1, + user_id: 'user-cross', + reason: 'rapid_merge_spike', + severity: 'high', + evidence: { items: [{ repo: 'other-org/other-repo' }] }, + detected_at: '2026-05-18T02:00:00Z', + }, + ]); + } + if (table === 'profiles') + return chain([{ id: 'user-cross', github_handle: 'cross', xp: 50, level: 1 }]); + return chain([]); + }); + + const res = await getFlaggedAccounts({ installationId: 1 }); + expect(res.ok).toBe(true); + if (res.ok) { + expect(res.data).toHaveLength(0); + } + }); }); // resolveFlaggedAccount diff --git a/src/app/actions/maintainer/flagged-accounts.ts b/src/app/actions/maintainer/flagged-accounts.ts index 0d0ee076..2ba3167a 100644 --- a/src/app/actions/maintainer/flagged-accounts.ts +++ b/src/app/actions/maintainer/flagged-accounts.ts @@ -39,11 +39,56 @@ export async function getFlaggedAccounts(args?: { return ok([]); } + // Step 1: Resolve user_ids that have activity in the maintainer's repos. + // We do this BEFORE touching `flagged_accounts` so that the flagged-accounts + // query can be scoped at the database level to those user_ids, eliminating + // the prior data-leak path where the global table was loaded unscoped and + // then filtered in JavaScript (see issue #755). + const [prUsersRes, recUsersRes] = await Promise.all([ + service.from('pull_requests').select('author_user_id').in('repo_full_name', repos), + service + .from('recommendations') + .select('user_id, issues!inner(repo_full_name)') + .in('issues.repo_full_name', repos), + ]); + + if (prUsersRes.error) { + return err('query_failed', prUsersRes.error.message); + } + if (recUsersRes.error) { + return err('query_failed', recUsersRes.error.message); + } + + const activeUserIds = new Set(); + for (const pr of prUsersRes.data ?? []) { + if (pr.author_user_id) { + activeUserIds.add(pr.author_user_id); + } + } + for (const rec of recUsersRes.data ?? []) { + if (rec.user_id) { + activeUserIds.add(rec.user_id); + } + } + + if (activeUserIds.size === 0) { + return ok([]); + } + + const userIdsFilter = Array.from(activeUserIds); + + // Step 2: Query `flagged_accounts` scoped to users with activity in the + // maintainer's repos. This is the query-level scoping the issue asked for. + // We still post-filter by `evidence.items[].repo` because the JSONB + // containment check is not portable across Supabase client versions, but + // the unbounded global load is gone — the query can now return at most + // flags for users the maintainer is already authorised to see. const { data: flags, error } = await service .from('flagged_accounts') .select('id, user_id, installation_id, reason, severity, evidence, detected_at') .eq('status', 'open') .or(`installation_id.is.null,installation_id.eq.${installationId}`) + .in('user_id', userIdsFilter) .order('detected_at', { ascending: false }) .limit(100); @@ -55,47 +100,7 @@ export async function getFlaggedAccounts(args?: { return ok([]); } - const userIds = Array.from(new Set(flags.map((flag) => flag.user_id).filter(Boolean))); - if (userIds.length === 0) { - return ok([]); - } - - const { data: prUsers, error: prError } = await service - .from('pull_requests') - .select('author_user_id') - .in('author_user_id', userIds) - .in('repo_full_name', repos); - - if (prError) { - return err('query_failed', prError.message); - } - - const { data: recUsers, error: recError } = await service - .from('recommendations') - .select('user_id, issues!inner(repo_full_name)') - .in('user_id', userIds) - .in('issues.repo_full_name', repos); - - if (recError) { - return err('query_failed', recError.message); - } - - const activeUserIds = new Set(); - for (const pr of prUsers ?? []) { - if (pr.author_user_id) { - activeUserIds.add(pr.author_user_id); - } - } - for (const rec of recUsers ?? []) { - if (rec.user_id) { - activeUserIds.add(rec.user_id); - } - } - const allowedFlags = flags.filter((flag) => { - if (!flag.user_id || !activeUserIds.has(flag.user_id)) { - return false; - } const evidence = flag.evidence as any; const items = Array.isArray(evidence?.items) ? evidence.items : []; return items.some((item: any) => {