From 06ba025bd95574c815ac6968454be7d3b024391c Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 29 Dec 2025 15:10:31 -0500 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=20fix:=20Access=20Control=20on=20A?= =?UTF-8?q?gent=20Permission=20Queries=20(#11145)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds access control check to GET /api/permissions/:resourceType/:resourceId endpoint to prevent unauthorized disclosure of agent permission information. ## Vulnerability Summary LibreChat version 0.8.1-rc2 did not enforce proper access control when querying agent permissions. Any authenticated user could read the permissions of arbitrary agents by knowing the agent ID, even for private agents they had no access to. **Impact:** - Attackers could enumerate which users have access to private agents - Permission levels (owner, editor, viewer) were exposed - User emails and names of permitted users were disclosed - Agent's public/private sharing status was revealed **Attack Vector:** ``` GET /api/permissions/agent/{agent_id} Authorization: Bearer ``` The MongoDB ObjectId format (timestamp + process ID + counter) made it feasible to brute-force discover valid agent IDs. ## Fix Added `checkResourcePermissionAccess` middleware factory that enforces SHARE permission before allowing access to permission queries. This middleware is now applied to the GET endpoint, matching the existing access control on the PUT endpoint. **Before:** ```javascript router.get('/:resourceType/:resourceId', getResourcePermissions); ``` **After:** ```javascript router.get( '/:resourceType/:resourceId', checkResourcePermissionAccess(PermissionBits.SHARE), getResourcePermissions, ); ``` The middleware handles all supported resource types: - Agent (ResourceType.AGENT) - Prompt Group (ResourceType.PROMPTGROUP) - MCP Server (ResourceType.MCPSERVER) ## Code Changes **api/server/routes/accessPermissions.js:** - Added `checkResourcePermissionAccess()` middleware factory - Applied middleware to GET /:resourceType/:resourceId endpoint - Refactored PUT endpoint to use the same middleware factory (DRY) **api/server/routes/accessPermissions.test.js:** - Added security tests verifying unauthorized access is denied - Tests confirm 403 Forbidden for users without SHARE permission ## Security Tests ``` ✓ should deny permission query for user without access (main vulnerability test) ✓ should return 400 for unsupported resource type ✓ should deny permission update for user without access --- api/server/routes/accessPermissions.js | 83 ++++--- api/server/routes/accessPermissions.test.js | 228 ++++++++++++++++++++ 2 files changed, 276 insertions(+), 35 deletions(-) create mode 100644 api/server/routes/accessPermissions.test.js diff --git a/api/server/routes/accessPermissions.js b/api/server/routes/accessPermissions.js index 7431a86f1e..3e70f2610f 100644 --- a/api/server/routes/accessPermissions.js +++ b/api/server/routes/accessPermissions.js @@ -36,52 +36,65 @@ router.get('/search-principals', checkPeoplePickerAccess, searchPrincipals); */ router.get('/:resourceType/roles', getResourceRoles); +/** + * Middleware factory to check resource access for permission-related operations. + * SECURITY: Users must have SHARE permission to view or modify resource permissions. + * @param {string} requiredPermission - The permission bit required (e.g., SHARE) + * @returns Express middleware function + */ +const checkResourcePermissionAccess = (requiredPermission) => (req, res, next) => { + const { resourceType } = req.params; + let middleware; + + if (resourceType === ResourceType.AGENT) { + middleware = canAccessResource({ + resourceType: ResourceType.AGENT, + requiredPermission, + resourceIdParam: 'resourceId', + }); + } else if (resourceType === ResourceType.PROMPTGROUP) { + middleware = canAccessResource({ + resourceType: ResourceType.PROMPTGROUP, + requiredPermission, + resourceIdParam: 'resourceId', + }); + } else if (resourceType === ResourceType.MCPSERVER) { + middleware = canAccessResource({ + resourceType: ResourceType.MCPSERVER, + requiredPermission, + resourceIdParam: 'resourceId', + idResolver: findMCPServerById, + }); + } else { + return res.status(400).json({ + error: 'Bad Request', + message: `Unsupported resource type: ${resourceType}`, + }); + } + + // Execute the middleware + middleware(req, res, next); +}; + /** * GET /api/permissions/{resourceType}/{resourceId} * Get all permissions for a specific resource + * SECURITY: Requires SHARE permission to view resource permissions */ -router.get('/:resourceType/:resourceId', getResourcePermissions); +router.get( + '/:resourceType/:resourceId', + checkResourcePermissionAccess(PermissionBits.SHARE), + getResourcePermissions, +); /** * PUT /api/permissions/{resourceType}/{resourceId} * Bulk update permissions for a specific resource + * SECURITY: Requires SHARE permission to modify resource permissions */ router.put( '/:resourceType/:resourceId', - // Use middleware that dynamically handles resource type and permissions - (req, res, next) => { - const { resourceType } = req.params; - let middleware; - - if (resourceType === ResourceType.AGENT) { - middleware = canAccessResource({ - resourceType: ResourceType.AGENT, - requiredPermission: PermissionBits.SHARE, - resourceIdParam: 'resourceId', - }); - } else if (resourceType === ResourceType.PROMPTGROUP) { - middleware = canAccessResource({ - resourceType: ResourceType.PROMPTGROUP, - requiredPermission: PermissionBits.SHARE, - resourceIdParam: 'resourceId', - }); - } else if (resourceType === ResourceType.MCPSERVER) { - middleware = canAccessResource({ - resourceType: ResourceType.MCPSERVER, - requiredPermission: PermissionBits.SHARE, - resourceIdParam: 'resourceId', - idResolver: findMCPServerById, - }); - } else { - return res.status(400).json({ - error: 'Bad Request', - message: `Unsupported resource type: ${resourceType}`, - }); - } - - // Execute the middleware - middleware(req, res, next); - }, + checkResourcePermissionAccess(PermissionBits.SHARE), updateResourcePermissions, ); diff --git a/api/server/routes/accessPermissions.test.js b/api/server/routes/accessPermissions.test.js new file mode 100644 index 0000000000..c7e7b5957c --- /dev/null +++ b/api/server/routes/accessPermissions.test.js @@ -0,0 +1,228 @@ +const express = require('express'); +const request = require('supertest'); +const mongoose = require('mongoose'); +const { v4: uuidv4 } = require('uuid'); +const { createMethods } = require('@librechat/data-schemas'); +const { MongoMemoryServer } = require('mongodb-memory-server'); +const { ResourceType, PermissionBits } = require('librechat-data-provider'); +const { createAgent } = require('~/models/Agent'); + +/** + * Mock the PermissionsController to isolate route testing + */ +jest.mock('~/server/controllers/PermissionsController', () => ({ + getUserEffectivePermissions: jest.fn((req, res) => res.json({ permissions: [] })), + getAllEffectivePermissions: jest.fn((req, res) => res.json({ permissions: [] })), + updateResourcePermissions: jest.fn((req, res) => res.json({ success: true })), + getResourcePermissions: jest.fn((req, res) => + res.json({ + resourceType: req.params.resourceType, + resourceId: req.params.resourceId, + principals: [], + public: false, + }), + ), + getResourceRoles: jest.fn((req, res) => res.json({ roles: [] })), + searchPrincipals: jest.fn((req, res) => res.json({ principals: [] })), +})); + +jest.mock('~/server/middleware/checkPeoplePickerAccess', () => ({ + checkPeoplePickerAccess: jest.fn((req, res, next) => next()), +})); + +// Import actual middleware to get canAccessResource +const { canAccessResource } = require('~/server/middleware'); +const { findMCPServerById } = require('~/models'); + +/** + * Security Tests for SBA-ADV-20251203-02 + * + * These tests verify that users cannot query or modify agent permissions + * without proper SHARE permission. + */ +describe('Access Permissions Routes - Security Tests (SBA-ADV-20251203-02)', () => { + let app; + let mongoServer; + let authorId; + let attackerId; + let agentId; + let methods; + let User; + let modelsToCleanup = []; + + beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + const mongoUri = mongoServer.getUri(); + await mongoose.connect(mongoUri); + + // Initialize models + const { createModels } = require('@librechat/data-schemas'); + const models = createModels(mongoose); + modelsToCleanup = Object.keys(models); + Object.assign(mongoose.models, models); + + methods = createMethods(mongoose); + User = models.User; + + await methods.seedDefaultRoles(); + }); + + afterAll(async () => { + const collections = mongoose.connection.collections; + for (const key in collections) { + await collections[key].deleteMany({}); + } + for (const modelName of modelsToCleanup) { + delete mongoose.models[modelName]; + } + await mongoose.disconnect(); + await mongoServer.stop(); + }); + + beforeEach(async () => { + // Clear all collections + const collections = mongoose.connection.collections; + for (const key in collections) { + await collections[key].deleteMany({}); + } + await methods.seedDefaultRoles(); + + // Create author (owner of the agent) + authorId = new mongoose.Types.ObjectId().toString(); + await User.create({ + _id: authorId, + name: 'Agent Owner', + email: 'owner@example.com', + username: 'owner@example.com', + provider: 'local', + }); + + // Create attacker (should not have access) + attackerId = new mongoose.Types.ObjectId().toString(); + await User.create({ + _id: attackerId, + name: 'Attacker', + email: 'attacker@example.com', + username: 'attacker@example.com', + provider: 'local', + }); + + // Create private agent owned by author + const customAgentId = `agent_${uuidv4().replace(/-/g, '').substring(0, 20)}`; + await createAgent({ + id: customAgentId, + name: 'Private Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + }); + agentId = customAgentId; + + // Create Express app with attacker as current user + app = express(); + app.use(express.json()); + + // Mock authentication middleware - attacker is the current user + app.use((req, res, next) => { + req.user = { id: attackerId, role: 'USER' }; + req.app = { locals: {} }; + next(); + }); + + // Middleware factory for permission access check (mirrors actual implementation) + const checkResourcePermissionAccess = (requiredPermission) => (req, res, next) => { + const { resourceType } = req.params; + let middleware; + + if (resourceType === ResourceType.AGENT) { + middleware = canAccessResource({ + resourceType: ResourceType.AGENT, + requiredPermission, + resourceIdParam: 'resourceId', + }); + } else if (resourceType === ResourceType.PROMPTGROUP) { + middleware = canAccessResource({ + resourceType: ResourceType.PROMPTGROUP, + requiredPermission, + resourceIdParam: 'resourceId', + }); + } else if (resourceType === ResourceType.MCPSERVER) { + middleware = canAccessResource({ + resourceType: ResourceType.MCPSERVER, + requiredPermission, + resourceIdParam: 'resourceId', + idResolver: findMCPServerById, + }); + } else { + return res.status(400).json({ + error: 'Bad Request', + message: `Unsupported resource type: ${resourceType}`, + }); + } + + middleware(req, res, next); + }; + + // GET route with access control (THE FIX) + app.get( + '/permissions/:resourceType/:resourceId', + checkResourcePermissionAccess(PermissionBits.SHARE), + (req, res) => + res.json({ + resourceType: req.params.resourceType, + resourceId: req.params.resourceId, + principals: [], + public: false, + }), + ); + + // PUT route with access control + app.put( + '/permissions/:resourceType/:resourceId', + checkResourcePermissionAccess(PermissionBits.SHARE), + (req, res) => res.json({ success: true }), + ); + }); + + describe('GET /permissions/:resourceType/:resourceId', () => { + it('should deny permission query for user without access (main vulnerability test)', async () => { + /** + * SECURITY TEST: This is the core test for SBA-ADV-20251203-02 + * + * Before the fix, any authenticated user could query permissions for + * any agent by just knowing the agent ID, exposing information about + * who has access to private agents. + * + * After the fix, users must have SHARE permission to view permissions. + */ + const response = await request(app) + .get(`/permissions/agent/${agentId}`) + .set('Content-Type', 'application/json'); + + // Should be denied - attacker has no permission on the agent + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); + }); + + it('should return 400 for unsupported resource type', async () => { + const response = await request(app) + .get(`/permissions/unsupported/${agentId}`) + .set('Content-Type', 'application/json'); + + expect(response.status).toBe(400); + expect(response.body.message).toContain('Unsupported resource type'); + }); + }); + + describe('PUT /permissions/:resourceType/:resourceId', () => { + it('should deny permission update for user without access', async () => { + const response = await request(app) + .put(`/permissions/agent/${agentId}`) + .set('Content-Type', 'application/json') + .send({ principals: [] }); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); + }); + }); +});