Skip to content

Commit 19e0fb1

Browse files
georgestephanisclaude
georgestephanis
and
claude
committed
Only clear the login nonce once it has expired
An unrecognized nonce no longer discards the pending one. Preserve it and let it run out its own expiration instead. Delete-on-mismatch dates back to the original implementation, where the mismatch and expiry cases shared one branch. It offers no brute-force resistance -- the key is 256 bits from random_bytes() inside a ten minute window, it is useless without the second factor, and validate_login_form_2fa() already rotates it whenever a factor fails -- while allowing any unauthenticated request to end another user's in-progress login. Expiry is now checked before the key, so an expired nonce is cleared whatever was presented alongside it. It can never succeed again, so deleting it is not destructive, and an abandoned login no longer leaves dead weight in usermeta until the next password success overwrites it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 93cd055 commit 19e0fb1

2 files changed

Lines changed: 73 additions & 13 deletions

File tree

‎class-two-factor-core.php‎

Lines changed: 22 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1377,23 +1377,39 @@ public static function verify_login_nonce( $user_id, $nonce ) {
13771377
return false;
13781378
}
13791379

1380+
/*
1381+
* An expired nonce can never succeed again, whatever was presented alongside it,
1382+
* so clear it before looking at the key. Deleting it here is not destructive --
1383+
* it was already dead -- and it keeps abandoned logins from leaving dead weight
1384+
* in usermeta until the user's next password success overwrites it.
1385+
*/
1386+
if ( time() >= $login_nonce['expiration'] ) {
1387+
self::log_login_nonce_failure( $user_id, 'expired' );
1388+
self::delete_login_nonce( $user_id );
1389+
1390+
return false;
1391+
}
1392+
13801393
$unverified_nonce = array(
13811394
'user_id' => $user_id,
13821395
'expiration' => $login_nonce['expiration'],
13831396
'key' => $nonce,
13841397
);
13851398

13861399
$unverified_hash = self::hash_login_nonce( $unverified_nonce );
1387-
$hashes_match = $unverified_hash && hash_equals( $login_nonce['key'], $unverified_hash );
13881400

1389-
if ( $hashes_match && time() < $login_nonce['expiration'] ) {
1401+
if ( $unverified_hash && hash_equals( $login_nonce['key'], $unverified_hash ) ) {
13901402
return true;
13911403
}
13921404

1393-
self::log_login_nonce_failure( $user_id, $hashes_match ? 'expired' : 'mismatch' );
1394-
1395-
// Require a fresh nonce if verification fails.
1396-
self::delete_login_nonce( $user_id );
1405+
/*
1406+
* A value we never issued, presented against a nonce that is still live. Leave it
1407+
* in place: discarding it would let an unauthenticated request end someone else's
1408+
* in-progress login, and it buys no brute-force resistance -- the key is 256 bits
1409+
* of random_bytes(), and a failed second factor rotates it in
1410+
* validate_login_form_2fa() regardless.
1411+
*/
1412+
self::log_login_nonce_failure( $user_id, 'mismatch' );
13971413

13981414
return false;
13991415
}

‎tests/class-two-factor-core.php‎

Lines changed: 51 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -700,7 +700,6 @@ public function test_can_verify_login_nonce() {
700700
'Invalid nonce is invalid'
701701
);
702702

703-
// Must create a new one since incorrect nonces deletes them.
704703
$nonce = $this->create_expired_login_nonce( $user_id );
705704

706705
$this->assertFalse(
@@ -802,9 +801,10 @@ public function test_failed_login_nonce_fires_action( $expected_reason, $create_
802801
*/
803802
public function data_login_nonce_failure_reasons() {
804803
return array(
805-
'no pending login' => array( 'no_nonce_stored', false, false, false ),
806-
'wrong value' => array( 'mismatch', true, false, false ),
807-
'past expiration' => array( 'expired', true, true, true ),
804+
'no pending login' => array( 'no_nonce_stored', false, false, false ),
805+
'wrong value' => array( 'mismatch', true, false, false ),
806+
'past expiration' => array( 'expired', true, true, true ),
807+
'past expiration, any value' => array( 'expired', true, true, false ),
808808
);
809809
}
810810

@@ -897,11 +897,14 @@ public function test_successful_login_nonce_does_not_fire_action() {
897897
}
898898

899899
/**
900-
* Invalid nonce deletes the valid nonce.
900+
* An unrecognized nonce leaves the pending nonce intact.
901+
*
902+
* Discarding it would let an unauthenticated request end another user's in-progress
903+
* login, and it buys no brute-force resistance against a 256-bit key.
901904
*
902905
* @covers Two_Factor_Core::verify_login_nonce()
903906
*/
904-
public function test_invalid_nonce_deletes_valid_nonce() {
907+
public function test_invalid_nonce_preserves_valid_nonce() {
905908
$user_id = 123456;
906909
$nonce = Two_Factor_Core::create_login_nonce( $user_id );
907910

@@ -910,9 +913,50 @@ public function test_invalid_nonce_deletes_valid_nonce() {
910913
'Invalid nonce is invalid'
911914
);
912915

916+
$this->assertNotEmpty(
917+
get_user_meta( $user_id, Two_Factor_Core::USER_META_NONCE_KEY, true ),
918+
'The pending nonce survives an unrecognized value'
919+
);
920+
921+
$this->assertTrue(
922+
Two_Factor_Core::verify_login_nonce( $user_id, $nonce['key'] ),
923+
'The correct nonce is still accepted after an invalid one has been attempted'
924+
);
925+
}
926+
927+
/**
928+
* An expired nonce is cleared out of usermeta.
929+
*
930+
* Unlike an unrecognized value, an expired nonce can never succeed again, so there
931+
* is nothing to preserve.
932+
*
933+
* @covers Two_Factor_Core::verify_login_nonce()
934+
*/
935+
public function test_expired_nonce_is_deleted() {
936+
$user_id = 123456;
937+
$nonce = $this->create_expired_login_nonce( $user_id );
938+
913939
$this->assertFalse(
914940
Two_Factor_Core::verify_login_nonce( $user_id, $nonce['key'] ),
915-
'The correct nonce is not accepted after an invalid has been attempted'
941+
'Expired nonce is invalid'
942+
);
943+
944+
$this->assertEmpty(
945+
get_user_meta( $user_id, Two_Factor_Core::USER_META_NONCE_KEY, true ),
946+
'Expired nonce is removed from usermeta'
947+
);
948+
949+
// Cleanup must not depend on the caller knowing the key.
950+
$this->create_expired_login_nonce( $user_id );
951+
952+
$this->assertFalse(
953+
Two_Factor_Core::verify_login_nonce( $user_id, 'not-a-real-nonce' ),
954+
'Expired nonce is invalid whatever is presented'
955+
);
956+
957+
$this->assertEmpty(
958+
get_user_meta( $user_id, Two_Factor_Core::USER_META_NONCE_KEY, true ),
959+
'Expired nonce is removed even when the presented key is wrong'
916960
);
917961
}
918962

0 commit comments

Comments
 (0)