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

Fix checking subdomains #2253

Merged
merged 1 commit into from Oct 18, 2018

Conversation

@varjolintu
Copy link
Member

varjolintu commented Aug 30, 2018

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]** 
}

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

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 11, 2018

Member

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

QUrl qurl = QUrl::fromUserInput(url);
QString hostname = qurl.host();

if (hostname.isEmpty() || hostname.indexOf(qurl.topLevelDomain()) < 0) {

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 11, 2018

Member

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

@@ -388,14 +388,16 @@ QList<Entry*> BrowserService::searchEntries(Database* db, const QString& hostnam
return entries;
}

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

This comment has been minimized.

Copy link
@droidmonkey

droidmonkey Sep 11, 2018

Member

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

@varjolintu varjolintu force-pushed the varjolintu:subdomain_fix branch from f439aef to 9028fb9 Sep 11, 2018

@varjolintu

This comment has been minimized.

Copy link
Member Author

varjolintu commented Sep 11, 2018

Issues fixed & PR rebased.

@pillarsdotnet

This comment has been minimized.

Copy link

pillarsdotnet commented Sep 13, 2018

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

This comment has been minimized.

Copy link
Member Author

varjolintu commented Sep 13, 2018

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

TheZ3ro left a comment

Seems good

@droidmonkey droidmonkey force-pushed the varjolintu:subdomain_fix branch from 9028fb9 to a8167e3 Sep 19, 2018

@@ -780,6 +780,25 @@ bool BrowserService::removeFirstDomain(QString& hostname)
return false;
}

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

This comment has been minimized.

Copy link
@phoerious

phoerious Sep 24, 2018

Member

Nitpick: why is this not a proper docblock?

@varjolintu varjolintu force-pushed the varjolintu:subdomain_fix branch from a8167e3 to a20d837 Sep 25, 2018

@droidmonkey droidmonkey force-pushed the varjolintu:subdomain_fix branch from a20d837 to 8cbd571 Sep 26, 2018

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

3 checks passed

CodeFactor No issues found.
Details
Ubuntu Linux (KeepassXC) TeamCity build finished
Details
Windows 10 (KeepassXC) TeamCity build finished
Details

@varjolintu varjolintu deleted the varjolintu:subdomain_fix branch Oct 18, 2018

@pahhur

This comment has been minimized.

Copy link

pahhur commented Feb 6, 2019

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

This comment has been minimized.

Copy link
Member

droidmonkey commented Feb 6, 2019

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.

droidmonkey added a commit that referenced this pull request Mar 19, 2019

Release 2.4.0
- 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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.