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(deps): upgrade polished dep due to security vulnerability #1127

Closed
wants to merge 1 commit into from
Closed

fix(deps): upgrade polished dep due to security vulnerability #1127

wants to merge 1 commit into from

Conversation

JamesSingleton
Copy link
Contributor

https://app.snyk.io/vuln/npm:polished

                                                                                                                  <!-- 🎗add a PR label 🎗-->

Description

Upgrades the polished dependency to address https://app.snyk.io/vuln/npm:polished

Detail

Checklist

  • 👌 design updates are Garden Designer approved (add the
    designer as a reviewer)
  • 🌐 demo is up-to-date (yarn start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • ♿ analyzed via axe and evaluated using VoiceOver
  • 💂‍♂️ includes new unit tests
  • 📝 tested in Chrome, Firefox, Safari, Edge, and IE11

@JamesSingleton JamesSingleton requested a review from a team as a code owner June 30, 2021 17:42
@hzhu
Copy link
Contributor

hzhu commented Jun 30, 2021

Looks good. What was the steps (commands) to make this upgrade? I want to verify it locally on my machine.

@JamesSingleton
Copy link
Contributor Author

Looks good. What was the steps (commands) to make this upgrade? I want to verify it locally on my machine.

@hzhu, unfortunately, yarn does not give an easy way to upgrade sub dependencies like npm. Basically what I had to do was rm yarn.lock && yarn install. Copy the bit for polished from the yarn.lock file and then git checkout . and replace the old polished in the yarn.lock with the new. Doing anything like yarn upgrade or yarn upgrade polished@latest didn't work as doing yarn upgrade polished@latest is like doing yarn add polished@latest. There is a GitHub Issue in yarn that talks about this and it not getting native support in yarn.

@JamesSingleton
Copy link
Contributor Author

@hzhu actually might have to decline this PR as it looks like I might have to update @babel/runtime as well... But let me know.

@hzhu
Copy link
Contributor

hzhu commented Jul 1, 2021

It looks like polished isn't a shared dependency at the root mono repo level, and the dependency found in the root yarn.lock is a result of an indirect dependency. Notice that polished isn't found in the root package.json. Also, this project strives to not directly modify the yarn.lock manually.

There are sub-packages that use polished, and those may be the packages that you're looking to upgrade? E.g. react-forms, react-colorpickers.

@JamesSingleton
Copy link
Contributor Author

@hzhu the issue looks to be @zendeskgarden/react-theming as it requires polished@^4.0.0. However, when installed into new deps it will install whatever is the latest at the time.

@hzhu
Copy link
Contributor

hzhu commented Jul 7, 2021

@JamesSingleton

Looks like react-theming is one of the sub-packages that use polished. You could try yarn upgrade polished command inside the sub-package directory, which should update the corresponding sub-package yarn.lock file.

Since polished isn't a cross-package dependency (e.g not defined at the top-level package.json), I think the top level yarn.lock should remain unchanged. It should update accordingly as cross-package dependencies update to later versions of polished. These top-level dependencies are managed by Renovate.

@hzhu
Copy link
Contributor

hzhu commented Aug 23, 2021

@JamesSingleton any updates on this one?

@hzhu hzhu closed this Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants