PS - Filtering activeTab URL

PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through activeTab permission) down to the origin.

This change also causes the tab object passed to the [page|browser]Action.onClicked to be scrubbed for the given extension.

TEST=
  unit_tests --gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*

BUG=717945
Review-Url: https://codereview.chromium.org/2858643002
Cr-Commit-Position: refs/heads/master@{#469342}
Committed: https://chromium.googlesource.com/chromium/src/+/1296202771665ed3ecd53b31c16391258933cb08

[Ivan Šandrk, 2017-05-02 18:08:21]

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

[commit-bot: I haz the power, 2017-05-02 18:08:44]

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, 2017-05-02 18:55:17]

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

[commit-bot: I haz the power, 2017-05-02 18:55:20]

Dry run: This issue passed the CQ dry run.

[Ivan Šandrk, 2017-05-03 11:56:11]

Description was changed from

==========
Filtering activeTab URL (PlatformSpecificFiltering -> some refactoring)

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

to

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:

- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

BUG=717945
==========

[Ivan Šandrk, 2017-05-03 12:02:08]

Description was changed from

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:

- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

BUG=717945
==========

to

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

Implementation notes:
- DeviceLocalAccountManagementPolicyProvider::IsWhitelisted is changed to take
an extension_id (instead of extension) so it's more versatile;
- ExtensionTabUtilDelegateChromeOS::ScrubTabForExtension takes extension_id/tab
(instead of extension/context/tab) - versatility;
- ScrubTabForExtension also checks whether the extension is whitelisted now
(don't want to filter URL for whitelisted).

BUG=717945
==========

[Ivan Šandrk, 2017-05-03 12:09:50]

Description was changed from

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

Implementation notes:
- DeviceLocalAccountManagementPolicyProvider::IsWhitelisted is changed to take
an extension_id (instead of extension) so it's more versatile;
- ExtensionTabUtilDelegateChromeOS::ScrubTabForExtension takes extension_id/tab
(instead of extension/context/tab) - versatility;
- ScrubTabForExtension also checks whether the extension is whitelisted now
(don't want to filter URL for whitelisted).

BUG=717945
==========

to

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

Implementation notes:
- DeviceLocalAccountManagementPolicyProvider::IsWhitelisted is changed to take
an extension_id (instead of extension) so it's more versatile;
- ExtensionTabUtilDelegateChromeOS::ScrubTabForExtension takes extension_id/tab
(instead of extension/context/tab) - versatility;
- ScrubTabForExtension also checks whether the extension is whitelisted now
(don't want to filter URL for whitelisted).

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945
==========

[Ivan Šandrk, 2017-05-03 12:59:36]

Description was changed from

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

Implementation notes:
- DeviceLocalAccountManagementPolicyProvider::IsWhitelisted is changed to take
an extension_id (instead of extension) so it's more versatile;
- ExtensionTabUtilDelegateChromeOS::ScrubTabForExtension takes extension_id/tab
(instead of extension/context/tab) - versatility;
- ScrubTabForExtension also checks whether the extension is whitelisted now
(don't want to filter URL for whitelisted).

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945
==========

to

==========
PS - activeTab 1/2 - Filtering URL

This is part #1 of changes necessary for activeTab. More info about #2 at linked
bug.

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

Implementation notes:
- DeviceLocalAccountManagementPolicyProvider::IsWhitelisted is changed to take
an extension_id (instead of extension) so it's more versatile;
- ExtensionTabUtilDelegateChromeOS::ScrubTabForExtension takes extension_id/tab
(instead of extension/context/tab) - versatility;
- ScrubTabForExtension also checks whether the extension is whitelisted now
(don't want to filter URL for whitelisted).

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945
==========

[Ivan Šandrk, 2017-05-03 13:00:50]

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

[Ivan Šandrk, 2017-05-03 13:00:51]

Hey Devlin, Drew, ptal!

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:230:
ExtensionTabUtil::PlatformSpecificFiltering(extension_action.extension_id(),
Should I test this part maybe?

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_util.cc:133:
g_delegate->ScrubTabForExtension(extension_id, tab);
PlatformSpecificFiltering vs. ScrubTabForExtension

Seems as now I have a naming inconsistency maybe? Anyway I think I prefer the
name PlatformSpecificFiltering. WDYT?

[Devlin, 2017-05-03 15:14:15]

Optional description nits:

I personally prefer when CLs don't have "1/n", just because it seems that <n>
can often change over time. :)  Instead, I'd focus on what this CL does as an
independent patch (you can still allude to future CLs, e.g. "followups will do
xyz", but it shouldn't be necessary).

A few of the implementation notes in the description are a bit *too* specific
(for instance, the notes about changing the method signature).  If anyone's
really interested, they can look at the raw diff, and chances are people would
just find this through git blame anyway.  And, similar to above, things have a
funny way of not always remaining accurate (I've been guilty of accidentally
forgetting to update a CL description and having it be out-of-date).  Trying to
focus on the behavior changes, rather than the implementation, can make the CL
descriptions easier to read and understand, and less likely to stale out.

That said... all that's just my $0.02, and they are very small nits (despite the
length of the comment - sorry!).

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/chromeos/ext...
File
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos_unittest.cc:58:
chromeos::LoginState::LOGGED_IN_ACTIVE,
drive-by: indentation looks off here.

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos_unittest.cc:71:
chromeos::LoginState::Initialize();
This pattern:
chromeos::LoginState::Initialize();
chromeos::LoginState::Get()->SetLoggedInState(
    chromeos::LoginState::LOGGED_IN_ACTIVE,
    chromeos::LoginState::LOGGED_IN_USER_PUBLIC_ACCOUNT);

...

chromeos::LoginState::Shutdown();

is getting more common.  What would you think of adding a public test utility
class somewhere, like this:

// A class to start and shutdown public session state for a test.
class TestScopedLoginState {
 public:
  TestScopedLoginState() {
    chromeos::LoginState::Initialize();
    chromeos::LoginState::Get()->SetLoggedInState(
        chromeos::LoginState::LOGGED_IN_ACTIVE,
        chromeos::LoginState::LOGGED_IN_USER_PUBLIC_ACCOUNT);
  }
  ~TestScopedLoginState() {
    chromeos::LoginState::Shutdown();
  }
};

and then in these tests, we just do:
TEST_F(...) {
  TestScopedLoginState public_session_state;
  // Rest of the test.
  // No need to call shutdown at the end; the dtor of TestScopedLoginState
handles it.
}

(Other names might be better than TestScopedLoginState - ScopedPublicSession,
etc would also work.)

WDYT?

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos_unittest.cc:72:
chromeos::LoginState::Get()->SetLoggedInState(
ditto re indentation

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:230:
ExtensionTabUtil::PlatformSpecificFiltering(extension_action.extension_id(),
On 2017/05/03 13:00:50, Ivan Šandrk wrote:
> Should I test this part maybe?

Actually, I think that what we should be doing is passing in the extension as
part of the CreateTabObject call so that we perform the scrubbing there.  I
think it's probably a bug that we don't do that already.

Note that doing this *could* potentially break extensions if they happen to be
relying on getting the url of the tab while also not having host permission,
tabs permission, or activeTab permission, but I think that's the right behavior.
 We shouldn't be surfacing this information to the extension otherwise.

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_tab_util.cc (right):

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_tab_util.cc:133:
g_delegate->ScrubTabForExtension(extension_id, tab);
On 2017/05/03 13:00:51, Ivan Šandrk wrote:
> PlatformSpecificFiltering vs. ScrubTabForExtension
> 
> Seems as now I have a naming inconsistency maybe? Anyway I think I prefer the
> name PlatformSpecificFiltering. WDYT?

I don't feel strongly on the naming, but see also comment in
extension_action_api.cc that would make this method unnecessary.

[Ivan Šandrk, 2017-05-03 16:29:49]

Description was changed from

==========
PS - activeTab 1/2 - Filtering URL

This is part #1 of changes necessary for activeTab. More info about #2 at linked
bug.

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

Implementation notes:
- DeviceLocalAccountManagementPolicyProvider::IsWhitelisted is changed to take
an extension_id (instead of extension) so it's more versatile;
- ExtensionTabUtilDelegateChromeOS::ScrubTabForExtension takes extension_id/tab
(instead of extension/context/tab) - versatility;
- ScrubTabForExtension also checks whether the extension is whitelisted now
(don't want to filter URL for whitelisted).

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945
==========

to

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945
==========

[Ivan Šandrk, 2017-05-03 17:39:36]

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

[commit-bot: I haz the power, 2017-05-03 17:40:06]

Dry run: CQ is trying da patch.

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

[Ivan Šandrk, 2017-05-03 17:52:46]

On 2017/05/03 15:14:15, Devlin wrote:
> Optional description nits:
> 
> I personally prefer when CLs don't have "1/n", just because it seems that <n>
> can often change over time. :)  Instead, I'd focus on what this CL does as an
> independent patch (you can still allude to future CLs, e.g. "followups will do
> xyz", but it shouldn't be necessary).
> 
> A few of the implementation notes in the description are a bit *too* specific
> (for instance, the notes about changing the method signature).  If anyone's
> really interested, they can look at the raw diff, and chances are people would
> just find this through git blame anyway.  And, similar to above, things have a
> funny way of not always remaining accurate (I've been guilty of accidentally
> forgetting to update a CL description and having it be out-of-date).  Trying
to
> focus on the behavior changes, rather than the implementation, can make the CL
> descriptions easier to read and understand, and less likely to stale out.
> 
> That said... all that's just my $0.02, and they are very small nits (despite
the
> length of the comment - sorry!).

I see, good points. I mistakenly thought it would make reviewing easier :)


Btw, after changing CreateTabObject call to also take the extension, I've
decided to keep some of the changes I introduced  earlier (namely IsWhitelisted
still takes extension_id - more versatility).

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/chromeos/ext...
File
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos_unittest.cc
(right):

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/extension_tab_util_delegate_chromeos_unittest.cc:71:
chromeos::LoginState::Initialize();
On 2017/05/03 15:14:15, Devlin wrote:
> This pattern:
> chromeos::LoginState::Initialize();
> chromeos::LoginState::Get()->SetLoggedInState(
>     chromeos::LoginState::LOGGED_IN_ACTIVE,
>     chromeos::LoginState::LOGGED_IN_USER_PUBLIC_ACCOUNT);
> 
> ...
> 
> chromeos::LoginState::Shutdown();
> 
> is getting more common.  What would you think of adding a public test utility
> class somewhere, like this:
> 
> // A class to start and shutdown public session state for a test.
> class TestScopedLoginState {
>  public:
>   TestScopedLoginState() {
>     chromeos::LoginState::Initialize();
>     chromeos::LoginState::Get()->SetLoggedInState(
>         chromeos::LoginState::LOGGED_IN_ACTIVE,
>         chromeos::LoginState::LOGGED_IN_USER_PUBLIC_ACCOUNT);
>   }
>   ~TestScopedLoginState() {
>     chromeos::LoginState::Shutdown();
>   }
> };
> 
> and then in these tests, we just do:
> TEST_F(...) {
>   TestScopedLoginState public_session_state;
>   // Rest of the test.
>   // No need to call shutdown at the end; the dtor of TestScopedLoginState
> handles it.
> }
> 
> (Other names might be better than TestScopedLoginState - ScopedPublicSession,
> etc would also work.)
> 
> WDYT?

Yes, perfect! Done.

Logged a bug for myself to refactor other usages crbug.com/718078

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/2858643002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:230:
ExtensionTabUtil::PlatformSpecificFiltering(extension_action.extension_id(),
On 2017/05/03 15:14:15, Devlin wrote:
> On 2017/05/03 13:00:50, Ivan Šandrk wrote:
> > Should I test this part maybe?
> 
> Actually, I think that what we should be doing is passing in the extension as
> part of the CreateTabObject call so that we perform the scrubbing there.  I
> think it's probably a bug that we don't do that already.
> 
> Note that doing this *could* potentially break extensions if they happen to be
> relying on getting the url of the tab while also not having host permission,
> tabs permission, or activeTab permission, but I think that's the right
behavior.
>  We shouldn't be surfacing this information to the extension otherwise.

Right, thanks for the suggestion. Done.

[Ivan Šandrk, 2017-05-03 17:54:47]

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

[Ivan Šandrk, 2017-05-03 17:54:47]

Alexander, ptal at chromeos/login!

[commit-bot: I haz the power, 2017-05-03 18:18:56]

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

[commit-bot: I haz the power, 2017-05-03 18:18:58]

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, 2017-05-03 19:31:53]

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

[commit-bot: I haz the power, 2017-05-03 19:32:21]

Dry run: CQ is trying da patch.

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

[Ivan Šandrk, 2017-05-03 19:45:10]

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

[commit-bot: I haz the power, 2017-05-03 19:45:20]

Dry run: CQ is trying da patch.

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

[Ivan Šandrk, 2017-05-03 19:46:27]

Apparently I forgot to update one of the tests, that's fixed now. Ptal

[commit-bot: I haz the power, 2017-05-03 20:52:30]

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

[commit-bot: I haz the power, 2017-05-03 20:52:30]

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_...)

[Alexander Alekseev, 2017-05-04 01:19:29]

chromeos/login/ lgtm

[Andrew T Wilson (Slow), 2017-05-04 14:33:01]

lgtm, you might consider breaking out the refactoring from the functionality
change though, since I'm not sure they are really related.

[Ivan Šandrk, 2017-05-04 14:52:53]

On 2017/05/04 14:33:01, Andrew T Wilson (Slow) wrote:
> lgtm, you might consider breaking out the refactoring from the functionality
> change though, since I'm not sure they are really related.

I will do that. I'll keep the refactoring in this CL, and create a new one for
the actual changes (and those are actually quite small).

[Devlin, 2017-05-04 15:08:39]

Nice, LGTM!

One more nit: it might be worth mentioning in the CL description that this also
causes the tab object passed to the [page|browser]Action.onClicked to be
scrubbed for the given extension.

[Ivan Šandrk, 2017-05-04 15:12:30]

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

[commit-bot: I haz the power, 2017-05-04 15:12:48]

Dry run: CQ is trying da patch.

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

[Ivan Šandrk, 2017-05-04 15:13:52]

Patchset #5 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-05-04 15:15:26]

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, 2017-05-04 15:15:36]

On 2017/05/04 15:08:39, Devlin wrote:
> Nice, LGTM!
> 
> One more nit: it might be worth mentioning in the CL description that this
also
> causes the tab object passed to the [page|browser]Action.onClicked to be
> scrubbed for the given extension.

I guess at this point I'll drop the refactoring heh :D

And will update the CL description.

[Ivan Šandrk, 2017-05-04 15:20:00]

Description was changed from

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945
==========

to

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

This change also causes the tab object passed to the
[page|browser]Action.onClicked to be scrubbed for the given extension.

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945
==========

[Ivan Šandrk, 2017-05-04 15:20:17]

The CQ bit was unchecked by isandrk@chromium.org

[Ivan Šandrk, 2017-05-04 15:20:18]

The CQ bit was checked by isandrk@chromium.org

[commit-bot: I haz the power, 2017-05-04 15:20:26]

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, 2017-05-04 15:27:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493911218653850,
"parent_rev": "6efb1855ab2713794bf632ed88b9bc353bf5ec0a", "commit_rev":
"1296202771665ed3ecd53b31c16391258933cb08"}

[commit-bot: I haz the power, 2017-05-04 15:27:46]

Description was changed from

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

This change also causes the tab object passed to the
[page|browser]Action.onClicked to be scrubbed for the given extension.

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945
==========

to

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

This change also causes the tab object passed to the
[page|browser]Action.onClicked to be scrubbed for the given extension.

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945

Review-Url: https://codereview.chromium.org/2858643002
Cr-Commit-Position: refs/heads/master@{#469342}
Committed:
https://chromium.googlesource.com/chromium/src/+/1296202771665ed3ecd53b31c163...
==========

[commit-bot: I haz the power, 2017-05-04 15:27:47]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/1296202771665ed3ecd53b31c163...

[Ivan Šandrk, 2017-05-15 16:50:03]

Description was changed from

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

This change also causes the tab object passed to the
[page|browser]Action.onClicked to be scrubbed for the given extension.

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*
  unit_tests --gtest_filter=ExtensionTabUtilTest.Delegate

BUG=717945

Review-Url: https://codereview.chromium.org/2858643002
Cr-Commit-Position: refs/heads/master@{#469342}
Committed:
https://chromium.googlesource.com/chromium/src/+/1296202771665ed3ecd53b31c163...
==========

to

==========
PS - Filtering activeTab URL

In Public Sessions, apps and extensions are force-installed by admin policy so
the user does not get a chance to review the permissions for these apps. This is
not acceptable from a security standpoint, so we:
- scrub the URL available to chrome.tabs.executeScript context (through
activeTab permission) down to the origin.

This change also causes the tab object passed to the
[page|browser]Action.onClicked to be scrubbed for the given extension.

TEST=
  unit_tests
--gtest_filter=DeviceLocalAccountManagementPolicyProviderTest.IsWhitelisted
  unit_tests --gtest_filter=ExtensionTabUtilDelegateChromeOSTest.*

BUG=717945

Review-Url: https://codereview.chromium.org/2858643002
Cr-Commit-Position: refs/heads/master@{#469342}
Committed:
https://chromium.googlesource.com/chromium/src/+/1296202771665ed3ecd53b31c163...
==========