Proposed extension for react script editor to become script and cewp

[HuwSy]

Sample (which sample are you talking about)

react-script-editor https://github.com/pnp/sp-dev-fx-webparts/tree/master/samples/react-script-editor

Authors

@MikaelSvenson

Suggestion (the more details, the better)

Not having proper git access through firewall i cant raise this as a pull req. But adding something along these lines to ScriptEditorWebPart.ts around line 46 and properties to manage it would make this a full replacement for SEWP and CEWP

        if ((this.properties.cewpFile || '') != '') {
            var ths = this;
            var req = new XMLHttpRequest();
            req.onreadystatechange = function () {
                if (req.readyState === 4) {
                    if (req.status === 401 || ~(req.responseURL || '').indexOf('/AccessDenied.aspx') || ~(req.response || '').indexOf('Access required'))
                        ths.domElement.innerHTML = '<h3>401 Access Denied</h3>';
                    if (req.status === 404)
                        ths.domElement.innerHTML = '<h3>404 File Not Found</h3>';
                    if (req.status === 200)
                        ths.domElement.innerHTML = req.response;
                    ths.executeScript(ths.domElement);
                }
            };
            req.open("GET", this.properties.cewpFile, true);
            req.send();
        }
        else {
            this.domElement.innerHTML = this.properties.script;
            this.executeScript(this.domElement);
        }

[ghost]

Thank you for reporting this issue. We will be triaging your incoming issue as soon as possible.

[PathToSharePoint]

Your issue is not following the standard format for suggestions. I assume you are talking about:

Sample (which sample are you talking about)

react-script-editor https://github.com/pnp/sp-dev-fx-webparts/tree/master/samples/react-script-editor

Authors

@mikaelsvenson

I looked at the code, as I had similar inquiries recently. Your proposed update is consistent with what is already there. With that said, how about using createContextualFragment instead of innerHTML and the custom executeScript method?

One concern with the executeScript method - if I read it correctly - is that it works with globals. In particular, I wonder what would happen if two such WebParts were included in a same page. Just my two cents, anyway my point is that createContextualFragment could bypass all the hassle.

[HuwSy]

Yes. Apologies for incorrect formatting. Updated now along with a typo just spotted using this when it should have been ths.

As for createContextualFragment, as these still need to work in classic sites which still go in to IE10 mode and IE is still supported until March then I wouldn’t change this until then.

[PathToSharePoint]

You have a point about classic environments. My assumption was that people would use the OOTB SEWP/CEWP wherever applicable, and only resort to third party on modern pages.

[update] I double checked and it seems that classic pages on SharePoint Online are running in 11 mode. Am I missing something?

[HuwSy]

Perhaps IE mode has changed since I last checked. I’ve still been working back to IE10 mode for no reason.

In that case I can’t argue with createContextualFragment.

[PathToSharePoint]

Let's see if the author or other contributors have input on this. As for me, I'll do more testing on IE mode, I was surprised too. I was actually trying to understand why recent versions of SPFx can run on classic pages.

cc recent contributors to the sample: @petkir @wobba

[PathToSharePoint]

fyi this is the meta I got from my classic pages: <meta http-equiv="X-UA-Compatible" content="IE=edge"/>

It could be interesting to compare and contrast with SP 2016 and SP 2019.

[wobba]

I would rather create a safe sanitized non-script cewp. But allowing loading from a file should be quite easy to add.

All in all, using the script wp is a bad code smell imo 😉

[PathToSharePoint]

Amen to that... but then maybe removing the current sample is best? Especially with the issue I was hinting at when using multiple instances on a page.

React actually has a method for HTML non-script injection, called dangerouslySetInnerHTML The beauty of this method is that the injected HTML is kept separate from the rest of the React DOM.

I'll be happy to help on this. The current OOTB Web Parts - content and markdown - are still limited and a broader Web Part could be useful. And at the same time could help educate people on script injection risks.

[HuwSy]

I think the SEWP and a CEWP that can run scripts from the loaded file have merit in some scenarios. Mainly people like myself migrating 6+ years old solutions into Teams that used CEWP firing scripts as a 1st pass before redeveloping to SPFx or PowerApps. A CEWP with no script execution here wouldn’t be of use at all.

Maybe we end up with 2 web parts one using a variant of the suggestion above giving a script running SEWP and CEWP combined and a completely separate web part doing the same but with no script running capability. The former could have big red warning splashed all over it about its script execution risks and to use the latter unless absolutely required.

[PathToSharePoint]

The migration scenario makes perfect sense. That ties back to the objective(s) of the samples library. Is it meant to help with the transition? To educate on best practices?

Having a separate "non-script" sample makes sense to me. I can work on a NSEWP!

For the SEWP, my findings seem to confirm that createContextualFragment will work.

[Edit] Actually those two Web Parts should end up being very similar, just use createContextualFragment for one and dangerouslySetInnerHTML for the other.

[wobba]

@PathToSharePoint sample stays, if not someone else will create one :) We've had that debate ever since adding it.

Creating a CEWP which allows html/css should be easy enough to get going. Allow load from a file, and disallow script.

@PathToSharePoint as to your point on globals and multiple web parts. It should not be an issue as the scripts are marked with a unique id and processed if they match. And due to the single thread model of javascript, I don't believe two parts would process the same scripts.

We'll happily accept a PR to load script/markup from a file, as I won't prioritize doing this myself right now.

[petkir]

@PathToSharePoint thanks for the mention, sorry I was out during the week.

the current SPFX version is, 1.10 for this reason I would not think about sp2016 or 2019 (it's not supported with this SPFX version).

@HuwSy I think is better to have one Sample, not two Samples or 2 Web parts, because you can describe the difference, and over 90% of the Code would be the same.

dangerouslySetInnerHTML createContextualFragment these two Methods are common to solve such things.

Usage of this Sample Please think about, if you include a SEWP on a page:

• mainly the script is not documented
• after years, nobody knows something about it
• this script will not be maintained, think about XSS ... Browser security updates, and many more things

You give this Power, everyone how can edit pages.

For editing this Web part, please take care that the default functionality should be the same as before. This web part is referenced in PNP SharePoint Modernization Framework. MS Docs Link for this Tool This script web part is used to migrate the old script so the new pages, UseCommunityScriptEditor

Personal Statement, I'm not a big fan of this Script Editor, but in some situations, it's a quick win solution.

[wobba]

@petkir Having one web part which can or cannot allow arbitrary script is a deployment and security concern imo as many these samples are used as in-prod solutions.

[petkir]

@wobba from this point you are right 2 solutions are better.

[PathToSharePoint]

Thanks for the clarification @wobba. I missed the fact that a unique id was used.

Roger on the Web Part being used in prod and referenced in the docs. That also addresses my earlier question.

[PathToSharePoint]

fyi I have created a first draft of a "Dangerously Set Editor Web Part", using a function component and the React dangerouslySetInnerHTML method. I'll share the draft after doing some more testing.

For the reasons stated above, and to prevent any mix up, for now I'll share the draft on my own repo. As said, my Web Part is not addressing the request.

[Update] A similar script using createContextualFragment seems to work too. But a lot of testing will be needed to validate those two samples. Again, I'll post both in my repo separately from the current sample which is used in prod - confirmation here for example: https://twitter.com/Joao12Ferreira/status/1312834842640424967

[wobba]

What does createContextualFragment give over innrHTML in this scenario where you are adding arbitrary markup? And why use dangerouslySetInnerHTML when it's not really React coming into play?

[PathToSharePoint]

The key difference is that createContextualFragment will let the scripts run, while all other methods don't. It greatly simplifies the code.

As for dangerouslySetInnerHTML, what it does is isolate the snippet from the React DOM. I suppose it means more efficient rendering than innerHTML. It might not be important, but considering that the Web Part is already using React then why not... There might be other subtleties I am not aware of.

[wobba]

innerHTML is a pure browser DOM operation so no logic added to it, so cannot see it being less efficient :)

To summarize the original issue. If we support loading from a file there should be parity with cewp more or less. Any other change could be an improvement.

[PathToSharePoint]

Remember that we are talking about a React Web Part, so we are dealing with two DOMs. innerHTML will definitely be more efficient on the "browser DOM", but not on the React DOM. In this specific case it certainly makes no difference.

Building on the existing solution and having parity makes perfect sense. That's why I'll publish my two samples separately to let the community take a look. I think it's also good for educational purposes to have different samples, some based on class components (like this Web Part) and others on function components (like the ones I'll release).

Side note: is the author handle "MikaelSvenson" mentioned in the readme correct?

[wobba]

@mikaelsvenson is my twitter handle, not github, as clicking it will show :)

[PathToSharePoint]

Just a note that I have released, as planned, my "Dangerous Content Web Part" that is meant as a replacement for a classic CEWP.

The current version is 0.9.1 and license is MIT - use at your own risk. You can download the sppkg file from github directly: https://github.com/PathToSharePoint/dangerous-content-web-part/

As mentioned before, its interest is to show a different approach, with hooks used instead of class components.

I did some more research on dangerouslySetInnerHTML, to confirm that it is meant to be more efficient than innerHTML on React re-renders. Also worth a note is that dangerouslySetInnerHTML strips the code from script tags, but doesn't offer a full protection against script attacks. There are libraries for that but I have yet to find one that is bulletproof.