From c6982dc180c26d6c26e1802ee2c63b9dcf3046ee Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sat, 14 Mar 2026 02:57:56 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20fix:=20Agent=20Permissi?= =?UTF-8?q?on=20Check=20on=20Image=20Upload=20Route=20(#12219)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix: add agent permission check to image upload route * refactor: remove unused SystemRoles import and format test file for clarity * fix: address review findings for image upload agent permission check * refactor: move agent upload auth logic to TypeScript in packages/api Extract pure authorization logic from agentPermCheck.js into checkAgentUploadAuth() in packages/api/src/files/agentUploadAuth.ts. The function returns a structured result ({ allowed, status, error }) instead of writing HTTP responses directly, eliminating the dual responsibility and confusing sentinel return value. The JS wrapper in /api is now a thin adapter that translates the result to HTTP. * test: rewrite image upload permission tests as integration tests Replace mock-heavy images-agent-perm.spec.js with integration tests using MongoMemoryServer, real models, and real PermissionService. Follows the established pattern in files.agents.test.js. Moves test to sibling location (images.agents.test.js) matching backend convention. Adds temp file cleanup assertions on 403/404 responses and covers message_file exemption paths (boolean true, string "true", false). * fix: widen AgentUploadAuthDeps types to accept ObjectId from Mongoose The injected getAgent returns Mongoose documents where _id and author are Types.ObjectId at runtime, not string. Widen the DI interface to accept string | Types.ObjectId for _id, author, and resourceId so the contract accurately reflects real callers. * chore: move agent upload auth into files/agents/ subdirectory * refactor: delete agentPermCheck.js wrapper, move verifyAgentUploadPermission to packages/api The /api-only dependencies (getAgent, checkPermission) are now passed as object-field params from the route call sites. Both images.js and files.js import verifyAgentUploadPermission from @librechat/api and inject the deps directly, eliminating the intermediate JS wrapper. * style: fix import type ordering in agent upload auth * fix: prevent token TTL race in MCPTokenStorage.storeTokens When expires_in is provided, use it directly instead of round-tripping through Date arithmetic. The previous code computed accessTokenExpiry as a Date, then after an async encryptV2 call, recomputed expiresIn by subtracting Date.now(). On loaded CI runners the elapsed time caused Math.floor to truncate to 0, triggering the 1-year fallback and making the token appear permanently valid — so refresh never fired. --- api/server/routes/files/files.js | 53 +-- api/server/routes/files/images.agents.test.js | 376 ++++++++++++++++++ api/server/routes/files/images.js | 13 + packages/api/src/files/agents/auth.ts | 113 ++++++ packages/api/src/files/agents/index.ts | 1 + packages/api/src/files/index.ts | 1 + packages/api/src/mcp/oauth/tokens.ts | 28 +- 7 files changed, 525 insertions(+), 60 deletions(-) create mode 100644 api/server/routes/files/images.agents.test.js create mode 100644 packages/api/src/files/agents/auth.ts create mode 100644 packages/api/src/files/agents/index.ts diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index 5de2ddb379..9290d1a7ed 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -2,12 +2,12 @@ const fs = require('fs').promises; const express = require('express'); const { EnvVar } = require('@librechat/agents'); const { logger } = require('@librechat/data-schemas'); +const { verifyAgentUploadPermission } = require('@librechat/api'); const { Time, isUUID, CacheKeys, FileSources, - SystemRoles, ResourceType, EModelEndpoint, PermissionBits, @@ -381,48 +381,15 @@ router.post('/', async (req, res) => { return await processFileUpload({ req, res, metadata }); } - /** - * Check agent permissions for permanent agent file uploads (not message attachments). - * Message attachments (message_file=true) are temporary files for a single conversation - * and should be allowed for users who can chat with the agent. - * Permanent file uploads to tool_resources require EDIT permission. - */ - const isMessageAttachment = metadata.message_file === true || metadata.message_file === 'true'; - if (metadata.agent_id && metadata.tool_resource && !isMessageAttachment) { - const userId = req.user.id; - - /** Admin users bypass permission checks */ - if (req.user.role !== SystemRoles.ADMIN) { - const agent = await getAgent({ id: metadata.agent_id }); - - if (!agent) { - return res.status(404).json({ - error: 'Not Found', - message: 'Agent not found', - }); - } - - /** Check if user is the author or has edit permission */ - if (agent.author.toString() !== userId) { - const hasEditPermission = await checkPermission({ - userId, - role: req.user.role, - resourceType: ResourceType.AGENT, - resourceId: agent._id, - requiredPermission: PermissionBits.EDIT, - }); - - if (!hasEditPermission) { - logger.warn( - `[/files] User ${userId} denied upload to agent ${metadata.agent_id} (insufficient permissions)`, - ); - return res.status(403).json({ - error: 'Forbidden', - message: 'Insufficient permissions to upload files to this agent', - }); - } - } - } + const denied = await verifyAgentUploadPermission({ + req, + res, + metadata, + getAgent, + checkPermission, + }); + if (denied) { + return; } return await processAgentFileUpload({ req, res, metadata }); diff --git a/api/server/routes/files/images.agents.test.js b/api/server/routes/files/images.agents.test.js new file mode 100644 index 0000000000..862ab87d63 --- /dev/null +++ b/api/server/routes/files/images.agents.test.js @@ -0,0 +1,376 @@ +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 { + SystemRoles, + AccessRoleIds, + ResourceType, + PrincipalType, +} = require('librechat-data-provider'); +const { createAgent } = require('~/models/Agent'); + +jest.mock('~/server/services/Files/process', () => ({ + processAgentFileUpload: jest.fn().mockImplementation(async ({ res }) => { + return res.status(200).json({ message: 'Agent file uploaded', file_id: 'test-file-id' }); + }), + processImageFile: jest.fn().mockImplementation(async ({ res }) => { + return res.status(200).json({ message: 'Image processed' }); + }), + filterFile: jest.fn(), +})); + +jest.mock('fs', () => { + const actualFs = jest.requireActual('fs'); + return { + ...actualFs, + promises: { + ...actualFs.promises, + unlink: jest.fn().mockResolvedValue(undefined), + }, + }; +}); + +const fs = require('fs'); +const { processAgentFileUpload } = require('~/server/services/Files/process'); + +const router = require('~/server/routes/files/images'); + +describe('POST /images - Agent Upload Permission Check (Integration)', () => { + let mongoServer; + let authorId; + let otherUserId; + let agentCustomId; + let User; + let Agent; + let AclEntry; + let methods; + let modelsToCleanup = []; + + beforeAll(async () => { + mongoServer = await MongoMemoryServer.create(); + const mongoUri = mongoServer.getUri(); + await mongoose.connect(mongoUri); + + 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; + Agent = models.Agent; + AclEntry = models.AclEntry; + + await methods.seedDefaultRoles(); + }); + + afterAll(async () => { + const collections = mongoose.connection.collections; + for (const key in collections) { + await collections[key].deleteMany({}); + } + for (const modelName of modelsToCleanup) { + if (mongoose.models[modelName]) { + delete mongoose.models[modelName]; + } + } + await mongoose.disconnect(); + await mongoServer.stop(); + }); + + beforeEach(async () => { + await Agent.deleteMany({}); + await User.deleteMany({}); + await AclEntry.deleteMany({}); + + authorId = new mongoose.Types.ObjectId(); + otherUserId = new mongoose.Types.ObjectId(); + agentCustomId = `agent_${uuidv4().replace(/-/g, '').substring(0, 21)}`; + + await User.create({ _id: authorId, username: 'author', email: 'author@test.com' }); + await User.create({ _id: otherUserId, username: 'other', email: 'other@test.com' }); + + jest.clearAllMocks(); + }); + + const createAppWithUser = (userId, userRole = SystemRoles.USER) => { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + if (req.method === 'POST') { + req.file = { + originalname: 'test.png', + mimetype: 'image/png', + size: 100, + path: '/tmp/t.png', + filename: 'test.png', + }; + req.file_id = uuidv4(); + } + next(); + }); + app.use((req, _res, next) => { + req.user = { id: userId.toString(), role: userRole }; + req.app = { locals: {} }; + req.config = { fileStrategy: 'local', paths: { imageOutput: '/tmp/images' } }; + next(); + }); + app.use('/images', router); + return app; + }; + + it('should return 403 when user has no permission on agent', async () => { + await createAgent({ + id: agentCustomId, + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + }); + + const app = createAppWithUser(otherUserId); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + agent_id: agentCustomId, + tool_resource: 'context', + file_id: uuidv4(), + }); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); + expect(processAgentFileUpload).not.toHaveBeenCalled(); + expect(fs.promises.unlink).toHaveBeenCalledWith('/tmp/t.png'); + }); + + it('should allow upload for agent owner', async () => { + await createAgent({ + id: agentCustomId, + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + }); + + const app = createAppWithUser(authorId); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + agent_id: agentCustomId, + tool_resource: 'context', + file_id: uuidv4(), + }); + + expect(response.status).toBe(200); + expect(processAgentFileUpload).toHaveBeenCalled(); + }); + + it('should allow upload for admin regardless of ownership', async () => { + await createAgent({ + id: agentCustomId, + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + }); + + const app = createAppWithUser(otherUserId, SystemRoles.ADMIN); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + agent_id: agentCustomId, + tool_resource: 'context', + file_id: uuidv4(), + }); + + expect(response.status).toBe(200); + expect(processAgentFileUpload).toHaveBeenCalled(); + }); + + it('should allow upload for user with EDIT permission', async () => { + const agent = await createAgent({ + id: agentCustomId, + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + }); + + const { grantPermission } = require('~/server/services/PermissionService'); + await grantPermission({ + principalType: PrincipalType.USER, + principalId: otherUserId, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + accessRoleId: AccessRoleIds.AGENT_EDITOR, + grantedBy: authorId, + }); + + const app = createAppWithUser(otherUserId); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + agent_id: agentCustomId, + tool_resource: 'context', + file_id: uuidv4(), + }); + + expect(response.status).toBe(200); + expect(processAgentFileUpload).toHaveBeenCalled(); + }); + + it('should deny upload for user with only VIEW permission', async () => { + const agent = await createAgent({ + id: agentCustomId, + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + }); + + const { grantPermission } = require('~/server/services/PermissionService'); + await grantPermission({ + principalType: PrincipalType.USER, + principalId: otherUserId, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + accessRoleId: AccessRoleIds.AGENT_VIEWER, + grantedBy: authorId, + }); + + const app = createAppWithUser(otherUserId); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + agent_id: agentCustomId, + tool_resource: 'context', + file_id: uuidv4(), + }); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); + expect(processAgentFileUpload).not.toHaveBeenCalled(); + expect(fs.promises.unlink).toHaveBeenCalledWith('/tmp/t.png'); + }); + + it('should skip permission check for regular image uploads without agent_id/tool_resource', async () => { + const app = createAppWithUser(otherUserId); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + file_id: uuidv4(), + }); + + expect(response.status).toBe(200); + }); + + it('should return 404 for non-existent agent', async () => { + const app = createAppWithUser(otherUserId); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + agent_id: 'agent_nonexistent123456789', + tool_resource: 'context', + file_id: uuidv4(), + }); + + expect(response.status).toBe(404); + expect(response.body.error).toBe('Not Found'); + expect(processAgentFileUpload).not.toHaveBeenCalled(); + expect(fs.promises.unlink).toHaveBeenCalledWith('/tmp/t.png'); + }); + + it('should allow message_file attachment (boolean true) without EDIT permission', async () => { + const agent = await createAgent({ + id: agentCustomId, + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + }); + + const { grantPermission } = require('~/server/services/PermissionService'); + await grantPermission({ + principalType: PrincipalType.USER, + principalId: otherUserId, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + accessRoleId: AccessRoleIds.AGENT_VIEWER, + grantedBy: authorId, + }); + + const app = createAppWithUser(otherUserId); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + agent_id: agentCustomId, + tool_resource: 'context', + message_file: true, + file_id: uuidv4(), + }); + + expect(response.status).toBe(200); + expect(processAgentFileUpload).toHaveBeenCalled(); + }); + + it('should allow message_file attachment (string "true") without EDIT permission', async () => { + const agent = await createAgent({ + id: agentCustomId, + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + }); + + const { grantPermission } = require('~/server/services/PermissionService'); + await grantPermission({ + principalType: PrincipalType.USER, + principalId: otherUserId, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + accessRoleId: AccessRoleIds.AGENT_VIEWER, + grantedBy: authorId, + }); + + const app = createAppWithUser(otherUserId); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + agent_id: agentCustomId, + tool_resource: 'context', + message_file: 'true', + file_id: uuidv4(), + }); + + expect(response.status).toBe(200); + expect(processAgentFileUpload).toHaveBeenCalled(); + }); + + it('should deny upload when message_file is false (not a message attachment)', async () => { + const agent = await createAgent({ + id: agentCustomId, + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + }); + + const { grantPermission } = require('~/server/services/PermissionService'); + await grantPermission({ + principalType: PrincipalType.USER, + principalId: otherUserId, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + accessRoleId: AccessRoleIds.AGENT_VIEWER, + grantedBy: authorId, + }); + + const app = createAppWithUser(otherUserId); + const response = await request(app).post('/images').send({ + endpoint: 'agents', + agent_id: agentCustomId, + tool_resource: 'context', + message_file: false, + file_id: uuidv4(), + }); + + expect(response.status).toBe(403); + expect(response.body.error).toBe('Forbidden'); + expect(processAgentFileUpload).not.toHaveBeenCalled(); + expect(fs.promises.unlink).toHaveBeenCalledWith('/tmp/t.png'); + }); +}); diff --git a/api/server/routes/files/images.js b/api/server/routes/files/images.js index 8072612a69..185ec7a671 100644 --- a/api/server/routes/files/images.js +++ b/api/server/routes/files/images.js @@ -2,12 +2,15 @@ const path = require('path'); const fs = require('fs').promises; const express = require('express'); const { logger } = require('@librechat/data-schemas'); +const { verifyAgentUploadPermission } = require('@librechat/api'); const { isAssistantsEndpoint } = require('librechat-data-provider'); const { processAgentFileUpload, processImageFile, filterFile, } = require('~/server/services/Files/process'); +const { checkPermission } = require('~/server/services/PermissionService'); +const { getAgent } = require('~/models/Agent'); const router = express.Router(); @@ -22,6 +25,16 @@ router.post('/', async (req, res) => { metadata.file_id = req.file_id; if (!isAssistantsEndpoint(metadata.endpoint) && metadata.tool_resource != null) { + const denied = await verifyAgentUploadPermission({ + req, + res, + metadata, + getAgent, + checkPermission, + }); + if (denied) { + return; + } return await processAgentFileUpload({ req, res, metadata }); } diff --git a/packages/api/src/files/agents/auth.ts b/packages/api/src/files/agents/auth.ts new file mode 100644 index 0000000000..d9fb2b7423 --- /dev/null +++ b/packages/api/src/files/agents/auth.ts @@ -0,0 +1,113 @@ +import type { IUser } from '@librechat/data-schemas'; +import type { Response } from 'express'; +import type { Types } from 'mongoose'; +import { logger } from '@librechat/data-schemas'; +import { SystemRoles, ResourceType, PermissionBits } from 'librechat-data-provider'; +import type { ServerRequest } from '~/types'; + +export type AgentUploadAuthResult = + | { allowed: true } + | { allowed: false; status: number; error: string; message: string }; + +export interface AgentUploadAuthParams { + userId: string; + userRole: string; + agentId?: string; + toolResource?: string | null; + messageFile?: boolean | string; +} + +export interface AgentUploadAuthDeps { + getAgent: (params: { id: string }) => Promise<{ + _id: string | Types.ObjectId; + author?: string | Types.ObjectId | null; + } | null>; + checkPermission: (params: { + userId: string; + role: string; + resourceType: ResourceType; + resourceId: string | Types.ObjectId; + requiredPermission: number; + }) => Promise; +} + +export async function checkAgentUploadAuth( + params: AgentUploadAuthParams, + deps: AgentUploadAuthDeps, +): Promise { + const { userId, userRole, agentId, toolResource, messageFile } = params; + const { getAgent, checkPermission } = deps; + + const isMessageAttachment = messageFile === true || messageFile === 'true'; + if (!agentId || toolResource == null || isMessageAttachment) { + return { allowed: true }; + } + + if (userRole === SystemRoles.ADMIN) { + return { allowed: true }; + } + + const agent = await getAgent({ id: agentId }); + if (!agent) { + return { allowed: false, status: 404, error: 'Not Found', message: 'Agent not found' }; + } + + if (agent.author?.toString() === userId) { + return { allowed: true }; + } + + const hasEditPermission = await checkPermission({ + userId, + role: userRole, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + requiredPermission: PermissionBits.EDIT, + }); + + if (hasEditPermission) { + return { allowed: true }; + } + + logger.warn( + `[agentUploadAuth] User ${userId} denied upload to agent ${agentId} (insufficient permissions)`, + ); + return { + allowed: false, + status: 403, + error: 'Forbidden', + message: 'Insufficient permissions to upload files to this agent', + }; +} + +/** @returns true if denied (response already sent), false if allowed */ +export async function verifyAgentUploadPermission({ + req, + res, + metadata, + getAgent, + checkPermission, +}: { + req: ServerRequest; + res: Response; + metadata: { agent_id?: string; tool_resource?: string | null; message_file?: boolean | string }; + getAgent: AgentUploadAuthDeps['getAgent']; + checkPermission: AgentUploadAuthDeps['checkPermission']; +}): Promise { + const user = req.user as IUser; + const result = await checkAgentUploadAuth( + { + userId: user.id, + userRole: user.role ?? '', + agentId: metadata.agent_id, + toolResource: metadata.tool_resource, + messageFile: metadata.message_file, + }, + { getAgent, checkPermission }, + ); + + if (!result.allowed) { + res.status(result.status).json({ error: result.error, message: result.message }); + return true; + } + return false; +} diff --git a/packages/api/src/files/agents/index.ts b/packages/api/src/files/agents/index.ts new file mode 100644 index 0000000000..269586ee8b --- /dev/null +++ b/packages/api/src/files/agents/index.ts @@ -0,0 +1 @@ +export * from './auth'; diff --git a/packages/api/src/files/index.ts b/packages/api/src/files/index.ts index 707f2ef7fb..c3bdb49478 100644 --- a/packages/api/src/files/index.ts +++ b/packages/api/src/files/index.ts @@ -1,3 +1,4 @@ +export * from './agents'; export * from './audio'; export * from './context'; export * from './documents/crud'; diff --git a/packages/api/src/mcp/oauth/tokens.ts b/packages/api/src/mcp/oauth/tokens.ts index 7b1d189347..6094a05386 100644 --- a/packages/api/src/mcp/oauth/tokens.ts +++ b/packages/api/src/mcp/oauth/tokens.ts @@ -83,46 +83,40 @@ export class MCPTokenStorage { `${logPrefix} Token expires_in: ${'expires_in' in tokens ? tokens.expires_in : 'N/A'}, expires_at: ${'expires_at' in tokens ? tokens.expires_at : 'N/A'}`, ); - // Handle both expires_in and expires_at formats + const defaultTTL = 365 * 24 * 60 * 60; + let accessTokenExpiry: Date; + let expiresInSeconds: number; if ('expires_at' in tokens && tokens.expires_at) { /** MCPOAuthTokens format - already has calculated expiry */ logger.debug(`${logPrefix} Using expires_at: ${tokens.expires_at}`); accessTokenExpiry = new Date(tokens.expires_at); + expiresInSeconds = Math.floor((accessTokenExpiry.getTime() - Date.now()) / 1000); } else if (tokens.expires_in) { - /** Standard OAuthTokens format - calculate expiry */ + /** Standard OAuthTokens format - use expires_in directly to avoid lossy Date round-trip */ logger.debug(`${logPrefix} Using expires_in: ${tokens.expires_in}`); + expiresInSeconds = tokens.expires_in; accessTokenExpiry = new Date(Date.now() + tokens.expires_in * 1000); } else { - /** No expiry provided - default to 1 year */ logger.debug(`${logPrefix} No expiry provided, using default`); - accessTokenExpiry = new Date(Date.now() + 365 * 24 * 60 * 60 * 1000); + expiresInSeconds = defaultTTL; + accessTokenExpiry = new Date(Date.now() + defaultTTL * 1000); } logger.debug(`${logPrefix} Calculated expiry date: ${accessTokenExpiry.toISOString()}`); - logger.debug( - `${logPrefix} Date object: ${JSON.stringify({ - time: accessTokenExpiry.getTime(), - valid: !isNaN(accessTokenExpiry.getTime()), - iso: accessTokenExpiry.toISOString(), - })}`, - ); - // Ensure the date is valid before passing to createToken if (isNaN(accessTokenExpiry.getTime())) { logger.error(`${logPrefix} Invalid expiry date calculated, using default`); - accessTokenExpiry = new Date(Date.now() + 365 * 24 * 60 * 60 * 1000); + accessTokenExpiry = new Date(Date.now() + defaultTTL * 1000); + expiresInSeconds = defaultTTL; } - // Calculate expiresIn (seconds from now) - const expiresIn = Math.floor((accessTokenExpiry.getTime() - Date.now()) / 1000); - const accessTokenData = { userId, type: 'mcp_oauth', identifier, token: encryptedAccessToken, - expiresIn: expiresIn > 0 ? expiresIn : 365 * 24 * 60 * 60, // Default to 1 year if negative + expiresIn: expiresInSeconds > 0 ? expiresInSeconds : defaultTTL, }; // Check if token already exists and update if it does