Repository navigation
Add folded response header support to SocketsHttpHandler - #27727
Conversation
97f5c58 to
0b74a73
Compare
| // to RFC 7230 3.2.4, a valid approach to dealing with them is to "replace each | ||
| // received obs-fold with one or more SP octets prior to interpreting the field | ||
| // value or forwarding the message downstream", so that's what we do. | ||
| _readBuffer[lfIndex] = (byte)' '; |
There was a problem hiding this comment.
This shouldn't be necessary. TryAddWithoutValidation can handle embedded newlines. I.e. this works fine and produces the same result as if \r\n was not present:
var request = new HttpRequestMessage();
request.Headers.TryAddWithoutValidation("Accept", "text/html,\r\n */*");
foreach (var v in request.Headers.Accept)
{
Console.WriteLine(v);
}There was a problem hiding this comment.
It doesn't always normalize them, e.g. this:
var request = new HttpRequestMessage();
request.Headers.TryAddWithoutValidation("FooBar", "this is a \r\n\t test");
foreach (string v in request.Headers.GetValues("FooBar"))
{
Console.WriteLine(v);
}produces:
this is a
test
There was a problem hiding this comment.
Interesting. Maybe it should? Or if not TryAddWithoutValidation, perhaps when the value is retrieved? There's a whole process here about when stuff is validated/parsed/normalized. If we can just reuse that, it would be nice. Let me look into this real quick.
There was a problem hiding this comment.
What's interesting is that we specifically check for this situation in HttpHeaders.ParseSingleRawHeaderValue. A header value with "\r\n" followed by space or tab is considered valid; one with "\r\n" followed by anything else is not.
If you change the code above to
request.Headers.TryAddWithoutValidation("FooBar", "this is a \r\ntest");Then you'll get an exception of GetValues("FooBar") saying that the FooBar header was not found.
There was a problem hiding this comment.
It seems to me that the validation logic in ParseSingleRawHeaderValue should remove the \r\n -- that seems most in keeping with what this logic is intended to do, which is to produce a validated header value.
There was a problem hiding this comment.
I'll open an issue for it. Changing that shared logic is a much more impactful change than the localized one I have here, and I don't want to block this on that broader issue.
| // Parse the response status line. | ||
| var response = new HttpResponseMessage() { RequestMessage = request, Content = new HttpConnectionResponseContent() }; | ||
| ParseStatusLine(await ReadNextLineAsync().ConfigureAwait(false), response); | ||
| ParseStatusLine(await ReadNextResponseHeaderLineAsync().ConfigureAwait(false), response); |
There was a problem hiding this comment.
Folding should only be valid for header fields, not for status line.
|
Since the perf impact seems negligible, I think we should do this. |
The feature is deprecated by RFC 7230, but some servers still utilize it, and if they do, SocketsHttpHandler currently fails (WinHttpHandler and the netfx handler both allow it, CurlHandler currently fails as well). This commit updates the header parsing logic to allow for them.
0b74a73 to
c3e23bf
Compare
…fx#27727) * Add folded response header support to SocketsHttpHandler The feature is deprecated by RFC 7230, but some servers still utilize it, and if they do, SocketsHttpHandler currently fails (WinHttpHandler and the netfx handler both allow it, CurlHandler currently fails as well). This commit updates the header parsing logic to allow for them. * Address PR feedback Commit migrated from dotnet/corefx@dc355e9
4 participants
The feature is deprecated by RFC 7230, but some servers still utilize it, and if they do, SocketsHttpHandler currently fails (WinHttpHandler and the netfx handler both allow it, CurlHandler currently fails as well). This commit updates the header parsing logic to allow for them.
cc: @geoffkizer, @davidsh