Fix checking subdomains by varjolintu · Pull Request #2253 · keepassxreboot/keepassxc

varjolintu · 2018-08-30T10:54:46Z

Fix checking subdomains when retrieving credentials.

Description

After title matching was removed with release 2.3.4 the subdomain handling broke. Base domain of the URL will be used for EntrySearcher instead of hostname.

With this fix:

Entry URL https://google.com: matches https://*.google.com, does not match http://*.google.com
Entry URL google.com with scheme matching off: matches *://*.google.com
Entry URL https://accounts.google.com: matches https://accounts.google.com, does not match with https://*.google.com
Entry URL accounts.google.com with scheme matching off: matches *://accounts.google.com

Motivation and context

Fixes keepassxreboot/keepassxc-browser#283.
Fixes #2291


## How has this been tested?
Manually, and with local tests.

## Types of changes
<!--- What types of changes does your code introduce? -->
<!--- Please remove all lines which don't apply. -->
- ✅ Bug fix (non-breaking change which fixes an issue)

## Checklist:
<!--- Please go over all the following points. -->
<!--- Again, remove any lines which don't apply. -->
<!--- Pull Requests that don't fulfill all [REQUIRED] requisites are likely -->
<!--- to be sent back to you for correction or will be rejected.  -->
- ✅ 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]**

droidmonkey · 2018-09-11T13:42:30Z

src/browser/BrowserService.cpp

+    }
+
+    hostname.chop(qurl.topLevelDomain().length());
+    return hostname.split('.').last().append(qurl.topLevelDomain());


These two lines need to be broken up and comments added. Took me a while to figure out what was going on.

droidmonkey · 2018-09-11T13:43:49Z

src/browser/BrowserService.cpp

