Repository: getsentry/sentry
Pull request: Discover source maps if not on own last line
Fixes #5058 

cc @mattrobenolt

Problem statement:
Sentry should respect sourceMappingURL at EOL
Right now Sentry requires the `sourceMappingURL` directive to be on its own line at the end of a file, which is the literal interpretation of the spec.

But, Chrome and Firefox are more liberal, and will respect `sourceMappingURL` if it follows a bunch of code on the last line of the file, e.g.

```javascript
function add(){for(var a=arguments.length,b=Array(a),c=0;c<a;c++)b[c]=arguments[c];return b.reduce(function(d,e){return d+=e,d},0)}//# sourceMappingURL=app.min.js.map
```

As pointed out by a user, this is easier to produce than first thought:

> However, Webpack's SourceMapDevToolPlugin has an 'append' option, and this requires the dev to remember to add a new line before their sourceMappingURL comment. This is the part that I missed.

Source maps are hard enough as it is; we should probably make this change to be more flexible and make integration easier on users.

cc @maxbittker @mattrobenolt

Hints from issue discussion:

Commit message: Discover source maps if not on own last line

Fixes ###

Patch to review:
diff --git a/src/sentry/lang/javascript/processor.py b/src/sentry/lang/javascript/processor.py
index 4b7f8cadb332df..9d10cb11af888f 100644
--- a/src/sentry/lang/javascript/processor.py
+++ b/src/sentry/lang/javascript/processor.py
@@ -52,7 +52,7 @@ class ZeroReturnError(Exception):
 """, re.X | re.I)
 VERSION_RE = re.compile(r'^[a-f0-9]{32}|[a-f0-9]{40}$', re.I)
 NODE_MODULES_RE = re.compile(r'\bnode_modules/')
-
+SOURCE_MAPPING_URL_RE = re.compile(r'\/\/(?:#|@) ?sourceMappingURL=(.*)$', re.I)
 # the maximum number of remote resources (i.e. sourc eifles) that should be
 # fetched
 MAX_RESOURCE_FETCHES = 100
@@ -155,6 +155,19 @@ def discover_sourcemap(result):
                 # We want everything AFTER the indicator, which is 21 chars long
                 sourcemap = line[21:].rstrip()
 
+        # If we still haven't found anything, check end of last line AFTER source code.
+        # This is not the literal interpretation of the spec, but browsers support this.
+        # e.g. {code}#// sourceMappingURL={url}
+        if not sourcemap:
+            # Only look at last 300 characters to keep search space reasonable (minified
+            # JS on a single line could be tens of thousands of chars). 200 is a totally
+            # arbitrary number / best guess; most sourceMappingURLs are relative and
+            # not very long.
+            search_space = possibilities[-1][-300:].rstrip()
+            match = SOURCE_MAPPING_URL_RE.search(search_space)
+            if match:
+                sourcemap = match.groups()[0]
+
     if sourcemap:
         # react-native shoves a comment at the end of the
         # sourceMappingURL line.
diff --git a/tests/sentry/lang/javascript/test_processor.py b/tests/sentry/lang/javascript/test_processor.py
index 31d4fd750b4681..ad5682a2e4f015 100644
--- a/tests/sentry/lang/javascript/test_processor.py
+++ b/tests/sentry/lang/javascript/test_processor.py
@@ -216,6 +216,10 @@ def test_simple(self):
         result = http.UrlResult('http://example.com', {}, 'console.log(true)\n//# sourceMappingURL=http://example.com/source.map.js\n//# sourceMappingURL=http://example.com/source2.map.js', 200, None)
         assert discover_sourcemap(result) == 'http://example.com/source2.map.js'
 
+        # sourceMappingURL found directly after code w/o newline
+        result = http.UrlResult('http://example.com', {}, 'console.log(true);//# sourceMappingURL=http://example.com/source.map.js', 200, None)
+        assert discover_sourcemap(result) == 'http://example.com/source.map.js'
+
         result = http.UrlResult('http://example.com', {}, '//# sourceMappingURL=app.map.js/*ascii:lol*/', 200, None)
         assert discover_sourcemap(result) == 'http://example.com/app.map.js'
 


Code review comments:
diff_hunk: @@ -155,6 +155,19 @@ def discover_sourcemap(result):
                 # We want everything AFTER the indicator, which is 21 chars long
                 sourcemap = line[21:].rstrip()
 
+        # If we still haven't found anything, check end of last line AFTER source code.
+        # This is not the literal interpretation of the spec, but browsers support this.
+        # e.g. {code}#// sourceMappingURL={url}
+        if not sourcemap:
+            # Only look at last 300 characters to keep search space reasonable (minified
+            # JS on a single line could be tens of thousands of chars). 200 is a totally
+            # arbitrary number / best guess; most sourceMappingURLs are relative and
+            # not very long.
+            search_space = possibilities[-1][-300:].rstrip()
+            match = SOURCE_MAPPING_URL_RE.search(search_space)
+            if match:
+                sourcemap = match.groups()[0]
path: src/sentry/lang/javascript/processor.py
text: @user1:
You could alternatively do `match.group(1)`

diff_hunk: @@ -155,6 +155,19 @@ def discover_sourcemap(result):
                 # We want everything AFTER the indicator, which is 21 chars long
                 sourcemap = line[21:].rstrip()
 
+        # If we still haven't found anything, check end of last line AFTER source code.
+        # This is not the literal interpretation of the spec, but browsers support this.
+        # e.g. {code}#// sourceMappingURL={url}
path: src/sentry/lang/javascript/processor.py
text: @user1:
This comment is wrong.

diff_hunk: @@ -52,7 +52,7 @@ class ZeroReturnError(Exception):
 """, re.X | re.I)
 VERSION_RE = re.compile(r'^[a-f0-9]{32}|[a-f0-9]{40}$', re.I)
 NODE_MODULES_RE = re.compile(r'\bnode_modules/')
-
+SOURCE_MAPPING_URL_RE = re.compile(r'\/\/(?:#|@) ?sourceMappingURL=(.*)$', re.I)
path: src/sentry/lang/javascript/processor.py
text: @user1:
Is the `//@` syntax valid here? I know it's definitely legacy and shouldn't be used for anything newer.

@user1:
I'd say we shouldn't be case insensitive. Everythign else we require an exact `sourceMappingURL` where the case does matter. And back in the day when I looked at source code, browsers were explicitly on casing too.

@user1:
Should the space after `//#` be optional? We don't allow it to be optional elsewhere.

@author:
> Should the space after //# be optional? We don't allow it to be optional elsewhere.

I suppose I haven't seen anyone complain about this, so I'll remove.

@author:
> Is the //@ syntax valid here? I know it's definitely legacy and shouldn't be used for anything newer.

Sure.