Repository navigation
Fix: sanitize field_id input and escape rel attribute in oEmbed handler - #1559
Conversation
`$_REQUEST['field_id']` was passed to `get_oembed()` directly — no `isset()` guard (PHP notice if absent) and no sanitization. In `get_oembed()`, the value was concatenated into an HTML attribute (`rel="..."`) without `esc_attr()`, which is a WordPress Plugin Check violation and a potential reflected XSS vector in admin. Two-line fix: 1. Sanitize at collection: `isset()` guard + `sanitize_text_field( wp_unslash() )` 2. Escape at output: `esc_attr()` on the `rel` attribute value Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Security hardening for the CMB2 oEmbed AJAX handler by sanitizing input and escaping output for the field_id request parameter.
Changes:
- Sanitize
$_REQUEST['field_id']withsanitize_text_field( wp_unslash( ... ) )and default to an empty string when not set. - Escape
field_idwithesc_attr()when rendered into therelattribute of the remove-embed link.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves security and robustness in includes/CMB2_Ajax.php by sanitizing and escaping the field_id parameter. The review feedback correctly points out that $_REQUEST['field_id'] should be validated as a string before sanitization to prevent potential PHP warnings or TypeErrors on PHP 8.x if an array is provided.
| 'object_type' => isset( $_REQUEST['object_type'] ) ? $_REQUEST['object_type'] : 'post', | ||
| 'oembed_args' => $embed_args, | ||
| 'field_id' => $_REQUEST['field_id'], | ||
| 'field_id' => isset( $_REQUEST['field_id'] ) ? sanitize_text_field( wp_unslash( $_REQUEST['field_id'] ) ) : '', |
There was a problem hiding this comment.
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'] ) ) : '',
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
Address review feedback: if field_id is passed as an array (?field_id[]=value), sanitize_text_field( wp_unslash( array ) ) triggers a PHP 8 TypeError. Added is_string() guard so array input falls back to the empty-string default, matching the intent of the original isset() guard. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Locks in the esc_attr() output escaping added in this PR. Verified RED by
temporarily reverting the esc_attr() call — the raw, attribute-breaking
field_id ('evil" onmouseover="alert(1)') was reflected verbatim into rel="";
GREEN with the escaping in place. Runs in the default suite via get_oembed()
(not the excluded ajax group).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
object_id was read unguarded one line above the field_id fix, emitting an "Undefined array key" warning (PHP 8+) when absent — the same class of notice the field_id guard addresses. Mirror that pattern: isset() guard + sanitize_text_field( wp_unslash() ). TDD: added test_oembed_handler_tolerates_missing_object_id, which drives oembed_handler() with object_id absent (capturing wp_send_json's wp_die via a filtered handler). Verified RED first — the test errored with 'Undefined array key "object_id"' at CMB2_Ajax.php:94 — then GREEN with the guard in place. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Aligns the oembed_url input handling with the field_id/object_id pattern (wp_unslash before sanitize_text_field). Behavior-preserving: the value is immediately esc_url()'d, and esc_url() already strips backslashes, so unslashing first produces identical output — hence no dedicated test; the existing Test_CMB2_Ajax suite stays green (7 tests, 14 assertions). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…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) <[email protected]>
get_oembed()/oembed_handler() mutate the shared cmb2_ajax() singleton (ajax_update, hijack, object_id/type) and register hijack_oembed_cache_* metadata filters on it. Because it's a singleton, that state persisted across tests: the new oembed_handler() tests left ajax_update=true, which flipped the later network-dependent Test_CMB2_Types_Display::test_oembed onto its fallback branch (where a stale "no_connection" verifier then failed). Reset the singleton's mutable state and remove its hijack filters in tear_down, restoring constructor defaults between tests. Full suite green and stable across 3 consecutive runs (217 tests, 708 assertions). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
Pushed a few follow-up commits to the branch (TDD throughout — each behavior has a test that was watched fail before the fix): Reverted the
So Other commits:
Full PHPUnit suite green and stable (217 tests, 708 assertions). |
Props @thisismyurl. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Lead with user-facing summary + why (WPCS escaping flag) instead of listing internal Class::method() signatures, matching the #1559 sibling. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Hey folks,
Two related issues in
CMB2_Ajax::oembed_handler()andget_oembed():$_REQUEST['field_id']is accessed without anisset()guard (PHP notice if absent) and withoutsanitize_text_field()/wp_unslash()before passing it into the embed args array.In
get_oembed(), that value is concatenated directly into an HTMLrelattribute —rel="' . $oembed['args']['field_id'] . '"— withoutesc_attr(). WordPress Plugin Check flags this, and it's a reflected XSS vector for any admin with access to an oEmbed field.Two-line fix:
isset()+sanitize_text_field( wp_unslash() )at input,esc_attr()at therelattribute on output.(full disclosure, I used AI help with the testing and tweaks - Christopher)