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.

Sign up for GitHub

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

Support removeparam #15943

Merged
merged 5 commits into from
Dec 1, 2022
Merged

Support removeparam #15943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e4ca144
support removeparam
antonok-edm Nov 14, 2022
c0f2abe
check/apply removeparam in domain blocking throttle
antonok-edm Nov 14, 2022
49cce79
add tests
antonok-edm Nov 15, 2022
f933b42
restrict query parameter stripping to GET, HEAD, and OPTIONS requests
antonok-edm Nov 16, 2022
b1f03d4
ensure URLs are valid
antonok-edm Nov 17, 2022
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
77 changes: 77 additions & 0 deletions browser/brave_shields/ad_block_service_browsertest.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1674,6 +1674,83 @@ IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, RedirectWithoutBlockIsNoop) {
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 1ULL);
}

std::unique_ptr<net::test_server::HttpResponse> NoParamHandler(
const net::test_server::HttpRequest& request) {
const GURL request_url = request.GetURL();

auto http_response = std::make_unique<net::test_server::BasicHttpResponse>();
http_response->set_code(net::HttpStatusCode::HTTP_OK);

if (request_url.has_query()) {
// Should not happen, abort test
CHECK(false);
return nullptr;
} else {
std::string body =
"<html><head><script>window.success = "
"true;</script></head><body><p>test</p></body></html>";
http_response->set_content(body);
http_response->set_content_type("text/html");
return http_response;
}
}

// `$removeparam` should be respected for subresource requests
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, RemoveparamSubresource) {
ASSERT_TRUE(InstallDefaultAdBlockExtension());
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL);

UpdateAdBlockInstanceWithRules("*$subdocument,removeparam=evil");

GURL tab_url =
embedded_test_server()->GetURL("b.com", "/cosmetic_filtering.html");
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), tab_url));

content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();

GURL frame_url = embedded_test_server()->GetURL(
"frame.com", "/cosmetic_frame.html?evil=true&test=true");
content::NavigateIframeToURL(contents, "iframe", frame_url);

ASSERT_EQ(nullptr,
content::FrameMatchingPredicateOrNullptr(
contents->GetPrimaryPage(),
base::BindRepeating(content::FrameHasSourceUrl, frame_url)));

GURL redirected_frame_url = embedded_test_server()->GetURL(
"frame.com", "/cosmetic_frame.html?test=true");

content::RenderFrameHost* inner_frame = content::FrameMatchingPredicate(
contents->GetPrimaryPage(),
base::BindRepeating(content::FrameHasSourceUrl, redirected_frame_url));

ASSERT_EQ("?test=true", EvalJs(inner_frame, "window.location.search"));
}

// `$removeparam` should be respected for top-level navigations
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, RemoveparamTopLevelNavigation) {
ASSERT_TRUE(InstallDefaultAdBlockExtension());
EXPECT_EQ(browser()->profile()->GetPrefs()->GetUint64(kAdsBlocked), 0ULL);

UpdateAdBlockInstanceWithRules("*$document,removeparam=evil");

dynamic_server_.RegisterRequestHandler(base::BindRepeating(&NoParamHandler));
ASSERT_TRUE(dynamic_server_.Start());

GURL original_url = dynamic_server_.GetURL("/?evil=true");
GURL landing_url = dynamic_server_.GetURL("/");
ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), original_url));
content::WebContents* contents =
browser()->tab_strip_model()->GetActiveWebContents();
ASSERT_TRUE(content::WaitForLoadStop(contents));
EXPECT_EQ(contents->GetLastCommittedURL(), landing_url);

ASSERT_EQ(true, EvalJs(contents, "window.success"));

ASSERT_TRUE(dynamic_server_.ShutdownAndWaitUntilComplete());
}

// Verify that scripts violating a Content Security Policy from a `$csp` rule
// are not loaded.
IN_PROC_BROWSER_TEST_F(AdBlockServiceTest, CspRule) {
Expand Down
1 change: 1 addition & 0 deletions browser/brave_shields/ad_block_service_browsertest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class AdBlockServiceTest : public extensions::ExtensionBrowserTest {
source_providers_;

net::SpawnedTestServer ws_server_;
net::EmbeddedTestServer dynamic_server_;
};

#endif // BRAVE_BROWSER_BRAVE_SHIELDS_AD_BLOCK_SERVICE_BROWSERTEST_H_
11 changes: 10 additions & 1 deletion browser/net/brave_ad_block_tp_network_delegate_helper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,12 +197,21 @@ EngineFlags ShouldBlockRequestOnTaskRunner(
url::Origin::CreateFromNormalizedTuple("https", "youtube.com", 80),
net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);

std::string rewritten_url;

SCOPED_UMA_HISTOGRAM_TIMER("Brave.Adblock.ShouldBlockRequest");
g_brave_browser_process->ad_block_service()->ShouldStartRequest(
url_to_check, ctx->resource_type, source_host,
ctx->aggressive_blocking || force_aggressive,
&previous_result.did_match_rule, &previous_result.did_match_exception,
&previous_result.did_match_important, &ctx->mock_data_url);
&previous_result.did_match_important, &ctx->mock_data_url,
&rewritten_url);

if (GURL(rewritten_url).is_valid() &&
(ctx->method == "GET" || ctx->method == "HEAD" ||
ctx->method == "OPTIONS")) {
ctx->new_url_spec = rewritten_url;
Copy link
Contributor

Choose a reason for hiding this comment

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

check validity of the url?

Copy link
Member

Choose a reason for hiding this comment

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

Would it also be worth checking that the scheme is HTTP/HTTPS?

Copy link
Collaborator Author

@antonok-edm antonok-edm Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

updated

@fmarier I think we want it to be able to apply to WS/WSS as well. But anyways, unsupported schemes are already not even passed to the adblock engine so it should be alright here.

}

if (previous_result.did_match_important ||
(previous_result.did_match_rule &&
Expand Down
4 changes: 2 additions & 2 deletions build/rust/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion components/adblock_rust_ffi/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ authors = ["Brian R. Bondy <netzen@gmail.com>"]
edition = "2018"

[dependencies]
adblock = { version = "0.5.8", default-features = false, features = ["full-regex-handling", "object-pooling", "unsync-regex-caching"] }
adblock = { version = "0.6.0", default-features = false, features = ["full-regex-handling", "object-pooling", "unsync-regex-caching"] }
serde_json = "1.0"
libc = "0.2"

Expand Down
3 changes: 2 additions & 1 deletion components/adblock_rust_ffi/src/lib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,8 @@ void engine_match(struct C_Engine* engine,
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
char** redirect);
char** redirect,
char** rewritten_url);

/**
* Returns any CSP directives that should be added to a subdocument or document
Expand Down
19 changes: 9 additions & 10 deletions components/adblock_rust_ffi/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
use adblock::blocker::Redirection;
use adblock::engine::Engine;
use adblock::lists::FilterListMetadata;
use adblock::resources::{MimeType, Resource, ResourceType};
Expand Down Expand Up @@ -130,6 +129,7 @@ pub unsafe extern "C" fn engine_match(
did_match_exception: *mut bool,
did_match_important: *mut bool,
redirect: *mut *mut c_char,
rewritten_url: *mut *mut c_char,
) {
let url = CStr::from_ptr(url).to_str().unwrap();
let host = CStr::from_ptr(host).to_str().unwrap();
Expand All @@ -151,15 +151,14 @@ pub unsafe extern "C" fn engine_match(
*did_match_rule |= blocker_result.matched;
*did_match_exception |= blocker_result.exception.is_some();
*did_match_important |= blocker_result.important;
*redirect = match blocker_result.redirect {
Some(Redirection::Resource(x)) => match CString::new(x) {
Ok(y) => y.into_raw(),
_ => ptr::null_mut(),
},
// Ignore `redirect-url` for now.
Some(Redirection::Url(_)) => ptr::null_mut(),
None => ptr::null_mut(),
};
*redirect = blocker_result
.redirect
.and_then(|x| CString::new(x).map(CString::into_raw).ok())
.unwrap_or(ptr::null_mut());
*rewritten_url = blocker_result
.rewritten_url
.and_then(|x| CString::new(x).map(CString::into_raw).ok())
.unwrap_or(ptr::null_mut());
}

/// Returns any CSP directives that should be added to a subdocument or document request's response
Expand Down
12 changes: 10 additions & 2 deletions components/adblock_rust_ffi/src/wrapper.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,25 @@ void Engine::matches(const std::string& url,
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* redirect) {
std::string* redirect,
std::string* rewritten_url) {
char* redirect_char_ptr = nullptr;
char* rewritten_url_ptr = nullptr;
engine_match(raw, url.c_str(), host.c_str(), tab_host.c_str(), is_third_party,
resource_type.c_str(), did_match_rule, did_match_exception,
did_match_important, &redirect_char_ptr);
did_match_important, &redirect_char_ptr, &rewritten_url_ptr);
if (redirect_char_ptr) {
if (redirect) {
*redirect = redirect_char_ptr;
}
c_char_buffer_destroy(redirect_char_ptr);
}
if (rewritten_url_ptr) {
if (rewritten_url) {
*rewritten_url = rewritten_url_ptr;
}
c_char_buffer_destroy(rewritten_url_ptr);
}
}

std::string Engine::getCspDirectives(const std::string& url,
Expand Down
3 changes: 2 additions & 1 deletion components/adblock_rust_ffi/src/wrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ class ADBLOCK_EXPORT Engine {
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* redirect);
std::string* redirect,
std::string* rewritten_url);
std::string getCspDirectives(const std::string& url,
const std::string& host,
const std::string& tab_host,
Expand Down
5 changes: 3 additions & 2 deletions components/brave_shields/browser/ad_block_engine.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ void AdBlockEngine::ShouldStartRequest(const GURL& url,
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* mock_data_url) {
std::string* mock_data_url,
std::string* rewritten_url) {
// Determine third-party here so the library doesn't need to figure it out.
// CreateFromNormalizedTuple is needed because SameDomainOrHost needs
// a URL or origin and not a string to a host name.
Expand All @@ -120,7 +121,7 @@ void AdBlockEngine::ShouldStartRequest(const GURL& url,
ad_block_client_->matches(url.spec(), url.host(), tab_host, is_third_party,
ResourceTypeToString(resource_type), did_match_rule,
did_match_exception, did_match_important,
mock_data_url);
mock_data_url, rewritten_url);

// LOG(ERROR) << "AdBlockEngine::ShouldStartRequest(), host: "
// << tab_host
Expand Down
3 changes: 2 additions & 1 deletion components/brave_shields/browser/ad_block_engine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ class AdBlockEngine : public base::SupportsWeakPtr<AdBlockEngine> {
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* mock_data_url);
std::string* mock_data_url,
std::string* rewritten_url);
absl::optional<std::string> GetCspDirectives(
const GURL& url,
blink::mojom::ResourceType resource_type,
Expand Down
12 changes: 9 additions & 3 deletions components/brave_shields/browser/ad_block_regional_service_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,13 +169,19 @@ void AdBlockRegionalServiceManager::ShouldStartRequest(
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* mock_data_url) {
std::string* mock_data_url,
std::string* rewritten_url) {
base::AutoLock lock(regional_services_lock_);

GURL request_url;

for (const auto& regional_service : regional_services_) {
request_url =
rewritten_url && !rewritten_url->empty() ? GURL(*rewritten_url) : url;
regional_service.second->ShouldStartRequest(
url, resource_type, tab_host, aggressive_blocking, did_match_rule,
did_match_exception, did_match_important, mock_data_url);
request_url, resource_type, tab_host, aggressive_blocking,
did_match_rule, did_match_exception, did_match_important, mock_data_url,
rewritten_url);
if (did_match_important && *did_match_important) {
return;
}
Expand Down
3 changes: 2 additions & 1 deletion components/brave_shields/browser/ad_block_regional_service_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ class AdBlockRegionalServiceManager
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* mock_data_url);
std::string* mock_data_url,
std::string* rewritten_url);
absl::optional<std::string> GetCspDirectives(
const GURL& url,
blink::mojom::ResourceType resource_type,
Expand Down
31 changes: 22 additions & 9 deletions components/brave_shields/browser/ad_block_service.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,39 +133,52 @@ void AdBlockService::ShouldStartRequest(
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* mock_data_url) {
std::string* mock_data_url,
std::string* rewritten_url) {
DCHECK(GetTaskRunner()->RunsTasksInCurrentSequence());

GURL request_url;

if (aggressive_blocking ||
base::FeatureList::IsEnabled(
brave_shields::features::kBraveAdblockDefault1pBlocking) ||
!SameDomainOrHost(
url, url::Origin::CreateFromNormalizedTuple("https", tab_host, 80),
net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES)) {
request_url =
rewritten_url && !rewritten_url->empty() ? GURL(*rewritten_url) : url;
default_service()->ShouldStartRequest(
url, resource_type, tab_host, aggressive_blocking, did_match_rule,
did_match_exception, did_match_important, mock_data_url);
request_url, resource_type, tab_host, aggressive_blocking,
did_match_rule, did_match_exception, did_match_important, mock_data_url,
rewritten_url);
if (did_match_important && *did_match_important) {
return;
}
}

request_url =
rewritten_url && !rewritten_url->empty() ? GURL(*rewritten_url) : url;
regional_service_manager()->ShouldStartRequest(
url, resource_type, tab_host, aggressive_blocking, did_match_rule,
did_match_exception, did_match_important, mock_data_url);
request_url, resource_type, tab_host, aggressive_blocking, did_match_rule,
did_match_exception, did_match_important, mock_data_url, rewritten_url);
if (did_match_important && *did_match_important) {
return;
}

request_url =
rewritten_url && !rewritten_url->empty() ? GURL(*rewritten_url) : url;
subscription_service_manager()->ShouldStartRequest(
url, resource_type, tab_host, aggressive_blocking, did_match_rule,
did_match_exception, did_match_important, mock_data_url);
request_url, resource_type, tab_host, aggressive_blocking, did_match_rule,
did_match_exception, did_match_important, mock_data_url, rewritten_url);
if (did_match_important && *did_match_important) {
return;
}

request_url =
rewritten_url && !rewritten_url->empty() ? GURL(*rewritten_url) : url;
custom_filters_service()->ShouldStartRequest(
url, resource_type, tab_host, aggressive_blocking, did_match_rule,
did_match_exception, did_match_important, mock_data_url);
request_url, resource_type, tab_host, aggressive_blocking, did_match_rule,
did_match_exception, did_match_important, mock_data_url, rewritten_url);
}

absl::optional<std::string> AdBlockService::GetCspDirectives(
Expand Down
3 changes: 2 additions & 1 deletion components/brave_shields/browser/ad_block_service.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,8 @@ class AdBlockService {
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* mock_data_url);
std::string* mock_data_url,
std::string* rewritten_url);
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should stop adding new params to this method and introduce a structure instead. Otherwise, it goes out of control - there is no single line of comment describing what is for what, and every new change touches a dozen of irrelevant files

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @bridiver

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally we just need brave/brave-browser#5461, which I'm hoping to get to soon:tm:

or definitely at least before adding any further arguments to these methods

absl::optional<std::string> GetCspDirectives(
const GURL& url,
blink::mojom::ResourceType resource_type,
Expand Down
13 changes: 10 additions & 3 deletions components/brave_shields/browser/ad_block_subscription_service_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,14 +511,21 @@ void AdBlockSubscriptionServiceManager::ShouldStartRequest(
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* mock_data_url) {
std::string* mock_data_url,
std::string* rewritten_url) {
base::AutoLock lock(subscription_services_lock_);

GURL request_url;

for (const auto& subscription_service : subscription_services_) {
auto info = GetInfo(subscriptions_, subscription_service.first);
if (info && info->enabled) {
request_url =
rewritten_url && !rewritten_url->empty() ? GURL(*rewritten_url) : url;
subscription_service.second->ShouldStartRequest(
url, resource_type, tab_host, aggressive_blocking, did_match_rule,
did_match_exception, did_match_important, mock_data_url);
request_url, resource_type, tab_host, aggressive_blocking,
did_match_rule, did_match_exception, did_match_important,
mock_data_url, rewritten_url);
if (did_match_important && *did_match_important) {
return;
}
Expand Down
3 changes: 2 additions & 1 deletion components/brave_shields/browser/ad_block_subscription_service_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,8 @@ class AdBlockSubscriptionServiceManager {
bool* did_match_rule,
bool* did_match_exception,
bool* did_match_important,
std::string* mock_data_url);
std::string* mock_data_url,
std::string* rewritten_url);
void EnableTag(const std::string& tag, bool enabled);
void AddResources(const std::string& resources);

Expand Down
Loading