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

updated dependency of semver to the latest version #8859

Merged

Conversation

fschaeffler
Copy link
Contributor

The currently used version of semver does have a medium-level security issue. Therefore, the dependencies of meteor-dev-bundle had been updated.

Regular Expression Denial of Service
Name semver
CVSS 5.3 (Medium)
Installed 4.1.0
Vulnerable <4.3.2
Patched 4.3.2
Path meteor-dev-bundle@0.0.0 > semver@4.1.0
More Info https://nodesecurity.io/advisories/31

@apollo-cla
Copy link

@fschaeffler: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@abernix
Copy link
Contributor

abernix commented Jun 29, 2017

Looking at the commit history I'm fairly sure we can go all the way to 5.3.0. Was there a particular reason you avoided it?

@fschaeffler
Copy link
Contributor Author

fschaeffler commented Jun 30, 2017

@abernix In order to prevent possible side-effects, I did update the package only up until to first version, when the issue got fixed. But you're right, an update to 5.3.0 does make sense. So I'll update the PR.

@fschaeffler fschaeffler changed the title updated dependency of semver to the first patched one updated dependency of semver to the latest version Jun 30, 2017
Copy link
Contributor

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fschaeffler, looks good! We're almost LGTM here - we just need to generate and publish a new dev bundle version, and see the tests all pass with that new version, then we'll be good to go ( 👋 MDG 🙂 ).

@fschaeffler
Copy link
Contributor Author

@hwillson are there any further steps needed from my side for the publish of a new dev bundle version? As this is my first PR into https://github.com/meteor/meteor, I'm not sure what steps are all needed for this to happen.

@hwillson
Copy link
Contributor

No, nothing else is required on your side - the next steps have to be handled by MDG staff (see Submitting "Dev Bundle" Pull Requests for more details). Thanks for asking!

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had a chance to look into it, but I think we need to actually look at the reasoning behind semver410.js (added in 259f90b) before merging this.

@hwillson
Copy link
Contributor

Wow, good catch @abernix! Here are the only differences between semver410.js and semver v4.1.0:

// <METEOR>
// Fool the module system detection code below so that it doesn't
// do anything special.
var exports = SemVer, module = {}, define = {};
// Create a package-private variable.  Can't use SemVer because
// of the code that says `function SemVer(...)` below (implicitly
// declaring a var).  Can't use "semver" because that's a var in
// package-version-parser.js.
SemVer410 = SemVer;
// </METEOR>

So it looks like at one point it was necessary to override semver's default module handling code, to work within Meteor's package system. Digging a bit deeper, it looks like the package-version-parser.js file is being used both by the Meteor Tool and by the package-version-parser package. When used by the Tool the npm based version of semver (from the dev_bundle) is used. When used by the package-version-parser package the overridden version above (via SemVer410) is used.

A lot has changed since this code was put in place, so we should (fingers crossed) be able to adjust things such that the package-version-parser package references the same npm based semver version.

benjamn pushed a commit that referenced this pull request Jul 14, 2017
Also removed the underscore dependency while I was at it.

cc @abernix @hwillson @fschaeffler

Prerequisite for #8859.
@benjamn benjamn added this to the Release 1.5.2 milestone Jul 14, 2017
@benjamn benjamn changed the base branch from devel to release-1.5.2 July 14, 2017 16:21
@benjamn benjamn self-assigned this Jul 14, 2017
@hwillson
Copy link
Contributor

Quick update here - once #8914 has been merged, we should be all set to continue with this PR.

benjamn pushed a commit that referenced this pull request Jul 19, 2017
Also removed the underscore dependency while I was at it.

cc @abernix @hwillson @fschaeffler

Prerequisite for #8859.
benjamn pushed a commit that referenced this pull request Jul 21, 2017
Also removed the underscore dependency while I was at it.

cc @abernix @hwillson @fschaeffler

Prerequisite for #8859.
benjamn added a commit that referenced this pull request Jul 23, 2017
Also removed the underscore dependency while I was at it.

cc @abernix @hwillson @fschaeffler

Prerequisite for #8859.
@benjamn benjamn merged commit 60776a6 into meteor:release-1.5.2 Jul 23, 2017
abernix pushed a commit that referenced this pull request Mar 30, 2018
Also removed the underscore dependency while I was at it.

Prerequisite for #8859.

Backports: d3aff77
abernix added a commit that referenced this pull request Mar 30, 2018
This wasn't available to cherry-pick for some reason.

Backports: 9fdde2b

Ref: #8859
@abernix abernix mentioned this pull request Mar 30, 2018
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.

5 participants