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: override uri-js with fast-uri #18762

Closed
wants to merge 4 commits into from

Conversation

JudahZF
Copy link

@JudahZF JudahZF commented Aug 8, 2024

By overriding uri-js with fasturi, the punycode deprecation can be fixed (#17720) without having to upgrade ajv to version 8 (#13888: comment-872591875)

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

Tell us about your environment (npx eslint --env-info):

  • Node version: v22.6.0
  • npm version: v10.2.4
  • Local ESLint version:
  • Global ESLint version:
  • Operating System: linux 6.10.3-arch1-2

What parser are you using (place an "X" next to just one item)?

[ ] Default (Espree)
[X] @typescript-eslint/parser
[ ] @babel/eslint-parser
[ ] vue-eslint-parser
[ ] @angular-eslint/template-parser
[ ] Other

Please show your full configuration:

Configuration

What did you do? Please include the actual source code causing the issue.

What did you expect to happen?

What actually happened? Please include the actual, raw output from ESLint.
NodeJS repeatedly logs punycode deprecation error

What changes did you make? (Give an overview)

Added an override to replace uri-js (dependency of ajv@6) with fast-uri (version now used by ajv@8)

Is there anything you'd like reviewers to focus on?

By overriding uri-js with fasturi, the punycode deprecation can be fixed (eslint#17720) without having to upgrade ajv to version 8 (eslint#13888: comment-872591875)
@JudahZF JudahZF requested a review from a team as a code owner August 8, 2024 11:27
@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label Aug 8, 2024
Copy link

linux-foundation-easycla bot commented Aug 8, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link

netlify bot commented Aug 8, 2024

Deploy Preview for docs-eslint canceled.

Name Link
🔨 Latest commit 3d78b41
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/66bb3762786e6d0008bee6bd

package.json Outdated
@@ -165,6 +165,11 @@
"webpack-cli": "^4.5.0",
"yorkie": "^2.0.0"
},
"overrides": {
"ajv": {
"uri-js": "npm:fast-uri@^2.3.0"
Copy link
Member

Choose a reason for hiding this comment

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

Is this the correct syntax? Do we need "npm:" here?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the npm is required to tell the package.json that it should retrive a different package

@fasttime fasttime mentioned this pull request Aug 9, 2024
45 tasks
@JudahZF
Copy link
Author

JudahZF commented Aug 9, 2024

Looks like there are some other dependencies that include punycode, but only show up after overriding this one. I will look into over-riding them over the next week.

@JudahZF
Copy link
Author

JudahZF commented Aug 13, 2024

So it looks like the other packages that use punycode use a different version (punycode.js) that still triggers the warning. I would still recommend that this goes through as it reduces the number of dependencies that use punycode and brings the uri dependency in-line with current versions of ajv.

@nzakas
Copy link
Member

nzakas commented Aug 13, 2024

If this is just to fix one deprecation warning, then I don't think it's very valuable. Deprecations aren't bad and I don't think deprecation hunting in our dependency tree is a good use of time.

@aladdin-add
Copy link
Member

aladdin-add commented Aug 14, 2024

Note that not all package managers support overrides. A better way is adding it in your own projects:

  "overrides": {
    "eslint":{"ajv": {"uri-js": "npm:fast-uri@^2.3.0"}}
  },

@HowieG
Copy link

HowieG commented Aug 14, 2024

@nzakas for what it's worth this user noted there may be a security vuln downstream of uri-js

is is not only that there is deprecated punycode dependency, but also security scan shows issue with old version of braces library:
The NPM package braces, versions prior to 3.0.3, fails to limit the number of characters it can handle, which could lead to Memory Exhaustion. In lib/parse.js, if a malicious user sends "imbalanced braces" as input, the parsing will enter a loop, which will cause the program to start allocating heap memory without freeing it at any moment of the loop. Eventually, the JavaScript heap limit is reached, and the program will crash.
old ajv use old uri-js that use old babel-cli that use old chokidar that use old anymatch that use old micromatch that finally use old braces

#17733 (comment)

@nzakas nzakas closed this Aug 15, 2024
@nzakas
Copy link
Member

nzakas commented Aug 15, 2024

@HowieG that use case doesn't happen in ESLint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug ESLint is working incorrectly
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

5 participants