Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: cherry-pick f781748dcb3c from chromium #32257

Merged
merged 2 commits into from Jan 3, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -121,3 +121,4 @@ cherry-pick-1a8af2da50e4.patch
cherry-pick-a5f54612590d.patch
cachestorage_store_partial_opaque_responses.patch
fix_aspect_ratio_with_max_size.patch
cherry-pick-f781748dcb3c.patch
195 changes: 195 additions & 0 deletions patches/chromium/cherry-pick-f781748dcb3c.patch
@@ -0,0 +1,195 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Ravjit <ravjit@chromium.org>
Date: Thu, 16 Sep 2021 12:29:55 +0000
Subject: Show the origin of the last redirecting server in external protocol
handler dialogs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

External protocol handlers always show the initiating url's origin. This can be misleading if there are server side redirects.
Now we will show the origin of the last redirecting server (falling back to the request_initiator if there were no redirects / if the request goes straight to an external protocol).

Bug: 1197889
Change-Id: I3cf7ccf3a8bd79d161364680a1871d1c88bec813
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3113931
Commit-Queue: Ravjit Singh Uppal <ravjit@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Collin Baker <collinbaker@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/main@{#922096}

diff --git a/chrome/browser/ui/views/external_protocol_dialog_browsertest.cc b/chrome/browser/ui/views/external_protocol_dialog_browsertest.cc
index 7e953fe89ff8dadf4da9578c0fdc6c3aada11aca..5c7e331ea4fbbcf36ac6463b37884cc9c6ca7e41 100644
--- a/chrome/browser/ui/views/external_protocol_dialog_browsertest.cc
+++ b/chrome/browser/ui/views/external_protocol_dialog_browsertest.cc
@@ -14,11 +14,15 @@
#include "chrome/browser/ui/test/test_browser_dialog.h"
#include "chrome/browser/ui/views/external_protocol_dialog.h"
#include "chrome/test/base/in_process_browser_test.h"
+#include "chrome/test/base/ui_test_utils.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/browser/render_view_host.h"
#include "content/public/browser/web_contents.h"
#include "content/public/test/browser_test.h"
+#include "net/dns/mock_host_resolver.h"
+#include "net/test/embedded_test_server/http_request.h"
+#include "net/test/embedded_test_server/http_response.h"
#include "ui/views/controls/button/checkbox.h"
#include "url/gurl.h"

@@ -41,6 +45,33 @@ class ExternalProtocolDialogTestApi {

} // namespace test

+namespace {
+constexpr char kInitiatingOrigin[] = "a.test";
+constexpr char kRedirectingOrigin[] = "b.test";
+
+class FakeDefaultProtocolClientWorker
+ : public shell_integration::DefaultProtocolClientWorker {
+ public:
+ explicit FakeDefaultProtocolClientWorker(const std::string& protocol)
+ : DefaultProtocolClientWorker(protocol) {}
+ FakeDefaultProtocolClientWorker(const FakeDefaultProtocolClientWorker&) =
+ delete;
+ FakeDefaultProtocolClientWorker& operator=(
+ const FakeDefaultProtocolClientWorker&) = delete;
+
+ private:
+ ~FakeDefaultProtocolClientWorker() override = default;
+ shell_integration::DefaultWebClientState CheckIsDefaultImpl() override {
+ return shell_integration::DefaultWebClientState::NOT_DEFAULT;
+ }
+
+ void SetAsDefaultImpl(base::OnceClosure on_finished_callback) override {
+ base::SequencedTaskRunnerHandle::Get()->PostTask(
+ FROM_HERE, std::move(on_finished_callback));
+ }
+};
+} // namespace
+
class ExternalProtocolDialogBrowserTest
: public DialogBrowserTest,
public ExternalProtocolHandler::Delegate {
@@ -71,11 +102,11 @@ class ExternalProtocolDialogBrowserTest
// ExternalProtocolHander::Delegate:
scoped_refptr<shell_integration::DefaultProtocolClientWorker>
CreateShellWorker(const std::string& protocol) override {
- return nullptr;
+ return base::MakeRefCounted<FakeDefaultProtocolClientWorker>(protocol);
}
ExternalProtocolHandler::BlockState GetBlockState(const std::string& scheme,
Profile* profile) override {
- return ExternalProtocolHandler::DONT_BLOCK;
+ return ExternalProtocolHandler::UNKNOWN;
}
void BlockRequest() override {}
void RunExternalProtocolDialog(
@@ -83,7 +114,10 @@ class ExternalProtocolDialogBrowserTest
content::WebContents* web_contents,
ui::PageTransition page_transition,
bool has_user_gesture,
- const absl::optional<url::Origin>& initiating_origin) override {}
+ const absl::optional<url::Origin>& initiating_origin) override {
+ url_did_launch_ = true;
+ launch_url_ = initiating_origin->host();
+ }
void LaunchUrlWithoutSecurityCheck(
const GURL& url,
content::WebContents* web_contents) override {
@@ -98,6 +132,12 @@ class ExternalProtocolDialogBrowserTest
blocked_state_ = state;
}

+ void SetUpOnMainThread() override {
+ DialogBrowserTest::SetUpOnMainThread();
+ host_resolver()->AddRule(kInitiatingOrigin, "127.0.0.1");
+ host_resolver()->AddRule(kRedirectingOrigin, "127.0.0.1");
+ }
+
base::HistogramTester histogram_tester_;

protected:
@@ -106,6 +146,7 @@ class ExternalProtocolDialogBrowserTest
url::Origin blocked_origin_;
BlockState blocked_state_ = BlockState::UNKNOWN;
bool url_did_launch_ = false;
+ std::string launch_url_;

private:
DISALLOW_COPY_AND_ASSIGN(ExternalProtocolDialogBrowserTest);
@@ -231,3 +272,21 @@ IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, TestFocus) {
const views::View* focused_view = focus_manager->GetFocusedView();
EXPECT_TRUE(focused_view);
}
+
+IN_PROC_BROWSER_TEST_F(ExternalProtocolDialogBrowserTest, OriginNameTest) {
+ ASSERT_TRUE(embedded_test_server()->Start());
+ content::WebContents* web_contents =
+ browser()->tab_strip_model()->GetActiveWebContents();
+ EXPECT_TRUE(ui_test_utils::NavigateToURL(
+ browser(), embedded_test_server()->GetURL("a.test", "/empty.html")));
+ EXPECT_TRUE(content::ExecJs(
+ web_contents,
+ content::JsReplace("location.href = $1",
+ embedded_test_server()->GetURL(
+ "b.test", "/server-redirect?ms-calc:"))));
+ content::WaitForLoadStop(web_contents);
+ EXPECT_TRUE(url_did_launch_);
+ // The url should be the url of the last redirecting server and not of the
+ // request initiator
+ EXPECT_EQ(launch_url_, "b.test");
+}
diff --git a/content/browser/loader/navigation_url_loader_impl.cc b/content/browser/loader/navigation_url_loader_impl.cc
index 42f62ccb3830bf718806f2361d9fb8a4abd53c6a..3d91eadbed7fc9cfd9a70805415eb660b15398d4 100644
--- a/content/browser/loader/navigation_url_loader_impl.cc
+++ b/content/browser/loader/navigation_url_loader_impl.cc
@@ -626,6 +626,13 @@ NavigationURLLoaderImpl::PrepareForNonInterceptedRequest(
if (known_schemes_.find(resource_request_->url.scheme()) ==
known_schemes_.end()) {
mojo::PendingRemote<network::mojom::URLLoaderFactory> loader_factory;
+ absl::optional<url::Origin> initiating_origin;
+ if (url_chain_.size() > 1) {
+ initiating_origin =
+ url::Origin::Create(url_chain_[url_chain_.size() - 2]);
+ } else {
+ initiating_origin = resource_request_->request_initiator;
+ }
bool handled = GetContentClient()->browser()->HandleExternalProtocol(
resource_request_->url, web_contents_getter_,
ChildProcessHost::kInvalidUniqueID, frame_tree_node_id_,
@@ -633,8 +640,8 @@ NavigationURLLoaderImpl::PrepareForNonInterceptedRequest(
resource_request_->resource_type ==
static_cast<int>(blink::mojom::ResourceType::kMainFrame),
static_cast<ui::PageTransition>(resource_request_->transition_type),
- resource_request_->has_user_gesture,
- resource_request_->request_initiator, &loader_factory);
+ resource_request_->has_user_gesture, initiating_origin,
+ &loader_factory);

if (loader_factory) {
factory = base::MakeRefCounted<network::WrapperSharedURLLoaderFactory>(
diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h
index 86b630ca2fa26150d71451a1ffe77552cdd589a9..c8981fea3bf8596797d68a3a164309444bb6e1ec 100644
--- a/content/public/browser/content_browser_client.h
+++ b/content/public/browser/content_browser_client.h
@@ -1758,10 +1758,12 @@ class CONTENT_EXPORT ContentBrowserClient {
// Otherwise child_id will be the process id and |navigation_ui_data| will be
// nullptr.
//
- // |initiating_origin| is the origin that initiated the navigation to the
- // external protocol, and may be null, e.g. in the case of browser-initiated
- // navigations. The initiating origin is intended to help users make security
- // decisions about whether to allow an external application to launch.
+ // |initiating_origin| is the origin of the last redirecting server (falling
+ // back to the request initiator if there were no redirects / if the request
+ // goes straight to an external protocol, or null, e.g. in the case of
+ // browser-initiated navigations. The initiating origin is intended to help
+ // users make security decisions about whether to allow an external
+ // application to launch.
virtual bool HandleExternalProtocol(
const GURL& url,
base::RepeatingCallback<WebContents*()> web_contents_getter,