Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2766263003:
Extensions: Only load incognito-enabled extensions in an incognito renderer. (Closed)
|
Created:
3 years, 9 months ago by karandeepb Modified:
3 years, 8 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExtensions: Only load incognito-enabled extensions in an incognito renderer.
Currently, extensions which are disabled in incognito mode are also loaded in an
incognito renderer. This is redundant and can cause non-required bindings to be
generated in the renderer. Also, it has potential security implications.
Prevent this by only loading incognito-enabled extensions in an incognito
renderer. However extensions which can't be enabled in the incognito mode (e.g.
platform apps etc.) still need to be loaded in incognito renderers, so that
incognito tabs can connect with them (issue 305394).
Also, add tests for the same.
BUG=527548
Review-Url: https://codereview.chromium.org/2766263003
Cr-Commit-Position: refs/heads/master@{#462707}
Committed: https://chromium.googlesource.com/chromium/src/+/18ab4ab8c381424b30163a763515ecc0fdfc8db2
Patch Set 1 : -- #Patch Set 2 : -- #
Total comments: 18
Patch Set 3 : Rebase. #Patch Set 4 : Address review. #Patch Set 5 : -- #Patch Set 6 : -- #Patch Set 7 : Rebase. #
Total comments: 1
Patch Set 8 : Add another test. Nits. #Patch Set 9 : Split test cleanups to another CL. #Patch Set 10 : Nits. #
Total comments: 2
Patch Set 11 : Rebase. #Patch Set 12 : Address review #
Depends on Patchset: Messages
Total messages: 43 (34 generated)
Description was changed from ========== Extensions Incognito 2 ========== to ========== Extensions: Only load incognito-enabled extensions in an incognito renderer. Currently, extensions which are disabled in incognito mode are also loaded in an incognito renderer. This is redundant as this means bindings for extensions which should be disabled are also loaded in an incognito renderer. Also, it has potential security implications. Prevent this by only loading incognito-enabled extensions in an incognito renderer. However extensions which can't be enabled in the incognito mode (e.g. platform apps etc.) still need to be loaded in incognito renderers, so that incognito tabs can connect with them (issue 305394). BUG=527548 ==========
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
karandeepb@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
PTAL Devlin. This is dependent on two CLs, however this can probably be given an initial review first. Will proceed to add tests if you think this seems fine. https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/message_service.cc:335: DCHECK(!util::CanBeIncognitoEnabled(target_extension)); At least in this case, CanBeIncognitoEnabled seems like a misnomer. Wonder if we should also introduce a separate SupportsIncognitoToggle or maybe we can directly use checks to find whether it is a platform app or component extension.
Overall, this seems like the right idea. A few subtle cases here, but overall looks like we're on the right track! https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/message_service.cc:335: DCHECK(!util::CanBeIncognitoEnabled(target_extension)); On 2017/03/23 02:54:03, karandeepb wrote: > At least in this case, CanBeIncognitoEnabled seems like a misnomer. Wonder if we > should also introduce a separate SupportsIncognitoToggle or maybe we can > directly use checks to find whether it is a platform app or component extension. > I'm not sure I love SupportsIncognitoToggle, since it's pretty UI-dependent, and, as mentioned in the bug, might lead to confusion if policy settings get involved. I think what we're trying to convey here is that the extension can't ever "run" in an incognito context (and that assumption is valid here - we only pass messages back and forth). But CanExtensionBeEnabledToRunInIncognito is too long. ;) If you have other suggestions, I'm happy to hear them, but I don't think the name is dramatically incorrect. https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/message_service.cc:335: DCHECK(!util::CanBeIncognitoEnabled(target_extension)); Is this check correct? What about the case of a website trying to connect with an extension that can be incognito enabled, but isn't? (Given this passed on the bots, does that indicate we need another test?) https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_messages_apitest.cc (right): https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_messages_apitest.cc:803: extension.get(), NULL)); prefer nullptr in new code https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_messages_apitest.cc:803: extension.get(), NULL)); Subtle: when we reload the extension, the Extension object that we previously had (stored here in |extension|) is no longer the "current" extension stored in the registry. Instead, we can use TestExtensionRegistryObserver::WaitForExtensionLoaded(), which returns the new Extension object. https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_messages_apitest.cc:982: extension.get(), NULL)); (Same comment here) https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (left): https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:164: return; I wonder if it's worth keeping this short-circuit, since the creation of ExtensionMsg_Loaded_Params is non-trivial, and we know that no process ever needs to know about themes. https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:43: // ensure connections from incognito tabs to such extensions work fine. nitty nit: omit ' fine' -> 'ensure connections from incognito tabs to such extensions work.' https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:125: if (!IsExtensionVisibleToProcess(*ext, process)) nitty nit: We actually only need the browser context here, so we can cache it before iterating through the loop and just pass it directly, saving the (admittedly very cheap) repeated call to RenderProcessHost::GetBrowserContext(). (Note that you'll still want to use process->GetBrowserContext() when you cache it, rather than passing in browser_context_, since this handles both on- and off-the-record versions.)
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Extensions: Only load incognito-enabled extensions in an incognito renderer. Currently, extensions which are disabled in incognito mode are also loaded in an incognito renderer. This is redundant as this means bindings for extensions which should be disabled are also loaded in an incognito renderer. Also, it has potential security implications. Prevent this by only loading incognito-enabled extensions in an incognito renderer. However extensions which can't be enabled in the incognito mode (e.g. platform apps etc.) still need to be loaded in incognito renderers, so that incognito tabs can connect with them (issue 305394). BUG=527548 ========== to ========== Extensions: Only load incognito-enabled extensions in an incognito renderer. Currently, extensions which are disabled in incognito mode are also loaded in an incognito renderer. This is redundant and has potential security implications. Prevent this by only loading incognito-enabled extensions in an incognito renderer. However extensions which can't be enabled in the incognito mode (e.g. platform apps etc.) still need to be loaded in incognito renderers, so that incognito tabs can connect with them (issue 305394). BUG=527548 ==========
Description was changed from ========== Extensions: Only load incognito-enabled extensions in an incognito renderer. Currently, extensions which are disabled in incognito mode are also loaded in an incognito renderer. This is redundant and has potential security implications. Prevent this by only loading incognito-enabled extensions in an incognito renderer. However extensions which can't be enabled in the incognito mode (e.g. platform apps etc.) still need to be loaded in incognito renderers, so that incognito tabs can connect with them (issue 305394). BUG=527548 ========== to ========== Extensions: Only load incognito-enabled extensions in an incognito renderer. Currently, extensions which are disabled in incognito mode are also loaded in an incognito renderer. This is redundant and can cause non-required bindings to be generated in the renderer. Also, it has potential security implications. Prevent this by only loading incognito-enabled extensions in an incognito renderer. However extensions which can't be enabled in the incognito mode (e.g. platform apps etc.) still need to be loaded in incognito renderers, so that incognito tabs can connect with them (issue 305394). BUG=527548 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Extensions: Only load incognito-enabled extensions in an incognito renderer. Currently, extensions which are disabled in incognito mode are also loaded in an incognito renderer. This is redundant and can cause non-required bindings to be generated in the renderer. Also, it has potential security implications. Prevent this by only loading incognito-enabled extensions in an incognito renderer. However extensions which can't be enabled in the incognito mode (e.g. platform apps etc.) still need to be loaded in incognito renderers, so that incognito tabs can connect with them (issue 305394). BUG=527548 ========== to ========== Extensions: Only load incognito-enabled extensions in an incognito renderer. Currently, extensions which are disabled in incognito mode are also loaded in an incognito renderer. This is redundant and can cause non-required bindings to be generated in the renderer. Also, it has potential security implications. Prevent this by only loading incognito-enabled extensions in an incognito renderer. However extensions which can't be enabled in the incognito mode (e.g. platform apps etc.) still need to be loaded in incognito renderers, so that incognito tabs can connect with them (issue 305394). Also, add tests for the same. BUG=527548 ==========
The CQ bit was checked by karandeepb@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL Devlin. https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/api/messaging/message_service.cc (right): https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/message_service.cc:335: DCHECK(!util::CanBeIncognitoEnabled(target_extension)); On 2017/03/23 22:08:36, Devlin wrote: > On 2017/03/23 02:54:03, karandeepb wrote: > > At least in this case, CanBeIncognitoEnabled seems like a misnomer. Wonder if > we > > should also introduce a separate SupportsIncognitoToggle or maybe we can > > directly use checks to find whether it is a platform app or component > extension. > > > > I'm not sure I love SupportsIncognitoToggle, since it's pretty UI-dependent, > and, as mentioned in the bug, might lead to confusion if policy settings get > involved. I think what we're trying to convey here is that the extension can't > ever "run" in an incognito context (and that assumption is valid here - we only > pass messages back and forth). But CanExtensionBeEnabledToRunInIncognito is too > long. ;) If you have other suggestions, I'm happy to hear them, but I don't > think the name is dramatically incorrect. The current naming SGTM then. https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/api/messaging/message_service.cc:335: DCHECK(!util::CanBeIncognitoEnabled(target_extension)); On 2017/03/23 22:08:36, Devlin wrote: > Is this check correct? What about the case of a website trying to connect with > an extension that can be incognito enabled, but isn't? (Given this passed on > the bots, does that indicate we need another test?) It isn't. I had misunderstood what was happening. So as I now understand, bindings for the chrome.runtime API will only be generated into a (non-extension) renderer, if at least one of the extensions loaded into it is externally_connectible. Modified a test so that this DCHECK was triggered. https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_messages_apitest.cc (right): https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_messages_apitest.cc:803: extension.get(), NULL)); On 2017/03/23 22:08:37, Devlin wrote: > prefer nullptr in new code Done. https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_messages_apitest.cc:803: extension.get(), NULL)); On 2017/03/23 22:08:36, Devlin wrote: > Subtle: when we reload the extension, the Extension object that we previously > had (stored here in |extension|) is no longer the "current" extension stored in > the registry. > > Instead, we can use TestExtensionRegistryObserver::WaitForExtensionLoaded(), > which returns the new Extension object. Done. https://codereview.chromium.org/2766263003/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_messages_apitest.cc:982: extension.get(), NULL)); On 2017/03/23 22:08:37, Devlin wrote: > (Same comment here) Done. https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (left): https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:164: return; On 2017/03/23 22:08:37, Devlin wrote: > I wonder if it's worth keeping this short-circuit, since the creation of > ExtensionMsg_Loaded_Params is non-trivial, and we know that no process ever > needs to know about themes. Centralizing the logic in IsExtensionVisibleToProcess seemed better to me. Though don't mind changing it either. https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:43: // ensure connections from incognito tabs to such extensions work fine. On 2017/03/23 22:08:37, Devlin wrote: > nitty nit: omit ' fine' -> 'ensure connections from incognito tabs to such > extensions work.' Done. https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:125: if (!IsExtensionVisibleToProcess(*ext, process)) On 2017/03/23 22:08:37, Devlin wrote: > nitty nit: We actually only need the browser context here, so we can cache it > before iterating through the loop and just pass it directly, saving the > (admittedly very cheap) repeated call to RenderProcessHost::GetBrowserContext(). > > (Note that you'll still want to use process->GetBrowserContext() when you cache > it, rather than passing in browser_context_, since this handles both on- and > off-the-record versions.) Done. https://codereview.chromium.org/2766263003/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_messages_apitest.cc (right): https://codereview.chromium.org/2766263003/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_messages_apitest.cc:514: TestExtensionDir web_connectable_dir_extension_; Necessary so that they don't get the same extension id.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM. The diff is a bit odd, since https://codereview.chromium.org/2766063003/ isn't listed as a dependency, but as long as we land that one first, should be fine. https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (left): https://codereview.chromium.org/2766263003/diff/60001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:164: return; On 2017/04/04 03:44:15, karandeepb wrote: > On 2017/03/23 22:08:37, Devlin wrote: > > I wonder if it's worth keeping this short-circuit, since the creation of > > ExtensionMsg_Loaded_Params is non-trivial, and we know that no process ever > > needs to know about themes. > > Centralizing the logic in IsExtensionVisibleToProcess seemed better to me. > Though don't mind changing it either. Given it's somewhat performance-sensitive code, let's keep it with a comment like: // IsExtensionVisibleToProcess() would filter out themes, but we choose // to early-out for performance reasons. https://codereview.chromium.org/2766263003/diff/240001/extensions/browser/ren... File extensions/browser/renderer_startup_helper_unittest.cc (right): https://codereview.chromium.org/2766263003/diff/240001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:310: IsExtensionLoadedInProcess(*extension_, render_process_host_.get())); add EXPECT_FALSE(IsExtensionLoadedInProcess(*extension_, incognito_rendeer_process_host_.get());
Patchset #11 (id:260001) has been deleted
Patchset #11 (id:280001) has been deleted
>>The diff is a bit odd, since https://codereview.chromium.org/2766063003/ isn't >>listed as a dependency, but as long as we land that one first, should be fine. https://codereview.chromium.org/2766063003/ is an indirect dependency, so will land first. https://codereview.chromium.org/2766263003/diff/240001/extensions/browser/ren... File extensions/browser/renderer_startup_helper_unittest.cc (right): https://codereview.chromium.org/2766263003/diff/240001/extensions/browser/ren... extensions/browser/renderer_startup_helper_unittest.cc:310: IsExtensionLoadedInProcess(*extension_, render_process_host_.get())); On 2017/04/04 15:15:05, Devlin wrote: > add > EXPECT_FALSE(IsExtensionLoadedInProcess(*extension_, > incognito_rendeer_process_host_.get()); Done.
Another thing, in the linked bug you also mentioned some IPC messages in permission_updater. I didn't get a chance to look at them yet. But do you know if the changes introduced here, might have some unintended side effect for those IPC messages.
On 2017/04/04 21:16:09, karandeepb wrote: > Another thing, in the linked bug you also mentioned some IPC messages in > permission_updater. I didn't get a chance to look at them yet. But do you know > if the changes introduced here, might have some unintended side effect for those > IPC messages. Good question. The PermissionsUpdater class sends messages when extension permissions are updated. Currently in the dispatcher, we have an early return if the extension isn't known to the process: https://chromium.googlesource.com/chromium/src/+/ae47bdf7f95637309d76c3b68fbe... So we should be fine. (Though ideally, we could make that stricter, too)
The CQ bit was checked by karandeepb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2766263003/#ps320001 (title: "Address review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1491518759509590,
"parent_rev": "2f0fa56eb8fc2eee27d437e7b6c8038ba5a52d87", "commit_rev":
"18ab4ab8c381424b30163a763515ecc0fdfc8db2"}
Message was sent while issue was closed.
Description was changed from ========== Extensions: Only load incognito-enabled extensions in an incognito renderer. Currently, extensions which are disabled in incognito mode are also loaded in an incognito renderer. This is redundant and can cause non-required bindings to be generated in the renderer. Also, it has potential security implications. Prevent this by only loading incognito-enabled extensions in an incognito renderer. However extensions which can't be enabled in the incognito mode (e.g. platform apps etc.) still need to be loaded in incognito renderers, so that incognito tabs can connect with them (issue 305394). Also, add tests for the same. BUG=527548 ========== to ========== Extensions: Only load incognito-enabled extensions in an incognito renderer. Currently, extensions which are disabled in incognito mode are also loaded in an incognito renderer. This is redundant and can cause non-required bindings to be generated in the renderer. Also, it has potential security implications. Prevent this by only loading incognito-enabled extensions in an incognito renderer. However extensions which can't be enabled in the incognito mode (e.g. platform apps etc.) still need to be loaded in incognito renderers, so that incognito tabs can connect with them (issue 305394). Also, add tests for the same. BUG=527548 Review-Url: https://codereview.chromium.org/2766263003 Cr-Commit-Position: refs/heads/master@{#462707} Committed: https://chromium.googlesource.com/chromium/src/+/18ab4ab8c381424b30163a763515... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:320001) as https://chromium.googlesource.com/chromium/src/+/18ab4ab8c381424b30163a763515... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
