fix: add a more aggressive rate limit for spammy users

package.json

@@ -37,7 +37,7 @@
     "cors": "^2.8.5",
     "dotenv": "^16.0.2",
     "express": "^4.18.1",
-    "express-rate-limit": "^6.9.0",
+    "express-rate-limit": "^6.11.2",
     "lodash": "^4.17.21",
     "mysql": "^2.18.1",
     "node-fetch": "^2.7.0",

src/helpers/rateLimit.ts

@@ -5,10 +5,16 @@ import { getIp, sendError, sha256 } from './utils';
 
 const hashedIp = (req): string => sha256(getIp(req)).slice(0, 7);
 
-export default rateLimit({
-  windowMs: 60 * 1e3,
-  max: 100,
-  keyGenerator: req => hashedIp(req),
+function getStore() {
+  return redisClient
+    ? new RedisStore({
+        sendCommand: (...args: string[]) => redisClient.sendCommand(args),
+        prefix: process.env.RATE_LIMIT_KEYS_PREFIX || 'snapshot-sequencer:'
+      })
+    : undefined;
+}
+
+const rateLimitConfig = {
   standardHeaders: true,
   legacyHeaders: false,
   handler: (req, res) => {
@@ -17,11 +23,23 @@ export default rateLimit({
       'too many requests, refer to https://docs.snapshot.org/tools/api/api-keys#limits',
       429
     );
-  },
-  store: redisClient
-    ? new RedisStore({
-        sendCommand: (...args: string[]) => redisClient.sendCommand(args),
-        prefix: process.env.RATE_LIMIT_KEYS_PREFIX || 'snapshot-sequencer:'
-      })
-    : undefined
+  }
+};
+
+const highErroredRateLimit = rateLimit({
+  keyGenerator: req => `rl-s:${hashedIp(req)}`,
+  windowMs: 15 * 1e3,
+  max: 15,
+  skipSuccessfulRequests: true,
+  store: getStore(),
+  ...rateLimitConfig
 });
+const regularRateLimit = rateLimit({
+  keyGenerator: req => `rl:${hashedIp(req)}`,
+  windowMs: 60 * 1e3,
+  max: 100,
+  store: getStore(),
+  ...rateLimitConfig
+});
+
+export { regularRateLimit, highErroredRateLimit };

src/index.ts

@@ -3,7 +3,7 @@ import cors from 'cors';
 import { initLogger, fallbackLogger } from '@snapshot-labs/snapshot-sentry';
 import express from 'express';
 import api from './api';
-import rateLimit from './helpers/rateLimit';
+import { regularRateLimit, highErroredRateLimit } from './helpers/rateLimit';
 import shutter from './helpers/shutter';
 import log from './helpers/log';
 import refreshModeration from './helpers/moderation';
@@ -19,7 +19,7 @@ app.disable('x-powered-by');
 app.use(express.json({ limit: '20mb' }));
 app.use(express.urlencoded({ limit: '20mb', extended: false }));
 app.use(cors({ maxAge: 86400 }));
-app.use(rateLimit);
+app.use(regularRateLimit, highErroredRateLimit);
 app.set('trust proxy', 1);
 app.use('/', api);
 app.use('/shutter', shutter);

test/e2e/api.test.ts

@@ -1,5 +1,6 @@
 import fetch from 'node-fetch';
 import proposalInput from '../fixtures/ingestor-payload/proposal.json';
+import redis from '../../src/helpers/redis';
 import { spacesSqlFixtures } from '../fixtures/space';
 import proposalsFixtures from '../fixtures/proposal';
 import db from '../../src/helpers/mysql';
@@ -19,140 +20,198 @@ async function flagSpace(space: string, action = 'flag') {
   });
 }
 
-describe('POST /', () => {
-  describe('on invalid client input', () => {
-    it('returns a 400 error', async () => {
-      const response = await fetch(HOST, {
-        method: 'POST',
-        headers: { 'Content-Type': 'application/json' },
-        body: JSON.stringify(proposalInput)
-      });
-      const body = await response.json();
+function successRequest() {
+  return fetch(`${HOST}`);
+}
 
-      expect(response.status).toBe(400);
-      expect(body.error).toBe('client_error');
-      expect(body.error_description).toBe('wrong timestamp');
-    });
+function failRequest() {
+  return fetch(HOST, {
+    method: 'POST',
+    headers: { 'Content-Type': 'application/json' },
+    body: JSON.stringify(proposalInput)
   });
-});
+}
 
-describe('POST /flag', () => {
-  afterAll(() => {
-    db.endAsync();
+describe('api', () => {
+  beforeEach(async () => {
+    const keyPrefix = process.env.RATE_LIMIT_KEYS_PREFIX || 'snapshot-sequencer:';
+    const keys = await redis.keys(`${keyPrefix}*`);
+
+    await Promise.all(keys.map(k => redis.del(k)));
   });
 
-  beforeEach(async () => {
-    await db.queryAsync(
-      `
-      DELETE FROM snapshot_sequencer_test.spaces WHERE id LIKE ?;
-      TRUNCATE TABLE snapshot_sequencer_test.proposals
-    `,
-      [`${SPACE_PREFIX}%`]
-    );
-
-    await Promise.all(
-      spacesSqlFixtures
-        .map(space => ({
-          ...space,
-          id: `${SPACE_PREFIX}${space.id}`,
-          settings: JSON.stringify(space.settings)
-        }))
-        .map(async space => {
-          db.queryAsync('INSERT INTO snapshot_sequencer_test.spaces SET ?', space);
-        })
-    );
-
-    await Promise.all(
-      proposalsFixtures
-        .map(proposal => ({
-          ...proposal,
-          strategies: JSON.stringify(proposal.strategies),
-          validation: JSON.stringify(proposal.validation),
-          plugins: JSON.stringify(proposal.plugins),
-          choices: JSON.stringify(proposal.choices),
-          scores: JSON.stringify(proposal.scores),
-          scores_by_strategy: JSON.stringify(proposal.scores_by_strategy)
-        }))
-        .map(async proposal => {
-          db.queryAsync('INSERT INTO snapshot_sequencer_test.proposals SET ?', proposal);
-        })
-    );
+  afterAll(async () => {
+    await redis.quit();
   });
 
-  it('returns a 401 error when not authorized', async () => {
-    const response = await fetch(`${HOST}/flag`, {
-      method: 'POST',
-      headers: { 'Content-Type': 'application/json' },
-      body: JSON.stringify({})
+  describe('POST /', () => {
+    describe('on invalid client input', () => {
+      it('returns a 400 error', async () => {
+        const response = await failRequest();
+        const body = await response.json();
+
+        expect(response.status).toBe(400);
+        expect(body.error).toBe('client_error');
+        expect(body.error_description).toBe('wrong timestamp');
+      });
     });
 
-    expect(response.status).toBe(401);
-  });
+    describe('rate limit', () => {
+      describe('on a mix of success and failed requests', () => {
+        it('should return a 429 errors only after 100 requests / min', async () => {
+          for (let i = 1; i <= 100; i++) {
+            // 2% of failing requests
+            const response = await (Math.random() < 0.02 ? failRequest() : successRequest());
+            expect(response.status).not.toEqual(429);
+          }
+
+          const response = await fetch(HOST, {
+            method: 'POST',
+            headers: { 'Content-Type': 'application/json' },
+            body: JSON.stringify(proposalInput)
+          });
+          expect(response.status).toBe(429);
+        });
+      });
 
-  describe('when flagging a space', () => {
-    it('does nothing when the space does not exist', async () => {
-      const beforeFlaggedSpacesCount = await getFlaggedSpacesCount();
-      const response = await flagSpace('test-not-exist.eth');
-      const body = await response.json();
+      describe('on multiple failed requests', () => {
+        it('should return a 429 errors after 15 requests / 15s', async () => {
+          for (let i = 1; i <= 15; i++) {
+            const response = await failRequest();
+            expect(response.status).toBe(400);
+          }
 
-      expect(response.status).toBe(200);
-      expect(body.success).toBe(false);
-      expect(await getFlaggedSpacesCount()).toBe(beforeFlaggedSpacesCount);
+          const response = await fetch(`${HOST}/scores/proposal-id`);
+          expect(response.status).toBe(429);
+        });
+      });
     });
+  });
 
-    it('return true when the space is already flagged', async () => {
-      await db.queryAsync('UPDATE spaces SET flagged = 1 WHERE id = ?', `${SPACE_PREFIX}test.eth`);
-
-      const response = await flagSpace('test.eth');
-      const body = await response.json();
+  describe('POST /flag', () => {
+    afterAll(() => {
+      db.endAsync();
+    });
 
-      expect(response.status).toBe(200);
-      expect(body.success).toBe(true);
-      expect(await getFlaggedSpacesCount()).toBe(1);
+    beforeEach(async () => {
+      await db.queryAsync(
+        `
+      DELETE FROM snapshot_sequencer_test.spaces WHERE id LIKE ?;
+      TRUNCATE TABLE snapshot_sequencer_test.proposals
+    `,
+        [`${SPACE_PREFIX}%`]
+      );
+
+      await Promise.all(
+        spacesSqlFixtures
+          .map(space => ({
+            ...space,
+            id: `${SPACE_PREFIX}${space.id}`,
+            settings: JSON.stringify(space.settings)
+          }))
+          .map(async space => {
+            db.queryAsync('INSERT INTO snapshot_sequencer_test.spaces SET ?', space);
+          })
+      );
+
+      await Promise.all(
+        proposalsFixtures
+          .map(proposal => ({
+            ...proposal,
+            strategies: JSON.stringify(proposal.strategies),
+            validation: JSON.stringify(proposal.validation),
+            plugins: JSON.stringify(proposal.plugins),
+            choices: JSON.stringify(proposal.choices),
+            scores: JSON.stringify(proposal.scores),
+            scores_by_strategy: JSON.stringify(proposal.scores_by_strategy)
+          }))
+          .map(async proposal => {
+            db.queryAsync('INSERT INTO snapshot_sequencer_test.proposals SET ?', proposal);
+          })
+      );
     });
 
-    it('flags the space when it exists', async () => {
-      const response = await flagSpace('test.eth');
-      const body = await response.json();
+    it('returns a 401 error when not authorized', async () => {
+      const response = await fetch(`${HOST}/flag`, {
+        method: 'POST',
+        headers: { 'Content-Type': 'application/json' },
+        body: JSON.stringify({})
+      });
 
-      expect(body).toEqual({ success: true });
-      expect(
-        (await db.queryAsync('SELECT id FROM spaces WHERE flagged = 1')).map(r => r.id)
-      ).toEqual([`${SPACE_PREFIX}test.eth`]);
+      expect(response.status).toBe(401);
     });
-  });
 
-  describe('when un-flagging a space', () => {
-    it('does nothing when the space does not exist', async () => {
-      const beforeFlaggedSpacesCount = await getFlaggedSpacesCount();
+    describe('when flagging a space', () => {
+      it('does nothing when the space does not exist', async () => {
+        const beforeFlaggedSpacesCount = await getFlaggedSpacesCount();
+        const response = await flagSpace('test-not-exist.eth');
+        const body = await response.json();
 
-      const response = await flagSpace('test-not-exist.eth', 'unflag');
-      const body = await response.json();
+        expect(response.status).toBe(200);
+        expect(body.success).toBe(false);
+        expect(await getFlaggedSpacesCount()).toBe(beforeFlaggedSpacesCount);
+      });
 
-      expect(response.status).toBe(200);
-      expect(body.success).toBe(false);
-      expect(await getFlaggedSpacesCount()).toBe(beforeFlaggedSpacesCount);
-    });
+      it('return true when the space is already flagged', async () => {
+        await db.queryAsync(
+          'UPDATE spaces SET flagged = 1 WHERE id = ?',
+          `${SPACE_PREFIX}test.eth`
+        );
 
-    it('returns true when the space is not flagged', async () => {
-      const beforeFlaggedSpacesCount = await getFlaggedSpacesCount();
-      const response = await flagSpace('test.eth', 'unflag');
-      const body = await response.json();
+        const response = await flagSpace('test.eth');
+        const body = await response.json();
+
+        expect(response.status).toBe(200);
+        expect(body.success).toBe(true);
+        expect(await getFlaggedSpacesCount()).toBe(1);
+      });
 
-      expect(response.status).toBe(200);
-      expect(body.success).toBe(true);
-      expect(await getFlaggedSpacesCount()).toBe(beforeFlaggedSpacesCount);
+      it('flags the space when it exists', async () => {
+        const response = await flagSpace('test.eth');
+        const body = await response.json();
+
+        expect(body).toEqual({ success: true });
+        expect(
+          (await db.queryAsync('SELECT id FROM spaces WHERE flagged = 1')).map(r => r.id)
+        ).toEqual([`${SPACE_PREFIX}test.eth`]);
+      });
     });
 
-    it('un-flags the space when it is flagged', async () => {
-      await db.queryAsync('UPDATE spaces SET flagged = 1 WHERE id = ?', `${SPACE_PREFIX}test.eth`);
+    describe('when un-flagging a space', () => {
+      it('does nothing when the space does not exist', async () => {
+        const beforeFlaggedSpacesCount = await getFlaggedSpacesCount();
+
+        const response = await flagSpace('test-not-exist.eth', 'unflag');
+        const body = await response.json();
+
+        expect(response.status).toBe(200);
+        expect(body.success).toBe(false);
+        expect(await getFlaggedSpacesCount()).toBe(beforeFlaggedSpacesCount);
+      });
+
+      it('returns true when the space is not flagged', async () => {
+        const beforeFlaggedSpacesCount = await getFlaggedSpacesCount();
+        const response = await flagSpace('test.eth', 'unflag');
+        const body = await response.json();
+
+        expect(response.status).toBe(200);
+        expect(body.success).toBe(true);
+        expect(await getFlaggedSpacesCount()).toBe(beforeFlaggedSpacesCount);
+      });
 
-      const response = await flagSpace('test.eth', 'unflag');
-      const body = await response.json();
+      it('un-flags the space when it is flagged', async () => {
+        await db.queryAsync(
+          'UPDATE spaces SET flagged = 1 WHERE id = ?',
+          `${SPACE_PREFIX}test.eth`
+        );
 
-      expect(response.status).toBe(200);
-      expect(body.success).toBe(true);
-      expect(await getFlaggedSpacesCount()).toBe(0);
+        const response = await flagSpace('test.eth', 'unflag');
+        const body = await response.json();
+
+        expect(response.status).toBe(200);
+        expect(body.success).toBe(true);
+        expect(await getFlaggedSpacesCount()).toBe(0);
+      });
     });
   });
 });