Add cross-origin loading of NEXEs for MIME type handlers

Add cross-origin loading of NEXEs for MIME type handlers

We only load NEXEs across origins when rendering non-NaCl MIME
types and when the NEXE is stored in a Chrome extension. That limits this
privilege to only being used when the NEXE is being used as a MIME handler for
another MIME type (e.g., application/pdf).
Committed: http://src.chromium.org/viewvc/native_client?view=rev&revision=4899

[bbudge-google, 2011-04-10 12:33:19]

Added Polina, because she's the owner of this code.

[bbudge-google, 2011-04-10 12:37:53]

Otherwise, LGTM.

[polina, 2011-04-13 07:01:27]

Just to document what we talked about today.

We have agreed that we won't be allowing cross-origin nexes and manifest files
for the 1st release, so granting universal access by default will be a problem.
This and other pdf-specific changes (e.g. proxies) might benefit from a
pdf-specific flag.

[abarth-chromium, 2011-04-13 07:10:40]

Yep.  The main question is how the NaCl plugin should know that we're
dealing with pdf.nexe.  One thought is that we could look at the
public key in the chrome-extension, which should be pretty hard to
forge.  If you think that's a good approach, I can work up a patch
that does that.

Adam


On Wed, Apr 13, 2011 at 12:01 AM,  <polina@google.com> wrote:
> Just to document what we talked about today.
>
> We have agreed that we won't be allowing cross-origin nexes and manifest
> files
> for the 1st release, so granting universal access by default will be a
> problem.
> This and other pdf-specific changes (e.g. proxies) might benefit from a
> pdf-specific flag.
>
> http://codereview.chromium.org/6813070/
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[abarth-chromium, 2011-04-14 16:50:09]

This is now the last code change blocking the NaCl PDF renderer initial dogfood.

[abarth-chromium, 2011-04-14 18:59:41]

Polina, would you be willing to take another look?

I've limited the access request to only taking effect when we're dealing with a
Chrome extension that's registered the NEXE as a MIME type handler (e.g., as a
PDF renderer).

Thanks!
Adam

[Brad Chen, 2011-04-14 19:36:33]

I know this morning Polina is tied up with a meeting, a mentoring thing. I
expect she will be around later today.

Brad

On Thu, Apr 14, 2011 at 11:59 AM, <abarth@chromium.org> wrote:

> Polina, would you be willing to take another look?
>
> I've limited the access request to only taking effect when we're dealing
> with a
> Chrome extension that's registered the NEXE as a MIME type handler (e.g.,
> as a
> PDF renderer).
>
> Thanks!
> Adam
>
>
>
> http://codereview.chromium.org/6813070/
>
> --
> You received this message because you are subscribed to the Google Groups
> "Native-Client-Reviews" group.
> To post to this group, send email to
> native-client-reviews@googlegroups.com.
> To unsubscribe from this group, send email to
> native-client-reviews+unsubscribe@googlegroups.com.
> For more options, visit this group at
> http://groups.google.com/group/native-client-reviews?hl=en.
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[polina, 2011-04-15 06:24:57]

The approach makes sense. What happens if somebody includes type twice? What
happens if somebody sets type to not x-nacl and url to a regular nexe. It would
be nice to see some tests for this.

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
File src/trusted/plugin/ppapi/plugin_ppapi.cc (right):

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
src/trusted/plugin/ppapi/plugin_ppapi.cc:71: const char* const kNaClMIMEType =
"application/x-nacl";
please put this at the top with other constants

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
File src/trusted/plugin/ppapi/plugin_ppapi.h (right):

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
src/trusted/plugin/ppapi/plugin_ppapi.h:39: //               can be shared with
the NPAPI code.
what NPAPI code?

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
src/trusted/plugin/ppapi/plugin_ppapi.h:103: const std::string& mime_type()
const { return mime_type_; }
we use nacl::string

[abarth-chromium, 2011-04-15 06:36:40]

Thanks for the review.

> The approach makes sense. What happens if somebody includes type twice?
> What happens if somebody sets type to not x-nacl and url to a regular nexe.

We'll attempt to render the NEXE as a PDF.

> It would be nice to see some tests for this.

I'll look around for a testing harness.

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
File src/trusted/plugin/ppapi/plugin_ppapi.cc (right):

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
src/trusted/plugin/ppapi/plugin_ppapi.cc:71: const char* const kNaClMIMEType =
"application/x-nacl";
On 2011/04/15 06:24:57, polina wrote:
> please put this at the top with other constants

