fix: security hardening from 2026-07 audit#1903
Open
arifulhoque7 wants to merge 2 commits into
Open
Conversation
Security-only, feature-preserving fixes for the free-plugin findings from the 2026-07 audit (tracked with the pro findings on weDevsOfficial/wpuf-pro#1622). - URL custom-field output: esc_url/esc_attr/esc_html (stored XSS) (weDevsOfficial/wpuf-pro#1608) - wpuf_clear_schedule_lock: require edit_others_posts + absint (missing authz) (weDevsOfficial/wpuf-pro#1609) - custom-field labels esc_html, embed URL esc_url_raw, transaction query strict in_array + absint (weDevsOfficial/wpuf-pro#1610) - repeat-field submissions: per-element sanitize on storage (weDevsOfficial/wpuf-pro#1620) - JSON import: allowlist post_type/post_status (mass-assignment) (weDevsOfficial/wpuf-pro#1621) - Settings API: replace deprecated create_function with a closure No functional behaviour change for legitimate flows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Reformat repeat-field sanitizer closures to WPCS style and restore the original single-line // phpcs:ignore on the settings-desc callback, so the security-fix changes introduce zero new PHPCS errors vs develop. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThis PR hardens sanitization, escaping, and authorization across several files: settings section descriptions are escaped via wp_kses_post, JSON import restricts post_type/post_status to allowlists, repeater field values are sanitized before storage, custom field output is escaped, transaction query parameters are strictly validated, and schedule lock clearing now requires edit_others_posts capability. ChangesSecurity Hardening: Sanitization, Escaping, and Authorization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (4)
Lib/WeDevs_Settings_API.php (1)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueGood fix: closure replaces deprecated
create_functionwith escaped output.Replacing the deprecated
create_function()callback with a closure that echoes viawp_kses_post()is a solid security fix —create_function()was removed in PHP 8.0, and the raw$section['desc']HTML is now sanitized before output. The closure correctly captures$sectionby value per loop iteration, so no closure-reuse bug across sections.One nit: the blanket
// phpcs:ignoreon Line 110 suppresses all sniffs on that line rather than the specific one(s) being silenced (likelySquiz.Scope.StaticThisUsageor a closure/anonymous-function sniff). Targeting the specific sniff name preserves coverage for other rules on that line.♻️ Suggested tightening
- $callback = function () use ( $section ) { echo wp_kses_post( $section['desc'] ); }; // phpcs:ignore + $callback = function () use ( $section ) { echo wp_kses_post( $section['desc'] ); }; // phpcs:ignore Generic.Files.LineLength.TooLongPlease confirm the exact sniff being suppressed here (the actual identifier depends on what PHPCS flagged locally) and swap in the precise name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Lib/WeDevs_Settings_API.php` around lines 108 - 110, The closure in WeDevs_Settings_API is fine, but the inline PHPCS suppression is too broad; replace the generic // phpcs:ignore on the callback line with the exact sniff identifier that PHPCS reports locally. Keep the existing closure and wp_kses_post output, and narrow the ignore to only the specific sniff affecting this anonymous function so other checks on that line still run.includes/Admin/Admin_Tools.php (2)
208-209: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNormalize
ping_statusandcomment_statustoo.These are still mass-assigned from the JSON payload. Keep them to
open/closedlike the newpost_statusallowlist so imports cannot persist malformed post state.Proposed hardening
+ $allowed_comment_statuses = [ 'open', 'closed' ]; + $ping_status = $value['post_data']['ping_status'] ?? ''; + $comment_status = $value['post_data']['comment_status'] ?? ''; + + if ( ! in_array( $ping_status, $allowed_comment_statuses, true ) ) { + $ping_status = 'closed'; + } + + if ( ! in_array( $comment_status, $allowed_comment_statuses, true ) ) { + $comment_status = 'closed'; + } + $generate_post = [ 'post_title' => $value['post_data']['post_title'] ?? '', 'post_status' => $post_status, 'post_type' => $post_type, - 'ping_status' => $value['post_data']['ping_status'] ?? '', - 'comment_status' => $value['post_data']['comment_status'] ?? '', + 'ping_status' => $ping_status, + 'comment_status' => $comment_status, ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/Admin/Admin_Tools.php` around lines 208 - 209, The `Admin_Tools` import mapping still assigns `ping_status` and `comment_status` directly from the JSON payload, so they can persist invalid values. Update the post-data normalization in `Admin_Tools` to apply the same allowlist style used for `post_status`, keeping only `open` or `closed` and falling back safely for anything else. Use the existing `post_data` assignment block in `Admin_Tools` as the fix point so all imported post state stays normalized.
190-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDrop the unused
$keyloop variable.PHPMD flags
$keyas unused; iterate values only.Proposed cleanup
- foreach ( $options as $key => $value ) { + foreach ( $options as $value ) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/Admin/Admin_Tools.php` at line 190, The foreach in Admin_Tools::process-style options iteration is binding an unused $key variable, which PHPMD flags; update the loop to iterate over values only and remove $key from the foreach signature, keeping the existing $value handling unchanged.Source: Linters/SAST tools
includes/Traits/FieldableTrait.php (1)
887-925: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting the repeated sanitization closure into a shared helper.
The same
strip_shortcodes( sanitize_text_field( $item ) )logic is duplicated at lines 892, 897, 901, and 917 (array and scalar variants). Extracting a small private static helper (e.g.sanitize_repeater_value( $value )that internally handles both array/scalar) would reduce duplication and ease future maintenance.♻️ Example helper extraction
+ /** + * Sanitize a repeater field value (scalar or array) for storage. + * + * `@since` WPUF_SINCE + * + * `@param` mixed $value + * + * `@return` mixed + */ + protected static function sanitize_repeater_value( $value ) { + if ( is_array( $value ) ) { + return array_map( [ __CLASS__, 'sanitize_repeater_value' ], $value ); + } + + return strip_shortcodes( sanitize_text_field( $value ) ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@includes/Traits/FieldableTrait.php` around lines 887 - 925, The repeater sanitization logic in FieldableTrait is duplicated in several branches, making it harder to maintain. Extract the repeated strip_shortcodes(sanitize_text_field(...)) behavior into a shared private static helper in FieldableTrait, such as a helper used by the repeater handling logic in the row-processing block. Update both the array mapping path and the scalar/fallback path to call that helper so sanitization stays consistent and future changes only need to be made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@includes/class-frontend-render-form.php`:
- Around line 786-801: The repeat-child storage path in
class-frontend-render-form is missing shortcode stripping, unlike the other
repeater storage flow. Update the sanitization in the repeat field handling
around the raw_repeat_value and sanitized_repeat_value logic to apply
strip_shortcodes alongside the existing
sanitize_text_field/sanitize_textarea_field behavior for both scalar and array
values. Keep the change localized to the repeat storage branch so the
repeat_fields and meta_key_value assignments remain consistent with the other
storage path.
In `@wpuf-functions.php`:
- Around line 2370-2385: The transaction query in wpuf_get_all_transactions() is
still interpolating the LIMIT values directly, even though `$offset` and
`$number` are sanitized. Update the `$wpdb->get_results()` query to use
`$wpdb->prepare()` with `%d` placeholders for the LIMIT clause, matching the
existing SQL-safety pattern already used elsewhere in wpuf-functions.php. Keep
the validation for `orderby` and `order` as-is, but move the dynamic limit
values into the prepared statement for consistency and defense-in-depth.
---
Nitpick comments:
In `@includes/Admin/Admin_Tools.php`:
- Around line 208-209: The `Admin_Tools` import mapping still assigns
`ping_status` and `comment_status` directly from the JSON payload, so they can
persist invalid values. Update the post-data normalization in `Admin_Tools` to
apply the same allowlist style used for `post_status`, keeping only `open` or
`closed` and falling back safely for anything else. Use the existing `post_data`
assignment block in `Admin_Tools` as the fix point so all imported post state
stays normalized.
- Line 190: The foreach in Admin_Tools::process-style options iteration is
binding an unused $key variable, which PHPMD flags; update the loop to iterate
over values only and remove $key from the foreach signature, keeping the
existing $value handling unchanged.
In `@includes/Traits/FieldableTrait.php`:
- Around line 887-925: The repeater sanitization logic in FieldableTrait is
duplicated in several branches, making it harder to maintain. Extract the
repeated strip_shortcodes(sanitize_text_field(...)) behavior into a shared
private static helper in FieldableTrait, such as a helper used by the repeater
handling logic in the row-processing block. Update both the array mapping path
and the scalar/fallback path to call that helper so sanitization stays
consistent and future changes only need to be made in one place.
In `@Lib/WeDevs_Settings_API.php`:
- Around line 108-110: The closure in WeDevs_Settings_API is fine, but the
inline PHPCS suppression is too broad; replace the generic // phpcs:ignore on
the callback line with the exact sniff identifier that PHPCS reports locally.
Keep the existing closure and wp_kses_post output, and narrow the ignore to only
the specific sniff affecting this anonymous function so other checks on that
line still run.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a512c6f2-5cb3-4ad4-8023-f11a704ce027
📒 Files selected for processing (5)
Lib/WeDevs_Settings_API.phpincludes/Admin/Admin_Tools.phpincludes/Traits/FieldableTrait.phpincludes/class-frontend-render-form.phpwpuf-functions.php
Comment on lines
+786
to
+801
| // Storage-layer sanitization for repeat-field child values. Use the | ||
| // textarea sanitizer for textarea children (preserve newlines), | ||
| // otherwise the text-field sanitizer. Sanitize per element so | ||
| // multi-value repeat data still round-trips. | ||
| $repeat_sanitize_cb = ( isset( $value['input_type'] ) && 'textarea' === $value['input_type'] ) | ||
| ? 'sanitize_textarea_field' | ||
| : 'sanitize_text_field'; | ||
|
|
||
| $raw_repeat_value = wp_unslash( $_POST[ $formatted_key ] ); // phpcs:ignore WordPress.Security.NonceVerification | ||
|
|
||
| $sanitized_repeat_value = is_array( $raw_repeat_value ) | ||
| ? array_map( $repeat_sanitize_cb, $raw_repeat_value ) | ||
| : call_user_func( $repeat_sanitize_cb, $raw_repeat_value ); | ||
|
|
||
| $repeat_fields['fields'][ $formatted_key ] = $sanitized_repeat_value; | ||
| $meta_key_value[ $formatted_key ] = $sanitized_repeat_value; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm which prepare_meta_fields is actually invoked for repeat fields at runtime,
# and whether output for this storage path (formatted_key meta) is escaped/stripped downstream.
rg -n "prepare_meta_fields" --type=php -C3Repository: weDevsOfficial/wp-user-frontend
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and locate repeat-field sanitization and any shortcode stripping.
git ls-files 'includes/class-frontend-render-form.php' 'includes/Traits/FieldableTrait.php' \
| xargs -r -n1 sh -c 'echo "### $1"; wc -l "$1"; echo; ast-grep outline "$1" --view expanded || true; echo' sh
printf '\n## strip_shortcodes references\n'
rg -n "strip_shortcodes|prepare_meta_fields|repeat" includes/class-frontend-render-form.php includes/Traits/FieldableTrait.php -C 3Repository: weDevsOfficial/wp-user-frontend
Length of output: 19831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact surrounding code for the repeater sanitization paths.
sed -n '760,830p' includes/class-frontend-render-form.php
printf '\n---\n'
sed -n '860,950p' includes/Traits/FieldableTrait.phpRepository: weDevsOfficial/wp-user-frontend
Length of output: 8720
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all repeater-related sanitization paths and any downstream shortcode stripping.
rg -n "sanitize_text_field|sanitize_textarea_field|strip_shortcodes|repeat" includes -g '*.php' -C 2Repository: weDevsOfficial/wp-user-frontend
Length of output: 50388
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the call sites for both prepare_meta_fields implementations and the submit path that reaches them.
rg -n "class-frontend-render-form::prepare_meta_fields|FieldableTrait::prepare_meta_fields|prepare_meta_fields\(" includes -g '*.php' -C 2
printf '\n## class definitions / inheritance\n'
rg -n "class .*Frontend_Render_Form|use FieldableTrait|extends .*FieldableTrait|trait FieldableTrait" includes -g '*.php' -C 2Repository: weDevsOfficial/wp-user-frontend
Length of output: 4506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the call sites for both prepare_meta_fields implementations and the submit path that reaches them.
rg -n "prepare_meta_fields\(" includes -g '*.php' -C 2
printf '\n## class definitions / inheritance\n'
rg -n "class .*Frontend_Render_Form|use FieldableTrait|extends .*FieldableTrait|trait FieldableTrait" includes -g '*.php' -C 2Repository: weDevsOfficial/wp-user-frontend
Length of output: 4506
Strip shortcodes from repeat-child storage too includes/class-frontend-render-form.php:786-801 only applies sanitize_text_field() / sanitize_textarea_field() here, while the other repeater storage path also runs strip_shortcodes(). Add the same stripping here so shortcode handling stays consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@includes/class-frontend-render-form.php` around lines 786 - 801, The
repeat-child storage path in class-frontend-render-form is missing shortcode
stripping, unlike the other repeater storage flow. Update the sanitization in
the repeat field handling around the raw_repeat_value and sanitized_repeat_value
logic to apply strip_shortcodes alongside the existing
sanitize_text_field/sanitize_textarea_field behavior for both scalar and array
values. Keep the change localized to the repeat storage branch so the
repeat_fields and meta_key_value assignments remain consistent with the other
storage path.
Comment on lines
+2370
to
+2385
| if ( ! in_array( $args['orderby'], $orderby, true ) ) { | ||
| $args['orderby'] = 'id'; | ||
| } | ||
|
|
||
| if ( ! in_array( $args['order'], $order ) ) { | ||
| if ( ! in_array( $args['order'], $order, true ) ) { | ||
| $args['order'] = 'DESC'; | ||
| } | ||
|
|
||
| if ( $args['count'] ) { | ||
| return $wpdb->get_var( "SELECT COUNT(*) FROM {$wpdb->prefix}wpuf_transaction" ); | ||
| } | ||
|
|
||
| $result = $wpdb->get_results( "SELECT * FROM {$wpdb->prefix}wpuf_transaction ORDER BY `{$args['orderby']}` {$args['order']} LIMIT {$args['offset']}, {$args['number']}", OBJECT ); | ||
| $offset = absint( $args['offset'] ); | ||
| $number = absint( $args['number'] ); | ||
|
|
||
| $result = $wpdb->get_results( "SELECT * FROM {$wpdb->prefix}wpuf_transaction ORDER BY `{$args['orderby']}` {$args['order']} LIMIT {$offset}, {$number}", OBJECT ); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use $wpdb->prepare() for the LIMIT clause instead of raw interpolation.
$offset/$number are safely absint()'d, so this isn't currently exploitable, but the raw string interpolation into LIMIT {$offset}, {$number} doesn't follow the project's SQL-safety convention. wpuf_get_all_transactions() (below) already uses $wpdb->prepare() with %d placeholders for the same purpose — this function should match that pattern for consistency and defense-in-depth. As per coding guidelines, "Use $wpdb->prepare() for all dynamic SQL values to prevent SQL injection."
🛡️ Proposed fix
- $offset = absint( $args['offset'] );
- $number = absint( $args['number'] );
-
- $result = $wpdb->get_results( "SELECT * FROM {$wpdb->prefix}wpuf_transaction ORDER BY `{$args['orderby']}` {$args['order']} LIMIT {$offset}, {$number}", OBJECT );
+ $offset = absint( $args['offset'] );
+ $number = absint( $args['number'] );
+
+ $result = $wpdb->get_results(
+ $wpdb->prepare(
+ "SELECT * FROM {$wpdb->prefix}wpuf_transaction ORDER BY `{$args['orderby']}` {$args['order']} LIMIT %d, %d",
+ $offset,
+ $number
+ ),
+ OBJECT
+ );📝 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.
Suggested change
| if ( ! in_array( $args['orderby'], $orderby, true ) ) { | |
| $args['orderby'] = 'id'; | |
| } | |
| if ( ! in_array( $args['order'], $order ) ) { | |
| if ( ! in_array( $args['order'], $order, true ) ) { | |
| $args['order'] = 'DESC'; | |
| } | |
| if ( $args['count'] ) { | |
| return $wpdb->get_var( "SELECT COUNT(*) FROM {$wpdb->prefix}wpuf_transaction" ); | |
| } | |
| $result = $wpdb->get_results( "SELECT * FROM {$wpdb->prefix}wpuf_transaction ORDER BY `{$args['orderby']}` {$args['order']} LIMIT {$args['offset']}, {$args['number']}", OBJECT ); | |
| $offset = absint( $args['offset'] ); | |
| $number = absint( $args['number'] ); | |
| $result = $wpdb->get_results( "SELECT * FROM {$wpdb->prefix}wpuf_transaction ORDER BY `{$args['orderby']}` {$args['order']} LIMIT {$offset}, {$number}", OBJECT ); | |
| if ( ! in_array( $args['orderby'], $orderby, true ) ) { | |
| $args['orderby'] = 'id'; | |
| } | |
| if ( ! in_array( $args['order'], $order, true ) ) { | |
| $args['order'] = 'DESC'; | |
| } | |
| if ( $args['count'] ) { | |
| return $wpdb->get_var( "SELECT COUNT(*) FROM {$wpdb->prefix}wpuf_transaction" ); | |
| } | |
| $offset = absint( $args['offset'] ); | |
| $number = absint( $args['number'] ); | |
| $result = $wpdb->get_results( | |
| $wpdb->prepare( | |
| "SELECT * FROM {$wpdb->prefix}wpuf_transaction ORDER BY `{$args['orderby']}` {$args['order']} LIMIT %d, %d", | |
| $offset, | |
| $number | |
| ), | |
| OBJECT | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@wpuf-functions.php` around lines 2370 - 2385, The transaction query in
wpuf_get_all_transactions() is still interpolating the LIMIT values directly,
even though `$offset` and `$number` are sanitized. Update the
`$wpdb->get_results()` query to use `$wpdb->prepare()` with `%d` placeholders
for the LIMIT clause, matching the existing SQL-safety pattern already used
elsewhere in wpuf-functions.php. Keep the validation for `orderby` and `order`
as-is, but move the dynamic limit values into the prepared statement for
consistency and defense-in-depth.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security hardening — 2026-07 audit (free plugin)
Free-plugin fixes from the 2026-07 WPUF security audit. Findings are tracked alongside the pro set on weDevsOfficial/wpuf-pro#1622.
Security-only, feature-preserving.
php -lclean on all changed files.esc_url/esc_attr/esc_html.wpuf_clear_schedule_lockmissing authorization →current_user_can( 'edit_others_posts' )+absint.esc_url_raw, transaction query strictin_array+absint, deprecatedcreate_function→ closure.post_type/post_status(mass-assignment).No behaviour change for legitimate flows (rich text / shortcodes still render; storage-layer sanitization preserves multi-value data).
🤖 Generated with Claude Code
Recreated against weDevsOfficial (replaces fork-internal PR arifulhoque7#1).
Summary by CodeRabbit