Handling Invalid http response

[kapilash]

This PR introduces changes in CurlHandler so that its behavior matches that of WinHttpHandler vis-a-vis invalid status line or invalid http headers in the response.

Fix for #3269

[kapilash]

cc @stephentoub @vijaykota

[kapilash]

Negative cases are tested manually via a simple socket program

[stephentoub]

Nit: period missing at the end of the sentence.

[kapilash]

Am leaving this for now, if you do not mind. Am using the error message in the validation and the exception message in the windows version does not contain the period.
If there is any change in the tests, I will add the period here.

[kapilash]

Fixed now that we do not need this message.

[stephentoub]

Negative cases are tested manually via a simple socket program

Why manually? Why not have some socket-based functional tests?

[davidsh]

Why manually? Why not have some socket-based functional tests?

cc: @CIPop

My opinion here:

My recommendation is that this library should have UNIT-TESTS. I.e. the kind of tests that System.Net.Http and a few other System.Net libraries have. UNIT-TESTS don't need functional dependencies such as a separate socket program or http server. They mock everything they need. They simply test the specific code itself. We factor our source code in a way that is testable from a unit test perspective.

Regardless, testing this functionality needs to be "automatic" and part of some XUnit test that can run in CI.

[stephentoub]

I wasn't trying to draw a distinction between the kind of test, other than manual vs automatic (the "unit" vs "functional" naming distinction the networking tests make isn't one made in most of the rest of the repo). As long as it's part of the repo and running as part of all PRs / builds / etc., I'm happy.

[vijaykota]

Nit: Use CreateHttpRequestException

[kapilash]

Using a new HttpRequestException instead of CreateHttpRequestException to maintain parity with WinHttpHandler w.r.t the message of the exception thrown.

[vijaykota]

Sorry @kapilash, I didn't realize that CreateHttp.. uses a generic message. Change looks fine to me

[kapilash]

Added a few Unit tests and some functional tests.
I could not introduce the dependency on System.Net.Sockets in the right way.
Introducing "System.Net.Sockets" in project.json is breaking the build. (System.Net.Security etc start failing).
I can eliminate build failure by removing "-*" constraints on the version, and instead rely on some hard-coded build numbers for a platform, But I could not get any build numbers that is common across windows and unix.

So , for now, I am referring to the csproj, till the project.json isssue is resolved.
If you prefer, I can remove the function test for now and just rely on unit tests.

[davidsh]

So , for now, I am referring to the csproj, till the project.json isssue is resolved.
If you prefer, I can remove the function test for now and just rely on unit tests.

I'm not comfortable with adding these functional tests as-is using a localhost sockets server in the tests themselves. And the inclusion of the cross-project reference with Sockets (to workaround a project.json) issue is somthing we should avoid.

So, please exclude the functionaltests for now.

[stephentoub]

I'm not comfortable with adding these functional tests as-is using a localhost sockets server in the tests

Is it the notion of a localhost sockets server that makes you uncomfortable, or this specific implementation of one? If the former, how does this differ from what we do in the sockets functional tests? If the latter, what would you like to see changed (besides the project.json)?

I'm ok going ahead with the PR without tests for now, but I'd like to get some covering this soon.

[davidsh]

@kapilash I like the unit tests you have. The only thing we might do later is to consider to split up the unit tests into multiple test projects based on platform. The inclusion of these additional unit tests is the first time we're blending platform-agnostic Http unit tests with platform-specific tests. Also, the inclusion of the CurlResponseParseUtils.cs file into the platform-agnostic Http src library is also something we tend to avoid. We might end up later refactoring this set of methods into something more generally named.

[davidsh]

Is it the notion of a localhost sockets server that makes you uncomfortable, or this specific implementation of one? If the former, how does this differ from what we do in the sockets functional tests? If the latter, what would you like to see changed (besides the project.json)?

Both.

As we've seen from the Socket tests, the use of the localhost server running on the CI machine has been problematic.

I understand that we want to have some test coverage here. So, perhaps we can let this come in as-is. But I plan to study this and propose some changes.

[stephentoub]

Thanks. I'm happy for us to do whatever you think is the right thing here... mostly I was trying to understand what was driving your concerns.

[davidsh]

This test class should be named consistently against the product class it is testing.

