Skip to content

fix: reword mixed-audience login failure notice to be informational #922

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Open
dknauss wants to merge 4 commits into
base: master
Choose a base branch
from
+34 −7
Open

fix: reword mixed-audience login failure notice to be informational #922

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2cb9233
fix: reword mixed-audience login failure notice to be informational
dknauss Jul 3, 2026
907c16e
test: pin singular/plural forms in login failure notice assertions
dknauss Jul 16, 2026
b294cb0
Merge branch 'master' into ux/notice-mixed-audience
masteradhoc Jul 16, 2026
ad1ea7f
fix: give concrete, correct password guidance in login failure notice
dknauss Jul 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1022,10 +1022,10 @@ public static function maybe_show_last_login_failure_notice( $user ) {
echo '<div id="login_notice" class="message"><strong>';
printf(
esc_html(
/* translators: 1: number of failed login attempts, 2: time since last failed attempt */
/* translators: 1: number of failed verification code attempts, 2: human-readable time since the last attempt, e.g. "5 minutes" */
_n(
'WARNING: Your account has attempted to login %1$s time without providing a valid two factor token. The last failed login occurred %2$s ago. If this wasn\'t you, you should reset your password.',
'WARNING: Your account has attempted to login %1$s times without providing a valid two factor token. The last failed login occurred %2$s ago. If this wasn\'t you, you should reset your password.',
'%1$s failed verification code attempt on this account. The last attempt was %2$s ago. If you did not make this attempt, review your account security after logging in.',
'%1$s failed verification code attempts on this account. The last attempt was %2$s ago. If you did not make these attempts, review your account security after logging in.',

@masteradhoc masteradhoc Jul 17, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dknauss didnt test it on my side yet - but im not sure how clear "review your account security after logging in" is. Can we be more clear on what we actually would suggest the user?

@dknauss dknauss Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@masteradhoc Your question sent me back to trace when this notice actually fires, and it turned up a mistake in my own reasoning. (It's quite a confusing trail to keep track of!)

The failed-attempt counter only increments after verify_login_nonce() passes in process_provider() — meaning this screen is only reachable after a correct password submission. So if the account holder didn't make those attempts, someone else submitted their correct password. "Review your account security" wasn't just vague, it was not appropriately flagging the urgency of this situation.

The right remedy is a password change, since that invalidates the password the other party (an assumed attacker) already has.

New wording: "If you did not make these attempts, someone else may know your password. Change your password after you log in."

(Deferred to after login because they can't change it mid-flow.) I also corrected the PR description, which had argued this backwards — it claimed a reset "doesn't help when the password is already known," which is exactly wrong. Pushed in ad1ea7f.

$failed_login_count,
'two-factor'
)
Expand Down
30 changes: 26 additions & 4 deletions tests/class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,9 +808,10 @@ public function test_maybe_show_last_login_failure_notice() {
$contents = ob_get_clean();

$this->assertNotEmpty( $contents );
$this->assertStringNotContainsString( '1 times', $contents );
$this->assertStringContainsString( 'attempted to login', $contents );
$this->assertStringContainsString( 'without providing a valid two factor token', $contents );
// A single failure uses the singular form; assert the plural is absent, since
// "attempt" alone also matches "attempts".
$this->assertStringContainsString( '1 failed verification code attempt on this account', $contents );
$this->assertStringNotContainsString( 'failed verification code attempts', $contents );

// 5 failed login attempts 5 hours ago - User should be informed.
$five_hours_ago = time() - 5 * HOUR_IN_SECONDS;
Expand All @@ -821,10 +822,31 @@ public function test_maybe_show_last_login_failure_notice() {
$contents = ob_get_clean();

$this->assertNotEmpty( $contents );
$this->assertStringContainsString( '5 times', $contents );
$this->assertStringContainsString( '5 failed verification code attempts on this account', $contents );
$this->assertStringContainsString( human_time_diff( $five_hours_ago ), $contents );
}

/**
* Test that the login failure notice uses calm, informational language.
*
* @covers Two_Factor_Core::maybe_show_last_login_failure_notice()
*/
public function test_login_failure_notice_language_is_calm_and_informational() {

@masteradhoc masteradhoc Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dknauss
None of these actually check that %2$s rendered as something like "1 minute ago." Is that intended?

@dknauss dknauss Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right — the test seeded the timestamp a minute back but never checked the rendered output. Added an assertion that the string contains "The last attempt was " . human_time_diff( $one_minute_ago ) . " ago", so the %2$s interpolation is actually exercised. Pushed in ad1ea7f.

$user = $this->get_dummy_user();
update_user_meta( $user->ID, Two_Factor_Core::USER_FAILED_LOGIN_ATTEMPTS_KEY, 3 );
update_user_meta( $user->ID, Two_Factor_Core::USER_RATE_LIMIT_KEY, time() - MINUTE_IN_SECONDS );

ob_start();
Two_Factor_Core::maybe_show_last_login_failure_notice( $user );
$contents = ob_get_clean();

$this->assertStringNotContainsString( 'WARNING', $contents );
$this->assertStringNotContainsString( "wasn't you", $contents );
$this->assertStringNotContainsString( 'reset your password', $contents );
$this->assertStringContainsString( 'failed verification code', $contents );
$this->assertStringContainsString( 'review your account security', $contents );
}

/**
* Test no reset notice when no errors.
*
Expand Down
Loading