fix: only update content-length header if one was present in spy

[Bourg]

• Closes Using cy.intercept to spy adds a "Content-Length: 0" request header #24407

Additional details

Prior to this change, if cy.intercept is used to spy on a request with no modifications, a content-length header will be added to the outbound request regardless of whether one was present on the spied request. The code that does this is trying to automatically fix the content-length header in cases where the body was changed, but this behavior only makes sense if the original request included a content-length to update.

Steps to test

The issue links to a reproduction repo I created when I filed the issue. I want to add some automated test coverage, but the affected middleware does not currently have any coverage, so could use some guidance on whether / where I could test this.

How has the user experience changed?

No change to user experience

PR Tasks

• Have tests been added/updated?
• Has a PR for user-facing changes been opened in cypress-documentation?
• Have API changes been updated in the type definitions?

[CLAassistant]

All committers have signed the CLA.

[flotwig]

@Bourg Looks good to me. To add tests, I'd recommend adding an integration test in this file: https://github.com/cypress-io/cypress/blob/47c787f5ae930fb0fa4b215a29330f5e362ed98b/packages/proxy/test/integration/net-stubbing.spec.ts

You can run it via yarn test-integration in packages/proxy.

[AtofStryker]

@Bourg this change also looks good on my end. Would you be able to add a test like mentioned above? If not let us know and we can come up with something!

[Bourg]

@AtofStryker I made an attempt at adding a test but honestly, with my level of knowledge of the net-stubbing internals, setting up the correct state of the supertest to simulate this scenario proved a bit too much for me in the time I had to work on this. Some help on the test would be greatly appreciated.

[AtofStryker]

@Bourg I completely understand. I should be able to take a look and add a test in the near future.

[AtofStryker]

@Bourg I had some time to go ahead an add a small unit test in 04f03d6. It wasn't trivial 😅 . It isn't a full on integration test like mentioned above, but I think it gets us what we need and it lives in the same package as the code

added code. can no longer review

[Bourg]

Thank you @AtofStryker - I don't think I would have gotten there on the test implementation, I really appreciate it! I suppose now all that's needed is review.

[mschile]

Should a test be added here instead of the unit test (or maybe in addition to)?

[Bourg]

@mschile as far as I have been able to tell, reproducing this from inside a Cypress test requires that there actually be an API upstream that behaves differently depending on the content-length header - there's no way to test it in isolation that I've found. That was part of why this was so hard for me to identify this as a root cause of an issue I was having in the first place - everything looks untouched from inside the test runner.

The way we originally found this issue was that healthy API endpoints would start replying 400 when spied on by Cypress, and the reason turned out to be that AWS ELB - at least as configured in our case - did not expect the content-length header on a GET request and would reject the requests. For the minimal reproduction linked in the issue, I made a simple HTML document that makes a single fetch call to an Express server that echoes back the request headers. You would basically need to move all that machinery into those tests - the only point at which you can see the issue is from the perspective of the Express server, not Cypress.

I do not believe there is any event you can listen to in Cypress that gets you at the request headers of a spied request at the relevant point of the lifecycle. I agree it would be better if we could integration test this, but I think it may be more trouble than it's worth unless there's a more pure way to see the modified headers without an external server.

[mschile]

Thanks @Bourg, we do start an external server in the driver tests so we should be able to output the content-length header to verify it doesn't get inadvertently added.

[AtofStryker]

I'm going to take a look into this in a few 😃

[AtofStryker]

@mschile I added a simple test in b05eda4. What do you think?

[mschile]

Looks good! You could even use the existing dump-headers request if you wanted. Something like:

it('does not calculate content-length on spied request if one does not exist on the initial request (if merging)', { retries: 0 }, function () {
  cy.intercept('/dump-headers', function (req) {
    // modify the intercepted request to trigger a request merge in net_stubbing
    req.headers['foo'] = 'bar'
    // send the modified request and skip any other
    // matching request handlers
    req.continue()
  })

  cy.visit('/dump-headers')

  // ensure the content-length header does not exist
  cy.contains('content-length').should('not.exist')
})