Packaged (CRX) extensions shouldn't be able to get 'dev' interfaces in NaCl.

Packaged (CRX) extensions shouldn't be able to get 'dev' interfaces in NaCl.BUG=100228
TEST=manual
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=105985

[bbudge, 2011-10-14 18:39:36]

jam for "chrome_content_renderer_client"
sehr for the NaCl plugin code.

[bbudge, 2011-10-14 20:02:10]

fixed compile on mac, linux

[bbudge, 2011-10-14 20:39:21]

Added changes to tests so nacl_integration passes. We can't currently test that
CWS apps can't get dev interfaces. I verified this manually.

[bbudge, 2011-10-14 20:50:34]

Adding Adam for chrome_content_renderer_client.cc and whatever else you care to
review.

[bbudge, 2011-10-14 21:37:47]

Added check for attempts to inject @dev attribute.

[sehr, 2011-10-14 21:38:32]

One cleanup, one bug, one question.

http://codereview.chromium.org/8301006/diff/7008/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8301006/diff/7008/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:473: allow_dev_interfaces =
true;
You initialized allow_dev_interfaces to true, so this is a nop.  You could just
invert the condition and only do the else part.  That would let you merge the
"if (allowed_extension)" part with the inner if also.

http://codereview.chromium.org/8301006/diff/7008/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/8301006/diff/7008/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:108: const char* const
kDevAttribute = "@dev";
Does this naming scheme prevent JavaScript from creating an embed tag and then
setting the attribute before splicing into the DOM?

http://codereview.chromium.org/8301006/diff/7008/ppapi/native_client/tests/pp...
File ppapi/native_client/tests/ppapi_browser/ppb_dev/ppapi_ppb_dev.cc (right):

http://codereview.chromium.org/8301006/diff/7008/ppapi/native_client/tests/pp...
ppapi/native_client/tests/ppapi_browser/ppb_dev/ppapi_ppb_dev.cc:54: // have
access to all dev interfaces.
Shouldn't we have a corresponding test that these are not available from
appstore nexes?

[bbudge, 2011-10-14 22:03:27]

PTAL

http://codereview.chromium.org/8301006/diff/7008/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8301006/diff/7008/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:473: allow_dev_interfaces =
true;
On 2011/10/14 21:38:32, sehr1 wrote:
> You initialized allow_dev_interfaces to true, so this is a nop.  You could
just
> invert the condition and only do the else part.  That would let you merge the
> "if (allowed_extension)" part with the inner if also.

Done.

http://codereview.chromium.org/8301006/diff/7008/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/8301006/diff/7008/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.cc:108: const char* const
kDevAttribute = "@dev";
Added a check for this on the browser side. Thanks for catching this.
On 2011/10/14 21:38:32, sehr1 wrote:
> Does this naming scheme prevent JavaScript from creating an embed tag and then
> setting the attribute before splicing into the DOM?

http://codereview.chromium.org/8301006/diff/7008/ppapi/native_client/tests/pp...
File ppapi/native_client/tests/ppapi_browser/ppb_dev/ppapi_ppb_dev.cc (right):

http://codereview.chromium.org/8301006/diff/7008/ppapi/native_client/tests/pp...
ppapi/native_client/tests/ppapi_browser/ppb_dev/ppapi_ppb_dev.cc:54: // have
access to all dev interfaces.
Yes, but it would be very difficult to set this up in this test. Ideas?
On 2011/10/14 21:38:32, sehr1 wrote:
> Shouldn't we have a corresponding test that these are not available from
> appstore nexes?

[bbudge, 2011-10-14 22:50:03]

Modified the test. I think otherwise it will be a maintenance nightmare.

[jam, 2011-10-14 22:51:09]

http://codereview.chromium.org/8301006/diff/3012/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8301006/diff/3012/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:420: // If this is the NaCl
plugin, get the manifest URL for the app so we can
this function is already too big, can we move the nacl part to another function?

[bbudge, 2011-10-14 23:40:14]

Broke out NaCl test into a separate function.
Modified the tests to prevent maintenance nightmare.
PTAL

http://codereview.chromium.org/8301006/diff/3012/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8301006/diff/3012/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:420: // If this is the NaCl
plugin, get the manifest URL for the app so we can
On 2011/10/14 22:51:09, John Abd-El-Malek wrote:
> this function is already too big, can we move the nacl part to another
function?

Done. It's true, it was turning into a monster.

[jam, 2011-10-14 23:52:46]

lgtm (you missed adding chrome_content_renderer_client.h)

[bbudge, 2011-10-15 00:07:39]

On 2011/10/14 23:52:46, John Abd-El-Malek wrote:
> lgtm (you missed adding chrome_content_renderer_client.h)

Thanks for catching that. It must be time to go home.

[sehr, 2011-10-17 22:45:52]

One comment nit.  Otherwise, LGTM.

http://codereview.chromium.org/8301006/diff/4013/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8301006/diff/4013/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:461: // Content type handling
NaCl plugin; the "nacl" param on the
nacl param is on the plugin, not the MIME type.

[bbudge, 2011-10-17 22:56:05]

http://codereview.chromium.org/8301006/diff/4013/chrome/renderer/chrome_conte...
File chrome/renderer/chrome_content_renderer_client.cc (right):

http://codereview.chromium.org/8301006/diff/4013/chrome/renderer/chrome_conte...
chrome/renderer/chrome_content_renderer_client.cc:461: // Content type handling
NaCl plugin; the "nacl" param on the
Fixed the comment. It's a bit confusing. This isn't the 'nacl' attribute from
the embed element - it's an extra bit of info on the WebPluginMimeType.
On 2011/10/17 22:45:52, sehr1 wrote:
> nacl param is on the plugin, not the MIME type.

[commit-bot: I haz the power, 2011-10-17 22:58:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/8301006/9003

[commit-bot: I haz the power, 2011-10-17 22:58:31]

Can't apply patch for file
ppapi/native_client/tests/ppapi_browser/ppb_dev/ppapi_ppb_dev.cc.
While running patch -p0 --forward --force;
patching file ppapi/native_client/tests/ppapi_browser/ppb_dev/ppapi_ppb_dev.cc
Hunk #1 FAILED at 16.
Hunk #2 FAILED at 50.
2 out of 2 hunks FAILED -- saving rejects to file
ppapi/native_client/tests/ppapi_browser/ppb_dev/ppapi_ppb_dev.cc.rej

[bbudge, 2011-10-17 23:04:11]

Had to update patch.

[commit-bot: I haz the power, 2011-10-17 23:04:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/8301006/19003

[commit-bot: I haz the power, 2011-10-18 00:28:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/8301006/19005

[bbudge, 2011-10-18 00:28:22]

Patch got out of date again.