express-rate-limit / express-slow-down Goto Github PK

I am trying to dynamically turn on and off the rate slowdown. Whenever I set the delayAfter = 0 it defaults to 1 and immediately starts to slow down the requests. Since this is the only option that supports using a function handler for returning the option value this is the only option we can use to handle this use case.

I'm getting this error when trying to set up a simple example with express-slowdown and rate-limit-redis. Here's the full example:

import express from 'express';
import RedisStore from 'rate-limit-redis';
import slowDown from 'express-slow-down';
import { createClient } from 'redis';

(async () => {
  const client = createClient();

  await client.connect();

  const app = express();

  const speedLimiter = slowDown({
    windowMs: 30 * 1000,
    delayAfter: 1,
    delayMs: 500,
    store: new RedisStore({
      sendCommand: (...args) => client.sendCommand(args),
    }),
  });

  app.use(speedLimiter);

  app.all('*', (req, res) => {
    res.send('Hello');
  });

  app.listen(3000, () => {
    console.log('Server listening on port 3000');
  });
})();

Versions of relevant packages:

[email protected]
[email protected]
[email protected]
[email protected]

I'm assuming the error is on my end somewhere, but any help would be greatly appreciated. Thanks!

Description

Are you aware of the express.js malformed URLs vulnerability?

Ref: GHSA-rv95-896h-c2vc
Ref: https://nvd.nist.gov/vuln/detail/CVE-2024-29041

"Open Redirect: express is vulnerable to Open Redirect. The vulnerability is due to improper handling of user-provided URLs during redirection in Express.js, which performs encoding using the encodeurl library before passing it to the 'location' header. It allows bypass of properly implemented allow lists and leading to an Open Redirect vulnerability"

We are using express-rate-limit in its latest version, and Veracode reports this vulnerability.
Is there going to be a fix for this?

Library version

2.0.1

Node version

= 16

Typescript version (if you are using it)

No response

Module system

ESM

We need to update express-slow-down to support the v2 store interface that was introduced in express-rate-limit v6.

This could be done by either directly adding support, or by re-working express-slow-down to build on top of express-rate-limit, thereby inheriting it's store support.

Relates to #40

I want the delay to be for each IP accessing the JSON API separately.
For example:
Client 1 --> Server (100 Requests in under 1 Minute)
Client 2 --> Server (less than 100 Requests in under 1 Minute)

Is the slow down for all Clients or only for Client 1?
(I have already configured slowDown to match my little Demo above:

const speedLimiter = slowDown({
    windowMs: 1 * 60 * 1000, // 1 minute
    delayAfter: 100, // allow 100 requests per minute, then...
    delayMs: 500 // add delay to every request after 100 requests in 1 minute
});

Thanks for any answers,
Alex

I think it would be cool if you could enable logging of the request / slow down details when a slow down occurs.

i was using onLimitReached to log the possible attacker's ip, but figured out that req.ip is undefiend.
in addition, the limit is applying to all computers, because everyone's req.ip is undefiend!

Essentially, the current options skipSuccessfulRequests and skipFailedRequests take boolean values which modify the counting behavior by either adding 0 or 1 to the counter. This could be taken a step further:

Introduce two new options:

successfulRequestWeight: number 
failedRequestWeight: number

Deprecate the existing options:

skipSuccessfulRequests = true -> successfulRequestWeight = 0
skipSuccessfulRequests = false -> successfulRequestWeight = 1
skipFailedRequests = true -> failedRequestWeight = 0
skipFailedRequests = false -> failedRequestWeight = 1

Update the stores (in a backward-compatible way):

increment: (key: any, cb: (err: Error | null, current: number, resetTime: number) => unknown, amount: number = 1)
decrement: (key: any, amount: number = 1)

This would allow anyone to configure just how much successful or failed requests contribute towards the limit, while still keeping the options simple.

If you'd like this (or something similar), I'd be happy to provide a pull request for express-slow-down and/or express-rate-limit

The exported object RedisStore, of npm module rate-limit-redis does not have property incr. Even if I change the name from increment to incr it gives run time error.

Hello,

I m using @types/express-slow-down for my typescript expressjs project. And I noticed that the option maxDelayMs is missing in the typescript adapter. It's a shame because without this option, the delay applied to my requests keeps increasing. Is this on purpose ?

Thanks

Description

I get lots of warnings like:

ChangeWarning: Setting limit or max to 0 disables rate limiting in express-rate-limit v6 and older, but will cause all requests to be blocked in v7 See https://express-rate-limit.github.io/WRN_ERL_MAX_ZERO/ for more information.

looking at the code I can see you try to take care of this:

But I'm passing in my own validate to suppress delayMs warnings. This is then inside notUndefinedOptions and so its spreading overrides your addition on line 143

Library version

2.0.0

Node version

v18.17.1

Typescript version (if you are using it)

No response

Module system

CommonJS

Having upgraded from using version 1.4.0 in our code I'm now seeing tests timing out.

I then installed/run the code using node v14.18.3 and am seeing one of your test cases fail too:

• Test failure : should decrement hits with closed response and skipFailedRequests

The problem appears to have been caused by a https://github.com/nfriedly/express-slow-down/blob/459f2d532246c263356137dde6b840f522b6bf1d/lib/express-slow-down.js#L123 which is the only significant change since the previous revision at 1.4.0. I have no idea what fix is needed.

On v1.4.1 when slow delay is triggered the http request end up hanging up indefinitly and never being resolved

I'm on Windows with NodeJS v16.15.0

From what i saw the issue is that on this line we are sending the req object and with this object being sent the on-finished library trigger immediatly the onFinished event because the req.complete is already set (complete on req mean that the parsing of the request is done but not the http context being resolved source )

There are 2 possible solutions :

• Using the res object instead of the req one on the line linked above
• Not using on-finished library at all and replacing line linked above with req.on('close', () => {

I personnaly prefer the second solution because it means One less dependency to have so much cleaner to me ...

Both stores include their data on the request object, so it shouldn't be necessary to calculate it twice.

Description

In v1.5.0 we used onLimitReached to log and record metrics of our limiting. onLimitReached is no more, as its not in express-rate-limit, however in express-rate-limit we can use a handler to perform those actions. As described in your readme.md, handler is not supported in slow-down. Is there an alternative way to run custom code, or are there plans to?

Why

Logging and metrics

Alternatives

Giving up on logging and recording these events

Can one explain why linear backoff (additive) is appropriate here? Rather than exponential backoff (multiplicative) for instance.

It would be really nice to have a way to set a maximum delay for slow down (e.g.: a maxDelayMs config property).

If such property is set, the delay would increase as per delayMs config up to maxDelayMs. After reaching it, every subsequent request would be delayed exactly by maxDelayMs.

Rationale

1. Most HTTP client libraries come with a built-in timeout configuration that will simply drop a request after a certain period of time.
2. When the application is behind a reverse proxy, usually there's also a timeout that will force the connection dropping for such requests.

By boundlessly increasing the response delay, we could probably end up with many dangling requests, that will never see the light of day, consuming resources for nothing at all.