errors: do not call resolve on URLs with schemes

We were incorrectly trying to run path.resolve on absolute sources URLs. This was breaking webpack:// URLs in stack trace output. Refs: nodejs#35325 Backport-PR-URL: nodejs#37717 PR-URL: nodejs#35903 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
bcoe · Mar 16, 2021 · f91f140 · f91f140
1 parent 6562506
commit f91f140
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 36 deletions.
diff --git a/lib/internal/source_map/prepare_stack_trace.js b/lib/internal/source_map/prepare_stack_trace.js
@@ -1,7 +1,9 @@
 'use strict';
 
 const {
+  ArrayPrototypeIndexOf,
   Error,
+  StringPrototypeStartsWith,
 } = primordials;
 
 let debug = require('internal/util/debuglog').debuglog('source_map', (fn) => {
@@ -15,6 +17,7 @@ const {
   overrideStackTrace,
   maybeOverridePrepareStackTrace
 } = require('internal/errors');
+const { fileURLToPath } = require('internal/url');
 
 // Create a prettified stacktrace, inserting context from source maps
 // if possible.
@@ -40,14 +43,12 @@ const prepareStackTrace = (globalThis, error, trace) => {
   }
 
   let errorSource = '';
-  let firstSource;
   let firstLine;
   let firstColumn;
   const preparedTrace = trace.map((t, i) => {
     if (i === 0) {
       firstLine = t.getLineNumber();
       firstColumn = t.getColumnNumber();
-      firstSource = t.getFileName();
     }
     let str = i !== 0 ? '\n    at ' : '';
     str = `${str}${t}`;
@@ -63,16 +64,22 @@ const prepareStackTrace = (globalThis, error, trace) => {
         } = sm.findEntry(t.getLineNumber() - 1, t.getColumnNumber() - 1);
         if (originalSource && originalLine !== undefined &&
             originalColumn !== undefined) {
-          const originalSourceNoScheme = originalSource
-            .replace(/^file:\/\//, '');
           if (i === 0) {
             firstLine = originalLine + 1;
             firstColumn = originalColumn + 1;
-            firstSource = originalSourceNoScheme;
+
             // Show error in original source context to help user pinpoint it:
-            errorSource = getErrorSource(firstSource, firstLine, firstColumn);
+            errorSource = getErrorSource(
+              sm.payload,
+              originalSource,
+              firstLine,
+              firstColumn
+            );
           }
           // Show both original and transpiled stack trace information:
+          const originalSourceNoScheme =
+            StringPrototypeStartsWith(originalSource, 'file://') ?
+              fileURLToPath(originalSource) : originalSource;
           str += `\n        -> ${originalSourceNoScheme}:${originalLine + 1}:` +
             `${originalColumn + 1}`;
         }
@@ -88,15 +95,29 @@ const prepareStackTrace = (globalThis, error, trace) => {
 // Places a snippet of code from where the exception was originally thrown
 // above the stack trace. This logic is modeled after GetErrorSource in
 // node_errors.cc.
-function getErrorSource(firstSource, firstLine, firstColumn) {
+function getErrorSource(payload, originalSource, firstLine, firstColumn) {
   let exceptionLine = '';
+  const originalSourceNoScheme =
+    StringPrototypeStartsWith(originalSource, 'file://') ?
+      fileURLToPath(originalSource) : originalSource;
+
   let source;
-  try {
-    source = readFileSync(firstSource, 'utf8');
-  } catch (err) {
-    debug(err);
-    return exceptionLine;
+  const sourceContentIndex =
+    ArrayPrototypeIndexOf(payload.sources, originalSource);
+  if (payload.sourcesContent?.[sourceContentIndex]) {
+    // First we check if the original source content was provided in the
+    // source map itself:
+    source = payload.sourcesContent[sourceContentIndex];
+  } else {
+    // If no sourcesContent was found, attempt to load the original source
+    // from disk:
+    try {
+      source = readFileSync(originalSourceNoScheme, 'utf8');
+    } catch (err) {
+      debug(err);
+    }
   }
+
   const lines = source.split(/\r?\n/, firstLine);
   const line = lines[firstLine - 1];
   if (!line) return exceptionLine;
@@ -110,7 +131,8 @@ function getErrorSource(firstSource, firstLine, firstColumn) {
   }
   prefix = prefix.slice(0, -1); // The last character is the '^'.
 
-  exceptionLine = `${firstSource}:${firstLine}\n${line}\n${prefix}^\n\n`;
+  exceptionLine =
+              `${originalSourceNoScheme}:${firstLine}\n${line}\n${prefix}^\n\n`;
   return exceptionLine;
 }
 

diff --git a/lib/internal/source_map/source_map_cache.js b/lib/internal/source_map/source_map_cache.js
@@ -25,7 +25,6 @@ const { Buffer } = require('buffer');
 let debug = require('internal/util/debuglog').debuglog('source_map', (fn) => {
   debug = fn;
 });
-const { dirname, resolve } = require('path');
 const fs = require('fs');
 const { getOptionValue } = require('internal/options');
 const {
@@ -63,10 +62,8 @@ function getSourceMapsEnabled() {
 function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
   const sourceMapsEnabled = getSourceMapsEnabled();
   if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
-  let basePath;
   try {
     filename = normalizeReferrerURL(filename);
-    basePath = dirname(fileURLToPath(filename));
   } catch (err) {
     // This is most likely an [eval]-wrapper, which is currently not
     // supported.
@@ -76,7 +73,7 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
 
   const match = content.match(/\/[*/]#\s+sourceMappingURL=(?<sourceMappingURL>[^\s]+)/);
   if (match) {
-    const data = dataFromUrl(basePath, match.groups.sourceMappingURL);
+    const data = dataFromUrl(filename, match.groups.sourceMappingURL);
     const url = data ? null : match.groups.sourceMappingURL;
     if (cjsModuleInstance) {
       if (!Module) Module = require('internal/modules/cjs/loader').Module;
@@ -98,21 +95,21 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
   }
 }
 
-function dataFromUrl(basePath, sourceMappingURL) {
+function dataFromUrl(sourceURL, sourceMappingURL) {
   try {
     const url = new URL(sourceMappingURL);
     switch (url.protocol) {
       case 'data:':
-        return sourceMapFromDataUrl(basePath, url.pathname);
+        return sourceMapFromDataUrl(sourceURL, url.pathname);
       default:
         debug(`unknown protocol ${url.protocol}`);
         return null;
     }
   } catch (err) {
     debug(err.stack);
     // If no scheme is present, we assume we are dealing with a file path.
-    const sourceMapFile = resolve(basePath, sourceMappingURL);
-    return sourceMapFromFile(sourceMapFile);
+    const mapURL = new URL(sourceMappingURL, sourceURL).href;
+    return sourceMapFromFile(mapURL);
   }
 }
 
@@ -128,11 +125,11 @@ function lineLengths(content) {
   });
 }
 
-function sourceMapFromFile(sourceMapFile) {
+function sourceMapFromFile(mapURL) {
   try {
-    const content = fs.readFileSync(sourceMapFile, 'utf8');
+    const content = fs.readFileSync(fileURLToPath(mapURL), 'utf8');
     const data = JSONParse(content);
-    return sourcesToAbsolute(dirname(sourceMapFile), data);
+    return sourcesToAbsolute(mapURL, data);
   } catch (err) {
     debug(err.stack);
     return null;
@@ -141,7 +138,7 @@ function sourceMapFromFile(sourceMapFile) {
 
 // data:[<mediatype>][;base64],<data> see:
 // https://tools.ietf.org/html/rfc2397#section-2
-function sourceMapFromDataUrl(basePath, url) {
+function sourceMapFromDataUrl(sourceURL, url) {
   const [format, data] = url.split(',');
   const splitFormat = format.split(';');
   const contentType = splitFormat[0];
@@ -151,7 +148,7 @@ function sourceMapFromDataUrl(basePath, url) {
       Buffer.from(data, 'base64').toString('utf8') : data;
     try {
       const parsedData = JSONParse(decodedData);
-      return sourcesToAbsolute(basePath, parsedData);
+      return sourcesToAbsolute(sourceURL, parsedData);
     } catch (err) {
       debug(err.stack);
       return null;
@@ -165,14 +162,10 @@ function sourceMapFromDataUrl(basePath, url) {
 // If the sources are not absolute URLs after prepending of the "sourceRoot",
 // the sources are resolved relative to the SourceMap (like resolving script
 // src in a html document).
-function sourcesToAbsolute(base, data) {
+function sourcesToAbsolute(baseURL, data) {
   data.sources = data.sources.map((source) => {
     source = (data.sourceRoot || '') + source;
-    if (!/^[\\/]/.test(source[0])) {
-      source = resolve(base, source);
-    }
-    if (!source.startsWith('file://')) source = `file://${source}`;
-    return source;
+    return new URL(source, baseURL).href;
   });
   // The sources array is now resolved to absolute URLs, sourceRoot should
   // be updated to noop.

diff --git a/test/fixtures/source-map/webpack.js b/test/fixtures/source-map/webpack.js
diff --git a/test/fixtures/source-map/webpack.js.map b/test/fixtures/source-map/webpack.js.map
diff --git a/test/parallel/test-source-map-enable.js b/test/parallel/test-source-map-enable.js
@@ -8,6 +8,7 @@ const { dirname } = require('path');
 const fs = require('fs');
 const path = require('path');
 const { spawnSync } = require('child_process');
+const { pathToFileURL } = require('url');
 
 const tmpdir = require('../common/tmpdir');
 tmpdir.refresh();
@@ -88,8 +89,8 @@ function nextdir() {
   // Source-map should have been loaded from disk and sources should have been
   // rewritten, such that they're absolute paths.
   assert.strictEqual(
-    dirname(
-      `file://${require.resolve('../fixtures/source-map/disk-relative-path')}`),
+    dirname(pathToFileURL(
+      require.resolve('../fixtures/source-map/disk-relative-path')).href),
     dirname(sourceMap.data.sources[0])
   );
 }
@@ -109,8 +110,8 @@ function nextdir() {
   // base64 JSON should have been decoded, and paths to sources should have
   // been rewritten such that they're absolute:
   assert.strictEqual(
-    dirname(
-      `file://${require.resolve('../fixtures/source-map/inline-base64')}`),
+    dirname(pathToFileURL(
+      require.resolve('../fixtures/source-map/inline-base64')).href),
     dirname(sourceMap.data.sources[0])
   );
 }
@@ -265,6 +266,23 @@ function nextdir() {
   );
 }
 
+// Does not attempt to apply path resolution logic to absolute URLs
+// with schemes.
+// Refs: https://github.com/webpack/webpack/issues/9601
+// Refs: https://sourcemaps.info/spec.html#h.75yo6yoyk7x5
+{
+  const output = spawnSync(process.execPath, [
+    '--enable-source-maps',
+    require.resolve('../fixtures/source-map/webpack.js')
+  ]);
+  // Error in original context of source content:
+  assert.ok(
+    output.stderr.toString().match(/throw new Error\('oh no!'\)\r?\n.*\^/)
+  );
+  // Rewritten stack trace:
+  assert.ok(output.stderr.toString().includes('webpack:///webpack.js:14:9'));
+}
+
 function getSourceMapFromCache(fixtureFile, coverageDirectory) {
   const jsonFiles = fs.readdirSync(coverageDirectory);
   for (const jsonFile of jsonFiles) {