From 991acaf56e1939166880d4a7847cb3c97d1f277b Mon Sep 17 00:00:00 2001 From: Krishna Kumar Date: Thu, 11 Jun 2026 16:18:36 +0530 Subject: [PATCH 1/2] fix: verify notification account ownership --- app/api/notify/route.test.ts | 78 ++++++++++++++++++++++- app/api/notify/route.ts | 20 +++++- lib/github-owner-verification.test.ts | 73 ++++++++++++++++++++++ lib/github-owner-verification.ts | 90 +++++++++++++++++++++++++++ 4 files changed, 258 insertions(+), 3 deletions(-) create mode 100644 lib/github-owner-verification.test.ts create mode 100644 lib/github-owner-verification.ts diff --git a/app/api/notify/route.test.ts b/app/api/notify/route.test.ts index 9f745eedb..77aefe3d1 100644 --- a/app/api/notify/route.test.ts +++ b/app/api/notify/route.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import { NextRequest } from 'next/server'; -import { GET, POST } from './route'; +import { DELETE, GET, POST } from './route'; import dbConnect from '@/lib/mongodb'; // Mock dependencies @@ -9,6 +9,7 @@ vi.mock('@/models/Notification', () => ({ Notification: { findOneAndUpdate: vi.fn(), findOne: vi.fn(), + deleteOne: vi.fn(), }, })); vi.mock('@/lib/rate-limit', () => ({ @@ -35,17 +36,24 @@ vi.mock('@/services/github/validate-user', () => ({ validateUser: vi.fn().mockResolvedValue(true), }, })); +vi.mock('@/lib/github-owner-verification', () => ({ + verifyGitHubOwner: vi.fn().mockResolvedValue({ verified: true }), +})); import { Notification } from '@/models/Notification'; import { notifyRateLimiter } from '@/lib/rate-limit'; import { gitHubUserValidator } from '@/services/github/validate-user'; import { getClientIp } from '@/utils/getClientIp'; +import { verifyGitHubOwner } from '@/lib/github-owner-verification'; const makeRequest = (method: string, body?: object, search?: string) => { const url = `http://localhost:3000/api/notify${search ? '?' + search : ''}`; return new NextRequest(url, { method, - headers: { 'x-forwarded-for': '127.0.0.1' }, + headers: { + 'x-forwarded-for': '127.0.0.1', + Authorization: 'Bearer test-owner-token', + }, body: body ? JSON.stringify(body) : undefined, }); }; @@ -58,6 +66,7 @@ describe('POST /api/notify', () => { process.env = { ...originalEnv, MONGODB_URI: 'mongodb://localhost/test' }; vi.mocked(notifyRateLimiter.check).mockResolvedValue(true); vi.mocked(gitHubUserValidator.validateUser).mockResolvedValue(true); + vi.mocked(verifyGitHubOwner).mockResolvedValue({ verified: true }); }); afterEach(() => { @@ -131,6 +140,33 @@ describe('POST /api/notify', () => { expect(res.headers.get('x-ratelimit-reset')).toBe(reset.toString()); }); + it('returns 401 when GitHub authentication is missing or invalid', async () => { + vi.mocked(verifyGitHubOwner).mockResolvedValueOnce({ + verified: false, + status: 401, + message: 'GitHub authentication is required.', + }); + + const res = await POST(makeRequest('POST', { username: 'testuser', email: 'a@b.com' })); + + expect(res.status).toBe(401); + expect(Notification.findOneAndUpdate).not.toHaveBeenCalled(); + }); + + it('returns 403 when the authenticated GitHub account does not own the username', async () => { + vi.mocked(verifyGitHubOwner).mockResolvedValueOnce({ + verified: false, + status: 403, + message: 'The authenticated GitHub account does not own this username.', + }); + + const res = await POST(makeRequest('POST', { username: 'victim', email: 'a@b.com' })); + + expect(res.status).toBe(403); + expect(verifyGitHubOwner).toHaveBeenCalledWith(expect.any(Request), 'victim'); + expect(Notification.findOneAndUpdate).not.toHaveBeenCalled(); + }); + // ── Per-username write cooldown ─────────────────────────────────────────── it('returns 429 when same username is written within cooldown period', async () => { @@ -225,6 +261,44 @@ describe('POST /api/notify', () => { }); }); +describe('DELETE /api/notify', () => { + const originalEnv = process.env; + + beforeEach(() => { + vi.clearAllMocks(); + process.env = { ...originalEnv, MONGODB_URI: 'mongodb://localhost/test' }; + vi.mocked(notifyRateLimiter.check).mockResolvedValue(true); + vi.mocked(verifyGitHubOwner).mockResolvedValue({ verified: true }); + }); + + afterEach(() => { + process.env = originalEnv; + }); + + it('rejects deletion when the authenticated account does not own the username', async () => { + vi.mocked(verifyGitHubOwner).mockResolvedValueOnce({ + verified: false, + status: 403, + message: 'The authenticated GitHub account does not own this username.', + }); + + const res = await DELETE(makeRequest('DELETE', undefined, 'user=victim')); + + expect(res.status).toBe(403); + expect(Notification.deleteOne).not.toHaveBeenCalled(); + }); + + it('deletes preferences after ownership is verified', async () => { + vi.mocked(Notification.deleteOne).mockResolvedValue({ deletedCount: 1 } as never); + + const res = await DELETE(makeRequest('DELETE', undefined, 'user=testuser')); + + expect(res.status).toBe(200); + expect(verifyGitHubOwner).toHaveBeenCalledWith(expect.any(Request), 'testuser'); + expect(Notification.deleteOne).toHaveBeenCalledWith({ username: 'testuser' }); + }); +}); + describe('GET /api/notify', () => { const originalEnv = process.env; diff --git a/app/api/notify/route.ts b/app/api/notify/route.ts index 665668a14..48d40d009 100644 --- a/app/api/notify/route.ts +++ b/app/api/notify/route.ts @@ -6,6 +6,7 @@ import { getClientIp } from '@/utils/getClientIp'; import { DistributedCache } from '@/lib/cache'; import { gitHubUserValidator } from '@/services/github/validate-user'; import { getRateLimitHeaders, notifyRateLimiter } from '@/lib/rate-limit'; +import { verifyGitHubOwner } from '@/lib/github-owner-verification'; const notifyWriteCache = new DistributedCache(5000, 60000); const NOTIFY_WRITE_COOLDOWN_MS = 5 * 60 * 1000; @@ -78,6 +79,14 @@ export async function POST(req: Request) { const { username, email, frequency, preferences } = parsed.data; const normalizedUsername = username.toLowerCase().trim(); + const ownership = await verifyGitHubOwner(req, normalizedUsername); + if (!ownership.verified) { + return NextResponse.json( + { success: false, message: ownership.message }, + { status: ownership.status } + ); + } + try { // Graceful MONGODB_URI handling if (!process.env.MONGODB_URI) { @@ -205,6 +214,15 @@ export async function DELETE(req: NextRequest) { } const { user: username } = parsed.data; + const normalizedUsername = username.toLowerCase(); + + const ownership = await verifyGitHubOwner(req, normalizedUsername); + if (!ownership.verified) { + return NextResponse.json( + { success: false, message: ownership.message }, + { status: ownership.status } + ); + } try { // Graceful MONGODB_URI handling @@ -230,7 +248,7 @@ export async function DELETE(req: NextRequest) { await dbConnect(); - const result = await Notification.deleteOne({ username: username.toLowerCase() }); + const result = await Notification.deleteOne({ username: normalizedUsername }); if (result.deletedCount === 0) { return NextResponse.json( diff --git a/lib/github-owner-verification.test.ts b/lib/github-owner-verification.test.ts new file mode 100644 index 000000000..788f4a07f --- /dev/null +++ b/lib/github-owner-verification.test.ts @@ -0,0 +1,73 @@ +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { verifyGitHubOwner } from './github-owner-verification'; + +describe('verifyGitHubOwner', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + it('rejects requests without a bearer token', async () => { + const result = await verifyGitHubOwner(new Request('http://localhost/api/notify'), 'octocat'); + + expect(result).toEqual({ + verified: false, + status: 401, + message: 'GitHub authentication is required.', + }); + }); + + it('verifies a matching GitHub account without leaking or storing the token', async () => { + const fetchSpy = vi + .spyOn(globalThis, 'fetch') + .mockResolvedValue(new Response(JSON.stringify({ login: 'OctoCat' }), { status: 200 })); + const request = new Request('http://localhost/api/notify', { + headers: { Authorization: 'Bearer caller-token' }, + }); + + await expect(verifyGitHubOwner(request, 'octocat')).resolves.toEqual({ verified: true }); + expect(fetchSpy).toHaveBeenCalledWith( + 'https://api.github.com/user', + expect.objectContaining({ + headers: expect.objectContaining({ Authorization: 'Bearer caller-token' }), + cache: 'no-store', + }) + ); + }); + + it('rejects a token belonging to a different GitHub account', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValue( + new Response(JSON.stringify({ login: 'attacker' }), { status: 200 }) + ); + const request = new Request('http://localhost/api/notify', { + headers: { Authorization: 'Bearer attacker-token' }, + }); + + const result = await verifyGitHubOwner(request, 'victim'); + + expect(result).toMatchObject({ verified: false, status: 403 }); + }); + + it('rejects invalid tokens with a controlled response', async () => { + vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response(null, { status: 401 })); + const request = new Request('http://localhost/api/notify', { + headers: { Authorization: 'Bearer expired-token' }, + }); + + const result = await verifyGitHubOwner(request, 'octocat'); + + expect(result).toMatchObject({ verified: false, status: 401 }); + expect(JSON.stringify(result)).not.toContain('expired-token'); + }); + + it('fails closed when GitHub ownership verification is unavailable', async () => { + vi.spyOn(globalThis, 'fetch').mockRejectedValue(new Error('network failure')); + const request = new Request('http://localhost/api/notify', { + headers: { Authorization: 'Bearer caller-token' }, + }); + + await expect(verifyGitHubOwner(request, 'octocat')).resolves.toMatchObject({ + verified: false, + status: 502, + }); + }); +}); diff --git a/lib/github-owner-verification.ts b/lib/github-owner-verification.ts new file mode 100644 index 000000000..fd8eb0341 --- /dev/null +++ b/lib/github-owner-verification.ts @@ -0,0 +1,90 @@ +const GITHUB_USER_API_URL = 'https://api.github.com/user'; +const VERIFY_TIMEOUT_MS = 5000; + +export type GitHubOwnerVerificationResult = + | { verified: true } + | { + verified: false; + status: 401 | 403 | 502; + message: string; + }; + +function getBearerToken(request: Request): string | null { + const authorization = request.headers.get('authorization'); + const match = authorization?.match(/^Bearer\s+(\S+)$/i); + return match?.[1] ?? null; +} + +/** + * Verifies that the caller's GitHub access token belongs to the requested username. + * + * Tokens are used only for the GitHub `/user` verification request and are never + * stored, logged, or included in error responses. + */ +export async function verifyGitHubOwner( + request: Request, + requestedUsername: string +): Promise { + const token = getBearerToken(request); + if (!token) { + return { + verified: false, + status: 401, + message: 'GitHub authentication is required.', + }; + } + + const controller = new AbortController(); + const timeoutId = setTimeout(() => controller.abort(), VERIFY_TIMEOUT_MS); + + try { + const response = await fetch(GITHUB_USER_API_URL, { + headers: { + Authorization: `Bearer ${token}`, + Accept: 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', + 'User-Agent': 'CommitPulse', + }, + cache: 'no-store', + signal: controller.signal, + }); + + if (response.status === 401) { + return { + verified: false, + status: 401, + message: 'Invalid or expired GitHub access token.', + }; + } + + if (!response.ok) { + return { + verified: false, + status: 502, + message: 'Unable to verify GitHub account ownership.', + }; + } + + const profile = (await response.json()) as { login?: unknown }; + if ( + typeof profile.login !== 'string' || + profile.login.toLowerCase() !== requestedUsername.trim().toLowerCase() + ) { + return { + verified: false, + status: 403, + message: 'The authenticated GitHub account does not own this username.', + }; + } + + return { verified: true }; + } catch { + return { + verified: false, + status: 502, + message: 'Unable to verify GitHub account ownership.', + }; + } finally { + clearTimeout(timeoutId); + } +} From 1ce749a3446699e27058a4a75a0feee6b00d06ae Mon Sep 17 00:00:00 2001 From: Krishna Kumar Date: Fri, 12 Jun 2026 11:20:52 +0530 Subject: [PATCH 2/2] fix: remove notification ownership merge artifacts --- lib/github-owner-verification.test.ts | 25 ------------------------- 1 file changed, 25 deletions(-) diff --git a/lib/github-owner-verification.test.ts b/lib/github-owner-verification.test.ts index d4115ef57..788f4a07f 100644 --- a/lib/github-owner-verification.test.ts +++ b/lib/github-owner-verification.test.ts @@ -7,10 +7,7 @@ describe('verifyGitHubOwner', () => { }); it('rejects requests without a bearer token', async () => { - fix/5193-notification-ownership const result = await verifyGitHubOwner(new Request('http://localhost/api/notify'), 'octocat'); - const result = await verifyGitHubOwner(new Request('http://localhost/api/profile'), 'octocat'); - main expect(result).toEqual({ verified: false, @@ -19,19 +16,11 @@ describe('verifyGitHubOwner', () => { }); }); - fix/5193-notification-ownership it('verifies a matching GitHub account without leaking or storing the token', async () => { const fetchSpy = vi .spyOn(globalThis, 'fetch') .mockResolvedValue(new Response(JSON.stringify({ login: 'OctoCat' }), { status: 200 })); const request = new Request('http://localhost/api/notify', { - - it('verifies a matching GitHub account', async () => { - const fetchSpy = vi - .spyOn(globalThis, 'fetch') - .mockResolvedValue(new Response(JSON.stringify({ login: 'OctoCat' }), { status: 200 })); - const request = new Request('http://localhost/api/profile', { - main headers: { Authorization: 'Bearer caller-token' }, }); @@ -49,11 +38,7 @@ describe('verifyGitHubOwner', () => { vi.spyOn(globalThis, 'fetch').mockResolvedValue( new Response(JSON.stringify({ login: 'attacker' }), { status: 200 }) ); - fix/5193-notification-ownership const request = new Request('http://localhost/api/notify', { - - const request = new Request('http://localhost/api/profile', { - main headers: { Authorization: 'Bearer attacker-token' }, }); @@ -62,15 +47,9 @@ describe('verifyGitHubOwner', () => { expect(result).toMatchObject({ verified: false, status: 403 }); }); - fix/5193-notification-ownership it('rejects invalid tokens with a controlled response', async () => { vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response(null, { status: 401 })); const request = new Request('http://localhost/api/notify', { - - it('rejects invalid tokens without exposing them', async () => { - vi.spyOn(globalThis, 'fetch').mockResolvedValue(new Response(null, { status: 401 })); - const request = new Request('http://localhost/api/profile', { - main headers: { Authorization: 'Bearer expired-token' }, }); @@ -82,11 +61,7 @@ describe('verifyGitHubOwner', () => { it('fails closed when GitHub ownership verification is unavailable', async () => { vi.spyOn(globalThis, 'fetch').mockRejectedValue(new Error('network failure')); -fix/5193-notification-ownership const request = new Request('http://localhost/api/notify', { - - const request = new Request('http://localhost/api/profile', { - main headers: { Authorization: 'Bearer caller-token' }, });