Skip to content

Commit

Permalink
fix(sourcemaps): don't attempt to treat remote URL as a local file path
Browse files Browse the repository at this point in the history
When a sourceMappingURL is a remote URL (e.g. static storage in an S3 bucket),
`sentry-cli sourcemaps inject` was treating it like a normal file path, attempting
to "normalize" it with the source path, and producing a bogus url such as
`path/to/source/dir/https://some-static-host.example.com/path/to/foo.js.map`,
which of course doesn't exist, and thus the sourcemap isn't found and doesn't have
the debug id injected - even if it's sitting right there in the local directory,
next to its corresponding bundled source file.

With this change, the sourcemap discovery step does not attempt to use the
discovered sourcemap URL if it is a remote URL. Instead, it falls back to guessing
the sourcemap location based on the source filename and its location.

I also changed a couple trycmd tests to remove the `+` before the timezone offset,
as it causes the tests to fail when the local timezone is west of UTC.

Fixes #1846.
  • Loading branch information
brettdh committed Nov 30, 2023
1 parent 791a1f8 commit 7413eff
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 22 deletions.
45 changes: 34 additions & 11 deletions src/utils/sourcemaps.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,14 @@ fn url_matches_extension(url: &str, extensions: &[&str]) -> bool {
.unwrap_or(false)
}

/// Return true iff url is a remote url (not a local path or embedded sourcemap).
fn is_remote_url(url: &str) -> bool {
return match Url::parse(url) {
Ok(url) => url.scheme() != "data",
Err(_) => false,
}
}

impl SourceMapProcessor {
/// Creates a new sourcemap validator.
pub fn new() -> SourceMapProcessor {
Expand Down Expand Up @@ -364,17 +372,32 @@ impl SourceMapProcessor {
continue;
};

let sourcemap_reference = match discover_sourcemaps_location(contents) {
Some(url) => SourceMapReference::from_url(url.to_string()),
None => match guess_sourcemap_reference(&sourcemaps, &source.url) {
Ok(target) => target,
Err(err) => {
source.warn(format!(
"could not determine a source map reference ({err})"
));
self.sourcemap_references
.insert(source.url.to_string(), None);
continue;
let location = match discover_sourcemaps_location(contents) {
// If this is a full external URL, the code below is going to attempt
// to "normalize" it with the source path, resulting in a bogus path
// like "path/to/source/dir/https://some-static-host.example.com/path/to/foo.js.map"
// that can't be resolved to a source map file.
// Instead, we pretend we failed to discover the location, and we fall back to
// guessing the source map location based on the source location.
Some(url) if is_remote_url(url) => None,
Some(filepath) => Some(filepath),
None => None,
};
let sourcemap_reference = match location {
Some(url) => {
SourceMapReference::from_url(url.to_string())
},
None => {
match guess_sourcemap_reference(&sourcemaps, &source.url) {
Ok(target) => target,
Err(err) => {
source.warn(format!(
"could not determine a source map reference ({err})"
));
self.sourcemap_references
.insert(source.url.to_string(), None);
continue;
}
}
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ $ sentry-cli sourcemaps inject ./code ./maps ./maps2 --log-level warn
> Found 1 file
> Analyzing 5 sources
> Injecting debug ids
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] Ambiguous matches for sourcemap path ./code/foo/index.js.map:
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] ./maps/foo/index.js.map
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] ./maps2/foo/index.js.map
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] Ambiguous matches for sourcemap path ./code/foo/index.js.map:
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] ./maps/foo/index.js.map
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] ./maps2/foo/index.js.map

Source Map Debug ID Injection Report
Modified: The following source files have been modified to have debug ids
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@ $ sentry-cli sourcemaps upload tests/integration/_fixtures/upload_some_debugids
> Analyzing 20 sources
> Rewriting sources
> Adding source map references
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] Some source files don't have debug ids:
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] - ~/server/app/page.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] - ~/server/chunks/1.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] - ~/server/dummy_embedded.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] - ~/server/pages/_document.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] - ~/server/pages/api/hello.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] - ~/static/chunks/575-bb7d7e0e6de8d623.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] +[..]:[..] - ~/static/chunks/pages/asdf-05b39167abbe433b.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] Some source files don't have debug ids:
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] - ~/server/app/page.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] - ~/server/chunks/1.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] - ~/server/dummy_embedded.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] - ~/server/pages/_document.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] - ~/server/pages/api/hello.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] - ~/static/chunks/575-bb7d7e0e6de8d623.js
WARN [..]-[..]-[..] [..]:[..]:[..].[..] [..]:[..] - ~/static/chunks/pages/asdf-05b39167abbe433b.js
> Bundled 20 files for upload
> Bundle ID: [..]-[..]-[..]-[..]-[..]
> Uploaded files to Sentry
Expand Down

0 comments on commit 7413eff

Please sign in to comment.