Fix favicon download from URL with non-standard port.

[libklein]

Fixes #5001 and #2843.

The favicon download URL was constructed from scheme and host only. This is fixed by simply replacing the path of the original URL with "/favicon.ico", thus keeping scheme, host, auth and port intact.

Further modification: URL's with a non-http schema are now rejected. (See #2843)

Testing strategy

Tested URLs mentioned in #5001 and several well-known domains.

Type of change

• ✅ Bug fix (non-breaking change that fixes an issue)
• ✅ Breaking change (causes existing functionality to change)

[libklein]

As a side note: Perhaps a refactor of the IconDownloader is in order. Right now it tries to download the favicon not only from the host itself, but also from the second-level domain. I'd argue that it would make more sense to consider the host only, i.e. get rid of the second-level domain request.

Moreover, it would be nice to support the W3C recommendation regarding favicons, i.e. to (rudimentarily) parse the page and extract the various tags (c.f. wikipedia).

Any feelings towards this? I'd be happy to provide an implementation, just wanted to hear your opinion on the matter first.

[droidmonkey]

We are not interested in parsing html... but maybe if we just did a very simple regex version.

[droidmonkey]

Also when using the scheme functions calls of QUrl you do not add the ://

[libklein]

Also when using the scheme functions calls of QUrl you do not add the ://

Sorry about that. Will fix it tomorrow (or perhaps wait for #5504, if that is preferred) and then rebase on top of that.

We are not interested in parsing html... but maybe if we just did a very simple regex version.

I agree, that seems like a bit much given that most cases are covered by "/favicon.ico" anyway. What about getting rid of the second-level domain request?

[droidmonkey]

If we regex the html we can get rid of the second level domains.

[libklein]

I've rebased onto #5504, fixed the schema bug and added a test case covering some corner cases, such as query strings, username/password in url and nested domains.

[libklein]

Hi, i just wanted to ask whats the state on this? Are you planning to merge this (soon) or is there something wrong with the PR?

[droidmonkey]

Its in the review queue, you don't have to do anything

[phoerious]

Perhaps we can even split all the downloading functionality into a separate binary for address space separation as an additional mitigation against parser exploits. Then I'd even be fine with doing some more advanced logic than regex.

[libklein]

Hi,

sorry to bother you guys again, but are there any plans to merge this in the near future?

Best regards,
Patrick.

[droidmonkey]

Yes sorry this has been on my review list. Getting through it!