Skip to content

Commit

Permalink
feat(core): Rate-limit login endpoint to mitigate brute force passwor…
Browse files Browse the repository at this point in the history
…d guessing attacks (#9028)
  • Loading branch information
netroy committed Apr 3, 2024
1 parent 4668db2 commit a6446fe
Show file tree
Hide file tree
Showing 6 changed files with 65 additions and 14 deletions.
2 changes: 1 addition & 1 deletion packages/cli/src/controllers/auth.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export class AuthController {
) {}

/** Log in a user */
@Post('/login', { skipAuth: true })
@Post('/login', { skipAuth: true, rateLimit: true })
async login(req: LoginRequest, res: Response): Promise<PublicUser | undefined> {
const { email, password, mfaToken, mfaRecoveryCode } = req.body;
if (!email) throw new ApplicationError('Email is required to log in');
Expand Down
14 changes: 2 additions & 12 deletions packages/cli/src/controllers/passwordReset.controller.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Response } from 'express';
import { rateLimit } from 'express-rate-limit';
import validator from 'validator';

import { AuthService } from '@/auth/auth.service';
Expand All @@ -10,7 +9,7 @@ import { PasswordResetRequest } from '@/requests';
import { isSamlCurrentAuthenticationMethod } from '@/sso/ssoHelpers';
import { UserService } from '@/services/user.service';
import { License } from '@/License';
import { RESPONSE_ERROR_MESSAGES, inTest } from '@/constants';
import { RESPONSE_ERROR_MESSAGES } from '@/constants';
import { MfaService } from '@/Mfa/mfa.service';
import { Logger } from '@/Logger';
import { ExternalHooks } from '@/ExternalHooks';
Expand All @@ -23,12 +22,6 @@ import { NotFoundError } from '@/errors/response-errors/not-found.error';
import { UnprocessableRequestError } from '@/errors/response-errors/unprocessable.error';
import { UserRepository } from '@/databases/repositories/user.repository';

const throttle = rateLimit({
windowMs: 5 * 60 * 1000, // 5 minutes
limit: 5, // Limit each IP to 5 requests per `window` (here, per 5 minutes).
message: { message: 'Too many requests' },
});

@RestController()
export class PasswordResetController {
constructor(
Expand All @@ -48,10 +41,7 @@ export class PasswordResetController {
/**
* Send a password reset email.
*/
@Post('/forgot-password', {
middlewares: !inTest ? [throttle] : [],
skipAuth: true,
})
@Post('/forgot-password', { skipAuth: true, rateLimit: true })
async forgotPassword(req: PasswordResetRequest.Email) {
if (!this.mailer.isEmailSetUp) {
this.logger.debug(
Expand Down
3 changes: 3 additions & 0 deletions packages/cli/src/decorators/Route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ interface RouteOptions {
usesTemplates?: boolean;
/** When this flag is set to true, auth cookie isn't validated, and req.user will not be set */
skipAuth?: boolean;
/** When this flag is set to true, calls to this endpoint is rate limited to a max of 5 over a window of 5 minutes **/
rateLimit?: boolean;
}

const RouteFactory =
Expand All @@ -23,6 +25,7 @@ const RouteFactory =
handlerName: String(handlerName),
usesTemplates: options.usesTemplates ?? false,
skipAuth: options.skipAuth ?? false,
rateLimit: options.rateLimit ?? false,
});
Reflect.defineMetadata(CONTROLLER_ROUTES, routes, controllerClass);
};
Expand Down
19 changes: 18 additions & 1 deletion packages/cli/src/decorators/registerController.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { Container } from 'typedi';
import { Router } from 'express';
import type { Application, Request, Response, RequestHandler } from 'express';
import { rateLimit as expressRateLimit } from 'express-rate-limit';
import type { Scope } from '@n8n/permissions';
import { ApplicationError } from 'n8n-workflow';
import type { Class } from 'n8n-core';

import { AuthService } from '@/auth/auth.service';
import config from '@/config';
import { inE2ETests, inTest } from '@/constants';
import type { BooleanLicenseFeature } from '@/Interfaces';
import { License } from '@/License';
import type { AuthenticatedRequest } from '@/requests';
Expand All @@ -26,6 +28,12 @@ import type {
ScopeMetadata,
} from './types';

const throttle = expressRateLimit({
windowMs: 5 * 60 * 1000, // 5 minutes
limit: 5, // Limit each IP to 5 requests per `window` (here, per 5 minutes).
message: { message: 'Too many requests' },
});

export const createLicenseMiddleware =
(features: BooleanLicenseFeature[]): RequestHandler =>
(_req, res, next) => {
Expand Down Expand Up @@ -94,13 +102,22 @@ export const registerController = (app: Application, controllerClass: Class<obje
const authService = Container.get(AuthService);

routes.forEach(
({ method, path, middlewares: routeMiddlewares, handlerName, usesTemplates, skipAuth }) => {
({
method,
path,
middlewares: routeMiddlewares,
handlerName,
usesTemplates,
skipAuth,
rateLimit,
}) => {
const features = licenseFeatures?.[handlerName] ?? licenseFeatures?.['*'];
const scopes = requiredScopes?.[handlerName] ?? requiredScopes?.['*'];
const handler = async (req: Request, res: Response) =>
await controller[handlerName](req, res);
router[method](
path,
...(!inTest && !inE2ETests && rateLimit ? [throttle] : []),
// eslint-disable-next-line @typescript-eslint/unbound-method
...(skipAuth ? [] : [authService.authMiddleware]),
...(features ? [createLicenseMiddleware(features)] : []),
Expand Down
1 change: 1 addition & 0 deletions packages/cli/src/decorators/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface RouteMetadata {
middlewares: RequestHandler[];
usesTemplates: boolean;
skipAuth: boolean;
rateLimit: boolean;
}

export type Controller = Record<
Expand Down
40 changes: 40 additions & 0 deletions packages/cli/test/unit/decorators/registerController.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
jest.mock('@/constants', () => ({
inE2ETests: false,
inTest: false,
}));

import express from 'express';
import { agent as testAgent } from 'supertest';

import { Get, RestController, registerController } from '@/decorators';
import { AuthService } from '@/auth/auth.service';
import { mockInstance } from '../../shared/mocking';

describe('registerController', () => {
@RestController('/test')
class TestController {
@Get('/unlimited', { skipAuth: true })
@Get('/rate-limited', { skipAuth: true, rateLimit: true })
endpoint() {
return { ok: true };
}
}

mockInstance(AuthService);
const app = express();
registerController(app, TestController);
const agent = testAgent(app);

it('should not rate-limit by default', async () => {
for (let i = 0; i < 6; i++) {
await agent.get('/rest/test/unlimited').expect(200);
}
});

it('should rate-limit when configured', async () => {
for (let i = 0; i < 5; i++) {
await agent.get('/rest/test/rate-limited').expect(200);
}
await agent.get('/rest/test/rate-limited').expect(429);
});
});

0 comments on commit a6446fe

Please sign in to comment.