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

[GHSA-f7xj-rg7h-mc87] Stylelint has vulnerability in semver dependency #2491

Closed

Conversation

G-Rath
Copy link

@G-Rath G-Rath commented Jul 12, 2023

Updates

  • Affected products

Comments
Update ranges to reflect that stylelint is no longer vulnerable

@github
Copy link
Collaborator

github commented Jul 12, 2023

Hi there @ofrolenko and @ybiquitous! A community member has suggested an improvement to your security advisory. If approved, this change will affect the global advisory listed at github.com/advisories. It will not affect the version listed in your project repository.

This change will be reviewed by our highly-trained Security Curation Team. If you have thoughts or feedback, please share them in a comment here! If this PR has already been closed, you can start a new community contribution for this advisory

@G-Rath G-Rath mentioned this pull request Jul 12, 2023
5 tasks
@github-actions github-actions bot changed the base branch from main to G-Rath/advisory-improvement-2491 July 12, 2023 00:07
Copy link

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

I have a few questions:

  • What happens after this update to blank like 0.0.0 versions?
  • Should we leave an advisory comment about this update to tell people the reason?
  • What is the difference between 0 and 0.0.0?

@G-Rath
Copy link
Author

G-Rath commented Jul 12, 2023

What happens after this update to blank like 0.0.0 versions?

I'm not sure what you mean by this, but the result of this update will be that no versions of stylelint will be flagged as vulnerable.

Should we leave an advisory comment about this update to tell people the reason?

The fact that the affected version ranges have changed should be enough by itself - my comment will also be visible in the improvement log for the advisory.

What is the difference between 0 and 0.0.0?

"0" indicates "the first version of the package" - effectively a bottomless range as its common for vulnerabilities to be discovered as existing in all versions before the initial patch.

Copy link

@ybiquitous ybiquitous left a comment

Choose a reason for hiding this comment

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

@G-Rath Thanks for the instant answers. I now understand this change well. LGTM.

@darakian
Copy link
Contributor

Question on this from my end @G-Rath . Shouldn't the upper end of the range be 15.10.1 based on the repo advisory?
GHSA-f7xj-rg7h-mc87

@ljharb
Copy link

ljharb commented Jul 12, 2023

Either way, this CVE is on semver, not stylelint, so I'm not sure why this has an entry at all. Stylelint doesn't ship a vulnerability, they just ship instructions to tell the end user to download another dependency that happens to be vulnerable.

@darakian
Copy link
Contributor

darakian commented Jul 12, 2023

Either way, this CVE is on semver, not stylelint, so I'm not sure why this has an entry at all.

This advisory doesn't have a CVE. It's independent and created to reflect the repo advisory created by the package author. The semver cve is listed on GHSA-c2qf-rxjj-qqgw

@ljharb
Copy link

ljharb commented Jul 12, 2023

An advisory on something for a transitive dep of it doesn't make sense either.

@darakian
Copy link
Contributor

So this advisory already has 15.10.1 as a fixed version. Is that a no op in this case? If the author(s) would like to withdraw the advisory that's fine, but if they would like to keep it up that's also fine.

@G-Rath
Copy link
Author

G-Rath commented Jul 12, 2023

When the advisory was originally released, the semver fix had not been backported so v15.10.1 of stylelint was the version that upgraded the dependency that was bring in the unpatchable version of semver.

Now that it has been backported however, no versions of stylelint will result in a vulnerable version of semver being installed - hence the change of this advisory to mark no version as vulnerable (so yes this is now a no-op).

I agree with @ljharb which is why I originally recommended the advisory be withdrawn especially since the backports were on their way - however GitHub Support said that the advisory should not be withdrawn, and @ybiquitous wanted to follow their recommendation even after the backports had been released and it was confirmed that no version of stylelint would pull in a vulnerable version of semver.

@darakian
Copy link
Contributor

darakian commented Jul 12, 2023

I see. I was unaware of the reach out to support though digging back I do see the conversation. @ybiquitous how would you like to handle this situation? We can withdraw the global advisory if you like, but so far as I am concerned if you would like to leave it up for whatever reason then we will respect that.

To the advisory about a transitive dependency question. Ya it's not ideal, but I think there's little harm in keeping the advisory.

IMO zeroing out the affected version range would make for more confusion than withdrawing the advisory with the versions intact.

@ybiquitous
Copy link

Thanks for the comments, everyone.

@ybiquitous how would you like to handle this situation? We can withdraw the global advisory if you like, but so far as I am concerned if you would like to leave it up for whatever reason then we will respect that.

To be honest, I'm unsure whether we should keep the advisory to provide the latest information for people or withdraw it to prevent false security alerts from tools (e.g. Dependabot).

I have wanted to keep it just because GitHub support recommends it, and if many people think keeping is inconvenient, I'm not strongly against withdrawing.

Just in case, I'll ask GitHub Support again.

@ntwb
Copy link

ntwb commented Jul 13, 2023

In hindsight Stylelint should have never created the advisory, thus if the recommendation is to remove it and remove confusion let's do that

@ybiquitous
Copy link

@ntwb Thanks for the comment.

I've just received an answer from GitHub Support, but it has not a strong recommendation.

So, I agree with withdrawing this advisory. It is more beneficial for many people.

@darakian Sorry to bother you, but could you please start withdrawing the advisory? Please let me know if I need to do anything.

@darakian
Copy link
Contributor

darakian commented Jul 13, 2023

@darakian Sorry to bother you, but could you please start withdrawing the advisory? Please let me know if I need to do anything.

No bother at all. I'll withdraw the global advisory on our end which will stop any future alerts. The repo advisory on the stylelint repo will be untouched, but feel free to update that as you see fit 👍
Also gonna close this PR out since we've come to a conclusion.

Edit:
The advisory is now withdrawn
GHSA-f7xj-rg7h-mc87

Thank you all for the input and @ybiquitous thank you for being proactive with alerting your users! 😄

@darakian darakian closed this Jul 13, 2023
@github-actions github-actions bot deleted the G-Rath-GHSA-f7xj-rg7h-mc87 branch July 13, 2023 16:50
@ybiquitous
Copy link

@darakian Thanks for the support. I've confirmed the withdrawn advisory! 👍🏼

And @G-Rath, thanks a lot for your helpful feedback! 😊

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.

None yet

6 participants