Skip to content

Commit a766c19

Browse files
Keep the unbounded failure reason out of the error log by default
'expired' and 'mismatch' both require a nonce to already be stored for the user, which only happens after a successful password check, so their volume is bounded by real login activity. 'no_nonce_stored' is not bounded. Any unauthenticated request carrying a guessed user ID reaches it, so logging it by default would hand anyone an unthrottled write to the error log. It is also the least informative of the three, firing for every stale bookmark and resubmitted form. The action still fires for all three, so anything routing these into an audit log or intrusion detection system is unaffected, and sites that want 'no_nonce_stored' in the error log can opt in through the filter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent fdbfdb9 commit a766c19

2 files changed

Lines changed: 59 additions & 12 deletions

File tree

‎class-two-factor-core.php‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1432,20 +1432,32 @@ protected static function log_login_nonce_failure( $user_id, $reason ) {
14321432
*/
14331433
do_action( 'two_factor_login_nonce_failed', $user_id, $reason );
14341434

1435+
/*
1436+
* Only 'expired' and 'mismatch' are written to the error log. Both require a nonce
1437+
* to already be stored for the user, which only happens after a successful password
1438+
* check, so their volume is bounded by real login activity.
1439+
*
1440+
* 'no_nonce_stored' is not. Any unauthenticated request carrying a guessed user ID
1441+
* reaches it, so logging it by default would hand anyone an unbounded write to the
1442+
* error log -- and it is the least informative of the three, firing for every stale
1443+
* bookmark and resubmitted form. Sites that want it can opt in via the filter below.
1444+
*/
1445+
$log_by_default = in_array( $reason, array( 'expired', 'mismatch' ), true );
1446+
14351447
/**
1436-
* Filters whether failed login nonce verifications are written to the PHP error log.
1448+
* Filters whether a failed login nonce verification is written to the PHP error log.
14371449
*
1438-
* These are written unconditionally so that the failures are visible without any
1439-
* configuration. Sites that would rather not carry the log volume, or that handle
1440-
* the `two_factor_login_nonce_failed` action themselves, can return false here.
1450+
* Defaults to true for 'expired' and 'mismatch', and false for 'no_nonce_stored'.
1451+
* Sites that would rather not carry the log volume, or that handle the
1452+
* `two_factor_login_nonce_failed` action themselves, can return false for everything.
14411453
*
14421454
* @since 0.17.0
14431455
*
1444-
* @param bool $log Whether to write the failure to the error log. Default true.
1456+
* @param bool $log Whether to write this failure to the error log.
14451457
* @param int $user_id The user ID the nonce was presented for.
14461458
* @param string $reason One of 'no_nonce_stored', 'expired', or 'mismatch'.
14471459
*/
1448-
if ( ! apply_filters( 'two_factor_log_login_nonce_failures', true, $user_id, $reason ) ) {
1460+
if ( ! apply_filters( 'two_factor_log_login_nonce_failures', $log_by_default, $user_id, $reason ) ) {
14491461
return;
14501462
}
14511463

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

Lines changed: 41 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -779,14 +779,36 @@ public function data_login_nonce_failure_reasons() {
779779
/**
780780
* Error log output for failed nonces can be turned off, and is passed the context.
781781
*
782+
* The incoming value differs by reason: 'expired' and 'mismatch' both require a stored
783+
* nonce, so they are bounded by real logins and default to true. 'no_nonce_stored' is
784+
* reachable by any unauthenticated request and defaults to false.
785+
*
786+
* @dataProvider data_login_nonce_failure_log_defaults
787+
*
782788
* @covers Two_Factor_Core::log_login_nonce_failure()
789+
*
790+
* @param string $reason The expected failure reason.
791+
* @param bool $expected_default The expected incoming filter value.
783792
*/
784-
public function test_login_nonce_failure_logging_can_be_filtered() {
793+
public function test_login_nonce_failure_logging_can_be_filtered( $reason, $expected_default ) {
785794
$user_id = self::factory()->user->create();
786795
$args = array();
796+
$nonce = 'not-a-real-nonce';
797+
798+
if ( 'no_nonce_stored' !== $reason ) {
799+
$created = Two_Factor_Core::create_login_nonce( $user_id );
800+
801+
if ( 'expired' === $reason ) {
802+
$nonce = $created['key'];
803+
804+
$stored = get_user_meta( $user_id, Two_Factor_Core::USER_META_NONCE_KEY, true );
805+
$stored['expiration'] = time() - 1;
806+
update_user_meta( $user_id, Two_Factor_Core::USER_META_NONCE_KEY, $stored );
807+
}
808+
}
787809

788-
$recorder = function ( $log, $logged_user_id, $reason ) use ( &$args ) {
789-
$args[] = array( $log, $logged_user_id, $reason );
810+
$recorder = function ( $log, $logged_user_id, $logged_reason ) use ( &$args ) {
811+
$args[] = array( $log, $logged_user_id, $logged_reason );
790812

791813
return false;
792814
};
@@ -795,15 +817,28 @@ public function test_login_nonce_failure_logging_can_be_filtered() {
795817
remove_filter( 'two_factor_log_login_nonce_failures', '__return_false' );
796818
add_filter( 'two_factor_log_login_nonce_failures', $recorder, 10, 3 );
797819

798-
$this->assertFalse( Two_Factor_Core::verify_login_nonce( $user_id, 'not-a-real-nonce' ) );
820+
$this->assertFalse( Two_Factor_Core::verify_login_nonce( $user_id, $nonce ) );
799821

800822
remove_filter( 'two_factor_log_login_nonce_failures', $recorder, 10 );
801823
add_filter( 'two_factor_log_login_nonce_failures', '__return_false' );
802824

803825
$this->assertSame(
804-
array( array( true, $user_id, 'no_nonce_stored' ) ),
826+
array( array( $expected_default, $user_id, $reason ) ),
805827
$args,
806-
'The filter receives the incoming value plus the user and reason'
828+
'The filter receives the per-reason default plus the user and reason'
829+
);
830+
}
831+
832+
/**
833+
* Data provider for the per-reason error log defaults.
834+
*
835+
* @return array
836+
*/
837+
public function data_login_nonce_failure_log_defaults() {
838+
return array(
839+
'nothing stored' => array( 'no_nonce_stored', false ),
840+
'wrong value' => array( 'mismatch', true ),
841+
'past expiration' => array( 'expired', true ),
807842
);
808843
}
809844

0 commit comments

Comments
 (0)