From 1c50714729bef0db31a9ca814c0c5b600d360e4e Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Sat, 18 May 2019 22:48:46 -0500 Subject: [PATCH] lib: rework logic of stripping BOM+Shebang from commonjs Fixes https://github.com/nodejs/node/issues/27767 Backport-PR-URL: https://github.com/nodejs/node/pull/31228 PR-URL: https://github.com/nodejs/node/pull/27768 Reviewed-By: Ruben Bridgewater Reviewed-By: Rich Trott Reviewed-By: Matteo Collina --- lib/internal/bootstrap/pre_execution.js | 6 +- lib/internal/main/check_syntax.js | 19 ++-- lib/internal/main/run_main_module.js | 2 +- lib/internal/modules/cjs/helpers.js | 14 ++- lib/internal/modules/cjs/loader.js | 114 ++++++++++++------------ lib/internal/modules/esm/translators.js | 11 ++- lib/internal/process/execution.js | 2 +- lib/internal/util/inspector.js | 2 +- lib/module.js | 2 +- lib/repl.js | 2 +- test/fixtures/utf8-bom-shebang.js | 2 + test/fixtures/utf8-shebang-bom.js | 2 + test/sequential/test-module-loading.js | 7 ++ 13 files changed, 100 insertions(+), 85 deletions(-) create mode 100644 test/fixtures/utf8-bom-shebang.js create mode 100644 test/fixtures/utf8-shebang-bom.js diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js index 2aa2a3b46c5fc1..745f03f2a51c7e 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -389,7 +389,7 @@ function initializePolicy() { } function initializeCJSLoader() { - require('internal/modules/cjs/loader')._initPaths(); + require('internal/modules/cjs/loader').Module._initPaths(); } function initializeESMLoader() { @@ -438,7 +438,9 @@ function loadPreloadModules() { const preloadModules = getOptionValue('--require'); if (preloadModules && preloadModules.length > 0) { const { - _preloadModules + Module: { + _preloadModules + }, } = require('internal/modules/cjs/loader'); _preloadModules(preloadModules); } diff --git a/lib/internal/main/check_syntax.js b/lib/internal/main/check_syntax.js index 8b7a85e29d200d..1a5287520414cb 100644 --- a/lib/internal/main/check_syntax.js +++ b/lib/internal/main/check_syntax.js @@ -13,14 +13,15 @@ const { const { pathToFileURL } = require('url'); -const vm = require('vm'); const { - stripShebang, stripBOM + stripShebangOrBOM, } = require('internal/modules/cjs/helpers'); const { - _resolveFilename: resolveCJSModuleName, - wrap: wrapCJSModule + Module: { + _resolveFilename: resolveCJSModuleName, + }, + wrapSafe, } = require('internal/modules/cjs/loader'); // TODO(joyeecheung): not every one of these are necessary @@ -49,9 +50,6 @@ if (process.argv[1] && process.argv[1] !== '-') { } function checkSyntax(source, filename) { - // Remove Shebang. - source = stripShebang(source); - const { getOptionValue } = require('internal/options'); const experimentalModules = getOptionValue('--experimental-modules'); if (experimentalModules) { @@ -70,10 +68,5 @@ function checkSyntax(source, filename) { } } - // Remove BOM. - source = stripBOM(source); - // Wrap it. - source = wrapCJSModule(source); - // Compile the script, this will throw if it fails. - new vm.Script(source, { displayErrors: true, filename }); + wrapSafe(filename, stripShebangOrBOM(source)); } diff --git a/lib/internal/main/run_main_module.js b/lib/internal/main/run_main_module.js index 8f9d256ecf2c73..2cad569dcce9fd 100644 --- a/lib/internal/main/run_main_module.js +++ b/lib/internal/main/run_main_module.js @@ -6,7 +6,7 @@ const { prepareMainThreadExecution(true); -const CJSModule = require('internal/modules/cjs/loader'); +const CJSModule = require('internal/modules/cjs/loader').Module; markBootstrapComplete(); diff --git a/lib/internal/modules/cjs/helpers.js b/lib/internal/modules/cjs/helpers.js index ca5a7329beec9d..b922f01e36b2af 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -131,6 +131,17 @@ function stripShebang(content) { return content; } +// Strip either the shebang or UTF BOM of a file. +// Note that this only processes one. If both occur in +// either order, the one that comes second is not +// significant. +function stripShebangOrBOM(content) { + if (content.charCodeAt(0) === 0xFEFF) { + return content.slice(1); + } + return stripShebang(content); +} + const builtinLibs = [ 'assert', 'async_hooks', 'buffer', 'child_process', 'cluster', 'crypto', 'dgram', 'dns', 'domain', 'events', 'fs', 'http', 'http2', 'https', 'net', @@ -197,5 +208,6 @@ module.exports = { makeRequireFunction, normalizeReferrerURL, stripBOM, - stripShebang + stripShebang, + stripShebangOrBOM, }; diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index c1e031f28fbd25..bd0b4348116937 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -51,8 +51,7 @@ const { makeRequireFunction, normalizeReferrerURL, stripBOM, - stripShebang, - loadNativeModule + stripShebangOrBOM, } = require('internal/modules/cjs/helpers'); const { getOptionValue } = require('internal/options'); const enableSourceMaps = getOptionValue('--enable-source-maps'); @@ -73,7 +72,7 @@ const { const { validateString } = require('internal/validators'); const pendingDeprecation = getOptionValue('--pending-deprecation'); -module.exports = Module; +module.exports = { wrapSafe, Module }; let asyncESM, ModuleJob, ModuleWrap, kInstantiated; @@ -948,26 +947,10 @@ Module.prototype.require = function(id) { let resolvedArgv; let hasPausedEntry = false; -// Run the file contents in the correct scope or sandbox. Expose -// the correct helper variables (require, module, exports) to -// the file. -// Returns exception, if any. -Module.prototype._compile = function(content, filename) { - let moduleURL; - let redirects; - if (manifest) { - moduleURL = pathToFileURL(filename); - redirects = manifest.getRedirector(moduleURL); - manifest.assertIntegrity(moduleURL, content); - } - - content = stripShebang(content); - maybeCacheSourceMap(filename, content, this); - - let compiledWrapper; +function wrapSafe(filename, content) { if (patched) { const wrapper = Module.wrap(content); - compiledWrapper = vm.runInThisContext(wrapper, { + return vm.runInThisContext(wrapper, { filename, lineOffset: 0, displayErrors: true, @@ -976,46 +959,61 @@ Module.prototype._compile = function(content, filename) { return loader.import(specifier, normalizeReferrerURL(filename)); } : undefined, }); - } else { - let compiled; - try { - compiled = compileFunction( - content, - filename, - 0, - 0, - undefined, - false, - undefined, - [], - [ - 'exports', - 'require', - 'module', - '__filename', - '__dirname', - ] - ); - } catch (err) { - if (experimentalModules) { - enrichCJSError(err); + } + + let compiledWrapper; + try { + compiledWrapper = compileFunction( + content, + filename, + 0, + 0, + undefined, + false, + undefined, + [], + [ + 'exports', + 'require', + 'module', + '__filename', + '__dirname', + ] + ); + } catch (err) { + enrichCJSError(err); + throw err; + } + + if (experimentalModules) { + const { callbackMap } = internalBinding('module_wrap'); + callbackMap.set(compiledWrapper, { + importModuleDynamically: async (specifier) => { + const loader = await asyncESM.loaderPromise; + return loader.import(specifier, normalizeReferrerURL(filename)); } - throw err; - } + }); + } - if (experimentalModules) { - const { callbackMap } = internalBinding('module_wrap'); - callbackMap.set(compiled.cacheKey, { - importModuleDynamically: async (specifier) => { - const loader = await asyncESM.loaderPromise; - return loader.import(specifier, normalizeReferrerURL(filename)); - } - }); - } - compiledWrapper = compiled.function; + return compiledWrapper; +} + +// Run the file contents in the correct scope or sandbox. Expose +// the correct helper variables (require, module, exports) to +// the file. +// Returns exception, if any. +Module.prototype._compile = function(content, filename) { + if (manifest) { + const moduleURL = pathToFileURL(filename); + manifest.assertIntegrity(moduleURL, content); } - let inspectorWrapper = null; + // Strip after manifest integrity check + content = stripShebangOrBOM(content); + + const compiledWrapper = wrapSafe(filename, content); + + var inspectorWrapper = null; if (getOptionValue('--inspect-brk') && process._eval == null) { if (!resolvedArgv) { // We enter the repl if we're not given a filename argument. @@ -1079,7 +1077,7 @@ Module._extensions['.js'] = function(module, filename) { } } const content = fs.readFileSync(filename, 'utf8'); - module._compile(stripBOM(content), filename); + module._compile(content, filename); }; diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 6c710202b85b77..5aede5490feb45 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -12,11 +12,9 @@ const { const { Buffer } = require('buffer'); const { - stripShebang, - stripBOM, - loadNativeModule + stripBOM } = require('internal/modules/cjs/helpers'); -const CJSModule = require('internal/modules/cjs/loader'); +const CJSModule = require('internal/modules/cjs/loader').Module; const internalURLModule = require('internal/url'); const createDynamicModule = require( 'internal/modules/esm/create_dynamic_module'); @@ -80,8 +78,9 @@ translators.set('module', async function moduleStrategy(url) { const source = `${await getSource(url)}`; maybeCacheSourceMap(url, source); debug(`Translating StandardModule ${url}`); - const module = new ModuleWrap(stripShebang(source), url); - moduleWrap.callbackMap.set(module, { + const { ModuleWrap, callbackMap } = internalBinding('module_wrap'); + const module = new ModuleWrap(source, url); + callbackMap.set(module, { initializeImportMeta, importModuleDynamically, }); diff --git a/lib/internal/process/execution.js b/lib/internal/process/execution.js index b8c4e673b2c8ca..06dfbce2958f08 100644 --- a/lib/internal/process/execution.js +++ b/lib/internal/process/execution.js @@ -53,7 +53,7 @@ function evalModule(source, print) { } function evalScript(name, body, breakFirstLine, print) { - const CJSModule = require('internal/modules/cjs/loader'); + const CJSModule = require('internal/modules/cjs/loader').Module; const { kVmBreakFirstLineSymbol } = require('internal/util'); const cwd = tryGetCwd(); diff --git a/lib/internal/util/inspector.js b/lib/internal/util/inspector.js index 70868e73244813..5230137fce6ef9 100644 --- a/lib/internal/util/inspector.js +++ b/lib/internal/util/inspector.js @@ -24,7 +24,7 @@ function sendInspectorCommand(cb, onError) { function installConsoleExtensions(commandLineApi) { if (commandLineApi.require) { return; } const { tryGetCwd } = require('internal/process/execution'); - const CJSModule = require('internal/modules/cjs/loader'); + const CJSModule = require('internal/modules/cjs/loader').Module; const { makeRequireFunction } = require('internal/modules/cjs/helpers'); const consoleAPIModule = new CJSModule(''); const cwd = tryGetCwd(); diff --git a/lib/module.js b/lib/module.js index 962f18b054cc90..a767330f5e3d6e 100644 --- a/lib/module.js +++ b/lib/module.js @@ -1,3 +1,3 @@ 'use strict'; -module.exports = require('internal/modules/cjs/loader'); +module.exports = require('internal/modules/cjs/loader').Module; diff --git a/lib/repl.js b/lib/repl.js index 0007a3610ad319..e2daf2f15759d1 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -65,7 +65,7 @@ const path = require('path'); const fs = require('fs'); const { Interface } = require('readline'); const { Console } = require('console'); -const CJSModule = require('internal/modules/cjs/loader'); +const CJSModule = require('internal/modules/cjs/loader').Module; const domain = require('domain'); const debug = require('internal/util/debuglog').debuglog('repl'); const { diff --git a/test/fixtures/utf8-bom-shebang.js b/test/fixtures/utf8-bom-shebang.js new file mode 100644 index 00000000000000..d0495f04552d41 --- /dev/null +++ b/test/fixtures/utf8-bom-shebang.js @@ -0,0 +1,2 @@ +#!shebang +module.exports = 42; \ No newline at end of file diff --git a/test/fixtures/utf8-shebang-bom.js b/test/fixtures/utf8-shebang-bom.js new file mode 100644 index 00000000000000..4ff8fd8d453334 --- /dev/null +++ b/test/fixtures/utf8-shebang-bom.js @@ -0,0 +1,2 @@ +#!shebang +module.exports = 42; \ No newline at end of file diff --git a/test/sequential/test-module-loading.js b/test/sequential/test-module-loading.js index 612cde3ab0261f..94acdb191b9044 100644 --- a/test/sequential/test-module-loading.js +++ b/test/sequential/test-module-loading.js @@ -352,6 +352,13 @@ process.on('exit', function() { assert.strictEqual(require('../fixtures/utf8-bom.js'), 42); assert.strictEqual(require('../fixtures/utf8-bom.json'), 42); +// Loading files with BOM + shebang. +// See https://github.com/nodejs/node/issues/27767 +assert.throws(() => { + require('../fixtures/utf8-bom-shebang.js'); +}, { name: 'SyntaxError' }); +assert.strictEqual(require('../fixtures/utf8-shebang-bom.js'), 42); + // Error on the first line of a module should // have the correct line number assert.throws(function() {