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

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

Conversation

sikachu
Copy link
Member

@sikachu sikachu commented Oct 12, 2021

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): 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
Copy link
Member

dhh commented Oct 12, 2021

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
Copy link
Member Author

sikachu commented Oct 13, 2021

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
Copy link
Contributor

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

@justin808 justin808 closed this Dec 5, 2021
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.

3 participants