Skip to content

Fix: sanitize field_id input and escape rel attribute in oEmbed handler #1559

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jtsternberg merged 7 commits into from
Jun 4, 2026
Merged

Fix: sanitize field_id input and escape rel attribute in oEmbed handler #1559

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6d0f7d0
Fix: sanitize field_id input and escape rel attribute in oEmbed handler
thisismyurl Jun 1, 2026
7e91829
Fix: add is_string() guard before sanitize_text_field() on field_id
thisismyurl Jun 2, 2026
739949f
Test: assert field_id is escaped in oEmbed rel attribute
jtsternberg Jun 1, 2026
217c6f4
Fix: guard and sanitize $_REQUEST['object_id'] in oEmbed handler
jtsternberg Jun 4, 2026
c95d013
Refactor: wp_unslash() oembed_url before sanitize for consistency
jtsternberg Jun 4, 2026
05c3bef
Revert redundant is_string() guard on field_id; prove array-safety wi…
jtsternberg Jun 4, 2026
3a06cbf
Test: reset CMB2_Ajax singleton state in tear_down to prevent leak
jtsternberg Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Revert redundant is_string() guard on field_id; prove array-safety wi… 
…th a test

7e91829 added `&& is_string( $_REQUEST['field_id'] )` to guard against a claimed
PHP 8 TypeError from sanitize_text_field( wp_unslash( array ) ). That premise is
incorrect: WordPress's _sanitize_text_fields() returns '' for array/object input
(wp-includes/formatting.php), and wp_unslash() (stripslashes_deep → map_deep) is
array-safe. So the existing isset() guard already collapses an array field_id to
the empty-string default with no error, on every supported WP (the array/object
early-return has been in core since WP 4.7; CMB2 supports WP 6.3+).

Restore the clean isset() + sanitize_text_field( wp_unslash() ) pattern, keeping
field_id consistent with the object_id line.

Adds test_oembed_handler_handles_array_field_id, which drives oembed_handler()
with field_id = array(...) and asserts it returns success with an empty rel=""
(no PHP error). Verified green both with the guard present and after removing it,
proving the removal is behavior-preserving and pinning the behavior so the guard
is not reintroduced on the same false premise.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
  • Loading branch information
@jtsternberg @claude
jtsternberg and claude committed Jun 4, 2026
commit 05c3bef1fb2288697b24ac09315f0fa2960cd69e
2 changes: 1 addition & 1 deletion includes/CMB2_Ajax.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public function oembed_handler() {
'object_id' => isset( $_REQUEST['object_id'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['object_id'] ) ) : '',
'object_type' => isset( $_REQUEST['object_type'] ) ? $_REQUEST['object_type'] : 'post',
'oembed_args' => $embed_args,
'field_id' => isset( $_REQUEST['field_id'] ) && is_string( $_REQUEST['field_id'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['field_id'] ) ) : '',
'field_id' => isset( $_REQUEST['field_id'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['field_id'] ) ) : '',

@gemini-code-assist gemini-code-assist Bot Jun 1, 2026 •
edited by jtsternberg
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If $_REQUEST['field_id'] is passed as an array (e.g., ?field_id[]=value), passing it directly to wp_unslash() and sanitize_text_field() will trigger a PHP warning or a TypeError on PHP 8.x because sanitize_text_field() expects a string.

To ensure robustness and prevent potential crashes on PHP 8.x, verify that the input is a string using is_string() before sanitizing.

			'field_id'    => ! empty( $_REQUEST['field_id'] ) && is_string( $_REQUEST['field_id'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['field_id'] ) ) : '',

) );

wp_send_json_success( $html );
Expand Down
40 changes: 40 additions & 0 deletions tests/test-cmb-ajax.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,46 @@ public function test_oembed_handler_tolerates_missing_object_id() {
$this->assertTrue( ! empty( $decoded['success'] ), 'Handler should return a successful JSON response.' );
}

/**
* A field_id submitted as an array (?field_id[]=x) must be handled safely.
* WordPress's sanitize_text_field() returns '' for array/object input (and
* wp_unslash() is array-safe), so sanitize_text_field( wp_unslash( array ) )
* yields '' with no PHP error — no is_string() guard is required. This locks
* that behavior so the guard is not reintroduced on a false TypeError premise.
*
* @group cmb2-ajax-embed
*/
public function test_oembed_handler_handles_array_field_id() {
add_filter( 'wp_doing_ajax', '__return_true' );
add_filter( 'wp_die_ajax_handler', array( $this, 'throw_oembed_die_handler' ), 99 );

$_REQUEST['cmb2_ajax_nonce'] = wp_create_nonce( 'ajax_nonce' );
$_REQUEST['oembed_url'] = $this->oembed_args['url'];
$_REQUEST['object_id'] = 'options-page-id';
$_REQUEST['field_id'] = array( 'unexpected', 'array' ); // The odd input under test.

$reached_send_json = false;
$json = '';

ob_start();
try {
cmb2_ajax()->oembed_handler();
} catch ( CMB2_Test_Oembed_Die $e ) {
$reached_send_json = true;
$json = ob_get_contents();
} finally {
ob_end_clean();
unset( $_REQUEST['cmb2_ajax_nonce'], $_REQUEST['oembed_url'], $_REQUEST['object_id'], $_REQUEST['field_id'] );
}

$this->assertTrue( $reached_send_json, 'oembed_handler() should handle an array field_id without a PHP error.' );

$decoded = json_decode( $json, true );
$this->assertTrue( ! empty( $decoded['success'] ), 'Handler should return a successful JSON response.' );
// The array field_id collapses to '' and is escaped into an empty rel attribute.
$this->assertStringContainsString( 'rel=""', $decoded['data'] );
}

/**
* Returns a wp_die handler (for the wp_die_ajax_handler filter) that throws a
* marker exception, letting the test observe that wp_die() was reached.
Expand Down