Catch CSP violations in InProcessBrowserTest

chrome/test/base/in_process_browser_test.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 "chrome/test/base/in_process_browser_test.h"
 
 #include "ash/ash_switches.h"
 #include "base/auto_reset.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 72 matching lines @@
 #endif
 
 #if defined(USE_ASH)
 #include "chrome/test/base/default_ash_event_generator_delegate.h"
 #endif
 
 namespace {
 
 // Passed as value of kTestType.
 const char kBrowserTestType[] = "browser";
 

[Tom Sepez, 2016/05/31 16:21:38]

nit: |static| redundant within namespace {}

[wychen, 2016/06/08 05:41:57]

Done.

+static bool seen_csp_error_messages_ = false;
+
+bool CSPLogHandler(int severity,

[Dan Beam, 2016/06/07 01:18:30]

this will conflict (potentially) with JS-based browser tests as they also
globally override log handling :(

[wychen, 2016/06/08 05:41:57]

I've rebased this CL on top of https://codereview.chromium.org/2034393004/,
which turns log message handler from a pointer to a stack.

+                   const char* file,
+                   int line,
+                   size_t message_start,
+                   const std::string& str) {
+  if (file == NULL || std::string("CONSOLE") != file)
+    return false;
+
+  // Catch CSP violation.
+  bool contains_error =
+      str.find("Content Security Policy") != std::string::npos;

[Paweł Hajdan Jr., 2016/05/30 08:43:47]

It seems fragile to me to rely on string parsing and a log handler to catch CSP
violations.

What happens if something else registers its own log handler in tests?

Is there rather e.g. some observer to register for notifications?

[wychen, 2016/05/31 08:11:27]

It's indeed not very robust in that case. Even though all CSP violation messages
contain that string now, there's no guarantee it will remain this case.

mkwst@, would it make sense to add notification in functions like
CSPDirectiveList::reportViolation*() so that violations are guaranteed to be
caught in browsertest level?

[Mike West, 2016/05/31 09:20:00]

What would you like to see added? Sorry, I'm not entirely following the thrust
of this CL. :)

[Tom Sepez, 2016/05/31 16:21:38]

I'm OK so long as we don't silently pass failures as the result of a change. As
it is now, we'll silently pass them.  Which is why I think the
SetExpectCSPErrorMessges() change is required to induce failures when the string
changes and the expected substrings are no longer found.

[wychen, 2016/05/31 18:17:45]

This sounds like a good median between current fragile implementation and the
heavyweight notification. This could detect changed messages, but if a test
registers its own log handler while violating CSP, it still silently passes. To
solve this hijacking issue, we can turn the log message handler to a stack, so
that derived test classes can still add/remove its own handler, while the old
ones still get the messages. I'm just thinking out loud. Would this worth the
trouble? All the callers of GetLogMessageHandler() seems to be compatible with
the stack usage.

Mike, what I was trying to do is to catch all unexpected CSP violations in
browsertest, especially for WebUI-related tests. Currently all the CSP
violations that we care in WebUI generate console messages containing "Content
Security Policy", hence this quick-n-dirty detection method.

+  if (severity == logging::LOG_INFO && contains_error) {
+    seen_csp_error_messages_ = true;
+  }
+
+  return false;
+}
+
 }  // namespace
 
 // Library used for testing accessibility.
 const base::FilePath::CharType kAXSTesting[] =
     FILE_PATH_LITERAL("third_party/accessibility-audit/axs_testing.js");
 // JavaScript snippet to configure and run the accessibility audit.
 const char kAccessibilityTestString[] =
     "var config = new axs.AuditConfiguration();"
     "/* Disable warning about rules that cannot be checked. */"
     "config.showUnsupportedRulesWarning = false;"
@@ skipping 24 matching lines @@
     "    error = axs.Audit.createReport(result);"
     "    break;"
     "  }"
     "}"
     "domAutomationController.send(error);";
 
 InProcessBrowserTest::InProcessBrowserTest()
     : browser_(NULL),
       exit_when_last_browser_closes_(true),
       open_about_blank_on_browser_launch_(true),
+      ignore_csp_messages_(false),
       run_accessibility_checks_for_test_case_(false)
 #if defined(OS_MACOSX)
       , autorelease_pool_(NULL)
 #endif  // OS_MACOSX
     {
 #if defined(OS_MACOSX)
   // TODO(phajdan.jr): Make browser_tests self-contained on Mac, remove this.
   // Before we run the browser, we have to hack the path to the exe to match
   // what it would be if Chrome was running, because it is used to fork renderer
   // processes, on Linux at least (failure to do so will cause a browser_test to
@@ skipping 15 matching lines @@
   CHECK(PathService::Override(chrome::DIR_TEST_DATA,
                               src_dir.AppendASCII("chrome/test/data")));
 
 #if defined(OS_MACOSX)
   bundle_swizzler_.reset(new ScopedBundleSwizzlerMac);
 #endif
 
 #if defined(USE_ASH)
   DefaultAshEventGeneratorDelegate::GetInstance();
 #endif
+
+  logging::SetLogMessageHandler(&CSPLogHandler);
+  seen_csp_error_messages_ = false;
 }
 
 InProcessBrowserTest::~InProcessBrowserTest() {
+  logging::SetLogMessageHandler(nullptr);
 }
 
 void InProcessBrowserTest::SetUp() {
   // Browser tests will create their own g_browser_process later.
   DCHECK(!g_browser_process);
 
   // Clear the FeatureList instance from base/test/test_suite.cc. Since this is
   // a browser test, a FeatureList will be registered as part of normal browser
   // start up in ChromeBrowserMainParts::SetupMetricsAndFieldTrials().
   base::FeatureList::ClearInstanceForTesting();
@@ skipping 151 matching lines @@
     }
   }
   return test_launcher_utils::OverrideUserDataDir(user_data_dir);
 }
 
 void InProcessBrowserTest::TearDown() {
   DCHECK(!g_browser_process);
 #if defined(OS_WIN)
   com_initializer_.reset();
 #endif
+  if (!ignore_csp_messages_) {
+    EXPECT_FALSE(seen_csp_error_messages_);
+  }
   BrowserTestBase::TearDown();
 }
 
 void InProcessBrowserTest::CloseBrowserSynchronously(Browser* browser) {
   content::WindowedNotificationObserver observer(
       chrome::NOTIFICATION_BROWSER_CLOSED,
       content::Source<Browser>(browser));
   CloseBrowserAsynchronously(browser);
   observer.Wait();
 }
@@ skipping 40 matching lines @@
     int index,
     const GURL& url,
     ui::PageTransition transition) {
   AddTabAtIndexToBrowser(browser(), index, url, transition, true);
 }
 
 bool InProcessBrowserTest::SetUpUserDataDirectory() {
   return true;
 }
 
+void InProcessBrowserTest::SetIgnoreCSPErrorMessages(bool ignore) {
+  ignore_csp_messages_ = ignore;
+}
+
 #if !defined(OS_MACOSX)
 void InProcessBrowserTest::OpenDevToolsWindow(
     content::WebContents* web_contents) {
   ASSERT_FALSE(content::DevToolsAgentHost::HasFor(web_contents));
   DevToolsWindow::OpenDevToolsWindow(web_contents);
   ASSERT_TRUE(content::DevToolsAgentHost::HasFor(web_contents));
 }
 
 Browser* InProcessBrowserTest::OpenURLOffTheRecord(Profile* profile,
                                                    const GURL& url) {
@@ skipping 175 matching lines @@
   // On the Mac, this eventually reaches
   // -[BrowserWindowController windowWillClose:], which will post a deferred
   // -autorelease on itself to ultimately destroy the Browser object. The line
   // below is necessary to pump these pending messages to ensure all Browsers
   // get deleted.
   content::RunAllPendingInMessageLoop();
   delete autorelease_pool_;
   autorelease_pool_ = NULL;
 #endif
 }