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 an explicitcancel ad-block filter option #1981

Merged
merged 3 commits into from
Mar 19, 2019
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DEPS
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use_relative_paths = True

deps = {
"vendor/ad-block": "https://github.com/brave/ad-block.git@74b167c8b49c33f3f61c15dafa2ffde953a25653",
"vendor/ad-block": "https://github.com/brave/ad-block.git@e54c59fe288d8f08de683b8f4f320a4c7bead4eb",
"vendor/autoplay-whitelist": "https://github.com/brave/autoplay-whitelist.git@458053a3c95b403cbe0872f289a2aafa106ee9d8",
"vendor/extension-whitelist": "https://github.com/brave/extension-whitelist.git@463e5e4e06e0ca84927176e8c72f6076ae9b6829",
"vendor/tracking-protection": "https://github.com/brave/tracking-protection.git@29b1f86b11a8c7438fd7d57b446a77a84946712a",
Expand Down
11 changes: 7 additions & 4 deletions browser/net/brave_ad_block_tp_network_delegate_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -98,22 +98,25 @@ void OnBeforeURLRequestAdBlockTPOnTaskRunner(
std::string tab_host = ctx->tab_origin.host();
if (!g_brave_browser_process->ad_block_service()->ShouldStartRequest(
ctx->request_url, ctx->resource_type, tab_host,
&did_match_exception)) {
&did_match_exception, &ctx->cancel_request_explicitly)) {
ctx->blocked_by = kAdBlocked;
} else if (!did_match_exception &&
!g_brave_browser_process->ad_block_regional_service()
->ShouldStartRequest(ctx->request_url, ctx->resource_type,
tab_host, &did_match_exception)) {
tab_host, &did_match_exception,
&ctx->cancel_request_explicitly)) {
ctx->blocked_by = kAdBlocked;
} else if (!did_match_exception &&
!g_brave_browser_process->ad_block_custom_filters_service()
->ShouldStartRequest(ctx->request_url, ctx->resource_type,
tab_host, &did_match_exception)) {
tab_host, &did_match_exception,
&ctx->cancel_request_explicitly)) {
ctx->blocked_by = kAdBlocked;
} else if (!did_match_exception &&
!g_brave_browser_process->tracking_protection_service()
->ShouldStartRequest(ctx->request_url, ctx->resource_type,
tab_host, &did_match_exception)) {
tab_host, &did_match_exception,
&ctx->cancel_request_explicitly)) {
ctx->blocked_by = kTrackerBlocked;
}
}
Expand Down
5 changes: 5 additions & 0 deletions browser/net/brave_network_delegate_base.cc
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,11 @@ void BraveNetworkDelegateBase::RunNextCallback(
ctx->blocked_by == brave::kTrackerBlocked) {
// We are going to intercept this request and block it later in the
// network stack.
if (ctx->cancel_request_explicitly) {
RunCallbackForRequestIdentifier(ctx->request_identifier,
net::ERR_ABORTED);
return;
}
request->SetExtraRequestHeaderByName("X-Brave-Block", "", true);
}
rv = ChromeNetworkDelegate::OnBeforeURLRequest(
Expand Down
1 change: 1 addition & 0 deletions browser/net/url_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ struct BraveRequestInfo {
BraveNetworkDelegateEventType event_type = kUnknownEventType;
const base::ListValue* referral_headers_list = nullptr;
BlockedBy blocked_by = kNotBlocked;
bool cancel_request_explicitly = false;
// Default to invalid type for resource_type, so delegate helpers
// can properly detect that the info couldn't be obtained.
content::ResourceType resource_type = content::RESOURCE_TYPE_LAST_TYPE;
Expand Down
9 changes: 7 additions & 2 deletions components/brave_shields/browser/ad_block_base_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ void AdBlockBaseService::Cleanup() {

bool AdBlockBaseService::ShouldStartRequest(const GURL& url,
content::ResourceType resource_type, const std::string& tab_host,
bool* did_match_exception) {
bool* did_match_exception, bool* cancel_request_explicitly) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
FilterOption current_option = ResourceTypeToFilterOption(resource_type);

Expand All @@ -134,10 +134,15 @@ bool AdBlockBaseService::ShouldStartRequest(const GURL& url,
current_option = static_cast<FilterOption>(current_option | FOThirdParty);
}

Filter* matching_filter = nullptr;
Filter* matching_exception_filter = nullptr;
if (ad_block_client_->matches(url.spec().c_str(),
current_option, tab_host.c_str(), nullptr,
current_option, tab_host.c_str(), &matching_filter,
&matching_exception_filter)) {
if (matching_filter && cancel_request_explicitly &&
(matching_filter->filterOption & FOExplicitCancel)) {
*cancel_request_explicitly = true;
}
// We'd only possibly match an exception filter if we're returning true.
*did_match_exception = false;
// LOG(ERROR) << "AdBlockBaseService::ShouldStartRequest(), host: "
Expand Down
3 changes: 2 additions & 1 deletion components/brave_shields/browser/ad_block_base_service.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ class AdBlockBaseService : public BaseBraveShieldsService {
~AdBlockBaseService() override;

bool ShouldStartRequest(const GURL &url, content::ResourceType resource_type,
const std::string& tab_host, bool* matching_exception_filter) override;
const std::string& tab_host, bool* did_match_exception,
bool* cancel_request_explicitly) override;
void EnableTag(const std::string& tag, bool enabled);

protected:
Expand Down
94 changes: 58 additions & 36 deletions components/brave_shields/browser/ad_block_service_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdsGetBlockedByDefaultBlocker) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 1, 0, 0);"
"setExpectations(0, 1, 0, 0, 0, 0);"
"addImage('ad_banner.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -262,7 +262,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(1, 0, 0, 0);"
"setExpectations(1, 0, 0, 0, 0, 0);"
"addImage('logo.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -283,7 +283,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdsGetBlockedByCustomBlocker) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 1, 0, 0);"
"setExpectations(0, 1, 0, 0, 0, 0);"
"addImage('ad_banner.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -307,7 +307,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(1, 0, 0, 0);"
"setExpectations(1, 0, 0, 0, 0, 0);"
"addImage('logo.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand Down Expand Up @@ -335,7 +335,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdsGetBlockedByRegionalBlocker) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 1, 0, 0);"
"setExpectations(0, 1, 0, 0, 0, 0);"
"addImage('ad_fr.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand Down Expand Up @@ -364,7 +364,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(1, 0, 0, 0);"
"setExpectations(1, 0, 0, 0, 0, 0);"
"addImage('logo.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand Down Expand Up @@ -398,7 +398,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 1, 0, 0);"
"setExpectations(0, 1, 0, 0, 0, 0);"
"addImage('v4_specific_banner.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -421,7 +421,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, TwoSameAdsGetCountedAsOne) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 0, 1, 2);"
"setExpectations(0, 0, 0, 1, 2, 0);"
"xhr('adbanner.js');"
"xhr('normal.js');"
"xhr('adbanner.js')",
Expand All @@ -445,7 +445,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, TwoDiffAdsGetCountedAsTwo) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 0, 1, 2);"
"setExpectations(0, 0, 0, 1, 2, 0);"
"xhr('adbanner.js?1');"
"xhr('normal.js');"
"xhr('adbanner.js?2')",
Expand All @@ -469,7 +469,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, NewTabContinuesToBlock) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 0, 0, 1);"
"setExpectations(0, 0, 0, 0, 1, 0);"
"xhr('adbanner.js');",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -480,7 +480,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, NewTabContinuesToBlock) {

as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(0, 0, 0, 1);"
"setExpectations(0, 0, 0, 0, 1, 0);"
"xhr('adbanner.js');",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -504,7 +504,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, SubFrame) {

bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents->GetAllFrames()[1],
"setExpectations(0, 0, 0, 1);"
"setExpectations(0, 0, 0, 0, 1, 0);"
"xhr('adbanner.js?1');",
&as_expected));
EXPECT_TRUE(as_expected);
Expand Down Expand Up @@ -544,7 +544,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,
browser()->tab_strip_model()->GetActiveWebContents();
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
"setExpectations(1, 0, 0, 0);"
"setExpectations(1, 0, 0, 0, 0, 0);"
"addImage('ad_fr.png')",
&as_expected));
EXPECT_TRUE(as_expected);
Expand All @@ -570,11 +570,11 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,
GURL test_url = embedded_test_server()->GetURL("365dm.com", "/logo.png");
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
base::StringPrintf(
"setExpectations(0, 1, 0, 0);"
"addImage('%s')",
test_url.spec().c_str()),
&as_expected));
base::StringPrintf(
"setExpectations(0, 1, 0, 0, 0, 0);"
"addImage('%s')",
test_url.spec().c_str()),
&as_expected));
EXPECT_TRUE(as_expected);
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kTrackersBlocked),
1ULL);
Expand All @@ -596,11 +596,11 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,
GURL test_url = embedded_test_server()->GetURL("365dm.com", "/logo.png");
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
base::StringPrintf(
"setExpectations(1, 0, 0, 0);"
"addImage('%s')",
test_url.spec().c_str()),
&as_expected));
base::StringPrintf(
"setExpectations(1, 0, 0, 0, 0, 0);"
"addImage('%s')",
test_url.spec().c_str()),
&as_expected));
EXPECT_TRUE(as_expected);
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kTrackersBlocked),
0ULL);
Expand All @@ -620,11 +620,11 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, AdBlockThirdPartyWorksByETLDP1) {
browser()->tab_strip_model()->GetActiveWebContents();
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
base::StringPrintf(
"setExpectations(1, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
base::StringPrintf(
"setExpectations(1, 0, 0, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
EXPECT_TRUE(as_expected);
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL);
}
Expand All @@ -643,11 +643,11 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest,
browser()->tab_strip_model()->GetActiveWebContents();
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
base::StringPrintf(
"setExpectations(0, 1, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
base::StringPrintf(
"setExpectations(0, 1, 0, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
EXPECT_TRUE(as_expected);
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL);
}
Expand All @@ -666,7 +666,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, BlockNYP) {
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
base::StringPrintf(
"setExpectations(0, 1, 0, 0);"
"setExpectations(0, 1, 0, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
Expand All @@ -692,7 +692,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, SocialButttonAdBLockTagTest) {
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
base::StringPrintf(
"setExpectations(0, 1, 0, 0);"
"setExpectations(0, 1, 0, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
Expand All @@ -717,7 +717,7 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, SocialButttonAdBlockDiffTagTest) {
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
base::StringPrintf(
"setExpectations(1, 0, 0, 0);"
"setExpectations(1, 0, 0, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
Expand Down Expand Up @@ -772,3 +772,25 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, TagPrefsControlTags) {
AssertTagExists(brave_shields::kTwitterEmbeds, true);
AssertTagExists(brave_shields::kLinkedInEmbeds, false);
}

// Make sure that cancelrequest actually blocks
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CancelRequestOptionTest) {
AddRulesToAdBlock("logo.png$explicitcancel");
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL);
GURL tab_url = embedded_test_server()->GetURL("b.com",
kAdBlockTestPage);
GURL resource_url =
embedded_test_server()->GetURL("example.com", "/logo.png");
ui_test_utils::NavigateToURL(browser(), tab_url);
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
bool as_expected = false;
ASSERT_TRUE(ExecuteScriptAndExtractBool(contents,
base::StringPrintf(
"setExpectations(0, 0, 1, 0, 0, 0);"
"addImage('%s')",
resource_url.spec().c_str()),
&as_expected));
EXPECT_TRUE(as_expected);
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL);
}
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,12 @@ void BaseBraveShieldsService::Stop() {
bool BaseBraveShieldsService::ShouldStartRequest(const GURL& url,
content::ResourceType resource_type,
const std::string& tab_host,
bool* did_match_exception) {
bool* did_match_exception,
bool* cancel_request_explicitly) {
if (did_match_exception) {
*did_match_exception = false;
}
// Intentionally don't set cancel_request_explicitly
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ class BaseBraveShieldsService : public BraveComponentExtension {
virtual bool ShouldStartRequest(const GURL& url,
content::ResourceType resource_type,
const std::string& tab_host,
bool* did_match_exception);
bool* did_match_exception,
bool* cancel_request_explicitly);
virtual scoped_refptr<base::SequencedTaskRunner> GetTaskRunner();

protected:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,14 @@ TrackingProtectionService::~TrackingProtectionService() {
bool TrackingProtectionService::ShouldStartRequest(const GURL& url,
content::ResourceType resource_type,
const std::string &tab_host,
bool* matching_exception_filter) {
bool* matching_exception_filter,
bool* cancel_request_explicitly) {
// There are no exceptions in the TP service, but exceptions are
// combined with brave/ad-block.
if (matching_exception_filter) {
*matching_exception_filter = false;
}
// Intentionally don't set cancel_request_explicitly
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
std::string host = url.host();
if (!tracking_protection_client_->matchesTracker(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ class TrackingProtectionService : public BaseLocalDataFilesObserver {
bool ShouldStartRequest(const GURL& spec,
content::ResourceType resource_type,
const std::string& tab_host,
bool* matching_exception_filter);
bool* matching_exception_filter,
bool* cancel_request_explicitly);
scoped_refptr<base::SequencedTaskRunner> GetTaskRunner();

// implementation of BaseLocalDataFilesObserver
Expand Down
Loading