Test downloads.download() disallows unsafe headers

Test downloads.download() disallows unsafe headers


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

[Randy Smith (Not in Mondays), 2012-05-01 17:57:34]

How was the list of headers to block determined?

[benjhayden, 2012-05-01 18:09:55]

On 2012/05/01 17:57:34, rdsmith wrote:
> How was the list of headers to block determined?

http://www.w3.org/TR/XMLHttpRequest/#the-setrequestheader-method

[Randy Smith (Not in Mondays), 2012-05-02 17:52:26]

You're not testing:

Access-Control-Request-Headers
Access-Control-Request-Method
Origin
Upgrade
User-Agent
Via

Why not?

(You're getting "zen-mind" questions here, which may mean you don't have the
right reviewer, but hey, we'll see where it goes :-}.)

[benjhayden, 2012-05-02 18:18:38]

On 2012/05/02 17:52:26, rdsmith wrote:
> You're not testing:
> 
> Access-Control-Request-Headers
> Access-Control-Request-Method
> Origin
> Upgrade
> User-Agent
> Via
> 
> Why not?
> 
> (You're getting "zen-mind" questions here, which may mean you don't have the
> right reviewer, but hey, we'll see where it goes :-}.)

Actually, I was testing the last three, and the first two aren't caught by
net::HttpUtil::IsSafeHeader(), but I added Origin. Note that I jumbled the case
of some characters to test case-insensitivity.

[Randy Smith (Not in Mondays), 2012-05-03 19:13:17]

LGTM (but agree it's worthwhile at least putting a moment or two into figuring
out why the filter isn't catching all the headers it should.)

[benjhayden, 2012-05-03 19:44:45]

On 2012/05/03 19:13:17, rdsmith wrote:
> LGTM (but agree it's worthwhile at least putting a moment or two into figuring
> out why the filter isn't catching all the headers it should.)

I'll go see who wrote IsSafeHeader().
Eek, it was me!
Looks like the spec changed to include some new headers.
I'll add them in a separate CL.

[benjhayden, 2012-05-03 19:46:07]

On 2012/05/03 19:44:45, benjhayden_chromium wrote:
> On 2012/05/03 19:13:17, rdsmith wrote:
> > LGTM (but agree it's worthwhile at least putting a moment or two into
figuring
> > out why the filter isn't catching all the headers it should.)
> 
> I'll go see who wrote IsSafeHeader().
> Eek, it was me!
> Looks like the spec changed to include some new headers.
> I'll add them in a separate CL.

Do you want this cl 10260018 to wait for the cl that adds the new headers to
IsSafeHeader()? Or can this go on in?

[Randy Smith (Not in Mondays), 2012-05-03 20:05:03]

I can't think of any reason to delay adding valid tests; I'm fine with this
going in.

[commit-bot: I haz the power, 2012-05-03 21:05:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/benjhayden@chromium.org/10260018/14001

[commit-bot: I haz the power, 2012-05-03 23:56:39]

Change committed as 135243