From e2c0c0c68074c539ae2ae30debdeafb9f94d989b 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 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 | 95 +++++++++++++------------ lib/internal/modules/esm/translators.js | 5 +- 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, 92 insertions(+), 68 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 24d25158f9e8ad..ed5a5afe4b8072 100644 --- a/lib/internal/bootstrap/pre_execution.js +++ b/lib/internal/bootstrap/pre_execution.js @@ -338,7 +338,7 @@ function initializePolicy() { } function initializeCJSLoader() { - require('internal/modules/cjs/loader')._initPaths(); + require('internal/modules/cjs/loader').Module._initPaths(); } function initializeESMLoader() { @@ -387,7 +387,9 @@ function loadPreloadModules() { const preloadModules = getOptionValue('--require'); if (preloadModules) { 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 4b35302944f111..c3bfc2c35cde19 100644 --- a/lib/internal/modules/cjs/helpers.js +++ b/lib/internal/modules/cjs/helpers.js @@ -72,6 +72,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', @@ -137,5 +148,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 2798032fd96239..e676d06fa91456 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -40,7 +40,7 @@ const { makeRequireFunction, normalizeReferrerURL, stripBOM, - stripShebang + stripShebangOrBOM, } = require('internal/modules/cjs/helpers'); const { getOptionValue } = require('internal/options'); const preserveSymlinks = getOptionValue('--preserve-symlinks'); @@ -59,7 +59,7 @@ const { const { validateString } = require('internal/validators'); const pendingDeprecation = getOptionValue('--pending-deprecation'); -module.exports = Module; +module.exports = { wrapSafe, Module }; let asyncESM; let ModuleJob; @@ -690,22 +690,10 @@ Module.prototype.require = function(id) { var 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) { - if (manifest) { - const moduleURL = pathToFileURL(filename); - manifest.assertIntegrity(moduleURL, content); - } - - content = stripShebang(content); - - 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, @@ -714,35 +702,54 @@ Module.prototype._compile = function(content, filename) { return loader.import(specifier, normalizeReferrerURL(filename)); } : undefined, }); - } else { - compiledWrapper = compileFunction( - content, - filename, - 0, - 0, - undefined, - false, - undefined, - [], - [ - 'exports', - 'require', - 'module', - '__filename', - '__dirname', - ] - ); - if (experimentalModules) { - const { callbackMap } = internalBinding('module_wrap'); - callbackMap.set(compiledWrapper, { - importModuleDynamically: async (specifier) => { - const loader = await asyncESM.loaderPromise; - return loader.import(specifier, normalizeReferrerURL(filename)); - } - }); - } } + const compiledWrapper = compileFunction( + content, + filename, + 0, + 0, + undefined, + false, + undefined, + [], + [ + 'exports', + 'require', + 'module', + '__filename', + '__dirname', + ] + ); + + if (experimentalModules) { + const { callbackMap } = internalBinding('module_wrap'); + callbackMap.set(compiledWrapper, { + importModuleDynamically: async (specifier) => { + const loader = await asyncESM.loaderPromise; + return loader.import(specifier, normalizeReferrerURL(filename)); + } + }); + } + + 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); + } + + // 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) { @@ -782,7 +789,7 @@ Module.prototype._compile = function(content, filename) { // Native extension for .js 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 4ca9be4d622faa..9526e052637725 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -11,10 +11,9 @@ const { const { NativeModule } = require('internal/bootstrap/loaders'); const { - stripShebang, 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'); @@ -48,7 +47,7 @@ translators.set('module', async function moduleStrategy(url) { const source = `${await readFileAsync(new URL(url))}`; debug(`Translating StandardModule ${url}`); const { ModuleWrap, callbackMap } = internalBinding('module_wrap'); - const module = new ModuleWrap(stripShebang(source), url); + 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 2b7fc41ccddf01..077d225556ca81 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 e4a366664c10d6..12461d67de481b 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() {