-
Notifications
You must be signed in to change notification settings - Fork 47.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Have Fragments support dangerouslySetInnerHTML #12014
Comments
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. |
It doesn't seem quite right to me to put something like this on I think that essentially you’re asking for a “standalone” import { RawHTML } from 'react-dom';
const content = {
__dangerousHTML: '<h1>hi</h1>'
};
<RawHTML>{content}</RawHTML> (Also inspired by #2134 (comment)) I don’t know how hard it would be to implement something like this. |
@gaearon That is essentially what html-react-parser accomplishes. However, the purposes behind the intentionally ugly/repetitive From the docs:
I feel like adding this to 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. |
@jonnyasmar Please refer to discussion in #2134 (comment) about this, I intentionally kept |
@gaearon After combing through your issue, I'm still not entirely convinced this is the best approach. Partially due to scenarios like the following:
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:
Dev B comes along and discovers in the source code this neat little component called
Now, you may be asking yourself:
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?SimplicityBy allowing this functionality on the ConsistencyBy utilizing 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. Regarding React NativeWe gracefully degrade if necessary and The (Opinionated) Future!I personally LOVE having
The idea of event handlers on I say let's make |
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 ) ) |
On SSR, using Fragment, might help avoiding this can of invalid html:
On SSR: [update]: not all browsers are clever... |
There is nothing hacky in injecting HTML you generated from markdown for example. I second @gaearon's proposition of adding |
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 If Dev A understood tainting and wanted to create a reusable component that accepts HTML as a prop, they would make it accept the The idea of tainting is that you should be able to search for
It's inconsistent. Inconsistency isn't always obviously "bad" but typically it exposes more fundamental problems in the API. |
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. |
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. |
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. |
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 Dans suggestion above seems like it would allow for this, without changing the semantics of |
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:
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!!!!!!! |
Maybe I can give two examples of where we would have needed such a feature in my current project: HTML comments nested in a
|
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. |
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 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 |
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 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 {
"version":"https://jsonfeed.org/version/1",
// more here
} When I saw the output, I thought, "oh yeah, React escapes strings, I'll have to use 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 So with all this said, here's another +1 for some kind of "standalone" |
@martpie example is my use case as well
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. |
Going with https://github.com/remarkablemark/html-react-parser for now. |
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> |
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 |
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:
But instead we end up with:
etc. Another option is for us to manually strip the wrapping tags from the inner HTML but it is quite a fragile approach. |
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. |
@bvaughn Did someone actually submit an RFC for this? |
@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. |
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 If anyone thinks this is worth doing why don't we go through RFC process to accomplish this? |
I don't like the |
There is some RFC open (it even is linked): reactjs/rfcs#129 |
@gaearon I've got error when import: |
@namlq93 Dan's example was just for discussion purposes. Also FWIW, TypeScript types are not managed by the React team either. They're in a separate, community driven repo: |
Any news or workaround for that? |
@fr33z3 The corresponding RFC seems to be not having enough attention :( reactjs/rfcs#129 |
This comment was marked as off-topic.
This comment was marked as off-topic.
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 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 <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> EDIT: Simplified usage a little bit, also if you're planning to use ReactDOMServer.renderToStaticMarkup(<MyRootComponent />)
// Remove `<RawHtml />` injected scripts
.replace(/<script><\/script>/g, "") |
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, |
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. |
Why was this closed? I need this. |
I have written an (elegant) answer on Stackoverflow Basically using 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}/> |
@yairEO This was closed because they wanted to have a RFC: reactjs/rfcs#129 |
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. |
Thanks @yairEO I agree. |
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.which will render:
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 supportdangerouslySetInnerHTML
so that you may inject HTML without wrapping elements.would render:
Simple, obvious and aligned with the current API.
The text was updated successfully, but these errors were encountered: