rayriffy / elysia-rate-limit

Lightweight rate limiter plugin for Elysia.js
https://npm.im/elysia-rate-limit
MIT License
82 stars 6 forks source link

Rate limit for individual endpoint #24

Closed Mikey-ShenSu closed 5 months ago

Mikey-ShenSu commented 5 months ago

Is your feature request related to a problem? Please describe. Currently I am rate limiting the project with 10 requests per minute, but is it possible we can have rate limit for individual endpoints or group of endpoints? For instance, you may only be able to request SMS verification code once every 60 seconds, but for retrieving the data you can do max 10 requests every minute.

Describe the solution you'd like An argument that set the rate limit for individual endpoint without sharing the limit across the connection.

Describe alternatives you've considered I’ve thought that I may introduce the rate limit in each endpoint, but it adds extra code and complexity.

rayriffy commented 5 months ago

I think it might be possible, let me get into that. Related discord/elysiajs#1197625891778482276

rayriffy commented 5 months ago

I've created version 3.2.0-beta.0 with feature that might matched your requirements, can you give it a try? More information at #27. Basically, you set plugin scoping to local then separate SMS verification api route as separate new Elysia() as one instance

const smsVerificationRoutes = new Elysia()
  .use(rateLimit())
  // your sms routes here

const apiRoutes = new Elysia()
  .use(rateLimit())
  // your api routes here

const server = new Elysia()
  .use(smsVerificationRoutes)
  .use(apiRoutes)
  .listen(3000)
Mikey-ShenSu commented 5 months ago

I've created version 3.2.0-beta.0 with feature that might matched your requirements, can you give it a try? More information at #27. Basically, you set plugin scoping to local then separate SMS verification api route as separate new Elysia() as one instance

const smsVerificationRoutes = new Elysia()
  .use(rateLimit())
  // your sms routes here

const apiRoutes = new Elysia()
  .use(rateLimit())
  // your api routes here

const server = new Elysia()
  .use(smsVerificationRoutes)
  .use(apiRoutes)
  .listen(3000)
[elysia-rate-limit] failed to determine client address (reason: .requestIP() returns null)
35 |                 }
36 |             }
37 |         });
38 |         app.onError(async ({ request }) => {
39 |             if (app.server === null)
40 |                 throw new Error('Elysia is not initialized yet. Please call .listen() first.');
                           ^
error: Elysia is not initialized yet. Please call .listen() first.
      at F:\ddxx\api\node_modules\elysia-rate-limit\dist\services\plugin.js:40:23
      at F:\ddxx\api\node_modules\elysia-rate-limit\dist\services\plugin.js:38:30
GET - http://localhost:3002/items?type=textbooks failed

I think this error is related to my code in the index.ts:

async beforeHandle() {
        await connectToMongoDB();
        await connectToRedis();
      },

Is there any method, that I can make the rate limit wait until those 2 connections finish? What I want to achieve here, is to re-use connections across the endpoint, so client dont need to call the db everytime.

rayriffy commented 5 months ago

Maybe finishing connection to db before starting defining any elysia instances? You can use global to set variable globally to reuse them

JohnRoseDev commented 5 months ago

Yeah I was also curious whether there is any argument against defining multiple instances of this rate limit plugin in order to allow for scoping of different rate limit configurations, similar to how is's a very common pattern with guards and the Elysia instance itself to create scopes.

Should something like this be perfectly fine?

const somePublicRoutes = new Elysia() // some public routes that need stricter rate limit checks
  .guard({}, app => app
    .use(rateLimit()) // with a strict config
    .post()
    .get()
  );

const someProtectedRoutes = new Elysia() // some private routes that don't need as strict rate limit checks
  .guard({}, app => app
    .use(rateLimit()) // with a less strict config
    .use(authGuard)
    .post()
    .get()
  );

Edit: I guess this was already answered here: https://github.com/rayriffy/elysia-rate-limit/issues/24#issuecomment-2035442981

Edit 2: This is causing silent 500's in the 3.2.0-beta.0. Even using the rate limit plugin outside of the guard directly on the Elysia sub-instances causes silent 500's.

rayriffy commented 5 months ago

i've updated to 3.2.0-beta.1 with requirement of server to be initialized removed. you should be able to get pass through that throw statement now.

doroved commented 5 months ago

Any plans for when it comes out of beta)?

rayriffy commented 5 months ago

You tell me 😅. If it works as expected, then I will push to mainstream.

rayriffy commented 5 months ago

@Mikey-ShenSu is this works as expected?

Mikey-ShenSu commented 5 months ago

@Mikey-ShenSu is this works as expected?

This works for me fine on my end, I will make an issue ticket if I encounter anything. Thanks for your work tho!

doroved commented 5 months ago

@Mikey-ShenSu is this works as expected?

Confirmed, everything works at first glance, but it looks like null is used instead of client IP.

[elysia-rate-limit] failed to determine client address (reason: server is null)
// ./src/routes/v1/index.ts
const routerAuth = new Elysia().group('/auth', (app) =>
  app
    .use(
      rateLimit({
        max: 3,
      })
    )
...
export const v1 = new Elysia({ prefix: '/v1' }).use(routerAuth)

// ./src/index.ts
const app = new Elysia()
  .use(cors())
  .use(v1)
  .listen(3000)
doroved commented 5 months ago

I've created version 3.2.0-beta.0 with feature that might matched your requirements, can you give it a try? More information at #27. Basically, you set plugin scoping to local then separate SMS verification api route as separate new Elysia() as one instance

const smsVerificationRoutes = new Elysia()
  .use(rateLimit())
  // your sms routes here

const apiRoutes = new Elysia()
  .use(rateLimit())
  // your api routes here

const server = new Elysia()
  .use(smsVerificationRoutes)
  .use(apiRoutes)
  .listen(3000)

This doesn't work if we want to use the default generator, because the server is null and therefore we won't get the client IP. So the correct use of the default generator is only available in the main Elysia instance where .listen() is present?

const app = new Elysia()
  .use(cors())
  .use(rateLimit())
  .use(v1)
  .listen(3000)

Why can't routers plugged into the master instance access the server? Then using rateLimit() on the main instance for the whole application makes no sense. It would be ideal to be able to limit by IP on roots outside the mainApp.

For example, Rate Limit for /auth should be limited by IP, because at this stage the user has no authorization data (JWT, cookie) that can be used. Rate Limit for /pay- should be limited by user's access token And so on

doroved commented 5 months ago

@rayriffy That's how it works, but the types are collapsing.

const smsVerificationRoutes = new Elysia().use(
  rateLimit({
    generator: (req, _server, { ipClient }) => {
      console.log(ipClient);
      return ipClient;
    },
  })
);
// your sms routes here

const apiRoutes = new Elysia().use(
  rateLimit({
    generator: (req, _server, { ipClient }) => {
      console.log(ipClient);
      return ipClient;
    },
  })
);
// your api routes here

const app = new Elysia()
  .derive(({ request }) => {
    return {
      ipClient: app.server?.requestIP(request)?.address,
    };
  })
  .use(smsVerificationRoutes)
  .use(apiRoutes)
  .listen(3000);
rayriffy commented 5 months ago

Thanks released at 3.2.0 (Ref: #34)

doroved commented 5 months ago

Thanks released at 3.2.0 (Ref: #34)

Have you read my previous posts? :)

doroved commented 5 months ago

@rayriffy ? Maybe I should create a new problem and describe everything there

rayriffy commented 5 months ago

No, you should considering refactor all reusable parts. Also, generator types fixed in 3.2.1

doroved commented 5 months ago

No, you should considering refactor all reusable parts. Also, generator types fixed in 3.2.1

Are you aware that this will not work because the server is only available in the Elysia instance where .listen is used ? How can we IP restrict the routes we want outside the main Elysia instance?

const smsVerificationRoutes = new Elysia()
  .use(rateLimit())
  // your sms routes here

const apiRoutes = new Elysia()
  .use(rateLimit())
  // your api routes here

const server = new Elysia()
  .use(smsVerificationRoutes)
  .use(apiRoutes)
  .listen(3000)