Modifying globals within module leaks to global with Node >=10

[rensbaardman]

When running the test suite with Node 10 or higher, the test should not influence other modules when injecting mocks in the shared test cases fails. This happens because __set__ing a global in moduleA.js doesn't set that global only within that module, but globally (so also in moduleB.js and in the test suite itself). This should not happen, since it leaks to other tests and can have various unwanted effects. It did not happen with Node 9 (which is why Travis also didn't pick this up - it tests with Node 6, 8 and 9), and I do not know why this happens using higher versions (Node 10, 11 and 12 all have this behaviour, starting from the very first v10.0.0).

What makes this case even more mysterious, is that since that test (should not influence ...) sets console to a dummy mock { } (which should only have effect in moduleA, but because of this bug also changes the global console), all further Mocha output (which uses console.log) is suppressed. It took me a while therefore, to actually notice this test was failing!

All other tests seem to pass fine – it.skip the failing test, and you see the other tests pass:

63 passing (1s)
1 pending

This is also the easiest way to check that Mocha actually ran the other tests.

How to reproduce

I use nvm to switch between node versions. (I left out some of the test results, as marked by ...)

$ nvm use 9
Now using node v9.11.2 (npm v5.6.0)

$ npm test
> rewire@4.0.1 test /Users/Rens/Dev/rewire
> mocha -R spec

  __get__
    ✓ should return the initial value
      ...

  ...

  rewire
    ✓ should work like require() (101ms)
    ✓ should return a fresh instance of the module
    ✓ should not cache the rewired module
    ✓ should modify the module so it provides the __set__ - function (45ms)
    ✓ should modify the module so it provides the __get__ - function (57ms)
    ✓ should modify the module so it provides the __with__ - function (55ms)
    ✓ should provide __get__ as a non-enumerable property
    ✓ should provide __get__ as a writable property
    ✓ should provide __set__ as a non-enumerable property
    ✓ should provide __set__ as a writable property
    ✓ should provide __with__ as a non-enumerable property
    ✓ should provide __with__ as a writable property
    ✓ should not influence other modules
    ✓ should not override/influence global objects by default
    ✓ should provide a working __set__ method
    ✓ should provide a working __get__ method
    ✓ should provide a working __with__ method
    ✓ should provide the ability to inject mocks
    ✓ should not influence other modules when injecting mocks
    ✓ should provide the ability to mock global objects just within the module (41ms)
    ✓ should be possible to mock global objects that are added on runtime
    ✓ should not be a problem to have a comment on file end
    ✓ should not be a problem to have a module that exports a boolean
    ✓ should not be a problem to have a module that exports null
    ✓ should not be a problem to have a module that exports a sealed object
    ✓ should not be a problem to have a module that uses object spread operator
    ✓ should not be a problem to have a module that uses object rest operator
    ✓ should not influence the original require if nothing has been required within the rewired module
    ✓ subsequent calls of rewire should always return a new instance (46ms)
    ✓ should preserve the strict mode
    ✓ should not modify line numbers in stack traces
    ✓ should be possible to set implicit globals
    ✓ should throw a TypeError if the path is not a string
    ✓ should also revert nested changes (with dot notation)
    ✓ should be possible to mock undefined, implicit globals
    ✓ should be possible to mock and revert JSON.parse (see #40)
    ✓ should be possible to set a const variable
    ✓ should fail with a helpful TypeError when const is re-assigned
    ✓ should also work with CoffeeScript (43ms)

  64 passing (1s)

$ nvm use 10
Now using node v10.16.0 (npm v6.9.0)

$ npm test
> rewire@4.0.1 test /Users/Rens/Dev/rewire
> mocha -R spec

  __get__
    ✓ should return the initial value
      ...

  ...

  rewire
    ✓ should work like require() (86ms)
    ✓ should return a fresh instance of the module
    ✓ should not cache the rewired module
    ✓ should modify the module so it provides the __set__ - function (54ms)
    ✓ should modify the module so it provides the __get__ - function (49ms)
    ✓ should modify the module so it provides the __with__ - function (45ms)
    ✓ should provide __get__ as a non-enumerable property
    ✓ should provide __get__ as a writable property
    ✓ should provide __set__ as a non-enumerable property
    ✓ should provide __set__ as a writable property
    ✓ should provide __with__ as a non-enumerable property
    ✓ should provide __with__ as a writable property
    ✓ should not influence other modules
    ✓ should not override/influence global objects by default
    ✓ should provide a working __set__ method
    ✓ should provide a working __get__ method
    ✓ should provide a working __with__ method
    ✓ should provide the ability to inject mocks
    ✓ should not influence other modules when injecting mocks

Note that the last 20-or-so tests disappeared, and there was no test result 'x passing'.

[rensbaardman]

I think I found the problem: clearly, console is not appropriately declared with var by getImportGlobalsSrc.js, else modifications by __set__ wouldn't have leaked.

Now, getImportGlobalsSrc.js tries to find all globals by using for (key in globalObj), which only goes over all enumerable properties of globalObj (e.g. those in Object.keys(globalObj)). Apparently, console was an enumerable property of global in Node 9, but not in Node >= 10:

$ nvm use 9
Now using node v9.11.2 (npm v5.6.0)

$ node
> Object.keys(global)
[ 'console',
  'DTRACE_NET_SERVER_CONNECTION',
  'DTRACE_NET_STREAM_END',
  'DTRACE_HTTP_SERVER_REQUEST',
  'DTRACE_HTTP_SERVER_RESPONSE',
  'DTRACE_HTTP_CLIENT_REQUEST',
  'DTRACE_HTTP_CLIENT_RESPONSE',
  'global',
  'process',
  'Buffer',
  'clearImmediate',
  'clearInterval',
  'clearTimeout',
  'setImmediate',
  'setInterval',
  'setTimeout',
  'module',
  'require' ]
> .exit

$ nvm use 10
Now using node v10.16.0 (npm v6.9.0)

$ node
> Object.keys(global)
[ 'DTRACE_NET_SERVER_CONNECTION',
  'DTRACE_NET_STREAM_END',
  'DTRACE_HTTP_SERVER_REQUEST',
  'DTRACE_HTTP_SERVER_RESPONSE',
  'DTRACE_HTTP_CLIENT_REQUEST',
  'DTRACE_HTTP_CLIENT_RESPONSE',
  'global',
  'process',
  'Buffer',
  'clearImmediate',
  'clearInterval',
  'clearTimeout',
  'setImmediate',
  'setInterval',
  'setTimeout' ]

Note that console is missing in the second case! (along with some other variables) update: for those interested, console was made non-enumerable in https://github.com/nodejs/node/pull/17708 because apparantly that's what the standards say it should be

I think however, that simply adding console to the 'list-of-global-vars' isn't enough: there are also a lot of global variables that aren't enumerable, and which we would miss in that case:

$ node
> console.dir(Object.getOwnPropertyNames(global), {'maxArrayLength': null} )
[ 'Object',
  'Function',
  'Array',
  'Number',
  'parseFloat',
  'parseInt',
  'Infinity',
  'NaN',
  'undefined',
  'Boolean',
  'String',
  'Symbol',
  'Date',
  'Promise',
  'RegExp',
  'Error',
  'EvalError',
  'RangeError',
  'ReferenceError',
  'SyntaxError',
  'TypeError',
  'URIError',
  'JSON',
  'Math',
  'console',
  'Intl',
  'ArrayBuffer',
  'Uint8Array',
  'Int8Array',
  'Uint16Array',
  'Int16Array',
  'Uint32Array',
  'Int32Array',
  'Float32Array',
  'Float64Array',
  'Uint8ClampedArray',
  'BigUint64Array',
  'BigInt64Array',
  'DataView',
  'Map',
  'Set',
  'WeakMap',
  'WeakSet',
  'Proxy',
  'Reflect',
  'decodeURI',
  'decodeURIComponent',
  'encodeURI',
  'encodeURIComponent',
  'escape',
  'unescape',
  'eval',
  'isFinite',
  'isNaN',
  'SharedArrayBuffer',
  'Atomics',
  'BigInt',
  'WebAssembly',
  'DTRACE_NET_SERVER_CONNECTION',
  'DTRACE_NET_STREAM_END',
  'DTRACE_HTTP_SERVER_REQUEST',
  'DTRACE_HTTP_SERVER_RESPONSE',
  'DTRACE_HTTP_CLIENT_REQUEST',
  'DTRACE_HTTP_CLIENT_RESPONSE',
  'global',
  'process',
  'GLOBAL',
  'root',
  'Buffer',
  'clearImmediate',
  'clearInterval',
  'clearTimeout',
  'setImmediate',
  'setInterval',
  'setTimeout',
  'URL',
  'URLSearchParams',
  'module',
  'require',
  'assert',
  'async_hooks',
  'buffer',
  'child_process',
  'cluster',
  'crypto',
  'dgram',
  'dns',
  'domain',
  'events',
  'fs',
  'http',
  'http2',
  'https',
  'inspector',
  'net',
  'os',
  'path',
  'perf_hooks',
  'punycode',
  'querystring',
  'readline',
  'repl',
  'stream',
  'string_decoder',
  'tls',
  'trace_events',
  'tty',
  'url',
  'util',
  'v8',
  'vm',
  'zlib',
  '_',
  '_error' ]

(that is Node 10 output, Node 9 is similar but missing BigUint64Array, BigInt64Array, BigInt, URL, URLSearchParams, and trace_events.) Note that this one does include console.

The fix should be simple: replace for (key in globalObj) with for (key of Object.getOwnPropertyNames(globalObj)). But I'm a little hesitant since I don't know if there's anything else to consider when declaring all these globals with var.

An extra test has to be added too, to cover both enumerable and non-enumerable globals. Care should be taken that the chosen example globals are available in all versions of Node under test, and are always either enumerable or non-enumerable (so console is not suited).

[rensbaardman]

And – but I've never worked with them, so I am not sure how relevant they are – consider symbols:

$ node
> Object.getOwnPropertySymbols(global)
[ Symbol(Symbol.toStringTag) ]

They are (the only?) global variables that are not included in Object.getOwnPropertyNames(global).

[rensbaardman]

Possibly relevant is that in browsers, there are way more global variables

> Object.keys(window)
Array(247) [ "close", "stop", "focus", "blur", "open", "alert", "confirm", "prompt", "print", "postMessage", … ]

> Object.getOwnPropertyNames(window)
Array(826) [ "undefined", "Boolean", "InternalError", "EvalError", "RangeError", "SyntaxError", "TypeError", "URIError", "ArrayBuffer", "Int8Array", … ]

Could declaring them all with var cause a possible speed/memory problem of any kind?

[breautek]

I think this is relevant to my issue where we are trying to override process.exit.

It works in Node 10, but stops working in Node 12.

Node 10:

Running node v10.16.0 (npm v6.9.0)
> Object.keys(global)
[ 'global',
  'process',
  'Buffer',
  'clearImmediate',
  'clearInterval',
  'clearTimeout',
  'setImmediate',
  'setInterval',
  'setTimeout' ]

Node 12:

Welcome to Node.js v12.4.0.
Type ".help" for more information.
> Object.keys(global)
[
  'global',
  'clearInterval',
  'clearTimeout',
  'setInterval',
  'setTimeout',
  'queueMicrotask',
  'clearImmediate',
  'setImmediate'
]

process is no longer in global in Node 12.

[brodycj]

And to quote from https://nodejs.org/docs/latest-v12.x/api/process.html#process_process:

The process object is a global that provides information about, and control over, the current Node.js process. As a global, it is always available to Node.js applications without using require(). It can also be explicitly accessed using require():

const process = require('process');

Does this sound right?

Any other explanations?

[breautek]

Not quite sure how to explain it, I'm not sure on their terminology. When I said "process is no longer in global in Node 12", I mean it is no longer on the global object. It is still "global" in the sense that you don't need to require it.

[breautek]

Using

__set__({
    process : () => {}
});

on NodeJS 12 is broken, however...

__set__("process.exit", () => {});

does work on NodeJS 12 for me.

[rensbaardman]

So, process is still global, it is only not part of the enumerable keys of global. You can check this with

$ nvm use 12
$ node
> Object.getOwnPropertyNames(global).indexOf('process')

which should return an integer (for me, it is 60) indicating it is in that list.

But indeed, the same bug is the cause of your problems. I have a fork that fixes it. I also submitted it as a PR, but it has been a long time since the last PR in this repo was merged.

Note, though, that the workaround to __set__ process.exit directly is strongly discouraged. Are you sure you are not simply modifying the global process?

[breautek]

Note, though, that the workaround to set process.exit directly is strongly discouraged. Are you sure you are not simply modifying the global process?

Thanks for the note... I'll forward this to the cordova peeps. We've tried the dot notation workaround and it didn't break existing tests, but would be beneficial to keep an eye on your PR so that we can update asap.

[jhnns]

Yes, I can confirm this. This is because they started to make some global objects non-enumerable. I just skimmed https://github.com/jhnns/rewire/pull/168 and it looks like it's a good fix for this.

[john-perks]

In case anyone else is having problems with this, in the absence of a fix I've stumbled into a workaround. For each module whose test module is __set__()ing a global, add something like this at the top:

var {process, console} = global;

and add globals as needed.

One caveat is that this may not work with repackaging tools. For instance, we have a check on if (typeof __webpack_require__ === 'function') {...} to distinguish between packed and debug code at runtime, and trying the above solution causes problems when running the code packed. The way round that was for the tests to use a real global __webpack_require__ variable, and to have delete global.__webpack_require__; in an afterEach() to cleanup.

[jhnns]

Published as v6.0.0 🚀

[edjeordjian]

Thank you!!! Version 6.0.0 seems to work for me: there is no longer a leak with global overrides (like Date, which was my case).