From 78b320cbbc2915f22f0fbf375736404def32d73f Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Thu, 3 Sep 2026 23:52:36 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=9C=EF=B8=8F=20fix:=20Bound=20Conversa?= =?UTF-8?q?tion=20Page=20Size=20(#15576)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /api/convos` read its page size as `parseInt(req.query.limit, 10) || 25` and handed it to `getConvosByCursor` unclamped, where it becomes `.limit(limit + 1)`. `?limit=-1` therefore issued `.limit(0)`, which MongoDB reads as no limit at all, and the route returned the caller's entire conversation list in one response; `?limit=999999999` forced the same unbounded fetch. `attachSharedFlags` then amplified it with a second query whose `$in` carried every returned id. Every other list route already clamps, so the fix reuses that shape: the projects handler's local `normalizeLimit` moves to `packages/api/src/utils/query` and both routes share it, and `getConvosByCursor` bounds its own page size so the invariant holds for any caller. --- .../__test-utils__/convos-route-mocks.js | 9 +++ api/server/routes/__tests__/convos.spec.js | 67 +++++++++++++++++++ api/server/routes/convos.js | 3 +- packages/api/src/projects/handlers.ts | 20 +----- packages/api/src/utils/index.ts | 1 + packages/api/src/utils/query.spec.ts | 56 ++++++++++++++++ packages/api/src/utils/query.ts | 32 +++++++++ .../src/methods/conversation.spec.ts | 66 ++++++++++++++++++ .../data-schemas/src/methods/conversation.ts | 14 +++- 9 files changed, 246 insertions(+), 22 deletions(-) create mode 100644 packages/api/src/utils/query.spec.ts create mode 100644 packages/api/src/utils/query.ts diff --git a/api/server/routes/__test-utils__/convos-route-mocks.js b/api/server/routes/__test-utils__/convos-route-mocks.js index 9240fe2759..9c6ddc2aef 100644 --- a/api/server/routes/__test-utils__/convos-route-mocks.js +++ b/api/server/routes/__test-utils__/convos-route-mocks.js @@ -36,6 +36,15 @@ module.exports = { } return false; }), + /** Mirrors the real helper so the route's page-size clamping is exercised. */ + normalizeLimit: jest.fn((value, { fallback = 25, max = 100 } = {}) => { + const raw = Array.isArray(value) ? value[0] : value; + const limit = parseInt(typeof raw === 'string' ? raw : '', 10); + if (!Number.isFinite(limit)) { + return fallback; + } + return Math.min(Math.max(limit, 1), max); + }), resolveImportMaxFileSize: jest.fn(() => 262144000), createAxiosInstance: jest.fn(() => ({ get: jest.fn(), diff --git a/api/server/routes/__tests__/convos.spec.js b/api/server/routes/__tests__/convos.spec.js index 65bd45141d..620d9c1f54 100644 --- a/api/server/routes/__tests__/convos.spec.js +++ b/api/server/routes/__tests__/convos.spec.js @@ -1417,6 +1417,73 @@ describe('Convos Routes', () => { }); }); + describe('GET / limit clamping', () => { + const { getConvosByCursor } = require('~/models'); + + beforeEach(() => { + getConvosByCursor.mockResolvedValue({ conversations: [], nextCursor: null }); + }); + + /** `parseInt('-1') || 25` kept the -1, and `.limit(-1 + 1)` is `.limit(0)`, which + * MongoDB reads as "no limit" — one request drained the caller's whole list. */ + it('clamps a negative limit instead of forwarding it', async () => { + const response = await request(app).get('/api/convos').query({ limit: '-1' }); + + expect(response.status).toBe(200); + expect(getConvosByCursor).toHaveBeenCalledWith( + 'test-user-123', + expect.objectContaining({ limit: 1 }), + ); + }); + + it('caps an oversized limit at the page ceiling', async () => { + const response = await request(app).get('/api/convos').query({ limit: '999999999' }); + + expect(response.status).toBe(200); + expect(getConvosByCursor).toHaveBeenCalledWith( + 'test-user-123', + expect.objectContaining({ limit: 100 }), + ); + }); + + it('raises a zero limit to a single row', async () => { + const response = await request(app).get('/api/convos').query({ limit: '0' }); + + expect(response.status).toBe(200); + expect(getConvosByCursor).toHaveBeenCalledWith( + 'test-user-123', + expect.objectContaining({ limit: 1 }), + ); + }); + + it.each([ + ['absent', undefined], + ['non-numeric', 'all'], + ])('falls back to the default page size when the limit is %s', async (_label, limit) => { + const response = await request(app) + .get('/api/convos') + .query(limit === undefined ? {} : { limit }); + + expect(response.status).toBe(200); + expect(getConvosByCursor).toHaveBeenCalledWith( + 'test-user-123', + expect.objectContaining({ limit: 25 }), + ); + }); + + /** Express parses a repeated key into an array, which `parseInt` silently reads as + * its first element's digits rather than rejecting. */ + it('clamps a repeated limit parameter', async () => { + const response = await request(app).get('/api/convos?limit=999999&limit=5'); + + expect(response.status).toBe(200); + expect(getConvosByCursor).toHaveBeenCalledWith( + 'test-user-123', + expect.objectContaining({ limit: 100 }), + ); + }); + }); + describe('POST /archive', () => { it('should archive a conversation successfully', async () => { const mockConversationId = 'conv-123'; diff --git a/api/server/routes/convos.js b/api/server/routes/convos.js index 06250c8f3d..ef2de837d2 100644 --- a/api/server/routes/convos.js +++ b/api/server/routes/convos.js @@ -3,6 +3,7 @@ const express = require('express'); const { sleep } = require('@librechat/agents'); const { isEnabled, + normalizeLimit, deleteAgentCheckpoints, createArchiveAllHandler, createSubagentActivityStreamHandler, @@ -141,7 +142,7 @@ const isValidProjectFilter = (projectId) => !projectId || projectId === 'unassigned' || /^[a-f\d]{24}$/i.test(projectId); router.get('/', async (req, res) => { - const limit = parseInt(req.query.limit, 10) || 25; + const limit = normalizeLimit(req.query.limit); const cursor = req.query.cursor; const isArchived = isEnabled(req.query.isArchived); const pinned = isEnabled(req.query.pinned); diff --git a/packages/api/src/projects/handlers.ts b/packages/api/src/projects/handlers.ts index 6c05457d07..8ef4f72456 100644 --- a/packages/api/src/projects/handlers.ts +++ b/packages/api/src/projects/handlers.ts @@ -9,6 +9,8 @@ import type { } from '@librechat/data-schemas'; import type { Request, Response } from 'express'; +import { normalizeLimit, queryString } from '~/utils'; + const PROJECT_NOT_FOUND = 'Project not found'; const CONVERSATION_NOT_FOUND = 'Conversation not found'; @@ -37,27 +39,9 @@ type ProjectHandlerDependencies = Pick< const getUserId = (req: ProjectRequest): string => req.user?.id ?? req.user?._id?.toString() ?? ''; -const queryString = (value: Request['query'][string]): string | undefined => { - if (typeof value === 'string') { - return value; - } - if (Array.isArray(value)) { - return queryString(value[0]); - } - return undefined; -}; - const normalizeString = (value: string | null | undefined): string => typeof value === 'string' ? value.trim() : ''; -const normalizeLimit = (value: Request['query'][string]): number => { - const limit = parseInt(queryString(value) ?? '', 10); - if (!Number.isFinite(limit)) { - return 25; - } - return Math.min(Math.max(limit, 1), 100); -}; - const normalizeSortBy = (value: Request['query'][string]): ChatProjectSortBy | undefined => { const sortBy = queryString(value); return PROJECT_SORT_FIELDS.has(sortBy as ChatProjectSortBy) diff --git a/packages/api/src/utils/index.ts b/packages/api/src/utils/index.ts index 8a12471b3f..136a194f3c 100644 --- a/packages/api/src/utils/index.ts +++ b/packages/api/src/utils/index.ts @@ -23,6 +23,7 @@ export * from './oidc'; export * from './openid'; export * from './promise'; export * from './proxy'; +export * from './query'; export * from './ports'; export * from './sanitizeTitle'; export * from './text'; diff --git a/packages/api/src/utils/query.spec.ts b/packages/api/src/utils/query.spec.ts new file mode 100644 index 0000000000..405eb284c4 --- /dev/null +++ b/packages/api/src/utils/query.spec.ts @@ -0,0 +1,56 @@ +import { DEFAULT_PAGE_LIMIT, MAX_PAGE_LIMIT, normalizeLimit, queryString } from './query'; + +describe('queryString', () => { + it('returns a string value unchanged', () => { + expect(queryString('dev')).toBe('dev'); + }); + + it('reads the first entry of a repeated parameter', () => { + expect(queryString(['dev', 'main'])).toBe('dev'); + }); + + it.each([[undefined], [{ nested: 'value' }], [[]]])('returns undefined for %p', (value) => { + expect(queryString(value as Parameters[0])).toBeUndefined(); + }); +}); + +describe('normalizeLimit', () => { + it('keeps a limit inside the allowed range', () => { + expect(normalizeLimit('40')).toBe(40); + }); + + /** `.limit(0)` is "no limit" in MongoDB, so a negative page size returned everything. */ + it.each([ + ['-1', 1], + ['-5', 1], + ['0', 1], + ])('raises %s to the minimum page size', (value, expected) => { + expect(normalizeLimit(value)).toBe(expected); + }); + + it.each([['999999999'], ['101']])('caps %s at the maximum page size', (value) => { + expect(normalizeLimit(value)).toBe(MAX_PAGE_LIMIT); + }); + + it.each([[undefined], ['all'], [''], [{ limit: '10' }]])( + 'falls back to the default for %p', + (value) => { + expect(normalizeLimit(value as Parameters[0])).toBe( + DEFAULT_PAGE_LIMIT, + ); + }, + ); + + it('reads the first entry of a repeated parameter before clamping', () => { + expect(normalizeLimit(['999999', '5'])).toBe(MAX_PAGE_LIMIT); + }); + + it('honors caller-supplied bounds', () => { + expect(normalizeLimit('500', { max: 1000 })).toBe(500); + expect(normalizeLimit('nope', { fallback: 10 })).toBe(10); + }); + + it('truncates a fractional limit', () => { + expect(normalizeLimit('7.9')).toBe(7); + }); +}); diff --git a/packages/api/src/utils/query.ts b/packages/api/src/utils/query.ts new file mode 100644 index 0000000000..4ca5a4e9bd --- /dev/null +++ b/packages/api/src/utils/query.ts @@ -0,0 +1,32 @@ +import type { Request } from 'express'; + +type QueryValue = Request['query'][string]; + +export const DEFAULT_PAGE_LIMIT = 25; +export const MAX_PAGE_LIMIT = 100; + +/** Express parses a repeated `?limit=a&limit=b` into an array, which `parseInt` reads as its first element's digits. */ +export const queryString = (value: QueryValue): string | undefined => { + if (typeof value === 'string') { + return value; + } + if (Array.isArray(value)) { + return queryString(value[0]); + } + return undefined; +}; + +/** + * Clamps a page size from user input. An unclamped limit reaches Mongo's `.limit()`, + * where a negative value means "one batch" and `0` means "no limit at all". + */ +export const normalizeLimit = ( + value: QueryValue, + { fallback = DEFAULT_PAGE_LIMIT, max = MAX_PAGE_LIMIT }: { fallback?: number; max?: number } = {}, +): number => { + const limit = parseInt(queryString(value) ?? '', 10); + if (!Number.isFinite(limit)) { + return fallback; + } + return Math.min(Math.max(limit, 1), max); +}; diff --git a/packages/data-schemas/src/methods/conversation.spec.ts b/packages/data-schemas/src/methods/conversation.spec.ts index 2a3cf7b9b4..47d3d5effd 100644 --- a/packages/data-schemas/src/methods/conversation.spec.ts +++ b/packages/data-schemas/src/methods/conversation.spec.ts @@ -3665,6 +3665,72 @@ describe('Conversation Operations', () => { }); }); + describe('getConvosByCursor page size bounds', () => { + const TOTAL = 105; + const user = 'page-size-user'; + + beforeEach(async () => { + const baseTime = new Date('2026-06-01T00:00:00.000Z').getTime(); + await Conversation.collection.insertMany( + Array.from({ length: TOTAL }, (_, index) => ({ + conversationId: uuidv4(), + user, + title: `Conversation ${index}`, + endpoint: EModelEndpoint.openAI, + expiredAt: null, + isArchived: false, + createdAt: new Date(baseTime + index * 1000), + updatedAt: new Date(baseTime + index * 1000), + })), + ); + }); + + afterEach(async () => { + await Conversation.deleteMany({ user }); + }); + + /** `.limit(limit + 1)` turned a -1 page size into `.limit(0)`, which MongoDB reads as + * "no limit" — the query returned the caller's entire collection in one response. */ + it.each([ + ['negative', -1], + ['zero', 0], + ['oversized', 999999999], + ])('bounds a %s page size', async (_label, limit) => { + const result = await getConvosByCursor(user, { limit }); + + expect(result.conversations.length).toBeLessThanOrEqual(100); + expect(result.conversations.length).toBeGreaterThan(0); + expect(result.nextCursor).not.toBeNull(); + }); + + it('caps an oversized page size at the ceiling and still pages the remainder', async () => { + const first = await getConvosByCursor(user, { limit: Number.MAX_SAFE_INTEGER }); + + expect(first.conversations).toHaveLength(100); + expect(first.nextCursor).not.toBeNull(); + + const second = await getConvosByCursor(user, { + limit: Number.MAX_SAFE_INTEGER, + cursor: first.nextCursor, + }); + + expect(second.conversations).toHaveLength(TOTAL - 100); + expect(second.nextCursor).toBeNull(); + + const ids = new Set( + [...first.conversations, ...second.conversations].map((convo) => convo.conversationId), + ); + expect(ids.size).toBe(TOTAL); + }); + + it('serves a page size inside the range unchanged', async () => { + const result = await getConvosByCursor(user, { limit: 10 }); + + expect(result.conversations).toHaveLength(10); + expect(result.nextCursor).not.toBeNull(); + }); + }); + describe('subagent thread leases', () => { it('reserves child lineage once without overwriting a concurrent winner', async () => { await Conversation.init(); diff --git a/packages/data-schemas/src/methods/conversation.ts b/packages/data-schemas/src/methods/conversation.ts index 036cf31d46..c6576134b9 100644 --- a/packages/data-schemas/src/methods/conversation.ts +++ b/packages/data-schemas/src/methods/conversation.ts @@ -54,6 +54,9 @@ const AGENT_EVENT_ACTOR_RECEIPT_RETENTION_MS = 90 * 24 * 60 * 60_000; const MAX_AGENT_EVENT_ACTOR_SUSPENSION_BYTES = 64 * 1_024; /** MeiliSearch's default `pagination.maxTotalHits` ceiling. */ const MEILI_SEARCH_LIMIT = 1000; +/** Ceiling for a single conversation page; the sidebar's largest request is 100. */ +const MAX_CONVO_PAGE_SIZE = 100; +const DEFAULT_CONVO_PAGE_SIZE = 25; const escapeMeiliFilterValue = (value: string): string => value.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); @@ -2596,7 +2599,7 @@ export function createConversationMethods( user: string, { cursor, - limit = 25, + limit = DEFAULT_CONVO_PAGE_SIZE, isArchived = false, pinned = false, tags, @@ -2618,6 +2621,11 @@ export function createConversationMethods( ) { const Conversation = mongoose.models.Conversation as Model & Pick; + /* `.limit(0)` is "no limit" to MongoDB and a negative one caps to a single batch, so an + unclamped page size hands the caller the whole collection rather than a page of it. */ + const pageSize = Number.isFinite(limit) + ? Math.min(Math.max(Math.trunc(limit), 1), MAX_CONVO_PAGE_SIZE) + : DEFAULT_CONVO_PAGE_SIZE; const filters: FilterQuery[] = [{ user } as FilterQuery]; if (isArchived) { filters.push({ isArchived: true } as FilterQuery); @@ -2792,11 +2800,11 @@ export function createConversationMethods( 'conversationId endpoint title createdAt updatedAt archivedAt user model agent_id assistant_id spec iconURL chatProjectId pinned', ) .sort(sortObj) - .limit(limit + 1) + .limit(pageSize + 1) .lean(); let nextCursor: string | null = null; - if (convos.length > limit) { + if (convos.length > pageSize) { convos.pop(); const lastReturned = convos[convos.length - 1]; const sortValues: Record = {