Skip to content

Commit

Permalink
errors: do not call resolve on URLs with schemes
Browse files Browse the repository at this point in the history
We were incorrectly trying to run path.resolve on absolute
sources URLs. This was breaking webpack:// URLs in stack
trace output.

Refs: #35325

PR-URL: #35903
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  • Loading branch information
bcoe authored and targos committed Nov 3, 2020
1 parent b11c737 commit 7c8b5e5
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 36 deletions.
48 changes: 35 additions & 13 deletions 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) => {
Expand All @@ -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.
Expand All @@ -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}`;
Expand All @@ -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}`;
}
Expand All @@ -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;
Expand All @@ -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;
}

Expand Down
31 changes: 12 additions & 19 deletions lib/internal/source_map/source_map_cache.js
Expand Up @@ -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 {
Expand Down Expand Up @@ -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.
Expand All @@ -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;
Expand All @@ -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);
}
}

Expand All @@ -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;
Expand All @@ -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];
Expand All @@ -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;
Expand All @@ -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.
Expand Down
2 changes: 2 additions & 0 deletions test/fixtures/source-map/webpack.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions test/fixtures/source-map/webpack.js.map

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 22 additions & 4 deletions test/parallel/test-source-map-enable.js
Expand Up @@ -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();
Expand Down Expand Up @@ -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])
);
}
Expand All @@ -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])
);
}
Expand Down Expand Up @@ -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) {
Expand Down

0 comments on commit 7c8b5e5

Please sign in to comment.