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

isCrossOrigin test Fail on IE 11 #3100

Closed
Naouak opened this Issue Feb 10, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@Naouak
Contributor

Naouak commented Feb 10, 2016

When trying to build on windows 10, I get this output with Karma tests:

IE 11.0.0 (Windows 10 0.0.0) url isCrossOrigin can identify cross origin urls FAILED
        //google.com from http://google.com is not cross origin
        Expected: true
        Actual: false
           at Anonymous function (/video.js/node_modules/qunitjs/qunit/qunit.js:2145:5)
           at Anonymous function (***/Temp/eaf8d6bff56c5e86c49b7c014c3af2eb.browserify:34838:3 <- ..\unit\utils\test\unit\utils\url.test.js:86:2)
           at runTest (/video.js/node_modules/qunitjs/qunit/qunit.js:903:4)
           at Test.prototype.run (/video.js/node_modules/qunitjs/qunit/qunit.js:888:4)
           at Anonymous function (/video.js/node_modules/qunitjs/qunit/qunit.js:1030:6)
           at process (/video.js/node_modules/qunitjs/qunit/qunit.js:691:4)

.........
IE 11.0.0 (Windows 10 0.0.0): Executed 285 of 285 (1 FAILED) (6.589 secs / 6.506 secs)
@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 10, 2016

Member

Sounds like we should just remove that particular check then. I did try and make sure it worked across all browsers and it passes locally.
What version of IE11 are you using? I wonder if it's related to being Windows 10 as well.
We should try to fix it if possible but otherwise just remove that assertion because it's not worth it otherwise.

Member

gkatsev commented Feb 10, 2016

Sounds like we should just remove that particular check then. I did try and make sure it worked across all browsers and it passes locally.
What version of IE11 are you using? I wonder if it's related to being Windows 10 as well.
We should try to fix it if possible but otherwise just remove that assertion because it's not worth it otherwise.

@Naouak

This comment has been minimized.

Show comment
Hide comment
@Naouak

Naouak Feb 10, 2016

Contributor

Karma launch IE and not Edge (don't really know why).
I got IE 11.63.10586.0 / 11.0.26 (KB3104002).

I'm not sure what the test is use for as it is a security enforced by the browser and the player shouldn't really have to check for it.

Contributor

Naouak commented Feb 10, 2016

Karma launch IE and not Edge (don't really know why).
I got IE 11.63.10586.0 / 11.0.26 (KB3104002).

I'm not sure what the test is use for as it is a security enforced by the browser and the player shouldn't really have to check for it.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 10, 2016

Member

It probably isn't launching edge because we don't have the karma edge launcher installed. As for the security aspect, this is a test to make sure that the isCorssOrigin method is returning the correct results. It isn't testing whether the browser is enforcing cross origin. The way that the check is written, though, makes it hard to test in a completely browser-independent way since it relies on a browser feature to do the check.

Member

gkatsev commented Feb 10, 2016

It probably isn't launching edge because we don't have the karma edge launcher installed. As for the security aspect, this is a test to make sure that the isCorssOrigin method is returning the correct results. It isn't testing whether the browser is enforcing cross origin. The way that the check is written, though, makes it hard to test in a completely browser-independent way since it relies on a browser feature to do the check.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Feb 10, 2016

Member

Also, you have a newer version of IE11 than I do, which may or may not be the issue.

Member

gkatsev commented Feb 10, 2016

Also, you have a newer version of IE11 than I do, which may or may not be the issue.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Jul 26, 2016

Member

Closing this issue due to lack of activity. If it is still a problem and we get more information and a reduced test case, we will happily re-open the issue.

Member

gkatsev commented Jul 26, 2016

Closing this issue due to lack of activity. If it is still a problem and we get more information and a reduced test case, we will happily re-open the issue.

@gkatsev gkatsev closed this Jul 26, 2016

@mmodrow

This comment has been minimized.

Show comment
Hide comment
@mmodrow

mmodrow Oct 18, 2017

Contributor

I tried to reproduce this issue, as the test fails for me as well (two different machines with Win10x64 and IE11).
Apparently the problem is, that parseUrl returns {[...], protocol: ""} on IE and {[...], protocol: "https:"} on chrome when calling parseUrl("//www.google.com") from "https://www.google.com".

As isCrossOrigin tests the protocol as well it seems obvious, that "" is not identical to "https:".

Adding the following to parseUrl right before the return would fix it:

if (details.protocol === ''){
  details.protocol = window.location.protocol;
}

I am not sure what causes this, but this makes the test pass.

Contributor

mmodrow commented Oct 18, 2017

I tried to reproduce this issue, as the test fails for me as well (two different machines with Win10x64 and IE11).
Apparently the problem is, that parseUrl returns {[...], protocol: ""} on IE and {[...], protocol: "https:"} on chrome when calling parseUrl("//www.google.com") from "https://www.google.com".

As isCrossOrigin tests the protocol as well it seems obvious, that "" is not identical to "https:".

Adding the following to parseUrl right before the return would fix it:

if (details.protocol === ''){
  details.protocol = window.location.protocol;
}

I am not sure what causes this, but this makes the test pass.

mmodrow pushed a commit to mmodrow/video.js that referenced this issue Oct 18, 2017

mmodrow added a commit to mmodrow/video.js that referenced this issue Oct 18, 2017

mmodrow added a commit to mmodrow/video.js that referenced this issue Oct 18, 2017

mmodrow added a commit to mmodrow/video.js that referenced this issue Oct 18, 2017

@mmodrow mmodrow referenced this issue Oct 18, 2017

Merged

Bugfix for issue #3100 #4673

2 of 7 tasks complete

mmodrow added a commit to mmodrow/video.js that referenced this issue Oct 25, 2017

#3100 fix: moved protocol fix to other protocol related conditionals …
…and expanded from (details.protocol === '') to (!details.protocol)

Both implement suggestions from @misteroneill .

gkatsev added a commit that referenced this issue Oct 31, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment