diff --git a/api/app/clients/tools/structured/DALLE3.js b/api/app/clients/tools/structured/DALLE3.js index e78c0cba60..8ce46323af 100644 --- a/api/app/clients/tools/structured/DALLE3.js +++ b/api/app/clients/tools/structured/DALLE3.js @@ -48,6 +48,7 @@ class DALLE3 extends Tool { this.returnMetadata = fields.returnMetadata ?? false; this.userId = fields.userId; + this.tenantId = fields.req?.user?.tenantId; this.fileStrategy = fields.fileStrategy; /** @type {boolean} */ this.isAgent = fields.isAgent; @@ -228,6 +229,7 @@ Error Message: ${error.message}`); fileName: imageName, fileStrategy: this.fileStrategy, context: FileContext.image_generation, + tenantId: this.tenantId, }); if (this.returnMetadata) { diff --git a/api/app/clients/tools/structured/FluxAPI.js b/api/app/clients/tools/structured/FluxAPI.js index 9fce38b86a..dc94a25e82 100644 --- a/api/app/clients/tools/structured/FluxAPI.js +++ b/api/app/clients/tools/structured/FluxAPI.js @@ -109,6 +109,7 @@ class FluxAPI extends Tool { this.override = fields.override ?? false; this.userId = fields.userId; + this.tenantId = fields.req?.user?.tenantId; this.fileStrategy = fields.fileStrategy; /** @type {boolean} **/ @@ -341,6 +342,7 @@ class FluxAPI extends Tool { fileName: imageName, basePath: 'images', context: FileContext.image_generation, + tenantId: this.tenantId, }); logger.debug('[FluxAPI] Image saved to path:', result.filepath); @@ -571,6 +573,7 @@ class FluxAPI extends Tool { fileName: imageName, basePath: 'images', context: FileContext.image_generation, + tenantId: this.tenantId, }); logger.debug('[FluxAPI] Finetuned image saved to path:', result.filepath); diff --git a/api/app/clients/tools/structured/specs/imageTools-agent.spec.js b/api/app/clients/tools/structured/specs/imageTools-agent.spec.js index 50345f3504..f88b76a116 100644 --- a/api/app/clients/tools/structured/specs/imageTools-agent.spec.js +++ b/api/app/clients/tools/structured/specs/imageTools-agent.spec.js @@ -99,6 +99,14 @@ describe('image tools - agent mode ToolMessage format', () => { expect(dalle.responseFormat).not.toBe('content_and_artifact'); }); + it('keeps tenant context without retaining the request object', () => { + const req = { user: { tenantId: 'tenant-a' }, socket: {} }; + const dalle = new DALLE3({ isAgent: false, processFileURL: jest.fn(), req }); + + expect(dalle.tenantId).toBe('tenant-a'); + expect(dalle.req).toBeUndefined(); + }); + it('invoke() returns ToolMessage with base64 in artifact, not serialized in content', async () => { const dalle = new DALLE3({ isAgent: true }); const result = await dalle.invoke( @@ -172,6 +180,14 @@ describe('image tools - agent mode ToolMessage format', () => { expect(flux.responseFormat).not.toBe('content_and_artifact'); }); + it('keeps tenant context without retaining the request object', () => { + const req = { user: { tenantId: 'tenant-a' }, socket: {} }; + const flux = new FluxAPI({ isAgent: false, processFileURL: jest.fn(), req }); + + expect(flux.tenantId).toBe('tenant-a'); + expect(flux.req).toBeUndefined(); + }); + it('invoke() returns ToolMessage with base64 in artifact, not serialized in content', async () => { const flux = new FluxAPI({ isAgent: true }); const invokePromise = flux.invoke( diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js index d61bcc2844..5a4ce8a10a 100644 --- a/api/server/controllers/AuthController.js +++ b/api/server/controllers/AuthController.js @@ -120,7 +120,11 @@ const refreshController = async (req, res) => { ); } - const token = setOpenIDAuthTokens(tokenset, req, res, user._id.toString(), refreshToken); + const token = setOpenIDAuthTokens(tokenset, req, res, { + userId: user._id.toString(), + existingRefreshToken: refreshToken, + tenantId: user.tenantId, + }); const { password: _pw, __v: _v, totpSecret: _ts, backupCodes: _bc, ...safeUser } = user; return res.status(200).send({ token, user: safeUser }); @@ -146,7 +150,7 @@ const refreshController = async (req, res) => { const userId = payload.id; if (process.env.NODE_ENV === 'CI') { - const token = await setAuthTokens(userId, res); + const token = await setAuthTokens(userId, res, null, req); return res.status(200).send({ token, user }); } @@ -160,7 +164,7 @@ const refreshController = async (req, res) => { ); if (session && session.expiration > new Date()) { - const token = await setAuthTokens(userId, res, session); + const token = await setAuthTokens(userId, res, session, req); res.status(200).send({ token, user }); } else if (req?.query?.retry) { diff --git a/api/server/controllers/agents/v1.js b/api/server/controllers/agents/v1.js index 23c8341839..4f04a00be7 100644 --- a/api/server/controllers/agents/v1.js +++ b/api/server/controllers/agents/v1.js @@ -960,6 +960,7 @@ const uploadAgentAvatarHandler = async (req, res) => { userId: req.user.id, manual: 'false', agentId: agent_id, + tenantId: req.user.tenantId, }); const image = { @@ -972,7 +973,11 @@ const uploadAgentAvatarHandler = async (req, res) => { if (_avatar && _avatar.source) { const { deleteFile } = getStrategyFunctions(_avatar.source); try { - await deleteFile(req, { filepath: _avatar.filepath }); + await deleteFile(req, { + filepath: _avatar.filepath, + user: req.user.id, + tenantId: req.user.tenantId, + }); await db.deleteFileByFilter({ user: req.user.id, filepath: _avatar.filepath }); } catch (error) { logger.error('[/:agent_id/avatar] Error deleting old avatar', error); diff --git a/api/server/controllers/assistants/v1.js b/api/server/controllers/assistants/v1.js index c441b7ec59..19be145795 100644 --- a/api/server/controllers/assistants/v1.js +++ b/api/server/controllers/assistants/v1.js @@ -328,7 +328,11 @@ const uploadAssistantAvatar = async (req, res) => { if (_metadata.avatar && _metadata.avatar_source) { const { deleteFile } = getStrategyFunctions(_metadata.avatar_source); try { - await deleteFile(req, { filepath: _metadata.avatar }); + await deleteFile(req, { + filepath: _metadata.avatar, + user: req.user.id, + tenantId: req.user.tenantId, + }); await deleteFileByFilter({ user: req.user.id, filepath: _metadata.avatar }); } catch (error) { logger.error('[/:assistant_id/avatar] Error deleting old avatar', error); diff --git a/api/server/controllers/auth/LoginController.js b/api/server/controllers/auth/LoginController.js index 930eee86be..bf606b1dea 100644 --- a/api/server/controllers/auth/LoginController.js +++ b/api/server/controllers/auth/LoginController.js @@ -16,7 +16,7 @@ const loginController = async (req, res) => { const { password: _p, totpSecret: _t, __v, ...user } = req.user; user.id = user._id.toString(); - const token = await setAuthTokens(req.user._id, res); + const token = await setAuthTokens(req.user._id, res, null, req); return res.status(200).send({ token, user }); } catch (err) { diff --git a/api/server/controllers/auth/LogoutController.js b/api/server/controllers/auth/LogoutController.js index ae1c94a7c9..95e6c8965c 100644 --- a/api/server/controllers/auth/LogoutController.js +++ b/api/server/controllers/auth/LogoutController.js @@ -44,7 +44,10 @@ const logoutController = async (req, res) => { res.clearCookie('openid_id_token'); res.clearCookie('openid_user_id'); res.clearCookie('token_provider'); - clearCloudFrontCookies(res); + clearCloudFrontCookies(res, { + userId: req.user?.id ?? req.user?._id?.toString?.(), + tenantId: req.user?.tenantId, + }); const response = { message }; if ( isOpenIdUser && diff --git a/api/server/controllers/auth/LogoutController.spec.js b/api/server/controllers/auth/LogoutController.spec.js index ff02f5237e..e6c3149217 100644 --- a/api/server/controllers/auth/LogoutController.spec.js +++ b/api/server/controllers/auth/LogoutController.spec.js @@ -261,12 +261,15 @@ describe('LogoutController', () => { }); it('calls clearCloudFrontCookies on successful logout', async () => { - const req = buildReq(); + const req = buildReq({ user: { _id: 'user1', tenantId: 'tenantA' } }); const res = buildRes(); await logoutController(req, res); - expect(mockClearCloudFrontCookies).toHaveBeenCalledWith(res); + expect(mockClearCloudFrontCookies).toHaveBeenCalledWith(res, { + userId: 'user1', + tenantId: 'tenantA', + }); }); }); diff --git a/api/server/controllers/auth/TwoFactorAuthController.js b/api/server/controllers/auth/TwoFactorAuthController.js index 9e4af2e986..23cdd8b078 100644 --- a/api/server/controllers/auth/TwoFactorAuthController.js +++ b/api/server/controllers/auth/TwoFactorAuthController.js @@ -50,7 +50,7 @@ const verify2FAWithTempToken = async (req, res) => { delete userData.backupCodes; userData.id = user._id.toString(); - const authToken = await setAuthTokens(user._id, res); + const authToken = await setAuthTokens(user._id, res, null, req); return res.status(200).json({ token: authToken, user: userData }); } catch (err) { logger.error('[verify2FAWithTempToken]', err); diff --git a/api/server/controllers/auth/oauth.js b/api/server/controllers/auth/oauth.js index 917e9e2bef..31499f7de6 100644 --- a/api/server/controllers/auth/oauth.js +++ b/api/server/controllers/auth/oauth.js @@ -68,9 +68,12 @@ function createOAuthHandler(redirectUri = domains.client) { isEnabled(process.env.OPENID_REUSE_TOKENS) === true ) { await syncUserEntraGroupMemberships(req.user, req.user.tokenset.access_token); - setOpenIDAuthTokens(req.user.tokenset, req, res, req.user._id.toString()); + setOpenIDAuthTokens(req.user.tokenset, req, res, { + userId: req.user._id.toString(), + tenantId: req.user.tenantId, + }); } else { - await setAuthTokens(req.user._id, res); + await setAuthTokens(req.user._id, res, null, req); } res.redirect(redirectUri); } catch (err) { diff --git a/api/server/middleware/accessResources/fileAccess.js b/api/server/middleware/accessResources/fileAccess.js index 0f77a61175..230148fb53 100644 --- a/api/server/middleware/accessResources/fileAccess.js +++ b/api/server/middleware/accessResources/fileAccess.js @@ -60,6 +60,14 @@ const checkAgentBasedFileAccess = async ({ userId, role, fileId }) => { } }; +const getTenantId = (value) => value?.toString?.() ?? null; + +const denyFileAccess = (res) => + res.status(403).json({ + error: 'Forbidden', + message: 'Insufficient permissions to access this file', + }); + /** * Middleware to check if user can access a file * Checks: 1) File ownership, 2) Agent-based access (file inherits agent permissions) @@ -91,6 +99,15 @@ const fileAccess = async (req, res, next) => { }); } + const fileTenantId = getTenantId(file.tenantId); + const userTenantId = getTenantId(req.user?.tenantId); + // Tenant-scoped files are restricted to their tenant. Legacy files without + // tenantId remain governed by owner/agent ACLs for non-tenant migrations. + if (fileTenantId && fileTenantId !== userTenantId) { + logger.warn(`[fileAccess] User ${userId} denied cross-tenant access to file ${fileId}`); + return denyFileAccess(res); + } + if (file.user && file.user.toString() === userId) { req.fileAccess = { file }; return next(); @@ -104,10 +121,7 @@ const fileAccess = async (req, res, next) => { } logger.warn(`[fileAccess] User ${userId} denied access to file ${fileId}`); - return res.status(403).json({ - error: 'Forbidden', - message: 'Insufficient permissions to access this file', - }); + return denyFileAccess(res); } catch (error) { logger.error('[fileAccess] Error checking file access:', error); return res.status(500).json({ diff --git a/api/server/middleware/accessResources/fileAccess.spec.js b/api/server/middleware/accessResources/fileAccess.spec.js index 72896b0629..90b7d8dca7 100644 --- a/api/server/middleware/accessResources/fileAccess.spec.js +++ b/api/server/middleware/accessResources/fileAccess.spec.js @@ -1,4 +1,5 @@ const mongoose = require('mongoose'); +const { tenantStorage } = require('@librechat/data-schemas'); const { ResourceType, PrincipalType, PrincipalModel } = require('librechat-data-provider'); const { MongoMemoryServer } = require('mongodb-memory-server'); const { fileAccess } = require('./fileAccess'); @@ -115,6 +116,50 @@ describe('fileAccess middleware', () => { }); }); + test('should deny access when tenant does not match even if user owns the file', async () => { + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + createFile({ + user: testUser._id.toString(), + file_id: 'file_owned_by_user_other_tenant', + filepath: '/test/file.txt', + filename: 'file.txt', + type: 'text/plain', + size: 100, + tenantId: 'tenant-a', + }), + ); + + req.user.tenantId = 'tenant-b'; + req.params.file_id = 'file_owned_by_user_other_tenant'; + await fileAccess(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + expect(res.json).toHaveBeenCalledWith({ + error: 'Forbidden', + message: 'Insufficient permissions to access this file', + }); + }); + + test('should allow tenant-scoped users to access owned legacy files without tenantId', async () => { + await createFile({ + user: testUser._id.toString(), + file_id: 'legacy_file_owned_by_user', + filepath: '/test/legacy.txt', + filename: 'legacy.txt', + type: 'text/plain', + size: 100, + }); + + req.user.tenantId = 'tenant-b'; + req.params.file_id = 'legacy_file_owned_by_user'; + await fileAccess(req, res, next); + + expect(next).toHaveBeenCalled(); + expect(req.fileAccess.file.file_id).toBe('legacy_file_owned_by_user'); + expect(res.status).not.toHaveBeenCalled(); + }); + test('should return 404 when file does not exist', async () => { req.params.file_id = 'non_existent_file'; await fileAccess(req, res, next); @@ -223,6 +268,50 @@ describe('fileAccess middleware', () => { expect(req.fileAccess).toBeDefined(); }); + test('should deny cross-tenant access even when user has VIEW permission on agent with file', async () => { + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + createFile({ + user: otherUser._id.toString(), + file_id: 'cross_tenant_shared_file', + filepath: '/test/cross-tenant.txt', + filename: 'cross-tenant.txt', + type: 'text/plain', + size: 100, + tenantId: 'tenant-a', + }), + ); + + const agent = await createAgent({ + id: `agent_cross_tenant_${Date.now()}`, + name: 'Cross Tenant Agent', + provider: 'openai', + model: 'gpt-4', + author: otherUser._id, + tool_resources: { + execute_code: { + file_ids: ['cross_tenant_shared_file'], + }, + }, + }); + + await AclEntry.create({ + principalType: PrincipalType.USER, + principalId: testUser._id, + principalModel: PrincipalModel.USER, + resourceType: ResourceType.AGENT, + resourceId: agent._id, + permBits: 1, + grantedBy: otherUser._id, + }); + + req.user.tenantId = 'tenant-b'; + req.params.file_id = 'cross_tenant_shared_file'; + await fileAccess(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + test('should check file in ocr tool_resources', async () => { await createAgent({ id: `agent_ocr_${Date.now()}`, diff --git a/api/server/routes/files/avatar.js b/api/server/routes/files/avatar.js index f5c9379178..db00674f82 100644 --- a/api/server/routes/files/avatar.js +++ b/api/server/routes/files/avatar.js @@ -29,7 +29,12 @@ router.post('/', async (req, res) => { }); const { processAvatar } = getStrategyFunctions(fileStrategy); - const url = await processAvatar({ buffer: resizedBuffer, userId, manual }); + const url = await processAvatar({ + buffer: resizedBuffer, + userId, + manual, + tenantId: req.user.tenantId, + }); res.json({ url }); } catch (error) { diff --git a/api/server/routes/files/files.js b/api/server/routes/files/files.js index 0e3273efe2..8183bca973 100644 --- a/api/server/routes/files/files.js +++ b/api/server/routes/files/files.js @@ -29,7 +29,7 @@ const { getOpenAIClient } = require('~/server/controllers/assistants/helpers'); const { hasCapability } = require('~/server/middleware/roles/capabilities'); const { checkPermission } = require('~/server/services/PermissionService'); const { hasAccessToFilesViaAgent } = require('~/server/services/Files'); -const { getContentDisposition } = require('~/server/utils/files'); +const { cleanFileName, getContentDisposition } = require('~/server/utils/files'); const { getLogStores } = require('~/cache'); const { Readable } = require('stream'); const db = require('~/models'); @@ -164,7 +164,7 @@ router.delete('/', async (req, res) => { } } - if (nonOwnedFiles.length === 0) { + if (dbFiles.length > 0 && nonOwnedFiles.length === 0) { await processDeleteRequest({ req, files: ownedFiles }); logger.debug( `[/files] Files deleted successfully: ${ownedFiles @@ -214,9 +214,28 @@ router.delete('/', async (req, res) => { }); const toolResourceFiles = agent.tool_resources?.[req.body.tool_resource]?.file_ids ?? []; - const agentFiles = files.filter((f) => toolResourceFiles.includes(f.file_id)); + const agentFiles = files + .filter((f) => toolResourceFiles.includes(f.file_id)) + .map((file) => ({ tool_resource: req.body.tool_resource, file_id: file.file_id })); + const accessMap = await hasAccessToFilesViaAgent({ + userId: req.user.id, + role: req.user.role, + fileIds: agentFiles.map((file) => file.file_id), + agentId: req.body.agent_id, + isDelete: true, + }); + const unauthorizedFiles = agentFiles.filter((file) => !accessMap.get(file.file_id)); + if (unauthorizedFiles.length > 0) { + return res.status(403).json({ + message: 'You can only delete files you have access to', + unauthorizedFiles: unauthorizedFiles.map((file) => file.file_id), + }); + } - await processDeleteRequest({ req, files: agentFiles }); + await db.removeAgentResourceFiles({ + agent_id: req.body.agent_id, + files: agentFiles, + }); res.status(200).json({ message: 'File associations removed successfully from agent' }); return; } @@ -375,6 +394,104 @@ router.get('/:file_id/preview', fileAccess, async (req, res) => { } }); +/** + * Returns a strategy-managed signed URL for an already-authorized file record. + */ +const getDirectDownloadURL = async ({ + req, + file, + customFilename = cleanFileName(file.filename), +}) => { + const { getDownloadURL } = getStrategyFunctions(file.source); + if (!getDownloadURL) { + return null; + } + + return getDownloadURL({ + req, + file, + customFilename, + contentType: file.type || 'application/octet-stream', + }); +}; + +// Security allowlist: excludes internal ids, owner/tenant identifiers, and extracted text. +// `filepath` stays included because cached TFile records need it for previews/deletes. +const DOWNLOAD_METADATA_FIELDS = [ + 'conversationId', + 'message', + 'file_id', + 'temp_file_id', + 'bytes', + 'model', + 'embedded', + 'filename', + 'filepath', + 'object', + 'type', + 'usage', + 'context', + 'source', + 'filterSource', + 'width', + 'height', + 'expiresAt', + 'preview', + 'textFormat', + 'status', + 'previewError', + 'createdAt', + 'updatedAt', +]; + +const getDownloadFileMetadata = (file) => { + const rawFile = typeof file.toObject === 'function' ? file.toObject() : file; + return DOWNLOAD_METADATA_FIELDS.reduce((metadata, field) => { + if (rawFile[field] !== undefined) { + metadata[field] = rawFile[field]; + } + return metadata; + }, {}); +}; + +router.get('/download-url/:userId/:file_id', fileAccess, async (req, res) => { + try { + const { userId, file_id } = req.params; + logger.debug(`File download URL requested by user ${userId}: ${file_id}`); + + const file = req.fileAccess.file; + if (checkOpenAIStorage(file.source) && !file.model) { + logger.warn( + `File download URL requested by user ${userId} has no associated model: ${file_id}`, + ); + return res.status(400).send('The model used when creating this file is not available'); + } + + const filename = cleanFileName(file.filename); + const downloadURL = checkOpenAIStorage(file.source) + ? null + : await getDirectDownloadURL({ req, file, customFilename: filename }); + + if (!downloadURL) { + logger.debug( + `File download URL requested by user ${userId} is not supported for source: ${file.source}`, + ); + return res.status(501).send('Not Implemented'); + } + + res.setHeader('Cache-Control', 'no-store'); + return res.status(200).json({ + url: downloadURL, + filename, + type: file.type || 'application/octet-stream', + metadata: getDownloadFileMetadata(file), + }); + } catch (error) { + logger.error('[DOWNLOAD URL ROUTE] Error generating file download URL:', error); + res.status(500).send('Error generating file download URL'); + } +}); + router.get('/download/:userId/:file_id', fileAccess, async (req, res) => { try { const { userId, file_id } = req.params; @@ -388,10 +505,10 @@ router.get('/download/:userId/:file_id', fileAccess, async (req, res) => { return res.status(400).send('The model used when creating this file is not available'); } - const { getDownloadStream } = getStrategyFunctions(file.source); - if (!getDownloadStream) { + const { getDownloadStream, getDownloadURL } = getStrategyFunctions(file.source); + if (!getDownloadStream && !getDownloadURL) { logger.warn( - `File download requested by user ${userId} has no stream method implemented: ${file.source}`, + `File download requested by user ${userId} has no download method implemented: ${file.source}`, ); return res.status(501).send('Not Implemented'); } @@ -399,7 +516,7 @@ router.get('/download/:userId/:file_id', fileAccess, async (req, res) => { const setHeaders = () => { res.setHeader('Content-Disposition', getContentDisposition(file.filename)); res.setHeader('Content-Type', 'application/octet-stream'); - res.setHeader('X-File-Metadata', JSON.stringify(file)); + res.setHeader('X-File-Metadata', JSON.stringify(getDownloadFileMetadata(file))); }; if (checkOpenAIStorage(file.source)) { @@ -426,6 +543,28 @@ router.get('/download/:userId/:file_id', fileAccess, async (req, res) => { stream.pipe(res); } else { + if (getDownloadURL && req.query.direct === 'true') { + try { + const downloadURL = await getDirectDownloadURL({ req, file }); + if (downloadURL) { + res.setHeader('Cache-Control', 'no-store'); + return res.redirect(302, downloadURL); + } + } catch (error) { + logger.warn( + '[DOWNLOAD ROUTE] Falling back to stream after URL generation failed:', + error, + ); + } + } + + if (!getDownloadStream) { + logger.warn( + `File download requested by user ${userId} has no stream method implemented: ${file.source}`, + ); + return res.status(501).send('Not Implemented'); + } + const fileStream = await getDownloadStream(req, file.filepath); fileStream.on('error', (streamError) => { diff --git a/api/server/routes/files/files.test.js b/api/server/routes/files/files.test.js index 37cbf68b93..ac1167c09e 100644 --- a/api/server/routes/files/files.test.js +++ b/api/server/routes/files/files.test.js @@ -1,14 +1,16 @@ const express = require('express'); const request = require('supertest'); const mongoose = require('mongoose'); +const { Readable } = require('stream'); const { v4: uuidv4 } = require('uuid'); -const { createMethods } = require('@librechat/data-schemas'); +const { createMethods, tenantStorage } = require('@librechat/data-schemas'); const { MongoMemoryServer } = require('mongodb-memory-server'); const { SystemRoles, ResourceType, AccessRoleIds, PrincipalType, + FileSources, } = require('librechat-data-provider'); const { createAgent, createFile } = require('~/models'); @@ -61,6 +63,7 @@ jest.mock('~/config', () => ({ })); const { processDeleteRequest } = require('~/server/services/Files/process'); +const { getStrategyFunctions } = require('~/server/services/Files/strategies'); // Import the router after mocks const router = require('./files'); @@ -110,10 +113,10 @@ describe('File Routes - Delete with Agent Access', () => { app.use((req, res, next) => { req.user = { - id: otherUserId || 'default-user', + id: otherUserId?.toString() || 'default-user', role: SystemRoles.USER, }; - req.app = { locals: {} }; + req.app.locals = {}; next(); }); @@ -430,5 +433,311 @@ describe('File Routes - Delete with Agent Access', () => { expect(response.body.unauthorizedFiles).toContain(fileId); expect(processDeleteRequest).not.toHaveBeenCalled(); }); + + it('unlinks missing agent resource files without invoking storage deletion', async () => { + const missingFileId = uuidv4(); + const agent = await createAgent({ + id: uuidv4(), + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: otherUserId, + tool_resources: { + file_search: { + file_ids: [missingFileId], + }, + }, + }); + + const response = await request(app) + .delete('/files') + .send({ + agent_id: agent.id, + tool_resource: 'file_search', + files: [{ file_id: missingFileId, filepath: '/uploads/missing.txt' }], + }); + + expect(response.status).toBe(200); + expect(response.body.message).toBe('File associations removed successfully from agent'); + expect(processDeleteRequest).not.toHaveBeenCalled(); + + const updatedAgent = await Agent.findOne({ id: agent.id }).lean(); + expect(updatedAgent.tool_resources.file_search.file_ids).toEqual([]); + }); + + it('prevents unlinking missing agent resource files without agent edit access', async () => { + const missingFileId = uuidv4(); + const agent = await createAgent({ + id: uuidv4(), + name: 'Test Agent', + provider: 'openai', + model: 'gpt-4', + author: authorId, + tool_resources: { + file_search: { + file_ids: [missingFileId], + }, + }, + }); + + const response = await request(app) + .delete('/files') + .send({ + agent_id: agent.id, + tool_resource: 'file_search', + files: [{ file_id: missingFileId, filepath: '/uploads/missing.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(missingFileId); + expect(processDeleteRequest).not.toHaveBeenCalled(); + + const updatedAgent = await Agent.findOne({ id: agent.id }).lean(); + expect(updatedAgent.tool_resources.file_search.file_ids).toEqual([missingFileId]); + }); + }); + + describe('GET /files/download-url/:userId/:file_id', () => { + it('returns a direct signed download URL when the strategy supports it', async () => { + const userFileId = uuidv4(); + const getDownloadURL = jest.fn().mockResolvedValue('https://cdn.example.com/file.pdf?signed'); + getStrategyFunctions.mockReturnValue({ getDownloadURL }); + + await createFile({ + user: otherUserId, + file_id: userFileId, + filename: 'file.pdf', + filepath: 'uploads/user/file.pdf', + bytes: 200, + type: 'application/pdf', + source: FileSources.s3, + text: 'private extracted text', + }); + + const response = await request(app).get(`/files/download-url/${otherUserId}/${userFileId}`); + + expect(response.status).toBe(200); + expect(response.body).toMatchObject({ + url: 'https://cdn.example.com/file.pdf?signed', + filename: 'file.pdf', + type: 'application/pdf', + }); + expect(response.headers['cache-control']).toBe('no-store'); + expect(response.body.metadata).toMatchObject({ + file_id: userFileId, + filename: 'file.pdf', + filepath: 'uploads/user/file.pdf', + source: FileSources.s3, + }); + expect(response.body.metadata).not.toHaveProperty('_id'); + expect(response.body.metadata).not.toHaveProperty('__v'); + expect(response.body.metadata).not.toHaveProperty('user'); + expect(response.body.metadata).not.toHaveProperty('tenantId'); + expect(response.body.metadata).not.toHaveProperty('text'); + expect(getDownloadURL).toHaveBeenCalledWith( + expect.objectContaining({ + file: expect.objectContaining({ file_id: userFileId }), + customFilename: 'file.pdf', + contentType: 'application/pdf', + }), + ); + }); + + it('returns 501 when the strategy does not support direct URLs', async () => { + const userFileId = uuidv4(); + getStrategyFunctions.mockReturnValue({}); + + await createFile({ + user: otherUserId, + file_id: userFileId, + filename: 'file.txt', + filepath: 'uploads/user/file.txt', + bytes: 200, + type: 'text/plain', + source: FileSources.local, + }); + + const response = await request(app).get(`/files/download-url/${otherUserId}/${userFileId}`); + + expect(response.status).toBe(501); + }); + + it('denies tenant-scoped files before issuing a signed URL', async () => { + const userFileId = uuidv4(); + const getDownloadURL = jest.fn().mockResolvedValue('https://cdn.example.com/file.pdf?signed'); + getStrategyFunctions.mockReturnValue({ getDownloadURL }); + + await tenantStorage.run({ tenantId: 'tenant-a' }, async () => + createFile({ + user: otherUserId, + file_id: userFileId, + filename: 'file.pdf', + filepath: 'uploads/user/file.pdf', + bytes: 200, + type: 'application/pdf', + source: FileSources.s3, + tenantId: 'tenant-a', + }), + ); + + const response = await request(app).get(`/files/download-url/${otherUserId}/${userFileId}`); + + expect(response.status).toBe(403); + expect(getDownloadURL).not.toHaveBeenCalled(); + }); + + it('returns 500 when direct URL generation fails', async () => { + const userFileId = uuidv4(); + const getDownloadURL = jest.fn().mockRejectedValue(new Error('signing failed')); + getStrategyFunctions.mockReturnValue({ getDownloadURL }); + + await createFile({ + user: otherUserId, + file_id: userFileId, + filename: 'file.pdf', + filepath: 'uploads/user/file.pdf', + bytes: 200, + type: 'application/pdf', + source: FileSources.s3, + }); + + const response = await request(app).get(`/files/download-url/${otherUserId}/${userFileId}`); + + expect(response.status).toBe(500); + expect(response.text).toBe('Error generating file download URL'); + }); + }); + + describe('GET /files/download/:userId/:file_id', () => { + it('streams proxied downloads by default when a direct URL is available', async () => { + const userFileId = uuidv4(); + const getDownloadURL = jest.fn().mockResolvedValue('https://cdn.example.com/file.pdf?signed'); + const getDownloadStream = jest.fn().mockResolvedValue(Readable.from(['file content'])); + getStrategyFunctions.mockReturnValue({ getDownloadURL, getDownloadStream }); + + await createFile({ + user: otherUserId, + file_id: userFileId, + filename: 'file.pdf', + filepath: 'uploads/user/file.pdf', + bytes: 200, + type: 'application/pdf', + source: FileSources.cloudfront, + text: 'private extracted text', + }); + + const response = await request(app).get(`/files/download/${otherUserId}/${userFileId}`); + + expect(response.status).toBe(200); + expect(response.body.toString()).toBe('file content'); + expect(response.headers.location).toBeUndefined(); + const metadata = JSON.parse(response.headers['x-file-metadata']); + expect(metadata).toMatchObject({ + file_id: userFileId, + filename: 'file.pdf', + filepath: 'uploads/user/file.pdf', + source: FileSources.cloudfront, + }); + expect(metadata).not.toHaveProperty('_id'); + expect(metadata).not.toHaveProperty('__v'); + expect(metadata).not.toHaveProperty('user'); + expect(metadata).not.toHaveProperty('tenantId'); + expect(metadata).not.toHaveProperty('text'); + expect(getDownloadURL).not.toHaveBeenCalled(); + expect(getDownloadStream).toHaveBeenCalledWith(expect.any(Object), 'uploads/user/file.pdf'); + }); + + it('redirects to a direct signed download URL when explicitly requested', async () => { + const userFileId = uuidv4(); + const getDownloadURL = jest.fn().mockResolvedValue('https://cdn.example.com/file.pdf?signed'); + const getDownloadStream = jest.fn(); + getStrategyFunctions.mockReturnValue({ getDownloadURL, getDownloadStream }); + + await createFile({ + user: otherUserId, + file_id: userFileId, + filename: 'file.pdf', + filepath: 'uploads/user/file.pdf', + bytes: 200, + type: 'application/pdf', + source: FileSources.cloudfront, + }); + + const response = await request(app).get( + `/files/download/${otherUserId}/${userFileId}?direct=true`, + ); + + expect(response.status).toBe(302); + expect(response.headers.location).toBe('https://cdn.example.com/file.pdf?signed'); + expect(response.headers['x-file-metadata']).toBeUndefined(); + expect(response.headers['cache-control']).toBe('no-store'); + expect(getDownloadStream).not.toHaveBeenCalled(); + }); + + it('falls back to streaming when direct URL generation fails', async () => { + const userFileId = uuidv4(); + const getDownloadURL = jest.fn().mockRejectedValue(new Error('missing signing keys')); + const getDownloadStream = jest.fn().mockResolvedValue(Readable.from(['file content'])); + getStrategyFunctions.mockReturnValue({ getDownloadURL, getDownloadStream }); + + await createFile({ + user: otherUserId, + file_id: userFileId, + filename: 'file.txt', + filepath: 'uploads/user/file.txt', + bytes: 200, + type: 'text/plain', + source: FileSources.s3, + }); + + const response = await request(app).get( + `/files/download/${otherUserId}/${userFileId}?direct=true`, + ); + + expect(response.status).toBe(200); + expect(response.body.toString()).toBe('file content'); + expect(response.headers.location).toBeUndefined(); + expect(response.headers['cache-control']).toBeUndefined(); + expect(getDownloadURL).toHaveBeenCalledWith( + expect.objectContaining({ + file: expect.objectContaining({ file_id: userFileId }), + customFilename: 'file.txt', + contentType: 'text/plain', + }), + ); + expect(getDownloadStream).toHaveBeenCalledWith(expect.any(Object), 'uploads/user/file.txt'); + }); + + it('returns 501 when direct URL generation fails and no stream fallback exists', async () => { + const userFileId = uuidv4(); + const getDownloadURL = jest.fn().mockRejectedValue(new Error('missing signing keys')); + getStrategyFunctions.mockReturnValue({ getDownloadURL }); + + await createFile({ + user: otherUserId, + file_id: userFileId, + filename: 'file.txt', + filepath: 'uploads/user/file.txt', + bytes: 200, + type: 'text/plain', + source: FileSources.cloudfront, + }); + + const response = await request(app).get( + `/files/download/${otherUserId}/${userFileId}?direct=true`, + ); + + expect(response.status).toBe(501); + expect(response.text).toBe('Not Implemented'); + expect(response.headers.location).toBeUndefined(); + expect(getDownloadURL).toHaveBeenCalledWith( + expect.objectContaining({ + file: expect.objectContaining({ file_id: userFileId }), + customFilename: 'file.txt', + contentType: 'text/plain', + }), + ); + }); }); }); diff --git a/api/server/routes/skills.js b/api/server/routes/skills.js index 9e9676fb45..84a303d145 100644 --- a/api/server/routes/skills.js +++ b/api/server/routes/skills.js @@ -138,10 +138,12 @@ const importHandler = createImportHandler({ upsertSkillFile, saveBuffer: (req, { userId, buffer, fileName, basePath, isImage }) => { const storage = resolveSkillStorage(req, { isImage }); - return storage.saveBuffer({ userId, buffer, fileName, basePath }).then((filepath) => ({ - filepath, - source: storage.source, - })); + return storage + .saveBuffer({ userId, buffer, fileName, basePath, tenantId: req.user.tenantId }) + .then((filepath) => ({ + filepath, + source: storage.source, + })); }, deleteFile: (req, file) => { const { deleteFile } = getStrategyFunctions(file.source); @@ -195,6 +197,7 @@ async function uploadFileHandler(req, res) { buffer: file.buffer, fileName: storageFileName, basePath: 'uploads', + tenantId: req.user.tenantId, }); let result; @@ -216,7 +219,7 @@ async function uploadFileHandler(req, res) { try { const { deleteFile } = getStrategyFunctions(storage.source); if (deleteFile) { - await deleteFile(req, { filepath }); + await deleteFile(req, { filepath, user: req.user.id, tenantId: req.user.tenantId }); } } catch (cleanupErr) { logger.error('[uploadFile] Failed to clean up orphaned blob:', cleanupErr); @@ -228,9 +231,11 @@ async function uploadFileHandler(req, res) { if (existingFile && existingFile.filepath !== filepath) { const { deleteFile: delOld } = getStrategyFunctions(existingFile.source); if (delOld) { - delOld(req, { filepath: existingFile.filepath }).catch((e) => - logger.error('[uploadFile] Old blob cleanup failed:', e), - ); + delOld(req, { + filepath: existingFile.filepath, + user: existingFile.author ?? req.user.id, + tenantId: existingFile.tenantId ?? req.user.tenantId, + }).catch((e) => logger.error('[uploadFile] Old blob cleanup failed:', e)); } } diff --git a/api/server/services/AuthService.js b/api/server/services/AuthService.js index 40b3c1a725..8053a605b4 100644 --- a/api/server/services/AuthService.js +++ b/api/server/services/AuthService.js @@ -12,6 +12,8 @@ const { isEnabled, checkEmailConfig, setCloudFrontCookies, + parseCloudFrontCookieScope, + CLOUDFRONT_SCOPE_COOKIE, isEmailDomainAllowed, shouldUseSecureCookie, resolveAppConfigForUser, @@ -401,14 +403,23 @@ const resetPassword = async (userId, token, password) => { return { message: 'Password reset was successful' }; }; +/** + * Reads the previously issued CloudFront cookie scope used for stale cookie cleanup. + * @param {ServerRequest | null} [req=null] + * @returns {import('@librechat/api').CloudFrontCookieScope | null} + */ +const getPreviousCloudFrontScope = (req) => + parseCloudFrontCookieScope(req?.cookies?.[CLOUDFRONT_SCOPE_COOKIE]); + /** * Set Auth Tokens * @param {String | ObjectId} userId * @param {ServerResponse} res - * @param {ISession | null} [session=null] + * @param {ISession | null} [_session=null] + * @param {ServerRequest | null} [req=null] * @returns */ -const setAuthTokens = async (userId, res, _session = null) => { +const setAuthTokens = async (userId, res, _session = null, req = null) => { try { let session = _session; let refreshToken; @@ -442,7 +453,14 @@ const setAuthTokens = async (userId, res, _session = null) => { sameSite: 'strict', }); - setCloudFrontCookies(res); + setCloudFrontCookies( + res, + { + userId: user?._id?.toString?.() ?? userId, + tenantId: user?.tenantId?.toString?.(), + }, + getPreviousCloudFrontScope(req), + ); return token; } catch (error) { @@ -451,6 +469,21 @@ const setAuthTokens = async (userId, res, _session = null) => { } }; +const resolveOpenIDAuthTokenOptions = (optionsOrUserId, existingRefreshToken, tenantId) => { + if (optionsOrUserId != null && typeof optionsOrUserId === 'object') { + if ( + 'userId' in optionsOrUserId || + 'existingRefreshToken' in optionsOrUserId || + 'tenantId' in optionsOrUserId + ) { + return optionsOrUserId; + } + return {}; + } + + return { userId: optionsOrUserId, existingRefreshToken, tenantId }; +}; + /** * @function setOpenIDAuthTokens * Set OpenID Authentication Tokens @@ -461,11 +494,27 @@ const setAuthTokens = async (userId, res, _session = null) => { * - The tokenset object containing access and refresh tokens * @param {Object} req - request object (for session access) * @param {Object} res - response object - * @param {string} [userId] - Optional MongoDB user ID for image path validation + * @param {Object} [options] - Optional token/cookie context + * @param {string} [options.userId] - Optional MongoDB user ID for image path validation + * @param {string} [options.existingRefreshToken] - Optional existing refresh token to preserve + * @param {string} [options.tenantId] - Optional tenant identifier for CloudFront cookie scoping * @returns {String} - id_token (preferred) or access_token as the app auth token */ -const setOpenIDAuthTokens = (tokenset, req, res, userId, existingRefreshToken) => { +const setOpenIDAuthTokens = ( + tokenset, + req, + res, + optionsOrUserId = null, + existingRefreshTokenArg, + tenantIdArg, +) => { try { + const { userId, existingRefreshToken, tenantId } = resolveOpenIDAuthTokenOptions( + optionsOrUserId, + existingRefreshTokenArg, + tenantIdArg, + ); + if (!tokenset) { logger.error('[setOpenIDAuthTokens] No tokenset found in request'); return; @@ -475,10 +524,6 @@ const setOpenIDAuthTokens = (tokenset, req, res, userId, existingRefreshToken) = DEFAULT_REFRESH_TOKEN_EXPIRY, ); const expirationDate = new Date(Date.now() + expiryInMilliseconds); - if (tokenset == null) { - logger.error('[setOpenIDAuthTokens] No tokenset found in request'); - return; - } if (!tokenset.access_token) { logger.error('[setOpenIDAuthTokens] No access token found in tokenset'); return; @@ -562,7 +607,14 @@ const setOpenIDAuthTokens = (tokenset, req, res, userId, existingRefreshToken) = }); } - setCloudFrontCookies(res); + setCloudFrontCookies( + res, + { + userId, + tenantId: tenantId ?? req.user?.tenantId, + }, + getPreviousCloudFrontScope(req), + ); return appAuthToken; } catch (error) { diff --git a/api/server/services/AuthService.spec.js b/api/server/services/AuthService.spec.js index df89f3f2c9..cf893e1d6f 100644 --- a/api/server/services/AuthService.spec.js +++ b/api/server/services/AuthService.spec.js @@ -16,6 +16,8 @@ jest.mock('@librechat/api', () => ({ shouldUseSecureCookie: jest.fn(() => false), resolveAppConfigForUser: jest.fn(async (_getAppConfig, _user) => ({})), setCloudFrontCookies: jest.fn(() => true), + parseCloudFrontCookieScope: jest.fn(() => null), + CLOUDFRONT_SCOPE_COOKIE: 'LibreChat-CloudFront-Scope', })); jest.mock('~/models', () => ({ findUser: jest.fn(), @@ -42,6 +44,7 @@ const { isEmailDomainAllowed, resolveAppConfigForUser, setCloudFrontCookies, + parseCloudFrontCookieScope, } = require('@librechat/api'); const { findUser, @@ -66,9 +69,10 @@ function mockResponse() { } /** Helper to build a mock Express request with session */ -function mockRequest(sessionData = {}) { +function mockRequest(sessionData = {}, cookies = {}) { return { session: { openidTokens: null, ...sessionData }, + cookies, }; } @@ -360,13 +364,90 @@ describe('CloudFront cookie integration', () => { refresh_token: 'the-refresh-token', }; - it('calls setCloudFrontCookies with response object', () => { + it('calls setCloudFrontCookies with response object and user scope from options', () => { const req = mockRequest(); const res = mockResponse(); - setOpenIDAuthTokens(validTokenset, req, res, 'user-123'); + setOpenIDAuthTokens(validTokenset, req, res, { + userId: 'user-123', + tenantId: 'tenantA', + }); - expect(setCloudFrontCookies).toHaveBeenCalledWith(res); + expect(setCloudFrontCookies).toHaveBeenCalledWith( + res, + { + userId: 'user-123', + tenantId: 'tenantA', + }, + null, + ); + }); + + it('keeps backward compatibility with positional user and tenant params', () => { + const req = mockRequest(); + const res = mockResponse(); + + setOpenIDAuthTokens(validTokenset, req, res, 'user-123', undefined, 'tenantA'); + + expect(setCloudFrontCookies).toHaveBeenCalledWith( + res, + { + userId: 'user-123', + tenantId: 'tenantA', + }, + null, + ); + }); + + it('treats a null options argument as an empty legacy user id', () => { + const req = mockRequest(); + const res = mockResponse(); + + const result = setOpenIDAuthTokens(validTokenset, req, res, null); + + expect(result).toBe('the-id-token'); + expect(setCloudFrontCookies).toHaveBeenCalledWith( + res, + { + userId: null, + tenantId: undefined, + }, + null, + ); + }); + + it('treats omitted options as an empty legacy user id', () => { + const req = mockRequest(); + const res = mockResponse(); + + const result = setOpenIDAuthTokens(validTokenset, req, res); + + expect(result).toBe('the-id-token'); + expect(setCloudFrontCookies).toHaveBeenCalledWith( + res, + { + userId: null, + tenantId: undefined, + }, + null, + ); + }); + + it('treats an object without token option keys as empty options', () => { + const req = mockRequest(); + const res = mockResponse(); + + const result = setOpenIDAuthTokens(validTokenset, req, res, {}); + + expect(result).toBe('the-id-token'); + expect(setCloudFrontCookies).toHaveBeenCalledWith( + res, + { + userId: undefined, + tenantId: undefined, + }, + null, + ); }); it('succeeds even when setCloudFrontCookies returns false', () => { @@ -383,7 +464,7 @@ describe('CloudFront cookie integration', () => { describe('setAuthTokens', () => { beforeEach(() => { - getUserById.mockResolvedValue({ _id: 'user-123' }); + getUserById.mockResolvedValue({ _id: 'user-123', tenantId: 'tenantA' }); generateToken.mockResolvedValue('mock-access-token'); generateRefreshToken.mockReturnValue('mock-refresh-token'); createSession.mockResolvedValue({ @@ -392,12 +473,37 @@ describe('CloudFront cookie integration', () => { }); }); - it('calls setCloudFrontCookies with response object', async () => { + it('calls setCloudFrontCookies with response object and user scope', async () => { const res = mockResponse(); await setAuthTokens('user-123', res); - expect(setCloudFrontCookies).toHaveBeenCalledWith(res); + expect(setCloudFrontCookies).toHaveBeenCalledWith( + res, + { + userId: 'user-123', + tenantId: 'tenantA', + }, + null, + ); + }); + + it('passes the previous CloudFront cookie scope when present', async () => { + parseCloudFrontCookieScope.mockReturnValue({ userId: 'old-user', tenantId: 'old-tenant' }); + const res = mockResponse(); + const req = mockRequest({}, { 'LibreChat-CloudFront-Scope': 'encoded-scope' }); + + await setAuthTokens('user-123', res, null, req); + + expect(parseCloudFrontCookieScope).toHaveBeenCalledWith('encoded-scope'); + expect(setCloudFrontCookies).toHaveBeenCalledWith( + res, + { + userId: 'user-123', + tenantId: 'tenantA', + }, + { userId: 'old-user', tenantId: 'old-tenant' }, + ); }); it('succeeds even when setCloudFrontCookies returns false', async () => { diff --git a/api/server/services/Files/Code/__tests__/process-traversal.spec.js b/api/server/services/Files/Code/__tests__/process-traversal.spec.js index b6fcc2636f..6392e5b8bd 100644 --- a/api/server/services/Files/Code/__tests__/process-traversal.spec.js +++ b/api/server/services/Files/Code/__tests__/process-traversal.spec.js @@ -86,7 +86,7 @@ const { processCodeOutput } = require('../process'); const baseParams = { req: { - user: { id: 'user123' }, + user: { id: 'user123', tenantId: 'tenantA' }, config: { fileStrategy: 'local', imageOutputType: 'webp', @@ -129,6 +129,7 @@ describe('processCodeOutput path traversal protection', () => { const fileArg = createFile.mock.calls[0][0]; expect(fileArg.filename).toBe('safe-output.csv'); + expect(fileArg.tenantId).toBe('tenantA'); }); test('sanitized name is used for image file records', async () => { @@ -144,5 +145,6 @@ describe('processCodeOutput path traversal protection', () => { expect(mockSanitizeArtifactPath).toHaveBeenCalledWith('../../../chart.png'); const fileArg = createFile.mock.calls[0][0]; expect(fileArg.filename).toBe('safe-chart.png'); + expect(fileArg.tenantId).toBe('tenantA'); }); }); diff --git a/api/server/services/Files/Code/process.js b/api/server/services/Files/Code/process.js index 4efc938223..c8d63cc919 100644 --- a/api/server/services/Files/Code/process.js +++ b/api/server/services/Files/Code/process.js @@ -396,6 +396,7 @@ const processCodeOutput = async ({ conversationId, file_id: newFileId, user: req.user.id, + tenantId: req.user.tenantId, }); const file_id = claimed.file_id; const isUpdate = file_id !== newFileId; @@ -429,6 +430,7 @@ const processCodeOutput = async ({ filename: safeName, conversationId, user: req.user.id, + tenantId: req.user.tenantId, type: `image/${appConfig.imageOutputType}`, createdAt: isUpdate ? claimed.createdAt : formattedDate, updatedAt: formattedDate, @@ -490,6 +492,7 @@ const processCodeOutput = async ({ buffer, fileName, basePath: 'uploads', + tenantId: req.user.tenantId, }); /* `classifyCodeArtifact` and `extractCodeArtifactText` make @@ -523,6 +526,7 @@ const processCodeOutput = async ({ type: mimeType, conversationId, user: req.user.id, + tenantId: req.user.tenantId, bytes: buffer.length, updatedAt: formattedDate, metadata: { fileIdentifier }, diff --git a/api/server/services/Files/Code/process.spec.js b/api/server/services/Files/Code/process.spec.js index 98cbbf9fff..7ca7799bfb 100644 --- a/api/server/services/Files/Code/process.spec.js +++ b/api/server/services/Files/Code/process.spec.js @@ -245,6 +245,29 @@ describe('Code Process', () => { expect(result.filename).toBe('chart.png'); }); + it('persists tenantId on image code output records when present', async () => { + const tenantReq = { ...mockReq, user: { ...mockReq.user, tenantId: 'tenantA' } }; + const imageBuffer = Buffer.alloc(500); + mockAxios.mockResolvedValue({ data: imageBuffer }); + convertImage.mockResolvedValue({ + filepath: '/t/tenantA/images/user-123/mock-uuid-1234.webp', + }); + + await processCodeOutput({ + ...baseParams, + req: tenantReq, + name: 'chart.png', + }); + + expect(mockClaimCodeFile).toHaveBeenCalledWith( + expect.objectContaining({ tenantId: 'tenantA' }), + ); + expect(createFile).toHaveBeenCalledWith( + expect.objectContaining({ tenantId: 'tenantA' }), + true, + ); + }); + it('should update existing image file with cache-busted filepath', async () => { const imageParams = { ...baseParams, name: 'chart.png' }; mockClaimCodeFile.mockResolvedValue({ @@ -296,6 +319,33 @@ describe('Code Process', () => { expect(result.bytes).toBe(100); }); + it('passes and persists tenantId for non-image code output records', async () => { + const tenantReq = { ...mockReq, user: { ...mockReq.user, tenantId: 'tenantA' } }; + const smallBuffer = Buffer.alloc(100); + mockAxios.mockResolvedValue({ data: smallBuffer }); + + const mockSaveBuffer = jest + .fn() + .mockResolvedValue('/t/tenantA/uploads/user-123/mock-file-path.txt'); + getStrategyFunctions.mockReturnValue({ saveBuffer: mockSaveBuffer }); + + await processCodeOutput({ + ...baseParams, + req: tenantReq, + }); + + expect(mockClaimCodeFile).toHaveBeenCalledWith( + expect.objectContaining({ tenantId: 'tenantA' }), + ); + expect(mockSaveBuffer).toHaveBeenCalledWith( + expect.objectContaining({ tenantId: 'tenantA' }), + ); + expect(createFile).toHaveBeenCalledWith( + expect.objectContaining({ tenantId: 'tenantA' }), + true, + ); + }); + it('preserves nested directory paths in the DB record while flattening the storage key', async () => { /* Regression test for the silent-data-loss path: when codeapi reports a * file with a nested name like "test_folder/test_file.txt", LibreChat diff --git a/api/server/services/Files/images/convert.js b/api/server/services/Files/images/convert.js index d5f5c2893b..dd0d795448 100644 --- a/api/server/services/Files/images/convert.js +++ b/api/server/services/Files/images/convert.js @@ -58,6 +58,7 @@ async function convertImage(req, file, resolution = 'high', basename = '') { userId: req.user.id, buffer: outputBuffer, fileName: newFileName, + tenantId: req.user.tenantId, }); const bytes = Buffer.byteLength(outputBuffer); diff --git a/api/server/services/Files/process.js b/api/server/services/Files/process.js index 07c101fc33..057d3e531d 100644 --- a/api/server/services/Files/process.js +++ b/api/server/services/Files/process.js @@ -245,18 +245,44 @@ const processDeleteRequest = async ({ req, files }) => { * @param {string} params.fileName - The name that will be used to save the file (including extension) * @param {string} params.basePath - The base path or directory where the file will be saved or retrieved from. * @param {FileContext} params.context - The context of the file (e.g., 'avatar', 'image_generation', etc.) + * @param {string} [params.tenantId] - Optional tenant identifier for tenant-prefixed storage paths. * @returns {Promise} A promise that resolves to the DB representation (MongoFile) * of the processed file. It throws an error if the file processing fails at any stage. */ -const processFileURL = async ({ fileStrategy, userId, URL, fileName, basePath, context }) => { +const processFileURL = async ({ + fileStrategy, + userId, + URL, + fileName, + basePath, + context, + tenantId, +}) => { const { saveURL, getFileURL } = getStrategyFunctions(fileStrategy); try { + const savedFile = await saveURL({ userId, URL, fileName, basePath, tenantId }); + if (!savedFile) { + throw new Error(`Strategy "${fileStrategy}" did not save "${fileName}"`); + } + const { bytes = 0, type = '', dimensions = {}, - } = (await saveURL({ userId, URL, fileName, basePath })) || {}; - const filepath = await getFileURL({ fileName: `${userId}/${fileName}`, basePath }); + } = typeof savedFile === 'string' ? {} : savedFile; + const fallbackFileName = + fileStrategy === FileSources.local || fileStrategy === FileSources.firebase + ? `${userId}/${fileName}` + : fileName; + const filepath = + typeof savedFile === 'string' + ? savedFile + : (savedFile.filepath ?? + (await getFileURL({ userId, fileName: fallbackFileName, basePath, tenantId }))); + if (!filepath) { + throw new Error(`Strategy "${fileStrategy}" did not return a file URL for "${fileName}"`); + } + return await db.createFile( { user: userId, @@ -267,6 +293,7 @@ const processFileURL = async ({ fileStrategy, userId, URL, fileName, basePath, c source: fileStrategy, type, context, + tenantId, width: dimensions.width, height: dimensions.height, }, @@ -316,6 +343,7 @@ const processImageFile = async ({ req, res, metadata, returnFile = false }) => { type: `image/${appConfig.imageOutputType}`, width, height, + tenantId: req.user.tenantId, }, true, ); @@ -354,7 +382,12 @@ const uploadImageBuffer = async ({ req, context, metadata = {}, resize = true }) }`; } const fileName = `${file_id}-${filename}`; - const filepath = await saveBuffer({ userId: req.user.id, fileName, buffer }); + const filepath = await saveBuffer({ + userId: req.user.id, + fileName, + buffer, + tenantId: req.user.tenantId, + }); return await db.createFile( { user: req.user.id, @@ -367,6 +400,7 @@ const uploadImageBuffer = async ({ req, context, metadata = {}, resize = true }) type, width, height, + tenantId: req.user.tenantId, }, true, ); @@ -456,6 +490,7 @@ const processFileUpload = async ({ req, res, metadata }) => { source, height, width, + tenantId: req.user.tenantId, }, true, ); @@ -546,6 +581,7 @@ const processAgentFileUpload = async ({ req, res, metadata }) => { filename: file.originalname, model: messageAttachment ? undefined : req.body.model, context: messageAttachment ? FileContext.message_attachment : FileContext.agents, + tenantId: req.user.tenantId, }); if (!messageAttachment && tool_resource) { @@ -722,6 +758,7 @@ const processAgentFileUpload = async ({ req, res, metadata }) => { source, height, width, + tenantId: req.user.tenantId, }); const result = await db.createFile(fileInfo, true); @@ -767,6 +804,7 @@ const processOpenAIFile = async ({ source, model: openai.req.body.model, filename: originalName ?? file_id, + tenantId: openai.req?.user?.tenantId, }; if (saveFile) { @@ -810,6 +848,7 @@ const processOpenAIImageOutput = async ({ req, buffer, file_id, filename, fileEx context: FileContext.assistants_output, file_id, filename, + tenantId: req.user.tenantId, }; db.createFile(file, true); return file; @@ -954,6 +993,7 @@ async function saveBase64Image( userId: req.user.id, fileName: filename, buffer: image.buffer, + tenantId: req.user.tenantId, }); return await db.createFile( { @@ -967,6 +1007,7 @@ async function saveBase64Image( bytes: image.bytes, width: image.width, height: image.height, + tenantId: req.user.tenantId, }, true, ); diff --git a/api/server/services/Files/process.spec.js b/api/server/services/Files/process.spec.js index 88f2bb7b6b..02d12673e0 100644 --- a/api/server/services/Files/process.spec.js +++ b/api/server/services/Files/process.spec.js @@ -68,11 +68,17 @@ jest.mock('~/server/services/Files/Audio/STTService', () => ({ STTService: { getInstance: jest.fn() }, })); -const { EToolResources, FileSources, AgentCapabilities } = require('librechat-data-provider'); +const { + EToolResources, + FileSources, + FileContext, + AgentCapabilities, +} = require('librechat-data-provider'); const { mergeFileConfig } = require('librechat-data-provider'); const { checkCapability } = require('~/server/services/Config'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); -const { processAgentFileUpload } = require('./process'); +const db = require('~/models'); +const { processAgentFileUpload, processFileURL } = require('./process'); const PDF_MIME = 'application/pdf'; const DOCX_MIME = 'application/vnd.openxmlformats-officedocument.wordprocessingml.document'; @@ -84,7 +90,7 @@ const ODP_MIME = 'application/vnd.oasis.opendocument.presentation'; const ODG_MIME = 'application/vnd.oasis.opendocument.graphics'; const makeReq = ({ mimetype = PDF_MIME, ocrConfig = null } = {}) => ({ - user: { id: 'user-123' }, + user: { id: 'user-123', tenantId: 'tenant-a' }, file: { path: '/tmp/upload.bin', originalname: 'upload.bin', @@ -340,3 +346,136 @@ describe('processAgentFileUpload', () => { }); }); }); + +describe('processFileURL', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('throws and skips DB persistence when saveURL returns null', async () => { + const saveURL = jest.fn().mockResolvedValue(null); + const getFileURL = jest.fn(); + getStrategyFunctions.mockReturnValue({ saveURL, getFileURL }); + + await expect( + processFileURL({ + fileStrategy: FileSources.local, + userId: 'user-123', + URL: 'https://example.com/image.png', + fileName: 'image.png', + basePath: 'images', + context: FileContext.image_generation, + tenantId: 'tenant-a', + }), + ).rejects.toThrow('Strategy "local" did not save "image.png"'); + + expect(getFileURL).not.toHaveBeenCalled(); + expect(db.createFile).not.toHaveBeenCalled(); + }); + + it('persists tenantId and strategy-returned filepath metadata', async () => { + const saveURL = jest.fn().mockResolvedValue({ + filepath: 'https://cdn.example.com/t/tenant-a/images/user-123/image.png', + bytes: 512, + type: 'image/png', + dimensions: { width: 32, height: 64 }, + }); + const getFileURL = jest.fn(); + getStrategyFunctions.mockReturnValue({ saveURL, getFileURL }); + + await processFileURL({ + fileStrategy: FileSources.cloudfront, + userId: 'user-123', + URL: 'https://example.com/image.png', + fileName: 'image.png', + basePath: 'images', + context: FileContext.image_generation, + tenantId: 'tenant-a', + }); + + expect(getFileURL).not.toHaveBeenCalled(); + expect(db.createFile).toHaveBeenCalledWith( + expect.objectContaining({ + user: 'user-123', + filepath: 'https://cdn.example.com/t/tenant-a/images/user-123/image.png', + bytes: 512, + filename: 'image.png', + source: FileSources.cloudfront, + type: 'image/png', + context: FileContext.image_generation, + tenantId: 'tenant-a', + width: 32, + height: 64, + }), + true, + ); + }); + + it('falls back to getFileURL with user and tenant context when metadata lacks filepath', async () => { + const saveURL = jest.fn().mockResolvedValue({ + bytes: 256, + type: 'image/png', + }); + const getFileURL = jest + .fn() + .mockResolvedValue('https://cdn.example.com/t/tenant-a/images/user-123/image.png'); + getStrategyFunctions.mockReturnValue({ saveURL, getFileURL }); + + await processFileURL({ + fileStrategy: FileSources.cloudfront, + userId: 'user-123', + URL: 'https://example.com/image.png', + fileName: 'image.png', + basePath: 'images', + context: FileContext.image_generation, + tenantId: 'tenant-a', + }); + + expect(getFileURL).toHaveBeenCalledWith({ + userId: 'user-123', + fileName: 'image.png', + basePath: 'images', + tenantId: 'tenant-a', + }); + expect(db.createFile).toHaveBeenCalledWith( + expect.objectContaining({ + filepath: 'https://cdn.example.com/t/tenant-a/images/user-123/image.png', + tenantId: 'tenant-a', + }), + true, + ); + }); + + it('preserves the user path segment for local fallback URLs', async () => { + const saveURL = jest.fn().mockResolvedValue({ + bytes: 256, + type: 'image/png', + }); + const getFileURL = jest.fn().mockResolvedValue('/images/user-123/image.png'); + getStrategyFunctions.mockReturnValue({ saveURL, getFileURL }); + + await processFileURL({ + fileStrategy: FileSources.local, + userId: 'user-123', + URL: 'https://example.com/image.png', + fileName: 'image.png', + basePath: 'images', + context: FileContext.image_generation, + tenantId: 'tenant-a', + }); + + expect(getFileURL).toHaveBeenCalledWith({ + userId: 'user-123', + fileName: 'user-123/image.png', + basePath: 'images', + tenantId: 'tenant-a', + }); + expect(db.createFile).toHaveBeenCalledWith( + expect.objectContaining({ + filepath: '/images/user-123/image.png', + tenantId: 'tenant-a', + }), + true, + ); + }); +}); diff --git a/api/server/services/Files/strategies.js b/api/server/services/Files/strategies.js index 8a88e9d2e1..e5acbd6903 100644 --- a/api/server/services/Files/strategies.js +++ b/api/server/services/Files/strategies.js @@ -1,20 +1,22 @@ const { FileSources } = require('librechat-data-provider'); const { getS3URL, - saveURLToS3, + saveURLToS3WithMetadata, ImageService, parseDocument, uploadFileToS3, saveBufferToS3, getS3FileStream, + getS3DownloadURL, deleteFileFromS3, getCloudFrontURL, uploadMistralOCR, - saveURLToCloudFront, + saveURLToCloudFrontWithMetadata, uploadAzureMistralOCR, uploadFileToCloudFront, saveBufferToCloudFront, getCloudFrontFileStream, + getCloudFrontDownloadURL, deleteFileFromCloudFront, uploadGoogleVertexMistralOCR, } = require('@librechat/api'); @@ -111,7 +113,7 @@ const localStrategy = () => ({ * */ const s3Strategy = () => ({ handleFileUpload: uploadFileToS3, - saveURL: saveURLToS3, + saveURL: saveURLToS3WithMetadata, getFileURL: getS3URL, deleteFile: deleteFileFromS3, saveBuffer: saveBufferToS3, @@ -119,6 +121,7 @@ const s3Strategy = () => ({ processAvatar: processS3Avatar, handleImageUpload: uploadImageToS3, getDownloadStream: getS3FileStream, + getDownloadURL: getS3DownloadURL, }); /** @@ -127,7 +130,7 @@ const s3Strategy = () => ({ */ const cloudfrontStrategy = () => ({ handleFileUpload: uploadFileToCloudFront, - saveURL: saveURLToCloudFront, + saveURL: saveURLToCloudFrontWithMetadata, getFileURL: getCloudFrontURL, deleteFile: deleteFileFromCloudFront, saveBuffer: saveBufferToCloudFront, @@ -135,6 +138,7 @@ const cloudfrontStrategy = () => ({ processAvatar: processCloudFrontAvatar, handleImageUpload: uploadImageToCloudFront, getDownloadStream: getCloudFrontFileStream, + getDownloadURL: getCloudFrontDownloadURL, }); /** diff --git a/api/strategies/openidStrategy.js b/api/strategies/openidStrategy.js index 6d08fa1e59..595c2b535a 100644 --- a/api/strategies/openidStrategy.js +++ b/api/strategies/openidStrategy.js @@ -17,6 +17,8 @@ const { getOpenIdIssuer, getBalanceConfig, isEmailDomainAllowed, + getAvatarFileStrategy, + getAvatarSaveParams, resolveAppConfigForUser, } = require('@librechat/api'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); @@ -662,14 +664,16 @@ async function processOpenIDAuth(tokenset, existingUsersOnly = false) { userinfo.sub, ); if (imageBuffer) { - const { saveBuffer } = getStrategyFunctions( - appConfig?.fileStrategy ?? process.env.CDN_PROVIDER, + const fileStrategy = getAvatarFileStrategy(appConfig, process.env.CDN_PROVIDER); + const { saveBuffer } = getStrategyFunctions(fileStrategy); + const imagePath = await saveBuffer( + getAvatarSaveParams(fileStrategy, { + fileName, + userId: user._id.toString(), + buffer: imageBuffer, + tenantId: user.tenantId, + }), ); - const imagePath = await saveBuffer({ - fileName, - userId: user._id.toString(), - buffer: imageBuffer, - }); user.avatar = imagePath ?? ''; } } diff --git a/api/strategies/openidStrategy.spec.js b/api/strategies/openidStrategy.spec.js index 2812341e5c..15e507e9b5 100644 --- a/api/strategies/openidStrategy.spec.js +++ b/api/strategies/openidStrategy.spec.js @@ -1,7 +1,7 @@ const undici = require('undici'); const fetch = require('node-fetch'); const jwtDecode = require('jsonwebtoken/decode'); -const { ErrorTypes } = require('librechat-data-provider'); +const { ErrorTypes, FileSources } = require('librechat-data-provider'); const { findUser, createUser, updateUser } = require('~/models'); const { getOpenIdIssuer, resolveAppConfigForUser } = require('@librechat/api'); const { getAppConfig } = require('~/server/services/Config'); @@ -1097,15 +1097,50 @@ describe('setupOpenId', () => { }); it('should attempt to download and save the avatar if picture is provided', async () => { + const { getStrategyFunctions } = require('~/server/services/Files/strategies'); + // Act const { user } = await validate(tokenset); + const strategyResult = + getStrategyFunctions.mock.results[getStrategyFunctions.mock.results.length - 1]; + const { saveBuffer } = strategyResult.value; + const [saveParams] = saveBuffer.mock.calls[0]; // Assert – verify that download was attempted and the avatar field was set via updateUser expect(fetch).toHaveBeenCalled(); + expect(saveParams).toEqual( + expect.objectContaining({ + fileName: 'hashed-token.png', + userId: 'newUserId', + buffer: expect.any(Buffer), + }), + ); + expect(saveParams).not.toHaveProperty('basePath'); // Our mock getStrategyFunctions.saveBuffer returns '/fake/path/to/avatar.png' expect(user.avatar).toBe('/fake/path/to/avatar.png'); }); + it('should save CloudFront IdP avatars under the shared avatar prefix', async () => { + const { getStrategyFunctions } = require('~/server/services/Files/strategies'); + getAppConfig.mockResolvedValueOnce({ fileStrategy: FileSources.cloudfront }); + + const { user } = await validate(tokenset); + const strategyResult = + getStrategyFunctions.mock.results[getStrategyFunctions.mock.results.length - 1]; + const { saveBuffer } = strategyResult.value; + const [saveParams] = saveBuffer.mock.calls[0]; + + expect(getStrategyFunctions).toHaveBeenLastCalledWith(FileSources.cloudfront); + expect(saveParams).toEqual( + expect.objectContaining({ + basePath: 'avatars', + fileName: 'hashed-token.png', + userId: 'newUserId', + }), + ); + expect(user.avatar).toBe('/fake/path/to/avatar.png'); + }); + it('should not attempt to download avatar if picture is not provided', async () => { // Arrange – remove picture const userinfo = { ...tokenset.claims() }; diff --git a/api/strategies/samlStrategy.js b/api/strategies/samlStrategy.js index 4f4bfac158..b21a4a482b 100644 --- a/api/strategies/samlStrategy.js +++ b/api/strategies/samlStrategy.js @@ -8,6 +8,8 @@ const { Strategy: SamlStrategy } = require('@node-saml/passport-saml'); const { getBalanceConfig, isEmailDomainAllowed, + getAvatarFileStrategy, + getAvatarSaveParams, resolveAppConfigForUser, } = require('@librechat/api'); const { getStrategyFunctions } = require('~/server/services/Files/strategies'); @@ -271,14 +273,16 @@ function createSamlCallback(existingUsersOnly = false) { fileName = profile.nameID + '.png'; } - const { saveBuffer } = getStrategyFunctions( - appConfig?.fileStrategy ?? process.env.CDN_PROVIDER, + const fileStrategy = getAvatarFileStrategy(appConfig, process.env.CDN_PROVIDER); + const { saveBuffer } = getStrategyFunctions(fileStrategy); + const imagePath = await saveBuffer( + getAvatarSaveParams(fileStrategy, { + fileName, + userId: user._id.toString(), + buffer: imageBuffer, + tenantId: user.tenantId, + }), ); - const imagePath = await saveBuffer({ - fileName, - userId: user._id.toString(), - buffer: imageBuffer, - }); user.avatar = imagePath ?? ''; } } diff --git a/api/strategies/samlStrategy.spec.js b/api/strategies/samlStrategy.spec.js index 965fb157ef..dbf003e58c 100644 --- a/api/strategies/samlStrategy.spec.js +++ b/api/strategies/samlStrategy.spec.js @@ -30,6 +30,19 @@ jest.mock('@librechat/api', () => ({ tokenCredits: 1000, startBalance: 1000, })), + getAvatarFileStrategy: jest.fn((config, fallbackStrategy) => { + const { FileSources } = jest.requireActual('librechat-data-provider'); + if (config?.fileStrategies) { + return config.fileStrategies.avatar ?? config.fileStrategies.default ?? config.fileStrategy; + } + return config?.fileStrategy ?? fallbackStrategy ?? FileSources.local; + }), + getAvatarSaveParams: jest.fn((strategy, params) => { + const { FileSources } = jest.requireActual('librechat-data-provider'); + return strategy === FileSources.s3 || strategy === FileSources.cloudfront + ? { ...params, basePath: 'avatars' } + : params; + }), resolveAppConfigForUser: jest.fn(async (_getAppConfig, _user) => ({})), })); jest.mock('~/server/services/Config/EndpointService', () => ({ @@ -48,6 +61,7 @@ const fs = require('fs'); const path = require('path'); const fetch = require('node-fetch'); const { Strategy: SamlStrategy } = require('@node-saml/passport-saml'); +const { FileSources } = require('librechat-data-provider'); const { findUser } = require('~/models'); const { resolveAppConfigForUser } = require('@librechat/api'); const { getAppConfig } = require('~/server/services/Config'); @@ -434,11 +448,46 @@ u7wlOSk+oFzDIO/UILIA }); it('should attempt to download and save the avatar if picture is provided', async () => { + const { getStrategyFunctions } = require('~/server/services/Files/strategies'); const profile = { ...baseProfile }; const { user } = await validate(profile); + const strategyResult = + getStrategyFunctions.mock.results[getStrategyFunctions.mock.results.length - 1]; + const { saveBuffer } = strategyResult.value; + const [saveParams] = saveBuffer.mock.calls[0]; expect(fetch).toHaveBeenCalled(); + expect(saveParams).toEqual( + expect.objectContaining({ + fileName: 'hashed-token.png', + userId: 'mock-user-id', + buffer: expect.any(Buffer), + }), + ); + expect(saveParams).not.toHaveProperty('basePath'); + expect(user.avatar).toBe('/fake/path/to/avatar.png'); + }); + + it('should save CloudFront SAML avatars under the shared avatar prefix', async () => { + const { getStrategyFunctions } = require('~/server/services/Files/strategies'); + getAppConfig.mockResolvedValueOnce({ fileStrategies: { avatar: FileSources.cloudfront } }); + const profile = { ...baseProfile }; + + const { user } = await validate(profile); + const strategyResult = + getStrategyFunctions.mock.results[getStrategyFunctions.mock.results.length - 1]; + const { saveBuffer } = strategyResult.value; + const [saveParams] = saveBuffer.mock.calls[0]; + + expect(getStrategyFunctions).toHaveBeenLastCalledWith(FileSources.cloudfront); + expect(saveParams).toEqual( + expect.objectContaining({ + basePath: 'avatars', + fileName: 'hashed-token.png', + userId: 'mock-user-id', + }), + ); expect(user.avatar).toBe('/fake/path/to/avatar.png'); }); diff --git a/client/src/components/Chat/Messages/Content/FilePreviewDialog.tsx b/client/src/components/Chat/Messages/Content/FilePreviewDialog.tsx index c02e2fee4b..43e0abd7d6 100644 --- a/client/src/components/Chat/Messages/Content/FilePreviewDialog.tsx +++ b/client/src/components/Chat/Messages/Content/FilePreviewDialog.tsx @@ -5,7 +5,7 @@ import { Download } from 'lucide-react'; import { OGDialog, OGDialogContent, OGDialogTitle, OGDialogDescription } from '@librechat/client'; import CopyButton from '~/components/Messages/Content/CopyButton'; import { logger, sortPagesByRelevance } from '~/utils'; -import { useFileDownload } from '~/data-provider'; +import { revokeDownloadURL, useFileDownload } from '~/data-provider'; import { useLocalize } from '~/hooks'; import store from '~/store'; @@ -136,7 +136,7 @@ export default function FilePreviewDialog({ }: FilePreviewDialogProps) { const localize = useLocalize(); const user = useRecoilValue(store.user); - const { refetch: downloadFile } = useFileDownload(user?.id ?? '', fileId); + const { refetch: downloadFile } = useFileDownload(user?.id ?? '', fileId, { direct: false }); const [fileContent, setFileContent] = useState(null); const [fileBlobUrl, setFileBlobUrl] = useState(null); @@ -207,7 +207,7 @@ export default function FilePreviewDialog({ document.body.appendChild(a); a.click(); document.body.removeChild(a); - setTimeout(() => URL.revokeObjectURL(result.data), 1000); + setTimeout(() => revokeDownloadURL(result.data), 1000); } catch (err) { logger.error('[FilePreviewDialog] Download failed:', err); } diff --git a/client/src/components/Chat/Messages/Content/MarkdownComponents.tsx b/client/src/components/Chat/Messages/Content/MarkdownComponents.tsx index 1f2a3ff594..e98b648013 100644 --- a/client/src/components/Chat/Messages/Content/MarkdownComponents.tsx +++ b/client/src/components/Chat/Messages/Content/MarkdownComponents.tsx @@ -5,7 +5,7 @@ import { PermissionTypes, Permissions, apiBaseUrl } from 'librechat-data-provide import Mermaid, { MermaidErrorBoundary } from '~/components/Messages/Content/Mermaid'; import CodeBlock from '~/components/Messages/Content/CodeBlock'; import useHasAccess from '~/hooks/Roles/useHasAccess'; -import { useFileDownload } from '~/data-provider'; +import { revokeDownloadURL, useFileDownload } from '~/data-provider'; import { useCodeBlockContext } from '~/Providers'; import { handleDoubleClick } from '~/utils'; import { useLocalize } from '~/hooks'; @@ -127,7 +127,7 @@ export const a: React.ElementType = memo(function MarkdownAnchor({ href, childre return { file_id: '', filename: '', filepath: '' }; }, [user?.id, href]); - const { refetch: downloadFile } = useFileDownload(user?.id ?? '', file_id); + const { refetch: downloadFile } = useFileDownload(user?.id ?? '', file_id, { direct: false }); const props: { target?: string; onClick?: React.MouseEventHandler } = { target: '_blank' }; if (!file_id || !filename) { @@ -156,7 +156,7 @@ export const a: React.ElementType = memo(function MarkdownAnchor({ href, childre document.body.appendChild(link); link.click(); document.body.removeChild(link); - window.URL.revokeObjectURL(stream.data); + revokeDownloadURL(stream.data); } catch (error) { console.error('Error downloading file:', error); } diff --git a/client/src/components/Chat/Messages/Content/Parts/LogLink.tsx b/client/src/components/Chat/Messages/Content/Parts/LogLink.tsx index 070becf517..d45e3d38d4 100644 --- a/client/src/components/Chat/Messages/Content/Parts/LogLink.tsx +++ b/client/src/components/Chat/Messages/Content/Parts/LogLink.tsx @@ -1,7 +1,7 @@ import React from 'react'; import { FileSources } from 'librechat-data-provider'; import { useToastContext } from '@librechat/client'; -import { useCodeOutputDownload, useFileDownload } from '~/data-provider'; +import { revokeDownloadURL, useCodeOutputDownload, useFileDownload } from '~/data-provider'; interface LogLinkProps { href: string; @@ -29,9 +29,13 @@ const isLocallyStoredSource = (source?: string): boolean => { if (!source) { return false; } - return [FileSources.local, FileSources.firebase, FileSources.s3, FileSources.azure_blob].includes( - source as FileSources, - ); + return [ + FileSources.local, + FileSources.firebase, + FileSources.s3, + FileSources.cloudfront, + FileSources.azure_blob, + ].includes(source as FileSources); }; export const useAttachmentLink = ({ @@ -44,7 +48,7 @@ export const useAttachmentLink = ({ const { showToast } = useToastContext(); const useLocalDownload = isLocallyStoredSource(source) && !!file_id && !!user; - const { refetch: downloadFromApi } = useFileDownload(user, file_id); + const { refetch: downloadFromApi } = useFileDownload(user, file_id, { source }); const { refetch: downloadFromUrl } = useCodeOutputDownload(href); const handleDownload = async (event: React.MouseEvent) => { @@ -65,7 +69,7 @@ export const useAttachmentLink = ({ document.body.appendChild(link); link.click(); document.body.removeChild(link); - window.URL.revokeObjectURL(stream.data); + revokeDownloadURL(stream.data); } catch (error) { console.error('Error downloading file:', error); } diff --git a/client/src/components/Web/Sources.tsx b/client/src/components/Web/Sources.tsx index 97e646d5d0..06f8a576b0 100644 --- a/client/src/components/Web/Sources.tsx +++ b/client/src/components/Web/Sources.tsx @@ -16,7 +16,7 @@ import { import type { ValidSource, ImageResult } from 'librechat-data-provider'; import { FaviconImage, getCleanDomain } from '~/components/Web/SourceHovercard'; import SourcesErrorBoundary from './SourcesErrorBoundary'; -import { useFileDownload } from '~/data-provider'; +import { revokeDownloadURL, useFileDownload } from '~/data-provider'; import { useSearchContext } from '~/Providers'; import { useLocalize } from '~/hooks'; import { cn } from '~/utils'; @@ -212,7 +212,9 @@ const FileItem = React.memo(function FileItem({ const user = useRecoilValue(store.user); const { showToast } = useToastContext(); - const { refetch: downloadFile } = useFileDownload(user?.id ?? '', file.file_id); + const { refetch: downloadFile } = useFileDownload(user?.id ?? '', file.file_id, { + source: file.source, + }); // Extract error message logic to avoid duplication const getErrorMessage = useCallback( @@ -261,7 +263,7 @@ const FileItem = React.memo(function FileItem({ document.body.appendChild(link); link.click(); document.body.removeChild(link); - window.URL.revokeObjectURL(stream.data); + revokeDownloadURL(stream.data); } catch (error) { console.error('Error downloading file:', error); } diff --git a/client/src/data-provider/Files/__tests__/previewRefetchInterval.spec.ts b/client/src/data-provider/Files/__tests__/previewRefetchInterval.spec.ts index b23d8f996d..cc0fc2f22f 100644 --- a/client/src/data-provider/Files/__tests__/previewRefetchInterval.spec.ts +++ b/client/src/data-provider/Files/__tests__/previewRefetchInterval.spec.ts @@ -1,3 +1,4 @@ +import { FileSources } from 'librechat-data-provider'; import type { TFilePreview } from 'librechat-data-provider'; const mockGetFilePreview = jest.fn(); @@ -16,7 +17,9 @@ import { PREVIEW_MAX_CONSECUTIVE_ERRORS, _resetPreviewErrorCounter, fetchFilePreview, + isDirectDownloadSource, previewRefetchInterval, + revokeDownloadURL, } from '../queries'; const q = (fileId: string) => ({ queryKey: ['filePreview' as const, fileId] }); @@ -95,3 +98,26 @@ describe('previewRefetchInterval', () => { expect(previewRefetchInterval(undefined, q('fid-healthy'))).toBe(2500); }); }); + +describe('download URL helpers', () => { + it('uses direct download URLs only for strategies that implement them', () => { + expect(isDirectDownloadSource(FileSources.s3)).toBe(true); + expect(isDirectDownloadSource(FileSources.cloudfront)).toBe(true); + expect(isDirectDownloadSource(FileSources.local)).toBe(false); + expect(isDirectDownloadSource(FileSources.firebase)).toBe(false); + expect(isDirectDownloadSource(undefined)).toBe(false); + }); + + it('revokes only blob URLs', () => { + const originalRevokeObjectURL = window.URL.revokeObjectURL; + const revokeObjectURL = jest.fn(); + window.URL.revokeObjectURL = revokeObjectURL; + + revokeDownloadURL('https://cdn.example.com/file.pdf'); + revokeDownloadURL('blob:https://app.example.com/id'); + + expect(revokeObjectURL).toHaveBeenCalledTimes(1); + expect(revokeObjectURL).toHaveBeenCalledWith('blob:https://app.example.com/id'); + window.URL.revokeObjectURL = originalRevokeObjectURL; + }); +}); diff --git a/client/src/data-provider/Files/queries.ts b/client/src/data-provider/Files/queries.ts index 8502704676..db02bfb64b 100644 --- a/client/src/data-provider/Files/queries.ts +++ b/client/src/data-provider/Files/queries.ts @@ -1,6 +1,6 @@ import { useRecoilValue } from 'recoil'; import { useQuery, useQueryClient } from '@tanstack/react-query'; -import { QueryKeys, DynamicQueryKeys, dataService } from 'librechat-data-provider'; +import { FileSources, QueryKeys, DynamicQueryKeys, dataService } from 'librechat-data-provider'; import type { QueryObserverResult, UseQueryOptions } from '@tanstack/react-query'; import type t from 'librechat-data-provider'; import { isEphemeralAgent } from '~/common'; @@ -53,15 +53,45 @@ export const useGetFileConfig = ( ); }; -export const useFileDownload = (userId?: string, file_id?: string): QueryObserverResult => { +type FileDownloadOptions = { + source?: string | null; + direct?: boolean; +}; + +export const isDirectDownloadSource = (source?: string | null): boolean => + source === FileSources.s3 || source === FileSources.cloudfront; + +export const revokeDownloadURL = (url?: string | null): void => { + if (!url?.startsWith('blob:')) { + return; + } + window.URL.revokeObjectURL(url); +}; + +export const useFileDownload = ( + userId?: string, + file_id?: string, + options: FileDownloadOptions = {}, +): QueryObserverResult => { const queryClient = useQueryClient(); return useQuery( - [QueryKeys.fileDownload, file_id], + [QueryKeys.fileDownload, file_id, options.source ?? '', options.direct ?? true], async () => { if (!userId || !file_id) { console.warn('No user ID provided for file download'); return; } + if ((options.direct ?? true) && isDirectDownloadSource(options.source)) { + try { + const directDownload = await dataService.getFileDownloadURL(userId, file_id); + if (directDownload.url) { + return directDownload.url; + } + } catch { + // Fall back to the legacy proxied download for direct URL failures. + } + } + const response = await dataService.getFileDownload(userId, file_id); const blob = response.data; const downloadURL = window.URL.createObjectURL(blob); diff --git a/librechat.example.yaml b/librechat.example.yaml index dd275718a2..963189584c 100644 --- a/librechat.example.yaml +++ b/librechat.example.yaml @@ -29,17 +29,25 @@ cache: true # CloudFront CDN Configuration (optional) # Use when fileStrategy: "cloudfront" or fileStrategies includes cloudfront # Requires: AWS_REGION, AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, AWS_BUCKET_NAME -# For signed cookies/URLs: CLOUDFRONT_KEY_PAIR_ID, CLOUDFRONT_PRIVATE_KEY +# For signed cookies and direct download URLs: CLOUDFRONT_KEY_PAIR_ID, CLOUDFRONT_PRIVATE_KEY # cloudfront: # domain: "https://cdn.example.com" # CloudFront domain (CNAME recommended for cookies) # distributionId: "E1234ABCD" # Required if invalidateOnDelete is true # invalidateOnDelete: false # Create cache invalidation on file delete # imageSigning: "none" # "none" (public) | "cookies" (signed cookies) -# # When imageSigning: "cookies", API + CloudFront must share a parent domain: +# # When imageSigning: "cookies", API + CloudFront must share a parent domain. +# # Cookies are path-scoped to private image prefixes and avatar prefixes. +# # If adding tenantId to a private pre-release CloudFront deployment, re-key +# # legacy /images and /avatars objects under /t/{tenantId}/ before enabling. # # API: api.example.com, CloudFront CNAME: cdn.example.com, cookieDomain: ".example.com" # cookieDomain: ".example.com" # Required for "cookies" - shared parent domain # cookieExpiry: 1800 # Cookie lifetime in seconds (max: 604800 / 7 days, default: 1800 / 30 min) -# urlExpiry: 3600 # Reserved for future signed-URL mode (not yet implemented) +# urlExpiry: 3600 # Signed CloudFront download URL lifetime in seconds +# # Direct-download filename/content-type overrides require the CloudFront cache/origin +# # request policy to forward and cache on response-content-disposition and +# # response-content-type query strings to S3. +# # Recommended for download paths: attach a CloudFront response headers policy +# # with X-Content-Type-Options: nosniff and CSP default-src 'none'. # Custom interface configuration interface: diff --git a/packages/api/src/cdn/__tests__/cloudfront-cookies.test.ts b/packages/api/src/cdn/__tests__/cloudfront-cookies.test.ts index 73f0e25872..fcfd63d1f8 100644 --- a/packages/api/src/cdn/__tests__/cloudfront-cookies.test.ts +++ b/packages/api/src/cdn/__tests__/cloudfront-cookies.test.ts @@ -16,31 +16,43 @@ jest.mock('@librechat/data-schemas', () => ({ })); import type { Response } from 'express'; -import { setCloudFrontCookies, clearCloudFrontCookies } from '../cloudfront-cookies'; +import { + setCloudFrontCookies, + clearCloudFrontCookies, + parseCloudFrontCookieScope, +} from '../cloudfront-cookies'; const { logger: mockLogger } = jest.requireMock('@librechat/data-schemas') as { logger: { warn: jest.Mock; error: jest.Mock; info: jest.Mock; debug: jest.Mock }; }; +const defaultScope = { userId: 'user123' }; + describe('setCloudFrontCookies', () => { let mockRes: Partial; let cookieArgs: Array<[string, string, object]>; + let clearedCookies: Array<[string, object]>; beforeEach(() => { jest.clearAllMocks(); cookieArgs = []; + clearedCookies = []; mockRes = { cookie: jest.fn((name: string, value: string, options: object) => { cookieArgs.push([name, value, options]); return mockRes as Response; }) as unknown as Response['cookie'], + clearCookie: jest.fn((name: string, options: object) => { + clearedCookies.push([name, options]); + return mockRes as Response; + }) as unknown as Response['clearCookie'], }; }); it('returns false when CloudFront config is null', () => { mockGetCloudFrontConfig.mockReturnValue(null); - const result = setCloudFrontCookies(mockRes as Response); + const result = setCloudFrontCookies(mockRes as Response, defaultScope); expect(result).toBe(false); expect(mockRes.cookie).not.toHaveBeenCalled(); @@ -55,7 +67,7 @@ describe('setCloudFrontCookies', () => { keyPairId: 'K123', }); - const result = setCloudFrontCookies(mockRes as Response); + const result = setCloudFrontCookies(mockRes as Response, defaultScope); expect(result).toBe(false); expect(mockRes.cookie).not.toHaveBeenCalled(); @@ -70,7 +82,7 @@ describe('setCloudFrontCookies', () => { keyPairId: null, }); - const result = setCloudFrontCookies(mockRes as Response); + const result = setCloudFrontCookies(mockRes as Response, defaultScope); expect(result).toBe(false); expect(mockRes.cookie).not.toHaveBeenCalled(); @@ -85,7 +97,7 @@ describe('setCloudFrontCookies', () => { keyPairId: 'K123ABC', }); - const result = setCloudFrontCookies(mockRes as Response); + const result = setCloudFrontCookies(mockRes as Response, defaultScope); expect(result).toBe(false); expect(mockRes.cookie).not.toHaveBeenCalled(); @@ -107,7 +119,7 @@ describe('setCloudFrontCookies', () => { 'CloudFront-Key-Pair-Id': 'K123ABC', }); - const result = setCloudFrontCookies(mockRes as Response); + const result = setCloudFrontCookies(mockRes as Response, defaultScope); expect(result).toBe(true); expect(mockLogger.warn).not.toHaveBeenCalled(); @@ -116,7 +128,7 @@ describe('setCloudFrontCookies', () => { expect(isNaN((options as { expires: Date }).expires.getTime())).toBe(false); }); - it('sets three CloudFront cookies when enabled', () => { + it('sets separate CloudFront cookie sets for private images and avatars when enabled', () => { mockGetCloudFrontConfig.mockReturnValue({ domain: 'https://cdn.example.com', imageSigning: 'cookies', @@ -132,10 +144,11 @@ describe('setCloudFrontCookies', () => { 'CloudFront-Key-Pair-Id': 'K123ABC', }); - const result = setCloudFrontCookies(mockRes as Response); + const result = setCloudFrontCookies(mockRes as Response, defaultScope); expect(result).toBe(true); - expect(mockRes.cookie).toHaveBeenCalledTimes(3); + expect(mockRes.cookie).toHaveBeenCalledTimes(7); + expect(mockRes.clearCookie).toHaveBeenCalledTimes(6); const cookieNames = cookieArgs.map(([name]) => name); expect(cookieNames).toContain('CloudFront-Policy'); @@ -143,7 +156,7 @@ describe('setCloudFrontCookies', () => { expect(cookieNames).toContain('CloudFront-Key-Pair-Id'); }); - it('uses cookieDomain from config with path', () => { + it('uses cookieDomain from config with path-scoped cookies', () => { mockGetCloudFrontConfig.mockReturnValue({ domain: 'https://cdn.example.com', imageSigning: 'cookies', @@ -159,7 +172,7 @@ describe('setCloudFrontCookies', () => { 'CloudFront-Key-Pair-Id': 'K123ABC', }); - setCloudFrontCookies(mockRes as Response); + setCloudFrontCookies(mockRes as Response, defaultScope); const [, , options] = cookieArgs[0]; expect(options).toMatchObject({ @@ -167,11 +180,12 @@ describe('setCloudFrontCookies', () => { secure: true, sameSite: 'none', domain: '.example.com', - path: '/images', + path: '/images/user123', }); + expect(cookieArgs[3][2]).toMatchObject({ path: '/avatars' }); }); - it('builds correct custom policy for images resource', () => { + it('clears legacy image-wide and avatar cookie paths before setting scoped cookies', () => { mockGetCloudFrontConfig.mockReturnValue({ domain: 'https://cdn.example.com', imageSigning: 'cookies', @@ -187,15 +201,146 @@ describe('setCloudFrontCookies', () => { 'CloudFront-Key-Pair-Id': 'K123ABC', }); - setCloudFrontCookies(mockRes as Response); + setCloudFrontCookies(mockRes as Response, defaultScope); + expect(clearedCookies).toHaveLength(6); + expect(clearedCookies).toContainEqual([ + 'CloudFront-Policy', + expect.objectContaining({ path: '/images' }), + ]); + expect(clearedCookies).toContainEqual([ + 'CloudFront-Key-Pair-Id', + expect.objectContaining({ path: '/avatars' }), + ]); + }); + + it('clears the previously issued scoped cookie paths before setting new cookies', () => { + mockGetCloudFrontConfig.mockReturnValue({ + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieExpiry: 1800, + cookieDomain: '.example.com', + privateKey: '-----BEGIN RSA PRIVATE KEY-----\ntest\n-----END RSA PRIVATE KEY-----', + keyPairId: 'K123ABC', + }); + + mockGetSignedCookies.mockReturnValue({ + 'CloudFront-Policy': 'policy-value', + 'CloudFront-Signature': 'signature-value', + 'CloudFront-Key-Pair-Id': 'K123ABC', + }); + + setCloudFrontCookies( + mockRes as Response, + { userId: 'newUser', tenantId: 'newTenant' }, + { userId: 'oldUser', tenantId: 'oldTenant' }, + ); + + expect(clearedCookies).toContainEqual([ + 'CloudFront-Policy', + expect.objectContaining({ path: '/t/oldTenant/images/oldUser' }), + ]); + expect(clearedCookies).toContainEqual([ + 'CloudFront-Signature', + expect.objectContaining({ path: '/t/oldTenant/avatars' }), + ]); + }); + + it('stores the issued CloudFront cookie scope for later cleanup', () => { + mockGetCloudFrontConfig.mockReturnValue({ + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieExpiry: 1800, + cookieDomain: '.example.com', + privateKey: '-----BEGIN RSA PRIVATE KEY-----\ntest\n-----END RSA PRIVATE KEY-----', + keyPairId: 'K123ABC', + }); + + mockGetSignedCookies.mockReturnValue({ + 'CloudFront-Policy': 'policy-value', + 'CloudFront-Signature': 'signature-value', + 'CloudFront-Key-Pair-Id': 'K123ABC', + }); + + setCloudFrontCookies(mockRes as Response, { userId: 'user123', tenantId: 'tenantA' }); + + const [name, value, options] = cookieArgs[cookieArgs.length - 1]; + expect(name).toBe('LibreChat-CloudFront-Scope'); + expect(options).toMatchObject({ domain: '.example.com', path: '/' }); + expect(Buffer.from(value, 'base64url').toString('utf8')).toBe( + JSON.stringify({ userId: 'user123', tenantId: 'tenantA' }), + ); + }); + + it('builds user-scoped custom policies for private images and avatars', () => { + mockGetCloudFrontConfig.mockReturnValue({ + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieExpiry: 1800, + cookieDomain: '.example.com', + privateKey: '-----BEGIN RSA PRIVATE KEY-----\ntest\n-----END RSA PRIVATE KEY-----', + keyPairId: 'K123ABC', + }); + + mockGetSignedCookies.mockReturnValue({ + 'CloudFront-Policy': 'policy-value', + 'CloudFront-Signature': 'signature-value', + 'CloudFront-Key-Pair-Id': 'K123ABC', + }); + + setCloudFrontCookies(mockRes as Response, defaultScope); + + const privatePolicy = JSON.parse(mockGetSignedCookies.mock.calls[0][0].policy); + const avatarPolicy = JSON.parse(mockGetSignedCookies.mock.calls[1][0].policy); + expect(mockGetSignedCookies).toHaveBeenCalledTimes(2); expect(mockGetSignedCookies).toHaveBeenCalledWith( expect.objectContaining({ keyPairId: 'K123ABC', privateKey: expect.stringContaining('BEGIN RSA PRIVATE KEY'), - policy: expect.stringContaining('https://cdn.example.com/images/*'), }), ); + expect(privatePolicy.Statement).toEqual([ + expect.objectContaining({ Resource: 'https://cdn.example.com/images/user123/*' }), + ]); + expect(avatarPolicy.Statement).toEqual([ + expect.objectContaining({ Resource: 'https://cdn.example.com/avatars/*' }), + ]); + }); + + it('builds a tenant-scoped custom policy and cookie path', () => { + mockGetCloudFrontConfig.mockReturnValue({ + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieExpiry: 1800, + cookieDomain: '.example.com', + privateKey: '-----BEGIN RSA PRIVATE KEY-----\ntest\n-----END RSA PRIVATE KEY-----', + keyPairId: 'K123ABC', + }); + + mockGetSignedCookies.mockReturnValue({ + 'CloudFront-Policy': 'policy-value', + 'CloudFront-Signature': 'signature-value', + 'CloudFront-Key-Pair-Id': 'K123ABC', + }); + + const result = setCloudFrontCookies(mockRes as Response, { + userId: 'user123', + tenantId: 'tenantA', + }); + + const privatePolicy = JSON.parse(mockGetSignedCookies.mock.calls[0][0].policy); + const avatarPolicy = JSON.parse(mockGetSignedCookies.mock.calls[1][0].policy); + expect(result).toBe(true); + expect(privatePolicy.Statement).toEqual([ + expect.objectContaining({ + Resource: 'https://cdn.example.com/t/tenantA/images/user123/*', + }), + ]); + expect(avatarPolicy.Statement).toEqual([ + expect.objectContaining({ Resource: 'https://cdn.example.com/t/tenantA/avatars/*' }), + ]); + expect(cookieArgs[0][2]).toMatchObject({ path: '/t/tenantA/images/user123' }); + expect(cookieArgs[3][2]).toMatchObject({ path: '/t/tenantA/avatars' }); }); it('handles multiple trailing slashes in domain', () => { @@ -214,11 +359,11 @@ describe('setCloudFrontCookies', () => { 'CloudFront-Key-Pair-Id': 'K123ABC', }); - setCloudFrontCookies(mockRes as Response); + setCloudFrontCookies(mockRes as Response, defaultScope); expect(mockGetSignedCookies).toHaveBeenCalledWith( expect.objectContaining({ - policy: expect.stringContaining('https://cdn.example.com/images/*'), + policy: expect.stringContaining('https://cdn.example.com/images/user123/*'), }), ); }); @@ -235,10 +380,11 @@ describe('setCloudFrontCookies', () => { mockGetSignedCookies.mockReturnValue({}); - const result = setCloudFrontCookies(mockRes as Response); + const result = setCloudFrontCookies(mockRes as Response, defaultScope); expect(result).toBe(false); expect(mockRes.cookie).not.toHaveBeenCalled(); + expect(mockRes.clearCookie).not.toHaveBeenCalled(); expect(mockLogger.error).toHaveBeenCalledWith( expect.stringContaining('Missing expected cookie from AWS SDK'), ); @@ -256,9 +402,50 @@ describe('setCloudFrontCookies', () => { mockGetSignedCookies.mockReturnValue({ 'CloudFront-Policy': 'policy-value' }); + const result = setCloudFrontCookies(mockRes as Response, defaultScope); + + expect(result).toBe(false); + expect(mockRes.cookie).not.toHaveBeenCalled(); + expect(mockRes.clearCookie).not.toHaveBeenCalled(); + }); + + it('returns false when userId is missing from scope', () => { + mockGetCloudFrontConfig.mockReturnValue({ + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieExpiry: 1800, + cookieDomain: '.example.com', + privateKey: '-----BEGIN RSA PRIVATE KEY-----\ntest\n-----END RSA PRIVATE KEY-----', + keyPairId: 'K123ABC', + }); + const result = setCloudFrontCookies(mockRes as Response); expect(result).toBe(false); + expect(mockLogger.warn).toHaveBeenCalledWith( + '[setCloudFrontCookies] CloudFront configured but userId missing from scope', + ); + expect(mockRes.cookie).not.toHaveBeenCalled(); + }); + + it('returns false when scope path segments contain policy wildcards or traversal', () => { + mockGetCloudFrontConfig.mockReturnValue({ + domain: 'https://cdn.example.com', + imageSigning: 'cookies', + cookieExpiry: 1800, + cookieDomain: '.example.com', + privateKey: '-----BEGIN RSA PRIVATE KEY-----\ntest\n-----END RSA PRIVATE KEY-----', + keyPairId: 'K123ABC', + }); + + expect(setCloudFrontCookies(mockRes as Response, { userId: 'user*' })).toBe(false); + expect( + setCloudFrontCookies(mockRes as Response, { userId: 'user123', tenantId: '../tenantA' }), + ).toBe(false); + expect( + setCloudFrontCookies(mockRes as Response, { userId: 'user123', tenantId: 'tenant A' }), + ).toBe(false); + expect(mockGetSignedCookies).not.toHaveBeenCalled(); expect(mockRes.cookie).not.toHaveBeenCalled(); }); @@ -277,7 +464,7 @@ describe('setCloudFrontCookies', () => { throw signingError; }); - const result = setCloudFrontCookies(mockRes as Response); + const result = setCloudFrontCookies(mockRes as Response, defaultScope); expect(result).toBe(false); expect(mockRes.cookie).not.toHaveBeenCalled(); @@ -288,6 +475,36 @@ describe('setCloudFrontCookies', () => { }); }); +describe('parseCloudFrontCookieScope', () => { + const encodeScope = (scope: object) => + Buffer.from(JSON.stringify(scope), 'utf8').toString('base64url'); + + it('round-trips a valid user and tenant scope', () => { + const value = encodeScope({ userId: 'user123', tenantId: 'tenantA' }); + + expect(parseCloudFrontCookieScope(value)).toEqual({ + userId: 'user123', + tenantId: 'tenantA', + }); + }); + + it('returns null for empty, malformed, or userless values', () => { + expect(parseCloudFrontCookieScope(null)).toBeNull(); + expect(parseCloudFrontCookieScope(undefined)).toBeNull(); + expect(parseCloudFrontCookieScope('')).toBeNull(); + expect(parseCloudFrontCookieScope('not-json')).toBeNull(); + expect(parseCloudFrontCookieScope(encodeScope({ tenantId: 'tenantA' }))).toBeNull(); + }); + + it('rejects traversal and wildcard path segments', () => { + expect(parseCloudFrontCookieScope(encodeScope({ userId: '../user' }))).toBeNull(); + expect(parseCloudFrontCookieScope(encodeScope({ userId: 'user*' }))).toBeNull(); + expect( + parseCloudFrontCookieScope(encodeScope({ userId: 'user123', tenantId: 'tenant A' })), + ).toBeNull(); + }); +}); + describe('clearCloudFrontCookies', () => { let mockRes: Partial; let clearedCookies: Array<[string, object]>; @@ -311,7 +528,7 @@ describe('clearCloudFrontCookies', () => { expect(mockRes.clearCookie).not.toHaveBeenCalled(); }); - it('does nothing when imageSigning is not "cookies"', () => { + it('clears stale cookies when imageSigning is not "cookies"', () => { mockGetCloudFrontConfig.mockReturnValue({ domain: 'https://cdn.example.com', imageSigning: 'none', @@ -320,7 +537,7 @@ describe('clearCloudFrontCookies', () => { clearCloudFrontCookies(mockRes as Response); - expect(mockRes.clearCookie).not.toHaveBeenCalled(); + expect(mockRes.clearCookie).toHaveBeenCalledTimes(10); }); it('does nothing when cookieDomain is missing', () => { @@ -334,7 +551,7 @@ describe('clearCloudFrontCookies', () => { expect(mockRes.clearCookie).not.toHaveBeenCalled(); }); - it('clears all three CloudFront cookies with correct domain', () => { + it('clears all CloudFront cookies with correct domain and legacy paths', () => { mockGetCloudFrontConfig.mockReturnValue({ domain: 'https://cdn.example.com', imageSigning: 'cookies', @@ -345,21 +562,31 @@ describe('clearCloudFrontCookies', () => { clearCloudFrontCookies(mockRes as Response); - expect(mockRes.clearCookie).toHaveBeenCalledTimes(3); + expect(mockRes.clearCookie).toHaveBeenCalledTimes(10); - const expectedOptions = { + const legacyPathOptions = { domain: '.example.com', path: '/images', httpOnly: true, secure: true, sameSite: 'none', }; - expect(clearedCookies).toContainEqual(['CloudFront-Policy', expectedOptions]); - expect(clearedCookies).toContainEqual(['CloudFront-Signature', expectedOptions]); - expect(clearedCookies).toContainEqual(['CloudFront-Key-Pair-Id', expectedOptions]); + const rootPathOptions = { + domain: '.example.com', + path: '/', + httpOnly: true, + secure: true, + sameSite: 'none', + }; + expect(clearedCookies).toContainEqual(['CloudFront-Policy', legacyPathOptions]); + expect(clearedCookies).toContainEqual(['CloudFront-Signature', legacyPathOptions]); + expect(clearedCookies).toContainEqual(['CloudFront-Key-Pair-Id', legacyPathOptions]); + expect(clearedCookies).toContainEqual(['CloudFront-Policy', rootPathOptions]); + expect(clearedCookies).toContainEqual(['CloudFront-Signature', rootPathOptions]); + expect(clearedCookies).toContainEqual(['CloudFront-Key-Pair-Id', rootPathOptions]); }); - it('clears cookies with full security attributes matching set path', () => { + it('clears tenant-scoped cookies', () => { mockGetCloudFrontConfig.mockReturnValue({ domain: 'https://cdn.example.com', imageSigning: 'cookies', @@ -368,20 +595,29 @@ describe('clearCloudFrontCookies', () => { keyPairId: 'K123', }); - clearCloudFrontCookies(mockRes as Response); + clearCloudFrontCookies(mockRes as Response, { userId: 'user123', tenantId: 'tenantA' }); - expect(mockRes.clearCookie).toHaveBeenCalledTimes(3); - - const expectedOptions = { - domain: '.example.com', - path: '/images', - httpOnly: true, - secure: true, - sameSite: 'none', - }; - expect(clearedCookies).toContainEqual(['CloudFront-Policy', expectedOptions]); - expect(clearedCookies).toContainEqual(['CloudFront-Signature', expectedOptions]); - expect(clearedCookies).toContainEqual(['CloudFront-Key-Pair-Id', expectedOptions]); + expect(mockRes.clearCookie).toHaveBeenCalledTimes(19); + expect(clearedCookies).toContainEqual([ + 'CloudFront-Policy', + { + domain: '.example.com', + path: '/t/tenantA/images/user123', + httpOnly: true, + secure: true, + sameSite: 'none', + }, + ]); + expect(clearedCookies).toContainEqual([ + 'LibreChat-CloudFront-Scope', + { + domain: '.example.com', + path: '/', + httpOnly: true, + secure: true, + sameSite: 'none', + }, + ]); }); it('logs warning and does not throw when clearing fails', () => { diff --git a/packages/api/src/cdn/__tests__/s3.test.ts b/packages/api/src/cdn/__tests__/s3.test.ts index 9a522ecc4f..703ec40f86 100644 --- a/packages/api/src/cdn/__tests__/s3.test.ts +++ b/packages/api/src/cdn/__tests__/s3.test.ts @@ -62,6 +62,14 @@ describe('initializeS3', () => { ); }); + it('should only calculate request checksums when S3 requires them', async () => { + const { MockS3Client, initializeS3 } = await load(); + initializeS3(); + expect(MockS3Client).toHaveBeenCalledWith( + expect.objectContaining({ requestChecksumCalculation: 'WHEN_REQUIRED' }), + ); + }); + it('should not include endpoint when AWS_ENDPOINT_URL is not set', async () => { const { MockS3Client, initializeS3 } = await load(); initializeS3(); diff --git a/packages/api/src/cdn/cloudfront-cookies.ts b/packages/api/src/cdn/cloudfront-cookies.ts index 8ed23dc1d1..b62b5533fd 100644 --- a/packages/api/src/cdn/cloudfront-cookies.ts +++ b/packages/api/src/cdn/cloudfront-cookies.ts @@ -1,8 +1,9 @@ -import { logger } from '@librechat/data-schemas'; import { getSignedCookies } from '@aws-sdk/cloudfront-signer'; +import { logger } from '@librechat/data-schemas'; import type { Response } from 'express'; +import { assertPathSegment } from '~/storage/validation'; import { getCloudFrontConfig } from './cloudfront'; const DEFAULT_COOKIE_EXPIRY = 1800; @@ -13,26 +14,147 @@ const REQUIRED_CF_COOKIES = [ 'CloudFront-Key-Pair-Id', ] as const; +export const CLOUDFRONT_SCOPE_COOKIE = 'LibreChat-CloudFront-Scope'; +const unsafePolicySegmentPattern = /[?*[\]\s]/; + +export interface CloudFrontCookieScope { + userId?: string | null; + tenantId?: string | null; +} + +type CookieOptions = { + domain: string; + httpOnly: boolean; + secure: boolean; + sameSite: 'none'; +}; + +function assertPolicyPathSegment(label: string, value: string | null | undefined): string { + const segment = assertPathSegment(label, value, 'CloudFront cookies'); + if (unsafePolicySegmentPattern.test(segment)) { + throw new Error(`[CloudFront cookies] ${label} contains unsafe policy characters.`); + } + return segment; +} + +function getPolicyScopes( + domain: string, + { userId, tenantId }: CloudFrontCookieScope, +): Array<{ resource: string; path: string }> { + if (!userId) { + throw new Error('[CloudFront cookies] userId is required for private image access.'); + } + + const safeUserId = assertPolicyPathSegment('userId', userId); + if (tenantId) { + const safeTenantId = assertPolicyPathSegment('tenantId', tenantId); + return [ + { + resource: `${domain}/t/${safeTenantId}/images/${safeUserId}/*`, + path: `/t/${safeTenantId}/images/${safeUserId}`, + }, + { resource: `${domain}/t/${safeTenantId}/avatars/*`, path: `/t/${safeTenantId}/avatars` }, + ]; + } + + return [ + { resource: `${domain}/images/${safeUserId}/*`, path: `/images/${safeUserId}` }, + { resource: `${domain}/avatars/*`, path: '/avatars' }, + ]; +} + +function getScopeCookiePaths( + scope: CloudFrontCookieScope, + { includeTenantRoot = false }: { includeTenantRoot?: boolean } = {}, +): string[] { + if (!scope.userId) { + return []; + } + + const safeUserId = assertPolicyPathSegment('userId', scope.userId); + if (scope.tenantId) { + const safeTenantId = assertPolicyPathSegment('tenantId', scope.tenantId); + const paths = [`/t/${safeTenantId}/images/${safeUserId}`, `/t/${safeTenantId}/avatars`]; + if (includeTenantRoot) { + paths.push(`/t/${safeTenantId}`); + } + return paths; + } + + return [`/images/${safeUserId}`, '/avatars']; +} + +function encodeCloudFrontCookieScope(scope: CloudFrontCookieScope): string { + const payload = { + userId: scope.userId ?? null, + tenantId: scope.tenantId ?? null, + }; + return Buffer.from(JSON.stringify(payload), 'utf8').toString('base64url'); +} + +export function parseCloudFrontCookieScope( + value: string | null | undefined, +): CloudFrontCookieScope | null { + if (!value) { + return null; + } + + try { + const parsed = JSON.parse(Buffer.from(value, 'base64url').toString('utf8')) as { + userId?: unknown; + tenantId?: unknown; + }; + const scope: CloudFrontCookieScope = {}; + if (typeof parsed.userId === 'string') { + scope.userId = assertPolicyPathSegment('userId', parsed.userId); + } + if (typeof parsed.tenantId === 'string') { + scope.tenantId = assertPolicyPathSegment('tenantId', parsed.tenantId); + } + return scope.userId ? scope : null; + } catch { + return null; + } +} + +function clearCookiePaths( + res: Response, + baseOptions: CookieOptions, + paths: Iterable, +): void { + for (const path of paths) { + const options = { ...baseOptions, path }; + for (const key of REQUIRED_CF_COOKIES) { + res.clearCookie(key, options); + } + } +} + /** * Clears CloudFront signed cookies from the response. * Should be called during logout to revoke CDN access. */ -export function clearCloudFrontCookies(res: Response): void { +export function clearCloudFrontCookies(res: Response, scope: CloudFrontCookieScope = {}): void { try { const config = getCloudFrontConfig(); - if (!config?.cookieDomain || config.imageSigning !== 'cookies') { + if (!config?.cookieDomain) { return; } - const options = { + const baseOptions = { domain: config.cookieDomain, - path: '/images', httpOnly: true, secure: true, sameSite: 'none' as const, }; - res.clearCookie('CloudFront-Policy', options); - res.clearCookie('CloudFront-Signature', options); - res.clearCookie('CloudFront-Key-Pair-Id', options); + const paths = new Set(['/images', '/avatars', '/']); + if (scope.userId) { + for (const path of getScopeCookiePaths(scope, { includeTenantRoot: true })) { + paths.add(path); + } + } + + clearCookiePaths(res, baseOptions, paths); + res.clearCookie(CLOUDFRONT_SCOPE_COOKIE, { ...baseOptions, path: '/' }); } catch (error) { logger.warn('[clearCloudFrontCookies] Failed to clear cookies:', error); } @@ -42,65 +164,104 @@ export function clearCloudFrontCookies(res: Response): void { * Sets CloudFront signed cookies on the response for CDN access. * Returns true if cookies were set, false if CloudFront cookies are not enabled. */ -export function setCloudFrontCookies(res: Response): boolean { +export function setCloudFrontCookies( + res: Response, + scope: CloudFrontCookieScope = {}, + previousScope: CloudFrontCookieScope | null = null, +): boolean { const config = getCloudFrontConfig(); + if ( + config?.imageSigning === 'cookies' && + config.privateKey && + config.keyPairId && + config.cookieDomain && + !scope.userId + ) { + logger.warn('[setCloudFrontCookies] CloudFront configured but userId missing from scope'); + return false; + } if ( !config || config.imageSigning !== 'cookies' || !config.privateKey || !config.keyPairId || - !config.cookieDomain + !config.cookieDomain || + !scope.userId ) { return false; } try { + const { keyPairId, privateKey } = config; const cookieExpiry = config.cookieExpiry ?? DEFAULT_COOKIE_EXPIRY; const expiresAtMs = Date.now() + cookieExpiry * 1000; const expiresAt = new Date(expiresAtMs); const expiresAtEpoch = Math.floor(expiresAtMs / 1000); - const resourceUrl = `${config.domain.replace(/\/+$/, '')}/images/*`; - - const policy = JSON.stringify({ - Statement: [ - { - Resource: resourceUrl, - Condition: { - DateLessThan: { - 'AWS:EpochTime': expiresAtEpoch, + const cleanDomain = config.domain.replace(/\/+$/, ''); + const policyScopes = getPolicyScopes(cleanDomain, scope); + // CloudFront custom-policy cookies are scoped to one resource, so issue + // separate path-specific cookie sets for private files and shared avatars. + const signedCookieSets = policyScopes.map(({ resource, path }) => { + const policy = JSON.stringify({ + Statement: [ + { + Resource: resource, + Condition: { + DateLessThan: { + 'AWS:EpochTime': expiresAtEpoch, + }, }, }, - }, - ], + ], + }); + + return { + path, + cookies: getSignedCookies({ + keyPairId, + privateKey, + policy, + }), + }; }); - const signedCookies = getSignedCookies({ - keyPairId: config.keyPairId, - privateKey: config.privateKey, - policy, - }); - - const cookieOptions = { - expires: expiresAt, + const sharedCookieOptions = { httpOnly: true, secure: true, sameSite: 'none' as const, domain: config.cookieDomain, - path: '/images', }; - - for (const key of REQUIRED_CF_COOKIES) { - if (!signedCookies[key]) { - logger.error(`[setCloudFrontCookies] Missing expected cookie from AWS SDK: ${key}`); - return false; + const stalePaths = new Set(['/images', '/avatars']); + if (previousScope?.userId) { + for (const path of getScopeCookiePaths(previousScope)) { + stalePaths.add(path); } } - for (const key of REQUIRED_CF_COOKIES) { - res.cookie(key, signedCookies[key], cookieOptions); + for (const { cookies } of signedCookieSets) { + for (const key of REQUIRED_CF_COOKIES) { + if (!cookies[key]) { + logger.error(`[setCloudFrontCookies] Missing expected cookie from AWS SDK: ${key}`); + return false; + } + } } + clearCookiePaths(res, sharedCookieOptions, stalePaths); + const baseCookieOptions = { ...sharedCookieOptions, expires: expiresAt }; + + for (const { cookies, path } of signedCookieSets) { + const cookieOptions = { ...baseCookieOptions, path }; + for (const key of REQUIRED_CF_COOKIES) { + res.cookie(key, cookies[key], cookieOptions); + } + } + res.cookie(CLOUDFRONT_SCOPE_COOKIE, encodeCloudFrontCookieScope(scope), { + ...baseCookieOptions, + path: '/', + }); + return true; } catch (error) { logger.error('[setCloudFrontCookies] Failed to generate signed cookies:', error); diff --git a/packages/api/src/cdn/s3.ts b/packages/api/src/cdn/s3.ts index c2d0e4d1eb..475cf0aa69 100644 --- a/packages/api/src/cdn/s3.ts +++ b/packages/api/src/cdn/s3.ts @@ -39,6 +39,7 @@ export const initializeS3 = (): S3Client | null => { const config = { region, + requestChecksumCalculation: 'WHEN_REQUIRED' as const, ...(endpoint ? { endpoint } : {}), ...(isEnabled(process.env.AWS_FORCE_PATH_STYLE) ? { forcePathStyle: true } : {}), }; diff --git a/packages/api/src/files/code/form.ts b/packages/api/src/files/code/form.ts index 6f9abab7d4..707cf72182 100644 --- a/packages/api/src/files/code/form.ts +++ b/packages/api/src/files/code/form.ts @@ -8,9 +8,7 @@ export interface CodeEnvFileOptions { } const CODE_ENV_SAFE_ASCII_FILEPATH_CHAR_PATTERN = /^[a-zA-Z0-9._\-/]$/; -const CODE_ENV_UNSAFE_UNICODE_FILEPATH_CHAR_PATTERN = - /[^\p{L}\p{M}\p{N}\p{Emoji}\u200d._\-/]/u; -const CODE_ENV_FILENAME_CONTROL_CHARS_PATTERN = /[\x00-\x1f\x7f]/g; +const CODE_ENV_UNSAFE_UNICODE_FILEPATH_CHAR_PATTERN = /[^\p{L}\p{M}\p{N}\p{Emoji}\u200d._\-/]/u; function hasUnsafeCodeEnvFilepathChar(filepath: string): boolean { for (const char of filepath) { @@ -49,7 +47,10 @@ function getCodeEnvBasename(filepath: string): string { } function getSafeCodeEnvFilename(filename: string): string { - return filename.replace(CODE_ENV_FILENAME_CONTROL_CHARS_PATTERN, '_'); + return Array.from(filename, (char) => { + const code = char.charCodeAt(0); + return code <= 0x1f || code === 0x7f ? '_' : char; + }).join(''); } /** diff --git a/packages/api/src/skills/handlers.ts b/packages/api/src/skills/handlers.ts index 5b713d4596..81982186aa 100644 --- a/packages/api/src/skills/handlers.ts +++ b/packages/api/src/skills/handlers.ts @@ -31,7 +31,7 @@ import type { TSkillConflictResponse, TSkillFileContentResponse, } from 'librechat-data-provider'; -import type { ServerRequest } from '~/types/http'; +import type { ServerRequest, StrategyFunctions } from '~/types'; import { isBinaryBuffer } from './binary'; /** Thin error shape the skill methods throw on validation failure. */ @@ -107,10 +107,7 @@ export interface SkillsHandlersDeps { ) => Promise; /** Storage strategy resolver — returns stream/URL helpers keyed by source. */ - getStrategyFunctions: (source: string) => { - getDownloadStream?: (req: ServerRequest, filepath: string) => Promise; - [key: string]: unknown; - }; + getStrategyFunctions: (source: string) => Partial; /** ObjectId validation helper from data-schemas. */ isValidObjectIdString: (value: unknown) => boolean; @@ -288,19 +285,6 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps) { isValidObjectIdString, } = deps; - async function getPublicSkillIdSet(): Promise> { - try { - const publicIds = await findPubliclyAccessibleResources({ - resourceType: ResourceType.SKILL, - requiredPermissions: PermissionBits.VIEW, - }); - return new Set(publicIds.map((id) => id.toString())); - } catch (error) { - logger.error('[skills] Failed to fetch public skill IDs', error); - return new Set(); - } - } - /** O(1) public check for a single skill (avoids fetching all public IDs). */ async function isSkillPublic(skillId: string | Types.ObjectId): Promise { try { @@ -556,11 +540,13 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps) { // Fire-and-forget blob cleanup for each file for (const file of files) { - const { deleteFile: deleteBlob } = getStrategyFunctions(file.source) as { - deleteFile?: (r: ServerRequest, f: { filepath: string }) => Promise; - }; + const { deleteFile: deleteBlob } = getStrategyFunctions(file.source); if (deleteBlob) { - deleteBlob(req, { filepath: file.filepath }).catch((e) => + deleteBlob(req, { + filepath: file.filepath, + user: file.author?.toString?.(), + tenantId: file.tenantId?.toString?.(), + }).catch((e) => logger.error(`[deleteSkill] Blob cleanup failed for ${file.relativePath}:`, e), ); } @@ -759,13 +745,13 @@ export function createSkillsHandlers(deps: SkillsHandlersDeps) { } // Clean up the stored blob — fire-and-forget so the response isn't delayed - const { deleteFile: deleteBlob } = getStrategyFunctions(file.source) as { - deleteFile?: (req: ServerRequest, file: { filepath: string }) => Promise; - }; + const { deleteFile: deleteBlob } = getStrategyFunctions(file.source); if (deleteBlob) { - deleteBlob(req, { filepath: file.filepath }).catch((e) => - logger.error('[deleteFile] Storage cleanup failed:', e), - ); + deleteBlob(req, { + filepath: file.filepath, + user: file.author?.toString?.(), + tenantId: file.tenantId?.toString?.(), + }).catch((e) => logger.error('[deleteFile] Storage cleanup failed:', e)); } const response: TDeleteSkillFileResponse = { diff --git a/packages/api/src/skills/import.ts b/packages/api/src/skills/import.ts index f0223b8700..af6e0e3466 100644 --- a/packages/api/src/skills/import.ts +++ b/packages/api/src/skills/import.ts @@ -168,6 +168,7 @@ export interface ImportSkillDeps { fileName: string; basePath?: string; isImage?: boolean; + tenantId?: string; }, ) => Promise<{ filepath: string; source: string }>; deleteFile?: ( @@ -524,6 +525,7 @@ async function handleZip( fileName: storageFileName, basePath: 'uploads', isImage: mimeType.startsWith('image/'), + tenantId, }); // Upsert the SkillFile DB record (runs path validation internally). @@ -545,7 +547,7 @@ async function handleZip( } catch (dbError) { if (deps.deleteFile) { await deps - .deleteFile(req, { filepath, source }) + .deleteFile(req, { filepath, source, user: authorId, tenantId }) .catch((e) => logger.error(`[importSkill] Orphan cleanup failed for ${relativePath}:`, e), ); diff --git a/packages/api/src/storage/__tests__/images.test.ts b/packages/api/src/storage/__tests__/images.test.ts index 9d151a1044..ec8f71910e 100644 --- a/packages/api/src/storage/__tests__/images.test.ts +++ b/packages/api/src/storage/__tests__/images.test.ts @@ -89,6 +89,7 @@ describe('ImageService', () => { buffer: expect.any(Buffer), fileName: expect.stringContaining('file-456__'), basePath: 'images', + tenantId: null, }); expect(fs.promises.unlink).toHaveBeenCalledWith('/tmp/upload-123.jpg'); @@ -233,7 +234,8 @@ describe('ImageService', () => { userId: 'user123', buffer, fileName: expect.stringMatching(/^avatar-\d+\.png$/), - basePath: 'images', + basePath: 'avatars', + tenantId: null, }); expect(mockDeps.updateUser).toHaveBeenCalledWith('user123', { avatar: 'https://storage.example.com/images/user123/file.webp', @@ -270,6 +272,24 @@ describe('ImageService', () => { expect(mockDeps.updateUser).not.toHaveBeenCalled(); }); + it('passes tenantId through for avatar storage', async () => { + const buffer = Buffer.from('avatar-data'); + + await service.processAvatar({ + buffer, + userId: 'user123', + manual: 'false', + tenantId: 'tenantA', + }); + + expect(mockSaveBuffer).toHaveBeenCalledWith( + expect.objectContaining({ + basePath: 'avatars', + tenantId: 'tenantA', + }), + ); + }); + it('appends manual param when config.appendManualParam is true', async () => { const serviceWithManualParam = new ImageService(mockSaveBuffer, mockDeps, { appendManualParam: true, diff --git a/packages/api/src/storage/__tests__/validation.test.ts b/packages/api/src/storage/__tests__/validation.test.ts new file mode 100644 index 0000000000..dafb172ac2 --- /dev/null +++ b/packages/api/src/storage/__tests__/validation.test.ts @@ -0,0 +1,69 @@ +import { + assertS3FileName, + assertPathSegment, + sanitizeContentDispositionFilename, +} from '../validation'; + +describe('assertPathSegment', () => { + it('returns safe single path segments', () => { + expect(assertPathSegment('userId', 'user123', 'test')).toBe('user123'); + }); + + it('rejects empty, slash, traversal, and control-character segments', () => { + expect(() => assertPathSegment('userId', '', 'test')).toThrow('must not be empty'); + expect(() => assertPathSegment('userId', null, 'test')).toThrow('must not be empty'); + expect(() => assertPathSegment('userId', undefined, 'test')).toThrow('must not be empty'); + expect(() => assertPathSegment('userId', 'user/123', 'test')).toThrow( + 'must not contain slashes', + ); + expect(() => assertPathSegment('userId', '..', 'test')).toThrow( + 'must not contain path traversal', + ); + expect(assertPathSegment('tenantId', 'tenant..legacy', 'test')).toBe('tenant..legacy'); + expect(() => assertPathSegment('userId', 'user\u0000id', 'test')).toThrow( + 'contains unsafe path characters', + ); + expect(() => assertPathSegment('userId', 'user\u007fid', 'test')).toThrow( + 'contains unsafe path characters', + ); + }); +}); + +describe('assertS3FileName', () => { + it('allows nested S3 file names', () => { + expect(assertS3FileName('fileName', 'reports/2026/output.csv', 'test')).toBe( + 'reports/2026/output.csv', + ); + }); + + it('rejects traversal, empty components, backslashes, and control characters', () => { + expect(() => assertS3FileName('fileName', '../secret.txt', 'test')).toThrow( + 'must not contain path traversal', + ); + expect(() => assertS3FileName('fileName', 'reports//output.csv', 'test')).toThrow( + 'must not contain empty path components', + ); + expect(() => assertS3FileName('fileName', 'reports\\output.csv', 'test')).toThrow( + 'must not contain backslashes', + ); + expect(() => assertS3FileName('fileName', 'report\u0000.csv', 'test')).toThrow( + 'contains unsafe path characters', + ); + }); +}); + +describe('sanitizeContentDispositionFilename', () => { + it('strips quoted-string and header separator characters', () => { + expect(sanitizeContentDispositionFilename('report";\\\r\nbad.pdf')).toBe('reportbad.pdf'); + }); + + it('strips all ASCII control characters from header filenames', () => { + expect(sanitizeContentDispositionFilename('report\u0000\t\u001fbad\u007f.pdf')).toBe( + 'reportbad.pdf', + ); + }); + + it('returns a safe fallback when all filename characters are stripped', () => { + expect(sanitizeContentDispositionFilename('";\\\r\n')).toBe('download'); + }); +}); diff --git a/packages/api/src/storage/avatar.ts b/packages/api/src/storage/avatar.ts new file mode 100644 index 0000000000..3ab9f0fc76 --- /dev/null +++ b/packages/api/src/storage/avatar.ts @@ -0,0 +1,54 @@ +import { FileSources } from 'librechat-data-provider'; + +import type { SaveBufferParams } from './types'; + +import { AVATAR_BASE_PATH } from './constants'; + +type AvatarConfig = + | { + fileStrategy?: string | null; + fileStrategies?: { + avatar?: string | null; + default?: string | null; + } | null; + } + | null + | undefined; + +const sharedAvatarBasePathStrategies = new Set([FileSources.s3, FileSources.cloudfront]); + +/** + * Resolves the storage strategy used for avatars. `fallbackStrategy` is usually + * `process.env.CDN_PROVIDER`; undefined is valid and falls back to local storage. + */ +export function getAvatarFileStrategy( + appConfig: AvatarConfig, + fallbackStrategy?: string | null, +): string { + const config: AvatarConfig = + appConfig?.fileStrategy || appConfig?.fileStrategies + ? appConfig + : { fileStrategy: fallbackStrategy }; + + if (!config?.fileStrategies) { + return config?.fileStrategy ?? FileSources.local; + } + + return ( + config.fileStrategies.avatar ?? + config.fileStrategies.default ?? + config.fileStrategy ?? + FileSources.local + ); +} + +export function getAvatarSaveParams( + fileStrategy: string, + params: T, +): T { + if (!sharedAvatarBasePathStrategies.has(fileStrategy)) { + return params; + } + + return { ...params, basePath: AVATAR_BASE_PATH }; +} diff --git a/packages/api/src/storage/cloudfront/__tests__/crud.test.ts b/packages/api/src/storage/cloudfront/__tests__/crud.test.ts index e5da63e836..163b191751 100644 --- a/packages/api/src/storage/cloudfront/__tests__/crud.test.ts +++ b/packages/api/src/storage/cloudfront/__tests__/crud.test.ts @@ -4,9 +4,9 @@ import type { CloudFrontFullConfig } from '~/cdn/cloudfront'; import type { ServerRequest } from '~/types'; const mockGetCloudFrontConfig = jest.fn(); -const mockGetS3Key = jest.fn(); +const mockGetS3Key = jest.fn(); const mockSaveBufferToS3 = jest.fn(); -const mockSaveURLToS3 = jest.fn(); +const mockSaveURLToS3WithMetadata = jest.fn(); const mockUploadFileToS3 = jest.fn(); const mockDeleteFileFromS3 = jest.fn(); const mockGetS3FileStream = jest.fn(); @@ -22,7 +22,7 @@ jest.mock('~/cdn/cloudfront', () => ({ jest.mock('~/storage/s3/crud', () => ({ getS3Key: mockGetS3Key, saveBufferToS3: mockSaveBufferToS3, - saveURLToS3: mockSaveURLToS3, + saveURLToS3WithMetadata: mockSaveURLToS3WithMetadata, uploadFileToS3: mockUploadFileToS3, deleteFileFromS3: mockDeleteFileFromS3, getS3FileStream: mockGetS3FileStream, @@ -60,8 +60,10 @@ function makeConfig(overrides: Partial = {}): CloudFrontFu describe('CloudFront CRUD', () => { beforeEach(() => { jest.clearAllMocks(); - mockGetS3Key.mockImplementation( - (basePath, userId, fileName) => `${basePath}/${userId}/${fileName}`, + mockGetS3Key.mockImplementation((basePath, userId, fileName, tenantId) => + tenantId + ? `t/${tenantId}/${basePath}/${userId}/${fileName}` + : `${basePath}/${userId}/${fileName}`, ); mockGetCloudFrontConfig.mockReturnValue(makeConfig()); }); @@ -84,6 +86,18 @@ describe('CloudFront CRUD', () => { expect(url).toBe('https://d123.cloudfront.net/documents/user1/doc.pdf'); }); + it('uses tenant-prefixed keys when tenantId is provided', async () => { + const { getCloudFrontURL } = await import('~/storage/cloudfront/crud'); + const url = await getCloudFrontURL({ + userId: 'user1', + fileName: 'doc.pdf', + basePath: 'documents', + tenantId: 'tenantA', + }); + expect(url).toBe('https://d123.cloudfront.net/t/tenantA/documents/user1/doc.pdf'); + expect(mockGetS3Key).toHaveBeenCalledWith('documents', 'user1', 'doc.pdf', 'tenantA'); + }); + it('strips trailing slash from domain', async () => { mockGetCloudFrontConfig.mockReturnValue( makeConfig({ domain: 'https://d123.cloudfront.net/' }), @@ -258,8 +272,14 @@ describe('CloudFront CRUD', () => { }); describe('saveURLToCloudFront', () => { - it('delegates to saveURLToS3 with a urlBuilder', async () => { - mockSaveURLToS3.mockResolvedValue('https://d123.cloudfront.net/images/u/f.webp'); + it('returns the saved filepath for public API compatibility', async () => { + const savedFile = { + filepath: 'https://d123.cloudfront.net/images/u/f.webp', + bytes: 128, + type: 'image/webp', + dimensions: {}, + }; + mockSaveURLToS3WithMetadata.mockResolvedValue(savedFile); const { saveURLToCloudFront } = await import('~/storage/cloudfront/crud'); const result = await saveURLToCloudFront({ userId: 'u', @@ -267,7 +287,7 @@ describe('CloudFront CRUD', () => { fileName: 'f.webp', }); - expect(mockSaveURLToS3).toHaveBeenCalledWith( + expect(mockSaveURLToS3WithMetadata).toHaveBeenCalledWith( expect.objectContaining({ userId: 'u', URL: 'https://external.com/image.jpg', @@ -275,7 +295,25 @@ describe('CloudFront CRUD', () => { urlBuilder: expect.any(Function), }), ); - expect(result).toBe('https://d123.cloudfront.net/images/u/f.webp'); + expect(result).toBe(savedFile.filepath); + }); + + it('returns metadata from the explicit metadata variant', async () => { + const savedFile = { + filepath: 'https://d123.cloudfront.net/images/u/f.webp', + bytes: 128, + type: 'image/webp', + dimensions: {}, + }; + mockSaveURLToS3WithMetadata.mockResolvedValue(savedFile); + const { saveURLToCloudFrontWithMetadata } = await import('~/storage/cloudfront/crud'); + const result = await saveURLToCloudFrontWithMetadata({ + userId: 'u', + URL: 'https://external.com/image.jpg', + fileName: 'f.webp', + }); + + expect(result).toBe(savedFile); }); }); @@ -437,4 +475,93 @@ describe('CloudFront CRUD', () => { expect(result).toBe(readable); }); }); + + describe('getCloudFrontDownloadURL', () => { + it('returns a signed CloudFront URL for an existing file path', async () => { + mockGetCloudFrontConfig.mockReturnValue( + makeConfig({ privateKey: 'pk-secret', keyPairId: 'K123' }), + ); + mockExtractKeyFromS3Url.mockReturnValue('t/tenantA/uploads/user1/doc.pdf'); + mockGetSignedUrl.mockReturnValue( + 'https://d123.cloudfront.net/t/tenantA/uploads/user1/doc.pdf?Policy=abc', + ); + + const { getCloudFrontDownloadURL } = await import('~/storage/cloudfront/crud'); + const result = await getCloudFrontDownloadURL({ + req: { user: { id: 'user1', tenantId: 'tenantA' } } as ServerRequest, + file: { + filepath: 'https://d123.cloudfront.net/t/tenantA/uploads/user1/doc.pdf', + } as TFile, + }); + + expect(result).toContain('Policy=abc'); + expect(mockExtractKeyFromS3Url).toHaveBeenCalledWith( + 'https://d123.cloudfront.net/t/tenantA/uploads/user1/doc.pdf', + ); + expect(mockGetSignedUrl).toHaveBeenCalledWith( + expect.objectContaining({ + url: 'https://d123.cloudfront.net/t/tenantA/uploads/user1/doc.pdf', + keyPairId: 'K123', + privateKey: 'pk-secret', + }), + ); + }); + + it('includes response header overrides before signing download URLs', async () => { + mockGetCloudFrontConfig.mockReturnValue( + makeConfig({ privateKey: 'pk-secret', keyPairId: 'K123' }), + ); + mockExtractKeyFromS3Url.mockReturnValue('uploads/user1/report.pdf'); + mockGetSignedUrl.mockReturnValue('signed-url'); + + const { getCloudFrontDownloadURL } = await import('~/storage/cloudfront/crud'); + const result = await getCloudFrontDownloadURL({ + file: { filepath: 'https://d123.cloudfront.net/uploads/user1/report.pdf' } as TFile, + customFilename: 'report";\\bad.pdf', + contentType: 'application/pdf', + }); + + expect(result).toBe('signed-url'); + const signedInputUrl = new URL((mockGetSignedUrl.mock.calls[0][0] as { url: string }).url); + expect(signedInputUrl.searchParams.get('response-content-disposition')).toBe( + 'attachment; filename="reportbad.pdf"', + ); + expect(mockGetSignedUrl).toHaveBeenCalledWith( + expect.objectContaining({ + url: expect.stringContaining('response-content-disposition=attachment'), + }), + ); + expect(mockGetSignedUrl).toHaveBeenCalledWith( + expect.objectContaining({ + url: expect.stringContaining('response-content-type=application%2Fpdf'), + }), + ); + const signingInput = mockGetSignedUrl.mock.calls[0][0] as { policy: string }; + const policy = JSON.parse(signingInput.policy) as { + Statement: Array<{ + Resource: string; + Condition: { DateLessThan: { 'AWS:EpochTime': number } }; + }>; + }; + expect(policy.Statement[0].Resource).toBe( + 'https://d123.cloudfront.net/uploads/user1/report.pdf?*', + ); + expect(policy.Statement[0].Condition.DateLessThan['AWS:EpochTime']).toEqual( + expect.any(Number), + ); + }); + + it('throws when signing keys are missing', async () => { + mockGetCloudFrontConfig.mockReturnValue(makeConfig({ privateKey: null, keyPairId: null })); + mockExtractKeyFromS3Url.mockReturnValue('uploads/user1/doc.pdf'); + + const { getCloudFrontDownloadURL } = await import('~/storage/cloudfront/crud'); + await expect( + getCloudFrontDownloadURL({ + req: { user: { id: 'user1' } } as ServerRequest, + file: { filepath: 'https://d123.cloudfront.net/uploads/user1/doc.pdf' } as TFile, + }), + ).rejects.toThrow('Signing keys not configured'); + }); + }); }); diff --git a/packages/api/src/storage/cloudfront/crud.ts b/packages/api/src/storage/cloudfront/crud.ts index 37e41f5fa4..074ed3e5df 100644 --- a/packages/api/src/storage/cloudfront/crud.ts +++ b/packages/api/src/storage/cloudfront/crud.ts @@ -1,7 +1,7 @@ import crypto from 'crypto'; -import { logger } from '@librechat/data-schemas'; import { getSignedUrl } from '@aws-sdk/cloudfront-signer'; import { CloudFrontClient, CreateInvalidationCommand } from '@aws-sdk/client-cloudfront'; +import { logger } from '@librechat/data-schemas'; import type { TFile } from 'librechat-data-provider'; import type { Readable } from 'stream'; import type { ServerRequest } from '~/types'; @@ -10,15 +10,18 @@ import type { GetURLParams, SaveURLParams, UploadFileParams, + DownloadURLParams, + SaveURLResult, UploadResult, } from '~/storage/types'; import { getCloudFrontConfig } from '~/cdn/cloudfront'; import { s3Config } from '~/storage/s3/s3Config'; import { DEFAULT_BASE_PATH as defaultBasePath } from '~/storage/constants'; +import { sanitizeContentDispositionFilename } from '~/storage/validation'; import { getS3Key, saveBufferToS3, - saveURLToS3, + saveURLToS3WithMetadata, uploadFileToS3, deleteFileFromS3, getS3FileStream, @@ -52,23 +55,69 @@ function buildCloudFrontUrl(s3Key: string): string { return `${cleanDomain}/${cleanKey}`; } -function signUrl(url: string): string { +function signUrl(url: string | URL): string { const config = getCloudFrontConfig(); if (!config?.privateKey || !config?.keyPairId) { throw new Error('[signUrl] Signing keys not configured.'); } const expiry = config.urlExpiry ?? s3Config.S3_URL_EXPIRY_SECONDS; - const dateLessThan = new Date(Date.now() + expiry * 1000).toISOString(); + const expiresAtMs = Date.now() + expiry * 1000; + const expiresAtEpoch = Math.floor(expiresAtMs / 1000); + + const urlString = url.toString(); + const parsedUrl = url instanceof URL ? url : new URL(urlString); + if (parsedUrl.search) { + const policy = JSON.stringify({ + Statement: [ + { + Resource: `${parsedUrl.origin}${parsedUrl.pathname}?*`, + Condition: { + DateLessThan: { + 'AWS:EpochTime': expiresAtEpoch, + }, + }, + }, + ], + }); + + return getSignedUrl({ + url: urlString, + keyPairId: config.keyPairId, + privateKey: config.privateKey, + policy, + }); + } return getSignedUrl({ - url, + url: urlString, keyPairId: config.keyPairId, privateKey: config.privateKey, - dateLessThan, + dateLessThan: new Date(expiresAtMs).toISOString(), }); } +function appendDownloadOverrides( + url: string, + customFilename: string | null, + contentType: string | null, +): URL { + const downloadUrl = new URL(url); + + if (customFilename) { + const safeFilename = sanitizeContentDispositionFilename(customFilename); + downloadUrl.searchParams.set( + 'response-content-disposition', + `attachment; filename="${safeFilename}"`, + ); + } + if (contentType) { + downloadUrl.searchParams.set('response-content-type', contentType); + } + + return downloadUrl; +} + /** * Get CloudFront URL for a file. * @param sign - If true, returns a signed URL. Caller (strategy) decides based on config. @@ -77,9 +126,10 @@ export async function getCloudFrontURL({ userId, fileName, basePath = defaultBasePath, + tenantId = null, sign = false, }: CloudFrontURLParams): Promise { - const key = getS3Key(basePath, userId, fileName); + const key = getS3Key(basePath, userId, fileName, tenantId); const url = buildCloudFrontUrl(key); return sign ? signUrl(url) : url; } @@ -96,8 +146,19 @@ export async function saveBufferToCloudFront( export async function saveURLToCloudFront( params: SaveURLParams & { sign?: boolean }, ): Promise { + const { filepath } = await saveURLToCloudFrontWithMetadata(params); + return filepath; +} + +/** Save file from URL to S3 and return CloudFront URL with fetched metadata. */ +export async function saveURLToCloudFrontWithMetadata( + params: SaveURLParams & { sign?: boolean }, +): Promise { const { sign = false, ...rest } = params; - return saveURLToS3({ ...rest, urlBuilder: (p) => getCloudFrontURL({ ...p, sign }) }); + return saveURLToS3WithMetadata({ + ...rest, + urlBuilder: (p) => getCloudFrontURL({ ...p, sign }), + }); } /** Upload file to S3 and return CloudFront URL. */ @@ -147,3 +208,17 @@ export async function getCloudFrontFileStream( ): Promise { return getS3FileStream(req, filePath); } + +/** Get a signed CloudFront URL for an authorized file download. */ +export async function getCloudFrontDownloadURL({ + file, + customFilename = null, + contentType = null, +}: DownloadURLParams): Promise { + const key = extractKeyFromS3Url(file.filepath); + if (!key) { + throw new Error('[getCloudFrontDownloadURL] Unable to extract S3 key from file path'); + } + const url = appendDownloadOverrides(buildCloudFrontUrl(key), customFilename, contentType); + return signUrl(url); +} diff --git a/packages/api/src/storage/constants.ts b/packages/api/src/storage/constants.ts index e6f5330aaf..c2f6ac5939 100644 --- a/packages/api/src/storage/constants.ts +++ b/packages/api/src/storage/constants.ts @@ -1,2 +1,5 @@ /** Default base path for cloud-stored files (used by all storage strategies). */ export const DEFAULT_BASE_PATH = 'images'; + +/** Shared avatar base path for cloud-stored public/avatar assets. */ +export const AVATAR_BASE_PATH = 'avatars'; diff --git a/packages/api/src/storage/images.ts b/packages/api/src/storage/images.ts index 479763102a..e1c770923c 100644 --- a/packages/api/src/storage/images.ts +++ b/packages/api/src/storage/images.ts @@ -11,7 +11,7 @@ import type { ImageUploadResult, ProcessAvatarParams, } from '~/storage/types'; -import { DEFAULT_BASE_PATH as defaultBasePath } from '~/storage/constants'; +import { AVATAR_BASE_PATH, DEFAULT_BASE_PATH as defaultBasePath } from '~/storage/constants'; export interface ImageServiceDeps { resizeImageBuffer: ( @@ -97,6 +97,7 @@ export class ImageService { buffer: processedBuffer, fileName, basePath, + tenantId: req.user.tenantId ?? null, }); const bytes = processedBuffer.length; return { filepath: downloadURL, bytes, width, height }; @@ -137,7 +138,8 @@ export class ImageService { userId, manual, agentId, - basePath = defaultBasePath, + basePath = AVATAR_BASE_PATH, + tenantId = null, }: ProcessAvatarParams): Promise { try { const metadata = await sharp(buffer).metadata(); @@ -148,7 +150,7 @@ export class ImageService { ? `agent-${agentId}-avatar-${timestamp}.${extension}` : `avatar-${timestamp}.${extension}`; - const downloadURL = await this.saveBuffer({ userId, buffer, fileName, basePath }); + const downloadURL = await this.saveBuffer({ userId, buffer, fileName, basePath, tenantId }); const finalURL = this.config.appendManualParam ? `${downloadURL}?manual=${manual === 'true'}` diff --git a/packages/api/src/storage/index.ts b/packages/api/src/storage/index.ts index 0d2be2208a..fe545dcc01 100644 --- a/packages/api/src/storage/index.ts +++ b/packages/api/src/storage/index.ts @@ -2,3 +2,4 @@ export * from './cloudfront'; export * from './s3'; export * from './types'; export * from './images'; +export * from './avatar'; diff --git a/packages/api/src/storage/s3/__tests__/crud.test.ts b/packages/api/src/storage/s3/__tests__/crud.test.ts index 46e66541ec..55b5200e61 100644 --- a/packages/api/src/storage/s3/__tests__/crud.test.ts +++ b/packages/api/src/storage/s3/__tests__/crud.test.ts @@ -5,8 +5,11 @@ import { sdkStreamMixin } from '@smithy/util-stream'; import { FileSources } from 'librechat-data-provider'; import { S3Client, + UploadPartCommand, PutObjectCommand, GetObjectCommand, + CreateMultipartUploadCommand, + CompleteMultipartUploadCommand, HeadObjectCommand, DeleteObjectCommand, } from '@aws-sdk/client-s3'; @@ -63,6 +66,9 @@ describe('S3 CRUD', () => { beforeEach(() => { s3Mock.reset(); s3Mock.on(PutObjectCommand).resolves({}); + s3Mock.on(CreateMultipartUploadCommand).resolves({ UploadId: 'upload-123' }); + s3Mock.on(UploadPartCommand).resolves({ ETag: '"part-etag"' }); + s3Mock.on(CompleteMultipartUploadCommand).resolves({}); s3Mock.on(DeleteObjectCommand).resolves({}); const stream = new Readable(); @@ -87,12 +93,78 @@ describe('S3 CRUD', () => { expect(key).toBe('files/user456/folder/subfolder/doc.pdf'); }); + it('constructs tenant-prefixed keys when tenantId is provided', async () => { + const { getS3Key } = await import('../crud'); + const key = getS3Key('images', 'user123', 'file.png', 'tenantA'); + expect(key).toBe('t/tenantA/images/user123/file.png'); + }); + it('throws if basePath contains a slash', async () => { const { getS3Key } = await import('../crud'); expect(() => getS3Key('a/b', 'user123', 'file.png')).toThrow( '[getS3Key] basePath must not contain slashes: "a/b"', ); }); + + it('throws if tenantId contains path traversal characters', async () => { + const { getS3Key } = await import('../crud'); + expect(() => getS3Key('images', 'user123', 'file.png', '../tenantB')).toThrow( + '[getS3Key] tenantId must not contain slashes: "../tenantB"', + ); + }); + + it('throws if userId contains path traversal characters', async () => { + const { getS3Key } = await import('../crud'); + expect(() => getS3Key('images', 'user/123', 'file.png')).toThrow( + '[getS3Key] userId must not contain slashes: "user/123"', + ); + }); + + it('throws if fileName contains traversal or unsafe path characters', async () => { + const { getS3Key } = await import('../crud'); + expect(() => getS3Key('images', 'user123', '../file.png')).toThrow( + '[getS3Key] fileName must not contain path traversal: "../file.png"', + ); + expect(() => getS3Key('images', 'user123', 'folder//file.png')).toThrow( + '[getS3Key] fileName must not contain empty path components', + ); + expect(() => getS3Key('images', 'user123', 'file\u0000.png')).toThrow( + '[getS3Key] fileName contains unsafe path characters', + ); + }); + }); + + describe('parseS3Key', () => { + it('parses legacy keys', async () => { + const { parseS3Key } = await import('../crud'); + expect(parseS3Key('images/user123/folder/file.png')).toEqual({ + basePath: 'images', + userId: 'user123', + fileName: 'folder/file.png', + }); + }); + + it('parses tenant-prefixed keys', async () => { + const { parseS3Key } = await import('../crud'); + expect(parseS3Key('t/tenantA/images/user123/file.png')).toEqual({ + tenantId: 'tenantA', + basePath: 'images', + userId: 'user123', + fileName: 'file.png', + }); + }); + + it('returns null for incomplete keys', async () => { + const { parseS3Key } = await import('../crud'); + expect(parseS3Key('images/user123')).toBeNull(); + expect(parseS3Key('t/tenantA/images/user123')).toBeNull(); + }); + + it('returns null for unsafe tenant or user segments', async () => { + const { parseS3Key } = await import('../crud'); + expect(parseS3Key('t/../images/user123/file.png')).toBeNull(); + expect(parseS3Key('images/../file.png')).toBeNull(); + }); }); describe('saveBufferToS3', () => { @@ -125,6 +197,28 @@ describe('S3 CRUD', () => { }); }); + it('uses tenant-prefixed key and URL params when tenantId is provided', async () => { + const urlBuilder = jest.fn().mockResolvedValue('https://cdn.example.com/t/tenantA/file.txt'); + const { saveBufferToS3 } = await import('../crud'); + await saveBufferToS3({ + userId: 'user123', + buffer: Buffer.from('test content'), + fileName: 'document.pdf', + basePath: 'documents', + tenantId: 'tenantA', + urlBuilder, + }); + + const calls = s3Mock.commandCalls(PutObjectCommand); + expect(calls[0].args[0].input.Key).toBe('t/tenantA/documents/user123/document.pdf'); + expect(urlBuilder).toHaveBeenCalledWith({ + userId: 'user123', + fileName: 'document.pdf', + basePath: 'documents', + tenantId: 'tenantA', + }); + }); + it('uses default basePath if not provided', async () => { const { saveBufferToS3 } = await import('../crud'); await saveBufferToS3({ @@ -227,11 +321,18 @@ describe('S3 CRUD', () => { beforeEach(() => { global.fetch = jest.fn().mockResolvedValue({ ok: true, + headers: { + get: (name: string) => + ({ + 'content-length': '8', + 'content-type': 'image/jpeg', + })[name.toLowerCase()] ?? null, + }, arrayBuffer: jest.fn().mockResolvedValue(new ArrayBuffer(8)), }) as unknown as typeof fetch; }); - it('fetches file from URL and saves to S3', async () => { + it('fetches file from URL and returns the saved filepath', async () => { const { saveURLToS3 } = await import('../crud'); const result = await saveURLToS3({ userId: 'user123', @@ -241,7 +342,162 @@ describe('S3 CRUD', () => { expect(global.fetch).toHaveBeenCalledWith('https://example.com/image.jpg'); expect(s3Mock.commandCalls(PutObjectCommand)).toHaveLength(1); - expect(result).toContain('signed=true'); + expect(result).toBe('https://bucket.s3.amazonaws.com/test-key?signed=true'); + }); + + it('fetches file from URL and returns metadata when requested', async () => { + const { saveURLToS3WithMetadata } = await import('../crud'); + const result = await saveURLToS3WithMetadata({ + userId: 'user123', + URL: 'https://example.com/image.jpg', + fileName: 'downloaded.jpg', + }); + + expect(global.fetch).toHaveBeenCalledWith('https://example.com/image.jpg'); + expect(s3Mock.commandCalls(PutObjectCommand)).toHaveLength(1); + expect(result).toEqual({ + filepath: 'https://bucket.s3.amazonaws.com/test-key?signed=true', + bytes: 8, + type: 'image/jpeg', + dimensions: {}, + }); + }); + + it('uses the downloaded buffer size instead of a stale content-length header', async () => { + (global.fetch as unknown as jest.Mock).mockResolvedValueOnce({ + ok: true, + headers: { + get: (name: string) => + ({ + 'content-length': '999', + 'content-type': 'image/jpeg', + })[name.toLowerCase()] ?? null, + }, + arrayBuffer: jest.fn().mockResolvedValue(new ArrayBuffer(8)), + }); + + const { saveURLToS3WithMetadata } = await import('../crud'); + const result = await saveURLToS3WithMetadata({ + userId: 'user123', + URL: 'https://example.com/image.jpg', + fileName: 'downloaded.jpg', + }); + + expect(result.bytes).toBe(8); + }); + + it('streams response bodies into S3 when fetch provides a stream', async () => { + const streamedBody = Buffer.from('streamed'); + const arrayBuffer = jest.fn(); + (global.fetch as unknown as jest.Mock).mockResolvedValueOnce({ + ok: true, + headers: { + get: (name: string) => + ({ + 'content-length': String(streamedBody.byteLength), + 'content-type': 'image/png', + })[name.toLowerCase()] ?? null, + }, + body: new ReadableStream({ + start(controller) { + controller.enqueue(streamedBody); + controller.close(); + }, + }), + arrayBuffer, + }); + + const { saveURLToS3WithMetadata } = await import('../crud'); + const result = await saveURLToS3WithMetadata({ + userId: 'user123', + URL: 'https://example.com/image.jpg', + fileName: 'downloaded.jpg', + }); + + expect(arrayBuffer).not.toHaveBeenCalled(); + expect(result).toMatchObject({ + bytes: streamedBody.byteLength, + type: 'image/png', + }); + const putInput = s3Mock.commandCalls(PutObjectCommand)[0].args[0].input; + expect(putInput.Body).toBeInstanceOf(Buffer); + expect(putInput.ContentLength).toBeUndefined(); + expect((putInput.Body as Buffer).toString()).toBe('streamed'); + }); + + it('uses multipart upload for streamed responses larger than one part', async () => { + const firstPart = Buffer.alloc(5 * 1024 * 1024, 'a'); + const finalPart = Buffer.from('tail'); + const uploadedParts: Buffer[] = []; + (global.fetch as unknown as jest.Mock).mockResolvedValueOnce({ + ok: true, + headers: { + get: (name: string) => + ({ + 'content-length': String(firstPart.length + finalPart.length), + 'content-type': 'image/png', + })[name.toLowerCase()] ?? null, + }, + body: new ReadableStream({ + start(controller) { + controller.enqueue(firstPart); + controller.enqueue(finalPart); + controller.close(); + }, + }), + arrayBuffer: jest.fn(), + }); + s3Mock.on(UploadPartCommand).callsFake(async (input) => { + uploadedParts.push(input.Body as Buffer); + return { ETag: `"part-${input.PartNumber}"` }; + }); + + const { saveURLToS3WithMetadata } = await import('../crud'); + const result = await saveURLToS3WithMetadata({ + userId: 'user123', + URL: 'https://example.com/image.jpg', + fileName: 'downloaded.jpg', + }); + + expect(result.bytes).toBe(firstPart.length + finalPart.length); + expect(s3Mock.commandCalls(PutObjectCommand)).toHaveLength(0); + expect(s3Mock.commandCalls(CreateMultipartUploadCommand)).toHaveLength(1); + expect(s3Mock.commandCalls(UploadPartCommand)).toHaveLength(2); + expect(s3Mock.commandCalls(CompleteMultipartUploadCommand)).toHaveLength(1); + expect(uploadedParts[0]).toEqual(firstPart); + expect(uploadedParts[1]).toEqual(finalPart); + }); + + it('does not trust remote ContentLength for streamed uploads', async () => { + (global.fetch as unknown as jest.Mock).mockResolvedValueOnce({ + ok: true, + headers: { + get: (name: string) => + ({ + 'content-length': '4', + 'content-type': 'image/png', + })[name.toLowerCase()] ?? null, + }, + body: new ReadableStream({ + start(controller) { + controller.enqueue(Buffer.from('decoded')); + controller.close(); + }, + }), + arrayBuffer: jest.fn(), + }); + + const { saveURLToS3WithMetadata } = await import('../crud'); + const result = await saveURLToS3WithMetadata({ + userId: 'user123', + URL: 'https://example.com/image.jpg', + fileName: 'downloaded.jpg', + }); + + expect(result.bytes).toBe(Buffer.byteLength('decoded')); + const putInput = s3Mock.commandCalls(PutObjectCommand)[0].args[0].input; + expect(putInput.ContentLength).toBeUndefined(); + expect(putInput.Body).toEqual(Buffer.from('decoded')); }); it('throws error on non-ok response', async () => { @@ -285,6 +541,7 @@ describe('S3 CRUD', () => { const mockFile = { filepath: 'https://bucket.s3.amazonaws.com/images/user123/file.jpg', file_id: 'file123', + user: 'user123', } as TFile; s3Mock.on(HeadObjectCommand).resolvesOnce({}); @@ -297,10 +554,28 @@ describe('S3 CRUD', () => { expect(s3Mock.commandCalls(DeleteObjectCommand)).toHaveLength(1); }); + it('uses the file owner for RAG cleanup when a different authorized user deletes', async () => { + const requesterReq = { user: { id: 'sharedUser' } } as ServerRequest; + const mockFile = { + filepath: 'https://bucket.s3.amazonaws.com/images/user123/file.jpg', + file_id: 'file123', + user: 'user123', + } as TFile; + + s3Mock.on(HeadObjectCommand).resolvesOnce({}); + + const { deleteFileFromS3 } = await import('../crud'); + await deleteFileFromS3(requesterReq, mockFile); + + expect(deleteRagFile).toHaveBeenCalledWith({ userId: 'user123', file: mockFile }); + expect(s3Mock.commandCalls(DeleteObjectCommand)).toHaveLength(1); + }); + it('handles file not found gracefully and cleans up RAG', async () => { const mockFile = { filepath: 'https://bucket.s3.amazonaws.com/images/user123/nonexistent.jpg', file_id: 'file123', + user: 'user123', } as TFile; s3Mock.on(HeadObjectCommand).rejects({ name: 'NotFound' }); @@ -317,17 +592,32 @@ describe('S3 CRUD', () => { const mockFile = { filepath: 'https://bucket.s3.amazonaws.com/images/different-user/file.jpg', file_id: 'file123', + user: 'user123', } as TFile; const { deleteFileFromS3 } = await import('../crud'); - await expect(deleteFileFromS3(mockReq, mockFile)).rejects.toThrow('User ID mismatch'); + await expect(deleteFileFromS3(mockReq, mockFile)).rejects.toThrow('File owner mismatch'); expect(logger.error).toHaveBeenCalled(); }); - it('handles NoSuchKey error without calling deleteRagFile', async () => { + it('throws error if tenant ID does not match', async () => { + const mockFile = { + filepath: 'https://bucket.s3.amazonaws.com/t/tenantB/images/user123/file.jpg', + file_id: 'file123', + user: 'user123', + tenantId: 'tenantA', + } as TFile; + + const { deleteFileFromS3 } = await import('../crud'); + await expect(deleteFileFromS3(mockReq, mockFile)).rejects.toThrow('Tenant ID mismatch'); + expect(logger.error).toHaveBeenCalled(); + }); + + it('handles NoSuchKey error and cleans up RAG', async () => { const mockFile = { filepath: 'https://bucket.s3.amazonaws.com/images/user123/file.jpg', file_id: 'file123', + user: 'user123', } as TFile; s3Mock.on(HeadObjectCommand).resolvesOnce({}); @@ -336,7 +626,28 @@ describe('S3 CRUD', () => { const { deleteFileFromS3 } = await import('../crud'); await expect(deleteFileFromS3(mockReq, mockFile)).resolves.toBeUndefined(); - expect(deleteRagFile).not.toHaveBeenCalled(); + expect(deleteRagFile).toHaveBeenCalledWith({ userId: 'user123', file: mockFile }); + }); + + it('rejects tenant-prefixed keys when the file record lacks tenantId', async () => { + const mockFile = { + filepath: 'https://bucket.s3.amazonaws.com/t/tenantA/images/user123/file.jpg', + file_id: 'file123', + user: 'user123', + } as TFile; + + const { deleteFileFromS3 } = await import('../crud'); + await expect(deleteFileFromS3(mockReq, mockFile)).rejects.toThrow('Tenant ID mismatch'); + }); + + it('rejects file records without an owner', async () => { + const mockFile = { + filepath: 'https://bucket.s3.amazonaws.com/images/user123/file.jpg', + file_id: 'file123', + } as TFile; + + const { deleteFileFromS3 } = await import('../crud'); + await expect(deleteFileFromS3(mockReq, mockFile)).rejects.toThrow('File record has no owner'); }); }); @@ -369,6 +680,30 @@ describe('S3 CRUD', () => { expect(fs.promises.unlink).not.toHaveBeenCalled(); }); + it('uses tenantId from request when uploading a file', async () => { + const mockReqWithTenant = { + user: { id: 'user123', tenantId: 'tenantA' }, + } as ServerRequest; + const mockFile = { + path: '/tmp/upload.jpg', + originalname: 'photo.jpg', + } as Express.Multer.File; + + (fs.promises.stat as jest.Mock).mockResolvedValue({ size: 1024 }); + (fs.createReadStream as jest.Mock).mockReturnValue(new Readable()); + + const { uploadFileToS3 } = await import('../crud'); + await uploadFileToS3({ + req: mockReqWithTenant, + file: mockFile, + file_id: 'file123', + basePath: 'images', + }); + + const calls = s3Mock.commandCalls(PutObjectCommand); + expect(calls[0].args[0].input.Key).toBe('t/tenantA/images/user123/file123__photo.jpg'); + }); + it('handles upload errors and cleans up temp file', async () => { const mockFile = { path: '/tmp/upload.jpg', @@ -420,6 +755,36 @@ describe('S3 CRUD', () => { }); }); + describe('getS3DownloadURL', () => { + it('returns a signed URL for an existing file path', async () => { + const mockFile = { + filepath: 'https://bucket.s3.amazonaws.com/t/tenantA/uploads/user123/file.pdf', + filename: 'file.pdf', + } as TFile; + + const { getS3DownloadURL } = await import('../crud'); + const result = await getS3DownloadURL({ + req: {} as ServerRequest, + file: mockFile, + customFilename: 'download";\\bad.pdf', + contentType: 'application/pdf', + }); + + expect(result).toContain('signed=true'); + expect(getSignedUrl).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + input: expect.objectContaining({ + Key: 't/tenantA/uploads/user123/file.pdf', + ResponseContentDisposition: 'attachment; filename="downloadbad.pdf"', + ResponseContentType: 'application/pdf', + }), + }), + expect.anything(), + ); + }); + }); + describe('needsRefresh', () => { it('returns false for non-signed URLs', async () => { const { needsRefresh } = await import('../crud'); @@ -469,6 +834,23 @@ describe('S3 CRUD', () => { expect(result).toContain('signed=true'); }); + it('generates a new URL from a tenant-prefixed S3 URL', async () => { + const { getNewS3URL } = await import('../crud'); + await getNewS3URL( + 'https://bucket.s3.amazonaws.com/t/tenantA/images/user123/file.jpg?signature=old', + ); + + expect(getSignedUrl).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ + input: expect.objectContaining({ + Key: 't/tenantA/images/user123/file.jpg', + }), + }), + expect.anything(), + ); + }); + it('returns undefined for invalid URLs', async () => { const { getNewS3URL } = await import('../crud'); const result = await getNewS3URL('simple-file.txt'); diff --git a/packages/api/src/storage/s3/__tests__/s3.integration.spec.ts b/packages/api/src/storage/s3/__tests__/s3.integration.spec.ts index 5dc1e3b754..4f52ded5be 100644 --- a/packages/api/src/storage/s3/__tests__/s3.integration.spec.ts +++ b/packages/api/src/storage/s3/__tests__/s3.integration.spec.ts @@ -188,18 +188,19 @@ describe('S3 Integration Tests', () => { describe('saveURLToS3', () => { it('fetches URL content and uploads to S3', async () => { - const { saveURLToS3 } = await import('~/storage/s3/crud'); + const { saveURLToS3WithMetadata } = await import('~/storage/s3/crud'); const fileName = `url-upload-${Date.now()}.json`; - const downloadURL = await saveURLToS3({ + const savedFile = await saveURLToS3WithMetadata({ userId: TEST_USER_ID, URL: 'https://raw.githubusercontent.com/danny-avila/LibreChat/main/package.json', fileName, basePath: TEST_BASE_PATH, }); - expect(downloadURL).toBeDefined(); - expect(downloadURL).toContain('X-Amz-Signature'); + expect(savedFile.filepath).toBeDefined(); + expect(savedFile.filepath).toContain('X-Amz-Signature'); + expect(savedFile.bytes).toBeGreaterThan(0); }); }); diff --git a/packages/api/src/storage/s3/crud.ts b/packages/api/src/storage/s3/crud.ts index 1ea6d32dd0..b6a1695ddc 100644 --- a/packages/api/src/storage/s3/crud.ts +++ b/packages/api/src/storage/s3/crud.ts @@ -1,20 +1,30 @@ import fs from 'fs'; +import { Readable } from 'stream'; import { + UploadPartCommand, PutObjectCommand, GetObjectCommand, + CreateMultipartUploadCommand, + AbortMultipartUploadCommand, + CompleteMultipartUploadCommand, HeadObjectCommand, DeleteObjectCommand, } from '@aws-sdk/client-s3'; import { logger } from '@librechat/data-schemas'; import { FileSources } from 'librechat-data-provider'; import { getSignedUrl } from '@aws-sdk/s3-request-presigner'; -import type { GetObjectCommandInput } from '@aws-sdk/client-s3'; +import type { + CompletedPart, + GetObjectCommandInput, + PutObjectCommandInput, +} from '@aws-sdk/client-s3'; import type { TFile } from 'librechat-data-provider'; -import type { Readable } from 'stream'; import type { ServerRequest } from '~/types'; import type { UploadFileParams, SaveBufferParams, + DownloadURLParams, + SaveURLResult, BatchUpdateFn, SaveURLParams, GetURLParams, @@ -22,6 +32,11 @@ import type { UrlBuilder, S3FileRef, } from '~/storage/types'; +import { + assertS3FileName, + assertPathSegment, + sanitizeContentDispositionFilename, +} from '~/storage/validation'; import { initializeS3 } from '~/cdn/s3'; import { deleteRagFile } from '~/files'; import { s3Config } from './s3Config'; @@ -35,25 +50,88 @@ const { DEFAULT_BASE_PATH: defaultBasePath, } = s3Config; -export const getS3Key = (basePath: string, userId: string, fileName: string): string => { - if (basePath.includes('/')) { - throw new Error(`[getS3Key] basePath must not contain slashes: "${basePath}"`); +const MULTIPART_UPLOAD_PART_SIZE = 5 * 1024 * 1024; + +export interface S3KeyParts { + basePath: string; + userId: string; + fileName: string; + tenantId?: string; +} + +const parseS3PathSegment = (value: string | undefined): string | null => { + try { + return assertPathSegment('S3 key segment', value, 'getS3Key'); + } catch { + return null; } - return `${basePath}/${userId}/${fileName}`; }; -export async function getS3URL({ - userId, - fileName, - basePath = defaultBasePath, +export const getS3Key = ( + basePath: string, + userId: string, + fileName: string, + tenantId?: string | null, +): string => { + const safeBasePath = assertPathSegment('basePath', basePath, 'getS3Key'); + const safeUserId = assertPathSegment('userId', userId, 'getS3Key'); + const safeFileName = assertS3FileName('fileName', fileName, 'getS3Key'); + + if (tenantId) { + const safeTenantId = assertPathSegment('tenantId', tenantId, 'getS3Key'); + return `t/${safeTenantId}/${safeBasePath}/${safeUserId}/${safeFileName}`; + } + return `${safeBasePath}/${safeUserId}/${safeFileName}`; +}; + +export const parseS3Key = (key: string): S3KeyParts | null => { + const normalizedKey = key.replace(/^\/+/, ''); + const keyParts = normalizedKey.split('/'); + + if (keyParts[0] === 't') { + if (keyParts.length < 5) { + return null; + } + const [, tenantId, basePath, userId, ...fileNameParts] = keyParts; + const safeTenantId = parseS3PathSegment(tenantId); + const safeBasePath = parseS3PathSegment(basePath); + const safeUserId = parseS3PathSegment(userId); + if (!safeTenantId || !safeBasePath || !safeUserId) { + return null; + } + return { + tenantId: safeTenantId, + basePath: safeBasePath, + userId: safeUserId, + fileName: fileNameParts.join('/'), + }; + } + + if (keyParts.length < 3) { + return null; + } + const [basePath, userId, ...fileNameParts] = keyParts; + const safeBasePath = parseS3PathSegment(basePath); + const safeUserId = parseS3PathSegment(userId); + if (!safeBasePath || !safeUserId) { + return null; + } + return { basePath: safeBasePath, userId: safeUserId, fileName: fileNameParts.join('/') }; +}; + +async function getS3URLForKey({ + key, customFilename = null, contentType = null, -}: GetURLParams): Promise { - const key = getS3Key(basePath, userId, fileName); +}: { + key: string; + customFilename?: string | null; + contentType?: string | null; +}): Promise { const params: GetObjectCommandInput = { Bucket: bucketName, Key: key }; if (customFilename) { - const safeFilename = customFilename.replace(/["\r\n]/g, ''); + const safeFilename = sanitizeContentDispositionFilename(customFilename); params.ResponseContentDisposition = `attachment; filename="${safeFilename}"`; } if (contentType) { @@ -73,14 +151,27 @@ export async function getS3URL({ } } +export async function getS3URL({ + userId, + fileName, + basePath = defaultBasePath, + customFilename = null, + contentType = null, + tenantId = null, +}: GetURLParams): Promise { + const key = getS3Key(basePath, userId, fileName, tenantId); + return getS3URLForKey({ key, customFilename, contentType }); +} + export async function saveBufferToS3({ userId, buffer, fileName, basePath = defaultBasePath, + tenantId = null, urlBuilder, }: SaveBufferParams & { urlBuilder?: UrlBuilder }): Promise { - const key = getS3Key(basePath, userId, fileName); + const key = getS3Key(basePath, userId, fileName, tenantId); const params = { Bucket: bucketName, Key: key, Body: buffer }; try { @@ -91,34 +182,212 @@ export async function saveBufferToS3({ await s3.send(new PutObjectCommand(params)); const getUrl = urlBuilder ?? getS3URL; - return await getUrl({ userId, fileName, basePath }); + return await getUrl({ userId, fileName, basePath, tenantId }); } catch (error) { logger.error('[saveBufferToS3] Error uploading buffer to S3:', (error as Error).message); throw error; } } -export async function saveURLToS3({ +interface PendingUploadBuffers { + buffers: Buffer[]; + bytes: number; +} + +const toUploadBuffer = (chunk: Buffer | string | Uint8Array): Buffer => { + if (Buffer.isBuffer(chunk)) { + return chunk; + } + return Buffer.from(chunk); +}; + +const takePendingBytes = (pending: PendingUploadBuffers, size: number): Buffer => { + const output = Buffer.allocUnsafe(size); + let offset = 0; + + while (offset < size) { + const buffer = pending.buffers[0]; + const bytesNeeded = size - offset; + + if (buffer.length <= bytesNeeded) { + buffer.copy(output, offset); + offset += buffer.length; + pending.bytes -= buffer.length; + pending.buffers.shift(); + continue; + } + + buffer.copy(output, offset, 0, bytesNeeded); + pending.buffers[0] = buffer.subarray(bytesNeeded); + pending.bytes -= bytesNeeded; + offset += bytesNeeded; + } + + return output; +}; + +async function saveReadableToS3({ + userId, + body, + fileName, + basePath = defaultBasePath, + tenantId = null, + urlBuilder, +}: Omit & { + body: Readable; + urlBuilder?: UrlBuilder; +}): Promise<{ filepath: string; bytes: number }> { + const key = getS3Key(basePath, userId, fileName, tenantId); + const pending: PendingUploadBuffers = { buffers: [], bytes: 0 }; + const completedParts: CompletedPart[] = []; + let totalBytes = 0; + let partNumber = 1; + let uploadId: string | undefined; + + try { + const s3 = initializeS3(); + if (!s3) { + throw new Error('[saveReadableToS3] S3 not initialized'); + } + + const createMultipartUpload = async (): Promise => { + if (uploadId) { + return uploadId; + } + const response = await s3.send( + new CreateMultipartUploadCommand({ Bucket: bucketName, Key: key }), + ); + if (!response.UploadId) { + throw new Error('[saveReadableToS3] S3 did not return an upload ID'); + } + uploadId = response.UploadId; + return uploadId; + }; + + const uploadPart = async (partBody: Buffer): Promise => { + const currentUploadId = await createMultipartUpload(); + const response = await s3.send( + new UploadPartCommand({ + Bucket: bucketName, + Key: key, + UploadId: currentUploadId, + PartNumber: partNumber, + Body: partBody, + }), + ); + completedParts.push({ ETag: response.ETag, PartNumber: partNumber }); + partNumber += 1; + }; + + for await (const chunk of body as AsyncIterable) { + const buffer = toUploadBuffer(chunk); + pending.buffers.push(buffer); + pending.bytes += buffer.length; + totalBytes += buffer.length; + + while (pending.bytes >= MULTIPART_UPLOAD_PART_SIZE) { + await uploadPart(takePendingBytes(pending, MULTIPART_UPLOAD_PART_SIZE)); + } + } + + if (!uploadId) { + const bodyBuffer = + pending.bytes > 0 ? takePendingBytes(pending, pending.bytes) : Buffer.alloc(0); + const params: PutObjectCommandInput = { Bucket: bucketName, Key: key, Body: bodyBuffer }; + await s3.send(new PutObjectCommand(params)); + } else { + if (pending.bytes > 0) { + await uploadPart(takePendingBytes(pending, pending.bytes)); + } + await s3.send( + new CompleteMultipartUploadCommand({ + Bucket: bucketName, + Key: key, + UploadId: uploadId, + MultipartUpload: { Parts: completedParts }, + }), + ); + } + + const getUrl = urlBuilder ?? getS3URL; + return { filepath: await getUrl({ userId, fileName, basePath, tenantId }), bytes: totalBytes }; + } catch (error) { + if (uploadId) { + try { + await initializeS3()?.send( + new AbortMultipartUploadCommand({ Bucket: bucketName, Key: key, UploadId: uploadId }), + ); + } catch (abortError) { + logger.warn('[saveReadableToS3] Error aborting multipart upload:', abortError); + } + } + logger.error('[saveReadableToS3] Error uploading stream to S3:', (error as Error).message); + throw error; + } +} + +export async function saveURLToS3WithMetadata({ userId, URL, fileName, basePath = defaultBasePath, + tenantId = null, urlBuilder, -}: SaveURLParams & { urlBuilder?: UrlBuilder }): Promise { +}: SaveURLParams & { urlBuilder?: UrlBuilder }): Promise { try { const response = await fetch(URL); if (!response.ok) { throw new Error(`Failed to fetch URL: ${response.status} ${response.statusText}`); } - const arrayBuffer = await response.arrayBuffer(); - const buffer = Buffer.from(arrayBuffer); - return await saveBufferToS3({ userId, buffer, fileName, basePath, urlBuilder }); + const contentType = response.headers.get('content-type') ?? ''; + if (response.body) { + const source = Readable.fromWeb( + response.body as unknown as Parameters[0], + ); + const result = await saveReadableToS3({ + userId, + body: source, + fileName, + basePath, + tenantId, + urlBuilder, + }); + return { + filepath: result.filepath, + bytes: result.bytes, + type: contentType, + dimensions: {}, + }; + } + + const buffer = Buffer.from(await response.arrayBuffer()); + const filepath = await saveBufferToS3({ + userId, + buffer, + fileName, + basePath, + tenantId, + urlBuilder, + }); + return { + filepath, + bytes: buffer.byteLength, + type: contentType, + dimensions: {}, + }; } catch (error) { logger.error('[saveURLToS3] Error uploading file from URL to S3:', (error as Error).message); throw error; } } +export async function saveURLToS3( + params: SaveURLParams & { urlBuilder?: UrlBuilder }, +): Promise { + const { filepath } = await saveURLToS3WithMetadata(params); + return filepath; +} + export function extractKeyFromS3Url(fileUrlOrKey: string): string { if (!fileUrlOrKey) { throw new Error('Invalid input: URL or key is empty'); @@ -201,12 +470,24 @@ export async function deleteFileFromS3(req: ServerRequest, file: TFile): Promise throw new Error('[deleteFileFromS3] User not authenticated'); } - const userId = req.user.id; const key = extractKeyFromS3Url(file.filepath); + const parsedKey = parseS3Key(key); + const ownerId = file.user?.toString?.(); + const fileTenantId = file.tenantId?.toString?.() ?? null; - const keyParts = key.split('/'); - if (keyParts.length < 2 || keyParts[1] !== userId) { - const message = `[deleteFileFromS3] User ID mismatch: ${userId} vs ${key}`; + if (!ownerId) { + const message = `[deleteFileFromS3] File record has no owner: ${key}`; + logger.error(message); + throw new Error(message); + } + + if (!parsedKey || parsedKey.userId !== ownerId) { + const message = `[deleteFileFromS3] File owner mismatch: ${ownerId} vs ${key}`; + logger.error(message); + throw new Error(message); + } + if ((parsedKey.tenantId ?? null) !== fileTenantId) { + const message = `[deleteFileFromS3] Tenant ID mismatch: ${fileTenantId} vs ${key}`; logger.error(message); throw new Error(message); } @@ -226,20 +507,21 @@ export async function deleteFileFromS3(req: ServerRequest, file: TFile): Promise } catch (headErr) { if ((headErr as { name?: string }).name === 'NotFound') { logger.warn(`[deleteFileFromS3] File does not exist: ${key}`); - await deleteRagFile({ userId, file }); + await deleteRagFile({ userId: ownerId, file }); return; } throw headErr; } await s3.send(new DeleteObjectCommand(params)); - await deleteRagFile({ userId, file }); + await deleteRagFile({ userId: ownerId, file }); logger.debug('[deleteFileFromS3] S3 File deletion completed'); } catch (error) { logger.error(`[deleteFileFromS3] Error deleting file from S3: ${(error as Error).message}`); logger.error((error as Error).stack); if ((error as { name?: string }).name === 'NoSuchKey') { + await deleteRagFile({ userId: ownerId, file }); return; } throw error; @@ -251,6 +533,7 @@ export async function uploadFileToS3({ file, file_id, basePath = defaultBasePath, + tenantId = null, urlBuilder, }: UploadFileParams & { urlBuilder?: UrlBuilder }): Promise { if (!req.user) { @@ -260,8 +543,9 @@ export async function uploadFileToS3({ try { const inputFilePath = file.path; const userId = req.user.id; + const resolvedTenantId = tenantId ?? req.user.tenantId ?? null; const fileName = `${file_id}__${file.originalname}`; - const key = getS3Key(basePath, userId, fileName); + const key = getS3Key(basePath, userId, fileName, resolvedTenantId); const stats = await fs.promises.stat(inputFilePath); const bytes = stats.size; @@ -280,7 +564,7 @@ export async function uploadFileToS3({ await s3.send(new PutObjectCommand(uploadParams)); const getUrl = urlBuilder ?? getS3URL; - const fileURL = await getUrl({ userId, fileName, basePath }); + const fileURL = await getUrl({ userId, fileName, basePath, tenantId: resolvedTenantId }); // NOTE: temp file is intentionally NOT deleted on the success path. // The caller (processAgentFileUpload) reads file.path after this returns // to stream the file to the RAG vector embedding service (POST /embed). @@ -320,6 +604,18 @@ export async function getS3FileStream(_req: ServerRequest, filePath: string): Pr } } +export async function getS3DownloadURL({ + file, + customFilename = null, + contentType = null, +}: DownloadURLParams): Promise { + const key = extractKeyFromS3Url(file.filepath); + if (!key) { + throw new Error('[getS3DownloadURL] Unable to extract S3 key from file path'); + } + return getS3URLForKey({ key, customFilename, contentType }); +} + export function needsRefresh(signedUrl: string, bufferSeconds: number): boolean { try { const url = new URL(signedUrl); @@ -366,16 +662,12 @@ export async function getNewS3URL(currentURL: string): Promise; getProviderSchema(req: ServerRequest): Promise<[string, object]>; @@ -170,6 +171,12 @@ export interface ProcessedFile { }; } +/** Subset of storage strategy functions needed by download and delete access flows. */ export interface StrategyFunctions { getDownloadStream: (req: ServerRequest, filepath: string) => Promise; + getDownloadURL?: (params: DownloadURLParams) => Promise; + deleteFile?: ( + req: ServerRequest, + file: { filepath: string; user?: string; tenantId?: string | null }, + ) => Promise; } diff --git a/packages/api/src/utils/files.ts b/packages/api/src/utils/files.ts index e98797b97b..91db199562 100644 --- a/packages/api/src/utils/files.ts +++ b/packages/api/src/utils/files.ts @@ -14,10 +14,14 @@ const UNSAFE_UNICODE_FILENAME_PATTERN = /[^\p{L}\p{M}\p{N}\p{Emoji}\u200d._-]/gu const FILENAME_SEGMENT_MAX_BYTES = 255; function sanitizeFilenameSegment(segment: string): string { - return segment - .normalize('NFC') - .replace(/[\u0000-\u007f]/g, (char) => (ASCII_FILENAME_SAFE_PATTERN.test(char) ? char : '_')) - .replace(UNSAFE_UNICODE_FILENAME_PATTERN, '_'); + const asciiSanitized = Array.from(segment.normalize('NFC'), (char) => { + if (char.charCodeAt(0) > 0x7f) { + return char; + } + return ASCII_FILENAME_SAFE_PATTERN.test(char) ? char : '_'; + }).join(''); + + return asciiSanitized.replace(UNSAFE_UNICODE_FILENAME_PATTERN, '_'); } function utf8ByteLength(value: string): number { diff --git a/packages/data-provider/src/data-service.ts b/packages/data-provider/src/data-service.ts index 5c4dc8dfde..15cf7ed17d 100644 --- a/packages/data-provider/src/data-service.ts +++ b/packages/data-provider/src/data-service.ts @@ -697,6 +697,13 @@ export const getFileDownload = async (userId: string, file_id: string): Promise< }); }; +export const getFileDownloadURL = async ( + userId: string, + file_id: string, +): Promise => { + return request.get(`${endpoints.files()}/download-url/${userId}/${file_id}`); +}; + export const getCodeOutputDownload = async (url: string): Promise => { return request.getResponse(url, { responseType: 'blob', diff --git a/packages/data-provider/src/types/files.ts b/packages/data-provider/src/types/files.ts index c8f9621cc9..f4bd260e6e 100644 --- a/packages/data-provider/src/types/files.ts +++ b/packages/data-provider/src/types/files.ts @@ -99,6 +99,7 @@ export type TFile = { _id?: string; __v?: number; user: string; + tenantId?: string; conversationId?: string; message?: string; file_id: string; @@ -176,6 +177,13 @@ export type AvatarUploadResponse = { url: string; }; +export type FileDownloadURLResponse = { + url: string; + filename: string; + type: string; + metadata: Partial; +}; + export type SpeechToTextResponse = { text: string; }; diff --git a/packages/data-schemas/src/methods/file.spec.ts b/packages/data-schemas/src/methods/file.spec.ts index e8012feb5f..1d3efb56a2 100644 --- a/packages/data-schemas/src/methods/file.spec.ts +++ b/packages/data-schemas/src/methods/file.spec.ts @@ -88,6 +88,84 @@ describe('File Methods', () => { }); }); + describe('claimCodeFile', () => { + it('claims code output files independently per tenant', async () => { + const userId = new mongoose.Types.ObjectId().toString(); + + const tenantA = await fileMethods.claimCodeFile({ + filename: 'report.csv', + conversationId: 'conversation-1', + file_id: 'file-tenant-a', + user: userId, + tenantId: 'tenant-a', + }); + const tenantB = await fileMethods.claimCodeFile({ + filename: 'report.csv', + conversationId: 'conversation-1', + file_id: 'file-tenant-b', + user: userId, + tenantId: 'tenant-b', + }); + const tenantAAgain = await fileMethods.claimCodeFile({ + filename: 'report.csv', + conversationId: 'conversation-1', + file_id: 'file-tenant-a-new', + user: userId, + tenantId: 'tenant-a', + }); + + expect(tenantA.file_id).toBe('file-tenant-a'); + expect(tenantA.tenantId).toBe('tenant-a'); + expect(tenantB.file_id).toBe('file-tenant-b'); + expect(tenantB.tenantId).toBe('tenant-b'); + expect(tenantAAgain.file_id).toBe('file-tenant-a'); + }); + + it('keeps non-tenant code output claims in the legacy namespace', async () => { + const userId = new mongoose.Types.ObjectId().toString(); + + const legacy = await fileMethods.claimCodeFile({ + filename: 'legacy.csv', + conversationId: 'conversation-1', + file_id: 'legacy-file', + user: userId, + }); + const tenant = await fileMethods.claimCodeFile({ + filename: 'legacy.csv', + conversationId: 'conversation-1', + file_id: 'tenant-file', + user: userId, + tenantId: 'tenant-a', + }); + + expect(legacy.file_id).toBe('legacy-file'); + expect(legacy.tenantId).toBeNull(); + expect(tenant.file_id).toBe('tenant-file'); + expect(tenant.tenantId).toBe('tenant-a'); + }); + + it('treats null tenantId as the legacy code output namespace', async () => { + const userId = new mongoose.Types.ObjectId().toString(); + + const legacy = await fileMethods.claimCodeFile({ + filename: 'nullable-legacy.csv', + conversationId: 'conversation-1', + file_id: 'legacy-null-file', + user: userId, + tenantId: null, + }); + const legacyAgain = await fileMethods.claimCodeFile({ + filename: 'nullable-legacy.csv', + conversationId: 'conversation-1', + file_id: 'legacy-null-file-new', + user: userId, + }); + + expect(legacy.file_id).toBe('legacy-null-file'); + expect(legacyAgain.file_id).toBe('legacy-null-file'); + }); + }); + describe('findFileById', () => { it('should find a file by file_id', async () => { const fileId = uuidv4(); diff --git a/packages/data-schemas/src/methods/file.ts b/packages/data-schemas/src/methods/file.ts index 7393f4d78d..5ed87e002c 100644 --- a/packages/data-schemas/src/methods/file.ts +++ b/packages/data-schemas/src/methods/file.ts @@ -182,15 +182,21 @@ export function createFileMethods(mongoose: typeof import('mongoose')) { conversationId: string; file_id: string; user: string; + tenantId?: string | null; }): Promise { const File = mongoose.models.File as Model; + const tenantFilter = data.tenantId ? { tenantId: data.tenantId } : { tenantId: null }; + const insertData = data.tenantId + ? { file_id: data.file_id, user: data.user, tenantId: data.tenantId } + : { file_id: data.file_id, user: data.user }; const result = await File.findOneAndUpdate( { filename: data.filename, conversationId: data.conversationId, context: FileContext.execute_code, + ...tenantFilter, }, - { $setOnInsert: { file_id: data.file_id, user: data.user } }, + { $setOnInsert: insertData }, { upsert: true, new: true }, ).lean(); if (!result) {