Implement CORS preflights for Expect-CT reports

chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc

Index: chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
diff --git a/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc b/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
index 755444ab3a84b658e51f69774cc3481fb5b9e445..d2f07a7fa0b216ba93eec192d7a1867f9c363d61 100644
--- a/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
+++ b/chrome/browser/ssl/chrome_expect_ct_reporter_unittest.cc
@@ -9,7 +9,6 @@
 #include "base/base64.h"
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
-#include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/test/histogram_tester.h"
 #include "base/test/scoped_feature_list.h"
@@ -19,6 +18,8 @@
 #include "net/cert/ct_serialization.h"
 #include "net/cert/signed_certificate_timestamp_and_status.h"
 #include "net/test/cert_test_util.h"
+#include "net/test/embedded_test_server/embedded_test_server.h"
+#include "net/test/embedded_test_server/http_response.h"
 #include "net/test/test_data_directory.h"
 #include "net/test/url_request/url_request_failed_job.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
@@ -50,6 +51,10 @@ class TestCertificateReportSender : public net::ReportSender {
     latest_report_uri_ = report_uri;
     serialized_report.CopyToString(&latest_serialized_report_);
     content_type.CopyToString(&latest_content_type_);
+    if (!report_callback_.is_null()) {
+      EXPECT_EQ(expected_report_uri_, latest_report_uri_);
+      report_callback_.Run();
+    }
   }
 
   const GURL& latest_report_uri() const { return latest_report_uri_; }
@@ -62,10 +67,26 @@ class TestCertificateReportSender : public net::ReportSender {
     return latest_serialized_report_;
   }
 
+  // Can be called to wait for a single report, which is expected to be sent to
+  // |report_uri|. |callback| will be run immediately (before return) if a
+  // report has already been sent in the past, and otherwise it will be called
+  // on the next Send() call.
+  void WaitForReport(const GURL& report_uri, const base::Closure& callback) {

[meacer, 2017/07/05 23:48:59]

This isn't actually waiting for a report, is it?

Perhaps pass the runloop as a const ref and run it directly from here. 

But even then I'm confused about how this method would work.

The test cases below create a runloop, call WaitForReport and then run the run
loop. So
- If the Send() happens before WaitForReport() is called, the run loop would
never run because it's quit closure will run before the run loop runs. That's
Working as intended.
- But if the Send() happens after WaitForReport(), the test will timeout because
the runloop will start, and the quit closure will never run.

Seems like what you want is to have the runloop as a member of this class, then
run its quit closure when a Send() happens. Then WaitForReport() can simply
start the runloop if latest_report_uri_.is_empty().

[estark, 2017/07/06 06:39:43]

I changed it to create and run the run loop inside the function, but other than
that I don't understand: why would the test timeout if Send() happens after
WaitForReport()? AFAICT this is basically following the canonical usage pattern
of creating a run loop and running it, with an asynchronous task that calls the
QuitClosure():
https://cs.chromium.org/chromium/src/base/run_loop.h?sq=package:chromium&dr=S...

(Anecdotally, I saw in my local runs that WaitForReport() happens before Send(),
and the test passes, doesn't time out.)

[meacer, 2017/07/06 22:03:45]

Sorry, you are right. I missed the fact that you are setting report_callback_ =
callback here and then calling report_callback_ in Send(). Without that there
wouldn't be any connection between the two which is why I said it would time
out. 

+    report_callback_ = callback;
+    expected_report_uri_ = report_uri;
+    if (!latest_report_uri_.is_empty()) {
+      EXPECT_EQ(expected_report_uri_, latest_report_uri_);
+      callback.Run();
+      return;

[meacer, 2017/07/05 23:48:59]

nit: not necessary

[estark, 2017/07/06 06:39:43]

Done.

+    }
+  }
+
  private:
   GURL latest_report_uri_;
   std::string latest_content_type_;
   std::string latest_serialized_report_;
+  base::Closure report_callback_;
+  GURL expected_report_uri_;
 };
 
 // Constructs a net::SignedCertificateTimestampAndStatus with the given
@@ -299,14 +320,100 @@ class ChromeExpectCTReporterWaitTest : public ::testing::Test {
   DISALLOW_COPY_AND_ASSIGN(ChromeExpectCTReporterWaitTest);
 };
 
+// A test fixture that responds properly to CORS preflights so that reports can
+// be successfully sent to test_server().
+class ChromeExpectCTReporterTest : public ::testing::Test {
+ public:
+  ChromeExpectCTReporterTest()
+      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP) {}
+  ~ChromeExpectCTReporterTest() override {}
+
+  void SetUp() override {
+    report_server_.RegisterRequestHandler(
+        base::Bind(&ChromeExpectCTReporterTest::HandleReportPreflight,
+                   base::Unretained(this)));
+    ASSERT_TRUE(report_server_.Start());
+  }
+
+  std::unique_ptr<net::test_server::HttpResponse> HandleReportPreflight(
+      const net::test_server::HttpRequest& request) {
+    num_requests_++;
+    if (!requests_callback_.is_null()) {
+      requests_callback_.Run();
+    }
+    std::unique_ptr<net::test_server::BasicHttpResponse> http_response(
+        new net::test_server::BasicHttpResponse());
+    http_response->set_code(net::HTTP_OK);
+    for (const auto& cors_header : cors_headers_) {
+      http_response->AddCustomHeader(cors_header.first, cors_header.second);
+    }
+    return http_response;
+  }
+
+  void WaitForReportPreflight(const base::Closure& callback) {
+    if (num_requests_ >= 1) {
+      callback.Run();
+      return;
+    }
+    requests_callback_ = callback;
+  }
+
+ protected:
+  const net::EmbeddedTestServer& test_server() { return report_server_; }
+
+  // Tests that reports are not sent when the CORS preflight request returns the
+  // header field |preflight_header_name| with value given by
+  // |preflight_header_bad_value|, and that reports are successfully sent when
+  // it has value given by |preflight_header_good_value|.
+  void TestForReportPreflightFailure(
+      ChromeExpectCTReporter* reporter,
+      TestCertificateReportSender* sender,
+      const net::HostPortPair host_port,

[meacer, 2017/07/05 23:48:59]

Pass by ref

[estark, 2017/07/06 06:39:43]

Done.

+      const net::SSLInfo& ssl_info,
+      const std::string& preflight_header_name,
+      const std::string& preflight_header_bad_value,
+      const std::string& preflight_header_good_value) {
+    cors_headers_[preflight_header_name] = preflight_header_bad_value;
+    const GURL fail_report_uri = test_server().GetURL("/report1");
+    reporter->OnExpectCTFailed(
+        host_port, fail_report_uri, base::Time(), ssl_info.cert.get(),
+        ssl_info.unverified_cert.get(), ssl_info.signed_certificate_timestamps);
+    base::RunLoop preflight_run_loop;
+    WaitForReportPreflight(preflight_run_loop.QuitClosure());
+    preflight_run_loop.Run();
+    EXPECT_TRUE(sender->latest_report_uri().is_empty());
+    EXPECT_TRUE(sender->latest_serialized_report().empty());
+
+    // Set the proper header value and send a dummy report. The test will fail
+    // if the previous OnExpectCTFailed() call unexpectedly resulted in a
+    // report, as WaitForReport() would see the previous report to /report1
+    // instead of the expected report to /report2.
+    const GURL successful_report_uri = test_server().GetURL("/report2");
+    cors_headers_[preflight_header_name] = preflight_header_good_value;
+    reporter->OnExpectCTFailed(
+        host_port, successful_report_uri, base::Time(), ssl_info.cert.get(),
+        ssl_info.unverified_cert.get(), ssl_info.signed_certificate_timestamps);
+    base::RunLoop run_loop;
+    sender->WaitForReport(successful_report_uri, run_loop.QuitClosure());
+    run_loop.Run();
+    EXPECT_EQ(successful_report_uri, sender->latest_report_uri());
+  }
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  net::EmbeddedTestServer report_server_;
+  uint32_t num_requests_ = 0;
+  base::Closure requests_callback_;
+  std::map<std::string, std::string> cors_headers_{
+      {"Access-Control-Allow-Origin", "*"},
+      {"Access-Control-Allow-Methods", "GET,POST"},
+      {"Access-Control-Allow-Headers", "content-type,another-header"}};
+};
+
 }  // namespace
 
 // Test that no report is sent when the feature is not enabled.
-TEST(ChromeExpectCTReporterTest, FeatureDisabled) {
-  base::test::ScopedFeatureList scoped_feature_list;
-  scoped_feature_list.InitAndDisableFeature(features::kExpectCTReporting);
-
-  base::MessageLoop message_loop;
+TEST_F(ChromeExpectCTReporterTest, FeatureDisabled) {
   base::HistogramTester histograms;
   histograms.ExpectTotalCount(kSendHistogramName, 0);
 
@@ -324,20 +431,41 @@ TEST(ChromeExpectCTReporterTest, FeatureDisabled) {
       net::GetTestCertsDirectory(), "localhost_cert.pem");
 
   net::HostPortPair host_port("example.test", 443);
-  GURL report_uri("http://example-report.test");
 
-  reporter.OnExpectCTFailed(host_port, report_uri, base::Time(),
-                            ssl_info.cert.get(), ssl_info.unverified_cert.get(),
-                            ssl_info.signed_certificate_timestamps);
-  EXPECT_TRUE(sender->latest_report_uri().is_empty());
-  EXPECT_TRUE(sender->latest_serialized_report().empty());
+  {
+    const GURL report_uri = test_server().GetURL("/report1");
+    base::test::ScopedFeatureList scoped_feature_list;
+    scoped_feature_list.InitAndDisableFeature(features::kExpectCTReporting);
 
-  histograms.ExpectTotalCount(kSendHistogramName, 0);
+    reporter.OnExpectCTFailed(
+        host_port, report_uri, base::Time(), ssl_info.cert.get(),
+        ssl_info.unverified_cert.get(), ssl_info.signed_certificate_timestamps);
+    EXPECT_TRUE(sender->latest_report_uri().is_empty());
+    EXPECT_TRUE(sender->latest_serialized_report().empty());
+
+    histograms.ExpectTotalCount(kSendHistogramName, 0);
+  }
+
+  // Enable the feature and send a dummy report. The test will fail if the
+  // previous OnExpectCTFailed() call unexpectedly resulted in a report, as the
+  // WaitForReport() would see the previous report to /report1 instead of the
+  // expected report to /report2.
+  {
+    const GURL report_uri = test_server().GetURL("/report2");
+    base::test::ScopedFeatureList scoped_feature_list;
+    scoped_feature_list.InitAndEnableFeature(features::kExpectCTReporting);
+    reporter.OnExpectCTFailed(
+        host_port, report_uri, base::Time(), ssl_info.cert.get(),
+        ssl_info.unverified_cert.get(), ssl_info.signed_certificate_timestamps);
+    base::RunLoop run_loop;
+    sender->WaitForReport(report_uri, run_loop.QuitClosure());
+    run_loop.Run();
+    EXPECT_EQ(report_uri, sender->latest_report_uri());
+  }
 }
 
 // Test that no report is sent if the report URI is empty.
-TEST(ChromeExpectCTReporterTest, EmptyReportURI) {
-  base::MessageLoop message_loop;
+TEST_F(ChromeExpectCTReporterTest, EmptyReportURI) {
   base::HistogramTester histograms;
   histograms.ExpectTotalCount(kSendHistogramName, 0);
 
@@ -385,8 +513,7 @@ TEST_F(ChromeExpectCTReporterWaitTest, SendReportFailure) {
 }
 
 // Test that a sent report has the right format.
-TEST(ChromeExpectCTReporterTest, SendReport) {
-  base::MessageLoop message_loop;
+TEST_F(ChromeExpectCTReporterTest, SendReport) {
   base::HistogramTester histograms;
   histograms.ExpectTotalCount(kFailureHistogramName, 0);
   histograms.ExpectTotalCount(kSendHistogramName, 0);
@@ -454,26 +581,115 @@ TEST(ChromeExpectCTReporterTest, SendReport) {
                        net::ct::SCT_STATUS_OK,
                        &ssl_info.signed_certificate_timestamps);
 
-  net::HostPortPair host_port("example.test", 443);
-  GURL report_uri("http://example-report.test");
-
   const char kExpirationTimeStr[] = "2017-01-01T00:00:00.000Z";
   base::Time expiration;
   ASSERT_TRUE(
       base::Time::FromUTCExploded({2017, 1, 0, 1, 0, 0, 0, 0}, &expiration));
 
+  const GURL report_uri = test_server().GetURL("/report");
+
   // Check that the report is sent and contains the correct information.
-  reporter.OnExpectCTFailed(host_port, report_uri, expiration,
-                            ssl_info.cert.get(), ssl_info.unverified_cert.get(),
+  reporter.OnExpectCTFailed(net::HostPortPair::FromURL(report_uri), report_uri,
+                            expiration, ssl_info.cert.get(),
+                            ssl_info.unverified_cert.get(),
                             ssl_info.signed_certificate_timestamps);
+
+  // A CORS preflight request should be sent before the actual report.
+  base::RunLoop preflight_run_loop;
+  WaitForReportPreflight(preflight_run_loop.QuitClosure());
+  preflight_run_loop.Run();
+
+  base::RunLoop report_run_loop;
+  sender->WaitForReport(report_uri, report_run_loop.QuitClosure());
+  report_run_loop.Run();
+
   EXPECT_EQ(report_uri, sender->latest_report_uri());
   EXPECT_FALSE(sender->latest_serialized_report().empty());
-  EXPECT_EQ("application/json; charset=utf-8", sender->latest_content_type());
-  ASSERT_NO_FATAL_FAILURE(
-      CheckExpectCTReport(sender->latest_serialized_report(), host_port,
-                          kExpirationTimeStr, ssl_info));
+  EXPECT_EQ("application/expect-ct-report+json; charset=utf-8",
+            sender->latest_content_type());
+  ASSERT_NO_FATAL_FAILURE(CheckExpectCTReport(
+      sender->latest_serialized_report(),
+      net::HostPortPair::FromURL(report_uri), kExpirationTimeStr, ssl_info));
 
   histograms.ExpectTotalCount(kFailureHistogramName, 0);
   histograms.ExpectTotalCount(kSendHistogramName, 1);
   histograms.ExpectBucketCount(kSendHistogramName, true, 1);
 }
+
+// Test that no report is sent when the CORS preflight returns an invalid
+// Access-Control-Allow-Origin.
+TEST_F(ChromeExpectCTReporterTest, BadCORSPreflightResponseOrigin) {
+  TestCertificateReportSender* sender = new TestCertificateReportSender();
+  net::TestURLRequestContext context;
+  ChromeExpectCTReporter reporter(&context);
+  reporter.report_sender_.reset(sender);
+  EXPECT_TRUE(sender->latest_report_uri().is_empty());
+  EXPECT_TRUE(sender->latest_serialized_report().empty());
+
+  net::SSLInfo ssl_info;
+  ssl_info.cert =
+      net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
+  ssl_info.unverified_cert = net::ImportCertFromFile(
+      net::GetTestCertsDirectory(), "localhost_cert.pem");
+
+  net::HostPortPair host_port("example.test", 443);

[meacer, 2017/07/05 23:48:59]

nit: const (or perhaps inline these in lines 641, 667 and 693)

[estark, 2017/07/06 06:39:43]

Done.

+
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kExpectCTReporting);
+  EXPECT_TRUE(sender->latest_serialized_report().empty());
+  ASSERT_NO_FATAL_FAILURE(TestForReportPreflightFailure(
+      &reporter, sender, host_port, ssl_info, "Access-Control-Allow-Origin",
+      "https://another-origin.test", "null"));
+}
+
+// Test that no report is sent when the CORS preflight returns an invalid
+// Access-Control-Allow-Methods.
+TEST_F(ChromeExpectCTReporterTest, BadCORSPreflightResponseMethods) {
+  TestCertificateReportSender* sender = new TestCertificateReportSender();
+  net::TestURLRequestContext context;
+  ChromeExpectCTReporter reporter(&context);
+  reporter.report_sender_.reset(sender);
+  EXPECT_TRUE(sender->latest_report_uri().is_empty());
+  EXPECT_TRUE(sender->latest_serialized_report().empty());
+
+  net::SSLInfo ssl_info;
+  ssl_info.cert =
+      net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
+  ssl_info.unverified_cert = net::ImportCertFromFile(
+      net::GetTestCertsDirectory(), "localhost_cert.pem");
+
+  net::HostPortPair host_port("example.test", 443);
+
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kExpectCTReporting);
+  EXPECT_TRUE(sender->latest_serialized_report().empty());
+  ASSERT_NO_FATAL_FAILURE(TestForReportPreflightFailure(
+      &reporter, sender, host_port, ssl_info, "Access-Control-Allow-Methods",
+      "GET,HEAD,POSSSST", "POST"));
+}
+
+// Test that no report is sent when the CORS preflight returns an invalid
+// Access-Control-Allow-Headers.
+TEST_F(ChromeExpectCTReporterTest, BadCORSPreflightResponseHeaders) {
+  TestCertificateReportSender* sender = new TestCertificateReportSender();
+  net::TestURLRequestContext context;
+  ChromeExpectCTReporter reporter(&context);
+  reporter.report_sender_.reset(sender);
+  EXPECT_TRUE(sender->latest_report_uri().is_empty());
+  EXPECT_TRUE(sender->latest_serialized_report().empty());
+
+  net::SSLInfo ssl_info;
+  ssl_info.cert =
+      net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
+  ssl_info.unverified_cert = net::ImportCertFromFile(
+      net::GetTestCertsDirectory(), "localhost_cert.pem");
+
+  net::HostPortPair host_port("example.test", 443);
+
+  base::test::ScopedFeatureList scoped_feature_list;
+  scoped_feature_list.InitAndEnableFeature(features::kExpectCTReporting);
+  EXPECT_TRUE(sender->latest_serialized_report().empty());
+  ASSERT_NO_FATAL_FAILURE(TestForReportPreflightFailure(
+      &reporter, sender, host_port, ssl_info, "Access-Control-Allow-Headers",
+      "Not-Content-Type", "Content-Type"));
+}