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 vinyl-fs depedency to version ^3.0.3 #547

Conversation

joekrump
Copy link
Contributor

@joekrump joekrump commented May 13, 2020

This update was made because this newer version of vinyl-fs does not have a dependency on a version of "braces" that is vulnerable to a ReDoS attack. This addresses this issue: #537

vinyl-fs is used in just a few places in this project and its methods that are used have not changed between version 2.4.4 (the previous version of this dependency that was specified in package.json and version 3.0.3.

Details about the braces vulnerability can be found here: https://snyk.io/vuln/npm:braces:20180219

This update was made because this version of `vinyl-fs` does not have a dedendency on a version of "braces" that is vulnerable to a ReDoS attack. https://snyk.io/vuln/npm:braces:20180219
@coveralls
Copy link

Coverage Status

Coverage remained the same at 94.752% when pulling 369bf40 on joekrump:update-vinyl-fs-dependency-to-latest-version into 21bf591 on SassDoc:master.

@joekrump
Copy link
Contributor Author

@pascalduez, are you able to review this pull request or recommend another maintainer or contributor who can review and merge + publish a new version of this package once this is merged?

@joekrump
Copy link
Contributor Author

@hugogiraudel are you able to assist with reviewing and perhaps merging and deploying a new version with the security related update?

@KittyGiraudel
Copy link
Member

KittyGiraudel commented May 18, 2020

Hello. :)

That change is a breaking change and we would have to make sure everything works exactly the same before merging. Unfortunately I do not have the capacity to conduct these tests right now.

@joekrump
Copy link
Contributor Author

joekrump commented May 19, 2020

@hugogiraudel is there some way I can help with the testing? As I note in the PR description, although there are breaking changes, they do not impact sassdoc because the methods that are used are not impacted by the breaking changes.

Alternatively, if you are too busy, is there another maintainer of this project that you'd recommend I reach out to who can help with this?

Here are the 3 instances of vinyl-fs being used in source code:

  1. https://github.com/SassDoc/sassdoc/blob/master/develop/index.js#L39
  2. https://github.com/SassDoc/sassdoc/blob/master/src/recurse.js#L32
  3. https://github.com/SassDoc/sassdoc/blob/master/src/sassdoc.js#L255

3 additional usages of vinyl-fs appear in unit test files and also are not impacted by the breaking changes as far as I've been able to investigate. These tests should also fail if the breaking changes impact sassdoc.

@joekrump
Copy link
Contributor Author

@valeriangalliat or @FWeinb are you able to please help here or advise on the next actionable steps I can take? I'm trying to help make this library more secure but feel I'm hitting a brick wall here.

@KittyGiraudel
Copy link
Member

KittyGiraudel commented May 21, 2020

Our tests seem to be passing fine after the dependency update. I will merge this, but I don’t know if I’ll be able to publish a patch. I will get in touch with Pascal to see how we can have it published. :)

Thank you for your help @joekrump!

@joekrump
Copy link
Contributor Author

joekrump commented Jun 1, 2020

Hey @hugogiraudel, how's it going? Were you able to get in touch with Pascal?

@KittyGiraudel KittyGiraudel merged commit bdab3cb into SassDoc:master Jun 2, 2020
@KittyGiraudel
Copy link
Member

I think it should be published in 2.7.2. ;)

@joekrump
Copy link
Contributor Author

joekrump commented Jun 2, 2020

Thank you for your help @hugogiraudel!

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

Successfully merging this pull request may close these issues.

3 participants