From c80d728a5842f1e9964b9f6ffcbb726aee983684 Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Sat, 2 Mar 2019 06:52:09 -0800 Subject: [PATCH] =?UTF-8?q?[BUGFIX]=20restore=20old=20behavior=20=E2=80=93?= =?UTF-8?q?=20hashing=20garbage=20input=20should=20not=20error?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit hashForDep with invalid input, would not error rather would produce a checksum. So restoring this for now, although we should remove in the next major version. One deviation from the old behavior is to ensure the all garbage input doesn’t produce the same hash, rather it will get a hash based on inputs provided. Context: * https://github.com/emberjs/ember.js/pull/17679 * https://github.com/emberjs/ember.js/issues/17678 --- lib/module-entry.js | 20 ++++++++++++++++---- tests/hash-for-dep-test.js | 7 ++++++- tests/module-entry-test.js | 20 ++++++++++++-------- 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/lib/module-entry.js b/lib/module-entry.js index 77455ac..88e7728 100644 --- a/lib/module-entry.js +++ b/lib/module-entry.js @@ -17,7 +17,7 @@ var crypto = require('crypto'); */ module.exports = ModuleEntry; -function ModuleEntry(name, version, rootDir, sourceHash) { +function ModuleEntry(name, version, rootDir, sourceHash, isValid) { // The name of the module, from its package.json file. this.name = name; @@ -43,6 +43,8 @@ function ModuleEntry(name, version, rootDir, sourceHash) { // to the dependencies listed in the package.json file. // See locate() for initialization of this value. this._dependencies = Object.create(null); + + this.isValid = isValid === false ? false : true; } ModuleEntry.prototype.addDependency = function(dependency, moduleEntry) { @@ -115,6 +117,17 @@ ModuleEntry.prototype.getHash = function(heimdallNode) { return (this._hash = hash); }; +/* + * This exists to maintain compatibbility with some unexpected old behavior. + * Specifically invalid input, would still produce a hash. + * + * This is clearly not helpful, but to fix a regression we will restore this + * behavior, but the next major version (2.x.x) we should remove support for + * this, and instead error with a helpful error message + */ +ModuleEntry.buildInvalidModule = function(name, dir) { + return new this(name, '0.0.0' , dir, crypto.createHash('sha1').update([name, dir].filter(Boolean).join('\x00')).digest('hex'), false); +}; /* * Compute and return the ModuleEntry for a given name/dir pair. This is done @@ -139,7 +152,7 @@ ModuleEntry.locate = function(caches, name, dir, hashTreeFn) { if (realPathKey !== null) { return caches.MODULE_ENTRY.get(realPathKey); } else { - return null; + return this.buildInvalidModule(name, dir); } } @@ -150,8 +163,7 @@ ModuleEntry.locate = function(caches, name, dir, hashTreeFn) { var realPath = resolvePackagePath(name, dir, caches);; if (realPath === null) { - caches.PATH.set(nameDirKey, null); - return null; + return this.buildInvalidModule(name, dir); } // We have a path to a file that supposedly is package.json. We need to be sure diff --git a/tests/hash-for-dep-test.js b/tests/hash-for-dep-test.js index caaa5f9..1db86f4 100644 --- a/tests/hash-for-dep-test.js +++ b/tests/hash-for-dep-test.js @@ -69,6 +69,11 @@ describe('hashForDep', function() { assert.equal(result, 'b2d270f1274267a5fe29a49b5d44bb86125977f9', 'Expected sha1'); }); + it('maintains compatibility for now, not erroring for garbage input', function() { + expect(hashForDep('garbage-non-existing')).to.eql('c5c7d7981c22e790055a9b6e98a2972b8d14a599'); + expect(hashForDep('garbage-non-existing', 'with-garbage-basedir')).to.eql('507cb109ec6bd0781751f7eb65faf98183c79375'); + }); + describe('cache', function() { it('caches', function() { expect(hashForDep._cache.size).to.eql(0); @@ -129,7 +134,7 @@ describe('hashForDep', function() { expect(hashForDep._cache.size).to.eql(4); expect(hashForDep._caches.MODULE_ENTRY.size).to.eql(4); - expect(hashForDep._caches.PATH.size).to.eql(10); + expect(hashForDep._caches.PATH.size).to.eql(9); }); }); }); diff --git a/tests/module-entry-test.js b/tests/module-entry-test.js index 42708c1..be4fd40 100644 --- a/tests/module-entry-test.js +++ b/tests/module-entry-test.js @@ -35,7 +35,7 @@ describe('ModuleEntry', function() { it('._gatherDependencies', function() { // gatherDependencies only occurs during getHash(). - var moduleEntry = ModuleEntry.locate(caches, 'foo', fixturesPath, hashTree); + var moduleEntry = ModuleEntry.locate(caches, 'foo', fixturesPath, hashTree); var dependencies = Object.create(null); moduleEntry._gatherDependencies(dependencies); @@ -62,8 +62,8 @@ describe('ModuleEntry', function() { assert.equal(moduleEntry.rootDir, FOO_DIR_PATH, - 'rootDir must be in node_modules/foo'); - assert.equal(Object.keys(moduleEntry._dependencies).length, 2, 'foo._dependencies must have 2 entries'); + 'rootDir must be in node_modules/foo'); + assert.equal(Object.keys(moduleEntry._dependencies).length, 3, 'foo._dependencies must have 2 entries'); // XXX we should check which dependencies they are. @@ -73,7 +73,7 @@ describe('ModuleEntry', function() { moduleEntry = ModuleEntry.locate(caches, 'foo', fixturesPath, hashTree); var hash2 = moduleEntry.getHash(); - + assert.equal(hash1, hash2, 'getHash should return the same hash after clearing and recalculating'); }); @@ -84,7 +84,7 @@ describe('ModuleEntry', function() { var moduleEntry = ModuleEntry.locate(caches, 'foo', fixturesPath, newHashFn); assert.equal(moduleEntry.rootDir, FOO_DIR_PATH, 'rootDir must be in node_modules/foo'); - assert.equal(Object.keys(moduleEntry._dependencies).length, 2, 'foo._dependencies must have 2 entries'); + assert.equal(Object.keys(moduleEntry._dependencies).length, 3, 'foo._dependencies must have 2 entries'); var hash1 = moduleEntry.getHash(); @@ -92,15 +92,19 @@ describe('ModuleEntry', function() { moduleEntry = ModuleEntry.locate(caches, 'foo', fixturesPath, newHashFn); var hash2 = moduleEntry.getHash(); - + assert.equal(hash1, hash2, 'getHash should return the same hash after clearing and recalculating'); }); it('.locate (class method) with invalid package', function() { var moduleEntry = ModuleEntry.locate(caches, 'foo2', fixturesPath, hashTree); - assert.isNull(moduleEntry); + var moduleEntry2 = ModuleEntry.locate(caches, 'foo2', fixturesPath, hashTree); + assert.isNotNull(moduleEntry); + var hash1 = moduleEntry.getHash(); + assert.isOk(typeof hash1 === 'string'); + assert.equal(hash1, moduleEntry2.getHash()); }); - + it('.getHash', function() { var moduleEntry = ModuleEntry.locate(caches, 'foo', fixturesPath, hashTree);