Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history 
#3239 (review)
#3239 (review)
#3239 (review)
#3239 (review)
#3239 (review)
#3239 (review)
#3239 (review)
#3239 (review)
#3239 (review)
#3239 (review)
  • Loading branch information
@fmarier
fmarier committed Aug 23, 2019
1 parent 8045692 commit 57f61a0
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 79 deletions.
34 changes: 16 additions & 18 deletions browser/net/brave_site_hacks_network_delegate_helper.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,11 @@ namespace brave {

namespace {

const std::vector<std::string> query_string_trackers = {
"fbclid", "gclid", "msclkid", "mc_eid"
};
const std::vector<const std::string>& GetQueryStringTrackers() {
static const base::NoDestructor<std::vector<const std::string>> trackers(
{"fbclid", "gclid", "msclkid", "mc_eid"});
return *trackers;
}

bool ApplyPotentialReferrerBlock(std::shared_ptr<BraveRequestInfo> ctx) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
Expand All @@ -55,17 +57,14 @@ bool ApplyPotentialReferrerBlock(std::shared_ptr<BraveRequestInfo> ctx) {
return false;
}

bool ApplyPotentialQueryStringFilter(std::shared_ptr<BraveRequestInfo> ctx) {
DCHECK_CURRENTLY_ON(BrowserThread::IO);
if (!ctx->request_url.has_query()) {
return false;
}

std::string new_query = ctx->request_url.query();
void ApplyPotentialQueryStringFilter(const GURL& request_url,
std::string* new_url_spec) {
std::string new_query = request_url.query();

bool modified = false;
for (auto tracker : query_string_trackers) {
size_t key_size, key_start;
for (auto tracker : GetQueryStringTrackers()) {
size_t key_size = 0;
size_t key_start = std::string::npos;

// Look for the tracker anywhere in the query string
// (e.g. ?...fbclid=1234...).
Expand Down Expand Up @@ -125,12 +124,8 @@ bool ApplyPotentialQueryStringFilter(std::shared_ptr<BraveRequestInfo> ctx) {
replacements.SetQuery(new_query.c_str(),
url::Component(0, new_query.size()));
}
ctx->new_url_spec =
ctx->request_url.ReplaceComponents(replacements).spec();
return true;
*new_url_spec = request_url.ReplaceComponents(replacements).spec();
}

return false;
}

} // namespace
Expand All @@ -139,7 +134,10 @@ int OnBeforeURLRequest_SiteHacksWork(
const ResponseCallback& next_callback,
std::shared_ptr<BraveRequestInfo> ctx) {
ApplyPotentialReferrerBlock(ctx);
ApplyPotentialQueryStringFilter(ctx);

if (ctx->request_url.has_query()) {
ApplyPotentialQueryStringFilter(ctx->request_url, &ctx->new_url_spec);
}
return net::OK;
}

Expand Down
121 changes: 60 additions & 61 deletions browser/net/brave_site_hacks_network_delegate_helper_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -269,85 +269,84 @@ TEST_F(BraveSiteHacksNetworkDelegateHelperTest,
}

TEST_F(BraveSiteHacksNetworkDelegateHelperTest, QueryStringUntouched) {
std::vector<GURL> urls({
GURL("https://example.com/"),
GURL("https://example.com/?"),
GURL("https://example.com/?+%20"),
GURL("https://user:pass@example.com/path/file.html?foo=1#fragment"),
GURL("http://user:pass@example.com/path/file.html?foo=1&bar=2#fragment"),
GURL("https://example.com/?file=https%3A%2F%2Fexample.com%2Ftest.pdf"),
GURL("https://example.com/?title=1+2&caption=1%202"),
GURL("https://example.com/?foo=1&&bar=2#fragment"),
GURL("https://example.com/?foo&bar=&#fragment"),
GURL("https://example.com/?foo=1&fbcid=no&gcid=no&mc_cid=no&bar=&#frag"),
GURL("https://example.com/?fbclid=&gclid&=mc_eid&msclkid="),
GURL("https://example.com/?value=fbclid=1&not-gclid=2&foo+mc_eid=3"),
GURL("https://example.com/?+fbclid=1"),
GURL("https://example.com/?%20fbclid=1"),
GURL("https://example.com/#fbclid=1"),
const std::vector<const std::string> urls({
"https://example.com/",
"https://example.com/?",
"https://example.com/?+%20",
"https://user:pass@example.com/path/file.html?foo=1#fragment",
"http://user:pass@example.com/path/file.html?foo=1&bar=2#fragment",
"https://example.com/?file=https%3A%2F%2Fexample.com%2Ftest.pdf",
"https://example.com/?title=1+2&caption=1%202",
"https://example.com/?foo=1&&bar=2#fragment",
"https://example.com/?foo&bar=&#fragment",
"https://example.com/?foo=1&fbcid=no&gcid=no&mc_cid=no&bar=&#frag",
"https://example.com/?fbclid=&gclid&=mc_eid&msclkid=",
"https://example.com/?value=fbclid=1&not-gclid=2&foo+mc_eid=3",
"https://example.com/?+fbclid=1",
"https://example.com/?%20fbclid=1",
"https://example.com/#fbclid=1",
});
std::for_each(urls.begin(), urls.end(), [this](GURL url){
for (const auto& url : urls) {
net::TestDelegate test_delegate;
std::unique_ptr<net::URLRequest> request =
context()->CreateRequest(url, net::IDLE, &test_delegate,
TRAFFIC_ANNOTATION_FOR_TESTS);
std::unique_ptr<net::URLRequest> request = context()->CreateRequest(
GURL(url), net::IDLE, &test_delegate, TRAFFIC_ANNOTATION_FOR_TESTS);

std::shared_ptr<brave::BraveRequestInfo>
brave_request_info(new brave::BraveRequestInfo());
std::shared_ptr<brave::BraveRequestInfo> brave_request_info(
new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(),
brave_request_info);
brave_request_info);
brave::ResponseCallback callback;
int ret = brave::OnBeforeURLRequest_SiteHacksWork(callback,
brave_request_info);
int ret =
brave::OnBeforeURLRequest_SiteHacksWork(callback, brave_request_info);
EXPECT_EQ(ret, net::OK);
// new_url should not be set
EXPECT_TRUE(brave_request_info->new_url_spec.empty());
EXPECT_EQ(request->url(), url);
});
EXPECT_EQ(request->url(), GURL(url));
}
}

TEST_F(BraveSiteHacksNetworkDelegateHelperTest, QueryStringFiltered) {
std::vector<std::pair<GURL, GURL> > urls({
// { original url, expected url after filtering }
{ GURL("https://example.com/?fbclid=1234"), GURL("https://example.com/") },
{ GURL("https://example.com/?fbclid=1234&"), GURL("https://example.com/") },
{ GURL("https://example.com/?&fbclid=1234"), GURL("https://example.com/") },
{ GURL("https://example.com/?gclid=1234"), GURL("https://example.com/") },
{ GURL("https://example.com/?fbclid=0&gclid=1&msclkid=a&mc_eid=a1"),
GURL("https://example.com/") },
{ GURL("https://example.com/?fbclid=&foo=1&bar=2&gclid=abc"),
GURL("https://example.com/?fbclid=&foo=1&bar=2") },
{ GURL("https://example.com/?fbclid=&foo=1&gclid=1234&bar=2"),
GURL("https://example.com/?fbclid=&foo=1&bar=2") },
{ GURL("http://u:p@example.com/path/file.html?foo=1&fbclid=abcd#fragment"),
GURL("http://u:p@example.com/path/file.html?foo=1#fragment") },
// Obscure edge cases that break most parsers:
{ GURL("https://example.com/?fbclid&foo&&gclid=2&bar=&%20"),
GURL("https://example.com/?fbclid&foo&&bar=&%20") },
{ GURL("https://example.com/?fbclid=1&1==2&=msclkid&foo=bar&&a=b=c&"),
GURL("https://example.com/?1==2&=msclkid&foo=bar&&a=b=c&") },
{ GURL("https://example.com/?fbclid=1&=2&?foo=yes&bar=2+"),
GURL("https://example.com/?=2&?foo=yes&bar=2+") },
{ GURL("https://example.com/?fbclid=1&a+b+c=some%20thing&1%202=3+4"),
GURL("https://example.com/?a+b+c=some%20thing&1%202=3+4") },
});
std::for_each(urls.begin(), urls.end(), [this](std::pair<GURL, GURL> url){
const std::vector<const std::pair<const std::string, const std::string>> urls(
{
// { original url, expected url after filtering }
{"https://example.com/?fbclid=1234", "https://example.com/"},
{"https://example.com/?fbclid=1234&", "https://example.com/"},
{"https://example.com/?&fbclid=1234", "https://example.com/"},
{"https://example.com/?gclid=1234", "https://example.com/"},
{"https://example.com/?fbclid=0&gclid=1&msclkid=a&mc_eid=a1",
"https://example.com/"},
{"https://example.com/?fbclid=&foo=1&bar=2&gclid=abc",
"https://example.com/?fbclid=&foo=1&bar=2"},
{"https://example.com/?fbclid=&foo=1&gclid=1234&bar=2",
"https://example.com/?fbclid=&foo=1&bar=2"},
{"http://u:p@example.com/path/file.html?foo=1&fbclid=abcd#fragment",
"http://u:p@example.com/path/file.html?foo=1#fragment"},
// Obscure edge cases that break most parsers:
{"https://example.com/?fbclid&foo&&gclid=2&bar=&%20",
"https://example.com/?fbclid&foo&&bar=&%20"},
{"https://example.com/?fbclid=1&1==2&=msclkid&foo=bar&&a=b=c&",
"https://example.com/?1==2&=msclkid&foo=bar&&a=b=c&"},
{"https://example.com/?fbclid=1&=2&?foo=yes&bar=2+",
"https://example.com/?=2&?foo=yes&bar=2+"},
{"https://example.com/?fbclid=1&a+b+c=some%20thing&1%202=3+4",
"https://example.com/?a+b+c=some%20thing&1%202=3+4"},
});
for (const auto& pair : urls) {
net::TestDelegate test_delegate;
std::unique_ptr<net::URLRequest> request =
context()->CreateRequest(url.first, net::IDLE, &test_delegate,
context()->CreateRequest(GURL(pair.first), net::IDLE, &test_delegate,
TRAFFIC_ANNOTATION_FOR_TESTS);

std::shared_ptr<brave::BraveRequestInfo>
brave_request_info(new brave::BraveRequestInfo());
std::shared_ptr<brave::BraveRequestInfo> brave_request_info(
new brave::BraveRequestInfo());
brave::BraveRequestInfo::FillCTXFromRequest(request.get(),
brave_request_info);
brave_request_info);
brave::ResponseCallback callback;
int ret = brave::OnBeforeURLRequest_SiteHacksWork(callback,
brave_request_info);
int ret =
brave::OnBeforeURLRequest_SiteHacksWork(callback, brave_request_info);
EXPECT_EQ(ret, net::OK);
EXPECT_STREQ(brave_request_info->new_url_spec.c_str(),
url.second.spec().c_str());
});
EXPECT_EQ(brave_request_info->new_url_spec, pair.second);
}
}

} // namespace

0 comments on commit 57f61a0

Please sign in to comment.