Enforce SafetyMode for YouTube if prefs::kForceSafeSearch is on.

Enforce SafetyMode for YouTube if prefs::kForceSafeSearch is on, by modifying (or adding) YouTube's PrefCookie in the HTTP header.

This is essentially a followup to https://codereview.chromium.org/11186002/

BUG=344815
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282546

[Marc Treib, 2014-07-07 07:47:49]

battre@chromium.org: Please review changes in c/b/net

brettw@chromium.org: Please review changes in base/strings

[battre, 2014-07-07 09:00:06]

https://codereview.chromium.org/354183002/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/354183002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:166: const char
kYoutubePrefCookieName[] = "PREF";
What do you think of moving the youtube and safebrowsing code into a separate
file? This manipulation of bits feels like very grungy code.

Please add a unit test.

https://codereview.chromium.org/354183002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:185: net::HttpRequestHeaders*
headers) {
Could you add more newlines into this function and document what the blocks are
doing?

[Marc Treib, 2014-07-07 16:06:46]

PTAL again. Thanks!

https://codereview.chromium.org/354183002/diff/20001/chrome/browser/net/chrom...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/354183002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:166: const char
kYoutubePrefCookieName[] = "PREF";
On 2014/07/07 09:00:06, battre wrote:
> What do you think of moving the youtube and safebrowsing code into a separate
> file? This manipulation of bits feels like very grungy code.

It is grungy :-/
I moved the code into new safe_search_util.h/cc files.

> Please add a unit test.

Done.

https://codereview.chromium.org/354183002/diff/20001/chrome/browser/net/chrom...
chrome/browser/net/chrome_network_delegate.cc:185: net::HttpRequestHeaders*
headers) {
On 2014/07/07 09:00:06, battre wrote:
> Could you add more newlines into this function and document what the blocks
are
> doing?

Done.

[Pam (message me for reviews), 2014-07-07 19:23:10]

Adding Erik as a reviewer for a quick look at the cookie manipulation -- mostly
with an eye toward code reuse.

https://codereview.chromium.org/354183002/diff/80001/chrome/browser/net/safe_...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/354183002/diff/80001/chrome/browser/net/safe_...
chrome/browser/net/safe_search_util.cc:61: const char kYoutubePrefCookieName[] =
"PREF";
nit: YouTube should have a capital T, here and throughout the CL

[battre, 2014-07-08 13:43:06]

LGTM. Thanks.

I agree that it would be great if Erik could take a look.

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:66: const char
kYouTubePrefCookieFlagsValueFormat[] = "%x";
I think the style guide says that constants should go to the top. [for the
record: I disagree as well.]

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:66: const char
kYouTubePrefCookieFlagsValueFormat[] = "%x";
I would consider to inline the format.

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.h:17: // If |request| is a request to Google
Web Search the function
nit: comma after "Google Web Search"

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.h:19: // Sets the query part of |new_url|
with the new value of the parameters.
maybe: Sets *new_url to a copy of the request url in which the query part
contains the new values of the parameters.

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util_unittest.cc (right):

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util_unittest.cc:70: SetCookie(&headers,
"PREF=f1=123&f2=4321&foo=bar");
did you check that uppler/lower cases of the hex numbers are consistent?

[Marc Treib, 2014-07-08 14:16:15]

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:66: const char
kYouTubePrefCookieFlagsValueFormat[] = "%x";
On 2014/07/08 13:43:06, battre wrote:
> I think the style guide says that constants should go to the top. [for the
> record: I disagree as well.]

Done.

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:66: const char
kYouTubePrefCookieFlagsValueFormat[] = "%x";
On 2014/07/08 13:43:06, battre wrote:
> I would consider to inline the format.

Done.

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.h (right):

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.h:17: // If |request| is a request to Google
Web Search the function
On 2014/07/08 13:43:06, battre wrote:
> nit: comma after "Google Web Search"

Done.

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.h:19: // Sets the query part of |new_url|
with the new value of the parameters.
On 2014/07/08 13:43:06, battre wrote:
> maybe: Sets *new_url to a copy of the request url in which the query part
> contains the new values of the parameters.

Done.

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util_unittest.cc (right):

https://codereview.chromium.org/354183002/diff/100001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util_unittest.cc:70: SetCookie(&headers,
"PREF=f1=123&f2=4321&foo=bar");
On 2014/07/08 13:43:06, battre wrote:
> did you check that uppler/lower cases of the hex numbers are consistent?

YouTube uses lowercase as well
(https://cs.corp.google.com/#piper///depot/google3/video/youtube/src/python/lo...)
and their parsing code should handle both upper and lower case
(https://cs.corp.google.com/#piper///depot/google3/video/youtube/src/python/lo...).
Good catch though, I hadn't thought about this!

[erikwright (departed), 2014-07-08 14:28:26]

https://codereview.chromium.org/354183002/diff/120001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/354183002/diff/120001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:110:
headers->GetHeader(base::StringPiece(net::HttpRequestHeaders::kCookie),
Of course, the Chrome cookie store does not normally need to _parse_ request
cookie headers, so this feature is not part of its implementation.

But this is not the first bit of code to do something like this. A quick query
turned up:

chrome/browser/extensions/api/web_request/web_request_api_helpers.cc

Yes, it would be best if there was a common utility to do this, and tests to
make sure that the utility could parse the output generated by the cookie store
implementation

[Marc Treib, 2014-07-09 12:33:46]

On 2014/07/08 14:28:26, erikwright wrote:
>
https://codereview.chromium.org/354183002/diff/120001/chrome/browser/net/safe...
> File chrome/browser/net/safe_search_util.cc (right):
> 
>
https://codereview.chromium.org/354183002/diff/120001/chrome/browser/net/safe...
> chrome/browser/net/safe_search_util.cc:110:
> headers->GetHeader(base::StringPiece(net::HttpRequestHeaders::kCookie),
> Of course, the Chrome cookie store does not normally need to _parse_ request
> cookie headers, so this feature is not part of its implementation.
> 
> But this is not the first bit of code to do something like this. A quick query
> turned up:
> 
> chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
> 
> Yes, it would be best if there was a common utility to do this, and tests to
> make sure that the utility could parse the output generated by the cookie
store
> implementation

I've moved the relevant helper functions from web_request_api_helpers into
cookie_util, and updated my code to use them. If that looks good so far, I can
also add some tests for the helpers.

[battre, 2014-07-09 12:41:04]

On 2014/07/09 12:33:46, treib wrote:
> On 2014/07/08 14:28:26, erikwright wrote:
> >
>
https://codereview.chromium.org/354183002/diff/120001/chrome/browser/net/safe...
> > File chrome/browser/net/safe_search_util.cc (right):
> > 
> >
>
https://codereview.chromium.org/354183002/diff/120001/chrome/browser/net/safe...
> > chrome/browser/net/safe_search_util.cc:110:
> > headers->GetHeader(base::StringPiece(net::HttpRequestHeaders::kCookie),
> > Of course, the Chrome cookie store does not normally need to _parse_ request
> > cookie headers, so this feature is not part of its implementation.
> > 
> > But this is not the first bit of code to do something like this. A quick
query
> > turned up:
> > 
> > chrome/browser/extensions/api/web_request/web_request_api_helpers.cc
> > 
> > Yes, it would be best if there was a common utility to do this, and tests to
> > make sure that the utility could parse the output generated by the cookie
> store
> > implementation
> 
> I've moved the relevant helper functions from web_request_api_helpers into
> cookie_util, and updated my code to use them. If that looks good so far, I can
> also add some tests for the helpers.

I think it is a good idea to reuse the code.

[erikwright (departed), 2014-07-09 15:36:23]

https://codereview.chromium.org/354183002/diff/140001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/354183002/diff/140001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:125: // The pref cookie's value again
consists of key/value pairs. Parse them.
strike "again". It's no longer relevant.

https://codereview.chromium.org/354183002/diff/140001/net/cookies/cookie_util.h
File net/cookies/cookie_util.h (right):

https://codereview.chromium.org/354183002/diff/140001/net/cookies/cookie_util...
net/cookies/cookie_util.h:55: NET_EXPORT void ParseRequestCookieLine(const
std::string& header_value,
Please add tests for these methods.

I wish that "BuildCookieLine" in cookie_monster.cc were public so you could
easily test against the cookies built by Chromium, but at a minimum you can at
least have some hardcoded strings to test against.

https://codereview.chromium.org/354183002/diff/140001/net/cookies/cookie_util...
net/cookies/cookie_util.h:60: NET_EXPORT std::string SerializeRequestCookieLine(
This method requires that |parsed_cookies| be appropriately escaped. This should
be documented.

[Marc Treib, 2014-07-10 09:03:37]

https://codereview.chromium.org/354183002/diff/140001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/354183002/diff/140001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:125: // The pref cookie's value again
consists of key/value pairs. Parse them.
On 2014/07/09 15:36:23, erikwright wrote:
> strike "again". It's no longer relevant.

Done.

https://codereview.chromium.org/354183002/diff/140001/net/cookies/cookie_util.h
File net/cookies/cookie_util.h (right):

https://codereview.chromium.org/354183002/diff/140001/net/cookies/cookie_util...
net/cookies/cookie_util.h:55: NET_EXPORT void ParseRequestCookieLine(const
std::string& header_value,
On 2014/07/09 15:36:23, erikwright wrote:
> Please add tests for these methods.
> 
> I wish that "BuildCookieLine" in cookie_monster.cc were public so you could
> easily test against the cookies built by Chromium, but at a minimum you can at
> least have some hardcoded strings to test against.

I've added some tests (which actually uncovered a bug in
SerializeRequestCookieLine), PTAL!

https://codereview.chromium.org/354183002/diff/140001/net/cookies/cookie_util...
net/cookies/cookie_util.h:60: NET_EXPORT std::string SerializeRequestCookieLine(
On 2014/07/09 15:36:23, erikwright wrote:
> This method requires that |parsed_cookies| be appropriately escaped. This
should
> be documented.

Done.

[battre, 2014-07-10 10:02:56]

https://codereview.chromium.org/354183002/diff/160001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/354183002/diff/160001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:150: std::string pref_string =
JoinStringKeyValuePairs(pref_values, '=', '&');
I wonder whether this is really resilient... Does it work for the following
line?
PREF="f1=123&f2=0"; OtherCookie=value

Do you want to make it resilient to a potential change in which PREF would be
double quoted?

https://codereview.chromium.org/354183002/diff/160001/net/cookies/cookie_util...
File net/cookies/cookie_util_unittest.cc (right):

https://codereview.chromium.org/354183002/diff/160001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:185: tests.back().str = "key=\"abcdef\";
otherkey=1234";
can you change this to "key=\"ab;cdef\"; otherkey=1234" ?

https://codereview.chromium.org/354183002/diff/160001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:191: for (size_t i = 0; i < tests.size();
i++) {
How about adding
SCOPED_TRACE(testing::Message() << "Test " << i);
here?

[Marc Treib, 2014-07-10 11:35:46]

https://codereview.chromium.org/354183002/diff/160001/chrome/browser/net/safe...
File chrome/browser/net/safe_search_util.cc (right):

https://codereview.chromium.org/354183002/diff/160001/chrome/browser/net/safe...
chrome/browser/net/safe_search_util.cc:150: std::string pref_string =
JoinStringKeyValuePairs(pref_values, '=', '&');
On 2014/07/10 10:02:56, battre wrote:
> I wonder whether this is really resilient... Does it work for the following
> line?
> PREF="f1=123&f2=0"; OtherCookie=value
> 
> Do you want to make it resilient to a potential change in which PREF would be
> double quoted?

Done. I've also added a test which checks this behavior.

https://codereview.chromium.org/354183002/diff/160001/net/cookies/cookie_util...
File net/cookies/cookie_util_unittest.cc (right):

https://codereview.chromium.org/354183002/diff/160001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:185: tests.back().str = "key=\"abcdef\";
otherkey=1234";
On 2014/07/10 10:02:56, battre wrote:
> can you change this to "key=\"ab;cdef\"; otherkey=1234" ?

Semicolons are not allowed in cookie values, even with quotes, see
http://tools.ietf.org/html/rfc6265#section-4.1.1 (not sure what the quotes even
do then, but still).

https://codereview.chromium.org/354183002/diff/160001/net/cookies/cookie_util...
net/cookies/cookie_util_unittest.cc:191: for (size_t i = 0; i < tests.size();
i++) {
On 2014/07/10 10:02:56, battre wrote:
> How about adding
> SCOPED_TRACE(testing::Message() << "Test " << i);
> here?

Done. Didn't know about that!

[battre, 2014-07-10 11:52:14]

Thanks, LGTM

[erikwright (departed), 2014-07-10 14:26:24]

https://codereview.chromium.org/354183002/diff/180001/base/strings/string_uti...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/354183002/diff/180001/base/strings/string_uti...
base/strings/string_util.cc:620: static std::string JoinStringKeyValuePair(
These methods are only used from a single file AFAICT.

They seem relatively specialized (i.e., many application might require escaping,
etc).

There also are no tests for them.

Personally I suggest just moving them to the anonymous namespace in the file
that uses them. Otherwise tests would be required.

In any case I would expect the owners of base to object to their addition with a
single client. OTOH, if you can find some existing code that could be replaced
with this, more power to you!

https://codereview.chromium.org/354183002/diff/180001/net/cookies/cookie_util.cc
File net/cookies/cookie_util.cc (right):

https://codereview.chromium.org/354183002/diff/180001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:247: // Eat ';'
add '.'.

https://codereview.chromium.org/354183002/diff/180001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:259: buffer += i->first.as_string() + "=" +
i->second.as_string();
How about:

buffer.append(i->first.begin(), i->first.end());
buffer.push_back('=');
buffer.append(i->second.begin(), i->second.end());

Avoids a few temporaries. I would also .append("; ") rather than +=. Perhaps
there is an optimization that makes them equivalent, but I'm not aware of it.

[Marc Treib, 2014-07-10 15:40:39]

-brettw as his review is not required anymore (moved string util functions into
local files)

https://codereview.chromium.org/354183002/diff/180001/base/strings/string_uti...
File base/strings/string_util.cc (right):

https://codereview.chromium.org/354183002/diff/180001/base/strings/string_uti...
base/strings/string_util.cc:620: static std::string JoinStringKeyValuePair(
On 2014/07/10 14:26:24, erikwright wrote:
> These methods are only used from a single file AFAICT.
> 
> They seem relatively specialized (i.e., many application might require
escaping,
> etc).
> 
> There also are no tests for them.
> 
> Personally I suggest just moving them to the anonymous namespace in the file
> that uses them. Otherwise tests would be required.
> 
> In any case I would expect the owners of base to object to their addition with
a
> single client. OTOH, if you can find some existing code that could be replaced
> with this, more power to you!

I moved these functions into safe_search_util.cc.

https://codereview.chromium.org/354183002/diff/180001/net/cookies/cookie_util.cc
File net/cookies/cookie_util.cc (right):

https://codereview.chromium.org/354183002/diff/180001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:247: // Eat ';'
On 2014/07/10 14:26:24, erikwright wrote:
> add '.'.

Done.

https://codereview.chromium.org/354183002/diff/180001/net/cookies/cookie_util...
net/cookies/cookie_util.cc:259: buffer += i->first.as_string() + "=" +
i->second.as_string();
On 2014/07/10 14:26:24, erikwright wrote:
> How about:
> 
> buffer.append(i->first.begin(), i->first.end());
> buffer.push_back('=');
> buffer.append(i->second.begin(), i->second.end());
> 
> Avoids a few temporaries. I would also .append("; ") rather than +=. Perhaps
> there is an optimization that makes them equivalent, but I'm not aware of it.

Done.

[erikwright (departed), 2014-07-10 15:45:35]

LGTM.

[Marc Treib, 2014-07-10 16:01:32]

The CQ bit was checked by treib@chromium.org

[commit-bot: I haz the power, 2014-07-10 16:03:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/treib@chromium.org/354183002/220001

[commit-bot: I haz the power, 2014-07-10 17:56:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[commit-bot: I haz the power, 2014-07-10 18:04:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-10 18:04:52]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)

[Pam (message me for reviews), 2014-07-10 22:32:22]

The CQ bit was checked by pam@chromium.org

[commit-bot: I haz the power, 2014-07-10 22:33:26]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/treib@chromium.org/354183002/220001

[commit-bot: I haz the power, 2014-07-11 04:54:46]

Change committed as 282546