Skip to content

v1.9.1#763

Merged
ilicfilip merged 7 commits into
mainfrom
filip/sanitize-recommendations-title
May 29, 2026
Merged

v1.9.1#763
ilicfilip merged 7 commits into
mainfrom
filip/sanitize-recommendations-title

Conversation

@ilicfilip
Copy link
Copy Markdown
Collaborator

No description provided.

ilicfilip added 2 commits May 26, 2026 11:36
An authenticated Editor (or higher) could create a recommendation via
POST /wp/v2/prpl_recommendations with an HTML payload in the `title`
field (e.g. `<img src=x onerror=alert(1)>`). The dashboard JS template
(views/js-templates/suggested-task.html) renders `title.rendered` with
Underscore's unescaped `{{{ }}}` syntax, so the payload executed when an
admin loaded the dashboard.

Defense in depth:

- Input: add a `rest_pre_insert_prpl_recommendations` filter that strips
  tags from `post_title` on every REST insert/update, regardless of the
  user's `unfiltered_html` capability. Recommendation titles are plain
  text, so this neutralizes the payload at the source.
- Output (JS): route the two raw `{{{ }}}` title sinks through a new
  `prplSuggestedTask.sanitizeTitle()` helper, which inert-parses the
  value with DOMParser (no script/resource side effects) and re-escapes
  it, preserving legitimate entities like `&amp;` without double-encoding
  the server-side `esc_html`'d provider titles.
- Output (admin bar): the PRPL debug tool printed `post_title` unescaped
  into a `WP_Admin_Bar` node id (an HTML attribute) and title (rendered
  as raw HTML), firing the payload on every admin page in debug mode.
  Escape the title with `esc_html()`, use the post ID for the node id,
  and escape the activities node title too.
- Also switch `updateTaskTitle` to set `.textContent` instead of
  `.innerHTML` for the screen-reader label, closing a self-XSS sink.

Adds tests/phpunit/test-class-rest-recommendations-xss.php covering
Editor and Administrator payloads plus a plain-text regression check.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 26, 2026

Test on Playground
Test this pull request on the Playground
or download the zip

@tacoverdo
Copy link
Copy Markdown
Contributor

tacoverdo commented May 26, 2026

When trying to reproduce the bug and fix, I ran into the following:

Update_191::sanitize_recommendation_titles() calls wp_update_post() with the stripped title. When the original was pure markup (e.g., ), wp_strip_all_tags() returns ''. If the post also has
empty content + excerpt, WordPress rejects the update with empty_content. update_recommendation() returns false, the migration silently moves on, and the malicious row stays in the DB — still exploitable
when rendered.

Repro on the PR branch:

wp post create --post_type=prpl_recommendations --post_title='<img src=x onerror=alert(1)>' --user=1
  wp post term set <ID> prpl_recommendations_provider <non-user-term> --by=id
  wp option delete progress_planner_version
  # trigger migration (e.g., load any admin page)
  wp eval 'echo get_post(<ID>)->post_title;'   # still raw markup

Suggested fix (in order of preference):

  1. Trash the row when sanitized title is empty — the recommendation was junk anyway:>
if ( '' === $sanitized ) {
     wp_delete_post( $recommendation->ID, true );
     continue;
 }
  1. Direct $wpdb update to bypass WP's empty-content guard:
$wpdb->update( $wpdb->posts, [ 'post_title' => $sanitized ], [ 'ID' => $recommendation->ID ] );
clean_post_cache( $recommendation->ID );
  1. Placeholder fallback — preserves the row but with a safe stub.

Also worth flagging: rest_pre_insert_prpl_recommendations only covers REST writes. wp_insert_post/CLI/programmatic creates bypass it. Moving the sanitizer to wp_insert_post_data (or
save_post_prpl_recommendations) would close that gap too — which is the same WP-CLI bypass we used to plant post 262 in the first place.

A title that is pure markup strips to an empty string. wp_update_post()
rejects an update that would leave the title, content, and excerpt all
empty, so the malicious title was left in the DB. The plugin never stores
title-less recommendations, so delete such rows instead.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@tacoverdo
Copy link
Copy Markdown
Contributor

Migration fix verified — but write-side bypass is still open

What works now

Migration empty-content bug is fixed. Existing title-only payloads (e.g. <img src=x onerror=alert(1)>) are now correctly removed:

wp eval '(new Progress_Planner\Update\Update_191())->run();'
wp eval 'echo (get_post(262) ? get_post(262)->post_title : "DELETED");'
→ DELETED

What's still broken: WP-CLI / wp_insert_post bypass

rest_pre_insert_prpl_recommendations only fires for REST API inserts. Any call to wp_insert_post() — WP-CLI, custom plugins, scheduled events, programmatic recommendation creation, third-party integrations — bypasses the sanitizer entirely.

Reproduction on the patched branch:

wp post create --post_type=prpl_recommendations \
  --post_title='<img src=x onerror=alert("post-patch-cli")>' \
  --post_status=publish --user=1
# → Created post 286
wp post term set 286 prpl_recommendations_provider 8 --by=id
wp cache flush
wp eval 'echo get_post(286)->post_title;'
# → <img src=x onerror=alert("post-patch-cli")>   (raw, not sanitized)

Loading /wp-admin/admin.php?page=progress-planner&prpl_show_all_recommendations= fires the alert via views/js-templates/suggested-task.html:18 ({{{ data.post.title.rendered }}}).

