Skip to content

Commit fe3be65

Browse files
theothersideofgodclaude
theothersideofgod
and
claude
committed
feat(oauth): add rate limiting to OAuth endpoints
- Add express-rate-limit for OAuth initiation (10 req/min) and callback (30 req/min) - Skip rate limiting in development/test environments - Update tests with rate limiter mock Addresses PR #1141 Follow-up #3: OAuth rate limiting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 728e631 commit fe3be65

4 files changed

Lines changed: 86 additions & 18 deletions

File tree

‎graphql/server/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
"cors": "^2.8.6",
5959
"deepmerge": "^4.3.1",
6060
"express": "^5.2.1",
61+
"express-rate-limit": "^8.5.1",
6162
"gql-ast": "workspace:^",
6263
"grafast": "1.0.0",
6364
"grafserv": "1.0.0",

‎graphql/server/src/middleware/__tests__/oauth.test.ts‎

Lines changed: 41 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ jest.mock('@pgpmjs/logger', () => ({
2828
})),
2929
}));
3030

31+
jest.mock('express-rate-limit', () => {
32+
return jest.fn(() => (_req: any, _res: any, next: any) => next());
33+
});
34+
3135
// Import after mocks
3236
import { createOAuthRoutes } from '../oauth';
3337
import { OAuthClient } from '@constructive-io/oauth';
@@ -112,6 +116,27 @@ describe('OAuth Middleware', () => {
112116
// Should have 4 routes: /providers, /error, /:provider, /:provider/callback
113117
expect(router.stack.length).toBe(4);
114118
});
119+
120+
it('applies rate limiting to OAuth initiation and callback routes', () => {
121+
const router = createOAuthRoutes(mockOpts as any);
122+
123+
const initiateRoute = router.stack.find(
124+
(layer: any) => layer.route?.path === '/:provider' && layer.route?.methods?.get
125+
);
126+
const callbackRoute = router.stack.find(
127+
(layer: any) => layer.route?.path === '/:provider/callback'
128+
);
129+
const providersRoute = router.stack.find(
130+
(layer: any) => layer.route?.path === '/providers'
131+
);
132+
133+
// Initiate and callback routes should have 2 handlers (rate limiter + handler)
134+
expect(initiateRoute!.route.stack.length).toBe(2);
135+
expect(callbackRoute!.route.stack.length).toBe(2);
136+
137+
// Providers route should have 1 handler (no rate limiting)
138+
expect(providersRoute!.route.stack.length).toBe(1);
139+
});
115140
});
116141

