diff --git a/api/cache/banViolation.js b/api/cache/banViolation.js index 36945ca420..acae73b91e 100644 --- a/api/cache/banViolation.js +++ b/api/cache/banViolation.js @@ -1,6 +1,6 @@ const { logger } = require('@librechat/data-schemas'); const { ViolationTypes } = require('librechat-data-provider'); -const { isEnabled, math, removePorts } = require('@librechat/api'); +const { isEnabled, math, removePorts, getBanIp } = require('@librechat/api'); const { deleteAllUserSessions } = require('~/models'); const getLogStores = require('./getLogStores'); @@ -65,6 +65,7 @@ const banViolation = async (req, res, errorMessage) => { } req.ip = removePorts(req); + const banIp = getBanIp(req); logger.info( `[BAN] Banning user ${user_id} ${req.ip ? `@ ${req.ip} ` : ''}for ${ duration / 1000 / 60 @@ -73,8 +74,8 @@ const banViolation = async (req, res, errorMessage) => { const expiresAt = Date.now() + duration; await banLogs.set(user_id, { type, violation_count, duration, expiresAt }); - if (req.ip) { - await banLogs.set(req.ip, { type, user_id, violation_count, duration, expiresAt }); + if (banIp) { + await banLogs.set(banIp, { type, user_id, violation_count, duration, expiresAt }); } errorMessage.ban = true; diff --git a/api/cache/banViolation.spec.js b/api/cache/banViolation.spec.js index df98753498..dbcbddd4e4 100644 --- a/api/cache/banViolation.spec.js +++ b/api/cache/banViolation.spec.js @@ -1,5 +1,8 @@ const mongoose = require('mongoose'); const { MongoMemoryServer } = require('mongodb-memory-server'); +const { ViolationTypes } = require('librechat-data-provider'); +const { deleteAllUserSessions } = require('~/models'); +const getLogStores = require('./getLogStores'); const banViolation = require('./banViolation'); // Mock deleteAllUserSessions since we're testing ban logic, not session deletion @@ -81,6 +84,40 @@ describe('banViolation', () => { expect(errorMessage.ban).toBeTruthy(); }); + it.each(['127.0.0.1', '::1', '::ffff:127.0.0.1'])( + 'bans only the user for a verified trigger request from %s', + async (ip) => { + const banLogs = getLogStores(ViolationTypes.BAN); + await banLogs.delete(ip); + req.ip = ip; + req._isAgentTrigger = true; + errorMessage.violation_count = 20; + + await banViolation(req, res, errorMessage); + + expect(await banLogs.get(errorMessage.user_id)).toEqual( + expect.objectContaining({ violation_count: 20 }), + ); + expect(await banLogs.get(ip)).toBeUndefined(); + expect(deleteAllUserSessions).toHaveBeenCalledWith({ userId: errorMessage.user_id }); + expect(res.clearCookie).toHaveBeenCalledWith('refreshToken'); + expect(errorMessage.ban).toBe(true); + expect(req.ip).toBe(ip); + }, + ); + + it('still bans the IP when an ordinary request sends a trigger header', async () => { + req.headers = { 'x-lc-agent-trigger': '1' }; + errorMessage.violation_count = 20; + + await banViolation(req, res, errorMessage); + + const banLogs = getLogStores(ViolationTypes.BAN); + expect(await banLogs.get(req.ip)).toEqual( + expect.objectContaining({ user_id: errorMessage.user_id, violation_count: 20 }), + ); + }); + it('should handle invalid BAN_INTERVAL and default to 20', async () => { process.env.BAN_INTERVAL = 'invalid'; errorMessage.prev_count = 19; diff --git a/api/server/middleware/checkBan.js b/api/server/middleware/checkBan.js index 0871e51c04..305c87bfe4 100644 --- a/api/server/middleware/checkBan.js +++ b/api/server/middleware/checkBan.js @@ -2,7 +2,7 @@ const { Keyv } = require('keyv'); const uap = require('ua-parser-js'); const { logger } = require('@librechat/data-schemas'); const { ErrorTypes, ViolationTypes } = require('librechat-data-provider'); -const { isEnabled, keyvMongo, removePorts } = require('@librechat/api'); +const { isEnabled, keyvMongo, removePorts, getBanIp } = require('@librechat/api'); const { getLogStores } = require('~/cache'); const { isOAuthNavigation, redirectOAuthFailure } = require('./oauthNavigation'); const denyRequest = require('./denyRequest'); @@ -88,6 +88,7 @@ const checkBan = async (req, res, next = () => {}) => { } req.ip = removePorts(req); + const banIp = getBanIp(req); let userId = req.user?.id ?? req.user?._id?.toString() ?? null; if (!userId && req?.body?.email) { @@ -95,12 +96,12 @@ const checkBan = async (req, res, next = () => {}) => { userId = user?._id ? user._id.toString() : userId; } - if (!userId && !req.ip) { + if (!userId && !banIp) { return next(); } const useRedis = isEnabled(process.env.USE_REDIS); - const ipKey = getBanCacheKey('ip', req.ip, useRedis); + const ipKey = getBanCacheKey('ip', banIp, useRedis); const userKey = getBanCacheKey('user', userId, useRedis); const [cachedIPBan, cachedUserBan] = await Promise.all([ @@ -121,7 +122,7 @@ const checkBan = async (req, res, next = () => {}) => { } const [ipBan, userBan] = await Promise.all([ - req.ip ? banLogs.get(req.ip) : undefined, + banIp ? banLogs.get(banIp) : undefined, userId ? banLogs.get(userId) : undefined, ]); @@ -142,7 +143,7 @@ const checkBan = async (req, res, next = () => {}) => { if (timeLeft <= 0) { const cleanups = []; if (ipBan) { - cleanups.push(banLogs.delete(req.ip)); + cleanups.push(banLogs.delete(banIp)); } if (userBan) { cleanups.push(banLogs.delete(userId)); diff --git a/api/server/middleware/checkBan.spec.js b/api/server/middleware/checkBan.spec.js index ee83f0bef8..8e52719cc0 100644 --- a/api/server/middleware/checkBan.spec.js +++ b/api/server/middleware/checkBan.spec.js @@ -75,6 +75,38 @@ describe('checkBan with namespaced Keyv stores and ObjectId user ids', () => { expect(req.banned).toBeUndefined(); }); + it('isolates concurrent trigger violations to their user on the shared loopback transport', async () => { + const userId = new mongoose.Types.ObjectId(); + const req = { + ip: '::1', + user: { id: userId.toString() }, + headers: {}, + _isAgentTrigger: true, + }; + const errorMessage = { type: ViolationTypes.CONCURRENT }; + + await logViolation(req, createRes(), ViolationTypes.CONCURRENT, errorMessage, 20); + expect(errorMessage.ban).toBe(true); + + const otherUserReq = { + ...req, + user: { id: new mongoose.Types.ObjectId().toString() }, + }; + const next = jest.fn(); + await checkBan(otherUserReq, createRes(), next); + + expect(next).toHaveBeenCalledWith(); + expect(otherUserReq.banned).toBeUndefined(); + + const bannedUserRes = createRes(); + const bannedUserNext = jest.fn(); + await checkBan(req, bannedUserRes, bannedUserNext); + + expect(bannedUserNext).not.toHaveBeenCalled(); + expect(bannedUserRes.status).toHaveBeenCalledWith(403); + expect(req.banned).toBe(true); + }); + it('enforces a ban recorded for the same user from another address', async () => { const userId = new mongoose.Types.ObjectId(); const violationReq = createOAuthCallbackReq(userId, '10.0.0.2'); diff --git a/api/test/server/middleware/checkBan.test.js b/api/test/server/middleware/checkBan.test.js index 39775c389a..86024bd82d 100644 --- a/api/test/server/middleware/checkBan.test.js +++ b/api/test/server/middleware/checkBan.test.js @@ -40,6 +40,7 @@ jest.mock('@librechat/api', () => ({ return false; }, keyvMongo: {}, + getBanIp: jest.requireActual('../../../../packages/api/src/middleware/ban').getBanIp, removePorts: jest.fn((req) => req.ip), redirectToAuthFailure: (res, { clientDomain, authFailedError }) => res.redirect(`${clientDomain}/login?redirect=false&error=${authFailedError}`), @@ -141,6 +142,93 @@ describe('checkBan middleware', () => { }); }); + describe.each([false, true])('verified trigger requests (USE_REDIS=%s)', (useRedis) => { + beforeEach(() => { + process.env.USE_REDIS = String(useRedis); + }); + + const userKey = useRedis ? 'ban_cache:user:user123' : 'user123'; + const ipKey = useRedis ? 'ban_cache:ip:127.0.0.1' : '127.0.0.1'; + const createTriggerReq = () => createReq({ ip: '127.0.0.1', _isAgentTrigger: true }); + + it('ignores cached and persistent loopback bans without querying either IP key', async () => { + const ban = { expiresAt: Date.now() + 60000 }; + mockBanCacheGet.mockImplementation(async (key) => (key === ipKey ? ban : undefined)); + mockBanLogsGet.mockImplementation(async (key) => (key === '127.0.0.1' ? ban : undefined)); + const req = createTriggerReq(); + const next = jest.fn(); + + await checkBan(req, createRes(), next); + + expect(next).toHaveBeenCalledWith(); + expect(req.banned).toBeUndefined(); + expect(req.ip).toBe('127.0.0.1'); + expect(mockBanCacheGet.mock.calls).toEqual([[userKey]]); + expect(mockBanLogsGet.mock.calls).toEqual([['user123']]); + expect(mockBanCacheSet).not.toHaveBeenCalled(); + }); + + it('still rejects a cached user ban', async () => { + mockBanCacheGet.mockResolvedValueOnce({ expiresAt: Date.now() + 60000 }); + const res = createRes(); + const next = jest.fn(); + + await checkBan(createTriggerReq(), res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + expect(mockBanCacheGet.mock.calls).toEqual([[userKey]]); + expect(mockBanLogsGet).not.toHaveBeenCalled(); + }); + + it('caches a persistent user ban only under the user key', async () => { + const ban = { expiresAt: Date.now() + 60000 }; + mockBanLogsGet.mockResolvedValueOnce(ban); + const res = createRes(); + const next = jest.fn(); + + await checkBan(createTriggerReq(), res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + expect(mockBanCacheSet).toHaveBeenCalledTimes(1); + expect(mockBanCacheSet).toHaveBeenCalledWith(userKey, ban, expect.any(Number)); + }); + + it('cleans up an expired user ban without touching the transport IP', async () => { + mockBanLogsGet.mockResolvedValueOnce({ expiresAt: Date.now() - 1000 }); + const next = jest.fn(); + + await checkBan(createTriggerReq(), createRes(), next); + + expect(next).toHaveBeenCalledWith(); + expect(mockBanLogsDelete.mock.calls).toEqual([['user123']]); + expect(mockBanCacheSet).not.toHaveBeenCalled(); + }); + + it('does not treat a trigger header on an ordinary request as an exemption', async () => { + mockBanCacheGet.mockResolvedValueOnce({ expiresAt: Date.now() + 60000 }); + const req = createReq({ + ip: '127.0.0.1', + headers: { 'x-lc-agent-trigger': '1' }, + _isAgentTrigger: false, + }); + const res = createRes(); + const next = jest.fn(); + + await checkBan(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + expect(mockBanCacheGet).toHaveBeenCalledWith(ipKey); + }); + + afterEach(() => { + mockBanCacheGet.mockReset().mockResolvedValue(undefined); + mockBanLogsGet.mockReset().mockResolvedValue(undefined); + }); + }); + describe('cache hit path', () => { it('returns 403 when IP ban is cached', async () => { mockBanCacheGet.mockResolvedValueOnce({ expiresAt: Date.now() + 60000 }); diff --git a/packages/api/src/middleware/ban.spec.ts b/packages/api/src/middleware/ban.spec.ts new file mode 100644 index 0000000000..34eaeca17a --- /dev/null +++ b/packages/api/src/middleware/ban.spec.ts @@ -0,0 +1,25 @@ +import { getBanIp } from './ban'; + +describe('getBanIp', () => { + it.each(['127.0.0.1', '::1', '::ffff:127.0.0.1'])( + 'excludes the shared address %s only for verified trigger requests', + (ip) => { + const req = { ip, _isAgentTrigger: true }; + + expect(getBanIp(req)).toBeUndefined(); + expect(req.ip).toBe(ip); + expect(getBanIp({ ip, _isAgentTrigger: false })).toBe(ip); + expect(getBanIp({ ip })).toBe(ip); + }, + ); + + it('does not trust a trigger transport header without a verified identity', () => { + const req = { ip: '127.0.0.1', headers: { 'x-lc-agent-trigger': '1' } }; + + expect(getBanIp(req)).toBe(req.ip); + }); + + it('preserves a missing IP address', () => { + expect(getBanIp({ ip: undefined })).toBeUndefined(); + }); +}); diff --git a/packages/api/src/middleware/ban.ts b/packages/api/src/middleware/ban.ts new file mode 100644 index 0000000000..c23e6df27a --- /dev/null +++ b/packages/api/src/middleware/ban.ts @@ -0,0 +1,8 @@ +import type { Request } from 'express'; + +/** Only the router-verified trigger identity exempts the shared delivery transport from IP bans. */ +export function getBanIp( + req: Pick & { _isAgentTrigger?: boolean }, +): string | undefined { + return req._isAgentTrigger === true ? undefined : req.ip; +} diff --git a/packages/api/src/middleware/index.ts b/packages/api/src/middleware/index.ts index 7d161ca404..6373e1dc96 100644 --- a/packages/api/src/middleware/index.ts +++ b/packages/api/src/middleware/index.ts @@ -5,6 +5,7 @@ export * from './email'; export * from './notFound'; export * from './origin'; export * from './balance'; +export * from './ban'; export * from './json'; export * from './capabilities'; export * from './auth';