Skip to content

Conversation

@mcollina
Copy link
Member

No description provided.

@mcollina mcollina requested a review from indutny October 12, 2021 17:48
@jellelicht
Copy link

FWIW, generating C with the latest commit here and simply copying it over to the Node 14.18.1 tarball leads to a failing regression test;
not ok 1035 parallel/test-http-request-smuggling-content-length
Additionally, the generated C sources don’t look similar to what is included in the Node tarball. Quick question; I’m assuming that the llhttp sources included with yesterday’s security release are generated using llhttp; why not fix and tag llhttp first (or at the same time) as the Node release?

@jellelicht
Copy link

@mcollina I added a PR at #133 that Works For Me ™️

@mcollina
Copy link
Member Author

I need to fix this - I will have time tomorrow morning (I'm missing a commit)

@jellelicht
Copy link

jellelicht commented Oct 13, 2021

I know, and a commit that I assume is very similar to that missing commit can be found at #133 😄

@jellelicht
Copy link

Polite ping, it would be nice to have this known CVE addressed in a release.

@mcollina
Copy link
Member Author

Could you check? This should be good to go

@jellelicht
Copy link

The latest changes seem to pass the test in node

@mcollina mcollina merged commit d6ea943 into v2.1.x Oct 26, 2021
@mcollina mcollina deleted the v2.1.3-fix branch October 26, 2021 12:32
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.

5 participants