From 54fc9c2c9958262e24967950d0e175d984388dd8 Mon Sep 17 00:00:00 2001 From: JooyoungChoi14 <63181822+JooyoungChoi14@users.noreply.github.com> Date: Sat, 21 Mar 2026 01:47:51 +0900 Subject: [PATCH] =?UTF-8?q?=E2=99=BE=EF=B8=8F=20fix:=20Permanent=20Ban=20C?= =?UTF-8?q?ache=20and=20Expired=20Ban=20Cleanup=20Defects=20(#12324)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: preserve ban data object in checkBan to prevent permanent cache The !! operator on line 108 coerces the ban data object to a boolean, losing the expiresAt property. This causes: 1. Number(true.expiresAt) = NaN → expired bans never cleaned from banLogs 2. banCache.set(key, true, NaN) → Keyv stores with expires: null (permanent) 3. IP-based cache entries persist indefinitely, blocking unrelated users Fix: replace isBanned (boolean) with banData (original object) so that expiresAt is accessible for TTL calculation and proper cache expiry. * fix: address checkBan cleanup defects exposed by ban data fix The prior commit correctly replaced boolean coercion with the ban data object, but activated previously-dead cleanup code with several defects: - IP-only expired bans fell through cleanup without returning next(), re-caching with negative TTL (permanent entry) and blocking the user - Redis deployments used cache-prefixed keys for banLogs.delete(), silently failing since bans are stored at raw keys - banCache.set() calls were fire-and-forget, silently dropping errors - No guard for missing/invalid expiresAt reproduced the NaN TTL bug on legacy ban records Consolidate expired-ban cleanup into a single block that always returns next(), use raw keys (req.ip, userId) for banLogs.delete(), add an expiresAt validity guard, await cache writes with error logging, and parallelize independent I/O with Promise.all. Add 25 tests covering all checkBan code paths including the specific regressions for IP-only cleanup, Redis key mismatch, missing expiresAt, and cache write failures. --------- Co-authored-by: Danny Avila --- api/server/middleware/checkBan.js | 81 ++-- api/test/server/middleware/checkBan.test.js | 426 ++++++++++++++++++++ 2 files changed, 469 insertions(+), 38 deletions(-) create mode 100644 api/test/server/middleware/checkBan.test.js diff --git a/api/server/middleware/checkBan.js b/api/server/middleware/checkBan.js index 0c98f3a824..5d1b60297f 100644 --- a/api/server/middleware/checkBan.js +++ b/api/server/middleware/checkBan.js @@ -10,6 +10,14 @@ const { findUser } = require('~/models'); const banCache = new Keyv({ store: keyvMongo, namespace: ViolationTypes.BAN, ttl: 0 }); const message = 'Your account has been temporarily banned due to violations of our service.'; +/** @returns {string} Cache key for ban lookups, prefixed for Redis or raw for MongoDB */ +const getBanCacheKey = (prefix, value, useRedis) => { + if (!value) { + return ''; + } + return useRedis ? `ban_cache:${prefix}:${value}` : value; +}; + /** * Respond to the request if the user is banned. * @@ -63,25 +71,16 @@ const checkBan = async (req, res, next = () => {}) => { return next(); } - let cachedIPBan; - let cachedUserBan; + const useRedis = isEnabled(process.env.USE_REDIS); + const ipKey = getBanCacheKey('ip', req.ip, useRedis); + const userKey = getBanCacheKey('user', userId, useRedis); - let ipKey = ''; - let userKey = ''; + const [cachedIPBan, cachedUserBan] = await Promise.all([ + ipKey ? banCache.get(ipKey) : undefined, + userKey ? banCache.get(userKey) : undefined, + ]); - if (req.ip) { - ipKey = isEnabled(process.env.USE_REDIS) ? `ban_cache:ip:${req.ip}` : req.ip; - cachedIPBan = await banCache.get(ipKey); - } - - if (userId) { - userKey = isEnabled(process.env.USE_REDIS) ? `ban_cache:user:${userId}` : userId; - cachedUserBan = await banCache.get(userKey); - } - - const cachedBan = cachedIPBan || cachedUserBan; - - if (cachedBan) { + if (cachedIPBan || cachedUserBan) { req.banned = true; return await banResponse(req, res); } @@ -93,41 +92,47 @@ const checkBan = async (req, res, next = () => {}) => { return next(); } - let ipBan; - let userBan; + const [ipBan, userBan] = await Promise.all([ + req.ip ? banLogs.get(req.ip) : undefined, + userId ? banLogs.get(userId) : undefined, + ]); - if (req.ip) { - ipBan = await banLogs.get(req.ip); - } + const banData = ipBan || userBan; - if (userId) { - userBan = await banLogs.get(userId); - } - - const isBanned = !!(ipBan || userBan); - - if (!isBanned) { + if (!banData) { return next(); } - const timeLeft = Number(isBanned.expiresAt) - Date.now(); - - if (timeLeft <= 0 && ipKey) { - await banLogs.delete(ipKey); + const expiresAt = Number(banData.expiresAt); + if (!banData.expiresAt || isNaN(expiresAt)) { + req.banned = true; + return await banResponse(req, res); } - if (timeLeft <= 0 && userKey) { - await banLogs.delete(userKey); + const timeLeft = expiresAt - Date.now(); + + if (timeLeft <= 0) { + const cleanups = []; + if (ipBan) { + cleanups.push(banLogs.delete(req.ip)); + } + if (userBan) { + cleanups.push(banLogs.delete(userId)); + } + await Promise.all(cleanups); return next(); } + const cacheWrites = []; if (ipKey) { - banCache.set(ipKey, isBanned, timeLeft); + cacheWrites.push(banCache.set(ipKey, banData, timeLeft)); } - if (userKey) { - banCache.set(userKey, isBanned, timeLeft); + cacheWrites.push(banCache.set(userKey, banData, timeLeft)); } + await Promise.all(cacheWrites).catch((err) => + logger.warn('[checkBan] Failed to write ban cache:', err), + ); req.banned = true; return await banResponse(req, res); diff --git a/api/test/server/middleware/checkBan.test.js b/api/test/server/middleware/checkBan.test.js new file mode 100644 index 0000000000..518153be67 --- /dev/null +++ b/api/test/server/middleware/checkBan.test.js @@ -0,0 +1,426 @@ +const mockBanCacheGet = jest.fn().mockResolvedValue(undefined); +const mockBanCacheSet = jest.fn().mockResolvedValue(undefined); + +jest.mock('keyv', () => ({ + Keyv: jest.fn().mockImplementation(() => ({ + get: mockBanCacheGet, + set: mockBanCacheSet, + })), +})); + +const mockBanLogsGet = jest.fn().mockResolvedValue(undefined); +const mockBanLogsDelete = jest.fn().mockResolvedValue(true); +const mockBanLogs = { + get: mockBanLogsGet, + delete: mockBanLogsDelete, + opts: { ttl: 7200000 }, +}; + +jest.mock('~/cache', () => ({ + getLogStores: jest.fn(() => mockBanLogs), +})); + +jest.mock('@librechat/data-schemas', () => ({ + logger: { + info: jest.fn(), + warn: jest.fn(), + debug: jest.fn(), + error: jest.fn(), + }, +})); + +jest.mock('@librechat/api', () => ({ + isEnabled: (value) => { + if (typeof value === 'boolean') { + return value; + } + if (typeof value === 'string') { + return value.toLowerCase().trim() === 'true'; + } + return false; + }, + keyvMongo: {}, + removePorts: jest.fn((req) => req.ip), +})); + +jest.mock('~/models', () => ({ + findUser: jest.fn(), +})); + +jest.mock('~/server/middleware/denyRequest', () => jest.fn().mockResolvedValue(undefined)); + +jest.mock('ua-parser-js', () => jest.fn(() => ({ browser: { name: 'Chrome' } }))); + +const checkBan = require('~/server/middleware/checkBan'); +const { logger } = require('@librechat/data-schemas'); +const { findUser } = require('~/models'); + +const createReq = (overrides = {}) => ({ + ip: '192.168.1.1', + user: { id: 'user123' }, + headers: { 'user-agent': 'Mozilla/5.0' }, + body: {}, + baseUrl: '/api', + originalUrl: '/api/test', + ...overrides, +}); + +const createRes = () => ({ + status: jest.fn().mockReturnThis(), + json: jest.fn().mockReturnThis(), +}); + +describe('checkBan middleware', () => { + let originalEnv; + + beforeEach(() => { + originalEnv = { ...process.env }; + process.env.BAN_VIOLATIONS = 'true'; + delete process.env.USE_REDIS; + mockBanLogs.opts.ttl = 7200000; + }); + + afterEach(() => { + process.env = originalEnv; + jest.clearAllMocks(); + }); + + describe('early exits', () => { + it('calls next() when BAN_VIOLATIONS is disabled', async () => { + process.env.BAN_VIOLATIONS = 'false'; + const next = jest.fn(); + + await checkBan(createReq(), createRes(), next); + + expect(next).toHaveBeenCalledWith(); + expect(mockBanCacheGet).not.toHaveBeenCalled(); + }); + + it('calls next() when BAN_VIOLATIONS is unset', async () => { + delete process.env.BAN_VIOLATIONS; + const next = jest.fn(); + + await checkBan(createReq(), createRes(), next); + + expect(next).toHaveBeenCalledWith(); + }); + + it('calls next() when neither userId nor IP is available', async () => { + const next = jest.fn(); + const req = createReq({ ip: null, user: null }); + + await checkBan(req, createRes(), next); + + expect(next).toHaveBeenCalledWith(); + }); + + it('calls next() when ban duration is <= 0', async () => { + mockBanLogs.opts.ttl = 0; + const next = jest.fn(); + + await checkBan(createReq(), createRes(), next); + + expect(next).toHaveBeenCalledWith(); + }); + + it('calls next() when no ban exists in cache or DB', async () => { + const next = jest.fn(); + + await checkBan(createReq(), createRes(), next); + + expect(next).toHaveBeenCalledWith(); + expect(mockBanCacheGet).toHaveBeenCalled(); + expect(mockBanLogsGet).toHaveBeenCalled(); + }); + }); + + describe('cache hit path', () => { + it('returns 403 when IP ban is cached', async () => { + mockBanCacheGet.mockResolvedValueOnce({ expiresAt: Date.now() + 60000 }); + const next = jest.fn(); + const req = createReq(); + const res = createRes(); + + await checkBan(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(req.banned).toBe(true); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('returns 403 when user ban is cached (IP miss)', async () => { + mockBanCacheGet + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce({ expiresAt: Date.now() + 60000 }); + const next = jest.fn(); + const req = createReq(); + const res = createRes(); + + await checkBan(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(req.banned).toBe(true); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('does not query banLogs when cache hit occurs', async () => { + mockBanCacheGet.mockResolvedValueOnce({ expiresAt: Date.now() + 60000 }); + + await checkBan(createReq(), createRes(), jest.fn()); + + expect(mockBanLogsGet).not.toHaveBeenCalled(); + }); + }); + + describe('active ban (positive timeLeft)', () => { + it('caches ban with correct TTL and returns 403', async () => { + const expiresAt = Date.now() + 3600000; + const banRecord = { expiresAt, type: 'ban', violation_count: 3 }; + mockBanLogsGet.mockResolvedValueOnce(banRecord); + const next = jest.fn(); + const req = createReq(); + const res = createRes(); + + await checkBan(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(req.banned).toBe(true); + expect(res.status).toHaveBeenCalledWith(403); + expect(mockBanCacheSet).toHaveBeenCalledTimes(2); + + const [ipCacheCall, userCacheCall] = mockBanCacheSet.mock.calls; + expect(ipCacheCall[0]).toBe('192.168.1.1'); + expect(ipCacheCall[1]).toBe(banRecord); + expect(ipCacheCall[2]).toBeGreaterThan(0); + expect(ipCacheCall[2]).toBeLessThanOrEqual(3600000); + + expect(userCacheCall[0]).toBe('user123'); + expect(userCacheCall[1]).toBe(banRecord); + }); + + it('caches only IP when no userId is present', async () => { + const expiresAt = Date.now() + 3600000; + mockBanLogsGet.mockResolvedValueOnce({ expiresAt, type: 'ban' }); + const req = createReq({ user: null }); + + await checkBan(req, createRes(), jest.fn()); + + expect(mockBanCacheSet).toHaveBeenCalledTimes(1); + expect(mockBanCacheSet).toHaveBeenCalledWith( + '192.168.1.1', + expect.any(Object), + expect.any(Number), + ); + }); + }); + + describe('expired ban cleanup', () => { + it('cleans up and calls next() for expired user-key ban', async () => { + const expiresAt = Date.now() - 1000; + mockBanLogsGet + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce({ expiresAt, type: 'ban' }); + const next = jest.fn(); + const req = createReq(); + + await checkBan(req, createRes(), next); + + expect(next).toHaveBeenCalledWith(); + expect(req.banned).toBeUndefined(); + expect(mockBanLogsDelete).toHaveBeenCalledWith('user123'); + expect(mockBanCacheSet).not.toHaveBeenCalled(); + }); + + it('cleans up and calls next() for expired IP-only ban (Finding 1 regression)', async () => { + const expiresAt = Date.now() - 1000; + mockBanLogsGet.mockResolvedValueOnce({ expiresAt, type: 'ban' }); + const next = jest.fn(); + const req = createReq({ user: null }); + + await checkBan(req, createRes(), next); + + expect(next).toHaveBeenCalledWith(); + expect(req.banned).toBeUndefined(); + expect(mockBanLogsDelete).toHaveBeenCalledWith('192.168.1.1'); + expect(mockBanCacheSet).not.toHaveBeenCalled(); + }); + + it('cleans up both IP and user bans when both are expired', async () => { + const expiresAt = Date.now() - 1000; + mockBanLogsGet + .mockResolvedValueOnce({ expiresAt, type: 'ban' }) + .mockResolvedValueOnce({ expiresAt, type: 'ban' }); + const next = jest.fn(); + + await checkBan(createReq(), createRes(), next); + + expect(next).toHaveBeenCalledWith(); + expect(mockBanLogsDelete).toHaveBeenCalledTimes(2); + expect(mockBanLogsDelete).toHaveBeenCalledWith('192.168.1.1'); + expect(mockBanLogsDelete).toHaveBeenCalledWith('user123'); + }); + + it('does not write to banCache when ban is expired', async () => { + const expiresAt = Date.now() - 60000; + mockBanLogsGet.mockResolvedValueOnce({ expiresAt, type: 'ban' }); + + await checkBan(createReq({ user: null }), createRes(), jest.fn()); + + expect(mockBanCacheSet).not.toHaveBeenCalled(); + }); + }); + + describe('Redis key paths (Finding 2 regression)', () => { + beforeEach(() => { + process.env.USE_REDIS = 'true'; + }); + + it('uses cache-prefixed keys for banCache.get', async () => { + await checkBan(createReq(), createRes(), jest.fn()); + + expect(mockBanCacheGet).toHaveBeenCalledWith('ban_cache:ip:192.168.1.1'); + expect(mockBanCacheGet).toHaveBeenCalledWith('ban_cache:user:user123'); + }); + + it('uses raw keys (not cache-prefixed) for banLogs.delete on cleanup', async () => { + const expiresAt = Date.now() - 1000; + mockBanLogsGet + .mockResolvedValueOnce({ expiresAt, type: 'ban' }) + .mockResolvedValueOnce({ expiresAt, type: 'ban' }); + + await checkBan(createReq(), createRes(), jest.fn()); + + expect(mockBanLogsDelete).toHaveBeenCalledWith('192.168.1.1'); + expect(mockBanLogsDelete).toHaveBeenCalledWith('user123'); + for (const call of mockBanLogsDelete.mock.calls) { + expect(call[0]).not.toMatch(/^ban_cache:/); + } + }); + + it('uses cache-prefixed keys for banCache.set on active ban', async () => { + const expiresAt = Date.now() + 3600000; + mockBanLogsGet.mockResolvedValueOnce({ expiresAt, type: 'ban' }); + + await checkBan(createReq(), createRes(), jest.fn()); + + expect(mockBanCacheSet).toHaveBeenCalledWith( + 'ban_cache:ip:192.168.1.1', + expect.any(Object), + expect.any(Number), + ); + expect(mockBanCacheSet).toHaveBeenCalledWith( + 'ban_cache:user:user123', + expect.any(Object), + expect.any(Number), + ); + }); + }); + + describe('missing expiresAt guard (Finding 5)', () => { + it('returns 403 without caching when expiresAt is missing', async () => { + mockBanLogsGet.mockResolvedValueOnce({ type: 'ban' }); + const next = jest.fn(); + const req = createReq(); + const res = createRes(); + + await checkBan(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(req.banned).toBe(true); + expect(res.status).toHaveBeenCalledWith(403); + expect(mockBanCacheSet).not.toHaveBeenCalled(); + }); + + it('returns 403 without caching when expiresAt is NaN-producing', async () => { + mockBanLogsGet.mockResolvedValueOnce({ type: 'ban', expiresAt: 'not-a-number' }); + const next = jest.fn(); + const res = createRes(); + + await checkBan(createReq(), res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + expect(mockBanCacheSet).not.toHaveBeenCalled(); + }); + + it('returns 403 without caching when expiresAt is null', async () => { + mockBanLogsGet.mockResolvedValueOnce({ type: 'ban', expiresAt: null }); + const next = jest.fn(); + const res = createRes(); + + await checkBan(createReq(), res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + expect(mockBanCacheSet).not.toHaveBeenCalled(); + }); + }); + + describe('cache write error handling (Finding 4)', () => { + it('still returns 403 when banCache.set rejects', async () => { + const expiresAt = Date.now() + 3600000; + mockBanLogsGet.mockResolvedValueOnce({ expiresAt, type: 'ban' }); + mockBanCacheSet.mockRejectedValue(new Error('MongoDB write failure')); + const next = jest.fn(); + const req = createReq(); + const res = createRes(); + + await checkBan(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(req.banned).toBe(true); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('logs a warning when banCache.set fails', async () => { + const expiresAt = Date.now() + 3600000; + mockBanLogsGet.mockResolvedValueOnce({ expiresAt, type: 'ban' }); + mockBanCacheSet.mockRejectedValue(new Error('write failed')); + + await checkBan(createReq(), createRes(), jest.fn()); + + expect(logger.warn).toHaveBeenCalledWith( + '[checkBan] Failed to write ban cache:', + expect.any(Error), + ); + }); + }); + + describe('user lookup by email', () => { + it('resolves userId from email when not on request', async () => { + const req = createReq({ user: null, body: { email: 'test@example.com' } }); + findUser.mockResolvedValueOnce({ _id: 'resolved-user-id' }); + const expiresAt = Date.now() + 3600000; + mockBanLogsGet + .mockResolvedValueOnce(undefined) + .mockResolvedValueOnce({ expiresAt, type: 'ban' }); + + await checkBan(req, createRes(), jest.fn()); + + expect(findUser).toHaveBeenCalledWith({ email: 'test@example.com' }, '_id'); + expect(req.banned).toBe(true); + }); + + it('continues with IP-only check when email lookup finds no user', async () => { + const req = createReq({ user: null, body: { email: 'unknown@example.com' } }); + findUser.mockResolvedValueOnce(null); + const next = jest.fn(); + + await checkBan(req, createRes(), next); + + expect(next).toHaveBeenCalledWith(); + }); + }); + + describe('error handling', () => { + it('calls next(error) when an unexpected error occurs', async () => { + mockBanCacheGet.mockRejectedValueOnce(new Error('connection lost')); + const next = jest.fn(); + + await checkBan(createReq(), createRes(), next); + + expect(next).toHaveBeenCalledWith(expect.any(Error)); + expect(logger.error).toHaveBeenCalled(); + }); + }); +});