From 14187c9277beba14ede066a0215273c4efe2e883 Mon Sep 17 00:00:00 2001 From: Zachary Belford Date: Mon, 25 Nov 2024 11:17:21 -0800 Subject: [PATCH 1/6] fix: rate limits should respect batches --- blacklist.json | 2 +- src/middlewares/rateLimit.ts | 54 ++++++++ src/server.ts | 30 +--- src/tests/rateLimit.test.ts | 82 +++++++++++ test/unit/middlewares/rateLimit.test.ts | 177 ++++++++++++++++++++++++ 5 files changed, 317 insertions(+), 28 deletions(-) create mode 100644 src/middlewares/rateLimit.ts create mode 100644 src/tests/rateLimit.test.ts create mode 100644 test/unit/middlewares/rateLimit.test.ts diff --git a/blacklist.json b/blacklist.json index fe51488c..8d1622c6 100644 --- a/blacklist.json +++ b/blacklist.json @@ -1 +1 @@ -[] +["127.0.0.1"] \ No newline at end of file diff --git a/src/middlewares/rateLimit.ts b/src/middlewares/rateLimit.ts new file mode 100644 index 00000000..acd7eedf --- /dev/null +++ b/src/middlewares/rateLimit.ts @@ -0,0 +1,54 @@ +import { Request, Response, NextFunction } from 'express' +import { RequestersList } from '../utils' +import { CONFIG as config } from '../config' +import { sleep } from '../utils' +import blackList from '../../blacklist.json' +import spammerList from '../../spammerlist.json' + +// Export for testing +export const requestersList = new RequestersList(blackList, spammerList) + +interface RpcRequest { + method: string + params: any[] +} + +async function handleRejection(res: Response, softReject: boolean): Promise { + if (softReject) { + const randomSleepTime = 10 + Math.floor(Math.random() * 10) + await sleep(randomSleepTime * 1000) + res.status(503).send('Network is currently busy. Please try again later.') + } else { + res.status(503).send('Rejected by rate-limiting') + } +} + +async function checkRequest(ip: string, request: RpcRequest): Promise { + return await requestersList.isRequestOkay(ip, request.method, request.params) +} + +export async function rateLimitMiddleware(req: Request, res: Response, next: NextFunction) { + let ip = String(req.socket.remoteAddress) + if (ip.substring(0, 7) == '::ffff:') { + ip = ip.substring(7) + } + + const requests: RpcRequest[] = Array.isArray(req.body) ? req.body : [req.body] + + try { + const results = await Promise.all( + requests.map(request => checkRequest(ip, request)) + ) + + // If any request is not okay, reject the entire batch + if (results.some(result => !result)) { + await handleRejection(res, config.rateLimitOption.softReject) + return + } + + next() + } catch (error) { + console.error('Rate limiting error:', error) + res.status(500).send('Internal server error during rate limiting') + } +} \ No newline at end of file diff --git a/src/server.ts b/src/server.ts index 36417c0d..77b4d70e 100644 --- a/src/server.ts +++ b/src/server.ts @@ -35,6 +35,7 @@ import { nestedCountersInstance } from './utils/nestedCounters' import { methodWhitelist } from './middlewares/methodWhitelist' import { isDebugModeMiddlewareLow, rateLimitedDebugAuth } from './middlewares/debugMiddleware' import { isIPv4 } from 'net' +import { rateLimitMiddleware } from './middlewares/rateLimit' setDefaultResultOrder('ipv4first') @@ -180,33 +181,8 @@ app.use((err: CustomError, req: Request, res: Response, next: NextFunction) => { } next() }) - -app.use(async (req: Request, res: Response, next: NextFunction) => { - if (!config.rateLimit) { - next() - return - } - let ip = String(req.socket.remoteAddress) - if (ip.substring(0, 7) == '::ffff:') { - ip = ip.substring(7) - } - //console.log('IP is ', ip) - - const reqParams = req.body.params - const isRequestOkay = await requestersList.isRequestOkay(ip, req.body.method, reqParams) - if (!isRequestOkay) { - if (config.rateLimitOption.softReject) { - const randomSleepTime = 10 + Math.floor(Math.random() * 10) - await sleep(randomSleepTime * 1000) - res.status(503).send('Network is currently busy. Please try again later.') - return - } else { - res.status(503).send('Rejected by rate-limiting') - return - } - } - next() -}) +console.log('Is this log getting eaten??') +app.use(rateLimitMiddleware) app.use('/', logRoute) app.use('/', healthCheckRouter) diff --git a/src/tests/rateLimit.test.ts b/src/tests/rateLimit.test.ts new file mode 100644 index 00000000..96a6d346 --- /dev/null +++ b/src/tests/rateLimit.test.ts @@ -0,0 +1,82 @@ +import { RequestersList } from '../utils' +import { Request, Response } from 'express' +import { rateLimitMiddleware } from '../middlewares/rateLimit' + +describe('Rate Limiting', () => { + let requestersList: RequestersList + + beforeEach(() => { + requestersList = new RequestersList([], []) + }) + + it('should allow valid single requests', async () => { + const mockReq = { + socket: { remoteAddress: '127.0.0.1' }, + body: { + method: 'eth_call', + params: [] + } + } as Request + + const mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn() + } as unknown as Response + + const mockNext = jest.fn() + + await rateLimitMiddleware(mockReq, mockRes, mockNext) + expect(mockNext).toHaveBeenCalled() + }) + + it('should allow valid batch requests', async () => { + const mockReq = { + socket: { remoteAddress: '127.0.0.1' }, + body: [ + { + method: 'eth_call', + params: [] + }, + { + method: 'eth_getBalance', + params: [] + } + ] + } as Request + + const mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn() + } as unknown as Response + + const mockNext = jest.fn() + + await rateLimitMiddleware(mockReq, mockRes, mockNext) + expect(mockNext).toHaveBeenCalled() + }) + + it('should reject when rate limit exceeded', async () => { + const mockReq = { + socket: { remoteAddress: '127.0.0.1' }, + body: { + method: 'eth_sendRawTransaction', + params: [] + } + } as Request + + const mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn() + } as unknown as Response + + const mockNext = jest.fn() + + // Make multiple requests to exceed rate limit + for (let i = 0; i < 61; i++) { + await rateLimitMiddleware(mockReq, mockRes, mockNext) + } + + expect(mockRes.status).toHaveBeenCalledWith(503) + expect(mockRes.send).toHaveBeenCalledWith('Rejected by rate-limiting') + }) +}) \ No newline at end of file diff --git a/test/unit/middlewares/rateLimit.test.ts b/test/unit/middlewares/rateLimit.test.ts new file mode 100644 index 00000000..4a6bc95b --- /dev/null +++ b/test/unit/middlewares/rateLimit.test.ts @@ -0,0 +1,177 @@ +import { Request, Response } from 'express' +import { rateLimitMiddleware, requestersList } from '../../../src/middlewares/rateLimit' +import { CONFIG } from '../../../src/config' + +// Mock the Collector class +jest.mock('../../../src/external/Collector', () => ({ + Collector: jest.fn().mockImplementation(() => ({ + fetchAccount: jest.fn(), + getBlock: jest.fn(), + getTransactionByHash: jest.fn() + })), + collectorAPI: { + fetchAccount: jest.fn(), + getBlock: jest.fn(), + getTransactionByHash: jest.fn() + } +})) + +// Mock the RequestersList instance +jest.mock('../../../src/utils', () => { + return { + RequestersList: jest.fn().mockImplementation(() => ({ + isRequestOkay: jest.fn() + })), + sleep: jest.fn().mockImplementation(() => Promise.resolve()) + } +}) + +// Mock config with minimal required properties +jest.mock('../../../src/config', () => ({ + CONFIG: { + rateLimitOption: { + softReject: false, + allowedTxCountInCheckInterval: 60 + } + } +})) + +describe('Rate Limiting', () => { + beforeEach(() => { + jest.clearAllMocks() + }) + + it('should allow valid single requests', async () => { + // Setup the mock for this specific test + jest.spyOn(requestersList, 'isRequestOkay').mockResolvedValueOnce(true) + + const mockReq = { + socket: { remoteAddress: '127.0.0.1' }, + body: { + method: 'eth_call', + params: [] + } + } as Request + + const mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn() + } as unknown as Response + + const mockNext = jest.fn() + + await rateLimitMiddleware(mockReq, mockRes, mockNext) + expect(mockNext).toHaveBeenCalled() + expect(requestersList.isRequestOkay).toHaveBeenCalledTimes(1) + }) + + it('should allow valid batch requests', async () => { + const spy = jest.spyOn(requestersList, 'isRequestOkay') + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(true) + + const mockReq = { + socket: { remoteAddress: '127.0.0.1' }, + body: [ + { + method: 'eth_call', + params: [] + }, + { + method: 'eth_getBalance', + params: [] + } + ] + } as Request + + const mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn() + } as unknown as Response + + const mockNext = jest.fn() + + await rateLimitMiddleware(mockReq, mockRes, mockNext) + expect(mockNext).toHaveBeenCalled() + expect(spy).toHaveBeenCalledTimes(2) + }) + + it('should reject when rate limit exceeded', async () => { + jest.spyOn(requestersList, 'isRequestOkay').mockResolvedValue(false) + + const mockReq = { + socket: { remoteAddress: '127.0.0.1' }, + body: { + method: 'eth_sendRawTransaction', + params: [] + } + } as Request + + const mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn() + } as unknown as Response + + const mockNext = jest.fn() + + await rateLimitMiddleware(mockReq, mockRes, mockNext) + expect(mockRes.status).toHaveBeenCalledWith(503) + expect(mockRes.send).toHaveBeenCalledWith('Rejected by rate-limiting') + expect(mockNext).not.toHaveBeenCalled() + }) + + it('should handle soft rejection when configured', async () => { + jest.spyOn(requestersList, 'isRequestOkay').mockResolvedValue(false) + + // Temporarily modify config for this test + const originalConfig = CONFIG.rateLimitOption.softReject + CONFIG.rateLimitOption.softReject = true + + const mockReq = { + socket: { remoteAddress: '127.0.0.1' }, + body: { + method: 'eth_sendRawTransaction', + params: [] + } + } as Request + + const mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn() + } as unknown as Response + + const mockNext = jest.fn() + + await rateLimitMiddleware(mockReq, mockRes, mockNext) + expect(mockRes.status).toHaveBeenCalledWith(503) + expect(mockRes.send).toHaveBeenCalledWith('Network is currently busy. Please try again later.') + expect(mockNext).not.toHaveBeenCalled() + + // Restore original config + CONFIG.rateLimitOption.softReject = originalConfig + }) + + it('should handle errors gracefully', async () => { + jest.spyOn(requestersList, 'isRequestOkay').mockRejectedValueOnce(new Error('Mock error')) + + const mockReq = { + socket: { remoteAddress: '127.0.0.1' }, + body: { + method: 'eth_call', + params: [] + } + } as Request + + const mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn() + } as unknown as Response + + const mockNext = jest.fn() + + await rateLimitMiddleware(mockReq, mockRes, mockNext) + expect(mockRes.status).toHaveBeenCalledWith(500) + expect(mockRes.send).toHaveBeenCalledWith('Internal server error during rate limiting') + expect(mockNext).not.toHaveBeenCalled() + }) +}) \ No newline at end of file From 8f072ea5c7523f3a186da62c1c512958f872e811 Mon Sep 17 00:00:00 2001 From: Zachary Belford Date: Mon, 25 Nov 2024 11:28:03 -0800 Subject: [PATCH 2/6] fix: check config if ratelimiting enabled or not --- blacklist.json | 2 +- src/middlewares/rateLimit.ts | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/blacklist.json b/blacklist.json index 8d1622c6..fe51488c 100644 --- a/blacklist.json +++ b/blacklist.json @@ -1 +1 @@ -["127.0.0.1"] \ No newline at end of file +[] diff --git a/src/middlewares/rateLimit.ts b/src/middlewares/rateLimit.ts index acd7eedf..bf3f1a1b 100644 --- a/src/middlewares/rateLimit.ts +++ b/src/middlewares/rateLimit.ts @@ -28,6 +28,10 @@ async function checkRequest(ip: string, request: RpcRequest): Promise { } export async function rateLimitMiddleware(req: Request, res: Response, next: NextFunction) { + if (!config.rateLimit) { + next() + return + } let ip = String(req.socket.remoteAddress) if (ip.substring(0, 7) == '::ffff:') { ip = ip.substring(7) From 3b346783dfab8cb6ccb7e0bd5d4fc3269d2fc588 Mon Sep 17 00:00:00 2001 From: Zachary Belford Date: Mon, 25 Nov 2024 11:29:39 -0800 Subject: [PATCH 3/6] fix: update test for rateLimit config --- test/unit/middlewares/rateLimit.test.ts | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/unit/middlewares/rateLimit.test.ts b/test/unit/middlewares/rateLimit.test.ts index 4a6bc95b..81ac8c5b 100644 --- a/test/unit/middlewares/rateLimit.test.ts +++ b/test/unit/middlewares/rateLimit.test.ts @@ -29,6 +29,7 @@ jest.mock('../../../src/utils', () => { // Mock config with minimal required properties jest.mock('../../../src/config', () => ({ CONFIG: { + rateLimit: true, rateLimitOption: { softReject: false, allowedTxCountInCheckInterval: 60 @@ -39,6 +40,31 @@ jest.mock('../../../src/config', () => ({ describe('Rate Limiting', () => { beforeEach(() => { jest.clearAllMocks() + // Reset config to default state + CONFIG.rateLimit = true + }) + + it('should skip rate limiting when disabled in config', async () => { + CONFIG.rateLimit = false + + const mockReq = { + socket: { remoteAddress: '127.0.0.1' }, + body: { + method: 'eth_call', + params: [] + } + } as Request + + const mockRes = { + status: jest.fn().mockReturnThis(), + send: jest.fn() + } as unknown as Response + + const mockNext = jest.fn() + + await rateLimitMiddleware(mockReq, mockRes, mockNext) + expect(mockNext).toHaveBeenCalled() + expect(requestersList.isRequestOkay).not.toHaveBeenCalled() }) it('should allow valid single requests', async () => { From bf0dabbe6d11ed417519174d1fd0b8797eeefe55 Mon Sep 17 00:00:00 2001 From: Sonali Thakur Date: Thu, 28 Nov 2024 21:55:39 +0530 Subject: [PATCH 4/6] Change rate-limit status to 429 and remove debug log --- src/middlewares/rateLimit.ts | 2 +- src/server.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/middlewares/rateLimit.ts b/src/middlewares/rateLimit.ts index bf3f1a1b..8b6a2f16 100644 --- a/src/middlewares/rateLimit.ts +++ b/src/middlewares/rateLimit.ts @@ -19,7 +19,7 @@ async function handleRejection(res: Response, softReject: boolean): Promise { } next() }) -console.log('Is this log getting eaten??') + app.use(rateLimitMiddleware) app.use('/', logRoute) From d2f69f7a5cf5aa88a918a2590b82196765541121 Mon Sep 17 00:00:00 2001 From: Sonali Thakur Date: Fri, 29 Nov 2024 16:55:53 +0530 Subject: [PATCH 5/6] Use req.ip for accurate client IP behind proxies and add trustproxy config --- src/config.ts | 8 +++++--- src/middlewares/rateLimit.ts | 3 ++- src/server.ts | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/config.ts b/src/config.ts index a633cf32..0703bd1c 100644 --- a/src/config.ts +++ b/src/config.ts @@ -15,6 +15,7 @@ type Config = { enabled: boolean serveSubscriptions: boolean } + trustProxy: boolean // Whether to trust the X-Forwarded-For header log_server: { ip: string port: number @@ -115,6 +116,7 @@ export const CONFIG: Config = { enabled: true, serveSubscriptions: Boolean(process.env.WS_SAVE_SUBSCRIPTIONS) || false, }, + trustProxy: false, log_server: { ip: process.env.LOG_SERVER_HOST || '0.0.0.0', port: Number(process.env.LOG_SERVER_PORT) || 4446, @@ -150,12 +152,12 @@ export const CONFIG: Config = { aalgWarmup: Boolean(process.env.AALG_WARMUP) || true, aalgWarmupServiceTPS: 10, recordTxStatus: false, // not safe for production, keep this off. Known issue. - rateLimit: false, + rateLimit: true, rateLimitOption: { softReject: true, limitFromAddress: true, limitToAddress: true, - banIpAddress: false, + banIpAddress: true, banSpammerAddress: true, allowFaucetAccount: true, allowedTxCountInCheckInterval: 10, // allow 1 txs in every 12s = (checkInterval * 60 / allowedTxCountInCheckInterval) @@ -166,7 +168,7 @@ export const CONFIG: Config = { statLog: false, // not safe for production, keep this off adaptiveRejection: true, filterDeadNodesFromArchiver: false, - verbose: false, + verbose: true, verboseRequestWithRetry: false, verboseAALG: false, firstLineLogs: true, // default is true and turn off for prod for perf diff --git a/src/middlewares/rateLimit.ts b/src/middlewares/rateLimit.ts index 8b6a2f16..3a004c1d 100644 --- a/src/middlewares/rateLimit.ts +++ b/src/middlewares/rateLimit.ts @@ -32,7 +32,8 @@ export async function rateLimitMiddleware(req: Request, res: Response, next: Nex next() return } - let ip = String(req.socket.remoteAddress) + let ip = req.ip + if (ip.substring(0, 7) == '::ffff:') { ip = ip.substring(7) } diff --git a/src/server.ts b/src/server.ts index f693e718..6a3fbf61 100644 --- a/src/server.ts +++ b/src/server.ts @@ -87,7 +87,7 @@ process.on('unhandledRejection', (err) => { console.log('unhandledRejection:' + err) }) -app.set('trust proxy', false) +app.set('trust proxy', config.trustProxy) app.use(cors({ methods: ['POST'] })) app.use(express.json()) app.use(cookieParser()) From 9460ce57df9842c73b923814a1dce6cf4eed9894 Mon Sep 17 00:00:00 2001 From: Sonali Thakur Date: Fri, 29 Nov 2024 17:25:22 +0530 Subject: [PATCH 6/6] error message fix and clean up --- src/config.ts | 2 +- src/middlewares/rateLimit.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/config.ts b/src/config.ts index 0703bd1c..69057aa3 100644 --- a/src/config.ts +++ b/src/config.ts @@ -168,7 +168,7 @@ export const CONFIG: Config = { statLog: false, // not safe for production, keep this off adaptiveRejection: true, filterDeadNodesFromArchiver: false, - verbose: true, + verbose: false, verboseRequestWithRetry: false, verboseAALG: false, firstLineLogs: true, // default is true and turn off for prod for perf diff --git a/src/middlewares/rateLimit.ts b/src/middlewares/rateLimit.ts index 3a004c1d..324337af 100644 --- a/src/middlewares/rateLimit.ts +++ b/src/middlewares/rateLimit.ts @@ -54,6 +54,6 @@ export async function rateLimitMiddleware(req: Request, res: Response, next: Nex next() } catch (error) { console.error('Rate limiting error:', error) - res.status(500).send('Internal server error during rate limiting') + res.status(500).send('Internal server error') } } \ No newline at end of file