Skip to content

Commit c93a41e

Browse files
fs,win: fix bug in paths with trailing slashes
Fixes: nodejs#17801 Refs: nodejs#33831
1 parent 5210cd8 commit c93a41e

File tree

5 files changed

+209
-20
lines changed

5 files changed

+209
-20
lines changed

lib/fs.js

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -384,7 +384,7 @@ function readFile(path, options, callback) {
384384
const req = new FSReqCallback();
385385
req.context = context;
386386
req.oncomplete = readFileAfterOpen;
387-
binding.open(getValidatedPath(path), flagsNumber, 0o666, req);
387+
binding.open(getValidatedPath(path, 'path', true), flagsNumber, 0o666, req);
388388
}
389389

390390
function tryStatSync(fd, isUserFd) {
@@ -436,7 +436,7 @@ function readFileSync(path, options) {
436436

437437
if (options.encoding === 'utf8' || options.encoding === 'utf-8') {
438438
if (!isInt32(path)) {
439-
path = getValidatedPath(path);
439+
path = getValidatedPath(path, 'path', true);
440440
}
441441
return binding.readFileUtf8(path, stringToFlags(options.flag));
442442
}
@@ -530,7 +530,7 @@ function closeSync(fd) {
530530
* @returns {void}
531531
*/
532532
function open(path, flags, mode, callback) {
533-
path = getValidatedPath(path);
533+
path = getValidatedPath(path, 'path', true);
534534
if (arguments.length < 3) {
535535
callback = flags;
536536
flags = 'r';
@@ -559,7 +559,7 @@ function open(path, flags, mode, callback) {
559559
*/
560560
function openSync(path, flags, mode) {
561561
return binding.open(
562-
getValidatedPath(path),
562+
getValidatedPath(path, 'path', true),
563563
stringToFlags(flags),
564564
parseFileMode(mode, 'mode', 0o666),
565565
);
@@ -2336,7 +2336,7 @@ function writeFileSync(path, data, options) {
23362336
// C++ fast path for string data and UTF8 encoding
23372337
if (typeof data === 'string' && (options.encoding === 'utf8' || options.encoding === 'utf-8')) {
23382338
if (!isInt32(path)) {
2339-
path = getValidatedPath(path);
2339+
path = getValidatedPath(path, 'path', true);
23402340
}
23412341

23422342
return binding.writeFileUtf8(
@@ -2981,8 +2981,8 @@ function copyFile(src, dest, mode, callback) {
29812981
mode = 0;
29822982
}
29832983

2984-
src = getValidatedPath(src, 'src');
2985-
dest = getValidatedPath(dest, 'dest');
2984+
src = getValidatedPath(src, 'src', true);
2985+
dest = getValidatedPath(dest, 'dest', true);
29862986
callback = makeCallback(callback);
29872987

29882988
const req = new FSReqCallback();
@@ -3000,8 +3000,8 @@ function copyFile(src, dest, mode, callback) {
30003000
*/
30013001
function copyFileSync(src, dest, mode) {
30023002
binding.copyFile(
3003-
getValidatedPath(src, 'src'),
3004-
getValidatedPath(dest, 'dest'),
3003+
getValidatedPath(src, 'src', true),
3004+
getValidatedPath(dest, 'dest', true),
30053005
mode,
30063006
);
30073007
}

lib/internal/fs/promises.js

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -620,8 +620,8 @@ async function cp(src, dest, options) {
620620
async function copyFile(src, dest, mode) {
621621
return await PromisePrototypeThen(
622622
binding.copyFile(
623-
getValidatedPath(src, 'src'),
624-
getValidatedPath(dest, 'dest'),
623+
getValidatedPath(src, 'src', 'path', true),
624+
getValidatedPath(dest, 'dest', 'path', true),
625625
mode,
626626
kUsePromises,
627627
),
@@ -633,7 +633,7 @@ async function copyFile(src, dest, mode) {
633633
// Note that unlike fs.open() which uses numeric file descriptors,
634634
// fsPromises.open() uses the fs.FileHandle class.
635635
async function open(path, flags, mode) {
636-
path = getValidatedPath(path);
636+
path = getValidatedPath(path, 'path', true);
637637
const flagsNumber = stringToFlags(flags);
638638
mode = parseFileMode(mode, 'mode', 0o666);
639639
return new FileHandle(await PromisePrototypeThen(

lib/internal/fs/streams.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,7 @@ function ReadStream(path, options) {
182182
this.flags = options.flags === undefined ? 'r' : options.flags;
183183
this.mode = options.mode === undefined ? 0o666 : options.mode;
184184

185-
validatePath(this.path);
185+
validatePath(this.path, 'path', true);
186186
} else {
187187
this.fd = getValidatedFd(importFd(this, options));
188188
}
@@ -337,7 +337,7 @@ function WriteStream(path, options) {
337337
this.flags = options.flags === undefined ? 'w' : options.flags;
338338
this.mode = options.mode === undefined ? 0o666 : options.mode;
339339

340-
validatePath(this.path);
340+
validatePath(this.path, 'path', true);
341341
} else {
342342
this.fd = getValidatedFd(importFd(this, options));
343343
}

lib/internal/fs/utils.js

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -709,14 +709,30 @@ const validateOffsetLengthWrite = hideStackFrames(
709709
},
710710
);
711711

712-
const validatePath = hideStackFrames((path, propName = 'path') => {
712+
const validatePath = hideStackFrames((path, propName = 'path', isFile) => {
713713
if (typeof path !== 'string' && !isUint8Array(path)) {
714714
throw new ERR_INVALID_ARG_TYPE.HideStackFramesError(propName, ['string', 'Buffer', 'URL'], path);
715715
}
716716

717717
const pathIsString = typeof path === 'string';
718718
const pathIsUint8Array = isUint8Array(path);
719719

720+
if (isFile) {
721+
const lastCharacter = path[path.length - 1];
722+
if (
723+
lastCharacter === '/' || lastCharacter === 47 ||
724+
(isWindows && (lastCharacter === '\\' || lastCharacter === 92))
725+
) {
726+
throw new ERR_FS_EISDIR({
727+
code: 'ERR_FS_EISDIR',
728+
message: 'is a directory',
729+
path,
730+
syscall: 'validatePath',
731+
errno: ERR_FS_EISDIR,
732+
});
733+
}
734+
}
735+
720736
// We can only perform meaningful checks on strings and Uint8Arrays.
721737
if ((!pathIsString && !pathIsUint8Array) ||
722738
(pathIsString && !StringPrototypeIncludes(path, '\u0000')) ||
@@ -731,11 +747,12 @@ const validatePath = hideStackFrames((path, propName = 'path') => {
731747
);
732748
});
733749

734-
const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => {
735-
const path = toPathIfFileURL(fileURLOrPath);
736-
validatePath(path, propName);
737-
return path;
738-
});
750+
const getValidatedPath =
751+
hideStackFrames((fileURLOrPath, propName = 'path', isFile = false) => {
752+
const path = toPathIfFileURL(fileURLOrPath);
753+
validatePath(path, propName, isFile);
754+
return path;
755+
});
739756

740757
const getValidatedFd = hideStackFrames((fd, propName = 'fd') => {
741758
if (ObjectIs(fd, -0)) {

test/sequential/test-fs-path-dir.js

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const tmpdir = require('../common/tmpdir');
5+
const assert = require('assert');
6+
const path = require('path');
7+
const fs = require('fs');
8+
const URL = require('url').URL;
9+
10+
tmpdir.refresh();
11+
12+
let fileCounter = 1;
13+
const nextFile = () => path.join(tmpdir.path, `file${fileCounter++}`);
14+
15+
const generateStringPath = (file, suffix = '') => file + suffix;
16+
17+
const generateURLPath = (file, suffix = '') =>
18+
new URL('file://' + path.resolve(file) + suffix);
19+
20+
const generateUint8ArrayPath = (file, suffix = '') =>
21+
new Uint8Array(Buffer.from(file + suffix));
22+
23+
const generatePaths = (file, suffix = '') => [
24+
generateStringPath(file, suffix),
25+
generateURLPath(file, suffix),
26+
generateUint8ArrayPath(file, suffix),
27+
];
28+
29+
const generateNewPaths = (suffix = '') => [
30+
generateStringPath(nextFile(), suffix),
31+
generateURLPath(nextFile(), suffix),
32+
generateUint8ArrayPath(nextFile(), suffix),
33+
];
34+
35+
function checkSyncFn(syncFn, p, args, fail) {
36+
const result = fail ? 'must fail' : 'must succeed';
37+
const failMsg = `failed testing sync ${p} ${syncFn.name} ${result}`;
38+
if (!fail) {
39+
try {
40+
syncFn(p, ...args);
41+
} catch (e) {
42+
console.log(failMsg, e);
43+
throw e;
44+
}
45+
} else {
46+
assert.throws(() => syncFn(p, ...args), {
47+
code: 'ERR_FS_EISDIR',
48+
}, failMsg);
49+
}
50+
}
51+
52+
function checkAsyncFn(asyncFn, p, args, fail) {
53+
const result = fail ? 'must fail' : 'must succeed';
54+
const failMsg = `failed testing async ${p} ${asyncFn.name} ${result}`;
55+
if (!fail) {
56+
try {
57+
asyncFn(p, ...args, common.mustCall());
58+
} catch (e) {
59+
console.log(failMsg, e);
60+
throw e;
61+
}
62+
} else {
63+
assert.throws(
64+
() => asyncFn(p, ...args, common.mustNotCall()), {
65+
code: 'ERR_FS_EISDIR',
66+
},
67+
failMsg
68+
);
69+
}
70+
}
71+
72+
function checkPromiseFn(promiseFn, p, args, fail) {
73+
const result = fail ? 'must fail' : 'must succeed';
74+
const failMsg = `failed testing promise ${p} ${promiseFn.name} ${result}`;
75+
if (!fail) {
76+
const r = promiseFn(p, ...args).catch((err) => {
77+
console.log(failMsg, err);
78+
throw err;
79+
});
80+
if (r && r.close) r.close();
81+
} else {
82+
assert.rejects(
83+
promiseFn(p, ...args), {
84+
code: 'ERR_FS_EISDIR',
85+
},
86+
failMsg
87+
).then(common.mustCall());
88+
}
89+
}
90+
91+
function check(asyncFn, syncFn, promiseFn,
92+
{ suffix, fail, args = [] }) {
93+
const file = nextFile();
94+
fs.writeFile(file, 'data', (e) => {
95+
assert.ifError(e);
96+
const paths = generatePaths(file, suffix);
97+
for (const p of paths) {
98+
if (asyncFn) checkAsyncFn(asyncFn, p, args, fail);
99+
if (promiseFn) checkPromiseFn(promiseFn, p, args, fail);
100+
if (syncFn) checkSyncFn(syncFn, p, args, fail);
101+
}
102+
});
103+
}
104+
105+
function checkManyToMany(asyncFn, syncFn, promiseFn,
106+
{ suffix, fail, args = [] }) {
107+
const source = nextFile();
108+
fs.writeFile(source, 'data', (err) => {
109+
assert.ifError(err);
110+
if (asyncFn) {
111+
generateNewPaths(suffix)
112+
.forEach((p) => checkAsyncFn(asyncFn, source, [p, ...args], fail));
113+
}
114+
if (promiseFn) {
115+
generateNewPaths(suffix)
116+
.forEach((p) => checkPromiseFn(promiseFn, source, [p, ...args], fail));
117+
}
118+
if (syncFn) {
119+
generateNewPaths(suffix)
120+
.forEach((p) => checkSyncFn(syncFn, source, [p, ...args], fail));
121+
}
122+
});
123+
}
124+
check(fs.readFile, fs.readFileSync, fs.promises.readFile,
125+
{ suffix: '', fail: false });
126+
check(fs.readFile, fs.readFileSync, fs.promises.readFile,
127+
{ suffix: '/', fail: true });
128+
check(fs.readFile, fs.readFileSync, fs.promises.readFile,
129+
{ suffix: '\\', fail: true });
130+
check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
131+
{ suffix: '', fail: false, args: ['data'] });
132+
check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
133+
{ suffix: '/', fail: true, args: ['data'] });
134+
check(fs.writeFile, fs.writeFileSync, fs.promises.writeFile,
135+
{ suffix: '\\', fail: true, args: ['data'] });
136+
check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
137+
{ suffix: '', fail: false, args: ['data'] });
138+
check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
139+
{ suffix: '/', fail: true, args: ['data'] });
140+
check(fs.appendFile, fs.appendFileSync, fs.promises.appendFile,
141+
{ suffix: '\\', fail: true, args: ['data'] });
142+
check(undefined, fs.createReadStream, undefined,
143+
{ suffix: '', fail: false });
144+
check(undefined, fs.createReadStream, undefined,
145+
{ suffix: '/', fail: true });
146+
check(undefined, fs.createReadStream, undefined,
147+
{ suffix: '\\', fail: true });
148+
check(undefined, fs.createWriteStream, undefined,
149+
{ suffix: '', fail: false });
150+
check(undefined, fs.createWriteStream, undefined,
151+
{ suffix: '/', fail: true });
152+
check(undefined, fs.createWriteStream, undefined,
153+
{ suffix: '\\', fail: true });
154+
check(fs.truncate, fs.truncateSync, fs.promises.truncate,
155+
{ suffix: '', fail: false, args: [42] });
156+
check(fs.truncate, fs.truncateSync, fs.promises.truncate,
157+
{ suffix: '/', fail: true, args: [42] });
158+
check(fs.truncate, fs.truncateSync, fs.promises.truncate,
159+
{ suffix: '\\', fail: true, args: [42] });
160+
161+
check(fs.open, fs.openSync, fs.promises.open, { suffix: '', fail: false });
162+
check(fs.open, fs.openSync, fs.promises.open, { suffix: '/', fail: true });
163+
check(fs.open, fs.openSync, fs.promises.open, { suffix: '\\', fail: true });
164+
165+
checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
166+
{ suffix: '', fail: false });
167+
168+
checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
169+
{ suffix: '/', fail: true });
170+
171+
checkManyToMany(fs.copyFile, fs.copyFileSync, fs.promises.copyFile,
172+
{ suffix: '\\', fail: true });

0 commit comments

Comments
 (0)