The <script>…</script> payload doesn't trigger because browsers don't execute <script> inserted via innerHTML (which is what Underscore's {{{ }}} does). Event-handler payloads (<img onerror>, <svg onload>, <iframe srcdoc>) do trigger — so <script> tests give a false sense of safety.

Suggested fix

Move the sanitizer from rest_pre_insert_prpl_recommendations (REST-only) to one of the lower-level chokepoints that catches every write path:

  • wp_insert_post_data (filter) — runs on all inserts/updates, lets you mutate the sanitized data array before it hits the DB.
  • save_post_prpl_recommendations (action) — fires after save, requires a follow-up wp_update_post if you need to rewrite.

wp_insert_post_data is cleaner — single chokepoint, no recursion concerns:

add_filter( 'wp_insert_post_data', function ( $data, $postarr ) {
    if ( 'prpl_recommendations' !== ( $data['post_type'] ?? '' ) ) {
        return $data;
    }
    $data['post_title'] = sanitize_text_field( wp_strip_all_tags( $data['post_title'] ) );
    return $data;
}, 10, 2 );

This closes both the REST hole and the CLI/programmatic hole with one filter, and obsoletes the existing rest_pre_insert_* hook.

Test coverage gap (still applies)

The current PHPUnit tests plant rows via $wpdb->update after creation, which bypasses all WP filters — so they validate the migration but don't exercise the write-side sanitizer. Adding tests that go through wp_insert_post() directly would catch this regression and any future ones.

@ilicfilip
Copy link
Copy Markdown
Collaborator Author

@tacoverdo , we discussed this yesterday - are we good?

@ilicfilip ilicfilip merged commit adf51ac into main May 29, 2026
28 checks passed
@ilicfilip ilicfilip deleted the filip/sanitize-recommendations-title branch May 29, 2026 12:27
ilicfilip added a commit that referenced this pull request May 29, 2026
* Fix PHPStan errors and phpunit CVE on main

Brings main's static analysis and dependency security checks back to green:

- Static Analysis: clear 25 pre-existing PHPStan errors. Ports develop's
  typed @return on Date::get_periods()/get_range() (which also resolves the
  Chart modify() errors), takes develop's exact versions of class-page-settings,
  class-activity-scores, class-chart and class-update-140, converts the WP-core
  require_once ignores to the @phpstan-ignore-next-line form that suppresses
  under PHPStan 2.1.x, and adds inline ignores elsewhere.
- Security check: bump phpunit/phpunit 9.6.30 -> 9.6.34 in composer.lock to
  resolve CVE-2026-24765 (unsafe deserialization in PHPT code coverage).

* Fix abstract method fatal in test-class-security.php

The anonymous classes extending the abstract Tasks_Interactive did not
implement the abstract Tasks::should_add_task() method. phpunit 9.6.30
did not surface this, but 9.6.34 (the CVE-2026-24765 fix) does, causing
a fatal when the test class loads. Implement should_add_task() in all 8
anonymous task providers.

* v1.9.1 (#763)

* Sanitize and escape prpl_recommendations title

An authenticated Editor (or higher) could create a recommendation via
POST /wp/v2/prpl_recommendations with an HTML payload in the `title`
field (e.g. `<img src=x onerror=alert(1)>`). The dashboard JS template
(views/js-templates/suggested-task.html) renders `title.rendered` with
Underscore's unescaped `{{{ }}}` syntax, so the payload executed when an
admin loaded the dashboard.

Defense in depth:

- Input: add a `rest_pre_insert_prpl_recommendations` filter that strips
  tags from `post_title` on every REST insert/update, regardless of the
  user's `unfiltered_html` capability. Recommendation titles are plain
  text, so this neutralizes the payload at the source.
- Output (JS): route the two raw `{{{ }}}` title sinks through a new
  `prplSuggestedTask.sanitizeTitle()` helper, which inert-parses the
  value with DOMParser (no script/resource side effects) and re-escapes
  it, preserving legitimate entities like `&amp;` without double-encoding
  the server-side `esc_html`'d provider titles.
- Output (admin bar): the PRPL debug tool printed `post_title` unescaped
  into a `WP_Admin_Bar` node id (an HTML attribute) and title (rendered
  as raw HTML), firing the payload on every admin page in debug mode.
  Escape the title with `esc_html()`, use the post ID for the node id,
  and escape the activities node title too.
- Also switch `updateTaskTitle` to set `.textContent` instead of
  `.innerHTML` for the screen-reader label, closing a self-XSS sink.

Adds tests/phpunit/test-class-rest-recommendations-xss.php covering
Editor and Administrator payloads plus a plain-text regression check.

* Bump version to 1.9.1

* add migration script and revert JS title escaping

* add inline comment, cc @tacoverdo

* Delete recommendation when sanitized title is empty

A title that is pure markup strips to an empty string. wp_update_post()
rejects an update that would leave the title, content, and excerpt all
empty, so the malicious title was left in the DB. The plugin never stores
title-less recommendations, so delete such rows instead.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* update readme.txt

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Fix plain-text title test to pass on multisite

On multisite, editors lack the unfiltered_html capability, so core's kses
encodes the ampersand in the test title and the byte-for-byte assertion
fails. Grant the capability (via super admin on multisite) so the test
isolates our XSS sanitization rather than core's kses behavior.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Grant unfiltered_html before switching user in title test

kses_init() runs on the set_current_user hook and decides whether to
attach the kses filters at switch time. The capability must be granted
before wp_set_current_user(), otherwise the filters are already attached
and the multisite assertion still sees the ampersand encoded.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Bump composer/composer 2.9.2 -> 2.10.0 to clear dev-dependency CVEs

Resolves the Security check failure: composer/composer 2.9.2 (pulled in
transitively via wp-cli/wp-cli-bundle in require-dev) carried CVE-2026-40176,
CVE-2026-40261, and CVE-2026-45793. Targeted `composer update composer/composer
--with-dependencies`; composer.json (runtime deps) unchanged. `composer audit`
now reports no advisories.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Joost de Valk <joost@altha.nl>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants