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

CSP that blocks unsafe eval errors on regenerator-runtime #954

Closed
rbrewington opened this issue Jan 12, 2021 · 7 comments
Closed

CSP that blocks unsafe eval errors on regenerator-runtime #954

rbrewington opened this issue Jan 12, 2021 · 7 comments
Labels
kind: support Asking for support with something or a specific use case scope: upstream Issue in upstream dependency solution: workaround available There is a workaround available for this issue

Comments

@rbrewington
Copy link

rbrewington commented Jan 12, 2021

Current Behavior

I've created a library which compiles and works fine. I can import it in my app and everything runs locally.
When I deploy the app to a production environment, the app cannot load and gives an Uncaught EvalError
Screen Shot 2021-01-12 at 1 51 28 PM

The app had no CSP issues prior to introducing tsdx (previously my library was bootstrapped by create-react-library)

Expected behavior

I can run an app with a library dependency bootstrapped by tsdx in an environment using a CSP

Suggested solution(s)

I'm not really sure how to fix it, but it seems like regenerator-runtime is running in strict mode but it's not supposed to be?
It looks like this might have been changed in later versions of regenerator-runtime, so maybe updating it would work?

here's the block from regenerator-runtime that the CSP gets tripped up on
(specifically the line in the catch block):

try {
  regeneratorRuntime = runtime;
} catch (accidentalStrictMode) {
  // This module should not be running in strict mode, so the above
  // assignment should always work unless something is misconfigured. Just
  // in case runtime.js accidentally runs in strict mode, we can escape
  // strict mode using a global Function call. This could conceivably fail
  // if a Content Security Policy forbids using Function, but in that case
  // the proper solution is to fix the accidental strict mode problem. If
  // you've misconfigured your bundler to force strict mode and applied a
  // CSP to forbid Function, and you're not willing to fix either of those
  // problems, please detail your unique predicament in a GitHub issue.
  Function("r", "regeneratorRuntime = r")(runtime);
}

Additional context

Here are some related(?) issues that were all I could really find when searching this
facebook/regenerator#336
babel/babel#12106
indico/react-overridable#2
mozilla/pdf.js#11036

Your environment

System:
    OS: macOS Mojave 10.14.6
    CPU: (8) x64 Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
    Memory: 201.23 MB / 16.00 GB
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 14.15.1 - /usr/local/bin/node
    Yarn: 1.22.10 - /usr/local/bin/yarn
    npm: 6.14.8 - /usr/local/bin/npm
  Browsers:
    Safari: 14.0.2
  npmPackages:
    tsdx: ^0.14.1 => 0.14.1 
    typescript: ^3.7.5 => 3.9.7 
  npmGlobalPackages:
    typescript: 3.1.6
@agilgur5 agilgur5 changed the title Compiled library cannot be imported into an app with a CSP that blocks unsafe Eval CSP that blocks unsafe eval errors on regenerator-runtime Jan 12, 2021
@agilgur5
Copy link
Collaborator

agilgur5 commented Jan 12, 2021

Thanks for the detailed issue @rbrewington . CSPs are important to support, so would like to make sure everything works, but not sure we have much control over issues in an upstream dependency, and much less so a polyfill.

It looks like this might have been changed in later versions of regenerator-runtime, so maybe updating it would work?

We're on the latest version of regenerator-runtime, v0.13.7 (package.json link), so I don't believe that's the issue. It was only recently added to TSDX as well in #795 , so I would be surprised if it were outdated already. That regenerator issue is from ~2 years ago, comparatively

Per TSDX's v0.14.0 Release Notes, if you don't need to support older browsers, you can add a browserslistrc file, which will make it so regenerator-runtime isn't added to the bundle.

@agilgur5 agilgur5 added kind: support Asking for support with something or a specific use case scope: upstream Issue in upstream dependency solution: workaround available There is a workaround available for this issue labels Jan 12, 2021
@agilgur5
Copy link
Collaborator

So https://github.com/yext/answers/pull/1062 suggests you might be able to downgrade to fix it. You can do that with a package.json#resolution

Per some other issues/PRs like indico/react-overridable#3, it seems like you can also workaround this by disabling strict mode, which you can do by customizing your Rollup config in tsdx.config.js to add config.output.strict = false.
Per mozilla/pdf.js#11036 (comment) and the Rollup docs, it is not good practice to disable this, so use at your own risk.

Seems like there are multiple workarounds available and this issue is upstream, so I'm not sure there's much more that can be done here in TSDX

@rbrewington
Copy link
Author

rbrewington commented Jan 13, 2021

Thank you for the advice. Unfortunately I have to support IE11 and pinning the regenerator runtime also sounds like a potential security concern. I had tried the rollup config approach, but had the path to the strict option wrong. I'll try that.

Hopefully the regenerator team can find a way to fix this. For anyone else experiencing this, I'm following this open issue to see if they do: facebook/regenerator#378

ETA:
Alas... the rollup configuration prevented 'use strict' from being added by my library, but the app that is using the library is built with CRA and I believe it added 'use strict' when I compiled the app. In any case, my app is still violating the secure CSP.

If anyone knows how to disable 'use strict' in CRA please let me know.

@agilgur5
Copy link
Collaborator

agilgur5 commented Jan 14, 2021

Unfortunately I have to support IE11

Well in that case you'd need regenerator-runtime one way or another to support async/await or generators, so wouldn't matter whether you use TSDX or something else.

For anyone else experiencing this, I'm following this open issue to see if they do: facebook/regenerator#378

Upvoted


Going to close the issue as it's upstream in a required polyfill, not something TSDX can control, and workarounds have been given for the things TSDX can control.

@AndrewCraswell
Copy link

I'm hitting this same issue and not really sure how to resolve it. My app is built with CRA, and consumes multiple libraries built with TSDX. I can force a resolution in my CRA app to use regenerator-runtime v0.3.1 which won't exhibit this issue, but each of the libraries built with TSDX are bundled with their own versions of regenerator-runtime.

@AndrewCraswell
Copy link

I think this is where I'm getting stuck:
#169

Is there a way to disable the auto injection of the regenerator-runtime? Seems this was included not too long ago. I generally prefer the consumer to be able to control which polyfills are bundled to avoid the duplication hell that causes these exact sort of pitfalls. Another approach I've seen is to include polyfills in the CJS bundle, but not in the ESM.

@agilgur5
Copy link
Collaborator

agilgur5 commented Mar 5, 2021

To begin, there's a workaround available now for this in facebook/regenerator#378 (comment) . It has to occur before the import, which is auto-inserted by babel-plugin-polyfill-regenerator, so you might need to import a file at the top of your entrypoints with that line in it.

I can force a resolution in my CRA app to use regenerator-runtime v0.3.1 which won't exhibit this issue

That's extremely outdated but you can force each of the libraries to use that version too and then build using that.

but each of the libraries built with TSDX are bundled with their own versions of regenerator-runtime.

@AndrewCraswell Yes, that's a known issue. Per #795 (review) , I had to bundle it in for two reasons

  1. backward-compatibility with the previous usage of async-to-promises (which was unmaintained and had many correctness issues and bugs)
  2. TSDX is installed as a devDep, so simple peerDeps wouldn't work to ensure you install a polyfill as a dep.

I'd like to fix 2. in the future by having TSDX itself check for polyfills in your deps when they are needed in your code, but there's a lot of legwork to get there (potentially a Babel plugin or upstream PR required for detection purposes, then some non-trivial work in TSDX too, and lots of testing, since it'd be a huge user-facing change).

I think this is where I'm getting stuck:
#169

I'm not sure what you're referring to there as it's an old issue about a Babel plugin that isn't used or needed anymore. Per the resolving comment, #795 resolved it.

Is there a way to disable the auto injection of the regenerator-runtime?

Please read the above comment: #954 (comment) .
Other than that, I don't think there's any workaround to remove it from the internal Babel preset as the options can't be changed on merge to make it no-op.
And this is one of those edge cases ones where if it's moved to an overridable preset in the future (as I've suggested in RFCs and attempted in some places) it may mean we can't detect it in TSDX in the future as I mentioned above. But still need to work through that.

I generally prefer the consumer to be able to control which polyfills are bundled to avoid the duplication hell that causes these exact sort of pitfalls.

This is a bit off-topic, but that is a preference, and one that I don't think is optimal. Many folks in the community also prefer the opposite extreme, explicitly bundling polyfills in all cases. A bundler like Webpack may be able to detect duplicates with a dedupe plugin as well.

I'd prefer the middle -- which I believe is optimal -- that we all actually specify our bundle deps and have NPM resolve them the same so that we avoid any duplication unless necessary due to version restrictions, as we do with all other deps. TSDX is in a position to help lots of libraries dedupe polyfills this way.

This does not put any onus on users to manually install polyfills, which can also be significantly error prone (e.g. versions of polyfill are important) and can also lead to duplication (as not every library does that etc).

Another approach I've seen is to include polyfills in the CJS bundle, but not in the ESM.

That's my first time hearing that one, that would create a lot of inconsistencies so I wouldn't recommend that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind: support Asking for support with something or a specific use case scope: upstream Issue in upstream dependency solution: workaround available There is a workaround available for this issue
Projects
None yet
Development

No branches or pull requests

3 participants