pagy_nav_bootstrap_responsive and CSP unsafe-inline #52

Bahanix · 2018-06-20T14:50:42Z

Hello, using pagy_nav_responsive or pagy_nav_bootstrap_responsive, I got this CSP error because of an inline <script> injected in the view:

Refused to execute inline script because it violates the following Content Security Policy directive: "script-src 'self' […] 'nonce-[…]'". Either the 'unsafe-inline' keyword, a hash ('sha256-[…]'), or a nonce ('nonce-...') is required to enable inline execution.

A way to fix without introducing the inline-safe vulnerability would be to replace theses <script> (eg. here) by something like:

<script type='application/json' class='pagy-responsive'>
{ id: #{id}, tags: #{tags}, widths: #{responsive[:widths].to_json}, series: #{responsive[:series].to_json} }
</script>

So one can process them in the main JS, eg:

window.addEventListener("turbolinks:load", function() {
  Array.from(document.getElementsByClassName("pagy-responsive")).forEach(function(pagination) {
    json = JSON.parse(pagination.innerHTML);
    PagyResponsive(json.id, json.tags, json.widths, json.series);
  });
});

Is that something Pagy may want? Do I try a PR?

Thanks!

The text was updated successfully, but these errors were encountered:

ddnexus · 2018-06-20T16:01:53Z

Hi @Bahanix. Yeah, the inline script maybe a little brutal, I know :)
The reason is there is that is almost self contained, in the sense that the user has only to add the pagy-responsive.js and the helper will take care of the rest.

Of course your PR would be welcome! However, please, keep in mind this 2 concepts:

the changes should not require any more writing from the user if possible (and I think it is)
the changes should be generic as much as possible (e.g. "turbolink:load" would be very specific)

If you have different possible alternative about how to implement it, please, feel free to discuss it in the chat in advance.

Thanks!

ddnexus · 2018-06-20T16:18:08Z

What about using a nonce? Maybe passing it as an extra variable, so it will become totally optional and its generation will be up to the user.

ddnexus · 2018-06-20T19:30:03Z

Actually, the only advantage of a nonce is that it wouldn't break the current implementation, but using it is probably more complex than using your approach of removing the inline script all together.

We could create a PagyResponsiveLoad function that will encapsulate the code for executing all the PagyReponsive calls (your code), and it could be used in an addEventListener like for example:

window.addEventListener("turbolinks:load", PagyReponsiveLoad)

That will break the current implementation and require to write an extra line, but it would be quite easy to use.

Waiting for your PR ;)

Bahanix · 2018-06-21T10:25:50Z

My WIP: Bahanix@dc0f24e

I had to replace a lot of simple-quotes and double-quotes since valid JSON need double-quotes for strings and keys to be parsed through JSON.parse. (I still did not updated tests for this quite painful change)

For testing purpose, I am writting the JS part in my app before adding it in Pagy:

  Array.from(document.getElementsByClassName("pagy-responsive")).forEach(function(pagination) {
    PagyResponsive.apply(null, JSON.parse(pagination.innerHTML));
  });

Note that function.apply(null, array) is a JS equivalent to ruby function(*array). I think it may be useful if we end by using this pattern for every Pagy JS extra that currently inject JS in the view.

So, it correctly triggers the Pagy render method on resize, but it does nothing. At this point I had no idea why, so I disabled CSP to give it a try without my WIP. It doesn't work, because my JS is included in the bottom of my <body> for performance purpose, so the PagyResponsive is still not defined when the Pagy <script> are injected. I think it makes this PR a bit more essential!

So, still for testing purpose, I moved my application.js to the <head> and it still not work. At this point I have no more idea to continue my work, I take a break :)

ddnexus · 2018-06-21T12:16:21Z

@Bahanix I am using the function.apply(null, array) in the global refactoring of the pagy.js passing it a simple array as in your example. It is just wrapped by a loop of the 3 extras.

Pagy.init = function(){
              ['compact', 'items', 'responsive'].forEach(function(name){
                Array.from(document.getElementsByClassName("pagy-"+name)).forEach(function(json) {
                  Pagy[name].apply(null, JSON.parse(json.innerHTML))
                })
              })
            };

I will push to dev the refactoring in a few hours. It works well for all the extras already, but I have just to adapt the tests and update the doc. Thanks for your effort and help.

): - removed inline scripts from all extras - one single pagy.js file shared among all the extras - the Pagy.init function should be executed at document load - updated tests and docs

ddnexus · 2018-06-21T13:55:53Z

@Bahanix please, try the dev branch and let me know if the docs is clear enough and if it works. Thanks.

Bahanix · 2018-06-21T15:41:23Z

@ddnexus it works perfectly!

ddnexus · 2018-06-21T17:08:55Z

Great then. Thanks.

…ty (#52): - removed inline scripts from all extras - one single pagy.js file shared among all the extras - the Pagy.init function should be executed at document load - updated tests and docs Co-authored-by: Giulien Grillot <julien.grillot@gmail.com>

ddnexus added WIP enhancement labels Jun 21, 2018

ddnexus added implemented and removed WIP labels Jun 21, 2018

ddnexus removed the implemented label Jun 21, 2018

ddnexus closed this as completed Jun 21, 2018

ddnexus mentioned this issue Jul 31, 2018

Semantic UI nav helper #73

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pagy_nav_bootstrap_responsive and CSP unsafe-inline #52

pagy_nav_bootstrap_responsive and CSP unsafe-inline #52

Bahanix commented Jun 20, 2018 •

edited

Loading

ddnexus commented Jun 20, 2018 •

edited

Loading

ddnexus commented Jun 20, 2018

ddnexus commented Jun 20, 2018

Bahanix commented Jun 21, 2018 •

edited

Loading

ddnexus commented Jun 21, 2018 •

edited

Loading

ddnexus commented Jun 21, 2018

Bahanix commented Jun 21, 2018

ddnexus commented Jun 21, 2018

pagy_nav_bootstrap_responsive and CSP unsafe-inline #52

pagy_nav_bootstrap_responsive and CSP unsafe-inline #52

Comments

Bahanix commented Jun 20, 2018 • edited Loading

ddnexus commented Jun 20, 2018 • edited Loading

ddnexus commented Jun 20, 2018

ddnexus commented Jun 20, 2018

Bahanix commented Jun 21, 2018 • edited Loading

ddnexus commented Jun 21, 2018 • edited Loading

ddnexus commented Jun 21, 2018

Bahanix commented Jun 21, 2018

ddnexus commented Jun 21, 2018

Bahanix commented Jun 20, 2018 •

edited

Loading

ddnexus commented Jun 20, 2018 •

edited

Loading

Bahanix commented Jun 21, 2018 •

edited

Loading

ddnexus commented Jun 21, 2018 •

edited

Loading