Skip to content

Conversation

IMSoP
Copy link
Contributor

@IMSoP IMSoP commented May 9, 2016

The stream handler assumed all HTTP headers contained exactly one space,
but the standard says there may be zero or more. Should fix Bug #47021,
and any other edge cases caused by a web server sending unusual spacing,
e.g. the MIME type discovered from Content-Type: can no longer contain
leading whitespace.

The stream handler assumed all HTTP headers contained exactly one space,
but the standard says there may be zero or more. Should fix Bug #47021,
and any other edge cases caused by a web server sending unusual spacing,
e.g. the MIME type discovered from Content-Type: can no longer contain
leading whitespace.
> Each header field consists of a case-insensitive field name followed
> by a colon (":"), optional leading whitespace, the field value, and
> optional trailing whitespace. */
char *http_header_value = http_header_line + 1;
Copy link
Member

Choose a reason for hiding this comment

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

Declarations need to go at the top of the block in C89.

@IMSoP
Copy link
Contributor Author

IMSoP commented May 9, 2016

Thanks for looking over this so quickly! I'll hopefully have time to look into your feedback properly tomorrow, and update the patch. :)

- fix removal of trailing whitespace (and add more test cases)
- more obvious detection if there is no : in the header
- coding style / C89 compliance
@smalyshev smalyshev added the Bug label Sep 5, 2016
@hlarsen
Copy link

hlarsen commented Dec 1, 2016

hey all,

i'd just like to check the status of this - it looks like we're seeing an issue that this patch will solve.

@IMSoP
Copy link
Contributor Author

IMSoP commented Dec 1, 2016

@nikic Since you reviewed before, does this look OK now? I'm pretty sure the Travis error was just a false report.

@krakjoe
Copy link
Member

krakjoe commented Jan 3, 2017

awaits nikitas response

@nikic nikic self-assigned this Jan 3, 2017
@nikic
Copy link
Member

nikic commented Jan 7, 2017

Question: What should happen with the headers we add to the $http_response_headers variable? As the PR implements it right now, if there is trailing whitespace, the header would contain a NUL byte. That's obviously not right, but I'm not sure whether its better to a) provide the header as-is (with trailing whitespace) or b) strip trailing whitespace.

The latter would certainly be easier to implement. Some of the APIs used there don't accept explicit lengths :/

@krakjoe
Copy link
Member

krakjoe commented Jan 7, 2017

The spec indicates that the trailing white space is allowed, however since it serves no practical purpose, I see no harm in removing it - the vast majority of the time, the programmer is surely going to do that anyway, and if they don't their code may exhibit unexpected behaviour.

@nikic
Copy link
Member

nikic commented Jan 7, 2017

Modified version merged as 5146d9f with a followup in ec1b7b9. Trailing whitespace is stripped from headers in $http_response_header. A "header" that only consists of whitespace is not considered the start of the body.

Please double check if the merged patch is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants