Skip to content
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

dangerouslySetInnerHTML should NOT remove script tag #8838

Closed
rajaraodv opened this issue Jan 21, 2017 · 14 comments
Closed

dangerouslySetInnerHTML should NOT remove script tag #8838

rajaraodv opened this issue Jan 21, 2017 · 14 comments

Comments

@rajaraodv
Copy link

@rajaraodv rajaraodv commented Jan 21, 2017

Do you want to request a feature or report a bug?
Bug
What is the current behavior?
There are many cases in the real-world where we need to inject external 3rd party script. For example, adding google-analytics code, adding stripe button and so on. Currently dangerouslySetInnerHTML removes the script tag making it hard to simply inject JS code to the page.
Because of this issue, people are writing bloated components like react-ga (google analytics) that's 12KB(minimized) and all it does it to add a script tag! There are other similar libs like: react-scripts and stripe-checkout (8KB minimized to add the script).

I believe that at least when using dangerouslySetInnerHTML, we should not remove script tag and instead run the script.

For example, adding stripe's button is as follows.
https://stripe.com/docs/checkout/tutorial#embedding

<form action="/your-server-side-code" method="POST">
  <script
    src="https://checkout.stripe.com/checkout.js" class="stripe-button"
    data-key="pk_test_5qV78InO5XtnYvFRZ2VKnIjy"
    data-amount="999"
    data-name="Demo Site"
    data-description="Widget"
    data-image="https://stripe.com/img/documentation/checkout/marketplace.png"
    data-locale="auto">
  </script>
</form>

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/reactjs/69z2wepo/).

var StripeCheckout = React.createClass({
  createMarkup: function() {
    return {__html: 
      `Below script should create a Stripe button:
      <form action="/your-server-side-code" method="POST">
        <script
          src="https://checkout.stripe.com/checkout.js" class="stripe-button"
          data-key="pk_test_5qV78InO5XtnYvFRZ2VKnIjy"
          data-amount="999"
          data-name="Demo Site"
          data-description="Widget"
          data-image="https://stripe.com/img/documentation/checkout/marketplace.png"
          data-locale="auto">
        </script>
      </form>`
    };
  },
  render: function() {
    return  <span dangerouslySetInnerHTML={this.createMarkup()} />;
  }
})

ReactDOM.render(
  <StripeCheckout />,
  document.getElementById('container')
);

What is the expected behavior?
It should display a "Stripe" button button.

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?
15.4.2. I believe that this is how React worked for a long time.

@niole
Copy link
Contributor

@niole niole commented Jan 22, 2017

A workaround could be to make a script DOM element and append it to the appropriate parent. This stack overflow question also seems to have some answers:
http://stackoverflow.com/questions/35614809/react-script-tag-not-working-when-inserted-using-dangerouslysetinnerhtml

@rajaraodv
Copy link
Author

@rajaraodv rajaraodv commented Jan 24, 2017

I think that dangerouslySetInnerHTML should NOT strip script tag. When we are using dangerouslySetInnerHTML, we know that it could be 'dangerous'. Because of this issue, people are writing bloated components like react-ga (google analytics) that's 10KB(minimized) and all it does it to add a script tag! There are other similar libs like: react-scripts andstripe-checkout
CC @gaearon

@rajaraodv rajaraodv changed the title "script" tag in the renderer is gets removed (or doesnt work even when in dangerouslySetInnerHTML) "script" tag in the renderer is gets removed (aka dangerouslySetInnerHTML should NOT strip script tag) Jan 24, 2017
@rajaraodv rajaraodv changed the title "script" tag in the renderer is gets removed (aka dangerouslySetInnerHTML should NOT strip script tag) dangerouslySetInnerHTML should NOT remove script tag Jan 24, 2017
@abhirathore2006
Copy link

@abhirathore2006 abhirathore2006 commented Jan 27, 2017

I use a very basic component to load scripts, you can customize it to load any script you wish for.
it gives you a callback, so you can continue doing stuff once script get loaded.

    export interface mapWrapperProps{
      asyncScriptOnLoad?:()=>void;
      libraries?:string;
    }

    class MapWrapper extends React.Component<mapWrapperProps,{}>{

      componentWillMount(){
         let scriptlibraries = this.props.libraries?"libraries="+this.props.libraries:"";
         this.loadScript("https://maps.googleapis.com/maps/api/js?key=apikey&" + scriptlibraries, this.props.asyncScriptOnLoad);
      }

      loadScript(src,callback){
        let len = $('script').filter(function () {
                    return ($(this).attr('src') == src);
                  }).length;
        if(len){
          if(callback)callback();
        }else { 
          var script = document.createElement("script");
          script.type = "text/javascript";
          if(callback)script.onload=callback;
          document.getElementsByTagName("head")[0].appendChild(script);
          script.src = src;
        }
      }
      render(){
        return (<div></div>);
      }
    }
    export default MapWrapper;

@brigand
Copy link
Contributor

@brigand brigand commented Apr 8, 2017

I don't think this is a React issue, it's just how setting innerHTML works.

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Jul 12, 2017

scripts injected via innerHTML will not execute:

Without React:

https://codepen.io/nhunzaker/pen/GEPBJz

The same is true for React:

https://codepen.io/nhunzaker/pen/VWqdJx

Why:

This is because script tags injected via innerHTML are not executed for security reasons:

https://developer.mozilla.org/en-US/docs/Web/API/Element/innerHTML

Although this may look like a cross-site scripting attack, the result is harmless. HTML5 specifies that a <script> tag inserted via innerHTML should not execute.

What to do

I think a solution like @abhirathore2006's answer is spot on. As a far as bloat is concerned, one developer's bloat is often another's response to edge cases. I would encourage you to reach out to the projects to learn their reasoning, or possibly fork the project for your specific requirements.

@nhunzaker nhunzaker closed this Jul 12, 2017
@jakearchibald
Copy link

@jakearchibald jakearchibald commented Dec 4, 2017

createContextualFragment will run scripts http://jsbin.com/pajubuz/edit?js,output.

I don't know how many people want this, but React could add dangerouslySetHtmlContent which used createContextualFragment rather than innerHTML.

@nhunzaker
Copy link
Contributor

@nhunzaker nhunzaker commented Dec 19, 2017

Nice! Sorry, I kept meaning to respond here. DOM parsing/serialization is really neat stuff!

Still, I'm not sure how much steam there would be behind adding something like that, but I don't want to kill anyone's enthusiasm! If anyone is particularly interested in making a case for it, React now has a formal RFC repo. Personally, I'd want to know:

  1. What advantages exist for a dedicated property over using createContextualFragment in a lifecycle hook?
  2. Is this safe? How do we help users avoid injecting "live" script tags unintentionally?
  3. Does this behave consistently across every browser, is the behavior intentional, and is IE11+ support okay?

@betancourtl
Copy link

@betancourtl betancourtl commented Mar 1, 2018

Running into issues with draft-js and dangerouslySetInnerHTML property. I need to embed tweets and script tags need to run in order to do that. A way around make it work is to use an image's onload property to run scripts.

<img
  style="display: none;"
  class="load-script-img"
  src="http://placehold.it/1x1"
  onload="(function() {
    var script = document.createElement('script');
      script.setAttribute('src', 'https://platform.twitter.com/widgets.js');
      script.setAttribute('charset', 'utf-8');
      script.setAttribute('async', 'true');
      document.body.append(script);
  })()"
>
 <!--  Tweeter embed  -->
${html}
/>

I know it's not the best solution but is a way to go around the limitations.

@rickschott
Copy link

@rickschott rickschott commented Apr 10, 2018

This feature is needed. If you have encapsulated html code with script you have to separate it out. In a pure React environment it's not normally needed, but in a large enterprise setting you don't always have control over the remote content you want to use. JQuery does this easily and I can't believe more people don't complain about this.

@sonhanguyen
Copy link

@sonhanguyen sonhanguyen commented Aug 15, 2019

what does this have anything to do with react though, why should react have code to strip script from what's already expected to be a "dangerous" (hence should behave like a normal) html string? There are plenty of ways xss can be performed without a script tag if that's the intention.

@christo-pr
Copy link

@christo-pr christo-pr commented Dec 12, 2019

