Skip to content

Commit

Permalink
fix(builtin): patched methods accept Buffer and URL types
Browse files Browse the repository at this point in the history
The node specification for `fs` methods lists possible types for `path` as string | Buffer | URL. However, because the `path.resolve` and `path.dirname` methods only accept strings, the patched `fs` methods errored if the path arg was a Buffer or URL. This would occur when calling `fs.rm`, for example.
  • Loading branch information
Alek committed Aug 30, 2024
1 parent 55eab9f commit 75e0a3e
Show file tree
Hide file tree
Showing 3 changed files with 120 additions and 16 deletions.
51 changes: 35 additions & 16 deletions internal/node/node_patches.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,15 @@ var __asyncGenerator = (commonjsGlobal && commonjsGlobal.__asyncGenerator) || fu
Object.defineProperty(exports, "__esModule", { value: true });
exports.escapeFunction = exports.isOutPath = exports.patcher = void 0;

function stringifyPath(path) {
if (path instanceof Buffer) {
return path.toString();
} else if (path instanceof URL) {
return path.toString().replace('file://', '');
} else {
return path;
}
}

// using require here on purpose so we can override methods with any
// also even though imports are mutable in typescript the cognitive dissonance is too high because
Expand Down Expand Up @@ -95,7 +104,8 @@ const patcher = (fs = fs__default['default'], roots) => {
args[args.length - 1] = (err, stats) => {
if (err)
return cb(err);
path__default['default'].resolve(args[0]);
const pathString = stringifyPath(args[0]);
path__default['default'].resolve(pathString);
if (!stats.isSymbolicLink()) {
return cb(null, stats);
}
Expand All @@ -116,7 +126,7 @@ const patcher = (fs = fs__default['default'], roots) => {
return cb(err);
}
}
str = path__default['default'].resolve(path__default['default'].dirname(args[0]), str);
str = path__default['default'].resolve(path__default['default'].dirname(pathString), str);
if (isEscape(str, args[0])) {
// if it's an out link we have to return the original stat.
return origStat(args[0], (err, plainStat) => {
Expand All @@ -142,8 +152,9 @@ const patcher = (fs = fs__default['default'], roots) => {
args[args.length - 1] = (err, str) => {
if (err)
return cb(err);
if (isEscape(str, args[0])) {
cb(null, path__default['default'].resolve(args[0]));
const pathString = stringifyPath(args[0]);
if (isEscape(str, pathString)) {
cb(null, path__default['default'].resolve(pathString));
}
else {
cb(null, str);
Expand All @@ -160,8 +171,9 @@ const patcher = (fs = fs__default['default'], roots) => {
args[args.length - 1] = (err, str) => {
if (err)
return cb(err);
if (isEscape(str, args[0])) {
cb(null, path__default['default'].resolve(args[0]));
const pathString = stringifyPath(args[0]);
if (isEscape(str, pathString)) {
cb(null, path__default['default'].resolve(pathString));
}
else {
cb(null, str);
Expand All @@ -176,7 +188,8 @@ const patcher = (fs = fs__default['default'], roots) => {
if (cb) {
cb = once(cb);
args[args.length - 1] = (err, str) => {
args[0] = path__default['default'].resolve(args[0]);
const pathString = stringifyPath(args[0]);
args[0] = path__default['default'].resolve(pathString);
if (str)
str = path__default['default'].resolve(path__default['default'].dirname(args[0]), str);
if (err)
Expand All @@ -196,13 +209,14 @@ const patcher = (fs = fs__default['default'], roots) => {
// tslint:disable-next-line:no-any
fs.lstatSync = (...args) => {
const stats = origLstatSync(...args);
const linkPath = path__default['default'].resolve(args[0]);
const pathString = stringifyPath(args[0]);
const linkPath = path__default['default'].resolve(pathString);
if (!stats.isSymbolicLink()) {
return stats;
}
let linkTarget;
try {
linkTarget = path__default['default'].resolve(path__default['default'].dirname(args[0]), origReadlinkSync(linkPath));
linkTarget = path__default['default'].resolve(path__default['default'].dirname(pathString), origReadlinkSync(linkPath));
}
catch (e) {
if (e.code === 'ENOENT') {
Expand All @@ -227,22 +241,25 @@ const patcher = (fs = fs__default['default'], roots) => {
// tslint:disable-next-line:no-any
fs.realpathSync = (...args) => {
const str = origRealpathSync(...args);
if (isEscape(str, args[0])) {
return path__default['default'].resolve(args[0]);
const pathString = stringifyPath(args[0]);
if (isEscape(str, pathString)) {
return path__default['default'].resolve(pathString);
}
return str;
};
// tslint:disable-next-line:no-any
fs.realpathSync.native = (...args) => {
const str = origRealpathSyncNative(...args);
if (isEscape(str, args[0])) {
return path__default['default'].resolve(args[0]);
const pathString = stringifyPath(args[0]);
if (isEscape(str, pathString)) {
return path__default['default'].resolve(pathString);
}
return str;
};
// tslint:disable-next-line:no-any
fs.readlinkSync = (...args) => {
args[0] = path__default['default'].resolve(args[0]);
const pathString = stringifyPath(args[0]);
args[0] = path__default['default'].resolve(pathString);
const str = path__default['default'].resolve(path__default['default'].dirname(args[0]), origReadlinkSync(...args));
if (isEscape(str, args[0]) || str === args[0]) {
const e = new Error('EINVAL: invalid argument, readlink \'' + args[0] + '\'');
Expand All @@ -254,7 +271,8 @@ const patcher = (fs = fs__default['default'], roots) => {
};
// tslint:disable-next-line:no-any
fs.readdir = (...args) => {
const p = path__default['default'].resolve(args[0]);
const pathString = stringifyPath(args[0]);
const p = path__default['default'].resolve(pathString);
let cb = args[args.length - 1];
if (typeof cb !== 'function') {
// this will likely throw callback required error.
Expand Down Expand Up @@ -284,7 +302,8 @@ const patcher = (fs = fs__default['default'], roots) => {
// tslint:disable-next-line:no-any
fs.readdirSync = (...args) => {
const res = origReaddirSync(...args);
const p = path__default['default'].resolve(args[0]);
const pathString = stringifyPath(args[0]);
const p = path__default['default'].resolve(pathString);
// tslint:disable-next-line:no-any
res.forEach((v) => {
handleDirentSync(p, v);
Expand Down
9 changes: 9 additions & 0 deletions internal/node/test/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -341,6 +341,15 @@ jasmine_node_test(
],
)

jasmine_node_test(
name = "patches_test",
srcs = ["patches.spec.js"],
data = [
"dir_output",
"minified.js",
],
)

[nodejs_toolchain_test(
name = "nodejs_toolchain_%s_test" % platform,
platform = platform,
Expand Down
76 changes: 76 additions & 0 deletions internal/node/test/patches.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
const fs = require('fs');
const path = require('path');
const runfiles = require(process.env['BAZEL_NODE_RUNFILES_HELPER']);

describe('node fs function does not throw:', () => {
const files = runfiles.resolvePackageRelative('dir_output')
const filesBuf = Buffer.from(files);
const filesUrl = new URL('file://' + files);
const doNothingCallback = () => {}

// listed in order of appearance in the patch file
it('lstat', () => {
expect(() => fs.lstat(files, doNothingCallback)).not.toThrow();
expect(() => fs.lstat(filesBuf, doNothingCallback)).not.toThrow();
expect(() => fs.lstat(filesUrl, doNothingCallback)).not.toThrow();
});

it('realpath', () => {
expect(() => fs.realpath(files, undefined, doNothingCallback)).not.toThrow();
expect(() => fs.realpath(filesBuf, undefined, doNothingCallback)).not.toThrow();
expect(() => fs.realpath(filesUrl, undefined, doNothingCallback)).not.toThrow();
});

it('realpath.native', () => {
expect(() => fs.realpath.native(files, undefined, doNothingCallback)).not.toThrow();
expect(() => fs.realpath.native(filesBuf, undefined, doNothingCallback)).not.toThrow();
expect(() => fs.realpath.native(filesUrl, undefined, doNothingCallback)).not.toThrow();
});

it('readlink', () => {
expect(() => fs.readlink(files, undefined, doNothingCallback)).not.toThrow();
expect(() => fs.readlink(filesBuf, undefined, doNothingCallback)).not.toThrow();
expect(() => fs.readlink(filesUrl, undefined, doNothingCallback)).not.toThrow();
});

it('lstatSync', () => {
expect(() => fs.lstatSync(files)).not.toThrow();
expect(() => fs.lstatSync(filesBuf)).not.toThrow();
expect(() => fs.lstatSync(filesUrl)).not.toThrow();
});

it('realpathSync', () => {
expect(() => fs.realpathSync(files)).not.toThrow();
expect(() => fs.realpathSync(filesBuf)).not.toThrow();
expect(() => fs.realpathSync(filesUrl)).not.toThrow();
});

it('realpathSync.native', () => {
expect(() => fs.realpathSync.native(files)).not.toThrow();
expect(() => fs.realpathSync.native(filesBuf)).not.toThrow();
expect(() => fs.realpathSync.native(filesUrl)).not.toThrow();
});

it('readlinkSync', () => {
const symlinkPath = path.join(files, 'symlink');
fs.symlinkSync(files, symlinkPath);
const symlinkPathBuf = Buffer.from(symlinkPath);
const symlinkPathUrl = new URL(`file://${symlinkPath}`);
expect(() => fs.readlinkSync(symlinkPath)).not.toThrow();
expect(() => fs.readlinkSync(symlinkPathBuf)).not.toThrow();
expect(() => fs.readlinkSync(symlinkPathUrl)).not.toThrow();
});

it('readdir', () => {
expect(() => fs.readdir(files, undefined, doNothingCallback)).not.toThrow();
expect(() => fs.readdir(filesBuf, undefined, doNothingCallback)).not.toThrow();
expect(() => fs.readdir(filesUrl, undefined, doNothingCallback)).not.toThrow();
});

it('readdirSync', () => {
expect(() => fs.readdirSync(files)).not.toThrow();
expect(() => fs.readdirSync(filesBuf)).not.toThrow();
expect(() => fs.readdirSync(filesUrl)).not.toThrow();
});

});

0 comments on commit 75e0a3e

Please sign in to comment.