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: remove/upgrade outdated deps #11

Merged
merged 12 commits into from
Nov 24, 2021
Merged

fix: remove/upgrade outdated deps #11

merged 12 commits into from
Nov 24, 2021

Conversation

JJ
Copy link

@JJ JJ commented Nov 18, 2021

As described in #10

PR Checklist:

PR Description

Was: Essentially upgrade to the version where it was patched.
Now: Well,

  • The karma suite has been upgraded, mainly because some of them were incompatible with current versions of node
  • I've eliminated two tests (and corresponding variables), one of which was already commented out because it failed. They all failed now, so it means that part is not tested now, because it would probably need some refactoring which is outside the scope of this PR (famous last words...)
  • Since the reviewer requested to add package-lock.json, I did... together with its disabling from .gitignore
  • har-validator has been eliminated, since it was deprecated, and no wonder no one had noticed, since it wasn't used anyway.
  • nyc upgraded to take care of the deprecation notice for non-monorepoid istanbul.

@JJ
Copy link
Author

JJ commented Nov 19, 2021

Ping?

@macGYves
Copy link

Since I'm not a maintainer of this project, I am actually glad that my approval was not sufficient to get this PR merged.
It would be good to get the vulnerability fixed soon.

@EtienneBruines
Copy link

@flotwig Could you look into merging this PR to mitigate the security vulnerability?

@sddtc
Copy link

sddtc commented Nov 22, 2021

Please merge it ASAP 🙏

Copy link

@flotwig flotwig left a comment

Choose a reason for hiding this comment

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

Please update the package-lock.json.

Could you look into merging this PR to mitigate the security vulnerability?

It would be good to get the vulnerability fixed soon.

Also, note that this is not a vulnerability in Cypress. We do not use options.httpSignature, nor is there any way for a third party to exploit it. Automated vulnerability scanners like Snyk are a great way to waste time and tick checkboxes.

Brought in from cypress-io#13, or couldn't install anything
@JJ
Copy link
Author

JJ commented Nov 22, 2021

Please update the package-lock.json.

Could you look into merging this PR to mitigate the security vulnerability?

It would be good to get the vulnerability fixed soon.

Also, note that this is not a vulnerability in Cypress. We do not use options.httpSignature, nor is there any way for a third party to exploit it. Automated vulnerability scanners like Snyk are a great way to waste time and tick checkboxes.

Consider it general upgrading and maintenance. npm i reveals this:

npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'karma-cli@1.0.1',
npm WARN EBADENGINE   required: { node: '0.10 || 0.12 || 4 || 5 || 6' },
npm WARN EBADENGINE   current: { node: 'v16.2.0', npm: '7.19.1' }
npm WARN EBADENGINE }
npm WARN deprecated urix@0.1.0: Please see https://github.com/lydell/urix#deprecated
npm WARN deprecated sprintf@0.1.5: The sprintf package is deprecated in favor of sprintf-js.
npm WARN deprecated har-validator@5.1.5: this library is no longer supported
npm WARN deprecated resolve-url@0.2.1: https://github.com/lydell/resolve-url#deprecated
npm WARN deprecated circular-json@0.5.9: CircularJSON is in maintenance only, flatted is its successor.
npm WARN deprecated circular-json@0.3.3: CircularJSON is in maintenance only, flatted is its successor.
npm WARN deprecated chokidar@2.1.8: Chokidar 2 will break on node v14+. Upgrade to chokidar 3 with 15x less dependencies.
npm WARN deprecated querystring@0.2.0: The querystring API is considered Legacy. new code should use the URLSearchParams API instead.
npm WARN deprecated uuid@3.4.0: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
npm WARN deprecated phantomjs-prebuilt@2.1.16: this package is now deprecated
npm WARN deprecated uuid@3.4.0: Please upgrade  to version 7 or higher.  Older versions may use Math.random() in certain circumstances, which is known to be problematic.  See https://v8.dev/blog/math-random for details.
npm WARN deprecated request@2.88.2: request has been deprecated, see https://github.com/request/request/issues/3142
npm WARN deprecated codecov@3.8.3: https://about.codecov.io/blog/codecov-uploader-deprecation-plan/
npm WARN deprecated istanbul@0.4.5: This module is no longer maintained, try this instead:
npm WARN deprecated   npm i nyc
npm WARN deprecated Visit https://istanbul.js.org/integrations for other alternatives.
npm WARN deprecated core-js@2.6.12: core-js@<3.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Please, upgrade your dependencies to the actual version of core-js.

I'll try to address some of them in this PR.

@JJ
Copy link
Author

JJ commented Nov 22, 2021

Please update the package-lock.json.

Hey, that's not even in the repo... It's even added to the .gitignore. I can add it if you want...

@flotwig
Copy link

flotwig commented Nov 22, 2021

Lol my bad, I had it locally, but it's not in the repo.

@JJ
Copy link
Author

JJ commented Nov 22, 2021

Lol my bad, I had it locally, but it's not in the repo.

No problem I was really surprised that I kept adding and eliminating stuff and it didn't show up in the git status. There's probably a good reason for that hidden in the original code... But I guess that nowadays it's better if we keep it in the repo. And a good thing to generate that too, since that revealed a series of errors that I tried to address in #13 (superseded now, I'll close it).

Copy link

@GrahamCampbell GrahamCampbell left a comment

Choose a reason for hiding this comment

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

The package-lock.json file should not be committed.

@flotwig
Copy link

flotwig commented Nov 22, 2021

The package-lock.json file should not be committed.

Direct contradiction with the NPM docs:

https://github.com/npm/cli/blob/latest/docs/content/configuring-npm/package-lock-json.md#description

This file is intended to be committed into source repositories

@JJ
Copy link
Author

JJ commented Nov 22, 2021

The package-lock.json file should not be committed.

Direct contradiction with the NPM docs:

https://github.com/npm/cli/blob/latest/docs/content/configuring-npm/package-lock-json.md#description

This file is intended to be committed into source repositories

🍿

@JJ
Copy link
Author

JJ commented Nov 22, 2021

Look, I don't want to go one way or the other. I'm just saying the change to .gitignore was introduced 4 years ago here ffdf0d3 The commit message does not seem to point to an issue or anything like that, so my POV is that in absence of clear repo-wide directives, the general directive that @flotwig mentions applies.

@JJ
Copy link
Author

JJ commented Nov 22, 2021

OTOH,

git log --full-history -- package-lock.json

Seems to indicate it's never actually been there, so there's that.

@flotwig flotwig changed the title Upgrade to avoid vulnerability fix: remove/upgrade outdated deps Nov 24, 2021
@flotwig flotwig merged commit 8608a5d into cypress-io:master Nov 24, 2021
@flotwig
Copy link

flotwig commented Nov 24, 2021

🎉 This PR is included in version 2.88.8 🎉

The release is available on:

Your semantic-release bot 📦🚀

@@ -34,8 +34,7 @@
"extend": "~3.0.2",
"forever-agent": "~0.6.1",
"form-data": "~2.3.2",
"har-validator": "~5.1.3",

Choose a reason for hiding this comment

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

we might want to keep this for now

Copy link
Author

Choose a reason for hiding this comment

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

It's deprecated, and it wasn't actually tested, which is why this didn't show up in a simple test.

JJ added a commit to JJ/request that referenced this pull request Nov 25, 2021
Mainly to avoid problems caused with cypress-io#11, which worked well with npm, crashed and burned with yarn
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants