[Extensions] Fix ExtensionMsg_Loaded_Params

[Extensions] Fix ExtensionMsg_Loaded_Params

The ExtensionMsg_Loaded_Params had a number of issues:
- it never pickled the id
- it never pickled the tab specific permissions
- it didn't use the explicit id when constructing the extension

The latter is useful to safe work in the normal case, and important in
tests since test extensions may not have keys or paths.

BUG=640837
Committed: https://crrev.com/2b3723bc43398ea016a3d109430492784b2be234
Cr-Commit-Position: refs/heads/master@{#414600}

[Devlin, 2016-08-25 16:39:19]

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

[commit-bot: I haz the power, 2016-08-25 16:40:02]

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-08-25 17:35:17]

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

[commit-bot: I haz the power, 2016-08-25 17:35:18]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-08-25 18:45:50]

Description was changed from

==========
fixed

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

to

==========
[Extensions] Fix ExtensionMsg_Loaded_Params

The ExtensionMsg_Loaded_Params had a number of issues:
- it never pickled the id
- it never pickled the tab specific permissions
- it didn't use the explicit id when constructing the extension

The latter is useful to safe work in the normal case, and important in
tests since test extensions may not have keys or paths.

BUG=640837
==========

[Devlin, 2016-08-25 18:46:01]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-08-25 18:47:48]

rdevlin.cronin@chromium.org changed reviewers:
+ asargent@chromium.org, meacer@chromium.org

[Devlin, 2016-08-25 18:47:49]

asargent@, can you take a look?
meacer@, can you stamp the IPC changes?  (No new fields or messages; just
updating pickling)

[meacer, 2016-08-25 18:58:49]

extension_messages.cc LGTM

[asargent_no_longer_on_chrome, 2016-08-25 19:02:50]

https://codereview.chromium.org/2279013002/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/2279013002/diff/20001/extensions/common/exten...
extensions/common/extension_messages.cc:85: Extension::Create(path, location,
*manifest, creation_flags, id, error);
Doesn't the non-id-taking version of Extension::Create already do the right
thing? It either grabs an id from the manifest (if the "key" field is in there),
or it makes one based on hash of path?

[Devlin, 2016-08-25 20:40:57]

https://codereview.chromium.org/2279013002/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/2279013002/diff/20001/extensions/common/exten...
extensions/common/extension_messages.cc:85: Extension::Create(path, location,
*manifest, creation_flags, id, error);
On 2016/08/25 19:02:50, Antony Sargent wrote:
> Doesn't the non-id-taking version of Extension::Create already do the right
> thing? It either grabs an id from the manifest (if the "key" field is in
there),
> or it makes one based on hash of path? 
> 

See CL (or bug) description. :)  But this is probably an indication that it's
worth mentioning in the code, too.  Added a comment.

[asargent_no_longer_on_chrome, 2016-08-25 22:06:16]

ok, lgtm

(BTW, I spent some time wondering about the following: if this code happened to
be used for renderer->browser IPC, would your change represent a security
vulnerability? But I guess a compromised renderer could craft a forged key field
in the manifest, so we already should not be be relying on this data for
renderer->browser messages)

https://codereview.chromium.org/2279013002/diff/40001/extensions/common/exten...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/2279013002/diff/40001/extensions/common/exten...
extensions/common/extension_messages.cc:84: // We pass in the |id| to the create
call because it will safe work in the
typo: "safe" - did you mean "still"? or maybe "save" ?

[Devlin, 2016-08-25 22:15:07]

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

[Devlin, 2016-08-25 22:15:09]

On 2016/08/25 22:06:16, Antony Sargent wrote:
> (BTW, I spent some time wondering about the following: if this code happened
to
> be used for renderer->browser IPC, would your change represent a security
> vulnerability? But I guess a compromised renderer could craft a forged key
field
> in the manifest, so we already should not be be relying on this data for
> renderer->browser messages)

Yep, if a renderer was compromised, it could just as easily modify the manifest
or path to craft its own key.  So yes, definitely a security vulnerability if we
ever went in the other direction.

https://codereview.chromium.org/2279013002/diff/40001/extensions/common/exten...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/2279013002/diff/40001/extensions/common/exten...
extensions/common/extension_messages.cc:84: // We pass in the |id| to the create
call because it will safe work in the
On 2016/08/25 22:06:16, Antony Sargent wrote:
> typo: "safe" - did you mean "still"? or maybe "save" ?
> 
> 

Save - whoops.

[commit-bot: I haz the power, 2016-08-25 22:15:38]

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-08-25 23:11:49]

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

[commit-bot: I haz the power, 2016-08-25 23:11:50]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-08-26 00:30:06]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-08-26 00:30:06]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/2279013002/#ps60001
(title: "typo")

[commit-bot: I haz the power, 2016-08-26 00:30:41]

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-08-26 00:36:39]

Description was changed from

==========
[Extensions] Fix ExtensionMsg_Loaded_Params

The ExtensionMsg_Loaded_Params had a number of issues:
- it never pickled the id
- it never pickled the tab specific permissions
- it didn't use the explicit id when constructing the extension

The latter is useful to safe work in the normal case, and important in
tests since test extensions may not have keys or paths.

BUG=640837
==========

to

==========
[Extensions] Fix ExtensionMsg_Loaded_Params

The ExtensionMsg_Loaded_Params had a number of issues:
- it never pickled the id
- it never pickled the tab specific permissions
- it didn't use the explicit id when constructing the extension

The latter is useful to safe work in the normal case, and important in
tests since test extensions may not have keys or paths.

BUG=640837
==========

[commit-bot: I haz the power, 2016-08-26 00:36:39]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-26 00:38:55]

Description was changed from

==========
[Extensions] Fix ExtensionMsg_Loaded_Params

The ExtensionMsg_Loaded_Params had a number of issues:
- it never pickled the id
- it never pickled the tab specific permissions
- it didn't use the explicit id when constructing the extension

The latter is useful to safe work in the normal case, and important in
tests since test extensions may not have keys or paths.

BUG=640837
==========

to

==========
[Extensions] Fix ExtensionMsg_Loaded_Params

The ExtensionMsg_Loaded_Params had a number of issues:
- it never pickled the id
- it never pickled the tab specific permissions
- it didn't use the explicit id when constructing the extension

The latter is useful to safe work in the normal case, and important in
tests since test extensions may not have keys or paths.

BUG=640837

Committed: https://crrev.com/2b3723bc43398ea016a3d109430492784b2be234
Cr-Commit-Position: refs/heads/master@{#414600}
==========

[commit-bot: I haz the power, 2016-08-26 00:38:56]

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