From b06e7ef8767d33ccc3344a4fafd2f6f8484e9f18 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 3 Jul 2019 11:32:32 -0700 Subject: [PATCH 1/3] Handle EISDIR from lchown in node <10.6 Fix #20 --- chownr.js | 57 +++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 47 insertions(+), 10 deletions(-) diff --git a/chownr.js b/chownr.js index 7e63928..b056912 100644 --- a/chownr.js +++ b/chownr.js @@ -7,6 +7,36 @@ const LCHOWN = fs.lchown ? 'lchown' : 'chown' /* istanbul ignore next */ const LCHOWNSYNC = fs.lchownSync ? 'lchownSync' : 'chownSync' +const needEISDIRHandled = fs.lchown && + !process.version.match(/v1[1-9]+\./) && + !process.version.match(/v10\.[6-9]/) + +/* istanbul ignore next */ +const handleEISDIR = + needEISDIRHandled ? (path, uid, gid, cb) => er => { + // Node prior to v10 had a very questionable implementation of + // fs.lchown, which would always try to call fs.open on a directory + // Fall back to fs.chown in those cases. + if (!er || er.code !== 'EISDIR') + cb(er) + else + fs.chown(path, uid, gid, cb) + } + : (_, __, ___, cb) => cb + +/* istanbul ignore next */ +const handleEISDirSync = + needEISDIRHandled ? (path, uid, gid) => { + try { + return fs[LCHOWNSYNC](path, uid, gid) + } catch (er) { + if (er.code !== 'EISDIR') + throw er + fs.chownSync(path, uid, gid) + } + } + : fs[LCHOWNSYNC] + // fs.readdir could only accept an options object as of node v6 const nodeVersion = process.version let readdir = (path, options, cb) => fs.readdir(path, options, cb) @@ -28,10 +58,13 @@ const chownrKid = (p, child, uid, gid, cb) => { chownr(path.resolve(p, child.name), uid, gid, er => { if (er) return cb(er) - fs[LCHOWN](path.resolve(p, child.name), uid, gid, cb) + const cpath = path.resolve(p, child.name) + fs[LCHOWN](cpath, uid, gid, handleEISDIR(cpath, uid, gid, cb)) }) - } else - fs[LCHOWN](path.resolve(p, child.name), uid, gid, cb) + } else { + const cpath = path.resolve(p, child.name) + fs[LCHOWN](cpath, uid, gid, handleEISDIR(cpath, uid, gid, cb)) + } } @@ -41,14 +74,18 @@ const chownr = (p, uid, gid, cb) => { // or doesn't exist. give up. if (er && er.code !== 'ENOTDIR' && er.code !== 'ENOTSUP') return cb(er) - if (er || !children.length) return fs[LCHOWN](p, uid, gid, cb) + if (er || !children.length) + return fs[LCHOWN](p, uid, gid, handleEISDIR(p, uid, gid, cb)) let len = children.length let errState = null const then = er => { - if (errState) return - if (er) return cb(errState = er) - if (-- len === 0) return fs[LCHOWN](p, uid, gid, cb) + if (errState) + return + if (er) + return cb(errState = er) + if (-- len === 0) + return fs[LCHOWN](p, uid, gid, handleEISDIR(p, uid, gid, cb)) } children.forEach(child => chownrKid(p, child, uid, gid, then)) @@ -65,7 +102,7 @@ const chownrKidSync = (p, child, uid, gid) => { if (child.isDirectory()) chownrSync(path.resolve(p, child.name), uid, gid) - fs[LCHOWNSYNC](path.resolve(p, child.name), uid, gid) + handleEISDirSync(path.resolve(p, child.name), uid, gid) } const chownrSync = (p, uid, gid) => { @@ -74,14 +111,14 @@ const chownrSync = (p, uid, gid) => { children = readdirSync(p, { withFileTypes: true }) } catch (er) { if (er && er.code === 'ENOTDIR' && er.code !== 'ENOTSUP') - return fs[LCHOWNSYNC](p, uid, gid) + return handleEISDirSync(p, uid, gid) throw er } if (children.length) children.forEach(child => chownrKidSync(p, child, uid, gid)) - return fs[LCHOWNSYNC](p, uid, gid) + return handleEISDirSync(p, uid, gid) } module.exports = chownr From 32723fe6234e776e0008adf084bccd53a57364ef Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 3 Jul 2019 11:38:32 -0700 Subject: [PATCH 2/3] Remove fundamentally broken symlink tests These tests depend on being able to lchown a symbolic link to /bin/sh, and rely on the assumption that /bin/sh is not owned by the current user. The first assumption will typically fail on Linux whenever the second assumption is true! Fixing the first, by linking to a local file instead, means that the second is violated. There is no way to save these tests, so they are being removed. --- test/symlink-sync.js | 61 ----------------------------------------- test/symlink.js | 65 -------------------------------------------- 2 files changed, 126 deletions(-) delete mode 100644 test/symlink-sync.js delete mode 100644 test/symlink.js diff --git a/test/symlink-sync.js b/test/symlink-sync.js deleted file mode 100644 index 2faa3d4..0000000 --- a/test/symlink-sync.js +++ /dev/null @@ -1,61 +0,0 @@ -if (!process.getuid || !process.getgid) { - throw new Error("Tests require getuid/getgid support") -} - -var curUid = +process.getuid() -, curGid = +process.getgid() -, chownr = require("../") -, test = require("tap").test -, mkdirp = require("mkdirp") -, rimraf = require("rimraf") -, fs = require("fs") - -// sniff the 'id' command for other groups that i can legally assign to -var exec = require("child_process").exec -, groups - -exec("id", function (code, output) { - if (code) throw new Error("failed to run 'id' command") - groups = output.trim().split("=")[3].split(",").map(function (s) { - return parseInt(s, 10) - }).filter(function (g) { - return g !== curGid - }) - - // console.error([curUid, groups[0]], "uid, gid") - - rimraf("dir", function (er) { - if (er) throw er - fs.mkdirSync("dir") - fs.symlinkSync("/bin/sh", "dir/sh-link") - runTest() - }) -}) - -function runTest () { - test("should complete successfully", function (t) { - // console.error("calling chownr", curUid, groups[0], typeof curUid, typeof groups[0]) - chownr.sync("dir", curUid, groups[0]) - t.end() - }) - - test("verify "+"sh-link", function (t) { - fs.stat("dir/sh-link", function (er, st) { - if (er) { - t.ifError(er) - return t.end() - } - t.notEqual(st.uid, curUid, "uid not should be " + curUid) - t.notEqual(st.gid, groups[0], "gid not should be "+groups[0]) - t.end() - }) - }) - - test("cleanup", function (t) { - rimraf("dir", function (er) { - t.ifError(er) - t.end() - }) - }) -} - diff --git a/test/symlink.js b/test/symlink.js deleted file mode 100644 index 9811558..0000000 --- a/test/symlink.js +++ /dev/null @@ -1,65 +0,0 @@ -if (!process.getuid || !process.getgid) { - throw new Error("Tests require getuid/getgid support") -} - -var curUid = +process.getuid() -, curGid = +process.getgid() -, chownr = require("../") -, test = require("tap").test -, mkdirp = require("mkdirp") -, rimraf = require("rimraf") -, fs = require("fs") - -// sniff the 'id' command for other groups that i can legally assign to -var exec = require("child_process").exec -, groups -, dirs = [] - -exec("id", function (code, output) { - if (code) throw new Error("failed to run 'id' command") - groups = output.trim().split("=")[3].split(",").map(function (s) { - return parseInt(s, 10) - }).filter(function (g) { - return g !== curGid - }) - - // console.error([curUid, groups[0]], "uid, gid") - - rimraf("dir", function (er) { - if (er) throw er - fs.mkdirSync("dir") - fs.symlinkSync("/bin/sh", "dir/sh-link") - runTest() - }) -}) - -function runTest () { - test("should complete successfully", function (t) { - // console.error("calling chownr", curUid, groups[0], typeof curUid, typeof groups[0]) - chownr("dir", curUid, groups[0], function (er) { - t.ifError(er) - t.end() - }) - }) - - test("verify "+"sh-link", function (t) { - fs.stat("dir/sh-link", function (er, st) { - if (er) { - t.ifError(er) - return t.end() - } - t.notEqual(st.uid, curUid, "uid not should be " + curUid) - t.notEqual(st.gid, groups[0], "gid not should be "+groups[0]) - t.end() - }) - }) - - - test("cleanup", function (t) { - rimraf("dir", function (er) { - t.ifError(er) - t.end() - }) - }) -} - From da434c3be622ecafac4bba86d2e090fce02560f2 Mon Sep 17 00:00:00 2001 From: isaacs Date: Wed, 3 Jul 2019 14:25:58 -0700 Subject: [PATCH 3/3] .travis version update --- .travis.yml | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index 77b2657..66e2818 100644 --- a/.travis.yml +++ b/.travis.yml @@ -1,11 +1,15 @@ language: node_js -sudo: false + node_js: - - 6 - - 8 + - node + - 12 - 10 -notifications: - email: false + - 8 + - 6 + cache: directories: - $HOME/.npm + +notifications: + email: false