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

Update lasso to add support for CSP nonce #93

Closed
ppattanayak opened this issue Jan 7, 2016 · 22 comments
Closed

Update lasso to add support for CSP nonce #93

ppattanayak opened this issue Jan 7, 2016 · 22 comments

Comments

@ppattanayak
Copy link

Need to update lasso to support the nonce feature of Content Security Policy 2, for the script tags in the HTML.

Example:

<script nonce="a613a14b8875acfa0fa40d2229d8786c9a5a2d260"> 
//JS Code 
</script>
@patrick-steele-idem
Copy link
Contributor

Hey @ppattanayak, thanks for opening an issue. This will require some changes to Lasso.js. At a high-level:

  • Introduce some new hooks:
    1. customInlineScriptRenderer(inlineCode, slot) ← Insert a {nonce} placeholder
    2. onSlotCreated(function(slot) { ... }) ← Calculate positions of all {nonce} placeholders in slot.html and store with slot.noncePositions
    3. customGetHtmlForSlot(slot, out) ← Replace all {nonce} tokens in slot.html with actual nonce using slot.noncePositions

NOTE: Nonce can be retrieved from out using following code:

var res = out.stream;
var req = res.req;
var csp = req.csp;
var nonce = csp.nonce;

As soon as I get a chance, I'll try to point you in the right direction so that you can make the appropriate changes to support these new hooks. In the meantime, please try to familiarize yourself with some of the Lasso.js code.

Thanks again, for opening the issue!

@patrick-steele-idem
Copy link
Contributor

@ppattanayak can you please confirm if the nonce is needed on all of the following?:

<script type="text/javascript" src="/foo.js" nonce="abc123"></script>
<script type="text/javascript" nonce="abc123">console.log('foo')</script>
<link rel="stylesheet" type="text/css" href="foo.css" nonce="abc123">
<style type="text/css" nonce="abc123">.foo { }</style>

Or, is it only needed for inline scripts?

Thanks.

@ppattanayak
Copy link
Author

Its needed in second and fourth. Not in first and third.

@patrick-steele-idem
Copy link
Contributor

Great. Thank you for confirming. So only for inline scripts and inline styles.

@ppattanayak
Copy link
Author

Yes, thats correct.

@patrick-steele-idem
Copy link
Contributor

Hey @ppattanayak and @tropperstyle, please review the following Pull Request to provide CSP nonce support: #94

@tropperstyle
Copy link

Getting the following error after running npm install lasso-js/lasso#issue-93

ReferenceError: rmod is not defined
    at Object.render [as _] (/app/node_modules/lasso/body-116-0-12-31904-1qpgh44.marko.js:14:17)
    at Object.Template.renderSync (/app/node_modules/marko/runtime/marko-runtime.js:114:14)
    at Object.LassoPageResult.getHtmlForSlot (/app/node_modules/lasso/lib/LassoPageResult.js:101:25)
    at Object.LassoPageResult.getSlotHtml (/app/node_modules/lasso/lib/LassoPageResult.js:140:21)
    at renderSlot (/app/node_modules/lasso/taglib/slot-tag.js:21:36)
    at Object.<anonymous> (/app/node_modules/lasso/taglib/slot-tag.js:56:13)
    at notifyCallbacks (/app/node_modules/lasso/node_modules/raptor-async/AsyncValue.js:76:35)
    at Object.AsyncValue.resolve (/app/node_modules/lasso/node_modules/raptor-async/AsyncValue.js:262:9)
    at done (/app/node_modules/lasso/taglib/page-tag.js:67:39)
    at done (/app/node_modules/lasso/lib/Lasso.js:664:17)
    at CacheEntry.proto.readValue (/app/node_modules/lasso/node_modules/raptor-cache/lib/CacheEntry.js:54:16)
    at /app/node_modules/lasso/node_modules/raptor-cache/lib/Cache.js:251:28
    at /app/node_modules/lasso/node_modules/raptor-cache/lib/Cache.js:84:20
    at Object.MemoryStore.get (/app/node_modules/lasso/node_modules/raptor-cache/lib/MemoryStore.js:14:16)
    at getCacheEntry (/app/node_modules/lasso/node_modules/raptor-cache/lib/Cache.js:62:22)
    at /app/node_modules/lasso/node_modules/raptor-cache/lib/Cache.js:126:21
    at /app/node_modules/lasso/lib/Lasso.js:287:13
    at /app/node_modules/lasso/node_modules/raptor-async/series.js:30:33
    at /app/node_modules/lasso/node_modules/raptor-async/series.js:30:33
    at done (/app/node_modules/lasso/lib/writers/Writer.js:96:20)
    at /app/node_modules/lasso/node_modules/raptor-async/series.js:30:33
    at /app/node_modules/lasso/lib/writers/Writer.js:172:32
    at /app/node_modules/lasso/lib/writers/file-writer.js:453:17
    at handleSuccess (/app/node_modules/lasso/lib/writers/file-writer.js:129:13)
    at /app/node_modules/lasso/lib/writers/file-writer.js:167:37
    at FSReqWrap.oncomplete (fs.js:82:15)

