ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

Fix Message Parsing Robustness of CRLF vs LF #185

Closed KryoStoffer closed 5 years ago

KryoStoffer commented 5 years ago

RFC7230 Section 3.5 Message Parsing Robustness contains: Although the line terminator for the start-line and header fields is the sequence CRLF, a recipient MAY recognize a single LF as a line terminator and ignore any preceding CR.

Minimal code change to allow for broken HTTP/1.1 implementations.

Also requires https://github.com/ninenines/cowlib/pull/74 to work properly.

essen commented 5 years ago

Something like this needs one or two things to get merged: you must specify which implementations are broken and if it's only custom implementations it must be disabled by default and an option can be used to enable it.

KryoStoffer commented 5 years ago

Thanks for the quick feedback.

Problem was noticed while talking to a device from vendor CTS with this model number: HES-3106-PLUS. In the server header from the device it claims to be running Boa/0.94.14rc21. I have attached cts.txt containing a raw stream dump example that shows the broken web-server.

Other http clients handles this without any options eg. httpc:request, curl or wget. I could implement some more testing of this, but the intention was not harm well behaving servers. What risk do you see by having this enabled by default ?

essen commented 5 years ago

I hear you. But that's the problem, though, isn't it? Clients are not following the spec strictly to accomodate for broken implementations, but then new implementations continue to be broken because they test against clients that are not strict. Note that this state of affairs is why the spec has this MAY; if the spec was written from scratch today they would not allow a single LF like this.

As a rule of thumb I add workarounds always-on only for the most popular software (Cowboy has some workarounds for Flash player for example), and otherwise they are hidden behind configuration.

KryoStoffer commented 5 years ago

Ok this does definitely not fall into the popular software category. I have also asked the vendor to fix there implementation.

The specification of "MAY" in RFC2119 does say that it is optional, but MAY also states: "MUST be prepared to interoperate with another implementation." So the usage of MAY is a bit ambiguous in that specific context since the whole section is all about interoperability.

I am aware of the ietf draft work https://tools.ietf.org/html/draft-iab-protocol-maintenance-01 but it is still a draft, so specifically for such an old and widely implemented protocol, I feel it is a bit early to pull the plug on Postel's law.

But in the end it is you project and decisions I respect that.

essen commented 5 years ago

It's just an option. It shouldn't be a much bigger patch than what you already have.

essen commented 5 years ago

Hello, are you going to update the patches by adding a configuration option as suggested, or should I close for now?

KryoStoffer commented 5 years ago

Sorry, for leaving you hanging.

I tried to make it configurable but the patch ended up being somewhat larger. I opted for using httpc for the few defective clients.

br.

Kristoffer Larsen