Prevent concurrent login submissions

[guerricv]

Description

Repeated Enter presses, clicks or YubiKey events can start overlapping login requests while an earlier attempt is still pending. This change allows one submission at a time and gives the form a consistent busy state.

• Lock submitted fields and controls until the attempt completes; restore their previous state after refusal, a network error or an MFA challenge.
• Keep the lock through session-key renewal and credential replay, and wait for an already-running background session check before submitting.
• Preserve the lock during successful login and Duo/OAuth2 redirects; reload a locked page restored by browser Back navigation.
• Add 40 JavaScript regression tests, a dedicated CI job and a manual authentication checklist, using Node.js built-ins only.

Server-side credential validation and brute-force controls are unchanged. Completed attempts can be retried immediately; no artificial delay is introduced.

Related issue

None. General login-form hardening.

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (existing behaviour changes)
• Documentation
• Translation
• Refactor / maintenance

How has this been tested?

All CI checks passed for commit 54b81982b2f7b482681da191863594fd7248751a.

• Quality workflow:
  • PHP 8.2 and 8.3: 1,712 tests and 59,377 assertions passed on each version.
  • Login JavaScript: 40 tests passed with Node.js 22.
  • PHPStan level 4: passed.
  • Production-autoloader and table-prefix guards: passed.
• CodeQL workflow: JavaScript/TypeScript and GitHub Actions analyses passed; the CodeQL PR check also passed.
• Scrutinizer: no new issues; tests passed.

Local checks also passed: 40 JavaScript tests, 94 targeted PHP 8.2 authentication tests (245 assertions), PHPStan level 4, PHP syntax validation on 8.2/8.3, and git diff --check.

The JavaScript suite executes the shipped login code with simulated controls, HTTP responses, navigation and timers. It covers duplicate events, failures, MFA, redirects and session renewal.

Live browser/provider validation remains to be completed: local/LDAP login, Google/YubiKey MFA, Duo/OAuth2 redirects and callbacks, browser Back navigation and expired sessions. Administrator/non-administrator redirects are simulated; live manager/standard/read-only roles, personal folders on/off and audit-log creation have not been exercised. See tests/JavaScript/README.md for the manual checklist.

Checklist

• PHPStan level 4 passes (php app/vendor/bin/phpstan analyse --memory-limit=2G)
• The test suite passes (php _tools/phpunit.phar — see CONTRIBUTING.md for the one-time setup)
• Every new public function has a docblock
• Variable names and comments are in English
• No var_dump() or console.log() left in the code — no new debugging statements; existing guarded logging is unchanged.
• New app/sources/*.queries.php files have a matching public/sources/ proxy shim — not applicable, no new handlers.
• Changes to teampassclasses were applied to both copies (app/includes/libraries/ and app/vendor/) — not applicable, no class changes.
• app/vendor/composer/ is in its production form (git checkout -- app/vendor/composer/)

Impact on install / upgrade

• No schema change
• Schema change — an install/upgrade_run_X.X.X.php script is included and the fresh install path was tested

No installer or upgrade migration is needed.

Screenshots

Not captured locally. The change concerns the form's busy state and submission lifecycle; browser validation is listed above.

[nilsteampassnet]

Thanks for this, the reasoning in the code comments made the review easy to follow.
The overall approach is right and I'd like to merge it after a few corrections.

A few things I want to call out as genuinely well done, because they're the parts I checked hardest: using readOnly instead of disabled on the text fields is the correct call and your docblock explains why (#ga_code / #yubico_key must stay focusable when the response opens the MFA step); the capture-then-restore pattern with the :enabled / :not([readonly]) filters correctly avoids re-enabling controls that were already disabled by something else (#but_identify_user under upgradeRequired(), #forgot-local-password-link during its own request); and loginNavigationPending is set on exactly the paths that really navigate, with showSessionRefreshDialog() always ending in a reload, so the "permanent" lock always terminates.
The pageshow / persisted reload is a good catch. Collapsing the two branches of launchIdentify() is a real simplification and I verified it is semantically identical to develop.

Could you address the following before I merge?

1. The MFA method buttons are not covered by the freeze.
beginLoginAttempt() selects .login-box button:enabled, .login-box input[type="radio"]:enabled.
The visible method selectors are generated by plugins/radioforbuttons/jquery.radiosforbuttons.min.js as <span class="btn radiosforbuttons-2fa_selector_select"> (spans, not buttons) and the plugin moves the underlying radios out of the container. The plugin binds its own click handler, so during an in-flight attempt clicking "Yubico" still highlights the button and checks the source radio, and your new guard at line 1645 then discards the panel switch silently. The visual state and the real state disagree.
Please include those spans in the freeze (or drive a pointer-events: none class off the aria-busy attribute you already set).

2. A hung is_session_key_valid request can wedge the submit button.
The $.post in checkSessionKeyFreshness() (line 265) has no timeout, and launchIdentify() chains on it with $.when(sessionKeyCheckRequest) (line 1043).
If that request never settles, the login attempt is never sent: the form stays frozen behind the "in progress" toast, with no error and no way out but a manual reload.
The ordering is right, it just needs a bound. A timeout: on that request, or racing it against a short timer, is enough.

3. Please drop the issue reference from tests/JavaScript/README.md.
"It does not establish the cause of issue #5365" is review context rather than shipped documentation, and the PR body says "Related issue: None". Move that sentence into the PR description and keep the README focused on how to run the suite.

4. The test harness anchors on comment text.
section(' // On page load') and section(' // Click on log in button\n') mean that rewording a comment fails the suite with Missing login source section: ….
Failing loudly is the right behaviour, but the cost lands on whoever next edits login.js.php. Where you can, anchor on code (function signatures) instead of prose, and add a line to the README explaining what that failure message means so the next person doesn't go hunting. Separately, the PHP stripper /<\?php[\s\S]*?\?>/g is non-greedy and will cut at the first ?> appearing inside a PHP string literal, there are none today, but a short comment saying so would help.

5. Please state the scope explicitly in the PR body.
#but_confirm_otc, #but_confirm_new_password, #but_confirm_defuse_psk and send_user_new_temporary_ga_code() each fire their own $.post with no in-flight guard. That is not a regression and I am fine deferring it, but the PR title reads as if the whole login form is covered, so it is worth saying which parts are in scope.

Two minor points:

• showLoginRequestError() (line 113) interpolates $lang->get() without addslashes(). That matches three existing occurrences in the file (871, 907, 1242), only showSessionRefreshDialog() guards and no language currently has an apostrophe in server_answer_error or caution, so nothing is broken. It is a latent trap a POEditor sync could spring, and it takes down the whole login script when it does.
• $('#but_login_with_oauth2').click() did not get the loginInProgress guard you added to #but_identify_user. Harmless, since launchIdentify() guards and the button is disabled, but the asymmetry invites a future reader to wonder why.

On #5365: I agree with your assessment and I appreciate you writing it down rather than claiming the fix. The reported symptom (a correct password rejected on the first two attempts and accepted on the third) is not what duplicate submissions produce on their own, so this should not be merged as closing that issue. It is still worth having: two spurious failed_auth rows per fast login is exactly what trips nb_bad_authentication, so this narrows the blast radius while we look for the real cause.

On my side I verified: the branch merges cleanly into current develop; the 40 JS tests pass locally (they even pass on Node 18, below the 22 you pinned in CI);
php -l is clean on the rendered template and PHPStan is unaffected;
no public/sources/ shim is needed since no handler was added
and tests/, .github/ and docs/ are excluded from files_reference.txt, so the new files do not require regenerating the integrity checksums.

Thanks again, ping me once the above is in and I'll merge.

[guerricv]

Thanks for the detailed review, @nilsteampassnet. The corrections are in aa593d469.

1. MFA selectors: their span elements now inherit the busy state through .login-box[aria-busy="true"] .radiosforbuttons-2fa_selector_select, with pointer-events: none and reduced opacity. This also covers selectors created while an attempt is pending, and clears automatically when the form is released. The JavaScript guard remains in place. One detail from the code check: the existing .off('click') removes the plugin's direct handler before ours is installed, so I could not confirm that particular two-handler desynchronization scenario. The explicit visual/pointer freeze is still appropriate. The harness now models spans and radios outside the login container, and verifies that method selection works again after a refused attempt.

2. Bounded session requests: both is_session_key_valid and refresh_session_key now use $.ajax() with a 10-second timeout per request. Bounding the renewal matters too, because the pending login waits for that complete chain. A failed/timed-out validity check lets the login continue; a failed/timed-out renewal opens the existing refresh dialog and countdown. jQuery aborts the timed-out request on the client, preventing a late response from changing the page's key or replaying credentials. The authentication request itself keeps its existing behavior, so this does not impose a 10-second limit on LDAP/MFA or a delay between completed attempts. Added tests cover both renewal paths, late responses, and a slower authentication request after successful housekeeping.

3. Issue reference: removed [BUG] Caution. Login credentials do not correspond! #5365 from the test README. Its context now lives in the PR description, explicitly stating that this change neither establishes the cause nor claims to resolve or close that issue.

4. Test extraction: replaced comment-based anchors with function signatures, declarations and event registrations. A regression test rewords the JavaScript line comments and still exercises login successfully. The README explains Missing login source section, and both the harness and README document the PHP substitution's assumption that a PHP string literal does not contain ?>.

5. Scope: the PR body now explicitly covers primary-factor/MFA submissions through launchIdentify(), associated session housekeeping and provider navigation. Independent guards for #but_confirm_otc, #but_confirm_new_password, #but_confirm_defuse_psk and send_user_new_temporary_ga_code() remain outside this change.

6. Translation escaping: the two new messages in showLoginRequestError() now use json_encode() with the JSON_HEX_* flags to produce JavaScript string literals. Two PHP tests render the actual helper with apostrophes, quotes, backslashes, line breaks, Unicode and script-closing text. This checks the PHP output directly, since the Node harness substitutes PHP with inert values.

7. OAuth2 consistency: added the same early loginInProgress / loginNavigationPending guard to the OAuth2 button handler and aligned its indentation with the normal login handler.

All checks are green for this commit:

• Quality CI: 45 JavaScript tests, 1,714 PHP tests / 59,397 assertions on each of PHP 8.2 and 8.3, PHPStan level 4, production-autoloader and table-prefix guards passed.
• CodeQL and its PR check passed.
• Scrutinizer: no new issues; tests passed.

Live LDAP/MFA/SSO testing and visual verification of the selectors remain listed as pending in the PR and manual checklist; the local browser tooling was unavailable. No server-side credential-validation change or new library was introduced.