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

Add default filter provider manager #18876

Merged
29 changes: 24 additions & 5 deletions browser/permissions/localhost_access_permission_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -433,13 +433,28 @@ IN_PROC_BROWSER_TEST_F(LocalhostAccessBrowserTest, AdblockRule) {
// Add adblock rule to block localhost.
std::string test_domain = "localhost";
embedding_url_ = https_server_->GetURL(kTestEmbeddingDomain, kSimplePage);
const auto& target_url = localhost_server_->GetURL(test_domain, "/logo.png");
const auto& target_url =
localhost_server_->GetURL(test_domain, kTestTargetPath);
auto rule = base::StrCat({"||", test_domain, "^"});
AddAdblockRule(rule);
// The image won't show up because of adblock rule.
CheckNoPromptFlow(false, target_url);
}

// Test that badfiltering a localhost adblock rule makes permission come up.
IN_PROC_BROWSER_TEST_F(LocalhostAccessBrowserTest, AdblockRuleBadfilter) {
std::string test_domain = "localhost";
embedding_url_ = https_server_->GetURL(kTestEmbeddingDomain, kSimplePage);
const auto& target_url =
localhost_server_->GetURL(test_domain, kTestTargetPath);

auto adblock_rule = base::StrCat({"||", test_domain, "^"});
auto badfilter_rule = base::StrCat({"\n", adblock_rule, "$badfilter"});
auto rules = adblock_rule + badfilter_rule;
ShivanKaul marked this conversation as resolved.
Show resolved Hide resolved
AddAdblockRule(rules);
CheckAskAndAcceptFlow(target_url);
}

// Test that localhost connections from website not on allowlist
// are blocked without permission prompt.
IN_PROC_BROWSER_TEST_F(LocalhostAccessBrowserTest, WebsiteNotOnAllowlist) {
Expand All @@ -449,7 +464,8 @@ IN_PROC_BROWSER_TEST_F(LocalhostAccessBrowserTest, WebsiteNotOnAllowlist) {
localhost_permission_component_->SetAllowedDomainsForTesting(
{base::StrCat({kTestEmbeddingDomain, "\n", "#b.com"})});
embedding_url_ = https_server_->GetURL("b.com", kSimplePage);
const auto& target_url = localhost_server_->GetURL(test_domain, "/logo.png");
const auto& target_url =
localhost_server_->GetURL(test_domain, kTestTargetPath);
CheckNoPromptFlow(false, target_url);
}

Expand All @@ -461,7 +477,8 @@ IN_PROC_BROWSER_TEST_F(LocalhostAccessBrowserTest,
// Clear out the allowlist.
localhost_permission_component_->SetAllowedDomainsForTesting({""});
embedding_url_ = https_server_->GetURL(kTestEmbeddingDomain, kSimplePage);
const auto& target_url = localhost_server_->GetURL(test_domain, "/logo.png");
const auto& target_url =
localhost_server_->GetURL(test_domain, kTestTargetPath);
SetCurrentStatus(ContentSetting::CONTENT_SETTING_ALLOW);
// Load subresource, should succeed.
InsertImage(target_url.spec(), true);
Expand All @@ -474,7 +491,8 @@ IN_PROC_BROWSER_TEST_F(LocalhostAccessBrowserTest, WebsitePartOfETLDP1) {
std::string test_domain = "localhost";
embedding_url_ = https_server_->GetURL(
base::StrCat({"test1.", kTestEmbeddingDomain}), kSimplePage);
const auto& target_url = localhost_server_->GetURL(test_domain, "/logo.png");
const auto& target_url =
localhost_server_->GetURL(test_domain, kTestTargetPath);
CheckAskAndAcceptFlow(target_url);
embedding_url_ = https_server_->GetURL(
base::StrCat({"test2.", kTestEmbeddingDomain}), kSimplePage);
Expand All @@ -487,7 +505,8 @@ IN_PROC_BROWSER_TEST_F(LocalhostAccessBrowserTest, AdblockRuleException) {
// Add adblock rule to block localhost.
std::string test_domain = "localhost";
embedding_url_ = https_server_->GetURL(kTestEmbeddingDomain, kSimplePage);
const auto& target_url = localhost_server_->GetURL(test_domain, "/logo.png");
const auto& target_url =
localhost_server_->GetURL(test_domain, kTestTargetPath);
auto original_rule = base::StrCat({"||", test_domain, "^", "\n"});
auto exception_rule = base::StrCat({"@@||", test_domain, "^"});
auto rules = original_rule + exception_rule;
Expand Down
2 changes: 2 additions & 0 deletions components/brave_shields/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ if (!is_ios) {
"ad_block_filters_provider.h",
"ad_block_filters_provider_manager.cc",
"ad_block_filters_provider_manager.h",
"ad_block_localhost_filters_provider.cc",
"ad_block_localhost_filters_provider.h",
"ad_block_pref_service.cc",
"ad_block_pref_service.h",
"ad_block_regional_service_manager.cc",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
#include "base/files/file_path.h"
#include "base/task/thread_pool.h"
#include "brave/components/brave_shields/browser/ad_block_component_installer.h"
#include "brave/components/brave_shields/browser/ad_block_filters_provider.h"
#include "brave/components/brave_shields/browser/ad_block_filters_provider_manager.h"
#include "brave/components/brave_shields/browser/filter_list_catalog_entry.h"
#include "components/component_updater/component_updater_service.h"

Expand All @@ -24,7 +26,9 @@ AdBlockComponentFiltersProvider::AdBlockComponentFiltersProvider(
std::string component_id,
std::string base64_public_key,
std::string title)
: component_id_(component_id), component_updater_service_(cus) {
: AdBlockFiltersProvider(true),
component_id_(component_id),
component_updater_service_(cus) {
// Can be nullptr in unit tests
if (cus) {
RegisterAdBlockFiltersComponent(
Expand All @@ -42,7 +46,7 @@ AdBlockComponentFiltersProvider::AdBlockComponentFiltersProvider(
catalog_entry.base64_public_key,
catalog_entry.title) {}

AdBlockComponentFiltersProvider::~AdBlockComponentFiltersProvider() = default;
AdBlockComponentFiltersProvider::~AdBlockComponentFiltersProvider() {}

void AdBlockComponentFiltersProvider::UnregisterComponent() {
// Can be nullptr in unit tests
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,9 @@ namespace brave_shields {

AdBlockCustomFiltersProvider::AdBlockCustomFiltersProvider(
PrefService* local_state)
: local_state_(local_state) {
AdBlockFiltersProviderManager::GetInstance()->AddProvider(this);
}
: AdBlockFiltersProvider(false), local_state_(local_state) {}

AdBlockCustomFiltersProvider::~AdBlockCustomFiltersProvider() {
AdBlockFiltersProviderManager::GetInstance()->RemoveProvider(this);
}
AdBlockCustomFiltersProvider::~AdBlockCustomFiltersProvider() {}

void AdBlockCustomFiltersProvider::HideElementOnHost(
const std::string& css_selector,
Expand Down
4 changes: 3 additions & 1 deletion components/brave_shields/browser/ad_block_engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ std::string ResourceTypeToString(blink::mojom::ResourceType resource_type) {

namespace brave_shields {

AdBlockEngine::AdBlockEngine() : ad_block_client_(new adblock::Engine()) {
AdBlockEngine::AdBlockEngine(bool is_default_engine)
: ad_block_client_(new adblock::Engine()),
is_default_engine_(is_default_engine) {
DETACH_FROM_SEQUENCE(sequence_checker_);
}

Expand Down
6 changes: 5 additions & 1 deletion components/brave_shields/browser/ad_block_engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,13 @@ class AdBlockEngine : public base::SupportsWeakPtr<AdBlockEngine> {
using GetDATFileDataResult =
brave_component_updater::LoadDATFileDataResult<adblock::Engine>;

AdBlockEngine();
explicit AdBlockEngine(bool is_default_engine);
AdBlockEngine(const AdBlockEngine&) = delete;
AdBlockEngine& operator=(const AdBlockEngine&) = delete;
~AdBlockEngine();

bool IsDefaultEngine() { return is_default_engine_; }

ShivanKaul marked this conversation as resolved.
Show resolved Hide resolved
void ShouldStartRequest(const GURL& url,
blink::mojom::ResourceType resource_type,
const std::string& tab_host,
Expand Down Expand Up @@ -111,6 +113,8 @@ class AdBlockEngine : public base::SupportsWeakPtr<AdBlockEngine> {

raw_ptr<TestObserver> test_observer_ = nullptr;

bool is_default_engine_;

SEQUENCE_CHECKER(sequence_checker_);
};

Expand Down
13 changes: 12 additions & 1 deletion components/brave_shields/browser/ad_block_filters_provider.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,22 @@

#include <utility>

#include "brave/components/brave_shields/browser/ad_block_filters_provider_manager.h"

namespace brave_shields {

AdBlockFiltersProvider::AdBlockFiltersProvider(bool engine_is_default)
: engine_is_default_(engine_is_default) {
AdBlockFiltersProviderManager::GetInstance()->AddProvider(this,
engine_is_default_);
}

AdBlockFiltersProvider::AdBlockFiltersProvider() = default;

AdBlockFiltersProvider::~AdBlockFiltersProvider() = default;
AdBlockFiltersProvider::~AdBlockFiltersProvider() {
AdBlockFiltersProviderManager::GetInstance()->RemoveProvider(
this, engine_is_default_);
}

void AdBlockFiltersProvider::AddObserver(
AdBlockFiltersProvider::Observer* observer) {
Expand Down
5 changes: 4 additions & 1 deletion components/brave_shields/browser/ad_block_filters_provider.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ class AdBlockFiltersProvider {
virtual void OnChanged() = 0;
};

explicit AdBlockFiltersProvider(bool engine_is_default);
// Used by AdblockFiltersProviderManager
AdBlockFiltersProvider();
AdBlockFiltersProvider(const AdBlockFiltersProvider&) = delete;
AdBlockFiltersProvider& operator=(const AdBlockFiltersProvider&) = delete;
Expand All @@ -39,10 +41,11 @@ class AdBlockFiltersProvider {
base::WeakPtr<AdBlockFiltersProvider> AsWeakPtr();

protected:
bool engine_is_default_;

virtual void LoadDATBuffer(
base::OnceCallback<void(bool deserialize,
const DATFileDataBuffer& dat_buf)>) = 0;

void NotifyObservers();

private:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include "base/location.h"
#include "base/logging.h"
#include "base/no_destructor.h"
#include "base/notreached.h"
#include "base/task/sequenced_task_runner.h"

namespace brave_shields {
Expand Down Expand Up @@ -44,38 +45,50 @@ AdBlockFiltersProviderManager::AdBlockFiltersProviderManager() = default;
AdBlockFiltersProviderManager::~AdBlockFiltersProviderManager() = default;

void AdBlockFiltersProviderManager::AddProvider(
AdBlockFiltersProvider* provider) {
auto rv = filters_providers_.insert(provider);
AdBlockFiltersProvider* provider,
bool is_for_default_engine) {
auto rv = is_for_default_engine
? default_engine_filters_providers_.insert(provider)
: additional_engine_filters_providers_.insert(provider);
DCHECK(rv.second);
provider->AddObserver(this);
}

void AdBlockFiltersProviderManager::RemoveProvider(
AdBlockFiltersProvider* provider) {
auto it = filters_providers_.find(provider);
DCHECK(it != filters_providers_.end());
(*it)->RemoveObserver(this);
filters_providers_.erase(it);
AdBlockFiltersProvider* provider,
bool is_for_default_engine) {
auto& filters_providers = is_for_default_engine
? default_engine_filters_providers_
: additional_engine_filters_providers_;
auto it = filters_providers.find(provider);
DCHECK(it != filters_providers.end());
filters_providers.erase(it);
NotifyObservers();
}

void AdBlockFiltersProviderManager::OnChanged() {
NotifyObservers();
}

// Use LoadDATBufferForEngine instead, for Filter Provider Manager.
void AdBlockFiltersProviderManager::LoadDATBuffer(
base::OnceCallback<void(bool deserialize, const DATFileDataBuffer& dat_buf)>
cb) {
if (task_tracker_.HasTrackedTasks()) {
// There's already an in-progress load, cancel it.
task_tracker_.TryCancelAll();
}
NOTREACHED();
}
ShivanKaul marked this conversation as resolved.
Show resolved Hide resolved

void AdBlockFiltersProviderManager::LoadDATBufferForEngine(
bool is_for_default_engine,
base::OnceCallback<void(bool deserialize, const DATFileDataBuffer& dat_buf)>
cb) {
auto& filters_providers = is_for_default_engine
? default_engine_filters_providers_
: additional_engine_filters_providers_;
const auto collect_and_merge = base::BarrierCallback<DATFileDataBuffer>(
filters_providers_.size(),
filters_providers.size(),
base::BindOnce(&AdBlockFiltersProviderManager::FinishCombinating,
weak_factory_.GetWeakPtr(), std::move(cb)));
for (auto* provider : filters_providers_) {
for (auto* const provider : filters_providers) {
task_tracker_.PostTask(
base::SequencedTaskRunner::GetCurrentDefault().get(), FROM_HERE,
base::BindOnce(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,11 @@ namespace brave_shields {
// AdBlockFiltersProviderManager is both an AdBlockFiltersProvider and an
// AdBlockFiltersProvider::Observer. It is used to observe multiple provider
// sources and combine their filter lists into a single compound filter list.
// Note that AdBlockFiltersProviderManager should technically not inherit from
// AdBlockFiltersProvider since it manages multiple providers and is not a
// filters provider itself. However, SourceProviderObserver needs it to be so
// for now because AdBlockFiltersProviderManager cannot be used for combining
// DAT files.
class AdBlockFiltersProviderManager : public AdBlockFiltersProvider,
public AdBlockFiltersProvider::Observer {
public:
Expand All @@ -40,19 +45,28 @@ class AdBlockFiltersProviderManager : public AdBlockFiltersProvider,
base::OnceCallback<void(bool deserialize,
const DATFileDataBuffer& dat_buf)>) override;

void LoadDATBufferForEngine(
bool is_for_default_engine,
base::OnceCallback<void(bool deserialize,
const DATFileDataBuffer& dat_buf)> cb);

// AdBlockFiltersProvider::Observer
void OnChanged() override;

void AddProvider(AdBlockFiltersProvider* provider);
void RemoveProvider(AdBlockFiltersProvider* provider);
void AddProvider(AdBlockFiltersProvider* provider,
bool is_for_default_engine);
void RemoveProvider(AdBlockFiltersProvider* provider,
bool is_for_default_engine);

private:
friend base::NoDestructor<AdBlockFiltersProviderManager>;

void FinishCombinating(
base::OnceCallback<void(bool, const DATFileDataBuffer&)> cb,
const std::vector<DATFileDataBuffer>& results);
base::flat_set<AdBlockFiltersProvider*> filters_providers_;

base::flat_set<AdBlockFiltersProvider*> default_engine_filters_providers_;
base::flat_set<AdBlockFiltersProvider*> additional_engine_filters_providers_;

base::CancelableTaskTracker task_tracker_;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// Copyright (c) 2023 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

#include "brave/components/brave_shields/browser/ad_block_localhost_filters_provider.h"

#include <utility>
#include <vector>

#include "base/task/single_thread_task_runner.h"
#include "brave/components/brave_shields/browser/ad_block_filters_provider_manager.h"
#include "components/prefs/pref_service.h"

namespace brave_shields {

AdBlockLocalhostFiltersProvider::AdBlockLocalhostFiltersProvider()
: AdBlockFiltersProvider(true) {}

AdBlockLocalhostFiltersProvider::~AdBlockLocalhostFiltersProvider() {}

void AdBlockLocalhostFiltersProvider::LoadDATBuffer(
base::OnceCallback<void(bool deserialize, const DATFileDataBuffer& dat_buf)>
cb) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

const std::string& badfilters_for_localhost =
R"(
||0.0.0.0^$third-party,domain=~[::]|~[::ffff:0:0],badfilter
||[::]^$third-party,domain=~0.0.0.0|~[::ffff:0:0],badfilter
||[::ffff:0:0]^$third-party,domain=~0.0.0.0|~[::],badfilter
||localhost^$third-party,domain=~127.0.0.1|~[::1]|~[::ffff:7f00:1],badfilter
||127.0.0.1^$third-party,domain=~localhost|~[::1]|~[::ffff:7f00:1],badfilter
||[::1]^$third-party,domain=~localhost|~127.0.0.1|~[::ffff:7f00:1],badfilter
||[::ffff:7f00:1]^$third-party,domain=~localhost|~127.0.0.1|~[::1],badfilter
)";

auto buffer = std::vector<unsigned char>(badfilters_for_localhost.begin(),
badfilters_for_localhost.end());

// PostTask so this has an async return to match other loaders
base::SingleThreadTaskRunner::GetCurrentDefault()->PostTask(
FROM_HERE, base::BindOnce(std::move(cb), false, std::move(buffer)));
thypon marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reported by reviewdog 🐶
[semgrep] BindOnce/BindRepeating may allow callers to access objects which may already be freed in the C++ lifecycle. Verify the occurrences manually

Source: Brave

Cc @brave/sec-team @fmarier @thypon

}

void AdBlockLocalhostFiltersProvider::AddObserver(
AdBlockFiltersProvider::Observer* observer) {
AdBlockFiltersProvider::AddObserver(observer);
NotifyObservers();
}

} // namespace brave_shields
Loading