Skip to content

Commit

Permalink
[CodeHealth] PrepareURLForNavigation with expected
Browse files Browse the repository at this point in the history 
This CL modernises ExtensionTabUtil::PrepareURLForNavigation signature,
to return an expected type, rather than using two out params to signal
success, and failure.

Bug: 1354063
Change-Id: I1715272e4d2b4e5640a4fec6ce64d424abb16307
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4264656
Reviewed-by: Giovanni Ortuno Urquidi <ortuno@chromium.org>
Commit-Queue: Claudio DeSouza <cdesouza@igalia.com>
Cr-Commit-Position: refs/heads/main@{#1108127}
  • Loading branch information
@cdesouza-chromium
cdesouza-chromium authored and Chromium LUCI CQ committed Feb 22, 2023
1 parent e37795d commit 0eead32
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 75 deletions.
23 changes: 11 additions & 12 deletions chrome/browser/extensions/api/tabs/tabs_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -589,13 +589,12 @@ ExtensionFunction::ResponseAction WindowsCreateFunction::Run() {

// Second, resolve, validate and convert them to GURLs.
for (auto& url_string : url_strings) {
GURL url;
std::string error;
if (!ExtensionTabUtil::PrepareURLForNavigation(url_string, extension(),
&url, &error)) {
return RespondNow(Error(std::move(error)));
auto url =
ExtensionTabUtil::PrepareURLForNavigation(url_string, extension());
if (!url.has_value()) {
return RespondNow(Error(std::move(url.error())));
}
urls.push_back(url);
urls.push_back(*url);
}
}

Expand Down Expand Up @@ -1598,19 +1597,19 @@ ExtensionFunction::ResponseAction TabsUpdateFunction::Run() {
bool TabsUpdateFunction::UpdateURL(const std::string& url_string,
int tab_id,
std::string* error) {
GURL url;
if (!ExtensionTabUtil::PrepareURLForNavigation(url_string, extension(), &url,
error)) {
auto url = ExtensionTabUtil::PrepareURLForNavigation(url_string, extension());
if (!url.has_value()) {
*error = std::move(url.error());
return false;
}

// JavaScript URLs are forbidden in chrome.tabs.update().
if (url.SchemeIs(url::kJavaScriptScheme)) {
if (url->SchemeIs(url::kJavaScriptScheme)) {
*error = tabs_constants::kJavaScriptUrlsNotAllowedInTabsUpdate;
return false;
}

NavigationController::LoadURLParams load_params(url);
NavigationController::LoadURLParams load_params(*url);

// Treat extension-initiated navigations as renderer-initiated so that the URL
// does not show in the omnibox until it commits. This avoids URL spoofs
Expand All @@ -1630,7 +1629,7 @@ bool TabsUpdateFunction::UpdateURL(const std::string& url_string,

web_contents_->GetController().LoadURLWithParams(load_params);

DCHECK_EQ(url,
DCHECK_EQ(*url,
web_contents_->GetController().GetPendingEntry()->GetVirtualURL());

return true;
Expand Down
32 changes: 14 additions & 18 deletions chrome/browser/extensions/extension_tab_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,10 +204,12 @@ base::expected<base::Value::Dict, std::string> ExtensionTabUtil::OpenTab(

GURL url;
if (params.url) {
if (!ExtensionTabUtil::PrepareURLForNavigation(
*params.url, function->extension(), &url, &error)) {
return base::unexpected(error);
auto result = ExtensionTabUtil::PrepareURLForNavigation(
*params.url, function->extension());
if (!result.has_value()) {
return base::unexpected(result.error());
}
url = std::move(*result);
} else {
url = GURL(chrome::kChromeUINewTabURL);
}
Expand Down Expand Up @@ -773,10 +775,9 @@ bool ExtensionTabUtil::IsKillURL(const GURL& url) {
return base::Contains(kill_hosts, url.host_piece());
}

bool ExtensionTabUtil::PrepareURLForNavigation(const std::string& url_string,
const Extension* extension,
GURL* return_url,
std::string* error) {
base::expected<GURL, std::string> ExtensionTabUtil::PrepareURLForNavigation(
const std::string& url_string,
const Extension* extension) {
GURL url =
ExtensionTabUtil::ResolvePossiblyRelativeURL(url_string, extension);

Expand All @@ -788,15 +789,13 @@ bool ExtensionTabUtil::PrepareURLForNavigation(const std::string& url_string,

// Reject invalid URLs.
if (!url.is_valid()) {
*error = ErrorUtils::FormatErrorMessage(tabs_constants::kInvalidUrlError,
url_string);
return false;
return base::unexpected(ErrorUtils::FormatErrorMessage(
tabs_constants::kInvalidUrlError, url_string));
}

// Don't let the extension crash the browser or renderers.
if (ExtensionTabUtil::IsKillURL(url)) {
*error = tabs_constants::kNoCrashBrowserError;
return false;
return base::unexpected(tabs_constants::kNoCrashBrowserError);
}

// Don't let the extension navigate directly to devtools scheme pages, unless
Expand All @@ -808,19 +807,16 @@ bool ExtensionTabUtil::PrepareURLForNavigation(const std::string& url_string,
extension->permissions_data()->HasAPIPermission(
APIPermissionID::kDebugger));
if (!has_permission) {
*error = tabs_constants::kCannotNavigateToDevtools;
return false;
return base::unexpected(tabs_constants::kCannotNavigateToDevtools);
}
}

// Don't let the extension navigate directly to chrome-untrusted scheme pages.
if (url.SchemeIs(content::kChromeUIUntrustedScheme)) {
*error = tabs_constants::kCannotNavigateToChromeUntrusted;
return false;
return base::unexpected(tabs_constants::kCannotNavigateToChromeUntrusted);
}

return_url->Swap(&url);
return true;
return url;
}

void ExtensionTabUtil::CreateTab(
Expand Down
10 changes: 4 additions & 6 deletions chrome/browser/extensions/extension_tab_util.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -209,12 +209,10 @@ class ExtensionTabUtil {
static bool IsKillURL(const GURL& url);

// Resolves the URL and ensures the extension is allowed to navigate to it.
// Returns true and sets |url| if successful. Returns false and sets |error|
// if an error occurs.
static bool PrepareURLForNavigation(const std::string& url_string,
const Extension* extension,
GURL* url,
std::string* error);
// Returns the url if successful, otherwise returns an error string.
static base::expected<GURL, std::string> PrepareURLForNavigation(
const std::string& url_string,
const Extension* extension);

// Opens a tab for the specified |web_contents|.
static void CreateTab(std::unique_ptr<content::WebContents> web_contents,
Expand Down
67 changes: 28 additions & 39 deletions chrome/browser/extensions/extension_tab_util_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,32 +92,27 @@ TEST(ExtensionTabUtilTest, PrepareURLForNavigation) {
// A fully qualified URL should return the same URL.
{
const std::string kTestUrl("http://google.com");
std::string error;
GURL url;
EXPECT_TRUE(ExtensionTabUtil::PrepareURLForNavigation(
kTestUrl, extension.get(), &url, &error));
EXPECT_EQ(GURL(kTestUrl), url);
EXPECT_EQ("", error);
auto url =
ExtensionTabUtil::PrepareURLForNavigation(kTestUrl, extension.get());
ASSERT_TRUE(url.has_value());
EXPECT_EQ(GURL(kTestUrl), *url);
}
// A relative path should return a URL relative to the extension's base URL.
{
const std::string kTestPath("foo");
std::string error;
GURL url;
EXPECT_TRUE(ExtensionTabUtil::PrepareURLForNavigation(
kTestPath, extension.get(), &url, &error));
EXPECT_EQ(extension->GetResourceURL(kTestPath), url);
EXPECT_EQ("", error);
auto url =
ExtensionTabUtil::PrepareURLForNavigation(kTestPath, extension.get());
ASSERT_TRUE(url.has_value());
EXPECT_EQ(extension->GetResourceURL(kTestPath), *url);
}
// A kill URL should return false and set the error. There are several
// different potential kill URLs and this just checks one of them.
{
const std::string kKillURL("chrome://crash");
std::string error;
GURL url;
EXPECT_FALSE(ExtensionTabUtil::PrepareURLForNavigation(
kKillURL, extension.get(), &url, &error));
EXPECT_EQ(tabs_constants::kNoCrashBrowserError, error);
auto url =
ExtensionTabUtil::PrepareURLForNavigation(kKillURL, extension.get());
ASSERT_FALSE(url.has_value());
EXPECT_EQ(tabs_constants::kNoCrashBrowserError, url.error());
}
}

Expand All @@ -127,45 +122,39 @@ TEST(ExtensionTabUtilTest, PrepareURLForNavigationOnDevtools) {
// A devtools url should return false and set the error.
{
auto no_permission_extension = ExtensionBuilder("none").Build();
std::string error;
GURL url;
EXPECT_FALSE(ExtensionTabUtil::PrepareURLForNavigation(
kDevtoolsURL, no_permission_extension.get(), &url, &error));
EXPECT_EQ(tabs_constants::kCannotNavigateToDevtools, error);
auto url = ExtensionTabUtil::PrepareURLForNavigation(
kDevtoolsURL, no_permission_extension.get());
ASSERT_FALSE(url.has_value());
EXPECT_EQ(tabs_constants::kCannotNavigateToDevtools, url.error());
}
// Having the devtools permissions should allow access.
{
auto devtools_extension = ExtensionBuilder("devtools")
.SetManifestKey("devtools_page", "foo.html")
.Build();
std::string error;
GURL url;
EXPECT_TRUE(ExtensionTabUtil::PrepareURLForNavigation(
kDevtoolsURL, devtools_extension.get(), &url, &error));
EXPECT_EQ(kDevtoolsURL, url);
EXPECT_TRUE(error.empty());
auto url = ExtensionTabUtil::PrepareURLForNavigation(
kDevtoolsURL, devtools_extension.get());
ASSERT_TRUE(url.has_value());
EXPECT_EQ(kDevtoolsURL, *url);
}
// Having the debugger permissions should also allow access.
{
auto debugger_extension =
ExtensionBuilder("debugger").AddPermission("debugger").Build();
std::string error;
GURL url;
EXPECT_TRUE(ExtensionTabUtil::PrepareURLForNavigation(
kDevtoolsURL, debugger_extension.get(), &url, &error));
EXPECT_EQ(kDevtoolsURL, url);
EXPECT_TRUE(error.empty());
auto url = ExtensionTabUtil::PrepareURLForNavigation(
kDevtoolsURL, debugger_extension.get());
ASSERT_TRUE(url.has_value());
EXPECT_EQ(kDevtoolsURL, *url);
}
}

TEST(ExtensionTabUtilTest, PrepareURLForNavigationOnChromeUntrusted) {
const std::string kChromeUntrustedURL("chrome-untrusted://terminal/");
auto extension = ExtensionBuilder("none").Build();
std::string error;
GURL url;
EXPECT_FALSE(ExtensionTabUtil::PrepareURLForNavigation(
kChromeUntrustedURL, extension.get(), &url, &error));
EXPECT_EQ(tabs_constants::kCannotNavigateToChromeUntrusted, error);
auto url = ExtensionTabUtil::PrepareURLForNavigation(kChromeUntrustedURL,
extension.get());
ASSERT_FALSE(url.has_value());
EXPECT_EQ(tabs_constants::kCannotNavigateToChromeUntrusted, url.error());
}

} // namespace extensions

0 comments on commit 0eead32

Please sign in to comment.