Skip to content

fix: mitigate libxmljs2 vulnerability (issue #1224) by omitting optional dependencies #1286

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

Closed
wants to merge 1 commit into from

Conversation

ericfitz
Copy link

@ericfitz ericfitz commented Apr 3, 2025

I changed the way that this is packaged so that it doesn't require optional, vulnerable dependencies. This should cause people who newly install this package, to not generate npm audit vulnerabilities.

For users who need to perform XML validation, they can install the optional dependency on xmljs.

I did leverage Claude Code (an AI coding assistant) to help me with this work. However I did not change the functionality of the npm, just the packaging, and I added new test cases to cover both the xml-validation-omitted and xml-validation-required scenarios, and I ensured that all existing and new tests passed.

• Added package.json configuration to omit optional dependencies by default
• Created alternate test snapshots for running without optional dependencies
• Updated integration tests to use different snapshots based on test context
• Fixed lint issues
• Added documentation about security measures

🤖 Generated with the help of Claude Code

• Added package.json configuration to omit optional dependencies by default
• Created alternate test snapshots for running without optional dependencies
• Updated integration tests to use different snapshots based on test context
• Fixed code style issues
• Added documentation about security measures

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@ericfitz ericfitz requested a review from a team as a code owner April 3, 2025 17:32
@jkowalleck
Copy link
Member

I changed the way that this is packaged so that it doesn't require optional, vulnerable dependencies.

thanks, but please open a ticket to discuss such critical breaking changes, first. I will not review it, until the details are discussed in such a ticket.

First, I'claim there simply is no vulnerable dependency.
the optional dependency you refer to is not vulnerable, in fact, it has a feature, that can, if configured wrong, be used for malicious behavior.
We have proved that the feature is configured correctly, and no malicious behavior is possible.

You really should not trust arbitrary security reports from actors that publish unreviewed security reports.
You really should not trust arbitrary security reports from actors that are payed by the claimed impact of their non-peer-reviewed findings.
See #1224

@jkowalleck
Copy link
Member

jkowalleck commented Apr 3, 2025

thank you for all your effort, but the content of this PR is not desirable

  • it changes the package manager - a NOGO
  • it does not what it claims to do.

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.

2 participants