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

Update dependencies #45

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

stusmall
Copy link

@stusmall stusmall commented Oct 4, 2023

PR Checklist:

  • I have run npm test locally and all tests are passing.
  • I have added/updated tests for any new behavior.
  • If this is a significant change, an issue has already been created where the problem / solution was discussed: [N/A, or add link to issue here]

PR Description

The real point of this PR is to update some old dependencies and clear up vulnerabilities. In service of this I did some clean up of key material that has been checked in and also rotated them. Any changes to the code was trigger by the update of standard. More details about specific changes can be found in each commit.

The keys were generated with 1024 bit keys.  Depending on the system
configuration this can be rejected as unsafe causing the test to fail.
This updates the config to use 4096 keys and regenerates all key
material
This doesn't update all dependencies or get everything to the newest
versions.  The goal was to eliminate current critical and high vulens
in the tree.  There are still a few mediums kicking around that could be
squashed too.  I also opportunistically bump other versions when
possible
These keys predate the ca folder and update script contained in there.
I udpated the tests using them to use one of the keys managed by the
update script and removed them
@cypress-app-bot
Copy link

@stusmall stusmall changed the title Update depedencies Update dependencies Oct 4, 2023
@stusmall
Copy link
Author

stusmall commented Oct 4, 2023

One small detail. I never could get test-tunnel working locally even before changes. It seems to be related to my local set up and not with the code. It works in CI

@MikeMcC399
Copy link

@stusmall

  • Does this PR resolve issue Test failures for Node.js 18.17 and 20.x #37?
  • Are you testing against Node.js 18 and 20?
  • GitHub Actions CI is using Node.js 18.16.0. I believe this was out of necessity instead of using a later version such as the current LTS version Node.js 18.18.0 which was failing.
  • CircleCI tests against Node.js 14. That is no longer very meaningful because this version is in end-of-life since Apr 30, 2023

@stusmall
Copy link
Author

stusmall commented Oct 11, 2023

@MikeMcC399

The scope of my changes was just to update some dependencies to clear up, old known CVEs. The goal isn't to try and tackle any other issues. It doesn't resolve existing issues with node 20. I have tested it with node 18.18.1 though and it passes. Here is an example of it passing on another branch where I was playing with a matrix with multiple versions. I don't know what the issue causing pinning on 18.16.0 was but it doesn't look to be an issue with 18.18.1.

EDIT: It appears to be specific with 18.17. Here is a run on a my matrix branch showing 18.16 and 18.18 passing but 18.17 failing.

@MikeMcC399
Copy link

MikeMcC399 commented Oct 12, 2023

@stusmall

Thanks for the feedback and explaining your goals. I cross-checked with Node.js 18.18.1 and the current version is now passing (without your PR changes).

There is something wrong with this PR now (or the branch stusmall:update-depedencies it is based on) as GitHub is continuously showing "Processing updates". Maybe you need to force push it to reset it after your matrix tests?

@stusmall
Copy link
Author

Hmmm. I saw that issue on my forked repo too. It seems I've caused some internal trouble with GitHub which is a new badge of honor. I'll try a couple things to get it unstuck

@stusmall stusmall closed this Oct 12, 2023
@stusmall stusmall reopened this Oct 12, 2023
@stusmall
Copy link
Author

@MikeMcC399 The issue with being stuck should be fixed. I had one more bit of clean up on the change to the github action workflow. It should be working now. The test is passing on my fork but you will need to approve the workflow to run for it run here.

@MikeMcC399
Copy link

@stusmall

The issue with being stuck should be fixed. I had one more bit of clean up on the change to the github action workflow. It should be working now. The test is passing on my fork but you will need to approve the workflow to run for it run here.

I'm a community Contributor, not a member of the Cypress team, and I don't have privileges to approve workflows or to merge PRs.

The next step would be for a member of the Cypress team to get involved with this PR. In previous conversations they have said they would like to move away from using this code and use fetch instead. Hopefully they will still want to use your contribution here.

@stusmall
Copy link
Author

@MikeMcC399 Do you ever see cypress team interact with this repo anymore or is it abandoned?

@MikeMcC399
Copy link

MikeMcC399 commented Oct 28, 2023

@stusmall

Do you ever see cypress team interact with this repo anymore or is it abandoned?

The last time that the Cypress team was visibly active here was Sep 2, 2023. Only they can say what their plans are, however this repo is an essential dependency for Cypress.

I'm also waiting for feedback on 2 issues and 1 PR that I have submitted in this repo.

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.

None yet

3 participants