Skip to content

[release/6.0] Correctly format chuncked response with Content-Length header #70744

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

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

MihaZupan
Copy link
Member

@MihaZupan MihaZupan commented Jun 14, 2022

Fixes #68581
Backport of #69016 to release/6.0

Customer Impact

This is fixing an edge-case bug in HttpClient where we currently violate RFC.
This is a regression in Core compared to Framework.

As per (RFC 2616 section 4.4 para 3):

If a message is received with both a Transfer-Encoding header field and a Content-Length header field, the latter MUST be ignored.

While the server should not respond with both headers, the spec is clear that the client must respect the latter one if it does happen.

Today's behavior (without this fix) means that the user of HttpClient will receive a response body including the chunk frames, which won't be removed. The only workaround is to detect this case and remove chunk framing manually (~600 LOC).

Testing

Added a targeted test case for the new behavior.
We have existing test coverage to ensure nothing else regressed.

Risk

A user could be relying on the current non-RFC-compliant 6.0 behavior. Such users will have to change their code.

The risk of unrelated regressions is low - the change is trivial (swapping the order of two else if branches).

@MihaZupan MihaZupan added area-System.Net.Http tenet-compatibility Incompatibility with previous versions or .NET Framework labels Jun 14, 2022
@MihaZupan MihaZupan changed the title [release/6.0] Correctly format chuncked response with Content-Length heade [release/6.0] Correctly format chuncked response with Content-Length header Jun 14, 2022
@ghost ghost assigned MihaZupan Jun 14, 2022
@ghost
Copy link

ghost commented Jun 14, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #68581
Backport of #69016 to release/6.0

Customer Impact

This is fixing an edge-case bug in HttpClient where we currently go against the RFC.
This is a regression in Core compared to Framework.

As per (RFC 2616 section 4.4 para 3):

If a message is received with both a Transfer-Encoding header field and a Content-Length header field, the latter MUST be ignored.

While the server should not respond with both headers, the spec is clear that the client must respect the latter one if it does happen.

In practice, the distinction means that the user of HttpClient will receive a malformed response body as the chunk framing will not be removed.

Without this change, the only workaround is to detect this case and remove chunk framing manually (~600 LOC).

Testing

Added a targeted test case for the new behavior.
We have existing test coverage to ensure nothing else regressed.

Risk

A user could be relying on the current non-RFC-compliant 6.0 behavior.

The risk of unrelated regressions is low - the change is trivial (swapping the order of two else if branches).

Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http, tenet-compatibility

Milestone: -

@MihaZupan MihaZupan requested review from wfurt and karelz June 14, 2022 18:46
@karelz karelz added this to the 6.0.x milestone Jun 14, 2022
@karelz karelz added the Servicing-consider Issue for next servicing release review label Jun 14, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jun 21, 2022
@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.8 Jun 21, 2022
@danmoseley
Copy link
Member

approved for August -- missed July.

@danmoseley danmoseley merged commit eede86f into dotnet:release/6.0 Jul 12, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http Servicing-approved Approved for servicing release tenet-compatibility Incompatibility with previous versions or .NET Framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants