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: ignore not throw on invalid response headers #950

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pke
Copy link

@pke pke commented Apr 14, 2021

@JakeChampion I think the fix is fine, however I struggle with the test. It runs into the same problem that the response headers are validated. But I can't figure out which package supplies this code. None of the node_modules has a _http_outgoing.js that I could monkey-patch for this one response to not validate the response headers.

I also can't just test parseHeaders() since its private and not globally exported (rightfully so).

So do you have any idea how to test my fix properly?

Fixes #930

@JakeChampion
Copy link
Collaborator

JakeChampion commented Apr 14, 2021

_http_outgoing.js is part of the built-in NodeJS http module, it is not possible to set a response header name in an invalid format with the built-in NodeJS http module.

The only way I can think to test this, would be to use another language to create a http server which can set a response header name in an invalid format. From the issue, we know that golang is able to do that.

I'm also happy to accept this pull-request without a test

@pke
Copy link
Author

pke commented May 1, 2021

I’m working on mocking xhr to Test this.

@github github deleted a comment Jun 22, 2021
@pke
Copy link
Author

pke commented Jun 23, 2021

What is going on here?
I am working on adding a test with mocked xhr and then put it in "Ready for review". So for the time being no more approvals needed.
And please github clean up the spam here, thanks ;)

S69y
S69y approved these changes Jun 26, 2021
@github github deleted a comment from S69y Jun 26, 2021
@github github deleted a comment Mar 22, 2022
@github github deleted a comment Mar 22, 2022
Copy link

@PandaSuits PandaSuits left a comment

Reviwed changes, merge commits

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.

normalizeName should not throw