Pepper: Add a property to URLRequestInfo to skip header validation.

Pepper: Add a property to URLRequestInfo to skip header validation.

The resulting URLRequestInfo is then only usable with a (trusted) URLLoader
which has universal access.

BUG=75350
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78047

[piman, 2011-03-09 02:10:10]

http://codereview.chromium.org/6652014/diff/1/webkit/plugins/ppapi/ppb_url_re...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/6652014/diff/1/webkit/plugins/ppapi/ppb_url_re...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:248: if (!universal_access_ &&
!AreValidHeaders(value))
I'm not sure we'd want to allow any arbitrary headers. E.g. connection or
content-length could mess up a lot of things with the protocol

http://codereview.chromium.org/6652014/diff/1/webkit/plugins/ppapi/ppb_url_re...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:321:
frame->setReferrerForRequest(web_request, WebURL());  // Use default.
You may want to avoid doing that if the referer is set from the headers ?
Or maybe make the referer an explicit property ?

[viettrungluu, 2011-03-09 02:16:35]

http://codereview.chromium.org/6652014/diff/1/webkit/plugins/ppapi/ppb_url_re...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/6652014/diff/1/webkit/plugins/ppapi/ppb_url_re...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:321:
frame->setReferrerForRequest(web_request, WebURL());  // Use default.
On 2011/03/09 02:10:10, piman wrote:
> You may want to avoid doing that if the referer is set from the headers ?
> Or maybe make the referer an explicit property ?

I like this and think we should be as conservative as possible. I don't like the
three-state (use default referer/no referer header/specified referer header)
problem though.

[brettw, 2011-03-09 03:32:06]

My feedback over IM was that currently all security restrictions are done based
on what interfaces are available. This provides a nice way to divide
trusted/untrusted in one place (GetInterface).

It would be nice if we could keep this property. We could have a
URLRequestTrusted interface (not my favorite). Or we could just let you set the
referrer and then ignore it or fail later when you make a URLRequest out of it
later.

[viettrungluu, 2011-03-11 19:19:48]

How about this?

[brettw, 2011-03-11 20:34:44]

I like this better.

http://codereview.chromium.org/6652014/diff/5001/ppapi/c/ppb_url_request_info.h
File ppapi/c/ppb_url_request_info.h (right):

http://codereview.chromium.org/6652014/diff/5001/ppapi/c/ppb_url_request_info...
ppapi/c/ppb_url_request_info.h:51: PP_URLREQUESTPROPERTY_USECUSTOMREFERRER,
Why do you need this flag? Can't you just key off of whether the custom referrer
is nonempty?

You should also mention somewhere what will happen if you set a custom referrer
and make a request that doesn't have permission.

[viettrungluu, 2011-03-11 20:47:16]

On 2011/03/11 20:34:44, brettw wrote:
> I like this better.
> 
>
http://codereview.chromium.org/6652014/diff/5001/ppapi/c/ppb_url_request_info.h
> File ppapi/c/ppb_url_request_info.h (right):
> 
>
http://codereview.chromium.org/6652014/diff/5001/ppapi/c/ppb_url_request_info...
> ppapi/c/ppb_url_request_info.h:51: PP_URLREQUESTPROPERTY_USECUSTOMREFERRER,
> Why do you need this flag? Can't you just key off of whether the custom
referrer
> is nonempty?

I'm not a big fan of the flag either, but what if the plugin wants to send no
referrer header? (Inspecting the code, Flapper might not set a referrer.)

> You should also mention somewhere what will happen if you set a custom
referrer
> and make a request that doesn't have permission.

Will do.

[brettw, 2011-03-11 21:01:39]

What about having an Undefined var vs. an empty string var? This would at least
make the API make more sense, although you will still need the extra tracking
information in the implementation.

[viettrungluu, 2011-03-11 21:02:53]

On 2011/03/11 21:01:39, brettw wrote:
> What about having an Undefined var vs. an empty string var? This would at
least
> make the API make more sense, although you will still need the extra tracking
> information in the implementation.

Good idea; I'll do that.

[viettrungluu, 2011-03-11 21:34:48]

On 2011/03/11 21:02:53, viettrungluu wrote:
> On 2011/03/11 21:01:39, brettw wrote:
> > What about having an Undefined var vs. an empty string var? This would at
> least
> > make the API make more sense, although you will still need the extra
tracking
> > information in the implementation.
> 
> Good idea; I'll do that.

Done, I think.

[brettw, 2011-03-11 23:31:05]

http://codereview.chromium.org/6652014/diff/11001/ppapi/c/ppb_url_request_info.h
File ppapi/c/ppb_url_request_info.h (right):

http://codereview.chromium.org/6652014/diff/11001/ppapi/c/ppb_url_request_inf...
ppapi/c/ppb_url_request_info.h:61: #define PPB_URLREQUESTINFO_INTERFACE
"PPB_URLRequestInfo;0.3"
I don't really want to break backwards compatibility, especially for NaCl, who
will no longer be able to make URL requests with this change until they sync.

We should continue to support the old interface. However, since the old
interface is the same as the new one, I actually think we don't need to rev the
interface version. If you try to set the referrer on a version that doesn't
support that attribute, it will fail, and that should be enough.

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:246: !has_universal_access_)
This should fit on one line.

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:106: if (var.type ==
PP_VARTYPE_UNDEFINED)
Now that we have 3 of these, I think it would be more clear if this was a switch
statement on the var.type.

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_request_info_impl.h (right):

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_request_info_impl.h:72: bool
has_custom_referrer_url_;
Can you add a comment here about how the boolean differentiates between no
referrer and and one that hasn't been set?

[viettrungluu, 2011-03-14 17:13:53]

http://codereview.chromium.org/6652014/diff/11001/ppapi/c/ppb_url_request_info.h
File ppapi/c/ppb_url_request_info.h (right):

http://codereview.chromium.org/6652014/diff/11001/ppapi/c/ppb_url_request_inf...
ppapi/c/ppb_url_request_info.h:61: #define PPB_URLREQUESTINFO_INTERFACE
"PPB_URLRequestInfo;0.3"
On 2011/03/11 23:31:05, brettw wrote:
> I don't really want to break backwards compatibility, especially for NaCl, who
> will no longer be able to make URL requests with this change until they sync.
> 
> We should continue to support the old interface. However, since the old
> interface is the same as the new one, I actually think we don't need to rev
the
> interface version. If you try to set the referrer on a version that doesn't
> support that attribute, it will fail, and that should be enough.

Done.

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:246: !has_universal_access_)
On 2011/03/11 23:31:05, brettw wrote:
> This should fit on one line.

Done.

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:106: if (var.type ==
PP_VARTYPE_UNDEFINED)
On 2011/03/11 23:31:05, brettw wrote:
> Now that we have 3 of these, I think it would be more clear if this was a
switch
> statement on the var.type.

Done.

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_request_info_impl.h (right):

http://codereview.chromium.org/6652014/diff/11001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_request_info_impl.h:72: bool
has_custom_referrer_url_;
On 2011/03/11 23:31:05, brettw wrote:
> Can you add a comment here about how the boolean differentiates between no
> referrer and and one that hasn't been set?

Done.

[brettw, 2011-03-14 17:15:26]

LGTM

[brettw, 2011-03-14 17:15:26]

LGTM