+    QUrl qurl = QUrl::fromUserInput(url);
+    QString hostname = qurl.host();
+
+    if (hostname.isEmpty() || hostname.indexOf(qurl.topLevelDomain()) < 0) {


Recommend changing the second term to !hostname.contains(qurl.topLevelDomain()) for clarity.

droidmonkey · 2018-09-11T13:45:24Z

src/browser/BrowserService.cpp


-    for (Entry* entry : EntrySearcher().search(hostname, rootGroup, Qt::CaseInsensitive)) {
+    QString base = baseDomain(hostname);
+    for (Entry* entry : EntrySearcher().search(base, rootGroup, Qt::CaseInsensitive)) {


Just replace "base" with "baseDomain(hostname)", no need for a temp variable with a questionable name.

varjolintu · 2018-09-11T14:46:16Z

Issues fixed & PR rebased.

pillarsdotnet · 2018-09-13T13:40:50Z

After being annoyed at the bug for a couple weeks, I found this PR, recompiled my copy, and ran it for a couple days. Got two console errors; the first popped up immediately and the second some time later:

bobvin@T560:~/github/keepassxc/build$ keepassxc
QObject::startTimer: Timers cannot have negative intervals
"Maximum depth of replacement has been reached. Entry uuid: {ae9f0413-cf2e-aa93-ac9c-76cb8b9c5986}"

varjolintu · 2018-09-13T14:40:53Z

@pillarsdotnet Those errors are not related to this PR. The first one I have seen multiple times, the second is caused by placeholder resolver.

TheZ3ro

Seems good

phoerious · 2018-09-24T21:40:42Z

src/browser/BrowserService.cpp

    return false;
 }

+// Gets the base domain of URL, e.g. https://another.example.co.uk -> example.co.uk


Nitpick: why is this not a proper docblock?

pahhur · 2019-02-06T17:27:51Z

Ok, I place my question and thoughts about the non-working matching algorithm here,
meaning those that maybe haven't been discussed yet.

Consider, you have a special link, the user wants to open.
Let's say, the link is xyz.de/login

xyz is an international company with locale support on their web sites.

If you call xyz.de/login, you get the german site, no matter if you are in Germany, or elsewhere.
The address is remapped by them to de.xyz.com/login. That's why the typical german user would specify xyz.de/login in the URL field (Yes he could specify de.xyz.com/login, too, but that is not the typical use case. The non-technical user even might not understand that the URL for him has been changed in the background. Furthermore, the webmaster of site xyz might be using a different nomen clatura for that in the next month, and instead of de.xyz.com, then germany.xyz.com would be used, while the original link of xyz.de/login would still work)

If you call xyz.com/login, you get the international site or a local site in the language of the country where you are. The link is remapped to something like www.xyz.com/login, **www.xyz.co.uk/login", or no.xyz.com/login (Norway)

Thus, if the (german) keepassxc user wants to open the correct site for him, he has to specify the .de link. However, for the credentials to be provided correctly by keepassxc, the field must have a value of xyz.com right now.

Also consider that the user is not always be able to change any language settings when in an internet cafe in a foreign country, or might be using a VPN.

Examples of sites with such a behaviour are linkedin, or xing.

Maybe it would be a good idea to separate the field in two. One field for the URL to open, and one optional field for the mapper, in case the mapping cannot be derived from the first URL.

I dont know if this point is a good idea

* Entry URL `https://accounts.google.com`: matches `https://accounts.google.com`, does not match with `https://*.google.com`

for the websites sometimes load balance their requests. Thus a request for

accounts.xyz.com

would sometimes be changed by them to

accounts1.xyz.com
accounts2.xyz.com
accounts3.xyz.com
accounts5.xyz.com
accounts99.xyz.com

If you apply your rule (saying *.xyz.com will not match), then there is no chance for the user to specify a URL that is matching at all, in case he wants to jump to the accounts page and not the xyz.com page.

droidmonkey · 2019-02-06T19:21:02Z

This is what cloned entries are for. You can clone the original one and change the URL to a redirect page or alternative login domain. The latter example you provided is increasingly rare due to modern load balancing being handled by smart dns routing instead of multiple sub domains.

- New Database Wizard [#1952] - Advanced Search [#1797] - Automatic update checker [#2648] - KeeShare database synchronization [#2109, #1992, #2738, #2742, #2746, #2739] - Improve favicon fetching; transition to Duck-Duck-Go [#2795, #2011, #2439] - Remove KeePassHttp support [#1752] - CLI: output info to stderr for easier scripting [#2558] - CLI: Add --quiet option [#2507] - CLI: Add create command [#2540] - CLI: Add recursive listing of entries [#2345] - CLI: Fix stdin/stdout encoding on Windows [#2425] - SSH Agent: Support OpenSSH for Windows [#1994] - macOS: TouchID Quick Unlock [#1851] - macOS: Multiple improvements; include CLI in DMG [#2165, #2331, #2583] - Linux: Prevent Klipper from storing secrets in clipboard [#1969] - Linux: Use polling based file watching for NFS [#2171] - Linux: Enable use of browser plugin in Snap build [#2802] - TOTP QR Code Generator [#1167] - High-DPI Scaling for 4k screens [#2404] - Make keyboard shortcuts more consistent [#2431] - Warn user if deleting referenced entries [#1744] - Allow toolbar to be hidden and repositioned [#1819, #2357] - Increase max allowed database timeout to 12 hours [#2173] - Password generator uses existing password length by default [#2318] - Improve alert message box button labels [#2376] - Show message when a database merge makes no changes [#2551] - Browser Integration Enhancements [#1497, #2253, #1904, #2232, #1850, #2218, #2391, #2396, #2542, #2622, #2637, #2790] - Overall Code Improvements [#2316, #2284, #2351, #2402, #2410, #2419, #2422, #2443, #2491, #2506, #2610, #2667, #2709, #2731]

varjolintu added bug feature: Browser regression labels Aug 30, 2018

varjolintu added this to the v2.4.0 milestone Aug 30, 2018

varjolintu requested review from TheZ3ro, droidmonkey, louib and phoerious August 30, 2018 10:54

This was referenced Aug 30, 2018

Fix Match URL scheme setting #2232

Merged

Passwords are no longer filled for subdomains keepassxreboot/keepassxc-browser#283

Closed

droidmonkey requested changes Sep 11, 2018

View reviewed changes

varjolintu force-pushed the subdomain_fix branch from f439aef to 9028fb9 Compare September 11, 2018 14:45

droidmonkey mentioned this pull request Sep 13, 2018

URL matching seems to be partially broken since v2.3.4 #2291

Closed

varjolintu mentioned this pull request Sep 14, 2018

KeepassXC-Browser no longer detecting credential fields or auto filling keepassxreboot/keepassxc-browser#300

Closed

TheZ3ro approved these changes Sep 14, 2018

View reviewed changes

droidmonkey approved these changes Sep 19, 2018

View reviewed changes

droidmonkey force-pushed the subdomain_fix branch from 9028fb9 to a8167e3 Compare September 19, 2018 02:59

phoerious reviewed Sep 24, 2018

View reviewed changes

varjolintu force-pushed the subdomain_fix branch from a8167e3 to a20d837 Compare September 25, 2018 04:46

varjolintu mentioned this pull request Sep 25, 2018

Match URLs without http/s keepassxreboot/keepassxc-browser#302

Closed

droidmonkey added the PR: Ready for Merge label Sep 26, 2018

Fix checking subdomains

8cbd571

droidmonkey force-pushed the subdomain_fix branch from a20d837 to 8cbd571 Compare September 26, 2018 01:27

varjolintu mentioned this pull request Oct 1, 2018

Allow selection of existing keepassxc entry for page keepassxreboot/keepassxc-browser#71

Closed

varjolintu mentioned this pull request Oct 13, 2018

Autofill no longer works after last update keepassxreboot/keepassxc-browser#311

Closed

droidmonkey merged commit 8074995 into keepassxreboot:develop Oct 18, 2018

varjolintu deleted the subdomain_fix branch October 18, 2018 13:38

droidmonkey mentioned this pull request Oct 19, 2018

keePassXC-Browser not working on Firefox with 2.3.4 #2398

Closed

phoerious added pr: bugfix Pull request fixes a bug and removed bug labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix checking subdomains#2253

Fix checking subdomains#2253
droidmonkey merged 1 commit intokeepassxreboot:developfrom
varjolintu:subdomain_fix

varjolintu commented Aug 30, 2018 •

edited by droidmonkey

Loading

Uh oh!

droidmonkey Sep 11, 2018

Uh oh!

droidmonkey Sep 11, 2018

Uh oh!

droidmonkey Sep 11, 2018

Uh oh!

varjolintu commented Sep 11, 2018

Uh oh!

pillarsdotnet commented Sep 13, 2018 •

edited

Loading

Uh oh!

varjolintu commented Sep 13, 2018

Uh oh!

TheZ3ro left a comment

Uh oh!

phoerious Sep 24, 2018

Uh oh!

pahhur commented Feb 6, 2019 •

edited

Loading

Uh oh!

droidmonkey commented Feb 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

varjolintu commented Aug 30, 2018 • edited by droidmonkey Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and context

Uh oh!

droidmonkey Sep 11, 2018

Choose a reason for hiding this comment

Uh oh!

droidmonkey Sep 11, 2018

Choose a reason for hiding this comment

Uh oh!

droidmonkey Sep 11, 2018

Choose a reason for hiding this comment

Uh oh!

varjolintu commented Sep 11, 2018

Uh oh!

pillarsdotnet commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

varjolintu commented Sep 13, 2018

Uh oh!

TheZ3ro left a comment

Choose a reason for hiding this comment

Uh oh!

phoerious Sep 24, 2018

Choose a reason for hiding this comment

Uh oh!

pahhur commented Feb 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droidmonkey commented Feb 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

varjolintu commented Aug 30, 2018 •

edited by droidmonkey

Loading

pillarsdotnet commented Sep 13, 2018 •

edited

Loading

pahhur commented Feb 6, 2019 •

edited

Loading