[Tiny PR] Media Placeholder: Allow SVG as URI #18092

ellatrix · 2019-10-24T09:44:41Z

Description

Ok, ok, SVGs in WordPress is a controversial topic. They cannot be uploaded. And the way towards that seems long as there are a bunch of security issues with SVGs. See https://core.trac.wordpress.org/ticket/24251.

But what if we don't upload them? What if we just encode them and use them in a data URI? This is safe as any scripts that could be embedded in an SVG will not be executed. The SVG is not embedded in the page.

Wait? Aren't data URIs too big to store in the post content? Yes, base64 encoded text would be long, but this isn't necessary. We can just encode the text.

So imagine I upload something like:

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink"> 
<circle cx="50" cy="18.4" r="18.4" fill="#f00" /> 
<script> 
 alert( 'Hello there!' ); 
</script> 
</svg>

The result would be:

<img src="data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' xmlns:xlink='http://www.w3.org/1999/xlink'%3E%3Ccircle cx='50' cy='18.4' r='18.4' fill='%23f00' /%3E%3Cscript%3E alert( 'Hello there!' ); %3C/script%3E%3C/svg%3E" alt="" width="196" height="196"/>

How has this been tested?

Add an image block and click "Upload", then select and SVG.

Screenshots

Types of changes

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

jorgefilipecosta · 2019-10-24T21:00:06Z

packages/block-editor/src/components/media-placeholder/index.js

+
+			if ( file.type === 'image/svg+xml' ) {
+				file.text().then( ( source ) => {
+					this.props.onSelectURL(


Should we check if onSelectURL is available?

Oh, sure, I didn't know it's optional. :)

jorgefilipecosta · 2019-10-24T21:00:29Z

packages/block-editor/src/components/media-placeholder/index.js

@@ -130,8 +131,23 @@ export class MediaPlaceholder extends Component {
 				setMedia = onSelect;
 			}
 		} else {
+			const [ file ] = files;
+
+			if ( file.type === 'image/svg+xml' ) {


Should we limit this logic to files smaller than a given amount of bytes?

That sounds like a good idea. Like me check some browser limitations for data URIs.

gwwar · 2019-10-25T22:41:28Z

Interesting. This might work? I'm not sure how cautious we'll be on the security side of things. Note that this should only be set on the image tag. In other contexts <iframe>, <object> or <embed> can execute code. For example https://www.owasp.org/index.php/XSS_Filter_Evasion_Cheat_Sheet#You_can_EMBED_SVG_which_can_contain_your_XSS_vector

draganescu · 2019-11-19T13:14:06Z

I love this idea! Other than @jorgefilipecosta 's points above I think it would be a great addition to the image block 😄🎉

paaljoachim · 2021-02-10T13:30:34Z

It looks like this PR has gone stale.
It could use a status update.
Thanks!

jorgefilipecosta

There is one more check that I think we should do. Users without the ability to output unsafe HTML e.g: authors can not include code with an SVG so I guess in these cases our mechanism should do nothing.

kathrynwp · 2022-09-01T20:51:01Z

@jorgefilipecosta Hey there, just circling back to see whether this is likely to be picked up and merged, or whether it should be closed out at this stage. Let me know, thanks!

jorgefilipecosta · 2022-09-06T14:27:57Z

@jorgefilipecosta Hey there, just circling back to see whether this is likely to be picked up and merged, or whether it should be closed out at this stage. Let me know, thanks!

I think this PR is useful and could be merged. I guess we should just add a check for the capability to not have this functionality on users without permission to use unsafe HTML.

priethor · 2023-06-30T12:04:14Z

@ellatrix, is this PR still relevant? If so, maybe @jorgefilipecosta can wrap it once he comes back from AFK.

ellatrix · 2024-04-25T19:15:15Z

I'm not going to work on it, anyone should feel free to take it and start a new PR.

github-actions · 2024-04-25T19:15:30Z

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 props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @gwwar.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: gwwar.

Co-authored-by: ellatrix <ellatrix@git.wordpress.org>
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org>
Co-authored-by: draganescu <andraganescu@git.wordpress.org>
Co-authored-by: paaljoachim <paaljoachim@git.wordpress.org>
Co-authored-by: kathrynwp <zoonini@git.wordpress.org>
Co-authored-by: priethor <priethor@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

ellatrix requested review from talldan and youknowriad as code owners October 24, 2019 09:44

ellatrix added [Block] Image Affects the Image Block [Feature] Media Anything that impacts the experience of managing media [Type] Enhancement A suggestion for improvement. labels Oct 24, 2019

Media Placeholder: Allow SVG as URI

4f05f24

ellatrix force-pushed the try/insert-svg-as-uri branch from 0655024 to 4f05f24 Compare October 24, 2019 16:24

jorgefilipecosta reviewed Oct 24, 2019

View reviewed changes

paaljoachim added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Feb 10, 2021

Base automatically changed from master to trunk March 1, 2021 15:42

jorgefilipecosta reviewed Apr 21, 2021

View reviewed changes

ellatrix closed this Apr 25, 2024

ellatrix deleted the try/insert-svg-as-uri branch April 25, 2024 19:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tiny PR] Media Placeholder: Allow SVG as URI #18092

[Tiny PR] Media Placeholder: Allow SVG as URI #18092

ellatrix commented Oct 24, 2019

jorgefilipecosta Oct 24, 2019

ellatrix Oct 25, 2019

jorgefilipecosta Oct 24, 2019

ellatrix Oct 25, 2019

gwwar commented Oct 25, 2019 •

edited

draganescu commented Nov 19, 2019 •

edited

paaljoachim commented Feb 10, 2021

jorgefilipecosta left a comment

kathrynwp commented Sep 1, 2022

jorgefilipecosta commented Sep 6, 2022

priethor commented Jun 30, 2023

ellatrix commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

[Tiny PR] Media Placeholder: Allow SVG as URI #18092

[Tiny PR] Media Placeholder: Allow SVG as URI #18092

Conversation

ellatrix commented Oct 24, 2019

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

jorgefilipecosta Oct 24, 2019

Choose a reason for hiding this comment

ellatrix Oct 25, 2019

Choose a reason for hiding this comment

jorgefilipecosta Oct 24, 2019

Choose a reason for hiding this comment

ellatrix Oct 25, 2019

Choose a reason for hiding this comment

gwwar commented Oct 25, 2019 • edited

draganescu commented Nov 19, 2019 • edited

paaljoachim commented Feb 10, 2021

jorgefilipecosta left a comment

Choose a reason for hiding this comment

kathrynwp commented Sep 1, 2022

jorgefilipecosta commented Sep 6, 2022

priethor commented Jun 30, 2023

ellatrix commented Apr 25, 2024

github-actions bot commented Apr 25, 2024

Unlinked Accounts

gwwar commented Oct 25, 2019 •

edited

draganescu commented Nov 19, 2019 •

edited