fix: reword mixed-audience login failure notice to be informational#922
fix: reword mixed-audience login failure notice to be informational#922dknauss wants to merge 4 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Seeded demo: 3 failed verification attempts 5 minutes ago, shows the updated failure notice in context (no rate-limit error). Landing page: /wp-login.php?action=two_factor_notice_demo Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
masteradhoc
left a comment
masteradhoc
left a comment
There was a problem hiding this comment.
Thanks for the PR @dknauss - LGTM. Can you check my two pending comments?
The previous notice used "WARNING:" and "If this wasn't you, you should reset your password." The reader has already entered the correct password, so the "if this wasn't you" framing is disorienting for the legitimate user (who likely mistyped a code) and the "reset your password" advice is wrong — the threat is to the second factor, not the password. New text is factual and defers action to after a successful login. Fixes WordPress#919
8ea2810 to
617594d
Compare
617594d to
2cb9233
Compare
There was a problem hiding this comment.
Pull request overview
This PR updates Two_Factor_Core::maybe_show_last_login_failure_notice() to present failed-2FA attempt messaging in a calmer, informational tone that better fits the mixed audience reading the 2FA prompt, and updates tests to match and enforce the new language.
Changes:
- Reword the failed-verification notice to remove alarming/severity phrasing and password-reset guidance, replacing it with post-login security review guidance.
- Update the existing core test to assert the new notice wording.
- Add a new test to ensure the notice language remains calm/informational and avoids specific previously-problematic phrases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| class-two-factor-core.php | Rewords the failed verification code notice text (and translator context) in maybe_show_last_login_failure_notice(). |
| tests/class-two-factor-core.php | Updates existing assertions for the new copy and adds a new test to guard the “calm/informational” language requirements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The '1 times' assertion was a no-op against the reworded notice, and 'failed verification code attempt' also matches the plural 'attempts', so neither form was actually pinned. Assert the rendered singular and plural strings and that the plural is absent for a single failure. Also use MINUTE_IN_SECONDS instead of a bare 60, matching nearby tests.
| * | ||
| * @covers Two_Factor_Core::maybe_show_last_login_failure_notice() | ||
| */ | ||
| public function test_login_failure_notice_language_is_calm_and_informational() { |
There was a problem hiding this comment.
@dknauss
None of these actually check that %2$s rendered as something like "1 minute ago." Is that intended?
There was a problem hiding this comment.
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.
| '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.', |
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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.
A failed 2FA attempt is only recorded after a correct password submission (wp_login -> show_two_factor_login issues the nonce; process_provider increments the counter only past verify_login_nonce). So if the account holder did not make the attempts, someone else knows their password, and a password change is exactly the right remedy. The prior 'review your account security after logging in' was vague, and the rationale for dropping the password advice was backwards. Keep the calm, no-WARNING framing but restore concrete, correct guidance. Also assert the rendered %2$s time in the calm-language test (it was seeded but never checked) and use MINUTE_IN_SECONDS instead of a bare 60.
Companion to PR #917 and PR #921. Fixes #919.
What changed
maybe_show_last_login_failure_notice()inclass-two-factor-core.php.Before:
After:
Why each change
Remove "WARNING:" — same rationale as PR #921: a severity prefix for a temporary informational notice is alarming and inappropriate.
Reframe "if this wasn't you," and keep concrete advice — This notice appears on the 2FA form, which is only reached after a correct password submission, and the failed-attempt counter increments only past that point (
process_provider()runs afterverify_login_nonce()). So if the account holder did not make these attempts, someone else submitted their correct password — and a password change is the correct remedy, because it invalidates the password the other party already has. The new copy drops the alarmingWARNING:framing and the awkward "if this wasn't you," but states the implication plainly and gives the concrete action, deferred to after login because the password can't be changed mid-flow."attempted to login … without providing a valid two factor token" → "failed verification code attempt on this account" — shorter, plain-language phrasing that doesn't require the user to parse "two factor token" to understand what happened.
Tests
test_maybe_show_last_login_failure_notice— updated string assertions to match the new text, pinning both the singular and plural forms.test_login_failure_notice_language_is_calm_and_informational— asserts the absence ofWARNINGandwasn't you; asserts the concrete guidance (someone else may know your password/Change your password after you log in) and the rendered time (The last attempt was … ago) are present.Not addressed here
This notice is only shown when there is no rate-limit error already displayed (see the
elseifcondition inlogin_html()). The rate-limit error message itself was addressed in PR #921.Try it in Playground
Try this change in WordPress Playground — boots the 2FA login showing the reworded failure notice.