Comments: Allow the Notes @mention chip markup in comment content (kses)#12503
Comments: Allow the Notes @mention chip markup in comment content (kses)#12503adamsilverstein wants to merge 18 commits into
Conversation
The notes @-mention completer stores a mention as `<a class="wp-note-mention" data-user-id="N" href="...">@name</a>`. The default comment kses allowlist only keeps `href` and `title` on links, so for users without `unfiltered_html` the attributes that make a mention a mention (the chip class and the mentioned user's ID) are stripped on save. Add a 'pre_comment_content' context to wp_kses_allowed_html() that allows `class` and `data-user-id` on links so saved mentions survive sanitization. Both attributes are inert markup. Backports the PHP changes from the Gutenberg mentions PR: WordPress/gutenberg#79604
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
Allowing class and data-user-id on links in the pre_comment_content context loosened sanitization for every comment, including anonymous front-end comments. Both attributes are CSS/JS selector hooks, so that would let any commenter publish links styled by theme classes or reachable by delegated script handlers. Attach the extended allowlist in wp_filter_comment() only while a note comment's content is being filtered instead. Notes can only be written by logged-in users who can edit the post and never render on the front end, and the sanitization of regular comments is unchanged.
Design review on the Gutenberg side changed the stored mention from a link to a plain span, since a mention marks a person rather than offering navigation. Allow span.class and span.data-user-id instead of the link attributes; the allowance is still attached only while a note comment is filtered.
… attribute The notes mention completer now stores a mention as `<span class="wp-note-mention user-N">@name</span>`, so the kses allowance for note comments shrinks to the single `class` attribute and no longer needs `data-user-id`. Add a test asserting that any other attribute is still stripped from note spans. Trac ticket: https://core.trac.wordpress.org/ticket/65622
1fc1250 to
f74c68b
Compare
Mentions are now inserted as links to the mentioned user's author page - <a class="wp-note-mention user-N" href="..."> - instead of spans, so the note allowance moves from span.class to a.class. Note content now also picks up wp_rel_ugc()'s rel="nofollow ugc" like any other comment link, which the tests cover with a deterministic external href.
|
Sorry, only have time for a surface review.
I think the general API for managing email (or other) notifications would be better for the project than handling each case individually. |
dc57c7f to
e31873f
Compare
Reducing this to the kses only changes which matches the merged PRs. Opening a follow up for the notifications part to dig deeper. |
|
Follow up: #12548 |
Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>
westonruter
left a comment
There was a problem hiding this comment.
I wonder, instead of relying on classes, would it have been better to use a data attribute instead? It could be like data-wp-note-mention-user="N". This would be less of a "blast radius" for allowing class names on notes, since in the admin allowing any class could produce undesirable results. Like someone could craft a link that is using some class that makes it look weird or maybe something malicious. In contrast, if a data attribute were used, it would be allowed by Kses by default so there would be nothing special needed. This entire PR wouldn't be necessary then.
Hmmm, thats a great suggestion and we can still change this in beta since we just introduced this feature. When we added this originally some commits explored other attributes to use that would already be accepted by kses to avoid having to add this increased permission. I think the PR went with classes in part because it offers styling for free and also... a data attribute may not have been considered. Let me explore that suggested in a Gutenberg PR and maybe we can eliminate the need for this PR entirely as you suggested. We could still add styles dynamically if we need them to style the mentions (admin or front end). |
|
Left to comment in WordPress/gutenberg#80496 (comment). It's not hard to switch between implementation details, but unless we have clear goals: code, UI, UX, we might end up going in circles. |
Match the reworked Gutenberg approach (WordPress/gutenberg#80528): mentions are span.wp-note-mention.user-N chips rather than links, so the allowance becomes two always-on filters - span.class in the comment kses context plus a post-kses pass reducing span classes to the two mention tokens - and the per-note arming inside wp_filter_comment() is no longer needed.
|
Updated in 69ec26b to match the approach that landed from the discussion here and on gutenberg#80496 / gutenberg#80528:
Local runs: kses 366/366, comment group 582/582, REST comments controller 200/200. |
There was a problem hiding this comment.
Pull request overview
This PR updates WordPress core comment KSES handling to allow the Notes @mention “chip” markup (<span class="wp-note-mention user-N">…</span>) to persist in comment content while keeping the allowance inert by stripping all non-mention class tokens immediately after KSES runs.
Changes:
- Extend the
pre_comment_contentKSES allowlist to permitspanwith aclassattribute. - Add a post-KSES sanitization pass that reduces all
spanclass tokens to onlywp-note-mentionanduser-N. - Add PHPUnit coverage validating the allowlist change, class-token reduction behavior, and non-expansion to other elements/attributes.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/wp-includes/kses.php |
Adds the narrow allowlist extension for span[class] and the companion sanitizer that strips all non-mention class tokens from spans. |
src/wp-includes/default-filters.php |
Wires the new allowlist and sanitizer into core defaults (wp_kses_allowed_html and pre_comment_content). |
tests/phpunit/tests/kses.php |
Adds unit tests ensuring the allowance is minimal and that mention markup survives/comment content stays appropriately sanitized. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The HTML API leaves the removed attribute's surrounding whitespace | ||
| // in place, hence `<span >`. | ||
| $this->assertSame( | ||
| 'Hello <span >there</span>!', | ||
| wp_unslash( $filtered['comment_content'] ), | ||
| 'A span with no valid mention tokens should lose its class attribute entirely.' | ||
| ); |
The HTML API's whitespace handling when removing the final attribute is not part of its contract, so asserting the exact '<span >' spacing is brittle.
|
Copilot's whitespace-brittleness point on the |
| $unslashed = wp_unslash( $content ); | ||
|
|
||
| if ( ! str_contains( $unslashed, '<span' ) ) { | ||
| return $content; | ||
| } |
There was a problem hiding this comment.
cf. https://github.com/WordPress/gutenberg/pull/80528/changes#r3624075789
Let's remove the if entirely.
| $content = 'Hello <span class="wp-note-mention user-2 is-destructive components-button">@admin</span>!'; | ||
| $filtered = wp_filter_comment( wp_slash( $this->get_mention_commentdata( 'note', $content ) ) ); | ||
|
|
||
| remove_filter( 'pre_comment_content', 'wp_filter_kses' ); | ||
|
|
||
| $this->assertSame( | ||
| 'Hello <span class="wp-note-mention user-2">@admin</span>!', | ||
| wp_unslash( $filtered['comment_content'] ), | ||
| 'Class tokens beyond `wp-note-mention` and `user-N` should be stripped from spans.' | ||
| ); |
Match westonruter's review on WordPress/gutenberg#80528: type the kses filter and sanitizer signatures, and drop the try/finally filter restoration in the test since the framework restores filters after each test.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/wp-includes/kses.php:1207
- The early bailout
str_contains( $unslashed, '<span' )is case-sensitive. A commenter could use<SPAN class="...">so this function returns early and never strips class tokens, leaving an open-endedclassattribute on spans (sinceclassis now allowed through KSES). Removing this check avoids the bypass and matches the intended “always reduce span classes” behavior.
$unslashed = wp_unslash( $content );
if ( ! str_contains( $unslashed, '<span' ) ) {
return $content;
}
Backports the mention kses allowance from the Gutenberg Notes @mention work:
What
The Notes
@mention completer stores a mention as a non-interactive chip carrying the mentioned user's ID in a class token:This PR allows exactly that markup - and nothing more - through comment kses, via two small always-on filters (hooked in
default-filters.php):_wp_kses_allow_note_mention_span()(new, private) extends thepre_comment_contentallowlist withclassonspanelements._wp_kses_sanitize_note_mention_classes()(new, private) runs onpre_comment_contentat priority 11 - right afterwp_filter_kses- and uses the HTML API (class_list()/remove_class()) to strip every span class token exceptwp-note-mentionanduser-N. It bails whilewp_filter_ksesis not attached, so users withunfiltered_html(filtered throughwp_filter_post_kses, where arbitrary classes are already permitted) are never narrowed below what core allows them.wp_filter_comment()is untouched: no per-comment-type arming or disarming of kses state.Why
This follows the discussion on this PR and on WordPress/gutenberg#80496:
classon note links is a live CSS/JS selector surface in the admin. His data-attribute suggestion turned out to need a kses allowance anyway (data-*is not allowed by default in the comment context), and per @Mamaduka's review, an anchor-based mention breaks in the Link format UI regardless of how the ID is carried - mentions aren't links, they're tokens rendered as chips.classhole a different way: the allowance is unconditional yet reduced to exactly two inert tokens on a semantics-free element, so there is nothing left to scope per comment type and the stateful arm/disarm machinery disappears.Behavior change: all commenters (including anonymous ones) can persist
<span>elements whoseclassis limited towp-note-mentionand/oruser-N. This is inert - the tokens are not styling or scripting hooks outside the notes sidebar, and mention notification parsing (#12548) only processesnote-type comments.Testing
Unit tests in
tests/phpunit/tests/kses.php:test_wp_kses_allowed_html_pre_comment_content_allows_only_the_mention_spanverifies the context gains the span allowance and nothing else beyond the stock$allowedtags.test_note_mention_markup_survives_note_content_sanitization/test_note_mention_markup_survives_regular_comment_content_sanitizationverify the chip survives for both comment types.test_note_mention_span_classes_are_reduced_to_the_mention_tokensverifies junk class tokens are stripped.test_note_mention_class_attribute_removed_when_no_tokens_remainverifies theclassattribute is dropped entirely when no valid tokens remain.test_note_mention_allows_only_class_on_mention_spansverifies every other span attribute (onclick,style,data-*,id) is still stripped.test_class_is_still_stripped_from_links_in_comment_contentverifies links keep the default sanitization.test_note_mention_class_reduction_skipped_when_restrictive_kses_is_inactiveverifies the reduction never narrowsunfiltered_htmlusers.Trac ticket: https://core.trac.wordpress.org/ticket/65622
Proposed commit message