@patrick-steele-idem
Copy link
Contributor

Hey @tropperstyle, can you try the following?:

rm -rf node_modules/
npm install
npm install lasso-js/lasso#issue-93
rm -rf .cache/ static/
MARKO_CLEAN=true node .

That should wipe out all caches which can cause problems when switching to a new version of a modules. Please let me konw if that solves your problem.

@patrick-steele-idem
Copy link
Contributor

@tropperstyle Actually, I am able to reproduce. Let me figure out what went wrong!

@tropperstyle
Copy link

I tried using rm -rf .cache and find . -name *.marko.js -exec rm {} \; - Would that suffice to wipe out the cache?

@patrick-steele-idem
Copy link
Contributor

It's not a caching issue. We converted the HTML for the head and body slots to templates and the Marko compiler is not liking the $ that appears in an inline script. I am working on a fix.

@patrick-steele-idem
Copy link
Contributor

Hey @tropperstyle, should be fixed now. Please try again:

npm install lasso-js/lasso#issue-93
rm -rf .cache/ static/
MARKO_CLEAN=true node .

Thanks for your help!

@tropperstyle
Copy link

It doesn't seem to be picking up the lasso-nonce attribute for templates in node_modules/*/marko-taglib.json

@patrick-steele-idem
Copy link
Contributor

@tropperstyle can you give me a more specific path to try?

@patrick-steele-idem
Copy link
Contributor

@tropperstyle I tried to reproduce your problem by doing the following:

Created a new template:

node_modules/foo/foo-template.marko

<script lasso-nonce>
    Hello foo!
</script>

Compiled it using markoc:

markoc node_modules/foo/foo-template.marko

Compiled output:

function create(__helpers) {
  var str = __helpers.s,
      empty = __helpers.e,
      notEmpty = __helpers.ne,
      __getNonce = require("lasso/taglib/helper-getNonce"),
      attr = __helpers.a;

  return function render(data, out) {
    out.w('<script' +
      attr("nonce", __getNonce(out)) +
      '>\n    Hello foo!\n</script>');
  };
}
(module.exports = require("marko").c(__filename)).c(create);

You can see from the following line that everything worked as expected:

attr("nonce", __getNonce(out))

Please let me know if there is another way I should try to reproduce. Thanks.

@tropperstyle
Copy link

Doh it was on my end. I was editing a file in node_modules/foo/template.marko but the actual file being loaded was nested under node_modules/another-page/node_modules/foo/template.marko - Which now has me paying closer attention to my dependency graph. Sorry about that. Looks like everything is working great now. Thanks!

@tropperstyle
Copy link

Looks like this output will also require the nonce attribute:
<span id="markoWidgets" data-ids="w0" style="display:none;"></span>

@patrick-steele-idem
Copy link
Contributor

Why do you say that @tropperstyle? There is no script on that line.

However, we will need to update Marko Widgets to not use eval() in CSP mode (see marko-widgets/lib/init-widgets-browser.js)

We use eval() for performance reasons, but there is no reason we can't use JSON.parse() (with a few minor changes).

@tropperstyle
Copy link

Inline styles are banned the same way as inline scripts and inline event handlers. Safari is throwing a CSP report on this line unless I add a nonce attribute or add unsafe-inline to style-src

@patrick-steele-idem
Copy link
Contributor

What world are we living in where inline styles are considered dangerous? :)

Ok, I propose we switch to the following:

<noscript id="markoWidgets" data-ids="w0"></noscript>

The <noscript> has no impacted on the view and is supported by all browsers so I think that will be a good change. Thoughts or concerns @tropperstyle?

@patrick-steele-idem
Copy link
Contributor

@tropperstyle @ppattanayak I opened a related issue on the Marko Widgets project to support CSP: marko-js-archive/marko-widgets#115

@patrick-steele-idem
Copy link
Contributor

New version published: [email protected]

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

No branches or pull requests

3 participants