Keepassxc: Reject junk URLs for browser integration instead of matching everything

[piegamesde]

I just got a wrong URL match using the KeepassXC browser extension.

Current behaviour

URL in question: https://sso.tu-darmstadt.de/login
URLs of matched entries: http://https//aur.archlinux.org https://archlinux.org, https://passport.twitch.tv https://www.twitch.tv/ https://sso.tu-darmstadt.de/login […] https://print.informatik.tu-darmstadt.de/user

Among the correct entry, there are two false positives. If I allow the correct entry, it is filled in and I can log in. If I click on one of the false positives, it does not fill in and "Error: No logins found" pops up. If I check "Return only best-matching credentials", there still are the false positives, but the correct entry doesn't show up anymore.

Expected behaviour

There are no false positives, only one suggestion. I think it is reasonable to expect that an url must match at least the first and second domain level example.com exactly to be sent over.

Steps to Reproduce (for bugs)

I cannot reproduce this deterministically yet (you can try creating entries with the urls above), but this is not the first time this happens. When it happens, it is always with the same two false positives.

Debug info

KeePassXC - 2.5.0
KeePassXC-Browser - 1.5.3
Operating system: Arch Linux
Browser: Chromium 78.0.3904.70

[droidmonkey]

Do you have multiple urls stuffed into the URL field of the (an) entry?

[piegamesde]

@droidmonkey Yes. But this does not justify such matching behaviour in any way.

[droidmonkey]

Use the new multi-url feature in the entry. Reduce the url field to the primary, then go to the browser integration page of the entry editor, add additional URLs.

By stuffing urls into the url field it confuses the URL parser which outputs junk. We opt to match liberally by default, you can turn on strict matching in the settings.

[varjolintu]

Please note that the URL field is not meant for multiple URL's.

[piegamesde]

If there is an option to be conservative in URL matching and that option is enabled, there is no excuse for false positives, no matter how bullshit the input is. If you make assumptions about the contents of the URL field, either enforce it in the input field or deal with it correctly.

[droidmonkey]

Not disagreeing with you there

[piegamesde]

So, I changed all my entries with multiple URLs and it does not recognize any of them except the "main" URL. But I don't know if this is another bug.

[droidmonkey]

Be careful with http vs https. Also don't put www in front of the domain name. Most sites are getting rid of that convention.

[varjolintu]

How are these multiple URL's entered in the entry URL field in the original issue?

[piegamesde]

@droidmonkey I have https everywhere, enforce protocol matching and don't use www.

@varjolintu In the original issue, I had the URLs as simple space separated list in the field.

---

Where is the current URL matching code located? I'd like to have a look at it.

[varjolintu]

@piegamesde It's this function: https://github.com/keepassxreboot/keepassxc/blob/develop/src/browser/BrowserService.cpp#L996

The parsing is currently done with QUrl::TolerantMode. QUrl::StrictMode would be much better, and using that with isValid() would probably make things right.

[LeBaux]

Use the new multi-url feature in the entry. Reduce the url field to the primary, then go to the browser integration page of the entry editor, add additional URLs.

By stuffing urls into the url field it confuses the URL parser which outputs junk. We opt to match liberally by default, you can turn on strict matching in the settings.

Not to be rude, but shouldn't the parser be able to deal with this? I probably have some deformed entry somewhere in my database and because of it, nothing works. One of my databases has over 1000 entries, a lot of them in Recycle bin. So now I should audit all of it by hand? Feels like the password manager should, you know, manage it. At least detect broken URL fields?

I can swallow the issue myself, but please consider some regex validator for URL field in the future, so the user can't enter invalid URL. On top of it, if one uses the auto-save feature, some websites populate field with possibly malformed URLs, such as:
https://auth.droplr.com/register?callback=https://d.pr/auth/registration&session=j_9ZsgzTT3KcBU3QagiMeg.

I understand you probably have a reason to change how the parser works, but if it breaks on malformed URL it does not feel like it does it job well. This was introduced with KeepassXC 2.5.0, it is possible the issue is tied to main program but I found this opened issue first so I am just adding my 2c.

This example website has 1 matching login. Instead, the parser outputs every login in the database.

Thank you.

[libklein]

I propose not matching invalid urls (i.e. by modifying https://github.com/keepassxreboot/keepassxc/blob/develop/src/browser/BrowserService.cpp#L996 accordingly).

I will prepare a pull request in https://github.com/keepassxreboot/keepassxc/. Should we perhaps move the issue there? Or at least open a new one and reference it from this issue?

[droidmonkey]

There is already an issue on the main application board

[varjolintu]

@LeBaux We are going to fix this soon.

[piegamesde]

Is there any good reason to do some fuzzy matching on URLs anymore? It feels to me that liberal matching was more like a workaround because we only had one URL per entry for a long time. Is there any disadvantage of strict matching that cannot be worked around by adding more specific URLs to an entry?

[droidmonkey]

@varjolintu can you move this to the KeePassXC board?

[libklein]

@varjolintu Do you have any plans on fixing this / rework the url validation/matching system? Thats something i'd like to do as well. If you have any ideas i'd be happy to discuss them, otherwise i'll just write up a pull request as outlined above tonight.

[droidmonkey]

This will be fixed for 2.5.1

[varjolintu]

@libklein I'm currently busy and cannot do much until next week. So if you have a proper solution ready, go ahead.

EDIT: Whoever makes the fix, please make some proper tests for it.

EDIT vol 2: Just got time.

[varjolintu]

I got the fix working. So releasing a PR soon.

[txtsd]

@droidmonkey What version was the Additional URLs feature added in?

[varjolintu]

@txtsd 2.5.0.

[txtsd]

Thanks, it isn't included in the changelog.

[varjolintu]

@txtsd It is. Named as _Browser: Add initial support for multiple URLs_.

[txtsd]

Oh good.