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

Update dependencies of @wordpress/scripts to fix semver security issue #52029

Closed
rosswintle opened this issue Jun 28, 2023 · 4 comments · Fixed by #53636
Closed

Update dependencies of @wordpress/scripts to fix semver security issue #52029

rosswintle opened this issue Jun 28, 2023 · 4 comments · Fixed by #53636
Assignees
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Status] In Progress Tracking issues with work in progress [Tool] WP Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling

Comments

@rosswintle
Copy link
Contributor

I'm not sure how to raise this as it doesn't seem to fit one of the issue types that has a template.

We have a number of repositories that use @wordpress/scripts.

These all have Dependabot reporting "semver vulnerable to Regular Expression Denial of Service" as a moderate security issue that is present in semver < v7.5.2 and fixed in semver v7.5.2

When I try to fix this, I can only get semver up to version 5.7.1 because of dependencies of @wordpress/scripts

The chain of dependencies seems to be:

  • @wordpress/scripts@26.6.0 requires npm-package-json-lint: ^5.0.0
  • npm-package-json-lint@5.4.2 requires meow: ^6.1.1
  • meow@6.1.1 requires normalize-package-data: ^2.5.0
  • normalize-package-data@2.5.0 requires semver: 2 || 3 || 4 || 5

Updating npm-package-json-lint to v6.4.0 would fix this:

  • npm-package-json-lint@6.4.0 requires meow: ^9.0.0
  • meow@9.0.0 requires normalize-package-data: ^3.0.0
  • normalize-package-data@3.0.3 requires semver: ^7.3.4

I'd love to try to make a PR for this, but don't know how to do this given the mono-repo nature here.

Are there plans to update this dependency? Can someone that has the repo set up try it and see if it's a breaking change?

Thanks

@rosswintle rosswintle changed the title Update all dependencies of @wordpress/scripts to fix semver security issue Update dependencies of @wordpress/scripts to fix semver security issue Jun 28, 2023
@rosswintle
Copy link
Contributor Author

Just an update on this...

I dived into the semver repos and followed some other dependency trails.

babel depends on semver v6.x and they have reasons not to update to semver version 7+.

They have said:

We are waiting for a backport fix to semver v6. If the fix is not available within this week, we will fork semver v6 with the proper fix.

There is an issue where this is being discussed on the semver repo.

The maintainers there have said:

At this time, seeing as how v5/6 is a single 1400+ line file with outdated testing deps and no working CI we do not have plans to backport this.

But then people have been submitting PRs updating all the CI, tests and stuff as well as patching the older versions.

So this could all be fixed in a few days time.

It still seems to me like @wordpress/scripts should update its dependencies though, if this is backwards compatible. npm-package-json-lint v5.4.2 is over 18 months old now.

@gziolo gziolo added the [Tool] WP Scripts /packages/scripts label Jul 1, 2023
@jordesign jordesign added the [Type] Build Tooling Issues or PRs related to build tooling label Jul 18, 2023
@gziolo
Copy link
Member

gziolo commented Aug 4, 2023

It still seems to me like @wordpress/scripts should update its dependencies though, if this is backwards compatible. npm-package-json-lint v5.4.2 is over 18 months old now.

It would be great to update npm-package-json-lint to a more recent version. I see that v7.0.0 dropped support for Node 14 so we would need to update to Node 16/18 in Gutenberg first. Therefore, we could go with v6.4.0 for the time being. Would it help?

@gziolo gziolo added Needs Dev Ready for, and needs developer efforts Good First Issue An issue that's suitable for someone looking to contribute for the first time labels Aug 4, 2023
@rosswintle
Copy link
Contributor Author

I've not checked, but yes. Any dependency updates here are good.

I think everything got updated/patched upstream now anyway, but would still be better to get an update.

@gziolo gziolo self-assigned this Aug 14, 2023
@gziolo gziolo added the [Status] In Progress Tracking issues with work in progress label Aug 14, 2023
@gziolo
Copy link
Member

gziolo commented Aug 14, 2023

I opened #53636 which should fix the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue An issue that's suitable for someone looking to contribute for the first time Needs Dev Ready for, and needs developer efforts [Status] In Progress Tracking issues with work in progress [Tool] WP Scripts /packages/scripts [Type] Build Tooling Issues or PRs related to build tooling
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants