1846189 - Top Site URL validation is broken

Status: NEW

(Firefox :: Top Sites, defect)

Description

[Daniel Veditz [:dveditz]]

I was reviewing the Top Sites change in bug 1838646 (RFE to add file: support) and found that the existing code was already a broken foundation to begin with. There appears to have been a real concern about getting schemeless urls and fixing them up by adding "http://". But most of the checks are done on strings and, as usually happens when you do that, gets things really muddled by having a simplistic view of URLs.

Do we really need to worry about schemeless URLs? What happens if you don't call cleanURL() everywhere? Or even anywhere? Won't those URLs simply break and users will then fix them? It looks like the top sites are mostly coming from history—shouldn't they be well-formed already? (some might have protocols we want to exclude, sure) If we only have to worry about the user-edited ones then stop babysitting them: return an error until they enter a valid URL with one of the protocols we accept (currently http: and https:, which seems perfectly reasonable and safe to me).

More details in my comment in D184802, but visible brokenness can be seen when you try to edit a Top Site to replace its URL:

• Doesn't add http: when it "should": localhost:8080/path/file.html is interpreted as having the protocol "localhost:" and rejected before the "add http://" cleanUrl() is called.
• Adds http: when it shouldn't: https;//foo.com/path/file doesn't appear to have a protocol because of the typo semi-colon, but instead of calling it an invalid URL we run it through cleanUrl() and get the no-less-broken http://https;//foo.com/path/file

We shouldn't be trying fix-up here. Unlike fix-up in the URL-bar, the user doesn't immediately see the result and the source of the wrongness might not be apparent to them. If we're going to do fix-up we cannot do simplistic string operations as this code tries to do. Instead we should do the same fix-up the URL-bar does so future bug fixes or new protocol handling don't diverge. That could be problematic, though, because it looks like this code is not privileged.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:dao, could you have a look please?

For more information, please visit BugBot documentation.