From cce5e3158fa9923d534c0996527c372bf955e875 Mon Sep 17 00:00:00 2001
From: Sam Atkins <atkinssj@serenityos.org>
Date: Mon, 27 Mar 2023 14:35:31 +0100
Subject: [PATCH] WebServer: Handle incomplete HTTP requests

Mostly by copying the code in LibWeb/WebDriver/Client.cpp
---
 Userland/Services/WebServer/Client.cpp | 95 +++++++++++++-------------
 Userland/Services/WebServer/Client.h   |  6 +-
 2 files changed, 52 insertions(+), 49 deletions(-)

diff --git a/Userland/Services/WebServer/Client.cpp b/Userland/Services/WebServer/Client.cpp
index f6cc8a408d..f3366a09e7 100644
--- a/Userland/Services/WebServer/Client.cpp
+++ b/Userland/Services/WebServer/Client.cpp
@@ -45,61 +45,60 @@ void Client::die()
 void Client::start()
 {
     m_socket->on_ready_to_read = [this] {
-        StringBuilder builder;
+        if (auto result = on_ready_to_read(); result.is_error()) {
+            result.error().visit(
+                [](AK::Error const& error) {
+                    warnln("Internal error: {}", error);
+                },
+                [](HTTP::HttpRequest::ParseError const& error) {
+                    warnln("HTTP request parsing error: {}", HTTP::HttpRequest::parse_error_to_string(error));
+                });
 
-        auto maybe_buffer = ByteBuffer::create_uninitialized(m_socket->buffer_size());
-        if (maybe_buffer.is_error()) {
-            warnln("Could not create buffer for client: {}", maybe_buffer.error());
             die();
-            return;
         }
-
-        auto buffer = maybe_buffer.release_value();
-        for (;;) {
-            auto maybe_can_read = m_socket->can_read_without_blocking();
-            if (maybe_can_read.is_error()) {
-                warnln("Failed to get the blocking status for the socket: {}", maybe_can_read.error());
-                die();
-                return;
-            }
-
-            if (!maybe_can_read.value())
-                break;
-
-            auto maybe_bytes_read = m_socket->read_until_any_of(buffer, Array { "\r"sv, "\n"sv, "\r\n"sv });
-            if (maybe_bytes_read.is_error()) {
-                warnln("Failed to read a line from the request: {}", maybe_bytes_read.error());
-                die();
-                return;
-            }
-
-            if (m_socket->is_eof()) {
-                die();
-                break;
-            }
-
-            builder.append(StringView { maybe_bytes_read.value() });
-            builder.append("\r\n"sv);
-        }
-
-        auto request = builder.to_byte_buffer().release_value_but_fixme_should_propagate_errors();
-        dbgln_if(WEBSERVER_DEBUG, "Got raw request: '{}'", DeprecatedString::copy(request));
-
-        auto maybe_did_handle = handle_request(request);
-        if (maybe_did_handle.is_error()) {
-            warnln("Failed to handle the request: {}", maybe_did_handle.error());
-        }
-
-        die();
     };
 }
 
-ErrorOr<bool> Client::handle_request(ReadonlyBytes raw_request)
+ErrorOr<void, Client::WrappedError> Client::on_ready_to_read()
+{
+    // FIXME: Mostly copied from LibWeb/WebDriver/Client.cpp. As noted there, this should be move the LibHTTP and made spec compliant.
+    auto buffer = TRY(ByteBuffer::create_uninitialized(m_socket->buffer_size()));
+
+    for (;;) {
+        if (!TRY(m_socket->can_read_without_blocking()))
+            break;
+
+        auto data = TRY(m_socket->read_some(buffer));
+        TRY(m_remaining_request.try_append(StringView { data }));
+
+        if (m_socket->is_eof())
+            break;
+    }
+
+    if (m_remaining_request.is_empty())
+        return {};
+
+    auto request = TRY(m_remaining_request.to_byte_buffer());
+    dbgln_if(WEBSERVER_DEBUG, "Got raw request: '{}'", DeprecatedString::copy(request));
+
+    auto maybe_parsed_request = HTTP::HttpRequest::from_raw_request(TRY(m_remaining_request.to_byte_buffer()));
+    if (maybe_parsed_request.is_error()) {
+        if (maybe_parsed_request.error() == HTTP::HttpRequest::ParseError::RequestIncomplete) {
+            // If request is not complete we need to wait for more data to arrive
+            return {};
+        }
+        return maybe_parsed_request.error();
+    }
+
+    m_remaining_request.clear();
+
+    TRY(handle_request(maybe_parsed_request.value()));
+
+    return {};
+}
+
+ErrorOr<bool> Client::handle_request(HTTP::HttpRequest const& request)
 {
-    auto request_or_error = HTTP::HttpRequest::from_raw_request(raw_request);
-    if (request_or_error.is_error())
-        return false;
-    auto& request = request_or_error.value();
     auto resource_decoded = URL::percent_decode(request.resource());
 
     if constexpr (WEBSERVER_DEBUG) {
diff --git a/Userland/Services/WebServer/Client.h b/Userland/Services/WebServer/Client.h
index 93365d7332..b9005290e2 100644
--- a/Userland/Services/WebServer/Client.h
+++ b/Userland/Services/WebServer/Client.h
@@ -24,12 +24,15 @@ public:
 private:
     Client(NonnullOwnPtr<Core::BufferedTCPSocket>, Core::Object* parent);
 
+    using WrappedError = Variant<AK::Error, HTTP::HttpRequest::ParseError>;
+
     struct ContentInfo {
         String type;
         size_t length {};
     };
 
-    ErrorOr<bool> handle_request(ReadonlyBytes);
+    ErrorOr<void, WrappedError> on_ready_to_read();
+    ErrorOr<bool> handle_request(HTTP::HttpRequest const&);
     ErrorOr<void> send_response(Stream&, HTTP::HttpRequest const&, ContentInfo);
     ErrorOr<void> send_redirect(StringView redirect, HTTP::HttpRequest const&);
     ErrorOr<void> send_error_response(unsigned code, HTTP::HttpRequest const&, Vector<String> const& headers = {});
@@ -39,6 +42,7 @@ private:
     bool verify_credentials(Vector<HTTP::HttpRequest::Header> const&);
 
     NonnullOwnPtr<Core::BufferedTCPSocket> m_socket;
+    StringBuilder m_remaining_request;
 };
 
 }