express-limiter icon indicating copy to clipboard operation
express-limiter copied to clipboard

🚨opts is mutable, security issue

Open bodinsamuel opened this issue 6 years ago • 0 comments

Hello,

I recently found out that when using lookup as a function, the param opts is mutable. That means, if you modify it, subsequent request made by same user or different users inherit the change you have made in it. This is a very dangerous thing and as it is not documented, I suppose it was not made to be that way.

Here is simple way to reproduce:

  • use a lookup in combinaison with total
  • in the lookup method, do not return a default value
  • next user will have the total computed for the previous request and not default one set in the root object.
context('mutation', () => {
  let express;
  let app;
  let limiter;

  before(() => {
    express = require('express');
    app = express();
    limiter = subject(app, redis);
    limiter({
      path: '*',
      method: 'all',
      lookup: function(req, res, opts, next) {
        opts.lookup = 'query.api_key';
        if (req.method === 'GET') {
          opts.total = 20;
        }
        return next();
      },
      total: 3,
      expire: 1000 * 60 * 60,
    });

    app
      .get('/route', function(req, res) {
        res.send(200, 'hello');
      })
      .post('/route', function(req, res) {
        res.send(200, 'hello');
      });
  });

  it('should have a special total', function(done) {
    request(app)
      .get('/route?api_key=foobar')
      .expect('X-RateLimit-Limit', 20)
      .expect('X-RateLimit-Remaining', 19)
      .expect(200, function(e) {
        done(e);
      });
  });

  // -> This test will fail
  // X-RateLimit-Limit will be equal to 20
  it('should rollback to default', function(done) {
    request(app)
      .post('/route?api_key=foobar')
      .expect('X-RateLimit-Limit', 3)
      .expect('X-RateLimit-Remaining', 2)
      .expect(200, function(e) {
        done(e);
      });
  });
});

Has the package seems not maintained, I assume it's best to emphasis this issue 🚨 Best regards

bodinsamuel avatar Sep 06 '19 14:09 bodinsamuel