Refactored loading of installable hooks

Move actual loading of package.json files into moduleloader, so it can be done safely (wrapped in a try/catch). refs #3639
balderdashy · Mar 7, 2016 · 3c03d47 · 3c03d47
1 parent ddfcca6
commit 3c03d47
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 6 deletions.
diff --git a/lib/hooks/moduleloader/index.js b/lib/hooks/moduleloader/index.js
@@ -435,8 +435,50 @@ module.exports = function(sails) {
             dirname: path.resolve(sails.config.appPath, 'node_modules'),
             filter: /^(package\.json)$/,
             excludeDirs: /^\./,
-            depth: 3
-          }, cb);
+            // Look inside namespaced folders e.g. node_modules/@sailsjs/sails-hook-foo
+            depth: 3,
+            // Don't actually load the files, since malformed once would cause a crash.
+            // Just keep track of where they are and we'll load them carefully below
+            dontLoad: true
+          }, function(err, modules) {
+            // Now that we have a map of where the package.json files are, flatten that
+            // map and load the files carefully.  Map might look something like:
+            // { angular2:
+            //    { animate: {},
+            //      bundles: { web_worker: undefined },
+            //      es6: { dev: undefined, prod: undefined },
+            //      examples: { router: undefined },
+            //      http: {},
+            //      'package.json': true,
+            //      etc...
+            modules = (function flattenDirectories(modules, foundPackageJsons, path) {
+              // Keep track of package.json files we've found
+              foundPackageJsons = foundPackageJsons || {};
+              // Loop through the keys in the current map object
+              Object.keys(modules).forEach(function(identity) {
+                // If it represents a package.json file, attempt to load it and, if
+                // successful, save it in our set of found files.  If unsuccessful,
+                // just ignore it.
+                if (identity === 'package.json' && modules[identity] === true) {
+                  var filePath = require('path').resolve(sails.config.appPath, 'node_modules', path, identity);
+                  try {
+                    var resolved = require.resolve(filePath);
+                    if (require.cache[resolved]) {delete require.cache[resolved];}
+                    foundPackageJsons[path] = require(filePath);
+                  } catch(e) {
+                    sails.log.verbose('While searching for installable hooks, found invalid package.json file:', filePath);
+                    return;
+                  }
+                }
+                // If the key represents an object, recursively search within it
+                if ('object' === typeof modules[identity]) {
+                  flattenDirectories(modules[identity], foundPackageJsons, path ? path + '/' + identity : identity );
+                }
+              });
+              return foundPackageJsons;
+            })(modules);
+            return cb(null, modules);
+          });
         }
       }, function(err, results) {
         if (err) {return cb(err);}
@@ -451,14 +493,13 @@ module.exports = function(sails) {
         }, {});
 
         try {
-          var modules = flattenNamespacedModules(results.nodeModulesFolder);
 
-          _.extend(hooks, _.reduce(modules, function(memo, module, identity) {
+          _.extend(hooks, _.reduce(results.nodeModulesFolder, function(memo, module, identity) {
 
             // Hooks loaded from "node_modules" need to have "sails.isHook: true" in order for us
             // to know that they are a sails hook
-            if (module['package.json'] && module['package.json'].sails && module['package.json'].sails.isHook) {
-              var hookConfig = module['package.json'].sails;
+            if (module.sails && module.sails.isHook) {
+              var hookConfig = module.sails;
 
               // Determine the name the hook should be added as
               var hookName;

diff --git a/test/integration/hook.3rdparty.test.js b/test/integration/hook.3rdparty.test.js
@@ -475,6 +475,38 @@ describe('hooks :: ', function() {
 
     });
 
+    describe('with an invalid package.json file', function(){
+
+      var sails;
+      before(function(done) {
+        fs.mkdirs(path.resolve(__dirname, "../..", appName, "node_modules", "@my-modules"), function(err) {
+          if (err) {return done(err);}
+          wrench.copyDirSyncRecursive(path.resolve(__dirname, 'fixtures/hooks/installable/shout'), path.resolve(__dirname,'../../testApp/node_modules/@my-modules/sails-hook-csrf'));
+          fs.outputFileSync(path.resolve(__dirname,'../../testApp/node_modules/@my-modules/sails-hook-csrf/package.json'), '{"foo":<%=bar%>}');
+          process.chdir(path.resolve(__dirname, "../..", appName));
+          return done();
+        });
+      });
+
+      after(function(done) {
+        sails ? sails.lower(function(err) {
+          process.chdir('../');
+          appHelper.teardown();
+          return done(err);
+        }): done();
+      });
+
+      it('should lift without crashing', function(done) {
+        appHelper.liftQuiet(function(err, _sails) {
+          if (err) {return done(err);}
+          sails = _sails;
+          assert(!sails.hooks.csrf.isShoutyHook);
+          return done();
+        });
+      });
+
+    });
+
   });