Should we also asume callto as safe, as it is still used by some old system for initiating Skype calls.

When in doubt, I'm against it. callto is no official standard, so I can't promise that there is no way to exploit people clicking on the link accidentally and maybe automatically call a paid number or something like this, because some old badly written software doesn't want to make a confirm screen.

This can also happen with tel if you (or some malware) changed the default program.

haven't tested but think the URL escape should be usually used only on components, not on the whole URL? maybe double check those links still work when they include protocol etc

from the docs

url: escapes a string for the URI or parameter contexts. This should not be used to escape an entire URI; only a subcomponent being inserted.

same for the other usages of this

@diosmosis can you change the escaping?

Updated the PR, added a whole_url filter that can be used to whitelist the url. Has to be used in conjunction w/ html_attr or |raw otherwise twig will automatically escape w/ html.

any reason this is not done for whole_url? wouldn't it be the same as simple adding like a filter whole_url that does below logic?

When twig sees an escape like |e('html_attr') it doesn't apply the default |e('html') escape to a variable. But when twig sees |e('whole_url') it applies the default escape. So if we call twig_escape_filter above, it double encodes the URL. Only way I could come up w/ to not do that, is to make sure whole_url doesn't escape anything.

My message was bit confusing, not sure if understood what I meant. Just to be sure... what I meant was we could add whole_url as a regular twig filter like this maybe:

    private function addFilter_wholeUrl()
    {
        $filter = new \Twig_SimpleFilter('wholeUrl', function ($value) {
            if (!UrlHelper::isLookLikeSafeUrl($string)) { ... } return $value
         });
        $this->twig->addFilter($filter);
    }

and then use it like {{ link|wholeUrl|e('html_attr')}}.
Then we wouldn't need the above if... as whole_url is not really escaping but more like filtering/changing the value? And using |e('whole_url') could give a developer the impression the value will be escaped there (and think another escape html_attr is not needed) which in reality it doesn't...

Oh I see, yes, makes sense.

would this need to be javascript:void(0)?

I guess it could be? I'm not sure if it would change anything (assuming it's for preventDefault()?).

Looks good to merge but maybe we should here also execute self:: isLookLikeUrl ($url) and only return true if it actually is a url?

tel:555... and sms:... links do not pass isLookLikeUrl, since they don't have //. I didn't want to regress anything since isLookLikeUrl is used in tracking and probably everywhere else, so didn't modify it.

Note: I did try and make the // optional, but then everything passes isLookLikeUrl

makes sense 👍

small tweak maybe... should the protocol also check for ends with this? as in '/^' . implode('|', $validLinkProtocols) . '$/i' otherwise smstest might be considered a valid protocol?