-
Notifications
You must be signed in to change notification settings - Fork 294
added 'tel' to list of acceptable hyperlink protocols #149
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
Conversation
Critic review: https://critic.hoppipolla.co.uk/r/1422 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
Please, merge this change! |
Yes, please merge this change. |
In case anyone is curious about the status, this is mostly blocked on me wanting to check that nothing does anything crazy with anything like |
Just to be sure, as per http://www.w3.org/TR/html5/embedded-content-0.html#attr-img-src :
So, I guess "tel" is NOT valid for image. As well as "mailto", by the way. |
It isn't valid, that's for sure. But the sanitizer doesn't currently try and maintain validity (because that's a lot more work), it's merely concerned with possible security consequences. |
This PR needs to be updated because the sanitizer has changed. @agafonovdmitry, @dieselmachine: Can either of you update this PR? |
I'm going to close this out. It needs to be updated. Further, it's still not clear whether |
@gsnedders reopened #138. Given that, I'll reopen this and merge it. |
@igloox Do you have time to update this PR in the next week? If not, I can do it. |
This is out of date and the original author hasn't responded. We've got an issue for supporting it already. Given that, I'm going to close it out. If someone wants this change, they can write up a PR with the change and a corresponding test. |
As per my issue, #138