keepassxc-browser
keepassxc-browser copied to clipboard
HackerOne TOTP is instead seen as password field
The current (far from ideal) HackerOne TOTP:
<input autocomplete="off" class="text-field__input" id="sign_in_totp_code" maxlength="6" name="user[totp_code]" type="password" value="">
Of course they should use autocomplete="one-time-code". However the /\btotp\b/ match in combination with maxlength=6, I think KeePassXC-browser should be enough hints to correctly detect TOTP here.
Expected Behavior
Detect TOTP field.
Current Behavior
The field is detected as password field.
Possible Solution
kpxcTOTPIcons.isAcceptedTOTPField(document.getElementById("sign_in_totp_code"))
is true, however because it's detected as a password field first, it seems the field is no longer detected as TOTP:
https://github.com/keepassxreboot/keepassxc-browser/blob/aa288ff797c0d9944734d7e08a6943b8715085d3/keepassxc-browser/content/fields.js#L23-L33
So I tried adding an explicit !isAcceptedTOTPField on line 23:
- if (input.getLowerCaseAttribute('type') === 'password') {
+ if (input.getLowerCaseAttribute('type') === 'password' && !kpxcTOTPIcons.isAcceptedTOTPField(input)) {
- However
passwordwas added as a negative check in #786 because of my complaining in #768.
This results in two solutions:
- Remove
passwordfromignoredTypesplus changingignoreRegexto/(bank|coupon|postal|user|zip)((?!(\b|_)totp(\b|_)).)*code|comment|author|error/i(note:user.*codeis probably to strict for negative, in this case it isuser[totp_code]but I can also imagineuser_mfa_codeetc.). - Adding an explicit
allowRegexwith strong indicators, e.g./\b(totp|otp|2fa|mfa)\b/i, in which case other soft checks are not performed (e.g.ignoredTypesandignoreRegex).
Steps to Reproduce (for bugs)
Enable 2FA on https://hackerone.com/ and sign in.
Debug info
KeePassXC - 2.7.9 KeePassXC-Browser - 1.9.3 Operating system: Linux x86_64 Browser: Mozilla Firefox 131.0
I've a PR for solution 1.
That looks like a good solution to me. I would say, a field that obscures the text (ie a password field) should definitely be weighted more towards a password detection though. False positive hits for TOTP on password fields might be worse then the current state noted on this issue. You can also use custom fields for the website to overcome some auto detection faults.
Your change would need to be tested well on common sites to ensure no regression.
I agree with you both @droidmonkey and @varjolintu that this should be seriously tested.
I do see TOTP having the type=password more often nowadays, e.g. also on namesilo.com 2FA:
<label data-v-0a2585d0="" data-v-779d6dc5="" class=""><span data-v-0a2585d0="" data-v-779d6dc5="">2-Factor Code:</span><input data-v-0a2585d0="" data-v-779d6dc5="" type="password" maxlength="12" class="form-control" autocomplete="off"></label>
Geez and that one had a max length 12 too. So weird.
I agree with you both @droidmonkey and @varjolintu that this should be seriously tested. I do see TOTP having the
type=passwordmore often nowadays, e.g. also on namesilo.com 2FA:<label data-v-0a2585d0="" data-v-779d6dc5="" class=""><span data-v-0a2585d0="" data-v-779d6dc5="">2-Factor Code:</span><input data-v-0a2585d0="" data-v-779d6dc5="" type="password" maxlength="12" class="form-control" autocomplete="off"></label>
Usually type=password fields are masked and useless for TOTP input.