Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
78 changes: 76 additions & 2 deletions app/api/notify/route.test.ts
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -9,6 +9,7 @@ vi.mock('@/models/Notification', () => ({
Notification: {
findOneAndUpdate: vi.fn(),
findOne: vi.fn(),
deleteOne: vi.fn(),
},
}));
vi.mock('@/lib/rate-limit', () => ({
Expand All @@ -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,
});
};
Expand All @@ -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(() => {
Expand Down Expand Up @@ -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 () => {
Expand Down Expand Up @@ -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;

Expand Down
20 changes: 19 additions & 1 deletion app/api/notify/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<number>(5000, 60000);
const NOTIFY_WRITE_COOLDOWN_MS = 5 * 60 * 1000;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down
14 changes: 7 additions & 7 deletions lib/github-owner-verification.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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' },
});

Expand All @@ -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' },
});

Expand All @@ -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' },
});

Expand All @@ -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' },
});

Expand Down
Loading