fix: HTTP response with invalid headers doesn't throw error #28865

[BernardoSousa03]

When receiving the described HTTP response Cypress resets the headers. This would cause the validateHeaderName method from node to be called which would cause an error, since the headers where invalid. Now Crypress verifies all the headers before reseting them, discards invalid ones and sends a warning in the console when debug module is on.

• Closes Errors from (fault)net::ERR_EMPTY_RESPONSE and TypeError [ERR_INVALID_HTTP_TOKEN]: Internal error during proxy #28865

Additional details

The error lead to Cypress throwing errors when it wasn't supposed to

Steps to test

Create a server that responds with invalid headers to a HTTP request

How has the user experience changed?

The tests don't fail anymore and with the debug module on there is a warning that shows the invalid headers

PR Tasks

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

[CLAassistant]

All committers have signed the CLA.

[cypress-app-bot]

• Create a Draft Pull Request if your PR is not ready for review. Mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.

[jennifer-shehane]

@BernardoSousa03 Thanks for the contribution and for adding a test case around the issue! I’ll check in the upcoming sprint to see who is available to review.

[cacieprins]

I think this approach is fine for now, especially considering the method of failure before. I'd like to see a warning logged to console rather than hiding it behind a debug statement, though - it's important that the modification of the headers be obvious, in case the client is expecting malformed headers.

[BernardoSousa03]

Thank you for your review @cacieprins. I'll get to it as soon as i can!

[cacieprins]

Great, thank you @BernardoSousa03 !

Additionally, it may be useful to include validation of the header value, as well, with validateHeaderValue from the http package.

I recommend taking a look at /packages/errors/.

• a new entry can be added to errors.ts for templating how the error message is displayed in stdout
• including the error thrown, original http header (if not included in the error object's message), as well as the the URL and method being proxied for the warning display would be great
• a new entry in packages/errors/test/unit/errors_spec.ts for CI to accept the new error template - make sure to run yarn test in this package to generate a snapshot of the formatted error message. You can review the error message visually by opening the corresponding HTML file in ./packages/errors/__snapshot-html__/
• errors.warning('PROXY_ENCOUNTERED_INVALID_HEADER_NAME', error, headerName) and errors.warning('PROXY_ENCOUNTERED_INVALID_HEADER_VALUE', error, headerValue) would be the expected usage in the proxy package.

This message could read similar to:

Warning: While proxying a ${fmt.highlight(method)} request to ${fmt.url(urll)}, an HTTP header did not pass validation, and was removed. This header will not be present in the response received by the application under test.

Invalid header name: ${fmt.code(JSON.stringify(header, undefined, 2))}

${fmt.highlightSecondary(error)

[BernardoSousa03]

@cacieprins while trying out the /packages/errors/ i found out that there is no visual representation for the warning while using errors.warning in the cypress GUI app. Is this what you had in mind? I was thinking, since there is no plausible reason for having badly formed headers, it might be of interest to show the user it's mistake. I tried it with errors.throwErr but with no success. Should i just stick to using the first solution?

[cacieprins]

The errors package is for displaying errors to the commandline - they won't show in the GUI, but they will be helpful for folks who may run into issues with this in CI. Please use errors.warning, instead of errors.throwErr, because we don't want to fully throw an error when this is encountered :)

[BernardoSousa03]

@cacieprins i have added the requested changes, anything else let me know!

[cacieprins]

It looks like there's a ts error in response-middleware.ts on L#321 - verify by running yarn check-ts from the root of the project. Other than that, looks good. I'm a little concerned this may invalidate some system test snapshots - can you run those to verify?