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

[Content Security Poilicy] Filtering in x-data-grid broken after update to v6.10.1 due to eval #9771

Closed
superkartoffel opened this issue Jul 24, 2023 · 8 comments · Fixed by #9863
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature regression A bug, but worse

Comments

@superkartoffel
Copy link

superkartoffel commented Jul 24, 2023

Steps to reproduce 🕹

Steps:

  1. update your project to latest v6.10.1
  2. set content-security-policy header to include script-src: "'self'"
  3. open your application and try to use filter

Current behavior 😯

filters are not applied and console shows errors that eval expressions have been blocked due to content-security-policy.

Expected behavior 🤔

filters should work without having to enable unsafe-eval CSP header.

Context 🔦

I report this as a bug, since I consider it a breaking change in a patch version update. The same content-security-policy works fine with v6.10.0

The change has most likely been introduced in #9635 and can easily be worked around by including content-security-policy header script-src: 'unsafe-eval'. But I do not consider it safe to do so.

Hence my question: Is there another way of enabling eval expressions just for this script instead of globally?
Also, please update you changelog to warn others about this change.

A bit related to:

Your environment 🌎

Firefox and chrome are affected, I did not test any other browser.

Order ID or Support key 💳 (optional)

No response

@superkartoffel superkartoffel added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 24, 2023
@superkartoffel superkartoffel changed the title Filtering broken after update to v6.10.1 due to eval content-security-policy Filtering in x-data-grid broken after update to v6.10.1 due to eval content-security-policy Jul 24, 2023
@zannager zannager added the component: data grid This is the name of the generic UI component, not the React module! label Jul 24, 2023
@m4theushw m4theushw added regression A bug, but worse and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 24, 2023
@m4theushw
Copy link
Member

m4theushw commented Jul 24, 2023

@romgrk How about making the use of eval opt-in?

@cherniavskii
Copy link
Member

Yeah, I totally forgot that eval might be a problem for CSP 🙃

@peterhirn
Copy link

peterhirn commented Jul 31, 2023

eval usage also adds 300KB to my bundle size using Vite.

WARN ../../.yarn/virtual/@mui-x-data-grid-virtual-e60fa12330/0/cache/@mui-x-data-grid-npm-6.10.1-a169528a91-7ae6335b82.zip/node_modules/@mui/x-data-grid/hooks/features/filter/gridFilterUtils.js (171:21) Use of eval in "../../.yarn/virtual/@mui-x-data-grid-virtual-e60fa12330/0/cache/@mui-x-data-grid-npm-6.10.1-a169528a91-7ae6335b82.zip/node_modules/@mui/x-data-grid/hooks/features/filter/gridFilterUtils.js" is strongly discouraged as it poses security risks and may cause issues with minification.

@romgrk
Copy link
Contributor

romgrk commented Jul 31, 2023

I'll check if we can feature-detect eval at runtime.

@peterhirn 300KB seems excessive for what we do with eval, are you sure about those numbers? Could it be a bundler issue?

@peterhirn
Copy link

Yes, I'm sure. Changing "@mui/x-data-grid": "6.10.0" to "@mui/x-data-grid": "6.10.1" adds an additional 314.8 kb and outputs the following warning:

WARN ../../.yarn/virtual/@mui-x-data-grid-virtual-e60fa12330/0/cache/@mui-x-data-grid-npm-6.10.1-a169528a91-7ae6335b82.zip/node_modules/@mui/x-data-grid/hooks/features/filter/gridFilterUtils.js (171:21) Use of eval in "../../.yarn/virtual/@mui-x-data-grid-virtual-e60fa12330/0/cache/@mui-x-data-grid-npm-6.10.1-a169528a91-7ae6335b82.zip/node_modules/@mui/x-data-grid/hooks/features/filter/gridFilterUtils.js" is strongly discouraged as it poses security risks and may cause issues with minification.

I don't feel like creating a minimal reproduction of this issue. Project setup: yarn (Plug'n'Play), vite, react, mui (all latest version).

@romgrk
Copy link
Contributor

romgrk commented Aug 1, 2023

Size at 6.10.0: 406.1kb
Size at 6.10.1: 417.5kb

I'm not sure what's going on with your bundle but there might be an issue there. I would open an issue in your bundler's repo so we can get some assistance from the maintainers there, it will be easier to figure out.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 16, 2023

eval usage also adds a cool 300KB to my bundle size using vite.

@peterhirn I had a quick look. It seems to be because Terser (used by Vite) can no longer minify function names as eval() has global scope access. I got this lead from terser/terser#455. It's referenced in https://rollupjs.org/troubleshooting/#avoiding-eval too and terser seems to have an option for this: https://github.com/terser/terser/tree/27c0a3b47b429c605e2243df86044fc00815060f#mangle-options

Edit: Ah no, I'm wrong. Vite uses esbuild to minify now. https://esbuild.github.io/api/#minify-considerations

Screenshot 2023-08-16 at 18 24 28

@romgrk maybe we should hide the eval call to code analyzer tools? using something like window[atob('ZXZhbA==')]("true") (this trick works in my minimal reproduction).

esbuild docs encourage to use https://esbuild.github.io/content-types/#direct-eval:

// Indirect eval (has no effect on the surrounding code)
let result = (0, eval)(something)

would this work? It would isolate the expression from the global scope.

@romgrk
Copy link
Contributor

romgrk commented Aug 16, 2023

It's a bit different, eval does magic to preserve the lexical context in which it is called. Any indirect call doesn't:

image

I think we can work around that by adding another real closure around and passing arguments instead of closure values.

Let's move the discussion to #10056.

@oliviertassinari oliviertassinari changed the title Filtering in x-data-grid broken after update to v6.10.1 due to eval content-security-policy [Content Security Poilicy] Filtering in x-data-grid broken after update to v6.10.1 due to eval content-security-policy Aug 21, 2023
@oliviertassinari oliviertassinari changed the title [Content Security Poilicy] Filtering in x-data-grid broken after update to v6.10.1 due to eval content-security-policy [Content Security Poilicy] Filtering in x-data-grid broken after update to v6.10.1 due to eval Aug 21, 2023
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work feature: Filtering Related to the data grid Filtering feature labels Aug 21, 2023
kris7t added a commit to kris7t/refinery that referenced this issue Aug 31, 2023
Versions 6.10.1 use eval, which causes problems with CSP and minification:

mui/mui-x#9771
mui/mui-x#10056
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: data grid This is the name of the generic UI component, not the React module! feature: Filtering Related to the data grid Filtering feature regression A bug, but worse
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants