diff --git a/api/server/services/Endpoints/agents/skillDeps.js b/api/server/services/Endpoints/agents/skillDeps.js index 1a0a5ff149..39cbc7e858 100644 --- a/api/server/services/Endpoints/agents/skillDeps.js +++ b/api/server/services/Endpoints/agents/skillDeps.js @@ -74,6 +74,11 @@ async function saveSkillFileContent({ req, skillId, relativePath, content, mimeT author: req.user._id ?? req.user.id, tenantId, }); + if (!result) { + const error = new Error('Skill file save failed to persist metadata'); + error.code = 'SKILL_FILE_UPSERT_NOT_FOUND'; + throw error; + } } catch (error) { const { deleteFile } = getStrategyFunctions(storage.source); if (deleteFile) { diff --git a/api/server/services/Endpoints/agents/skillDeps.spec.js b/api/server/services/Endpoints/agents/skillDeps.spec.js new file mode 100644 index 0000000000..2f221f89e6 --- /dev/null +++ b/api/server/services/Endpoints/agents/skillDeps.spec.js @@ -0,0 +1,102 @@ +const mockSaveBuffer = jest.fn(); +const mockDeleteFile = jest.fn(); +const mockGetStrategyFunctions = jest.fn(); +const mockGetFileStrategy = jest.fn(); +const mockGetStorageMetadata = jest.fn(); +const mockResolveRequestTenantId = jest.fn(); + +jest.mock('~/server/services/Files/strategies', () => ({ + getStrategyFunctions: (...args) => mockGetStrategyFunctions(...args), +})); + +jest.mock('~/server/services/Files/Code/crud', () => ({ + batchUploadCodeEnvFiles: jest.fn(), +})); + +jest.mock('~/server/services/Files/Code/process', () => ({ + getSessionInfo: jest.fn(), + checkIfActive: jest.fn(), + readSandboxFile: jest.fn(), + writeSandboxFile: jest.fn(), +})); + +jest.mock('@librechat/api', () => ({ + checkAccess: jest.fn(), + enrichWithSkillConfigurable: jest.fn(), + getStorageMetadata: (...args) => mockGetStorageMetadata(...args), + resolveRequestTenantId: (...args) => mockResolveRequestTenantId(...args), +})); + +jest.mock('librechat-data-provider', () => ({ + AccessRoleIds: { SKILL_OWNER: 'SKILL_OWNER' }, + FileContext: { skill_file: 'skill_file' }, + PermissionBits: { EDIT: 2 }, + Permissions: { USE: 'USE', CREATE: 'CREATE' }, + PermissionTypes: { SKILLS: 'SKILLS' }, + PrincipalType: { USER: 'USER' }, + ResourceType: { SKILL: 'SKILL' }, + isEphemeralAgentId: jest.fn(() => false), +})); + +jest.mock('~/server/services/PermissionService', () => ({ + checkPermission: jest.fn(), + grantPermission: jest.fn(), +})); + +jest.mock('~/server/utils/getFileStrategy', () => ({ + getFileStrategy: (...args) => mockGetFileStrategy(...args), +})); + +const mockDb = { + getSkillFileByPath: jest.fn(), + upsertSkillFile: jest.fn(), +}; + +jest.mock('~/models', () => mockDb); + +const { getSkillToolDeps } = require('./skillDeps'); + +describe('skillDeps saveSkillFileContent', () => { + beforeEach(() => { + jest.clearAllMocks(); + mockGetFileStrategy.mockReturnValue('s3'); + mockGetStrategyFunctions.mockReturnValue({ + saveBuffer: mockSaveBuffer, + deleteFile: mockDeleteFile, + }); + mockSaveBuffer.mockResolvedValue('https://files.example.test/uploads/file.txt'); + mockDeleteFile.mockResolvedValue(undefined); + mockGetStorageMetadata.mockReturnValue({ + storageKey: 'uploads/file.txt', + storageRegion: 'us-east-2', + }); + mockResolveRequestTenantId.mockReturnValue('tenant-1'); + mockDb.getSkillFileByPath.mockResolvedValue(null); + }); + + it('cleans up the uploaded object when metadata upsert returns no row', async () => { + mockDb.upsertSkillFile.mockResolvedValue(null); + + await expect( + getSkillToolDeps().saveSkillFileContent({ + req: { + user: { id: 'user-1', _id: 'user-1' }, + config: {}, + }, + skillId: 'skill-1', + relativePath: 'references/template.html', + content: '', + mimeType: 'text/html', + }), + ).rejects.toMatchObject({ code: 'SKILL_FILE_UPSERT_NOT_FOUND' }); + + expect(mockDeleteFile).toHaveBeenCalledWith( + expect.objectContaining({ user: expect.objectContaining({ id: 'user-1' }) }), + { + filepath: 'https://files.example.test/uploads/file.txt', + user: 'user-1', + tenantId: 'tenant-1', + }, + ); + }); +}); diff --git a/packages/data-schemas/src/methods/skill.spec.ts b/packages/data-schemas/src/methods/skill.spec.ts index c56ada418c..9d27105c0e 100644 --- a/packages/data-schemas/src/methods/skill.spec.ts +++ b/packages/data-schemas/src/methods/skill.spec.ts @@ -1390,6 +1390,40 @@ describe('SkillFile methods', () => { expect(files[0].storageRegion).toBe('us-east-2'); }); + it('throws an explicit error when the upsert returns no saved file row', async () => { + const { skill } = await methods.createSkill(makeSkillInput()); + const missingUpsert = { + lean: jest.fn().mockResolvedValue({ + value: null, + lastErrorObject: { updatedExisting: false }, + }), + } as unknown as ReturnType; + const findOneAndUpdateSpy = jest + .spyOn(SkillFile, 'findOneAndUpdate') + .mockReturnValueOnce(missingUpsert); + const bumpSpy = jest.spyOn(Skill, 'findByIdAndUpdate'); + + try { + await expect( + methods.upsertSkillFile({ + skillId: skill._id, + relativePath: 'scripts/parse.sh', + file_id: 'file-1', + filename: 'parse.sh', + filepath: '/tmp/v1', + source: 'local', + mimeType: 'text/plain', + bytes: 10, + author: owner._id, + }), + ).rejects.toMatchObject({ code: 'SKILL_FILE_UPSERT_NOT_FOUND' }); + expect(bumpSpy).not.toHaveBeenCalled(); + } finally { + findOneAndUpdateSpy.mockRestore(); + bumpSpy.mockRestore(); + } + }); + it('clears codeEnvRef when a skill file is upserted (replacement)', async () => { /* A re-upload of a skill file replaces the row's contents — but the * cached `codeEnvRef` refers to the OLD bytes living in codeapi. diff --git a/packages/data-schemas/src/methods/skill.ts b/packages/data-schemas/src/methods/skill.ts index bb4b2d8786..512ce90c13 100644 --- a/packages/data-schemas/src/methods/skill.ts +++ b/packages/data-schemas/src/methods/skill.ts @@ -42,6 +42,13 @@ export type ValidationIssue = { severity?: 'error' | 'warning'; }; +type SkillFileUpsertResult = { + value: (ISkillFile & { _id: Types.ObjectId }) | null; + lastErrorObject?: { + updatedExisting?: boolean; + }; +}; + /** Partition an issue list into blocking errors and non-blocking warnings. */ export function partitionIssues(issues: ValidationIssue[]): { errors: ValidationIssue[]; @@ -1437,12 +1444,7 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk } const SkillFile = mongoose.models.SkillFile as Model; const category = inferSkillFileCategory(row.relativePath); - // Atomic new-vs-replace detection: with `new: false, upsert: true`, - // `findOneAndUpdate` returns the pre-update document (or null if the doc - // did not exist and was just inserted). Checking the return value replaces - // a non-atomic `findOne` + `upsert` pair that could double-count on - // concurrent uploads of the same (skillId, relativePath). - const previous = await SkillFile.findOneAndUpdate( + const result = (await SkillFile.findOneAndUpdate( { skillId: row.skillId, relativePath: row.relativePath }, { $set: { @@ -1463,23 +1465,17 @@ export function createSkillMethods(mongoose: typeof import('mongoose'), deps: Sk }, $unset: { content: '', isBinary: '', codeEnvRef: '' }, }, - { new: false, upsert: true }, - ).lean(); - const delta = previous ? 0 : 1; + { new: true, upsert: true, includeResultMetadata: true }, + ).lean()) as unknown as SkillFileUpsertResult; + const current = result.value; + if (!current) { + const error = new Error('Skill file upsert failed to read the saved file row'); + (error as Error & { code?: string }).code = 'SKILL_FILE_UPSERT_NOT_FOUND'; + throw error; + } + const delta = result.lastErrorObject?.updatedExisting === false ? 1 : 0; await bumpSkillVersionAndAdjustFileCount(row.skillId, delta); - - // Fetch the current (post-upsert) document for the caller. This second - // round-trip is an intentional tradeoff for the TOCTOU-safe detection - // above: `new: false` is required to distinguish insert from replace - // atomically, which means `findOneAndUpdate` returns the pre-update - // document (null on insert). A separate `findOne` is the simplest way - // to return the authoritative post-upsert state. Performance impact is - // negligible compared to the file upload I/O this sits behind. - const current = await SkillFile.findOne({ - skillId: row.skillId, - relativePath: row.relativePath, - }).lean(); - return current as unknown as ISkillFile & { _id: Types.ObjectId }; + return current; } async function deleteSkillFile(