http: adding another smuggling test#15244
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
antoniovicente
left a comment
There was a problem hiding this comment.
Thanks for the extra tests for smuggling cases.
test/integration/integration_test.cc
Outdated
| { | ||
| std::string response; | ||
| const std::string full_request = "GET / HTTP/1.1\r\nHost: host\r\ncontent-length: " | ||
| "36\r\n\ttransfer-encoding: chunked\r\n\r\n" + |
There was a problem hiding this comment.
Would it be possible to break this request string at \r\n boundaries?
Something like:
"GET / HTTP/1.1\r\n"
"Host: host\r\n"
"content-length: 36\r\n"
"\ttransfer-encoding: chunked\r\n\r\n"
It took me a while to figure out that the difference between this request and the one above is the \t just before transfer-encoding. A comment about this being about \t would be helpful.
Also, consider a test of transfer-encoding with \t before it but no content-length, and \t before content-length and no transfer-encoding header.
test/integration/integration_test.cc
Outdated
| } | ||
| { | ||
| std::string response; | ||
| const std::string full_request = "GET / HTTP/1.1\r\nHost: host\r\ncontent-length: " |
There was a problem hiding this comment.
I think that in order to smuggle a request you need either content-length: 0 and possibly a request body that is chunk encoded prior to the smuggled request. A 400 here could be due to the request not being chunk encoded despite there being a transfer-encoding: chunked header.
| EXPECT_THAT(response, StartsWith("HTTP/1.1 400 Bad Request\r\n")); | ||
| } | ||
| { | ||
| std::string response; |
There was a problem hiding this comment.
Also, do we have smuggling tests that use HTTP methods that can have body; for example POST.
There was a problem hiding this comment.
GET can have body in Envoy :-/
antoniovicente
left a comment
There was a problem hiding this comment.
Thanks for the extra tests.
Making sure T-E + C-L checks handle tab based whitespace.
Risk Level: n/e (test only)
Testing: n/a
Docs Changes: n/a
Release Notes: n/a