[datagrid] Hide eval from bundlers

[romgrk]

Fixes #10056

Hide eval calls so bundlers can minify.

Before	After

[mui-bot]

Netlify deploy preview

Netlify deploy preview: https://deploy-preview-10329--material-ui-x.netlify.app/

Updated pages

No updates.

These are the results for the performance tests:

Test case	Unit	Min	Max	Median	Mean	σ
Filter 100k rows	ms	-203.3	-89.7	-203.3	-133.64	44.506
Sort 100k rows	ms	663.7	1,212.1	1,204.5	1,088.24	212.697
Select 100k rows	ms	563.1	727.6	688.9	666.8	56.011
Deselect 100k rows	ms	124.9	196.6	190.7	166.78	32.101

Generated by 🚫 dangerJS against d68ecd3

[oliviertassinari]

Great to see this performance issue fixed

[oliviertassinari]

From a security perspective, it would feel safer to block eval from accessing the global scope. What prevents using of the safer alternative to window.eval?

[romgrk]

Wdym by blocking access to the global scope? I'm not sure I understand, do you have an example of the concern you have and how it can be exploited? And how would the alternatives be safer? And by alternative you mean something like new Function?

[oliviertassinari]

concern

I wonder about security teams that we audit their codebase, they might be a bit scared of a non scoped eval call. Reviewing what gets evaluated inside these calls takes extra time for them. If we don't need access to global scope, then it feels safer to block its use in the first place.

alternatives

I mean the ones suggested in https://esbuild.github.io/content-types/#direct-eval indirect eval or new Function.

[romgrk]

We're already doing an indirect eval:

indirect	direct

eval has a bit of magic, it's not a normal function, as soon as it's re-assigned it loses its magic.

[oliviertassinari]

I would expect to see undefined in the console log output. I think the potential security concern is that it can read x in the global scope.

[romgrk]

Why is it a security concern? Any code can read the global scope, that's the point of global stuff ^^ And indirect eval doesn't restrict access to global scope, it restrict access to the local scope that eval has access to. The following paragraph from the esbuild link above explains it:

Modern bundlers contain an optimization called "scope hoisting" that merges all bundled files into a single file and renames variables to avoid name collisions. However, this means code evaluated by direct eval can read and write variables in any file in the bundle! This is a correctness issue because the evaluated code may try to access a global variable but may accidentally access a private variable with the same name from another file instead. It can potentially even be a security issue if a private variable in another file has sensitive data.

This is not an issue anymore with the new implementation in this PR because our eval'd function is now pure and only reads its input parameters. In addition to being indirect, which ensures it can't access its local scope.

[oliviertassinari]

OK thanks, it sounds like my previous point doesn't hold then. It's part of the challenge with using eval.

I'm left with only one aspect: the atob could feel a bit like we want to hide something 😁, using JavaScript API directly could be 👌, easier to audit for developers.

[romgrk]

But we do want to hide our eval calls from the bundlers to avoid them skipping optimizations ^^ Wdym by using javascript API directly?

[oliviertassinari]

Wdym by using javascript API directly?

Indirect eval or new Function, the ones I understand as the bundlers won't trigger deoptimization on and would also not be picked up by security audit software.