Block all scripts from setting unsafe headers in XMLHttpRequest

Block all scripts from setting unsafe headers in XMLHttpRequest.

Previously, privileged scripts (file:// URLs) could set all headers. This patch blocks all scripts from setting any unsafe headers.

BUG=196071
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=149364

[meacer, 2013-04-25 00:26:58]

Hi Adam, can you please review? Thanks!

[abarth-chromium, 2013-04-25 00:30:57]

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:455: return !equalIgnoringCase(name,
"referer");
What about Host and all the other sensitive HTTP headers?

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:931: if
(!securityOrigin()->canLoadLocalResources() && !isAllowedHTTPHeader(name)) {
IMHO, we should just remove canLoadLocalResources() from this line.  There's no
reason why being able to load local resources should let you set the dangerous
HTTP headers.

[meacer, 2013-04-25 00:34:58]

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:455: return !equalIgnoringCase(name,
"referer");
On 2013/04/25 00:30:57, abarth wrote:
> What about Host and all the other sensitive HTTP headers?

Per your comment below, looks like we don't need this method at all?

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:931: if
(!securityOrigin()->canLoadLocalResources() && !isAllowedHTTPHeader(name)) {
On 2013/04/25 00:30:57, abarth wrote:
> IMHO, we should just remove canLoadLocalResources() from this line.  There's
no
> reason why being able to load local resources should let you set the dangerous
> HTTP headers.

So just to clarify, we'll have a single set of disallowed headers among all
schemes, which means I won't need isLocallyAllowedHTTPHeader too. Is that
correct?

[abarth-chromium, 2013-04-25 03:28:10]

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:455: return !equalIgnoringCase(name,
"referer");
On 2013/04/25 00:34:58, Mustafa Emre Acer wrote:
> On 2013/04/25 00:30:57, abarth wrote:
> > What about Host and all the other sensitive HTTP headers?
> 
> Per your comment below, looks like we don't need this method at all?

Correct.

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:931: if
(!securityOrigin()->canLoadLocalResources() && !isAllowedHTTPHeader(name)) {
On 2013/04/25 00:34:58, Mustafa Emre Acer wrote:
> On 2013/04/25 00:30:57, abarth wrote:
> > IMHO, we should just remove canLoadLocalResources() from this line.  There's
> no
> > reason why being able to load local resources should let you set the
dangerous
> > HTTP headers.
> 
> So just to clarify, we'll have a single set of disallowed headers among all
> schemes, which means I won't need isLocallyAllowedHTTPHeader too. Is that
> correct?

Correct.

[meacer, 2013-04-25 17:43:21]

Thanks Adam, please see the latest patch.

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:455: return !equalIgnoringCase(name,
"referer");
On 2013/04/25 03:28:10, abarth wrote:
> On 2013/04/25 00:34:58, Mustafa Emre Acer wrote:
> > On 2013/04/25 00:30:57, abarth wrote:
> > > What about Host and all the other sensitive HTTP headers?
> > 
> > Per your comment below, looks like we don't need this method at all?
> 
> Correct.

Done.

https://codereview.chromium.org/13979011/diff/2001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:931: if
(!securityOrigin()->canLoadLocalResources() && !isAllowedHTTPHeader(name)) {
On 2013/04/25 03:28:10, abarth wrote:
> On 2013/04/25 00:34:58, Mustafa Emre Acer wrote:
> > On 2013/04/25 00:30:57, abarth wrote:
> > > IMHO, we should just remove canLoadLocalResources() from this line. 
There's
> > no
> > > reason why being able to load local resources should let you set the
> dangerous
> > > HTTP headers.
> > 
> > So just to clarify, we'll have a single set of disallowed headers among all
> > schemes, which means I won't need isLocallyAllowedHTTPHeader too. Is that
> > correct?
> 
> Correct.

Done.

[abarth-chromium, 2013-04-26 04:06:49]

lgtm

Great!

[commit-bot: I haz the power, 2013-04-26 04:12:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/13979011/13001

[commit-bot: I haz the power, 2013-04-26 04:18:11]

Presubmit check for 13979011-13001 failed and returned exit status -2001.
The presubmit check was hung. It took 360.0 seconds to execute and the time
limit is 360.0 seconds.

INFO:root:Found 3 file(s).

[commit-bot: I haz the power, 2013-04-26 17:46:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/13979011/13001

[commit-bot: I haz the power, 2013-04-26 18:26:19]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-04-29 17:28:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/13979011/13001

[commit-bot: I haz the power, 2013-04-29 18:06:52]

Retried try job too often on linux_layout_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layo...

[commit-bot: I haz the power, 2013-04-29 20:32:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/meacer@chromium.org/13979011/13001

[commit-bot: I haz the power, 2013-04-29 21:28:19]

Change committed as 149364