Web Inspector: Do not try to parse incomplete HTTP requests

https://bugs.webkit.org/show_bug.cgi?id=121123

Patch by Andre Moreira Magalhaes <andre.magalhaes@collabora.co.uk> on 2013-09-12
Reviewed by Darin Adler.

When working on a patch for bug #121121 I found an issue with the InspectorServer where it would try
to parse an HTTP message before receiving the full message and thus fail connecting with the
chromedevtools plugin.

What happens is that the WebSocketServerConnection receives buffers on
WebSocketServerConnection::didReceiveSocketStreamData and calls
WebSocketServerConnection::readHTTPMessage which then checks if we have a valid request by calling
HTTPRequest::parseHTTPRequestFromBuffer. If the request is valid it tries to parse the message and
clears the buffer, otherwise it continues adding data to the internal buffer until we have a valid
request.

The problem is that currently HTTPRequest::parseHTTPRequestFromBuffer considers the request as valid
before receiving the full message. To solve this we should make the method check if the request
headers end with a blank line otherwise we consider the request as invalid (see also
http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html).

* UIProcess/API/gtk/tests/TestInspectorServer.cpp:
(sendIncompleteRequest):
(beforeAll):
Add GTK specific test to check if the inspector server replies to incomplete requests.
* UIProcess/InspectorServer/HTTPRequest.cpp:
(WebKit::HTTPRequest::parseHeaders):
Do not consider request valid if headers didn't end with a blank line.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@155642 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog
index 9719ae67e2a8b0e1e61528f7eb30d6868e48e5a7..77518dfcb1a0e5fed07e176c162a3c27c4ce2f7f 100644 (file)
--- a/Source/WebKit2/ChangeLog
+++ b/Source/WebKit2/ChangeLog
@@ -1,3 +1,34 @@
+2013-09-12  Andre Moreira Magalhaes   <andre.magalhaes@collabora.co.uk>
+
+        Web Inspector: Do not try to parse incomplete HTTP requests
+        https://bugs.webkit.org/show_bug.cgi?id=121123
+
+        Reviewed by Darin Adler.
+
+        When working on a patch for bug #121121 I found an issue with the InspectorServer where it would try
+        to parse an HTTP message before receiving the full message and thus fail connecting with the
+        chromedevtools plugin.
+
+        What happens is that the WebSocketServerConnection receives buffers on
+        WebSocketServerConnection::didReceiveSocketStreamData and calls
+        WebSocketServerConnection::readHTTPMessage which then checks if we have a valid request by calling
+        HTTPRequest::parseHTTPRequestFromBuffer. If the request is valid it tries to parse the message and
+        clears the buffer, otherwise it continues adding data to the internal buffer until we have a valid
+        request.
+
+        The problem is that currently HTTPRequest::parseHTTPRequestFromBuffer considers the request as valid
+        before receiving the full message. To solve this we should make the method check if the request
+        headers end with a blank line otherwise we consider the request as invalid (see also
+        http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html).
+
+        * UIProcess/API/gtk/tests/TestInspectorServer.cpp:
+        (sendIncompleteRequest):
+        (beforeAll):
+        Add GTK specific test to check if the inspector server replies to incomplete requests.
+        * UIProcess/InspectorServer/HTTPRequest.cpp:
+        (WebKit::HTTPRequest::parseHeaders):
+        Do not consider request valid if headers didn't end with a blank line.
+

 2013-09-12  Anders Carlsson  <andersca@apple.com>
 
         SharedBuffer::createNSData should return a RetainPtr<NSData>
 2013-09-12  Anders Carlsson  <andersca@apple.com>
 
         SharedBuffer::createNSData should return a RetainPtr<NSData>

