From 6164b05e102102d2e7ea41a5bf2bf34ffa5a589d Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 25 Jul 2026 01:29:37 -0400 Subject: [PATCH] fix: IP-limit Run Now where the client's address actually exists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review 4778551336. A regression from 7b9aab364, my own previous commit: making Run Now subject to the interactive limiters routed it through messageIpLimiter at the CHAT router — but every fire arrives there over the server's own loopback, so its address is never the initiating client's. All users' manual runs therefore shared ONE bucket, and enough Run Now volume from any user could reject unrelated users. Closing a bypass by adding a cross-user denial-of-service is a bad trade. The two limiters need different treatment because they key on different things: - IP: unusable at the chat router for ANY fire, so all fires are exempt there and manual runs are IP-limited at POST /api/schedules/:id/run instead, which still holds the real client address. Automatic occurrences have no client address by definition. - USER: keyed on the authenticated id, which the fire token carries intact across the loopback, so it applies correctly to Run Now right where it already was. This is the limiter that actually bounds manual volume, and the original bypass stays closed. The predicates move into packages/api as `exemptFromIpLimiter`/`exemptFromUserLimiter` so they are directly testable rather than inlined in the JS route — a first attempt at this test duplicated the middleware in the spec, which would have passed against a broken route. Eight new cases fail against the pre-fix code, including the two that state the fix: the IP limiter exempts a manual fire, and the user limiter does not. The captured-flag case is also pinned, since re-verifying an expired token mid-chain would demote a valid automatic fire into a limited interactive turn. --- api/server/routes/agents/index.js | 32 ++------- api/server/routes/schedules.js | 16 ++++- packages/api/src/crypto/jwt.ts | 41 +++++++++++ packages/api/src/crypto/limiters.spec.ts | 88 ++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 28 deletions(-) create mode 100644 packages/api/src/crypto/limiters.spec.ts diff --git a/api/server/routes/agents/index.js b/api/server/routes/agents/index.js index c71d35ffa7..48dfc9721e 100644 --- a/api/server/routes/agents/index.js +++ b/api/server/routes/agents/index.js @@ -11,6 +11,8 @@ const { attachAskUserQuestionArgs, createMessageFilterPii, readScheduleFireClaims, + exemptFromIpLimiter, + exemptFromUserLimiter, } = require('@librechat/api'); const { createSseStreamTelemetry } = require('@librechat/api/telemetry'); const { logger } = require('@librechat/data-schemas'); @@ -495,36 +497,16 @@ chatRouter.use((req, _res, next) => { }); chatRouter.use(configMiddleware); -/** - * AUTOMATIC scheduled fires are exempt from the interactive message limiters (the - * token's scope claim is signature-verified): the scheduler's own caps govern them - * (cadence floor, global fireConcurrency), and stacking both throttles would record - * legitimate fires as errors that walk schedules toward auto-disable. - * - * Run Now is NOT exempt. It dispatches the same billed generation over the same - * schedule-scoped token, but it enforces no cadence floor and no per-user time window, - * so a permitted user could re-trigger as prior runs finish — or rotate across their - * schedules — and bypass LIMIT_MESSAGE_USER/LIMIT_MESSAGE_IP entirely. It is user-paced - * request volume wearing a scheduled token, so it belongs under the interactive limits. - * - * Reads the flags captured above so a token that expired during config/middleware still - * classifies a valid fire; the re-read fallback is fail-safe (an unverifiable token - * limits rather than exempts). - */ -const skipAutomaticFires = (limiter) => (req, res, next) => { - const claims = - typeof req._isScheduledFire === 'boolean' - ? { scheduled: req._isScheduledFire, manual: req._isManualScheduledFire === true } - : readScheduleFireClaims(req); - return claims.scheduled && !claims.manual ? next() : limiter(req, res, next); -}; +/** Applies `limiter` unless `isExempt` says this fire should skip it. */ +const unless = (isExempt, limiter) => (req, res, next) => + isExempt(req) ? next() : limiter(req, res, next); if (isEnabled(LIMIT_MESSAGE_IP)) { - chatRouter.use(skipAutomaticFires(messageIpLimiter)); + chatRouter.use(unless(exemptFromIpLimiter, messageIpLimiter)); } if (isEnabled(LIMIT_MESSAGE_USER)) { - chatRouter.use(skipAutomaticFires(messageUserLimiter)); + chatRouter.use(unless(exemptFromUserLimiter, messageUserLimiter)); } chatRouter.use('/', chat); diff --git a/api/server/routes/schedules.js b/api/server/routes/schedules.js index 93878c5ec9..6dac17a1f1 100644 --- a/api/server/routes/schedules.js +++ b/api/server/routes/schedules.js @@ -1,7 +1,7 @@ const express = require('express'); const { Permissions, PermissionTypes } = require('librechat-data-provider'); -const { createSchedulesHandlers, generateCheckAccess } = require('@librechat/api'); -const { requireJwtAuth, configMiddleware } = require('~/server/middleware'); +const { isEnabled, createSchedulesHandlers, generateCheckAccess } = require('@librechat/api'); +const { requireJwtAuth, configMiddleware, messageIpLimiter } = require('~/server/middleware'); const { getLimits, fireScheduleNow, @@ -71,6 +71,16 @@ router.post('/', checkSchedulesCreate, handlers.createSchedule); router.patch('/:id', checkSchedulesCreate, handlers.updateSchedule); router.delete('/:id', checkSchedulesCreate, handlers.deleteSchedule); // Run-now mutates runtime state; gate it on CREATE like the UI does (not USE). -router.post('/:id/run', checkSchedulesCreate, handlers.runScheduleNow); +// This is also where LIMIT_MESSAGE_IP has to apply to a manual run: the fire itself is a +// loopback POST carrying the server's address, so limiting by IP there would pool every +// user into one bucket. Here the initiating client's address is still on the request. +// The USER limiter is NOT duplicated here; the fire token carries the authenticated id, +// so the chat router applies it to the loopback exactly once. +router.post( + '/:id/run', + ...(isEnabled(process.env.LIMIT_MESSAGE_IP) ? [messageIpLimiter] : []), + checkSchedulesCreate, + handlers.runScheduleNow, +); module.exports = router; diff --git a/packages/api/src/crypto/jwt.ts b/packages/api/src/crypto/jwt.ts index d9e562c44a..287320f09d 100644 --- a/packages/api/src/crypto/jwt.ts +++ b/packages/api/src/crypto/jwt.ts @@ -67,3 +67,44 @@ export const readScheduleFireClaims = (req: { export const isScheduleFireRequest = (req: { headers: Record; }): boolean => readScheduleFireClaims(req).scheduled; + +/** + * A request whose fire classification the chat router already captured. Re-verifying + * downstream would demote a valid fire whose short-lived token expired during the + * slower middleware chain, so the captured decision wins when present. + */ +interface ClassifiedFireRequest { + headers: Record; + _isScheduledFire?: boolean; + _isManualScheduledFire?: boolean; +} + +const classify = (req: ClassifiedFireRequest): ScheduleFireClaims => + typeof req._isScheduledFire === 'boolean' + ? { scheduled: req._isScheduledFire, manual: req._isManualScheduledFire === true } + : readScheduleFireClaims(req); + +/** + * Whether the IP-based message limiter should be SKIPPED. + * + * Every fire — automatic or manual — reaches the chat router over the server's own + * loopback, so its address is never the initiating client's. Keying an IP limiter there + * would put all users in ONE bucket, letting any user's Run Now volume reject everyone + * else's. Manual runs are IP-limited at `/api/schedules/:id/run` instead, where the real + * address is still on the request. + */ +export const exemptFromIpLimiter = (req: ClassifiedFireRequest): boolean => classify(req).scheduled; + +/** + * Whether the USER-based message limiter should be SKIPPED. + * + * Only AUTOMATIC occurrences are exempt: the scheduler's own caps (cadence floor, global + * fireConcurrency) already bound them, and limiting them again would record legitimate + * fires as errors that walk schedules toward auto-disable. Run Now has neither cap, and + * this limiter keys on the authenticated id, which the fire token carries intact across + * the loopback — so it is the one that actually bounds manual volume. + */ +export const exemptFromUserLimiter = (req: ClassifiedFireRequest): boolean => { + const claims = classify(req); + return claims.scheduled && !claims.manual; +}; diff --git a/packages/api/src/crypto/limiters.spec.ts b/packages/api/src/crypto/limiters.spec.ts new file mode 100644 index 0000000000..49b3317a65 --- /dev/null +++ b/packages/api/src/crypto/limiters.spec.ts @@ -0,0 +1,88 @@ +import { + exemptFromIpLimiter, + SCHEDULE_FIRE_SCOPE, + exemptFromUserLimiter, + SCHEDULE_MANUAL_CLAIM, + readScheduleFireClaims, + generateShortLivedToken, +} from './jwt'; + +type FireKind = 'automatic' | 'manual' | 'interactive'; + +/** Builds a request as the chat router sees it, including its capture middleware. */ +function request(kind: FireKind) { + if (kind === 'interactive') { + return { headers: {} }; + } + const token = generateShortLivedToken('user-1', '60s', { + scope: SCHEDULE_FIRE_SCOPE, + ...(kind === 'manual' ? { [SCHEDULE_MANUAL_CLAIM]: '1' } : {}), + }); + const req: { + headers: Record; + _isScheduledFire?: boolean; + _isManualScheduledFire?: boolean; + } = { headers: { 'x-lc-scheduled': '1', authorization: `Bearer ${token}` } }; + const claims = readScheduleFireClaims(req); + req._isScheduledFire = claims.scheduled; + req._isManualScheduledFire = claims.manual; + return req; +} + +describe('message-limiter exemptions for scheduled fires', () => { + const original = process.env.JWT_SECRET; + + beforeAll(() => { + process.env.JWT_SECRET = 'test-secret'; + }); + + afterAll(() => { + process.env.JWT_SECRET = original; + }); + + describe('IP limiter', () => { + it.each(['automatic', 'manual'])('exempts a %s fire', (kind) => { + // Every fire reaches the chat router over the SERVER's loopback, so its address is + // never the initiating client's. Keying here would pool all users into one bucket + // and let any user's Run Now volume reject everyone else's. Manual runs are + // IP-limited at /api/schedules/:id/run, where the real address still exists. + expect(exemptFromIpLimiter(request(kind))).toBe(true); + }); + + it('does not exempt an ordinary interactive turn', () => { + expect(exemptFromIpLimiter(request('interactive'))).toBe(false); + }); + }); + + describe('user limiter', () => { + it('exempts an automatic occurrence', () => { + // Already bounded by the cadence floor and the global fireConcurrency slot; + // limiting again would record legitimate fires as errors toward auto-disable. + expect(exemptFromUserLimiter(request('automatic'))).toBe(true); + }); + + it('does NOT exempt a manual Run Now', () => { + // Keyed on the authenticated id, which the fire token carries intact across the + // loopback — so this is the limiter that actually bounds Run Now volume. + expect(exemptFromUserLimiter(request('manual'))).toBe(false); + }); + + it('does not exempt an ordinary interactive turn', () => { + expect(exemptFromUserLimiter(request('interactive'))).toBe(false); + }); + }); + + it('limits rather than exempts a request whose token cannot be verified', () => { + const req = { headers: { 'x-lc-scheduled': '1', authorization: 'Bearer not-a-jwt' } }; + expect(exemptFromIpLimiter(req)).toBe(false); + expect(exemptFromUserLimiter(req)).toBe(false); + }); + + it('trusts the captured classification over a token that expired mid-chain', () => { + // The short-lived token can expire during the slower chat middleware; re-verifying + // would demote a valid automatic fire into a limited interactive turn. + const expired = { headers: { authorization: 'Bearer expired' }, _isScheduledFire: true }; + expect(exemptFromIpLimiter(expired)).toBe(true); + expect(exemptFromUserLimiter(expired)).toBe(true); + }); +});