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

http: accept 0xFF as a valid character in header values #65

Closed
wants to merge 1 commit into from

Conversation

lukiano
Copy link
Contributor

@lukiano lukiano commented Sep 2, 2020

Hello! I took a stab at trying to fix the issues I'm seeing in nodejs/node#27711 (comment)

I wasn't able to generate a test case using the test suite of this project based on .md files, as I couldn't recreate the 0xFF iso-8859-1 character. However I downloaded the Node source code and copied over the generated C files of this project and then compiled Node itself. When I ran the short program described in the comment above it worked successfully.

I'd like to hear thoughts and opinions on this change.

Kind regards.

@indutny
Copy link
Member

indutny commented Sep 2, 2020

Oh gosh. Good catch!

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

LGTM. I'll add a test myself right after landing.

indutny pushed a commit that referenced this pull request Sep 2, 2020
PR-URL: #65
Reviewed-By: Fedor Indutny <fedor@indutny.com>
@indutny
Copy link
Member

indutny commented Sep 2, 2020

Landed in 883d9dd and 8e6903c. Thank you!

@lukiano
Copy link
Contributor Author

lukiano commented Sep 22, 2020

Hi and sorry to bother again, do you know by chance when this will land on Node.JS ? Thanks and regards.

@indutny
Copy link
Member

indutny commented Sep 30, 2020

Hey, sorry for a delay! There has been some release scheduling conflicts that had to be addressed. I've submitted a PR nodejs/node#35435

lpinca pushed a commit to nodejs/node that referenced this pull request Oct 4, 2020
PR-URL: #35435
Refs: nodejs/llhttp#65
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
danielleadams pushed a commit to nodejs/node that referenced this pull request Oct 6, 2020
PR-URL: #35435
Refs: nodejs/llhttp#65
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@indutny indutny closed this Oct 8, 2020
MylesBorins pushed a commit to nodejs/node that referenced this pull request Nov 3, 2020
PR-URL: #35435
Refs: nodejs/llhttp#65
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Nov 16, 2020
PR-URL: #35435
Refs: nodejs/llhttp#65
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
PR-URL: nodejs#35435
Refs: nodejs/llhttp#65
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
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.

2 participants