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

add empty protocol to link whitelist for IE relative links #1715

Merged
merged 2 commits into from
May 23, 2017

Conversation

alexcjohnson
Copy link
Collaborator

Currently relative links (<a href="/path/to/file">Link!</a>) that users insert in their plots fail in IE, because it treats the protocol of such a link as '' - other browsers have these inherit from the page you're in. This fixes that.

@etpinard I can't think of a way to test this (since I can't find any way on Chrome or FF to generate an empty protocol) can you?

@scjody I don't see any way to turn this into an XSS attack, do you?

@etpinard
Copy link
Contributor

can't think of a way to test this (since I can't find any way on Chrome or FF to generate an empty protocol) can you?

Maybe it's time to resurrect #431? Otherwise, no I can't think of an other way.

@scjody
Copy link
Contributor

scjody commented May 23, 2017

This sounds OK from a 🐍 point of view to me.

I checked on our Windows machine. In Edge, /cats has a protocol of "http:" like in Chrome and FF. In IE, /cats has a protocol of undefined.

More critically, for data: URLs, in Edge data:text/html,<script>alert();</script> has a protocol of data:, and in IE11 it has a protocol of data. So we should be good here.

The only issue I can see is that I'm not sure the new test will actually work. In IE11:

''.indexOf(undefined)
-1

@alexcjohnson Have you been able to test this in IE11? I know Browserling offers access for one thing.

@alexcjohnson
Copy link
Collaborator Author

In Edge, /cats has a protocol of "http:" like in Chrome and FF.

Ah great - I didn't test Edge, assumed it would behave like IE but even better if it behaves like Chrome and FF!

In IE, /cats has a protocol of undefined.

Huh, on Browserstack, IE11 in Win10, I saw '' and this fix worked. But I can change it to accept anything falsey, still seem reasonable?

@alexcjohnson
Copy link
Collaborator Author

Oh, even falsey isn't enough - IE9 gives ':' for the protocol here 🙄

@scjody
Copy link
Contributor

scjody commented May 23, 2017

Wow, weird.

Yeah, anything falsey seems reasonable to me.

@alexcjohnson
Copy link
Collaborator Author

Alright, @etpinard especially in light of all the divergent IE behaviors we've seen I don't see much point trying to test this... but at least this fix is now linked forever to #431 in case we ever do push that one through.

Anything else?

@etpinard
Copy link
Contributor

💃 for me.

@alexcjohnson alexcjohnson merged commit d0aa27a into master May 23, 2017
@alexcjohnson alexcjohnson deleted the ie-link-fix branch May 23, 2017 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants