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 f34396f5f34e6b33d0ed775d1ecec5572d5511d2..306bed7b0a07c2591f0aeeb95713d2e05ed89959 100644
--- a/chrome/browser/net/chrome_network_delegate.cc
+++ b/chrome/browser/net/chrome_network_delegate.cc
@@ -4,9 +4,13 @@
 
 #include "chrome/browser/net/chrome_network_delegate.h"
 
+#include <string>
+#include <vector>
+
 #include "base/logging.h"
 #include "base/base_paths.h"
 #include "base/path_service.h"
+#include "base/string_split.h"
 #include "chrome/browser/api/prefs/pref_member.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/cookie_settings.h"
@@ -17,6 +21,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"
@@ -87,6 +92,80 @@ void ForwardProxyErrors(net::URLRequest* request,
   }
 }
 
+// Checks to see if the current parameter matches the desired one. |parameter|
+// is the current parameter which is being checked; |parameter_to_find| is the
+// parameter we are looking for. Returns true if we find the parameter (we
+// don't care about the value of the parameter, only the key).
+bool CheckForParameter(const std::string& parameter,
+                       const std::string& parameter_to_find) {

[battre, 2012/10/30 12:56:10]

I think this function does way more than necessary. Why don't you replace the
second parameter with parameter_name? Then you don't need to deal with "=" in
|parameter_to_find|. Also you can change the function signature to "
// Returns whether a URL parameter, |parameter| (e.g. foo=bar), has a specific
|name| (e.g. "foo").
bool IsMatchingParameterName(const std::string& parameter, const std::string&
name) {"
(I don't like the "Check" so much because it does not tell me what the function
does.)

[Sergiu, 2012/10/30 21:58:17]

What I was trying to accomplish was use as few constants as possible. I have the
constant kConst = "foo=bar"; but in that function I'm trying to identify all
parameters where "foo=somethingelse", which I did by searching for the = in
"foo=bar" and then checking "foo=" that against "foo=somethingelse". Otherwise I
could put two constants (for "foo" and "bar") or just hardcode the value foo in
the call (but you would need to modify it in multiple places if it ever
changes). Don't know which is the best option here 

[battre, 2012/10/31 13:16:55]

I like your new implementation.

+  DCHECK(parameter_to_find.find("=") != std::string::npos);
+  // prefix for "safe=off" is "safe=".
+  std::string parameter_prefix = parameter_to_find.substr(
+      0, parameter_to_find.find("=") + 1);
+  if (StartsWithASCII(parameter, parameter_prefix, false))

[battre, 2012/10/30 12:56:10]

This does not work if the key in |parameter| is %xx encoded. Did you check
whether GURL resolves such encoding for you? Can you add a unit test for that or
check whether the RFC prevents this?

[Sergiu, 2012/10/30 21:58:17]

RFC 3986 [1] states:
   URIs that differ in the replacement of an unreserved character with
   its corresponding percent-encoded US-ASCII octet are equivalent: they
   identify the same resource.  However, URI comparison implementations
   do not always perform normalization prior to comparison (see Section
   6).  For consistency, percent-encoded octets in the ranges of ALPHA
   (%41-%5A and %61-%7A), DIGIT (%30-%39), hyphen (%2D), period (%2E),
   underscore (%5F), or tilde (%7E) should not be created by URI
   producers and, when found in a URI, should be decoded to their
   corresponding unreserved characters by URI normalizers.

Therefore, the answer is that this function would not work with safe= written in
percent-encoded form. However the end result would be that we would add
safe=active in non-encoded form and keep the percent-encoded one but the one we
added at the end would still be in effect. Since this is a corner case and I
don't expect query keys in Google Web Search to be percent-encoded any time soon
I don't think this is a major problem, but I can create a general solution if
you think that it is worth it. 

[1] http://tools.ietf.org/html/rfc3986#page-14

[battre, 2012/10/31 13:16:55]

If the last stated parameter wins in case of two conflicting parameters in
SafeSearch, I am fine with the current implementation and would not implement a
general solution because it would be quite costly. 

If not, I don't consider this an unimportant corner case because people who want
to work around security features are creative.

+    return true;
+  return false;

[Joao da Silva, 2012/10/30 09:09:49]

return StartsWithASCII(parameter, parameter_prefix, false);

[Sergiu, 2012/10/30 21:58:17]

Done.

+}
+
+// Examines a query to see if it should be modified to contain the SafeSearch
+// parameters. |query| is the string to examine and the return value is the
+// |query| string modified such that SafeSearch is active.
+std::string ExamineQueryForSafeSearch(const std::string& query) {

[battre, 2012/10/30 12:56:10]

How about "void AddSafeSearchParameters(const GURL& request, GURL* new_url)"?
("Examine" is a bad word for a function in my opinion)

[Sergiu, 2012/10/30 21:58:17]

Renamed, although the signature is the same as before (I don't think I need to
interact with those objects in that function). Please advise if you think it
should be changed.

[battre, 2012/10/31 13:16:55]

SGTM

+  std::vector<std::string> new_parameters;
+  std::string safe_parameter = chrome::kSafeSearchSafeParameter;
+  std::string ssui_parameter = chrome::kSafeSearchSsuiParameter;
+
+  std::vector<std::string> parameters;
+  base::SplitString(query, '&', &parameters);
+
+  std::vector<std::string>::iterator it;
+  for (it = parameters.begin(); it < parameters.end(); ++it) {
+    if (!CheckForParameter(*it, safe_parameter) &&
+        !CheckForParameter(*it, ssui_parameter)) {
+      new_parameters.push_back(*it);
+    }
+  }
+
+  new_parameters.push_back(safe_parameter);
+  new_parameters.push_back(ssui_parameter);
+  return JoinString(new_parameters, '&');
+}
+
+// Examines |request| and if it is a request to Google Web Search
+// it enforces that the SafeSearch query parameters are set to active

[Joao da Silva, 2012/10/30 09:09:49]

nit: terminate sentence

[Sergiu, 2012/10/30 21:58:17]

Done.

+// Sets the query part of |new_url| with the new value of the parameters.
+void ForceGoogleSafeSearch(net::URLRequest* request,
+                           GURL* new_url) {
+  const bool is_search_url = google_util::IsGoogleSearchUrl(
+      request->url().spec());
+  const bool is_homepage_url = google_util::IsGoogleHomePageUrl(
+      request->url().spec());
+  if (is_search_url || is_homepage_url) {
+    std::string query = request->url().query();
+    std::string new_query = ExamineQueryForSafeSearch(query);
+    if (query == new_query)
+      return;
+
+    GURL::Replacements replacements;
+    replacements.SetQueryStr(new_query);
+    *new_url = request->url().ReplaceComponents(replacements);
+  }
+}
+
+// Gets called when the extensions finish work on the URL. If the extensions
+// did not do a redirect (so |new_url| is empty) then we enforce the
+// SafeSearch parameters. Otherwise we will get called again after the
+// redirect and we enforce SafeSearch then.
+void ForceGoogleSafeSearchCallbackWrapper(
+    const net::CompletionCallback& callback,
+    net::URLRequest* request,
+    GURL* new_url,
+    int rv) {
+  if (rv == net::OK && new_url->is_empty())
+    ForceGoogleSafeSearch(request, new_url);
+  callback.Run(rv);
+}
+
 enum RequestStatus { REQUEST_STARTED, REQUEST_DONE };
 
 // Notifies the ExtensionProcessManager that a request has started or stopped
@@ -146,6 +225,7 @@ ChromeNetworkDelegate::ChromeNetworkDelegate(
     CookieSettings* cookie_settings,
     BooleanPrefMember* enable_referrers,
     BooleanPrefMember* enable_do_not_track,
+    BooleanPrefMember* force_google_safesearch,

[battre, 2012/10/30 12:56:10]

if your functions use the spelling SafeSearch, I think this should be
safe_search.

[Sergiu, 2012/10/30 21:58:17]

Yeah, SafeSearch was a bit tricky because it is officially written as
SafeSearch, which is a single word, but I changed it to *_safe_search.

     chrome_browser_net::LoadTimeStats* load_time_stats)
     : event_router_(event_router),
       profile_(profile),
@@ -153,6 +233,7 @@ ChromeNetworkDelegate::ChromeNetworkDelegate(
       extension_info_map_(extension_info_map),
       enable_referrers_(enable_referrers),
       enable_do_not_track_(enable_do_not_track),
+      force_google_safesearch_(force_google_safesearch),
       url_blacklist_manager_(url_blacklist_manager),
       managed_mode_url_filter_(managed_mode_url_filter),
       load_time_stats_(load_time_stats) {
@@ -172,6 +253,7 @@ void ChromeNetworkDelegate::NeverThrottleRequests() {
 void ChromeNetworkDelegate::InitializePrefsOnUIThread(
     BooleanPrefMember* enable_referrers,
     BooleanPrefMember* enable_do_not_track,
+    BooleanPrefMember* force_google_safesearch,
     PrefService* pref_service) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   enable_referrers->Init(prefs::kEnableReferrers, pref_service, NULL);
@@ -180,6 +262,10 @@ void ChromeNetworkDelegate::InitializePrefsOnUIThread(
     enable_do_not_track->Init(prefs::kEnableDoNotTrack, pref_service, NULL);
     enable_do_not_track->MoveToThread(BrowserThread::IO);
   }
+  if (force_google_safesearch) {
+    force_google_safesearch->Init(prefs::kForceSafeSearch, pref_service, NULL);
+    force_google_safesearch->MoveToThread(BrowserThread::IO);
+  }
 }
 
 // static
@@ -220,8 +306,26 @@ int ChromeNetworkDelegate::OnBeforeURLRequest(
     request->set_referrer(std::string());
   if (enable_do_not_track_ && enable_do_not_track_->GetValue())
     request->SetExtraRequestHeaderByName(kDNTHeader, "1", true /* override */);
-  return ExtensionWebRequestEventRouter::GetInstance()->OnBeforeRequest(
-      profile_, extension_info_map_.get(), request, callback, new_url);
+
+  bool force_safesearch = force_google_safesearch_ &&
+                          force_google_safesearch_->GetValue();
+
+  net::CompletionCallback wrapped_callback = callback;
+  if (force_safesearch) {
+    wrapped_callback = base::Bind(&ForceGoogleSafeSearchCallbackWrapper,
+                                  callback,
+                                  base::Unretained(request),
+                                  base::Unretained(new_url));
+  }
+
+  int rv = ExtensionWebRequestEventRouter::GetInstance()->OnBeforeRequest(
+      profile_, extension_info_map_.get(), request, wrapped_callback,
+      new_url);
+
+  if (force_safesearch && rv == net::OK && new_url->is_empty())
+    ForceGoogleSafeSearch(request, new_url);
+
+  return rv;
 }
 
 int ChromeNetworkDelegate::OnBeforeSendHeaders(