Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support other formats of URLs #12

Open
darkdragon-001 opened this issue Sep 28, 2014 · 14 comments
Open

support other formats of URLs #12

darkdragon-001 opened this issue Sep 28, 2014 · 14 comments

Comments

@darkdragon-001
Copy link

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
Copy link
Owner

luckyrat commented Oct 5, 2014

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
Copy link
Author

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
Copy link
Owner

luckyrat commented Oct 5, 2014

  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 [email protected] 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
#12 (comment)
.

@darkdragon-001
Copy link
Author

  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
Copy link
Owner

luckyrat commented Oct 5, 2014

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 [email protected] 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
#12 (comment)
.

@luckyrat
Copy link
Owner

luckyrat commented Oct 6, 2014

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
Copy link
Author

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
Copy link
Owner

luckyrat commented Oct 6, 2014

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
Copy link
Author

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
Copy link
Author

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
Copy link
Owner

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

@luckyrat
Copy link
Owner

luckyrat commented Jan 20, 2019

@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
Copy link
Author

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

@darkdragon-001
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants