Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route rate limits (sec #18) #2530

Merged
merged 12 commits into from
Jul 27, 2023
Merged

Route rate limits (sec #18) #2530

merged 12 commits into from
Jul 27, 2023

Conversation

Steve-Mcl
Copy link
Contributor

@Steve-Mcl Steve-Mcl commented Jul 26, 2023

Description

Implements the rate-limiting part of sec #18

Commits:

  • add rateLimits.js and required plugin
  • setup routes for inclusion/exclusion of limiting
  • login and account create pages handle 429 errors
  • add tests and required fastify-routes plugin
  • add default config to ff yaml file
  • update docs for rate-limiting
  • sync package lock
  • add upgrade section with link to new docs

This PR is probably best reviewed by commit as they were made in logical order

Implementation detail

  1. Adds /forge/routes/rateLimits.js
    • This has one function: getLimits which returns the rate limit settings. It is directly exported for unit testing.
  2. /config/index.js
    • Adds rate_limits to config with default values by calling to rateLimits.getLimits(...)
  3. /forge/forge.js
    • Registers @fastify/rate-limit plugin - but only if config.rate_limits.enabled is true
  4. Any routes that should be rate limited have config: { rate_limits: app.config.rate_limits } added to their route options.
  5. Any routes that should not be rate limited have config: { rate_limits: false } added to their route options.

Endpoint Rate Limiting

Rate limiting will be disabled by default.

To enable rate limiting, set the config setting rate_limits.enabled to true.

When enabled, the following config settings are available:

  • rate_limits.global - (boolean, default: true) - enable/disable rate limiting for all routes
  • rate_limits.timeWindow - (number, default: "1 minute") - time window in milliseconds for rate_limits.max requests
  • rate_limits.max - (number, default: 1000) - max number of requests per rate_limits.timeWindow for a given route
  • rate_limits.maxAnonymous - (number, default: not set) - max number of requests per rate_limits.timeWindow for a given route for anonymous users
  • Additional settings are possible - see @fastify/rate-limit#options for more details.

Specific routes

The following routes will be rate limited by default:
  • POST /account/login
  • POST /account/register
  • POST /account/forgot_password
  • POST /account/reset_password/:token
The following routes will never be rate limited:
  • POST /api/comms/auth/client
  • POST /api/comms/auth/acl
  • POST /account/logout
  • POST /account/verify/:token
  • POST /account/verify
  • GET /account/authorize
  • POST /account/token
  • PUT /api/v1/devices/:deviceId/editor
  • GET /api/v1/devices/:deviceId/editor
  • GET /api/v1/devices/:deviceId/editor/token
  • GET /api/v1/devices/:deviceId/editor/comms/:access_token
  • GET /api/v1/devices/:deviceId/editor/proxy/*
  • POST /api/v1/devices/:deviceId/editor/proxy/*
  • PUT /api/v1/devices/:deviceId/editor/proxy/*
  • HEAD /api/v1/devices/:deviceId/editor/proxy/*
  • DELETE /api/v1/devices/:deviceId/editor/proxy/*
  • OPTIONS /api/v1/devices/:deviceId/editor/proxy/*
When config setting rate_limits.global is true.
  • Everything not explicitly excluded will be rate limited.
When config setting rate_limits.global is true.
  • Nothing else will be rate limited

package.json changes

  • Adds the following to package.json
    • dependencies
      • @fastify/rate-limit
    • devDependencies
      • @fastify/routes

Important notes

  • In order to get access the fastify routes, the plugin @fastify/routes added to the dev dependencies.
    • It is only loaded when the setting options.config.test?.fastifyRoutes is true.
    • NOTE: access to app.routes is very useful - may want to consider loading the routes plugin by default.

Adds tests in /test/unit/forge/routes/api/rateLimits/rateLimits_spec.js

NOTE: There are 3 sections of tests under " Rate Limiting Routes" for global:true and global:false:

  1. routes that are explicitly enabled (and should always have limiting regardless of global setting)
  2. routes that are explicitly disabled (and should never be limited regardless of global setting)
  3. a random selection of endpoints that should be enabled by global:true, otherwise disabled.

Test results (local execution)

Endpoint Rate Limiting
    getLimits unit tests
      ✔ should be disabled by default
      ✔ should get correct default values
      ✔ max should be a function when maxAnonymous is set
      ✔ keyGenerator should return sid if logged in, ip if not
      ✔ keyGenerator should return x-real-ip first, then x-forwarded-for, then ip
    Rate Limiting plugin
      ✔ Plugin is not loaded when config settings are not specified (implicitly disabled) (1038ms)
      ✔ Plugin is not loaded when config settings explicitly disables rate limits (693ms)
      ✔ Plugin is loaded when config settings are enabled (724ms)
    Rate Limiting Routes
      All Routes use limits except explicitly excluded (global:true)
        Routes that should always be limited
          ✔ Route POST /account/login should be rate limited
          ✔ Route POST /account/register should be rate limited
          ✔ Route POST /account/forgot_password should be rate limited
          ✔ Route POST /account/reset_password/:token should be rate limited
        Routes that should never be limited
          ✔ Route POST /api/comms/auth/client should never be rate limited
          ✔ Route POST /api/comms/auth/acl should never be rate limited
          ✔ Route POST /account/logout should never be rate limited
          ✔ Route POST /account/verify/:token should never be rate limited
          ✔ Route POST /account/verify should never be rate limited
          ✔ Route GET /account/authorize should never be rate limited
          ✔ Route POST /account/token should never be rate limited
          ✔ Route PUT /api/v1/devices/:deviceId/editor should never be rate limited
          ✔ Route GET /api/v1/devices/:deviceId/editor should never be rate limited
          ✔ Route GET /api/v1/devices/:deviceId/editor/token should never be rate limited
          ✔ Route GET /api/v1/devices/:deviceId/editor/comms/:access_token should never be rate limited
          ✔ Route GET /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route POST /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route PUT /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route HEAD /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route DELETE /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route OPTIONS /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
        Routes that should be limited because the global is true
          ✔ Route GET / should be rate limited
          ✔ Route GET /app/main.js should be rate limited
          ✔ Route GET /admin/overview should be rate limited
          ✔ Route GET /api/v1/teams/:teamId/members should be rate limited
          ✔ Route GET /api/v1/applications/:applicationId should be rate limited
          ✔ Route GET /api/v1/projects/:instanceId/logs should be rate limited
      Only specific Routes use limits (global:false)
        Routes that should always be limited
          ✔ Route POST /account/login should be rate limited
          ✔ Route POST /account/register should be rate limited
          ✔ Route POST /account/forgot_password should be rate limited
          ✔ Route POST /account/reset_password/:token should be rate limited
        Routes that should never be limited
          ✔ Route POST /api/comms/auth/client should never be rate limited
          ✔ Route POST /api/comms/auth/acl should never be rate limited
          ✔ Route POST /account/logout should never be rate limited
          ✔ Route POST /account/verify/:token should never be rate limited
          ✔ Route POST /account/verify should never be rate limited
          ✔ Route GET /account/authorize should never be rate limited
          ✔ Route POST /account/token should never be rate limited
          ✔ Route PUT /api/v1/devices/:deviceId/editor should never be rate limited
          ✔ Route GET /api/v1/devices/:deviceId/editor should never be rate limited
          ✔ Route GET /api/v1/devices/:deviceId/editor/token should never be rate limited
          ✔ Route GET /api/v1/devices/:deviceId/editor/comms/:access_token should never be rate limited
          ✔ Route GET /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route POST /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route PUT /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route HEAD /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route DELETE /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
          ✔ Route OPTIONS /api/v1/devices/:deviceId/editor/proxy/* should never be rate limited
        Routes that should not be limited because global is false
          ✔ Route GET / should not be rate limited
          ✔ Route GET /app/main.js should not be rate limited
          ✔ Route GET /admin/overview should not be rate limited
          ✔ Route GET /api/v1/teams/:teamId/members should not be rate limited
          ✔ Route GET /api/v1/applications/:applicationId should not be rate limited
          ✔ Route GET /api/v1/projects/:instanceId/logs should not be rate limited

Related Issue(s)

https://github.com/flowforge/security/issues/18

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on flowforge/helm to update ConfigMap Template
    • Issue/PR raised on flowforge/CloudProject to update values for Staging/Production

Labels

  • Backport needed? -> add the backport label
  • Includes a DB migration? -> add the area:migration label

@Steve-Mcl Steve-Mcl requested a review from knolleary July 26, 2023 09:14
@Steve-Mcl Steve-Mcl changed the title Sec18 rate limit Route rate limits (sec #18) Jul 26, 2023
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #2530 (0a80fad) into main (4785889) will decrease coverage by 34.54%.
Report is 105 commits behind head on main.
The diff coverage is 38.70%.

@@             Coverage Diff             @@
##             main    #2530       +/-   ##
===========================================
- Coverage   74.38%   39.85%   -34.54%     
===========================================
  Files         224      495      +271     
  Lines        8855    17741     +8886     
  Branches     1822     4141     +2319     
===========================================
+ Hits         6587     7070      +483     
- Misses       2268    10671     +8403     
Flag Coverage Δ
backend 74.51% <70.58%> (+0.12%) ⬆️
frontend 1.58% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
forge/comms/authRoutes.js 86.66% <ø> (-13.34%) ⬇️
forge/ee/routes/deviceEditor/index.js 73.28% <ø> (-3.52%) ⬇️
forge/routes/auth/oauth.js 55.05% <ø> (-2.02%) ⬇️
frontend/src/pages/Login.vue 0.00% <0.00%> (ø)
frontend/src/pages/account/Create.vue 0.00% <0.00%> (ø)
frontend/src/store/account.js 0.00% <0.00%> (ø)
forge/routes/auth/index.js 48.75% <18.18%> (-1.08%) ⬇️
forge/routes/rateLimits.js 94.11% <94.11%> (ø)
forge/config/index.js 67.69% <100.00%> (-3.50%) ⬇️
forge/forge.js 90.90% <100.00%> (+0.90%) ⬆️

... and 360 files with indirect coverage changes

@Steve-Mcl Steve-Mcl requested review from hardillb and removed request for knolleary July 27, 2023 16:39
Co-authored-by: Stephen McLaughlin <44235289+Steve-Mcl@users.noreply.github.com>
@hardillb hardillb merged commit db87b53 into main Jul 27, 2023
4 of 5 checks passed
@hardillb hardillb deleted the SEC18-rate-limit branch July 27, 2023 17:15
hardillb added a commit to FlowFuse/helm that referenced this pull request Jul 31, 2023
@hardillb hardillb mentioned this pull request Jul 31, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants