luckyrat / KeePass-Favicon-Downloader

A KeePass plugin that downloads and stores favicons. A favicon is the little icon / logo used to identify many websites, typically displayed in the browser's address bar, bookmark list and on tabs.
70 stars 19 forks source link

support other formats of URLs #12

Open darkdragon-001 opened 9 years ago

darkdragon-001 commented 9 years ago

It would be nice when you also handle the following URLs correctly: //example.com -> this syntax is used when you don't want to specify protocol (try http: and https:) .example.com -> still try to go there (wildcard redirects like *.example.com should still match this pattern)

luckyrat commented 9 years ago

Neither of those examples are valid URLs and therefore are open to misinterpretation and alternative expectations from other users.

I think that the user should be informed that these URLs were not correct and therefore have the opportunity to modify their KeePass entry to a valid URL.

Specifically: Example 1: This has no meaning outside of an existing HTTP(S) context and while your suggestion to infer an intention to try two specific protocols has some merit, it is unlikely to be the expected behaviour in all cases. Example 2: Where is "there"? I don't see any way we can deterministically select a URL based on a typo like this. Any "best guess" is going to be wrong sometimes and without significant UI changes (probably at the expense of simplicity for all users) this will be annoying and confusing.

darkdragon-001 commented 9 years ago

Example 1: URLs within an html document are advised to be written like this to work for http and https the same way with avoiding the "insecure content" warning. It IS VALID according RFC 3986 within websites (http://stackoverflow.com/a/550073). So when a link like this is detected in the html source code it should be correctly handled! Example 2: favicons normally don't change within an URL, so it should be no problem. I don't know how browsers resolve the issue, but they DO redirect you...

luckyrat commented 9 years ago

1) You're correct, it is valid within websites... KeePass is not a website :-) 2) Browsers have a variety of "typo correction" features which vary between browsers. It works well within the context of a browser but not so well for external programs like KeePass, where I think it is more important to be told that there is a problem because it is due to a permanent (stored in the database) error rather than a temporary typo that may not even happen next time.

Thanks, Chris

On 5 October 2014 20:26, darkdragon-001 notifications@github.com wrote:

Example 1: URLs within an html document are advised to be written like this to work for http and https the same way with avoiding the "insecure content" warning. It IS VALID according RFC 3986 within websites ( http://stackoverflow.com/a/550073). So when a link like this is detected in the html source code it should be correctly handled! Example 2: favicons normally don't change within an URL, so it should be no problem. I don't know how browsers resolve the issue, but they DO redirect you...

— Reply to this email directly or view it on GitHub https://github.com/luckyrat/KeePass-Favicon-Downloader/issues/12#issuecomment-57948863 .

darkdragon-001 commented 9 years ago

1) but keypass uses strings within websites. so when you have a look at the source code it takes just the favicon url stated within the html of the websites (when it finds some). and there it is valid to ommit the protocol ;-) 2) okay

luckyrat commented 9 years ago

OK, I thought you were asking for support in terms of the URL stored in KeePass but if the // syntax does not work when it is the address of the favicon within the source code that's definitely something the plugin should support. I'll take a look into what might be needed to enable support for that.

On 5 October 2014 20:41, darkdragon-001 notifications@github.com wrote:

1) but keypass uses strings within websites. so when you have a look at the source code it takes just the favicon url stated within the html of the websites (when it finds some). and there it is valid to ommit the protocol ;-) 2) okay

— Reply to this email directly or view it on GitHub https://github.com/luckyrat/KeePass-Favicon-Downloader/issues/12#issuecomment-57949364 .

luckyrat commented 9 years ago

It looks like enabling // protocol relative favicon links is going to be possible but I won't have time to look at it until at least next year.

In the mean time, please can you supply a few examples of where this happens since I've not come across it myself yet and need some test cases before starting to implement the feature.

Thanks, Chris

darkdragon-001 commented 9 years ago

Okay, thanks!

And also currently when you only specify the domain and not the url (e.g. "example.com") it tries out to put http:// before. maybe you should try out http and https then ;)

I will look for some test cases or otherwise create them.

luckyrat commented 9 years ago

Yeah it might be worth trying https first and then http. Although I would expect most sites to send redirects to their canonical protocol so hopefully there aren't many examples of this causing a problem in the real world since we have followed redirects for quite a while now.

darkdragon-001 commented 9 years ago

i think most of them use http so, https could be just a fallback.

otherwise there are many more ways nowadays to specify favicons including dimensions, formats, etc. http://stackoverflow.com/questions/23849377/html-5-favicon-support

darkdragon-001 commented 6 years ago

I thought about this again: For the linked favicons: when you get the website via http, // should result to http:// and accordingly with https. For the best guess (trying /favicon.ico), you should try both.

Did you have time to look into this issue?

luckyrat commented 6 years ago

I'm afraid not yet. If you want to submit a PR I'll take a look though.

luckyrat commented 5 years ago

@darkdragon-001 I've been looking at making a new release soon and since I'm now primarily using Linux rather than Windows, that's a bit of a blocker since I doubt I'll have time to re-work everything from the current master into a state that I can build without Visual Studio.

From the looks of it, you have a working fork that has already solved this problem (and some others too if my brief look over your commit titles are anything to go by). Do you think your fork is in or close to being in a state where you could put in a PR to the master branch in this repo?

Being completely ignorant of what's required to build deb files and/or any type of linux package publishing, how would you see that working if I accept a PR containing your changes?

I can obviously add you as a collaborator on this repo if that'll make things easier?

Edit: I meant to put this comment on #11 but in any case, I suspect if we are going to proceed with a PR, we'll rapidly move the discussion to somewhere specific anyway rather than an old issue.

darkdragon-001 commented 4 years ago

@luckyrat Sorry for the delay, but I send you an email to discuss the details!

darkdragon-001 commented 3 years ago

@luckyrat Any news? I am using my changes daily since end of April in Ubuntu without any problems!