Autofill domain check

[st31ny]

This adds an option to check for exact domain matches when auto filling. When the additional security checks are enabled, a failure simply prevents auto fill rather than popping up a modal dialog.
#519

[tuxor1337]

Thanks a lot, this is a great contribution!

From a programming point of view, it's a bit odd to have these two functions without one of them calling the other:

• isSubdomainInclusive checks if the second hostname is a subdomain of the first.
• domainSecurityCheck checks if both hostnames are subdomains of the same domain.

I think the function domainSecurityCheck should extract the domain from the first hostname and then use isSubdomainInclusive to check whether the second hostname is a subdomain of the first's domain.

From a usability point of view, I think that the two check-mark options passff_prefs_autofill_subdomain_check_label and passff_prefs_domain_check_label are very specific and unnatural. It's not very transparent to a user what they do, how they differ and why you would go for one or the other option. We should rather have these options and several combinations thereof:

• A preference to decide which of the following three things to do when a website is not considered trustworthy:
  1. ignore (just auto-fill, don't even do the security checks),
  2. warn (the user may override the security checks),
  3. enforce (the auto-fill is silently omitted if the security checks fail).
• A preference to decide which security checks to apply:
  1. Require that the current website's URL is a valid URL (in the sense of JavaScript's URL feature).
  2. Requre that the URL stored in the password store is a valid URL (in the sense of JavaScript's URL feature).
  3. Require that the current website's hostname and the hostname stored in the password store are both subdomains of the same domain.
  4. Require that the current website's hostname is equal to or a subdomain of the hostname stored in the password store.
  5. Require that the security of the protocol (https or http) is at least as good as the one stored in the password store.
• A preference to decide whether the entry's key name is used as a hostname in the security checks if no url field is present.

Ideally, it would be possible for each of the 5 security checks separately to specify which of ignore/warn/enforce is applied if the check fails...

[st31ny]

From a usability point of view, I think that the two check-mark options passff_prefs_autofill_subdomain_check_label and passff_prefs_domain_check_label are very specific and unnatural. It's not very transparent to a user what they do, how they differ and why you would go for one or the other option.

Yeah, I 100 % agree on this. I actually also needed to look into the source to actually understand what exactly is checked.

In addition, I also thought that we should probably consider PassFF.Preferences.recognisedSuffixes and require that the entry's key name consists of more than a TLD in the security checks.

I hope that I'll find some time today or tomorrow to have a look at the implementation again.

[st31ny]

Sorry, didn't find the time earlier to finish the implementation. So far, I edited the settings page and added the new options for a detailed adjustments for checks. What do you think?

I think that requiring a valid URL can be implicit. When visiting a real website, the URL must be valid anyways and an invalid URL in the pass db can simply cause the check to fail but is not a security issue otherwise. Therefore, I only added the other three levels. Since ii, iii, and iv are getting in stricter in this order, I grouped them in a combo box and added another combo box for the independent setting of the protocol check.

The actual logic needs to be implemented still…

[xeruf]

Hey, any news on this great contribution? :)

[st31ny]

Hey, any news on this great contribution? :)

I was quite busy in the last weeks but hope that I'll find some time at the next weekend or so. But good to hear that others want to see this, too :).

[tuxor1337]

@st31ny Since people occasionally complain about the warnings caused by the security checks (e.g. #564 or #545), the features implemented in this PR would be very welcome. Do you still have plans to wrap up the work?

[st31ny]

@tuxor1337 , sorry that I did not manage to come back to this in such a long time. Do you think the concept still makes sense?

I cannot promise that I'll find some hours in the next weeks, please feel free to build upon my PR.

[tuxor1337]

Yes, I think the concept still makes sense. I just implemented the feature discussed in this PR as far as I understand it.

It would be great if you found the time to test this implementation and provide some feedback. :)