[ServiceWorker] Fills SSLInfo of the response from a SW with the SSLInfo of the SW script.

[ServiceWorker] Fills SSLInfo of the response from a SW with the SSLInfo of the SW script.

After https://codereview.chromium.org/874833003/, every mixed-content requests from the ServiceWorker are blocked.
So we can show the HTTPS padlock because every responses from the ServiceWorker to the page are created using the resources which are served through the secure protocol.

To show the padlock we fill the SSLInfo of the response from the ServiceWorker using the SSLInfo of the ServiceWorker script.

When we support fetching mixed-content requests from the ServiceWorker in the future, we have to check the security level of the responses.

BUG=392409
Committed: https://crrev.com/4bd0e01e3027e569f03b0cb18a46b72263f78a1d
Cr-Commit-Position: refs/heads/master@{#314549}

[horo, 2015-01-27 01:45:18]

Patchset #1 (id:1) has been deleted

[horo, 2015-01-27 01:45:26]

Patchset #1 (id:20001) has been deleted

[horo, 2015-01-27 01:45:32]

Patchset #1 (id:40001) has been deleted

[horo, 2015-01-27 02:06:03]

horo@chromium.org changed reviewers:
+ falken@chromium.org

[horo, 2015-01-27 02:06:03]

falken@
Could you please review this?

[falken, 2015-01-27 03:57:54]

This looks pretty good, just have some questions.

Also, I think you should get this reviewed by a security expert before
committing.

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:806:
IN_PROC_BROWSER_TEST_F(ServiceWorkerBrowserTest, Secure) {
Can you give a name that describes more what this tests? I guess
"ResponseFromServiceWorkerIsMarkedAsSecure"?

Should we test the negative case that an insecure SW doesn't have the padlock?
Like test "http://localhost/" somehow?

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:832:
shell()->web_contents()->GetController().GetActiveEntry();
GetActiveEntry comments say "THIS IS DEPRECATED. DO NOT USE. Use GetVisibleEntry
instead." Should we use GetVisibleEntry instead?

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:415:
version_->SetMainScriptSSLInfo(net_request_->response_info().ssl_info);
Is it OK to only look at the main script resource? Is it possible to do
importScripts("http://...")?

[horo, 2015-01-27 06:04:41]

Thank you!

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:806:
IN_PROC_BROWSER_TEST_F(ServiceWorkerBrowserTest, Secure) {
On 2015/01/27 03:57:54, falken wrote:
> Can you give a name that describes more what this tests? I guess
> "ResponseFromServiceWorkerIsMarkedAsSecure"?
> 
> Should we test the negative case that an insecure SW doesn't have the padlock?
> Like test "http://localhost/" somehow?

Done.

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_browsertest.cc:832:
shell()->web_contents()->GetController().GetActiveEntry();
On 2015/01/27 03:57:54, falken wrote:
> GetActiveEntry comments say "THIS IS DEPRECATED. DO NOT USE. Use
GetVisibleEntry
> instead." Should we use GetVisibleEntry instead?

Done.

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_write_to_cache_job.cc
(right):

https://codereview.chromium.org/877623002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_write_to_cache_job.cc:415:
version_->SetMainScriptSSLInfo(net_request_->response_info().ssl_info);
On 2015/01/27 03:57:54, falken wrote:
> Is it OK to only look at the main script resource? Is it possible to do
> importScripts("http://...")?

I think it is OK.
importScripts("http://...") is blocked by the mixed-content checking.

[falken, 2015-01-27 06:39:58]

lgtm

https://codereview.chromium.org/877623002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/877623002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:544: //
ServiceWorker, we have to check the security level of the responses.
can we DCHECK(!http_response_info_)

[horo, 2015-01-27 06:48:15]

https://codereview.chromium.org/877623002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/877623002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:544: //
ServiceWorker, we have to check the security level of the responses.
On 2015/01/27 06:39:58, falken wrote:
> can we DCHECK(!http_response_info_)

Done.

[horo, 2015-01-27 06:49:18]

horo@chromium.org changed reviewers:
+ rsleevi@chromium.org

[horo, 2015-01-27 06:49:19]

rsleevi@
Could you please review this?

[Ryan Sleevi, 2015-01-28 01:04:03]

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:548:
provider_host_->active_version()->GetMainScriptSSLInfo();
Why do you only copy the SSLInfo? Why not the full HttpResponseInfo?

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:135:
EXPECT_TRUE(ssl_info.is_valid());
Hrm. This is probably bad for //net that we expose a confusing API (in that it
doesn't check that |security_bits| or |connection_status| are within valid
ranges.

I would also set values for those and check to make sure they carry over.

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.h:299: void
SetMainScriptSSLInfo(const net::SSLInfo& ssl_info);
Why is this called "main script"? what's the difference?

[horo, 2015-01-28 02:30:21]

Patchset #5 (id:140001) has been deleted

[horo, 2015-01-28 02:32:06]

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:548:
provider_host_->active_version()->GetMainScriptSSLInfo();
On 2015/01/28 01:04:02, Ryan Sleevi wrote:
> Why do you only copy the SSLInfo? Why not the full HttpResponseInfo?

Done.

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job_unittest.cc
(right):

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job_unittest.cc:135:
EXPECT_TRUE(ssl_info.is_valid());
On 2015/01/28 01:04:02, Ryan Sleevi wrote:
> Hrm. This is probably bad for //net that we expose a confusing API (in that it
> doesn't check that |security_bits| or |connection_status| are within valid
> ranges.
> 
> I would also set values for those and check to make sure they carry over.

Done.

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/877623002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_version.h:299: void
SetMainScriptSSLInfo(const net::SSLInfo& ssl_info);
On 2015/01/28 01:04:02, Ryan Sleevi wrote:
> Why is this called "main script"? what's the difference?

We distinguish the main script and the imported scripts using importScript()
form the main script.

[horo, 2015-01-30 02:00:27]

rsleevi@
Ping?

[Ryan Sleevi, 2015-01-30 23:20:44]

Apologies! I thought I published these comments already.

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:814: const
std::string kPageUrl = "files/service_worker/fetch_event_blob.html";
nit: I find the "const std::string" here a little surprising/awkward, since so
we so rarely use it for literal constants.

Any reason not to just do

const char kPageUrl[] = "..."?

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:825: }
Why is this in a block? Is the destruction of observer important for the
correctness of this test?

Given that it's a scoped_refptr<>, it's not guaranteed to be destructed, so I
mean, it doesn't sound like it is.

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:835: }
Why is this in a block?

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:843: }
Why is this in a block? Is the destruction of the runloop important?

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:850: {
ditto comments above with these tests.

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_read_from_cache_job.h
(right):

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_read_from_cache_job.h:33:
ServiceWorkerVersion* version,
Since you're storing a reference count to this object, please pass it as a

const scoped_refptr<ServiceWorkerVersion>& version,

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:628:
NotifyHeadersComplete();
My understanding is that |request_time| and |response_time| are (eventually)
serviced up via Public API (Navigation Timing, I believe).

Should this information be made consistent with SW to reflect SW execution time?
(NB: I'm fine pushing that aspect to a separate detail, but I do want to make
sure it's considered)

[horo, 2015-01-31 08:34:07]

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:814: const
std::string kPageUrl = "files/service_worker/fetch_event_blob.html";
On 2015/01/30 23:20:43, Ryan Sleevi wrote:
> nit: I find the "const std::string" here a little surprising/awkward, since so
> we so rarely use it for literal constants.
> 
> Any reason not to just do
> 
> const char kPageUrl[] = "..."?

Done.

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:825: }
On 2015/01/30 23:20:43, Ryan Sleevi wrote:
> Why is this in a block? Is the destruction of observer important for the
> correctness of this test?
> 
> Given that it's a scoped_refptr<>, it's not guaranteed to be destructed, so I
> mean, it doesn't sound like it is.

Removed the bracket.

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:835: }
On 2015/01/30 23:20:43, Ryan Sleevi wrote:
> Why is this in a block?

Removed the bracket.

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:843: }
On 2015/01/30 23:20:43, Ryan Sleevi wrote:
> Why is this in a block? Is the destruction of the runloop important?

We don't need put them to the block.
Removed the bracket.

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_browsertest.cc:850: {
On 2015/01/30 23:20:43, Ryan Sleevi wrote:
> ditto comments above with these tests.

Done.

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_read_from_cache_job.h
(right):

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_read_from_cache_job.h:33:
ServiceWorkerVersion* version,
On 2015/01/30 23:20:43, Ryan Sleevi wrote:
> Since you're storing a reference count to this object, please pass it as a
> 
> const scoped_refptr<ServiceWorkerVersion>& version,

Done.

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/877623002/diff/180001/content/browser/service...
content/browser/service_worker/service_worker_url_request_job.cc:628:
NotifyHeadersComplete();
On 2015/01/30 23:20:43, Ryan Sleevi wrote:
> My understanding is that |request_time| and |response_time| are (eventually)
> serviced up via Public API (Navigation Timing, I believe).
> 
> Should this information be made consistent with SW to reflect SW execution
time?
> (NB: I'm fine pushing that aspect to a separate detail, but I do want to make
> sure it's considered)

Ah, in current implementation, info->request_time is set in URLRequest::Start()
but is overwritten to 0 in ServiceWorkerURLRequestJob::GetResponseInfo().
This is bug.
I changed GetResponseInfo() not to overwrite request_time.

info->response_time is correctly set to response_time_ in GetResponseInfo().

[Ryan Sleevi, 2015-02-02 19:39:07]

lgtm

https://codereview.chromium.org/877623002/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/877623002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_version.h:301: // from the
ServiceWorker to the page to show the HTTPS padlock.
comment nit: Reads a bit weird

// Sets the HttpResponseInfo used to load the main script.
// This HttpResponseInfo will be used for all responses sent back from the
// service worker, as the effective security of these responses is equivalent
// to that of the ServiceWorker.


(In addition to the wording weirdness, I consider it a bit of a layering
violation to talk about HTTPS padlock from this //content/browser/service_worker
code, since that's a UI element)

[horo, 2015-02-02 20:04:28]

Thank you!

https://codereview.chromium.org/877623002/diff/200001/content/browser/service...
File content/browser/service_worker/service_worker_version.h (right):

https://codereview.chromium.org/877623002/diff/200001/content/browser/service...
content/browser/service_worker/service_worker_version.h:301: // from the
ServiceWorker to the page to show the HTTPS padlock.
On 2015/02/02 19:39:07, Ryan Sleevi wrote:
> comment nit: Reads a bit weird
> 
> // Sets the HttpResponseInfo used to load the main script.
> // This HttpResponseInfo will be used for all responses sent back from the
> // service worker, as the effective security of these responses is equivalent
> // to that of the ServiceWorker.
> 
> 
> (In addition to the wording weirdness, I consider it a bit of a layering
> violation to talk about HTTPS padlock from this
//content/browser/service_worker
> code, since that's a UI element)

Done.

[horo, 2015-02-02 20:07:11]

horo@chromium.org changed reviewers:
+ jochen@chromium.org

[horo, 2015-02-02 20:07:11]

jochen@
Could you please review the change in content/content_unittests.isolate?
ServiceWorkerURLRequestJobTest uses /net/data/ssl/certificates/ok_cert.pem.

[jochen (gone - plz use gerrit), 2015-02-04 12:35:09]

lgtm

[horo, 2015-02-04 12:49:46]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-02-04 12:50:20]

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

[commit-bot: I haz the power, 2015-02-04 12:53:53]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2015-02-04 12:54:48]

Patchset 8 (id:??) landed as
https://crrev.com/4bd0e01e3027e569f03b0cb18a46b72263f78a1d
Cr-Commit-Position: refs/heads/master@{#314549}