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

Add validity checks for HTTP datagrams #85

Merged
merged 8 commits into from Jan 27, 2017

Conversation

Projects
None yet
2 participants
@FlorianHockmann
Contributor

FlorianHockmann commented Jan 9, 2017

Added overloads for the CalculateIsValid method in HttpRequestDatagram and HttpResponseDatagram. This allows to check whether a HTTP datagram is actually a valid datagram before parsing it.

According to the RFCs of HTTP/1.0 and 1.1 a HTTP request must contain at least a method, an URI, and the version. So HttpRequestDatagram.CalculateIsValid() returns true if the parsed method, URI, and version are not null (or empty in case of the URI string).

For responses, the RFCs require at least the version, status code, and a reason phrase. Since the reason phrase can be basically any string I excluded it from the validity check and just check the version and status code. So HttpResponseDatagram.CalculateIsValid() returns true if the parsed version and the status code are not null.

This pull request also includes unit tests for the different cases of missing / present values for the mentioned elements.

FlorianHockmann added some commits Jan 23, 2017

HTTP validity checks don't override CalculateIsValid()
Instead of overriding CalculateIsValid, a new property IsValidStart is introduced for HTTP datagrams - as proposed by Boaz Brickner.
The validity checks only validate the header of an HTTP datagram. Since a datagram can be transmitted in multiple packets (due to TCP segmentation and IP fragmentation), a HTTP datagram can be valid without having a header at all.
Move common behavior up to HttpDatagram
IsValidStart in HttpDatagram now implements the property with the template method pattern. HttpRequestDatagram and HttpResponseDatagram only override CalculateIsValidStart().
Add IsValidStart check to RandomHttpTest
RandomHttpTest now checks for all generated HttpDatagrams that contain a header whether IsValidStart is true.
@FlorianHockmann

This comment has been minimized.

Show comment
Hide comment
@FlorianHockmann

FlorianHockmann Jan 23, 2017

Contributor

I am not sure if I got the RandomHttpTest right. It now checks that IsValidStart is true for all HttpDatagrams that contain a header. This should be the case at least for all packets generated during this test due to the way the RandomHttpExtensions work.
But I am of course open for any suggestions for a better test.

Contributor

FlorianHockmann commented Jan 23, 2017

I am not sure if I got the RandomHttpTest right. It now checks that IsValidStart is true for all HttpDatagrams that contain a header. This should be the case at least for all packets generated during this test due to the way the RandomHttpExtensions work.
But I am of course open for any suggestions for a better test.

var packet = BuildPacket("HTTP/1.0 OK \r\n\r\n");
Assert.IsFalse(packet.Ethernet.IpV4.Tcp.Http.IsValidStart);
}

This comment has been minimized.

@bricknerb

bricknerb Jan 27, 2017

Contributor

If you only care about IsValidStart in these tests, you could put "IsValidStart" as part of the test names.
Otherwise you could test other properties for these use cases.

@bricknerb

bricknerb Jan 27, 2017

Contributor

If you only care about IsValidStart in these tests, you could put "IsValidStart" as part of the test names.
Otherwise you could test other properties for these use cases.

@bricknerb

Thanks for improving Pcap.Net!
I've added a few final comments you may want to address.

FlorianHockmann added some commits Jan 27, 2017

Rename HTTP IsValidStart tests to be more descriptive
The names of those tests now follow a schema that should be more in line with the other tests in Pcap.Net.
@FlorianHockmann

This comment has been minimized.

Show comment
Hide comment
@FlorianHockmann

FlorianHockmann Jan 27, 2017

Contributor

Thank you for creating such a great library!

I fixed the typo and renamed the tests to follow a common schema that is more descriptive and fits in better with the other tests.

Contributor

FlorianHockmann commented Jan 27, 2017

Thank you for creating such a great library!

I fixed the typo and renamed the tests to follow a common schema that is more descriptive and fits in better with the other tests.

@bricknerb bricknerb merged commit 57a21ac into PcapDotNet:master Jan 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment