From 18abc6c85d921ca0dddb825cefb4c6f910a0d361 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 11 Nov 2022 11:00:45 -0500 Subject: [PATCH] Browser+WebContent+WebDriver: Move Add Cookie to WebContent The new implementation is made a bit simpler by way of TRY. It also implements setting the SameSite attribute. --- .../Browser/WebDriverConnection.cpp | 14 --- .../Browser/WebDriverConnection.h | 1 - .../Browser/WebDriverSessionClient.ipc | 1 - .../Services/WebContent/WebDriverClient.ipc | 1 + .../WebContent/WebDriverConnection.cpp | 83 ++++++++++++ .../Services/WebContent/WebDriverConnection.h | 1 + Userland/Services/WebDriver/Client.cpp | 3 +- Userland/Services/WebDriver/Session.cpp | 118 ------------------ Userland/Services/WebDriver/Session.h | 1 - 9 files changed, 86 insertions(+), 137 deletions(-) diff --git a/Userland/Applications/Browser/WebDriverConnection.cpp b/Userland/Applications/Browser/WebDriverConnection.cpp index 4119f8e3a6..5fc59ad373 100644 --- a/Userland/Applications/Browser/WebDriverConnection.cpp +++ b/Userland/Applications/Browser/WebDriverConnection.cpp @@ -69,20 +69,6 @@ Messages::WebDriverSessionClient::GetAllCookiesResponse WebDriverConnection::get return { {} }; } -void WebDriverConnection::add_cookie(Web::Cookie::ParsedCookie const& cookie) -{ - dbgln_if(WEBDRIVER_DEBUG, "WebDriverConnection: add_cookie {}", cookie.name); - if (auto browser_window = m_browser_window.strong_ref()) { - auto& tab = browser_window->active_tab(); - if (tab.on_set_cookie) { - // FIXME: The spec doesn't say anything about the source - // but can we assume a cookie created through a HTTP-request to the WebDriver - // to be (source) from an HTTP-API? - tab.on_set_cookie(tab.url(), cookie, Web::Cookie::Source::Http); - } - } -} - void WebDriverConnection::update_cookie(Web::Cookie::Cookie const& cookie) { dbgln_if(WEBDRIVER_DEBUG, "WebDriverConnection: update_cookie {}", cookie.name); diff --git a/Userland/Applications/Browser/WebDriverConnection.h b/Userland/Applications/Browser/WebDriverConnection.h index 9da9b4bf41..d6a92e8e83 100644 --- a/Userland/Applications/Browser/WebDriverConnection.h +++ b/Userland/Applications/Browser/WebDriverConnection.h @@ -43,7 +43,6 @@ public: virtual void back() override; virtual void forward() override; virtual Messages::WebDriverSessionClient::GetAllCookiesResponse get_all_cookies() override; - virtual void add_cookie(Web::Cookie::ParsedCookie const&) override; virtual void update_cookie(Web::Cookie::Cookie const&) override; private: diff --git a/Userland/Applications/Browser/WebDriverSessionClient.ipc b/Userland/Applications/Browser/WebDriverSessionClient.ipc index 98103889d2..e7ac4b9a36 100644 --- a/Userland/Applications/Browser/WebDriverSessionClient.ipc +++ b/Userland/Applications/Browser/WebDriverSessionClient.ipc @@ -19,6 +19,5 @@ endpoint WebDriverSessionClient { back() =| forward() =| get_all_cookies() => (Vector cookies) - add_cookie(Web::Cookie::ParsedCookie cookie) =| update_cookie(Web::Cookie::Cookie cookie) =| } diff --git a/Userland/Services/WebContent/WebDriverClient.ipc b/Userland/Services/WebContent/WebDriverClient.ipc index 9fd833e53e..9ff40b94d8 100644 --- a/Userland/Services/WebContent/WebDriverClient.ipc +++ b/Userland/Services/WebContent/WebDriverClient.ipc @@ -26,6 +26,7 @@ endpoint WebDriverClient { execute_async_script(JsonValue payload) => (Web::WebDriver::Response response) get_all_cookies() => (Web::WebDriver::Response response) get_named_cookie(String name) => (Web::WebDriver::Response response) + add_cookie(JsonValue payload) => (Web::WebDriver::Response response) take_screenshot() => (Web::WebDriver::Response response) take_element_screenshot(String element_id) => (Web::WebDriver::Response response) } diff --git a/Userland/Services/WebContent/WebDriverConnection.cpp b/Userland/Services/WebContent/WebDriverConnection.cpp index 81470c73f0..bc850fd910 100644 --- a/Userland/Services/WebContent/WebDriverConnection.cpp +++ b/Userland/Services/WebContent/WebDriverConnection.cpp @@ -188,10 +188,22 @@ static ErrorOr get_property(JsonValue const if (!property->is_string()) return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, String::formatted("Property '{}' is not a String", key)); return property->as_string(); + } else if constexpr (IsSame) { + if (!property->is_bool()) + return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, String::formatted("Property '{}' is not a Boolean", key)); + return property->as_bool(); + } else if constexpr (IsSame) { + if (!property->is_u32()) + return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, String::formatted("Property '{}' is not a Number", key)); + return property->as_u32(); } else if constexpr (IsSame) { if (!property->is_array()) return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, String::formatted("Property '{}' is not an Array", key)); return &property->as_array(); + } else if constexpr (IsSame) { + if (!property->is_object()) + return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, String::formatted("Property '{}' is not an Object", key)); + return &property->as_object(); } else { static_assert(DependentFalse, "get_property invoked with unknown property type"); VERIFY_NOT_REACHED(); @@ -892,6 +904,77 @@ Messages::WebDriverClient::GetNamedCookieResponse WebDriverConnection::get_named return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::NoSuchCookie, String::formatted("Cookie '{}' not found", name)); } +// 14.3 Add Cookie, https://w3c.github.io/webdriver/#dfn-adding-a-cookie +Messages::WebDriverClient::AddCookieResponse WebDriverConnection::add_cookie(JsonValue const& payload) +{ + // 1. Let data be the result of getting a property named cookie from the parameters argument. + auto const& data = *TRY(get_property(payload, "cookie"sv)); + + // 2. If data is not a JSON Object with all the required (non-optional) JSON keys listed in the table for cookie conversion, return error with error code invalid argument. + // NOTE: This validation is performed in subsequent steps. + + // 3. If the current browsing context is no longer open, return error with error code no such window. + TRY(ensure_open_top_level_browsing_context()); + + // FIXME: 4. Handle any user prompts, and return its value if it is an error. + // FIXME: 5. If the current browsing context’s document element is a cookie-averse Document object, return error with error code invalid cookie domain. + + // 6. If cookie name or cookie value is null, cookie domain is not equal to the current browsing context’s active document’s domain, cookie secure only or cookie HTTP only are not boolean types, or cookie expiry time is not an integer type, or it less than 0 or greater than the maximum safe integer, return error with error code invalid argument. + // NOTE: This validation is either performed in subsequent steps, or is performed by the CookieJar (namely domain matching). + + // 7. Create a cookie in the cookie store associated with the active document’s address using cookie name name, cookie value value, and an attribute-value list of the following cookie concepts listed in the table for cookie conversion from data: + Web::Cookie::ParsedCookie cookie {}; + cookie.name = TRY(get_property(data, "name"sv)); + cookie.value = TRY(get_property(data, "value"sv)); + + // Cookie path + // The value if the entry exists, otherwise "/". + if (data.has("path"sv)) + cookie.path = TRY(get_property(data, "path"sv)); + else + cookie.path = "/"; + + // Cookie domain + // The value if the entry exists, otherwise the current browsing context’s active document’s URL domain. + // NOTE: The otherwise case is handled by the CookieJar + if (data.has("domain"sv)) + cookie.domain = TRY(get_property(data, "domain"sv)); + + // Cookie secure only + // The value if the entry exists, otherwise false. + if (data.has("secure"sv)) + cookie.secure_attribute_present = TRY(get_property(data, "secure"sv)); + + // Cookie HTTP only + // The value if the entry exists, otherwise false. + if (data.has("httpOnly"sv)) + cookie.http_only_attribute_present = TRY(get_property(data, "httpOnly"sv)); + + // Cookie expiry time + // The value if the entry exists, otherwise leave unset to indicate that this is a session cookie. + if (data.has("expiry"sv)) { + // NOTE: less than 0 or greater than safe integer are handled by the JSON parser + auto expiry = TRY(get_property(data, "expiry"sv)); + cookie.expiry_time_from_expires_attribute = Core::DateTime::from_timestamp(expiry); + } + + // Cookie same site + // The value if the entry exists, otherwise leave unset to indicate that no same site policy is defined. + if (data.has("sameSite"sv)) { + auto same_site = TRY(get_property(data, "sameSite"sv)); + cookie.same_site_attribute = Web::Cookie::same_site_from_string(same_site); + } + + auto* document = m_page_host.page().top_level_browsing_context().active_document(); + m_web_content_client.async_did_set_cookie(document->url(), cookie, to_underlying(Web::Cookie::Source::Http)); + + // If there is an error during this step, return error with error code unable to set cookie. + // NOTE: This probably should only apply to the actual setting of the cookie in the Browser, which cannot fail in our case. + + // 8. Return success with data null. + return make_success_response({}); +} + // 17.1 Take Screenshot, https://w3c.github.io/webdriver/#take-screenshot Messages::WebDriverClient::TakeScreenshotResponse WebDriverConnection::take_screenshot() { diff --git a/Userland/Services/WebContent/WebDriverConnection.h b/Userland/Services/WebContent/WebDriverConnection.h index 69a38fb765..2e48343a3e 100644 --- a/Userland/Services/WebContent/WebDriverConnection.h +++ b/Userland/Services/WebContent/WebDriverConnection.h @@ -58,6 +58,7 @@ private: virtual Messages::WebDriverClient::ExecuteAsyncScriptResponse execute_async_script(JsonValue const& payload) override; virtual Messages::WebDriverClient::GetAllCookiesResponse get_all_cookies() override; virtual Messages::WebDriverClient::GetNamedCookieResponse get_named_cookie(String const& name) override; + virtual Messages::WebDriverClient::AddCookieResponse add_cookie(JsonValue const& payload) override; virtual Messages::WebDriverClient::TakeScreenshotResponse take_screenshot() override; virtual Messages::WebDriverClient::TakeElementScreenshotResponse take_element_screenshot(String const& element_id) override; diff --git a/Userland/Services/WebDriver/Client.cpp b/Userland/Services/WebDriver/Client.cpp index 5ee58593af..c3daca6e0d 100644 --- a/Userland/Services/WebDriver/Client.cpp +++ b/Userland/Services/WebDriver/Client.cpp @@ -771,8 +771,7 @@ Web::WebDriver::Response Client::handle_add_cookie(Vector const& par { dbgln_if(WEBDRIVER_DEBUG, "Handling POST /session//cookie"); auto* session = TRY(find_session_with_id(parameters[0])); - auto result = TRY(session->add_cookie(payload)); - return make_json_value(result); + return session->web_content_connection().add_cookie(payload); } // 14.4 Delete Cookie, https://w3c.github.io/webdriver/#dfn-delete-cookie diff --git a/Userland/Services/WebDriver/Session.cpp b/Userland/Services/WebDriver/Session.cpp index 16aa2f94ea..efd9b43afc 100644 --- a/Userland/Services/WebDriver/Session.cpp +++ b/Userland/Services/WebDriver/Session.cpp @@ -297,124 +297,6 @@ Web::WebDriver::Response Session::get_window_handles() const return JsonValue { handles }; } -// 14.3 Add Cookie, https://w3c.github.io/webdriver/#dfn-adding-a-cookie -Web::WebDriver::Response Session::add_cookie(JsonValue const& payload) -{ - // 1. Let data be the result of getting a property named cookie from the parameters argument. - if (!payload.is_object() || !payload.as_object().has_object("cookie"sv)) - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Payload doesn't have a cookie object"); - - auto const& maybe_data = payload.as_object().get("cookie"sv); - - // 2. If data is not a JSON Object with all the required (non-optional) JSON keys listed in the table for cookie conversion, - // return error with error code invalid argument. - // NOTE: Table is here: https://w3c.github.io/webdriver/#dfn-table-for-cookie-conversion - if (!maybe_data.is_object()) - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Value \"cookie\' is not an object"); - - auto const& data = maybe_data.as_object(); - - if (!data.has("name"sv) || !data.has("value"sv)) - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Cookie-Object doesn't contain all required keys"); - - // 3. If the current browsing context is no longer open, return error with error code no such window. - TRY(check_for_open_top_level_browsing_context_or_return_error()); - - // FIXME: 4. Handle any user prompts, and return its value if it is an error. - - // FIXME: 5. If the current browsing context’s document element is a cookie-averse Document object, - // return error with error code invalid cookie domain. - - // 6. If cookie name or cookie value is null, - // FIXME: cookie domain is not equal to the current browsing context’s active document’s domain, - // cookie secure only or cookie HTTP only are not boolean types, - // or cookie expiry time is not an integer type, or it less than 0 or greater than the maximum safe integer, - // return error with error code invalid argument. - if (data.get("name"sv).is_null() || data.get("value"sv).is_null()) - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Cookie-Object is malformed: name or value are null"); - if (data.has("secure"sv) && !data.get("secure"sv).is_bool()) - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Cookie-Object is malformed: secure is not bool"); - if (data.has("httpOnly"sv) && !data.get("httpOnly"sv).is_bool()) - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Cookie-Object is malformed: httpOnly is not bool"); - Optional expiry_time; - if (data.has("expiry"sv)) { - auto expiry_argument = data.get("expiry"sv); - if (!expiry_argument.is_u32()) { - // NOTE: less than 0 or greater than safe integer are handled by the JSON parser - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Cookie-Object is malformed: expiry is not u32"); - } - expiry_time = Core::DateTime::from_timestamp(expiry_argument.as_u32()); - } - - // 7. Create a cookie in the cookie store associated with the active document’s address using - // cookie name name, cookie value value, and an attribute-value list of the following cookie concepts - // listed in the table for cookie conversion from data: - Web::Cookie::ParsedCookie cookie; - if (auto name_attribute = data.get("name"sv); name_attribute.is_string()) - cookie.name = name_attribute.as_string(); - else - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Expect name attribute to be string"); - - if (auto value_attribute = data.get("value"sv); value_attribute.is_string()) - cookie.value = value_attribute.as_string(); - else - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Expect value attribute to be string"); - - // Cookie path - // The value if the entry exists, otherwise "/". - if (data.has("path"sv)) { - if (auto path_attribute = data.get("path"sv); path_attribute.is_string()) - cookie.path = path_attribute.as_string(); - else - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Expect path attribute to be string"); - } else { - cookie.path = "/"; - } - - // Cookie domain - // The value if the entry exists, otherwise the current browsing context’s active document’s URL domain. - // NOTE: The otherwise case is handled by the CookieJar - if (data.has("domain"sv)) { - if (auto domain_attribute = data.get("domain"sv); domain_attribute.is_string()) - cookie.domain = domain_attribute.as_string(); - else - return Web::WebDriver::Error::from_code(Web::WebDriver::ErrorCode::InvalidArgument, "Expect domain attribute to be string"); - } - - // Cookie secure only - // The value if the entry exists, otherwise false. - if (data.has("secure"sv)) { - cookie.secure_attribute_present = data.get("secure"sv).as_bool(); - } else { - cookie.secure_attribute_present = false; - } - - // Cookie HTTP only - // The value if the entry exists, otherwise false. - if (data.has("httpOnly"sv)) { - cookie.http_only_attribute_present = data.get("httpOnly"sv).as_bool(); - } else { - cookie.http_only_attribute_present = false; - } - - // Cookie expiry time - // The value if the entry exists, otherwise leave unset to indicate that this is a session cookie. - cookie.expiry_time_from_expires_attribute = expiry_time; - - // FIXME: Cookie same site - // The value if the entry exists, otherwise leave unset to indicate that no same site policy is defined. - - m_browser_connection->async_add_cookie(move(cookie)); - - // If there is an error during this step, return error with error code unable to set cookie. - // NOTE: This probably should only apply to the actual setting of the cookie in the Browser, - // which cannot fail in our case. - // Thus, the error-codes used above are 400 "invalid argument". - - // 8. Return success with data null. - return JsonValue(); -} - // https://w3c.github.io/webdriver/#dfn-delete-cookies void Session::delete_cookies(Optional const& name) { diff --git a/Userland/Services/WebDriver/Session.h b/Userland/Services/WebDriver/Session.h index c60cc8b654..0f3ee28f07 100644 --- a/Userland/Services/WebDriver/Session.h +++ b/Userland/Services/WebDriver/Session.h @@ -58,7 +58,6 @@ public: Web::WebDriver::Response get_window_handle(); ErrorOr> close_window(); Web::WebDriver::Response get_window_handles() const; - Web::WebDriver::Response add_cookie(JsonValue const& payload); Web::WebDriver::Response delete_cookie(StringView name); Web::WebDriver::Response delete_all_cookies(); Web::WebDriver::Response take_element_screenshot(StringView element_id);