Add a SafeSearch preference, policy and implementation.

chrome/browser/net/chrome_network_delegate.cc

Index: chrome/browser/net/chrome_network_delegate.cc
diff --git a/chrome/browser/net/chrome_network_delegate.cc b/chrome/browser/net/chrome_network_delegate.cc
index 828fe6665fb9348faaa06f9ad9780ee62a6c5ed2..d9a76e2b129c6050945e3d945d6ba33eb7060fef 100644
--- a/chrome/browser/net/chrome_network_delegate.cc
+++ b/chrome/browser/net/chrome_network_delegate.cc
@@ -4,6 +4,7 @@
 
 #include "chrome/browser/net/chrome_network_delegate.h"
 
+#include <string.h>  // For strlen

[Joao da Silva, 2012/10/16 16:26:46]

nit: newline

[Sergiu, 2012/10/17 14:48:58]

Done.

 #include "base/logging.h"
 #include "base/base_paths.h"
 #include "base/path_service.h"
@@ -17,6 +18,7 @@
 #include "chrome/browser/extensions/event_router_forwarder.h"
 #include "chrome/browser/extensions/extension_info_map.h"
 #include "chrome/browser/extensions/extension_process_manager.h"
+#include "chrome/browser/google/google_util.h"
 #include "chrome/browser/net/load_time_stats.h"
 #include "chrome/browser/performance_monitor/performance_monitor.h"
 #include "chrome/browser/prefs/pref_service.h"
@@ -146,6 +148,7 @@ ChromeNetworkDelegate::ChromeNetworkDelegate(
     CookieSettings* cookie_settings,
     BooleanPrefMember* enable_referrers,
     BooleanPrefMember* enable_do_not_track,
+    BooleanPrefMember* enable_safesearch,
     chrome_browser_net::LoadTimeStats* load_time_stats)
     : event_router_(event_router),
       profile_(profile),
@@ -153,6 +156,7 @@ ChromeNetworkDelegate::ChromeNetworkDelegate(
       extension_info_map_(extension_info_map),
       enable_referrers_(enable_referrers),
       enable_do_not_track_(enable_do_not_track),
+      enable_safesearch_(enable_safesearch),
       url_blacklist_manager_(url_blacklist_manager),
       managed_mode_url_filter_(managed_mode_url_filter),
       load_time_stats_(load_time_stats) {
@@ -172,6 +176,7 @@ void ChromeNetworkDelegate::NeverThrottleRequests() {
 void ChromeNetworkDelegate::InitializePrefsOnUIThread(
     BooleanPrefMember* enable_referrers,
     BooleanPrefMember* enable_do_not_track,
+    BooleanPrefMember* enable_safesearch,
     PrefService* pref_service) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   enable_referrers->Init(prefs::kEnableReferrers, pref_service, NULL);
@@ -180,6 +185,10 @@ void ChromeNetworkDelegate::InitializePrefsOnUIThread(
     enable_do_not_track->Init(prefs::kEnableDoNotTrack, pref_service, NULL);
     enable_do_not_track->MoveToThread(BrowserThread::IO);
   }
+  if (enable_safesearch) {
+    enable_safesearch->Init(prefs::kSafeSearchEnabled, pref_service, NULL);
+    enable_safesearch->MoveToThread(BrowserThread::IO);
+  }
 }
 
 // static
@@ -220,6 +229,44 @@ int ChromeNetworkDelegate::OnBeforeURLRequest(
     request->set_referrer(std::string());
   if (enable_do_not_track_ && enable_do_not_track_->GetValue())
     request->SetExtraRequestHeaderByName(kDNTHeader, "1", true /* override */);
+
+  // Enforce SafeSearch.
+  if (enable_safesearch_ && enable_safesearch_->GetValue()) {

[battre, 2012/10/16 16:05:49]

how about moving lines 235-264 into a function? 

[Bernhard Bauer, 2012/10/16 16:37:33]

Yesplz. That would make unit-testing it a lot easier.

[Sergiu, 2012/10/17 14:48:58]

Done.

[Sergiu, 2012/10/17 14:48:58]

Done.

+    const bool is_search_url = google_util::IsGoogleSearchUrl(
+        request->url().spec());
+    const bool is_homepage_url = google_util::IsGoogleHomePageUrl(
+        request->url().spec());

[Bernhard Bauer, 2012/10/16 16:37:33]

Can we add a sanity-DCHECK that |is_search_url| and |is_homepage_url| are
mutually exclusive?

[Sergiu, 2012/10/17 14:48:58]

Well, they're not completely, that's the problem. For example:
http://google.com -> !is_search_url && is_homepage_url
http://google.com?q=test -> is_search_url && is_homepage_url
http://google.com/search?q=test -> is_search_url && !is_homepage_url
http://cnn.com -> !is_search_url && !is_homepage_url

+    if (mod_by_managed_mode_.find(request) == mod_by_managed_mode_.end() &&

[Bernhard Bauer, 2012/10/16 16:37:33]

Can OnBeforeURLRequest() be called multiple times for the same request?

[Sergiu, 2012/10/17 14:48:58]

No longer relevant since we changed the way we do it

+        (is_search_url || is_homepage_url)) {
+      std::string query = request->url().query();
+      std::string append_param;
+
+      if (is_homepage_url && query.length() == 0) {

[Bernhard Bauer, 2012/10/16 16:37:33]

`query.length() == 0` can be written a bit shorter as `query.empty()`.

[Sergiu, 2012/10/17 14:48:58]

Done.

+        append_param = chrome::kSSearchParamsSingle;
+      } else {
+        append_param = chrome::kSSearchParamsMultiple;
+      }
+      // Make sure that the last our parameters are either the only ones

[battre, 2012/10/16 16:05:49]

nit: grammar in sentence above "the last our"

[Sergiu, 2012/10/17 14:48:58]

Done.

+      // or have a & before them so that we don't accept &bogussafe=active.
+      if ((query.rfind(chrome::kSSearchParamsSingle) != std::string::npos &&

[battre, 2012/10/16 16:05:49]

KSSearch --> this is again an abbreviation that is not obvious. Please expand
the abbrev.

[Sergiu, 2012/10/17 14:48:58]

Done.

+          query.length() == strlen(chrome::kSSearchParamsSingle)) ||

[battre, 2012/10/16 16:05:49]

if query contains kSSearchParamsSingle and has the same length it is equal
right? Why don't you test that?

[Sergiu, 2012/10/17 14:48:58]

Done.

+          query.rfind(chrome::kSSearchParamsMultiple) != std::string::npos)
+        return net::OK;

[Joao da Silva, 2012/10/16 16:26:46]

|request| is not added to mod_by_managed_mode_ in this case?

[Bernhard Bauer, 2012/10/16 16:37:33]

Nit: newline

[Sergiu, 2012/10/17 14:48:58]

Done.

[Sergiu, 2012/10/17 14:48:58]

Changed the way this works, see new patch.

+      // Add this parameter at the end of this query since it will override the
+      // other parameters.
+      query.append(append_param);
+
+      GURL::Replacements replacements;
+      replacements.SetQueryStr(query);
+      *new_url = request->url().ReplaceComponents(replacements);
+      // Add it to the set so that we know that it can't be altered by an
+      // extension.
+      mod_by_managed_mode_.insert(request);

[Joao da Silva, 2012/10/16 16:26:46]

Suggestion: instead of using this std::set, why not store a private key in the
request? net::URLRequest implements base::SupportsUserData, and you can attach
anything to it. In this case, you could attach a bogus value (e.g. a new int)
and then later test that GetUserData(g_safe_search_key) is not NULL.

+
+      return net::OK;
+    }
+  }
+
   return ExtensionWebRequestEventRouter::GetInstance()->OnBeforeRequest(
       profile_, extension_info_map_.get(), request, callback, new_url);
 }
@@ -251,8 +298,14 @@ int ChromeNetworkDelegate::OnHeadersReceived(
 
 void ChromeNetworkDelegate::OnBeforeRedirect(net::URLRequest* request,
                                              const GURL& new_location) {
-  ExtensionWebRequestEventRouter::GetInstance()->OnBeforeRedirect(
-      profile_, extension_info_map_.get(), request, new_location);
+  // If the request has been modified by the managed mode then do not route

[Pam (message me for reviews), 2012/10/17 11:37:37]

"...by policy" rather than "by the managed mode", for generality

[Sergiu, 2012/10/17 14:48:58]

No longer applicable (done).

+  // it through the extension so that it doesn't get overwritten.
+  if (mod_by_managed_mode_.find(request) != mod_by_managed_mode_.end()) {

[Joao da Silva, 2012/10/16 16:26:46]

I'm not sure about this, because the extension will see other events
(OnHeadersReceived, OnBeforeSendHeaders, etc) but not these. This might confuse
some extensions that expect to see them all. I leave it to Dominic to decide if
this is acceptable.

[Bernhard Bauer, 2012/10/16 16:37:33]

+1. Can't we just apply the filter after any extensions?

[Pam (message me for reviews), 2012/10/17 11:37:37]

Agreed, it's odd to remove these requests from what an extension sees. We don't
know what the extension might be doing; it could be something else entirely, not
affecting the URL.

[Sergiu, 2012/10/17 14:48:58]

Please review the new way in which we are doing this.

+    mod_by_managed_mode_.erase(request);

[battre, 2012/10/16 16:05:49]

I would consider adding a return statement here and deleting the else below.

[Sergiu, 2012/10/17 14:48:58]

Done.

+  } else {
+    ExtensionWebRequestEventRouter::GetInstance()->OnBeforeRedirect(
+        profile_, extension_info_map_.get(), request, new_location);
+  }
 }