October: assets/js/framework.js forces the use of the 'unsafe-eval' Content-Security-Policy directive.

[MeLlamoPablo]

Expected behavior

The OctoberCMS backend works with the following Content-Security-Policy:

default-src 'none'; script-src 'self'; connect-src 'self'; img-src 'self'; style-src 'self';

Actual behavior

The OctoberCMS backend crashes because the code violates the previous Content-Security-Policy. Here is the error:

Uncaught Error: Error parsing the data-request-update attribute value. EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self' 'unsafe-inline' data: connect.facebook.net sdk.accountkit.com www.google-analytics.com".
    at paramToObj (framework.js?v1:459)
    at n.fn.init.$.fn.request (framework.js?v1:426)
    at Function.$.request (framework.js?v1:437)
    at HTMLDocument.<anonymous> (backend:238)
    at j (jquery.min.js?v1:2)
    at Object.fireWith [as resolveWith] (jquery.min.js?v1:2)
    at Function.ready (jquery.min.js?v1:2)
    at HTMLDocument.I (jquery.min.js?v1:2)

The error is produced at exactly this line in assets/js/framework.js.

This error doesn't occur when including 'unsafe-eval' in the CSP header, like so:

default-src 'none'; script-src 'self' 'unsafe-eval'; connect-src 'self'; img-src 'self'; style-src 'self';

This error occurs because eval is used to parse arbitrary JSON data from the client, I presume. I don't understand the reasoning for using eval > stringify > parse, as I believe that simply parse should be enough, and that would fix this issue.

Reproduce steps

1. Create a fresh OctoberCMS install.
2. Put it behind an NGINX reverse proxy.
3. Add the following directive to the NGINX configuration:
   add_header Content-Security-Policy "default-src 'none'; script-src 'self'; connect-src 'self'; img-src 'self'; style-src 'self';" always;
4. Navigate to https://my-site.test/backend, and log in

October build

v1.0.435

[LukeTowers]

Excellent question @MeLlamoPablo, I'll ping @daftspunk about it.

[daftspunk]

eval() probably is an unsafe way to handle what is a safe procedure. I think we originally tried it and eval() was the only approach that worked for the desired (now necessary) syntax. An alternative approach here would be welcomed.

[MeLlamoPablo]

So I've been playing with this for a bit. I've added a console.log that logs the parameters name and value, and started modifying stuff in the dashboard (adding blog posts, modifying templates, etc.), but the value parameter has always been an empty string.

I'm not familiar with the codebase, so if you want to point me in the right direction, I might be able to help with this.

[daftspunk]

IIRC to explain the issue, the framework is converting a "JSON-like" string in to a JS object. I say JSON-like string because it's actually a JS-object string. For example, this is a valid JS-object:

{ abc: 'value' } 

However, it is not valid JSON to parse JSON it requires this

{ 'abc': 'value' } 

Essentially it means the difference between

data-request-update="targetId: 'partial'"

and

data-request-update="'targetId': 'partial'" 

( note the extra '' )

If we can find a way to safely parse this format then we can replace the eval() code performing the same task at the moment.

[MeLlamoPablo]

Off the top of my head, the easiest way is JSON5. For example:

const json5 = require("json5")
console.log(json5.parse("{ hello: 'world' }")) // Prints { hello: 'world' }

That piece of code alone is 28KB minified and 7,9KB minified + gzipped, including the JSON5 dependency. Kind of sucks considering that's almost 50% of moment.js' build size, excluding locales, for essentially JSON with steroids. However if that's acceptable to you, then this would solve the problem right away.

Another solution could be some kind of regexes. For instance:

^([\w]+)\s*:\s*'(.+)'$
^([\w]+)\s*:\s*"(.+)"$

These one match the following:

hello: 'world'
hello: "world"

And don't match the following:

hyphens-do: "not-work"
spaces do: 'not-either'

For numbers you could use this one:

^([\w]+)\s*:\s*([0-9.]+)$

