From 89d10a0b837b603d9a4862762cb5e91fa2724966 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 18 May 2026 09:04:31 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Escape=20People?= =?UTF-8?q?=20Picker=20Search=20Regex=20(#13169)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../controllers/PermissionsController.js | 17 +++-- .../__tests__/PermissionsController.spec.js | 74 ++++++++++++++++++- .../src/methods/user.methods.spec.ts | 28 +++++++ packages/data-schemas/src/methods/user.ts | 11 +-- .../src/methods/userGroup.spec.ts | 43 ++++++++++- .../data-schemas/src/methods/userGroup.ts | 18 ++--- 6 files changed, 165 insertions(+), 26 deletions(-) diff --git a/api/server/controllers/PermissionsController.js b/api/server/controllers/PermissionsController.js index 1f200fce83..ffe159a82c 100644 --- a/api/server/controllers/PermissionsController.js +++ b/api/server/controllers/PermissionsController.js @@ -385,15 +385,17 @@ const getUserEffectivePermissions = async (req, res) => { */ const searchPrincipals = async (req, res) => { try { - const { q: query, limit = 20, types } = req.query; + const { q: rawQuery, limit = 20, types } = req.query; - if (!query || query.trim().length === 0) { + if (typeof rawQuery !== 'string' || rawQuery.trim().length === 0) { return res.status(400).json({ error: 'Query parameter "q" is required and must not be empty', }); } - if (query.trim().length < 2) { + const query = rawQuery.trim(); + + if (query.length < 2) { return res.status(400).json({ error: 'Query must be at least 2 characters long', }); @@ -410,7 +412,7 @@ const searchPrincipals = async (req, res) => { typeFilters = validTypes.length > 0 ? validTypes : null; } - const localResults = await db.searchPrincipals(query.trim(), searchLimit, typeFilters); + const localResults = await db.searchPrincipals(query, searchLimit, typeFilters); let allPrincipals = [...localResults]; const useEntraId = entraIdPrincipalFeatureEnabled(req.user); @@ -437,7 +439,7 @@ const searchPrincipals = async (req, res) => { const graphResults = await searchEntraIdPrincipals( accessToken, req.user.openidId, - query.trim(), + query, graphType, searchLimit - localResults.length, ); @@ -466,7 +468,7 @@ const searchPrincipals = async (req, res) => { } const scoredResults = allPrincipals.map((item) => ({ ...item, - _searchScore: db.calculateRelevanceScore(item, query.trim()), + _searchScore: db.calculateRelevanceScore(item, query), })); const finalResults = db @@ -478,7 +480,7 @@ const searchPrincipals = async (req, res) => { }); res.status(200).json({ - query: query.trim(), + query, limit: searchLimit, types: typeFilters, results: finalResults, @@ -492,7 +494,6 @@ const searchPrincipals = async (req, res) => { logger.error('Error searching principals:', error); res.status(500).json({ error: 'Failed to search principals', - details: error.message, }); } }; diff --git a/api/server/controllers/__tests__/PermissionsController.spec.js b/api/server/controllers/__tests__/PermissionsController.spec.js index a8d9518455..6c42ccc59b 100644 --- a/api/server/controllers/__tests__/PermissionsController.spec.js +++ b/api/server/controllers/__tests__/PermissionsController.spec.js @@ -43,7 +43,8 @@ jest.mock('~/server/services/GraphApiService', () => ({ searchEntraIdPrincipals: jest.fn(), })); -const { updateResourcePermissions } = require('../PermissionsController'); +const db = require('~/models'); +const { updateResourcePermissions, searchPrincipals } = require('../PermissionsController'); const createMockReq = (overrides = {}) => ({ params: { resourceType: ResourceType.AGENT, resourceId: '507f1f77bcf86cd799439011' }, @@ -67,6 +68,77 @@ describe('PermissionsController', () => { jest.clearAllMocks(); }); + describe('searchPrincipals', () => { + beforeEach(() => { + db.searchPrincipals.mockResolvedValue([]); + db.calculateRelevanceScore.mockReturnValue(50); + db.sortPrincipalsByRelevance.mockImplementation((results) => results); + }); + + it('rejects non-string query parameters', async () => { + const req = createMockReq({ + query: { q: ['alice'] }, + }); + const res = createMockRes(); + + await searchPrincipals(req, res); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith({ + error: 'Query parameter "q" is required and must not be empty', + }); + expect(db.searchPrincipals).not.toHaveBeenCalled(); + }); + + it('searches with the trimmed literal query', async () => { + db.searchPrincipals.mockResolvedValue([ + { + id: 'user-1', + type: PrincipalType.USER, + name: 'Regex [invalid User', + source: 'local', + }, + ]); + + const req = createMockReq({ + query: { q: ' [invalid ', limit: '5', types: PrincipalType.USER }, + }); + const res = createMockRes(); + + await searchPrincipals(req, res); + + expect(db.searchPrincipals).toHaveBeenCalledWith('[invalid', 5, [PrincipalType.USER]); + expect(db.calculateRelevanceScore).toHaveBeenCalledWith( + expect.objectContaining({ name: 'Regex [invalid User' }), + '[invalid', + ); + expect(res.status).toHaveBeenCalledWith(200); + expect(res.json).toHaveBeenCalledWith( + expect.objectContaining({ + query: '[invalid', + limit: 5, + count: 1, + }), + ); + }); + + it('does not expose internal error details on search failures', async () => { + db.searchPrincipals.mockRejectedValue(new Error('database failure with internal detail')); + + const req = createMockReq({ + query: { q: 'alice' }, + }); + const res = createMockRes(); + + await searchPrincipals(req, res); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ + error: 'Failed to search principals', + }); + }); + }); + describe('updateResourcePermissions — favorites cleanup', () => { const agentObjectId = new mongoose.Types.ObjectId().toString(); const revokedUserId = new mongoose.Types.ObjectId().toString(); diff --git a/packages/data-schemas/src/methods/user.methods.spec.ts b/packages/data-schemas/src/methods/user.methods.spec.ts index 091cdf4f76..9298447198 100644 --- a/packages/data-schemas/src/methods/user.methods.spec.ts +++ b/packages/data-schemas/src/methods/user.methods.spec.ts @@ -476,6 +476,34 @@ describe('User Methods - Database Tests', () => { expect(results).toEqual([]); }); + test('should treat regex metacharacters as literal search text', async () => { + await User.create({ + name: 'Literal .* User', + email: 'literal-star@test.com', + username: 'literal-star', + provider: 'local', + }); + + const results = await methods.searchUsers({ searchPattern: '.*' }); + + expect(results).toHaveLength(1); + expect((results[0] as unknown as t.IUser).name).toBe('Literal .* User'); + }); + + test('should handle invalid regex syntax as literal search text', async () => { + await User.create({ + name: 'Regex [invalid User', + email: 'regex-invalid@test.com', + username: 'regex-invalid', + provider: 'local', + }); + + const results = await methods.searchUsers({ searchPattern: '[invalid' }); + + expect(results).toHaveLength(1); + expect((results[0] as unknown as t.IUser).name).toBe('Regex [invalid User'); + }); + test('should apply field selection', async () => { const results = await methods.searchUsers({ searchPattern: 'john', diff --git a/packages/data-schemas/src/methods/user.ts b/packages/data-schemas/src/methods/user.ts index d7aa3f90c9..7573a48f6d 100644 --- a/packages/data-schemas/src/methods/user.ts +++ b/packages/data-schemas/src/methods/user.ts @@ -1,6 +1,7 @@ import mongoose, { FilterQuery } from 'mongoose'; import type { RefillIntervalUnit } from 'librechat-data-provider'; import type { IUser, BalanceConfig, CreateUserRequest, UserDeleteResult } from '~/types'; +import { escapeRegExp } from '~/utils/string'; import { signPayload } from '~/crypto'; /** Default JWT session expiry: 15 minutes in milliseconds */ @@ -259,7 +260,8 @@ export function createUserMethods(mongoose: typeof import('mongoose')) { return []; } - const regex = new RegExp(searchPattern.trim(), 'i'); + const trimmedPattern = searchPattern.trim(); + const regex = new RegExp(escapeRegExp(trimmedPattern), 'i'); const User = mongoose.models.User; const query = User.find({ @@ -273,8 +275,7 @@ export function createUserMethods(mongoose: typeof import('mongoose')) { const users = await query.lean(); // Score results by relevance - const exactRegex = new RegExp(`^${searchPattern.trim()}$`, 'i'); - const startsWithPattern = searchPattern.trim().toLowerCase(); + const startsWithPattern = trimmedPattern.toLowerCase(); const scoredUsers = users.map((user) => { const searchableFields = [user.name, user.email, user.username].filter( @@ -287,7 +288,7 @@ export function createUserMethods(mongoose: typeof import('mongoose')) { let score = 0; // Exact match gets highest score - if (exactRegex.test(field)) { + if (fieldLower === startsWithPattern) { score = 100; } // Starts with query gets high score @@ -298,7 +299,7 @@ export function createUserMethods(mongoose: typeof import('mongoose')) { else if (fieldLower.includes(startsWithPattern)) { score = 50; } - // Default score for regex match + // Default score for database match else { score = 10; } diff --git a/packages/data-schemas/src/methods/userGroup.spec.ts b/packages/data-schemas/src/methods/userGroup.spec.ts index ca83ced7d9..2048fe65a5 100644 --- a/packages/data-schemas/src/methods/userGroup.spec.ts +++ b/packages/data-schemas/src/methods/userGroup.spec.ts @@ -100,6 +100,7 @@ describe('userGroup methods', () => { { name: 'Engineering', source: 'local', description: 'Eng team' }, { name: 'Design', source: 'local', email: 'design@co.com' }, { name: 'Entra Eng', source: 'entra', idOnTheSource: 'ext-1' }, + { name: 'Literal .* Group', source: 'local' }, ]); }); @@ -120,6 +121,12 @@ describe('userGroup methods', () => { expect(results[0].name).toBe('Engineering'); }); + it('treats regex metacharacters as literal text', async () => { + const results = await methods.findGroupsByNamePattern('.*'); + expect(results).toHaveLength(1); + expect(results[0].name).toBe('Literal .* Group'); + }); + it('filters by source when provided', async () => { const results = await methods.findGroupsByNamePattern('eng', 'entra'); expect(results).toHaveLength(1); @@ -525,7 +532,7 @@ describe('userGroup methods', () => { expect(score).toBe(50); }); - it('returns 10 (default) when no substring or exact match — regex fallback', () => { + it('returns 10 (default) when no substring or exact match', () => { const score = methods.calculateRelevanceScore( { type: PrincipalType.USER, name: 'bob', source: 'local' }, 'zzz', @@ -573,12 +580,12 @@ describe('userGroup methods', () => { expect(score).toBe(80); }); - it('returns 100 when regex pattern matches exactly via dot wildcard', () => { + it('does not treat regex metacharacters as wildcards', () => { const score = methods.calculateRelevanceScore( { type: PrincipalType.USER, name: 'xYz', source: 'local' }, 'x.z', ); - expect(score).toBe(100); + expect(score).toBe(10); }); }); @@ -671,6 +678,36 @@ describe('userGroup methods', () => { expect(results).toEqual([]); }); + it('treats regex metacharacters as literal search text', async () => { + await User.create({ + name: 'Literal .* User', + email: 'literal-star@test.com', + username: 'literal-star', + password: 'password123', + provider: 'local', + }); + + const results = await methods.searchPrincipals('.*', 10, [PrincipalType.USER]); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe('Literal .* User'); + }); + + it('handles invalid regex syntax as literal search text', async () => { + await User.create({ + name: 'Regex [invalid User', + email: 'regex-invalid@test.com', + username: 'regex-invalid', + password: 'password123', + provider: 'local', + }); + + const results = await methods.searchPrincipals('[invalid', 10, [PrincipalType.USER]); + + expect(results).toHaveLength(1); + expect(results[0].name).toBe('Regex [invalid User'); + }); + it('finds matching users', async () => { const results = await methods.searchPrincipals('alice'); const userResults = results.filter((r) => r.type === PrincipalType.USER); diff --git a/packages/data-schemas/src/methods/userGroup.ts b/packages/data-schemas/src/methods/userGroup.ts index 5e7db57b62..3122806627 100644 --- a/packages/data-schemas/src/methods/userGroup.ts +++ b/packages/data-schemas/src/methods/userGroup.ts @@ -86,7 +86,7 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { session?: ClientSession, ): Promise { const Group = mongoose.models.Group as Model; - const regex = new RegExp(namePattern, 'i'); + const regex = new RegExp(escapeRegExp(namePattern), 'i'); const query: Record = { $or: [{ name: regex }, { email: regex }, { description: regex }], }; @@ -436,8 +436,7 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { * @returns Relevance score (0-100) */ function calculateRelevanceScore(item: TPrincipalSearchResult, searchPattern: string): number { - const exactRegex = new RegExp(`^${searchPattern}$`, 'i'); - const startsWithPattern = searchPattern.toLowerCase(); + const normalizedPattern = searchPattern.toLowerCase(); /** Get searchable text based on type */ const searchableFields = @@ -453,16 +452,16 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { let score = 0; /** Exact match gets highest score */ - if (exactRegex.test(field)) { + if (fieldLower === normalizedPattern) { score = 100; - } else if (fieldLower.startsWith(startsWithPattern)) { + } else if (fieldLower.startsWith(normalizedPattern)) { /** Starts with query gets high score */ score = 80; - } else if (fieldLower.includes(startsWithPattern)) { + } else if (fieldLower.includes(normalizedPattern)) { /** Contains query gets medium score */ score = 50; } else { - /** Default score for regex match */ + /** Default score for database match */ score = 10; } @@ -551,6 +550,7 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { } const trimmedPattern = searchPattern.trim(); + const escapedPattern = escapeRegExp(trimmedPattern); const promises: Promise[] = []; if (!typeFilter || typeFilter.includes(PrincipalType.USER)) { @@ -558,7 +558,7 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { const userFields = 'name email username avatar provider idOnTheSource'; /** For now, we'll use a direct query instead of searchUsers */ const User = mongoose.models.User as Model; - const regex = new RegExp(trimmedPattern, 'i'); + const regex = new RegExp(escapedPattern, 'i'); const userQuery = User.find({ $or: [{ name: regex }, { email: regex }, { username: regex }], }) @@ -601,7 +601,7 @@ export function createUserGroupMethods(mongoose: typeof import('mongoose')) { if (!typeFilter || typeFilter.includes(PrincipalType.ROLE)) { const Role = mongoose.models.Role as Model; if (Role) { - const regex = new RegExp(trimmedPattern, 'i'); + const regex = new RegExp(escapedPattern, 'i'); const roleQuery = Role.find({ name: regex }).select('name').limit(limitPerType); if (session) {