Properly decode IDN in interstitials

chrome/browser/ssl/ssl_browser_tests.cc

Index: chrome/browser/ssl/ssl_browser_tests.cc
diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc
index 581ebe48479e46f40bc3af9fd44f194f7e745bbc..0c498d3d3e3ed48042ff47eda332e9654478b2c0 100644
--- a/chrome/browser/ssl/ssl_browser_tests.cc
+++ b/chrome/browser/ssl/ssl_browser_tests.cc
@@ -4,6 +4,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback.h"
 #include "base/command_line.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/string_util.h"
@@ -12,6 +13,7 @@
 #include "base/time/time.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/interstitials/security_interstitial_page_test_utils.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ssl/ssl_blocking_page.h"
 #include "chrome/browser/ui/browser.h"
@@ -44,6 +46,8 @@
 #include "net/base/net_errors.h"
 #include "net/base/test_data_directory.h"
 #include "net/cert/cert_status_flags.h"
+#include "net/cert/x509_certificate.h"
+#include "net/ssl/ssl_info.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 
 #if defined(USE_NSS)
@@ -53,6 +57,7 @@
 #endif  // defined(USE_NSS)
 
 using base::ASCIIToUTF16;
+using chrome_browser_interstitials::IsInterstitialDisplayingText;
 using content::InterstitialPage;
 using content::NavigationController;
 using content::NavigationEntry;
@@ -1928,6 +1933,48 @@ IN_PROC_BROWSER_TEST_F(SSLUITest, InterstitialNotAffectedByHideShow) {
   EXPECT_TRUE(tab->GetRenderWidgetHostView()->IsShowing());
 }
 
+class SSLUITestIDN : public SSLUITest {

[meacer, 2015/03/24 20:22:25]

I'm wondering if we should have a base class such as SecurityInterstitialIDNTest
and derive from it in here? That way you could push SetUpOnMainThread and other
common code into the base class, and just have a pure virtual
|CreateInterstitial| method which each interstitial IDN test (SafeBrowsing, SSL
and CaptivePortal) implements. It would then be trivial to write the IDN test
case for all three interstitials. The base class could live in, say,
security_interstitial_page_test_utils.h|cc.

[estark, 2015/03/24 22:35:47]

Done, except I haven't yet written the test for SafeBrowsingBlockingPage,
because it looks like there aren't yet browser tests for
SafeBrowsingBlockingPage. I can add safe_browsing_blocking_page_browsertest.cc
and put the IDN test there, but I'll have to move some code around to do that
(e.g. I'll need access to TestSafeBrowsingUIManager in both
safe_browsing_blocking_page_unittest.cc and _browsertest.cc), so I think that
might be better done in a follow-up CL... unless I'm missing a simpler way?

[meacer, 2015/03/24 23:16:59]

Sure, in that case feel free to do it in a follow up CL. I suspect moving a
couple of SB classes will end up touching many more files (I tried it once!)

[estark, 2015/03/25 00:45:24]

Heh, yeah, I started working on it and thought that it was going to triple the
size of this CL so it was probably worth doing separately. :)

+ public:
+  // InProcessBrowserTest:
+  void SetUpOnMainThread() override {
+    // Clear AcceptLanguages to force punycode decoding.
+    browser()->profile()->GetPrefs()->SetString(prefs::kAcceptLanguages,
+                                                std::string());
+  }
+};
+
+void DoNothing(bool arg) {
+}
+
+IN_PROC_BROWSER_TEST_F(SSLUITestIDN, IDNHostnameDecodedInInterstitial) {
+  const char kHostname[] = "xn--d1abbgf6aiiy.xn--p1ai";
+  const char kHostnameJSUnicode[] =
+      "\\u043f\\u0440\\u0435\\u0437\\u0438\\u0434\\u0435\\u043d\\u0442."
+      "\\u0440\\u0444";
+  std::string request_url_spec = base::StringPrintf("https://%s/", kHostname);
+  GURL request_url(request_url_spec);
+
+  content::WebContents* contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  DCHECK(contents);
+
+  // Create a SSLBlockingPage with dummy data.
+  net::SSLInfo ssl_info;
+  ssl_info.cert = new net::X509Certificate(
+      request_url.host(), "CA", base::Time::Max(), base::Time::Max());
+  SSLBlockingPage* blocking_page = new SSLBlockingPage(
+      contents, net::ERR_CERT_CONTAINS_ERRORS, ssl_info, request_url, 0,
+      base::Time::NowFromSystemTime(), base::Bind(&DoNothing));

[meacer, 2015/03/24 20:22:24]

You can use base::Callback<void(bool)>() instead of base::Bind(&DoNothing).

[estark, 2015/03/24 22:35:47]

Unfortunately if I do that I run into a DCHECK here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...

[meacer, 2015/03/24 23:16:59]

Oh that's odd. Normally when an |SSLBlockingPage| instance is destroyed, it
should cancel the cert via |NotifyDenyCertificate|, not allow it unless
|OnProceed| is explicitly called.

Could it be that a message is causing the interstitial to proceed, e.g. the
message from |IsInterstitialDisplayingText|?

[estark, 2015/03/25 00:45:25]

Oh you're right, it was the message from searching for the text! I resolved this
by introducing new commands for the text search (i.e.
IsInterstitialDisplayingText now sends 11 or 12 instead of 0 or 1, which are the
proceed/don't proceed commands). What do you think?

[meacer, 2015/03/25 01:08:39]

I think I understand it now. That "found" or "not found" command used to be a
boolean, but we recently converted it to an int:
https://codereview.chromium.org/981243003/diff/160001/chrome/browser/ssl/capt...
I'm actually surprised that didn't break any existing tests and is probably a
bug, so it definitely makes sense to use different values for the found/not
found message.

+
+  blocking_page->Show();
+  WaitForInterstitialAttach(contents);
+  EXPECT_TRUE(
+      WaitForRenderFrameReady(contents->GetInterstitialPage()->GetMainFrame()));
+
+  EXPECT_TRUE(IsInterstitialDisplayingText(contents->GetInterstitialPage(),
+                                           kHostnameJSUnicode));
+}
+
 // TODO(jcampan): more tests to do below.
 
 // Visit a page over https that contains a frame with a redirect.