Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

Commit

Permalink
*: Switch from mkdirp to correctMkdir to preserve perms and owners
Browse files Browse the repository at this point in the history
Switch to correctMkdir as much as possible to ensure that permissions on
directories are correct.
  • Loading branch information
iarna committed Feb 20, 2018
1 parent 17079c2 commit 94227e1
Show file tree
Hide file tree
Showing 9 changed files with 28 additions and 28 deletions.
8 changes: 4 additions & 4 deletions lib/config/core.js
Expand Up @@ -8,7 +8,7 @@ var path = require('path')
var nopt = require('nopt')
var ini = require('ini')
var Umask = configDefs.Umask
var mkdirp = require('mkdirp')
var correctMkdir = require('../utils/correct-mkdir.js')
var umask = require('../utils/umask')
var isWindows = require('../utils/is-windows.js')

Expand Down Expand Up @@ -153,7 +153,7 @@ function load_ (builtin, rc, cli, cb) {
// annoying humans and their expectations!

This comment has been minimized.

Copy link
@bobagold

bobagold Feb 23, 2018

so true :-)

if (conf.get('prefix')) {
var etc = path.resolve(conf.get('prefix'), 'etc')
mkdirp(etc, function () {
correctMkdir(etc, function () {
defaults.globalconfig = path.resolve(etc, 'npmrc')
defaults.globalignorefile = path.resolve(etc, 'npmignore')
afterUserContinuation()
Expand Down Expand Up @@ -235,7 +235,7 @@ Conf.prototype.loadExtras = function (cb) {
this.loadUid(function (er) {
if (er) return cb(er)
// Without prefix, nothing will ever work
mkdirp(this.prefix, cb)
correctMkdir(this.prefix, cb)
}.bind(this))
}.bind(this))
}
Expand Down Expand Up @@ -292,7 +292,7 @@ Conf.prototype.save = function (where, cb) {
done(null)
})
} else {
mkdirp(path.dirname(target.path), function (er) {
correctMkdir(path.dirname(target.path), function (er) {
if (er) return then(er)
fs.writeFile(target.path, data, 'utf8', function (er) {
if (er) return then(er)
Expand Down
4 changes: 2 additions & 2 deletions lib/config/set-user.js
Expand Up @@ -3,7 +3,7 @@ module.exports = setUser
var assert = require('assert')
var path = require('path')
var fs = require('fs')
var mkdirp = require('mkdirp')
var correctMkdir = require('../utils/correct-mkdir.js')

function setUser (cb) {
var defaultConf = this.root
Expand All @@ -19,7 +19,7 @@ function setUser (cb) {
}

var prefix = path.resolve(this.get('prefix'))
mkdirp(prefix, function (er) {
correctMkdir(prefix, function (er) {
if (er) return cb(er)
fs.stat(prefix, function (er, st) {
defaultConf.user = st && st.uid
Expand Down
6 changes: 3 additions & 3 deletions lib/install.js
Expand Up @@ -102,7 +102,7 @@ var readPackageJson = require('read-package-json')
var chain = require('slide').chain
var asyncMap = require('slide').asyncMap
var archy = require('archy')
var mkdirp = require('mkdirp')
var correctMkdir = require('./utils/correct-mkdir.js')
var rimraf = require('rimraf')
var iferr = require('iferr')
var validate = require('aproba')
Expand Down Expand Up @@ -648,7 +648,7 @@ Installer.prototype.readGlobalPackageData = function (cb) {
log.silly('install', 'readGlobalPackageData')
var self = this
this.loadArgMetadata(iferr(cb, function () {
mkdirp(self.where, iferr(cb, function () {
correctMkdir(self.where, iferr(cb, function () {
var pkgs = {}
self.args.forEach(function (pkg) {
pkgs[pkg.name] = true
Expand All @@ -665,7 +665,7 @@ Installer.prototype.readLocalPackageData = function (cb) {
validate('F', arguments)
log.silly('install', 'readLocalPackageData')
var self = this
mkdirp(this.where, iferr(cb, function () {
correctMkdir(this.where, iferr(cb, function () {
readPackageTree(self.where, iferr(cb, function (currentTree) {
self.currentTree = currentTree
self.currentTree.warnings = []
Expand Down
4 changes: 2 additions & 2 deletions lib/install/action/extract.js
Expand Up @@ -5,7 +5,7 @@ const BB = require('bluebird')
const stat = BB.promisify(require('graceful-fs').stat)
const gentlyRm = BB.promisify(require('../../utils/gently-rm.js'))
const log = require('npmlog')
const mkdirp = BB.promisify(require('mkdirp'))
const correctMkdir = BB.promisify(require('../../utils/correct-mkdir'))
const moduleName = require('../../utils/module-name.js')
const moduleStagingPath = require('../module-staging-path.js')
const move = require('../../utils/move.js')
Expand Down Expand Up @@ -142,7 +142,7 @@ function stageBundledModule (bundler, child, staging, parentPath) {
function finishModule (bundler, child, stageTo, stageFrom) {
// If we were the one's who bundled this module…
if (child.fromBundle === bundler) {
return mkdirp(path.dirname(stageTo)).then(() => {
return correctMkdir(path.dirname(stageTo)).then(() => {
return move(stageFrom, stageTo)
})
} else {
Expand Down
6 changes: 3 additions & 3 deletions lib/install/action/finalize.js
Expand Up @@ -3,7 +3,6 @@ const path = require('path')
const fs = require('graceful-fs')
const Bluebird = require('bluebird')
const rimraf = Bluebird.promisify(require('rimraf'))
const mkdirp = Bluebird.promisify(require('mkdirp'))
const lstat = Bluebird.promisify(fs.lstat)
const readdir = Bluebird.promisify(fs.readdir)
const symlink = Bluebird.promisify(fs.symlink)
Expand All @@ -14,6 +13,7 @@ const moveOpts = {fs: fs, Promise: Bluebird, maxConcurrency: 4}
const getRequested = require('../get-requested.js')
const log = require('npmlog')
const packageId = require('../../utils/package-id.js')
const correctMkdir = Bluebird.promisify(require('../../utils/correct-mkdir.js'))

module.exports = function (staging, pkg, log) {
log.silly('finalize', pkg.realpath)
Expand Down Expand Up @@ -48,7 +48,7 @@ module.exports = function (staging, pkg, log) {
}

function makeParentPath (dir) {
return mkdirp(path.dirname(dir))
return correctMkdir(path.dirname(dir))
}

function moveStagingToDestination () {
Expand Down Expand Up @@ -81,7 +81,7 @@ module.exports = function (staging, pkg, log) {
if (!movedDestAway) return
return readdir(path.join(delpath, 'node_modules')).catch(() => []).then((modules) => {
if (!modules.length) return
return mkdirp(path.join(pkg.realpath, 'node_modules')).then(() => Bluebird.map(modules, (file) => {
return correctMkdir(path.join(pkg.realpath, 'node_modules')).then(() => Bluebird.map(modules, (file) => {
const from = path.join(delpath, 'node_modules', file)
const to = path.join(pkg.realpath, 'node_modules', file)
return move(from, to, moveOpts)
Expand Down
6 changes: 3 additions & 3 deletions lib/install/action/move.js
Expand Up @@ -4,7 +4,7 @@ var path = require('path')
var chain = require('slide').chain
var iferr = require('iferr')
var rimraf = require('rimraf')
var mkdirp = require('mkdirp')
var correctMkdir = require('../../utils/correct-mkdir')
var rmStuff = require('../../unbuild.js').rmStuff
var lifecycle = require('../../utils/lifecycle.js')
var move = require('../../utils/move.js')
Expand Down Expand Up @@ -67,7 +67,7 @@ function moveModuleOnly (from, to, log, done) {
function makeDestination (next) {
return function () {
log.silly('move', 'make sure destination parent exists', path.resolve(to, '..'))
mkdirp(path.resolve(to, '..'), iferr(done, moveNodeModules(next)))
correctMkdir(path.resolve(to, '..'), iferr(done, moveNodeModules(next)))
}
}

Expand All @@ -87,7 +87,7 @@ function moveModuleOnly (from, to, log, done) {

function moveNodeModulesBack (next) {
return function () {
mkdirp(from, iferr(done, function () {
correctMkdir(from, iferr(done, function () {
log.silly('move', 'put source node_modules back', fromModules)
move(tempFromModules, fromModules).then(next, done)
}))
Expand Down
4 changes: 2 additions & 2 deletions lib/install/action/remove.js
Expand Up @@ -3,7 +3,7 @@ var path = require('path')
var fs = require('graceful-fs')
var rimraf = require('rimraf')
var asyncMap = require('slide').asyncMap
var mkdirp = require('mkdirp')
var correctMkdir = require('../../utils/correct-mkdir')
var npm = require('../../npm.js')
var andIgnoreErrors = require('../and-ignore-errors.js')
var move = require('../../utils/move.js')
Expand Down Expand Up @@ -52,7 +52,7 @@ function removeDir (pkg, log, next) {
function makeTarget (readdirEr, files) {
if (readdirEr) return cleanup()
if (!files.length) return cleanup()
mkdirp(path.join(pkg.path, 'node_modules'), function (mkdirEr) { moveModules(mkdirEr, files) })
correctMkdir(path.join(pkg.path, 'node_modules'), function (mkdirEr) { moveModules(mkdirEr, files) })
}

function moveModules (mkdirEr, files) {
Expand Down
4 changes: 2 additions & 2 deletions lib/utils/completion/file-completion.js
@@ -1,14 +1,14 @@
module.exports = fileCompletion

var mkdir = require('mkdirp')
var correctMkdir = require('../correct-mkdir.js')
var glob = require('glob')

function fileCompletion (root, req, depth, cb) {
if (typeof cb !== 'function') {
cb = depth
depth = Infinity
}
mkdir(root, function (er) {
correctMkdir(root, function (er) {
if (er) return cb(er)

// can be either exactly the req, or a descendent
Expand Down
14 changes: 7 additions & 7 deletions lib/utils/correct-mkdir.js
Expand Up @@ -6,21 +6,21 @@ var log = require('npmlog')
var mkdirp = require('mkdirp')

// memoize the directories created by this step
var stats = {}
var effectiveOwner
module.exports = function correctMkdir (path, cb) {
cb = dezalgo(cb)
cb = inflight('correctMkdir:' + path, cb)
if (!cb) {
return log.verbose('correctMkdir', path, 'correctMkdir already in flight; waiting')
return log.silly('correctMkdir', path, 'correctMkdir already in flight; waiting')
} else {
log.verbose('correctMkdir', path, 'correctMkdir not in flight; initializing')
log.silly('correctMkdir', path, 'correctMkdir not in flight; initializing')
}
var stats = {}

if (stats[path]) return cb(null, stats[path])

fs.stat(path, function (er, st) {
if (er) return makeDirectory(path, cb)
if (er) return makeDirectory(path, stats, cb)

if (!st.isDirectory()) {
log.error('correctMkdir', 'invalid dir %s', path)
Expand Down Expand Up @@ -60,12 +60,12 @@ function calculateOwner () {
return effectiveOwner
}

function makeDirectory (path, cb) {
function makeDirectory (path, stats, cb) {
cb = inflight('makeDirectory:' + path, cb)
if (!cb) {
return log.verbose('makeDirectory', path, 'creation already in flight; waiting')
return log.silly('makeDirectory', path, 'creation already in flight; waiting')
} else {
log.verbose('makeDirectory', path, 'creation not in flight; initializing')
log.silly('makeDirectory', path, 'creation not in flight; initializing')
}

var owner = calculateOwner()
Expand Down

6 comments on commit 94227e1

@dosire
Copy link

@dosire dosire commented on 94227e1 Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #19883 for a potential problem with this.

@Qix-
Copy link

@Qix- Qix- commented on 94227e1 Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is correctMkdir()? How is that naming convention any better? Why not just name it betterMkdir() or aksjdfkajsdfkj() while you're at it, since both of those are equally as terrible at describing what the function does differently than mkdirp.

@posixpascal
Copy link

@posixpascal posixpascal commented on 94227e1 Feb 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick question: Isn't the problem in the setPermissions function which recursively changes ownership from given path? And somehow / is passed to that setPermissions function? I'm not seeing how using correctMkDir() at multiple places does prevent this. And the added stats property is in a different code branch than setPermission.

Is there a guide to reproduce the error introduced in 5.7.0? Is there any testcase?

Now that correctMkDirp is used in multiple places after this commit I'd like to test it a bit more thoroughly.

@luispabon
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FAIL

@ansballard
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@posixpascal #19883 (comment) the original issue seems to have a lot of info, but it seems like any npm command that is 1) prefixed with sudo and 2) run as a non-root user will change permissions on root directories like /etc and /bin. So spinning up a node docker container, adding a non-root sudo user, and running sudo npm --help or whatever other command should reproduce the issue on 5.7.0.

@gluons
Copy link

@gluons gluons commented on 94227e1 Feb 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When correctMkdir turn into corruptedMkdir. 😛

Please sign in to comment.