Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

igloox
Copy link

@igloox igloox commented Apr 30, 2014

As per my issue, #138

@hoppipolla-critic-bot
Copy link

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 git rebase -i or git commit --amend) when updating this pull request.

@agafonovdmitry
Copy link

Please, merge this change!

@dieselmachine
Copy link

Yes, please merge this change.

@gsnedders
Copy link
Member

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 <img src="tel:xxx">. Evidence either way appreciated.

@gsnedders gsnedders changed the title added \'tel\' to list of acceptable hyperlink protocols added 'tel' to list of acceptable hyperlink protocols Apr 28, 2015
@agafonovdmitry
Copy link

Just to be sure, as per http://www.w3.org/TR/html5/embedded-content-0.html#attr-img-src :

The src attribute must be present, and must contain a valid non-empty URL potentially surrounded by spaces referencing a non-interactive, optionally animated, image resource that is neither paged nor scripted.

So, I guess "tel" is NOT valid for image. As well as "mailto", by the way.

@gsnedders
Copy link
Member

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.

@willkg
Copy link
Contributor

willkg commented Sep 1, 2017

This PR needs to be updated because the sanitizer has changed.

@agafonovdmitry, @dieselmachine: Can either of you update this PR?

@willkg
Copy link
Contributor

willkg commented Oct 3, 2017

I'm going to close this out. It needs to be updated. Further, it's still not clear whether tel: is safe in all contexts. That needs to be researched. That's better done in the issue rather than in this PR.

@willkg
Copy link
Contributor

willkg commented Oct 5, 2017

@gsnedders reopened #138. Given that, I'll reopen this and merge it.

@willkg willkg reopened this Oct 5, 2017
@willkg
Copy link
Contributor

willkg commented Oct 5, 2017

@igloox Do you have time to update this PR in the next week? If not, I can do it.

@willkg
Copy link
Contributor

willkg commented Oct 31, 2017

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.

@willkg willkg closed this Oct 31, 2017
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.

6 participants