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 index 2a9eb7c00..788f4a07f 100644 --- a/lib/github-owner-verification.test.ts +++ b/lib/github-owner-verification.test.ts @@ -7,7 +7,7 @@ describe('verifyGitHubOwner', () => { }); it('rejects requests without a bearer token', async () => { - const result = await verifyGitHubOwner(new Request('http://localhost/api/profile'), 'octocat'); + const result = await verifyGitHubOwner(new Request('http://localhost/api/notify'), 'octocat'); expect(result).toEqual({ verified: false, @@ -16,11 +16,11 @@ describe('verifyGitHubOwner', () => { }); }); - it('verifies a matching GitHub account', async () => { + 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/profile', { + const request = new Request('http://localhost/api/notify', { headers: { Authorization: 'Bearer caller-token' }, }); @@ -38,7 +38,7 @@ describe('verifyGitHubOwner', () => { vi.spyOn(globalThis, 'fetch').mockResolvedValue( new Response(JSON.stringify({ login: 'attacker' }), { status: 200 }) ); - const request = new Request('http://localhost/api/profile', { + const request = new Request('http://localhost/api/notify', { headers: { Authorization: 'Bearer attacker-token' }, }); @@ -47,9 +47,9 @@ describe('verifyGitHubOwner', () => { expect(result).toMatchObject({ verified: false, status: 403 }); }); - it('rejects invalid tokens without exposing them', async () => { + 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/profile', { + const request = new Request('http://localhost/api/notify', { headers: { Authorization: 'Bearer expired-token' }, }); @@ -61,7 +61,7 @@ describe('verifyGitHubOwner', () => { 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/profile', { + const request = new Request('http://localhost/api/notify', { headers: { Authorization: 'Bearer caller-token' }, });