mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-03-15 20:26:33 +01:00
🧹 fix: Sanitize Artifact Filenames in Code Execution Output (#12222)
* fix: sanitize artifact filenames to prevent path traversal in code output * test: Mock sanitizeFilename function in process.spec.js to return the original filename - Added a mock implementation for the `sanitizeFilename` function in the `process.spec.js` test file to return the original filename, ensuring that tests can run without altering the filename during the testing process. * fix: use path.relative for traversal check, sanitize all filenames, add security logging - Replace startsWith with path.relative pattern in saveLocalBuffer, consistent with deleteLocalFile and getLocalFileStream in the same file - Hoist sanitizeFilename call before the image/non-image branch so both code paths store the sanitized name in MongoDB - Log a warning when sanitizeFilename mutates a filename (potential traversal) - Log a specific warning when saveLocalBuffer throws a traversal error, so security events are distinguishable from generic network errors in the catch * test: improve traversal test coverage and remove mock reimplementation - Remove partial sanitizeFilename reimplementation from process-traversal tests; use controlled mock returns to verify processCodeOutput wiring instead - Add test for image branch sanitization - Use mkdtempSync for test isolation in crud-traversal to avoid parallel worker collisions - Add prefix-collision bypass test case (../user10/evil vs user1 directory) * fix: use path.relative in isValidPath to prevent prefix-collision bypass Pre-existing startsWith check without path separator had the same class of prefix-collision vulnerability fixed in saveLocalBuffer.
This commit is contained in:
parent
35a35dc2e9
commit
f67bbb2bc5
5 changed files with 221 additions and 9 deletions
|
|
@ -78,7 +78,13 @@ async function saveLocalBuffer({ userId, buffer, fileName, basePath = 'images' }
|
|||
fs.mkdirSync(directoryPath, { recursive: true });
|
||||
}
|
||||
|
||||
fs.writeFileSync(path.join(directoryPath, fileName), buffer);
|
||||
const resolvedDir = path.resolve(directoryPath);
|
||||
const resolvedPath = path.resolve(resolvedDir, fileName);
|
||||
const rel = path.relative(resolvedDir, resolvedPath);
|
||||
if (rel.startsWith('..') || path.isAbsolute(rel) || rel.includes(`..${path.sep}`)) {
|
||||
throw new Error('Path traversal detected in filename');
|
||||
}
|
||||
fs.writeFileSync(resolvedPath, buffer);
|
||||
|
||||
const filePath = path.posix.join('/', basePath, userId, fileName);
|
||||
|
||||
|
|
@ -165,9 +171,8 @@ async function getLocalFileURL({ fileName, basePath = 'images' }) {
|
|||
}
|
||||
|
||||
/**
|
||||
* Validates if a given filepath is within a specified subdirectory under a base path. This function constructs
|
||||
* the expected base path using the base, subfolder, and user id from the request, and then checks if the
|
||||
* provided filepath starts with this constructed base path.
|
||||
* Validates that a filepath is strictly contained within a subdirectory under a base path,
|
||||
* using path.relative to prevent prefix-collision bypasses.
|
||||
*
|
||||
* @param {ServerRequest} req - The request object from Express. It should contain a `user` property with an `id`.
|
||||
* @param {string} base - The base directory path.
|
||||
|
|
@ -180,7 +185,8 @@ async function getLocalFileURL({ fileName, basePath = 'images' }) {
|
|||
const isValidPath = (req, base, subfolder, filepath) => {
|
||||
const normalizedBase = path.resolve(base, subfolder, req.user.id);
|
||||
const normalizedFilepath = path.resolve(filepath);
|
||||
return normalizedFilepath.startsWith(normalizedBase);
|
||||
const rel = path.relative(normalizedBase, normalizedFilepath);
|
||||
return !rel.startsWith('..') && !path.isAbsolute(rel) && !rel.includes(`..${path.sep}`);
|
||||
};
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue