From 8ff830920247c432e479514c039e1f0f3c7a1519 Mon Sep 17 00:00:00 2001 From: Timothy Flynn Date: Fri, 26 May 2023 09:51:53 -0400 Subject: [PATCH] LibWeb: Update workarounds for fetching CORS cross-origin responses Now that the processResponseConsumeBody algorithm receives the internal response body of the fetched object, we do not need to go out of our way to read its body from outside of fetch. However, several elements do still need to manually inspect the internal response for other data, such as response headers and status. Note that HTMLScriptElement already does the new workaround as a proper spec step. --- .../LibWeb/HTML/HTMLImageElement.cpp | 25 ++-- .../Libraries/LibWeb/HTML/HTMLLinkElement.cpp | 80 +++---------- .../LibWeb/HTML/HTMLMediaElement.cpp | 12 +- .../LibWeb/HTML/Scripting/Fetching.cpp | 112 ++++++------------ 4 files changed, 65 insertions(+), 164 deletions(-) diff --git a/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp index 363cd05687..1dc0109084 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLImageElement.cpp @@ -492,6 +492,10 @@ after_step_6: Fetch::Infrastructure::FetchAlgorithms::Input fetch_algorithms_input {}; fetch_algorithms_input.process_response = [this, image_request, url_string](JS::NonnullGCPtr response) { + // FIXME: If the response is CORS cross-origin, we must use its internal response to query any of its data. See: + // https://github.com/whatwg/html/issues/9355 + response = response->unsafe_response(); + // 25. As soon as possible, jump to the first applicable entry from the following list: // FIXME: - If the resource type is multipart/x-mixed-replace @@ -500,30 +504,15 @@ after_step_6: // - The next task that is queued by the networking task source while the image is being fetched must run the following steps: queue_an_element_task(HTML::Task::Source::Networking, [this, response, image_request, url_string] { auto process_body = [response, image_request, url_string, this](ByteBuffer data) { - // FIXME: Another instance of the CORS cross-origin response workaround here: - auto response_with_headers = response; - if (response->is_cors_cross_origin() - && response->header_list()->is_empty() - && !response->unsafe_response()->header_list()->is_empty()) { - response_with_headers = response->unsafe_response(); - } - auto extracted_mime_type = response_with_headers->header_list()->extract_mime_type().release_value_but_fixme_should_propagate_errors(); + auto extracted_mime_type = response->header_list()->extract_mime_type().release_value_but_fixme_should_propagate_errors(); auto mime_type = extracted_mime_type.has_value() ? extracted_mime_type.value().essence().bytes_as_string_view() : StringView {}; handle_successful_fetch(url_string, mime_type, image_request, move(data)); }; auto process_body_error = [this](auto) { handle_failed_fetch(); }; - // FIXME: See HTMLLinkElement::default_fetch_and_process_linked_resource for thorough notes on the workaround - // added here for CORS cross-origin responses. The gist is that all cross-origin responses will have a - // null bodyBytes. So we must read the actual body from the unsafe response. - // https://github.com/whatwg/html/issues/9066 - if (response->is_cors_cross_origin() && !response->body().has_value() && response->unsafe_response()->body().has_value()) { - auto unsafe_response = static_cast(*response).internal_response(); - unsafe_response->body()->fully_read(realm(), move(process_body), move(process_body_error), JS::NonnullGCPtr { realm().global_object() }).release_value_but_fixme_should_propagate_errors(); - } else if (response->body().has_value()) { - response->body().value().fully_read(realm(), move(process_body), move(process_body_error), JS::NonnullGCPtr { realm().global_object() }).release_value_but_fixme_should_propagate_errors(); - } + + response->body().value().fully_read(realm(), move(process_body), move(process_body_error), JS::NonnullGCPtr { realm().global_object() }).release_value_but_fixme_should_propagate_errors(); }); }; diff --git a/Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp index db12adb5c6..db7dfe0d35 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLLinkElement.cpp @@ -279,72 +279,30 @@ void HTMLLinkElement::default_fetch_and_process_linked_resource() } // 7. Fetch request with processResponseConsumeBody set to the following steps given response response and null, failure, or a byte sequence bodyBytes: - Fetch::Fetching::fetch( - realm(), *request, - Fetch::Infrastructure::FetchAlgorithms::create(vm(), - { .process_request_body_chunk_length = {}, - .process_request_end_of_body = {}, - .process_early_hints_response = {}, - .process_response = {}, - .process_response_end_of_body = {}, - .process_response_consume_body = [this, hr = options](auto response, auto body_bytes) { - // 1. Let success be true. - bool success = true; + Fetch::Infrastructure::FetchAlgorithms::Input fetch_algorithms_input {}; + fetch_algorithms_input.process_response_consume_body = [this, hr = options](auto response, auto body_bytes) { + // FIXME: If the response is CORS cross-origin, we must use its internal response to query any of its data. See: + // https://github.com/whatwg/html/issues/9355 + response = response->unsafe_response(); - // 2. If either of the following conditions are met: - // - bodyBytes is null or failure; or - // - response's status is not an ok status, - // then set success to false. - // NOTE: content-specific errors, e.g., CSS parse errors or PNG decoding errors, do not affect success. - if (body_bytes.template has()) { - // CORS cross-origin responses in the No CORS request mode provide an opaque filtered response, which is the original response - // with certain attributes removed/changed. + // 1. Let success be true. + bool success = true; - // The relevant effect it has is setting the body to `null`, which means `body_bytes` has `Empty` here. This effectively - // disables cross-origin linked resources (e.g. stylesheets). + // 2. If either of the following conditions are met: + // - bodyBytes is null or failure; or + // - response's status is not an ok status, + if (body_bytes.template has() || body_bytes.template has() || !Fetch::Infrastructure::is_ok_status(response->status())) { + // then set success to false. + success = false; + } - // However, the web actually depends on this, especially for stylesheets retrieved from a cross-origin CDN. For example, - // Shopify websites request stylesheets from `cdn.shopify.com` and Substack websites request stylesheets from `substackcdn.com`. + // FIXME: 3. Otherwise, wait for the link resource's critical subresources to finish loading. - // This makes this a specification bug, as this code was written from it. + // 4. Process the linked resource given el, success, response, and bodyBytes. + process_linked_resource(success, response, body_bytes); + }; - // The workaround is to read the actual body from the unfiltered response and then call `process_linked_resource` from there. - - // This _should_ be safe to do, as linked resource fetches do not include credentials (i.e. cookies and the Authorization - // header), so it cannot provide personalized responses. - - // FIXME: Replace this workaround with a proper fix that has landed in the specification. - // See: https://github.com/whatwg/html/issues/9066 - if (is(response.ptr())) { - auto unsafe_response = static_cast(*response).internal_response(); - if (unsafe_response->body().has_value()) { - // NOTE: `this` and `unsafe_response` are protected by `fully_read` using JS::SafeFunction. - auto process_body = [this, unsafe_response](ByteBuffer bytes) { - process_linked_resource(true, unsafe_response, bytes); - }; - - // NOTE: `this` and `unsafe_response` are protected by `fully_read` using JS::SafeFunction. - auto process_body_error = [this, unsafe_response](auto) { - process_linked_resource(false, unsafe_response, Fetch::Infrastructure::FetchAlgorithms::ConsumeBodyFailureTag {}); - }; - - unsafe_response->body()->fully_read(realm(), move(process_body), move(process_body_error), JS::NonnullGCPtr { realm().global_object() }).release_value_but_fixme_should_propagate_errors(); - return; - } else { - success = false; - } - } else { - success = false; - } - } else if (body_bytes.template has() || !Fetch::Infrastructure::is_ok_status(response->status())) { - success = false; - } - // FIXME: 3. Otherwise, wait for the link resource's critical subresources to finish loading. - - // 4. Process the linked resource given el, success, response, and bodyBytes. - process_linked_resource(success, response, body_bytes); - } })) - .release_value_but_fixme_should_propagate_errors(); + Fetch::Fetching::fetch(realm(), *request, Fetch::Infrastructure::FetchAlgorithms::create(vm(), move(fetch_algorithms_input))).release_value_but_fixme_should_propagate_errors(); } // https://html.spec.whatwg.org/multipage/links.html#link-type-stylesheet:process-the-linked-resource diff --git a/Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp b/Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp index 0c4fe4483c..0ab52addac 100644 --- a/Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp +++ b/Userland/Libraries/LibWeb/HTML/HTMLMediaElement.cpp @@ -840,6 +840,10 @@ WebIDL::ExceptionOr HTMLMediaElement::fetch_resource(AK::URL const& url_re fetch_algorithms_input.process_response = [this, byte_range = move(byte_range), failure_callback = move(failure_callback)](auto response) mutable { auto& realm = this->realm(); + // FIXME: If the response is CORS cross-origin, we must use its internal response to query any of its data. See: + // https://github.com/whatwg/html/issues/9355 + response = response->unsafe_response(); + // 1. Let global be the media element's node document's relevant global object. auto& global = document().realm().global_object(); @@ -882,14 +886,6 @@ WebIDL::ExceptionOr HTMLMediaElement::fetch_resource(AK::URL const& url_re // 5. Otherwise, incrementally read response's body given updateMedia, processEndOfMedia, an empty algorithm, and global. - // FIXME: Spec issue: If the response is CORS-cross-origin, we need to read from its internal response instead. - // https://github.com/whatwg/html/issues/3483 - // https://github.com/whatwg/html/issues/9066 - if (response->type() == Fetch::Infrastructure::Response::Type::Opaque || response->type() == Fetch::Infrastructure::Response::Type::OpaqueRedirect) { - auto& filtered_response = static_cast(*response); - response = filtered_response.internal_response(); - } - VERIFY(response->body().has_value()); auto empty_algorithm = [](auto) {}; diff --git a/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp b/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp index 9bf122df4c..5171433e43 100644 --- a/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp +++ b/Userland/Libraries/LibWeb/HTML/Scripting/Fetching.cpp @@ -234,66 +234,6 @@ static void set_up_classic_script_request(Fetch::Infrastructure::Request& reques request.set_priority(options.fetch_priority); } -class ClassicScriptResponseHandler final : public RefCounted { -public: - ClassicScriptResponseHandler(JS::NonnullGCPtr element, EnvironmentSettingsObject& settings_object, ScriptFetchOptions options, String character_encoding, OnFetchScriptComplete on_complete) - : m_element(element) - , m_settings_object(settings_object) - , m_options(move(options)) - , m_character_encoding(move(character_encoding)) - , m_on_complete(move(on_complete)) - { - } - - // https://html.spec.whatwg.org/multipage/webappapis.html#fetching-scripts:concept-fetch-4 - void process_response(JS::NonnullGCPtr response, Fetch::Infrastructure::FetchAlgorithms::BodyBytes body_bytes) - { - // 1. Set response to response's unsafe response. - response = response->unsafe_response(); - - // 2. If either of the following conditions are met: - // - bodyBytes is null or failure; or - // - response's status is not an ok status, - if (body_bytes.template has() || body_bytes.template has() || !Fetch::Infrastructure::is_ok_status(response->status())) { - // then run onComplete given null, and abort these steps. - m_on_complete(nullptr); - return; - } - - // 3. Let potentialMIMETypeForEncoding be the result of extracting a MIME type given response's header list. - auto potential_mime_type_for_encoding = response->header_list()->extract_mime_type().release_value_but_fixme_should_propagate_errors(); - - // 4. Set character encoding to the result of legacy extracting an encoding given potentialMIMETypeForEncoding - // and character encoding. - auto character_encoding = Fetch::Infrastructure::legacy_extract_an_encoding(potential_mime_type_for_encoding, m_character_encoding); - - // 5. Let source text be the result of decoding bodyBytes to Unicode, using character encoding as the fallback - // encoding. - auto fallback_decoder = TextCodec::decoder_for(character_encoding); - VERIFY(fallback_decoder.has_value()); - - auto source_text = TextCodec::convert_input_to_utf8_using_given_decoder_unless_there_is_a_byte_order_mark(*fallback_decoder, body_bytes.template get()).release_value_but_fixme_should_propagate_errors(); - - // 6. Let muted errors be true if response was CORS-cross-origin, and false otherwise. - auto muted_errors = response->is_cors_cross_origin() ? ClassicScript::MutedErrors::Yes : ClassicScript::MutedErrors::No; - - // 7. Let script be the result of creating a classic script given source text, settings object, response's URL, - // options, and muted errors. - // FIXME: Pass options. - auto script = ClassicScript::create(m_element->document().url().to_deprecated_string(), source_text, *m_settings_object, response->url().value_or({}), 1, muted_errors); - - // 8. Run onComplete given script. - m_on_complete(script); - } - -private: - JS::Handle m_element; - JS::Handle m_settings_object; - ScriptFetchOptions m_options; - String m_character_encoding; - OnFetchScriptComplete m_on_complete; -}; - // https://html.spec.whatwg.org/multipage/webappapis.html#fetch-a-classic-script WebIDL::ExceptionOr fetch_classic_script(JS::NonnullGCPtr element, AK::URL const& url, EnvironmentSettingsObject& settings_object, ScriptFetchOptions options, CORSSettingAttribute cors_setting, String character_encoding, OnFetchScriptComplete on_complete) { @@ -314,26 +254,44 @@ WebIDL::ExceptionOr fetch_classic_script(JS::NonnullGCPtr(element, settings_object, move(options), move(character_encoding), move(on_complete)); - Fetch::Infrastructure::FetchAlgorithms::Input fetch_algorithms_input {}; - fetch_algorithms_input.process_response_consume_body = [&realm, response_handler = move(response_handler)](auto response, auto body_bytes) { - // FIXME: See HTMLLinkElement::default_fetch_and_process_linked_resource for thorough notes on the workaround - // added here for CORS cross-origin responses. The gist is that all cross-origin responses will have a - // null bodyBytes. So we must read the actual body from the unsafe response. - // https://github.com/whatwg/html/issues/9066 - if (response->is_cors_cross_origin() && body_bytes.template has() && response->unsafe_response()->body().has_value()) { - auto process_body = [response, response_handler](auto bytes) { - response_handler->process_response(response, move(bytes)); - }; - auto process_body_error = [response, response_handler](auto) { - response_handler->process_response(response, Fetch::Infrastructure::FetchAlgorithms::ConsumeBodyFailureTag {}); - }; + fetch_algorithms_input.process_response_consume_body = [element, &settings_object, options = move(options), character_encoding = move(character_encoding), on_complete = move(on_complete)](auto response, auto body_bytes) { + // 1. Set response to response's unsafe response. + response = response->unsafe_response(); - response->unsafe_response()->body()->fully_read(realm, move(process_body), move(process_body_error), JS::NonnullGCPtr { realm.global_object() }).release_value_but_fixme_should_propagate_errors(); - } else { - response_handler->process_response(response, move(body_bytes)); + // 2. If either of the following conditions are met: + // - bodyBytes is null or failure; or + // - response's status is not an ok status, + if (body_bytes.template has() || body_bytes.template has() || !Fetch::Infrastructure::is_ok_status(response->status())) { + // then run onComplete given null, and abort these steps. + on_complete(nullptr); + return; } + + // 3. Let potentialMIMETypeForEncoding be the result of extracting a MIME type given response's header list. + auto potential_mime_type_for_encoding = response->header_list()->extract_mime_type().release_value_but_fixme_should_propagate_errors(); + + // 4. Set character encoding to the result of legacy extracting an encoding given potentialMIMETypeForEncoding + // and character encoding. + auto extracted_character_encoding = Fetch::Infrastructure::legacy_extract_an_encoding(potential_mime_type_for_encoding, character_encoding); + + // 5. Let source text be the result of decoding bodyBytes to Unicode, using character encoding as the fallback + // encoding. + auto fallback_decoder = TextCodec::decoder_for(extracted_character_encoding); + VERIFY(fallback_decoder.has_value()); + + auto source_text = TextCodec::convert_input_to_utf8_using_given_decoder_unless_there_is_a_byte_order_mark(*fallback_decoder, body_bytes.template get()).release_value_but_fixme_should_propagate_errors(); + + // 6. Let muted errors be true if response was CORS-cross-origin, and false otherwise. + auto muted_errors = response->is_cors_cross_origin() ? ClassicScript::MutedErrors::Yes : ClassicScript::MutedErrors::No; + + // 7. Let script be the result of creating a classic script given source text, settings object, response's URL, + // options, and muted errors. + // FIXME: Pass options. + auto script = ClassicScript::create(element->document().url().to_deprecated_string(), source_text, settings_object, response->url().value_or({}), 1, muted_errors); + + // 8. Run onComplete given script. + on_complete(script); }; TRY(Fetch::Fetching::fetch(element->realm(), request, Fetch::Infrastructure::FetchAlgorithms::create(vm, move(fetch_algorithms_input))));