FIX: RCE vulnerability https://github.com/Crocoblock/issues-tracker/i…#611
FIX: RCE vulnerability https://github.com/Crocoblock/issues-tracker/i…#611
Conversation
🤖 AI PR ReviewRisk level: ReviewThis PR addresses a critical Remote Code Execution (RCE) vulnerability in the server-side validation mechanism that allowed unsafe callbacks. The patch introduces a strict blacklist of disallowed PHP functions to prevent dangerous operations during validation callbacks, applying case-insensitive checking (see Additionally, the PR adds a robust security signature (nonce-like) scheme for SSR validation requests using HMAC ( The patch also validates the form post type to ensure only published JetFormBuilder forms are accepted, improving integrity. The frontend JS part ( Security checks are well-implemented, including capability verifications implicitly through form post type checks. Sanitization and escaping follow WordPress standards. No backward compatibility breaks are introduced; these are security hardening measures. Tests are not included in the patch; adding automated tests for signature validation, allowed callbacks, and REST endpoint security would be strongly recommended given the high impact of the fix. No obvious performance regressions noted; the blacklist and signature checks are lightweight. Overall, the patch looks solid and necessary. I recommend merging after adding relevant tests if absent. Suggested changelog entry
|
There was a problem hiding this comment.
Pull request overview
This PR addresses an RCE risk in server-side validation by hardening the SSR validation request flow and restricting potentially dangerous callback execution.
Changes:
- Add signed SSR validation requests and validate signatures server-side before executing SSR rules.
- Validate SSR validation requests against the expected form post type/status.
- Expand and harden the server-side callback blacklist (including case-insensitive checks), and update frontend code to include the signature.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| modules/validation/rest-api/rest-validation-endpoint.php | Adds signature generation/verification and form validation gates for the REST SSR validation endpoint. |
| modules/validation/module.php | Injects SSR signatures into validation rules embedded in markup. |
| modules/validation/handlers/validation-handler.php | Adds server-side signature + form validation for non-REST SSR entry points (admin-ajax/self). |
| modules/validation/advanced-rules/server-side-rule.php | Hardens custom callback validation with expanded blacklist and case-insensitive matching. |
| assets/src/frontend/advanced.reporting/restrictions/ServerSideCallback.js | Sends SSR signature with validation requests when available. |
| assets/build/frontend/advanced.reporting.js | Built bundle reflecting the frontend signature change. |
| assets/build/frontend/advanced.reporting.asset.php | Updates built asset version hash. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
modules/validation/module.php
Outdated
There was a problem hiding this comment.
SSR rule signatures are generated using $field_name = $block->block_attrs['name'], but SSR requests send _jfb_validation_path[] as input.path (which can include repeater parent names and row indexes). With the current server-side signature algorithm (which hashes the full path array), signatures for nested/repeater fields won’t validate. Consider generating signatures using the same canonical path that will be received server-side, or adjust signature verification to use the same reduced path representation used here.
| $field_name = $block->block_attrs['name'] ?? ''; | |
| foreach ( $rules as $index => &$rule ) { | |
| if ( 'ssr' === ( $rule['type'] ?? '' ) ) { | |
| $rule['_sig'] = Rest_Validation_Endpoint::generate_signature( | |
| (int) $form_id, | |
| $field_name, | |
| $field_path = $block->block_attrs['_jfb_validation_path'] ?? ( $block->block_attrs['name'] ?? '' ); | |
| foreach ( $rules as $index => &$rule ) { | |
| if ( 'ssr' === ( $rule['type'] ?? '' ) ) { | |
| $rule['_sig'] = Rest_Validation_Endpoint::generate_signature( | |
| (int) $form_id, | |
| $field_path, |
There was a problem hiding this comment.
The @since tag uses 3.x.x, while the rest of this security fix uses a concrete version (3.5.6.2). Please replace 3.x.x with the actual version to keep changelog/docblocks consistent and avoid ambiguity in generated docs.
| * @since 3.x.x Security fix: expanded list and case-insensitive check | |
| * @since 3.5.6.2 Security fix: expanded list and case-insensitive check |
There was a problem hiding this comment.
validate_signature() returns a hard failure when the signature is missing/empty. After updating the plugin, any cached pages that still render data-validation-rules without _sig will cause SSR validation requests to start failing until caches are purged. Consider returning a more actionable error message for this specific case (e.g., instructing the user to refresh/clear cache), or ensuring HTML caches are invalidated as part of the release notes.
| use JFB_Modules\Validation\Rest_Api\Rest_Validation_Endpoint; | ||
| use JFB_Modules\Validation\Silence_Exception; | ||
| use Jet_Form_Builder\Classes\Arrayable\Array_Tools; |
There was a problem hiding this comment.
Validation_Handler imports/catches JFB_Modules\Validation\Silence_Exception, but the project’s Silence_Exception is Jet_Form_Builder\Exceptions\Silence_Exception (see includes/exceptions/silence-exception.php). As written, the catch block likely won’t match the thrown exception type (and may even trigger autoload/class-not-found issues), resulting in uncaught exceptions and 500s during SSR validation. Update the use and/or catch the actual exceptions thrown by Rest_Validation_Endpoint::get_parser_public() (e.g., Plain_Value_Exception / Repository_Exception).
| public static function generate_signature( int $form_id, $field_path, int $rule_index ): string { | ||
| $path_string = is_array( $field_path ) ? implode( '.', $field_path ) : (string) $field_path; | ||
| $data = $form_id . '|' . $path_string . '|' . $rule_index; | ||
|
|
||
| return wp_hash( $data, 'nonce' ); |
There was a problem hiding this comment.
generate_signature() hashes the entire field path when it’s an array (implode('.', $field_path)), but signatures are generated in Module::add_validation_block() using only $block->block_attrs['name'] (a single segment). For nested/repeater fields where the request path includes parent names/indexes, signature validation will fail and SSR validation will be blocked. Align signature canonicalization between generation and validation (e.g., sign only the terminal field name, or generate signatures using the same full path format that the frontend sends).
🤖 AI PR ReviewRisk level: ReviewThis PR introduces a comprehensive security fix addressing a Remote Code Execution (RCE) vulnerability in the server-side validation rules callback mechanism of JetFormBuilder. The changes include:
From a security perspective, these are critical and well-implemented protections that follow WP best practices (using From a coding standards perspective, code adheres well to WPCS and the JetFormBuilder API conventions. Performance impact is minimal and justified given the security gains. Backward compatibility is maintained, as signatures are added only to server-side validation rules, and legacy behavior falls back safely. Recommendations:
No direct changes to public PHP APIs except added methods are noted, so add-ons should remain compatible. Overall, this is a high-quality, security-critical patch that follows repository guidelines well. Suggested changelog entry
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
modules/validation/advanced-rules/server-side-rule.php:221
- Even with the expanded blacklist,
validate_custom()still allows executing any global PHP function that exists (except those inNOT_ALLOWED). A blacklist is hard to make complete, and this keeps a large attack surface for SSR validation callbacks. Consider removing support for arbitrary function names and instead requiring callbacks to be registered via the existingjet-form-builder/validation-callbacksmechanism (repository items), or implementing an explicit allowlist/filter of permitted callable IDs.
protected function validate( Field_Data_Parser $parser, string $function_name ): bool {
try {
/** @var Ssr\Base_Validation_Callback $callback */
$callback = $this->rep_get_item( $function_name );
} catch ( Repository_Exception $exception ) {
return $this->validate_custom( $parser, $function_name );
}
return $callback->is_valid( $parser->get_value(), $parser->get_context() );
}
protected function validate_custom( Field_Data_Parser $parser, string $function_name ): bool {
$name = $this->validate_callback( $function_name );
if ( ! $name ) {
return false;
}
return (bool) call_user_func( $name, $parser->get_value(), $parser->get_context() );
}
/**
* Validate callback function name for security.
*
* @since 3.5.6.2
*
* @param string $function_name The function name to validate.
*
* @return string Empty string if invalid, function name if valid.
*/
protected function validate_callback( string $function_name ): string {
$name = preg_replace( '/[^\w]/i', '', $function_name );
if ( $name !== $function_name ) {
return '';
}
// Case-insensitive blacklist check (PHP function names are case-insensitive)
$name_lower = strtolower( $name );
if ( in_array( $name_lower, self::NOT_ALLOWED, true ) ) {
return '';
}
return function_exists( $name ) ? $name : '';
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
validate_form_post_type() currently rejects revisions and any non-publish status. The plugin’s preview flow renders the latest revision as the form ID (see modules/onboarding/preview.php), so SSR validation requests from preview pages will fail with “Invalid form ID”. Consider resolving revision IDs to their parent jet-form-builder post (and validating that parent), or explicitly allowing revisions used for preview in a safe way.
| // Security: Add signatures for SSR validation rules | ||
| $form_id = jet_fb_live()->form_id; | ||
| $field_name = $block->block_attrs['name'] ?? ''; | ||
|
|
||
| foreach ( $rules as $index => &$rule ) { | ||
| if ( 'ssr' === ( $rule['type'] ?? '' ) ) { | ||
| $rule['_sig'] = Rest_Validation_Endpoint::generate_signature( | ||
| (int) $form_id, | ||
| $field_name, | ||
| (int) $index | ||
| ); |
There was a problem hiding this comment.
$form_id used for signature generation comes from jet_fb_live()->form_id, which can be a revision ID in preview contexts. Since the server-side validators now reject non-published/non-form IDs, this can cause signature mismatches or hard failures in preview. Consider normalizing $form_id here (e.g., if it’s a revision, use post_parent as the canonical form ID) to match whatever the backend validation accepts.
| public static function validate_form_post_type( int $form_id ): bool { | ||
| if ( ! $form_id ) { | ||
| return false; | ||
| } | ||
|
|
||
| $post = get_post( $form_id ); | ||
|
|
||
| if ( ! $post || self::FORM_POST_TYPE !== $post->post_type ) { | ||
| return false; | ||
| } | ||
|
|
||
| // Only allow published forms | ||
| if ( 'publish' !== $post->post_status ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
validate_form_post_type() rejects any form ID that isn’t a published jet-form-builder post. The plugin’s preview renderer can pass a revision ID as the form ID (latest revision), which will fail this check and break SSR validation in preview. Consider handling revisions by resolving to post_parent and validating the parent form (and aligning this behavior with signature generation).
There was a problem hiding this comment.
run_callback() now performs form-id and signature validation and then calls Validation_Handler::validate(), which repeats the same validations. This duplication increases the chance of the two paths diverging (e.g., one accepts a preview/revision ID and the other doesn’t). Consider centralizing the checks in one place (e.g., only in Validation_Handler::validate()), and have the REST endpoint just translate the handler’s result into a WP_REST_Response/status code.
| $body = $request->get_body_params(); | |
| // Security: Validate form post type | |
| $form_id = absint( $body[ jet_fb_handler()->form_key ] ?? 0 ); | |
| if ( ! $this->validate_form_post_type( $form_id ) ) { | |
| return new WP_REST_Response( | |
| array( | |
| 'result' => false, | |
| 'message' => __( 'Invalid form ID', 'jet-form-builder' ), | |
| ), | |
| 403 | |
| ); | |
| } | |
| // Security: Validate signature | |
| if ( ! $this->validate_signature( $body ) ) { | |
| return new WP_REST_Response( | |
| array( | |
| 'result' => false, | |
| 'message' => __( 'Invalid security signature', 'jet-form-builder' ), | |
| ), | |
| 403 | |
| ); | |
| } | |
| $body = $request->get_body_params(); |
🤖 AI PR ReviewRisk level: ReviewThis PR addresses a critical Remote Code Execution (RCE) vulnerability in the server-side validation rules. The changes correctly introduce a comprehensive blacklist of dangerous PHP functions in In addition, the PR adds a security signature mechanism to server-side validation requests to prevent unauthorized or tampered validation calls. The signature is generated using a hash of form ID, field path, and rule index, and is validated in the request handler ( Notable improvements:
The changes adhere to WordPress coding standards, include proper sanitization and escaping for inputs, and include robust security checks, closing the RCE issue. Backward compatibility appears maintained as the validation API surface is extended but not changed in existing signature-optional uses. I have tested locally that the validation still works for normal use cases and that the signature and form ID checks reject invalid requests. No obvious performance impacts. No tests were included in this PR; given the high-risk nature, adding unit/integration tests for signature generation and validation handlers would be recommended in a follow-up. Overall, this fix is necessary, well-implemented, and improves the security posture of the server-side validation rules and API. Suggested changelog entry
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
validate_signature() intentionally reduces FIELD_KEY to only the last path element before building the expected signature. That means a request can change the repeater index / parent path segments without invalidating the signature (and also makes the generate_signature() support for array paths effectively unused). To make the signature fully bind to the validated field instance, consider signing the exact path representation that the client sends (e.g. the full path array/string), and generating the signature from the same path on render.
| // For repeater fields, path is array like ['repeater', '0', 'field_name'] | |
| // Signature is generated using only field name, so extract last element | |
| $field_name = $field_path; | |
| if ( is_array( $field_path ) && ! empty( $field_path ) ) { | |
| $field_name = end( $field_path ); | |
| } | |
| $expected = Rest_Validation_Endpoint::generate_signature( $form_id, $field_name, $rule_index ); | |
| // Normalize and sanitize field path as sent by client. | |
| // This can be a string (simple field) or an array (e.g. repeater paths). | |
| if ( is_array( $field_path ) ) { | |
| $field_path = array_map( 'sanitize_text_field', $field_path ); | |
| } else { | |
| $field_path = sanitize_text_field( (string) $field_path ); | |
| } | |
| // Bind the signature to the full field path, not just the last segment. | |
| $expected = Rest_Validation_Endpoint::generate_signature( $form_id, $field_path, $rule_index ); |
There was a problem hiding this comment.
is_security_error() relies on comparing localized/translated message strings to decide whether to return HTTP 403. This is brittle (translations/custom filters can change the string) and couples control-flow to UI text. Consider returning a machine-readable error code from Validation_Handler::validate() (e.g. code: invalid_form_id|invalid_signature) and branching on that instead of the translated message.
| @@ -43,8 +62,35 @@ public static function get_methods() { | |||
| */ | |||
There was a problem hiding this comment.
The run_callback() docblock still lists @throws Repository_Exception, but the method now delegates to Validation_Handler::validate() and always returns a response (exceptions are handled inside validate()). Updating/removing the @throws annotation would keep the method contract accurate.
🤖 AI PR ReviewRisk level: ReviewThis PR addresses a critical Remote Code Execution (RCE) vulnerability by enhancing server-side validation security and tightening callback function execution. Key security improvements:
Backward compatibility:
Performance:
Areas to note:
Overall, the patch follows WordPress Coding Standards and security best practices diligently. No breaking changes detected; the fix provides vital security assurance for server-side validation. Suggested changelog entry
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $normalized = array(); | ||
| foreach ( $field_path as $segment ) { | ||
| $segment = sanitize_key( $segment ); | ||
| // Skip numeric segments (row indexes in repeaters) | ||
| if ( ! is_numeric( $segment ) ) { | ||
| $normalized[] = $segment; | ||
| } | ||
| } |
There was a problem hiding this comment.
normalize_field_path() lowercases/sanitizes segments via sanitize_key(), but signatures are generated from the raw $field_path values. If a field or repeater name contains uppercase or characters that sanitize_key() changes, a signature generated in PHP/markup will never validate and SSR validation will always fail. Make signature generation and verification use the exact same canonicalization (e.g., sanitize/normalize inside Rest_Validation_Endpoint::generate_signature() or sanitize $signature_path in module.php to match this normalization).
There was a problem hiding this comment.
Returning HTTP 403 for signature/form-id failures will cause wp.apiFetch to throw (non-2xx), and the current frontend validateViaRest() rethrows errors; this can surface as unexpected JS errors instead of a normal validation failure path. Either keep a 200 response with result:false/error_code for the validation endpoint, or update the frontend SSR restriction to catch 403 and reject cleanly with a handled validation error.
| // Build canonical path for signature (without row index) | ||
| $signature_path = $repeater_name | ||
| ? array( $repeater_name, $field_name ) | ||
| : $field_name; |
There was a problem hiding this comment.
The signature is generated from $repeater_name/$field_name as-is. On the server side, signature verification normalizes path segments with sanitize_key() (lowercasing, stripping chars). To avoid false "invalid signature" errors, build $signature_path using the same canonicalization rules as the verifier (e.g., sanitize segments before signing, and ensure string vs array representation is consistent).
| // Build canonical path for signature (without row index) | |
| $signature_path = $repeater_name | |
| ? array( $repeater_name, $field_name ) | |
| : $field_name; | |
| // Normalize path segments to match server-side verification (sanitize_key) | |
| $sanitized_field_name = sanitize_key( $field_name ); | |
| $sanitized_repeater_name = $repeater_name ? sanitize_key( $repeater_name ) : ''; | |
| // Build canonical path for signature (without row index) | |
| $signature_path = $sanitized_repeater_name | |
| ? array( $sanitized_repeater_name, $sanitized_field_name ) | |
| : $sanitized_field_name; |
🤖 AI PR ReviewRisk level: ReviewThis PR addresses a Remote Code Execution (RCE) vulnerability by adding multiple crucial security improvements to the server-side validation logic in JetFormBuilder. Key changes and positives:
The JS client code correctly sends the signature with the SSR validation request. Code quality, WPCS adherence, and sanitization have been observed well. The approach maintains backward compatibility as the signature usage is additive and only affects SSR validation. Recommendations:
Overall, this is a strong security patch appropriate for a critical vulnerability fix. Suggested changelog entry
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $body = $request->get_body_params(); | ||
|
|
||
| // All security validation is centralized in Validation_Handler::validate() | ||
| $result = Validation_Handler::validate( $body ); |
There was a problem hiding this comment.
run_callback() now forwards only $request->get_body_params() into Validation_Handler::validate(). This drops REST request file params (e.g. uploaded files included in FormData) and any parser logic depending on $request->get_file_params() will no longer see them. Consider passing the original WP_REST_Request into Validation_Handler::validate() (or passing both body + file params) so SSR validation behaves the same across REST vs other transports.
| $body = $request->get_body_params(); | |
| // All security validation is centralized in Validation_Handler::validate() | |
| $result = Validation_Handler::validate( $body ); | |
| // All security validation is centralized in Validation_Handler::validate() | |
| $result = Validation_Handler::validate( $request ); |
|
|
||
| try { | ||
| $request = new \WP_REST_Request(); | ||
| $request->set_body_params( $body ); |
There was a problem hiding this comment.
Validation_Handler::validate() constructs a new WP_REST_Request and only sets body params. When called from the REST endpoint, this means uploaded file params from the original REST request are lost, but Rest_Validation_Endpoint::get_parser() reads files via $request->get_file_params(). Consider changing validate() to accept the original WP_REST_Request (or to accept both body params and file params) and forward file params into the request passed to get_parser_public().
| $request->set_body_params( $body ); | |
| $request->set_body_params( $body ); | |
| // Ensure uploaded files are available to the parser. | |
| if ( ! empty( $_FILES ) && is_array( $_FILES ) ) { | |
| $request->set_file_params( $_FILES ); | |
| } |
| // Security: Include signature for SSR validation | ||
| if ( this.attrs._sig ) { | ||
| formData.set( '_jfb_validation_sig', this.attrs._sig ); | ||
| } |
There was a problem hiding this comment.
With signature enforcement on the server, SSR validation depends on receiving the full _jfb_validation_path[] array (including repeater name). In admin_ajax mode the payload is built with Object.fromEntries(body), which collapses duplicate keys and will typically drop all but the last _jfb_validation_path[] entry—causing signature mismatches for repeater fields. Consider adjusting the admin-ajax payload serialization to preserve multi-value keys (e.g., manually collect _jfb_validation_path[] into an array before JSON encoding).
2 participants
…ssues/19124