Also match against the query string if present.

BUG=none
TEST=BlacklistTest.QueryStringMatch

Review URL: http://codereview.chromium.org/523137

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@36013 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 107 insertions, 24 deletions

diff --git a/chrome/browser/privacy_blacklist/blacklist.cc b/chrome/browser/privacy_blacklist/blacklist.cc
index 581f65a..ea9dc19 100644
--- a/chrome/browser/privacy_blacklist/blacklist.cc
+++ b/chrome/browser/privacy_blacklist/blacklist.cc
@@ -60,6 +60,7 @@ unsigned int Blacklist::String2Attribute(const std::string& s) {
   return 0;
 }
 
+// static
 bool Blacklist::Matches(const std::string& pattern, const std::string& url) {
   if (pattern.size() > url.size())
     return false;
@@ -178,7 +179,7 @@ void Blacklist::AddProvider(Provider* provider) {
 
 // Returns a pointer to the Blacklist-owned entry which matches the given
 // URL. If no matching Entry is found, returns null.
-Blacklist::Match* Blacklist::findMatch(const GURL& url) const {
+Blacklist::Match* Blacklist::FindMatch(const GURL& url) const {
   // Never match something which is not http, https or ftp.
   // TODO(idanan): Investigate if this would be an inclusion test instead of an
   // exclusion test and if there are other schemes to test for.
@@ -186,10 +187,11 @@ Blacklist::Match* Blacklist::findMatch(const GURL& url) const {
       !url.SchemeIs(chrome::kHttpsScheme) &&
       !url.SchemeIs(chrome::kFtpScheme))
     return 0;
+  std::string url_spec = GetURLAsLookupString(url);
   Match* match = NULL;
   for (EntryList::const_iterator i = blacklist_.begin();
        i != blacklist_.end(); ++i) {
-    if (Matches((*i)->pattern(), url.host() + url.path())) {
+    if (Matches((*i)->pattern(), url_spec)) {
       if (!match)
         match = new Match;
       match->AddEntry(i->get());
@@ -198,6 +200,15 @@ Blacklist::Match* Blacklist::findMatch(const GURL& url) const {
   return match;
 }
 
+// static
+std::string Blacklist::GetURLAsLookupString(const GURL& url) {
+  std::string url_spec = url.host() + url.path();
+  if (!url.query().empty())
+    url_spec = url_spec + "?" + url.query();
+
+  return url_spec;
+}
+
 std::string Blacklist::StripCookies(const std::string& header) {
   return net::HttpUtil::StripHeaders(header, cookie_headers, 2);
 }
diff --git a/chrome/browser/privacy_blacklist/blacklist.h b/chrome/browser/privacy_blacklist/blacklist.h
index b4d26d0..44df410 100644
--- a/chrome/browser/privacy_blacklist/blacklist.h
+++ b/chrome/browser/privacy_blacklist/blacklist.h
@@ -177,7 +177,7 @@ class Blacklist {
   // Returns a pointer to a Match structure holding all matching entries.
   // If no matching Entry is found, returns null. Ownership belongs to the
   // caller.
-  Match* findMatch(const GURL&) const;
+  Match* FindMatch(const GURL&) const;
 
   // Helper to remove cookies from a header.
   static std::string StripCookies(const std::string&);
@@ -186,14 +186,17 @@ class Blacklist {
   static std::string StripCookieExpiry(const std::string&);
 
  private:
+  // Converts a GURL into the string to match against.
+  static std::string GetURLAsLookupString(const GURL& url);
+
   // Matches a pattern to a core URL which is host/path with all the other
-  // optional parts (scheme, user, password, port) stripped away. Used only
-  // internally but made static so that access can be given to tests.
+  // optional parts (scheme, user, password, port) stripped away.
   static bool Matches(const std::string& pattern, const std::string& url);
 
   EntryList blacklist_;
   ProviderList providers_;
 
+  FRIEND_TEST(BlacklistTest, Generic);
   FRIEND_TEST(BlacklistTest, PatternMatch);
   DISALLOW_COPY_AND_ASSIGN(Blacklist);
 };
diff --git a/chrome/browser/privacy_blacklist/blacklist_interceptor.cc b/chrome/browser/privacy_blacklist/blacklist_interceptor.cc
index 79c47fb..2a571e9 100644
--- a/chrome/browser/privacy_blacklist/blacklist_interceptor.cc
+++ b/chrome/browser/privacy_blacklist/blacklist_interceptor.cc
@@ -81,7 +81,7 @@ class URLRequestBlacklistJob : public URLRequestSimpleJob {
     const BlacklistManager* blacklist_manager =
         request_info_.GetBlacklistManager();
     const Blacklist* blacklist = blacklist_manager->GetCompiledBlacklist();
-    scoped_ptr<Blacklist::Match> match(blacklist->findMatch(request_->url()));
+    scoped_ptr<Blacklist::Match> match(blacklist->FindMatch(request_->url()));
     const Blacklist::Entry* entry = NULL;
     if (match->attributes() & Blacklist::kBlockAll) {
       for (std::vector<const Blacklist::Entry*>::const_iterator i =
@@ -123,7 +123,7 @@ URLRequestJob* BlacklistInterceptor::MaybeIntercept(URLRequest* request) {
   const BlacklistManager* blacklist_manager =
       request_info->GetBlacklistManager();
   const Blacklist* blacklist = blacklist_manager->GetCompiledBlacklist();
-  scoped_ptr<Blacklist::Match> match(blacklist->findMatch(request->url()));
+  scoped_ptr<Blacklist::Match> match(blacklist->FindMatch(request->url()));
 
   if (!match.get()) {
     // Nothing is blacklisted for this request. Do not intercept.
diff --git a/chrome/browser/privacy_blacklist/blacklist_io_unittest.cc b/chrome/browser/privacy_blacklist/blacklist_io_unittest.cc
index 20607bf..e92e120 100644
--- a/chrome/browser/privacy_blacklist/blacklist_io_unittest.cc
+++ b/chrome/browser/privacy_blacklist/blacklist_io_unittest.cc
@@ -25,17 +25,19 @@ TEST(BlacklistIOTest, Generic) {
   std::string error_string;
   ASSERT_TRUE(BlacklistIO::ReadText(&blacklist, input, &error_string));
   EXPECT_TRUE(error_string.empty());
-  
+
   const Blacklist::EntryList entries(blacklist.entries_begin(),
                                      blacklist.entries_end());
-  ASSERT_EQ(5U, entries.size());
+  ASSERT_EQ(7U, entries.size());
 
   EXPECT_EQ("@", entries[0]->pattern());
   EXPECT_EQ("@poor-security-site.com", entries[1]->pattern());
   EXPECT_EQ("@.ad-serving-place.com", entries[2]->pattern());
   EXPECT_EQ("www.site.com/anonymous/folder/@", entries[3]->pattern());
   EXPECT_EQ("www.site.com/bad/url", entries[4]->pattern());
-  
+  EXPECT_EQ("@/script?@", entries[5]->pattern());
+  EXPECT_EQ("@?badparam@", entries[6]->pattern());
+
   const Blacklist::ProviderList providers(blacklist.providers_begin(),
                                           blacklist.providers_end());
 
diff --git a/chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc b/chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc
index b228af0..d6e6992 100644
--- a/chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc
+++ b/chrome/browser/privacy_blacklist/blacklist_manager_browsertest.cc
@@ -28,7 +28,7 @@ void GetBlacklistHasMatchOnIOThread(const BlacklistManager* manager,
                                     bool *has_match) {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   const Blacklist* blacklist = manager->GetCompiledBlacklist();
-  scoped_ptr<Blacklist::Match> match(blacklist->findMatch(url));
+  scoped_ptr<Blacklist::Match> match(blacklist->FindMatch(url));
   *has_match = (match.get() != NULL);
   ChromeThread::PostTask(ChromeThread::UI, FROM_HERE,
                          new MessageLoop::QuitTask());
diff --git a/chrome/browser/privacy_blacklist/blacklist_manager_unittest.cc b/chrome/browser/privacy_blacklist/blacklist_manager_unittest.cc
index a2b0a17..55089a4 100644
--- a/chrome/browser/privacy_blacklist/blacklist_manager_unittest.cc
+++ b/chrome/browser/privacy_blacklist/blacklist_manager_unittest.cc
@@ -78,7 +78,7 @@ class BlacklistManagerTest : public testing::Test, public NotificationObserver {
 
 // Returns true if |blacklist| contains a match for |url|.
 bool BlacklistHasMatch(const Blacklist* blacklist, const char* url) {
-  Blacklist::Match* match = blacklist->findMatch(GURL(url));
+  Blacklist::Match* match = blacklist->FindMatch(GURL(url));
 
   if (!match)
     return false;
diff --git a/chrome/browser/privacy_blacklist/blacklist_ui.cc b/chrome/browser/privacy_blacklist/blacklist_ui.cc
index 846ddb9..9425176 100644
--- a/chrome/browser/privacy_blacklist/blacklist_ui.cc
+++ b/chrome/browser/privacy_blacklist/blacklist_ui.cc
@@ -72,7 +72,7 @@ void BlacklistUI::OnNonvisualContentBlocked(const URLRequest* request) {
   const BlacklistManager* blacklist_manager =
       request_info->GetBlacklistManager();
   const Blacklist* blacklist = blacklist_manager->GetCompiledBlacklist();
-  scoped_ptr<Blacklist::Match> match(blacklist->findMatch(request->url()));
+  scoped_ptr<Blacklist::Match> match(blacklist->FindMatch(request->url()));
   const ResourceDispatcherHostRequestInfo* info =
       ResourceDispatcherHost::InfoForRequest(request);
 
diff --git a/chrome/browser/privacy_blacklist/blacklist_unittest.cc b/chrome/browser/privacy_blacklist/blacklist_unittest.cc
index 2331319..1a237e2 100644
--- a/chrome/browser/privacy_blacklist/blacklist_unittest.cc
+++ b/chrome/browser/privacy_blacklist/blacklist_unittest.cc
@@ -20,11 +20,11 @@ TEST(BlacklistTest, Generic) {
 
   Blacklist blacklist;
   ASSERT_TRUE(BlacklistIO::ReadBinary(&blacklist, input));
-  
+
   Blacklist::EntryList entries(blacklist.entries_begin(),
                                blacklist.entries_end());
 
-  ASSERT_EQ(5U, entries.size());
+  ASSERT_EQ(7U, entries.size());
 
   EXPECT_EQ(Blacklist::kBlockByType|Blacklist::kDontPersistCookies,
             entries[0]->attributes());
@@ -57,21 +57,33 @@ TEST(BlacklistTest, Generic) {
   EXPECT_FALSE(entries[4]->MatchesType("image/jpeg"));
   EXPECT_EQ("www.site.com/bad/url", entries[4]->pattern());
 
+  // NOTE: Silly bitwise-or with zero to workaround a Mac compiler bug.
+  EXPECT_EQ(Blacklist::kBlockAll|0, entries[5]->attributes());
+  EXPECT_FALSE(entries[5]->MatchesType("application/x-shockwave-flash"));
+  EXPECT_FALSE(entries[5]->MatchesType("image/jpeg"));
+  EXPECT_EQ("@/script?@", entries[5]->pattern());
+
+  // NOTE: Silly bitwise-or with zero to workaround a Mac compiler bug.
+  EXPECT_EQ(Blacklist::kBlockAll|0, entries[6]->attributes());
+  EXPECT_FALSE(entries[6]->MatchesType("application/x-shockwave-flash"));
+  EXPECT_FALSE(entries[6]->MatchesType("image/jpeg"));
+  EXPECT_EQ("@?badparam@", entries[6]->pattern());
+
   Blacklist::ProviderList providers(blacklist.providers_begin(),
                                     blacklist.providers_end());
-  
+
   ASSERT_EQ(1U, providers.size());
   EXPECT_EQ("Sample", providers[0]->name());
   EXPECT_EQ("http://www.google.com", providers[0]->url());
 
   // No match for chrome, about or empty URLs.
-  EXPECT_FALSE(blacklist.findMatch(GURL()));
-  EXPECT_FALSE(blacklist.findMatch(GURL("chrome://new-tab")));
-  EXPECT_FALSE(blacklist.findMatch(GURL("about:blank")));
+  EXPECT_FALSE(blacklist.FindMatch(GURL()));
+  EXPECT_FALSE(blacklist.FindMatch(GURL("chrome://new-tab")));
+  EXPECT_FALSE(blacklist.FindMatch(GURL("about:blank")));
 
   // Expected rule matches.
   Blacklist::Match* match;
-  match = blacklist.findMatch(GURL("http://www.google.com"));
+  match = blacklist.FindMatch(GURL("http://www.google.com"));
   EXPECT_TRUE(match);
   if (match) {
     EXPECT_EQ(Blacklist::kBlockByType|Blacklist::kDontPersistCookies,
@@ -80,7 +92,7 @@ TEST(BlacklistTest, Generic) {
     delete match;
   }
 
-  match = blacklist.findMatch(GURL("http://www.site.com/bad/url"));
+  match = blacklist.FindMatch(GURL("http://www.site.com/bad/url"));
   EXPECT_TRUE(match);
   if (match) {
     EXPECT_EQ(Blacklist::kBlockAll|
@@ -90,7 +102,7 @@ TEST(BlacklistTest, Generic) {
     delete match;
   }
 
-  match = blacklist.findMatch(GURL("http://www.site.com/anonymous"));
+  match = blacklist.FindMatch(GURL("http://www.site.com/anonymous"));
   EXPECT_TRUE(match);
   if (match) {
     EXPECT_EQ(Blacklist::kBlockByType|Blacklist::kDontPersistCookies,
@@ -99,7 +111,7 @@ TEST(BlacklistTest, Generic) {
     delete match;
   }
 
-  match = blacklist.findMatch(GURL("http://www.site.com/anonymous/folder"));
+  match = blacklist.FindMatch(GURL("http://www.site.com/anonymous/folder"));
   EXPECT_TRUE(match);
   if (match) {
     EXPECT_EQ(Blacklist::kBlockByType|Blacklist::kDontPersistCookies,
@@ -108,7 +120,7 @@ TEST(BlacklistTest, Generic) {
     delete match;
   }
 
-  match = blacklist.findMatch(
+  match = blacklist.FindMatch(
       GURL("http://www.site.com/anonymous/folder/subfolder"));
   EXPECT_TRUE(match);
   if (match) {
@@ -119,6 +131,46 @@ TEST(BlacklistTest, Generic) {
     delete match;
   }
 
+  // No matches for URLs without query string
+  match = blacklist.FindMatch(GURL("http://badparam.com/"));
+  EXPECT_TRUE(match);
+  if (match) {
+    EXPECT_EQ(1U, match->entries().size());
+    EXPECT_EQ(Blacklist::kBlockByType|Blacklist::kDontPersistCookies,
+              match->attributes());
+    delete match;
+  }
+
+  match = blacklist.FindMatch(GURL("http://script.bad.org/"));
+  EXPECT_TRUE(match);
+  if (match) {
+    EXPECT_EQ(1U, match->entries().size());
+    EXPECT_EQ(Blacklist::kBlockByType|Blacklist::kDontPersistCookies,
+              match->attributes());
+    delete match;
+  }
+
+  // Expected rule matches.
+  match = blacklist.FindMatch(GURL("http://host.com/script?q=x"));
+  EXPECT_TRUE(match);
+  if (match) {
+    EXPECT_EQ(2U, match->entries().size());
+    EXPECT_EQ(Blacklist::kBlockByType|Blacklist::kDontPersistCookies|
+              Blacklist::kBlockAll,
+              match->attributes());
+    delete match;
+  }
+
+  match = blacklist.FindMatch(GURL("http://host.com/img?badparam=x"));
+  EXPECT_TRUE(match);
+  if (match) {
+    EXPECT_EQ(2U, match->entries().size());
+    EXPECT_EQ(Blacklist::kBlockByType|Blacklist::kDontPersistCookies|
+              Blacklist::kBlockAll,
+              match->attributes());
+    delete match;
+  }
+
   // StripCookieExpiry Tests
   std::string cookie1(
       "PREF=ID=14a549990453e42a:TM=1245183232:LM=1245183232:S=Occ7khRVIEE36Ao5;"
@@ -167,6 +219,21 @@ TEST(BlacklistTest, Generic) {
   EXPECT_TRUE(header1 == Blacklist::StripCookies(header1));
   EXPECT_TRUE(header2 == Blacklist::StripCookies(header2));
   EXPECT_TRUE(header4 == Blacklist::StripCookies(header3));
+
+  // GetURLAsLookupString Test.
+  std::string url_spec1("example.com/some/path");
+  std::string url_spec2("example.com/script?param=1");
+
+  EXPECT_TRUE(url_spec1 == Blacklist::GetURLAsLookupString(
+              GURL("http://example.com/some/path")));
+  EXPECT_TRUE(url_spec1 == Blacklist::GetURLAsLookupString(
+              GURL("ftp://example.com/some/path")));
+  EXPECT_TRUE(url_spec1 == Blacklist::GetURLAsLookupString(
+              GURL("http://example.com:8080/some/path")));
+  EXPECT_TRUE(url_spec1 == Blacklist::GetURLAsLookupString(
+              GURL("http://user:login@example.com/some/path")));
+  EXPECT_TRUE(url_spec2 == Blacklist::GetURLAsLookupString(
+              GURL("http://example.com/script?param=1")));
 }
 
 TEST(BlacklistTest, PatternMatch) {