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

Upgrade react-syntax-highlighter to pick up security patch upstream #17116

Merged
merged 3 commits into from Jan 10, 2022

Conversation

lucasgonze
Copy link
Contributor

@lucasgonze lucasgonze commented Jan 4, 2022

Issue: #16848

Signed-off-by: Lucas Gonze lucas@gonze.com

What I did

Cherrypicked changes from PR to upgrade package to pick up a security patch.

Ran yarn run test. Found no new test failures compared with the count in the previous commit.

How to test

  • [ This has no new functionality that would require a test. ] Is this testable with Jest or Chromatic screenshots?
  • [ No ] Does this need a new example in the kitchen sink apps?
  • [ No ] Does this need an update to the documentation?

…n highlight.js

Signed-off-by: Lucas Gonze <lucas@gonze.com>
@lucasgonze
Copy link
Contributor Author

yarn upgrade react-syntax-highlighter@^15.4.2 in addons/storysource/ in order to close #16848 in this branch

@VanessaHenderson
Copy link
Contributor

React-syntax-highlighter doesn't use the non-vulnerable version for sure (because of transitive dependency resolution) until 15.4.5, upgrading to 15.4.2 isn't going to resolve it. Its written in the comments of the fix PR for the package. I was about to submit a PR doing the non-vulnerable version

Fix PR reference for react-syntax-highlighter: react-syntax-highlighter/react-syntax-highlighter#430
Fix commit: react-syntax-highlighter/react-syntax-highlighter@20d9444

@shilman
Copy link
Member

shilman commented Jan 5, 2022

Thanks @VanessaHenderson @lucasgonze -- would upgrading to ^15.4.5 help? Once the issue is confirmed fixed in the latest alpha as implemented in #17100, I can merge and release this.

@lucasgonze
Copy link
Contributor Author

It's ^15.4.2, not 15.4.2 exactly. As a result you see the version of prismjs with the patch.

$ git status
On branch release/5.3

M1-Air-2020:storybook lucasgonze$ yarn why prismjs
=> Found "prismjs@1.25.0"
info Reasons this module exists
   - "_project_#@storybook#addon-storysource#react-syntax-highlighter" depends on it
   - Hoisted from "_project_#@storybook#addon-storysource#react-syntax-highlighter#prismjs"
   - Hoisted from "_project_#@storybook#addon-storysource#react-syntax-highlighter#refractor#prismjs"

M1-Air-2020:storybook lucasgonze$ yarn why react-syntax-highlighter
=> Found "react-syntax-highlighter@15.4.5"
info Reasons this module exists
   - "_project_#@storybook#addon-storysource" depends on it
   - Hoisted from "_project_#@storybook#addon-storysource#react-syntax-highlighter"
   - Hoisted from "_project_#@storybook#components#react-syntax-highlighter"

M1-Air-2020:storybook lucasgonze$ grep react-syntax-highlighter lib/components/package.json 
    "@types/react-syntax-highlighter": "11.0.4",
    "react-syntax-highlighter": "^15.4.2",

@shilman
Copy link
Member

shilman commented Jan 5, 2022

@lucasgonze sure, but if the fix is only available in 15.4.5 and above, shouldn't we force that? i haven't looked at the details of the package, just trying to make sure I understand @VanessaHenderson 's comment

@lucasgonze
Copy link
Contributor Author

@lucasgonze sure, but if the fix is only available in 15.4.5 and above, shouldn't we force that?

Additional force wouldn't cause harm but isn't necessary.

  1. yarn will continue to evaluate the ^ flag as it does now. yarn evaluated the version as 15.4.5 in yarn.lock.
  2. yarn.lock with 15.4.5 is included in the commit.

@lucasgonze
Copy link
Contributor Author

@shilman I'll happily list 15.4.5 explicitly in order to help this go through.

Signed-off-by: Lucas Gonze <lucas@gonze.com>
@shilman
Copy link
Member

shilman commented Jan 10, 2022

@lucasgonze I merged this to release it along with a colors.js fix. However, during the release I noticed that the snapshots from the syntax highlighter have changed pretty significantly. This change bumps react-syntax-highlighter by four major versions and I don't want to release this and accidentally break 5.3 users, of which there are still many. I am reverting this PR in #17183 for now so that I can publish the colors.js fix on its own.

@shilman
Copy link
Member

shilman commented Jan 10, 2022

I have also updated the security policy accordingly. This doesn't mean we won't ever publish patch releases for earlier versions, but I want to set expectations.

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

Successfully merging this pull request may close these issues.

None yet

3 participants