Relax need for all line endings to have CR

[richardhopton]

Some requests don't have CRs so BufferedReadStream would throw an exception on certain multipart requests

Fixes: https://github.com/aspnet/Home/issues/2939

[dnfclas]

All CLA requirements met.

[Tratcher]

The spec is quite specific that CRLF is required: https://tools.ietf.org/html/rfc2046#section-4.1.1 What client are you using that is generating invalid content? https://stackoverflow.com/questions/10765243/how-can-i-rewrite-this-curl-multipart-form-data-request-without-using-f

[richardhopton]

The attached ticket shows the exact curl I am using - I get exactly the same error in Postman.

Agreed that's what the spec says, however if requiring a CR breaks a percentage of requests the pragmatist in me feels it should be supported. My solution doesn't break any existing requests but if more forgiving on this one aspect. It's worth noting that other languages that I've built this test app in work just fine with the same request.

[Tratcher]

Your curl sample is not compelling given curl is capable of generating correct multipart requests (https://stackoverflow.com/questions/10765243/how-can-i-rewrite-this-curl-multipart-form-data-request-without-using-f). Do you have any other clients we should consider?

[richardhopton]

I created the body of this POST many months ago using flow.js - It's possible that it got garbled during copy/paste, however as stated I've built this test application in a bunch of other languages (Go/Clojure/Java/Javascript) and they all accept the request. Interestingly even .net Framework and Mono are able to process this same request.

Looking at the existing .net core implementation it's also possible to build a request that is treated as valid that accidentally trims the last character of the line - something none of the other implementations I mentioned above do.

[Tratcher]

@glennc

[Eilon]

I'm very weary of not following the RFCs on issues like this. It almost always comes back to bite us because some other standards-respecting client is probably expecting the current behavior.

I'm aware that in general the rule is still "be generous in what you accept, and strict in what you return," but I'm a hesitant to apply that logic in this scenario...

[glennc]

Sure, but the HTTP 1.1 RFC also says in its appendix that:

The line terminator for message-header fields is the sequence CRLF. However, we recommend that applications, when parsing such headers, recognize a single LF as a line terminator and ignore the leading CR.

I like the idea of following that lead here and being more tolerant. This would not be the first place we have implemented something more forgiving/different to the strict interpretation of the RFC. Though I agree that in general we should stick to the RFC and each discussion should be weighed individually.

Should we separate the discussion about CRLF with the one about trimming the last char? Is there controversy about the last char bit?

[blowdart]

We need to be very careful of response splitting here. https://www.acunetix.com/websitesecurity/crlf-injection/

[richardhopton]

does the response splitting issue apply to POST bodies which is what being parsed by this logic? Also I don't believe my change would actually introduce any additional vulnerabilities that didn't already exist in this code if it is used to process more than just POST bodies.

[richardhopton]

I’ll send a separate PR for the CR in the middle of the string issue, and rebase this PR on it.

[richardhopton]

As promised I've raised https://github.com/aspnet/HttpAbstractions/pull/1006 and rebased this commit on it...

[richardhopton]

Did you reach a conclusion on this PR? Do you plan to merge or will you document how the .net core implementation is stricter to the spec than the afore mentioned more relaxed implementations?

[Eilon]

@glennc / @Tratcher / @muratg - any further action planned on this PR?

[muratg]

@richardhopton Thanks for the PR and the discussions!

@Eilon, I think we should possibly document that we're strict with the RFC and close this one.

@glennc if agreed, could you close? Thanks.

[Eilon]

@muratg - where should/could we document it? In a doc comment? docs.asp.net?

[muratg]

@Eilon probably here: https://docs.microsoft.com/en-us/aspnet/core/mvc/models/file-uploads

Let's do that if we get more complaints about line number limits and whatnot.