mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-10-20 17:13:44 +00:00
🧵 fix: Preserve Upload Context Across Multipart Routes (#13072)
* fix skill multipart imports under strict isolation * fix file upload context after multipart parsing * fix skill upload tenant resolution * fix rejected upload cleanup
This commit is contained in:
parent
b32b328b87
commit
8735c1763c
11 changed files with 826 additions and 20 deletions
|
|
@ -11,6 +11,7 @@ module.exports = {
|
|||
delete: jest.fn(),
|
||||
})),
|
||||
logAxiosError: jest.fn(),
|
||||
restoreTenantContextFromReq: jest.fn((req, res, next) => next()),
|
||||
...overrides,
|
||||
}),
|
||||
|
||||
|
|
|
|||
|
|
@ -1,7 +1,11 @@
|
|||
const multer = require('multer');
|
||||
const express = require('express');
|
||||
const { sleep } = require('@librechat/agents');
|
||||
const { isEnabled, resolveImportMaxFileSize } = require('@librechat/api');
|
||||
const {
|
||||
isEnabled,
|
||||
resolveImportMaxFileSize,
|
||||
restoreTenantContextFromReq,
|
||||
} = require('@librechat/api');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const { CacheKeys, EModelEndpoint } = require('librechat-data-provider');
|
||||
const {
|
||||
|
|
@ -264,6 +268,7 @@ router.post(
|
|||
importUserLimiter,
|
||||
configMiddleware,
|
||||
handleUpload,
|
||||
restoreTenantContextFromReq,
|
||||
async (req, res) => {
|
||||
try {
|
||||
/* TODO: optimize to return imported conversations and add manually */
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ const {
|
|||
uaParser,
|
||||
checkBan,
|
||||
} = require('~/server/middleware');
|
||||
const { restoreTenantContextFromReq } = require('@librechat/api');
|
||||
const { avatar: asstAvatarRouter } = require('~/server/routes/assistants/v1');
|
||||
const { avatar: agentAvatarRouter } = require('~/server/routes/agents/v1');
|
||||
const { createMulterInstance } = require('./multer');
|
||||
|
|
@ -23,7 +24,7 @@ const initialize = async () => {
|
|||
router.use(uaParser);
|
||||
|
||||
const upload = await createMulterInstance();
|
||||
router.post('/speech/stt', upload.single('audio'));
|
||||
router.post('/speech/stt', upload.single('audio'), restoreTenantContextFromReq);
|
||||
|
||||
/* Important: speech route must be added before the upload limiters */
|
||||
router.use('/speech', speech);
|
||||
|
|
@ -43,11 +44,19 @@ const initialize = async () => {
|
|||
next();
|
||||
});
|
||||
|
||||
router.post('/', upload.single('file'));
|
||||
router.post('/images', upload.single('file'));
|
||||
router.post('/images/avatar', upload.single('file'));
|
||||
router.post('/images/agents/:agent_id/avatar', upload.single('file'));
|
||||
router.post('/images/assistants/:assistant_id/avatar', upload.single('file'));
|
||||
router.post('/', upload.single('file'), restoreTenantContextFromReq);
|
||||
router.post('/images', upload.single('file'), restoreTenantContextFromReq);
|
||||
router.post('/images/avatar', upload.single('file'), restoreTenantContextFromReq);
|
||||
router.post(
|
||||
'/images/agents/:agent_id/avatar',
|
||||
upload.single('file'),
|
||||
restoreTenantContextFromReq,
|
||||
);
|
||||
router.post(
|
||||
'/images/assistants/:assistant_id/avatar',
|
||||
upload.single('file'),
|
||||
restoreTenantContextFromReq,
|
||||
);
|
||||
|
||||
router.use('/', files);
|
||||
router.use('/images', images);
|
||||
|
|
|
|||
151
api/server/routes/files/index.tenant.test.js
Normal file
151
api/server/routes/files/index.tenant.test.js
Normal file
|
|
@ -0,0 +1,151 @@
|
|||
process.env.TENANT_ISOLATION_STRICT = 'true';
|
||||
|
||||
const express = require('express');
|
||||
const request = require('supertest');
|
||||
const fs = require('fs/promises');
|
||||
const { getTenantId, tenantStorage: mockTenantStorage } = require('@librechat/data-schemas');
|
||||
|
||||
const TEST_TENANT = 'tenant-files-strict';
|
||||
|
||||
let mockCurrentUser;
|
||||
|
||||
jest.mock('fs/promises', () => ({
|
||||
unlink: jest.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
|
||||
const mockTenantResponse = (route) => (req, res) =>
|
||||
res.status(200).json({ route, tenantId: getTenantId() });
|
||||
|
||||
jest.mock('~/server/middleware', () => ({
|
||||
createFileLimiters: jest.fn(() => ({
|
||||
fileUploadIpLimiter: (req, res, next) => next(),
|
||||
fileUploadUserLimiter: (req, res, next) => next(),
|
||||
})),
|
||||
configMiddleware: (req, res, next) => {
|
||||
req.config = {
|
||||
fileStrategy: 'local',
|
||||
paths: { uploads: '/tmp/uploads', images: '/tmp/images' },
|
||||
};
|
||||
next();
|
||||
},
|
||||
requireJwtAuth: (req, res, next) => {
|
||||
req.user = mockCurrentUser;
|
||||
const tenantId = req.user?.tenantId;
|
||||
if (!tenantId) {
|
||||
next();
|
||||
return;
|
||||
}
|
||||
mockTenantStorage.run({ tenantId }, async () => next());
|
||||
},
|
||||
uaParser: (req, res, next) => next(),
|
||||
checkBan: (req, res, next) => next(),
|
||||
}));
|
||||
|
||||
jest.mock('./multer', () => ({
|
||||
createMulterInstance: jest.fn(async () => ({
|
||||
single: jest.fn(() => (req, res, next) => {
|
||||
req.file = {
|
||||
path: '/tmp/uploaded-file',
|
||||
originalname: 'uploaded.txt',
|
||||
filename: 'uploaded.txt',
|
||||
mimetype: 'text/plain',
|
||||
size: 8,
|
||||
};
|
||||
req.file_id = 'file-upload-id';
|
||||
mockTenantStorage.enterWith({});
|
||||
next();
|
||||
}),
|
||||
})),
|
||||
}));
|
||||
|
||||
jest.mock('./files', () => {
|
||||
const express = require('express');
|
||||
const router = express.Router();
|
||||
router.post('/', mockTenantResponse('files'));
|
||||
return router;
|
||||
});
|
||||
|
||||
jest.mock('./images', () => {
|
||||
const express = require('express');
|
||||
const router = express.Router();
|
||||
router.post('/', mockTenantResponse('images'));
|
||||
return router;
|
||||
});
|
||||
|
||||
jest.mock('./avatar', () => {
|
||||
const express = require('express');
|
||||
const router = express.Router();
|
||||
router.post('/', mockTenantResponse('avatar'));
|
||||
return router;
|
||||
});
|
||||
|
||||
jest.mock('./speech', () => {
|
||||
const express = require('express');
|
||||
const router = express.Router();
|
||||
router.post('/stt', mockTenantResponse('speech-stt'));
|
||||
return router;
|
||||
});
|
||||
|
||||
jest.mock('~/server/routes/agents/v1', () => {
|
||||
const express = require('express');
|
||||
const avatar = express.Router();
|
||||
avatar.post('/:agent_id/avatar/', mockTenantResponse('agent-avatar'));
|
||||
return { avatar };
|
||||
});
|
||||
|
||||
jest.mock('~/server/routes/assistants/v1', () => {
|
||||
const express = require('express');
|
||||
const avatar = express.Router();
|
||||
avatar.post('/:assistant_id/avatar/', mockTenantResponse('assistant-avatar'));
|
||||
return { avatar };
|
||||
});
|
||||
|
||||
describe('file upload routes restore strict isolation context after multer', () => {
|
||||
let app;
|
||||
|
||||
beforeAll(async () => {
|
||||
const { initialize } = require('./index');
|
||||
app = express();
|
||||
app.use('/api/files', await initialize());
|
||||
});
|
||||
|
||||
beforeEach(() => {
|
||||
fs.unlink.mockClear();
|
||||
mockCurrentUser = {
|
||||
id: 'user-files-strict',
|
||||
role: 'USER',
|
||||
tenantId: TEST_TENANT,
|
||||
};
|
||||
});
|
||||
|
||||
afterAll(() => {
|
||||
delete process.env.TENANT_ISOLATION_STRICT;
|
||||
});
|
||||
|
||||
it.each([
|
||||
['files', '/api/files'],
|
||||
['images', '/api/files/images'],
|
||||
['avatar', '/api/files/images/avatar'],
|
||||
['agent-avatar', '/api/files/images/agents/agent-1/avatar'],
|
||||
['assistant-avatar', '/api/files/images/assistants/asst-1/avatar'],
|
||||
['speech-stt', '/api/files/speech/stt'],
|
||||
])('restores context for %s upload', async (route, url) => {
|
||||
const res = await request(app).post(url);
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body).toEqual({ route, tenantId: TEST_TENANT });
|
||||
});
|
||||
|
||||
it('rejects strict upload continuations when no tenant can be resolved', async () => {
|
||||
mockCurrentUser = {
|
||||
id: 'user-without-tenant',
|
||||
role: 'USER',
|
||||
};
|
||||
|
||||
const res = await request(app).post('/api/files');
|
||||
|
||||
expect(res.status).toBe(403);
|
||||
expect(res.body.error).toMatch(/Tenant context required/);
|
||||
expect(fs.unlink).toHaveBeenCalledWith('/tmp/uploaded-file');
|
||||
});
|
||||
});
|
||||
|
|
@ -1,5 +1,6 @@
|
|||
const multer = require('multer');
|
||||
const express = require('express');
|
||||
const { restoreTenantContextFromReq } = require('@librechat/api');
|
||||
const { logger } = require('@librechat/data-schemas');
|
||||
const { CacheKeys } = require('librechat-data-provider');
|
||||
const { getVoices, streamAudio, textToSpeech } = require('~/server/services/Files/Audio');
|
||||
|
|
@ -8,7 +9,7 @@ const { getLogStores } = require('~/cache');
|
|||
const router = express.Router();
|
||||
const upload = multer();
|
||||
|
||||
router.post('/manual', upload.none(), async (req, res) => {
|
||||
router.post('/manual', upload.none(), restoreTenantContextFromReq, async (req, res) => {
|
||||
await textToSpeech(req, res);
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -7,6 +7,8 @@ const {
|
|||
createImportHandler,
|
||||
generateCheckAccess,
|
||||
getStorageMetadata,
|
||||
resolveRequestTenantId,
|
||||
restoreTenantContextFromReq,
|
||||
} = require('@librechat/api');
|
||||
const { isValidObjectIdString, logger } = require('@librechat/data-schemas');
|
||||
const {
|
||||
|
|
@ -137,10 +139,11 @@ const importHandler = createImportHandler({
|
|||
getSkillById,
|
||||
deleteSkill,
|
||||
upsertSkillFile,
|
||||
saveBuffer: (req, { userId, buffer, fileName, basePath, isImage }) => {
|
||||
saveBuffer: (req, { userId, buffer, fileName, basePath, isImage, tenantId }) => {
|
||||
const requestTenantId = tenantId ?? resolveRequestTenantId(req);
|
||||
const storage = resolveSkillStorage(req, { isImage });
|
||||
return storage
|
||||
.saveBuffer({ userId, buffer, fileName, basePath, tenantId: req.user.tenantId })
|
||||
.saveBuffer({ userId, buffer, fileName, basePath, tenantId: requestTenantId })
|
||||
.then((filepath) => ({
|
||||
filepath,
|
||||
source: storage.source,
|
||||
|
|
@ -185,6 +188,8 @@ async function uploadFileHandler(req, res) {
|
|||
return res.status(400).json({ error: 'Invalid file path' });
|
||||
}
|
||||
|
||||
const tenantId = resolveRequestTenantId(req);
|
||||
|
||||
// Look up existing file before saving — needed to clean up old blob on replace
|
||||
const existingFile = await getSkillFileByPath(skillId, relativePath);
|
||||
|
||||
|
|
@ -199,7 +204,7 @@ async function uploadFileHandler(req, res) {
|
|||
buffer: file.buffer,
|
||||
fileName: storageFileName,
|
||||
basePath: 'uploads',
|
||||
tenantId: req.user.tenantId,
|
||||
tenantId,
|
||||
});
|
||||
const storageMetadata = getStorageMetadata({ filepath, source: storage.source });
|
||||
|
||||
|
|
@ -217,13 +222,14 @@ async function uploadFileHandler(req, res) {
|
|||
bytes: file.size,
|
||||
isExecutable: false,
|
||||
author: req.user._id,
|
||||
tenantId,
|
||||
});
|
||||
} catch (dbError) {
|
||||
// Clean up the stored blob so it doesn't leak on DB failure
|
||||
try {
|
||||
const { deleteFile } = getStrategyFunctions(storage.source);
|
||||
if (deleteFile) {
|
||||
await deleteFile(req, { filepath, user: req.user.id, tenantId: req.user.tenantId });
|
||||
await deleteFile(req, { filepath, user: req.user.id, tenantId });
|
||||
}
|
||||
} catch (cleanupErr) {
|
||||
logger.error('[uploadFile] Failed to clean up orphaned blob:', cleanupErr);
|
||||
|
|
@ -238,7 +244,7 @@ async function uploadFileHandler(req, res) {
|
|||
delOld(req, {
|
||||
filepath: existingFile.filepath,
|
||||
user: existingFile.author ?? req.user.id,
|
||||
tenantId: existingFile.tenantId ?? req.user.tenantId,
|
||||
tenantId: existingFile.tenantId ?? tenantId,
|
||||
}).catch((e) => logger.error('[uploadFile] Old blob cleanup failed:', e));
|
||||
}
|
||||
}
|
||||
|
|
@ -264,6 +270,7 @@ router.post(
|
|||
fileUploadIpLimiter,
|
||||
fileUploadUserLimiter,
|
||||
skillUpload.single('file'),
|
||||
restoreTenantContextFromReq,
|
||||
importHandler,
|
||||
);
|
||||
|
||||
|
|
@ -303,6 +310,7 @@ router.post(
|
|||
fileUploadIpLimiter,
|
||||
fileUploadUserLimiter,
|
||||
singleFileUpload.single('file'),
|
||||
restoreTenantContextFromReq,
|
||||
uploadFileHandler,
|
||||
);
|
||||
|
||||
|
|
|
|||
377
api/server/routes/skills.tenant.test.js
Normal file
377
api/server/routes/skills.tenant.test.js
Normal file
|
|
@ -0,0 +1,377 @@
|
|||
process.env.TENANT_ISOLATION_STRICT = 'true';
|
||||
|
||||
const express = require('express');
|
||||
const request = require('supertest');
|
||||
const JSZip = require('jszip');
|
||||
const mongoose = require('mongoose');
|
||||
const { MongoMemoryServer } = require('mongodb-memory-server');
|
||||
const { tenantStorage } = require('@librechat/data-schemas');
|
||||
const {
|
||||
SystemRoles,
|
||||
ResourceType,
|
||||
AccessRoleIds,
|
||||
PrincipalType,
|
||||
PermissionBits,
|
||||
} = require('librechat-data-provider');
|
||||
|
||||
const TEST_TENANT = 'tenant-skills-strict';
|
||||
|
||||
jest.mock('~/server/services/Config', () => ({
|
||||
getCachedTools: jest.fn().mockResolvedValue({}),
|
||||
getAppConfig: jest.fn().mockResolvedValue({
|
||||
fileStrategy: 'local',
|
||||
paths: { uploads: '/tmp/uploads', images: '/tmp/images' },
|
||||
}),
|
||||
}));
|
||||
|
||||
jest.mock('~/server/middleware/config/app', () => (req, _res, next) => {
|
||||
req.config = {
|
||||
fileStrategy: 'local',
|
||||
paths: { uploads: '/tmp/uploads', images: '/tmp/images' },
|
||||
};
|
||||
next();
|
||||
});
|
||||
|
||||
jest.mock('~/server/services/Files/strategies', () => ({
|
||||
getStrategyFunctions: jest.fn().mockReturnValue({
|
||||
saveBuffer: jest.fn().mockResolvedValue('/uploads/test/file.txt'),
|
||||
getDownloadStream: jest.fn().mockResolvedValue({
|
||||
pipe: jest.fn(),
|
||||
on: jest.fn(),
|
||||
[Symbol.asyncIterator]: async function* () {
|
||||
yield Buffer.from('test content');
|
||||
},
|
||||
}),
|
||||
}),
|
||||
}));
|
||||
|
||||
jest.mock('~/server/utils/getFileStrategy', () => ({
|
||||
getFileStrategy: jest.fn().mockReturnValue('local'),
|
||||
}));
|
||||
|
||||
jest.mock('~/models', () => {
|
||||
const mongoose = require('mongoose');
|
||||
const { createMethods } = require('@librechat/data-schemas');
|
||||
const methods = createMethods(mongoose, {
|
||||
removeAllPermissions: async ({ resourceType, resourceId }) => {
|
||||
const AclEntry = mongoose.models.AclEntry;
|
||||
if (AclEntry) {
|
||||
await AclEntry.deleteMany({ resourceType, resourceId });
|
||||
}
|
||||
},
|
||||
});
|
||||
return {
|
||||
...methods,
|
||||
getRoleByName: jest.fn(),
|
||||
};
|
||||
});
|
||||
|
||||
jest.mock('~/server/middleware', () => {
|
||||
const actual = jest.requireActual('~/server/middleware');
|
||||
const { tenantStorage } = require('@librechat/data-schemas');
|
||||
|
||||
return {
|
||||
requireJwtAuth: (req, _res, next) => {
|
||||
const tenantId = req.tenantId ?? req.user?.tenantId;
|
||||
if (!tenantId) {
|
||||
next();
|
||||
return;
|
||||
}
|
||||
tenantStorage.run({ tenantId }, async () => next());
|
||||
},
|
||||
canAccessSkillResource: actual.canAccessSkillResource,
|
||||
};
|
||||
});
|
||||
|
||||
let app;
|
||||
let mongoServer;
|
||||
let Skill;
|
||||
let SkillFile;
|
||||
let AclEntry;
|
||||
let AccessRole;
|
||||
let User;
|
||||
let testUsers;
|
||||
let testRoles;
|
||||
let currentTestUser;
|
||||
let currentRequestTenantId;
|
||||
|
||||
function setTestUser(user) {
|
||||
currentTestUser = user;
|
||||
}
|
||||
|
||||
function toRequestUser(user) {
|
||||
const raw = user.toObject ? user.toObject() : user;
|
||||
return {
|
||||
...raw,
|
||||
id: user._id.toString(),
|
||||
_id: user._id,
|
||||
name: user.name,
|
||||
role: user.role,
|
||||
tenantId: user.tenantId,
|
||||
};
|
||||
}
|
||||
|
||||
async function runInTenant(fn) {
|
||||
return tenantStorage.run({ tenantId: TEST_TENANT }, fn);
|
||||
}
|
||||
|
||||
beforeAll(async () => {
|
||||
mongoServer = await MongoMemoryServer.create();
|
||||
await mongoose.connect(mongoServer.getUri());
|
||||
|
||||
const dbModels = require('~/db/models');
|
||||
Skill = dbModels.Skill;
|
||||
SkillFile = dbModels.SkillFile;
|
||||
AclEntry = dbModels.AclEntry;
|
||||
AccessRole = dbModels.AccessRole;
|
||||
User = dbModels.User;
|
||||
|
||||
await runInTenant(setupTestData);
|
||||
|
||||
app = express();
|
||||
app.use(express.json());
|
||||
app.use((req, _res, next) => {
|
||||
if (currentRequestTenantId) {
|
||||
req.tenantId = currentRequestTenantId;
|
||||
}
|
||||
if (currentTestUser) {
|
||||
req.user = toRequestUser(currentTestUser);
|
||||
}
|
||||
next();
|
||||
});
|
||||
|
||||
currentTestUser = testUsers.owner;
|
||||
app.use('/api/skills', require('./skills'));
|
||||
});
|
||||
|
||||
afterEach(async () => {
|
||||
await runInTenant(async () => {
|
||||
await Skill.deleteMany({});
|
||||
await SkillFile.deleteMany({});
|
||||
await AclEntry.deleteMany({});
|
||||
});
|
||||
currentTestUser = testUsers.owner;
|
||||
currentRequestTenantId = undefined;
|
||||
});
|
||||
|
||||
afterAll(async () => {
|
||||
await mongoose.disconnect();
|
||||
await mongoServer.stop();
|
||||
delete process.env.TENANT_ISOLATION_STRICT;
|
||||
jest.clearAllMocks();
|
||||
});
|
||||
|
||||
async function setupTestData() {
|
||||
testRoles = {
|
||||
owner: await AccessRole.create({
|
||||
accessRoleId: AccessRoleIds.SKILL_OWNER,
|
||||
name: 'Owner',
|
||||
resourceType: ResourceType.SKILL,
|
||||
permBits:
|
||||
PermissionBits.VIEW | PermissionBits.EDIT | PermissionBits.DELETE | PermissionBits.SHARE,
|
||||
tenantId: TEST_TENANT,
|
||||
}),
|
||||
};
|
||||
|
||||
testUsers = {
|
||||
owner: await User.create({
|
||||
name: 'Strict Skill Owner',
|
||||
email: 'strict-skill-owner@test.com',
|
||||
role: SystemRoles.USER,
|
||||
tenantId: TEST_TENANT,
|
||||
}),
|
||||
};
|
||||
|
||||
const { getRoleByName } = require('~/models');
|
||||
getRoleByName.mockImplementation((roleName) => {
|
||||
if (roleName === SystemRoles.USER || roleName === SystemRoles.ADMIN) {
|
||||
return {
|
||||
permissions: {
|
||||
SKILLS: {
|
||||
USE: true,
|
||||
CREATE: true,
|
||||
SHARE: true,
|
||||
SHARE_PUBLIC: true,
|
||||
},
|
||||
},
|
||||
};
|
||||
}
|
||||
return null;
|
||||
});
|
||||
}
|
||||
|
||||
async function createSkillAsOwner(overrides = {}) {
|
||||
return request(app)
|
||||
.post('/api/skills')
|
||||
.send({
|
||||
name: 'strict-file-skill',
|
||||
description: 'A strict tenant skill used in multipart route tests.',
|
||||
body: '# Strict File Skill',
|
||||
...overrides,
|
||||
});
|
||||
}
|
||||
|
||||
describe('Skill multipart routes under strict tenant isolation', () => {
|
||||
it('imports a skill zip and writes skill, ACL, and files in the request tenant', async () => {
|
||||
const zip = new JSZip();
|
||||
zip.file(
|
||||
'SKILL.md',
|
||||
[
|
||||
'---',
|
||||
'name: strict-import',
|
||||
'description: Strict tenant import route test skill.',
|
||||
'---',
|
||||
'# Strict Import',
|
||||
].join('\n'),
|
||||
);
|
||||
zip.file('scripts/run.sh', 'echo strict');
|
||||
const buffer = await zip.generateAsync({ type: 'nodebuffer' });
|
||||
|
||||
const res = await request(app).post('/api/skills/import').attach('file', buffer, {
|
||||
filename: 'strict-import.skill',
|
||||
contentType: 'application/zip',
|
||||
});
|
||||
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.tenantId).toBe(TEST_TENANT);
|
||||
expect(res.body._importSummary.filesSucceeded).toBe(1);
|
||||
|
||||
const { skill, acl, file } = await runInTenant(async () => {
|
||||
const skill = await Skill.findOne({ name: 'strict-import' }).lean();
|
||||
const acl = await AclEntry.findOne({
|
||||
resourceType: ResourceType.SKILL,
|
||||
resourceId: skill._id,
|
||||
principalType: PrincipalType.USER,
|
||||
principalId: testUsers.owner._id,
|
||||
}).lean();
|
||||
const file = await SkillFile.findOne({
|
||||
skillId: skill._id,
|
||||
relativePath: 'scripts/run.sh',
|
||||
}).lean();
|
||||
return { skill, acl, file };
|
||||
});
|
||||
|
||||
expect(skill.tenantId).toBe(TEST_TENANT);
|
||||
expect(acl).toEqual(
|
||||
expect.objectContaining({
|
||||
tenantId: TEST_TENANT,
|
||||
roleId: testRoles.owner._id,
|
||||
}),
|
||||
);
|
||||
expect(file).toEqual(
|
||||
expect.objectContaining({
|
||||
tenantId: TEST_TENANT,
|
||||
relativePath: 'scripts/run.sh',
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('imports using the resolved request tenant when user tenant differs', async () => {
|
||||
currentRequestTenantId = TEST_TENANT;
|
||||
setTestUser({
|
||||
...testUsers.owner.toObject(),
|
||||
tenantId: 'stale-user-tenant',
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.post('/api/skills/import')
|
||||
.attach('file', Buffer.from('# Request Tenant Markdown'), {
|
||||
filename: 'request-tenant.md',
|
||||
contentType: 'text/markdown',
|
||||
});
|
||||
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.tenantId).toBe(TEST_TENANT);
|
||||
|
||||
const savedSkill = await runInTenant(async () =>
|
||||
Skill.findOne({ name: 'request-tenant' }).lean(),
|
||||
);
|
||||
expect(savedSkill).toEqual(
|
||||
expect.objectContaining({
|
||||
tenantId: TEST_TENANT,
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('rejects multipart import in strict mode when the request has no tenant', async () => {
|
||||
setTestUser({
|
||||
_id: new mongoose.Types.ObjectId(),
|
||||
name: 'No Tenant',
|
||||
role: SystemRoles.USER,
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.post('/api/skills/import')
|
||||
.attach('file', Buffer.from('# No Tenant'), {
|
||||
filename: 'no-tenant.md',
|
||||
contentType: 'text/markdown',
|
||||
});
|
||||
|
||||
expect(res.status).toBe(403);
|
||||
expect(res.body.error).toMatch(/Tenant context required/);
|
||||
});
|
||||
|
||||
it('uploads an individual skill file in the request tenant', async () => {
|
||||
const created = await createSkillAsOwner();
|
||||
expect(created.status).toBe(201);
|
||||
|
||||
const res = await request(app)
|
||||
.post(`/api/skills/${created.body._id}/files`)
|
||||
.field('relativePath', 'scripts/manual.sh')
|
||||
.attach('file', Buffer.from('echo manual'), {
|
||||
filename: 'manual.sh',
|
||||
contentType: 'text/x-shellscript',
|
||||
});
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.tenantId).toBe(TEST_TENANT);
|
||||
|
||||
const savedFile = await runInTenant(async () =>
|
||||
SkillFile.findOne({
|
||||
skillId: created.body._id,
|
||||
relativePath: 'scripts/manual.sh',
|
||||
}).lean(),
|
||||
);
|
||||
expect(savedFile).toEqual(
|
||||
expect.objectContaining({
|
||||
tenantId: TEST_TENANT,
|
||||
relativePath: 'scripts/manual.sh',
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it('uploads an individual skill file using the resolved request tenant', async () => {
|
||||
const created = await createSkillAsOwner({ name: 'request-tenant-file-skill' });
|
||||
expect(created.status).toBe(201);
|
||||
|
||||
currentRequestTenantId = TEST_TENANT;
|
||||
setTestUser({
|
||||
...testUsers.owner.toObject(),
|
||||
tenantId: 'stale-user-tenant',
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.post(`/api/skills/${created.body._id}/files`)
|
||||
.field('relativePath', 'scripts/request-tenant.sh')
|
||||
.attach('file', Buffer.from('echo request tenant'), {
|
||||
filename: 'request-tenant.sh',
|
||||
contentType: 'text/x-shellscript',
|
||||
});
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.tenantId).toBe(TEST_TENANT);
|
||||
|
||||
const savedFile = await runInTenant(async () =>
|
||||
SkillFile.findOne({
|
||||
skillId: created.body._id,
|
||||
relativePath: 'scripts/request-tenant.sh',
|
||||
}).lean(),
|
||||
);
|
||||
expect(savedFile).toEqual(
|
||||
expect.objectContaining({
|
||||
tenantId: TEST_TENANT,
|
||||
relativePath: 'scripts/request-tenant.sh',
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
|
@ -1,14 +1,30 @@
|
|||
import { getTenantId } from '@librechat/data-schemas';
|
||||
import { unlink } from 'fs/promises';
|
||||
import { getTenantId, SYSTEM_TENANT_ID } from '@librechat/data-schemas';
|
||||
import type { Response, NextFunction } from 'express';
|
||||
import type { ServerRequest } from '~/types/http';
|
||||
// Import directly from source file — _resetTenantMiddlewareStrictCache is intentionally
|
||||
// excluded from the public barrel export (index.ts).
|
||||
import { tenantContextMiddleware, _resetTenantMiddlewareStrictCache } from '../tenant';
|
||||
import {
|
||||
tenantContextMiddleware,
|
||||
restoreTenantContextFromReq,
|
||||
resolveRequestTenantId,
|
||||
_resetTenantMiddlewareStrictCache,
|
||||
} from '../tenant';
|
||||
|
||||
jest.mock('fs/promises', () => ({
|
||||
unlink: jest.fn().mockResolvedValue(undefined),
|
||||
}));
|
||||
|
||||
const unlinkMock = unlink as jest.MockedFunction<typeof unlink>;
|
||||
|
||||
function mockReq(user?: Record<string, unknown>): ServerRequest {
|
||||
return { user } as unknown as ServerRequest;
|
||||
}
|
||||
|
||||
function mockTenantReq(user?: Record<string, unknown>, tenantId?: string): ServerRequest {
|
||||
return { user, tenantId } as unknown as ServerRequest;
|
||||
}
|
||||
|
||||
function mockRes(): Response {
|
||||
const res = {
|
||||
status: jest.fn().mockReturnThis(),
|
||||
|
|
@ -31,6 +47,7 @@ describe('tenantContextMiddleware', () => {
|
|||
afterEach(() => {
|
||||
_resetTenantMiddlewareStrictCache();
|
||||
delete process.env.TENANT_ISOLATION_STRICT;
|
||||
unlinkMock.mockClear();
|
||||
});
|
||||
|
||||
it('sets ALS tenant context for authenticated requests with tenantId', async () => {
|
||||
|
|
@ -57,7 +74,7 @@ describe('tenantContextMiddleware', () => {
|
|||
expect(tenantId).toBeUndefined();
|
||||
});
|
||||
|
||||
it('returns 403 when user has no tenantId in strict mode', () => {
|
||||
it('returns 403 when user has no tenantId in strict mode', async () => {
|
||||
process.env.TENANT_ISOLATION_STRICT = 'true';
|
||||
_resetTenantMiddlewareStrictCache();
|
||||
|
||||
|
|
@ -65,7 +82,7 @@ describe('tenantContextMiddleware', () => {
|
|||
const res = mockRes();
|
||||
const next: NextFunction = jest.fn();
|
||||
|
||||
tenantContextMiddleware(req, res, next);
|
||||
await tenantContextMiddleware(req, res, next);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
expect(res.json).toHaveBeenCalledWith(
|
||||
|
|
@ -99,3 +116,119 @@ describe('tenantContextMiddleware', () => {
|
|||
expect(results).toContain('tenant-2');
|
||||
});
|
||||
});
|
||||
|
||||
describe('restoreTenantContextFromReq', () => {
|
||||
afterEach(() => {
|
||||
_resetTenantMiddlewareStrictCache();
|
||||
delete process.env.TENANT_ISOLATION_STRICT;
|
||||
unlinkMock.mockClear();
|
||||
});
|
||||
|
||||
it('restores ALS tenant context from req.user.tenantId', async () => {
|
||||
const req = mockReq({ tenantId: 'tenant-user', role: 'user' });
|
||||
const res = mockRes();
|
||||
|
||||
const tenantId = await new Promise<string | undefined>((resolve) => {
|
||||
const next: NextFunction = async () => {
|
||||
await Promise.resolve();
|
||||
resolve(getTenantId());
|
||||
};
|
||||
restoreTenantContextFromReq(req, res, next);
|
||||
});
|
||||
|
||||
expect(tenantId).toBe('tenant-user');
|
||||
});
|
||||
|
||||
it('prefers server-resolved req.tenantId over req.user.tenantId', async () => {
|
||||
const req = mockTenantReq({ tenantId: 'tenant-user', role: 'user' }, 'tenant-request');
|
||||
const res = mockRes();
|
||||
|
||||
const tenantId = await new Promise<string | undefined>((resolve) => {
|
||||
restoreTenantContextFromReq(req, res, () => {
|
||||
resolve(getTenantId());
|
||||
});
|
||||
});
|
||||
|
||||
expect(tenantId).toBe('tenant-request');
|
||||
});
|
||||
|
||||
it('returns 403 in strict mode when no request tenant can be resolved', async () => {
|
||||
process.env.TENANT_ISOLATION_STRICT = 'true';
|
||||
_resetTenantMiddlewareStrictCache();
|
||||
|
||||
const req = mockReq({ role: 'user' });
|
||||
const res = mockRes();
|
||||
const next: NextFunction = jest.fn();
|
||||
|
||||
await restoreTenantContextFromReq(req, res, next);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
expect(res.json).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ error: expect.stringContaining('Tenant context required') }),
|
||||
);
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('deletes uploaded temp files before rejecting strict requests without a tenant', async () => {
|
||||
process.env.TENANT_ISOLATION_STRICT = 'true';
|
||||
_resetTenantMiddlewareStrictCache();
|
||||
|
||||
const req = {
|
||||
...mockReq({ role: 'user' }),
|
||||
file: { path: '/tmp/no-tenant-upload' },
|
||||
} as ServerRequest;
|
||||
const res = mockRes();
|
||||
const next: NextFunction = jest.fn();
|
||||
|
||||
await restoreTenantContextFromReq(req, res, next);
|
||||
|
||||
expect(unlinkMock).toHaveBeenCalledWith('/tmp/no-tenant-upload');
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('rejects the system tenant sentinel for request-owned work', async () => {
|
||||
const req = mockReq({ tenantId: SYSTEM_TENANT_ID, role: 'user' });
|
||||
const res = mockRes();
|
||||
const next: NextFunction = jest.fn();
|
||||
|
||||
await restoreTenantContextFromReq(req, res, next);
|
||||
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('deletes uploaded temp files before rejecting system-tenant requests', async () => {
|
||||
const req = {
|
||||
...mockReq({ tenantId: SYSTEM_TENANT_ID, role: 'user' }),
|
||||
file: { path: '/tmp/system-tenant-upload' },
|
||||
files: [{ path: '/tmp/system-tenant-extra' }],
|
||||
} as ServerRequest;
|
||||
const res = mockRes();
|
||||
const next: NextFunction = jest.fn();
|
||||
|
||||
await restoreTenantContextFromReq(req, res, next);
|
||||
|
||||
expect(unlinkMock).toHaveBeenCalledWith('/tmp/system-tenant-upload');
|
||||
expect(unlinkMock).toHaveBeenCalledWith('/tmp/system-tenant-extra');
|
||||
expect(res.status).toHaveBeenCalledWith(403);
|
||||
expect(res.json).toHaveBeenCalledWith({
|
||||
error: 'System tenant is not allowed for request-scoped routes',
|
||||
});
|
||||
expect(next).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
describe('resolveRequestTenantId', () => {
|
||||
it('uses req.tenantId before req.user.tenantId', () => {
|
||||
const req = mockTenantReq({ tenantId: 'tenant-user', role: 'user' }, 'tenant-request');
|
||||
|
||||
expect(resolveRequestTenantId(req)).toBe('tenant-request');
|
||||
});
|
||||
|
||||
it('falls back to req.user.tenantId', () => {
|
||||
const req = mockReq({ tenantId: 'tenant-user', role: 'user' });
|
||||
|
||||
expect(resolveRequestTenantId(req)).toBe('tenant-user');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -5,7 +5,11 @@ export * from './notFound';
|
|||
export * from './balance';
|
||||
export * from './json';
|
||||
export * from './capabilities';
|
||||
export { tenantContextMiddleware } from './tenant';
|
||||
export {
|
||||
tenantContextMiddleware,
|
||||
restoreTenantContextFromReq,
|
||||
resolveRequestTenantId,
|
||||
} from './tenant';
|
||||
export { preAuthTenantMiddleware } from './preAuthTenant';
|
||||
export * from './concurrency';
|
||||
export * from './checkBalance';
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import { unlink } from 'fs/promises';
|
||||
import { isMainThread } from 'worker_threads';
|
||||
import { tenantStorage, logger } from '@librechat/data-schemas';
|
||||
import { getTenantId, tenantStorage, logger, SYSTEM_TENANT_ID } from '@librechat/data-schemas';
|
||||
import type { Response, NextFunction } from 'express';
|
||||
import type { ServerRequest } from '~/types/http';
|
||||
|
||||
|
|
@ -68,3 +69,117 @@ export function tenantContextMiddleware(
|
|||
next();
|
||||
});
|
||||
}
|
||||
|
||||
export type RequestTenantSource = {
|
||||
tenantId?: string;
|
||||
user?: { tenantId?: string } | null;
|
||||
};
|
||||
|
||||
export function resolveRequestTenantId(req: RequestTenantSource): string | undefined {
|
||||
return req.tenantId ?? req.user?.tenantId;
|
||||
}
|
||||
|
||||
type UploadFile = {
|
||||
path?: string;
|
||||
};
|
||||
|
||||
type UploadRequest = ServerRequest & {
|
||||
file?: UploadFile;
|
||||
files?: UploadFile[] | Record<string, UploadFile[]>;
|
||||
};
|
||||
|
||||
function collectUploadPaths(req: UploadRequest): string[] {
|
||||
const paths = new Set<string>();
|
||||
if (req.file?.path) {
|
||||
paths.add(req.file.path);
|
||||
}
|
||||
const { files } = req;
|
||||
if (Array.isArray(files)) {
|
||||
files.forEach((file) => {
|
||||
if (file.path) {
|
||||
paths.add(file.path);
|
||||
}
|
||||
});
|
||||
} else if (files) {
|
||||
Object.values(files).forEach((uploads) => {
|
||||
uploads.forEach((file) => {
|
||||
if (file.path) {
|
||||
paths.add(file.path);
|
||||
}
|
||||
});
|
||||
});
|
||||
}
|
||||
return [...paths];
|
||||
}
|
||||
|
||||
async function cleanupUploadedFiles(req: ServerRequest): Promise<void> {
|
||||
const paths = collectUploadPaths(req as UploadRequest);
|
||||
if (paths.length === 0) {
|
||||
return;
|
||||
}
|
||||
const results = await Promise.allSettled(paths.map((filepath) => unlink(filepath)));
|
||||
results.forEach((result, index) => {
|
||||
if (result.status === 'rejected') {
|
||||
logger.error('[restoreTenantContextFromReq] Failed to delete rejected upload:', {
|
||||
path: paths[index],
|
||||
error: result.reason,
|
||||
});
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
async function rejectRequestWithUploadCleanup(
|
||||
req: ServerRequest,
|
||||
res: Response,
|
||||
message: string,
|
||||
): Promise<void> {
|
||||
await cleanupUploadedFiles(req);
|
||||
res.status(403).json({ error: message });
|
||||
}
|
||||
|
||||
/**
|
||||
* Re-enters tenant ALS from the server-resolved request tenant.
|
||||
*
|
||||
* Use this after middleware that may cross async stream boundaries (for example
|
||||
* multipart parsers) and before tenant-isolated model calls. The tenant source
|
||||
* is restricted to authenticated/resolved request fields, never form data.
|
||||
*/
|
||||
export function restoreTenantContextFromReq(
|
||||
req: ServerRequest,
|
||||
res: Response,
|
||||
next: NextFunction,
|
||||
): void | Promise<void> {
|
||||
const tenantId = resolveRequestTenantId(req as RequestTenantSource);
|
||||
|
||||
if (!tenantId) {
|
||||
if (isStrict()) {
|
||||
return rejectRequestWithUploadCleanup(
|
||||
req,
|
||||
res,
|
||||
'Tenant context required in strict isolation mode',
|
||||
);
|
||||
}
|
||||
next();
|
||||
return;
|
||||
}
|
||||
|
||||
if (tenantId === SYSTEM_TENANT_ID) {
|
||||
logger.warn('[restoreTenantContextFromReq] Rejected system tenant for request route', {
|
||||
path: req.path,
|
||||
});
|
||||
return rejectRequestWithUploadCleanup(
|
||||
req,
|
||||
res,
|
||||
'System tenant is not allowed for request-scoped routes',
|
||||
);
|
||||
}
|
||||
|
||||
if (getTenantId() === tenantId) {
|
||||
next();
|
||||
return;
|
||||
}
|
||||
|
||||
return void tenantStorage.run({ tenantId }, async () => {
|
||||
next();
|
||||
});
|
||||
}
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ import type {
|
|||
CreateSkillResult,
|
||||
UpsertSkillFileInput,
|
||||
} from '@librechat/data-schemas';
|
||||
import { resolveRequestTenantId } from '~/middleware/tenant';
|
||||
|
||||
/** Security limits for zip processing. */
|
||||
const MAX_ZIP_BYTES = 50 * 1024 * 1024; // 50 MB compressed
|
||||
|
|
@ -194,6 +195,7 @@ export interface ImportSkillDeps {
|
|||
}
|
||||
|
||||
interface ServerRequest extends Request {
|
||||
tenantId?: string;
|
||||
user: {
|
||||
id: string;
|
||||
_id: Types.ObjectId;
|
||||
|
|
@ -260,7 +262,7 @@ function getAuthorInfo(req: ServerRequest) {
|
|||
const user = req.user;
|
||||
const authorId = (user._id ?? user.id) as unknown as Types.ObjectId;
|
||||
const authorName = user.name ?? user.username ?? 'Unknown';
|
||||
const tenantId = user.tenantId;
|
||||
const tenantId = resolveRequestTenantId(req);
|
||||
return { authorId, authorName, tenantId };
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue