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

fix: vuln in markdown > markdown-it #255

Merged
merged 2 commits into from Jan 14, 2022

Conversation

smikulcik
Copy link
Contributor

@smikulcik smikulcik commented Jan 14, 2022

Pulls in fix to update vuln in markdown-it

I just ran npm audit fix

DavidAnson/markdownlint@v0.25.0...v0.25.1

npm audit
                       === npm audit security report ===                        
                                                                                
# Run  npm install markdownlint@0.25.1  to resolve 1 vulnerability
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Uncontrolled Resource Consumption in markdown-it             │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ markdown-it                                                  │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ markdownlint                                                 │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ markdownlint > markdown-it                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://github.com/advisories/GHSA-6vfc-qv3f-vr6c            │
└───────────────┴──────────────────────────────────────────────────────────────┘


found 1 moderate severity vulnerability in 576 scanned packages
  run `npm audit fix` to fix 1 of them.

@smikulcik smikulcik changed the title fix: run npm audit fix fix: vuln in markdown > markdown-it Jan 14, 2022
@nschonni
Copy link
Contributor

I believe this is already fixed in the next branch that this would need to be made against

@smikulcik
Copy link
Contributor Author

@nschonni What is the next branch? next doesn't appear to be a valid branch: https://github.com/igorshubovych/markdownlint-cli/tree/next

I'm having trouble resolving my npm audit since i'm depending on markdownlint-cli

@nschonni
Copy link
Contributor

Apologies, I was mixing this up with https://github.com/DavidAnson/markdownlint that uses that branching setup and was recently patched for this

@@ -1,7484 +1,8 @@
{
"name": "markdownlint-cli",
"version": "0.30.0",
"lockfileVersion": 2,
"lockfileVersion": 1,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure the package lock version should be downgraded, although it is "better" because Node 12 still ships with NPM 6

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh my apologies. Let me get my nvm up and running to be able to fix this.

For everything else I do, I'm still using node 14 (which comes with NPM 6)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nschonni Ok take another look. I re-ran with node 16 and the diffs look much better

Copy link
Collaborator

@DavidAnson DavidAnson left a comment

Choose a reason for hiding this comment

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

It looks like this project is inconsistent between tilde and caret, but why did you change that as part of this pull request?

@smikulcik
Copy link
Contributor Author

@DavidAnson I tried running npm audit fix with node 14. I ran it again with node 16 and the diffs look much cleaner!

Copy link
Collaborator

@DavidAnson DavidAnson left a comment

Choose a reason for hiding this comment

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

Thanks!

@DavidAnson DavidAnson merged commit e1f0f3b into igorshubovych:master Jan 14, 2022
@rsclarke-vgw
Copy link

Is there to be a corresponding release?

@DavidAnson
Copy link
Collaborator

Upcoming. This tends to be the last markdownlint project I update because it has the biggest reach and I'd like to find any issues with the smallest audience. Probably in a couple of weeks.

@Mister-Hope
Copy link

Just to be curious, why fixing markdown-it version?

@DavidAnson
Copy link
Collaborator

I do not understand. This commit updates to the latest markdownlint library version.

@smikulcik
Copy link
Contributor Author

@Mister-Hope markdown-it had a CVE: CVE-2022-21670. This is just one of many updates to all consumers of this library.

@smikulcik smikulcik deleted the updateMarkdownlint branch January 18, 2022 17:23
@Mister-Hope
Copy link

Mister-Hope commented Jan 19, 2022

I‘m talking about this Library is fixing version(I.e. “x.y.z”) not ”~x.y.z” or “^x.y,z”, can’t we let users to install newer deps without waiting for a release?

I may have “markdown-it” used in other tools(e.g. a doc generator), and I would like it to have the newest version of all time, instead of fixing to a version because of this package in devDeps in some situations.

Wait, I think I got it, markdownlint is actually the one which fix the version right? @DavidAnson

@DavidAnson
Copy link
Collaborator

I have had to fix more CI breaks due to (improper) patch-level changes and spent more hours dealing with malicious patch-level packages than I think I have gained from automatic patch-level updates because of loose versioning. It also seems problematic that the bits someone runs depends on the day of the week they installed the package. That's a recipe for mysterious bugs and hard-to-find problems. I know my decision to use explicit package versions does not solve this problem for the ecosystem, but I felt that it was the more sensible approach.

@DavidAnson
Copy link
Collaborator

@Mister-Hope I posted more about my thinking here: https://twitter.com/davidans/status/1484626749950283777?s=21

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

5 participants