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(core): use zkochan/js-yaml directly to avoid false audit errors #25999

Merged
merged 1 commit into from
May 24, 2024

Conversation

meeroslav
Copy link
Contributor

@meeroslav meeroslav commented May 24, 2024

Some of the audit tools have been falsely flagging the alias to @zkochan/js-yaml as [email protected] (which has security holes) so we decided to use the package explicitly.

Current Behavior

Expected Behavior

Related Issue(s)

Fixes #

@meeroslav meeroslav self-assigned this May 24, 2024
@meeroslav meeroslav requested review from vsavkin, mandarini and a team as code owners May 24, 2024 09:02
@meeroslav meeroslav requested a review from AgentEnder May 24, 2024 09:02
Copy link

vercel bot commented May 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview May 24, 2024 9:15am

@@ -31,7 +31,7 @@
"generators": "./generators.json",
"executors": "./executors.json",
"peerDependencies": {
"js-yaml": "npm:@zkochan/js-yaml@0.0.7"
"@zkochan/js-yaml": "0.0.7"
Copy link
Contributor

Choose a reason for hiding this comment

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

@meeroslav Is the peerDependency needed? The nx package also depends on @zkochan/js-yaml so we can move it to the dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but it doesn't change the requirement need for this one. The eslint package only needs js-yaml if user needs to migrate yaml-based config to flat config.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it would eliminate the user confusion why the @zochkan/js-yaml package has to be installed instead of js-yaml. And it would allow existing usage to function without changes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only things we could do is make eslint depend on js-yaml since it has no need for Zoltan's overrides as it uses plain load and dump. But the entire usage might be completely removed once code mods from eslint's team land.

On top of that we generally don't have support for yaml configs, so the chance of someone using converter to convert yaml rc to flat config are almost zero.

If there is an issue created later on we can address it.

@meeroslav meeroslav merged commit 61e4ab2 into master May 24, 2024
6 checks passed
@meeroslav meeroslav deleted the feat/use-zkochan-yaml-without-alias branch May 24, 2024 10:43
FrozenPandaz pushed a commit that referenced this pull request May 24, 2024
…25999)

Some of the audit tools have been falsely flagging the alias to
`@zkochan/js-yaml` as `[email protected]` (which has security holes) so we
decided to use the package explicitly.

<!-- This is the behavior we have today -->

<!-- This is the behavior we should expect with the changes in this PR
-->

<!-- Please link the issue being fixed so it gets closed when this is
merged. -->

Fixes #

(cherry picked from commit 61e4ab2)
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants