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

Support legacy HTTP servers that use only LF instead of CRLF #130

Merged
merged 1 commit into from Apr 9, 2018

Conversation

clue
Copy link
Member

@clue clue commented Apr 7, 2018

As per https://tools.ietf.org/html/rfc7230#section-3.5 HTTP would require a CRLF, but apparently also accept a LF for robustness with legacy systems.

Resolves / closes #129

@clue clue added this to the v0.5.9 milestone Apr 7, 2018
@kelunik
Copy link

kelunik commented Apr 7, 2018

@clue Could you share some insights which legacy servers use \n only? Nevermind, issue seems enough for me.

@clue
Copy link
Member Author

clue commented Apr 8, 2018

@kelunik For the reference, I see you're already aware of #129. Other than that, I don't think that this is a particularly common problem, as this particular code didn't change over the last few years. That being said, I've linked to the relevant section of RFC 7230 above and don't see any reason why we wouldn't want to implement this for "robustness" with legacy servers.

@kelunik
Copy link

kelunik commented Apr 8, 2018

@clue The reason why you maybe don't want it is given in the RFC as well:

However, lenient parsing can result in security vulnerabilities if there are multiple recipients of the message and each has its own unique interpretation of robustness (see Section 9.5).

@clue
Copy link
Member Author

clue commented Apr 8, 2018

@kelunik Thank you, I'm well aware and agree on the potential issues on lenient parsers. That being said, I don't see an actual issue here and the specs specifically suggest this behavior (please correct me if I'm wrong).

My vote would be to get this in as it fixes a relevant issue right now and of course we're happy to revisit this should this turn out to open new issues now or in the future 👍

@kelunik
Copy link

kelunik commented Apr 8, 2018

@clue I agree that it's probably fine in that case. 👍

@WyriHaximus WyriHaximus merged commit 22e87bc into reactphp:master Apr 9, 2018
@clue clue deleted the legacy-line-feed branch April 9, 2018 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]Concurrent with more than 1000 requests.
4 participants