From 48440e5b7c6b2dc54181049fec67542a1ce081e5 Mon Sep 17 00:00:00 2001 From: Kristaps Fabians Geikins Date: Tue, 13 Feb 2024 15:08:48 +0200 Subject: [PATCH] fix(server): rate limiter path resolution (#2042) --- packages/server/logging/expressLogging.ts | 13 ++-- .../server/modules/auth/strategies/local.js | 8 +-- .../server/modules/auth/tests/auth.spec.js | 10 +-- .../modules/core/graph/resolvers/commits.js | 8 +-- .../modules/core/graph/resolvers/projects.ts | 8 +-- .../modules/core/graph/resolvers/streams.js | 6 +- .../server/modules/core/helpers/server.ts | 10 +++ .../modules/core/services/ratelimiter.ts | 61 +++++++------------ .../modules/core/tests/ratelimiter.spec.ts | 27 ++++---- 9 files changed, 62 insertions(+), 89 deletions(-) create mode 100644 packages/server/modules/core/helpers/server.ts diff --git a/packages/server/logging/expressLogging.ts b/packages/server/logging/expressLogging.ts index d76c54ea1a..1211d1af21 100644 --- a/packages/server/logging/expressLogging.ts +++ b/packages/server/logging/expressLogging.ts @@ -5,9 +5,10 @@ import { IncomingMessage } from 'http' import { NextFunction, Response } from 'express' import pino, { SerializedResponse } from 'pino' import { GenReqId } from 'pino-http' -import { get } from 'lodash' import type { ServerResponse } from 'http' import { Optional } from '@speckle/shared' +import { getRequestPath } from '@/modules/core/helpers/server' +import { get } from 'lodash' const REQUEST_ID_HEADER = 'x-request-id' @@ -44,8 +45,7 @@ export const LoggingExpressMiddleware = HttpLogger({ customSuccessObject(req, res, val: Record) { const isCompleted = !req.readableAborted && res.writableEnded const requestStatus = isCompleted ? 'completed' : 'aborted' - const requestPath = - (get(req, 'originalUrl') || get(req, 'url') || '').split('?')[0] || 'unknown' + const requestPath = getRequestPath(req) || 'unknown' const country = req.headers['cf-ipcountry'] as Optional return { @@ -59,10 +59,9 @@ export const LoggingExpressMiddleware = HttpLogger({ customErrorMessage() { return '{requestPath} request {requestStatus} in {responseTime} ms' }, - customErrorObject(req, res, err, val: Record) { + customErrorObject(req, _res, _err, val: Record) { const requestStatus = 'failed' - const requestPath = - (get(req, 'originalUrl') || get(req, 'url') || '').split('?')[0] || 'unknown' + const requestPath = getRequestPath(req) || 'unknown' const country = req.headers['cf-ipcountry'] as Optional return { @@ -81,7 +80,7 @@ export const LoggingExpressMiddleware = HttpLogger({ return { id: req.raw.id, method: req.raw.method, - path: (get(req.raw, 'originalUrl') || get(req.raw, 'url') || '').split('?')[0], + path: getRequestPath(req.raw), // Allowlist useful headers headers: Object.fromEntries( Object.entries(req.raw.headers).filter( diff --git a/packages/server/modules/auth/strategies/local.js b/packages/server/modules/auth/strategies/local.js index 6a2ebb4a89..ae46eabbc8 100644 --- a/packages/server/modules/auth/strategies/local.js +++ b/packages/server/modules/auth/strategies/local.js @@ -8,8 +8,7 @@ const { getServerInfo } = require('@/modules/core/services/generic') const { sendRateLimitResponse, getRateLimitResult, - isRateLimitBreached, - RateLimitAction + isRateLimitBreached } = require('@/modules/core/services/ratelimiter') const { validateServerInvite, @@ -71,10 +70,7 @@ module.exports = async (app, session, sessionAppId, finalizeAuth) => { const ip = getIpFromRequest(req) if (ip) user.ip = ip const source = ip ? ip : 'unknown' - const rateLimitResult = await getRateLimitResult( - RateLimitAction.USER_CREATE, - source - ) + const rateLimitResult = await getRateLimitResult('USER_CREATE', source) if (isRateLimitBreached(rateLimitResult)) { return sendRateLimitResponse(res, rateLimitResult) } diff --git a/packages/server/modules/auth/tests/auth.spec.js b/packages/server/modules/auth/tests/auth.spec.js index 3191c977b1..53969e074f 100644 --- a/packages/server/modules/auth/tests/auth.spec.js +++ b/packages/server/modules/auth/tests/auth.spec.js @@ -7,11 +7,7 @@ const { createStream } = require('@/modules/core/services/streams') const { updateServerInfo } = require('@/modules/core/services/generic') const { getUserByEmail } = require('@/modules/core/services/users') const { TIME } = require('@speckle/shared') -const { - RATE_LIMITERS, - createConsumer, - RateLimitAction -} = require('@/modules/core/services/ratelimiter') +const { RATE_LIMITERS, createConsumer } = require('@/modules/core/services/ratelimiter') const { beforeEachContext, initializeTestServer } = require('@/test/hooks') const { createInviteDirectly } = require('@/test/speckle-helpers/inviteHelper') const { getInvite } = require('@/modules/serverinvites/repositories') @@ -465,9 +461,9 @@ describe('Auth @auth', () => { const oldRateLimiter = RATE_LIMITERS.USER_CREATE RATE_LIMITERS.USER_CREATE = createConsumer( - RateLimitAction.USER_CREATE, + 'USER_CREATE', new RateLimiterMemory({ - keyPrefix: RateLimitAction.USER_CREATE, + keyPrefix: 'USER_CREATE', points: 1, duration: 1 * TIME.week }) diff --git a/packages/server/modules/core/graph/resolvers/commits.js b/packages/server/modules/core/graph/resolvers/commits.js index 7bdd742b9e..1832310795 100644 --- a/packages/server/modules/core/graph/resolvers/commits.js +++ b/packages/server/modules/core/graph/resolvers/commits.js @@ -32,8 +32,7 @@ const { getUser } = require('../../services/users') const { isRateLimitBreached, getRateLimitResult, - RateLimitError, - RateLimitAction + RateLimitError } = require('@/modules/core/services/ratelimiter') const { batchMoveCommits, @@ -198,10 +197,7 @@ module.exports = { context.resourceAccessRules ) - const rateLimitResult = await getRateLimitResult( - RateLimitAction.COMMIT_CREATE, - context.userId - ) + const rateLimitResult = await getRateLimitResult('COMMIT_CREATE', context.userId) if (isRateLimitBreached(rateLimitResult)) { throw new RateLimitError(rateLimitResult) } diff --git a/packages/server/modules/core/graph/resolvers/projects.ts b/packages/server/modules/core/graph/resolvers/projects.ts index 192d4c07ae..763a869b5b 100644 --- a/packages/server/modules/core/graph/resolvers/projects.ts +++ b/packages/server/modules/core/graph/resolvers/projects.ts @@ -15,8 +15,7 @@ import { } from '@/modules/core/repositories/streams' import { getRateLimitResult, - isRateLimitBreached, - RateLimitAction + isRateLimitBreached } from '@/modules/core/services/ratelimiter' import { createStreamReturnRecord, @@ -84,10 +83,7 @@ export = { return await updateStreamAndNotify(update, userId!, resourceAccessRules) }, async create(_parent, args, context) { - const rateLimitResult = await getRateLimitResult( - RateLimitAction.STREAM_CREATE, - context.userId! - ) + const rateLimitResult = await getRateLimitResult('STREAM_CREATE', context.userId!) if (isRateLimitBreached(rateLimitResult)) { throw new RateLimitError(rateLimitResult) } diff --git a/packages/server/modules/core/graph/resolvers/streams.js b/packages/server/modules/core/graph/resolvers/streams.js index 13069d2a4c..029101492a 100644 --- a/packages/server/modules/core/graph/resolvers/streams.js +++ b/packages/server/modules/core/graph/resolvers/streams.js @@ -21,7 +21,6 @@ const { const { authorizeResolver, validateScopes } = require(`@/modules/shared`) const { RateLimitError, - RateLimitAction, getRateLimitResult, isRateLimitBreached } = require('@/modules/core/services/ratelimiter') @@ -250,10 +249,7 @@ module.exports = { }, Mutation: { async streamCreate(parent, args, context) { - const rateLimitResult = await getRateLimitResult( - RateLimitAction.STREAM_CREATE, - context.userId - ) + const rateLimitResult = await getRateLimitResult('STREAM_CREATE', context.userId) if (isRateLimitBreached(rateLimitResult)) { throw new RateLimitError(rateLimitResult) } diff --git a/packages/server/modules/core/helpers/server.ts b/packages/server/modules/core/helpers/server.ts new file mode 100644 index 0000000000..c1298b1ba7 --- /dev/null +++ b/packages/server/modules/core/helpers/server.ts @@ -0,0 +1,10 @@ +import type { IncomingMessage } from 'http' +import type express from 'express' +import { get } from 'lodash' + +export const getRequestPath = (req: IncomingMessage | express.Request) => { + const path = (get(req, 'originalUrl') || get(req, 'url') || '').split( + '?' + )[0] as string + return path?.length ? path : null +} diff --git a/packages/server/modules/core/services/ratelimiter.ts b/packages/server/modules/core/services/ratelimiter.ts index cc98a2904f..a3b1a08304 100644 --- a/packages/server/modules/core/services/ratelimiter.ts +++ b/packages/server/modules/core/services/ratelimiter.ts @@ -16,29 +16,7 @@ import { getIpFromRequest } from '@/modules/shared/utils/ip' import { RateLimitError } from '@/modules/core/errors/ratelimit' import { rateLimiterLogger } from '@/logging/logging' import { createRedisClient } from '@/modules/shared/redis/redis' - -// typescript definitions -export enum RateLimitAction { - ALL_REQUESTS = 'ALL_REQUESTS', - USER_CREATE = 'USER_CREATE', - STREAM_CREATE = 'STREAM_CREATE', - COMMIT_CREATE = 'COMMIT_CREATE', - 'POST /api/getobjects/:streamId' = 'POST /api/getobjects/:streamId', - 'POST /api/diff/:streamId' = 'POST /api/diff/:streamId', - 'POST /objects/:streamId' = 'POST /objects/:streamId', - 'GET /objects/:streamId/:objectId' = 'GET /objects/:streamId/:objectId', - 'GET /objects/:streamId/:objectId/single' = 'GET /objects/:streamId/:objectId/single', - 'POST /graphql' = 'POST /graphql', - 'POST /auth/local/login' = 'POST /auth/local/login', - 'GET /auth/azure' = 'GET /auth/azure', - 'GET /auth/gh' = 'GET /auth/gh', - 'GET /auth/goog' = 'GET /auth/goog', - 'GET /auth/oidc' = 'GET /auth/oidc', - 'POST /auth/azure/callback' = 'POST /auth/azure/callback', - 'GET /auth/gh/callback' = 'GET /auth/gh/callback', - 'GET /auth/goog/callback' = 'GET /auth/goog/callback', - 'GET /auth/oidc/callback' = 'GET /auth/oidc/callback' -} +import { getRequestPath } from '@/modules/core/helpers/server' export interface RateLimitResult { isWithinLimits: boolean @@ -66,7 +44,7 @@ export type RateLimits = { } type RateLimiterOptions = { - [key in RateLimitAction]: BurstyRateLimiterOptions + [key: string]: BurstyRateLimiterOptions } export type RateLimiterMapping = { @@ -75,7 +53,9 @@ export type RateLimiterMapping = { ) => Promise } -export const LIMITS: RateLimiterOptions = { +export type RateLimitAction = keyof typeof LIMITS + +export const LIMITS = { ALL_REQUESTS: { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_ALL_REQUESTS', '500'), @@ -179,7 +159,7 @@ export const LIMITS: RateLimiterOptions = { duration: 1 * TIME.minute } }, - 'POST /auth/local/login': { + '/auth/local/login': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), duration: 10 * TIME.minute @@ -189,7 +169,7 @@ export const LIMITS: RateLimiterOptions = { duration: 30 * TIME.minute } }, - 'GET /auth/azure': { + '/auth/azure': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), duration: 10 * TIME.minute @@ -199,7 +179,7 @@ export const LIMITS: RateLimiterOptions = { duration: 30 * TIME.minute } }, - 'GET /auth/gh': { + '/auth/gh': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), duration: 10 * TIME.minute @@ -209,7 +189,7 @@ export const LIMITS: RateLimiterOptions = { duration: 30 * TIME.minute } }, - 'GET /auth/goog': { + '/auth/goog': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), duration: 10 * TIME.minute @@ -219,7 +199,7 @@ export const LIMITS: RateLimiterOptions = { duration: 30 * TIME.minute } }, - 'GET /auth/oidc': { + '/auth/oidc': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), duration: 10 * TIME.minute @@ -229,7 +209,7 @@ export const LIMITS: RateLimiterOptions = { duration: 30 * TIME.minute } }, - 'POST /auth/azure/callback': { + '/auth/azure/callback': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), duration: 10 * TIME.minute @@ -239,7 +219,7 @@ export const LIMITS: RateLimiterOptions = { duration: 30 * TIME.minute } }, - 'GET /auth/gh/callback': { + '/auth/gh/callback': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), duration: 10 * TIME.minute @@ -249,7 +229,7 @@ export const LIMITS: RateLimiterOptions = { duration: 30 * TIME.minute } }, - 'GET /auth/goog/callback': { + '/auth/goog/callback': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), duration: 10 * TIME.minute @@ -259,7 +239,7 @@ export const LIMITS: RateLimiterOptions = { duration: 30 * TIME.minute } }, - 'GET /auth/oidc/callback': { + '/auth/oidc/callback': { regularOptions: { limitCount: getIntFromEnv('RATELIMIT_GET_AUTH', '4'), duration: 10 * TIME.minute @@ -271,6 +251,8 @@ export const LIMITS: RateLimiterOptions = { } } +export const allActions = Object.keys(LIMITS) as RateLimitAction[] + export const sendRateLimitResponse = ( res: express.Response, rateLimitBreached: RateLimitBreached @@ -288,8 +270,12 @@ export const sendRateLimitResponse = ( } export const getActionForPath = (path: string, verb: string): RateLimitAction => { - const maybeAction = `${verb} ${path}` as keyof typeof RateLimitAction - return RateLimitAction[maybeAction] || RateLimitAction.ALL_REQUESTS + const maybeAction = `${verb} ${path}` as RateLimitAction + const maybeActionNoVerb = path as RateLimitAction + + if (LIMITS[maybeAction]) return maybeAction + if (LIMITS[maybeActionNoVerb]) return maybeActionNoVerb + return 'ALL_REQUESTS' } export const getSourceFromRequest = (req: express.Request): string => { @@ -308,7 +294,7 @@ export const createRateLimiterMiddleware = ( next: express.NextFunction ) => { if (isTestEnv()) return next() - const path = req.originalUrl ? req.originalUrl : req.path + const path = getRequestPath(req) || '' const action = getActionForPath(path, req.method) const source = getSourceFromRequest(req) @@ -358,7 +344,6 @@ const initializeRedisRateLimiters = ( maxRetriesPerRequest: null }) - const allActions = Object.values(RateLimitAction) const mapping = Object.fromEntries( allActions.map((action) => { const limits = options[action] diff --git a/packages/server/modules/core/tests/ratelimiter.spec.ts b/packages/server/modules/core/tests/ratelimiter.spec.ts index 6be60f1a07..1bd555828c 100644 --- a/packages/server/modules/core/tests/ratelimiter.spec.ts +++ b/packages/server/modules/core/tests/ratelimiter.spec.ts @@ -4,13 +4,14 @@ import { createRateLimiterMiddleware, getRateLimitResult, isRateLimitBreached, - RateLimitAction, getActionForPath, sendRateLimitResponse, RateLimitBreached, RateLimits, createConsumer, - RateLimiterMapping + RateLimiterMapping, + allActions, + RateLimitAction } from '@/modules/core/services/ratelimiter' import { expect } from 'chai' import httpMocks from 'node-mocks-http' @@ -23,7 +24,6 @@ type RateLimiterOptions = { const initializeInMemoryRateLimiters = ( options: RateLimiterOptions ): RateLimiterMapping => { - const allActions = Object.values(RateLimitAction) const mapping = Object.fromEntries( allActions.map((action) => { const limits = options[action] @@ -40,7 +40,6 @@ const initializeInMemoryRateLimiters = ( } const createTestRateLimiterMappings = () => { - const allActions = Object.values(RateLimitAction) const mapping = Object.fromEntries( allActions.map((action) => { return [action, { limitCount: 0, duration: 1 * TIME.week }] @@ -61,25 +60,25 @@ describe('Rate Limiting', () => { it('should rate limit known actions', async () => { const rateLimiterMapping = createTestRateLimiterMappings() const result = await getRateLimitResult( - RateLimitAction.STREAM_CREATE, + 'STREAM_CREATE', generateRandomIP(), rateLimiterMapping ) expect(isRateLimitBreached(result)).to.be.true - expect(result.action).to.equal(RateLimitAction.STREAM_CREATE) + expect(result.action).to.equal('STREAM_CREATE') }) }) describe('getActionForPath', () => { it('should rate limit unknown path as all request action', async () => { - expect(getActionForPath('/graphql', 'POST')).to.equal( - RateLimitAction['POST /graphql'] + expect(getActionForPath('/graphql', 'POST')).to.equal('POST /graphql') + expect(getActionForPath('/graphql', 'PATCH')).to.equal('ALL_REQUESTS') + expect(getActionForPath('/foobar', 'GET')).to.equal('ALL_REQUESTS') + expect(getActionForPath('/auth/local/login', 'POST')).to.equal( + '/auth/local/login' ) - expect(getActionForPath('/graphql', 'PATCH')).to.equal( - RateLimitAction.ALL_REQUESTS - ) - expect(getActionForPath('/foobar', 'GET')).to.equal(RateLimitAction.ALL_REQUESTS) + expect(getActionForPath('/auth/local/login', 'GET')).to.equal('/auth/local/login') }) }) @@ -87,7 +86,7 @@ describe('Rate Limiting', () => { it('should return 429 and set appropriate headers', async () => { const breached: RateLimitBreached = { isWithinLimits: false, - action: RateLimitAction['POST /graphql'], + action: 'POST /graphql', msBeforeNext: 4900 } const response = httpMocks.createResponse() @@ -112,7 +111,7 @@ describe('Rate Limiting', () => { const testMappings = createTestRateLimiterMappings() const limit = 100 testMappings[action] = createConsumer( - RateLimitAction[action], + action, new RateLimiterMemory({ keyPrefix: action, points: limit,