fix shrinkwrap style and refactor install

3 files changed, 139 insertions, 115 deletions

diff --git a/doc/cli/shrinkwrap.md b/doc/cli/shrinkwrap.md
index 9ed7750cb..24c729e33 100644
--- a/doc/cli/shrinkwrap.md
+++ b/doc/cli/shrinkwrap.md
@@ -146,6 +146,14 @@ shrinkwrap is constructed from a valid installation of B and recursively
 specifies all dependencies, the contents of B's shrinkwrap will implicitly be
 included in A's shrinkwrap.
 
+Shrinkwrap files only lock down package versions, not actual package contents.
+While discouraged, a package author can republish an existing version of a
+package, causing shrinkwrapped packages using that version to pick up different
+code than they were before. If you want to avoid any risk that a byzantine
+author replaces a package you're using with code that breaks your application,
+you could modify the shrinkwrap file to use git URL references rather than
+version numbers so that npm always fetches all packages from git.
+
 
 ## SEE ALSO
 
diff --git a/lib/install.js b/lib/install.js
index e8d21449b..f8553324a 100644
--- a/lib/install.js
+++ b/lib/install.js
@@ -3,12 +3,14 @@
 //
 // See doc/install.md for more description
 
-// Managing "family" lists...
-// every time we dive into a deeper node_modules folder, the "family"
-// list that gets passed along uses the previous "family" list as
-// it's __proto__.  Any "resolved precise dependency" things that aren't
-// already on this object get added, and then that's passed to the next
-// generation of installation.
+// Managing contexts...
+// there's a lot of state associated with an "install" operation, including
+// packages that are already installed, parent packages, current shrinkwrap, and
+// so on. We maintain this state in a "context" object that gets passed around.
+// every time we dive into a deeper node_modules folder, the "family" list that
+// gets passed along uses the previous "family" list as its __proto__.  Any
+// "resolved precise dependency" things that aren't already on this object get
+// added, and then that's passed to the next generation of installation.
 
 module.exports = install
 
@@ -112,33 +114,41 @@ function install (args, cb_) {
     if (!args.length) {
       if (npm.config.get("global")) args = ["."]
       else return readDependencies( null
-                          , where
-                          , { dev: !npm.config.get("production") }
-                          , function (er, data) {
+                                  , where
+                                  , { dev: !npm.config.get("production") }
+                                  , function (er, data) {
         if (er) return log.er(cb, "Couldn't read dependencies.")(er)
         var deps = Object.keys(data.dependencies || {})
         log.verbose([where, deps], "where, deps")
-        var family = {}
-          , ancestors = {}
-        family[data.name] = ancestors[data.name] = data.version
+        var context = { family: {}
+                      , ancestors: {}
+                      , explicit: false
+                      , parent: data
+                      , wrap: null }
+        context.family[data.name] = context.ancestors[data.name] = data.version
         installManyTop(deps.map(function (dep) {
           var target = data.dependencies[dep]
             , parsed = url.parse(target.replace(/^git\+/, "git"))
           target = dep + "@" + target
           return target
-        }), where, family, ancestors, null, false, data, cb)
+        }), where, context, cb)
       })
     }
 
     // initial "family" is the name:version of the root, if it's got
-    // a pacakge.json file.
+    // a package.json file.
     readJson(path.resolve(where, "package.json"), function (er, data) {
       if (er) data = null
-      var family = {}
-        , ancestors = {}
-      if (data) family[data.name] = ancestors[data.name] = data.version
+      var context = { family: {}
+                    , ancestors: {}
+                    , explicit: true
+                    , parent: data
+                    , wrap: null }
+      if (data) {
+        context.family[data.name] = context.ancestors[data.name] = data.version
+      }
       var fn = npm.config.get("global") ? installMany : installManyTop
-      fn(args, where, family, ancestors, null, true, data, cb)
+      fn(args, where, context, cb)
     })
   })
 }