But then you need to make sure that the number is valid (i.e: is below the maximum and above the minimum, does not contain two dots, etc). I'm sure I'm missing some edge cases on the first ones too. This solution is a lot clumsier, but with proper testing I think it's doable. I'd send a PR but I'm having trouble running the tests (most of them are failing but I get no error messages or anything).

[LukeTowers]

@MeLlamoPablo feel free to submit a PR for review when able

[MeLlamoPablo]

I've taken a look into this, and the issue is worse than I imagined. I've submitted a PR with a proof of concept (#3622), but the issue is not solved with it alone. Here are some concerns:

1. The framework.js file also makes use of eval here and here. This is its own issue and I'm not sure how to proceed on this one. Maybe you can provide some context on why is eval used in there and how could we replace it.
2. The October Storm framework also has the problem described on this issue. I have solved it in the PR, but I haven't modified the compiled storm-min.js file because I can't find any build instructions. On top of that, we'd also need to include JSON5 whenever storm-min.js is used, or better yet, bundle JSON5 within storm-min.js.
3. To expand on the previous point, if we're going to proceed with the JSON5 approach (as opposed to the manual parsing approach I described before), we should be looking to keep JSON5 in just one place instead of bundling it twice, which I believe is also a bit of a challenge because it would be required in two different places (framework.js and October Storm), and there is no unified build system.

4. modules/backend/formwidgets/codeeditor/assets/vendor/emmet/emmet.js depends on Underscore 1.3.3, which also uses eval (in the form of new Function("...")) here. Doing some research, it appears that Underscore hasn't fixed this nor plans to (see jashkenas/underscore#2273).

   I'm not sure if we can drop the usage of _.template. Looking at emmetio/emmet, they seem to have dropped Underscore? I'm not sure if that's where you pulled the emmet.js file from.

5. The biggest point of all: jQuery also uses eval. Here's an example stack trace:

   jquery.min.js?v1:formatted:128 Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self' 'unsafe-inline' data: connect.facebook.net sdk.accountkit.com unpkg.com www.google-analytics.com".
   
       at eval (<anonymous>)
       at Function.globalEval (jquery.min.js?v1:formatted:128)
       at n.fn.init.domManip (jquery.min.js?v1:formatted:2366)
       at n.fn.init.append (jquery.min.js?v1:formatted:2265)
       at n.fn.init.n.fn.(anonymous function) [as appendTo] (https://www.our-site.test/modules/backend/assets/js/vendor/jquery.min.js?v1:3:17494)
       at storm-min.js?v1:217
       at Modal.backdrop (storm-min.js?v1:255)
       at Modal.show (storm-min.js?v1:216)
       at HTMLDivElement.<anonymous> (storm-min.js?v1:261)
       at Function.each (jquery.min.js?v1:formatted:151)
   

   Relevant StackOverflow question and blog post.

   I don't have enough knowledge of the codebase to say whether or not this is doable, but it seems to me like a lot work, and I'm not sure if we can keep backwards compatibility either.

So at this point, I'm not sure if it's worth it to put more work into this; I think you should be the ones making that call. Maybe we should just document that in order to use October you need to enable 'unsafe-eval' in the CSP.

However, if you decide to proceed, I'll be happy to help.

Thanks for your time!

[LukeTowers]

@MeLlamoPablo

Thanks for digging into this! The following are my responses:

1. I think we should probably look into the use of eval in framework.js to minimize it as much as possible. @daftspunk would have to provide some more background on the other uses of it

2. Whatever solution we do for framework.js should be replicated across the codebase. As a note, you compile assets in october by running php artisan october:util compile assets with the last argument allowed to be assets | js | less

3. I think JSON5 is a bit heavy handed for this particular case, if we could get a solid and simple parsing implementation to use instead of eval or JSON5 that would be ideal.

4. October's frontend dependencies are manually managed, which means that they're not updated unless someone makes an issue for it / submits the PR themselves, so Emmet & other dependencies could probably be updated.

5. This is really the showstopper in this case, I think this will automatically mean that we have to require unsafe-eval in the CSP and mention it in the documentation simply because of the prevalence of jQuery within October. I do think that it would be a good idea to minimize the other uses of eval within October's JS though.

[ghost]

@LukeTowers @daftspunk Hi I just wondered what the status of this issue is because I see many "high" labels dating back several years still open, so I'm worried that this issue may not get fixed any time soon.

I am having this issue with my CSP Level 3 only when I try to use October's AJAX. In Google Chrome it totally blocks it from working and I really hope you could get the AJAX working asap.

This is really the showstopper in this case, I think this will automatically mean that we have to require unsafe-eval in the CSP and mention it in the documentation simply because of the prevalence of jQuery within October. I do think that it would be a good idea to minimize the other uses of eval within October's JS though.

Luke's comment about jQuery I am not sure what the problem would be? I have jQuery V3.3.1 running on my websites with full CSP and have zero problems! With jQuery and XSS you only need to patch certain things for example .text() you wouldn't need to do anything and with .html() you would need to protect it. Protecting jQuery I use the library called: DOMPurify. Maybe you could just mention that in the doc's.

I really don't think October CMS needs to worry about jQuery at all with XSS and CSP. It's totally down to the developers to add the extra layers of security. jQuery know about this and they are adding it to V4 next year, there is an issue open about that in their github.

All that is needed to be fixed is the October's AJAX issue if possible, because it's a big problem for us not having that working at all on any modern browsers! Please work out a balance between performance and security.

[LukeTowers]

@michalise are you saying that jQuery no longer uses eval internally?

[ghost]

@LukeTowers

are you saying that jQuery no longer uses eval internally?

According to the developers working on jQuery they have said, jQuery uses eval on V3 and below. On V4 that comes out next year no that will be fixed and patched.

Google has already written a spec on this issue and sent it to various commonly used frameworks and polyfill io. So the small list of main frameworks are working on this, I saw a list of around 10 frameworks actively working on this issue.

So I really don't think that will be a problem for October CMS. However, it would be a good idea to update jQuery once a year on the backend to fix security issues and features.

Hope that helps.

[LukeTowers]

@michalise we'll have to revisit this when jQuery 4 comes out then. Right now I believe October is still on 2, there is an open PR to update to 3 but I haven't had time to review it thoroughly and no one has been asking for it either which makes it less of a priority. In the meantime, perhaps you could make PRs for the other use cases of eval to improve this situation for your own uses?

[ghost]

@MeLlamoPablo @LukeTowers

I like the idea of using JSON5 (it's a solid library), but I agree with Luke's point about it being too big in size. It seems like this is a JSON Parse Encoding and Decoding issue. Did you know that the JSON5 is built using the following code from here: https://github.com/douglascrockford/JSON-js/blob/master/json_parse.js

That code is 9Kb instead of 20Kb in size and a min version is this:

var json_parse=function(){"use strict";var e,f,n,i,a={'"':'"',"\\":"\\","/":"/",b:"\b",f:"\f",n:"\n",r:"\r",t:"\t"},u=function(r){throw{name:"SyntaxError",message:r,at:e,text:n}},o=function(r){return r&&r!==f&&u("Expected '"+r+"' instead of '"+f+"'"),f=n.charAt(e),e+=1,f},r=function(){var r,t="";for("-"===f&&o(t="-");"0"<=f&&f<="9";)t+=f,o();if("."===f)for(t+=".";o()&&"0"<=f&&f<="9";)t+=f;if("e"===f||"E"===f)for(t+=f,o(),"-"!==f&&"+"!==f||(t+=f,o());"0"<=f&&f<="9";)t+=f,o();if(r=+t,isFinite(r))return r;u("Bad number")},c=function(){var r,t,e,n="";if('"'===f)for(;o();){if('"'===f)return o(),n;if("\\"===f)if(o(),"u"===f){for(t=e=0;t<4&&(r=parseInt(o(),16),isFinite(r));t+=1)e=16*e+r;n+=String.fromCharCode(e)}else{if("string"!=typeof a[f])break;n+=a[f]}else n+=f}u("Bad string")},s=function(){for(;f&&f<=" ";)o()};return i=function(){switch(s(),f){case"{":return function(){var r,t={};if("{"===f){if(o("{"),s(),"}"===f)return o("}"),t;for(;f;){if(r=c(),s(),o(":"),Object.hasOwnProperty.call(t,r)&&u("Duplicate key '"+r+"'"),t[r]=i(),s(),"}"===f)return o("}"),t;o(","),s()}}u("Bad object")}();case"[":return function(){var r=[];if("["===f){if(o("["),s(),"]"===f)return o("]"),r;for(;f;){if(r.push(i()),s(),"]"===f)return o("]"),r;o(","),s()}}u("Bad array")}();case'"':return c();case"-":return r();default:return"0"<=f&&f<="9"?r():function(){switch(f){case"t":return o("t"),o("r"),o("u"),o("e"),!0;case"f":return o("f"),o("a"),o("l"),o("s"),o("e"),!1;case"n":return o("n"),o("u"),o("l"),o("l"),null}u("Unexpected '"+f+"'")}()}},function(r,a){var t;return n=r,e=0,f=" ",t=i(),s(),f&&u("Syntax error"),"function"==typeof a?function r(t,e){var n,f,i=t[e];if(i&&"object"==typeof i)for(n in i)Object.prototype.hasOwnProperty.call(i,n)&&(void 0!==(f=r(i,n))?i[n]=f:delete i[n]);return a.call(t,e,i)}({"":t},""):t}}();

So would it just be a good idea to use this code and the same style as the PR created already?

What do you think? That's my suggestion on this issue.

[LukeTowers]

@michalise I'll run it by @daftspunk

[ghost]

@LukeTowers @daftspunk

Hi Luke, call me thick but can't you just do this with JSON.stringify() ?

For example see my code:

<!DOCTYPE html>
<html>
<body>

<h1>Convert Javascript Object into a JSON String.</h1>

<p id="example"></p>

<script>
var obj = {abc: 'value'};
var myJSON = JSON.stringify(obj);
document.getElementById("example").innerHTML = myJSON;
</script>

</body>
</html>

The output will be:

{"abc":"value"}

Or maybe I am not seeing something here?

[LukeTowers]

@ayumihamsaki the problem is that we need to go the other direction here, we have a JS object in a string that we need to turn into a JS object in code. See https://github.com/octobercms/october/issues/3608#issuecomment-401237383

[ghost]

@LukeTowers I wonder if @w20k would like to look at it? This is a really big issue for me because I can't use October's AJAX and CSP at the same time. It's stopping some of my plugins working! Any possible way you could change it from low to medium. :laughing:

[nathan-van-der-werf]

I agree with @ayumihamsaki that this is a big issue (certainly not a "low" priority).

[w20k]

We could try to adopt the new JQuery approach ($.globalEval() -> what it does is quite interesting ;) ), they dropped eval from their code base. Only test plugin Sinon.JS has it (if I'm not mistaken)!

jQuery Doc URL: https://api.jquery.com/jquery.globaleval/

@nathan-van-der-werf or @ayumihamsaki if you have time, we could deal with testing this functionality in no time 🗡
But the one thing is, that we have to ask @daftspunk to take a look at jQuery upgrade #3697 from @ayumihamsaki, before we deal with testing ;)

[LukeTowers]

jQuery has been upgraded, can we get the ball rolling on testing the .globalEval approach?

[w20k]

Minor update: I've started testing $.globalEval(). And found one bug (fixable) doesn't work with an empty string as an input "" 👍

[w20k]

Update2: $.globalEval() didn't work well with OCMS approach, have an idea but needs refactoring. ~new Function() with bind to local context should do the trick.
_Side Note_: It won't work with non-html5 browsers. Shouldn't be a problem, I guess 😄

[w20k]

@ayumihamsaki, not sure I understand what you mean.

There are currently two issues with eval in frameworks.js:

1. String to object 'conversion'
2. In scope function execution

[ghost]

@w20k If it's ok with you, can I request you add this issue to your 'To Do List' please. :1st_place_medal:

[ghost]

@w20k @LukeTowers

Did a quick bit of research into this issue, would a possible solution to this issue be found here: http://www.relaxedjson.org/

You can see here (to get an idea of the file size): https://cdn.jsdelivr.net/npm/[email protected]/relaxed-json.min.js

You can also use their Sandbox tester, to test combinations that October may, found here: http://www.relaxedjson.org/docs/converter.html

[ghost]

@LukeTowers @w20k

<script>
"use strict";!function(){function e(e,r,n){for(var t="",o=r;o<e.length;o++){if(n&&n===e[o])return t;if(!n&&(" "===e[o]||":"===e[o]))return t;t+=e[o],"\\"===e[o]&&o+1<e.length&&(t+=e[o+1],o++)}throw new Error("Broken JSON syntax near "+t)}function r(e,r){if('"'===e[r]||"'"===e[r]){for(var n='"',t=r+1;t<e.length;t++)if("\\"===e[t])n+=e[t],t+1<e.length&&(n+=e[t+1]),t++;else{if(e[t]===e[r])return n+='"',{originLength:n.length,body:n};n+=e[t]}throw new Error("Broken JSON string body near "+n)}if("t"===e[r]){if(e.indexOf("true",r)===r)return{originLength:"true".length,body:"true"};throw new Error("Broken JSON boolean body near "+e.substr(0,r+10))}if("f"===e[r]){if(e.indexOf("f",r)===r)return{originLength:"false".length,body:"false"};throw new Error("Broken JSON boolean body near "+e.substr(0,r+10))}if("n"===e[r]){if(e.indexOf("null",r)===r)return{originLength:"null".length,body:"null"};throw new Error("Broken JSON boolean body near "+e.substr(0,r+10))}if("-"===e[r]||"+"===e[r]||"."===e[r]||e[r]>="0"&&e[r]<="9"){for(var n="",t=r;t<e.length;t++){if(!("-"===e[t]||"+"===e[t]||"."===e[t]||e[t]>="0"&&e[t]<="9"))return{originLength:n.length,body:n};n+=e[t]}throw new Error("Broken JSON number body near "+n)}if("{"===e[r]||"["===e[r]){for(var o=[e[r]],n=e[r],t=r+1;t<e.length;t++){if("'"===o[o.length-1]&&'"'===e[t]?n+='\\"':n+=e[t],"\\"===e[t])t+1<e.length&&(n+=e[t+1]),t++;else if('"'===e[t])'"'===o[o.length-1]?o.pop():"'"!==o[o.length-1]&&o.push(e[t]);else if("'"===e[t])"'"===o[o.length-1]?o.pop():'"'!==o[o.length-1]&&o.push(e[t]);else if('"'!==o[o.length-1]&&"'"!==o[o.length-1])if("{"===e[t])o.push("{");else if("}"===e[t]){if("{"!==o[o.length-1])throw new Error("Broken JSON "+("{"===e[r]?"object":"array")+" body near "+n);o.pop()}else if("["===e[t])o.push("[");else if("]"===e[t]){if("["!==o[o.length-1])throw new Error("Broken JSON "+("{"===e[r]?"object":"array")+" body near "+n);o.pop()}if(!o.length)return{originLength:t-r,body:n}}throw new Error("Broken JSON "+("{"===e[r]?"object":"array")+" body near "+n)}throw new Error("Broken JSON body near "+e.substr(r-5>=0?r-5:0,50))}function n(e){return"\\"!==e[0]&&(e[0]>="a"&&e[0]<="z"||e[0]>="A"&&e[0]<="Z"||"_"===e[0]||(e[0]>="0"&&e[0]<="9"||("$"===e[0]||e.charCodeAt(0)>255)))}function t(e){return" "===e||"\n"===e||"\t"===e}function parse(o){if(o=o.trim(),!o.length)throw new Error("Broken JSON object.");for(var i="";o&&","===o[0];)o=o.substr(1);if('"'===o[0]||"'"===o[0]){if(o[o.length-1]!==o[0])throw new Error("Invalid string JSON object.");return"'"===o[0]&&(o[0]='"'),"'"===o[o.length-1]&&(o[o.length-1]='"'),o}if("true"===o||"false"===o)return o;if("null"===o)return"null";var f=parseFloat(o);if(!isNaN(f))return f.toString();if("{"===o[0]){for(var l="needKey",i="{",h=1;h<o.length;h++)if(!t(o[h]))if("needKey"!==l||'"'!==o[h]&&"'"!==o[h]){if("needKey"===l&&n(o[h])){var a=e(o,h);i+='"',i+=a,i+='"',h+=a.length-1,l="afterKey"}else if("afterKey"===l&&":"===o[h])i+=":",l=":";else if(":"===l){var g=r(o,h);h=h+g.originLength-1,i+=parse(g.body),l="afterBody"}else if("afterBody"===l||"needKey"===l){for(var u=h;","===o[u]||t(o[u]);)u++;if("}"===o[u]&&u===o.length-1){for(;","===i[i.length-1];)i=i.substr(0,i.length-1);return i+="}"}u!==h&&"{"!==i&&(i+=",",l="needKey",h=u-1)}}else{var a=e(o,h+1,o[h]);i+='"'+a+'"',h+=a.length,h+=1,l="afterKey"}throw new Error("Broken JSON object near "+i)}if("["===o[0]){for(var i="[",l="needBody",h=1;h<o.length;h++)if(" "!==o[h]&&"\n"!==o[h]&&"\t"!==o[h])if("needBody"===l){if(","===o[h]){i+="null,";continue}if("]"===o[h]&&h===o.length-1)return","===i[i.length-1]&&(i=i.substr(0,i.length-1)),i+="]";var g=r(o,h);h=h+g.originLength-1,i+=parse(g.body),l="afterBody"}else if("afterBody"===l)if(","===o[h])for(i+=",",l="needBody";","===o[h+1]||t(o[h+1]);)","===o[h+1]&&(i+="null,"),h++;else if("]"===o[h]&&h===o.length-1)return i+="]";throw new Error("Broken JSON array near "+i)}}("undefined"==typeof exports?window.octoberJSON={}:exports).parse=function(e){var r=parse(e);return JSON.parse(r)}}();

var test = octoberJSON.parse('{foo: "bar",baz: true}');
console.log(test);

var test2 = JSON.parse('{foo: "bar",baz: true}');
console.log(test2);
</script>

Above: Image show the first method octoberJSON working and second method JSON not working.

Let me know if this is on the right track to fixing this issue?

[w20k]

@ayumihamsaki I guess so, but still, I'm not sure if it's a good idea to add a new layer of complexity just because of the two eval functions being used inside of framework.js.

[ghost]

I'll try and create a pr today and list all the advantages and disadvantages of the pr.

[daftspunk]

This should be fixed in Build 460+ (ebcb7ee6fde73dc2b52afefd8d28c715df428446)

[tobias-kuendig]

This problem still exists. For example, when closing a relation modal, the following line is triggered, which still uses eval calls:

https://github.com/octobercms/october/blob/3a577cbbbb29b513072d5f18ab71d13d436779b4/modules/system/assets/js/framework.js#L153

<button ...
    data-request-success="$.oc.relationBehavior.changed('<?= e($this->vars['relationField']) ?>', 'deleted')">
    <?= e(trans('backend::lang.relation.delete')) ?>
</button>

Also, something in the codeeditor form widget triggers an error as well.

Without allowing unsave-eval you are not able to close any relation popup as the onSuccess handler is blocked. You also get a JS error in the console on every page that includes the codeeditor widget or the richeditor widget (as it includes the codeeditor build.js as well)

[LukeTowers]

@tobias-kuendig are you able to submit a PR to fix the issue? That should probably be a different issue.

[tobias-kuendig]

I do wonder if this can be fixed without breaking changes. The JS framework essentially allows to provide a function body in the data-request-success attribute that is then executed. I don't think there is a way around eval'ing this code. Alternatives like the new Function() constructor are blocked as well by the CSP headers.

Replacing all the data-request parts with explicit inline <script> sections would be an option. Even better would be to use a nonce for these script sections, but I don't think there is an elegant way to merge CSPs: If the server itself already sends a header, we cannot provide the nonce to the document without somehow detecting and manually merging all the rules.

What do you think?

[bennothommo]

@tobias-kuendig When I had a look at this, months ago, I came to the same conclusion. However, it's my understanding that the only time unsafe-eval is reported is when it's actually run, not that it's in the script. So if you use straight JavaScript and don't use the data attributes, you should be fine.

I'm not the biggest fan of allowing the JavaScript to be eval'd, but it's been in there since day one AFAIK, so it's not something we can just drop unfortunately.

We could perhaps offer a version of framework.js that does not have the eval stuff in there, with the caveat that it won't allow some of the data attributes to work, but it does mean duplicate code and double the maintenance.

[tobias-kuendig]

it's my understanding that the only time unsafe-eval is reported is when it's actually run

That is correct. The problem mostly arises when a user clicks on the "submit" button on a relation modal. The data-request-success call linked above will cause a CSP error, which leads to the popup overlay not being hidden correctly (the spinner remains visible).

[LukeTowers]

@tobias-kuendig in that case then it should be a simple enough fix, just refactor the relation modal to use the JS API instead of the data-request attributes.

[wordshop-git]

I think this is still happening in build 469?

framework.min.js is located here: /modules/system/assets/js/framework-min.js

Is there any way around this or do we just have to run unsafe-eval in our CSP

[LukeTowers]

@wordshop-git you could make a PR implementing my suggested fix in the comment above.

[ayumi-cloud]

@LukeTowers

Closing because the original issue contains our proposed fix (i.e. don't use the data-request API if you need CSP support and refactor the internals that use the data-request API to use the JS API instead).

Two things I want to ask.

1. Say a user links the framework.js file in their frontend and doesn't use data-request API. What's to stop a bad-actor from doing something bad with the eval() as the framework.js file has now been loaded into the browser. (I mean just because the developer is not using the eval doesn't stop a hacker from executing it from the client-side).

Q: Would it not be safer to remove a few lines of code out of the framework.js on a production mode website, to be on the safe side?

1. Please could you give a code example, how to convert this code:

<button
    data-request="onAccept"
    data-request-success="document.location.reload();document.querySelector('.example').remove();"
    data-request-update="'' : ''">Accept</button>

public function onAccept()
{
    // do something
}

[mjauvin]

@ayumi-cloud for #2, it's all explained with many details here:

https://octobercms.com/docs/ajax/javascript-api

But if you want it spelled out, just use this:

<button
    onClick="$(this).request('onAccept', {
         update: {}, 
         success: {} 
    })" 
>
Accept
</button>
)" 

or something similar, I typed it from memory.

[ayumi-cloud]

@mjauvin that's really helpful and gives me a pretty good idea! One thing, the onClick would probably get flagged by the csp. Would the October JavaScript API still work converting it to using an event.listener instead?

https://csp.withgoogle.com/docs/adopting-csp.html

[mjauvin]

Yes, of course, that was the easier way for me to type this quickly on my mobile...

[ayumi-cloud]

@LukeTowers @bennothommo @mjauvin

Surely the solution would be just to create another framework.js file with all the data-request API code stripped out (which will probably decrease the file size). I'm going to guess the 4 eval code lines are related to the data- attributes.

Say framework.js with both data-request API and October JavaScript API.

Then october-api.js with just October JavaScript API and all the data-request API stripped out.

The october-api.js containing the breaking changes shouldn't hurt anyone's existing code and we can move this issue forward to a working solution?

p.s. Luke's better at naming things than me!

Problematic Code Lines

https://github.com/octobercms/october/blob/5591e3fa945ad3c181ccedd9b759e12127b09f3e/modules/system/assets/js/framework.js#L146

https://github.com/octobercms/october/blob/5591e3fa945ad3c181ccedd9b759e12127b09f3e/modules/system/assets/js/framework.js#L168

https://github.com/octobercms/october/blob/5591e3fa945ad3c181ccedd9b759e12127b09f3e/modules/system/assets/js/framework.js#L215

https://github.com/octobercms/october/blob/5591e3fa945ad3c181ccedd9b759e12127b09f3e/modules/system/assets/js/framework.js#L225

[ayumi-cloud]

I should add, I also see many people in the October community use CSP incorrectly!

CSP should be added at a page level and not at a domain level. There are plugins allowing people to add a CSP policy at a domain level. Also there are organizations like HTML5 Boilerplate allowing users to add a CSP policy into their server, which will dump the same policy on every single page on the website.

I give a simple example why this is totally flawed. I have an October website and it has two frontend pages. The first being a contact form using Google Re-Captcha and the second page is an info page. On the contact form page I need to whitelist the Google Re-Captcha code, but on the info page I don't need to add the whitelist for Google Re-Captcha code. Because the CSP is injected into the HTTP Header section. You should try and reduce your CSP size as much as possible!

Now the reason I'm saying this is because when you add CSP to the domain. It will run on the frontend and backend of the October CMS. There is completely no point in running a CSP policy in the backend of the CMS. If a hacker can inject into a backend page, they wouldn't care about trying to bypass the CSP policy. They have more important things as they now have complete access to your website! Also October has a ton of protection to prevent illegal access into the backend.

This means that people should only care about the framework.js that manages the code on the frontend. So the framework files for the backend and all the form widgets that use the data-request API in the backend, should not be upgraded or changed.

The PR solution should just focus on the frontend only and not need to worry about trying to update any code in the backend.

The backend can use the data-request API or the October JavaScript API, whereas the frontend needs to only use the October JavaScript API and not show any eval code.

This simple fact reduces the complexity of this issue.

[bennothommo]

@ayumi-cloud I agree with everything above - the only issue is that it is practically guaranteed that people have used the Data Request API on the frontend, not to mention components from plugins and the like. We can't simply drop the Data Request API from the framework for the frontend.

I had a think about this the last time it came up, and I felt that the only way we could do this in a way that maintains backwards compatibility, allows the data request API to be dropped by the choice of the developer AND is easily maintainable is to make the AJAX framework more modular than it currently is.

I envisaged that the default framework.js would support both methods, and then an optional addon (kinda like framework.extras.js) would be attached to the framework that would disable the Data Request API for people using CSP policies.

[LukeTowers]

API code stripped out (which will probably decrease the file size)

Unlikely, the underlying logic is relatively small to begin with, and the lines that add support for data attributes probably number under 20 total.

Say a user links the framework.js file in their frontend and doesn't use data-request API. What's to stop a bad-actor from doing something bad with the eval() as the framework.js file has now been loaded into the browser. (I mean just because the developer is not using the eval doesn't stop a hacker from executing it from the client-side).

You're going to have to provide me a concrete example of how a hacker would actually achieve this without having JS access to begin with.

[tobias-kuendig]

Surely the solution would be just to create another framework.js file with all the data-request API code stripped out (which will probably decrease the file size). I'm going to guess the 4 eval code lines are related to the data- attributes.

On a tangential note, I once did try to implement the October framework completely without jQuery or any eval calls. Currently, it is just a prototype and hopefully I will find time to rewrite/complete it someday. Maybe someone can use it as inspiration in the meantime:

https://github.com/OFFLINE-GmbH/OctoberTS

I should add, I also see many people in the October community use CSP incorrectly!

Definitely true, @ayumi-cloud. As the author of such a plugin I have noticed this problem as well. I do however like the domain-level definition with the option to override the CSP for certain pages via event listeners, for example. This is a feature we will add to our CSP plugin soon.

[ayumi-cloud]

Just want to say thanks to @LukeTowers and @mjauvin for your help.

I got everything working fine my end. I wrote everything in vanilla js. I used passive event listeners to call my functions, see example:

function onAccept() {
    $(event.currentTarget).request('onAccept');
    // Run any custom js code
}

Testing I got zero issues with csp level 3 running.

In the near future I look forward to using @bennothommo modular suggestion and/or @tobias-kuendig TypeScript implementation. For me the TypeScript would be a best fit as all my vendor files are using TypeScript and I'm use to writing vanilla js connecting up to them.