Commit 0a131d1d authored by Lukasz Anforowicz's avatar Lukasz Anforowicz Committed by Commit Bot

Avoid fixing/rewriting/mutating invalid URLs in RewriteURLIfNecessary.

This CL changes BrowserURLHandlerImpl::RewriteURLIfNecessary so that it
returns early(and doesn't mutate the |url| in the in-out argument) if
|url| is invalid.  This helps avoid scenarios where
RewriteUrlForNavigation (in navigation_controller_impl.cc) ends up
generating a NavigationEntry with an invalid virtual URL that
(accidentally/incorrectly) gets rewritten into a valid URL.

Bug: 1116280
Change-Id: I114cf8c8d9459b6931ae659f62a100679b994d5e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2385921Reviewed-by: default avatarAlex Moshchuk <alexmos@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#809537}
parent bec22e23
......@@ -6,6 +6,7 @@
#include "base/feature_list.h"
#include "base/run_loop.h"
#include "base/strings/stringprintf.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/test_timeouts.h"
......@@ -415,17 +416,43 @@ IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest,
content::WebContents* main_contents =
browser()->tab_strip_model()->GetActiveWebContents();
// Simulate a click on the link and wait for the new window.
content::WebContentsAddedObserver new_tab_observer;
EXPECT_TRUE(ExecuteScript(main_contents, "simulateClick()"));
content::WebContents* new_contents = new_tab_observer.GetWebContents();
// The load in the new window should fail.
EXPECT_FALSE(WaitForLoadStop(new_contents));
// Ensure that there is no pending entry or visible URL.
EXPECT_EQ(nullptr, new_contents->GetController().GetPendingEntry());
EXPECT_EQ(GURL(), new_contents->GetVisibleURL());
const char* kTestUrls[] = {
// https://crbug.com/850824
"a.a:@javascript:foo()",
// https://crbug.com/1116280
"o.o:@javascript::://foo.com%0Aalert(document.domain)"};
for (const char* kTestUrl : kTestUrls) {
SCOPED_TRACE(testing::Message() << "kTestUrl = " << kTestUrl);
// Set the test URL.
const char kUrlSettingTemplate[] = R"(
var url = $1;
var anchor = document.getElementById('invalid_url_link');
anchor.target = 'target_name: ' + url;
anchor.href = url;
)";
EXPECT_TRUE(ExecuteScript(
main_contents, content::JsReplace(kUrlSettingTemplate, kTestUrl)));
// Simulate a click on the link and wait for the new window.
content::WebContentsAddedObserver new_tab_observer;
EXPECT_TRUE(ExecuteScript(main_contents, "simulateClick()"));
content::WebContents* new_contents = new_tab_observer.GetWebContents();
// The load in the new window should fail.
EXPECT_FALSE(WaitForLoadStop(new_contents));
// Ensure that there is no pending entry or visible URL.
EXPECT_EQ(nullptr, new_contents->GetController().GetPendingEntry());
EXPECT_EQ(GURL(), new_contents->GetVisibleURL());
// Ensure that the omnibox doesn't start with javascript: scheme.
OmniboxView* omnibox_view =
browser()->window()->GetLocationBar()->GetOmniboxView();
std::string omnibox_text = base::UTF16ToASCII(omnibox_view->GetText());
EXPECT_THAT(omnibox_text, testing::Not(testing::StartsWith("javascript:")));
}
}
// A test performing two simultaneous navigations, to ensure code in chrome/,
......
......@@ -157,6 +157,11 @@ void BrowserURLHandlerImpl::RewriteURLIfNecessary(
DCHECK(browser_context);
DCHECK(reverse_on_redirect);
if (!url->is_valid()) {
*reverse_on_redirect = false;
return;
}
for (const auto& it : url_handlers_) {
const URLHandler& handler = it.first;
bool has_reverse_rewriter = it.second;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment