Improve TOTP field

[DL6ER]

What does this implement/fix?

Fix misaligned TOTP field icon, make password field larger when TOTP is enabled and fix auto-submission to only fire when the user has already put in something into the password field.

Screenshots

TOTP not enabled, before and after

TOTP enabled, development-v6

(misaligned icon, small password field)

TOTP enabled, this PR

Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A

---

By submitting this pull request, I confirm the following:

1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
2. I have commented my proposed changes within the code.
3. I am willing to help maintain this change if there are issues with it later.
4. It is compatible with the EUPL 1.2 license
5. I have squashed any insignificant commits. (git rebase)

Checklist:

• The code change is tested and works locally.
• I based my code and PRs against the repositories developmental branch.
• I signed off all commits. Pi-hole enforces the DCO for all contributions
• I signed all my commits. Pi-hole requires signatures to verify authorship
• I have read the above and my PR is ready for review.

[DL6ER]

https://web.dev/articles/sign-in-form-best-practices

This adds a "show password" button:

[yubiuser]

I think we should add a bit of space between the https warning and show password

[rdwebdesign]

Do we need the key icon?
Can we use an eye icon as "show password" button?

Something like this:

[DL6ER]

That's a very good solution, if you have this already at hand ;-) Feel free to push ti this branch

[rdwebdesign]

I will do it in a few minutes.

[yubiuser]

The click/hover animation (color) changes stays after click until one clicks somewhere else.

I think this should be a hover only.

[yubiuser]

Entering the password first and submit correct TOTP afterwards shows "Wrong password" briefly before auto-login

[yubiuser]

There is no distinction between entering a wrong password or a wrong TOTP. It will always complain about "Wrong password" and autofocus on the password field - even when the TOTP was wrong.

[rdwebdesign]

This is the :focus style.
Every button has the same effect, but usually the focus changes to some other element after the click.

We can change the focus to the password field, like this:

[DL6ER]

Entering the password first and submit correct TOTP afterwards shows "Wrong password" briefly before auto-login

I am not able to reproduce this and looking again at the Javascript code, it also isn't immediately obvious to me how this could be happening. Could you try again and screenshot the Network tab frpm your browser for me with additional screenshots of the reply contents?

Please make sure to have "Persist Logs" enabled for them to "survive" the login process.

---

There is no distinction between entering a wrong password or a wrong TOTP. It will always complain about "Wrong password" and autofocus on the password field - even when the TOTP was wrong.

I will change this to be clearer.

[DL6ER]

Wrong password

Correct password but wrong 2FA token

[yubiuser]

If you think letting users know if they entered a wrong password or a wrong TOTP is a security issue, I'm fine with leaving it as it is. I know this discussion, but it feels a bit like "security by obfuscation". If the second factor is so weak that it can be brute-forced within 30 seconds (before a new TOTP is generated), the second factor is broken anyway.

[DL6ER]

Yeah, since online-bruteforcing is prevented by FTL and when they can get physical/root access to either the Pi-hole or the device having the Authenticator app they'll trivially have the TOTP secret, I am fine with showing the user whether it was the password or the token that was wrong. Seem more user-friendly to me.

[yubiuser]

Entering the password first and submit correct TOTP afterwards shows "Wrong password" briefly before auto-login

I'm not seeing this today, but there are two calls to /auth with the second one failing which might be the reason I saw it yesterday. Disabling 2FA, only one /auth request is logged.

[yubiuser]

The issue observed was triggered by pasting via Ctl+V being recognized as two events (paste and keyup). I pushed a commit to only trigger on input. I tested it with pre-filled password with: entering TOTP digit by digit, Ctr-V and pasting with mouse. No double login was performed.

[yubiuser]

Is there a reason we prevent a TOTP re-use within 30 seconds?

2023-11-05 23:13:25.448 [399838/T404370] INFO: 2FA code verified successfully at 0
2023-11-05 23:13:33.616 [399838/T404372] WARNING: 2FA code has already been used (-1, 437220), please wait 27 seconds
2023-11-05 23:13:33.618 [399838/T404372] WARNING: API: Invalid 2FA token

This seems to be a too long lock interval.

[DL6ER]

Is there a reason we prevent a TOTP re-use within 30 seconds?

It is a Time-Based One-Time Password Algorithm :-)

FTL has the answer to this question here:

Verify code
RFC 6238 (section 4.2): If the calculated value matches the value provided by the user, then the user is authenticated
RFC 6238 (section 4.3): The server MUST NOT accept a TOTP value generated more than 30 seconds in the future
RFC 6238 (section 4.3): The server MUST NOT accept a TOTP value generated more than 30 seconds in the past
RFC 6238 (section 4.3): The server MUST NOT accept a TOTP value it accepted previously

furthermore:

We RECOMMEND a default time-step size of 30 seconds. This default value of 30 seconds is selected as a balance between security and usability.

and sections 5.3:

Note that a prover may send the same OTP inside a given time-step window multiple times to a verifier. The verifier MUST NOT accept the second attempt of the OTP after the successful validation has been issued for the first OTP, which ensures one-time only use of an OTP.

https://github.com/pi-hole/FTL/blob/afb6f865187099557aa6bb3332286ca2bf6730e7/src/api/2fa.c#L223-L224

The point is: Someone can somehow see your keystrokes. You enter your password. You enter your TOTP code. They immediately try to use them to log in and they'd succeed. The motivation behind the last point is that, even though they're still within the 30 second window for that code, it shouldn't work for they - specifically because you already used it.

[DL6ER]

With the most recent changes, the login page can much better react on errors:

Initial state

No password is supplied, submission is prevented

Wrong password is entered

Rate-limited

(simply mash Enter)

Wrong 2FA token is provided

Number of API seats is exceeded

(set webserver.api.max_sessions = 1 and try to login in a private window)

---

For rate-limiting and API seat exhaustion to be reported correctly, you have to use FTL from pi-hole/FTL#1733 (branch new/num_sessions)

[DL6ER]

The last commit adds the inclusion of hints from FTL (if available). For example:

and

[rdwebdesign]

I noticed this wasn't reviewed after your last 2 commits.

I tested and it looks fine, but I'm requesting yubiuser approval, because you 2 were discussing about error/warning messages.

[sefinek]

With the LastPass extension, the width of the input is other.

brave_Ey6VtymkArH8.mp4

[rdwebdesign]

This is an external bug.

LastPass extension modifies HTML page content (adding a <div> element to hold the icon) and their modification doesn't work here.
Not our fault, they just make assumptions they shouldn't make.

For example:
if they had used position: absolute instead of relative, the layout wouldn't break here.

[sefinek]

Maybe it's worth informing them about it?

[yubiuser]

Sure, go ahead. You can link the issue/analysis here so they know what you are referring to.