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 xmldom to 0.7.x #112

Closed
wants to merge 2 commits into from
Closed

Update xmldom to 0.7.x #112

wants to merge 2 commits into from

Conversation

dylmye
Copy link

@dylmye dylmye commented Aug 20, 2021

The name of the package changed due to the owner abandoning it. More info at xmldom/xmldom#271.

This fixes #110, fixes #111.

@dylmye
Copy link
Author

dylmye commented Aug 20, 2021

Didn't notice the whitespace changes (thanks Prettier) - I'll remove this in the morning if nobody else gets to it first

@SudiptaAtWork
Copy link

Why title is saying update xmldom to 0.7.1, when it is 0.7.0. It is creating confusion as it seems it is updating to some beta version

@dylmye dylmye changed the title Update xmldom to 0.7.1 Update xmldom to 0.7.0 Aug 20, 2021
@dylmye
Copy link
Author

dylmye commented Aug 20, 2021

Apologies @SudiptaAtWork, this has been corrected

@dylmye
Copy link
Author

dylmye commented Aug 20, 2021

/cc @mreinstein

@SudiptaAtWork
Copy link

Any update when will this PR be closed and a new version released for plist?

@brodycj
Copy link

brodycj commented Aug 24, 2021

xmldom (@xmldom/xmldom) is now up to 0.7.2 ... is there anything we can do to help get this updated?

Apache Cordova has been using this wonderful package as well.

@mreinstein
Copy link
Collaborator

Where to begin with this whole clusterfuck....

  • Can anyone explain to me, in plain english, what is actually going on with xmldom?:
    • the github issues and comments are very circular, point in various places, and don't really explain what happened
    • how did we get into a situation where noone with github access to xmldom has npm access?
    • what happened to the original maintainers? there seem to be 2 of them on npm, and there was a publish just 5 months ago
    • was there some argument, or have they just disappeared?
  • I'm keeping the maintenance lights on for plist.js but
    • I'm not doing any active dev on it at the moment because work is kicking my ass
    • I'm not crazy about switching out a depedency for a whole new one; this has just as many but different security ramifications as open CVEs have
    • Has anyone evaluated if this security issue actually affects plist.js, or are we just panicking over automated security audit warnings?
  • @TooTallNate are you still alive out there? These are just my opinion/questions, would love some thoughts since you're the OG on this module

@dylmye
Copy link
Author

dylmye commented Aug 25, 2021

Hey Mike, thanks for checking in on this. Sorry for the bother! This is going to be a bit long, sorry. The TL;DR is:

  • we have to switch package name because npm/original contributor chaos
  • the package is the same in everything but name + security fixes
  • the actual issue doesn't really affect this package but it might be smart to move over anyway

Where to begin with this whole clusterfuck....

Here's my understanding of the timeline:

  1. xmldom was created by Dev A and last updated in 2017
  2. After a few years of being abandoned, the package was picked up by a developer (Dev B), under the newly-created xmldom org.
  3. Dev B adds a number of other interested contributors to the org and contacts npm to gain control of the xmldom package under their abandoned package/dispute policy (see here) - publishes versions 0.3.0 - 0.6.0 (0.6.0 pushed out 17 April 2021.)
  4. A vuln advisory is published on the 27th of July 2021.
  5. At some point between April and July, npm changes their policy on abandoned packages and relinquishes control from Dev B and their contributors without permission from Dev A.
  6. NPM/GitHub refuses to give control and Dev A is radio silent, so Dev B & co publish the same package (with security fix) under the @xmldom npm org.

how did we get into a situation where noone with github access to xmldom has npm access?

✨ magic ✨
aka NPM's current dispute policy, updated 16 July:

We are not currently accepting dispute requests to "adopt an abandoned package" or "Report Squatting" as we re-evaluate and update the overall dispute process.

what happened to the original maintainers?

Dev A hasn't got any public activity on their GitHub or NPM. The other contributor has published stuff on GitHub since Dev B & co reached out to Dev A. I don't know if they reached out to the other contributor.

I fully understand how busy you are - hopefully this PR helps and we can get more community support. Also would love input from Nate if possible.

Has anyone evaluated if this security issue actually affects plist.js

XMLDom's DOMParser is used in the parse() method. The affected method from their library is serializeToString() which we don't use. However switching to this package a) stops the warnings, and b) makes it easier to mitigate serious issues in the future that might affect the project.

Hope this helps!

@dylmye dylmye changed the title Update xmldom to 0.7.0 Update xmldom to 0.7.x Aug 25, 2021
@brodycj
Copy link

brodycj commented Aug 25, 2021

I have just added some quick background to xmldom/xmldom#271, with quick post-mortem in this comment: xmldom/xmldom#271 (comment)

I would highly recommend asking any questions for clarification in xmldom/xmldom#271.

@mreinstein
Copy link
Collaborator

mreinstein commented Aug 26, 2021

@dylmye thanks for taking the time to write up more details. Here are my current thoughts on potential solutions:

  1. copy the used parts of xmldom@0.6 into plist.js. It sounds like this is not really a critical severity for plist.js as we're not using the vulnerable parts of the xmldom package. This will eliminate the noise about vulnerabilities without introducing a new package to plist.
  2. patch release (plist@3.0.4)
  3. refactor plist to use another library (currently leaning towards htmlparser2)
  4. resolve some other open issues (ideally lumping in other potentially breaking changes like parser fixes)
  5. remove support for node < 12.17
  6. pure esm module (drop legacy commonjs)
  7. major release (plist@4 )

Open for suggestions/thoughts/ideas on this.

@SudiptaAtWork
Copy link

SudiptaAtWork commented Aug 27, 2021

Any ETA on when this PR will be complete? Our app is showing security vulnerability because of xmldom 0.6.0, and we do not use xmldom directly it is plist which is injecting xmldom in our package.lock file. Without plist fixing this we can not get through this vulnerability checkpoint. Please any update sooner will be helpful. At least if a temporary solution of using xmldom 0.7.2 can be checked in and a version can be created and you take other issues in a separate version, then it will be helpful for us who has indirect dependency on xmldom for plist.

@mreinstein
Copy link
Collaborator

mreinstein commented Aug 27, 2021

resolved via fa8e184 (steps 1-2 in my previous comment.)

Steps 3-7 moved to #113

@mreinstein mreinstein closed this Aug 27, 2021
@karfau
Copy link

karfau commented Sep 2, 2021

The codebase of (xmldom which is the same as) @xmldom/xmldom luckily is under active development again and we are fixing various bugs in every patch and minor release (as of writing 0.7.4 has been released and 0.8.0 is being prepared). If there are issues in this library because of the parsing done in xmldom, chances are high that we would love to fix them.

Of course you have the right to inline the sources into you project, I hope it also makes it easier for you to manage/own the contained bugs. As you can see from the issues that are already filed, there are also some that impact xml parsing: https://github.com/xmldom/xmldom/issues?q=is%3Aissue+is%3Aopen+label%3Abug%2Cspec%3AXML+ , xmldom/xmldom#69 being one that might be relevant for you which was fixed in 0.7.0

PS: I did see your plan to switch to another library, curious which one you will pick and be more confident about the level of maturity regarding parsing. I'm not claiming we are far ahead, just didn't look into alternatives since I joined the contriutors/maintainers.

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.

Upgrade xmldom to a non-vulnerable version Upgrade xlmdom dependency to fix security advisory
6 participants