@@ -146,9 +156,8 @@ function install (args, cb_) {
 // reads dependencies for the package at "where". There are several cases,
 // depending on our current state and the package's configuration:
 //
-// 1. If "wrap" is specified, then it's assumed we're processing a package
-//    underneath a shrinkwrap, so dependencies are read directly from the
-//    shrinkwrap.
+// 1. If "context" is specified, then we examine the context to see if there's a
+//    shrinkwrap there. In that case, dependencies are read from the shrinkwrap.
 // 2. Otherwise, if an npm-shrinkwrap.json file is present, dependencies are
 //    read from there.
 // 3. Otherwise, dependencies come from package.json.
@@ -158,22 +167,25 @@ function install (args, cb_) {
 // "dependencies" read as described above. The second argument to "cb" is the
 // shrinkwrap to use in processing this package's dependencies, which may be
 // "wrap" (in case 1) or a new shrinkwrap (in case 2).
-function readDependencies (wrap, where, opts, cb)
-{
+function readDependencies (context, where, opts, cb) {
+  var wrap = context ? context.wrap : null
+
   readJson( path.resolve(where, "package.json")
-             , opts
-             , function (er, data) {
+          , opts
+          , function (er, data) {
     if (er)  return cb(er)
 
     if (wrap) {
       log.verbose([where, wrap], "readDependencies: using existing wrap")
       var rv = {}
-      for (var key in data)
+      for (var key in data) {
         rv[key] = data[key]
-      rv["dependencies"] = {}
-      for (key in wrap)
-        rv["dependencies"][key] = wrap[key]["version"]
-      log.verbose([rv["dependencies"]], "readDependencies: returned deps")
+      }
+      rv.dependencies = {}
+      for (key in wrap) {
+        rv.dependencies[key] = wrap[key].version
+      }
+      log.verbose([rv.dependencies], "readDependencies: returned deps")
       return cb(null, rv, wrap)
     }
 
@@ -193,13 +205,15 @@ function readDependencies (wrap, where, opts, cb)
 
       log.info("using shrinkwrap file "+wrapfile)
       var rv = {}
-      for (var key in data)
+      for (var key in data) {
         rv[key] = data[key]
-      rv["dependencies"] = {}
-      for (key in newwrap["dependencies"])
-        rv["dependencies"][key] = newwrap["dependencies"][key]["version"]
-      log.verbose([rv["dependencies"]], "readDependencies: returned deps")
-      return cb(null, rv, newwrap["dependencies"])
+      }
+      rv.dependencies = {}
+      for (key in newwrap.dependencies) {
+        rv.dependencies[key] = newwrap.dependencies[key].version
+      }
+      log.verbose([rv.dependencies], "readDependencies: returned deps")
+      return cb(null, rv, newwrap.dependencies)
     })
   })
 }
@@ -319,11 +333,9 @@ function treeify (installed) {
 
 // just like installMany, but also add the existing packages in
 // where/node_modules to the family object.
-function installManyTop (what, where, family, ancestors, unused, explicit, parent,
-  cb_) {
-
+function installManyTop (what, where, context, cb_) {
   function cb (er, d) {
-    if (explicit || er) return cb_(er, d)
+    if (context.explicit || er) return cb_(er, d)
     // since this wasn't an explicit install, let's build the top
     // folder, so that `npm install` also runs the lifecycle scripts.
     npm.commands.build([where], false, true, function (er) {
@@ -331,7 +343,7 @@ function installManyTop (what, where, family, ancestors, unused, explicit, paren
     })
   }
 
-  if (explicit) return next()
+  if (context.explicit) return next()
 
   readJson(path.join(where, "package.json"), function (er, data) {
     if (er) return next(er)
@@ -340,22 +352,21 @@ function installManyTop (what, where, family, ancestors, unused, explicit, paren
 
   function next (er) {
     if (er) return cb(er)
-    installManyTop_(what, where, family, ancestors, explicit, parent, cb)
+    installManyTop_(what, where, context, cb)
   }
 }
 
-function installManyTop_ (what, where, family, ancestors, explicit, parent, cb) {
+function installManyTop_ (what, where, context, cb) {
   var nm = path.resolve(where, "node_modules")
-    , names = explicit
+    , names = context.explicit
             ? what.map(function (w) { return w.split(/@/).shift() })
             : []
 
   fs.readdir(nm, function (er, pkgs) {
-    if (er) return installMany(what, where, family, ancestors, null, explicit,
-      parent, cb)
+    if (er) return installMany(what, where, context, cb)
     pkgs = pkgs.filter(function (p) {
       return !p.match(/^[\._-]/)
-          && (!explicit || names.indexOf(p) === -1)
+          && (!context.explicit || names.indexOf(p) === -1)
     })
     asyncMap(pkgs.map(function (p) {
       return path.resolve(nm, p, "package.json")
@@ -368,48 +379,44 @@ function installManyTop_ (what, where, family, ancestors, explicit, parent, cb)
       // add all the existing packages to the family list.
       // however, do not add to the ancestors list.
       packages.forEach(function (p) {
-        family[p[0]] = p[1]
+        context.family[p[0]] = p[1]
       })
-      return installMany(what, where, family, ancestors, null, explicit, parent,
-        cb)
+      return installMany(what, where, context, cb)
     })
   })
 }
 
-function installMany (what, where, family, ancestors, oldwrap, explicit, parent,
-  cb) {
+function installMany (what, where, context, cb) {
   // readDependencies takes care of figuring out whether the list of
   // dependencies we'll iterate below comes from an existing shrinkwrap from a
   // parent level, a new shrinkwrap at this level, or package.json at this
   // level, as well as which shrinkwrap (if any) our dependencies should use.
-  readDependencies(oldwrap, where, {}, function (er, data, wrap) {
+  readDependencies(context, where, {}, function (er, data, wrap) {
     if (er) data = {}
 
     var parent = data
 
-    var d = data["dependencies"] || {}
+    var d = data.dependencies || {}
 
     // if we're explicitly installing "what" into "where", then the shrinkwrap
     // for "where" doesn't apply. This would be the case if someone were adding
     // a new package to a shrinkwrapped package. (data.dependencies will not be
     // used here except to indicate what packages are already present, so
     // there's no harm in using that.)
-    if (explicit)
-      wrap = null
+    if (context.explicit) wrap = null
 
     // what is a list of things.
     // resolve each one.
     asyncMap( what
-            , targetResolver(where, family, ancestors, wrap, explicit, d,
-              parent)
+            , targetResolver(where, context, d)
             , function (er, targets) {
 
       if (er) return cb(er)
 
       // each target will be a data object corresponding
       // to a package, folder, or whatever that is in the cache now.
-      var newPrev = Object.create(family)
-        , newAnc = Object.create(ancestors)
+      var newPrev = Object.create(context.family)
+        , newAnc = Object.create(context.ancestors)
 
       newAnc[data.name] = data.version
       targets.forEach(function (t) {
@@ -421,19 +428,25 @@ function installMany (what, where, family, ancestors, oldwrap, explicit, parent,
       })
       asyncMap(targets, function (target, cb) {
         log(target._id, "installOne")
-        var newWrap = wrap ? wrap[target.name]["dependencies"] || {} : null
-        installOne(target, where, newPrev, newAnc, newWrap, parent, cb)
+        var newWrap = wrap ? wrap[target.name].dependencies || {} : null
+        var newContext = { family: newPrev
+                         , ancestors: newAnc
+                         , parent: parent
+                         , explicit: false
+                         , wrap: newWrap }
+        installOne(target, where, newContext, cb)
       }, cb)
     })
   })
 }
 
-function targetResolver (where, family, ancestors, wrap, explicit, deps,
-  parent) {
-  var alreadyInstalledManually = explicit ? [] : null
+function targetResolver (where, context, deps) {
+  var alreadyInstalledManually = context.explicit ? [] : null
     , nm = path.resolve(where, "node_modules")
+    , parent = context.parent
+    , wrap = context.wrap
 
-  if (!explicit) fs.readdir(nm, function (er, inst) {
+  if (!context.explicit) fs.readdir(nm, function (er, inst) {
     if (er) return alreadyInstalledManually = []
     asyncMap(inst, function (pkg, cb) {
       readJson(path.resolve(nm, pkg, "package.json"), function (er, d) {
@@ -463,13 +476,13 @@ function targetResolver (where, family, ancestors, wrap, explicit, deps,
 
     // check for a version installed higher in the tree.
     // If installing from a shrinkwrap, it must match exactly.
-    if (family[what]) {
-      if (wrap && wrap[what]["version"] == family[what]) {
+    if (context.family[what]) {
+      if (wrap && wrap[what].version == context.family[what]) {
         log.verbose("using existing "+what+" (matches shrinkwrap)")
         return cb(null, [])
       }
 
-      if (!wrap && semver.satisfies(family[what], deps[what] || "")) {
+      if (!wrap && semver.satisfies(context.family[what], deps[what] || "")) {
         log.verbose("using existing "+what+" (no shrinkwrap)")
         return cb(null, [])
       }
@@ -478,8 +491,8 @@ function targetResolver (where, family, ancestors, wrap, explicit, deps,
     if (wrap) {
       name = what.split(/@/).shift()
       if (wrap[name]) {
-        log.verbose("shrinkwrap: resolving "+what+" to "+wrap[name]["version"])
-        what = name + "@" + wrap[name]["version"]
+        log.verbose("shrinkwrap: resolving "+what+" to "+wrap[name].version)
+        what = name + "@" + wrap[name].version
       } else {
         log.verbose("shrinkwrap: skipping "+what+" (not in shrinkwrap)")
       }
@@ -493,7 +506,7 @@ function targetResolver (where, family, ancestors, wrap, explicit, deps,
         log.warn(what, "optional dependency failed, continuing")
         return cb(null, [])
       }
-      if (!er && data && family[data.name] === data.version) {
+      if (!er && data && context.family[data.name] === data.version) {
         return cb(null, [])
       }
       return cb(er, data)
@@ -503,20 +516,21 @@ function targetResolver (where, family, ancestors, wrap, explicit, deps,
 
 // we've already decided to install this.  if anything's in the way,
 // then uninstall it first.
-function installOne (target, where, family, ancestors, wrap, parent, cb) {
+function installOne (target, where, context, cb) {
   // the --link flag makes this a "link" command if it's at the
   // the top level.
   if (where === npm.prefix && npm.config.get("link")
       && !npm.config.get("global")) {
-    return localLink(target, where, family, ancestors, wrap, parent, cb)
+    return localLink(target, where, context, cb)
   }
-  installOne_(target, where, family, ancestors, wrap, parent, cb)
+  installOne_(target, where, context, cb)
 }
 
-function localLink (target, where, family, ancestors, parent, cb) {
+function localLink (target, where, context, cb) {
   log.verbose(target._id, "try to link")
   var jsonFile = path.resolve( npm.dir, target.name
                              , "package.json" )
+    , parent = context.parent
 
   readJson(jsonFile, function (er, data) {
     if (er || data._id === target._id) {
@@ -538,7 +552,7 @@ function localLink (target, where, family, ancestors, parent, cb) {
 
     } else {
       log.verbose(target._id, "install locally (no link)")
-      installOne_(target, where, family, ancestors, wrap, parent, cb)
+      installOne_(target, where, context, cb)
     }
   })
 }
@@ -562,18 +576,19 @@ function resultList (target, where, parentId) {
          , parentId && prettyWhere ]
 }
 
-function installOne_ (target, where, family, ancestors, wrap, parent, cb) {
+function installOne_ (target, where, context, cb) {
   var nm = path.resolve(where, "node_modules")
     , targetFolder = path.resolve(nm, target.name)
     , prettyWhere = relativize(where, process.cwd() + "/x")
+    , parent = context.parent
 
   if (prettyWhere === ".") prettyWhere = null
 
   chain
     ( [ [checkEngine, target]
-      , [checkCycle, target, ancestors]
+      , [checkCycle, target, context.ancestors]
       , [checkGit, targetFolder]
-      , [write, target, targetFolder, family, ancestors, wrap] ]
+      , [write, target, targetFolder, context] ]
     , function (er, d) {
         log.verbose(target._id, "installOne cb")
         if (er) return cb(er)
@@ -657,10 +672,11 @@ function checkGit_ (folder, cb) {
   })
 }
 
-function write (target, targetFolder, family, ancestors, wrap, cb_) {
+function write (target, targetFolder, context, cb_) {
   var up = npm.config.get("unsafe-perm")
     , user = up ? null : npm.config.get("user")
     , group = up ? null : npm.config.get("group")
+    , family = context.family
 
   function cb (er, data) {
     // cache.unpack returns the data object, and all we care about
@@ -686,8 +702,13 @@ function write (target, targetFolder, family, ancestors, wrap, cb_) {
       if (er) return cb(er)
 
       // before continuing to installing dependencies, check for a shrinkwrap.
-      readDependencies(wrap, targetFolder, {}, function (er, data, wrap) {
+      readDependencies(context, targetFolder, {}, function (er, data, wrap) {
         var deps = Object.keys(data.dependencies || {})
+        var newcontext = { family: family
+                         , ancestors: context.ancestors
+                         , parent: target
+                         , explicit: false
+                         , wrap: wrap }
         installMany(deps.filter(function (d) {
           // prefer to not install things that are satisfied by
           // something in the "family" list, unless we're installing
@@ -698,8 +719,7 @@ function write (target, targetFolder, family, ancestors, wrap, cb_) {
             , parsed = url.parse(t.replace(/^git\+/, "git"))
           t = d + "@" + t
           return t
-        }), targetFolder, family, ancestors, wrap, false, target,
-          function (er, d) {
+        }), targetFolder, newcontext, function (er, d) {
             log.verbose(targetFolder, "about to build")
             if (er) return cb(er)
             npm.commands.build( [targetFolder]
@@ -708,5 +728,5 @@ function write (target, targetFolder, family, ancestors, wrap, cb_) {
                               , function (er) { return cb(er, d) })
         })
       })
-    } )
+    })
 }
diff --git a/lib/shrinkwrap.js b/lib/shrinkwrap.js
index 8a54dd5d4..f30876598 100644
--- a/lib/shrinkwrap.js
+++ b/lib/shrinkwrap.js
@@ -22,18 +22,20 @@ function shrinkwrap (args, silent, cb) {
     if (er) return cb(er)
 
     var wrapped = {}
-    var nerr
 
-    if (pkginfo['name'])
-      wrapped['name'] = pkginfo['name']
+    if (pkginfo.name) {
+      wrapped.name = pkginfo.name
+    }
 
-    nerr = shrinkwrapPkg(log, pkginfo['name'], pkginfo, wrapped)
-    if (nerr > 0)
-      return cb(new Error('failed with ' + nerr + ' errors'))
+    try {
+      shrinkwrapPkg(log, pkginfo.name, pkginfo, wrapped)
+    } catch (ex) {
+      return cb(ex);
+    }
 
     // leave the version field out of the top-level, since it's not used and
     // could only be confusing if it gets out of date.
-    delete wrapped['version']
+    delete wrapped.version
 
     fs.writeFile( path.join(process.cwd(), "npm-shrinkwrap.json")
                 , new Buffer(JSON.stringify(wrapped, null, 2) + "\n")
@@ -47,38 +49,32 @@ function shrinkwrap (args, silent, cb) {
 }
 
 function shrinkwrapPkg (log, pkgname, pkginfo, rv) {
-  var pkg, dep, nerr
+  var pkg, dep
 
-  if (typeof (pkginfo) == 'string') {
-    log.error('required dependency not installed: ' + pkgname + '@' + pkginfo)
-    return (1)
-  }
+  if (typeof (pkginfo) == 'string')
+    throw (new Error('required dependency not installed: ' + pkgname +
+        '@' + pkginfo))
 
-  if ('version' in pkginfo)
-    rv['version'] = pkginfo['version']
+  if (pkginfo.hasOwnProperty('version')) {
+    rv.version = pkginfo.version
+  }
 
-  if (Object.keys(pkginfo['dependencies']).length === 0)
-    return (0)
+  if (Object.keys(pkginfo.dependencies).length === 0) return;
 
-  rv['dependencies'] = {}
-  nerr = 0
+  rv.dependencies = {}
 
-  for (pkg in pkginfo['dependencies']) {
-    dep = pkginfo['dependencies'][pkg]
-    rv['dependencies'][pkg] = {}
-    nerr += shrinkwrapPkg(log, pkg, dep, rv['dependencies'][pkg])
+  for (pkg in pkginfo.dependencies) {
+    dep = pkginfo.dependencies[pkg]
+    rv.dependencies[pkg] = {}
+    shrinkwrapPkg(log, pkg, dep, rv.dependencies[pkg])
 
     // package.json must be consistent with the shrinkwrap bundle
-    if (dep['extraneous']) {
-      log.error('package is extraneous: ' + pkg + '@' + dep['version'])
-      nerr++
+    if (dep.extraneous) {
+      throw (new Error('package is extraneous: ' + pkg + '@' + dep.version))
     }
 
-    if (dep['invalid']) {
-      log.error('package is invalid: ' + pkg)
-      nerr++
+    if (dep.invalid) {
+      throw (new Error('package is invalid: ' + pkg))
     }
   }
-
-  return (nerr)
 }