Process only the first Strict-Transport-Security header.

Process only the first Strict-Transport-Security header.

In accordance with the specification.

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

[palmer, 2012-10-17 23:47:39]

Hopefully a quick one for you.

[abarth-chromium, 2012-10-18 00:26:08]

I can haz test?

[palmer, 2012-10-18 00:37:39]

> I can haz test?

Any idea where I can find tests for code like this? I was hoping to find your
original test for ProcessStrictTransportSecurityHeader but I didn't find one. ;)

I'm currently groveling around in url_request_unittest.cc, but if you can point
me to a better example, that'd be great.

[abarth-chromium, 2012-10-18 00:38:37]

Sorry, I don't remember how this stuff is tested.  Someone in the net OWNERS
file might know off hand.

[palmer, 2012-10-18 00:54:41]

Let's rope in rsleevi. :)

[Ryan Sleevi, 2012-10-18 01:01:20]

Wow. There is a depressing lack of (individual/specific) unittests overall in
net/url_request. This saddens me.

The "heavy handed" way would be to create a unit test that starts a
net::TestServer, which uses the Python test server, that serves a page with 1 -
n of these headers (such as just reflecting a query string back), and then check
in the TransportSecurityState that everything looks as expected.

The "ideal" way would be able to provide simulated HTTP Request/Response data
and simply use that to perform the testing, which will be scads quicker, as it
requires no test server startup/shutdown.

To do the heavy-handed way, see url_request_unittest.cc, which has the various
URLRequestTestHTTP tests that do exactly that.

To do the "ideal" way, you'd have to start messing with
DeterminisicSocketDataFactories and friends, and I'm not convinced that it's
strictly necessary (although a GREAT learning experience and a great
optimization that can be made)

[willchan no longer on Chromium, 2012-10-18 01:09:58]

For URLRequest we generally do regression tests via the TestServer. I
agree with Ryan that a MockSocket method would be better. But in
interest of time, I'd recommend a TestServer based approach. If you
look at net/data/url_request_unittest/ you'll see that the TestServer
can inject mock response headers as needed.

On Wed, Oct 17, 2012 at 6:01 PM,  <rsleevi@chromium.org> wrote:
> Wow. There is a depressing lack of (individual/specific) unittests overall
> in
> net/url_request. This saddens me.
>
> The "heavy handed" way would be to create a unit test that starts a
> net::TestServer, which uses the Python test server, that serves a page with
> 1 -
> n of these headers (such as just reflecting a query string back), and then
> check
> in the TransportSecurityState that everything looks as expected.
>
> The "ideal" way would be able to provide simulated HTTP Request/Response
> data
> and simply use that to perform the testing, which will be scads quicker, as
> it
> requires no test server startup/shutdown.
>
> To do the heavy-handed way, see url_request_unittest.cc, which has the
> various
> URLRequestTestHTTP tests that do exactly that.
>
> To do the "ideal" way, you'd have to start messing with
> DeterminisicSocketDataFactories and friends, and I'm not convinced that it's
> strictly necessary (although a GREAT learning experience and a great
> optimization that can be made)
>
> https://codereview.chromium.org/11192045/

[palmer, 2012-10-18 19:36:53]

Now with test. PTAL. Thank you, everyone!

[Ryan Sleevi, 2012-10-18 19:45:34]

Overall this test looks good, but a few nits below, and a request for one more
test :)

https://codereview.chromium.org/11192045/diff/8002/net/data/url_request_unitt...
File net/data/url_request_unittest/hsts-headers.html (right):

https://codereview.chromium.org/11192045/diff/8002/net/data/url_request_unitt...
net/data/url_request_unittest/hsts-headers.html:1: This file is boring; all the
action's in the .mock-http-headers.
naming nit: suggest that you rename this file to indicate it's testing duplicate
hsts-headers / non-coalesced-hsts-headers

https://codereview.chromium.org/11192045/diff/8002/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/11192045/diff/8002/net/http/http_util.cc#newc...
net/http/http_util.cc:398: // declare it non-coalescing.
comment nit: A nit I inherited from mark@, which is to discourage "we" in
comments. I find this is generally good, as it forces comments to be more
assertative.

// STS specifies that UAs must not process any STS headers after the first one.

https://codereview.chromium.org/11192045/diff/8002/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11192045/diff/8002/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:3063: TEST_F(URLRequestTest,
ProcessSTSOnce) {
nit: name this URLRequestTestHTTP

https://codereview.chromium.org/11192045/diff/8002/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:3087:
EXPECT_FALSE(domain_state.include_subdomains);
test nit: I would suggest adding a positive test that ensures a single STS
header, WITH include_subdomains, IS processed correctly.

Without the positive test, this test isn't fully covering the expected
behaviour, since it could be hidden by failing to handle include_subdomains
properly

[palmer, 2012-10-18 21:01:58]

https://codereview.chromium.org/11192045/diff/8002/net/data/url_request_unitt...
File net/data/url_request_unittest/hsts-headers.html (right):

https://codereview.chromium.org/11192045/diff/8002/net/data/url_request_unitt...
net/data/url_request_unittest/hsts-headers.html:1: This file is boring; all the
action's in the .mock-http-headers.
On 2012/10/18 19:45:34, Ryan Sleevi wrote:
> naming nit: suggest that you rename this file to indicate it's testing
duplicate
> hsts-headers / non-coalesced-hsts-headers

Done.

https://codereview.chromium.org/11192045/diff/8002/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/11192045/diff/8002/net/http/http_util.cc#newc...
net/http/http_util.cc:398: // declare it non-coalescing.
On 2012/10/18 19:45:34, Ryan Sleevi wrote:
> comment nit: A nit I inherited from mark@, which is to discourage "we" in
> comments. I find this is generally good, as it forces comments to be more
> assertative.
> 
> // STS specifies that UAs must not process any STS headers after the first
one.

Done.

https://codereview.chromium.org/11192045/diff/8002/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/11192045/diff/8002/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:3063: TEST_F(URLRequestTest,
ProcessSTSOnce) {
On 2012/10/18 19:45:34, Ryan Sleevi wrote:
> nit: name this URLRequestTestHTTP

Done.

https://codereview.chromium.org/11192045/diff/8002/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:3087:
EXPECT_FALSE(domain_state.include_subdomains);
On 2012/10/18 19:45:34, Ryan Sleevi wrote:
> test nit: I would suggest adding a positive test that ensures a single STS
> header, WITH include_subdomains, IS processed correctly.
> 
> Without the positive test, this test isn't fully covering the expected
> behaviour, since it could be hidden by failing to handle include_subdomains
> properly

Done.

[Ryan Sleevi, 2012-10-18 21:06:53]

lgtm

[commit-bot: I haz the power, 2012-10-18 21:49:07]

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

[commit-bot: I haz the power, 2012-10-19 03:22:40]

Retried try job too often for step(s) chrome_frame_net_tests

[commit-bot: I haz the power, 2012-10-19 03:30:17]

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

[commit-bot: I haz the power, 2012-10-19 07:00:42]

Retried try job too often for step(s) chrome_frame_net_tests

[commit-bot: I haz the power, 2012-10-19 17:11:59]

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

[commit-bot: I haz the power, 2012-10-19 21:37:58]

Retried try job too often for step(s) chrome_frame_net_tests

[commit-bot: I haz the power, 2012-10-30 18:58:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/11192045/38001

[commit-bot: I haz the power, 2012-10-30 18:59:03]

Presubmit check for 11192045-38001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome_frame

Presubmit checks took 2.5s to calculate.

[Chris Palmer, 2012-10-30 19:00:39]

Erik, can I bother you one last time :) for an OWNERS LGTM? Thank you!

[erikwright (departed), 2012-10-30 19:02:41]

chrome_frame LGTM

[commit-bot: I haz the power, 2012-10-30 19:04:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/11192045/38001

[commit-bot: I haz the power, 2012-10-30 19:11:12]

Sorry for I got bad news for ya.
Compile failed with a clobber build on mac_rel.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-10-30 19:52:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/11192045/38001

[commit-bot: I haz the power, 2012-10-30 21:20:48]

Change committed as 165013