Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Fix crossorigin attribute logic for Safari #334

Merged
merged 1 commit into from
Apr 11, 2018
Merged

Conversation

rtsao
Copy link
Member

@rtsao rtsao commented Apr 11, 2018

Turns out script.src and script.getAttribute('src') yield different results for relative sources, which made the logic faulty. script.src yields an absolute URL whereas script.getAttribute('src') yields the relative URL.

Fixes #314

The existing test had a false negative because tests are served via http:// instead of https://. The new implementation avoids this pitfall.

@rtsao rtsao force-pushed the fix-crossorigin-check branch from 164b1ec to 70cc371 Compare April 11, 2018 00:09
@rtsao rtsao changed the title Fix crossorigin attr logic for Safari Fix crossorigin attribute logic for Safari Apr 11, 2018
@rtsao rtsao requested review from KevinGrandon and ganemone April 11, 2018 00:18
@@ -29,7 +29,7 @@ class CrossoriginAutoAttributePlugin {
source => {
return source.replace(
/script\.src = [^;]*;/,
'$& if (!script.src.match(/^https:\\/\\//)) {script.crossOrigin = void 0;}'
'$& if (!script.src.indexOf(window.location.origin)) {script.crossOrigin = void 0;}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indexOf() will return a 0 if it starts at the beginning, so should we compare with -1 instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's actually how it's intended to work.

When expanded, the logic looks something like this:

var isSameOrigin = script.src.startsWith(window.location.origin);
// equivalent to:
var isSameOrigin = script.src.indexOf(window.location.origin) === 0;
// equivalent to:
var isSameOrigin = !(script.src.indexOf(window.location.origin) !== 0);
// equivalent to:
var isSameOrigin = !(Boolean(script.src.indexOf(window.location.origin)));
// equivalent to:
var isSameOrigin = !(script.src.indexOf(window.location.origin));

if (isSameOrigin) {
  removeCrossOriginAttribute();
}

I picked the smallest implementation (excluding startsWith because that would require polyfills) since it's going directly into the webpack bootstrap code and assigning variables, etc. is probably a bad idea. Also I don't think Uglify will be able to perform the simplification above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a comment then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's already a fairly extensive comment above that describes what this plugin does and why it is necessary. With that context, I don't think there should be any confusion.

@rtsao rtsao merged commit faa6f74 into master Apr 11, 2018
@KevinGrandon KevinGrandon deleted the fix-crossorigin-check branch July 17, 2018 16:53
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants