Node: require accepts incoherent files

[iglosiggio]

This is a really minor thing, but require can accept files formatted to exploit NativeModule.wrap

Example:

test.js:

});
console.log("This is buggy");
({apply: console.info

cli:

alumno@huayra:/tmp$ node
> require('./test.js')
This is buggy
{} [ {},
  { [Function: require]
    resolve: [Function: resolve],
    main: undefined,
    extensions: { '.js': [Function], '.json': [Function], '.node': [Function] },
    cache: { '/tmp/test.js': [Object] } },
  Module {
    id: '/tmp/test.js',
    exports: {},
    parent: 
     Module {
       id: '<repl>',
       exports: {},
       parent: undefined,
       filename: null,
       loaded: false,
       children: [Object],
       paths: [Object] },
    filename: '/tmp/test.js',
    loaded: false,
    children: [],
    paths: [ '/tmp/node_modules', '/node_modules' ] },
  '/tmp/test.js',
  '/tmp' ]
{}

[iglosiggio]

I know that it's really a dumb problem, but i felt the need to report it anyways.

[Fishrock123]

In hindsight I'm amazed I've never seen this before or thought about it.

Is there really a way to fix this? Other than first compiling the code without wrapping it to check?

[seishun]

Somewhat unrelated: would this still be a problem for ES6 modules?

[Fishrock123]

No, those can't have a function wrapper because that would render import/export statements ("declarations") nonfunctional. (No longer top-level.)

We could inject something before running user code though.

[iglosiggio]

require expects that the wrappedContent is the source code of an function expression but i don't know if it can be checked easily.

I think that the "correct" thing to do is to abort in NativeModule.wrap if it fails (the resulting ast is different from a file with only a function expression). But that doesn't sound easy at all.

Anyways if somebody is dinamically using require w/o a really big concern the fault weights more on his side hahahaha (modules already run in the local context when evaluating the function expression)

[bmeck]

we can move to use the Function constructor for the wrapper instead of syntactic function to fix this, but that means double parsing. v8::ScriptCompiler::CompileFunctionInContext might be a better choice if we can check benchmarks to make sure it doesn't do anything strange.

[iglosiggio]

And if we simply do const wrapped = new Function('exports', 'require', 'module', '__filename', '__dirname', content) ?

[bmeck]

@iglosiggio thats what my first sentence was referring too, but you should drop the const wrapped = bit. However, this invokes the JS parser twice (once to form the new Function, once to parse content). The C++ form avoids the double parsing, but might have different optimization profile due to the with like behavior.

[iglosiggio]

@bmeck Oh, thanks! I didn't understand why it required double parsing :sweat_smile:

Also, a ugly way to check if the input can be wrapped is to validate if the curly brackets and parentheses are balanced

[Fishrock123]

I would recommend doing nothing unless there is a tangible security risk present. We could break people in subtle ways.

[jasnell]

+1 to doing nothing unless a security risk can be demonstrated. There would be too much of a negative performance impact if we add any kind of checking here.

[bmeck]

I'm fine calling this a wontfix for now

[iglosiggio]

+1

[cjihrig]

+1 to wontfix

[addaleax]

Seems we have consensus to do nothing for now.