From 9a4336266374dd0eb34d00d58988b50fb7a2b46a Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Fri, 4 Sep 2026 00:14:44 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=A4=20chore:=20Scope=20Assistant=20Too?= =?UTF-8?q?l-Resource=20Lookups=20(#15579)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DELETE /api/files` looked the assistant up by `id`, but the Assistant schema stores it as `assistant_id`. `api/db/connect.js` sets `strictQuery`, under which Mongoose drops filter keys absent from the schema — so the lookup degraded to `findOne({})` and returned whichever assistant happened to be first in the collection, regardless of owner. The unlink path then read that stranger's `tool_resources` and intersected it with the caller's requested files, so a file the caller named could be unlinked on the strength of another user's assistant record. With an empty collection the same lookup returned `null` and the unguarded `assistant.tool_resources` threw, answering 400 instead of completing the request. Filter on `assistant_id` and guard the dereference. Adds three route tests that mirror the server's `strictQuery` setting, without which the harness cannot reproduce either failure. --- api/server/routes/files/files.js | 4 +- api/server/routes/files/files.test.js | 77 +++++++++++++++++++++++++++ 2 files changed, 79 insertions(+), 2 deletions(-) diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index 7b2c3935c8..281afd1d54 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -289,10 +289,10 @@ router.delete('/', async (req, res) => { /* Handle assistant unlinking even if no valid files to delete */ if (req.body.assistant_id && req.body.tool_resource && dbFiles.length === 0) { const assistant = await db.getAssistant({ - id: req.body.assistant_id, + assistant_id: req.body.assistant_id, }); - const toolResourceFiles = assistant.tool_resources?.[req.body.tool_resource]?.file_ids ?? []; + const toolResourceFiles = assistant?.tool_resources?.[req.body.tool_resource]?.file_ids ?? []; const assistantFiles = files.filter((f) => toolResourceFiles.includes(f.file_id)); const result = await processDeleteRequest({ req, files: assistantFiles }); diff --git a/api/server/routes/files/files.test.js b/api/server/routes/files/files.test.js index 8d80a9bbf0..2ad58d350d 100644 --- a/api/server/routes/files/files.test.js +++ b/api/server/routes/files/files.test.js @@ -694,6 +694,83 @@ describe('File Routes - Delete with Agent Access', () => { }); }); + /* Mirrors api/db/connect.js, which sets strictQuery for the running server. Under it + Mongoose DROPS filter keys absent from the schema, so a lookup on a misspelled path + degrades to findOne({}) — the first document in the collection, whoever owns it. */ + describe('DELETE /files - assistant tool resource unlinking', () => { + let previousStrictQuery; + + beforeAll(() => { + previousStrictQuery = mongoose.get('strictQuery'); + mongoose.set('strictQuery', true); + }); + + afterAll(async () => { + mongoose.set('strictQuery', previousStrictQuery); + await mongoose.connection.collection('assistants').deleteMany({}); + }); + + beforeEach(async () => { + await mongoose.connection.collection('assistants').deleteMany({}); + }); + + it("does not unlink another user's assistant files when the requested assistant is missing", async () => { + const strangerFileId = uuidv4(); + /* Written straight to the collection: `tool_resources` predates the current schema. */ + await mongoose.connection.collection('assistants').insertOne({ + user: new mongoose.Types.ObjectId(), + assistant_id: 'asst_belonging_to_someone_else', + tool_resources: { file_search: { file_ids: [strangerFileId] } }, + }); + + const response = await request(app) + .delete('/files') + .send({ + assistant_id: 'asst_that_does_not_exist', + tool_resource: 'file_search', + files: [{ file_id: strangerFileId, filepath: '/uploads/stranger.txt' }], + }); + + expect(response.status).toBe(200); + expect(processDeleteRequest).toHaveBeenCalledWith(expect.objectContaining({ files: [] })); + }); + + it('unlinks the requested assistant own files', async () => { + const ownFileId = uuidv4(); + await mongoose.connection.collection('assistants').insertOne({ + user: new mongoose.Types.ObjectId(), + assistant_id: 'asst_requested', + tool_resources: { file_search: { file_ids: [ownFileId] } }, + }); + + const response = await request(app) + .delete('/files') + .send({ + assistant_id: 'asst_requested', + tool_resource: 'file_search', + files: [{ file_id: ownFileId, filepath: '/uploads/own.txt' }], + }); + + expect(response.status).toBe(200); + expect(processDeleteRequest).toHaveBeenCalledWith( + expect.objectContaining({ files: [expect.objectContaining({ file_id: ownFileId })] }), + ); + }); + + it('answers instead of throwing when no assistants exist at all', async () => { + const response = await request(app) + .delete('/files') + .send({ + assistant_id: 'asst_that_does_not_exist', + tool_resource: 'file_search', + files: [{ file_id: uuidv4(), filepath: '/uploads/ghost.txt' }], + }); + + expect(response.status).toBe(200); + expect(processDeleteRequest).toHaveBeenCalledWith(expect.objectContaining({ files: [] })); + }); + }); + describe('GET /files/download-url/:userId/:file_id', () => { it('returns a direct signed download URL when the strategy supports it', async () => { const userFileId = uuidv4();