marko-js-archive / marko-widgets

[LEGACY] Module to support binding of behavior to rendered UI components rendered on the server or client
http://v3.markojs.com/docs/marko-widgets/
MIT License
142 stars 40 forks source link

marko and CSP #27

Open maberer opened 9 years ago

maberer commented 9 years ago

I am creating this issue to raise awareness for the use of CSP (content security policy) on a page that is built with marko.

There are primarily two concerns I currently see:

  1. If marko widgets should be rendered server side (in response to an ajax call) the documentation states the use of eval() to bind the widgets on the client side. https://github.com/raptorjs/marko-widgets#rendering-widgets-on-the-server When CSP is used, eval() is one of the first things that are prohibited; now if eval() is not an option, marko fails to bind the behaviour to the DOM. If one would be able to call the bind code manually (in response to the ajax call), eval() would not be necessary. It is also discussed here: https://github.com/raptorjs/marko-widgets/issues/26
  2. I am not completely sure about this one but I fear, that marko adds quite a few custom script tags (inlined) into the html on the first page load (or later). One of the strategies when using CSP is to refactor all inline script tags into script tags with a trusted resource… now, as there is usually only one client side bundle, why not calling all required code within this package… So if marko utilizes inlined script blocks (or adds them dynamically) please be aware, that it hurts CSP.

Libraries like youtube SPF seem to add inline script tags as well… I am not sure how/if they handle CSP… but I think that SPF really shines when used together with marko and I feel that both should play nice with the security settings of a page.

patrick-steele-idem commented 9 years ago

We can support CSP (specifically, disabling eval and inline scripts) but there will be a few tradeoffs. Originally, Marko Widgets allowed a more strict CSP but we backtracked a bit because of some performance concerns. I think we should revisit that decision and see what we can do to enable a more strict CSP for those apps that can take advantage of it.

I think Issue #26 should definitely be resolved, but here are a few other issues that would need to be resolved in order to enable a more strict CSP:

data-w-config="{"hello":"world"}"

Using JSON.parse also requires the JSON polyfill.


I feel like disallowing inline scripts will negatively impact progressive HTML rendering (with immediate initialization of widgets). Using JSON.parse instead of eval should be a minimal impact with a few slight changes, but it should probably be opt-in since it requires the JSON polyfill and will make the HTML slightly large (unless we replace double quotes with another safe character after JSON.stringify).

I'll be glad to work with you on this if you have some time. For the immediate timeframe I am focusing on making widgets stateful to bring in some of the great ideas from React.

Let us know if you have any other thoughts/questions/concerns.

Thanks for bringing up this important issue.

maberer commented 9 years ago

Ok, the statements make total sense - thank you.

For inline scripts, they seem to add a lot of the power that make marko awesome... therefore, avoiding inline scripts seems hard... as they are placed on the first, initial render, they might even work with a script nonce, to verify their origin... - then, policy can be made more strict... Anyway as they provide real value for marko, we might have to accept "unsafe-inline" for now.

eval() seems to be another beast.... and I was not aware of its use on widget config data, aside from the binding initialization that needs to run when templates are rendered server side... I am happy if https://github.com/raptorjs/marko-widgets/issues/26 will be resolved, though.

Basically, I think it is a good thing to avoid the use of eval() when possible; maybe even completely in the future - as always, making it optional is great.... not everybody likes to use polyfills for this...

So I am leaving this up for further discussion/brainstorming - thanks for participation.