Domain Reliability: More security review.

components/domain_reliability/monitor.cc

 // Copyright 2014 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 "components/domain_reliability/monitor.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 29 matching lines @@
   virtual scoped_refptr<base::SingleThreadTaskRunner>
   GetNetworkTaskRunner() const OVERRIDE {
     return main_task_runner_;
   }
 
  private:
   virtual ~TrivialURLRequestContextGetter() {}
 
   net::URLRequestContext* context_;
   const scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_;
 };

[Ryan Sleevi, 2014/04/26 02:37:39]

Why have this at all? If you need a URLRequestContextGetter, why not simply
*take* a URLRequestContextGetter?

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

See .h for explanation, although I'm willing to rework it if you'd prefer.

 
 }  // namespace
 
 namespace domain_reliability {
 
 DomainReliabilityMonitor::DomainReliabilityMonitor(
     net::URLRequestContext* url_request_context)
     : time_(new ActualTime()),
       url_request_context_getter_(scoped_refptr<net::URLRequestContextGetter>(
           new TrivialURLRequestContextGetter(
@@ skipping 20 matching lines @@
       scheduler_params_(
           DomainReliabilityScheduler::Params::GetFromFieldTrialsOrDefaults()),
       dispatcher_(time_.get()),
       uploader_(
           DomainReliabilityUploader::Create(url_request_context_getter_)) {
   DCHECK(OnIOThread());
 }
 
 DomainReliabilityMonitor::~DomainReliabilityMonitor() {
   DCHECK(OnIOThread());
-  STLDeleteContainerPairSecondPointers(
-      contexts_.begin(), contexts_.end());
+  STLDeleteContainerPairSecondPointers(contexts_.begin(), contexts_.end());
 }
 
 void DomainReliabilityMonitor::AddBakedInConfigs() {
   base::Time now = base::Time::Now();
   for (size_t i = 0; kBakedInJsonConfigs[i]; ++i) {
     std::string json(kBakedInJsonConfigs[i]);
     scoped_ptr<const DomainReliabilityConfig> config =
         DomainReliabilityConfig::FromJSON(json);
     if (config && config->IsExpired(now)) {
       LOG(WARNING) << "Baked-in Domain Reliability config for "
                    << config->domain << " is expired.";
       continue;
     }
     AddContext(config.Pass());
   }
 }
 
 void DomainReliabilityMonitor::OnBeforeRedirect(net::URLRequest* request) {
   DCHECK(OnIOThread());
-  RequestInfo request_info(*request);
   // Record the redirect itself in addition to the final request.
-  OnRequestLegComplete(request_info);
+  OnRequestLegComplete(RequestInfo(*request));
 }
 
 void DomainReliabilityMonitor::OnCompleted(net::URLRequest* request,
                                            bool started) {
   DCHECK(OnIOThread());
   if (!started)
     return;
   RequestInfo request_info(*request);
   if (request_info.DefinitelyReachedNetwork()) {
     OnRequestLegComplete(request_info);
     // A request was just using the network, so now is a good time to run any
     // pending and eligible uploads.
     dispatcher_.RunEligibleTasks();
   }
 }
 
 DomainReliabilityContext* DomainReliabilityMonitor::AddContextForTesting(
     scoped_ptr<const DomainReliabilityConfig> config) {
   return AddContext(config.Pass());
 }
 
 DomainReliabilityMonitor::RequestInfo::RequestInfo() {}
 
 DomainReliabilityMonitor::RequestInfo::RequestInfo(
     const net::URLRequest& request)
     : url(request.url()),

[Ryan Sleevi, 2014/04/26 02:37:39]

If request.url().is_valid() is false, you'll never log this, correct? So why not
filter these out in OnCompleted / OnBeforeRedirect?

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

I still need the URL to match against patterns in the context.

Also, can we have a request with an invalid URL? Wouldn't the URLRequest reject
it?

       status(request.status()),
       response_code(-1),
       socket_address(request.GetSocketAddress()),
       was_cached(request.was_cached()),
       load_flags(request.load_flags()) {
   request.GetLoadTimingInfo(&load_timing_info);
   // Can't get response code of a canceled request -- there's no transaction.
   if (status.status() != net::URLRequestStatus::CANCELED)
     response_code = request.GetResponseCode();
 }
 
 DomainReliabilityMonitor::RequestInfo::~RequestInfo() {}
 
 bool DomainReliabilityMonitor::RequestInfo::DefinitelyReachedNetwork() const {
   return status.status() != net::URLRequestStatus::CANCELED && !was_cached;
 }
 
 DomainReliabilityContext* DomainReliabilityMonitor::AddContext(
     scoped_ptr<const DomainReliabilityConfig> config) {
   DCHECK(config);
   DCHECK(config->IsValid());
 
   // Grab domain before we config.Pass().

[Ryan Sleevi, 2014/04/26 02:37:39]

// Grab a copy of the domain before transferring ownership of |config|

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Done.

   std::string domain = config->domain;
 
   DomainReliabilityContext* context = new DomainReliabilityContext(
       time_.get(),
       scheduler_params_,
       &dispatcher_,
       uploader_.get(),
       config.Pass());
 
   std::pair<ContextMap::iterator, bool> map_it =
@@ skipping 24 matching lines @@
   bool got_status = DomainReliabilityUtil::GetBeaconStatus(
       request.status.error(),
       request.response_code,
       &beacon_status);
   if (!got_status)
     return;
 
   DomainReliabilityBeacon beacon;
   beacon.status = beacon_status;
   beacon.chrome_error = request.status.error();
   beacon.server_ip = request.socket_address.host();

[Ryan Sleevi, 2014/04/26 02:37:39]

SECURITY/PRIVACY: Is there any risk that this may disclose the users' proxy IP?
If so, bug :)

[Deprecated (see juliatuttle), 2014/04/28 22:01:13]

Looking into it.

   beacon.http_response_code = request.response_code;
   beacon.start_time = request.load_timing_info.request_start;
   beacon.elapsed = time_->NowTicks() - beacon.start_time;
   context->AddBeacon(beacon, request.url);
 }
 
 }  // namespace domain_reliability