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

Fix #1139 - Flagged all packages with Function, eval, or with statement in dist #1142

Merged
merged 1 commit into from
Nov 24, 2022
Merged

Fix #1139 - Flagged all packages with Function, eval, or with statement in dist #1142

merged 1 commit into from
Nov 24, 2022

Conversation

WebReflection
Copy link
Contributor

While checking all dist files and folders in search for code evaluation as AsyncFunction constructor (alpine), Function (many others), explicit eval or indirection to reach a with statement in code, I've noticed the following cases:

  • the architecture requires usage of evaluation by author's choice.
  • the architecture requires code evaluation due WASM.
  • it's actually the bundler that puts in code evaluation.
  • it's possible that some library/framework allows evaluation through <script> tags, so I might have missed some but it's hard to understand if that's the case

I believe the purpose of the flag is to inform libraries and frameworks consumers that they need special CSP rules or the code might break in the wild. At the same time, I believe developers should be aware about poor choices previously made by bundlers, as example new Function('return this') to reach the global context. In the WASM case, I also believe developers should be aware special care of the CSP rules must be taken.

As result, all dist used to demo the benchmark in here that contains Functions either intentionally or as side effect due outdated or poor bundlers should be flagged equally as potentially problematic if deployed with those tools or those requirements (WASM) in the wild.

@WebReflection
Copy link
Contributor Author

P.S. if anyone believes I've made some mistake please let me know and I'll update the MR. One can imagine how tiresome this MR has been, crawling all dist folders of all benchmarks and reading some minified production artifact to understand if there is evaluation or not (I don't even trust minifiers not polluting the code there) so please forgive me if I've added your library by accident or if I've forgotten some other library to flag.

@pygy
Copy link

pygy commented Nov 22, 2022

I just ckecked Mithril and found two instances, both added by the bundler, and which should be dead code as Mithril doesn't rely on these:

The first is the scenario you describe (a globalThis polyfill) but is IMO a false positive because the eval statement is nested in a try block, with fallbacks that AFAIK work in iframes, even with a strict CSP. Not sure the fallback would work in a worker, but that concern is academic, running a Web framework without any access to the DOM sounds contrieved.

function(e, t) {
    var n;
    n = function() {
        return this
    }();
    try {
        n = n || Function("return this")() || (0, eval)("this")
    } catch (e) {
        "object" == typeof window && (n = window)
    }
    e.exports = n
}

The other one comes from a setImmediate() polyfill. We don't call it, but maybe the bundler injected code does (no time to review the whole thing).

            function r(e) {
                "function" != typeof e && (e = new Function("" + e));

Anyways, since both are dead code (to us), I'll try to update the bundler or tweak its params to get rid of them.

@WebReflection
Copy link
Contributor Author

WebReflection commented Nov 22, 2022

@pygy thanks for double checking. I believe many others have the same issue but it’s no eval at all or flag to make it fair with anyone that kept the bench updated or anyone else that cares about not having by any chance breaking code under CSP rules.

Happy to update the MR and drop Mithril from flag

@fabiospampinato
Copy link
Contributor

I didn't realize that some bundlers and some polyfills used runtime code generation 🤔 On one hand that's not necessarily representative of the framework itself, but the underlying bundler or polyfill can be replaced with a better one at the author's discretion, and on average it seems still better to flag all those rather than not to flag anyone.

@WebReflection
Copy link
Contributor Author

@fabiospampinato

On one hand that's not necessarily representative of the framework itself

true, but it represents the bundling choice and not enough care around the produced output. I could tell anyone my library is CSP errors free, but if the tooling needed to ship such library makes it CSP hostile, then I believe it's on me to fix that and/or care about it.

the underlying bundler or polyfill can be replaced with a better one at the author's discretion

precisely, which is why I believe any presence of evaluation should be flagged, also because many might use this benchmark as reference on how to build "performance oriented production code" out there ... so I believe flags are deserved, but also I'll be super happy to drop 'em all when it comes to bundler shenanigans that might have been overlooked.

@aclueless
Copy link
Contributor

I think spair-qr unfortunately has the same issue because it is also WASM. Hoping some improvments in wasm-bindgen to solve this.

@aclueless
Copy link
Contributor

I mean spair-qr should be flagged with 1139 as well.

@krausest krausest merged commit 442e59a into krausest:master Nov 24, 2022
This was referenced Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants