From 8cddf9b3aaf42db271d3fbc0c32d2b0e28a5a57a Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Thu, 6 Dec 2018 00:39:39 +0100 Subject: [PATCH 1/2] test: remove common.allowGlobals This removes the globals check that was executed in all tests so far. Instead just rely on the explicit globals test to verify that the globals did not leak. --- test/common/README.md | 6 --- test/common/index.js | 44 ------------------- test/common/index.mjs | 2 - test/fixtures/leakedGlobal.js | 5 --- test/parallel/test-common.js | 9 ---- test/parallel/test-domain-crypto.js | 1 - test/parallel/test-fs-write.js | 2 - test/parallel/test-global.js | 4 +- test/parallel/test-repl-autolibs.js | 3 +- test/parallel/test-repl-reset-event.js | 2 - test/parallel/test-repl.js | 1 - test/parallel/test-timer-immediate.js | 1 - .../test-vm-new-script-this-context.js | 10 +---- test/parallel/test-vm-run-in-new-context.js | 9 +--- test/parallel/test-vm-static-this.js | 9 +--- 15 files changed, 5 insertions(+), 103 deletions(-) delete mode 100644 test/fixtures/leakedGlobal.js diff --git a/test/common/README.md b/test/common/README.md index 0542c38bbcb4..691fcd67b361 100644 --- a/test/common/README.md +++ b/test/common/README.md @@ -33,12 +33,6 @@ The `benchmark` module is used by tests to run benchmarks. The `common` module is used by tests for consistency across repeated tasks. -### allowGlobals(...whitelist) -* `whitelist` [<Array>] Array of Globals -* return [<Array>] - -Takes `whitelist` and concats that with predefined `knownGlobals`. - ### busyLoop(time) * `time` [<number>] diff --git a/test/common/index.js b/test/common/index.js index 8c61fdc5cfcd..d6aa16028e45 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -262,49 +262,6 @@ function platformTimeout(ms) { return ms; // ARMv8+ } -let knownGlobals = [ - clearImmediate, - clearInterval, - clearTimeout, - global, - setImmediate, - setInterval, - setTimeout, - queueMicrotask, -]; - -if (global.gc) { - knownGlobals.push(global.gc); -} - -if (process.env.NODE_TEST_KNOWN_GLOBALS) { - const knownFromEnv = process.env.NODE_TEST_KNOWN_GLOBALS.split(','); - allowGlobals(...knownFromEnv); -} - -function allowGlobals(...whitelist) { - knownGlobals = knownGlobals.concat(whitelist); -} - -function leakedGlobals() { - const leaked = []; - - for (const val in global) { - if (!knownGlobals.includes(global[val])) { - leaked.push(val); - } - } - - return leaked; -} - -process.on('exit', function() { - const leaked = leakedGlobals(); - if (leaked.length > 0) { - assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`); - } -}); - const mustCallChecks = []; function runCallChecks(exitCode) { @@ -715,7 +672,6 @@ function runWithInvalidFD(func) { } module.exports = { - allowGlobals, buildType, busyLoop, canCreateSymLink, diff --git a/test/common/index.mjs b/test/common/index.mjs index 41592098eb50..1fcf40b1ab69 100644 --- a/test/common/index.mjs +++ b/test/common/index.mjs @@ -32,7 +32,6 @@ const { childShouldThrowAndAbort, createZeroFilledFile, platformTimeout, - allowGlobals, mustCall, mustCallAtLeast, hasMultiLocalhost, @@ -78,7 +77,6 @@ export { childShouldThrowAndAbort, createZeroFilledFile, platformTimeout, - allowGlobals, mustCall, mustCallAtLeast, hasMultiLocalhost, diff --git a/test/fixtures/leakedGlobal.js b/test/fixtures/leakedGlobal.js deleted file mode 100644 index 6f4b1b992e25..000000000000 --- a/test/fixtures/leakedGlobal.js +++ /dev/null @@ -1,5 +0,0 @@ -'use strict'; - -require('../common'); - -global.gc = 42; // intentionally leak a global diff --git a/test/parallel/test-common.js b/test/parallel/test-common.js index 62f4eb050fb5..abadf370e8a4 100644 --- a/test/parallel/test-common.js +++ b/test/parallel/test-common.js @@ -26,15 +26,6 @@ const fixtures = require('../common/fixtures'); const assert = require('assert'); const { execFile } = require('child_process'); -// Test for leaked global detection -{ - const p = fixtures.path('leakedGlobal.js'); - execFile(process.execPath, [p], common.mustCall((err, stdout, stderr) => { - assert.notStrictEqual(err.code, 0); - assert.ok(/\bAssertionError\b.*\bUnexpected global\b.*\bgc\b/.test(stderr)); - })); -} - // common.mustCall() tests assert.throws(function() { common.mustCall(function() {}, 'foo'); diff --git a/test/parallel/test-domain-crypto.js b/test/parallel/test-domain-crypto.js index cb397c2fe33e..e6a9881bd0f3 100644 --- a/test/parallel/test-domain-crypto.js +++ b/test/parallel/test-domain-crypto.js @@ -29,7 +29,6 @@ if (!common.hasCrypto) const crypto = require('crypto'); // Pollution of global is intentional as part of test. -common.allowGlobals(require('domain')); // See https://github.com/nodejs/node/commit/d1eff9ab global.domain = require('domain'); diff --git a/test/parallel/test-fs-write.js b/test/parallel/test-fs-write.js index d75974b58d33..8b61a8bd795a 100644 --- a/test/parallel/test-fs-write.js +++ b/test/parallel/test-fs-write.js @@ -36,8 +36,6 @@ const expected = 'ümlaut.'; const constants = fs.constants; /* eslint-disable no-undef */ -common.allowGlobals(externalizeString, isOneByteString, x); - { const expected = 'ümlaut eins'; // Must be a unique string. externalizeString(expected); diff --git a/test/parallel/test-global.js b/test/parallel/test-global.js index ff34a812c592..7b7f97be5e95 100644 --- a/test/parallel/test-global.js +++ b/test/parallel/test-global.js @@ -23,7 +23,7 @@ // treated as a global without being declared with `var`/`let`/`const`. /* eslint-disable strict */ -const common = require('../common'); +require('../common'); const fixtures = require('../common/fixtures'); const assert = require('assert'); @@ -54,8 +54,6 @@ builtinModules.forEach((moduleName) => { assert.deepStrictEqual(new Set(Object.keys(global)), new Set(expected)); } -common.allowGlobals('bar', 'foo'); - baseFoo = 'foo'; // eslint-disable-line no-undef global.baseBar = 'bar'; diff --git a/test/parallel/test-repl-autolibs.js b/test/parallel/test-repl-autolibs.js index 5cf3b1497221..bb288a2f5aba 100644 --- a/test/parallel/test-repl-autolibs.js +++ b/test/parallel/test-repl-autolibs.js @@ -20,7 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. 'use strict'; -const common = require('../common'); +require('../common'); const ArrayStream = require('../common/arraystream'); const assert = require('assert'); const util = require('util'); @@ -63,7 +63,6 @@ function test2() { }; const val = {}; global.url = val; - common.allowGlobals(val); assert(!gotWrite); putIn.run(['url']); assert(gotWrite); diff --git a/test/parallel/test-repl-reset-event.js b/test/parallel/test-repl-reset-event.js index 1f1347547e95..22c316a56725 100644 --- a/test/parallel/test-repl-reset-event.js +++ b/test/parallel/test-repl-reset-event.js @@ -26,8 +26,6 @@ const assert = require('assert'); const repl = require('repl'); const util = require('util'); -common.allowGlobals(42); - // Create a dummy stream that does nothing const dummy = new ArrayStream(); diff --git a/test/parallel/test-repl.js b/test/parallel/test-repl.js index 3cfc105201a9..aa01fc97ac34 100644 --- a/test/parallel/test-repl.js +++ b/test/parallel/test-repl.js @@ -770,7 +770,6 @@ const tcpTests = [ socket.end(); } - common.allowGlobals(...Object.values(global)); })(); function startTCPRepl() { diff --git a/test/parallel/test-timer-immediate.js b/test/parallel/test-timer-immediate.js index b0f52db1b713..328d8dba6667 100644 --- a/test/parallel/test-timer-immediate.js +++ b/test/parallel/test-timer-immediate.js @@ -1,5 +1,4 @@ 'use strict'; const common = require('../common'); global.process = {}; // Boom! -common.allowGlobals(global.process); setImmediate(common.mustCall()); diff --git a/test/parallel/test-vm-new-script-this-context.js b/test/parallel/test-vm-new-script-this-context.js index 18f39f9086ae..3f0b4cb5f26c 100644 --- a/test/parallel/test-vm-new-script-this-context.js +++ b/test/parallel/test-vm-new-script-this-context.js @@ -20,7 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. 'use strict'; -const common = require('../common'); +require('../common'); const assert = require('assert'); const Script = require('vm').Script; @@ -58,11 +58,3 @@ global.f = function() { global.foo = 100; }; script = new Script('f()'); script.runInThisContext(script); assert.strictEqual(global.foo, 100); - -common.allowGlobals( - global.hello, - global.code, - global.foo, - global.obj, - global.f -); diff --git a/test/parallel/test-vm-run-in-new-context.js b/test/parallel/test-vm-run-in-new-context.js index 6e8c42812bbc..92d464b8fee0 100644 --- a/test/parallel/test-vm-run-in-new-context.js +++ b/test/parallel/test-vm-run-in-new-context.js @@ -22,7 +22,7 @@ 'use strict'; // Flags: --expose-gc -const common = require('../common'); +require('../common'); const assert = require('assert'); const vm = require('vm'); @@ -91,10 +91,3 @@ for (const arg of [filename, { filename }]) { return true; }); } - -common.allowGlobals( - global.hello, - global.code, - global.foo, - global.obj -); diff --git a/test/parallel/test-vm-static-this.js b/test/parallel/test-vm-static-this.js index e9382d6c3b4c..54f401832f84 100644 --- a/test/parallel/test-vm-static-this.js +++ b/test/parallel/test-vm-static-this.js @@ -20,7 +20,7 @@ // USE OR OTHER DEALINGS IN THE SOFTWARE. /* eslint-disable strict */ -const common = require('../common'); +require('../common'); const assert = require('assert'); const vm = require('vm'); @@ -56,10 +56,3 @@ assert.strictEqual(global.foo, 1); global.f = function() { global.foo = 100; }; vm.runInThisContext('f()'); assert.strictEqual(global.foo, 100); - -common.allowGlobals( - global.hello, - global.foo, - global.obj, - global.f -); From 631ed0a49e81431e57bb9bca44db5c027d97ab3d Mon Sep 17 00:00:00 2001 From: Ruben Bridgewater Date: Sat, 29 Dec 2018 00:39:35 +0100 Subject: [PATCH 2/2] lib: add custom eslint rule against global pollution This adds a custom eslint rule to make sure the globals are not polluted in any way by adding extra properties to them in any way. Besides that it also removes access to `global` by marking some globals as available to eslint. --- lib/.eslintrc.yaml | 1 + lib/internal/main/eval_string.js | 1 + lib/internal/per_context/setup.js | 9 +++---- lib/internal/process/execution.js | 1 + tools/eslint-rules/no-global-pollute.js | 33 +++++++++++++++++++++++++ 5 files changed, 40 insertions(+), 5 deletions(-) create mode 100644 tools/eslint-rules/no-global-pollute.js diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml index 12375c06eded..559180bb706c 100644 --- a/lib/.eslintrc.yaml +++ b/lib/.eslintrc.yaml @@ -31,6 +31,7 @@ rules: # Custom rules in tools/eslint-rules node-core/lowercase-name-for-primitive: error node-core/non-ascii-character: error + node-core/no-global-pollute: error globals: Intl: false # Assertions diff --git a/lib/internal/main/eval_string.js b/lib/internal/main/eval_string.js index b0322819251e..6313a1983ca9 100644 --- a/lib/internal/main/eval_string.js +++ b/lib/internal/main/eval_string.js @@ -12,6 +12,7 @@ const { addBuiltinLibsToObject } = require('internal/modules/cjs/helpers'); const { getOptionValue } = require('internal/options'); const source = getOptionValue('--eval'); prepareMainThreadExecution(); +// eslint-disable-next-line node-core/no-global-pollute addBuiltinLibsToObject(global); markBootstrapComplete(); if (getOptionValue('--entry-type') === 'module') diff --git a/lib/internal/per_context/setup.js b/lib/internal/per_context/setup.js index 16bd7db586b1..3947013818e3 100644 --- a/lib/internal/per_context/setup.js +++ b/lib/internal/per_context/setup.js @@ -1,15 +1,14 @@ // This file is compiled as if it's wrapped in a function with arguments // passed by node::NewContext() -/* global global */ 'use strict'; // https://github.com/nodejs/node/issues/14909 -if (global.Intl) { - delete global.Intl.v8BreakIterator; +if (typeof Intl !== 'undefined') { + delete Intl.v8BreakIterator; } // https://github.com/nodejs/node/issues/21219 -if (global.Atomics) { - delete global.Atomics.wake; +if (typeof Atomics !== 'undefined') { + delete Atomics.wake; } diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js index 5712b80eafb6..cf2f6ca73de8 100644 --- a/lib/internal/process/execution.js +++ b/lib/internal/process/execution.js @@ -62,6 +62,7 @@ function evalScript(name, body, breakFirstLine) { const module = new CJSModule(name); module.filename = path.join(cwd, name); module.paths = CJSModule._nodeModulePaths(cwd); + // eslint-disable-next-line node-core/no-global-pollute global.kVmBreakFirstLineSymbol = kVmBreakFirstLineSymbol; const script = ` global.__filename = ${JSON.stringify(name)}; diff --git a/tools/eslint-rules/no-global-pollute.js b/tools/eslint-rules/no-global-pollute.js new file mode 100644 index 000000000000..aaf686c48831 --- /dev/null +++ b/tools/eslint-rules/no-global-pollute.js @@ -0,0 +1,33 @@ +/** + * @fileOverview This rule makes sure that the globals don't get polluted by + * adding new entries to `global`. + * @author Ruben Bridgewater + */ + +'use strict'; + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = function(context) { + const filename = context.getFilename(); + + return { + 'Program:exit': function() { + const globalScope = context.getScope(); + const variable = globalScope.set.get('global'); + if (variable && + !filename.includes('bootstrap') && + !filename.includes('repl.js')) { + const msg = 'Please do not pollute the global scope'; + variable.references.forEach((reference) => { + context.report({ + node: reference.identifier, + message: msg + }); + }); + } + } + }; +};