Skip escaping for data URI in WP_HTML_Tag_Processor #7114
Conversation
|
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. |
|
Hi @amitraj2203! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
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. |
Co-authored-by: Mukesh Panchal <mukeshpanchal27@users.noreply.github.com>
|
Hi @dmsnell Can you please review this PR. |
There was a problem hiding this comment.
Thanks for the patch, @amitraj2203.
I think for now we're not sure we want to skip all escaping of data URIs, but rather want to indicate that the update failed when WordPress rejects it.
That would be something more like checking if the $escaped_new_value is an empty string, and returning false in those cases.
Eventually we shouldn't be doing any sanitization in the HTML API directly, particularly because of issues like these, and because it pollutes the parsing logic with lots of conditional role-based logic, but esc_url() was an unfortunate necessity at the time.
That is, could we update this for now to simply indicate if WordPress has rejected the update?
… fix/handle-data-uris-in-set-attribute
…itraj2203/wordpress-develop into fix/handle-data-uris-in-set-attribute
|
Thanks for the review. I have made some changes. Could you please take another look? |
|
|
||
| if ( '' === $escaped_new_value ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
this looks almost complete! what happens if someone supplies '' as the new value? Maybe we need to check to make sure that $escaped_new_value !== $value && '' === $escaped_new_value?
There was a problem hiding this comment.
Thanks for pointing it out. I have updated it.
…s empty and the original value is not empty.
There was a problem hiding this comment.
Thanks again @amitraj2203!
Everything looks and tests well now. I'll make sure this gets in.
When setting an an attribute value in the HTML API, WordPress may reject an update based on rules in `kses`. In these cases, the return value from an escaping function will be an empty string, and the HTML API should reject the update. Unfortunately, it currently reports that it updates the attribute but sets an empty string value, which is misleading. In this patch, the HTML API will refuse the attribute update and return false to indicate as much when WordPress rejects the updates. Developed in #7114 Discussed in https://core.trac.wordpress.org/ticket/61719 Follow-up to [58472]. Props: amitraj2203, dmsnell, mukesh27. Fixes #61719. git-svn-id: https://develop.svn.wordpress.org/trunk@58844 602fd350-edb4-49c9-b593-d223f7449a82
When setting an an attribute value in the HTML API, WordPress may reject an update based on rules in `kses`. In these cases, the return value from an escaping function will be an empty string, and the HTML API should reject the update. Unfortunately, it currently reports that it updates the attribute but sets an empty string value, which is misleading. In this patch, the HTML API will refuse the attribute update and return false to indicate as much when WordPress rejects the updates. Developed in WordPress/wordpress-develop#7114 Discussed in https://core.trac.wordpress.org/ticket/61719 Follow-up to [58472]. Props: amitraj2203, dmsnell, mukesh27. Fixes #61719. Built from https://develop.svn.wordpress.org/trunk@58844 git-svn-id: http://core.svn.wordpress.org/trunk@58240 1a063a9b-81f0-0310-95a4-ce76da25c4cd
When setting an an attribute value in the HTML API, WordPress may reject an update based on rules in `kses`. In these cases, the return value from an escaping function will be an empty string, and the HTML API should reject the update. Unfortunately, it currently reports that it updates the attribute but sets an empty string value, which is misleading. In this patch, the HTML API will refuse the attribute update and return false to indicate as much when WordPress rejects the updates. Developed in WordPress/wordpress-develop#7114 Discussed in https://core.trac.wordpress.org/ticket/61719 Follow-up to [58472]. Props: amitraj2203, dmsnell, mukesh27. Fixes #61719. Built from https://develop.svn.wordpress.org/trunk@58844 git-svn-id: https://core.svn.wordpress.org/trunk@58240 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Trac ticket: https://core.trac.wordpress.org/ticket/61719
What:
This pull request updates the set_attribute method in WP_HTML_Tag_Processor to properly handle inline data URIs, such as SVGs.
Description
Previously, data URIs were altered or stripped due to the use of esc_url(), causing issues with attributes like src when using data URIs. This fix ensures data URIs are preserved as intended.