diff --git a/convex/lib/githubAccount.test.ts b/convex/lib/githubAccount.test.ts index 88013bd..a1070b5 100644 --- a/convex/lib/githubAccount.test.ts +++ b/convex/lib/githubAccount.test.ts @@ -8,6 +8,7 @@ vi.mock('../_generated/api', () => ({ internal: { users: { getByIdInternal: Symbol('getByIdInternal'), + getGithubAccountIdInternal: Symbol('getGithubAccountIdInternal'), updateGithubMetaInternal: Symbol('updateGithubMetaInternal'), }, }, @@ -20,6 +21,10 @@ describe('requireGitHubAccountAge', () => { vi.restoreAllMocks() }) + afterEach(() => { + vi.useRealTimers() + }) + it('uses cached githubCreatedAt when fresh', async () => { vi.useFakeTimers() const now = new Date('2026-02-02T12:00:00Z') @@ -39,8 +44,6 @@ describe('requireGitHubAccountAge', () => { expect(fetchMock).not.toHaveBeenCalled() expect(runMutation).not.toHaveBeenCalled() expect(runQuery).toHaveBeenCalledWith(internal.users.getByIdInternal, { userId: 'users:1' }) - - vi.useRealTimers() }) it('rejects accounts younger than 7 days', async () => { @@ -58,25 +61,26 @@ describe('requireGitHubAccountAge', () => { await expect( requireGitHubAccountAge({ runQuery, runMutation } as never, 'users:1' as never), ).rejects.toThrow(/GitHub account must be at least 7 days old/i) - - vi.useRealTimers() }) - it('refreshes githubCreatedAt when cache is stale', async () => { + it('refreshes githubCreatedAt when cache is stale (no stored GitHub ID)', async () => { vi.useFakeTimers() const now = new Date('2026-02-02T12:00:00Z') vi.setSystemTime(now) - const runQuery = vi.fn().mockResolvedValue({ - _id: 'users:1', - handle: 'steipete', - githubCreatedAt: undefined, - githubFetchedAt: now.getTime() - 2 * ONE_DAY_MS, - }) + const runQuery = vi.fn() + .mockResolvedValueOnce({ + _id: 'users:1', + handle: 'steipete', + githubCreatedAt: undefined, + githubFetchedAt: now.getTime() - 2 * ONE_DAY_MS, + }) + .mockResolvedValueOnce(null) // No stored GitHub ID (legacy user) const runMutation = vi.fn() const fetchMock = vi.fn().mockResolvedValue({ ok: true, json: async () => ({ + id: 12345, created_at: '2020-01-01T00:00:00Z', }), }) @@ -93,8 +97,6 @@ describe('requireGitHubAccountAge', () => { githubCreatedAt: Date.parse('2020-01-01T00:00:00Z'), githubFetchedAt: now.getTime(), }) - - vi.useRealTimers() }) it('throws when GitHub lookup fails', async () => { @@ -112,4 +114,88 @@ describe('requireGitHubAccountAge', () => { requireGitHubAccountAge({ runQuery, runMutation } as never, 'users:1' as never), ).rejects.toThrow(/GitHub account lookup failed/i) }) + + it('rejects when GitHub user ID does not match (username swap attack)', async () => { + vi.useFakeTimers() + const now = new Date('2026-02-02T12:00:00Z') + vi.setSystemTime(now) + + const runQuery = vi.fn() + .mockResolvedValueOnce({ + _id: 'users:1', + handle: 'oldusername', + githubCreatedAt: undefined, + githubFetchedAt: 0, + }) + .mockResolvedValueOnce('12345') // getGithubAccountIdInternal returns stored ID + const runMutation = vi.fn() + const fetchMock = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + id: 99999, // Different user now owns this username + created_at: '2020-01-01T00:00:00Z', + }), + }) + vi.stubGlobal('fetch', fetchMock) + + await expect( + requireGitHubAccountAge({ runQuery, runMutation } as never, 'users:1' as never), + ).rejects.toThrow(/GitHub account mismatch/i) + }) + + it('rejects when GitHub API omits user ID but stored ID exists (fail closed)', async () => { + vi.useFakeTimers() + const now = new Date('2026-02-02T12:00:00Z') + vi.setSystemTime(now) + + const runQuery = vi.fn() + .mockResolvedValueOnce({ + _id: 'users:1', + handle: 'someuser', + githubCreatedAt: undefined, + githubFetchedAt: 0, + }) + .mockResolvedValueOnce('12345') // Stored GitHub ID exists + const runMutation = vi.fn() + const fetchMock = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + // id is missing from response + created_at: '2020-01-01T00:00:00Z', + }), + }) + vi.stubGlobal('fetch', fetchMock) + + await expect( + requireGitHubAccountAge({ runQuery, runMutation } as never, 'users:1' as never), + ).rejects.toThrow(/unable to verify user identity/i) + }) + + it('allows when GitHub user ID matches', async () => { + vi.useFakeTimers() + const now = new Date('2026-02-02T12:00:00Z') + vi.setSystemTime(now) + + const runQuery = vi.fn() + .mockResolvedValueOnce({ + _id: 'users:1', + handle: 'steipete', + githubCreatedAt: undefined, + githubFetchedAt: 0, + }) + .mockResolvedValueOnce('12345') // getGithubAccountIdInternal returns stored ID + const runMutation = vi.fn() + const fetchMock = vi.fn().mockResolvedValue({ + ok: true, + json: async () => ({ + id: 12345, // Same user ID + created_at: '2020-01-01T00:00:00Z', + }), + }) + vi.stubGlobal('fetch', fetchMock) + + await requireGitHubAccountAge({ runQuery, runMutation } as never, 'users:1' as never) + + expect(runMutation).toHaveBeenCalled() + }) }) diff --git a/convex/lib/githubAccount.ts b/convex/lib/githubAccount.ts index e80618f..0df44a8 100644 --- a/convex/lib/githubAccount.ts +++ b/convex/lib/githubAccount.ts @@ -8,6 +8,7 @@ const MIN_ACCOUNT_AGE_MS = 7 * 24 * 60 * 60 * 1000 const FETCH_TTL_MS = 24 * 60 * 60 * 1000 type GitHubUser = { + id?: number created_at?: string } @@ -33,6 +34,17 @@ export async function requireGitHubAccountAge(ctx: ActionCtx, userId: Id<'users' const parsed = payload.created_at ? Date.parse(payload.created_at) : Number.NaN if (!Number.isFinite(parsed)) throw new ConvexError('GitHub account lookup failed') + // Verify the GitHub user ID matches to prevent username swap attacks + const storedGithubId = await ctx.runQuery(internal.users.getGithubAccountIdInternal, { userId }) + if (storedGithubId) { + if (payload.id === undefined) { + throw new ConvexError('GitHub account lookup failed - unable to verify user identity') + } + if (String(payload.id) !== storedGithubId) { + throw new ConvexError('GitHub account mismatch - username may have changed ownership') + } + } + createdAt = parsed await ctx.runMutation(internal.users.updateGithubMetaInternal, { userId, diff --git a/convex/users.ts b/convex/users.ts index d229872..bfe9115 100644 --- a/convex/users.ts +++ b/convex/users.ts @@ -20,6 +20,17 @@ export const getByIdInternal = internalQuery({ handler: async (ctx, args) => ctx.db.get(args.userId), }) +export const getGithubAccountIdInternal = internalQuery({ + args: { userId: v.id('users') }, + handler: async (ctx, args) => { + const account = await ctx.db + .query('authAccounts') + .withIndex('userIdAndProvider', (q) => q.eq('userId', args.userId).eq('provider', 'github')) + .unique() + return account?.providerAccountId ?? null + }, +}) + export const updateGithubMetaInternal = internalMutation({ args: { userId: v.id('users'),