Handle non-HTTP/1.1 requests more gracefully in net::HttpServer.

net/server/http_server_unittest.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/server/http_server.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 32 matching lines @@
 #include "net/test/gtest_util.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 using net::test::IsOk;
+using testing::_;
 
 namespace net {
 
 namespace {
 
 const int kMaxExpectedResponseLength = 2048;
 
 void SetTimedOutAndQuitLoop(const base::WeakPtr<bool> timed_out,
                             const base::Closure& quit_loop_func) {
   if (timed_out) {
@@ skipping 113 matching lines @@
         HttpUtil::AssembleRawHeaders(response.data(), end_of_headers)));
     return body_size >= headers->GetContentLength();
   }
 
   scoped_refptr<IOBufferWithSize> read_buffer_;
   scoped_refptr<DrainableIOBuffer> write_buffer_;
   std::unique_ptr<TCPClientSocket> socket_;
   int connect_result_;
 };
 
+class MockHttpServerDelegate : public HttpServer::Delegate {
+ public:
+  MOCK_METHOD1(OnConnect, void(int connection_id));
+  MOCK_METHOD2(OnHttpRequest,
+               void(int connection_id, const HttpServerRequestInfo& info));
+  MOCK_METHOD2(OnWebSocketRequest,
+               void(int connection_id, const HttpServerRequestInfo& info));
+  MOCK_METHOD2(OnWebSocketMessage,
+               void(int connection_id, const std::string& data));
+  MOCK_METHOD1(OnClose, void(int connection_id));
+};

[mmenke, 2016/09/12 20:35:04]

We try not to use Gmock in net - it makes tests significantly harder to follow,
because of all the baked-in magic, and a lot of committers aren't familiar with
it.

Can we just do something like HttpServerTest, and make sure the request fails
with ERR_CONNECTION_CLOSED, instead?  Think that's a better test of the API
exposed to consumers of the server.

[slan, 2016/09/14 15:50:57]

OK, I removed the use of GMock and added checks for two things:

1. Proper delegate use - I added some very simple state to assert that the
delegate methods were called in the right order. We expect to receive events
when the connection is opened, and when it is closed upon seeing the bad
protocol.

2. That the server actually hangs up - I added checks on the socket used by the
client to test that the server actually disconnects when it parses the bad
header.

(I tried to modify the test to use URLFetcher, but I couldn't find where to
override the HTTP version that's used for the request. So I went lower-level - I
think sending bytes over the socket is a little clearer anyway.)

PTAL at the additions and LMK what you think, Matt.

+
 }  // namespace
 
 class HttpServerTest : public testing::Test,
                        public HttpServer::Delegate {
  public:
   HttpServerTest() : quit_after_request_count_(0) {}
 
   void SetUp() override {
     std::unique_ptr<ServerSocket> server_socket(
         new TCPServerSocket(NULL, NetLog::Source()));
     server_socket->ListenWithAddressAndPort("127.0.0.1", 0, 1);
     server_.reset(new HttpServer(std::move(server_socket), this));
     ASSERT_THAT(server_->GetLocalAddress(&server_address_), IsOk());
   }
 
-  void OnConnect(int connection_id) override {}
+  void OnConnect(int connection_id) override {
+    mock_delegate_.OnConnect(connection_id);
+  }
 
   void OnHttpRequest(int connection_id,
                      const HttpServerRequestInfo& info) override {
     requests_.push_back(std::make_pair(info, connection_id));
     if (requests_.size() == quit_after_request_count_)
       run_loop_quit_func_.Run();
+    mock_delegate_.OnHttpRequest(connection_id, info);
   }
 
   void OnWebSocketRequest(int connection_id,
                           const HttpServerRequestInfo& info) override {
     NOTREACHED();
   }
 
   void OnWebSocketMessage(int connection_id, const std::string& data) override {
     NOTREACHED();
   }
 
-  void OnClose(int connection_id) override {}
+  void OnClose(int connection_id) override {
+    mock_delegate_.OnClose(connection_id);
+  }
 
   bool RunUntilRequestsReceived(size_t count) {
     quit_after_request_count_ = count;
     if (requests_.size() == count)
       return true;
 
     base::RunLoop run_loop;
     run_loop_quit_func_ = run_loop.QuitClosure();
     bool success = RunLoopWithTimeout(&run_loop);
     run_loop_quit_func_.Reset();
@@ skipping 11 matching lines @@
   void HandleAcceptResult(std::unique_ptr<StreamSocket> socket) {
     server_->accepted_socket_.reset(socket.release());
     server_->HandleAcceptResult(OK);
   }
 
  protected:
   std::unique_ptr<HttpServer> server_;
   IPEndPoint server_address_;
   base::Closure run_loop_quit_func_;
   std::vector<std::pair<HttpServerRequestInfo, int> > requests_;
+  testing::NiceMock<MockHttpServerDelegate> mock_delegate_;
 
  private:
   size_t quit_after_request_count_;
 };
 
 namespace {
 
 class WebSocketTest : public HttpServerTest {
   void OnHttpRequest(int connection_id,
                      const HttpServerRequestInfo& info) override {
@@ skipping 204 matching lines @@
   server_->SendRaw(GetConnectionId(0), "Raw Data ");
   server_->SendRaw(GetConnectionId(0), "More Data");
   server_->SendRaw(GetConnectionId(0), "Third Piece of Data");
 
   const std::string expected_response("Raw Data More DataThird Piece of Data");
   std::string response;
   ASSERT_TRUE(client.Read(&response, expected_response.length()));
   ASSERT_EQ(expected_response, response);
 }
 
+TEST_F(HttpServerTest, SendWrongProtocolRequest) {
+  EXPECT_CALL(mock_delegate_, OnConnect(_)).Times(1);
+  EXPECT_CALL(mock_delegate_, OnClose(_)).Times(1);
+
+  // Tests that non-HTTP/1.1 requests are simply ignored.
+  TestHttpClient client;
+  ASSERT_THAT(client.ConnectAndWait(server_address_), IsOk());
+
+  client.Send("GET /test HTTP/1.0\r\n\r\n");
+  ASSERT_FALSE(RunUntilRequestsReceived(1));

[mmenke, 2016/09/12 20:35:04]

Maybe also a test with "GET /test foo\r\n\r\n" and "GET / test\r\n\r\n".  Fine
to just have an array of test cases and loop through them, since they should all
fail the same way.

[slan, 2016/09/14 15:50:57]

Done.

+}
+
 class MockStreamSocket : public StreamSocket {
  public:
   MockStreamSocket()
       : connected_(true),
         read_buf_(NULL),
         read_buf_len_(0) {}
 
   // StreamSocket
   int Connect(const CompletionCallback& callback) override {
     return ERR_NOT_IMPLEMENTED;
@@ skipping 168 matching lines @@
   ASSERT_THAT(client.ConnectAndWait(server_address_), IsOk());
   client.Send("GET / HTTP/1.1\r\n\r\n");
   ASSERT_FALSE(RunUntilRequestsReceived(1));
   ASSERT_EQ(1ul, connection_ids_.size());
   ASSERT_EQ(0ul, requests_.size());
 }
 
 }  // namespace
 
 }  // namespace net