diff --git a/Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp b/Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp
index 35fb3fcdca4e35077042a6f480ce55f611114e58..0b2354f94ee34f467bf9cb080d22bed256e503d3 100644 (file)
--- a/Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp
+++ b/Source/WebKit2/UIProcess/API/gtk/tests/TestInspectorServer.cpp
@@ -240,6 +240,33 @@ static void openRemoteDebuggingSession(InspectorServerTest* test, gconstpointer)
     g_assert_cmpstr(webkit_web_view_get_title(test->m_webView), ==, "Web Inspector - http://127.0.0.1:2999/");
 }
 
     g_assert_cmpstr(webkit_web_view_get_title(test->m_webView), ==, "Web Inspector - http://127.0.0.1:2999/");
 }
 

+static void sendIncompleteRequest(InspectorServerTest* test, gconstpointer)
+{
+    GOwnPtr<GError> error;
+
+    // Connect to the inspector server.
+    GSocketClient* client = g_socket_client_new();
+    GSocketConnection* connection = g_socket_client_connect_to_host(client, "127.0.0.1", 2999, NULL, &error.outPtr());
+    g_assert(!error.get());
+
+    // Send incomplete request (missing blank line after headers) and check if inspector server
+    // replies. The server should not reply to an incomplete request and the test should timeout
+    // on read.
+    GOutputStream* ostream = g_io_stream_get_output_stream(G_IO_STREAM(connection));
+    // Request missing blank line after headers.
+    const gchar* incompleteRequest = "GET /devtools/page/1 HTTP/1.1\r\nHost: Localhost\r\n";
+    g_output_stream_write(ostream, incompleteRequest, strlen(incompleteRequest), NULL, &error.outPtr());
+    g_assert(!error.get());
+
+    GInputStream* istream = g_io_stream_get_input_stream(G_IO_STREAM(connection));
+    char response[16];
+    memset(response, 0, sizeof(response));
+    g_input_stream_read_async(istream, response, sizeof(response) - 1, G_PRIORITY_DEFAULT, NULL, NULL, NULL);
+    // Give a chance for the server to reply.
+    test->wait(2);
+    // If we got any answer it means the server replied to an incomplete request, lets fail.
+    g_assert(String(response).isEmpty());
+}

 
 void beforeAll()
 {
 
 void beforeAll()
 {


@@ -250,6 +277,7 @@ void beforeAll()
     InspectorServerTest::add("WebKitWebInspectorServer", "test-page-list", testInspectorServerPageList);
     InspectorServerTest::add("WebKitWebInspectorServer", "test-remote-debugging-message", testRemoteDebuggingMessage);
     InspectorServerTest::add("WebKitWebInspectorServer", "test-open-debugging-session", openRemoteDebuggingSession);
     InspectorServerTest::add("WebKitWebInspectorServer", "test-page-list", testInspectorServerPageList);
     InspectorServerTest::add("WebKitWebInspectorServer", "test-remote-debugging-message", testRemoteDebuggingMessage);
     InspectorServerTest::add("WebKitWebInspectorServer", "test-open-debugging-session", openRemoteDebuggingSession);

+    InspectorServerTest::add("WebKitWebInspectorServer", "test-incomplete-request", sendIncompleteRequest);

 
 }
 
 
 }
 

diff --git a/Source/WebKit2/UIProcess/InspectorServer/HTTPRequest.cpp b/Source/WebKit2/UIProcess/InspectorServer/HTTPRequest.cpp
index 734926c6c5600dbc56dd482f7c007d12cb249e8b..5d9c4b2ea3cc4d388c34b7c9585c767c92c8de68 100644 (file)
--- a/Source/WebKit2/UIProcess/InspectorServer/HTTPRequest.cpp
+++ b/Source/WebKit2/UIProcess/InspectorServer/HTTPRequest.cpp
@@ -93,6 +93,13 @@ size_t HTTPRequest::parseHeaders(const char* data, size_t length, String& failur
             break;
         m_headerFields.add(name, value);
     }
             break;
         m_headerFields.add(name, value);
     }

+
+    // If we got here and "name" is empty, it means the headers are valid and ended with a
+    // blank line (parseHTTPHeader returns "name" as empty if parsing a blank line), otherwise
+    // the headers didn't end with a blank line and we have an invalid request.
+    // See also http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html
+    if (!name.isEmpty())
+        return 0;

     return p - data;
 }
 
     return p - data;
 }