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.

Sign up for GitHub

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

Jump to bottom

Upgrade to strip-ansi ^6.0.1 #34383

Merged
merged 1 commit into from
Jan 7, 2022
Merged

Upgrade to strip-ansi ^6.0.1 #34383

merged 1 commit into from
Jan 7, 2022

Conversation

janaagaard75
Copy link
Contributor

This updates strip-ansi to version ^6.0.1 to fix this vulnerability issue: https://www.npmjs.com/advisories/1004946.

I don't know how to test this, but would gladly help, if I can get a bit of guidance.

  • According to https://github.com/chalk/strip-ansi/releases/tag/v6.0.0, upgrading to version 6 requires changing import stripAnsi from 'strip-ansi'; to import stripAnsi = require('strip-ansi');. I did not make this change because VSCode only accepted the current syntax, and my experience is that VSCode is usually right about these things. 🙂
  • I have updates to ^6.0.1 across all packages including the ones that already were on ^6.0.0 to align the version and to make it clear that v6.0.0 should be avoided.
  • I have not upgrade to version 7 because that changes strip-ansi to the ESM syntax, and I am unsure if this would work.
  • Running yarn install did not update yarn.lock, so there are probably still some packages using the strip-ansi in the older (and vulnerable) versions.
  • The starters haven't been touched. Unsure if this is acceptable.
  • This is a follow up to the discussion here: Severe npm audit vulnerability - how can I fix this? #28852

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jan 3, 2022
@janaagaard75
Copy link
Contributor Author

janaagaard75 commented Jan 3, 2022 •
edited
Loading

I have looked into the failing tests, but I cannot figure out how my changes should be the cause of the issue. stripAnsi is used in test-output.js, but the issue is in ssr.js and I don't see any connection between the two files.

@tyhopp
Copy link
Contributor

tyhopp commented Jan 5, 2022

Hi @janaagaard75, thanks for the PR and detailed description.

I looked into this and older versions of strip-ansi are used in several other upstream dependencies, so while we can merge this update it will not fix the npm audit vulnerability message. Completely fixing this would require us to update major versions of packages, which we currently can't prioritize.

This is also why the lock file didn't change as you suspected. We can still merge this as is.

@tyhopp tyhopp added status: awaiting author response Additional information has been requested from the author type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jan 5, 2022
@janaagaard75
Copy link
Contributor Author

I looked into this and older versions of strip-ansi are used in several other upstream dependencies, so while we can merge this update it will not fix the npm audit vulnerability message. Completely fixing this would require us to update major versions of packages, which we currently can't prioritize.

This is also why the lock file didn't change as you suspected. We can still merge this as is.

I think it would be nice to get this merged even though there is still some work to do before the vulnerability is gone.

BTW: How was the issue with failing test solved?

@LekoArts LekoArts removed the status: awaiting author response Additional information has been requested from the author label Jan 7, 2022
@tyhopp
Copy link
Contributor

tyhopp commented Jan 7, 2022

@janaagaard75 It turns out that the test itself is a flaky test that needs to be fixed. Apologies if it caused any undue headache!

@tyhopp tyhopp merged commit 73b4625 into gatsbyjs:master Jan 7, 2022
@tyhopp tyhopp mentioned this pull request Jan 11, 2022
Merged
moonmeister added a commit to moonmeister/gatsby that referenced this pull request Jan 11, 2022
@moonmeister
Merge branch 'master' into fix/validate/warnonunknown
eb2a22a 
* master: (24 commits)
  chore(docs): Release Notes v4.5 (gatsbyjs#34425)
  chore(docs): Update quick-start guide (gatsbyjs#34445)
  chore(docs) : Typo fix GatbsyImage -> GatsbyImage (gatsbyjs#34439)
  perf(gatsby): reuse rootNode & trackedRootNodes caches across instances of graphqlRunner (gatsbyjs#33695)
  Update media-item-processing.md (gatsbyjs#34434)
  chore(docs): Update localization doc (gatsbyjs#34429)
  test(ssr): Fix flakes (gatsbyjs#34443)
  chore(release): Publish next
  Revert "docs: Match egghead.io video instructions (gatsbyjs#34315)" (gatsbyjs#34384)
  fix(gatsby-plugin-manifest): generate icons sequentially (gatsbyjs#34331)
  Fix misspelling of "precedence" in log message (gatsbyjs#34428)
  chore(docs): Adjust doc mentions of gatsby-plugin-create-client-paths (gatsbyjs#34424)
  chore(docs): Update static-folder doc (gatsbyjs#34392)
  Upgrade to strip-ansi ^6.0.1 (gatsbyjs#34383)
  chore(gatsby-plugin-create-client-paths): Update client paths plugin readme with migration info (gatsbyjs#34423)
  chore: Remove deprecated client paths plugin references (gatsbyjs#34422)
  chore(docs): Old occurrences of gatbyjs.org (gatsbyjs#34402)
  Update plugins.md to have correct URL for gatsby-plugin-segment-js (gatsbyjs#34397)
  chore(gatsby): Give option to ignore output from workers and silence validate-engines (gatsbyjs#34416)
  chore(release): Publish next pre-minor
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tyhopp tyhopp tyhopp approved these changes

Assignees
No one assigned
Labels
type: maintenance An issue or pull request describing a change that isn't a bug, feature or documentation change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@janaagaard75 @tyhopp @LekoArts