Skip to content

Commit

Permalink
[Merge M115] webui: Filter out non-chrome scheme URLs in WebUIConfigMap
Browse files Browse the repository at this point in the history
url::Origin::Create() drops filesystem: and blob: from URLs so filter
those out before using Create().

(cherry picked from commit b46c372)

(cherry picked from commit 1ae7ab6)

Bug: b/291154025
Change-Id: Ia8fd0d1048deaef346accd7b4cb64b448e8dc9f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4686653
Commit-Queue: Giovanni Ortuno Urquidi <ortuno@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Original-Commit-Position: refs/heads/main@{#1172220}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4719366
Cr-Original-Commit-Position: refs/branch-heads/5845@{#833}
Cr-Original-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4728383
Auto-Submit: Giovanni Ortuno Urquidi <ortuno@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/branch-heads/5790@{#1864}
Cr-Branched-From: 1d71a33-refs/heads/main@{#1148114}
  • Loading branch information
Giovanni Ortuño Urquidi authored and Chromium LUCI CQ committed Jul 28, 2023
1 parent 0d93f66 commit b2354da
Show file tree
Hide file tree
Showing 11 changed files with 178 additions and 37 deletions.
Expand Up @@ -182,13 +182,12 @@ void SystemExtensionsInstallManager::Uninstall(
return;
}

const url::Origin& origin = url::Origin::Create(system_extension->base_url);

// Uninstallation Step #1: Unregister the Service Worker.
service_worker_manager_->UnregisterServiceWorker(system_extension_id);

// Uninstallation Step #2: Remove the WebUIConfig for the System Extension.
content::WebUIConfigMap::GetInstance().RemoveConfig(origin);
content::WebUIConfigMap::GetInstance().RemoveConfig(
system_extension->base_url);

// Installation Step #3: Remove the System Extension from persistent storage.
persistent_storage_->Remove(system_extension_id);
Expand Down
Expand Up @@ -76,7 +76,7 @@ class DemoModeAppIntegrationTestBase : public ash::SystemWebAppIntegrationTest {
base::ScopedAllowBlockingForTesting allow_blocking;
ASSERT_TRUE(component_dir_.CreateUniqueTempDir());
content::WebUIConfigMap::GetInstance().RemoveConfig(
url::Origin::Create(GURL(ash::kChromeUntrustedUIDemoModeAppURL)));
GURL(ash::kChromeUntrustedUIDemoModeAppURL));
content::WebUIConfigMap::GetInstance().AddUntrustedWebUIConfig(
std::make_unique<ash::DemoModeAppUntrustedUIConfig>(
base::BindLambdaForTesting(
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/chrome_content_browser_client.cc
Expand Up @@ -6549,8 +6549,8 @@ bool ChromeContentBrowserClient::HandleWebUI(

if (!ChromeWebUIControllerFactory::GetInstance()->UseWebUIForURL(
browser_context, *url) &&
!content::WebUIConfigMap::GetInstance().GetConfig(
browser_context, url::Origin::Create(*url))) {
!content::WebUIConfigMap::GetInstance().GetConfig(browser_context,
*url)) {
return false;
}

Expand Down
Expand Up @@ -1839,7 +1839,7 @@ ServiceWorkerContextWrapper::GetLoaderFactoryForBrowserInitiatedRequest(
context()->loader_factory_bundle_for_update_check()->Clone();

if (auto* config = content::WebUIConfigMap::GetInstance().GetConfig(
browser_context(), scope_origin)) {
browser_context(), scope)) {
// If this is a Service Worker for a WebUI, the WebUI's URLDataSource
// needs to be registered. Registering a URLDataSource allows the
// WebUIURLLoaderFactory below to serve the resources for the WebUI. We
Expand Down
2 changes: 1 addition & 1 deletion content/browser/webui/web_ui_browsertest.cc
Expand Up @@ -211,7 +211,7 @@ IN_PROC_BROWSER_TEST_F(WebUIImplBrowserTest, ForceSwapOnDifferenteWebUITypes) {
// has changed and the new process also has WebUI bindings.
const GURL web_ui_url2(GetWebUIURL(kChromeUIGpuHost));
EXPECT_TRUE(WebUIConfigMap::GetInstance().GetConfig(
web_contents->GetBrowserContext(), url::Origin::Create(web_ui_url2)));
web_contents->GetBrowserContext(), web_ui_url2));
ASSERT_TRUE(NavigateToURL(web_contents, web_ui_url2));
auto* new_site_instance = web_contents->GetSiteInstance();
EXPECT_NE(orig_site_instance, new_site_instance);
Expand Down
131 changes: 131 additions & 0 deletions content/browser/webui/webui_config_map_unittest.cc
@@ -0,0 +1,131 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "content/public/browser/webui_config_map.h"

#include "content/public/browser/webui_config.h"
#include "content/public/test/scoped_web_ui_controller_factory_registration.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace content {

class WebUIController;

namespace {

constexpr const char kChromeFoo[] = "chrome://foo";
constexpr const char kChromeBar[] = "chrome://bar";
constexpr const char kChromeUntrustedFoo[] = "chrome-untrusted://foo";
constexpr const char kChromeUntrustedBar[] = "chrome-untrusted://bar";

// A valid BrowserContext is not needed for these tests.
BrowserContext* kBrowserContext = nullptr;

class TestConfig : public WebUIConfig {
public:
TestConfig(base::StringPiece scheme, base::StringPiece host)
: WebUIConfig(scheme, host) {}
~TestConfig() override = default;

std::unique_ptr<WebUIController> CreateWebUIController(
WebUI* web_ui,
const GURL& url) override {
// Unused in these tests.
return nullptr;
}
};

} // namespace

TEST(WebUIConfigTest, AddAndRemoveChromeUrl) {
auto& map = WebUIConfigMap::GetInstance();
size_t initial_size = map.GetSizeForTesting();

auto chrome_config = std::make_unique<TestConfig>("chrome", "foo");
auto* chrome_config_ptr = chrome_config.get();
map.AddWebUIConfig(std::move(chrome_config));

EXPECT_EQ(initial_size + 1, map.GetSizeForTesting());

EXPECT_EQ(chrome_config_ptr,
map.GetConfig(kBrowserContext, GURL(kChromeFoo)));
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, GURL(kChromeBar)));
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, GURL(kChromeUntrustedFoo)));
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, GURL(kChromeUntrustedBar)));

EXPECT_EQ(nullptr, map.RemoveConfig(GURL(kChromeBar)));
EXPECT_EQ(nullptr, map.RemoveConfig(GURL(kChromeUntrustedFoo)));
EXPECT_EQ(nullptr, map.RemoveConfig(GURL(kChromeUntrustedBar)));

auto removed_config = map.RemoveConfig(GURL(kChromeFoo));
EXPECT_EQ(removed_config.get(), chrome_config_ptr);
EXPECT_EQ(initial_size, map.GetSizeForTesting());
}

TEST(WebUIConfigTest, AddAndRemoteChromeUntrustedUrl) {
auto& map = WebUIConfigMap::GetInstance();
size_t initial_size = map.GetSizeForTesting();

auto untrusted_config =
std::make_unique<TestConfig>("chrome-untrusted", "foo");
auto* untrusted_config_ptr = untrusted_config.get();
map.AddUntrustedWebUIConfig(std::move(untrusted_config));

EXPECT_EQ(initial_size + 1, map.GetSizeForTesting());

EXPECT_EQ(untrusted_config_ptr,
map.GetConfig(kBrowserContext, GURL(kChromeUntrustedFoo)));
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, GURL(kChromeUntrustedBar)));
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, GURL(kChromeFoo)));
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, GURL(kChromeBar)));

EXPECT_EQ(nullptr, map.RemoveConfig(GURL(kChromeUntrustedBar)));
EXPECT_EQ(nullptr, map.RemoveConfig(GURL(kChromeFoo)));
EXPECT_EQ(nullptr, map.RemoveConfig(GURL(kChromeBar)));

auto removed_config = map.RemoveConfig(GURL(kChromeUntrustedFoo));
EXPECT_EQ(removed_config.get(), untrusted_config_ptr);
EXPECT_EQ(initial_size, map.GetSizeForTesting());
}

// Regression test for https://crbug.com/1464456.
TEST(WebUIConfigTest, GetAndRemoveNonChromeUrls) {
auto& map = WebUIConfigMap::GetInstance();

ScopedWebUIConfigRegistration chrome_config(
std::make_unique<TestConfig>("chrome", "foo"));
ScopedWebUIConfigRegistration untrusted_config(
std::make_unique<TestConfig>("chrome-untrusted", "foo"));

// filesystem: URLs
const GURL file_system_chrome("filesystem:chrome://foo/external/file.txt");
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, file_system_chrome));
EXPECT_DEATH_IF_SUPPORTED(map.RemoveConfig(file_system_chrome), "");

const GURL file_system_chrome_untrusted(
"filesystem:chrome-untrusted://foo/external/file.txt");
EXPECT_EQ(nullptr,
map.GetConfig(kBrowserContext, file_system_chrome_untrusted));
EXPECT_DEATH_IF_SUPPORTED(map.RemoveConfig(file_system_chrome_untrusted), "");

// blob: URLs
const GURL blob_chrome("blob:chrome://foo/GUID");
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, blob_chrome));
EXPECT_DEATH_IF_SUPPORTED(map.RemoveConfig(blob_chrome), "");

const GURL blob_chrome_untrusted("blob:chrome-untrusted://foo/GUID");
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, blob_chrome_untrusted));
EXPECT_DEATH_IF_SUPPORTED(map.RemoveConfig(blob_chrome_untrusted), "");

// HTTP/HTTPS URLs
const GURL http("http://foo.com");
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, http));
EXPECT_DEATH_IF_SUPPORTED(map.RemoveConfig(http), "");

const GURL https("https://foo.com");
EXPECT_EQ(nullptr, map.GetConfig(kBrowserContext, https));
EXPECT_DEATH_IF_SUPPORTED(map.RemoveConfig(https), "");
}

} // namespace content
29 changes: 19 additions & 10 deletions content/public/browser/webui_config_map.cc
Expand Up @@ -29,8 +29,7 @@ class WebUIConfigMapWebUIControllerFactory : public WebUIControllerFactory {

WebUI::TypeID GetWebUIType(BrowserContext* browser_context,
const GURL& url) override {
auto* config =
config_map_->GetConfig(browser_context, url::Origin::Create(url));
auto* config = config_map_->GetConfig(browser_context, url);
if (!config)
return WebUI::kNoWebUI;

Expand All @@ -39,15 +38,14 @@ class WebUIConfigMapWebUIControllerFactory : public WebUIControllerFactory {

bool UseWebUIForURL(BrowserContext* browser_context,
const GURL& url) override {
return config_map_->GetConfig(browser_context, url::Origin::Create(url));
return config_map_->GetConfig(browser_context, url);
}

std::unique_ptr<WebUIController> CreateWebUIControllerForURL(
WebUI* web_ui,
const GURL& url) override {
auto* browser_context = web_ui->GetWebContents()->GetBrowserContext();
auto* config =
config_map_->GetConfig(browser_context, url::Origin::Create(url));
auto* config = config_map_->GetConfig(browser_context, url);
if (!config)
return nullptr;

Expand Down Expand Up @@ -95,8 +93,17 @@ void WebUIConfigMap::AddWebUIConfigImpl(std::unique_ptr<WebUIConfig> config) {
}

WebUIConfig* WebUIConfigMap::GetConfig(BrowserContext* browser_context,
const url::Origin& origin) {
auto origin_and_config = configs_map_.find(origin);
const GURL& url) {
// "filesystem:" and "blob:" get dropped by url::Origin::Create() below. We
// don't want navigations to these URLs to have WebUI bindings, e.g.
// chrome.send() or Mojo.bindInterface(), since some WebUIs currently expose
// untrusted content via these schemes.
if (url.scheme() != kChromeUIScheme &&
url.scheme() != kChromeUIUntrustedScheme) {
return nullptr;
}

auto origin_and_config = configs_map_.find(url::Origin::Create(url));
if (origin_and_config == configs_map_.end())
return nullptr;
auto& config = origin_and_config->second;
Expand All @@ -107,9 +114,11 @@ WebUIConfig* WebUIConfigMap::GetConfig(BrowserContext* browser_context,
return config.get();
}

std::unique_ptr<WebUIConfig> WebUIConfigMap::RemoveConfig(
const url::Origin& origin) {
auto it = configs_map_.find(origin);
std::unique_ptr<WebUIConfig> WebUIConfigMap::RemoveConfig(const GURL& url) {
CHECK(url.scheme() == kChromeUIScheme ||
url.scheme() == kChromeUIUntrustedScheme);

auto it = configs_map_.find(url::Origin::Create(url));
if (it == configs_map_.end())
return nullptr;

Expand Down
13 changes: 7 additions & 6 deletions content/public/browser/webui_config_map.h
Expand Up @@ -10,6 +10,8 @@

#include "content/common/content_export.h"

class GURL;

namespace url {
class Origin;
}
Expand Down Expand Up @@ -48,14 +50,13 @@ class CONTENT_EXPORT WebUIConfigMap {
// readers tell what type of WebUIConfig is being added.
void AddUntrustedWebUIConfig(std::unique_ptr<WebUIConfig> config);

// Returns the WebUIConfig for |origin| if it's registered and the WebUI is
// Returns the WebUIConfig for |url| if it's registered and the WebUI is
// enabled. (WebUIs can be disabled based on the profile or feature flags.)
WebUIConfig* GetConfig(BrowserContext* browser_context,
const url::Origin& origin);
WebUIConfig* GetConfig(BrowserContext* browser_context, const GURL& url);

// Removes and returns the WebUIConfig with |origin|. Returns nullptr if
// there is no WebUIConfig with |origin|.
std::unique_ptr<WebUIConfig> RemoveConfig(const url::Origin& origin);
// Removes and returns the WebUIConfig with |url|. Returns nullptr if
// there is no WebUIConfig with |url|.
std::unique_ptr<WebUIConfig> RemoveConfig(const GURL& url);

// Returns the size of the map, i.e. how many WebUIConfigs are registered.
size_t GetSizeForTesting() { return configs_map_.size(); }
Expand Down
Expand Up @@ -16,10 +16,10 @@ namespace content {

namespace {

url::Origin GetOriginFromConfig(WebUIConfig& webui_config) {
return url::Origin::Create(
GURL(base::StrCat({webui_config.scheme(), url::kStandardSchemeSeparator,
webui_config.host()})));
GURL GetUrlFromConfig(WebUIConfig& webui_config) {
return GURL(
base::StrCat({webui_config.scheme(), url::kStandardSchemeSeparator,
webui_config.host()}));
}

void AddWebUIConfig(std::unique_ptr<WebUIConfig> webui_config) {
Expand Down Expand Up @@ -63,23 +63,23 @@ ScopedWebUIControllerFactoryRegistration::

ScopedWebUIConfigRegistration::ScopedWebUIConfigRegistration(
std::unique_ptr<WebUIConfig> webui_config)
: webui_config_origin_(GetOriginFromConfig(*webui_config)) {
: webui_config_url_(GetUrlFromConfig(*webui_config)) {
auto& config_map = WebUIConfigMap::GetInstance();
replaced_webui_config_ = config_map.RemoveConfig(webui_config_origin_);
replaced_webui_config_ = config_map.RemoveConfig(webui_config_url_);

DCHECK(webui_config.get() != nullptr);
AddWebUIConfig(std::move(webui_config));
}

ScopedWebUIConfigRegistration::ScopedWebUIConfigRegistration(
const GURL& webui_origin)
: webui_config_origin_(url::Origin::Create(webui_origin)) {
: webui_config_url_(webui_origin) {
auto& config_map = WebUIConfigMap::GetInstance();
replaced_webui_config_ = config_map.RemoveConfig(webui_config_origin_);
replaced_webui_config_ = config_map.RemoveConfig(webui_config_url_);
}

ScopedWebUIConfigRegistration::~ScopedWebUIConfigRegistration() {
WebUIConfigMap::GetInstance().RemoveConfig(webui_config_origin_);
WebUIConfigMap::GetInstance().RemoveConfig(webui_config_url_);

// If we replaced a WebUIConfig, re-register it to keep the global state
// clean for future tests.
Expand Down
Expand Up @@ -7,7 +7,7 @@

#include "base/memory/raw_ptr.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "url/origin.h"
#include "url/gurl.h"

namespace content {

Expand Down Expand Up @@ -45,14 +45,14 @@ class ScopedWebUIConfigRegistration {
explicit ScopedWebUIConfigRegistration(
std::unique_ptr<WebUIConfig> webui_config);

// Removes the WebUIConfig with `webui_origin`. The removed WebUIConfig is
// Removes the WebUIConfig with `webui_url`. The removed WebUIConfig is
// re-registered on destruction.
explicit ScopedWebUIConfigRegistration(const GURL& webui_origin);
explicit ScopedWebUIConfigRegistration(const GURL& webui_url);

~ScopedWebUIConfigRegistration();

private:
const url::Origin webui_config_origin_;
const GURL webui_config_url_;
std::unique_ptr<WebUIConfig> replaced_webui_config_;
};

Expand Down
1 change: 1 addition & 0 deletions content/test/BUILD.gn
Expand Up @@ -2660,6 +2660,7 @@ test("content_unittests") {
"../browser/webui/web_ui_unittest.cc",
"../browser/webui/web_ui_url_loader_factory_unittest.cc",
"../browser/webui/web_ui_webui_js_bridge_unittest.cc",
"../browser/webui/webui_config_map_unittest.cc",
"../browser/worker_host/dedicated_worker_service_impl_unittest.cc",
"../browser/worker_host/mock_shared_worker.cc",
"../browser/worker_host/mock_shared_worker.h",
Expand Down

0 comments on commit b2354da

Please sign in to comment.