diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index 4f04a00be7..012eab36c0 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -220,6 +220,49 @@ const filterAuthorizedTools = async ({ return filteredTools; }; +/** + * Removes file IDs from tool resources unless the referenced file is owned by + * the agent owner. + * @param {object} params + * @param {object} params.tool_resources + * @param {string | object} params.ownerId + * @param {string} params.logPrefix + * @returns {Promise} Count of removed file references. + */ +const pruneToolResourceFileIdsForOwner = async ({ tool_resources, ownerId, logPrefix }) => { + const referencedFileIds = collectToolResourceFileIds(tool_resources); + if (referencedFileIds.length === 0) { + return 0; + } + if (!ownerId) { + return stripFileIdsFromToolResources(tool_resources, referencedFileIds).removedCount; + } + const ownerIdStr = ownerId.toString(); + + try { + const ownerFiles = await db.getFiles({ file_id: { $in: referencedFileIds } }, null, { + file_id: 1, + user: 1, + }); + const allowedIds = new Set( + (ownerFiles ?? []) + .filter((file) => file.user && file.user.toString() === ownerIdStr) + .map((file) => file.file_id), + ); + const disallowedIds = referencedFileIds.filter((id) => !allowedIds.has(id)); + if (disallowedIds.length > 0) { + logger.warn(`${logPrefix} Pruning ${disallowedIds.length} invalid file reference(s)`); + return stripFileIdsFromToolResources(tool_resources, disallowedIds).removedCount; + } + return 0; + } catch (fileCheckError) { + logger.warn(`${logPrefix} File ownership check failed, pruning incoming file references`, { + error: fileCheckError?.message, + }); + return stripFileIdsFromToolResources(tool_resources, referencedFileIds).removedCount; + } +}; + /** * Creates an Agent. * @route POST /Agents @@ -239,6 +282,14 @@ const createAgentHandler = async (req, res) => { const { id: userId, role: userRole } = req.user; + if (agentData.tool_resources) { + await pruneToolResourceFileIdsForOwner({ + tool_resources: agentData.tool_resources, + ownerId: userId, + logPrefix: '[/Agents]', + }); + } + if (agentData.edges?.length) { const unauthorized = await validateEdgeAgentAccess(agentData.edges, userId, userRole); if (unauthorized.length > 0) { @@ -509,36 +560,12 @@ const updateAgentHandler = async (req, res) => { updateData.tools = ocrConversion.tools; } - /* - * Strip orphaned file_id stubs from the incoming payload (see issue #12776). - * Scoped to updates that actually touch tool_resources: if the save does not - * modify that field, the delete-time cleanup in processDeleteRequest and the - * one-off migration already cover pre-existing corruption, so there's no - * reason to pay an extra DB round-trip here. Wrapped in try/catch so a - * transient failure in this integrity check never turns a good save into 500. - */ if (updateData.tool_resources) { - try { - const referencedFileIds = collectToolResourceFileIds(updateData.tool_resources); - if (referencedFileIds.length > 0) { - const existingFiles = await db.getFiles({ file_id: { $in: referencedFileIds } }, null, { - file_id: 1, - }); - const existingIds = new Set((existingFiles ?? []).map((f) => f.file_id)); - const orphans = referencedFileIds.filter((id) => !existingIds.has(id)); - if (orphans.length > 0) { - logger.warn( - `[/Agents/:id] Pruning ${orphans.length} orphaned file reference(s) from agent ${id}`, - ); - stripFileIdsFromToolResources(updateData.tool_resources, orphans); - } - } - } catch (orphanCheckError) { - logger.warn( - '[/Agents/:id] Orphan file check failed, skipping cleanup for this request', - orphanCheckError, - ); - } + await pruneToolResourceFileIdsForOwner({ + tool_resources: updateData.tool_resources, + ownerId: existingAgent.author, + logPrefix: `[/Agents/:id] Agent ${id}`, + }); } if (updateData.tools) { @@ -722,6 +749,14 @@ const duplicateAgentHandler = async (req, res) => { }); } + if (newAgentData.tool_resources) { + await pruneToolResourceFileIdsForOwner({ + tool_resources: newAgentData.tool_resources, + ownerId: userId, + logPrefix: '[/Agents/:id/duplicate]', + }); + } + const newAgent = await db.createAgent(newAgentData); try { @@ -1056,6 +1091,7 @@ const revertAgentVersionHandler = async (req, res) => { // Permissions are enforced via route middleware (ACL EDIT) let updatedAgent = await db.revertAgentVersion({ id }, version_index); + const revertUpdates = {}; if (updatedAgent.tools?.length) { const [availableTools, configServers] = await Promise.all([ @@ -1070,14 +1106,25 @@ const revertAgentVersionHandler = async (req, res) => { configServers, }); if (filteredTools.length !== updatedAgent.tools.length) { - updatedAgent = await db.updateAgent( - { id }, - { tools: filteredTools }, - { updatingUserId: req.user.id }, - ); + revertUpdates.tools = filteredTools; } } + if (updatedAgent.tool_resources) { + const removedCount = await pruneToolResourceFileIdsForOwner({ + tool_resources: updatedAgent.tool_resources, + ownerId: existingAgent.author, + logPrefix: '[/Agents/:id/revert]', + }); + if (removedCount > 0) { + revertUpdates.tool_resources = updatedAgent.tool_resources; + } + } + + if (Object.keys(revertUpdates).length > 0) { + updatedAgent = await db.updateAgent({ id }, revertUpdates, { updatingUserId: req.user.id }); + } + if (updatedAgent.author) { updatedAgent.author = updatedAgent.author.toString(); } diff --git a/api/server/controllers/agents/v1.spec.js b/api/server/controllers/agents/v1.spec.js index d191822add..81f72264bd 100644 --- a/api/server/controllers/agents/v1.spec.js +++ b/api/server/controllers/agents/v1.spec.js @@ -72,6 +72,8 @@ jest.mock('~/cache', () => ({ const { createAgent: createAgentHandler, + duplicateAgent: duplicateAgentHandler, + revertAgentVersion: revertAgentVersionHandler, updateAgent: updateAgentHandler, getListAgents: getListAgentsHandler, } = require('./v1'); @@ -111,6 +113,7 @@ describe('Agent Controllers - Mass Assignment Protection', () => { beforeEach(async () => { await Agent.deleteMany({}); + await mongoose.models.File.deleteMany({}); // Reset all mocks jest.clearAllMocks(); @@ -278,6 +281,48 @@ describe('Agent Controllers - Mass Assignment Protection', () => { expect(agentInDb.tool_resources.invalid_resource).toBeUndefined(); }); + test('should strip file_ids not owned by the creator from tool_resources', async () => { + const File = mongoose.models.File; + + const ownedFileId = `file_${uuidv4()}`; + const otherFileId = `file_${uuidv4()}`; + await File.create({ + file_id: ownedFileId, + user: mockReq.user.id, + filename: `${ownedFileId}.txt`, + filepath: `/tmp/${ownedFileId}`, + object: 'file', + type: 'text/plain', + bytes: 1, + source: FileSources.local, + }); + await File.create({ + file_id: otherFileId, + user: new mongoose.Types.ObjectId(), + filename: `${otherFileId}.txt`, + filepath: `/tmp/${otherFileId}`, + object: 'file', + type: 'text/plain', + bytes: 1, + source: FileSources.local, + }); + + mockReq.body = { + provider: 'openai', + model: 'gpt-4', + name: 'Agent with Files', + tool_resources: { + file_search: { file_ids: [ownedFileId, otherFileId] }, + }, + }; + + await createAgentHandler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(201); + const createdAgent = mockRes.json.mock.calls[0][0]; + expect(createdAgent.tool_resources.file_search.file_ids).toEqual([ownedFileId]); + }); + test('should handle support_contact with empty strings', async () => { const dataWithEmptyContact = { provider: 'openai', @@ -544,6 +589,48 @@ describe('Agent Controllers - Mass Assignment Protection', () => { expect(updatedAgent.name).toBe('Admin Update'); }); + test('should prune admin-supplied file_ids against the agent author', async () => { + const File = mongoose.models.File; + const adminUserId = new mongoose.Types.ObjectId().toString(); + const authorFileId = `file_${uuidv4()}`; + const adminFileId = `file_${uuidv4()}`; + + await File.create({ + file_id: authorFileId, + user: existingAgentAuthorId, + filename: `${authorFileId}.txt`, + filepath: `/tmp/${authorFileId}`, + object: 'file', + type: 'text/plain', + bytes: 1, + source: FileSources.local, + }); + await File.create({ + file_id: adminFileId, + user: adminUserId, + filename: `${adminFileId}.txt`, + filepath: `/tmp/${adminFileId}`, + object: 'file', + type: 'text/plain', + bytes: 1, + source: FileSources.local, + }); + + mockReq.user.id = adminUserId; + mockReq.user.role = 'ADMIN'; + mockReq.params.id = existingAgentId; + mockReq.body = { + tool_resources: { + file_search: { file_ids: [authorFileId, adminFileId] }, + }, + }; + + await updateAgentHandler(mockReq, mockRes); + + const agentInDb = await Agent.findOne({ id: existingAgentId }).lean(); + expect(agentInDb.tool_resources.file_search.file_ids).toEqual([authorFileId]); + }); + test('should validate tool_resources in updates', async () => { // Back these ids with real File docs so the orphan-pruning added for // issue #12776 does not strip them — this test is about OCR conversion @@ -808,10 +895,9 @@ describe('Agent Controllers - Mass Assignment Protection', () => { expect(agentInDb.tool_resources.file_search.file_ids).toEqual([orphan]); }); - test('swallows errors from the file-existence check and still completes the save', async () => { + test('prunes incoming file_ids when the file ownership check fails', async () => { const db = require('~/models'); - const originalGetFiles = db.getFiles; - db.getFiles = jest.fn().mockRejectedValue(new Error('transient DB error')); + jest.spyOn(db, 'getFiles').mockRejectedValueOnce(new Error('transient DB error')); const orphan = `file_${uuidv4()}`; mockReq.user.id = existingAgentAuthorId.toString(); @@ -821,20 +907,119 @@ describe('Agent Controllers - Mass Assignment Protection', () => { tool_resources: { file_search: { file_ids: [orphan] } }, }; - try { - await updateAgentHandler(mockReq, mockRes); + await updateAgentHandler(mockReq, mockRes); - expect(mockRes.status).not.toHaveBeenCalledWith(500); - expect(mockRes.json).toHaveBeenCalled(); - const agentInDb = await Agent.findOne({ id: existingAgentId }).lean(); - expect(agentInDb.name).toBe('Save Succeeds'); - // Cleanup skipped on error, so the id remains — the delete-time path - // or the next successful save will reconcile it. - expect(agentInDb.tool_resources.file_search.file_ids).toEqual([orphan]); - } finally { - db.getFiles = originalGetFiles; - } + expect(mockRes.status).not.toHaveBeenCalledWith(500); + expect(mockRes.json).toHaveBeenCalled(); + const agentInDb = await Agent.findOne({ id: existingAgentId }).lean(); + expect(agentInDb.name).toBe('Save Succeeds'); + expect(agentInDb.tool_resources.file_search.file_ids).toEqual([]); }); + + test('strips file_ids owned by another user from incoming tool_resources', async () => { + const keeper = `file_${uuidv4()}`; + const otherUsersFile = `file_${uuidv4()}`; + await createFileDoc(keeper, existingAgentAuthorId); + await createFileDoc(otherUsersFile, new mongoose.Types.ObjectId()); + + mockReq.user.id = existingAgentAuthorId.toString(); + mockReq.params.id = existingAgentId; + mockReq.body = { + tool_resources: { + file_search: { file_ids: [keeper, otherUsersFile] }, + }, + }; + + await updateAgentHandler(mockReq, mockRes); + + const agentInDb = await Agent.findOne({ id: existingAgentId }).lean(); + expect(agentInDb.tool_resources.file_search.file_ids).toEqual([keeper]); + }); + }); + }); + + describe('tool_resources ownership pruning in alternate write paths', () => { + const createFileDoc = (file_id, userId) => + mongoose.models.File.create({ + file_id, + user: userId, + filename: `${file_id}.txt`, + filepath: `/tmp/${file_id}`, + object: 'file', + type: 'text/plain', + bytes: 1, + source: FileSources.local, + }); + + test('duplicateAgentHandler should prune file_ids not owned by the clone author', async () => { + const sourceAuthorId = new mongoose.Types.ObjectId(); + const cloneAuthorId = new mongoose.Types.ObjectId(); + const sourceFileId = `file_${uuidv4()}`; + const cloneAuthorFileId = `file_${uuidv4()}`; + + await createFileDoc(sourceFileId, sourceAuthorId); + await createFileDoc(cloneAuthorFileId, cloneAuthorId); + const sourceAgent = await Agent.create({ + id: `agent_${uuidv4()}`, + name: 'Source Agent', + provider: 'openai', + model: 'gpt-4', + author: sourceAuthorId, + tool_resources: { + context: { file_ids: [sourceFileId, cloneAuthorFileId] }, + }, + }); + + const db = require('~/models'); + jest.spyOn(db, 'getActions').mockResolvedValueOnce([]); + + mockReq.user.id = cloneAuthorId.toString(); + mockReq.params.id = sourceAgent.id; + + await duplicateAgentHandler(mockReq, mockRes); + + expect(mockRes.status).toHaveBeenCalledWith(201); + const { agent } = mockRes.json.mock.calls[0][0]; + expect(agent.author.toString()).toBe(cloneAuthorId.toString()); + expect(agent.tool_resources.context.file_ids).toEqual([cloneAuthorFileId]); + }); + + test('revertAgentVersionHandler should prune restored file_ids not owned by the agent author', async () => { + const agentAuthorId = new mongoose.Types.ObjectId(); + const otherUserId = new mongoose.Types.ObjectId(); + const ownedFileId = `file_${uuidv4()}`; + const otherFileId = `file_${uuidv4()}`; + + await createFileDoc(ownedFileId, agentAuthorId); + await createFileDoc(otherFileId, otherUserId); + const agent = await Agent.create({ + id: `agent_${uuidv4()}`, + name: 'Current Agent', + provider: 'openai', + model: 'gpt-4', + author: agentAuthorId, + tool_resources: {}, + versions: [ + { + name: 'Historical Agent', + provider: 'openai', + model: 'gpt-4', + tool_resources: { + file_search: { file_ids: [ownedFileId, otherFileId] }, + }, + }, + ], + }); + + mockReq.user.id = agentAuthorId.toString(); + mockReq.params.id = agent.id; + mockReq.body = { version_index: 0 }; + + await revertAgentVersionHandler(mockReq, mockRes); + + expect(mockRes.json).toHaveBeenCalled(); + const agentInDb = await Agent.findOne({ id: agent.id }).lean(); + expect(agentInDb.tool_resources.file_search.file_ids).toEqual([ownedFileId]); }); }); diff --git a/api/server/middleware/accessResources/fileAccess.js b/api/server/middleware/accessResources/fileAccess.js index 230148fb53..e1c5803c64 100644 --- a/api/server/middleware/accessResources/fileAccess.js +++ b/api/server/middleware/accessResources/fileAccess.js @@ -5,15 +5,16 @@ const { getAgents, getFiles } = require('~/models'); /** * Checks if user has access to a file through agent permissions - * Files inherit permissions from agents - if you can view the agent, you can access its files + * Files inherit permissions from agents authored by the file owner */ -const checkAgentBasedFileAccess = async ({ userId, role, fileId }) => { +const checkAgentBasedFileAccess = async ({ userId, role, fileId, fileOwner }) => { try { /** Agents that have this file in their tool_resources */ const agentsWithFile = await getAgents({ $or: [ { 'tool_resources.execute_code.file_ids': fileId }, { 'tool_resources.file_search.file_ids': fileId }, + { 'tool_resources.image_edit.file_ids': fileId }, { 'tool_resources.context.file_ids': fileId }, { 'tool_resources.ocr.file_ids': fileId }, ], @@ -23,15 +24,19 @@ const checkAgentBasedFileAccess = async ({ userId, role, fileId }) => { return false; } - // Check if user has access to any of these agents + const fileOwnerId = fileOwner?.toString(); + const userIdStr = userId.toString(); for (const agent of agentsWithFile) { - // Check if user is the agent author - if (agent.author && agent.author.toString() === userId) { + const agentAuthorId = agent.author?.toString(); + if (!agentAuthorId || !fileOwnerId || agentAuthorId !== fileOwnerId) { + continue; + } + + if (agentAuthorId === userIdStr) { logger.debug(`[fileAccess] User is author of agent ${agent.id}`); return true; } - // Check ACL permissions for VIEW access on the agent try { const permissions = await getEffectivePermissions({ userId, @@ -49,7 +54,6 @@ const checkAgentBasedFileAccess = async ({ userId, role, fileId }) => { `[fileAccess] Permission check failed for agent ${agent.id}:`, permissionError.message, ); - // Continue checking other agents } } @@ -70,7 +74,7 @@ const denyFileAccess = (res) => /** * Middleware to check if user can access a file - * Checks: 1) File ownership, 2) Agent-based access (file inherits agent permissions) + * Checks: 1) File ownership, 2) Agent-based access through a file-owner agent */ const fileAccess = async (req, res, next) => { try { @@ -114,7 +118,12 @@ const fileAccess = async (req, res, next) => { } /** Agent-based access (file inherits agent permissions) */ - const hasAgentAccess = await checkAgentBasedFileAccess({ userId, role: userRole, fileId }); + const hasAgentAccess = await checkAgentBasedFileAccess({ + userId, + role: userRole, + fileId, + fileOwner: file.user, + }); if (hasAgentAccess) { req.fileAccess = { file }; return next(); diff --git a/api/server/middleware/accessResources/fileAccess.spec.js b/api/server/middleware/accessResources/fileAccess.spec.js index 90b7d8dca7..8fc13e585c 100644 --- a/api/server/middleware/accessResources/fileAccess.spec.js +++ b/api/server/middleware/accessResources/fileAccess.spec.js @@ -212,8 +212,7 @@ describe('fileAccess middleware', () => { }); }); - test('should allow access when user is author of agent with file', async () => { - // Create agent owned by testUser with the file + test('should deny access when user authored an agent with another user file id', async () => { await createAgent({ id: `agent_${Date.now()}`, name: 'Test Agent', @@ -230,9 +229,8 @@ describe('fileAccess middleware', () => { req.params.file_id = 'shared_file_via_agent'; await fileAccess(req, res, next); - expect(next).toHaveBeenCalled(); - expect(req.fileAccess).toBeDefined(); - expect(req.fileAccess.file).toBeDefined(); + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); }); test('should allow access when user has VIEW permission on agent with file', async () => { @@ -313,12 +311,12 @@ describe('fileAccess middleware', () => { }); test('should check file in ocr tool_resources', async () => { - await createAgent({ + const agent = await createAgent({ id: `agent_ocr_${Date.now()}`, name: 'OCR Agent', provider: 'openai', model: 'gpt-4', - author: testUser._id, + author: otherUser._id, tool_resources: { ocr: { file_ids: ['shared_file_via_agent'], @@ -326,6 +324,47 @@ describe('fileAccess middleware', () => { }, }); + await AclEntry.create({ + principalType: PrincipalType.USER, + principalId: testUser._id, + principalModel: PrincipalModel.USER, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + permBits: 1, + grantedBy: otherUser._id, + }); + + req.params.file_id = 'shared_file_via_agent'; + await fileAccess(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.fileAccess).toBeDefined(); + }); + + test('should check file in image_edit tool_resources', async () => { + const agent = await createAgent({ + id: `agent_image_${Date.now()}`, + name: 'Image Edit Agent', + provider: 'openai', + model: 'gpt-4', + author: otherUser._id, + tool_resources: { + image_edit: { + file_ids: ['shared_file_via_agent'], + }, + }, + }); + + await AclEntry.create({ + principalType: PrincipalType.USER, + principalId: testUser._id, + principalModel: PrincipalModel.USER, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + permBits: 1, + grantedBy: otherUser._id, + }); + req.params.file_id = 'shared_file_via_agent'; await fileAccess(req, res, next); @@ -377,7 +416,7 @@ describe('fileAccess middleware', () => { test('should check ALL agents with file, not just first one', async () => { // Create a file owned by someone else await createFile({ - user: otherUser._id.toString(), + user: thirdUser._id.toString(), file_id: 'multi_agent_file', filepath: '/test/multi.txt', filename: 'multi.txt', @@ -410,7 +449,7 @@ describe('fileAccess middleware', () => { grantedBy: otherUser._id, }); - // Create second agent (owned by thirdUser, but testUser has VIEW access) + // Create second agent (owned by the file owner, and testUser has VIEW access) const agent2 = await createAgent({ id: 'agent_with_access', name: 'Accessible Agent', @@ -439,9 +478,8 @@ describe('fileAccess middleware', () => { await fileAccess(req, res, next); /** - * Should succeed because testUser has access to agent2, - * even though they don't have access to agent1. - * The fix ensures all agents are checked, not just the first one. + * Should succeed because testUser has access to the file owner's agent, + * even though a non-owner agent without access is found first. */ expect(next).toHaveBeenCalled(); expect(req.fileAccess).toBeDefined(); @@ -474,12 +512,12 @@ describe('fileAccess middleware', () => { }); // Agent 2: same file in execute_code (testUser has access) - await createAgent({ + const agent2 = await createAgent({ id: 'agent_execute_code', name: 'Execute Code Agent', provider: 'openai', model: 'gpt-4', - author: thirdUser._id, + author: otherUser._id, tool_resources: { execute_code: { file_ids: ['multi_tool_file'], @@ -487,13 +525,23 @@ describe('fileAccess middleware', () => { }, }); - // Agent 3: same file in ocr (testUser also has access) + await AclEntry.create({ + principalType: PrincipalType.USER, + principalId: testUser._id, + principalModel: PrincipalModel.USER, + resourceType: ResourceType.AGENT, + resourceId: agent2._id, + permBits: 1, + grantedBy: otherUser._id, + }); + + // Agent 3: same file in ocr (bad reference from a non-owner agent) await createAgent({ id: 'agent_ocr', name: 'OCR Agent', provider: 'openai', model: 'gpt-4', - author: testUser._id, // testUser owns this one + author: testUser._id, tool_resources: { ocr: { file_ids: ['multi_tool_file'], @@ -505,7 +553,7 @@ describe('fileAccess middleware', () => { await fileAccess(req, res, next); /** - * Should succeed because testUser owns agent3, + * Should succeed through the file owner's execute_code agent, * even if other agents with the file are found first. */ expect(next).toHaveBeenCalled(); @@ -567,5 +615,35 @@ describe('fileAccess middleware', () => { expect(next).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(403); }); + + test('should deny agent-based access when file has no owner', async () => { + await mongoose.models.File.collection.insertOne({ + file_id: 'ownerless_file', + filepath: '/test/ownerless.txt', + filename: 'ownerless.txt', + type: 'text/plain', + bytes: 100, + object: 'file', + }); + + await createAgent({ + id: `agent_ownerless_${Date.now()}`, + name: 'Ownerless File Agent', + provider: 'openai', + model: 'gpt-4', + author: testUser._id, + tool_resources: { + file_search: { + file_ids: ['ownerless_file'], + }, + }, + }); + + req.params.file_id = 'ownerless_file'; + await fileAccess(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); }); }); diff --git a/api/server/routes/files/files.agents.test.js b/api/server/routes/files/files.agents.test.js index cb0e4ff3d2..d2c76ea139 100644 --- a/api/server/routes/files/files.agents.test.js +++ b/api/server/routes/files/files.agents.test.js @@ -333,7 +333,7 @@ describe('File Routes - Agent Files Endpoint', () => { expect(response.body).toHaveLength(2); }); - it('should return files uploaded by other users to shared agent for author', async () => { + it('should not return files owned by other users through agent file references', async () => { const anotherUserId = new mongoose.Types.ObjectId(); const otherUserFileId = uuidv4(); @@ -380,9 +380,9 @@ describe('File Routes - Agent Files Endpoint', () => { expect(response.status).toBe(200); expect(Array.isArray(response.body)).toBe(true); - expect(response.body).toHaveLength(2); + expect(response.body).toHaveLength(1); expect(response.body.map((f) => f.file_id)).toContain(fileId1); - expect(response.body.map((f) => f.file_id)).toContain(otherUserFileId); + expect(response.body.map((f) => f.file_id)).not.toContain(otherUserFileId); }); }); diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index 8183bca973..7ac1d55e53 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -106,7 +106,9 @@ router.get('/agent/:agent_id', async (req, res) => { return res.status(200).json([]); } - const files = await db.getFiles({ file_id: { $in: agentFileIds } }, null, { text: 0 }); + const files = await db.getFiles({ file_id: { $in: agentFileIds }, user: agent.author }, null, { + text: 0, + }); res.status(200).json(files); } catch (error) { @@ -187,6 +189,7 @@ router.delete('/', async (req, res) => { fileIds: nonOwnedFileIds, agentId: req.body.agent_id, isDelete: true, + files: nonOwnedFiles, }); for (const file of nonOwnedFiles) { diff --git a/api/server/routes/files/files.test.js b/api/server/routes/files/files.test.js index ac1167c09e..b7c4772890 100644 --- a/api/server/routes/files/files.test.js +++ b/api/server/routes/files/files.test.js @@ -270,6 +270,59 @@ describe('File Routes - Delete with Agent Access', () => { expect(processDeleteRequest).toHaveBeenCalled(); }); + it('should prevent deleting files not owned by the agent author', async () => { + const thirdUserId = new mongoose.Types.ObjectId(); + const thirdUserFileId = uuidv4(); + await createFile({ + user: thirdUserId, + file_id: thirdUserFileId, + filename: 'third-user-file.txt', + filepath: '/uploads/third-user-file.txt', + bytes: 300, + type: 'text/plain', + }); + + const agent = await createAgent({ + id: uuidv4(), + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + tool_resources: { + file_search: { + file_ids: [thirdUserFileId], + }, + }, + }); + + 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 response = await request(app) + .delete('/files') + .send({ + agent_id: agent.id, + files: [ + { + file_id: thirdUserFileId, + filepath: '/uploads/third-user-file.txt', + }, + ], + }); + + expect(response.status).toBe(403); + expect(response.body.message).toBe('You can only delete files you have access to'); + expect(response.body.unauthorizedFiles).toContain(thirdUserFileId); + expect(processDeleteRequest).not.toHaveBeenCalled(); + }); + it('should prevent deleting files not attached to the specified agent', async () => { // Create another file not attached to the agent const unattachedFileId = uuidv4(); diff --git a/api/server/routes/files/preview.spec.js b/api/server/routes/files/preview.spec.js index 84790c1e5f..426b0697c6 100644 --- a/api/server/routes/files/preview.spec.js +++ b/api/server/routes/files/preview.spec.js @@ -24,11 +24,12 @@ jest.mock('@librechat/api', () => ({ const mockFindFileById = jest.fn(); const mockGetFiles = jest.fn(); const mockUpdateFile = jest.fn(); +const mockGetAgents = jest.fn().mockResolvedValue([]); jest.mock('~/models', () => ({ findFileById: (...args) => mockFindFileById(...args), getFiles: (...args) => mockGetFiles(...args), updateFile: (...args) => mockUpdateFile(...args), - getAgents: jest.fn().mockResolvedValue([]), + getAgents: (...args) => mockGetAgents(...args), batchUpdateFiles: jest.fn(), })); @@ -96,6 +97,8 @@ describe('GET /files/:file_id/preview', () => { mockFindFileById.mockReset(); mockGetFiles.mockReset(); mockUpdateFile.mockReset(); + mockGetAgents.mockReset(); + mockGetAgents.mockResolvedValue([]); }); it('returns 404 when the file does not exist (auth check fails first via fileAccess)', async () => { @@ -121,6 +124,26 @@ describe('GET /files/:file_id/preview', () => { expect(mockFindFileById).not.toHaveBeenCalled(); }); + it('does not disclose preview text through an attacker-authored agent file reference', async () => { + mockGetFiles.mockResolvedValueOnce([ + { file_id: 'victim-file', user: 'victim-user', filename: 'secret.xlsx', status: 'ready' }, + ]); + mockGetAgents.mockResolvedValueOnce([ + { + id: 'agent-attacker', + author: 'attacker-user', + tool_resources: { execute_code: { file_ids: ['victim-file'] } }, + }, + ]); + + const res = await request(buildApp({ user: { id: 'attacker-user', role: 'user' } })).get( + '/files/victim-file/preview', + ); + + expect(res.status).toBe(403); + expect(mockFindFileById).not.toHaveBeenCalled(); + }); + it('returns status:pending without text/textFormat while the deferred render is in flight', async () => { mockGetFiles.mockResolvedValueOnce([ { diff --git a/api/server/services/Files/permissions.js b/api/server/services/Files/permissions.js index ffa8e74799..f3e2f7ce9c 100644 --- a/api/server/services/Files/permissions.js +++ b/api/server/services/Files/permissions.js @@ -1,7 +1,7 @@ const { logger } = require('@librechat/data-schemas'); const { PermissionBits, ResourceType, isEphemeralAgentId } = require('librechat-data-provider'); const { checkPermission } = require('~/server/services/PermissionService'); -const { getAgent } = require('~/models'); +const { getAgent, getFiles } = require('~/models'); /** * @param {Object} agent - The agent document (lean) @@ -21,18 +21,29 @@ function getAttachedFileIds(agent) { return attachedFileIds; } +function getFilesById(files) { + const filesById = new Map(); + for (const file of files ?? []) { + if (file?.file_id) { + filesById.set(file.file_id, file); + } + } + return filesById; +} + /** * Checks if a user has access to multiple files through a shared agent (batch operation). - * Access is always scoped to files actually attached to the agent's tool_resources. + * Access is scoped to files attached to the agent and owned by the agent author. * @param {Object} params - Parameters object * @param {string} params.userId - The user ID to check access for * @param {string} [params.role] - Optional user role to avoid DB query * @param {string[]} params.fileIds - Array of file IDs to check * @param {string} params.agentId - The agent ID that might grant access * @param {boolean} [params.isDelete] - Whether the operation is a delete operation + * @param {Array<{ file_id: string, user: string }>} [params.files] - Pre-fetched file documents * @returns {Promise>} Map of fileId to access status */ -const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId, isDelete }) => { +const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId, isDelete, files }) => { const accessMap = new Map(); fileIds.forEach((fileId) => accessMap.set(fileId, false)); @@ -45,10 +56,19 @@ const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId, isDele } const attachedFileIds = getAttachedFileIds(agent); + const agentAuthorId = agent.author?.toString(); + const filesById = + files != null + ? getFilesById(files) + : getFilesById( + await getFiles({ file_id: { $in: fileIds } }, null, { file_id: 1, user: 1 }), + ); + const canInheritFromAgent = (fileId) => + attachedFileIds.has(fileId) && filesById.get(fileId)?.user?.toString() === agentAuthorId; - if (agent.author.toString() === userId.toString()) { + if (agentAuthorId === userId.toString()) { fileIds.forEach((fileId) => { - if (attachedFileIds.has(fileId)) { + if (canInheritFromAgent(fileId)) { accessMap.set(fileId, true); } }); @@ -82,7 +102,7 @@ const hasAccessToFilesViaAgent = async ({ userId, role, fileIds, agentId, isDele } fileIds.forEach((fileId) => { - if (attachedFileIds.has(fileId)) { + if (canInheritFromAgent(fileId)) { accessMap.set(fileId, true); } }); @@ -126,7 +146,13 @@ const filterFilesByAgentAccess = async ({ files, userId, role, agentId }) => { // Batch check access for all non-owned files const fileIds = filesToCheck.map((f) => f.file_id); - const accessMap = await hasAccessToFilesViaAgent({ userId, role, fileIds, agentId }); + const accessMap = await hasAccessToFilesViaAgent({ + userId, + role, + fileIds, + agentId, + files: filesToCheck, + }); // Filter files based on access const accessibleFiles = filesToCheck.filter((file) => accessMap.get(file.file_id)); diff --git a/api/server/services/Files/permissions.spec.js b/api/server/services/Files/permissions.spec.js index c926e83464..83a0e4835d 100644 --- a/api/server/services/Files/permissions.spec.js +++ b/api/server/services/Files/permissions.spec.js @@ -8,12 +8,13 @@ jest.mock('~/server/services/PermissionService', () => ({ jest.mock('~/models', () => ({ getAgent: jest.fn(), + getFiles: jest.fn(), })); const { logger } = require('@librechat/data-schemas'); const { Constants, PermissionBits, ResourceType } = require('librechat-data-provider'); const { checkPermission } = require('~/server/services/PermissionService'); -const { getAgent } = require('~/models'); +const { getAgent, getFiles } = require('~/models'); const { filterFilesByAgentAccess, hasAccessToFilesViaAgent } = require('./permissions'); const AUTHOR_ID = 'author-user-id'; @@ -40,6 +41,12 @@ function makeAgent(overrides = {}) { beforeEach(() => { jest.clearAllMocks(); + getFiles.mockResolvedValue([ + makeFile('attached-1', AUTHOR_ID), + makeFile('attached-2', AUTHOR_ID), + makeFile('attached-3', AUTHOR_ID), + makeFile('not-attached', AUTHOR_ID), + ]); }); describe('filterFilesByAgentAccess', () => { @@ -151,6 +158,20 @@ describe('filterFilesByAgentAccess', () => { expect(result.map((f) => f.file_id)).not.toContain('not-attached'); }); + it('should not return a file referenced from an agent that is not authored by the file owner', async () => { + getAgent.mockResolvedValue(makeAgent({ author: USER_ID })); + checkPermission.mockResolvedValue(true); + + const result = await filterFilesByAgentAccess({ + files: [sharedFile], + userId: USER_ID, + role: 'USER', + agentId: AGENT_ID, + }); + + expect(result).toEqual([]); + }); + it('should return only owned files when user lacks VIEW permission', async () => { getAgent.mockResolvedValue(makeAgent()); checkPermission.mockResolvedValue(false); @@ -192,7 +213,7 @@ describe('filterFilesByAgentAccess', () => { }); describe('file with no user field', () => { - it('should treat file as non-owned and run through access check', async () => { + it('should exclude the file even when attached to the agent', async () => { const noUserFile = makeFile('attached-1', undefined); getAgent.mockResolvedValue(makeAgent()); checkPermission.mockResolvedValue(true); @@ -205,7 +226,7 @@ describe('filterFilesByAgentAccess', () => { }); expect(getAgent).toHaveBeenCalled(); - expect(result).toEqual([noUserFile]); + expect(result).toEqual([]); }); it('should exclude file with no user field when not attached to agent', async () => { @@ -299,6 +320,19 @@ describe('hasAccessToFilesViaAgent', () => { expect(result.get('not-attached')).toBe(false); expect(checkPermission).not.toHaveBeenCalled(); }); + + it('should deny attached files not owned by the agent author', async () => { + getAgent.mockResolvedValue(makeAgent({ author: USER_ID })); + getFiles.mockResolvedValue([makeFile('attached-1', AUTHOR_ID)]); + + const result = await hasAccessToFilesViaAgent({ + userId: USER_ID, + fileIds: ['attached-1'], + agentId: AGENT_ID, + }); + + expect(result.get('attached-1')).toBe(false); + }); }); describe('VIEW permission path', () => {