From d9b5075a841c156ad98a0cea0fdad93f467cc076 Mon Sep 17 00:00:00 2001 From: Allan Sandfeld Jensen Date: Thu, 23 Sep 2021 16:51:21 +0200 Subject: Block CORS from local URLs when remote access is not enabled Task-number: QTBUG-96849 Change-Id: I0e0a1530b8b31341c632a1fd00abd339b5152da0 Reviewed-by: Kirill Burtsev (cherry picked from commit f6f8f258be09fef90585b0228bd82a9708ef34a6) --- src/core/doc/src/qwebenginesettings_lgpl.qdoc | 13 ++++----- src/core/net/proxying_url_loader_factory_qt.cpp | 37 +++++++++++++++++++++++++ tests/auto/core/origins/tst_origins.cpp | 2 +- 3 files changed, 44 insertions(+), 8 deletions(-) diff --git a/src/core/doc/src/qwebenginesettings_lgpl.qdoc b/src/core/doc/src/qwebenginesettings_lgpl.qdoc index 7c6ccd3d3..bfe2713a2 100644 --- a/src/core/doc/src/qwebenginesettings_lgpl.qdoc +++ b/src/core/doc/src/qwebenginesettings_lgpl.qdoc @@ -97,13 +97,11 @@ Enables support for the HTML 5 local storage feature. Enabled by default. \value LocalContentCanAccessRemoteUrls Allows locally loaded documents to ignore cross-origin rules so that they can access - remote resources that would normally be blocked, because all remote resources are - considered cross-origin for a local file. Remote access that would not be blocked by + remote resources that would normally be blocked, since remote resources are + considered cross-origin for a local document. Remote access that would not be blocked by cross-origin rules is still possible when this setting is disabled (default). - Note that disabling this setting does not stop XMLHttpRequests or media elements in - local files from accessing remote content. Basically, it only stops some HTML - subresources, such as scripts, and therefore disabling this setting is not a safety - mechanism. + Note that disabling this setting does not prevent media elements in local files from + accessing remote content. Disabled by default. \value XSSAuditingEnabled Obsolete and has no effect. \value SpatialNavigationEnabled @@ -114,7 +112,8 @@ trying to reach towards the right and which element they probably want. Disabled by default. \value LocalContentCanAccessFileUrls - Allows locally loaded documents to access other local URLs. Enabled by default. + Allows locally loaded documents to access other local URLs. Disabling this makes QtWebEngine + behave more like Chrome and Firefox does by default. Enabled by default. \value HyperlinkAuditingEnabled Enables support for the \c ping attribute for hyperlinks. Disabled by default. \value ScrollAnimatorEnabled diff --git a/src/core/net/proxying_url_loader_factory_qt.cpp b/src/core/net/proxying_url_loader_factory_qt.cpp index c73da1455..6505e5505 100644 --- a/src/core/net/proxying_url_loader_factory_qt.cpp +++ b/src/core/net/proxying_url_loader_factory_qt.cpp @@ -47,8 +47,11 @@ #include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_thread.h" #include "content/public/browser/web_contents.h" +#include "content/public/common/content_switches.h" #include "net/http/http_status_code.h" +#include "services/network/public/cpp/cors/cors.h" #include "third_party/blink/public/mojom/loader/resource_load_info.mojom-shared.h" +#include "url/url_util.h" #include "api/qwebengineurlrequestinfo_p.h" #include "type_conversion.h" @@ -164,6 +167,7 @@ private: const uint64_t request_id_; const int32_t routing_id_; const uint32_t options_; + bool allowed_cors_ = true; // If the |target_loader_| called OnComplete with an error this stores it. // That way the destructor can send it to OnReceivedError if safe browsing @@ -205,12 +209,37 @@ InterceptedRequest::InterceptedRequest(ProfileAdapter *profile_adapter, , target_factory_(std::move(target_factory)) , weak_factory_(this) { + const bool disable_web_security = base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kDisableWebSecurity); current_response_ = network::mojom::URLResponseHead::New(); + current_response_->response_type = network::cors::CalculateResponseType( + request_.mode, + disable_web_security || ( + request_.request_initiator && request_.request_initiator->IsSameOriginWith(url::Origin::Create(request_.url)))); // If there is a client error, clean up the request. target_client_.set_disconnect_handler( base::BindOnce(&InterceptedRequest::OnURLLoaderClientError, base::Unretained(this))); proxied_loader_receiver_.set_disconnect_with_reason_handler( base::BindOnce(&InterceptedRequest::OnURLLoaderError, base::Unretained(this))); + if (!disable_web_security && request_.request_initiator) { + const std::vector &localSchemes = url::GetLocalSchemes(); + std::string fromScheme = request_.request_initiator->GetTupleOrPrecursorTupleIfOpaque().scheme(); + if (base::Contains(localSchemes, fromScheme)) { + content::WebContents *wc = webContents(); + std::string toScheme = request_.url.scheme(); + // local schemes must have universal access, or be accessing something local and have local access. + if (fromScheme != toScheme) { + // note allow_file_access_from_file_urls maps to LocalContentCanAccessFileUrls in our API + // and allow_universal_access_from_file_urls to LocalContentCanAccessRemoteUrls, so we are + // using them as proxies for our API here. + if (toScheme == "file") + allowed_cors_ = wc && wc->GetOrCreateWebPreferences().allow_file_access_from_file_urls; + else if (!base::Contains(localSchemes, toScheme)) + allowed_cors_ = wc && wc->GetOrCreateWebPreferences().allow_universal_access_from_file_urls; + else + allowed_cors_ = true; // We should think about this for future patches + } + } + } } InterceptedRequest::~InterceptedRequest() @@ -247,6 +276,14 @@ void InterceptedRequest::Restart() { DCHECK_CURRENTLY_ON(content::BrowserThread::UI); + // This is a CORS check on the from URL, the normal check on the to URL is applied later + if (!allowed_cors_ && current_response_->response_type == network::mojom::FetchResponseType::kCors) { + target_client_->OnComplete(network::URLLoaderCompletionStatus( + network::CorsErrorStatus(network::mojom::CorsError::kCorsDisabledScheme))); + delete this; + return; + } + // MEMO since all codepatch leading to Restart scheduled and executed as asynchronous tasks in main thread, // interceptors may change in meantime and also during intercept call, so they should be resolved anew. // Set here only profile's interceptor since it runs first without going to user code. diff --git a/tests/auto/core/origins/tst_origins.cpp b/tests/auto/core/origins/tst_origins.cpp index 518b20045..a8ec5b7b0 100644 --- a/tests/auto/core/origins/tst_origins.cpp +++ b/tests/auto/core/origins/tst_origins.cpp @@ -672,7 +672,7 @@ void tst_Origins::mixedXHR_data() QTest::newRow("file->cors") << QString("file:" + QDir(QT_TESTCASE_SOURCEDIR).canonicalPath() + "/resources/mixedXHR.html") << QString("sendXHR('cors:/resources/mixedXHR.txt')") - << QVariant(QString("ok")); + << QVariant(QString("error")); QTest::newRow("qrc->file") << QString("qrc:/resources/mixedXHR.html") << QString("sendXHR('file:" -- cgit v1.2.3