I created a React component that executes all the js code that it finds on the html string, check it out, it might help you:

https://www.npmjs.com/package/dangerously-set-html-content

@nightpool
Copy link

@nightpool nightpool commented Feb 17, 2020

@nhunzaker sorry to necro, but just found this thread today, and I think this is absolutely a really useful change for React to implement

What advantages exist for a dedicated property over using createContextualFragment in a lifecycle hook?

Getting the lifecycle hook right is very hard. The one @christo-pr linked doesn't work properly when the html prop can change, it doesn't work when server-side rendering, and it doesn't work for tests. If you add html to the dependency of the useEffect hook, then you also need to make sure to clear out all of the children that are already there, otherwise append will "double" your content every time the html changes. These are all problems that React is uniquely positioned to solve—it knows about server side rendering, it can clone the parent DOM node whenever the children need to change, etc etc.

Is this safe? How do we help users avoid injecting "live" script tags unintentionally?

It is no less safe then the existing dangerouslySetInnerHTML. It is trivial for an attacker to get around the "safety" feature of not executing script tags (for example, add an onload attribute to an img), the benefit is in allowing

Here's a concrete use-case. I'm writing a CMS, and I want to allow my users to embed rich content from a variety of providers. Twitter, like many providers, uses a combination of semantic HTML + a trusted <script> to progressively enhance the display. Here's an example of Twitter's supported embed format:

<blockquote class="twitter-tweet">
  <p lang="en" dir="ltr">
    Sunsets don&#39;t get much better than this one over
    <a href="https://twitter.com/GrandTetonNPS?ref_src=twsrc%5Etfw">@GrandTetonNPS</a>.
    <a href="https://twitter.com/hashtag/nature?src=hash&amp;ref_src=twsrc%5Etfw">#nature</a>
    <a href="https://twitter.com/hashtag/sunset?src=hash&amp;ref_src=twsrc%5Etfw">#sunset</a>
    <a href="http://t.co/YuKy2rcjyU">pic.twitter.com/YuKy2rcjyU</a>
  </p>&mdash; US Department of the Interior (@Interior)
  <a href="https://twitter.com/Interior/status/463440424141459456?ref_src=twsrc%5Etfw">May 5, 2014</a>
</blockquote>
<script async src="https://platform.twitter.com/widgets.js" charset="utf-8"></script>

If I naively used setDangerousInnerHTML to render this embed code, it wouldn't work. Embedding trusted rich context like this seems like a pretty common use-case, and it's really frustrating to have to hack around in weird DOM internals to get it to work.

Does this behave consistently across every browser, is the behavior intentional, and is IE11+ support okay?

Yes, this is an intentional interface exposed by the DOM Parsing spec, which specifically calls out that scripts should be executed when using it (step 4, here). I can't speak to the specifics of IE support, but the fact that createContextualFragments respects script tags is both per-spec and consistent across modern browsers.

(some details for those that are curious: innerHTML has legacy behavior with respect to scripts not for safety reasons, but to maintain compatibility with a once-common pattern of updating innerHTML in place, like so: element.innerHTML += '<h2>a new element</h2>'. For that reason, scripts inserted using innerHTML are marked as "already executed". Nodes created using createDocumentFragment, on the other hand, should be basically identical to parsing the HTML normally. https://lists.whatwg.org/pipermail/whatwg-whatwg.org/2010-November/071276.html is a whatwg thread from 2010 lays out the whole situation, including several statements from browser vendors and spec authors that this is intentional behavior.)

@nightpool
Copy link

@nightpool nightpool commented Feb 17, 2020

Frankly, as a user this is surprising enough behavior that I would consider it a bug in dangerouslySetInnerHTML, but I can also see the argument for making React's concepts as close to the DOM's concepts as possible, and implementing something new like dangerouslySetHtmlContent as the "recommended" way of adding trusted HTML instead.

@StevenBarquet
Copy link

@StevenBarquet StevenBarquet commented Feb 28, 2020

I created a React component that executes all the js code that it finds on the html string, check it out, it might help you:

https://www.npmjs.com/package/dangerously-set-html-content

It worked awesome for me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet