Fix correct favicon downloading #719

TheZ3ro · 2017-06-29T18:27:39Z

Description

The user provided URL field can contain various protocol URL other than http and https (like cmd, ftp, sftp, etc)
This PR adds an entry method called webUrl that takes the url field, checks if it's web-friendly (check if the protocol is http or https, adds https by default if the protocol is missing, tries to extract the url from cmd protocol, ...)
The webUrl is then used for downloadingFavicon and for the KeePassHTTP protocol.

If the website don't support https, the request will timeout so we need to retry under http

This fixes #238 and #240

How has this been tested?

Writing tests for this is pretty painful.
I've tested this manually with the following url:

www.google.com url without protocol -> fetch icon for https://www.google.com
www.stealmylogin.com url without protocol, website without https -> checks the https favicon but fails for timeout and then fetch icon for the http version
cmd://firefox www.google.com cmd with url as first command argument -> fetch icon for https://www.google.com
cmd://firefox "http://no-favicon.com/" cmd with url inside quote, unavailable website -> report Unable to fetch favicon
ftp://8.8.8.8 ftp protocol -> favicon download button is hidden.

If someone wants to try to write tests for this, I would be very happy 😄

Types of changes

✅ Bug fix (non-breaking change which fixes an issue)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

lopopolo · 2017-06-29T21:24:54Z

src/gui/EditWidgetIcons.cpp

