♾️ fix: Permanent Ban Cache and Expired Ban Cleanup Defects (#12324)

* 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 <danny@librechat.ai>
This commit is contained in:
JooyoungChoi14 2026-03-21 01:47:51 +09:00 committed by GitHub
parent 96f6976e00
commit 54fc9c2c99
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 469 additions and 38 deletions

View file

@ -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);

View file

@ -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();
});
});
});