CSP violation: Unsafe-eval

[Dassderdie]

Expected behavior and actual behavior.

I'd like to use this library without having to weaken the Content Security Policy of our application.

It seems like eval is used in the source code of this library (component.js line 150, main.js line 25). This isn't a good practice because of the potential XSS-vulnerabilities that come along with it.

On this note I would like to see information about the security against XSS-vulnerabilities of this package in the README.md.

Steps to reproduce the problem.

Our CSP:

default-src 'self';script-src 'self';frame-src 'self' blob:;frame-ancestors 'self';worker-src 'self';object-src 'none';font-src 'self';style-src 'self' 'unsafe-inline';img-src 'self' data: blob:;media-src 'self' blob:;upgrade-insecure-requests

The Error:

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'".
...
    at window.canvasDatagrid.window.require.window.canvasDatagrid (canvas-datagrid.js:1)

[Dassderdie]

@ndrsn Any update on this?

[TonyGermaneri]

eval shows up in two places in the source 1, 2

Both of these can be replaced with a function constructor. The constructed function can then be invoked immediately after definition. It's a small transparent change.

In the first case, it prevents ES6 class constructor syntax, which is required to instantiate a web component, from causing syntax errors in ES5 browsers. We can 1) drop support for es5 browsers (ideal), 2) create two different build artifacts (hard) or 3) allow function constructor to hide the ES6 syntax still.

In the second case, it is creating a proxy for the internal event hook for the web component. This can likely be replaced with another pattern if the function constructor is undesirable from a security point of view.

In all cases we can remove eval(). From a security point of view, there is no risk here, but I understand the need to streamline the DAST process.

PRs are always welcome!

[Dassderdie]

I think this issue isn't fixed yet. https://github.com/TonyGermaneri/canvas-datagrid/pull/322 only changed eval() to Function(), but according to MDN (search for Unsafe eval expressions):

if 'unsafe-eval' isn't specified with the script-src directive, the following methods are blocked and won't have any effect: eval(), Function().

If I understand the code correctly, this eval is needed to correctly handle functions passed via on-event-handlers when using the web-component. Here is some information about passing a function to a web-component...

(Note: I haven't tested it yet. Maybe with the new support of es6-modules it is now possible to shake the (in my case) unused web-component code, but I don't really know much about it...)

[TonyGermaneri]

Really sorry that eval() and Function() are treated the same. It was not my experience with previous lint/dast tools.

The real reason for eval() or Function is to prevent ES5 only browsers from choking on Reflect.construct(HTMLElement, [], new.target) because the syntax is invalid in ES5. If we drop support for ES5, or make another build for ES5 we can get around this problem. I don't think we can transpile our way out of the function @ndrsn? The specific syntax is used to create the web component. Perhaps there is another way? Maybe we could have web component specific build?

It's been a number of years since ES5 (IE support) was decided. I'm for dropping ES5 altogether. @ndrsn are you aware of much IE use?

For the web component attribute change problem. This is a really confusing problem, I'm not sure it has a solution.

We'll need some way of adding event listeners to the web component that does not use Function or eval().

attributeChangedCallback

Where it gets attached to the web component

The main problem is that attributeChangedCallback passes values as STRINGS. Why? Someone figured that would be a good idea. It really wasn't in my opinion. But I guess it makes it portable for SSR or something.

The last option, which is a breaking change so I would like to avoid it is to prevent event listener setters and force the use of addEventListener. I don't know how many people use the onxxxx = fn function, but I personally feel it's an anti-pattern, please don't point out all the places I use it, I am pond scum, I know. So I'm pro dropping support, but I'm anti-breaking changes, but I recognize we need to sometimes break legacy things to have nice new things.

I think we should drop support for IE and onxxxx = fn. I'd love to hear from you guys what you think.

[ndrsn]

@TonyGermaneri not aware of much IE use, and considering that IE itself will no longer be supported next August, I'd totally be in favor of dropping support now if it helps us move forward.

@Dassderdie Apologies for closing this prematurely, by the way — I merged the proposed fix in a hurry, but should have tested first. Thanks for the heads up.

[Dassderdie]

What is the current stand on this issue?

Maybe there could be some sort of compromise with the constructor issue like this:

try {
    // legacy workaround for es5
    eval('Reflect.construct(HTMLElement, [], new.target)');
} catch {
    // if eval is forbidden via csp and es6 is supported
    Reflect.construct(HTMLElement, [], new.target);
}

[ndrsn]

No change since November, unfortunately. I feel we could improve this project and get some things moving if we're OK with no longer attempting to support legacy browser versions. I would therefore be totally on board with dropping ES5 support.

I've got some other priorities I need to attend to work-wise, so if you're so inclined, we'd greatly appreciate a pull request for this? Otherwise, it might be another few weeks.

[ndrsn]

Whoops, I moved a little too fast here. That didn't quite work. Perhaps after the weekend!

[kemsky]

@ndrsn, @TonyGermaneri , is it possible to use polyfills in this case?

https://github.com/google/closure-compiler/blob/2725a4f3b73d1536a9795424ae56bea07c949dd3/src/com/google/javascript/jscomp/js/es6/util/construct.js

https://github.com/WebReflection/classtrophobic-es5

https://github.com/zloirock/core-js

[AgustinBanchio]

Is this still being looked at?

We have this warning on our builds:

Use of eval in "node_modules/.vite/deps_build-dist/canvas-datagrid.js" is strongly discouraged as it poses security risks and may cause issues with minification.