facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.26k stars 46.7k forks source link

Have Fragments support dangerouslySetInnerHTML #12014

Closed dominikwilkowski closed 5 years ago

dominikwilkowski commented 6 years ago

The addition of the Fragment in 16.2.0 is fantastic and helps keep our HTML semantic and clean. Unfortunately there is still no way to inject HTML without a wrapping tag.

const HTML = <span>Hello World</span>;

<div key={ ID } dangerouslySetInnerHTML={ { __html: HTML } } />

which will render:

<div><span>Hello World</span></div>

It would be mostly helpful for rendering HTML from jsx on the back end rather than in the SPA context. To me Fragment seems to be the ideal candidate to support dangerouslySetInnerHTML so that you may inject HTML without wrapping elements.

const HTML = <span>Hello World</span>;

<Fragment key={ ID } dangerouslySetInnerHTML={ { __html: HTML } } />

would render:

<span>Hello World</span>

Simple, obvious and aligned with the current API.

jonnyasmar commented 6 years ago

Second this!

Achieving this currently requires third-party support via libs like https://github.com/remarkablemark/html-react-parser which somewhat undermines the simplicity of Fragments.

gaearon commented 6 years ago

It doesn't seem quite right to me to put something like this on Fragment itself since it’s not DOM-specific (e.g. it works on React Native).

I think that essentially you’re asking for a “standalone” dangerouslySetInnerHTML. That sounds a bit like https://github.com/facebook/react/issues/9483. Maybe something like this would make more sense to me than the current API:

import { RawHTML } from 'react-dom';

const content = {
  __dangerousHTML: '<h1>hi</h1>'
};
<RawHTML>{content}</RawHTML>

(Also inspired by https://github.com/facebook/react/issues/2134#issuecomment-67687628)

I don’t know how hard it would be to implement something like this.

jonnyasmar commented 6 years ago

@gaearon That is essentially what html-react-parser accomplishes. However, the purposes behind the intentionally ugly/repetitive dangerouslySetInnerHTML={{__html: HTML}} procedure is actually to serve as a reminder that this is a dangerous/hacky thing to do:

From the docs:

So, you can set HTML directly from React, but you have to type out dangerouslySetInnerHTML and pass an object with a __html key, to remind yourself that it’s dangerous.

I feel like adding this to Fragment maintains the notion that this is "dangerous" & "hacky" as originally intended.

Your example (and by association, html-react-parser) could be used in a way that could very conveniently disguise the fact that it's dangerous; leaving future maintainers of the code with no indication of a potential vulnerability.

gaearon commented 6 years ago

@jonnyasmar Please refer to discussion in https://github.com/facebook/react/issues/2134#issuecomment-67687628 about this, I intentionally kept dangerous in the object property name because that's the part that's supposed to be "tainted" rather than the prop name.

jonnyasmar commented 6 years ago

@gaearon After combing through your issue, I'm still not entirely convinced this is the best approach.

Partially due to scenarios like the following:

There is much more following the code. Please stick with me.

Let's say we have two developers, Dev A & Dev B. They both work on the same project. Dev A is a senior dev who is very well acquainted with XSS vulnerabilities. Dev B is a junior dev who knows what XSS vulnerabilities are, but is not necessarily well-versed enough to recognize them.

Dev A creates the following component, because they know what they're doing:

import { RawHTML } from 'react-dom';

export class InjectHTML extends React.Component{
  props: {
    html: string
  };

  render(){
    const content = {
      __dangerousHTML: this.props.html
    };

    return <RawHTML>{content}</RawHTML>
  }
}

Dev B comes along and discovers in the source code this neat little component called InjectHTML.

Hold my beer...


import { InjectHTML } from './components/InjectHTML';

export class CoolComponentWithHTML extends React.Component{ render(){ return <InjectHTML html={'I AM BERY BERY SNEAKY SIR'}/> } }



Now, you may be asking yourself:

> What's to keep this from happening with the OPs suggested **Fragment** approach?

**Absolutely nothing.**

Thanks to React's unprecedented versatility, we could create the same sort of dangerously deceptive HTML injecting component.

## So, what is the benefit of this Fragment approach?

### Simplicity

By allowing this functionality on the `Fragment` component itself, I think we avoid encouraging overly-DRY implementations like what **Dev A** came up with.

### Consistency

By utilizing `Fragment` for this, we also avoid having to implement two different components that accomplish the same thing (wrapperless rendering). This is `Fragment`'s purpose. Why avoid using it?

We also avoid changing a behavior that devs are already used to while simultaneously creating a predictable implementation for devs like the OP [who (rightfully?) assumed this would work](https://stackoverflow.com/questions/48236588/using-fragment-to-insert-html-rendered-on-the-back-end-via-dangerouslysetinnerht/48236653#48236653).

### Regarding React Native

We gracefully degrade if necessary and `dangerouslySetInnerHTML` does nothing. I'm not sure I see any problem with that?

### The (Opinionated) Future!

I personally LOVE having `Fragment` at my disposal now and I'd be really interested to see what kind of cool functionality we can afford this little gem.

> key is the only attribute that can be passed to `Fragment`. **In the future, we may add support for additional attributes, such as event handlers**.

The idea of event handlers on `Fragment`'s would be **_awesome_** and I really think we should try to encourage in any way we can making `Fragment` a more robust feature of React. Having to unnecessarily wrap things is probably the only thing I have to do regularly in React that makes me cringe...

**I say let's make `Fragment` famous!**
dominikwilkowski commented 6 years ago

For those who are finding this issue and struggling to find a solution, see below what we do until react has a way to inject HTML without wrappers:

// In our generator code we use:
<cuttlebellesillywrapper key={ ID } dangerouslySetInnerHTML={ { __html: HTML } } />

// and before we write to disk we do
/**
 * Clean react output of any silly wrapping divs
 *
 * @param  {string} html - The HTML generated with react
 *
 * @return {string}      - The cleaned HTML
 */
export const ParseHTML = ( html ) => {
    if( typeof html === 'string' ) {
        return html
            .replace(/<cuttlebellesillywrapper>/g, '')
            .replace(/<\/cuttlebellesillywrapper>/g, '');
    }
    else {
        return html;
    }
}

CreateFile( pathToFile, ParseHTML( html ) )
rande commented 6 years ago

On SSR, using Fragment, might help avoiding this can of invalid html:

<div dangerouslySetInnerHTML={{ __html: `<link rel="preload" href="${props.assets.fonts}" as="style" onload="this.onload=null;this.rel='stylesheet';" />` }} />

On SSR: onload is removed, so the only solution is to used dangerouslySetInnerHTML. However, Fragment does not support it. So now, I have a div > link, browsers seems to be clever enough to make the fonts loads async.

[update]: not all browsers are clever...

ivan-kleshnin commented 6 years ago

That is essentially what html-react-parser accomplishes. However, the purposes behind the intentionally ugly/repetitive dangerouslySetInnerHTML={{__html: HTML}} procedure is actually to serve as a reminder that this is a dangerous/hacky thing to do:

There is nothing hacky in injecting HTML you generated from markdown for example. dangerouslySetInnerHTML was always a wonky name. False danger alarms hurt security as well.

I second @gaearon's proposition of adding RawHtml tag – I would even say it's long overdue. Don't mind the original proposition of extending React.Fragment either.

gaearon commented 6 years ago

Dev A creates the following component, because they know what they're doing

I think in your scenario Dev A doesn't fully understand how tainting works. If they did, they wouldn't write code like

    const content = {
      __dangerousHTML: this.props.html
    };

because it's a tainting security hole. The idea of tainting is that you only use __dangerousHTML as a key for trusted inputs. There's no reason why this.props.html would be trusted.

If Dev A understood tainting and wanted to create a reusable component that accepts HTML as a prop, they would make it accept the {__dangerousHTML} object itself. Then it would be up to the consumer of that component to specify that object.

The idea of tainting is that you should be able to search for __dangerousHTML in your codebase and audit each instance: is the right-hand side always safe? There's nothing that would guarantee that __dangerousHTML: this.props.html would be safe. However, once you have a safe object, you should be able to pass it around freely.

Regarding React Native We gracefully degrade if necessary and dangerouslySetInnerHTML does nothing. I'm not sure I see any problem with that?

It's inconsistent. Inconsistency isn't always obviously "bad" but typically it exposes more fundamental problems in the API.

HoraceShmorace commented 6 years ago

I think in your scenario Dev A doesn't fully understand how tainting works.

I've always thought that the naming of "dangerouslySetInnerHTML" was a bit heavy-handed. Don't mitigate developer incompetence. Decent developers understand injection and such. Let the inexperienced and/or bad developers make mistakes.

aweary commented 6 years ago

Let the inexperienced and/or bad developers make mistakes.

React needs to be accessible and safe to developers of all skill levels. Making it easier for less experienced developers to introduce security issues is bad for them, their users, and the React community in general.

gaearon commented 6 years ago

Developers of all experience levels make these mistakes, especially if data goes through many layers and it's hard to say where it gets sanitized and how strong the guarantees are.

Regardless, let's keep this thread on topic, and avoid turning this into a debate about usefulness of scary naming.

vasklund commented 5 years ago

One use-case for this that I just came across is when migrating non-React (server-side) components to React components.

Being able to insert HTML generated from the non-React components directly inside a React component makes migrating easier (we can take smaller steps). This is straight-forward if you're OK with adding wrapper elements around your (non-React component) HTML, but that might break existing tests or have other subtle issues.

Adding dangerouslySetInnerHTML on the parent element doesn't work if you want to have non-React and React elements as siblings.

Dans suggestion above seems like it would allow for this, without changing the semantics of Fragment and while still being honest about the security implications: https://github.com/facebook/react/issues/12014#issuecomment-357673890

Enalmada commented 5 years ago

I have always used the filamentgroup loadCSS method to load non-critical css because last I checked, it has much less FOUT than other methods (fontfaceobserver, Web Font Loader). When trying to migrate my old site over to react (nextjs), I discovered that the link elements don't have the onLoad attribute which is very strange to a new user, their mysterious disappearance caused me significant pain. I feel like I need it and am pretty unhappy something is removing it but I don't know what to do about it. Is react or nextjs removing the onLoad attribute? If it is react, can you please give me a safe best practice way to have it back so I can use it in my _document.js for nextjs to send to the client?

Anyway, it is a shame it is so hard to even hack around the problem. I found this issue during desperate google searching and really appreciate everyone who has posted workarounds in it. I tried doing Fragment dangerouslySetInnerHTML on my own before even learning of this ticket. How has this ticket been open for so long and that is not a thing yet? I obviously don't want to add a div to my head if it doesn't work in all browsers and I have no idea where to put ParseHTML to modify the text using nextjs. After sitting here for quite some time worrying that there might not actually be any practical solution to my problem and wondering if I have made a horrible mistake choosing react...this came to me. Close the script tag, put what you need in, and reopen it:

<script dangerouslySetInnerHTML={{ __html: `</script>
    <link rel="preload" href="https://fonts.googleapis.com/css?family=Open+Sans" as="style" onLoad="this.onload=null;this.rel='stylesheet'" crossOrigin="anonymous"/>
<script>`,}}/>

Obviously you have useless script tags in your head which is undesirable. html-react-parser doesn't seem to work for me...the onLoad is still removed which I need.

Please react team give us a better way to do this!!!!!!!

martpie commented 5 years ago

Maybe I can give two examples of where we would have needed such a feature in my current project:

HTML comments nested in a <div>

<div dangerouslySetInnerHTML={{ __html: `<!-- Released on ${BUILD_DATE} -->` }} />

will output

<div>
  <!-- Released on [blabla] -->
</div>

I can live with it, but it's not really satisfying.

SSR and injecting multiple tags in

Our client asked us if he could add scripts to the website we are building with Next.js (think analytics, optimizely...).

We created a scripts text field in the CMS so our client would just paste the different scripts he gets from the various services they are using.

<div dangerouslySetInnerHTML={{ __html: header.scripts }} />

would output:

<head>
  ...
  <div>
    <script>/* ... analytics */</script>
    <script>/* ... optimizely */</script>
  </div>
</head>

which is obviously not valid HTML.

We managed to fix it with html-react-parser, but it is annoying to add yet another dependency just to do that correctly.

jonathantneal commented 5 years ago

I needed something like this and wrote this drop-in replacement. https://github.com/jonathantneal/react-dom-fragment

There’s no container element and DOM updates are patched. It can add about 2kB to your bundle size, but half of that is because I had to bundle domdiff for patching. So, if any part of it can be used to improve React, I’ll freely hand the code over.

tfrijsewijk commented 5 years ago

Although I would love this, I don't think it'll work out for everything.

I'm generating an SVG using React. Inside this SVG there are 12 SVG icons (another ). The SVG icons are provided by an API and I place them with {{dangerouslySetInnerHTML}}. At the same time I'm positioning the icon using {{x}}- and {{y}}-attributes which are calculated using a obscure formula.

So basically I want this:

<svg width="400" height="400" viewBox="-200 -200 400 405">
  <React.Fragment
    x={iconPositionX}
    y={iconPositionY}
    dangerouslySetInnerHTML={({ __html: iconMarkup })}
  />
</svg>

But what happens with x/y ? By allowing dangerouslySetInnerHTML you create the impression one can set attributes on React.Fragment.

Either honour the attributes (placing them on the root node of the dangerouslySettedInnerHTML), or don't do anything (no attributes, no dangerouslySetInnerHTML) and avoid confusion

jimniels commented 5 years ago

Came here from a Google search and didn't see my particular use case noted, so allow me to explain:

I'm trying to use React/JSX for server-side templating of a static site. I have a bunch of react components that get populated with an initial set of props and then the server renders them and writes them out as pages of my site. The problem is, in addition to HTML pages, I also have some files that aren't HTML but i would still like to use JSX for templating. For example, I'm trying to write out a feed.json file:

const React = require("react");

module.exports = ({ site }) => {
  const feed = {
    version: "https://jsonfeed.org/version/1",
    title: site.title,
    description: site.description
    // more here
  };

  return JSON.stringify(feed);
}

In my node code, I'm basically looping through each of my component "template" files, rendering them, then writing to disk, i.e.

// looping through all files in my site and doing:
const stringToWrite = ReactDOMServer.renderToStaticMarkup(
  React.createElement(Component, siteData)
);

For the majority of my template files, this works great. It outputs a bunch of static .html files. But, as stated, I have a couple files that I want to write out that aren't HTML. The code above for feed.json outputs something like this:

{
  &quot;version&quot;:&quot;https://jsonfeed.org/version/1&quot;,
  // more here
}

When I saw the output, I thought, "oh yeah, React escapes strings, I'll have to use dangerouslySetInnerHTML". Since I was trying to write a .json file, I didn't want to output an actual node of anything, so I knew <div dangerouslySetInnerHTML={}> wouldn't work. Then I thought "I could use a Fragment!" And I tried it:

  return (
    <React.Fragment
      dangerouslySetInnerHTML={{ __html: JSON.stringify(feed) }}
    />
  );

But it didn't render anything. So I Google'd Fragment with dangerouslySetInnerHTML and here we are.

Perhaps there's a better way for me to handle this in my project and I'm just not experienced enough yet to understand what that would be. However, I write this comment to illustrate how a developer might jump to the conclusion that dangerouslySetInnerHTML on a Fragment will get them what they want.

So with all this said, here's another +1 for some kind of "standalone" dangerouslySetInnerHTML

abhishiv commented 5 years ago

@martpie example is my use case as well

HTML comments nested in a

<div dangerouslySetInnerHTML={{ __html: `<!-- Released on ${BUILD_DATE} -->` }} />

This would make possible direct manipulation tools to use comments as a way to attach metadata to JSXText/JSXExpression/JSXElemenet nodes. Would love to have this.

jaruesink commented 5 years ago

Going with https://github.com/remarkablemark/html-react-parser for now.

kuworking commented 5 years ago

Another use case would be to include google html tags

<div dangerouslySetInnerHTML={{ __html: '<!--googleoff: snippet-->' }} />

Doing it like this is not correct I presume*, so I assume it is currently not possible?

*correct

<div>
<!--googleoff: snippet-->
content
<!--googleon: snippet-->
</div>

*incorrect

<div>
<div><!--googleoff: snippet--></div>
content
<div><!--googleon: snippet--></div>
</div>
peterbe commented 5 years ago

The trick of using RawHTML doesn't appear to work in React 16.8.6 (cuz that's what I use here and now): https://codesandbox.io/s/nifty-breeze-5gbmy?fontsize=14

Not sure how or where this went.

peterbe commented 5 years ago

Oh, I think I misunderstood the idea of <RawHTML>. It was just pseudo code. Ignore my comment above.

SkReD commented 5 years ago

Possible solution for markup where only one root element is parse only that root element start and close tags. Then create react element from them and put inner html of that root element into dangerouslySetInnerHTML.

link to gist

nicl commented 5 years ago

To add another use case (from the Guardian here). We pass an article as 'elements' (structured data) over the wire and then have a react app that wants to render these as HTML. Some of these structured elements have an 'html' field. We don't want to be forced to add a wrapper element for these cases as it can worsen the HTML output - e.g. if we have some p elements interspersed with some img ones, ideally the html output would be close to:

p
p
img
p

But instead we end up with:

span(p)
span(p)
img
span(p)

etc.

Another option is for us to manually strip the wrapping

tags from the inner HTML but it is quite a fragile approach.

bvaughn commented 5 years ago

I think this issue should go through our RFC process rather than as an issue in this GitHub repository.

I'm going to close this issue. I know there's been some good discussion above, but since it seems to have settled- I think it would be appropriate to incorporate the discussion points into the RFC proposal.

jonnylynchy commented 5 years ago

@bvaughn Did someone actually submit an RFC for this?

FibreFoX commented 5 years ago

@jonnylynchy not yet, I was going to upcoming weekend ... but as my own reported stuff #16931 got closed too, my impression is that this is just some additional burden, makes me feel ignored.

sergeychernyshev commented 5 years ago

I am one of the maintainers of UX Capture React bindings (instrumentation libraries) and we need to inject HTML into SSR-ed or statically generated HTML before hydration happens, specifically for our <img> wrapper that is supposed to attach onload handler inline with the elements which would be much better if it didn't wrap it.

If anyone thinks this is worth doing why don't we go through RFC process to accomplish this?

hsluoyz commented 4 years ago

I don't like the <div></div> wrapper. Any update?

FibreFoX commented 4 years ago

There is some RFC open (it even is linked): https://github.com/reactjs/rfcs/pull/129

namlq93 commented 4 years ago

@gaearon I've got error when import: @types/react-dom"' has no exported member 'RawHTML'

bvaughn commented 4 years ago

@namlq93 Dan's example was just for discussion purposes. RawHTML is not something that has been implemented in React.

Also FWIW, TypeScript types are not managed by the React team either. They're in a separate, community driven repo: https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/react-dom

fr33z3 commented 3 years ago

Any news or workaround for that?

FibreFoX commented 3 years ago

@fr33z3 The corresponding RFC seems to be not having enough attention :( https://github.com/reactjs/rfcs/pull/129

sandorvasas commented 2 years ago

just add the freaking feature and let the dev decide about security.

zomars commented 2 years ago

Reposting my hacky solution in case someone came here from Google:

I know this is a hack but for my use case this allows me to inject arbitrary html inside head tags:

const RawHtml = ({ html = "" }) => (
  <script dangerouslySetInnerHTML={{ __html: `</script>${html}<script>` }} />
);

Usage:


const EmailHead = ({ title = "" }) => {
  return (
    <head>
      <title>{title}</title>
      <RawHtml html={`<!--[if !mso]><!--><meta http-equiv="X-UA-Compatible" content="IE=edge"><!--<![endif]-->`} />
    </head>
  )
}

The output will leave some empty script tags along the way which is not optimal but it works:

<html>
  <head>
    <title>Title</title>
    <script></script>
    <!--[if !mso]><!-->
    <meta http-equiv="X-UA-Compatible" content="IE=edge" />
    <!--<![endif]-->
    <script></script>
  </head>
  <body></body>
</html>
image

EDIT: Simplified usage a little bit, also if you're planning to use renderToStaticMarkup, you can do this to cleanup the empty scripts:

ReactDOMServer.renderToStaticMarkup(<MyRootComponent />)
  // Remove `<RawHtml />` injected scripts
  .replace(/<script><\/script>/g, "")
yehonadav-feel commented 1 year ago

maybe something like this:

export const RawHtml:FC<{ html: string }> = ({ html }) => {
  const refTemplate = useRef<HTMLTemplateElement|null>(null);
  const refInnerHtml = useRef<NodeListOf<ChildNode>|null>(null);
  const refInserted = useRef(false);

  useEffect(() => {
    if (refTemplate.current && refInnerHtml.current) {
      const template = refTemplate.current;
      const innerHtml = refInnerHtml.current;
      let nextSibling = template.nextSibling;
      if (refInserted.current) {
        innerHtml.forEach(node => {
          node.parentNode?.removeChild(node);
        });
      }
      innerHtml.forEach(node => {
        template.parentNode?.insertBefore(node, nextSibling);
        nextSibling = node;
      });
      refInserted.current = true;
    }
    return () => {
      if (refInnerHtml.current && refInserted.current) {
        refInnerHtml.current.forEach(node => node.parentNode?.removeChild(node));
        refInserted.current = false;
      }
    }
  }, [html]);

  return (
    <>
      <template ref={ref => {
        if (ref) {
          refTemplate.current = ref;
          ref.innerHTML = html.trim();
          refInnerHtml.current = ref.content.childNodes
        }
      }}/>
    </>
  );
}

i haven't tested it thoroughly, it seems to work but your left with this <template ... /> which is annoying. we can try and add a state that will un-render the template after the html is inserted but i'd like to hear what other people think first.

yehonadav-feel commented 1 year ago

so here's a code sample which removes the template after insertion:

export const RawHtml:FC<{ html: string }> = ({ html }) => {
  const refTemplate = useRef<HTMLTemplateElement|null>(null);
  const refInserted = useRef(false);
  const [showTemplate, setShowTemplate] = useState(true);

  useEffect(() => {
    if (refTemplate.current?.content.childNodes && showTemplate) {
      const template = refTemplate.current;
      const innerHtml = template.content.childNodes;
      let nextSibling = template.nextSibling;
      if (refInserted.current) {
        innerHtml.forEach(node => {
          node.parentNode?.removeChild(node);
        });
      }
      innerHtml.forEach(node => {
        template.parentNode?.insertBefore(node, nextSibling);
        nextSibling = node;
      });
      refInserted.current = true;
      setShowTemplate(false)
    }
    return () => {
      if (refTemplate.current?.content.childNodes && refInserted.current) {
        refTemplate.current.content.childNodes.forEach(node => node.parentNode?.removeChild(node));
        refInserted.current = false;
      }
    }
  }, [html, showTemplate]);

  useEffect(() => {
    setShowTemplate(true);
  }, [html]);

  return (
    // eslint-disable-next-line react/jsx-no-useless-fragment
    <>
      {showTemplate &&
        <template ref={ref => {
          if (ref) {
            ref.innerHTML = html.trim();
            refTemplate.current = ref;
          }
        }}/>
      }
    {/* html will render here */}
    </>
  );
}

which is kinda weird because the template remains, just without its inner content, hope that helps.

yairEO commented 1 year ago

Why was this closed? I need this.

yairEO commented 1 year ago

I have written an (elegant) answer on Stackoverflow

Basically using outerHTML:

const StringToNode = React.memo(({value}) => {
  const ref = React.useRef();

  // important to not have ANY deps
  React.useEffect(() => ref.current.outerHTML = value, []) 

  return <a ref={ref}/>;
});

Usage

<StringToNode value={HTMLString}/>
FibreFoX commented 1 year ago

@yairEO This was closed because they wanted to have a RFC: https://github.com/reactjs/rfcs/pull/129

yairEO commented 1 year ago

I think topics should not be closed unless absolutely obsolete, and not just by saying "ho we want to do another thing..." and ending up never doing it. This is going nowhere and people are eagerly waiting.

aiphee commented 1 year ago

Thanks @yairEO I agree. A also made TS version of your code https://gist.github.com/aiphee/fd5393ae7614ff3d34bfb33c00bb68df