Skip to content

Commit 720cdfd

Browse files
authored
fix: prevent path traversal via directory prefix collision in _joinDirectoryName (#1062)
The startsWith check compared string prefixes, not filesystem paths. When uploadDir lacks a trailing separator (the default), sibling directories sharing the same prefix bypass the check. Use path.resolve + path.sep to ensure the resolved path is inside the upload directory, not just sharing a string prefix. Fixes #1061
1 parent 5cf2889 commit 720cdfd

File tree

2 files changed

+30
-3
lines changed

2 files changed

+30
-3
lines changed

src/Formidable.js

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -596,14 +596,17 @@ class IncomingForm extends EventEmitter {
596596
}
597597

598598
_joinDirectoryName(name) {
599-
const newPath = path.join(this.uploadDir, name);
599+
const resolvedDir = path.resolve(this.uploadDir);
600+
const resolvedPath = path.resolve(resolvedDir, name);
600601

601602
// prevent directory traversal attacks
602-
if (!newPath.startsWith(this.uploadDir)) {
603+
// use resolvedDir + path.sep to avoid prefix collisions with sibling directories
604+
// e.g. uploadDir "/tmp/uploads" should not allow writes to "/tmp/uploads-evil/"
605+
if (resolvedPath === resolvedDir || !resolvedPath.startsWith(resolvedDir + path.sep)) {
603606
return path.join(this.uploadDir, this.options.defaultInvalidName);
604607
}
605608

606-
return newPath;
609+
return resolvedPath;
607610
}
608611

609612
_setUpRename() {

test/unit/formidable.test.js

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -307,4 +307,28 @@ function makeHeader(originalFilename) {
307307
// originalFilename: (_) => path.join(__dirname, 'sasa'),
308308
// });
309309
// });
310+
311+
test(`${name}#_joinDirectoryName blocks standard directory traversal`, () => {
312+
const form = getForm(name, { uploadDir: '/tmp/uploads' });
313+
const result = form._joinDirectoryName('../../../etc/passwd');
314+
expect(result).toBe(path.join('/tmp/uploads', 'invalid-name'));
315+
});
316+
317+
test(`${name}#_joinDirectoryName blocks sibling directory prefix collision`, () => {
318+
const form = getForm(name, { uploadDir: '/tmp/uploads' });
319+
const result = form._joinDirectoryName('../uploads-evil/payload.sh');
320+
expect(result).toBe(path.join('/tmp/uploads', 'invalid-name'));
321+
});
322+
323+
test(`${name}#_joinDirectoryName allows subdirectories within uploadDir`, () => {
324+
const form = getForm(name, { uploadDir: '/tmp/uploads' });
325+
const result = form._joinDirectoryName('images/photo.jpg');
326+
expect(result).toBe(path.resolve('/tmp/uploads', 'images/photo.jpg'));
327+
});
328+
329+
test(`${name}#_joinDirectoryName blocks name resolving to uploadDir itself`, () => {
330+
const form = getForm(name, { uploadDir: '/tmp/uploads' });
331+
const result = form._joinDirectoryName('.');
332+
expect(result).toBe(path.join('/tmp/uploads', 'invalid-name'));
333+
});
310334
});

0 commit comments

Comments
 (0)