fix WordPress.Security.EscapeOutput.OutputNotEscaped errors #776
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. |
| number_format_i18n( $failed_login_count ), | ||
| human_time_diff( $last_failed_two_factor_login, time() ) | ||
| esc_html( number_format_i18n( $failed_login_count ) ), | ||
| esc_html( human_time_diff( $last_failed_two_factor_login, time() ) ) |
There was a problem hiding this comment.
Could we wrap the whole output of sprintf( _n(), a, b ) into esc_html() so that we don't need to wrap individual fields?
The sprintf() placeholders such as %2$s get converted to %2 with esc_html() which is not desired for the replacement to actually work.
Tested with:
wp eval 'esc_html("%2$s");'
string(2) "%2"
There was a problem hiding this comment.
could you doublecheck this again @kasparsd ? i have no issue with changing the structure (feel free to propose it directly on github. but on my side this worked perfectly when i tested it. Just checked it once again, see:
Thanks for your view on this!
| public static function login_html( $user, $login_nonce, $redirect_to, $error_msg = '', $provider = null, $action = 'validate_2fa' ) { | ||
| $provider = self::get_provider_for_user( $user, $provider ); | ||
| if ( ! $provider ) { | ||
| wp_die( __( 'Cheatin’ uh?', 'two-factor' ) ); |
There was a problem hiding this comment.
This is how WP core did it before https://core.trac.wordpress.org/ticket/38332 and related tickets.
Question for outside of this pull request -- should we use more descriptive error names here? Like No two-factor provider enabled for the user ...?
There was a problem hiding this comment.
good point @kasparsd. i think thats easily solvable in this PR as well. chose "Two-factor provider not available for this user." for the moment. -what do you think?
Generally i think we should solve these multiple wp_die into a seperate function - seems quite repetetive. but probably out of scope for this PR
3 participants
Fixes #775
What?
Fixes WordPress Coding Standards (PHPCS) violations related to unescaped output in class-two-factor-core.php by properly escaping all localized and dynamic output.
Why?
The Two-Factor plugin was triggering WordPress.Security.EscapeOutput.OutputNotEscaped errors when running PHPCS. Several localized strings and formatted values (_n(), number_format_i18n(), human_time_diff(), and __()) were output directly without context-appropriate escaping. This violates WordPress security and coding standards and may block releases or CI checks.
How?
implement the correct escape functions
Testing Instructions
Changelog Entry