Security: Fix capability checks & shortcode exec vuln #1711
Conversation
WalkthroughAdds capability and nonce checks to admin and preview AJAX flows, enforces post ownership/edit permissions on frontend submissions, and strips shortcodes earlier across many field sanitization and rendering paths; also normalizes form_id handling to use absint and returns JSON/403 or validation errors when unauthorized or invalid. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
includes/Ajax/Admin_Form_Builder_Ajax.php (3)
30-36: Remove duplicate capability check.Lines 30 and 34 contain identical capability checks. This is redundant code duplication.
Apply this diff to remove the duplicate:
if ( ! current_user_can( wpuf_admin_role() ) ) { wp_send_json_error( __( 'Unauthorized operation', 'wp-user-frontend' ) ); } - if ( ! current_user_can( wpuf_admin_role() ) ) { - wp_send_json_error( __( 'Unauthorized operation', 'wp-user-frontend' ) ); - } - if ( empty( $form_data['wpuf_form_id'] ) ) { wp_send_json_error( __( 'Invalid form id', 'wp-user-frontend' ) ); }
94-100: Remove duplicate capability check.Lines 94 and 98 contain identical capability checks. This is redundant code duplication.
Apply this diff to remove the duplicate:
if ( ! current_user_can( wpuf_admin_role() ) ) { wp_send_json_error( __( 'Unauthorized operation', 'wp-user-frontend' ) ); } - if ( ! current_user_can( wpuf_admin_role() ) ) { - wp_send_json_error( __( 'Unauthorized operation', 'wp-user-frontend' ) ); - } - if ( isset( $post_type ) && empty( $post_data['post_type'] ) ) { wp_send_json_error( __( 'Invalid post type', 'wp-user-frontend' ) ); }
145-151: Remove duplicate capability check.Lines 145 and 149 contain identical capability checks. This is redundant code duplication.
Apply this diff to remove the duplicate:
if ( ! current_user_can( wpuf_admin_role() ) ) { wp_send_json_error( __( 'Unauthorized operation', 'wp-user-frontend' ) ); } - if ( ! current_user_can( wpuf_admin_role() ) ) { - wp_send_json_error( __( 'Unauthorized operation', 'wp-user-frontend' ) ); - } - if ( isset( $post_type ) && empty( $post_data['post_type'] ) ) { wp_send_json_error( __( 'Invalid post type', 'wp-user-frontend' ) ); }
🧹 Nitpick comments (1)
includes/Frontend_Render_Form.php (1)
126-126: Use absint() for consistency with the PR's security improvements.For consistency with the security hardening in other files (Form_Preview.php line 49, Admin_Tools.php line 480), use
absint()instead ofintval()for sanitizing the form_id parameter.Apply this diff:
- $form_id = isset( $_GET['form_id'] ) ? intval( wp_unslash( $_GET['form_id'] ) ) : 0; + $form_id = isset( $_GET['form_id'] ) ? absint( wp_unslash( $_GET['form_id'] ) ) : 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
includes/Admin/Admin_Tools.php(1 hunks)includes/Ajax/Admin_Form_Builder_Ajax.php(1 hunks)includes/Frontend/Form_Preview.php(2 hunks)includes/Frontend_Render_Form.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
includes/Frontend_Render_Form.php (1)
wpuf-functions.php (1)
wpuf_admin_role(834-836)
includes/Ajax/Admin_Form_Builder_Ajax.php (1)
wpuf-functions.php (1)
wpuf_admin_role(834-836)
includes/Frontend/Form_Preview.php (1)
wpuf-functions.php (1)
wpuf_admin_role(834-836)
includes/Admin/Admin_Tools.php (1)
wpuf-functions.php (1)
wpuf_admin_role(834-836)
🪛 GitHub Check: Run PHPCS inspection
includes/Frontend/Form_Preview.php
[warning] 49-49:
Processing form data without nonce verification.
[warning] 49-49:
Processing form data without nonce verification.
🔇 Additional comments (5)
includes/Ajax/Admin_Form_Builder_Ajax.php (1)
222-227: Security hardening looks good.The nonce verification and admin capability check properly protect the roles endpoint from unauthorized access. The implementation correctly addresses the information disclosure vulnerability mentioned in the PR objectives.
includes/Frontend_Render_Form.php (1)
121-124: Admin capability check properly secures the preview handler.The capability check correctly restricts form preview to administrators, addressing the security vulnerability described in the PR objectives.
includes/Admin/Admin_Tools.php (1)
458-469: Excellent security hardening for the form import endpoint.The implementation correctly:
- Places the capability check after nonce verification
- Uses proper HTTP status code (403 FORBIDDEN)
- Returns structured WP_Error response
- Addresses the unauthorized form import vulnerability described in the PR objectives
includes/Frontend/Form_Preview.php (2)
43-49: Admin capability check properly secures preview access.The capability check with wp_die() and HTTP 403 status correctly restricts preview functionality to administrators. The use of
absint()for form_id sanitization aligns with the PR's security improvements.However, the static analysis warning about missing nonce verification is worth considering. While the admin capability check prevents unauthorized users from accessing previews, adding nonce verification would provide defense against CSRF attacks where an admin might be tricked into previewing a malicious form ID.
Consider whether preview links require CSRF protection in your threat model. If preview actions have side effects (logging, analytics, etc.), a nonce would be advisable.
87-98: Defense-in-depth validation is well-implemented.The double capability check and form_id validation before shortcode execution properly prevents the arbitrary shortcode execution vulnerability (CVE-TBD) mentioned in the PR objectives. The validated $form_id ensures only safe integer values reach the shortcode.
arifulhoque7
added
needs: dev review
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
includes/Ajax/Frontend_Form_Ajax.php (1)
232-239: Consider using post-type-aware capability check.The current implementation uses
current_user_can('edit_others_posts'), which is a generic capability. For better post-type awareness, consider usingcurrent_user_can('edit_post', $post_id), which automatically handles post-type-specific capabilities likeedit_others_pagesor custom post type capabilities.Apply this diff for a more robust capability check:
// Security: Check if user has permission to edit this post (Broken Access Control fix) $post_author = get_post_field( 'post_author', $post_id ); $current_user_id = get_current_user_id(); - // Allow edit if: user is post author OR user has edit_others_posts capability - if ( $current_user_id !== $post_author && ! current_user_can( 'edit_others_posts' ) ) { + // Allow edit if: user has permission to edit this specific post + if ( ! current_user_can( 'edit_post', $post_id ) ) { wpuf()->ajax->send_error( __( 'You do not have permission to edit this post.', 'wp-user-frontend' ) ); }Note: This simplifies the logic since
current_user_can('edit_post', $post_id)already checks if the user is the post author or has the appropriate capability for that post type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Ajax/Frontend_Form_Ajax.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Ajax/Frontend_Form_Ajax.php (2)
wpuf.php (1)
wpuf(418-420)includes/Ajax.php (1)
send_error(95-102)
Rubaiyat-E-Mohammad
added
QA Approved
| } | ||
|
|
||
| // Security: Validate and sanitize form_id parameter | ||
| $this->form_id = isset( $_GET['form_id'] ) ? absint( $_GET['form_id'] ) : 0; |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
includes/Ajax/Frontend_Form_Ajax.php(2 hunks)includes/Fields/Form_Field_Textarea.php(2 hunks)includes/Traits/FieldableTrait.php(2 hunks)wpuf-functions.php(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
includes/Ajax/Frontend_Form_Ajax.php (2)
wpuf.php (1)
wpuf(418-420)includes/Ajax.php (1)
send_error(95-102)
There was a problem hiding this comment.
Guard strip_shortcodes() against array inputs
sanitize_field_data() is invoked from FieldableTrait::prepare_meta_fields() for every field exposing that method, regardless of whether the submitted value is scalar or an array. It’s easy to end up with textarea data posted as an array (e.g., field[], repeatable rows, multi-language variants). With the new code we now hand that array directly to strip_shortcodes(), which throws a fatal TypeError on PHP 8+, whereas the previous implementation handled arrays gracefully. Please make sure the shortcode stripping only runs on scalar values and recurse when needed. For example:
public function sanitize_field_data( $data, $field ) {
- return strip_shortcodes( wp_kses_post( $data ) );
+ $sanitized = wp_kses_post( $data );
+
+ if ( is_array( $sanitized ) ) {
+ return array_map(
+ static function ( $value ) {
+ return is_string( $value ) ? strip_shortcodes( $value ) : $value;
+ },
+ $sanitized
+ );
+ }
+
+ return strip_shortcodes( $sanitized );
}📝 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 sanitize_field_data( $data, $field ) { | |
| return wp_kses_post( $data ); | |
| return strip_shortcodes( wp_kses_post( $data ) ); | |
| } | |
| public function sanitize_field_data( $data, $field ) { | |
| $sanitized = wp_kses_post( $data ); | |
| if ( is_array( $sanitized ) ) { | |
| return array_map( | |
| static function ( $value ) { | |
| return is_string( $value ) ? strip_shortcodes( $value ) : $value; | |
| }, | |
| $sanitized | |
| ); | |
| } | |
| return strip_shortcodes( $sanitized ); | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
208-208: Avoid unused parameters such as '$field'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
In includes/Fields/Form_Field_Textarea.php around lines 208-210,
sanitize_field_data() currently calls strip_shortcodes() directly on $data,
which will throw a TypeError if $data is an array; update the method to only
call strip_shortcodes() on scalar values and to recurse over arrays (preserving
keys) so nested/repeatable inputs are sanitized element-wise (apply wp_kses_post
then strip_shortcodes for each scalar), leaving non-scalar non-array values
unchanged.
sapayth
left a comment
sapayth
left a comment
There was a problem hiding this comment.
did we run all the custom fields through strip_shortcodes() too @arifulhoque7 bhai? what about Form_Field_Text?
sapayth
added
needs: author reply
…gal shortcode execution
done @sapayth vai |
arifulhoque7
added
needs: dev review
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
includes/Fields/Form_Field_Email.php (1)
92-94: Optimize sanitization flow to remove redundancy.The value is sanitized with
sanitize_text_field()on line 92, then sanitized again on line 94. Sincestrip_shortcodes()only removes shortcode patterns and doesn't introduce unsafe content, the second sanitization is redundant. Consider reordering operations for clarity and efficiency: unslash → strip shortcodes → sanitize → trim.Apply this diff to optimize the sanitization:
- $value = ! empty( $_POST[ $field['name'] ] ) ? strip_shortcodes( sanitize_text_field( wp_unslash( $_POST[ $field['name'] ] ) ) ) : ''; + $value = ! empty( $_POST[ $field['name'] ] ) ? sanitize_text_field( strip_shortcodes( wp_unslash( $_POST[ $field['name'] ] ) ) ) : ''; - return sanitize_text_field( trim( $value ) ); + return trim( $value );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
includes/Fields/Field_Contract.php(1 hunks)includes/Fields/Form_Field_Checkbox.php(1 hunks)includes/Fields/Form_Field_Dropdown.php(1 hunks)includes/Fields/Form_Field_Email.php(1 hunks)includes/Fields/Form_Field_MultiDropdown.php(1 hunks)includes/Fields/Form_Field_Post_Content.php(1 hunks)includes/Fields/Form_Field_Post_Excerpt.php(1 hunks)includes/Fields/Form_Field_Post_Tags.php(1 hunks)includes/Fields/Form_Field_Post_Taxonomy.php(1 hunks)includes/Fields/Form_Field_Post_Title.php(1 hunks)includes/Fields/Form_Field_Radio.php(1 hunks)includes/Fields/Form_Field_Text.php(1 hunks)includes/Fields/Form_Field_Textarea.php(3 hunks)includes/Fields/Form_Field_URL.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/Fields/Form_Field_Textarea.php
🧰 Additional context used
🪛 GitHub Check: Run PHPCS inspection
includes/Fields/Form_Field_Email.php
[failure] 92-92:
Array keys must be surrounded by spaces unless they contain a string or an integer.
[failure] 92-92:
Array keys must be surrounded by spaces unless they contain a string or an integer.
[failure] 92-92:
Expected 1 space after "!"; 0 found
🔇 Additional comments (12)
includes/Fields/Form_Field_Post_Tags.php (2)
115-115: LGTM! AJAX nonce check properly enforced.The
check_ajax_referer()call prevents CSRF attacks on form submissions.
117-117: Excellent security improvement—shortcode stripping added.Adding
strip_shortcodes()to the sanitization chain mitigates arbitrary shortcode execution (CVE-TBD). The order of operations is correct:wp_unslash()→sanitize_text_field()→strip_shortcodes(). This aligns with the project-wide pattern applied across multiple field types.includes/Fields/Form_Field_Dropdown.php (1)
128-132: LGTM! Solid defense-in-depth security improvement.The addition of
strip_shortcodes()to the sanitization chain effectively mitigates arbitrary shortcode execution risk. Combined with the nonce check (line 128) and the options array lookup (line 132), this creates multiple layers of protection:
- CSRF protection via nonce validation
- Input sanitization with
wp_unslash()andsanitize_text_field()- Shortcode stripping to prevent execution
- Value validation against allowed options
The final options array lookup provides an additional safeguard, ensuring that even if shortcodes bypassed earlier filters, only valid option values from the field configuration would be returned.
includes/Fields/Field_Contract.php (1)
874-875: LGTM: Shortcode stripping prevents arbitrary execution.The addition of
strip_shortcodes()correctly addresses the arbitrary shortcode execution vulnerability (CVE-TBD) by removing shortcode syntax from user-submitted values after sanitization. The order of operations (unslash → sanitize → strip shortcodes) provides proper defense-in-depth.includes/Fields/Form_Field_Checkbox.php (1)
133-133: LGTM: Array-aware shortcode stripping is correctly implemented.Using
array_map('strip_shortcodes', ...)properly strips shortcodes from each checkbox value after sanitization, preventing arbitrary shortcode execution while maintaining the existing option lookup logic.includes/Fields/Form_Field_Radio.php (1)
132-132: LGTM: Shortcode stripping properly applied to radio field values.The shortcode stripping is correctly positioned after sanitization and before the option lookup, preventing arbitrary shortcode execution in radio button submissions.
includes/Fields/Form_Field_MultiDropdown.php (1)
106-106: LGTM: Multi-select shortcode stripping correctly implemented.The
array_map('strip_shortcodes', ...)call properly handles the array of selected values, stripping shortcodes from each element after sanitization and before option lookup.includes/Fields/Form_Field_Post_Content.php (1)
214-214: LGTM: Post content shortcode stripping is a necessary security trade-off.Stripping shortcodes from user-submitted post content prevents arbitrary shortcode execution, addressing the CVE-TBD vulnerability. While this removes the ability for users to include shortcodes in submissions, this security measure is essential to prevent malicious code execution.
includes/Fields/Form_Field_Post_Title.php (1)
136-136: LGTM: Post title shortcode stripping is appropriate.Stripping shortcodes from post titles prevents arbitrary execution while maintaining the expected behavior for title fields, which typically should not contain shortcode syntax.
includes/Fields/Form_Field_URL.php (1)
118-118: LGTM: URL field shortcode stripping is correct.Stripping shortcodes from URL fields before
esc_url()processing prevents arbitrary shortcode execution and ensures only valid URL content is processed.includes/Fields/Form_Field_Post_Taxonomy.php (2)
526-526: Good security improvement - shortcode stripping prevents execution.The addition of
strip_shortcodes()effectively mitigates arbitrary shortcode execution in taxonomy fields. The sanitization order is correct:
wp_unslash()- removes WordPress slashessanitize_text_field()- removes tags and normalizes whitespacestrip_shortcodes()- removes shortcode patternsThis defense-in-depth approach aligns with the PR's security objectives.
524-524: Nonce checks are valid. All forms render thewpuf_form_addnonce viawp_nonce_fieldand both AJAX and non-AJAX entry handlers invokecheck_ajax_referer('wpuf_form_add').
includes/Fields/Form_Field_Email.php
Outdated
There was a problem hiding this comment.
Fix code style violations.
Static analysis flagged spacing issues that should be addressed for consistency with coding standards:
- Array keys should be surrounded by spaces
- Add a space after the
!operator
Apply this diff to fix the style violations:
- $value = !empty( $_POST[$field['name']] ) ? strip_shortcodes( sanitize_text_field( wp_unslash( $_POST[$field['name']] ) ) ) : '';
+ $value = ! empty( $_POST[ $field['name'] ] ) ? strip_shortcodes( sanitize_text_field( wp_unslash( $_POST[ $field['name'] ] ) ) ) : '';📝 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.
| $value = !empty( $_POST[$field['name']] ) ? strip_shortcodes( sanitize_text_field( wp_unslash( $_POST[$field['name']] ) ) ) : ''; | |
| $value = ! empty( $_POST[ $field['name'] ] ) ? strip_shortcodes( sanitize_text_field( wp_unslash( $_POST[ $field['name'] ] ) ) ) : ''; |
🧰 Tools
🪛 GitHub Check: Run PHPCS inspection
[failure] 92-92:
Array keys must be surrounded by spaces unless they contain a string or an integer.
[failure] 92-92:
Array keys must be surrounded by spaces unless they contain a string or an integer.
[failure] 92-92:
Expected 1 space after "!"; 0 found
🤖 Prompt for AI Agents
In includes/Fields/Form_Field_Email.php around line 92, the expression lacks
proper spacing: add a space after the negation operator and surround array keys
with spaces to match coding standards. Change the condition from using
"!empty($_POST[$field['name']])" to use "! empty( $_POST[ $field[ 'name' ] ] )"
and apply the same spacing style inside the ternary expression so all array keys
and operators have the required spaces.
| $field = isset( $_POST[ $field['name'] ] ) ? strip_shortcodes( sanitize_text_field( wp_unslash( $_POST[ $field['name'] ] ) ) ) : ''; | ||
|
|
||
| return wp_kses_post( $field ); |
There was a problem hiding this comment.
Incorrect sanitization breaks rich text editing.
The use of sanitize_text_field() strips all HTML tags and line breaks, then wp_kses_post() becomes redundant. This breaks the rich text editing functionality supported by this field (see render method lines 41-87 where wp_editor is used for rich/teeny modes). WordPress post excerpts typically allow basic HTML formatting.
Apply this diff to fix the sanitization order while preserving the shortcode stripping security fix:
- $field = isset( $_POST[ $field['name'] ] ) ? strip_shortcodes( sanitize_text_field( wp_unslash( $_POST[ $field['name'] ] ) ) ) : '';
+ $field = isset( $_POST[ $field['name'] ] ) ? wp_kses_post( strip_shortcodes( wp_unslash( $_POST[ $field['name'] ] ) ) ) : '';
- return wp_kses_post( $field );
+ return $field;This approach:
- Removes slashes with
wp_unslash() - Strips shortcodes for security
- Sanitizes HTML with
wp_kses_post()to allow post-content tags - Eliminates the redundant second
wp_kses_post()call
📝 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.
| $field = isset( $_POST[ $field['name'] ] ) ? strip_shortcodes( sanitize_text_field( wp_unslash( $_POST[ $field['name'] ] ) ) ) : ''; | |
| return wp_kses_post( $field ); | |
| $field = isset( $_POST[ $field['name'] ] ) | |
| ? wp_kses_post( strip_shortcodes( wp_unslash( $_POST[ $field['name'] ] ) ) ) | |
| : ''; | |
| return $field; |
🤖 Prompt for AI Agents
In includes/Fields/Form_Field_Post_Excerpt.php around lines 154 to 156, the
current sanitization uses sanitize_text_field which strips HTML and makes the
later wp_kses_post redundant, breaking the wp_editor rich text; replace the
pipeline to first wp_unslash($_POST[$field['name']] ?? ''), then
strip_shortcodes on that value, then pass the result through wp_kses_post to
allow valid post HTML, and return that single wp_kses_post result (remove
sanitize_text_field and the redundant second wp_kses_post call).
| public function prepare_entry( $field ) { | ||
| check_ajax_referer( 'wpuf_form_add' ); | ||
|
|
||
| $value = isset( $_POST[ $field['name'] ] ) ? sanitize_text_field( wp_unslash( $_POST[ $field['name'] ] ) ) : ''; | ||
| $value = isset( $_POST[ $field['name'] ] ) ? strip_shortcodes( sanitize_text_field( wp_unslash( $_POST[ $field['name'] ] ) ) ) : ''; | ||
| // return sanitize_text_field( trim( $_POST[$field['name']] ) ); | ||
| return $value; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Security fix correctly implements shortcode stripping.
The addition of strip_shortcodes() in the sanitization chain effectively prevents arbitrary shortcode execution via user-submitted text fields. The order of operations is correct:
wp_unslash()removes WordPress slashessanitize_text_field()sanitizes input and encodes special charactersstrip_shortcodes()removes any shortcode patterns as a final safeguard
Combined with the nonce check on line 153, this provides defense-in-depth against shortcode injection attacks.
Run the following script to verify this pattern has been consistently applied across all field types mentioned in the AI summary:
🏁 Script executed:
#!/bin/bash
# Description: Verify strip_shortcodes is consistently applied in prepare_entry methods
# Search for prepare_entry methods that handle POST data
rg -n -A 5 'function prepare_entry' --type=php -g 'includes/Fields/*.php' | \
rg -C 3 '\$_POST'Length of output: 8468
Add missing shortcode stripping in taxonomy field sanitization
includes/Fields/Form_Field_Post_Taxonomy.php (lines 520–525): wrap the POST value with wp_unslash() → sanitize_text_field() → strip_shortcodes() before use, e.g.:
$value = isset($_POST[$field['name']])
? strip_shortcodes(sanitize_text_field(wp_unslash($_POST[$field['name']])))
: '';🤖 Prompt for AI Agents
In includes/Fields/Form_Field_Text.php around lines 152 to 158, the field value
is sanitized but not consistently stripped of shortcodes; update the POST value
handling to mirror the suggested pattern used elsewhere by wrapping the raw POST
value with wp_unslash(), then sanitize_text_field(), then strip_shortcodes()
before returning (i.e., $value = isset($_POST[$field['name']]) ?
strip_shortcodes(sanitize_text_field(wp_unslash($_POST[$field['name']]))) :
'';), preserving the existing check_ajax_referer() call and return behavior.
| check_ajax_referer( 'wpuf_form_add' ); | ||
|
|
||
| $field_name = isset( $_POST[$field['name']] ) ? array_map( 'sanitize_text_field', wp_unslash( $_POST[$field['name']] ) ) : []; | ||
| $field_name = array_map( 'strip_shortcodes', $field_name ); |
There was a problem hiding this comment.
we need to sanitize_text_field too
sapayth
added
needs: author reply
sapayth
removed
the
needs: author reply
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
includes/Fields/Form_Field_Checkbox.php (1)
132-133: Optional: Consolidate sanitization for efficiency.
sanitize_text_fieldis applied twice—once on line 132 when reading from POST, and again on line 133 after stripping shortcodes. While this redundancy is harmless (the function is idempotent), you could streamline the flow by stripping shortcodes before the first sanitization.Consider this consolidation:
-$field_name = isset( $_POST[$field['name']] ) ? array_map( 'sanitize_text_field', wp_unslash( $_POST[$field['name']] ) ) : []; -$field_name = array_map( 'sanitize_text_field', array_map( 'strip_shortcodes', $field_name ) ); +$field_name = isset( $_POST[$field['name']] ) ? array_map( 'sanitize_text_field', array_map( 'strip_shortcodes', wp_unslash( $_POST[$field['name']] ) ) ) : [];This avoids the double sanitization while maintaining the same security guarantees.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
includes/Fields/Form_Field_Checkbox.php(1 hunks)
🔇 Additional comments (1)
includes/Fields/Form_Field_Checkbox.php (1)
129-133: Security improvement LGTM: Shortcode stripping now in place.The addition of
strip_shortcodeson line 133 effectively mitigates the arbitrary shortcode execution vulnerability described in the PR objectives. Combined with the nonce check (line 130) and option validation (lines 140-142), this provides solid defense-in-depth.
3 participants
Security: Fix capability checks & shortcode execution vuln Close issue , close issue, Related PRO PR
This patch addresses multiple critical vulnerabilities in WP User Frontend v4.1.12:
Arbitrary Shortcode Execution (CVE-TBD)
with absint() validation
Missing Capability Check in AJAX Preview
Unauthorized Form Import
Information Disclosure via get_roles()
Security improvements:
Testing:
Summary by CodeRabbit