search

navossoc / KeePass-Yet-Another-Favicon-Downloader

Yet Another Favicon Downloader for KeePass 2.x
MIT License
534 stars 31 forks source link

Add an automatic https option #31

Closed firesoft-de closed 5 years ago

firesoft-de commented 5 years ago

Discoverd your plugin today. Missed an option to automatically prefix urls with the https:// tags. Might be useful at pages who don't allow unsecure http connections and have disabled automatic redirect to https. If I understood your code correct, this patch is just a cosmectic thing and might not improve security (see line below). https://github.com/navossoc/KeePass-Yet-Another-Favicon-Downloader/blob/aa57b95e27e614c20917ae41e663adeabcfa2580/YAFD/FaviconDownloader.cs#L58

This is just quick and dirty and I didn't had the time nor the knowledge to test it so: This pull request needs testing :)

navossoc commented 5 years ago

Thanks, going to check it later.

LevYas commented 5 years ago

Thanks, going to check it later.

Hi! Is it later enough to check it? ;)

navossoc commented 5 years ago

To be honest I did, but I never really started working on it.

@firesoft-de answering your question about that line, it's not about security or anything similar. It's just a quirk (hack) for older versions of mono work properly.

firesoft-de commented 5 years ago

@navosooc Ok, thx for the reply.

If the transport encryption can be specified by changing the prefix to https, there might be no need to do extensive testing on the code.

navossoc commented 5 years ago

Hey, I took a look, this code will not compile. It could be easily fixed though, but...

Since you choose to add a new option to the prefix with https: //. If you enable both options, http:// will take precedence over https and https will never be considered. (I think this is not exactly what you want)

When I did write the code I had the idea that one password entry can have only one URL, so it lead to this (bad) design. That was one of the reasons I took so long to address this pull request.

So, I did a small hack on the code and rewrite it as: If https:// fails, it will retry the url with http:// prefix.

I think this should address the suggestion, since https is becoming the de facto standard and it will fallback to http only in the last case.

Let me know what you think about it. Really sorry for the long delay, too much stuff going on.

Remembering: this feature will only work on URLs without a prefix (schema) and if you enable it!

PS: If everything goes ok in the next few days, I will release it. You can check it right now here: https://github.com/navossoc/KeePass-Yet-Another-Favicon-Downloader/releases/tag/v1.2.2.0-pre

[]'s

firesoft-de commented 5 years ago

Thats a good compromise. Thank you :)