fix: IP-limit Run Now where the client's address actually exists

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.
This commit is contained in:
Danny Avila 2026-07-25 01:29:37 -04:00
parent 7b9aab3646
commit 6164b05e10
4 changed files with 149 additions and 28 deletions

View file

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

View file

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

View file

@ -67,3 +67,44 @@ export const readScheduleFireClaims = (req: {
export const isScheduleFireRequest = (req: {
headers: Record<string, string | string[] | undefined>;
}): 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<string, string | string[] | undefined>;
_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;
};

View file

@ -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<string, string>;
_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<FireKind>(['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);
});
});