fastify / fastify-rate-limit

A low overhead rate limiter for your routes
MIT License
477 stars 66 forks source link

eslint no-misused-promise in notFoundHandler #355

Closed mindrunner closed 8 months ago

mindrunner commented 8 months ago

Prerequisites

Fastify version

4.25.2

Plugin version

9.0.1

Node.js version

20.10.0

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

-

Description

I am rate limiting 404 errors as documented here: https://github.com/fastify/fastify-rate-limit#preventing-guessing-of-urls-through-404s

notFoundHandler expects a preValidationHookHandler, but fastify.rateLimit() is a preHandlerAsyncHookHandler

Steps to Reproduce

Use

fastify.setNotFoundHandler({
  preHandler: fastify.rateLimit()
}, function (request, reply) {
  reply.code(404).send({ hello: 'world' })
})

Expected Behavior

No erorrs / warnings thrown

mcollina commented 8 months ago

Fastify normalizes the flow between promises and non-promisified code.

Essentially that rule is too strict for no specific reason. Reply has a .then() but it is not a Promise.

To solve, just add a void, like so:

fastify.setNotFoundHandler({
  preHandler: fastify.rateLimit()
}, function (request, reply) {
  void reply.code(404).send({ hello: 'world' })
})

or return it:

fastify.setNotFoundHandler({
  preHandler: fastify.rateLimit()
}, function (request, reply) {
  return reply.code(404).send({ hello: 'world' })
})
mindrunner commented 8 months ago

The problem here is not the handler itself, but the preHandler which does not match:

image

mcollina commented 8 months ago

Ah it might be a bug in our types. That does not return a promise. That returns a function that returns a promise.

mcollina commented 8 months ago

Would you like to send a PR?

mindrunner commented 8 months ago

Yea that was my assumption. Can have a look into this later.

mindrunner commented 8 months ago

I am not sure how to fix this. preHandler expects a preHandlerHookHandler, but rateLimit returns a preHandlerAsyncHookHandler

mcollina commented 8 months ago

Can you share a reproduction for this? So somebody can quickly work for a resolution.

mindrunner commented 8 months ago

See 'Steps to reproduce' in Original Post. What else would you need to reproduce?

mcollina commented 8 months ago

We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that.

TL;DR Set up a tiny repository that shows the problem.

mindrunner commented 8 months ago

app.ts:

import Fastify from "fastify";

const main = async () => {
  const fastify = Fastify();
  await fastify.register(import("@fastify/rate-limit"), {
    max: 100,
    timeWindow: "1 minute",
  });

  fastify.setNotFoundHandler(
    {
      preHandler: fastify.rateLimit(),
    },
    function (request, reply) {
      reply.code(404).send({ hello: "world" });
    },
  );
};

main().then(() => {});

package.json

{
  "name": "rl-fastify",
  "version": "1.0.0",
  "description": "",
  "dependencies": {
    "fastify": "^4.25.2",
    "@fastify/rate-limit": "^9.1.0"
  },
  "devDependencies": {
    "@typescript-eslint/eslint-plugin": "^6.16.0",
    "@typescript-eslint/parser": "^6.16.0",
    "eslint": "^8.56.0"
  }
}

tsconfig.json

{
  "compilerOptions": {
    "target": "es2016",
    "module": "commonjs",
    "esModuleInterop": true,
    "forceConsistentCasingInFileNames": true,
    "strict": true,
    "skipLibCheck": true
  },
  "include": [
    "app.ts",
    ".eslintrc.js"
  ]
}

.eslintrc.js

module.exports = {
  parser: "@typescript-eslint/parser",
  parserOptions: {
    sourceType: "module",
    project: ["./tsconfig.json"],
  },
  env: {
    es6: true,
    node: true,
  },
  rules: {},
  extends: ["plugin:@typescript-eslint/recommended"],
};

Steps to reproduce:

Actual output:

  12:19  error  Promise-returning function provided to property where a void return was expected  @typescript-eslint/no-misused-promises

✖ 1 problem (1 error, 0 warnings)

Expected output:

<empty> (e.g. eslint does not show any errors)
mcollina commented 8 months ago

I think you got your file names wrong. I recommend you to upload everything to a repository.

mindrunner commented 8 months ago

Sorry, accidentally hit save before I was ready.

mindrunner commented 8 months ago

https://github.com/mindrunner/rl-fastify

mcollina commented 8 months ago

Running npx eslint . in that repository does not trigger any errors.

mindrunner commented 8 months ago

updated repo. pls try again

mcollina commented 8 months ago

Thanks, that was helpful.

Here is the fix: https://github.com/fastify/fastify/pull/5258

mindrunner commented 8 months ago

Awesome, thanks for fixing!