117142
describe('Providers Endpoint', () => {
@@ -126,7 +151,7 @@ describe('OAuth Middleware', () => {
126151
const providersRoute = router.stack.find(
127152
(layer: any) => layer.route?.path === '/providers'
128153
);
129-
const handler = providersRoute!.route.stack[0].handle;
154+
const handler = providersRoute!.route.stack.slice(-1)[0].handle;
130155

131156
await handler(req, res, jest.fn());
132157

@@ -152,7 +177,7 @@ describe('OAuth Middleware', () => {
152177
const providersRoute = router.stack.find(
153178
(layer: any) => layer.route?.path === '/providers'
154179
);
155-
const handler = providersRoute!.route.stack[0].handle;
180+
const handler = providersRoute!.route.stack.slice(-1)[0].handle;
156181

157182
await handler(req, res, jest.fn());
158183

@@ -177,7 +202,7 @@ describe('OAuth Middleware', () => {
177202
const initiateRoute = router.stack.find(
178203
(layer: any) => layer.route?.path === '/:provider' && layer.route?.methods?.get
179204
);
180-
const handler = initiateRoute!.route.stack[0].handle;
205+
const handler = initiateRoute!.route.stack.slice(-1)[0].handle;
181206

182207
await handler(req, res, jest.fn());
183208

@@ -213,7 +238,7 @@ describe('OAuth Middleware', () => {
213238
const initiateRoute = router.stack.find(
214239
(layer: any) => layer.route?.path === '/:provider' && layer.route?.methods?.get
215240
);
216-
const handler = initiateRoute!.route.stack[0].handle;
241+
const handler = initiateRoute!.route.stack.slice(-1)[0].handle;
217242

218243
await handler(req, res, jest.fn());
219244

@@ -253,7 +278,7 @@ describe('OAuth Middleware', () => {
253278
const initiateRoute = router.stack.find(
254279
(layer: any) => layer.route?.path === '/:provider' && layer.route?.methods?.get
255280
);
256-
const handler = initiateRoute!.route.stack[0].handle;
281+
const handler = initiateRoute!.route.stack.slice(-1)[0].handle;
257282

258283
await handler(req, res, jest.fn());
259284

@@ -312,7 +337,7 @@ describe('OAuth Middleware', () => {
312337
const callbackRoute = router.stack.find(
313338
(layer: any) => layer.route?.path === '/:provider/callback'
314339
);
315-
const handler = callbackRoute!.route.stack[0].handle;
340+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
316341

317342
await handler(req, res, jest.fn());
318343

@@ -334,7 +359,7 @@ describe('OAuth Middleware', () => {
334359
const callbackRoute = router.stack.find(
335360
(layer: any) => layer.route?.path === '/:provider/callback'
336361
);
337-
const handler = callbackRoute!.route.stack[0].handle;
362+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
338363

339364
await handler(req, res, jest.fn());
340365

@@ -362,7 +387,7 @@ describe('OAuth Middleware', () => {
362387
const callbackRoute = router.stack.find(
363388
(layer: any) => layer.route?.path === '/:provider/callback'
364389
);
365-
const handler = callbackRoute!.route.stack[0].handle;
390+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
366391

367392
await handler(req, res, jest.fn());
368393

@@ -383,7 +408,7 @@ describe('OAuth Middleware', () => {
383408
const callbackRoute = router.stack.find(
384409
(layer: any) => layer.route?.path === '/:provider/callback'
385410
);
386-
const handler = callbackRoute!.route.stack[0].handle;
411+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
387412

388413
await handler(req, res, jest.fn());
389414

@@ -457,7 +482,7 @@ describe('OAuth Middleware', () => {
457482
const callbackRoute = router.stack.find(
458483
(layer: any) => layer.route?.path === '/:provider/callback'
459484
);
460-
const handler = callbackRoute!.route.stack[0].handle;
485+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
461486

462487
await handler(req, res, jest.fn());
463488

@@ -536,7 +561,7 @@ describe('OAuth Middleware', () => {
536561
const callbackRoute = router.stack.find(
537562
(layer: any) => layer.route?.path === '/:provider/callback'
538563
);
539-
const handler = callbackRoute!.route.stack[0].handle;
564+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
540565

541566
await handler(req, res, jest.fn());
542567

@@ -628,7 +653,7 @@ describe('OAuth Middleware', () => {
628653
const callbackRoute = router.stack.find(
629654
(layer: any) => layer.route?.path === '/:provider/callback'
630655
);
631-
const handler = callbackRoute!.route.stack[0].handle;
656+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
632657

633658
await handler(req, res, jest.fn());
634659

@@ -706,7 +731,7 @@ describe('OAuth Middleware', () => {
706731
const callbackRoute = router.stack.find(
707732
(layer: any) => layer.route?.path === '/:provider/callback'
708733
);
709-
const handler = callbackRoute!.route.stack[0].handle;
734+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
710735

711736
await handler(req, res, jest.fn());
712737

@@ -787,7 +812,7 @@ describe('OAuth Middleware', () => {
787812
const callbackRoute = router.stack.find(
788813
(layer: any) => layer.route?.path === '/:provider/callback'
789814
);
790-
const handler = callbackRoute!.route.stack[0].handle;
815+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
791816

792817
await handler(req, res, jest.fn());
793818

@@ -865,7 +890,7 @@ describe('OAuth Middleware', () => {
865890
const callbackRoute = router.stack.find(
866891
(layer: any) => layer.route?.path === '/:provider/callback'
867892
);
868-
const handler = callbackRoute!.route.stack[0].handle;
893+
const handler = callbackRoute!.route.stack.slice(-1)[0].handle;
869894

870895
await handler(req, res, jest.fn());
871896

@@ -919,7 +944,7 @@ describe('OAuth Middleware', () => {
919944
const initiateRoute = router.stack.find(
920945
(layer: any) => layer.route?.path === '/:provider' && layer.route?.methods?.get
921946
);
922-
const handler = initiateRoute!.route.stack[0].handle;
947+
const handler = initiateRoute!.route.stack.slice(-1)[0].handle;
923948

924949
await handler(req, res, jest.fn());
925950

‎graphql/server/src/middleware/oauth.ts‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import crypto from 'crypto';
22
import { Router, Request, Response } from 'express';
3+
import rateLimit from 'express-rate-limit';
34
import { OAuthClient, OAuthProfile } from '@constructive-io/oauth';
45
import { Logger } from '@pgpmjs/logger';
56
import { Pool } from 'pg';
@@ -238,6 +239,27 @@ export function createOAuthRoutes(opts: ConstructiveOptions): Router {
238239
const allowSignup = oauthConfig?.allowSignup ?? true;
239240
const requireVerifiedEmail = oauthConfig?.requireVerifiedEmail ?? true;
240241

242+
// Rate limiters for OAuth endpoints (disabled in development/test)
243+
const skipRateLimit = process.env.NODE_ENV === 'development' || process.env.NODE_ENV === 'test';
244+
245+
const oauthInitLimiter = rateLimit({
246+
windowMs: 60 * 1000, // 1 minute
247+
max: 10, // 10 requests per minute per IP
248+
skip: () => skipRateLimit,
249+
message: { error: 'TOO_MANY_REQUESTS', message: 'Too many OAuth requests, please try again later' },
250+
standardHeaders: true,
251+
legacyHeaders: false,
252+
});
253+
254+
const oauthCallbackLimiter = rateLimit({
255+
windowMs: 60 * 1000, // 1 minute
256+
max: 30, // 30 requests per minute per IP
257+
skip: () => skipRateLimit,
258+
message: { error: 'TOO_MANY_REQUESTS', message: 'Too many OAuth callback requests, please try again later' },
259+
standardHeaders: true,
260+
legacyHeaders: false,
261+
});
262+
241263
// GET /auth/providers - List available providers from database
242264
router.get('/providers', async (req: Request, res: Response) => {
243265
if (!req.api?.rlsModule?.privateSchema?.schemaName) {
@@ -264,7 +286,7 @@ export function createOAuthRoutes(opts: ConstructiveOptions): Router {
264286
});
265287

266288
// GET /auth/:provider - Initiate OAuth flow
267-
router.get('/:provider', async (req: Request, res: Response) => {
289+
router.get('/:provider', oauthInitLimiter, async (req: Request, res: Response) => {
268290
const { provider } = req.params;
269291
const redirectUri = (req.query.redirect_uri as string) || '/';
270292

@@ -326,7 +348,7 @@ export function createOAuthRoutes(opts: ConstructiveOptions): Router {
326348
});
327349

328350
// GET /auth/:provider/callback - Handle OAuth callback
329-
router.get('/:provider/callback', async (req: Request, res: Response) => {
351+
router.get('/:provider/callback', oauthCallbackLimiter, async (req: Request, res: Response) => {
330352
const { provider } = req.params;
331353
const { code, state, error: oauthError, error_description } = req.query;
332354

‎pnpm-lock.yaml‎

Lines changed: 20 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)