Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

TOTP on type=email|tel|username #797

Closed
bwbroersma opened this issue Mar 4, 2020 · 3 comments · Fixed by #798
Closed

TOTP on type=email|tel|username #797

bwbroersma opened this issue Mar 4, 2020 · 3 comments · Fixed by #798
Labels

Comments

@bwbroersma
Copy link
Contributor

bwbroersma commented Mar 4, 2020

Example from Uptime Robot (related to #768):
Fixed in current develop:
image

Still open:
image

Fixed in current develop:

<input id="showQR" class="checkbox" type="checkbox" value="0" name="userAuthSetting">

Because of the Auth in the name, it's detected as TOTP field, while it is a checkbox.

Still open:

<input type="email" name="mfa" placeholder="email"><br>
<input type="tel" name="mfa" placeholder="tel"><br>
<input type="number" name="mfa" placeholder="number"><br>
<input type="text" name="mfa" placeholder="text"><br>
<input type="username" name="mfa" placeholder="username">

Expected Behavior

No Fill in TOTP on a checkbox.
No TOTP on tel/email/username.

Current Behavior

Fill in TOTP overlay over a tel/email/username field. checkbox making it undetectable (I had to disable the addon to interact with the input).

Possible Solution

So the generic whitelist is a bit to broad for TOTP I think:

kpxcObserverHelper.inputTypes = [
'text',
'email',
'password',
'tel',
'number',
'username', // Note: Not a standard
undefined, // Input field can be without any type. Include this and null to the list.
null
];

Add email/tel/username to the negative check:

TOTPFieldIcon.prototype.initField = function(field) {
if (!field
|| field.type === 'password'
|| field.getAttribute('kpxc-totp-field') === 'true'
|| field.offsetWidth < MINIMUM_SIZE
|| field.size < 2
|| (field.maxLength > 0 && field.maxLength < 4)
|| field.id.match(ignoreRegex)
|| field.name.match(ignoreRegex)) {
return;
}

There is a size check underway which would probably fix this specific issue, but not the underlying issue that there currently is no type checking:

kpxc.initOTPFields = function(inputs, databaseClosed) {
for (const i of inputs) {
const id = i.getLowerCaseAttribute('id');
const name = i.getLowerCaseAttribute('name');
const autocomplete = i.getLowerCaseAttribute('autocomplete');
if (autocomplete === 'one-time-code' || acceptedOTPFields.some(f => (id && id.includes(f)) || (name && name.includes(f)))) {
kpxcTOTPIcons.newIcon(i, _databaseClosed);
}
}
};

Debug info

KeePassXC - 2.5.3
KeePassXC-Browser - 1.5.4 develop (1.6.0)
Operating system: Linux x86_64
Browser: Mozilla Firefox 75.0

@droidmonkey
Copy link
Member

This will be fixed in 1.6.0 of the browser extension, already merged into develop.

@bwbroersma
Copy link
Contributor Author

bwbroersma commented Mar 4, 2020

Thanks, I missed it! I just tested 1.6.0 (develop), I updated the main issue.

@bwbroersma bwbroersma changed the title TOTP on checkbox TOTP on type=email|tel|username Mar 4, 2020
@varjolintu
Copy link
Member

varjolintu commented Mar 4, 2020

Some sites actually use tel type for TOTP, so that must not be ignored.

EDIT: Also, it seems that if the type is username, both Firefox and Chromium reverts that back to text.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants