PS - Adjusting webRequest API for use in Public Sessions

Filter out privacy/security sensitive data from webRequests in Public Sessions.

Enable all URLs for webRequest for regular extensions in Public Sessions. Note: Origin/URL permissions are completely disabled in Public Sessions for privacy/security reasons so this is a workaround.

Doc explaining the changes to webRequest API in details go/webrequest-whitelisting.

BUG=661678
Committed: https://crrev.com/3e7318b652b0fa22e68b3cb4de0e93cfef12f15c
Cr-Commit-Position: refs/heads/master@{#432469}

[Ivan Šandrk, 2016-10-28 13:09:44]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 13:09:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-10-28 13:12:01]

Description was changed from

==========
PS - Enable all URLs for webRequest for regular extensions

BUG=
==========

to

==========
PS - Enable all URLs for webRequest for regular extensions in Public Sessions

BUG=
==========

[commit-bot: I haz the power, 2016-10-28 13:26:19]

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

[commit-bot: I haz the power, 2016-10-28 13:26:20]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Ivan Šandrk, 2016-10-28 14:17:51]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 14:18:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 15:03:00]

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

[commit-bot: I haz the power, 2016-10-28 15:03:01]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ivan Šandrk, 2016-10-31 11:32:05]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 11:32:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-10-31 11:35:15]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-31 11:35:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 12:33:20]

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

[commit-bot: I haz the power, 2016-10-31 12:33:21]

Dry run: This issue passed the CQ dry run.

[Ivan Šandrk, 2016-11-02 15:44:46]

isandrk@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Ivan Šandrk, 2016-11-02 15:44:47]

Hey Devlin, mind taking a preliminary look at this and tell me what you think?
This is part of the ongoing effort to extend Public Sessions, you participated
in a discussion about it 2 weeks ago.

Also I'd appreciate any hints on how to test this properly :)

Ivan

[Ivan Šandrk, 2016-11-02 17:51:56]

Description was changed from

==========
PS - Enable all URLs for webRequest for regular extensions in Public Sessions

BUG=
==========

to

==========
PS - Enable all URLs for webRequest for regular extensions in Public Sessions

BUG=661678
==========

[Ivan Šandrk, 2016-11-02 18:10:08]

Description was changed from

==========
PS - Enable all URLs for webRequest for regular extensions in Public Sessions

BUG=661678
==========

to

==========
Enable all URLs for webRequest for regular extensions in Public Sessions.

Note: Origin/URL permissions are completely disabled in Public Sessions for
privacy/security reasons so this is a workaround.

BUG=661678
==========

[Ivan Šandrk, 2016-11-02 18:17:12]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 18:18:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-02 19:15:38]

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

[commit-bot: I haz the power, 2016-11-02 19:15:39]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-03 00:33:46]

https://codereview.chromium.org/2455393002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1460: if
(IsPublicSession() && extension && extension->is_extension()) {
We want to do this for *all* extensions?  Is it always safe to assume that an
extension in a public session was installed by the admin?

Also, this will allow even restricted urls, like chrome:// urls.  I get the
feeling we don't want to allow that.

[Ivan Šandrk, 2016-11-03 14:45:12]

I also wonder what you think about the general structure of the changes, are you
fine with them? They seemed like the simplest and least intrusive way to me :)

https://codereview.chromium.org/2455393002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1460: if
(IsPublicSession() && extension && extension->is_extension()) {
On 2016/11/03 00:33:46, Devlin (slow) wrote:
> We want to do this for *all* extensions?  Is it always safe to assume that an
> extension in a public session was installed by the admin?
> 
> Also, this will allow even restricted urls, like chrome:// urls.  I get the
> feeling we don't want to allow that.

Yes, for all regular extensions. Yes, users aren't allowed to install
extensions, there is a check in place for this -
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/devic...
(EXTERNAL_POLICY means that the extension is force installed by policy).

That's a good point about restricted urls. I've got a simple test extension for
testing this stuff out, I couldn't register a chrome:// url. Following the error
message I ended up here
https://cs.chromium.org/chromium/src/extensions/browser/api/web_request/web_r...

Http/https/ftp links are fine, chrome-extension:// links should also be fine
(all extensions are admin installed), and I think file:// links should also be
fine. I don't think people use the browser to browse their local files +
everything is stripped out; for example file:///boot/abi-3.13.0-100-generic is
shown only as file:/// (URLs are stripped down to origin). I could limit the
urls only to http?/ftp, but I think that's just adding unnecessary complexity.
WDYT?

[Ivan Šandrk, 2016-11-03 15:37:04]

isandrk@chromium.org changed reviewers:
+ atwilson@chromium.org

[Ivan Šandrk, 2016-11-04 16:39:00]

Hey Devlin, polite ping!

[Devlin, 2016-11-05 06:04:37]

On 2016/11/03 14:45:12, Ivan Šandrk wrote:
> I also wonder what you think about the general structure of the changes, are
you
> fine with them? They seemed like the simplest and least intrusive way to me :)

This seems generally fine; my biggest concern is always around the "death by a
thousand paper cuts" - no one change my be intrusive or complicated, but the
combination is.  But so far this seems okay. :)

https://codereview.chromium.org/2455393002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1460: if
(IsPublicSession() && extension && extension->is_extension()) {
On 2016/11/03 14:45:12, Ivan Šandrk wrote:
> On 2016/11/03 00:33:46, Devlin (slow) wrote:
> > We want to do this for *all* extensions?  Is it always safe to assume that
an
> > extension in a public session was installed by the admin?
> > 
> > Also, this will allow even restricted urls, like chrome:// urls.  I get the
> > feeling we don't want to allow that.
> 
> Yes, for all regular extensions. Yes, users aren't allowed to install
> extensions, there is a check in place for this -
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/devic...
> (EXTERNAL_POLICY means that the extension is force installed by policy).
> 
> That's a good point about restricted urls. I've got a simple test extension
for
> testing this stuff out, I couldn't register a chrome:// url. Following the
error
> message I ended up here
>
https://cs.chromium.org/chromium/src/extensions/browser/api/web_request/web_r...
> 
> Http/https/ftp links are fine, chrome-extension:// links should also be fine
> (all extensions are admin installed), and I think file:// links should also be
> fine. I don't think people use the browser to browse their local files +
> everything is stripped out; for example file:///boot/abi-3.13.0-100-generic is
> shown only as file:/// (URLs are stripped down to origin). I could limit the
> urls only to http?/ftp, but I think that's just adding unnecessary complexity.
> WDYT?

If security/privacy are on board with this, then that's fine.  I was mostly
worried about chrome:// urls.

If all extensions must be installed by policy, can we add a
CHECK(Manifest::IsPolicyLocation(extension->location())); here?  That would make
me feel a bit better.

[Devlin, 2016-11-05 06:05:16]

Also note we will need tests for this.

[Ivan Šandrk, 2016-11-07 10:47:44]

Description was changed from

==========
Enable all URLs for webRequest for regular extensions in Public Sessions.

Note: Origin/URL permissions are completely disabled in Public Sessions for
privacy/security reasons so this is a workaround.

BUG=661678
==========

to

==========
Enable all URLs for webRequest for regular extensions in Public Sessions. Here's
a doc explaining the changes to webRequest API in details
go/webrequest-whitelisting.

Note: Origin/URL permissions are completely disabled in Public Sessions for
privacy/security reasons so this is a workaround.

BUG=661678
==========

[Ivan Šandrk, 2016-11-07 10:50:03]

Btw I guess you should also take a look at this doc
go/webrequest-whitelisting (doc explaining changes to webRequest API).

https://codereview.chromium.org/2455393002/diff/80001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/80001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_api.cc:1460: if
(IsPublicSession() && extension && extension->is_extension()) {
On 2016/11/05 06:04:37, Devlin (slow) wrote:
> On 2016/11/03 14:45:12, Ivan Šandrk wrote:
> > On 2016/11/03 00:33:46, Devlin (slow) wrote:
> > > We want to do this for *all* extensions?  Is it always safe to assume that
> an
> > > extension in a public session was installed by the admin?
> > > 
> > > Also, this will allow even restricted urls, like chrome:// urls.  I get
the
> > > feeling we don't want to allow that.
> > 
> > Yes, for all regular extensions. Yes, users aren't allowed to install
> > extensions, there is a check in place for this -
> >
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/extensions/devic...
> > (EXTERNAL_POLICY means that the extension is force installed by policy).
> > 
> > That's a good point about restricted urls. I've got a simple test extension
> for
> > testing this stuff out, I couldn't register a chrome:// url. Following the
> error
> > message I ended up here
> >
>
https://cs.chromium.org/chromium/src/extensions/browser/api/web_request/web_r...
> > 
> > Http/https/ftp links are fine, chrome-extension:// links should also be fine
> > (all extensions are admin installed), and I think file:// links should also
be
> > fine. I don't think people use the browser to browse their local files +
> > everything is stripped out; for example file:///boot/abi-3.13.0-100-generic
is
> > shown only as file:/// (URLs are stripped down to origin). I could limit the
> > urls only to http?/ftp, but I think that's just adding unnecessary
complexity.
> > WDYT?
> 
> If security/privacy are on board with this, then that's fine.  I was mostly
> worried about chrome:// urls.
> 
> If all extensions must be installed by policy, can we add a
> CHECK(Manifest::IsPolicyLocation(extension->location())); here?  That would
make
> me feel a bit better.

S&P are fine.

Done.

[Ivan Šandrk, 2016-11-07 14:50:30]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-07 14:50:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-11-07 14:59:39]

In Patch Set 7 I moved a part of code because I didn't see an easy way to test
it in its current form.

[commit-bot: I haz the power, 2016-11-07 15:58:12]

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

[commit-bot: I haz the power, 2016-11-07 15:58:13]

Dry run: This issue passed the CQ dry run.

[Andrew T Wilson (Slow), 2016-11-09 08:36:00]

LGTM with a few comment nits

https://codereview.chromium.org/2455393002/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:216:
false /*crosses_incognito*/,
please make the comment style consistent, here and up at line 202 (prefer //
over /*)

https://codereview.chromium.org/2455393002/diff/120001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/120001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1453:
WebRequestPermissions::REQUIRE_HOST_PERMISSION);
Remove blank line

https://codereview.chromium.org/2455393002/diff/120001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/2455393002/diff/120001/extensions/browser/api...
extensions/browser/api/web_request/web_request_permissions.cc:135:
CHECK(extensions::Manifest::IsPolicyLocation(extension->location()));
Document what this check is verifying (i.e. if someone hits this check, explain
what has gone wrong).

[Ivan Šandrk, 2016-11-09 12:08:29]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 12:08:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-11-09 12:12:13]

https://codereview.chromium.org/2455393002/diff/120001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:216:
false /*crosses_incognito*/,
On 2016/11/09 08:36:00, Andrew T Wilson (Slow) wrote:
> please make the comment style consistent, here and up at line 202 (prefer //
> over /*)

Done.

https://codereview.chromium.org/2455393002/diff/120001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/120001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1453:
WebRequestPermissions::REQUIRE_HOST_PERMISSION);
On 2016/11/09 08:36:00, Andrew T Wilson (Slow) wrote:
> Remove blank line

Done.

https://codereview.chromium.org/2455393002/diff/120001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/2455393002/diff/120001/extensions/browser/api...
extensions/browser/api/web_request/web_request_permissions.cc:135:
CHECK(extensions::Manifest::IsPolicyLocation(extension->location()));
On 2016/11/09 08:36:00, Andrew T Wilson (Slow) wrote:
> Document what this check is verifying (i.e. if someone hits this check,
explain
> what has gone wrong).

Done.

[commit-bot: I haz the power, 2016-11-09 13:05:55]

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

[commit-bot: I haz the power, 2016-11-09 13:05:56]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-10 16:04:58]

https://codereview.chromium.org/2455393002/diff/140001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:225:
// Host permission checks are disabled in Public Sessions, instead all URL's
What's the motivation for granting access beyond what the extension requests? 
If it's necessary, can we say why (probably in the web_request_permissions.cc
file)?  It seems like most policy extensions would just request all urls if they
want to intercept something, so there's no reason to implicitly grant more.

Additionally, some extensions might be expecting to have sites that they don't
request automatically filtered out (even though they should probably be using
the filters in the API instead of permissions, no reason to break it).

[Ivan Šandrk, 2016-11-10 17:44:20]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 17:44:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-11-10 17:56:19]

I've merged the other CL in this one because with the tests being so similar its
too complicated to divide them in two CLs.

I've also disabled the CHECK you requested for now because I'm unsure how to get
it working with the apitest (apitest loads the test extension with its manifest
and I'm unsure how to set the location to policy). I could use your input here.
But also IMO that check isn't that important, the user can't do any damage even
if he manages to install the extension.

I've got tests in for most things, gotta still write a test for filtering
event_details (web_request_event_details.cc), I'm thinking of just adding a
unittest for that file. I also didn't test all of BlockingRequest stuff (one
test makes sure that cancel still works, another makes sure redirectUrl is
blocked). I'm not sure at this point how to write those tests, could use your
help (but then again, one of the blocked things is checked, that's atleast
somewhat good for the others).

https://codereview.chromium.org/2455393002/diff/140001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:225:
// Host permission checks are disabled in Public Sessions, instead all URL's
On 2016/11/10 16:04:58, Devlin (slow) wrote:
> What's the motivation for granting access beyond what the extension requests? 
> If it's necessary, can we say why (probably in the web_request_permissions.cc
> file)?  It seems like most policy extensions would just request all urls if
they
> want to intercept something, so there's no reason to implicitly grant more.
> 
> Additionally, some extensions might be expecting to have sites that they don't
> request automatically filtered out (even though they should probably be using
> the filters in the API instead of permissions, no reason to break it).

The host permissions were deemed to be inherently unsafe in PS and therefore
were completely disabled for all APIs (extensions requesting such permisions are
not loaded). webRequest API needs them to work and this seemed like the simplest
way to enable them just for webRequest (I'm guessing any other solution would
require a lot of code duplication across many files).

The use case behind these changes is a website filter/blocker specifically made
for PS. It would have to have access to all URLs.

Anyway, this document explains all the changes to webRequest API
go/webrequest-whitelisting.

[Ivan Šandrk, 2016-11-10 18:00:26]

Description was changed from

==========
Enable all URLs for webRequest for regular extensions in Public Sessions. Here's
a doc explaining the changes to webRequest API in details
go/webrequest-whitelisting.

Note: Origin/URL permissions are completely disabled in Public Sessions for
privacy/security reasons so this is a workaround.

BUG=661678
==========

to

==========
Filter out privacy/security sensitive data from webRequests in Public Sessions.

Enable all URLs for webRequest for regular extensions in Public Sessions. Note:
Origin/URL permissions are completely disabled in Public Sessions for
privacy/security reasons so this is a workaround.

Doc explaining the changes to webRequest API in details
go/webrequest-whitelisting.

BUG=661678
==========

[commit-bot: I haz the power, 2016-11-10 18:05:27]

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

[commit-bot: I haz the power, 2016-11-10 18:05:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ivan Šandrk, 2016-11-10 18:09:12]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 18:09:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-11-10 18:10:45]

Patchset #10 (id:180001) has been deleted

[Ivan Šandrk, 2016-11-10 18:12:28]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-10 18:13:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-10 20:00:32]

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

[commit-bot: I haz the power, 2016-11-10 20:00:33]

Dry run: This issue passed the CQ dry run.

[Ivan Šandrk, 2016-11-11 00:23:57]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 00:24:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 00:34:07]

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

[commit-bot: I haz the power, 2016-11-11 00:34:08]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Ivan Šandrk, 2016-11-11 11:11:21]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 11:11:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 11:52:31]

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

[commit-bot: I haz the power, 2016-11-11 11:52:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ivan Šandrk, 2016-11-11 13:11:57]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 13:12:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-11-11 13:40:32]

Hi Devlin, could you take a look at this today? We'd like to land it today if
possible.

[commit-bot: I haz the power, 2016-11-11 14:20:35]

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

[commit-bot: I haz the power, 2016-11-11 14:20:36]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-14 17:44:39]

> I've also disabled the CHECK you requested for now because I'm unsure how to
get
it working with the apitest (apitest loads the test extension with its manifest
and I'm unsure how to set the location to policy). I could use your input here.
But also IMO that check isn't that important, the user can't do any damage even
if he manages to install the extension.

Two options here:
- Add wiring to load up an extension has a policy extension; this would mean
modifying ExtensionApiTest.  It shouldn't be terribly difficult and I think we
already have a similar option for component extensions.
- Add a runtime bool that's exposed for testing like
"allow_all_extensions_in_public_sessions", which is normally set to false and
set to true in the test.  Then CHECK(allow_all_extensions_in_public_sessions ||
      Manifest::IsPolicyLocation(extension->location));

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:35:
orig->dict_.SetString("foo", "foo value");
I'd prefer we stick to values that might be set - otherwise if we ever add
validation code, this will fail.

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:44:
EXPECT_EQ(copy->extra_info_spec_, 0);
EXPECT_EQ(expected, actual)

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:210:
// com_extension_ doesn't have host permission for .org URL's.
s/URL's/URLs (here and below)

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/webrequest_public_session/manifest.json
(right):

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/manifest.json:6:
"permissions": ["webRequest", "webRequestBlocking", "tabs",
We shouldn't need tabs and declarativeWebRequest, right?

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest_public_session/test.js
(right):

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/test.js:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
not 2011 :)

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/test.js:64: var
expectUrl = getURL('');
Should this be "getStrippedURL"?

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1134: event_details =
event_details->WhitelistedCopyForPublicSession();
This sanitizes the input for *all* listeners, not just the extension that might
be force-installed.  Is that desired behavior?  If so, we should comment to make
it explicit.

Additionally, if that's the case, we don't need to make a copy of event details;
we can just modify the current object.

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:2147: // When we are in a
Public Session, allow all URL's for webRequests
s/URL's/URLs

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:2233: if
(!IsPublicSession()) {
Could we instead have this be an early-out error?  i.e. something like:
if (IsPublicSession() &&
    (value->HasKey("redirectUrl") ||
     value->HasKey(keys::kAuthCredentialsKey) ||
     value->HasKey("requestHeaders") ||
     value->HasKey("responseHeaders")) {
  return RespondNow(Error("Only cancel allowed in public sessions"));
}

I think it would make it a bit easier to read (and the diff a lot cleaner)

[Ivan Šandrk, 2016-11-14 20:16:13]

This should be it. If I missed something, sorry. Pretty tired at this point.

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:1:
// Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/11/14 17:44:38, Devlin wrote:
> no (c)

Done.

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:35:
orig->dict_.SetString("foo", "foo value");
On 2016/11/14 17:44:38, Devlin wrote:
> I'd prefer we stick to values that might be set - otherwise if we ever add
> validation code, this will fail.

Done.

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:44:
EXPECT_EQ(copy->extra_info_spec_, 0);
On 2016/11/14 17:44:38, Devlin wrote:
> EXPECT_EQ(expected, actual)

Done.

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:210:
// com_extension_ doesn't have host permission for .org URL's.
On 2016/11/14 17:44:38, Devlin wrote:
> s/URL's/URLs (here and below)

Done.

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/webrequest_public_session/manifest.json
(right):

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/manifest.json:6:
"permissions": ["webRequest", "webRequestBlocking", "tabs",
On 2016/11/14 17:44:38, Devlin wrote:
> We shouldn't need tabs and declarativeWebRequest, right?

I copied over webrequest/framework.js which uses them both. I believe it needs
the tab permission for running the tests, but now I see that I can take away the
declarativeWebRequest.

EDIT: Doesn't work without the declarativeWebRequest, even after removing it
from framework.js. I don't think I would want to spend time investigating why at
this point. I hope you're fine with it as is.

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest_public_session/test.js
(right):

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/test.js:1: //
Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2016/11/14 17:44:38, Devlin wrote:
> not 2011 :)

Done.

https://codereview.chromium.org/2455393002/diff/260001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/test.js:64: var
expectUrl = getURL('');
On 2016/11/14 17:44:38, Devlin wrote:
> Should this be "getStrippedURL"?

It could be, but I figured it would be simpler this way (shorter test, less HTTP
events). getURL fetches URLs (not URL's heh) from the extension directory, and
this generates less HTTP events for some reason (didn't look much into it). And
yes, the test is also good written like this, I've verified that it fails
without the redirectUrl blocking code.

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1134: event_details =
event_details->WhitelistedCopyForPublicSession();
On 2016/11/14 17:44:38, Devlin wrote:
> This sanitizes the input for *all* listeners, not just the extension that
might
> be force-installed.  Is that desired behavior?  If so, we should comment to
make
> it explicit.
> 
> Additionally, if that's the case, we don't need to make a copy of event
details;
> we can just modify the current object.

Yes.

I guess I could also use some help here. I couldn't assign the new object inside
the WhitelistedCopyForPublicSession because the assignment operator is deleted
and I wanted to make the smallest change to the existing code, plus I was trying
to go with the approach of a clean copy, and then just copying over the safe
fields (future-proofing). Got any suggestions here?

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:2147: // When we are in a
Public Session, allow all URL's for webRequests
On 2016/11/14 17:44:38, Devlin wrote:
> s/URL's/URLs

Done.

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:2233: if
(!IsPublicSession()) {
On 2016/11/14 17:44:38, Devlin wrote:
> Could we instead have this be an early-out error?  i.e. something like:
> if (IsPublicSession() &&
>     (value->HasKey("redirectUrl") ||
>      value->HasKey(keys::kAuthCredentialsKey) ||
>      value->HasKey("requestHeaders") ||
>      value->HasKey("responseHeaders")) {
>   return RespondNow(Error("Only cancel allowed in public sessions"));
> }
> 
> I think it would make it a bit easier to read (and the diff a lot cleaner)

Agreed completely and done. But now I broke the redirectIsBlocked test, and I'm
unsure how to restructure it. It gives the error message "Unchecked
runtime.lastError while running webRequestInternal.eventHandled: Only cancel
allowed in Public Sessions.", and then timeouts.

I guess I was relying on the silent ignore that was previously observed. Got any
ideas/pointers/hints?

https://codereview.chromium.org/2455393002/diff/280001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_permissions.h (right):

https://codereview.chromium.org/2455393002/diff/280001/extensions/browser/api...
extensions/browser/api/web_request/web_request_permissions.h:54: static bool
allow_all_extensions_in_public_sessions_;
Added the runtime bool like this, but I'm unable to access it from the apitest
(... is a private member of WebRequestPermissions). Also tried using a private
setter function to no avail. Help?

[Devlin, 2016-11-15 01:19:37]

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1134: event_details =
event_details->WhitelistedCopyForPublicSession();
On 2016/11/14 20:16:13, Ivan Šandrk wrote:
> On 2016/11/14 17:44:38, Devlin wrote:
> > This sanitizes the input for *all* listeners, not just the extension that
> might
> > be force-installed.  Is that desired behavior?  If so, we should comment to
> make
> > it explicit.
> > 
> > Additionally, if that's the case, we don't need to make a copy of event
> details;
> > we can just modify the current object.
> 
> Yes.
> 
> I guess I could also use some help here. I couldn't assign the new object
inside
> the WhitelistedCopyForPublicSession because the assignment operator is deleted
> and I wanted to make the smallest change to the existing code, plus I was
trying
> to go with the approach of a clean copy, and then just copying over the safe
> fields (future-proofing). Got any suggestions here?

If we're okay with sanitizing for all listeners, which it sounds like we are,
then I'd say we should just make a ScrubForPublicSession() method or similar on
WebRequestEventDetails that clears those fields on itself.  Would that work?

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:2233: if
(!IsPublicSession()) {
On 2016/11/14 20:16:13, Ivan Šandrk wrote:
> On 2016/11/14 17:44:38, Devlin wrote:
> > Could we instead have this be an early-out error?  i.e. something like:
> > if (IsPublicSession() &&
> >     (value->HasKey("redirectUrl") ||
> >      value->HasKey(keys::kAuthCredentialsKey) ||
> >      value->HasKey("requestHeaders") ||
> >      value->HasKey("responseHeaders")) {
> >   return RespondNow(Error("Only cancel allowed in public sessions"));
> > }
> > 
> > I think it would make it a bit easier to read (and the diff a lot cleaner)
> 
> Agreed completely and done. But now I broke the redirectIsBlocked test, and
I'm
> unsure how to restructure it. It gives the error message "Unchecked
> runtime.lastError while running webRequestInternal.eventHandled: Only cancel
> allowed in Public Sessions.", and then timeouts.
> 
> I guess I was relying on the silent ignore that was previously observed. Got
any
> ideas/pointers/hints?

The basic approach here will be that you'll need to check the last error (e.g.
chrome.test.assertTrue(!!chrome.runtime.lastError);
chrome.test.assertEq('Only cancel allowed in Public Sessions.',
                     chrome.runtime.lastError.message);

It would probably be easiest to do this bit without the framework.js, but
probably doable either way.

https://codereview.chromium.org/2455393002/diff/280001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_permissions.h (right):

https://codereview.chromium.org/2455393002/diff/280001/extensions/browser/api...
extensions/browser/api/web_request/web_request_permissions.h:54: static bool
allow_all_extensions_in_public_sessions_;
On 2016/11/14 20:16:13, Ivan Šandrk wrote:
> Added the runtime bool like this, but I'm unable to access it from the apitest
> (... is a private member of WebRequestPermissions). Also tried using a private
> setter function to no avail. Help?

Hmm... I can't think of why that would be - it seems like this should work. 
I'll try patching it in locally tomorrow to take a look.

[Ivan Šandrk, 2016-11-15 13:46:41]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 13:46:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-11-15 13:49:58]

This should be it!

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api.cc (right):

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:1134: event_details =
event_details->WhitelistedCopyForPublicSession();
On 2016/11/15 01:19:37, Devlin wrote:
> On 2016/11/14 20:16:13, Ivan Šandrk wrote:
> > On 2016/11/14 17:44:38, Devlin wrote:
> > > This sanitizes the input for *all* listeners, not just the extension that
> > might
> > > be force-installed.  Is that desired behavior?  If so, we should comment
to
> > make
> > > it explicit.
> > > 
> > > Additionally, if that's the case, we don't need to make a copy of event
> > details;
> > > we can just modify the current object.
> > 
> > Yes.
> > 
> > I guess I could also use some help here. I couldn't assign the new object
> inside
> > the WhitelistedCopyForPublicSession because the assignment operator is
deleted
> > and I wanted to make the smallest change to the existing code, plus I was
> trying
> > to go with the approach of a clean copy, and then just copying over the safe
> > fields (future-proofing). Got any suggestions here?
> 
> If we're okay with sanitizing for all listeners, which it sounds like we are,
> then I'd say we should just make a ScrubForPublicSession() method or similar
on
> WebRequestEventDetails that clears those fields on itself.  Would that work?

Sure, done.

https://codereview.chromium.org/2455393002/diff/260001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api.cc:2233: if
(!IsPublicSession()) {
On 2016/11/15 01:19:37, Devlin wrote:
> On 2016/11/14 20:16:13, Ivan Šandrk wrote:
> > On 2016/11/14 17:44:38, Devlin wrote:
> > > Could we instead have this be an early-out error?  i.e. something like:
> > > if (IsPublicSession() &&
> > >     (value->HasKey("redirectUrl") ||
> > >      value->HasKey(keys::kAuthCredentialsKey) ||
> > >      value->HasKey("requestHeaders") ||
> > >      value->HasKey("responseHeaders")) {
> > >   return RespondNow(Error("Only cancel allowed in public sessions"));
> > > }
> > > 
> > > I think it would make it a bit easier to read (and the diff a lot cleaner)
> > 
> > Agreed completely and done. But now I broke the redirectIsBlocked test, and
> I'm
> > unsure how to restructure it. It gives the error message "Unchecked
> > runtime.lastError while running webRequestInternal.eventHandled: Only cancel
> > allowed in Public Sessions.", and then timeouts.
> > 
> > I guess I was relying on the silent ignore that was previously observed. Got
> any
> > ideas/pointers/hints?
> 
> The basic approach here will be that you'll need to check the last error (e.g.
> chrome.test.assertTrue(!!chrome.runtime.lastError);
> chrome.test.assertEq('Only cancel allowed in Public Sessions.',
>                      chrome.runtime.lastError.message);
> 
> It would probably be easiest to do this bit without the framework.js, but
> probably doable either way.

That's the first approach which I tried, but couldn't get it working. lastError
wasn't being set until after the test.js script was run (for reasons I'm not
aware of).

I managed to get it working today, turns out I forgot to call OnError() before
returning RespondNow(Error()). Simple as that heh.

https://codereview.chromium.org/2455393002/diff/280001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_permissions.h (right):

https://codereview.chromium.org/2455393002/diff/280001/extensions/browser/api...
extensions/browser/api/web_request/web_request_permissions.h:54: static bool
allow_all_extensions_in_public_sessions_;
On 2016/11/15 01:19:37, Devlin wrote:
> On 2016/11/14 20:16:13, Ivan Šandrk wrote:
> > Added the runtime bool like this, but I'm unable to access it from the
apitest
> > (... is a private member of WebRequestPermissions). Also tried using a
private
> > setter function to no avail. Help?
> 
> Hmm... I can't think of why that would be - it seems like this should work. 
> I'll try patching it in locally tomorrow to take a look.

Got it working in a bit different way (suggestion by Drew). It seems that
functions ending with "ForTesting" generate presubmit warnings if called from
non-testing code :-)

https://codereview.chromium.org/2455393002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2455393002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:177:
WebRequestPermissions::AllowAllExtensionLocationsInPublicSessionForTesting(
QQ - Does each test run from a clean state or is some part of state kept between
tests? I guess I can drop this line if each test is clean.

[commit-bot: I haz the power, 2016-11-15 13:56:31]

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

[commit-bot: I haz the power, 2016-11-15 13:56:32]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Ivan Šandrk, 2016-11-15 14:30:16]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 14:30:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-11-15 15:12:03]

https://codereview.chromium.org/2455393002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_request/web_request_apitest.cc (right):

https://codereview.chromium.org/2455393002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_apitest.cc:177:
WebRequestPermissions::AllowAllExtensionLocationsInPublicSessionForTesting(
On 2016/11/15 13:49:58, Ivan Šandrk wrote:
> QQ - Does each test run from a clean state or is some part of state kept
between
> tests? I guess I can drop this line if each test is clean.

Short answer: depends on the test and the state you are referring to.  Keep this
line. :)

Longer answer: Browser tests do a complete tear down/re set up after each test,
which would clear this bit.  However, that's only because we don't run multiple
browser tests in the same process for now, which we do for unit tests.  So if
this were a unit test, this line *would* be strictly necessary.  To make it more
confusing, unittesting has a bit of state reset between tests, namely the
command line, because so many unittests mangle it.

https://codereview.chromium.org/2455393002/diff/340001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:53:
std::string copy_str, orig_str;
nit: chromium style says one variable per line, so
std::string copy_str;
std::string orig_str;

Though this would be moot with below

https://codereview.chromium.org/2455393002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:54:
copy->dict_.GetString(safe_attr, &copy_str);
We should know that the string value is |safe_attr|, right?  So could we instead
just do
EXPECT_EQ(safe_attr, str);
?

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest_public_session/framework.js
(right):

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/framework.js:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
Is this file just copy-pasted?

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest_public_session/test.js
(right):

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/test.js:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
no (c)

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/test.js:61:
function redirectIsBlocked() {
From your comment, it sounded like you got checking the error figured out, but I
don't see it here in the test?

https://codereview.chromium.org/2455393002/diff/340001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api_constants.cc (right):

https://codereview.chromium.org/2455393002/diff/340001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api_constants.cc:66: "Only cancel
allowed in Public Sessions.";
Maybe rephrase this to
"Only the 'cancel' action is allowed in Public Sessions."
?

https://codereview.chromium.org/2455393002/diff/340001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/2455393002/diff/340001/extensions/browser/api...
extensions/browser/api/web_request/web_request_permissions.cc:114: bool
g_allow_all_extension_locations_in_public_session = false;
This should go in the existing anonymous namespace at line 83ish

[commit-bot: I haz the power, 2016-11-15 15:58:04]

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

[commit-bot: I haz the power, 2016-11-15 15:58:05]

Dry run: This issue passed the CQ dry run.

[Ivan Šandrk, 2016-11-15 16:06:11]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 16:06:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-11-15 16:07:40]

Thanks for explaining how tests are setup and torn down :)

https://codereview.chromium.org/2455393002/diff/340001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:53:
std::string copy_str, orig_str;
On 2016/11/15 15:12:02, Devlin (Busy Nov 15) wrote:
> nit: chromium style says one variable per line, so
> std::string copy_str;
> std::string orig_str;
> 
> Though this would be moot with below

Done.

https://codereview.chromium.org/2455393002/diff/340001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_event_details_unittest.cc:54:
copy->dict_.GetString(safe_attr, &copy_str);
On 2016/11/15 15:12:02, Devlin (Busy Nov 15) wrote:
> We should know that the string value is |safe_attr|, right?  So could we
instead
> just do
> EXPECT_EQ(safe_attr, str);
> ?

Good point, done.

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest_public_session/framework.js
(right):

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/framework.js:1:
// Copyright (c) 2012 The Chromium Authors. All rights reserved.
On 2016/11/15 15:12:02, Devlin (Busy Nov 15) wrote:
> Is this file just copy-pasted?

Yes. I couldn't figure out how to include the original one. Putting <script
src="../webrequest/framework.js"> into test.html doesn't work.

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest_public_session/test.js
(right):

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/test.js:1: //
Copyright (c) 2016 The Chromium Authors. All rights reserved.
On 2016/11/15 15:12:02, Devlin (Busy Nov 15) wrote:
> no (c)

Done.

https://codereview.chromium.org/2455393002/diff/340001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest_public_session/test.js:61:
function redirectIsBlocked() {
On 2016/11/15 15:12:02, Devlin (Busy Nov 15) wrote:
> From your comment, it sounded like you got checking the error figured out, but
I
> don't see it here in the test?

Yes, the test is working now, with *no* changes to it. The problem was in the
other part, I wasn't calling OnError() before returning RespondNow(Error()).

https://codereview.chromium.org/2455393002/diff/340001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api_constants.cc (right):

https://codereview.chromium.org/2455393002/diff/340001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api_constants.cc:66: "Only cancel
allowed in Public Sessions.";
On 2016/11/15 15:12:02, Devlin (Busy Nov 15) wrote:
> Maybe rephrase this to
> "Only the 'cancel' action is allowed in Public Sessions."
> ?

Done.

https://codereview.chromium.org/2455393002/diff/340001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_permissions.cc (right):

https://codereview.chromium.org/2455393002/diff/340001/extensions/browser/api...
extensions/browser/api/web_request/web_request_permissions.cc:114: bool
g_allow_all_extension_locations_in_public_session = false;
On 2016/11/15 15:12:02, Devlin (Busy Nov 15) wrote:
> This should go in the existing anonymous namespace at line 83ish

Done.

[commit-bot: I haz the power, 2016-11-15 16:08:41]

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

[commit-bot: I haz the power, 2016-11-15 16:08:42]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Ivan Šandrk, 2016-11-15 16:58:56]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-15 16:59:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-15 18:51:50]

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

[commit-bot: I haz the power, 2016-11-15 18:51:51]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Devlin, 2016-11-16 02:42:37]

lgtm; thanks for your patience!

https://codereview.chromium.org/2455393002/diff/370001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/370001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:207:
GURL("http://example.org"), net::DEFAULT_PRIORITY, NULL));
nit: nullptr in new code

https://codereview.chromium.org/2455393002/diff/370001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:240:
WebRequestPermissions::REQUIRE_ALL_URLS));
Let's also add a test for a chrome:// url, which is something we explicitly
disallow everywhere else.

[Ivan Šandrk, 2016-11-16 10:01:17]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 10:01:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-11-16 10:02:41]

The CQ bit was unchecked by isandrk@chromium.org

[Ivan Šandrk, 2016-11-16 10:04:59]

The CQ bit was checked by isandrk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-16 10:05:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ivan Šandrk, 2016-11-16 10:07:51]

All done. Thanks for your help!

https://codereview.chromium.org/2455393002/diff/370001/chrome/browser/extensi...
File
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc
(right):

https://codereview.chromium.org/2455393002/diff/370001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:207:
GURL("http://example.org"), net::DEFAULT_PRIORITY, NULL));
On 2016/11/16 02:42:37, Devlin (Busy Nov 15) wrote:
> nit: nullptr in new code

Done.

https://codereview.chromium.org/2455393002/diff/370001/chrome/browser/extensi...
chrome/browser/extensions/api/web_request/web_request_permissions_unittest.cc:240:
WebRequestPermissions::REQUIRE_ALL_URLS));
On 2016/11/16 02:42:37, Devlin (Busy Nov 15) wrote:
> Let's also add a test for a chrome:// url, which is something we explicitly
> disallow everywhere else.

Done.

[commit-bot: I haz the power, 2016-11-16 12:02:12]

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

[commit-bot: I haz the power, 2016-11-16 12:02:13]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Ivan Šandrk, 2016-11-16 12:19:00]

The CQ bit was checked by isandrk@chromium.org

[Ivan Šandrk, 2016-11-16 12:19:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
atwilson@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2455393002/#ps410001
(title: "webRequest and webRequestBlocking are safe permissions now")

[commit-bot: I haz the power, 2016-11-16 12:19:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-16 13:03:07]

Description was changed from

==========
Filter out privacy/security sensitive data from webRequests in Public Sessions.

Enable all URLs for webRequest for regular extensions in Public Sessions. Note:
Origin/URL permissions are completely disabled in Public Sessions for
privacy/security reasons so this is a workaround.

Doc explaining the changes to webRequest API in details
go/webrequest-whitelisting.

BUG=661678
==========

to

==========
Filter out privacy/security sensitive data from webRequests in Public Sessions.

Enable all URLs for webRequest for regular extensions in Public Sessions. Note:
Origin/URL permissions are completely disabled in Public Sessions for
privacy/security reasons so this is a workaround.

Doc explaining the changes to webRequest API in details
go/webrequest-whitelisting.

BUG=661678
==========

[commit-bot: I haz the power, 2016-11-16 13:03:09]

Committed patchset #21 (id:410001)

[commit-bot: I haz the power, 2016-11-16 13:07:09]

Description was changed from

==========
Filter out privacy/security sensitive data from webRequests in Public Sessions.

Enable all URLs for webRequest for regular extensions in Public Sessions. Note:
Origin/URL permissions are completely disabled in Public Sessions for
privacy/security reasons so this is a workaround.

Doc explaining the changes to webRequest API in details
go/webrequest-whitelisting.

BUG=661678
==========

to

==========
Filter out privacy/security sensitive data from webRequests in Public Sessions.

Enable all URLs for webRequest for regular extensions in Public Sessions. Note:
Origin/URL permissions are completely disabled in Public Sessions for
privacy/security reasons so this is a workaround.

Doc explaining the changes to webRequest API in details
go/webrequest-whitelisting.

BUG=661678

Committed: https://crrev.com/3e7318b652b0fa22e68b3cb4de0e93cfef12f15c
Cr-Commit-Position: refs/heads/master@{#432469}
==========

[commit-bot: I haz the power, 2016-11-16 13:07:11]

Patchset 21 (id:??) landed as
https://crrev.com/3e7318b652b0fa22e68b3cb4de0e93cfef12f15c
Cr-Commit-Position: refs/heads/master@{#432469}