fix(server): rate limiter path resolution (specklesystems#2042)

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<string, unknown>) {
     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<string>
 
     return {
@@ -59,10 +59,9 @@ export const LoggingExpressMiddleware = HttpLogger({
   customErrorMessage() {
     return '{requestPath} request {requestStatus} in {responseTime} ms'
   },
-  customErrorObject(req, res, err, val: Record<string, unknown>) {
+  customErrorObject(req, _res, _err, val: Record<string, unknown>) {
     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<string>
 
     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(

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)
         }

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
         })

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)
       }

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)
       }

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)
       }

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
+}

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<RateLimitSuccess | RateLimitBreached>
 }
 
-export const LIMITS: RateLimiterOptions = {
+export type RateLimitAction = keyof typeof LIMITS
+
+export const LIMITS = <const>{
   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]