The constants above have internal linkage, preventing me from linking to the
symbol from file_downloader.cc.

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
File src/trusted/plugin/ppapi/plugin_ppapi.h (right):

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
src/trusted/plugin/ppapi/plugin_ppapi.h:39: //               can be shared with
the NPAPI code.
On 2011/04/15 06:24:57, polina wrote:
> what NPAPI code?

Ah, I'm working against an old version of the plugin where that code still
exists.  Removed.

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
src/trusted/plugin/ppapi/plugin_ppapi.h:103: const std::string& mime_type()
const { return mime_type_; }
On 2011/04/15 06:24:57, polina wrote:
> we use nacl::string

Done.

[abarth-chromium, 2011-04-15 07:54:01]

Patch updated.  I don't fully understand the test I modified, but hopefully I
didn't screw it up.

[polina, 2011-04-15 22:52:10]

thanks for adding the test!

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
File src/trusted/plugin/ppapi/plugin_ppapi.cc (right):

http://codereview.chromium.org/6813070/diff/10002/src/trusted/plugin/ppapi/pl...
src/trusted/plugin/ppapi/plugin_ppapi.cc:71: const char* const kNaClMIMEType =
"application/x-nacl";
On 2011/04/15 06:36:40, abarth wrote:
> On 2011/04/15 06:24:57, polina wrote:
> > please put this at the top with other constants
> 
> The constants above have internal linkage, preventing me from linking to the
> symbol from file_downloader.cc.

Might be better to make it a static member of the plugin, perhaps?

http://codereview.chromium.org/6813070/diff/15003/src/trusted/plugin/ppapi/pl...
File src/trusted/plugin/ppapi/plugin_ppapi.h (right):

http://codereview.chromium.org/6813070/diff/15003/src/trusted/plugin/ppapi/pl...
src/trusted/plugin/ppapi/plugin_ppapi.h:38: extern const char* const
kNaClMIMEType;
don't you get presubmit errors for extern?

http://codereview.chromium.org/6813070/diff/15003/tests/ppapi_bad/ppapi_bad.html
File tests/ppapi_bad/ppapi_bad.html (right):

http://codereview.chromium.org/6813070/diff/15003/tests/ppapi_bad/ppapi_bad.h...
tests/ppapi_bad/ppapi_bad.html:33: type="application/pdf" />
so this one fails because it's not the right kind of cross-origin url?
please add a comment or change url to explain the issue

http://codereview.chromium.org/6813070/diff/15003/tests/ppapi_bad/ppapi_bad.h...
tests/ppapi_bad/ppapi_bad.html:92: type="application/pdf" />
you had a no-type case above

[abarth-chromium, 2011-04-15 23:08:52]

Thanks for the review.  Patch updated.

On 2011/04/15 22:52:10, polina wrote:
> thanks for adding the test!

My pleasure.

> > The constants above have internal linkage, preventing me from linking to the
> > symbol from file_downloader.cc.
> 
> Might be better to make it a static member of the plugin, perhaps?

Done.

>
http://codereview.chromium.org/6813070/diff/15003/src/trusted/plugin/ppapi/pl...
> File src/trusted/plugin/ppapi/plugin_ppapi.h (right):
> 
>
http://codereview.chromium.org/6813070/diff/15003/src/trusted/plugin/ppapi/pl...
> src/trusted/plugin/ppapi/plugin_ppapi.h:38: extern const char* const
> kNaClMIMEType;
> don't you get presubmit errors for extern?

I didn't see any, but the "extern" is gone now anyway.

>
http://codereview.chromium.org/6813070/diff/15003/tests/ppapi_bad/ppapi_bad.h...
> tests/ppapi_bad/ppapi_bad.html:33: type="application/pdf" />
> so this one fails because it's not the right kind of cross-origin url?
> please add a comment or change url to explain the issue

Correct, and because NaCl isn't registered as handling PDFs.  We'll only be able
to test the success cases in the integration test.

>
http://codereview.chromium.org/6813070/diff/15003/tests/ppapi_bad/ppapi_bad.h...
> tests/ppapi_bad/ppapi_bad.html:92: type="application/pdf" />
> you had a no-type case above

I've added a no-type case here too.

[polina, 2011-04-15 23:16:17]

http://codereview.chromium.org/6813070/diff/18003/tests/ppapi_bad/ppapi_bad.html
File tests/ppapi_bad/ppapi_bad.html (right):

http://codereview.chromium.org/6813070/diff/18003/tests/ppapi_bad/ppapi_bad.h...
tests/ppapi_bad/ppapi_bad.html:29: been registered as handling the PDF MIME
type.)<br>
I was thinking more of an html comment. The viewer of the page can't see the
embed details anyway.

[polina, 2011-04-15 23:16:36]

LGTM

(after comment fix/clarification)