rails / webpacker

Use Webpack to manage app-like JavaScript modules in Rails
MIT License
5.31k stars 1.47k forks source link

[5-x-stable] Bump optimize-css-assets-webpack-plugin to ^6.0.1 #3197

Closed sikachu closed 2 years ago

sikachu commented 2 years ago

This PR updates optimize-css-assets-webpack-plugin package to ^6.0.1.

We need to update optimize-css-assets-webpack-plugin to version 6.0.1 or later as version 5.0.8 eventually resolves to install a vulnerable version of nth-check (1.0.2): https://github.com/advisories/GHSA-rp65-9cf3-cjxr

You can see the dependencies from this reconstructed dependency tree:

@rails/webpacker@5.4.3
└─ optimize-css-assets-webpack-plugin@5.0.8
   └─ cssnano@4.1.11
      └─ cssnano-preset-default@4.0.8
          └─ postcss-svgo@4.0.3
             ├─ css-select@2.1.0
             │  └─ nth-check@1.0.2
             └─ nth-check@1.0.2

Updating optimize-css-assets-webpack-plugin to version 6.0.1 will update the dependency tree to look like this:

@rails/webpacker
└─ optimize-css-assets-webpack-plugin@6.0.1
   └─ cssnano@5.0.8
      └─ cssnano-preset-default@5.1.4
         └─ postcss-svgo@5.0.2
            └─ svgo@2.7.0
               └─ css-select@4.1.3
                  └─ nth-check@2.0.1

I followed the instructions in CONTRIBUTING.md and got a green test suite on both yarn test and rake test, so I don't think this will cause any issue.

dhh commented 2 years ago

The vulnerability isn't really relevant here, since this is all inside dev tools, which have no user input. This goes to the fundamental critique of the hamster wheel that yarn audit creates.

That said, don't mind the dependency upgrade, but I doubt we have good test coverage in this area, so probably need to either add that or validate by hand.

sikachu commented 2 years ago

Sure thing. I can look into it.

Alternatively, I think #2802 actually remove this dependency and made it on-demand (i.e. it won't be used if it's not installed). Maybe we can also do something similar here as well, but I'm not sure if that would be appropriate for a minor/patch release change.

justin808 commented 2 years ago

See comment on #3217. This can probably be resolved either by upgrading or using yarn resolutions.