Properly decode IDN in interstitials

Properly decode IDN in interstitials

Decode IDN hostnames in SecurityInterstitialPage before displaying
them. Add browser tests for SSLBlockingPage and
CaptivePortalBlockingPage to test this behavior. Other blocking pages
can subclass the new SecurityInterstitialIDNTest class to do similar
tests.

BUG=450883
Committed: https://crrev.com/489874cb190ad4c4fafef212d99082645582bd55
Cr-Commit-Position: refs/heads/master@{#322128}

[estark, 2015-03-24 19:10:13]

estark@chromium.org changed reviewers:
+ meacer@chromium.org

[estark, 2015-03-24 19:10:14]

meacer@, could you please take a look? This is the patch I stole from
https://codereview.chromium.org/863133003/ plus some tests. Thanks!

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/interstitial...
File chrome/browser/interstitials/security_interstitial_page_browsertest.cc
(right):

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/interstitial...
chrome/browser/interstitials/security_interstitial_page_browsertest.cc:59:
IN_PROC_BROWSER_TEST_F(SecurityInterstitialPageIDNTest,
This is really a unit test for |GetFormattedHostName|, but it is written as a
browser test to get a WebContents. Not sure if that's correct or not.

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

Great timing, you sent the CL just as I commented on the bug :)

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/interstitial...
File chrome/browser/interstitials/security_interstitial_page_browsertest.cc
(right):

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/interstitial...
chrome/browser/interstitials/security_interstitial_page_browsertest.cc:59:
IN_PROC_BROWSER_TEST_F(SecurityInterstitialPageIDNTest,
On 2015/03/24 19:10:13, emily wrote:
> This is really a unit test for |GetFormattedHostName|, but it is written as a
> browser test to get a WebContents. Not sure if that's correct or not.

I agree, it looks like this is testing |GetFormattedHostName| instead. How about
not writing a browsertest for |SecurityInterstitialPage| but writing a base
class that all implementors of |SecurityInterstitialPage| can derive in their
tests? (see my comments in ssl_browser_tests.cc)

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:1936: class SSLUITestIDN : public
SSLUITest {
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.

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:1967: base::Time::NowFromSystemTime(),
base::Bind(&DoNothing));
You can use base::Callback<void(bool)>() instead of base::Bind(&DoNothing).

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

Thanks Mustafa.

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/interstitial...
File chrome/browser/interstitials/security_interstitial_page_browsertest.cc
(right):

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/interstitial...
chrome/browser/interstitials/security_interstitial_page_browsertest.cc:59:
IN_PROC_BROWSER_TEST_F(SecurityInterstitialPageIDNTest,
On 2015/03/24 20:22:24, Mustafa Emre Acer wrote:
> On 2015/03/24 19:10:13, emily wrote:
> > This is really a unit test for |GetFormattedHostName|, but it is written as
a
> > browser test to get a WebContents. Not sure if that's correct or not.
> 
> I agree, it looks like this is testing |GetFormattedHostName| instead. How
about
> not writing a browsertest for |SecurityInterstitialPage| but writing a base
> class that all implementors of |SecurityInterstitialPage| can derive in their
> tests? (see my comments in ssl_browser_tests.cc)

Done.

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:1936: class SSLUITestIDN : public
SSLUITest {
On 2015/03/24 20:22:25, Mustafa Emre Acer wrote:
> 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.

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?

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:1967: base::Time::NowFromSystemTime(),
base::Bind(&DoNothing));
On 2015/03/24 20:22:24, Mustafa Emre Acer wrote:
> You can use base::Callback<void(bool)>() instead of base::Bind(&DoNothing).

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:17:00]

Looking good. Can you check whether SSLBlockingPage is trying to accept the
cert?

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:1936: class SSLUITestIDN : public
SSLUITest {
On 2015/03/24 22:35:47, emily wrote:
> On 2015/03/24 20:22:25, Mustafa Emre Acer wrote:
> > 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.
> 
> 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?

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!)

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:1967: base::Time::NowFromSystemTime(),
base::Bind(&DoNothing));
On 2015/03/24 22:35:47, emily wrote:
> On 2015/03/24 20:22:24, Mustafa Emre Acer wrote:
> > You can use base::Callback<void(bool)>() instead of base::Bind(&DoNothing).
> 
> Unfortunately if I do that I run into a DCHECK here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...

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|?

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page_test_utils.h
(right):

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.h:34: void
TestIDN();
You can also return an AssertionResult here. E.g. |AssertionResult
VerifyIDNDecoded()| and then do EXPECT_TRUE(VerifyIDNDecoded()).

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.h:34: void
TestIDN();
const method?

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.h:39: const
GURL& request_url) = 0;
const method?

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:1951: return blocking_page;
nit: directly |return new SSLBlockingPage...| instead?

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

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:1936: class SSLUITestIDN : public
SSLUITest {
On 2015/03/24 23:16:59, Mustafa Emre Acer wrote:
> On 2015/03/24 22:35:47, emily wrote:
> > On 2015/03/24 20:22:25, Mustafa Emre Acer wrote:
> > > 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.
> > 
> > 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?
> 
> 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!)

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. :)

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:1967: base::Time::NowFromSystemTime(),
base::Bind(&DoNothing));
On 2015/03/24 23:16:59, Mustafa Emre Acer wrote:
> On 2015/03/24 22:35:47, emily wrote:
> > On 2015/03/24 20:22:24, Mustafa Emre Acer wrote:
> > > You can use base::Callback<void(bool)>() instead of
base::Bind(&DoNothing).
> > 
> > Unfortunately if I do that I run into a DCHECK here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...
> 
> 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|?

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?

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page_test_utils.h
(right):

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.h:34: void
TestIDN();
On 2015/03/24 23:16:59, Mustafa Emre Acer wrote:
> You can also return an AssertionResult here. E.g. |AssertionResult
> VerifyIDNDecoded()| and then do EXPECT_TRUE(VerifyIDNDecoded()).

Done.

(Question: is the purpose of returning an AssertionResult to make the test
output more readable?)

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.h:34: void
TestIDN();
On 2015/03/24 23:16:59, Mustafa Emre Acer wrote:
> const method?

Done.

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.h:39: const
GURL& request_url) = 0;
On 2015/03/24 23:16:59, Mustafa Emre Acer wrote:
> const method?

Done.

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_browser_tests.cc:1951: return blocking_page;
On 2015/03/24 23:17:00, Mustafa Emre Acer wrote:
> nit: directly |return new SSLBlockingPage...| instead?

Done.

[meacer, 2015-03-25 01:08:40]

LGTM

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
File chrome/browser/ssl/ssl_browser_tests.cc (right):

https://codereview.chromium.org/1000333003/diff/1/chrome/browser/ssl/ssl_brow...
chrome/browser/ssl/ssl_browser_tests.cc:1967: base::Time::NowFromSystemTime(),
base::Bind(&DoNothing));
On 2015/03/25 00:45:25, emily wrote:
> On 2015/03/24 23:16:59, Mustafa Emre Acer wrote:
> > On 2015/03/24 22:35:47, emily wrote:
> > > On 2015/03/24 20:22:24, Mustafa Emre Acer wrote:
> > > > You can use base::Callback<void(bool)>() instead of
> base::Bind(&DoNothing).
> > > 
> > > Unfortunately if I do that I run into a DCHECK here:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ssl...
> > 
> > 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|?
> 
> 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?

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.

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page_test_utils.h
(right):

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.h:34: void
TestIDN();
On 2015/03/25 00:45:25, emily wrote:
> On 2015/03/24 23:16:59, Mustafa Emre Acer wrote:
> > You can also return an AssertionResult here. E.g. |AssertionResult
> > VerifyIDNDecoded()| and then do EXPECT_TRUE(VerifyIDNDecoded()).
> 
> Done.
> 
> (Question: is the purpose of returning an AssertionResult to make the test
> output more readable?)

In this case, yes I think it's slight more readable. General usage is when you
expect to do different types of assertions based on a value passed to the
function. So you could have something like this:

AssertionResult TestEven(int i) {
  return i % 2 == 0 ? AssertionSuccess() : AssertionFailure();
}

EXPECT_FALSE(TestEven(1));
EXPECT_TRUE(TestEven(2));

https://codereview.chromium.org/1000333003/diff/60001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/1000333003/diff/60001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:44: CMD_TEXT_NOT_FOUND
= 12
How about making these negative values? That way the page could just ignore them
by checking for values >=0. It's also possible that we'll add new actions which
will shift these values in the future (I don't think that's going to break
anything, but better to be safe).

https://codereview.chromium.org/1000333003/diff/60001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page_test_utils.cc
(right):

https://codereview.chromium.org/1000333003/diff/60001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.cc:70: return
testing::AssertionFailure() << "Interstitial not displaying text";
Not: You can remove |else| block and just return directly as a fallthrough.

[estark, 2015-03-25 04:23:49]

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page_test_utils.h
(right):

https://codereview.chromium.org/1000333003/diff/40001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.h:34: void
TestIDN();
On 2015/03/25 01:08:39, Mustafa Emre Acer wrote:
> On 2015/03/25 00:45:25, emily wrote:
> > On 2015/03/24 23:16:59, Mustafa Emre Acer wrote:
> > > You can also return an AssertionResult here. E.g. |AssertionResult
> > > VerifyIDNDecoded()| and then do EXPECT_TRUE(VerifyIDNDecoded()).
> > 
> > Done.
> > 
> > (Question: is the purpose of returning an AssertionResult to make the test
> > output more readable?)
> 
> In this case, yes I think it's slight more readable. General usage is when you
> expect to do different types of assertions based on a value passed to the
> function. So you could have something like this:
> 
> AssertionResult TestEven(int i) {
>   return i % 2 == 0 ? AssertionSuccess() : AssertionFailure();
> }
> 
> EXPECT_FALSE(TestEven(1));
> EXPECT_TRUE(TestEven(2));

Makes sense, thanks!

https://codereview.chromium.org/1000333003/diff/60001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page.h (right):

https://codereview.chromium.org/1000333003/diff/60001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page.h:44: CMD_TEXT_NOT_FOUND
= 12
On 2015/03/25 01:08:40, Mustafa Emre Acer wrote:
> How about making these negative values? That way the page could just ignore
them
> by checking for values >=0. It's also possible that we'll add new actions
which
> will shift these values in the future (I don't think that's going to break
> anything, but better to be safe).

Done.

https://codereview.chromium.org/1000333003/diff/60001/chrome/browser/intersti...
File chrome/browser/interstitials/security_interstitial_page_test_utils.cc
(right):

https://codereview.chromium.org/1000333003/diff/60001/chrome/browser/intersti...
chrome/browser/interstitials/security_interstitial_page_test_utils.cc:70: return
testing::AssertionFailure() << "Interstitial not displaying text";
On 2015/03/25 01:08:40, Mustafa Emre Acer wrote:
> Not: You can remove |else| block and just return directly as a fallthrough.

Done.

[estark, 2015-03-25 04:24:11]

The CQ bit was checked by estark@chromium.org

[estark, 2015-03-25 04:24:12]

The patchset sent to the CQ was uploaded after l-g-t-m from meacer@chromium.org
Link to the patchset: https://codereview.chromium.org/1000333003/#ps80001
(title: "move test commands to negative values, and control flow nit")

[estark, 2015-03-25 04:24:34]

The CQ bit was unchecked by estark@chromium.org

[estark, 2015-03-25 04:26:19]

The CQ bit was checked by estark@chromium.org

[commit-bot: I haz the power, 2015-03-25 04:27:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1000333003/80001

[commit-bot: I haz the power, 2015-03-25 07:17:49]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-03-25 07:18:40]

Patchset 5 (id:??) landed as
https://crrev.com/489874cb190ad4c4fafef212d99082645582bd55
Cr-Commit-Position: refs/heads/master@{#322128}