Add User-Agent header and set request timeout to avoid infinite download hangs

[phoerious]

Description

Fixes hanging favicon downloads for some websites.
Resolves #1573 and resolves #1645

Motivation and context

Some websites seem to accept a TCP connection, but send no HTTP response when no User-Agent header is set. Because we sent no User-Agent and only set a connect timeout, but not a total request timeout, the download hung infinitely.
This patch adds a generic "curl" User-Agent and sets an overall request timeout of 10 seconds.

I removed the CURLOPT_SSL_VERIFYPEER option, because 1L is already the default.

How has this been tested?

I tested it with the example URLs provided in the linked issue reports and could download the favicons successfully.

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]

[weslly]

Some websites block curl requests based on the user agent, wouldn't it be better to use something like Mozilla/5.0 (compatible; https://keepassxc.org/) instead?

[phoerious]

I didn't want to add KeePassXC to the User-Agent to protect users' privacy and adding a fake browser User-Agent seemed a little unnecessary, because we are only querying the favicon, which very often is just a static resource.

[weslly]

I didn't want to add KeePassXC to the User-Agent to protect users' privacy

That's a very good point.

[tycho]

I think keeping the user agent as just "curl" is a giveaway that it's KeePassXC -- indirectly, anyway. What version of curl uses the user agent string "curl" verbatim? None, the command-line curl will do this:

$ curl -v http://google.com 2>&1 | grep User-Agent
> User-Agent: curl/7.58.0

So maybe we should append the libcurl version to the string to match the CLI behavior?

[tycho]

Maybe something like this:

diff --git a/src/gui/EditWidgetIcons.cpp b/src/gui/EditWidgetIcons.cpp
index af4476ac..3a5a7bcf 100644
--- a/src/gui/EditWidgetIcons.cpp
+++ b/src/gui/EditWidgetIcons.cpp
@@ -197,7 +197,7 @@ QImage EditWidgetIcons::fetchFavicon(const QUrl& url)
         curl_easy_setopt(curl, CURLOPT_URL, baUrl.data());
         curl_easy_setopt(curl, CURLOPT_MAXREDIRS, 5L);
         curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
-        curl_easy_setopt(curl, CURLOPT_USERAGENT, "curl");
+        curl_easy_setopt(curl, CURLOPT_USERAGENT, "curl/" LIBCURL_VERSION);
         curl_easy_setopt(curl, CURLOPT_FOLLOWLOCATION, 1L);
         curl_easy_setopt(curl, CURLOPT_TIMEOUT, 10L);
         curl_easy_setopt(curl, CURLOPT_FAILONERROR, 1L);

[phoerious]

I was thinking about it, but I left it out on purpose, because IMHO stating the exact curl version is more information than a server needs.

[tycho]

I agree, but if we're talking about fingerprinting based on user agent strings, then the "curl" stands out more than "curl/someversionnumber"

[phoerious]

I just want it to not say "hey, look, I'm a password manager downloading things". At least PyCURL seems to also have a user agent variant without version number. I don't think a "curl" string without version will draw very much attention.