React: Rename dangerouslySetInnerHTML

[syranide]

From #1515, thoughts?

dangerousInnerHTML, unsafeInnerHTML, (disasterousInnerHTML :)) or something else? (I prefer the sound of dangerous)

It also makes sense to add a dangerousDefaultInnerHTML (default is the word we use elsewhere) which could be very useful for content-editables to avoid "hacks", etc.

cc @sebmarkbage @zpao

[chenglou]

dangerous! _bikesheds_

[sophiebits]

I don't mind dangerouslySetInnerHTML but out of the options you listed I prefer dangerousInnerHTML.

[rymohr]

How about rawHTML and react stops trying to scare developers with its api? There are valid (and safe) reasons to use this.

Plus raw is already in use in rails: http://apidock.com/rails/ActionView/Helpers/OutputSafetyHelper/raw

[syranide]

@rymohr The reason for dangerous _is_ to scare people into actually thinking very carefully before using it AFAIK. It _is_ dangerous to use it, but dangerous does not exclude safe or useful.

If the HTML is in any way invalid then you risk breaking the entire app, if the HTML also contains anything user-supplied you've very likely enabled a very serious security issue. Personally I think that means dangerous is more than justified.

Also, here's the justification for {html: ...}: #2256

[rymohr]

It's not dangerous though if you're passing the html (user generated or markup generated) through a sanitizer first. The sanitizer I wrote both sanitizes the html and makes sure it's well formed.

I understand the operation _could_ be dangerous, but it's better to warn the developer in a way that doesn't make the library awkward to use. Why not just log a warning message to the console each time it is used in development mode instead?

[alanhogan]

It _does_ make sense to rename this property to something that doesn’t sound like a function.

⁂

It does _not_ make sense to drop the “scary” part of the name.

“It’s not dangerous if…”

If something can be dangerous depending on how it’s used, then… it’s dangerous. Matches are dangerous, even though they can be used safely. Knives. Ovens. My motorcycle.

Listen, I have a shameful and distant past as a PHP developer. PHP is absolutely chock-full of danger, and was even more dangerous in the past. Start with the fact that it outputs unescaped HTML by default, and the most standard-looking database library encourages users to naively interpolate strings into queries (hello, Bobby Tables). Now, concatenating strings into a query _is not dangerous if you have escaped or safely generated those strings_, but that doesn’t mean it isn’t dangerous, and it doesn’t mean that everyone writing that code knows that. When I taught myself PHP+MySQL from a printed book in early high school, I didn’t know about email header injection, and I was furious to find out that the code I wrote by following the book left my contact form open to be used for sending spam to arbitrary recipients!

Now, don’t think that I have conflated making the right thing easy (read: escape by default & well-designed APIs) with taking the additional precaution of warning users of dangerous code.

I am aware you are only debating the latter in this thread, and so am I. I think both are the right thing to do.

I think you may be exhibiting a common programmer belief that I think is dangerous: The idea that since every developer _should_ know X, Y, and Z before hacking in language/library/system A, that every developer working with A _does_ know X, Y, and Z. I assure you that assumption is not true, and there are astounding numbers of people — some call themselves developers, others don’t — who simply google for a solution and paste code into their app, tweaking it until it “works”. And for those people, they _should_ see the word “danger” in their code when they are pasting something that can open the only XSS hole in React. Any other attitude is setting those less experienced than us up for failure.

React team did a smart thing with this name, and deserve applause. Congrats — you are writing ethical software.

[rtfeldman]

Well said, @alanhogan! Completely agreed.

[alanhogan]

Looks like #2257 is the latest iteration of this issue

[rymohr]

I think dangerousInnerHTML is much better. I'm still not a fan of the {__html: ...} syntax though, even after reading @syranide's follow up in https://github.com/facebook/react/issues/2256#issuecomment-63031960

The dangerous prefix is the warning flag, regardless of where the actual harm is done. Most developers won't think twice about the __html part.

function createMarkup() { return {__html: getUsername()}; }
dangerousInnerHTML={createMarkup()}
// vs
dangerousInnerHTML={{__html: getUsername()}
// vs
dangerousInnerHTML={getUsername()}

Leading underscores usually mean you're working with a private api that might change without warning. Not that what you're working with is inherently dangerous.

[appsforartists]

I tend to agree with @rymohr: since the prop name is _danger_ouslySetInnerHTML, making the API itself awkward with __html is probably overkill. Anyone who forgets to consider XSS when setting dangerouslySetInnerHTML is not going to be dissuaded by the extra __html.

On the other hand, it does prevent you from passing in an object you don't control without transforming it first, so _maybe_ that's good?

[syranide]

@appsforartists {__html} is not intended as another "warning", it's meant to signify that a value is safe for use as HTML, using it inline as dangerouslySetInnerHTML={{__html: ...}} is NOT how it's intended. dangerouslySetInnerHTML={getHtmlValue()} is the intention, where it makes _a lot_ of sense as the burden is now on the function to guarantee that the value is safe for use as HTML.

[sophiebits]

Really the "dangerous" is in the wrong place and it should be innerHTML={{__dangerousHTML: ...}} so that when you write innerHTML={foo} it looks safe (and it is, as long as whoever wrote the __dangerousHTML key made sure that the HTML was safe).

[syranide]

@spicyj :+1: (I guess dangerous still alludes to it expecting a special wrapped value though)

[aweary]

No movement on this in the last couple years. I'm not sure it'd be worth the API churn, and it doesn't seem like a frequently-requested change. What do you think @gaearon @nhunzaker? If we wanted to make this change, it might be worth doing in the next major release.

[gaearon]

Let’s close for inactivity. We can come back to this if it organically comes up again but I don’t think this has a high priority.