CERBERUS/LibreChat
1
0
Fork 0
mirror of https://github.com/danny-avila/LibreChat.git synced 2026-08-04 14:57:42 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 6

🛂 fix: Harden Agent File Preview Access (#12981)

Browse source 
* fix: harden agent file access

* style: format agent file query

* fix: prune agent file refs on alternate writes

* test: fix agent pruning specs
This commit is contained in:
Danny Avila 2026-05-06 19:56:04 -04:00 • committed by GitHub
parent f0ab71f4f4
commit 5c338a4642
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
10 changed files with 548 additions and 90 deletions
Download patch file Download diff file Expand all files Collapse all files

115
api/server/controllers/agents/v1.js
View file

@ -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<number>} 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();
}

215
api/server/controllers/agents/v1.spec.js
View file

@ -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]);
});
});

27
api/server/middleware/accessResources/fileAccess.js
View file

@ -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();

112
api/server/middleware/accessResources/fileAccess.spec.js
View file

@ -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);
});
});
});

6
api/server/routes/files/files.agents.test.js
View file

@ -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);
});
});

5
api/server/routes/files/files.js
View file

@ -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) {

53
api/server/routes/files/files.test.js
View file

@ -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();

25
api/server/routes/files/preview.spec.js
View file

@ -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([
{

40
api/server/services/Files/permissions.js
View file

@ -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<string, boolean>>} 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));

40
api/server/services/Files/permissions.spec.js
View file

@ -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', () => {