From 9220cdc285db3ac68cdef58d8beb15a0171c54c9 Mon Sep 17 00:00:00 2001 From: Aliaksandr Kalenik Date: Thu, 23 Mar 2023 02:52:06 +0300 Subject: [PATCH] LibHTTP+WebDriver+WebServer: Return error from HTTP request parser --- Meta/Lagom/Fuzzers/FuzzHttpRequest.cpp | 2 +- Userland/Libraries/LibHTTP/HttpRequest.cpp | 13 ++++++----- Userland/Libraries/LibHTTP/HttpRequest.h | 22 ++++++++++++++++++- .../Libraries/LibWeb/WebDriver/Client.cpp | 7 +++--- Userland/Libraries/LibWeb/WebDriver/Client.h | 2 +- Userland/Services/WebServer/Client.cpp | 2 +- 6 files changed, 35 insertions(+), 13 deletions(-) diff --git a/Meta/Lagom/Fuzzers/FuzzHttpRequest.cpp b/Meta/Lagom/Fuzzers/FuzzHttpRequest.cpp index 5800767379..6f85a08131 100644 --- a/Meta/Lagom/Fuzzers/FuzzHttpRequest.cpp +++ b/Meta/Lagom/Fuzzers/FuzzHttpRequest.cpp @@ -11,7 +11,7 @@ extern "C" int LLVMFuzzerTestOneInput(uint8_t const* data, size_t size) { auto request_wrapper = HTTP::HttpRequest::from_raw_request(ReadonlyBytes { data, size }); - if (!request_wrapper.has_value()) + if (!request_wrapper.is_error()) return 0; auto& request = request_wrapper.value(); diff --git a/Userland/Libraries/LibHTTP/HttpRequest.cpp b/Userland/Libraries/LibHTTP/HttpRequest.cpp index b213cf9bc6..4b16366c75 100644 --- a/Userland/Libraries/LibHTTP/HttpRequest.cpp +++ b/Userland/Libraries/LibHTTP/HttpRequest.cpp @@ -75,7 +75,7 @@ ErrorOr HttpRequest::to_raw_request() const return builder.to_byte_buffer(); } -Optional HttpRequest::from_raw_request(ReadonlyBytes raw_request) +ErrorOr HttpRequest::from_raw_request(ReadonlyBytes raw_request) { enum class State { InMethod, @@ -118,7 +118,7 @@ Optional HttpRequest::from_raw_request(ReadonlyBytes raw_request) while (index < raw_request.size()) { // FIXME: Figure out what the appropriate limitations should be. if (buffer.size() > 65536) - return {}; + return ParseError::RequestTooLarge; switch (state) { case State::InMethod: if (peek() == ' ') { @@ -178,9 +178,10 @@ Optional HttpRequest::from_raw_request(ReadonlyBytes raw_request) if (index == raw_request.size()) { // End of data, so store the body auto maybe_body = ByteBuffer::copy(buffer); - // FIXME: Propagate this error somehow. - if (maybe_body.is_error()) - return {}; + if (maybe_body.is_error()) { + VERIFY(maybe_body.error().code() == ENOMEM); + return ParseError::OutOfMemory; + } body = maybe_body.release_value(); buffer.clear(); } @@ -208,7 +209,7 @@ Optional HttpRequest::from_raw_request(ReadonlyBytes raw_request) else if (method == "PUT") request.set_method(HTTP::HttpRequest::Method::PUT); else - return {}; + return ParseError::UnsupportedMethod; request.m_headers = move(headers); auto url_parts = resource.split_limit('?', 2, SplitBehavior::KeepEmpty); diff --git a/Userland/Libraries/LibHTTP/HttpRequest.h b/Userland/Libraries/LibHTTP/HttpRequest.h index effa66b852..29b648cdc8 100644 --- a/Userland/Libraries/LibHTTP/HttpRequest.h +++ b/Userland/Libraries/LibHTTP/HttpRequest.h @@ -18,6 +18,26 @@ namespace HTTP { class HttpRequest { public: + enum class ParseError { + RequestTooLarge, + OutOfMemory, + UnsupportedMethod + }; + + static StringView parse_error_to_string(ParseError error) + { + switch (error) { + case ParseError::RequestTooLarge: + return "Request too large"sv; + case ParseError::OutOfMemory: + return "Out of memory"sv; + case ParseError::UnsupportedMethod: + return "Unsupported method"sv; + default: + VERIFY_NOT_REACHED(); + } + } + enum Method { Invalid, HEAD, @@ -61,7 +81,7 @@ public: void set_headers(HashMap const&); - static Optional from_raw_request(ReadonlyBytes); + static ErrorOr from_raw_request(ReadonlyBytes); static Optional
get_http_basic_authentication_header(URL const&); static Optional parse_http_basic_authentication_header(DeprecatedString const&); diff --git a/Userland/Libraries/LibWeb/WebDriver/Client.cpp b/Userland/Libraries/LibWeb/WebDriver/Client.cpp index 1db5df5254..3724c77606 100644 --- a/Userland/Libraries/LibWeb/WebDriver/Client.cpp +++ b/Userland/Libraries/LibWeb/WebDriver/Client.cpp @@ -182,6 +182,9 @@ Client::Client(NonnullOwnPtr socket, Core::Object* pare [](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)); + }, [this](WebDriver::Error const& error) { if (send_error_response(error).is_error()) warnln("Could not send error response"); @@ -221,9 +224,7 @@ ErrorOr Client::on_ready_to_read() break; } - m_request = HTTP::HttpRequest::from_raw_request(TRY(builder.to_byte_buffer())); - if (!m_request.has_value()) - return {}; + m_request = TRY(HTTP::HttpRequest::from_raw_request(TRY(builder.to_byte_buffer()))); auto body = TRY(read_body_as_json()); TRY(handle_request(move(body))); diff --git a/Userland/Libraries/LibWeb/WebDriver/Client.h b/Userland/Libraries/LibWeb/WebDriver/Client.h index 10a2b950ea..f6c1f07225 100644 --- a/Userland/Libraries/LibWeb/WebDriver/Client.h +++ b/Userland/Libraries/LibWeb/WebDriver/Client.h @@ -109,7 +109,7 @@ protected: Client(NonnullOwnPtr, Core::Object* parent); private: - using WrappedError = Variant; + using WrappedError = Variant; void die(); ErrorOr on_ready_to_read(); diff --git a/Userland/Services/WebServer/Client.cpp b/Userland/Services/WebServer/Client.cpp index c91d8a0b40..f6cc8a408d 100644 --- a/Userland/Services/WebServer/Client.cpp +++ b/Userland/Services/WebServer/Client.cpp @@ -97,7 +97,7 @@ void Client::start() ErrorOr Client::handle_request(ReadonlyBytes raw_request) { auto request_or_error = HTTP::HttpRequest::from_raw_request(raw_request); - if (!request_or_error.has_value()) + if (request_or_error.is_error()) return false; auto& request = request_or_error.value(); auto resource_decoded = URL::percent_decode(request.resource());