Fix: add missing sanitize_text_field() on provider input in login_form_validate_2fa()#899
Conversation
…m_validate_2fa() `login_form_revalidate_2fa()` already applies sanitize_text_field( wp_unslash() ) to the same $_REQUEST['provider'] input at line 1703. The parallel validate handler on line 1563 was missing the sanitize_text_field() wrapper — only wp_unslash() was called. This brings the two handlers in line with each other and with WordPress-VIP-Go coding standards (ValidatedSanitizedInput.InputNotSanitized). The provider value flows into get_provider_for_user() as an array key lookup, so there is no functional exploit path — this is a defensive coding and PHPCS compliance fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Tightens input handling in the 2FA login REST flow by sanitizing the provider value taken from the request.
Changes:
- Sanitize
$_REQUEST['provider']usingsanitize_text_field()afterwp_unslash()inlogin_form_validate_2fa().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $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'] ) : ''; | ||
| $provider = ! empty( $_REQUEST['provider'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['provider'] ) ) : ''; |
There was a problem hiding this comment.
I agree with this, using sanitize_text_field() is inappropriate in this context, this results in PHPCS being "OK" with the code, but it's not more correct, it's arguably more incorrect.
There was a problem hiding this comment.
Yeah, I believe it would need to be applied in way more instances across the plugin if we decided to use this approach.
Most of these inputs are compared and validated against a known set of values and not stored anywhere. Unknown providers make it return early and invalid nonces too.
What
login_form_validate_2fa()reads$_REQUEST['provider']with onlywp_unslash():The parallel
login_form_revalidate_2fa()handler on line 1703 already applies the full WordPress VIP Go pattern:Why
The
providervalue flows intoget_provider_for_user()as an array key lookup, so there is no functional exploit path. This is aValidatedSanitizedInput.InputNotSanitizedPHPCS violation and a defensive coding inconsistency — the two parallel handlers should treat the same$_REQUESTfield identically.Change
One line: add
sanitize_text_field()to the validate handler to match the revalidate handler.Developed with AI assistance (GitHub Copilot + Claude) for code review and pattern matching; the fix and description are my own.