Skip to content

fix(maintainer): scope flagged-accounts query at database level (closes #755) #810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jakharmonika364 merged 1 commit into from
Aug 5, 2026
Merged

fix(maintainer): scope flagged-accounts query at database level (closes #755) #810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
159 changes: 159 additions & 0 deletions src/app/actions/maintainer.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
85 changes: 45 additions & 40 deletions src/app/actions/maintainer/flagged-accounts.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string>();
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);

Expand All @@ -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<string>();
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) => {
Expand Down
Loading