Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Add crossorigin attribute to script HTML tags #2219

Merged
merged 1 commit into from
Jul 6, 2016

Conversation

pardoman
Copy link
Contributor

@pardoman pardoman commented Jun 29, 2016

The Problem
When an error occurs in a script that is not served by the
default server spawned by karma, the stack trace gets lost and
only the message "Script Error" is captured and reported back.
This makes it hard to pin-point where the error originated from.

The Solution
Add crossorigin attribute to generated script tags.

Solution is inspired by this article:
https://blog.getsentry.com/2016/05/17/what-is-script-error.html

@pardoman
Copy link
Contributor Author

To help better explain what is being fixed here, see the following images.

Here's the issue I'm experiencing due to running tests from another server (which given my setup, I require this to be the case). Notice the "Script Error" output circled in red.
image

With the code in this PR, I was able to get the full stack trace of the error:
image

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@pardoman
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@dignifiedquire
Copy link
Member

Thanks for this. Can you please change the commit message to follow our convention

@pardoman
Copy link
Contributor Author

Sure thing, let me try that.

@pardoman pardoman force-pushed the crossorigin-anonymous branch from 79a6d69 to 4ef7c7a Compare June 30, 2016 18:21
@pardoman pardoman changed the title Add crossorigin attribute to script tags fix: Add crossorigin attribute to script HTML tags Jun 30, 2016
@pardoman
Copy link
Contributor Author

@dignifiedquire updated

@pardoman
Copy link
Contributor Author

Hmm, I see errors in tests. I'll take a look

@pardoman pardoman force-pushed the crossorigin-anonymous branch from 4ef7c7a to 93c03fc Compare June 30, 2016 19:16
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

@pardoman pardoman force-pushed the crossorigin-anonymous branch from 93c03fc to edbe99a Compare June 30, 2016 19:23
@googlebot
Copy link

CLAs look good, thanks!

@pardoman pardoman force-pushed the crossorigin-anonymous branch from edbe99a to 39f43c1 Compare June 30, 2016 19:25
@pardoman
Copy link
Contributor Author

I've updated the unit tests

I still haven't figured out how to update the e2e tests for cucumberjs:ci which are still failing (locally)

@pardoman
Copy link
Contributor Author

pardoman commented Jul 1, 2016

Looks like all checks passed.

@dignifiedquire
Copy link
Member

Thanks, just one last thing can you squash the two commits into one?

@pardoman pardoman force-pushed the crossorigin-anonymous branch from 39f43c1 to a0efd42 Compare July 3, 2016 23:54
The Problem:
When an error occurs in a script that is not served by the
default server spawned by karma, the stack trace gets lost and
only the message "Script Error" is captured and reported back.
This makes it hard to pin-point where the error originated from.

The Solution:
Add "crossorigin" attribute to generated script tags.

Solution is inspired by this article:
https://blog.getsentry.com/2016/05/17/what-is-script-error.html

Other:
Update middleware unit test
@pardoman pardoman force-pushed the crossorigin-anonymous branch from a0efd42 to 5690ffe Compare July 3, 2016 23:56
@pardoman
Copy link
Contributor Author

pardoman commented Jul 3, 2016

Done, lets wait for the tests to run again. I've also rebased my branch.

@dignifiedquire dignifiedquire merged commit 49a1449 into karma-runner:master Jul 6, 2016
@dignifiedquire
Copy link
Member

Thanks :octocat:

@pardoman pardoman deleted the crossorigin-anonymous branch July 7, 2016 03:28
@pardoman
Copy link
Contributor Author

pardoman commented Jul 7, 2016

My pleasure 👍

dotch pushed a commit to dotch/karma that referenced this pull request Aug 16, 2016
…n-anonymous"

This reverts commit 49a1449, reversing
changes made to 43dccd1.
@Krinkle
Copy link
Contributor

Krinkle commented Apr 15, 2017

In retrospect, this "bug fix" released in a patch version (v1.1.1) is in fact an entirely new restriction that is imho considered a breaking change. Anyone using Karma with one of the test files served from another server needs to add crossOriginAttribute: false to the configuration. (And that option doesn't exist until v1.4.0).

This happens to coincide with regression fix #2220 for litixsoft/karma-mocha-reporter#67.

Upgrading to v1.1.1 fixed that but then broke everything else. The only way to get it fixed was to update directly to v1.4.0.

This could've been avoided if the option was disabled by default and kept opt-in until a later semver-minor or semver-major release.

Alternatively, it would've been avoided if it was supported to have Karma serve one of conf.files from a path in conf.proxies. That way it would've come from the same origin by default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants