Document proper use of RawHTML, escape-html, decodeEntities, etc

[iandunn]

XSS vulnerabilities are very common in plugins, but there's almost no documentation about how to protect against them when building blocks. RawHTML is only mentioned in passing, and decodeEntities isn't mentioned at all.

I think it's important to provide guidance on when to use each of those, how to use them (with examples), and also when not to use them.

It's true that in the context of an editor, protection may not be needed as often, but I think that makes it even more important to have good documentation. Otherwise, devs will be confused about when it's needed, especially when many are already overwhelmed by all the unfamiliar things they're learning right now.

[iandunn]

Something overlooked in the original discussion is that plugin devs are building React apps outside of the editor, but which use Gutenberg's abstraction layer and components (for compatibility, future-proofness, visual consistency, etc). In those cases, it's very important for us to understand that RawHTML is not a safe way to output user input.

Consider this example with internationalized strings that contain necessary HTML:

<p id="qni-instructions">
	<RawHTML>
		{ sprintf(
			__( 'Use the <code>%1$s</code> and <code>%2$s</code> keys to navigate, and press <code>%3$s</code> to open a link.', 'quick-navigation-interface' ),
			shortcuts.previousLink.label,
			shortcuts.nextLink.label,
			shortcuts.openLink.label
		) }
	</RawHTML>
</p>

RawHTML is needed to prevent React from escaping the <code> tags, but that creates a situation where untrusted user input (the localized version of the string) is output unescaped.

Security best practices and React's own philosophy states that, "it should be 'easy' to make things safe, and developers should explicitly state their intent when performing 'unsafe' operations. The prop name dangerouslySetInnerHTML is intentionally chosen to be frightening...".

I think it should be renamed back to DangerousHTML (or perhaps something more descriptive, like DangerousUnescapedHTML), and its docs should describe the situations where it's safe to use, and where it isn't.

I feel like there also needs to be an officially recommended/documented way to handle examples like the above. Perhaps that's using DomPurify? HTML within localized strings feels like such a common situation (including within Core) that we might want to provide a core wrapper around it, though, similar to what we do with Moment.js.

cc @aduth , @mtias

[aduth]

Security best practices and React's own philosophy states that, "it should be 'easy' to make things safe, and developers should explicitly state their intent when performing 'unsafe' operations. The prop name dangerouslySetInnerHTML is intentionally chosen to be frightening...".

I think it should be renamed back to DangerousHTML (or perhaps something more descriptive, like DangerousUnescapedHTML), and its docs should describe the situations where it's safe to use, and where it isn't.

Having proposed the name originally, I certainly understand and agree with this sentiment. And on reflection, I'm not sure RawHTML was a great choice of name. That being said, in consideration of backwards-compatibility, "renaming" is not possible at this point, strictly speaking.

I feel like there also needs to be an officially recommended/documented way to handle examples like the above.

This may be the more important point: Developers shouldn't find themselves reaching for a tool like RawHTML. Where they are with frequency, we should have some alternative recommendations to suit.

The specific example you provide is acknowledged as a required feature for internationalization, tracked at #9846 with some discussion there for how it might be implemented without needing RawHTML.

[iandunn]

Having proposed the name originally, I certainly understand and agree with this sentiment. And on reflection, I'm not sure RawHTML was a great choice of name. That being said, in consideration of backwards-compatibility, "renaming" is not possible at this point, strictly speaking.

Yeah, but it can become a wrapper for <DangerousUnescapedHtml> in the next version, and all of the official docs could be updated to use <DangerousUnescapedHtml>. In the version after that, a deprecation warning can be triggered for devs, letting them know to update their code.

The sooner we do that, the fewer devs will be impacted.

The specific example you provide is acknowledged as a required feature for internationalization, tracked at #9846 with some discussion there for how it might be implemented without needing RawHTML.

Thanks! I missed that.

This may be the more important point: Developers shouldn't find themselves reaching for a tool like RawHTML. Where they are with frequency, we should have some alternative recommendations to suit.

I agree, but I don't think the two are mutually exclusive, and we'll never be able to provide alternatives for all the the times when people need <RawHTML>, so I think it still needs to be correctly named and properly documented.

[talldan]

Removing 'Needs Technical Feedback' as this seems to have already been discussed.

[DaisyOlsen]

Is this still valid? Is this something we should work to document in the Block Editor Handbook? I wasn't able to find anything about sanitization, XSS, etc in that handbook, only the general security section in the common APIs handbook