keepassxc-browser icon indicating copy to clipboard operation
keepassxc-browser copied to clipboard

Confuses various input fields with OTP fields

Open Chealer opened this issue 1 year ago • 4 comments

Expected Behavior

KeePassXC should not readily offer to enter one-time passwords in form fields which are not designed to enter passwords (let alone OTPs).

Current Behavior

In several cases, KeePassXC offers a broken button with an OTP icon to enter a one-time password in a form field which is not designed to enter passwords. I suspect the scope of this problem has no limits, but a specific case affecting Desjardins's website is tracked in ticket #2243 . I just stumbled on the same on Building Stack too. As the following screenshot shows, 2 fields in the same form are affected: image

The first confusion comes from a field meant to contain an insurance company, while the second represents an amount of money.

Trying to activate the broken buttons just displays a warning indicating that no TOTP could be found ("Warning! Aucun TOTP n'a été trouvé."). Visibly, the display of at least one of the buttons is also broken.

This happens even if Building Stack does not supports OTPs.

Steps to Reproduce (for bugs)

This depends on the affected form. Regarding the Building Stack case just mentioned, just click the button to upload insurance documents from the email warning that an insurance proof is missing. Regarding desjardins.com, see the ticket specifically tracking that case.

Debug info

KeePassXC - 2.7.9 KeePassXC-Browser - 1.9.1.1 Operating system: Windows 11 Browser: Firefox

Chealer avatar Jul 15 '24 00:07 Chealer

Identifying those fields can be difficult because not many web developers use the standard way. This means false identifications sometimes pass through, especially if the input elements name, id etc. are being used with the local language.

There is not much we can do for there, except reducing or shortening the identification algorithm. That would then break some other sites.

varjolintu avatar Jul 15 '24 05:07 varjolintu

+1 this issue.

I realize that field identification is never flawless, particularly for this field due to lack of standardization as you say. But those icons can be quite a pest when they splash up in places they don't belong. This is especially irksome when it's trying to autofill an OTP for an entry that doesn't even have OTP set up, which in my case is most of them.

What if detection for OTP fields is simply not ran if none of the matched entries for a page have OTPs to fill? The button wouldn't do anything but pop up an error even if it identified a field correctly. I guess that isn't an entirely useless feature; it can serve to remind users that they didn't set up OTP for some entry when they attempt to autofill it, rather than the button not showing up and making users think the plugin is broken. Is that theoretical feature worth all these dud autofill buttons on irrelevant pages, though?

If at least one entry has an OTP, false positives for those pages can be dealt with by manual field selection using the existing override feature. Assuming, that is, that this feature works the way I think it does, that it ignores all automatic detection if a manual override is present. If the entry has both an OTP to fill and a URL relation to autofill into, I think it's reasonable to expect that for the vast majority of cases some OTP field exists that can be chosen to serve this purpose.

I believe the combination of the two above proposed solutions covers the vast bulk of this problem's cases. No adjustment of the identification algorithm necessary. Only the odd case of an entry with an OTP set autofilling to a site that doesn't accept OTPs anywhere is left uncovered.

If one or both of those solutions are not possible or not tenable, I wonder about a feature that could allow users to blacklist specific elements where the algorithm gets it wrong. The plugin already allows field whitelisting. Would field blacklisting be an unreasonably significant feature leap? Playing whack-a-mole site-by-site like this to manually stamp out false positives is an annoying, janky, and brittle solution, but it would be better than no solution.

DiamondIceNS avatar Aug 07 '24 15:08 DiamondIceNS

That's an awesome point we shouldn't be detecting totp fields if the entry doesn't even have totp

Where this might get weird as if somebody has their totp entries in a separate database then this detection would cause problems. However we could include in override mechanism for those rare cases.

droidmonkey avatar Aug 07 '24 16:08 droidmonkey

Also there could be problems if the TOTP is requested from a separate page and requires confirmation via Access Confirm Dialog. Before confirm the extension sees that there is no TOTP for the entry, but then it appears after that. Of course this could be solved. Just one more possible problem situation during the handling.

varjolintu avatar Aug 07 '24 16:08 varjolintu

I don't want to spawn yet another issue, this is closely related.

https://github.com/keepassxreboot/keepassxc-browser/blob/develop/keepassxc-browser/content/totp-field.js#L11

This simply searches for "code" in name and some other attributes. While that's on its own is understandable, there are other separate words it'll false-positively match against: https://www.thefreedictionary.com/words-containing-code

In my situation that was "encodedText". Let me list all worthy examples from the above list:

  • codec, coder
  • encode, decode
  • decoder, encoder / decoded, encoded
  • postcode
  • microcode

I think it's worth to filter these words from potentially false-positive matches. The only counter-example I can think of is "tokenCode" (words code vs encode) that could conceivably be used for some TOTP input.

vadcx avatar Oct 29 '24 14:10 vadcx

Yeah, this word identification method needs some fine-tuning.

varjolintu avatar Oct 29 '24 15:10 varjolintu