Do no AppCache responses with SSL cert errors.

content/browser/appcache/appcache_update_job.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/appcache/appcache_update_job.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 143 matching lines @@
 
 void AppCacheUpdateJob::URLFetcher::OnResponseStarted(
     net::URLRequest *request) {
   DCHECK(request == request_);
   int response_code = -1;
   if (request->status().is_success()) {
     response_code = request->GetResponseCode();
     job_->MadeProgress();
   }
   if ((response_code / 100) == 2) {
 

[Ryan Sleevi, 2014/10/11 00:54:44]

unnecessary newline?

[michaeln, 2014/10/15 19:20:22]

Done.

-    // See http://code.google.com/p/chromium/issues/detail?id=69594
-    // We willfully violate the HTML5 spec at this point in order
-    // to support the appcaching of cross-origin HTTPS resources.
-    // We've opted for a milder constraint and allow caching unless
-    // the resource has a "no-store" header. A spec change has been
-    // requested on the whatwg list.
-    // TODO(michaeln): Consider doing this for cross-origin HTTP resources too.
-    if (url_.SchemeIsSecure() &&
-        url_.GetOrigin() != job_->manifest_url_.GetOrigin()) {
-      if (request->response_headers()->
-              HasHeaderValue("cache-control", "no-store")) {
+    if (url_.SchemeIsSecure()) {
+      // See http://code.google.com/p/chromium/issues/detail?id=69594
+      // We willfully violate the HTML5 spec at this point in order
+      // to support the appcaching of cross-origin HTTPS resources.
+      // We've opted for a milder constraint and allow caching unless
+      // the resource has a "no-store" header. A spec change has been
+      // requested on the whatwg list.
+      // TODO(michaeln): Consider doing this for cross-origin HTTP too.
+      if (url_.GetOrigin() != job_->manifest_url_.GetOrigin()) {
+        if (request->response_headers()->
+                HasHeaderValue("cache-control", "no-store")) {
+          DCHECK_EQ(-1, redirect_response_code_);
+          request->Cancel();
+          result_ = SERVER_ERROR;  // Not the best match?

[palmer, 2014/10/10 23:48:10]

Maybe CANCELED_ERROR, or add a new one, like NOT_SUPPORTED_ERROR or something?

[michaeln, 2014/10/15 19:20:22]

SECURITY_ERROR?

+          OnResponseCompleted();
+          return;
+        }
+      }
+
+      // Do not cache content with cert errors.
+      if (net::IsCertStatusError(request->ssl_info().cert_status)) {

[palmer, 2014/10/10 23:50:32]

Lines 186 – 190 duplicate lines 176 – 180; maybe just add the condition on line
185 to the condition in lines 174 – 175?

[michaeln, 2014/10/15 19:20:21]

Done.

         DCHECK_EQ(-1, redirect_response_code_);
         request->Cancel();
         result_ = SERVER_ERROR;  // Not the best match?
         OnResponseCompleted();
         return;
       }
     }
 
     // Write response info to storage for URL fetches. Wait for async write
     // completion before reading any response data.
     if (fetch_type_ == URL_FETCH || fetch_type_ == MASTER_ENTRY_FETCH) {
       response_writer_.reset(job_->CreateResponseWriter());
       scoped_refptr<HttpResponseInfoIOBuffer> io_buffer(
           new HttpResponseInfoIOBuffer(
               new net::HttpResponseInfo(request->response_info())));
       response_writer_->WriteInfo(
           io_buffer.get(),
           base::Bind(&URLFetcher::OnWriteComplete, base::Unretained(this)));
     } else {
       ReadResponseData();
     }
   } else {
     if (response_code > 0)
       result_ = SERVER_ERROR;
     else
       result_ = NETWORK_ERROR;
     OnResponseCompleted();

[Ryan Sleevi, 2014/10/11 00:54:44]

This reads like error handling under success handling, which our style guide
discourages ( http://www.chromium.org/developers/coding-style#Code_Formatting
used to make this more explicit)

It definitely took me a while to read through and make sure this didn't also
need to change, so I'd encourage you to restructure as

if ((response_code / 100) != 2) {
  if (response_code > 0)
    result_ = SERVER_ERROR;
  else
    result_ = NETWORK_ERROR;
  OnResponseCompleted();
  return;
}

....

[michaeln, 2014/10/15 19:20:21]

Thnx, that helped with readability quite a lot.

   }
 }
 
 void AppCacheUpdateJob::URLFetcher::OnReadCompleted(
     net::URLRequest* request, int bytes_read) {
   DCHECK(request_ == request);
   bool data_consumed = true;
   if (request->status().is_success() && bytes_read > 0) {
     job_->MadeProgress();
     data_consumed = ConsumeResponseData(bytes_read);
@@ skipping 1391 matching lines @@
 
   // Break the connection with the group so the group cannot call delete
   // on this object after we've posted a task to delete ourselves.
   group_->SetUpdateAppCacheStatus(AppCacheGroup::IDLE);
   group_ = NULL;
 
   base::MessageLoop::current()->DeleteSoon(FROM_HERE, this);
 }
 
 }  // namespace content