Skip to content

#831 Fix nonce verification in 2FA flow #833

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
PANawkar wants to merge 3 commits into
base: master
Choose a base branch
from
+68 −24
Open

#831 Fix nonce verification in 2FA flow #833

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09bb677
Fix nonce verification in 2FA flow
Mar 12, 2026
498eb6d
Harden nonce/request handling, fix phpcs comments, add reset notice n…
Mar 12, 2026
6a1ac8e
use !empty() for rememberme check and simplify rememberme()
Mar 13, 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
40 changes: 23 additions & 17 deletions class-two-factor-core.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ public static function get_user_two_factor_revalidate_url( $interim = false ) {
* @return boolean
*/
public static function is_valid_user_action( $user_id, $action ) {
$request_nonce = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) : '';
$request_nonce = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) : ''; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Value only passed to wp_verify_nonce().

Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

$request_nonce comes from $_REQUEST and may be an array (e.g. ...?arg[]=x), which can trigger notices or unexpected behavior when passed into wp_verify_nonce(). Consider guarding with is_scalar() / casting to string and sanitizing (similar to the _wpnonce handling elsewhere) before verifying.

Suggested change
$request_nonce = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) : ''; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Value only passed to wp_verify_nonce().
$request_nonce_raw = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_NONCE_QUERY_ARG ] ) : ''; // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Value sanitized and then only passed to wp_verify_nonce().
if ( ! is_scalar( $request_nonce_raw ) ) {
$request_nonce = '';
} else {
$request_nonce = sanitize_text_field( (string) $request_nonce_raw );
}

Copilot uses AI. Check for mistakes.
if ( ! $user_id || ! $action || ! $request_nonce ) {
return false;
Expand All @@ -473,8 +473,8 @@ public static function is_valid_user_action( $user_id, $action ) {
*/
public static function current_user_being_edited() {
// Try to resolve the user ID from the request first.
if ( ! empty( $_REQUEST['user_id'] ) ) {
$user_id = intval( $_REQUEST['user_id'] );
if ( ! empty( $_REQUEST['user_id'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in trigger_user_settings_action() via is_valid_user_action() before any state change.
$user_id = intval( $_REQUEST['user_id'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in trigger_user_settings_action() via is_valid_user_action() before any state change.

if ( current_user_can( 'edit_user', $user_id ) ) {
return $user_id;
Expand All @@ -493,7 +493,7 @@ public static function current_user_being_edited() {
* @return void
*/
public static function trigger_user_settings_action() {
$action = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_QUERY_VAR ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_QUERY_VAR ] ) : '';
$action = isset( $_REQUEST[ self::USER_SETTINGS_ACTION_QUERY_VAR ] ) ? wp_unslash( $_REQUEST[ self::USER_SETTINGS_ACTION_QUERY_VAR ] ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Nonce verified in is_valid_user_action() before do_action.
$user_id = self::current_user_being_edited();

if ( self::is_valid_user_action( $user_id, $action ) ) {
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

$action is pulled from $_REQUEST and used in sprintf('%d-%s', ...) and later passed to do_action() if the nonce validates; if $_REQUEST[...] is an array this can produce PHP notices (“Array to string conversion”). Recommend ensuring $action is scalar and sanitizing it (e.g. sanitize_key() / sanitize_text_field()) before using it in the nonce action string and hook arguments.

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -906,7 +906,7 @@ public static function show_two_factor_login( $user ) {
wp_die( esc_html__( 'Failed to create a login nonce.', 'two-factor' ) );
}

$redirect_to = isset( $_REQUEST['redirect_to'] ) ? $_REQUEST['redirect_to'] : admin_url();
$redirect_to = isset( $_REQUEST['redirect_to'] ) ? esc_url_raw( wp_unslash( $_REQUEST['redirect_to'] ) ) : admin_url(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Value only used for redirect; auth protected by 2FA login nonce later.

Comment thread
PANawkar marked this conversation as resolved.
self::login_html( $user, $login_nonce['key'], $redirect_to );
}
Expand Down Expand Up @@ -960,6 +960,12 @@ public static function maybe_show_reset_password_notice( $errors ) {
return $errors;
}

// Verify login form nonce when present (e.g. wp-login.php); skip only when nonce is not sent (custom login forms).
if ( isset( $_POST['_wpnonce'] ) && ! wp_verify_nonce( sanitize_text_field( wp_unslash( $_POST['_wpnonce'] ) ), 'log-in' ) ) {
return $errors;
}
Comment thread
PANawkar marked this conversation as resolved.

// phpcs:ignore WordPress.Security.NonceVerification.Missing -- Nonce verified above when _wpnonce present; absent for custom login forms.
$user_name = sanitize_user( wp_unslash( $_POST['log'] ) );
$attempted_user = get_user_by( 'login', $user_name );
if ( ! $attempted_user && str_contains( $user_name, '@' ) ) {
Expand Down Expand Up @@ -1487,11 +1493,11 @@ public static function rest_api_can_edit_user_and_update_two_factor_options( $us
* @since 0.2.0
*/
public static function login_form_validate_2fa() {
$wp_auth_id = ! empty( $_REQUEST['wp-auth-id'] ) ? absint( $_REQUEST['wp-auth-id'] ) : 0;
$nonce = ! empty( $_REQUEST['wp-auth-nonce'] ) ? wp_unslash( $_REQUEST['wp-auth-nonce'] ) : '';
$provider = ! empty( $_REQUEST['provider'] ) ? wp_unslash( $_REQUEST['provider'] ) : '';
$redirect_to = ! empty( $_REQUEST['redirect_to'] ) ? wp_unslash( $_REQUEST['redirect_to'] ) : '';
$is_post_request = ( 'POST' === strtoupper( $_SERVER['REQUEST_METHOD'] ) );
$wp_auth_id = ! empty( $_REQUEST['wp-auth-id'] ) ? absint( $_REQUEST['wp-auth-id'] ) : 0; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_validate_2fa() via verify_login_nonce() before any use.
Comment thread
PANawkar marked this conversation as resolved.
$nonce = ! empty( $_REQUEST['wp-auth-nonce'] ) ? wp_unslash( $_REQUEST['wp-auth-nonce'] ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Nonce verified in _login_form_validate_2fa() before any use.
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

$_REQUEST['wp-auth-nonce'] can be an array; passing that through to verify_login_nonce() causes JSON encoding and hashing of attacker-controlled data, which can generate notices and do unnecessary work. Consider normalizing it to a string early (e.g. require scalar + sanitize_text_field( wp_unslash(...) )) and treating non-scalar values as empty/invalid.

Suggested change
$nonce = ! empty( $_REQUEST['wp-auth-nonce'] ) ? wp_unslash( $_REQUEST['wp-auth-nonce'] ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Nonce verified in _login_form_validate_2fa() before any use.
$nonce = ( isset( $_REQUEST['wp-auth-nonce'] ) && is_scalar( $_REQUEST['wp-auth-nonce'] ) ) ? sanitize_text_field( wp_unslash( (string) $_REQUEST['wp-auth-nonce'] ) ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_validate_2fa() before any use.

Copilot uses AI. Check for mistakes.
$provider = ! empty( $_REQUEST['provider'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['provider'] ) ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_validate_2fa() before any use.
$redirect_to = ! empty( $_REQUEST['redirect_to'] ) ? esc_url_raw( wp_unslash( $_REQUEST['redirect_to'] ) ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_validate_2fa() before any use.
Comment thread
PANawkar marked this conversation as resolved.
$is_post_request = isset( $_SERVER['REQUEST_METHOD'] ) && 'POST' === strtoupper( $_SERVER['REQUEST_METHOD'] ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- REQUEST_METHOD is not user input.
$user = get_user_by( 'id', $wp_auth_id );

if ( ! $wp_auth_id || ! $nonce || ! $user ) {
Expand Down Expand Up @@ -1553,7 +1559,7 @@ public static function _login_form_validate_2fa( $user, $nonce = '', $provider =
delete_user_meta( $user->ID, self::USER_FAILED_LOGIN_ATTEMPTS_KEY );

$rememberme = false;
if ( isset( $_REQUEST['rememberme'] ) && $_REQUEST['rememberme'] ) {
if ( isset( $_REQUEST['rememberme'] ) && $_REQUEST['rememberme'] ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Request read only after successful verify_login_nonce() in this request.
Copy link
Copy Markdown
Contributor

@sjinks sjinks Mar 13, 2026

Choose a reason for hiding this comment

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

Suggested change
if ( isset( $_REQUEST['rememberme'] ) && $_REQUEST['rememberme'] ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Request read only after successful verify_login_nonce() in this request.
if ( ! empty( $_REQUEST['rememberme'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Request read only after successful verify_login_nonce() in this request.

$rememberme = true;
}

Expand Down Expand Up @@ -1594,7 +1600,7 @@ public static function _login_form_validate_2fa( $user, $nonce = '', $provider =
$interim_login = isset( $_REQUEST['interim-login'] ); // phpcs:ignore WordPress.WP.GlobalVariablesOverride.Prohibited,WordPress.Security.NonceVerification.Recommended

if ( $interim_login ) {
$customize_login = isset( $_REQUEST['customize-login'] );
$customize_login = isset( $_REQUEST['customize-login'] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Request read only after successful verify_login_nonce() in this request.
if ( $customize_login ) {
wp_enqueue_script( 'customize-base' );
}
Expand Down Expand Up @@ -1627,10 +1633,10 @@ public static function _login_form_validate_2fa( $user, $nonce = '', $provider =
* @since 0.9.0
*/
public static function login_form_revalidate_2fa() {
$nonce = ! empty( $_REQUEST['wp-auth-nonce'] ) ? wp_unslash( $_REQUEST['wp-auth-nonce'] ) : '';
$provider = ! empty( $_REQUEST['provider'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['provider'] ) ) : false;
$redirect_to = ! empty( $_REQUEST['redirect_to'] ) ? wp_unslash( $_REQUEST['redirect_to'] ) : admin_url();
$is_post_request = ( 'POST' === strtoupper( $_SERVER['REQUEST_METHOD'] ) );
$nonce = ! empty( $_REQUEST['wp-auth-nonce'] ) ? wp_unslash( $_REQUEST['wp-auth-nonce'] ) : ''; // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Nonce verified in _login_form_revalidate_2fa() for POST before processing.
$provider = ! empty( $_REQUEST['provider'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['provider'] ) ) : false; // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_revalidate_2fa() for POST before processing.
$redirect_to = ! empty( $_REQUEST['redirect_to'] ) ? esc_url_raw( wp_unslash( $_REQUEST['redirect_to'] ) ) : admin_url(); // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Nonce verified in _login_form_revalidate_2fa() for POST before processing.
Comment thread
PANawkar marked this conversation as resolved.
$is_post_request = isset( $_SERVER['REQUEST_METHOD'] ) && 'POST' === strtoupper( $_SERVER['REQUEST_METHOD'] ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.MissingUnslash, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- REQUEST_METHOD is not user input.

self::_login_form_revalidate_2fa( $nonce, $provider, $redirect_to, $is_post_request );
exit;
Expand Down Expand Up @@ -2386,7 +2392,7 @@ public static function get_current_user_session() {
public static function rememberme() {
$rememberme = false;

if ( ! empty( $_REQUEST['rememberme'] ) ) {
if ( ! empty( $_REQUEST['rememberme'] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Only used after 2FA login nonce verified by caller.
$rememberme = true;
}
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The phpcs ignore reason here says the value is only used after the 2FA login nonce is verified by the caller, but rememberme() is also used while rendering the initial 2FA form (login_html()), before any 2FA nonce verification happens. Please update the ignore rationale to reflect the real trust boundary (e.g., it’s a non-destructive display/flow flag) and/or normalize the value explicitly to a boolean/int.

Copilot uses AI. Check for mistakes.

Expand Down
4 changes: 2 additions & 2 deletions providers/class-two-factor-provider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,11 +171,11 @@ public static function get_code( $length = 8, $chars = '1234567890' ) {
* @return false|string Auth code on success, false if the field is not set or not expected length.
*/
public static function sanitize_code_from_request( $field, $length = 0 ) {
if ( empty( $_REQUEST[ $field ] ) ) {
if ( empty( $_REQUEST[ $field ] ) ) { // phpcs:ignore WordPress.Security.NonceVerification.Recommended -- Caller (core) verifies nonce before provider processing.
return false;
}

$code = wp_unslash( $_REQUEST[ $field ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, handled by the core method already.
$code = wp_unslash( $_REQUEST[ $field ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Caller (core) verifies nonce; value sanitized below.
Copy link

Copilot AI Mar 12, 2026

Choose a reason for hiding this comment

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

The phpcs ignore rationale says the request value is “sanitized below”, but this function only strips whitespace and (optionally) checks length; it still permits arbitrary characters (per existing unit tests). Please either adjust the ignore comment to match the actual behavior (e.g., “normalized”) or strengthen validation/sanitization if the intent is to accept only a restricted code format.

Suggested change
$code = wp_unslash( $_REQUEST[ $field ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Caller (core) verifies nonce; value sanitized below.
$code = wp_unslash( $_REQUEST[ $field ] ); // phpcs:ignore WordPress.Security.NonceVerification.Recommended, WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- Caller (core) verifies nonce; value normalized below.

Copilot uses AI. Check for mistakes.
$code = preg_replace( '/\s+/', '', $code );

// Maybe validate the length.
Expand Down
Loading