Remove no-danger from recommended rules

[mjackson]

Using dangerouslySetInnerHTML is already very explicit and there are some things you actually can't do w/out it. I don't think I'd say that using it isn't "recommended". It's totally ok if you use it the way it's intended to be used.

[beefancohen]

Just out of curiosity, what can't you do without it?

I think it should be recommended, but maybe with a warning level of 1 and a clearer error message about why it is warning: Dangerous property \'{{name}}\' found: Improper use of the innerHTML can open you up to a cross-site scripting (XSS) attack.

[ljharb]

imo warnings aren't particularly useful - they just train people to ignore console output. Ideally things are either off or errors.

[beefancohen]

I see them more as an awareness feature - in this case "reconsider what
you're about to do, there's probably a better way to solve this"

In any case, I think improving the error message here is critical and in
@mjackson case you can always // eslint-disable-line

I doubt there are many use cases for needing dangerouslySetInnerHtml
as a solution, but correct me if I'm wrong

[mjackson]

Just out of curiosity, what can't you do without it?

Put simply, you can't tell React not to escape something. It's great to escape things by default, but the ability to opt-out of escaping when you need to is absolutely critical.

I've used dangerouslySetInnerHTML to render JavaScript strings. For example, you can't render this w/out it:

<script>console.log(true && 'hello')</script>

It's also needed when using 3rd-party DOM string manipulation libraries inside React, like Mustache (which already produces escaped output) or Markdown. When you render some HTML, you've gotta have a way to stick it in your component w/out further escaping it.

Also agree w @ljharb that warnings aren't useful. It's either an error or it's not, and this one is definitely not.

[ljharb]

When you say "render JS strings", do you mean, put an actual script block in the page, or show the literal text <script>blah</script>?

Is it that common to use "not jsx" inside React components?

[mjackson]

I might also point out that an example of using Markdown w React is found on the React homepage (see the last example).

[mjackson]

@ljharb I mean putting an actual <script> tag into the page.

Side note: we couldn't actually have this conversation here on GitHub unless there were a way for them to opt-out of escaping all the special HTML characters we're using ... :P

[ljharb]

The markdown/template approach is a good point, and this is one of our most common use cases for setting innerHTML. I can't believe that adding an actual script block to the page inside a React component is a common use case, and either way, I can't conceive of how it's not a horrific one.

At Airbnb we have a component that manually escapes safe HTML tags so that we don't need to set inner HTML, but so content with HTML can be used without exposing us to XSS. This seems like the usual solution for the template solution too (eg https://github.com/rexxars/react-markdown and similar approaches).

I don't have a strong opinion on whether it should be included in the recommended config or not - but I'm only partially convinced that it's a good thing to allow, primarily by the templating use case.

[mjackson]

I can't believe that adding an actual script block to the page inside a React component is a common use case, and either way, I can't conceive of how it's not a horrific one.

I never said it was common, but when I do use it I can do some really cool stuff:

https://github.com/mjackson/web-starter/blob/a8b425d85fdaa8000e719399ed8e23969f0f4c83/modules/server/components/HomePage.js#L27-L28

Anyway, my point about this conversation still holds. Somewhere, on some server, GitHub is "dangerously setting inner HTML" which is what's allowing us to have this conversation.

[ryanflorence]

I have a few thoughts

1. If there was never a use-case for rendering unescaped HTML then React wouldn't have allowed it in the first place.
2. historically every html-outputting (template) system has had this mechanism to render unescaped content, raw in rails IRB, {{{triple stash}}} in handlebars, ! in haml, etc.
3. Any app that has user generated content (Learning Management Systems, CRMs, basically anything with a WYSIWYG, this app right here in front of us--Github)
4. I've worked with several trusted APIs that send safe, raw HTML content (like the github API) that I need to render to the page as is.

I'd absolutely remove this from the recommended rules.

[developit]

Every use case for dangerouslySetInnerHTML can be achieved more safely by transforming the intended content to VDOM first and rendering it as children.

<script>{`console.log(true && 'hello')`}</script>

For markdown, convert md -> ast -> vdom, or md -> html -> vdom. Last step can be done easily using the native DOMParser, after which converting DOM to VDOM is cake.

Just my 2¢! 😊

[kentcdodds]

Just throwing my hat in there as another heavy user of dangerouslySetInnerHTML. https://javascriptair.com is a static site that's generated using React and Markdown. I use this API all over the place for that.

On the subject of recommended rules, I agree with @mjackson when he says:

Using dangerouslySetInnerHTML is already very explicit

Linting is generally used to help us avoid mistakes/typeos and guide us to using better "accepted" practices. In this case it's unlikely that you would use this API by mistake and I think that the name of this API is explicit enough to guide people to not use it if they don't have to.

Just my thoughts.

[developit]

(to Kent's point though, I have definitely seen a lot of people use dangerouslySetInnerHTML for server rendering use cases)

[mjackson]

Every use case for dangerouslySetInnerHTML can be achieved more safely by transforming the intended content to VDOM first and rendering it as children.

@developit You know I love you, but did you run that code?

renderToString(<script>console.log(true && "hello")</script>) => <script>console.log(true &amp;&amp; &#x27;hello&#x27;)</script>

:P

[ryanflorence]

converting DOM to VDOM is cake.

It's about a billion pounds more cake than just trusting developers to use an API with the word "dangerous" in the name correctly, and on the sever you don't have a browser environment to let you cheat on the parsing.

[developit]

I love cake 😄. I didn't mean to say it's always better to parse, just safer since it's an inactive DOM.

@mjackson - ah I totally misunderstood what you were getting at there, yes.

[ericclemmons]

Yet another user here with this issue. Everyone has already stated the case better than I.

(My specific scenario is rendering content from WordPress' API in React views on the server. These include all manners of tags, but it's trusted content despite the dangerous name)

[ljharb]

Recommended rules are supposed to cover the common use cases, not the uncommon ones. It's completely expected and acceptable that if you deviate from a linter rule for a good reason, you use an inline eslint comment to override it. The criteria for being a "recommended rule" is not "it's sometimes useful", it's "in the majority case, having the rule is more helpful than not".

Certainly the name "dangerous" makes it unlikely you'd use it by mistake, unless of course you copied and pasted a code example from somewhere on the internet - in which case the linter catching it, and requiring you to explicitly indicate that you did, in fact, intend to do that via an eslint comment - is a good thing.

To restate: "there's a use case" is utterly irrelevant when discussing whether something belongs in a recommended linter config or a style guide. The only thing that matters is "in the majority case, having the rule is more helpful than not."

[rileybracken]

I use it to render SVG's that are exported from AI or sketch. I can control the fill other CSS properties directly in my style sheets.

Here is an example.

const ACTIVITY_ICONS = {
  photography: "<svg>...</svg>",
  biking: "<svg>...</svg>",
  climbing: "<svg>...</svg>",
  backpacking: "<svg>...</svg>",
};

const createIcon = (svg) => ({ __html: svg });

const Activity = ({ name }) => ( 
  <div
    dangerouslySetInnerHTML={ createIcon(ACTIVITY_ICONS[name]) }
  />
);

<Activity name="backpacking" />

(I did this on my phone so I may have messed up somewhere)

[mjackson]

Having the no-danger rule as part of the "recommended" set is misleading, Jordan. You're telling people that using a sometimes-necessary piece of the API is not recommended. That makes no sense.

[ljharb]

Again, I don't feel strongly about this specific rule being or not being in the "recommended" set.

However, there's tons of "sometimes necessary" things that are generally discouraged. Sometimes being necessary in no way guarantees that a thing should be commonly recommended.

[mjackson]

Would you "generally discourage" making a website with user-generated content? Or "generally discourage" using Markdown? That's what makes no sense to me.

[iCodeForBananas]

I don't have a strong feeling about this but we use it four times in our application because we needed to interact with an API built for JS written years ago. We know we shouldn't use it and will remove it eventually.

[ryanflorence]

However, there's tons of "sometimes necessary" things that are generally discouraged.

Linting seems to exist to catch mistakes.

There are use cases for using set state in didUpdate, but there's a high chance people will be doing something weird, so that makes sense as a linting rule to require you to dance around.

It would be incredibly surprising to me if somebody accidentally rendered unescaped HTML with React. That's why I think it doesn't make sense to have it as a linting rule at all, let alone as a recommended one.

It's not catching mistakes, it's just a nuisance.

[mjackson]

We know we shouldn't use it

That's the sentiment I'm worried about. Is there another way to do what you want to do? Then yes, you should probably do that.

But there are times when you must use it.

[beefancohen]

Would you "generally discourage" making a website with user-generated content? Or "generally discourage" using Markdown? That's what makes no sense to me.

Recommending a rule isn't telling the consumer they have to use that rule. If her/his application needs that API to operate, they can easily turn off the linting rule. I also don't feel strongly about this, but I think this is super useful to newcomers to React who blindly consume the recommended ruleset of this plugin. I do, however, feel strongly about updating the error message to include why this may be a bad idea if there is a better way to solve her/his problem.

Regardless if this is recommended or not, I think the error message should include: Improper use of the innerHTML can open you up to a cross-site scripting (XSS) attack.

[erikmueller]

Totally agree with @mjackson regarding the explicit naming. We e.g. are in the process of rewriting parts of a legacy monolith to react. Naturally we have APIs returning old html content that needs to be rendered unescaped. I guess a lot of people (hopefully) start off by converting small parts of their existing architecture and are dealing with these kinds of "problems".

You're telling people that using a sometimes-necessary piece of the API is not recommended

true

If anyone insists what about dangerouslySetInnerHTMLWithPotentialXSS xD

[developit]

Someone suggested defaulting to a warn level, is that a happy medium here?

[yannickcr]

Well, dangerouslySetInnerHTML has indubitably some legitimate use cases and it's name by itself is already a warning for the users, so I am not against removing the rule from the recommended configuration.

Also, like @evcohen suggested, the current error message is not very helpful and can be greatly improved.

(NB: There is a similar issue about no-did-mount-set-state and no-did-update-set-state #596)

[mzabriskie]

Just adding my 2¢ to what has already been said. I use dangerouslySetInnerHTML for handling markdown content. I don't know of any other way to do this shy of doing something ugly:

componentDidUpdate() {
  this._output.innerHTML = marked(this.props.markdown);
}

[DylanPiercey]

@mjackson I personally think that this whole api is offensive and too opinionated. As others have said every templating language out there has the ability to output unescaped html for a reason.

I still personally think facebook/react#6253 or similar is needed in React for things like @rileybracken's issue with SVG's. Yes unescaped html needs to be explicit by why is it frowned upon? There are many legitimate use cases, all mentioned above.

I've personally built a CMS with React, and let me tell you it wouldn't be possible without this API.

[mjackson]

@yannickcr Any further objections here? @ljharb said a few times that he doesn't feel too strongly about it, and you've said you're not against removing it. I, for one, feel pretty strongly that no-danger shouldn't be part of the "recommended" rules, as do several others here.

So, can we merge?

[ljharb]

@DylanPiercey it's a recommended config. It's supposed to be opinionated. If you don't like the opinions (whether they recommend turning this particular rule on or not), then it's pretty easy - don't use the recommended options, and configure it yourself.

[DylanPiercey]

@ljharb, Sorry I wasn't meaning to call out the linting options for being opinionated, I was referring the the dangerouslySetInnerHTML={{__html: ...}} api in general. Just something that's always bothered me with React.

[yannickcr]

@mjackson Yes, it will be merged. But I consider this as a breaking change (not a big one, but still) so it will only be in the next major version (no ETA yet).

[jedwards1211]

@ljharb do you have your build set to fail on eslint errors or not? I'm always motivated to fix warnings so that I don't have to skim through a lot of cruft to find errors. If someone doesn't have that motivation it's probably because nothing is forcing them to fix the errors in the first place.

[jedwards1211]

@ljharb I find warnings very useful for rules like no-console because I often put in print statements temporarily for debugging, and don't want them to break the build, but I also want to have a reminder to remove them later.

[jedwards1211]

If you're a good dev and you know what you're doing, you'll just eslint-disable no-danger. If you're a bad dev and you don't know what you're doing, you can still just disable this rule and potentially pass code review. And if you're a clever bad dev you can figure out this pathetically easy workaround and avoid having to even eslint-disable anymore:

/* eslint-disable no-danger */
const Div = props => <div {...props}
  dangerouslySetInnerHTML={props.innerHTML && {__html: props.innerHTML}}
/>
/* eslint-enable no-danger */

// look ma, no errors!
const takeThatESLint = <Div innerHTML={`<script src="myEvilScript.js" />`} />

So I really don't see what this rule will save anyone from that the well-named dangerouslySetInnerHTML prop won't already.