Automated warnings for various potential issues

[nfriedly]

I've been thinking about adding a suite of runtime checks to express-rate-limit to detect certain misconfigurations and other potential issues and pro-actively alert users. Some of the checks I've thought of include:

• if the default key generator (that returns req.ip) is used:
  • X-Forwarded-For header is set, but trust proxy is not set
  • trust proxy is set to true rather than a number/ip/regex/etc.
  • The "IP" includes a port number
• (for external stores) windowMs set to a different value than the store's expiration time
  • Although, if we can detect this one, maybe we should just fix it automatically?
• the same key is updated multiple times for a given request
  • Might be difficult to detect with an external store, perhaps we could add an optional method to the Store API to enable this
• A Store returns an unexpected value, such as a boolean for the number of hits (rateLimit not being update #353)

(These are all based on real issues that have come up. I'm sure that digging through the issues history will reveal a few more.)

I've also been thinking about what response level is appropriate for failed checks - throw an error, log a warning, etc. Some folks will probably want to just disable checks. And some will want different behavior in production than development. (Although, rate-limiting is one of those things that may behave differently in development due to the lack of proxies and/or being disabled entirely.) And then granularity - do we need to allow different response levels for different checks (e.g. allow end-users to disable a specific check but keep all others)? (Also, do I want different defaults for different checks? Should the default change if process.env.node_env === "production"?)

The last thing on my mind is when to run these checks. Most of them could probably be run once, either at launch or on the first request, and then not need to run again. But I'm not sure if the extra complexity of ensuring the checks only ran on one request is worth it, especially when someone who is worried about the performance could just disable the check entirely.

[gamemaker1]

This sounds really great! I have put down some of my questions and thoughts below.

(These are all based on real issues that have come up. I'm sure that digging through the issues history will reveal a few more.)

I think you've covered all of the common issues we've had so far (especially the trust proxy one!).

2. (for external stores) windowMs set to a different value than the store's expiration time
   • Although, if we can detect this one, maybe we should just fix it automatically?

How would we detect this? The (legacy) stores do not provide a function to check and set the windowMs. Also, the new stores use the options passed to the rate limiter directly instead of accepting it in their constructors. Maybe we could just encourage the use of the new store interface instead?

3. the same key is updated multiple times for a given request
   • Might be difficult to detect with an external store, perhaps we could add an optional method to the Store API to enable this

Instead of adding an optional method to the Store API, we could just add a new option to the rate limiter (diagnostics: true/false), which the new stores could use to apply checks?

The stores themselves could check this by incrementing the hit count, then fetching the count and making sure it is increased by exactly 1. This would definitely have a big impact on performance though.

Points 1 and 4 we can implement as you detailed.

I've also been thinking about what response level is appropriate for failed checks - throw an error, log a warning, etc

Maybe we could use the debug package like express does, and log everything we do?

The checks logger's output can be toggled using the diagnostics (does this sound good for the option name?) option passed to the rate limiter while creating it.

Some folks will probably want to just disable checks.

I think we should keep the diagnostics option off by default, to be used only if the user is facing any issues.

And then granularity - do we need to allow different response levels for different checks (e.g. allow end-users to disable a specific check but keep all others)

We could make diagnostics an array of checks to run instead of a boolean then -

type Options = {
	// ...

	// which checks to enable
	diagnostics: [
		'ip-address', // checks that the ip is not undefined, and that it doesn't contain a port number
		'trust-proxy', // checks that trust proxy is set properly if the header is present
		'store-interface-compliance', // checks that the store function signatures are valid
		'store-function-integrity', // checks that increment, decrement and reset actually work
	]
}

... now I'm thinking we should keep the first two checks on by default, they seem very useful - especially given the number of issues that have been opened about it.

Also, do I want different defaults for different checks? Should the default change if process.env.NODE_ENV === "production"?

Could you please give an example as to when this would happen? I can't think of one sorry :O

Most of them could probably be run once, either at launch or on the first request, and then not need to run again

I think the IP address and trust proxy ones could run every time. They have negligible performance impact, and their input will change every request.

The store ones could run only once on launch, and the store can keep track of that using an internal variable.

---

I think this proposal is really brilliant, I look forward to implementing it :D

Regards,
Vedant

[nfriedly]

Cool... I'm a little unsure about terminology, though. I want something that can be configured to throw errors, so I don't think the word "diagnostics" is a good fit. Maybe "validation"?

Also, re the debug library - I like that one and have used it in other projects, but I also don't think it's a good match for this, as I think it's more designed for "normal" logs, whereas the warnings that these checks/validations could emit should be exceptional.

[nfriedly]

Also, do I want different defaults for different checks? Should the default change if process.env.NODE_ENV === "production"?

Could you please give an example as to when this would happen? I can't think of one sorry :O

I think you kid of answered that with your previous sentence:

... now I'm thinking we should keep the first two checks on by default, they seem very useful - especially given the number of issues that have been opened about it.

However, I think for the first pass, the checks/validations are just going to be all-or-none. We can expand it later if needed.

[gamemaker1]

Cool... I'm a little unsure about terminology, though. I want something that can be configured to throw errors, so I don't think the word "diagnostics" is a good fit. Maybe "validation"?

validation sounds good!

Also, re the debug library - I like that one and have used it in other projects, but I also don't think it's a good match for this, as I think it's more designed for "normal" logs, whereas the warnings that these checks/validations could emit should be exceptional.

Hmm makes sense, console.log it is :D

[nfriedly]

I'm thinking of using either https://www.npmjs.com/package/ip-validator or else just using the regex that it uses (https://www.regextester.com/104038) - the code in the library looks like it recreates the regex each time the function is called.. although I suspect the node.js is smart enough to cache that, so it probably doesn't matter in practice.

[nfriedly]

Draft PR up at #358

[nfriedly]

Based on #363, perhaps we should add the link as a separate property on the error object. (In addition to putting it in the message.)

[nfriedly]

I also have an idea for how to detect double-counting like what happened in #362. But it's a bit more complex to implement, which is why I skipped it for the initial batch of validations.

Basically I want to have a global WeakMap<request, [key]> (global so that it's shared across all validator instances), and if a given request sees a given key more than once, log an error. It might need to run on more than the first request, though, because I think this is more likely to appear when doing per-route limiters.

Also, I need to work out what to do with the MemoryStore and PreciseMemoryStore, since they don't share any state, and than therfore use the same key twice without issue. Maybe WeakMap<request, WeakMap<storeInstance, [key]>> for those two.

And, then, it's unlikely, but the same key being sent to a redis store and a memcached store or whatever also wouldn't cause duplication. Not sure if I really need to handle that case, but I think it may be possible. Maybe WeakMap<request, WeakMap<storeInstance|storeClassName, [key]>>. That still wouldn't handle two redis stores going to two different redis servers, but I think this entire paragraph is a little far fetched and probably not worth worrying about.

[nfriedly]

Maybe run this validator for 100 requests (or until 1 error is logged) and then shut it down.

[nfriedly]

#364 (only runs on the first request for now)

[nfriedly]

I think we could detect and warn about situations like #318 by checking for response.headersSent - if it's true then the request was already handled before the ratelimiter ran, and the rate limiter can't do anything!

[nfriedly]

Actually, that wouldn't handle that case - the rate limiter isn't even called in that scenario, so the validator never runs. Not sure we can detect issues when the code doesn't run.