diff --git a/src/app/actions/issues.test.ts b/src/app/actions/issues.test.ts index 1bf8e402..db49a4e6 100644 --- a/src/app/actions/issues.test.ts +++ b/src/app/actions/issues.test.ts @@ -20,6 +20,7 @@ const mocks = vi.hoisted(() => ({ mockGetSession: vi.fn(), mockServiceFrom: vi.fn(), mockGetInstallOctokit: vi.fn(), + mockRateLimit: vi.fn(), })); vi.mock('@/lib/supabase/server', () => ({ @@ -47,9 +48,17 @@ vi.mock('@/lib/cache', () => ({ cacheDel: vi.fn().mockResolvedValue(undefined), })); -import { getIssuesPage, getRepoOptions } from './issues'; +vi.mock('@/lib/rate-limit', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + rateLimit: mocks.mockRateLimit, + }; +}); + +import { getIssuesPage, getRepoOptions, claimIssue } from './issues'; -const createMockChain = (result: unknown) => { +const createMockChain = (result: unknown, singleResult: unknown = null) => { const chain: Record = { select: vi.fn().mockReturnThis(), eq: vi.fn().mockReturnThis(), @@ -57,6 +66,10 @@ const createMockChain = (result: unknown) => { ilike: vi.fn().mockReturnThis(), order: vi.fn().mockReturnThis(), range: vi.fn().mockReturnThis(), + insert: vi.fn().mockReturnThis(), + update: vi.fn().mockReturnThis(), + single: vi.fn(() => Promise.resolve(singleResult)), + maybeSingle: vi.fn(() => Promise.resolve(singleResult)), then: (resolve: (v: unknown) => void) => Promise.resolve(result).then(resolve), }; return chain; @@ -287,3 +300,66 @@ describe('getIssuesPage', () => { }); }); }); + +describe('claimIssue', () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.unstubAllGlobals(); + mocks.mockGetUser.mockResolvedValue({ + data: { user: { id: 'user-1' } }, + }); + mocks.mockGetSession.mockResolvedValue({ + data: { session: null }, + }); + mocks.mockGetInstallOctokit.mockResolvedValue({ + repos: { get: vi.fn().mockResolvedValue({ data: { fork: false } }) }, + }); + mocks.mockRateLimit.mockResolvedValue({ ok: true, remaining: 19, resetAt: 0 }); + }); + + it('creates a claim for an issue in a repo the user does not own', async () => { + mocks.mockServiceFrom + .mockReturnValueOnce( + createMockChain( + {}, + { + data: { id: 1, difficulty: 'E', xp_reward: 50, repo_full_name: 'other/repo' }, + error: null, + }, + ), + ) // issues single + .mockReturnValueOnce( + createMockChain({}, { data: { github_handle: 'contributor' }, error: null }), + ) // profiles maybeSingle + .mockReturnValueOnce(createMockChain({ data: [{ id: 10 }] })) // github_installations + .mockReturnValueOnce( + createMockChain({ data: [{ repo_full_name: 'other/repo', installation_id: 10 }] }), + ) // installation_repositories + .mockReturnValueOnce(createMockChain({}, { data: null, error: null })) // recommendations lookup + .mockReturnValueOnce(createMockChain({}, { data: { id: 5 }, error: null })) // insert + .mockReturnValueOnce(createMockChain({})); // activity_log + + const result = await claimIssue(1); + + expect(result).toEqual({ ok: true, data: { recId: 5 } }); + }); + + it('rejects claims on issues in a repository the user owns', async () => { + mocks.mockServiceFrom + .mockReturnValueOnce( + createMockChain( + {}, + { + data: { id: 1, difficulty: 'E', xp_reward: 50, repo_full_name: 'owner/repo' }, + error: null, + }, + ), + ) // issues single + .mockReturnValueOnce(createMockChain({}, { data: { github_handle: 'owner' }, error: null })); // profiles maybeSingle + + const result = await claimIssue(1); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error.code).toBe('forbidden'); + }); +}); diff --git a/src/app/actions/issues.ts b/src/app/actions/issues.ts index f9cd85a9..0a5ed790 100644 --- a/src/app/actions/issues.ts +++ b/src/app/actions/issues.ts @@ -7,6 +7,7 @@ import { rateLimit, RATE_LIMIT_TIERS } from '@/lib/rate-limit'; import { cacheDel, cacheGet, cacheSet } from '@/lib/cache'; import { repoFilterPattern } from './issues-helpers'; import { getInstallOctokit } from '@/lib/github/app'; +import { isSelfMerge } from '@/lib/xp/self-merge'; const PAGE_SIZE = 10; const DIFFICULTY_VALUES = ['E', 'M', 'H'] as const; @@ -318,6 +319,18 @@ export async function claimIssue(issueId: number): Promise merge -> recommended-XP loop can never be entered. + const { data: profile } = await service + .from('profiles') + .select('github_handle') + .eq('id', user.id) + .maybeSingle(); + if (profile?.github_handle && isSelfMerge(issue.repo_full_name, profile.github_handle)) { + return err('forbidden', 'you cannot claim issues in a repository you own'); + } + // Validate that the issue belongs to a repo the user has access to. const repoOptsRes = await getRepoOptions(); if (!repoOptsRes.ok) return err(repoOptsRes.error.code, repoOptsRes.error.message); diff --git a/src/app/actions/recommendations.test.ts b/src/app/actions/recommendations.test.ts index dff4e4f9..bbfaaff0 100644 --- a/src/app/actions/recommendations.test.ts +++ b/src/app/actions/recommendations.test.ts @@ -261,6 +261,13 @@ describe('Recommendations Server Actions', () => { describe('claimRecommendation', () => { it('updates status to claimed and sets claimed_at, invalidating cache', async () => { mocks.mockServiceFrom + .mockReturnValueOnce(createMockChain(null, { data: { github_handle: 'contributor' } })) // profile + .mockReturnValueOnce( + createMockChain(null, { + data: { id: 1, issue_id: 10, issues: { repo_full_name: 'other/repo' } }, + error: null, + }), + ) // rec + repo .mockReturnValueOnce(createMockChain({ count: 0 })) // count claims .mockReturnValueOnce(createMockChain(null, { data: { id: 1 }, error: null })) // update .mockReturnValueOnce(createMockChain({})); // insert activity_log @@ -273,6 +280,13 @@ describe('Recommendations Server Actions', () => { it('returns already_claimed error if status is not open', async () => { mocks.mockServiceFrom + .mockReturnValueOnce(createMockChain(null, { data: { github_handle: 'contributor' } })) // profile + .mockReturnValueOnce( + createMockChain(null, { + data: { id: 1, issue_id: 10, issues: { repo_full_name: 'other/repo' } }, + error: null, + }), + ) // rec + repo .mockReturnValueOnce(createMockChain({ count: 0 })) // count claims .mockReturnValueOnce(createMockChain(null, { data: null, error: null })); // update returns null row @@ -285,7 +299,15 @@ describe('Recommendations Server Actions', () => { }); it('returns claim_limit error if user has 3 or more claims', async () => { - mocks.mockServiceFrom.mockReturnValueOnce(createMockChain({ count: 3 })); // count claims + mocks.mockServiceFrom + .mockReturnValueOnce(createMockChain(null, { data: { github_handle: 'contributor' } })) // profile + .mockReturnValueOnce( + createMockChain(null, { + data: { id: 1, issue_id: 10, issues: { repo_full_name: 'other/repo' } }, + error: null, + }), + ) // rec + repo + .mockReturnValueOnce(createMockChain({ count: 3 })); // count claims const result = await claimRecommendation(1); @@ -304,8 +326,31 @@ describe('Recommendations Server Actions', () => { if (!result.ok) expect(result.error.code).toBe('not_configured'); }); + it('rejects claims on issues in a repository the user owns', async () => { + mocks.mockServiceFrom + .mockReturnValueOnce(createMockChain(null, { data: { github_handle: 'owner' } })) // profile + .mockReturnValueOnce( + createMockChain(null, { + data: { id: 1, issue_id: 10, issues: { repo_full_name: 'owner/repo' } }, + error: null, + }), + ); // rec + repo + + const result = await claimRecommendation(1); + + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error.code).toBe('forbidden'); + }); + it('returns persist_failed error if update fails', async () => { mocks.mockServiceFrom + .mockReturnValueOnce(createMockChain(null, { data: { github_handle: 'contributor' } })) // profile + .mockReturnValueOnce( + createMockChain(null, { + data: { id: 1, issue_id: 10, issues: { repo_full_name: 'other/repo' } }, + error: null, + }), + ) // rec + repo .mockReturnValueOnce(createMockChain({ count: 0 })) // count claims .mockReturnValueOnce(createMockChain(null, { data: null, error: new Error('DB Error') })); diff --git a/src/app/actions/recommendations.ts b/src/app/actions/recommendations.ts index 07b01e0f..6f9285a5 100644 --- a/src/app/actions/recommendations.ts +++ b/src/app/actions/recommendations.ts @@ -11,6 +11,8 @@ import { filterAndRank, type ScoredIssue } from '@/lib/pipeline/recommend'; import { capDifficulty, getAllowedDifficulties } from '@/lib/pipeline/difficulty'; import { getInstallationToken } from '@/lib/github/app'; import { listMaintainerInstalls, listMaintainerRepos } from '@/lib/maintainer/detect'; +import { unwrapJoin } from '@/lib/supabase/inner-join'; +import { isSelfMerge } from '@/lib/xp/self-merge'; /** * Server actions for the recommendation lifecycle. @@ -113,6 +115,33 @@ export async function claimRecommendation(recId: number): Promise merge -> recommended-XP loop can never be entered. + const { data: profile } = await service + .from('profiles') + .select('github_handle') + .eq('id', user.id) + .maybeSingle(); + + const { data: recRow } = await service + .from('recommendations') + .select('id, issue_id, issues!inner(repo_full_name)') + .eq('id', recId) + .eq('user_id', user.id) + .maybeSingle(); + + const recIssue = unwrapJoin<{ repo_full_name?: string }>( + (recRow as unknown as { issues?: unknown }).issues, + ); + if ( + profile?.github_handle && + recIssue?.repo_full_name && + isSelfMerge(recIssue.repo_full_name, profile.github_handle) + ) { + return err('forbidden', 'you cannot claim issues in a repository you own'); + } + // Fast pre-check: reject early if the user is obviously at the limit. // This is not authoritative — two concurrent requests can both pass it — // but it avoids unnecessary write attempts under normal conditions. diff --git a/src/inngest/functions/process-pr-event.test.ts b/src/inngest/functions/process-pr-event.test.ts index b36345e0..ab7ff961 100644 --- a/src/inngest/functions/process-pr-event.test.ts +++ b/src/inngest/functions/process-pr-event.test.ts @@ -26,7 +26,7 @@ const prRun = processPrEvent as unknown as (ctx: { }) => Promise; // Factory for a pull_request closed & merged event. -const ev = (prUrl: string, repo: string, number: number) => ({ +const ev = (prUrl: string, repo: string, number: number, login = 'contributor') => ({ data: { payload: { action: 'closed', @@ -43,7 +43,7 @@ const ev = (prUrl: string, repo: string, number: number) => ({ closed_at: '2026-01-01T00:00:00Z', created_at: '2026-01-01T00:00:00Z', updated_at: '2026-01-01T00:00:00Z', - user: { login: 'contributor' }, + user: { login }, base: { repo: { full_name: repo } }, }, }, @@ -280,6 +280,24 @@ describe('processPrEvent - awardRecommendedMerge XP capping', () => { }), ); }); + + it('denies recommended merge XP when the PR author owns the repo (self_merge)', async () => { + const { activityLogMock } = setupMock({ + id: 99, + user_id: 'owner-user', + difficulty: 'H', + xp_reward: 400, + status: 'claimed', + }); + + await prRun({ + event: ev('https://github.com/owner/repo/pull/99', 'owner/repo', 99, 'owner'), + step, + }); + + expect(insertXpEvent).not.toHaveBeenCalled(); + expect(activityLogMock.insert).not.toHaveBeenCalled(); + }); }); describe('processPrEvent - linkPrToClaim issues relation array', () => { diff --git a/src/inngest/functions/process-pr-event.ts b/src/inngest/functions/process-pr-event.ts index 6c1ffe38..e023aa53 100644 --- a/src/inngest/functions/process-pr-event.ts +++ b/src/inngest/functions/process-pr-event.ts @@ -2,6 +2,7 @@ import { inngest } from '../client'; import { getServiceSupabase } from '@/lib/supabase/service'; import { insertXpEvent } from '@/lib/xp/events'; import { XP_SOURCE, xpForMerge, refIds, XP_REWARDS } from '@/lib/xp/sources'; +import { isSelfMerge } from '@/lib/xp/self-merge'; import { cacheDelByPrefix } from '@/lib/cache'; import { buildPrRow, type IngestiblePr } from '@/lib/maintainer/pr-ingest'; @@ -350,6 +351,11 @@ export async function handleMerge( const sb = getServiceSupabase(); if (!sb) throw new Error('service role missing'); + // Anti-abuse (doc rule — self-actions on own repo don't count): a user can + // never earn merge XP for a PR they merged into a repository they own, on + // the recommended path or the unrecommended path. + if (isSelfMerge(repo, pr.user.login)) return { xpAwarded: false, reason: 'self_merge' }; + // First try the linked rec. const { data: rec } = await sb .from('recommendations') @@ -378,12 +384,7 @@ export async function handleMerge( } } - // Truly unrecommended. Anti-abuse: no XP when the author merges into - // their own repo (doc rule — self-actions on own repo don't count). - const repoOwner = repo.split('/')[0]?.toLowerCase(); - const author = pr.user.login.toLowerCase(); - if (repoOwner === author) return { xpAwarded: false, reason: 'self_merge' }; - + // Truly unrecommended: award baseline XP to the PR author. const { data: profile } = await sb .from('profiles') .select('id') @@ -409,7 +410,13 @@ async function awardRecommendedMerge( rec: { id: number; user_id: string; difficulty: string; xp_reward: number | null }, repo: string, pr: PrPayload['pull_request'], -): Promise<{ xpAwarded: boolean; recId: number }> { +): Promise<{ xpAwarded: boolean; recId: number; reason?: string }> { + // Same anti-abuse rule as the unrecommended path. handleMerge already guards + // this, but keep it here too so the award site can never drift. + if (isSelfMerge(repo, pr.user.login)) { + return { xpAwarded: false, recId: rec.id, reason: 'self_merge' }; + } + const difficulty = rec.difficulty as 'E' | 'M' | 'H'; const tierCap = XP_REWARDS.RECOMMENDED_MERGE[difficulty as keyof typeof XP_REWARDS.RECOMMENDED_MERGE] ?? diff --git a/src/lib/xp/self-merge.test.ts b/src/lib/xp/self-merge.test.ts new file mode 100644 index 00000000..f94154f7 --- /dev/null +++ b/src/lib/xp/self-merge.test.ts @@ -0,0 +1,19 @@ +import { describe, it, expect } from 'vitest'; +import { isSelfMerge } from './self-merge'; + +describe('isSelfMerge', () => { + it('matches the repo owner case-insensitively', () => { + expect(isSelfMerge('Owner/Repo', 'owner')).toBe(true); + expect(isSelfMerge('owner/repo', 'OWNER')).toBe(true); + }); + + it('does not match a non-owner author', () => { + expect(isSelfMerge('owner/repo', 'contributor')).toBe(false); + }); + + it('is false for missing inputs', () => { + expect(isSelfMerge('', 'owner')).toBe(false); + expect(isSelfMerge('owner/repo', '')).toBe(false); + expect(isSelfMerge('norepo', 'norepo')).toBe(false); + }); +}); diff --git a/src/lib/xp/self-merge.ts b/src/lib/xp/self-merge.ts new file mode 100644 index 00000000..58f8a911 --- /dev/null +++ b/src/lib/xp/self-merge.ts @@ -0,0 +1,12 @@ +/** + * Anti-abuse rule from the docs: "self-actions on own repo don't count". + * A user never earns merge XP (and never claims work) in a repository they + * own, regardless of whether the merge was recommended or not. + * + * Shared by handleMerge and the claim actions so the two layers cannot drift. + */ +export function isSelfMerge(repoFullName: string, githubLogin: string): boolean { + const slash = repoFullName.indexOf('/'); + if (slash <= 0 || !githubLogin) return false; + return repoFullName.slice(0, slash).toLowerCase() === githubLogin.toLowerCase(); +}