The product class is CurlResponseParseUtils. So, this test class should be named CurlResponseParseUtilsTest and the filename should be CurlResponseParseUtilsTest.cs

[kapilash]

The inclusion of these additional unit tests is the first time we're blending platform-agnostic Http unit tests with platform-specific tests. Also, the inclusion of the CurlResponseParseUtils.cs file into the platform-agnostic Http src library is also something we tend to avoid.

I agree with the need for refactoring, @davidsh. This test is sticking out ( "SoreThumb.cs" could be a valid name for this file). I will be happy to do anything you suggest on having a clean separation.

So, perhaps we can let this come in as-is.

Sorry If I misunderstood you here but I took it to mean that you are okay to have the new, TcpListener-based functional test.
I will see how to fix the project.json.

[kapilash]

Build failure with (not sure what this means):

 22:17:00 [native_code_format_check_prtest] $ cmd /c call /tmp/hudson2691810257018475300.bat
 22:17:00 FATAL: command execution failed
 22:17:00 java.io.IOException: Cannot run program "cmd" (in directory      "/mnt/j/workspace/dotnet_corefx/native_code_format_check_prtest"): error=2, No such file or directory
 22:17:00   at java.lang.ProcessBuilder.start(ProcessBuilder.java:1047)
 22:17:00   at hudson.Proc$LocalProc.<init>(Proc.java:244)
 22:17:00   at hudson.Proc$LocalProc.<init>(Proc.java:216)
 22:17:00   at hudson.Launcher$LocalLauncher.launch(Launcher.java:816)
 22:17:00   at hudson.Launcher$ProcStarter.start(Launcher.java:382)  
 22:17:00   at hudson.Launcher$RemoteLaunchCallable.call(Launcher.java:1149)
 22:17:00   at hudson.Launcher$RemoteLaunchCallable.call(Launcher.java:1114)

[vijaykota]

If responseHeader[responseHeader.Length - 1] == ':", index would be responseHeader.Length at this point

[kapilash]

This is a bug. This works in the cases that we have because of the space after the colon. But the space is optional and is handled via the Trim. I should not increment the index here.
Thank you

[vijaykota]

I was also referring to the increment in ReadHeaderName itself. You need a Check there that there is more of the responseHeader left. Also if all remaining chars are spaces, headerValue will get set to string.Empty - I am assuming that is ok

[kapilash]

I was also referring to the increment in ReadHeaderName itself.

Are you referring to index++;?

That is correct and safe; it reaches that line only when index < responseHeader.Length.
After the increment, index can at the most be equal to responseHeader.Length. And x.Substring(x.Length) returns empty string and does not throw.

if all remaining chars are spaces, headerValue will get set to string.Empty - I am assuming that is ok

Yes. empty header values are allowed, from HTTP point of view.

If you prefer, I can add a Unit test around this area. please let me know.

[vijaykota]

Sorry, I assumed that Substring(index) would fail if out-of-bounds. Since that is handling it, the code would still work. Thanks for adding the test case

[kapilash]

A couple of points about the newly introduced Functional Test (the test that is TcpListener based):

The test can only catch "false negatives" (the cases where response received from the server is invalid but CurlHandler treats it as valid response). And such cases are possible only when curlHandler is interacting with a non-compliant server. IMHO, not a realistic scenario.

At the same time it is not quite a fuzz tester to validate against all sorts of negative erroneous responses.

On the other hand, it adds non-trivial cost in terms of dependencies and potential flakiness.

It seems to me to that cost and benefit do not match in having the TcpListener test a part of check-in tests.

[davidsh]

On the other hand, it adds non-trivial cost in terms of dependencies and potential flakiness.
It seems to me to that cost and benefit do not match in having the TcpListener test a part of check-in tests.

Let's remove the TCP listener + functional tests from this PR until we come up with a cleaner, more stable solution. We should be able to get a sufficient test coverage using unit tests only because these changes are really just about parsing and really don't need real network i/o.

[kapilash]

Let's remove the TCP listener + functional tests from this PR until we come up with a cleaner, more stable solution.

Sure, thanks. Did that.

[davidsh]

LGTM. Please squash these commits before merge.

[kapilash]

@dotnet-bot test this please

[vijaykota]

LGTM

[kapilash]

@dotnet-bot test this please