mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-04 14:57:42 +00:00
fix: stop Run Now inheriting the automatic fire's limiter exemption
Codex review 4778490508, P1. Scheduled fires are exempt from LIMIT_MESSAGE_USER and LIMIT_MESSAGE_IP because the scheduler's own caps govern them and stacking both would record legitimate fires as errors that walk schedules toward auto-disable. That reasoning holds for an AUTOMATIC occurrence, which is bounded by the cadence floor and the global fireConcurrency slot. It does not hold for Run Now. Run Now dispatches the same billed generation over the same schedule-scoped token, so it was silently covered by the same blanket exemption — while enforcing no cadence floor and no per-user time window. A permitted user could therefore re-trigger as prior runs finish, or rotate across their own schedules, and bypass both configured message limiters entirely. It is user-paced request volume wearing a scheduled token. The token now carries a signed `manual` claim, and the exemption applies only to automatic fires. Distinguishing at the token rather than at the route matters because the loopback POST is otherwise byte-identical for both kinds, and because the controller must keep treating a manual fire AS a scheduled one — it attributes the run to its schedule off that same predicate, so demoting Run Now wholesale would orphan its run row. `isScheduleFireRequest` therefore keeps its meaning and is now derived from the new `readScheduleFireClaims`, which reports both facts. The re-read fallback in the limiter stays fail-safe: an unverifiable token limits rather than exempts. Six new cases across jwt.spec.ts and fire.spec.ts fail against the pre-fix code, including the one that pins the property most easily lost here — a manual fire must still be recognizable as a scheduled fire. All 21 pre-existing cases in those files pass unchanged on both sides.
This commit is contained in:
parent
25d5251d34
commit
7b9aab3646
7 changed files with 175 additions and 27 deletions
|
|
@ -10,7 +10,7 @@ const {
|
|||
deleteAgentCheckpoint,
|
||||
attachAskUserQuestionArgs,
|
||||
createMessageFilterPii,
|
||||
isScheduleFireRequest,
|
||||
readScheduleFireClaims,
|
||||
} = require('@librechat/api');
|
||||
const { createSseStreamTelemetry } = require('@librechat/api/telemetry');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
|
|
@ -488,29 +488,43 @@ const chatRouter = express.Router();
|
|||
// of re-verifying a token that may have expired in-flight, which would otherwise
|
||||
// throttle/limit a legitimate fire and record schedule errors toward auto-disable.
|
||||
chatRouter.use((req, _res, next) => {
|
||||
req._isScheduledFire = isScheduleFireRequest(req);
|
||||
const claims = readScheduleFireClaims(req);
|
||||
req._isScheduledFire = claims.scheduled;
|
||||
req._isManualScheduledFire = claims.manual;
|
||||
next();
|
||||
});
|
||||
chatRouter.use(configMiddleware);
|
||||
|
||||
/**
|
||||
* Scheduled fires are exempt from interactive message limiters (the token's
|
||||
* scope claim is signature-verified): the scheduler's own caps govern them,
|
||||
* and stacking both throttles would record legitimate fires as errors that
|
||||
* walk schedules toward auto-disable. Reads the flag captured above so a token
|
||||
* that expired during config/middleware still exempts a valid fire.
|
||||
* 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 skipScheduledFires = (limiter) => (req, res, next) =>
|
||||
(typeof req._isScheduledFire === 'boolean' ? req._isScheduledFire : isScheduleFireRequest(req))
|
||||
? next()
|
||||
: limiter(req, res, next);
|
||||
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);
|
||||
};
|
||||
|
||||
if (isEnabled(LIMIT_MESSAGE_IP)) {
|
||||
chatRouter.use(skipScheduledFires(messageIpLimiter));
|
||||
chatRouter.use(skipAutomaticFires(messageIpLimiter));
|
||||
}
|
||||
|
||||
if (isEnabled(LIMIT_MESSAGE_USER)) {
|
||||
chatRouter.use(skipScheduledFires(messageUserLimiter));
|
||||
chatRouter.use(skipAutomaticFires(messageUserLimiter));
|
||||
}
|
||||
|
||||
chatRouter.use('/', chat);
|
||||
|
|
|
|||
72
packages/api/src/crypto/jwt.spec.ts
Normal file
72
packages/api/src/crypto/jwt.spec.ts
Normal file
|
|
@ -0,0 +1,72 @@
|
|||
import {
|
||||
isScheduleFireRequest,
|
||||
generateShortLivedToken,
|
||||
readScheduleFireClaims,
|
||||
SCHEDULE_FIRE_SCOPE,
|
||||
SCHEDULE_MANUAL_CLAIM,
|
||||
} from './jwt';
|
||||
|
||||
/** Mirrors the loopback fire's headers, which is the only shape these helpers accept. */
|
||||
function fireHeaders(token: string, scheduled = true) {
|
||||
return {
|
||||
headers: {
|
||||
...(scheduled ? { 'x-lc-scheduled': '1' } : {}),
|
||||
authorization: `Bearer ${token}`,
|
||||
},
|
||||
};
|
||||
}
|
||||
|
||||
describe('schedule fire claims', () => {
|
||||
const original = process.env.JWT_SECRET;
|
||||
|
||||
beforeAll(() => {
|
||||
process.env.JWT_SECRET = 'test-secret';
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
process.env.JWT_SECRET = original;
|
||||
});
|
||||
|
||||
it('marks an automatic occurrence as scheduled but NOT manual', () => {
|
||||
const token = generateShortLivedToken('user-1', '60s', { scope: SCHEDULE_FIRE_SCOPE });
|
||||
expect(readScheduleFireClaims(fireHeaders(token))).toEqual({ scheduled: true, manual: false });
|
||||
});
|
||||
|
||||
it('marks a Run Now fire as manual', () => {
|
||||
const token = generateShortLivedToken('user-1', '60s', {
|
||||
scope: SCHEDULE_FIRE_SCOPE,
|
||||
[SCHEDULE_MANUAL_CLAIM]: '1',
|
||||
});
|
||||
// Run Now dispatches the same billed generation over the same scoped token, but
|
||||
// enforces no cadence floor and no per-user window, so the limiter has to be able
|
||||
// to tell the two apart.
|
||||
expect(readScheduleFireClaims(fireHeaders(token))).toEqual({ scheduled: true, manual: true });
|
||||
});
|
||||
|
||||
it('keeps a manual fire recognizable AS a scheduled fire', () => {
|
||||
const token = generateShortLivedToken('user-1', '60s', {
|
||||
scope: SCHEDULE_FIRE_SCOPE,
|
||||
[SCHEDULE_MANUAL_CLAIM]: '1',
|
||||
});
|
||||
// The controller attributes the run to its schedule off this predicate, so losing
|
||||
// it for Run Now would orphan the manual occurrence's run row.
|
||||
expect(isScheduleFireRequest(fireHeaders(token))).toBe(true);
|
||||
});
|
||||
|
||||
it('rejects an unsigned or wrong-scope token', () => {
|
||||
const wrongScope = generateShortLivedToken('user-1', '60s', { scope: 'something_else' });
|
||||
expect(readScheduleFireClaims(fireHeaders(wrongScope))).toEqual({
|
||||
scheduled: false,
|
||||
manual: false,
|
||||
});
|
||||
expect(readScheduleFireClaims(fireHeaders('not-a-jwt'))).toEqual({
|
||||
scheduled: false,
|
||||
manual: false,
|
||||
});
|
||||
});
|
||||
|
||||
it('rejects a valid token without the scheduled header', () => {
|
||||
const token = generateShortLivedToken('user-1', '60s', { scope: SCHEDULE_FIRE_SCOPE });
|
||||
expect(readScheduleFireClaims(fireHeaders(token, false)).scheduled).toBe(false);
|
||||
});
|
||||
});
|
||||
|
|
@ -23,22 +23,47 @@ export const generateShortLivedToken = (
|
|||
|
||||
export const SCHEDULE_FIRE_SCOPE = 'schedule_fire';
|
||||
|
||||
/** True when the request bears a server-minted schedule-fire token (scope claim verified). */
|
||||
export const isScheduleFireRequest = (req: {
|
||||
/** Claim marking a fire a USER-triggered Run Now rather than an automatic occurrence. */
|
||||
export const SCHEDULE_MANUAL_CLAIM = 'manual';
|
||||
|
||||
export interface ScheduleFireClaims {
|
||||
/** A server-minted schedule fire of either kind. */
|
||||
scheduled: boolean;
|
||||
/**
|
||||
* Triggered by the owner clicking Run Now. Automatic occurrences are governed by the
|
||||
* scheduler's own caps (cadence floor, fireConcurrency), which is what justifies
|
||||
* exempting them from the interactive limiters. Run Now has neither, so it is
|
||||
* user-paced request volume wearing a scheduled token and must NOT inherit that
|
||||
* exemption.
|
||||
*/
|
||||
manual: boolean;
|
||||
}
|
||||
|
||||
/** Verifies a request's schedule-fire token and reports which kind of fire it is. */
|
||||
export const readScheduleFireClaims = (req: {
|
||||
headers: Record<string, string | string[] | undefined>;
|
||||
}): boolean => {
|
||||
}): ScheduleFireClaims => {
|
||||
const none: ScheduleFireClaims = { scheduled: false, manual: false };
|
||||
if (req.headers['x-lc-scheduled'] !== '1') {
|
||||
return false;
|
||||
return none;
|
||||
}
|
||||
const auth = req.headers.authorization;
|
||||
const token = typeof auth === 'string' && auth.startsWith('Bearer ') ? auth.slice(7) : undefined;
|
||||
if (!token) {
|
||||
return false;
|
||||
return none;
|
||||
}
|
||||
try {
|
||||
const payload = jwt.verify(token, process.env.JWT_SECRET!, { algorithms: ['HS256'] });
|
||||
return typeof payload === 'object' && payload?.scope === SCHEDULE_FIRE_SCOPE;
|
||||
if (typeof payload !== 'object' || payload?.scope !== SCHEDULE_FIRE_SCOPE) {
|
||||
return none;
|
||||
}
|
||||
return { scheduled: true, manual: payload[SCHEDULE_MANUAL_CLAIM] === '1' };
|
||||
} catch {
|
||||
return false;
|
||||
return none;
|
||||
}
|
||||
};
|
||||
|
||||
/** True when the request bears a server-minted schedule-fire token (scope claim verified). */
|
||||
export const isScheduleFireRequest = (req: {
|
||||
headers: Record<string, string | string[] | undefined>;
|
||||
}): boolean => readScheduleFireClaims(req).scheduled;
|
||||
|
|
|
|||
|
|
@ -221,6 +221,28 @@ describe('fireSchedule', () => {
|
|||
expect([...runs.values()][0].status).toBe('started');
|
||||
});
|
||||
|
||||
it('mints a MANUAL fire token for Run Now', async () => {
|
||||
const { methods } = makeMethods();
|
||||
mockFetch(async () => okResponse());
|
||||
const mintFireToken = jest.fn(() => 'tok');
|
||||
await fireSchedule(makeDeps(methods, { mintFireToken }), makeSchedule(), LIMITS, dueAt(), {
|
||||
manual: true,
|
||||
});
|
||||
// Run Now dispatches the same billed generation over the same scoped token but
|
||||
// enforces no cadence floor and no per-user window, so it must NOT inherit the
|
||||
// automatic occurrence's exemption from the interactive message limiters.
|
||||
expect(mintFireToken).toHaveBeenCalledWith('user-1', { manual: true });
|
||||
});
|
||||
|
||||
it('mints a NON-manual token for an automatic occurrence', async () => {
|
||||
const { methods } = makeMethods();
|
||||
mockFetch(async () => okResponse());
|
||||
const mintFireToken = jest.fn(() => 'tok');
|
||||
await fireSchedule(makeDeps(methods, { mintFireToken }), makeSchedule(), LIMITS, dueAt());
|
||||
// The scheduler's own caps govern these, which is what justifies the exemption.
|
||||
expect(mintFireToken).toHaveBeenCalledWith('user-1', { manual: false });
|
||||
});
|
||||
|
||||
it('carries the claimed config revision on the loopback POST', async () => {
|
||||
const { methods } = makeMethods();
|
||||
mockFetch(async () => okResponse());
|
||||
|
|
|
|||
|
|
@ -76,6 +76,7 @@ async function postChatMessage(
|
|||
scheduledFor: Date,
|
||||
files: Awaited<ReturnType<ScheduleEngineDeps['resolveFiles']>>,
|
||||
conversationId: string,
|
||||
manual: boolean,
|
||||
): Promise<{ conversationId: string }> {
|
||||
const controller = new AbortController();
|
||||
const timeout = setTimeout(() => controller.abort(), FIRE_REQUEST_TIMEOUT_MS);
|
||||
|
|
@ -85,6 +86,7 @@ async function postChatMessage(
|
|||
try {
|
||||
return await postChatMessageInner(deps, schedule, userId, scheduledFor, files, conversationId, {
|
||||
controller,
|
||||
manual,
|
||||
});
|
||||
} finally {
|
||||
clearTimeout(timeout);
|
||||
|
|
@ -98,7 +100,7 @@ async function postChatMessageInner(
|
|||
scheduledFor: Date,
|
||||
files: Awaited<ReturnType<ScheduleEngineDeps['resolveFiles']>>,
|
||||
conversationId: string,
|
||||
{ controller }: { controller: AbortController },
|
||||
{ controller, manual }: { controller: AbortController; manual: boolean },
|
||||
): Promise<{ conversationId: string }> {
|
||||
let response: Response;
|
||||
try {
|
||||
|
|
@ -107,7 +109,7 @@ async function postChatMessageInner(
|
|||
signal: controller.signal,
|
||||
headers: {
|
||||
'Content-Type': 'application/json',
|
||||
Authorization: `Bearer ${deps.mintFireToken(userId)}`,
|
||||
Authorization: `Bearer ${deps.mintFireToken(userId, { manual })}`,
|
||||
'x-lc-scheduled': '1',
|
||||
// The agents router runs uaParser (rejects non-browser requests as
|
||||
// "Illegal request") before the scheduled-fire exemption, and Node/undici
|
||||
|
|
@ -494,7 +496,15 @@ export async function fireSchedule(
|
|||
}
|
||||
|
||||
try {
|
||||
await postChatMessage(deps, schedule, user.id, scheduledFor, files, conversationId);
|
||||
await postChatMessage(
|
||||
deps,
|
||||
schedule,
|
||||
user.id,
|
||||
scheduledFor,
|
||||
files,
|
||||
conversationId,
|
||||
options?.manual === true,
|
||||
);
|
||||
} catch (error) {
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
const ambiguous = error instanceof ScheduleFireError && error.ambiguous;
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@ import type {
|
|||
import type { SerializableJobData } from '../stream/interfaces/IJobStore';
|
||||
import type { BalanceUpdateFields } from '../types/balance';
|
||||
import type { GetAppConfigOptions } from '../app/service';
|
||||
import { generateShortLivedToken, SCHEDULE_FIRE_SCOPE } from '../crypto/jwt';
|
||||
import { generateShortLivedToken, SCHEDULE_FIRE_SCOPE, SCHEDULE_MANUAL_CLAIM } from '../crypto/jwt';
|
||||
import { GenerationJobManager } from '../stream/GenerationJobManager';
|
||||
import { buildBalanceUpdateFields } from '../middleware/balance';
|
||||
import { deleteAgentCheckpoint } from '../agents/checkpointer';
|
||||
|
|
@ -316,8 +316,13 @@ export function createSchedulesService(deps: SchedulesServiceDeps): SchedulesSer
|
|||
source: file.source,
|
||||
}));
|
||||
},
|
||||
mintFireToken: (userId) =>
|
||||
generateShortLivedToken(userId, SCHEDULE_FIRE_TOKEN_TTL, { scope: SCHEDULE_FIRE_SCOPE }),
|
||||
mintFireToken: (userId, options) =>
|
||||
generateShortLivedToken(userId, SCHEDULE_FIRE_TOKEN_TTL, {
|
||||
scope: SCHEDULE_FIRE_SCOPE,
|
||||
// Signed, so the limiter can trust it: Run Now must not inherit the automatic
|
||||
// occurrence's exemption from the interactive message limiters.
|
||||
...(options?.manual ? { [SCHEDULE_MANUAL_CLAIM]: '1' } : {}),
|
||||
}),
|
||||
getSelfUrl: () =>
|
||||
process.env.SCHEDULES_SELF_URL ?? `http://127.0.0.1:${process.env.PORT ?? 3080}`,
|
||||
runInTenantContext: (user, fn) =>
|
||||
|
|
|
|||
|
|
@ -53,7 +53,7 @@ export interface ScheduleEngineDeps {
|
|||
/** Re-resolves stored file_ids to attachment payloads; missing files are simply absent. */
|
||||
resolveFiles: (fileIds: string[], user: ScheduleUserContext) => Promise<ScheduleFileRef[]>;
|
||||
/** Mints the schedule-scoped short-lived JWT accepted by requireJwtAuth. */
|
||||
mintFireToken: (userId: string) => string;
|
||||
mintFireToken: (userId: string, options?: { manual?: boolean }) => string;
|
||||
/** Base URL of this server for the loopback fire POST. */
|
||||
getSelfUrl: () => string;
|
||||
/** Runs fn inside the owner's tenant ALS context. */
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue