Remove inline event listeners to support CSP environment

[wiredearp]

@zdlm @sampi @ehats

The minimum viable amount of quickfixes that at least in theory should make it possible to enable a (fairly liberal) Content Security Policy on the entire platform. We have been a blocker for getting this project off the ground, since Tradeshift UI runs on all pages.

There was also some random fixes to the docs pages.

Our target CSP aims to disable all inline JavaScript and this means that it is not enough to fix the framework (with this proposed PR). For use on the platform, it will also be required to update the implementation in all the apps so that this:

<aside data-ts="Aside" data-ts.onclose="alert('closed')">

— becomes replaced by this:

ts.ui.get('#myaside', aside => aside.onclose = () => alert('closed'));

— which in other words involves moving the business logic from the HTML view to some kind of JS file 😕

Apart from providing a better fit for the Angular or React way of life, where inline event listeners are the norm, it was also thought that inline event listeners would better support a scenario where the elements(s) get destroyed and created at random by the hosting framework. Now that the onclick statement is no longer expressed in the HTML, but instead as a method on the DOM element, Angular might in other words replace the active element with an inactive element, simply because that would be typical for Angular to do such a thing. It remains to be seen if this will actually happen in real life smoke tests.

[ehats]

Would it be possible to have our build pipeline convert inline handlers to statements like that? That'd kinda give us the best of both world. I'd assume we're not the only ones who are trying to do this, so I wouldn't be surprise if there was a webpack plugin or something?

[ehats]

Adding to the above, it seems that react 15 already does something about it, at least when it comes to style tags: facebook/react#5878

[ehats]

Looking at the actual changes, it's quite clear to me that I've no idea what all this EDBML stuff is :) I did, however, notice that you're specifying unsafe-inline for styles. Is there a particular reason for that? Not saying that I'm against it, just wondering.

[wiredearp]

1. Not possible in an automated step, really, because the code would need to be moved from a template file into a script file which may not exist and would need to be bundled into the app somehow. The script would also need to obtain a reference to the element somehow, this is also not needed in the HTML template view, and this process could depend on a lot of render-specific lifecycle hooks. The UI components often have a "facade" kind of implementation that could handle this once and for all to fix multiple apps at the same time, at least if and when these are done in Angular (since I don't know if react-tradeshift-ui is really used by many React apps).

2. About unsafe-inline, We do have a some inline styles in Tradeshift UI and I counted something like 200 errors in our unit tests with strict CSS enabled. Since CSS polities was not on the agenda [1], I just disabled the setting. It's worth noting however that the biggest hurdle is not with inline style attributes or style tags, but rather with libraries that modify CSS rules at runtime via the addRule method. In Tradeshift UI, we bundle the third party lib Spin.js which does exactly this, so this particular setting will require a bigger rewrite (also of some apps that use the same library independantly from Tradeshift UI). We could of course find other libraries on the platform that does the same.

[1] You said on Slack that "The only thing we really care about is getting rid of inline JS and eval(), new Function() and co. So basically script-src: * would be the first iteration".

Again I suggest that the gradual migration of all the apps from zero policy to full policy should be avoided if possible given that script-src can prevent most of these speculated attacks. The addRule method is for example only dangerous if someone can sneak in a script that calls the method and there is of course little chance that user input can end up in CSS declarations to begin with. It would be a better starting point, IMHO, to declare the policy that we want to end up with and then implement that on all new apps going forward. This way, at least new apps will not become added to the growing list of (orphaned) apps that must be retrofitted in multiple incremental steps in order to support a strict policy. With the end goal established, we can start enforcing a gradually stricter policy on the old apps, knowing that any stumbling block, such as for example having to replace Spin.js with a different library, will not stop us from reaching the ultimate end goal simply by being placed on a backlog for months before it can be resolved and the next policy settting can be examined. Time to market is important, I think, because the strictest possible policy should of course apply to all third party apps, since these are obviously more dangerous.

[sampi]

@wiredearp is this code ready to merge or are there more changes required to make it work as is?

[wiredearp]

@sampi Should be ready as in did pass our own tests (at some point) and looks good on the docs website, but smoke tests are as usual an open question.