Skip to content

Conversation

@AtofStryker
Copy link

minimum to resolve security vulnerability https://security.snyk.io/vuln/SNYK-JS-PBKDF2-10495498.

@AtofStryker AtofStryker mentioned this pull request Jun 26, 2025
@ljharb
Copy link
Member

ljharb commented Jun 26, 2025

Not needed, ever, because of semver ranges.

@ljharb ljharb closed this Jun 26, 2025
@AtofStryker
Copy link
Author

Not needed, ever, because of semver ranges.

Not needed, ever, because of semver ranges.

Respectfully, I disagree. 3.1.2 satisfies ^3.1.2. Sure you could update the lock and get the latest but the goal is to make sure the vulnerability isn't installed in the path. ever. It's best practice to update to the minimum allowable to avoid the possibly of a security vulnerability being installed and there are multiple ways that compromised version could be satisfied without prompting an update.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2025

"best practice" does not mean it's "needed", and either way, a PR isn't helping. Updating every single dependent in a tree every time there's a vulnerability is a lot of burden on unpaid maintainers, and there is simply no urgency to it.

@AtofStryker
Copy link
Author

"best practice" does not mean it's "needed", and either way, a PR isn't helping. Updating every single dependent in a tree every time there's a vulnerability is a lot of burden on unpaid maintainers, and there is simply no urgency to it.

@ljharb I am not asking, nor do I think others are asking unpaid maintainers to update every single dependency in a tree. That seems like a hasty generalization. It seems more reasonable to me that if an issue is filed and gets traction from a user base, that the issue should likely be addressed. And that contributors have a means to address the issue to avoid stress on maintainers.

I think I have explained sufficiently why this change is needed, regardless of being a best practice or not. The original reason for rejection was "Not needed, ever, because of semver ranges." I've explained why this is incorrect. It seems like a general reluctancy to accept the contribution. If a PR isn't helping, I would appreciate if you could elaborate as to why and how others can help contribute to lessen the burden on someone such as yourself.

@ljharb
Copy link
Member

ljharb commented Jun 26, 2025

It's still correct - because even if it never happens, new installs will use the fixed version by default, preexisting installs can update their lockfiles without any additional action from any open source maintainers, and dozens of security tools will ensure that a vulnerable version isn't unknowingly in the tree.

@AtofStryker
Copy link
Author

It's still correct - because even if it never happens, new installs will use the fixed version by default

It is not correct. This is true most of the time, but not all the time. For instance, in a fresh project:

npm install [email protected]
npm install crypto-browserify
npm uninstall pbkdf2

leaves the vulnerability installed (check the lockfile) because the dependency is satisfied. If crypto-browserify fixes the vulnerability, this issue doesn't happen as soon as the vulnerability is uninstalled.

preexisting installs can update their lockfiles without any additional action from any open source maintainers

Correct. I have done this already. But lockfiles are subject to change, and it leaves the projects vulnerable to the same pitfall mentioned above.

and dozens of security tools will ensure that a vulnerable version isn't unknowingly in the tree.

That's if its being actively looked for by the person or org installing the dependency. It could easily revert after being fixed in a lockfile and may not be caught. It's best to fix the issue upstream.

Are you able to answer my above asks? I feel like if I had clear instructions on what I would need to do to make this contribution acceptable and help... I would. But it's still unclear to me.

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