+            resetFaviconDownload();
+            MessageBox::warning(this, tr("Error"), tr("Unable to fetch favicon."));
+        } else {
+            m_url = m_url.replace("https","http");


does this work on this url?

https://https.com/https/https.https

I was actually trying to fix this as well and I used QRegExp on the URL by matching this: (https?://.+)$ it would pick the last URL, even match cmd://firefox https://https.com/https/https.https properly.

@lopopolo nice catch! I've pushed a fix for that

@hifi what if user's url doesn't hava a protocol? Like cmd://firefox www.google.com ?

@TheZ3ro don't know if it would be an ok workaround but if a fully qualified URL can't be found it would fall back into pulling a string that looks like a consistent domain name from the end of the string?

Quite many "fallbacks" required to support all kinds of scenarios.

weslly · 2017-06-30T20:19:22Z

src/core/Entry.cpp

+
+QString Entry::resolveUrl(const QString& url) const
+{
+    QStringList parts = url.toLower().split("://");


Maybe it would be better to use QUrl to parse the url and get the scheme

I was thinking about that as well, the problem here is that when passing to a QUrl an url like www.google.com this is the result:

qDebug() << url.scheme() << url.host() << url.url(); # "" "" "www.google.com" url.setScheme("https"); qDebug() << url.scheme() << url.host() << url.url(); # "https" "" "https:www.google.com"

Not really what I was expecting.

What happens if you ensure that the string ends with a slash if it doesn't contain any already?

And also, passing to QUrl an url like cmd://firefox www.google.com result in:

qDebug() << uurl.scheme() << uurl.host() << uurl.url(); # "cmd" "" ""

phoerious

I don't like this manual URL parsing. As @weslly suggested, you should really use QUrl for this purpose.

phoerious · 2017-06-30T21:09:20Z

src/gui/EditWidgetIcons.cpp

+            resetFaviconDownload();
+            MessageBox::warning(this, tr("Error"), tr("Unable to fetch favicon."));
+        } else {
+            m_url = m_url.replace(0, 5, "http");


You should change the scheme using QUrl::setScheme()

TheZ3ro · 2017-07-01T09:28:45Z

@phoerious @weslly I've rebased and pushed a new commit that use QUrl when possible.

droidmonkey · 2017-07-01T12:20:58Z

src/core/Entry.cpp

+    if(uurl.scheme() == "cmd") {
+        // URL is a cmd, hopefully the second argument it's an URL
+        QStringList cmd = newurl.split(" ");
+        return resolveUrl(cmd[1].remove("'").remove("\""));


Need to check if there actually is a second element here.

phoerious · 2017-07-01T13:50:12Z

src/core/Entry.cpp

+    }
+    QUrl uurl = QUrl(newurl);
+
+    if(uurl.scheme() == "cmd") {


Spaces please. ;-)

phoerious · 2017-07-01T13:51:03Z

src/core/Entry.cpp

+    QUrl uurl = QUrl(newurl);
+
+    if(uurl.scheme() == "cmd") {
+        // URL is a cmd, hopefully the second argument it's an URL


weslly · 2017-07-01T14:38:22Z

src/core/Entry.cpp

+    QString newurl = url;
+    if (!url.contains("://")) {
+        // URL doesn't have a protocol, add https by default
+        newurl.prepend("https://");


I'm not really sure if we should fallback to https instead of http when the url doesn't have a scheme. While it would be fine for most big sites like google or facebook, it could return a 404 on older sites that don't support SSL. I don't think this would be a security issue since we aren't really submitting any data to the site, just a request for the favicon

This will add https by default to every webUrl. The the webUrl is used for favicon downloading and for the KeePassHTTP protocol atm.
When downloading the favicon if the connection timeout or has an error, the protocol is downgraded to http.
You can test it with a website like www.stealmylogin.com that doesn't has https enabled

phoerious · 2017-07-02T11:11:07Z

src/gui/EditWidgetIcons.cpp

+        QUrl tempurl = QUrl(m_url);
+        if (tempurl.scheme() == "http") {
+            resetFaviconDownload();
+            MessageBox::warning(this, tr("Error"), tr("Unable to fetch favicon."));


Maybe add a hint here to enable Google fallback when it's off. Something along the lines of "Hint: You can enable Google as a fallback under Tools/Settings/Security."

phoerious · 2017-07-02T11:11:44Z

src/gui/EditWidgetIcons.cpp

@@ -235,7 +242,7 @@ void EditWidgetIcons::fetchFaviconFromGoogle(const QString& domain)
    if (config()->get("security/IconDownloadFallbackToGoogle", false).toBool() && m_fallbackToGoogle) {
        resetFaviconDownload();
        m_fallbackToGoogle = false;
-        fetchFavicon(QUrl("http://www.google.com/s2/favicons?domain=" + domain));
+        fetchFavicon(QUrl("https://www.google.com/s2/favicons?domain=" + domain));


Since you're already at it, you should properly escape the domain name. See #731.

louib · 2017-07-05T14:25:19Z

src/core/Entry.cpp

+QString Entry::resolveUrl(const QString& url) const
+{
+#ifdef WITH_XC_HTTP
+    QString newurl = url;


@TheZ3ro newUrl

TheZ3ro · 2017-07-13T22:30:51Z

I should have fixed all the problems @louib @phoerious

Punycode hostname are still a problem due to this #731 (comment)

TheZ3ro · 2017-07-17T18:31:24Z

@phoerious can you take a look at this?

phoerious · 2017-07-17T19:18:40Z

src/gui/EditWidgetIcons.cpp

@@ -235,7 +243,9 @@ void EditWidgetIcons::fetchFaviconFromGoogle(const QString& domain)
    if (config()->get("security/IconDownloadFallbackToGoogle", false).toBool() && m_fallbackToGoogle) {
        resetFaviconDownload();
        m_fallbackToGoogle = false;
-        fetchFavicon(QUrl("http://www.google.com/s2/favicons?domain=" + domain));
+        QUrl faviconUrl = QUrl("https://www.google.com/s2/favicons");
+        faviconUrl.setQuery("domain=" + domain);


This is not going to escape domain properly.

TheZ3ro · 2017-07-17T19:53:46Z

@phoerious now it's explicitly percent encoded

lopopolo · 2017-07-17T19:59:07Z

Why don't you use QUrlQuery like I suggested in GH-731? You shouldn't have to know how to construct and escape a URL query string

On Mon, Jul 17, 2017 at 12:53 PM TheZ3ro ***@***.***> wrote: @phoerious <https://github.com/phoerious> now it's explicitly percent encoded — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#719 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA0hEk9X97hE1mzrN6Zj99Mhuu2IHMGRks5sO7vMgaJpZM4OJshY> .

-- Ryan Lopopolo https://hyperbo.la

TheZ3ro · 2017-07-17T21:22:33Z

@lopopolo Using QUrl::setQuery(QString) is the same as defining a new QUrlQuery, calling QUrlQuery::addQueryItem on it and then doing QUrl::setQuery(QUrlQuery).

From QT documentation of setQuery(QUrlQuery) https://doc.qt.io/qt-5/qurl.html#setQuery-1

This function reconstructs the query string from the QUrlQuery object and sets on this QUrl object. This function does not have parsing parameters because the QUrlQuery contains data that is already parsed.

Also setQuery(QString) already take care of encoding https://doc.qt.io/qt-5/qurl.html#setQuery

Punycode domain are still a problem since QUrl can't encode them correctly (see #731 (comment))

Hope this clarifies the situation

lopopolo · 2017-07-18T00:28:32Z

This relies on a lot of implicit behavior (tolerant parsing mode?). I had to go source diving to verify that the two code paths are equivalent because the docs are unintelligible. I think the python zen of "explicit is better than implicit" applies here.

TheZ3ro · 2017-07-18T09:25:50Z

@lopopolo I agree, documentation for this is pretty bad

TheZ3ro · 2017-07-18T16:25:57Z

Rebased on top of current 2.2.1

- Corrected multiple snap issues [#934, #1011] - Corrected multiple custom icon issues [#708, #719, #994] - Corrected multiple Yubikey issues [#880] - Fixed single instance preventing load on occasion [#997] - Keep entry history when merging databases [#970] - Prevent data loss if passwords were mismatched [#1007] - Fixed crash after merge [#941] - Added configurable auto-type default delay [#703] - Unlock database dialog window comes to front [#663] - Translation and compiling fixes

TheZ3ro requested review from phoerious, droidmonkey and a team June 29, 2017 18:27

lopopolo reviewed Jun 29, 2017

View reviewed changes

weslly reviewed Jun 30, 2017

View reviewed changes

phoerious requested changes Jun 30, 2017

View reviewed changes

TheZ3ro force-pushed the fix/improveDownloadFavicon branch 3 times, most recently from 552b08b to 68c4af6 Compare July 1, 2017 09:28

droidmonkey requested changes Jul 1, 2017

View reviewed changes

droidmonkey approved these changes Jul 1, 2017

View reviewed changes

phoerious reviewed Jul 1, 2017

View reviewed changes

src/core/Entry.cpp Outdated

}

QUrl uurl = QUrl(newurl);

if(uurl.scheme() == "cmd") {

Copy link

Member

phoerious Jul 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spaces please. ;-)

phoerious reviewed Jul 1, 2017

View reviewed changes

weslly reviewed Jul 1, 2017

View reviewed changes

TheZ3ro mentioned this pull request Jul 2, 2017

Fetching favicon for Non-ASCII and Internationalized domains fails #731

Closed

weslly approved these changes Jul 2, 2017

View reviewed changes

phoerious requested changes Jul 2, 2017

View reviewed changes

droidmonkey added this to the v2.2.1 milestone Jul 3, 2017

louib reviewed Jul 5, 2017

View reviewed changes

TheZ3ro force-pushed the fix/improveDownloadFavicon branch 3 times, most recently from e70b3fd to 9a9b4ef Compare July 13, 2017 22:26

louib approved these changes Jul 13, 2017

View reviewed changes

phoerious reviewed Jul 17, 2017

View reviewed changes

TheZ3ro added 6 commits July 18, 2017 18:24

resolve URL for correct favicon downloading, fixes #240 #238

2f868db

resolve URL only with HTTP enabled

6b15f1b

fix google favicon download over https

e242969

update m_url scheme after fallback to http

dc41743

check if url is valid

906b502

use percentEncoding for url query

a7eabbb

TheZ3ro force-pushed the fix/improveDownloadFavicon branch from 5bba56f to a7eabbb Compare July 18, 2017 16:25

phoerious approved these changes Jul 18, 2017

View reviewed changes

phoerious merged commit 8ed8e57 into release/2.2.1 Jul 18, 2017

phoerious deleted the fix/improveDownloadFavicon branch July 18, 2017 16:40

phoerious added the bug label Jul 18, 2017

This was referenced Sep 5, 2017

No favicon download for HTTPS websites #678

Closed

Using cmd:// url breaks icon download and http plugin #238

Closed

droidmonkey mentioned this pull request Sep 10, 2017

Remove qhttp and KeePassHTTP functionality #913

Closed

phoerious added pr: bugfix Pull request that fixes a bug and removed bug labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix correct favicon downloading #719

Fix correct favicon downloading #719

TheZ3ro commented Jun 29, 2017

lopopolo Jun 29, 2017

hifi Jun 30, 2017

TheZ3ro Jun 30, 2017

hifi Jun 30, 2017

weslly Jun 30, 2017

phoerious Jun 30, 2017

TheZ3ro Jul 1, 2017

phoerious Jul 1, 2017

TheZ3ro Jul 1, 2017

phoerious left a comment

phoerious Jun 30, 2017

TheZ3ro commented Jul 1, 2017

droidmonkey Jul 1, 2017

phoerious Jul 1, 2017

phoerious Jul 1, 2017

weslly Jul 1, 2017 •

edited

Loading

TheZ3ro Jul 1, 2017

phoerious Jul 2, 2017

phoerious Jul 2, 2017 •

edited

Loading

louib Jul 5, 2017

TheZ3ro commented Jul 13, 2017

TheZ3ro commented Jul 17, 2017

phoerious Jul 17, 2017

TheZ3ro commented Jul 17, 2017

lopopolo commented Jul 17, 2017 via email

TheZ3ro commented Jul 17, 2017

lopopolo commented Jul 18, 2017 •

edited

Loading

TheZ3ro commented Jul 18, 2017

TheZ3ro commented Jul 18, 2017

Fix correct favicon downloading #719

Fix correct favicon downloading #719

Conversation

TheZ3ro commented Jun 29, 2017

Description

How has this been tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheZ3ro commented Jul 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weslly Jul 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jul 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheZ3ro commented Jul 13, 2017

TheZ3ro commented Jul 17, 2017

Choose a reason for hiding this comment

TheZ3ro commented Jul 17, 2017

lopopolo commented Jul 17, 2017 via email

TheZ3ro commented Jul 17, 2017

lopopolo commented Jul 18, 2017 • edited Loading

TheZ3ro commented Jul 18, 2017

TheZ3ro commented Jul 18, 2017

weslly Jul 1, 2017 •

edited

Loading

phoerious Jul 2, 2017 •

edited

Loading

lopopolo commented Jul 18, 2017 •

edited

Loading