Skip to content

Conversation

@ahayes91
Copy link

@ahayes91 ahayes91 commented Jul 21, 2025

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

https://security.snyk.io/vuln/SNYK-JS-FORMDATA-10841150 was identified and published over the weekend. For consumers using cypress, they may see a new critical Snyk vulnerability like [email protected] › @cypress/[email protected][email protected].
Updating to the patch version 4.0.4 should fix this issue.
(Similar issue in the wild: axios/axios#6970 )

It seems very likely that there are flaky tests and timeout issues here with the existing tests, as I'm seeing inconsistent failures on npm test locally and in the CI. Is this expected?

E.g.:

​ FAIL ​ tests/test-timeout.js
 ✖ Connect Timeout Error should set 'connect' property to true

  test: tests/test-timeout.js
  operator: ok
  expected: true
  actual: false
  at: Request._callback
    (/Users/aislinn.hayes/code/request/tests/test-timeout.js:178:7)
  stack: >-
    Error: Connect Timeout Error should set 'connect' property to true
        at Test.assert [as _assert] (/Users/aislinn.hayes/code/request/node_modules/tape/lib/test.js:443:48)
        at Test.bound [as _assert] (/Users/aislinn.hayes/code/request/node_modules/tape/lib/test.js:89:17)
        at Test.assert (/Users/aislinn.hayes/code/request/node_modules/tape/lib/test.js:562:7)
        at Test.bound [as ok] (/Users/aislinn.hayes/code/request/node_modules/tape/lib/test.js:89:17)
        at Request._callback (/Users/aislinn.hayes/code/request/tests/test-timeout.js:178:7)
        at self.callback (/Users/aislinn.hayes/code/request/request.js:22:129)
        at Request.emit (node:events:518:28)
        at ClientRequest.<anonymous> (/Users/aislinn.hayes/code/request/request.js:73:359)
        at Object.onceWrapper (node:events:632:28)
        at ClientRequest.emit (node:events:518:28)

​ FAIL ​ tests/test-timeout.js 1 failed of 29 3s
 ✖ Connect Timeout Error should set 'connect' property to true

and

​ FAIL ​ tests/test-form-data.js
 ✖ should be equal

  test: tests/test-form-data.js
  operator: equal
  expected: "null"
  actual: "{ [Error: read ECONNRESET] errno: -54, code: 'ECONNRESET', syscall: 'read' }"
  at: Request._callback
    (/Users/aislinn.hayes/code/request/tests/test-form-data.js:113:9)
  stack: |-
    Error: read ECONNRESET
        at TCP.onStreamRead (node:internal/stream_base_commons:216:20)

CI shows:

 FAIL ​ tests/test-form-data.js
 ✖ should be truthy

  test: tests/test-form-data.js
  operator: ok
  expected: true
  actual: false
  at: Server.<anonymous> (/home/circleci/repo/tests/test-form-data.js:32:7)
  stack: >-
    Error: should be truthy
        at Test.assert [as _assert]
(/home/circleci/repo/node_modules/tape/lib/test.js:443:48)
        at Test.bound [as _assert]
(/home/circleci/repo/node_modules/tape/lib/test.js:89:17)
        at Test.assert (/home/circleci/repo/node_modules/tape/lib/test.js:562:7)
        at Test.bound [as ok]
(/home/circleci/repo/node_modules/tape/lib/test.js:89:17)
        at Server.<anonymous> (/home/circleci/repo/tests/test-form-data.js:32:7)
        at Server.emit (node:events:517:28)
        at parserOnIncoming (node:_http_server:1130:12)
        at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17)

It kind of feels like building in some retries would be a good stop gap (not easy with tapjs apparently - tapjs/tapjs#151) or diving into the server logic in those tests to see if it needs to be replaced with something a bit more sophisticated (https://mswjs.io/ would be my vote!).
Both of those are quite a bit outside the scope of this PR, and even with a few different variations of Cursor I wasn't able to get all the tests passing reliably here - so I'm going to pass this back to the Cypress team to decide how much effort they want to invest here to get these working.

@cypress-app-bot
Copy link

@ahayes91 ahayes91 marked this pull request as ready for review July 21, 2025 10:27
Copy link

@derektiffany derektiffany left a comment

Choose a reason for hiding this comment

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

lgtm, yes plz 🙏

@MikeMcC399
Copy link

MikeMcC399 commented Jul 22, 2025

@ahayes91

@MikeMcC399
Copy link

@ahayes91

Regarding your PR - I do think this makes sense, independently of the test failures. The change forces an update to form-data@~4.0.4 which was already in its the semver range - so this should be intrinsically safe. The Cypress.io team would need to merge this PR and then update the version of @cypress/request used in Cypress.

  • Unfortunately this repo ( @cypress/request) is fighting a rear-guard action as it should ultimately be replaced by something else. See Replace @cypress/request cypress#29775

  • I closed a related issue test fails in all current Node.js versions #68 as unsolvable so I doubt if anybody is going to be able to resolve this, and you said that you had also tried.

  • As far as the new issue I posted is concerned Critical vulnerability CVE-2025-7783 using form-data 4.0.1 & 4.0.3 cypress#32066 - I believe that this will only be partially resolved by an update of @cypress/request as there is some other usage of a vulnerable form-data version in the Cypress binary. I was planning to look a bit deeper into this. I don't know if Snyk picks up this second usage, but it is certainly found by Trivy on the latest cypress/included Docker image

@ahayes91
Copy link
Author

Good info Mike, thank you!

We do have a workaround in our consumer repo in any case to force form-data to the latest in our package.json, so we're not completely stuck:

  "resolutions": {
    "form-data": "4.0.4"
  },

@MikeMcC399
Copy link

@ahayes91

I believe that your workaround is for Yarn only. In some cases you would be able to resolve the issue simply by uninstalling Cypress and reinstalling. For npm, the command npm audit fix can be used.

@cacieprins
Copy link

These tests are failing on our main branch as well, and may have to do with the lack of a lockfile. I'm investigating so that we can get this merged - thank you for opening it!

@MikeMcC399
Copy link

MikeMcC399 commented Jul 22, 2025

@cacieprins

These tests are failing on our main branch as well, and may have to do with the lack of a lockfile. I'm investigating so that we can get this merged - thank you for opening it!

These tests have been failing for a long time. See #68. It's not related to any lockfile. @jennifer-shehane is quite aware of this.

@cypress-app-bot
Copy link

🎉 This PR is included in version 3.0.9 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MikeMcC399
Copy link

@ahayes91

[email protected] has now been published, and this includes @cypress/[email protected] with your change in it. If you update, you shouldn't need your resolutions workaround any more.

Thanks for starting the ball rolling on this one! It had quite a (positive) knock-on effect in different places!

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.

5 participants