Skip to content

Commit 7c8b5e5

Browse files
bcoetargos
bcoe
authored and
targos
committed
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: #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>
1 parent b11c737 commit 7c8b5e5

File tree

5 files changed

+72
-36
lines changed

5 files changed

+72
-36
lines changed

‎lib/internal/source_map/prepare_stack_trace.js‎

Lines changed: 35 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
'use strict';
22

33
const {
4+
ArrayPrototypeIndexOf,
45
Error,
6+
StringPrototypeStartsWith,
57
} = primordials;
68

79
let debug = require('internal/util/debuglog').debuglog('source_map', (fn) => {
@@ -15,6 +17,7 @@ const {
1517
overrideStackTrace,
1618
maybeOverridePrepareStackTrace
1719
} = require('internal/errors');
20+
const { fileURLToPath } = require('internal/url');
1821

1922
// Create a prettified stacktrace, inserting context from source maps
2023
// if possible.
@@ -40,14 +43,12 @@ const prepareStackTrace = (globalThis, error, trace) => {
4043
}
4144

4245
let errorSource = '';
43-
let firstSource;
4446
let firstLine;
4547
let firstColumn;
4648
const preparedTrace = trace.map((t, i) => {
4749
if (i === 0) {
4850
firstLine = t.getLineNumber();
4951
firstColumn = t.getColumnNumber();
50-
firstSource = t.getFileName();
5152
}
5253
let str = i !== 0 ? '\n at ' : '';
5354
str = `${str}${t}`;
@@ -63,16 +64,22 @@ const prepareStackTrace = (globalThis, error, trace) => {
6364
} = sm.findEntry(t.getLineNumber() - 1, t.getColumnNumber() - 1);
6465
if (originalSource && originalLine !== undefined &&
6566
originalColumn !== undefined) {
66-
const originalSourceNoScheme = originalSource
67-
.replace(/^file:\/\//, '');
6867
if (i === 0) {
6968
firstLine = originalLine + 1;
7069
firstColumn = originalColumn + 1;
71-
firstSource = originalSourceNoScheme;
70+
7271
// Show error in original source context to help user pinpoint it:
73-
errorSource = getErrorSource(firstSource, firstLine, firstColumn);
72+
errorSource = getErrorSource(
73+
sm.payload,
74+
originalSource,
75+
firstLine,
76+
firstColumn
77+
);
7478
}
7579
// Show both original and transpiled stack trace information:
80+
const originalSourceNoScheme =
81+
StringPrototypeStartsWith(originalSource, 'file://') ?
82+
fileURLToPath(originalSource) : originalSource;
7683
str += `\n -> ${originalSourceNoScheme}:${originalLine + 1}:` +
7784
`${originalColumn + 1}`;
7885
}
@@ -88,15 +95,29 @@ const prepareStackTrace = (globalThis, error, trace) => {
8895
// Places a snippet of code from where the exception was originally thrown
8996
// above the stack trace. This logic is modeled after GetErrorSource in
9097
// node_errors.cc.
91-
function getErrorSource(firstSource, firstLine, firstColumn) {
98+
function getErrorSource(payload, originalSource, firstLine, firstColumn) {
9299
let exceptionLine = '';
100+
const originalSourceNoScheme =
101+
StringPrototypeStartsWith(originalSource, 'file://') ?
102+
fileURLToPath(originalSource) : originalSource;
103+
93104
let source;
94-
try {
95-
source = readFileSync(firstSource, 'utf8');
96-
} catch (err) {
97-
debug(err);
98-
return exceptionLine;
105+
const sourceContentIndex =
106+
ArrayPrototypeIndexOf(payload.sources, originalSource);
107+
if (payload.sourcesContent?.[sourceContentIndex]) {
108+
// First we check if the original source content was provided in the
109+
// source map itself:
110+
source = payload.sourcesContent[sourceContentIndex];
111+
} else {
112+
// If no sourcesContent was found, attempt to load the original source
113+
// from disk:
114+
try {
115+
source = readFileSync(originalSourceNoScheme, 'utf8');
116+
} catch (err) {
117+
debug(err);
118+
}
99119
}
120+
100121
const lines = source.split(/\r?\n/, firstLine);
101122
const line = lines[firstLine - 1];
102123
if (!line) return exceptionLine;
@@ -110,7 +131,8 @@ function getErrorSource(firstSource, firstLine, firstColumn) {
110131
}
111132
prefix = prefix.slice(0, -1); // The last character is the '^'.
112133

113-
exceptionLine = `${firstSource}:${firstLine}\n${line}\n${prefix}^\n\n`;
134+
exceptionLine =
135+
`${originalSourceNoScheme}:${firstLine}\n${line}\n${prefix}^\n\n`;
114136
return exceptionLine;
115137
}
116138

‎lib/internal/source_map/source_map_cache.js‎

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ const { Buffer } = require('buffer');
2525
let debug = require('internal/util/debuglog').debuglog('source_map', (fn) => {
2626
debug = fn;
2727
});
28-
const { dirname, resolve } = require('path');
2928
const fs = require('fs');
3029
const { getOptionValue } = require('internal/options');
3130
const {
@@ -63,10 +62,8 @@ function getSourceMapsEnabled() {
6362
function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
6463
const sourceMapsEnabled = getSourceMapsEnabled();
6564
if (!(process.env.NODE_V8_COVERAGE || sourceMapsEnabled)) return;
66-
let basePath;
6765
try {
6866
filename = normalizeReferrerURL(filename);
69-
basePath = dirname(fileURLToPath(filename));
7067
} catch (err) {
7168
// This is most likely an [eval]-wrapper, which is currently not
7269
// supported.
@@ -76,7 +73,7 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
7673

7774
const match = content.match(/\/[*/]#\s+sourceMappingURL=(?<sourceMappingURL>[^\s]+)/);
7875
if (match) {
79-
const data = dataFromUrl(basePath, match.groups.sourceMappingURL);
76+
const data = dataFromUrl(filename, match.groups.sourceMappingURL);
8077
const url = data ? null : match.groups.sourceMappingURL;
8178
if (cjsModuleInstance) {
8279
if (!Module) Module = require('internal/modules/cjs/loader').Module;
@@ -98,21 +95,21 @@ function maybeCacheSourceMap(filename, content, cjsModuleInstance) {
9895
}
9996
}
10097

101-
function dataFromUrl(basePath, sourceMappingURL) {
98+
function dataFromUrl(sourceURL, sourceMappingURL) {
10299
try {
103100
const url = new URL(sourceMappingURL);
104101
switch (url.protocol) {
105102
case 'data:':
106-
return sourceMapFromDataUrl(basePath, url.pathname);
103+
return sourceMapFromDataUrl(sourceURL, url.pathname);
107104
default:
108105
debug(`unknown protocol ${url.protocol}`);
109106
return null;
110107
}
111108
} catch (err) {
112109
debug(err.stack);
113110
// If no scheme is present, we assume we are dealing with a file path.
114-
const sourceMapFile = resolve(basePath, sourceMappingURL);
115-
return sourceMapFromFile(sourceMapFile);
111+
const mapURL = new URL(sourceMappingURL, sourceURL).href;
112+
return sourceMapFromFile(mapURL);
116113
}
117114
}
118115

@@ -128,11 +125,11 @@ function lineLengths(content) {
128125
});
129126
}
130127

131-
function sourceMapFromFile(sourceMapFile) {
128+
function sourceMapFromFile(mapURL) {
132129
try {
133-
const content = fs.readFileSync(sourceMapFile, 'utf8');
130+
const content = fs.readFileSync(fileURLToPath(mapURL), 'utf8');
134131
const data = JSONParse(content);
135-
return sourcesToAbsolute(dirname(sourceMapFile), data);
132+
return sourcesToAbsolute(mapURL, data);
136133
} catch (err) {
137134
debug(err.stack);
138135
return null;
@@ -141,7 +138,7 @@ function sourceMapFromFile(sourceMapFile) {
141138

142139
// data:[<mediatype>][;base64],<data> see:
143140
// https://tools.ietf.org/html/rfc2397#section-2
144-
function sourceMapFromDataUrl(basePath, url) {
141+
function sourceMapFromDataUrl(sourceURL, url) {
145142
const [format, data] = url.split(',');
146143
const splitFormat = format.split(';');
147144
const contentType = splitFormat[0];
@@ -151,7 +148,7 @@ function sourceMapFromDataUrl(basePath, url) {
151148
Buffer.from(data, 'base64').toString('utf8') : data;
152149
try {
153150
const parsedData = JSONParse(decodedData);
154-
return sourcesToAbsolute(basePath, parsedData);
151+
return sourcesToAbsolute(sourceURL, parsedData);
155152
} catch (err) {
156153
debug(err.stack);
157154
return null;
@@ -165,14 +162,10 @@ function sourceMapFromDataUrl(basePath, url) {
165162
// If the sources are not absolute URLs after prepending of the "sourceRoot",
166163
// the sources are resolved relative to the SourceMap (like resolving script
167164
// src in a html document).
168-
function sourcesToAbsolute(base, data) {
165+
function sourcesToAbsolute(baseURL, data) {
169166
data.sources = data.sources.map((source) => {
170167
source = (data.sourceRoot || '') + source;
171-
if (!/^[\\/]/.test(source[0])) {
172-
source = resolve(base, source);
173-
}
174-
if (!source.startsWith('file://')) source = `file://${source}`;
175-
return source;
168+
return new URL(source, baseURL).href;
176169
});
177170
// The sources array is now resolved to absolute URLs, sourceRoot should
178171
// be updated to noop.

‎test/fixtures/source-map/webpack.js‎

Lines changed: 2 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/fixtures/source-map/webpack.js.map‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎test/parallel/test-source-map-enable.js‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const { dirname } = require('path');
88
const fs = require('fs');
99
const path = require('path');
1010
const { spawnSync } = require('child_process');
11+
const { pathToFileURL } = require('url');
1112

1213
const tmpdir = require('../common/tmpdir');
1314
tmpdir.refresh();
@@ -88,8 +89,8 @@ function nextdir() {
8889
// Source-map should have been loaded from disk and sources should have been
8990
// rewritten, such that they're absolute paths.
9091
assert.strictEqual(
91-
dirname(
92-
`file://${require.resolve('../fixtures/source-map/disk-relative-path')}`),
92+
dirname(pathToFileURL(
93+
require.resolve('../fixtures/source-map/disk-relative-path')).href),
9394
dirname(sourceMap.data.sources[0])
9495
);
9596
}
@@ -109,8 +110,8 @@ function nextdir() {
109110
// base64 JSON should have been decoded, and paths to sources should have
110111
// been rewritten such that they're absolute:
111112
assert.strictEqual(
112-
dirname(
113-
`file://${require.resolve('../fixtures/source-map/inline-base64')}`),
113+
dirname(pathToFileURL(
114+
require.resolve('../fixtures/source-map/inline-base64')).href),
114115
dirname(sourceMap.data.sources[0])
115116
);
116117
}
@@ -265,6 +266,23 @@ function nextdir() {
265266
);
266267
}
267268

269+
// Does not attempt to apply path resolution logic to absolute URLs
270+
// with schemes.
271+
// Refs: https://github.com/webpack/webpack/issues/9601
272+
// Refs: https://sourcemaps.info/spec.html#h.75yo6yoyk7x5
273+
{
274+
const output = spawnSync(process.execPath, [
275+
'--enable-source-maps',
276+
require.resolve('../fixtures/source-map/webpack.js')
277+
]);
278+
// Error in original context of source content:
279+
assert.ok(
280+
output.stderr.toString().match(/throw new Error\('oh no!'\)\r?\n.*\^/)
281+
);
282+
// Rewritten stack trace:
283+
assert.ok(output.stderr.toString().includes('webpack:///webpack.js:14:9'));
284+
}
285+
268286
function getSourceMapFromCache(fixtureFile, coverageDirectory) {
269287
const jsonFiles = fs.readdirSync(coverageDirectory);
270288
for (const jsonFile of jsonFiles) {

0 commit comments

Comments
 (0)