Skip to content

Conversation

@FatTigerWang
Copy link
Contributor

fix #43523 HttpSys Content-Length and Transfer-Encoding conflicts

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

Fixes #43523 HttpSys Content-Length and Transfer-Encoding conflicts

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Sep 23, 2022
@ghost
Copy link

ghost commented Sep 23, 2022

Thanks for your PR, @FatTigerWang. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@Tratcher Tratcher self-assigned this Sep 23, 2022
@FatTigerWang FatTigerWang requested review from Tratcher and removed request for BrennanConroy, JamesNK and halter73 September 23, 2022 16:58
@Tratcher
Copy link
Member

This will need some unit tests.

Also, this part of the original issue is missing:

This should probably be triggered more eagerly from the constructor so apps don't read the CL header directly before it's dropped.
https://github.com/dotnet/aspnetcore/blob/main/src/Servers/HttpSys/src/RequestProcessing/Request.cs#L36

@FatTigerWang
Copy link
Contributor Author

This should probably be triggered more eagerly from the constructor so apps don't read the CL header directly before it's dropped.
https://github.com/dotnet/aspnetcore/blob/main/src/Servers/HttpSys/src/RequestProcessing/Request.cs#L36

Can you explain it specifically? Should I check the CL header in the constructor and remove it instead of removing it on the ContentLegnth's get accessor?

@Tratcher
Copy link
Member

The code you changed isn't run until someone accesses the Request.Body, right? You can test that in the debugger by setting a break point. That would allow an application to read the invalid Content-Length header text from the collection before your code runs to modify it. So we need to find a way to make that modification code run earlier, like calling it from the Request constructor.

@FatTigerWang
Copy link
Contributor Author

I will add unit tests later.

@FatTigerWang
Copy link
Contributor Author

FatTigerWang commented Sep 27, 2022

I found this test to be invalid:
https://github.com/dotnet/aspnetcore/blob/main/src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs#L58
It will pass the test even if the assert fails because the exception is caught and there is no assert response. I will create a new issue later and submit a related PR.

@FatTigerWang
Copy link
Contributor Author

I want to add a unit test here like this method, but it always gets skipped.

@Tratcher
Copy link
Member

Tratcher commented Oct 4, 2022

I want to add a unit test here like this method, but it always gets skipped.

Skipped? Note those tests are set up to run in several different projects and to skip if some of the dependencies aren't met. E.g. one project runs it for IIS, but only if installed and you're running as admin. The IIS Express test project should always run these on Windows.

@FatTigerWang
Copy link
Contributor Author

The unit tests were skipped because I didn't install ASP.NET Hosting bundle, and now the unit tests are working fine after the installation is complete.

@Tratcher Tratcher merged commit 3d54b42 into dotnet:main Oct 5, 2022
@ghost ghost added this to the 8.0-preview1 milestone Oct 5, 2022
@Tratcher
Copy link
Member

Tratcher commented Oct 5, 2022

Thanks

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Content-Length and Transfer-Encoding conflicts

3 participants