enhance: global settings for Cloudflare Turnstile #1495
Conversation
WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (4)
templates/login-form.php (1)
Line range hint 33-91: Add documentation for CAPTCHA implementation.
Consider adding a documentation block explaining:
- How the CAPTCHA system selection works
- What happens when both CAPTCHAs are enabled
- Requirements for each CAPTCHA system
Add this documentation block:
/**
* CAPTCHA Implementation
*
* The login form supports two CAPTCHA providers:
* 1. Google reCAPTCHA
* 2. Cloudflare Turnstile
*
* Configuration:
* - Both CAPTCHAs can be enabled simultaneously
* - Each CAPTCHA requires its own site key in the WPUF settings
* - reCAPTCHA requires the recaptcha.js script
* - Turnstile requires the turnstile.js script
*/assets/less/admin.less (1)
1172-1223: LGTM! Well-structured toggle switch implementation.
The toggle switch implementation follows CSS best practices with proper positioning, transitions, and state management.
Consider these improvements for better accessibility and maintainability:
.wpuf-toggle-switch {
position: relative;
display: inline-block;
width: 50px;
height: 26px;
+ /* Define reusable custom properties */
+ --switch-bg-checked: #0073aa;
+ --switch-bg-unchecked: #ccc;
+ --switch-knob-color: white;
+ --switch-focus-color: #2196F3;
input {
display: none;
&:checked + .slider {
- background-color: #0073aa;
+ background-color: var(--switch-bg-checked);
}
&:focus + .slider {
- box-shadow: 0 0 1px #2196F3;
+ box-shadow: 0 0 3px var(--switch-focus-color);
}
+ &:disabled + .slider {
+ opacity: 0.5;
+ cursor: not-allowed;
+ }
&:checked + .slider:before {
transform: translateX(26px);
}
}
.slider {
position: absolute;
cursor: pointer;
top: 0;
left: 0;
right: 0;
bottom: 0;
- background-color: #ccc;
+ background-color: var(--switch-bg-unchecked);
transition: .2s;
+ &:hover {
+ opacity: 0.8;
+ }
&.round {
border-radius: 34px;
}
&.round:before {
border-radius: 50%;
}
&::before {
position: absolute;
content: "";
height: 18px;
width: 18px;
left: 3px;
bottom: 4px;
- background-color: white;
+ background-color: var(--switch-knob-color);
transition: .2s;
}
}
}These changes:
- Add CSS custom properties for better maintainability
- Improve focus visibility for accessibility
- Add disabled state styles
- Add hover effect for better interactivity
- Use semantic color variable names
assets/css/admin.css (1)
947-990: LGTM with suggestions for the toggle switch implementation.
The toggle switch implementation is well-structured and follows best practices. However, consider these improvements:
- Use CSS custom properties for better maintainability:
.wpuf-toggle-switch input:checked + .slider {
- background-color: #0073aa;
+ background-color: var(--wpuf-toggle-active-color, #0073aa);
}- Consider adding RTL support:
.wpuf-toggle-switch .slider::before {
left: 3px;
+ [dir="rtl"] & {
+ left: auto;
+ right: 3px;
+ }
}assets/css/frontend-forms.css (1)
Line range hint 1-1441: Consider architectural improvements for better maintainability.
While the current changes are good, here are some suggestions for future improvements:
- Consider using CSS variables for colors and other repeated values to improve maintainability:
:root {
--wpuf-primary: #00a0d2;
--wpuf-error-bg: #f2dede;
--wpuf-error-border: #ebccd1;
/* etc */
}-
Consider consolidating media queries to reduce code duplication and improve maintainability.
-
Consider reducing specificity by removing unnecessary
bodynesting where possible.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- Lib/WeDevs_Settings_API.php (4 hunks)
- assets/css/admin.css (1 hunks)
- assets/css/admin/wpuf-module.css (0 hunks)
- assets/css/frontend-forms.css (1 hunks)
- assets/less/admin.less (1 hunks)
- assets/less/frontend-forms.less (1 hunks)
- includes/Admin/Plugin_Upgrade_Notice.php (1 hunks)
- includes/Assets.php (2 hunks)
- includes/Free/Simple_Login.php (5 hunks)
- includes/functions/settings-options.php (2 hunks)
- templates/login-form.php (2 hunks)
💤 Files with no reviewable changes (1)
- assets/css/admin/wpuf-module.css
✅ Files skipped from review due to trivial changes (1)
- includes/Admin/Plugin_Upgrade_Notice.php
🔇 Additional comments (11)
templates/login-form.php (2)
33-38: LGTM! Secure configuration retrieval implementation.
The configuration setup properly retrieves CAPTCHA settings with secure defaults.
76-90: Verify Turnstile script registration and verification logic.
Ensure that the required dependencies are properly implemented:
- Turnstile script registration in Assets.php
- Turnstile verification logic in Simple_Login.php
✅ Verification successful
Turnstile integration is properly implemented
The verification confirms that all required dependencies are correctly implemented:
- Assets.php: Properly registers the Turnstile script from Cloudflare's CDN with the correct callback
- Simple_Login.php: Contains comprehensive implementation including:
- Script enqueuing when Turnstile is enabled
- Complete verification logic with proper security measures (nonce verification, sanitization)
- Error handling for failed verifications
- Integration with the login form template
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Turnstile script is properly registered
echo "Checking Assets.php for Turnstile script registration..."
rg -A 5 "turnstile" "includes/Assets.php"
# Check if verification logic is implemented
echo "Checking Simple_Login.php for Turnstile verification..."
rg -A 10 "turnstile" "includes/Free/Simple_Login.php"
Length of output: 2999
includes/Assets.php (1)
377-379: 🛠️ Refactor suggestion
Consider improving the Turnstile script integration.
While the Cloudflare Turnstile script integration aligns with the PR objectives, consider the following improvements:
- Add version parameter for cache control
- Add integrity hash for security
- Ensure the
onloadTurnstileCallbackfunction is properly defined - Consider adding error handling for script loading failures
Let's verify the callback function definition:
Consider updating the script registration:
'turnstile' => [
- 'src' => 'https://challenges.cloudflare.com/turnstile/v0/api.js?onload=onloadTurnstileCallback',
+ 'src' => 'https://challenges.cloudflare.com/turnstile/v0/api.js?onload=onloadTurnstileCallback',
+ 'version' => WPUF_VERSION,
+ 'deps' => ['jquery'],
+ 'in_footer' => true,
],includes/functions/settings-options.php (2)
162-184: LGTM! Well-structured Turnstile configuration.
The implementation follows WordPress settings API best practices:
- Toggle field for enabling/disabling the feature
- Dependent fields for API keys
- Clear documentation link to Cloudflare Turnstile
457-467: LGTM! Well-implemented login form integration.
The login form Turnstile toggle is properly:
- Dependent on the global enable_turnstile setting
- Includes clear description of requirements
- Uses consistent naming convention
assets/less/frontend-forms.less (1)
119-119: LGTM: Improved error message alignment.
The margin adjustment for .wpuf-error improves alignment with form boundaries while maintaining consistent vertical spacing.
assets/css/frontend-forms.css (1)
87-87: LGTM: Error message margin adjustment improves alignment.
The change to remove horizontal margins for error messages allows them to span the full width of their container, providing better visual alignment with other form elements.
Lib/WeDevs_Settings_API.php (1)
143-143: LGTM: 'depends_on' parameter added for conditional fields
The addition of the 'depends_on' parameter enhances the settings fields by enabling conditional display based on dependencies. This improves the flexibility of the settings API.
includes/Free/Simple_Login.php (3)
22-28: New property $cf_messages added for Turnstile error handling
The addition of the $cf_messages property is appropriate for storing error codes from Cloudflare Turnstile verification.
349-349: Ensure consistent return type in login_form method
Changing return; to return ''; ensures that the login_form method consistently returns a string, as specified in its documentation.
404-408: Verify that the wpuf-turnstile script is registered properly
When the Turnstile feature is enabled, the script 'wpuf-turnstile' is enqueued. Please ensure that this script is registered correctly in the Assets class to prevent any issues during script loading.
| <?php if ( $turnstile === 'on' ) { ?> | ||
| <div | ||
| id="wpuf-turnstile" | ||
| class="wpuf-turnstile" | ||
| data-sitekey="<?php echo esc_attr( $turnstile_site_key ); ?>" | ||
| data-callback="javascriptCallback" | ||
| ></div> | ||
| <script> | ||
| window.onloadTurnstileCallback = function () { | ||
| turnstile.render("#wpuf-turnstile", { | ||
| sitekey: "<?php echo esc_js( $turnstile_site_key ); ?>", | ||
| }); | ||
| }; | ||
| </script> | ||
| <?php } ?> |
There was a problem hiding this comment.
Security and implementation improvements needed for Turnstile integration.
Several issues need to be addressed:
- Inline JavaScript violates Content Security Policy (CSP) best practices
- Missing error handling for empty site key
- The callback function 'javascriptCallback' is defined but not implemented
- The script might fail if the turnstile object is not loaded
Here's the suggested implementation:
<?php if ( $turnstile === 'on' ) { ?>
+ <?php if ( empty( $turnstile_site_key ) ) { ?>
+ <div class="wpuf-error">
+ <?php esc_html_e( 'Turnstile site key is not configured.', 'wp-user-frontend' ); ?>
+ </div>
+ <?php } else { ?>
<div
id="wpuf-turnstile"
class="wpuf-turnstile"
data-sitekey="<?php echo esc_attr( $turnstile_site_key ); ?>"
- data-callback="javascriptCallback"
></div>
- <script>
- window.onloadTurnstileCallback = function () {
- turnstile.render("#wpuf-turnstile", {
- sitekey: "<?php echo esc_js( $turnstile_site_key ); ?>",
- });
- };
- </script>
+ <?php } ?>
<?php } ?>Then, move the JavaScript to your main script file:
// In your main JavaScript file
document.addEventListener('DOMContentLoaded', function() {
const turnstileContainer = document.getElementById('wpuf-turnstile');
if (turnstileContainer && typeof turnstile !== 'undefined') {
const sitekey = turnstileContainer.getAttribute('data-sitekey');
if (sitekey) {
turnstile.ready(() => {
turnstile.render('#wpuf-turnstile', {
sitekey: sitekey,
callback: function(token) {
// Handle the token as needed
console.log('Turnstile token:', token);
}
});
});
}
}
});| $depends_on.on('keyup change', function() { | ||
| if ($(this).val() === $this.data('depends-on-value')) { | ||
| $this.closest('tr').show(); | ||
| } else { | ||
| $this.closest('tr').hide(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Define and use 'data-depends-on-value' for accurate dependency checking
The code references $this.data('depends-on-value'), but this attribute is not set in the HTML markup. To ensure the dependency check functions correctly, please define the data-depends-on-value attribute when rendering the input fields.
Suggested addition in the HTML input generation (e.g., in the callback_text method):
$html = sprintf(
'<input type="%1$s" class="%2$s-text" id="%3$s[%4$s]" name="%3$s[%4$s]" value="%5$s"%6$s %7$s data-depends-on="%8$s" data-depends-on-value="%9$s" />',
- $type, $size, $args['section'], $args['id'], $value, $placeholder, $disabled, $depends_on
+ esc_attr( $type ), esc_attr( $size ), esc_attr( $args['section'] ), esc_attr( $args['id'] ), esc_attr( $value ), $placeholder, $disabled, esc_attr( $depends_on ), esc_attr( $args['depends_on_value'] )
);Ensure that $args['depends_on_value'] is set appropriately in the settings fields array.
Committable suggestion was skipped due to low confidence.
| var fields = $('table.form-table input, table.form-table select, table.form-table textarea'); | ||
|
|
||
| // iterate over each field and check if it depends on another field | ||
| fields.each(function() { | ||
| var $this = $(this); | ||
| var data_depends_on = $this.data('depends-on'); | ||
|
|
||
| if (!data_depends_on) { | ||
| return; | ||
| } | ||
|
|
||
| var $depends_on = $("input[id*='"+ data_depends_on +"']"); | ||
| var $depends_on_type = $depends_on.attr('type'); | ||
|
|
||
| if ($depends_on_type === 'checkbox') { | ||
| if ($depends_on.is(':checked')) { | ||
| $this.closest('tr').show(); | ||
| } else { | ||
| $this.closest('tr').hide(); | ||
| } | ||
| $depends_on.on('change', function() { | ||
| if ($(this).is(':checked')) { | ||
| $this.closest('tr').show(); | ||
| } else { | ||
| $this.closest('tr').hide(); | ||
| } | ||
| }); | ||
| } else { | ||
| $depends_on.on('keyup change', function() { | ||
| if ($(this).val() === $this.data('depends-on-value')) { | ||
| $this.closest('tr').show(); | ||
| } else { | ||
| $this.closest('tr').hide(); | ||
| } | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Use specific selectors to prevent unintended matches in dependency logic
In the JavaScript code handling field dependencies, the selector $("input[id*='"+ data_depends_on +"']") may unintentionally match multiple elements if their IDs contain the data_depends_on string. To ensure the correct element is selected, use a more specific selector.
Apply this change to the selector:
- var $depends_on = $("input[id*='"+ data_depends_on +"']");
+ var $depends_on = $("#wpuf-" + data_depends_on);This change assumes that the dependent field's ID follows the format wpuf- plus the depends_on value, which aligns with how IDs are generated in the settings fields.
Committable suggestion was skipped due to low confidence.
| $depends_on = ! empty( $args['depends_on'] ) ? $args['depends_on'] : ''; | ||
|
|
||
| $html = sprintf( '<input type="%1$s" class="%2$s-text" id="%3$s[%4$s]" name="%3$s[%4$s]" value="%5$s"%6$s %7$s/>', $type, $size, $args['section'], $args['id'], $value, $placeholder, $disabled ); | ||
| $html = sprintf( | ||
| '<input type="%1$s" class="%2$s-text" id="%3$s[%4$s]" name="%3$s[%4$s]" value="%5$s"%6$s %7$s data-depends-on="%8$s" />', | ||
| $type, $size, $args['section'], $args['id'], $value, $placeholder, $disabled, $depends_on | ||
| ); |
There was a problem hiding this comment.
Escape the 'depends_on' attribute in the input field
The $depends_on variable is output directly into the data-depends-on attribute without sanitization. To prevent potential security vulnerabilities such as XSS attacks, please use esc_attr() when outputting $depends_on.
Apply this diff to fix the issue:
$depends_on = ! empty( $args['depends_on'] ) ? $args['depends_on'] : '';
$html = sprintf(
'<input type="%1$s" class="%2$s-text" id="%3$s[%4$s]" name="%3$s[%4$s]" value="%5$s"%6$s %7$s data-depends-on="%8$s" />',
- $type, $size, $args['section'], $args['id'], $value, $placeholder, $disabled, $depends_on
+ esc_attr( $type ), esc_attr( $size ), esc_attr( $args['section'] ), esc_attr( $args['id'] ), esc_attr( $value ), $placeholder, $disabled, esc_attr( $depends_on )
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $depends_on = ! empty( $args['depends_on'] ) ? $args['depends_on'] : ''; | |
| $html = sprintf( '<input type="%1$s" class="%2$s-text" id="%3$s[%4$s]" name="%3$s[%4$s]" value="%5$s"%6$s %7$s/>', $type, $size, $args['section'], $args['id'], $value, $placeholder, $disabled ); | |
| $html = sprintf( | |
| '<input type="%1$s" class="%2$s-text" id="%3$s[%4$s]" name="%3$s[%4$s]" value="%5$s"%6$s %7$s data-depends-on="%8$s" />', | |
| $type, $size, $args['section'], $args['id'], $value, $placeholder, $disabled, $depends_on | |
| ); | |
| $depends_on = ! empty( $args['depends_on'] ) ? $args['depends_on'] : ''; | |
| $html = sprintf( | |
| '<input type="%1$s" class="%2$s-text" id="%3$s[%4$s]" name="%3$s[%4$s]" value="%5$s"%6$s %7$s data-depends-on="%8$s" />', | |
| esc_attr( $type ), esc_attr( $size ), esc_attr( $args['section'] ), esc_attr( $args['id'] ), esc_attr( $value ), $placeholder, $disabled, esc_attr( $depends_on ) | |
| ); |
| public function callback_toggle( $args ) { | ||
| $value = esc_attr( $this->get_option( $args['id'], $args['section'], $args['std'] ) ); | ||
| $disabled = ! empty( $args['is_pro_preview'] ) && $args['is_pro_preview'] ? 'disabled' : ''; | ||
| $name = $args['section'] . '[' . $args['id'] . ']'; | ||
| ?> | ||
| <fieldset> | ||
| <label for="<?php echo 'wpuf-' . $name; ?>" class="wpuf-toggle-switch"> | ||
| <input | ||
| type="hidden" | ||
| name="<?php echo $name; ?>" | ||
| value="off" /> | ||
| <input | ||
| type="checkbox" | ||
| <?php echo $value === 'on' ? 'checked' : ''; ?> | ||
| <?php echo $disabled ? 'disabled' : ''; ?> | ||
| id="<?php echo 'wpuf-' . $name; ?>" | ||
| name="<?php echo $name; ?>" | ||
| class="wpuf-toggle-module checkbox" | ||
| value="on"> | ||
| <span class="slider round"></span> | ||
| </label> | ||
| </fieldset> | ||
|
|
||
| <?php echo $args['desc']; ?> | ||
| <?php | ||
| } |
There was a problem hiding this comment.
Sanitize output in 'callback_toggle' method to prevent XSS vulnerabilities
In the callback_toggle method, variables like $name are output directly into HTML attributes without escaping. To enhance security and prevent XSS attacks, consider using esc_attr() when echoing these variables.
Apply this diff to fix the issue:
$name = $args['section'] . '[' . $args['id'] . ']';
?>
<fieldset>
<label for="<?php echo 'wpuf-' . $name; ?>" class="wpuf-toggle-switch">
<input
type="hidden"
- name="<?php echo $name; ?>"
+ name="<?php echo esc_attr( $name ); ?>"
value="off" />
<input
type="checkbox"
<?php echo $value === 'on' ? 'checked' : ''; ?>
<?php echo $disabled ? 'disabled' : ''; ?>
- id="<?php echo 'wpuf-' . $name; ?>"
- name="<?php echo $name; ?>"
+ id="<?php echo esc_attr( 'wpuf-' . $name ); ?>"
+ name="<?php echo esc_attr( $name ); ?>"
class="wpuf-toggle-module checkbox"
value="on">
<span class="slider round"></span>
</label>
</fieldset>
<?php echo $args['desc']; ?>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function callback_toggle( $args ) { | |
| $value = esc_attr( $this->get_option( $args['id'], $args['section'], $args['std'] ) ); | |
| $disabled = ! empty( $args['is_pro_preview'] ) && $args['is_pro_preview'] ? 'disabled' : ''; | |
| $name = $args['section'] . '[' . $args['id'] . ']'; | |
| ?> | |
| <fieldset> | |
| <label for="<?php echo 'wpuf-' . $name; ?>" class="wpuf-toggle-switch"> | |
| <input | |
| type="hidden" | |
| name="<?php echo $name; ?>" | |
| value="off" /> | |
| <input | |
| type="checkbox" | |
| <?php echo $value === 'on' ? 'checked' : ''; ?> | |
| <?php echo $disabled ? 'disabled' : ''; ?> | |
| id="<?php echo 'wpuf-' . $name; ?>" | |
| name="<?php echo $name; ?>" | |
| class="wpuf-toggle-module checkbox" | |
| value="on"> | |
| <span class="slider round"></span> | |
| </label> | |
| </fieldset> | |
| <?php echo $args['desc']; ?> | |
| <?php | |
| } | |
| public function callback_toggle( $args ) { | |
| $value = esc_attr( $this->get_option( $args['id'], $args['section'], $args['std'] ) ); | |
| $disabled = ! empty( $args['is_pro_preview'] ) && $args['is_pro_preview'] ? 'disabled' : ''; | |
| $name = $args['section'] . '[' . $args['id'] . ']'; | |
| ?> | |
| <fieldset> | |
| <label for="<?php echo 'wpuf-' . $name; ?>" class="wpuf-toggle-switch"> | |
| <input | |
| type="hidden" | |
| name="<?php echo esc_attr( $name ); ?>" | |
| value="off" /> | |
| <input | |
| type="checkbox" | |
| <?php echo $value === 'on' ? 'checked' : ''; ?> | |
| <?php echo $disabled ? 'disabled' : ''; ?> | |
| id="<?php echo esc_attr( 'wpuf-' . $name ); ?>" | |
| name="<?php echo esc_attr( $name ); ?>" | |
| class="wpuf-toggle-module checkbox" | |
| value="on"> | |
| <span class="slider round"></span> | |
| </label> | |
| </fieldset> | |
| <?php echo $args['desc']; ?> | |
| <?php | |
| } |
| $remote_addr = ! empty( $_SERVER['REMOTE_ADDR'] ) ? sanitize_url( | ||
| wp_unslash( $_SERVER['REMOTE_ADDR'] ) | ||
| ) : ''; |
There was a problem hiding this comment.
Use appropriate sanitization for IP address
$_SERVER['REMOTE_ADDR'] contains the user's IP address and should be sanitized using sanitize_text_field instead of sanitize_url.
Apply this diff to correct the sanitization:
- $remote_addr = ! empty( $_SERVER['REMOTE_ADDR'] ) ? sanitize_url(
+ $remote_addr = ! empty( $_SERVER['REMOTE_ADDR'] ) ? sanitize_text_field(
wp_unslash( $_SERVER['REMOTE_ADDR'] )
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $remote_addr = ! empty( $_SERVER['REMOTE_ADDR'] ) ? sanitize_url( | |
| wp_unslash( $_SERVER['REMOTE_ADDR'] ) | |
| ) : ''; | |
| $remote_addr = ! empty( $_SERVER['REMOTE_ADDR'] ) ? sanitize_text_field( | |
| wp_unslash( $_SERVER['REMOTE_ADDR'] ) | |
| ) : ''; |
| $log = isset( $_POST['log'] ) ? sanitize_text_field( wp_unslash( $_POST['log'] ) ) : ''; | ||
| $pwd = isset( $_POST['pwd'] ) ? sanitize_text_field( ( wp_unslash( $_POST['pwd'] ) ) ) : ''; |
There was a problem hiding this comment.
Avoid sanitizing passwords with sanitize_text_field
Using sanitize_text_field on the password may alter it by stripping out characters, leading to authentication failures. Instead, only unslash the password without sanitization.
Apply this diff to correct the handling of the password:
$log = isset( $_POST['log'] ) ? sanitize_text_field( wp_unslash( $_POST['log'] ) ) : '';
- $pwd = isset( $_POST['pwd'] ) ? sanitize_text_field( ( wp_unslash( $_POST['pwd'] ) ) ) : '';
+ $pwd = isset( $_POST['pwd'] ) ? wp_unslash( $_POST['pwd'] ) : '';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $log = isset( $_POST['log'] ) ? sanitize_text_field( wp_unslash( $_POST['log'] ) ) : ''; | |
| $pwd = isset( $_POST['pwd'] ) ? sanitize_text_field( ( wp_unslash( $_POST['pwd'] ) ) ) : ''; | |
| $log = isset( $_POST['log'] ) ? sanitize_text_field( wp_unslash( $_POST['log'] ) ) : ''; | |
| $pwd = isset( $_POST['pwd'] ) ? wp_unslash( $_POST['pwd'] ) : ''; |
| $nonce = isset( $_POST['wpuf-login-nonce'] ) ? sanitize_key( wp_unslash( $_POST['wpuf-login-nonce'] ) ) : ''; | ||
|
|
||
| if ( isset( $nonce ) && ! wp_verify_nonce( $nonce, 'wpuf_login_action' ) ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Correct the nonce validation condition
The condition isset( $nonce ) && ! wp_verify_nonce( $nonce, 'wpuf_login_action' ) will always be true since $nonce is always set (possibly to an empty string). To properly validate the nonce, change isset( $nonce ) to ! empty( $nonce ).
Apply this diff to fix the condition:
- if ( isset( $nonce ) && ! wp_verify_nonce( $nonce, 'wpuf_login_action' ) ) {
+ if ( ! empty( $nonce ) && ! wp_verify_nonce( $nonce, 'wpuf_login_action' ) ) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $nonce = isset( $_POST['wpuf-login-nonce'] ) ? sanitize_key( wp_unslash( $_POST['wpuf-login-nonce'] ) ) : ''; | |
| if ( isset( $nonce ) && ! wp_verify_nonce( $nonce, 'wpuf_login_action' ) ) { | |
| return false; | |
| } | |
| $nonce = isset( $_POST['wpuf-login-nonce'] ) ? sanitize_key( wp_unslash( $_POST['wpuf-login-nonce'] ) ) : ''; | |
| if ( ! empty( $nonce ) && ! wp_verify_nonce( $nonce, 'wpuf_login_action' ) ) { | |
| return false; | |
| } |
| $response = wp_remote_post( $cf_url, [ 'body' => $data ] ); | ||
| $body = json_decode( wp_remote_retrieve_body( $response ), true ); | ||
|
|
There was a problem hiding this comment.
Add error handling for HTTP request to Cloudflare
The code does not check for errors when making the HTTP request to Cloudflare's verification endpoint. If the request fails or returns a WP_Error, this could lead to unexpected behavior.
Apply this diff to add error handling:
$response = wp_remote_post( $cf_url, [ 'body' => $data ] );
+ if ( is_wp_error( $response ) ) {
+ $this->cf_messages[] = $response->get_error_message();
+ return false;
+ }
$body = json_decode( wp_remote_retrieve_body( $response ), true );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $response = wp_remote_post( $cf_url, [ 'body' => $data ] ); | |
| $body = json_decode( wp_remote_retrieve_body( $response ), true ); | |
| $response = wp_remote_post( $cf_url, [ 'body' => $data ] ); | |
| if ( is_wp_error( $response ) ) { | |
| $this->cf_messages[] = $response->get_error_message(); | |
| return false; | |
| } | |
| $body = json_decode( wp_remote_retrieve_body( $response ), true ); | |
| if ( ! $this->verify_cloudflare_turnstile_on_login() ) { | ||
| $errors = ! empty( $this->cf_messages[0] ) ? $this->cf_messages[0] : ''; | ||
| $errors = implode( ', ', $errors ); | ||
| $this->login_errors[] = | ||
| sprintf( | ||
| // translators: %1$s and %2$s are strong tags, %3$s is the error message | ||
| __( '%1$sError%2$s: Cloudflare Turnstile verification failed. Reasons: [%3$s]', 'wp-user-frontend' ), | ||
| '<strong>', | ||
| '</strong>', | ||
| $errors | ||
| ); | ||
| '<strong>' . __( 'Error', 'wp-user-frontend' ) . ':</strong> ' . __( 'Cloudflare Turnstile verification failed. Reasons: [', 'wp-user-frontend' ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix error message construction in Turnstile verification
There are issues in constructing the error message:
$errorsmay not be an array when passed toimplode, causing warnings.- An extra line (line 523) appears redundant and should be removed.
Apply this diff to correct the error handling:
if ( ! $this->verify_cloudflare_turnstile_on_login() ) {
- $errors = ! empty( $this->cf_messages[0] ) ? $this->cf_messages[0] : '';
+ $errors = ! empty( $this->cf_messages ) ? $this->cf_messages : [];
+ $error_message = implode( ', ', $errors );
$this->login_errors[] =
sprintf(
// translators: %1$s and %2$s are strong tags, %3$s is the error message
__( '%1$sError%2$s: Cloudflare Turnstile verification failed. Reasons: [%3$s]', 'wp-user-frontend' ),
'<strong>',
'</strong>',
- $errors
+ $error_message
);
- '<strong>' . __( 'Error', 'wp-user-frontend' ) . ':</strong> ' . __( 'Cloudflare Turnstile verification failed. Reasons: [', 'wp-user-frontend' );
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( ! $this->verify_cloudflare_turnstile_on_login() ) { | |
| $errors = ! empty( $this->cf_messages[0] ) ? $this->cf_messages[0] : ''; | |
| $errors = implode( ', ', $errors ); | |
| $this->login_errors[] = | |
| sprintf( | |
| // translators: %1$s and %2$s are strong tags, %3$s is the error message | |
| __( '%1$sError%2$s: Cloudflare Turnstile verification failed. Reasons: [%3$s]', 'wp-user-frontend' ), | |
| '<strong>', | |
| '</strong>', | |
| $errors | |
| ); | |
| '<strong>' . __( 'Error', 'wp-user-frontend' ) . ':</strong> ' . __( 'Cloudflare Turnstile verification failed. Reasons: [', 'wp-user-frontend' ); | |
| } | |
| if ( ! $this->verify_cloudflare_turnstile_on_login() ) { | |
| $errors = ! empty( $this->cf_messages ) ? $this->cf_messages : []; | |
| $error_message = implode( ', ', $errors ); | |
| $this->login_errors[] = | |
| sprintf( | |
| // translators: %1$s and %2$s are strong tags, %3$s is the error message | |
| __( '%1$sError%2$s: Cloudflare Turnstile verification failed. Reasons: [%3$s]', 'wp-user-frontend' ), | |
| '<strong>', | |
| '</strong>', | |
| $error_message | |
| ); | |
| } |
Rubaiyat-E-Mohammad
added
QA In Progress
QA Approved
fixes #645
Images for reference:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Style
Documentation