Enabled CORS for chrome://resources.

URLDataSource can specify the value for 'Access-Control-Allow-Origin' response header based on 'Origin' request header.

SharedResourcesDataSource allows access for all 'chrome://*' origins.

BUG=418199
Committed: https://crrev.com/fd3b257aacfddcd11c925bbea203417dbab9f6be
Cr-Commit-Position: refs/heads/master@{#299153}

[dglazkov, 2014-09-29 17:01:20]

dglazkov@chromium.org changed reviewers:
+ dglazkov@chromium.org

[dglazkov, 2014-09-29 17:01:21]

https://codereview.chromium.org/613733002/diff/1/content/browser/webui/url_da...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/613733002/diff/1/content/browser/webui/url_da...
content/browser/webui/url_data_manager_backend.cc:274:
info->headers->AddHeader("Access-Control-Allow-Origin: *");
Is '*' really what you want here?

[dzhioev (left Google), 2014-09-29 17:32:11]

https://codereview.chromium.org/613733002/diff/1/content/browser/webui/url_da...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/613733002/diff/1/content/browser/webui/url_da...
content/browser/webui/url_data_manager_backend.cc:274:
info->headers->AddHeader("Access-Control-Allow-Origin: *");
On 2014/09/29 17:01:21, dglazkov wrote:
> Is '*' really what you want here?

It seems 'Access-Control-Allow-Origin' can be either "*", or "null", or single
"shcheme://host:port" (without wildcards) in Chrome. So we can't use
"chrome://*" or explicit list "chrome://host1 chrome://host2 chrome://host3"
here.

[dzhioev (left Google), 2014-10-03 13:25:02]

dzhioev@chromium.org changed reviewers:
+ creis@chromium.org, jhawkins@chromium.org

[dzhioev (left Google), 2014-10-03 13:25:46]

dzhioev@chromium.org changed reviewers:
+ tsepez@chromium.org

[dzhioev (left Google), 2014-10-03 13:56:42]

Hello, please review.
jhawkins@ as an owner of chrome/* and content/browser/webui/*
creis@ as an owner of content/*
tsepez@ for security and general review.

[Tom Sepez, 2014-10-03 17:02:45]

Nothing but nits.  LGTM.

https://codereview.chromium.org/613733002/diff/40001/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/613733002/diff/40001/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.cc:169: void
set_access_control_allow_origin_header(const std::string& value) {
Nit: we're actually setting the value for the header, not the full header
itself.  You might call this set_access_control_allow_origin_header_value() but
this name is getting ridiculously long as it is. It's your call.

https://codereview.chromium.org/613733002/diff/40001/content/public/browser/u...
File content/public/browser/url_data_source.cc (right):

https://codereview.chromium.org/613733002/diff/40001/content/public/browser/u...
content/public/browser/url_data_source.cc:61: return "";
nit:  I thought the preferred way of returning an empty string was to return
std::string().

[dzhioev (left Google), 2014-10-03 19:55:08]

https://codereview.chromium.org/613733002/diff/40001/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/613733002/diff/40001/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.cc:169: void
set_access_control_allow_origin_header(const std::string& value) {
On 2014/10/03 17:02:45, Tom Sepez wrote:
> Nit: we're actually setting the value for the header, not the full header
> itself.  You might call this set_access_control_allow_origin_header_value()
but
> this name is getting ridiculously long as it is. It's your call.

I ended up with "set_access_control_allow_origin" and
"GetAccessControlAllowOriginForOrigin".

https://codereview.chromium.org/613733002/diff/40001/content/public/browser/u...
File content/public/browser/url_data_source.cc (right):

https://codereview.chromium.org/613733002/diff/40001/content/public/browser/u...
content/public/browser/url_data_source.cc:61: return "";
On 2014/10/03 17:02:45, Tom Sepez wrote:
> nit:  I thought the preferred way of returning an empty string was to return
> std::string().
Right. Done.

[Charlie Reis, 2014-10-06 19:43:20]

Sorry for the delay; I was OOO on Friday.  Mostly good, based on Tom's review.

https://codereview.chromium.org/613733002/diff/60001/content/browser/webui/sh...
File content/browser/webui/shared_resources_data_source.cc (right):

https://codereview.chromium.org/613733002/diff/60001/content/browser/webui/sh...
content/browser/webui/shared_resources_data_source.cc:96: std::string
allowed_origin_prefix = content::kChromeUIScheme;
nit: Let's put a comment here saying that we're intentionally echoing chrome://
origins in the CORS header, and why.  (That will help future people reading this
code.)

https://codereview.chromium.org/613733002/diff/60001/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/613733002/diff/60001/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.cc:104: return result;
nit: This is a strange way to phrase it, because we "return result;" either way.

https://codereview.chromium.org/613733002/diff/60001/content/public/browser/u...
File content/public/browser/url_data_source.h (right):

https://codereview.chromium.org/613733002/diff/60001/content/public/browser/u...
content/public/browser/url_data_source.h:128: virtual std::string
GetAccessControlAllowOriginForOrigin(
This looks like it is only used within content/, so it should not be part of the
public API.

See http://www.chromium.org/developers/content-module/content-api:
"only expose methods in the public API that embedders need. If a method is only
used by other code in content, it belongs in foo_impl.h and not foo.h."

[dzhioev (left Google), 2014-10-08 18:41:38]

https://codereview.chromium.org/613733002/diff/60001/content/browser/webui/ur...
File content/browser/webui/url_data_manager_backend.cc (right):

https://codereview.chromium.org/613733002/diff/60001/content/browser/webui/ur...
content/browser/webui/url_data_manager_backend.cc:104: return result;
On 2014/10/06 19:43:20, Charlie Reis wrote:
> nit: This is a strange way to phrase it, because we "return result;" either
way.

Done.

https://codereview.chromium.org/613733002/diff/60001/content/public/browser/u...
File content/public/browser/url_data_source.h (right):

https://codereview.chromium.org/613733002/diff/60001/content/public/browser/u...
content/public/browser/url_data_source.h:128: virtual std::string
GetAccessControlAllowOriginForOrigin(
On 2014/10/06 19:43:20, Charlie Reis wrote:
> This looks like it is only used within content/, so it should not be part of
the
> public API.
>
We can't override this method in SharedResourceDataSource if it is not a part of
public API. Plus I don't see how this method differs from other customization
methods (GetContentSecurityPolicyObjectSrc or ShouldDenyXFrameOptions). The fact
that the only URLDataSource overriding this method lives in content doesn't mean
that other data sources will not override it in future.
 
> See http://www.chromium.org/developers/content-module/content-api:
> "only expose methods in the public API that embedders need. If a method is
only
> used by other code in content, it belongs in foo_impl.h and not foo.h."

[dzhioev (left Google), 2014-10-08 18:41:57]

https://codereview.chromium.org/613733002/diff/60001/content/browser/webui/sh...
File content/browser/webui/shared_resources_data_source.cc (right):

https://codereview.chromium.org/613733002/diff/60001/content/browser/webui/sh...
content/browser/webui/shared_resources_data_source.cc:96: std::string
allowed_origin_prefix = content::kChromeUIScheme;
On 2014/10/06 19:43:20, Charlie Reis wrote:
> nit: Let's put a comment here saying that we're intentionally echoing
chrome://
> origins in the CORS header, and why.  (That will help future people reading
this
> code.)

Done.

[Charlie Reis, 2014-10-08 20:46:23]

creis@chromium.org changed reviewers:
+ jam@chromium.org

[Charlie Reis, 2014-10-08 20:46:24]

[+jam for content public API question]

https://codereview.chromium.org/613733002/diff/60001/content/public/browser/u...
File content/public/browser/url_data_source.h (right):

https://codereview.chromium.org/613733002/diff/60001/content/public/browser/u...
content/public/browser/url_data_source.h:128: virtual std::string
GetAccessControlAllowOriginForOrigin(
On 2014/10/08 18:41:38, dzhioev wrote:
> On 2014/10/06 19:43:20, Charlie Reis wrote:
> > This looks like it is only used within content/, so it should not be part of
> the
> > public API.
> >
> We can't override this method in SharedResourceDataSource if it is not a part
of
> public API.

It would work if there were an impl class that SharedResourceDataSource
inherited from, which the (confusingly named) URLDataSourceImpl could return
from its source() method.

I see that it's a bit awkward in this case due to how URLDataSourceImpl works,
but I'd like John to weigh in on that vs. the Content API rule.

> Plus I don't see how this method differs from other customization
> methods (GetContentSecurityPolicyObjectSrc or ShouldDenyXFrameOptions). The
fact
> that the only URLDataSource overriding this method lives in content doesn't
mean
> that other data sources will not override it in future.

We tend to not expose things until they're needed.  Both
GetContentSecurityPolicyObjectSrc and ShouldDenyXFrameOptions are used in
chrome/.

>  
> > See http://www.chromium.org/developers/content-module/content-api:
> > "only expose methods in the public API that embedders need. If a method is
> only
> > used by other code in content, it belongs in foo_impl.h and not foo.h."
> 
>

[jam, 2014-10-08 21:45:37]

On 2014/10/08 20:46:24, Charlie Reis wrote:
> [+jam for content public API question]
> 
>
https://codereview.chromium.org/613733002/diff/60001/content/public/browser/u...
> File content/public/browser/url_data_source.h (right):
> 
>
https://codereview.chromium.org/613733002/diff/60001/content/public/browser/u...
> content/public/browser/url_data_source.h:128: virtual std::string
> GetAccessControlAllowOriginForOrigin(
> On 2014/10/08 18:41:38, dzhioev wrote:
> > On 2014/10/06 19:43:20, Charlie Reis wrote:
> > > This looks like it is only used within content/, so it should not be part
of
> > the
> > > public API.
> > >
> > We can't override this method in SharedResourceDataSource if it is not a
part
> of
> > public API.
> 
> It would work if there were an impl class that SharedResourceDataSource
> inherited from, which the (confusingly named) URLDataSourceImpl could return
> from its source() method.
> 
> I see that it's a bit awkward in this case due to how URLDataSourceImpl works,
> but I'd like John to weigh in on that vs. the Content API rule.

yeah agreed it's awkward. in this case, the current code seems fine (even though
it does break the content api rule, which in this case is an exception)
> 
> > Plus I don't see how this method differs from other customization
> > methods (GetContentSecurityPolicyObjectSrc or ShouldDenyXFrameOptions). The
> fact
> > that the only URLDataSource overriding this method lives in content doesn't
> mean
> > that other data sources will not override it in future.
> 
> We tend to not expose things until they're needed.  Both
> GetContentSecurityPolicyObjectSrc and ShouldDenyXFrameOptions are used in
> chrome/.
> 
> >  
> > > See http://www.chromium.org/developers/content-module/content-api:
> > > "only expose methods in the public API that embedders need. If a method is
> > only
> > > used by other code in content, it belongs in foo_impl.h and not foo.h."
> > 
> >

[Charlie Reis, 2014-10-08 21:47:48]

Thanks John!  LGTM then.

[dzhioev (left Google), 2014-10-10 15:07:02]

On 2014/10/08 21:47:48, Charlie Reis wrote:
> Thanks John!  LGTM then.

Thanks.
James, could you please review chrome/renderer/chrome_content_renderer_client.cc
and content/browser/webui/* ?

[James Hawkins, 2014-10-10 16:30:20]

lgtm

[dzhioev (left Google), 2014-10-10 16:37:49]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-10-10 16:38:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613733002/80001

[commit-bot: I haz the power, 2014-10-10 16:41:31]

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

[commit-bot: I haz the power, 2014-10-10 16:41:32]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/67260)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/72017)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dzhioev (left Google), 2014-10-10 17:18:08]

The CQ bit was checked by dzhioev@chromium.org

[commit-bot: I haz the power, 2014-10-10 17:19:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/613733002/240001

[commit-bot: I haz the power, 2014-10-10 18:55:59]

Committed patchset #6 (id:240001)

[commit-bot: I haz the power, 2014-10-10 18:57:26]

Patchset 6 (id:??) landed as
https://crrev.com/fd3b257aacfddcd11c925bbea203417dbab9f6be
Cr-Commit-Position: refs/heads/master@{#299153}