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

npm reports security vulnerability in rollup-plugin-uglify -> serialize-javascript #9129

Closed
kring opened this issue Sep 1, 2020 · 4 comments · Fixed by #9828
Closed

npm reports security vulnerability in rollup-plugin-uglify -> serialize-javascript #9129

kring opened this issue Sep 1, 2020 · 4 comments · Fixed by #9828

Comments

@kring
Copy link
Member

kring commented Sep 1, 2020

                       === npm audit security report ===


                                 Manual Review
             Some vulnerabilities require your attention to resolve

          Visit https://go.npm.me/audit-guide for additional guidance


  High            Remote Code Execution

  Package         serialize-javascript

  Patched in      >=3.1.0

  Dependency of   rollup-plugin-uglify [dev]

  Path            rollup-plugin-uglify > serialize-javascript

  More info       https://npmjs.com/advisories/1548

found 1 high severity vulnerability in 917 scanned packages

Because rollup-plugin-uglify is only used at build time to serialize the rollup plugin's configuration, it is not actually a relevant security vulnerability for CesiumJS. However, it is a scary-looking message at npm install time, either from a release ZIP or from github (installing CesiumJS from npm is not affected because we won't get any devDependencies that way). We should fix it.

TrySound/rollup-plugin-uglify#85 will fix it, but we need to wait for the rollup-plugin-uglify maintainers to merge that PR and release a new version. npm doesn't have an easy way to override a dependency of a dependency (unlike yarn). Other options:

@mramato
Copy link
Contributor

mramato commented Sep 1, 2020

The main reason I didn't switch to terser when we moved to ES6 modules was because it seemed to produce larger JS files, but there has been at least one major terser update since then so I'm not against it.

The other issue is that eslint does not catch const/let or other ES6 syntax in our code because of our use of modules and uglify would fail if we slip one in. If we switch to terser, we need to figure out how to catch those issues (since it would break IE11 completely)

@thw0rted
Copy link
Contributor

Also possibly of interest, TrySound/rollup-plugin-uglify#82 still isn't resolved and even the owner says you should move to Terser instead. (This matters starting with NPM v7 because they actually check peerdeps.)

@lilleyse
Copy link
Contributor

The main reason I didn't switch to terser when we moved to ES6 modules was because it seemed to produce larger JS files, but there has been at least one major terser update since then so I'm not against it.

As of terser 7.0.2 I'm seeing a reduction in size. Build/Cesium/Cesium.js was reduced from 3.7MB to 3.5MB.

The other issue is that eslint does not catch const/let or other ES6 syntax in our code because of our use of modules and uglify would fail if we slip one in. If we switch to terser, we need to figure out how to catch those issues (since it would break IE11 completely)

This should be ok given our plan to drop IE soon.

@lilleyse
Copy link
Contributor

lilleyse commented Jun 23, 2021

As of terser 7.0.2 I'm seeing a reduction in size. Build/Cesium/Cesium.js was reduced from 3.7MB to 3.5MB.

Never mind, I was trying that on an older branch. It's actually 3.68MB to 3.72MB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants