Remove Dangerous indicator after going back from interstitial

Remove Dangerous indicator after going back from interstitial

While showing a Safe Browsing interstitial, the top-level hostname for
the WebContents gets added to a "pending" whitelist set. Previously, we
were never removing URLs from that set, meaning that if you went Back
from an interstitial to a URL on the same hostname, the Dangerous
indicator persisted.

This CL fixes this bug by removing pending whitelist URLs when an
interstitial is dismissed with the Don't Proceed option.

BUG=659709
TEST=Visit http://testsafebrowsing.appspot.com, click first link. Observe that the icon in the omnibox is a red triangle. Click "Back to safety". Observe that the icon in the omnibox is an (i), not a red triangle.
TEST=Added browser test

Committed: https://crrev.com/1ca09ca22c714b67a7d2be4070686065f47fd9d7
Cr-Commit-Position: refs/heads/master@{#428932}

[estark, 2016-10-27 01:44:13]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[estark, 2016-10-27 01:44:48]

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

[estark, 2016-10-27 01:44:48]

nparker, another bug fix for you to take a look at, please.

[commit-bot: I haz the power, 2016-10-27 01:44:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 02:22:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 02:22:22]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[Nathan Parker, 2016-10-27 18:31:02]

lgtm

One remaining odd behavior is that if I proceed through a warning, then go back
to (say) the root of the origin, I'll still see a warning indicator even though
there's nothing bad on that page. I don't think that's terrible, though a bit
confusing. To fix that, it would probably require refactoring this logic to
instead check if the whitelist had be "used" to avoid a warning on the current
page.

https://codereview.chromium.org/2451623005/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2451623005/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ui_manager.cc:48: // specific WebContents, along
with pending entries that are still undecided.
Add a comment that the URLs here should come from GetWhitelistUrl()

https://codereview.chromium.org/2451623005/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ui_manager.cc:57: void RemovePending(const GURL
url) { pending_.erase(url.GetWithEmptyPath()); }
const GURL&
Though maybe the compiler would do that for you since it's inline -- not sure.

https://codereview.chromium.org/2451623005/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ui_manager.cc:85: return entry->GetURL();
How about returning .GetWithEmptyPath() so you don't have to do it repeatedly
above?  That'll also ensure you don't forget it some place above.

[estark, 2016-10-27 18:58:01]

Patchset #3 (id:40001) has been deleted

[estark, 2016-10-28 16:03:22]

Description was changed from

==========
Remove Dangerous indicator after going back from interstitial

While showing a Safe Browsing interstitial, the top-level hostname for
the WebContents gets added to a "pending" whitelist set. Previously, we
were never removing URLs from that set, meaning that if you went Back
from an interstitial to a URL on the same hostname, the Dangerous
indicator persisted.

This CL fixes this bug by removing pending whitelist URLs when an
interstitial is dismissed with the Don't Proceed option.

BUG=659709
==========

to

==========
Remove Dangerous indicator after going back from interstitial

While showing a Safe Browsing interstitial, the top-level hostname for
the WebContents gets added to a "pending" whitelist set. Previously, we
were never removing URLs from that set, meaning that if you went Back
from an interstitial to a URL on the same hostname, the Dangerous
indicator persisted.

This CL fixes this bug by removing pending whitelist URLs when an
interstitial is dismissed with the Don't Proceed option.

BUG=659709
TEST=Visit http://testsafebrowsing.appspot.com, click first link. Observe that
the icon in the omnibox is a red triangle. Click "Back to safety". Observe that
the icon in the omnibox is an (i), not a red triangle.
TEST=Added browser test
==========

[estark, 2016-10-28 16:06:23]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 16:06:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 16:43:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 16:43:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2016-10-28 17:14:45]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 17:15:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-28 17:21:08]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 17:21:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-28 17:39:29]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-28 17:39:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-10-28 17:57:47]

Some red bots alerted me that this was a little bit broken. Stupid tests,
getting in my way when I'm trying to land broken code! I wrote a probably
unnecessarily long essay below about the broken-ness, but the tl;dr is that I
had to take a bit of a different approach, so could you take another look
please?

The source of the broken-ness is that by the time OnBlockingPageDone() runs, a
"Back" navigation has already been committed. This means that the resource's
|web_contents_getter| might not work (in some cases, the getter gets the
WebContents for the RenderFrameHost of a request, but the RFH could be gone by
the time the "Back" navigation is committed.) Even if the |web_contents_getter|
does work, the interstitial navigation entry will be gone, meaning that we can't
use the visible navigation entry to decide what URL to remove from the pending
whitelist.

So, to fix this, I now have the SafeBrowsingBlockingPage passing the WebContents
and the main frame URL to OnBlockingPageDone(), so that OnBlockingPageDone() can
retrieve the whitelist for that WebContents (instead of trying to use the
now-possibly-invalid web_contents_getter) and remove the pending entry for the
main frame URL (instead of trying to use the visible NavigationEntry, which no
longer corresponds to the interstitial URL that we actually want to remove).

However, I think this is a little bit gross, so lmk if you see any better way. I
think what's gross to me is that we're passing around UnsafeResources for which
the web_contents_getter becomes invalid at some point, but there's no way to
tell at any particular point in the code whether the getter is valid or not. (I
have a vague feeling that |web_contents_getter| should be separated from
UnsafeResource and should just be passed as a separate argument on the thread
hops where it's needed. But, I think digging into that is a larger issue that we
shouldn't block this bug fix on.)

The other thing that's a little unfortunate is that some tests follow some
special snowflake code paths where they create SafeBrowsingBlockingPages on
their own (rather than through SafeBrowsingUIManager::ShowBlockingPage), or
simulate blocking pages being dismissed without actually creating them. This
means that tests can get in a state where they want to remove something from the
pending whitelist before the whitelist set has been created. This should never
happen in production code, so I didn't want to allow for it in
RemoveFromPendingWhitelistUrlSet(), so I made a CreateWhitelistForTesting() so
that tests can ensure that the whitelist exists before simulating the dismissal
of a blocking page. Blegh.

https://codereview.chromium.org/2451623005/diff/1/chrome/browser/safe_browsin...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2451623005/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ui_manager.cc:48: // specific WebContents, along
with pending entries that are still undecided.
On 2016/10/27 18:31:02, Nathan Parker wrote:
> Add a comment that the URLs here should come from GetWhitelistUrl()

Done.

https://codereview.chromium.org/2451623005/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ui_manager.cc:57: void RemovePending(const GURL
url) { pending_.erase(url.GetWithEmptyPath()); }
On 2016/10/27 18:31:02, Nathan Parker wrote:
> const GURL&
> Though maybe the compiler would do that for you since it's inline -- not sure.

Done.

https://codereview.chromium.org/2451623005/diff/1/chrome/browser/safe_browsin...
chrome/browser/safe_browsing/ui_manager.cc:85: return entry->GetURL();
On 2016/10/27 18:31:02, Nathan Parker wrote:
> How about returning .GetWithEmptyPath() so you don't have to do it repeatedly
> above?  That'll also ensure you don't forget it some place above.

Done.

[estark, 2016-10-28 18:01:33]

A couple forgotten comments/replies...

> One remaining odd behavior is that if I proceed through a warning, then go
back
> to (say) the root of the origin, I'll still see a warning indicator even
though
> there's nothing bad on that page. I don't think that's terrible, though a bit
> confusing. To fix that, it would probably require refactoring this logic to
> instead check if the whitelist had be "used" to avoid a warning on the current
> page.

I agree that this behavior is kind of confusing, but I also think it's the
reasonable thing to do from a security/safety perspective. Once you've clicked
through a SB warning, that whole origin is kind of tainted: for example the site
could have installed a ServiceWorker that intercepts all requests on that origin
and replaces them with malware. It's somewhat similar to how we handle active
mixed content, where once you run an http:// script on an https:// site, we mark
that origin as red/dangerous for the lifetime of the renderer process.

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:215: void AddToWhitelistUrlSet(const
GURL& whitelist_url,
I changed this from taking a |resource| to taking a |whitelist_url| to be
consistent with RemoveFromPendingWhitelistUrlSet below.

[commit-bot: I haz the power, 2016-10-28 18:24:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-28 18:24:06]

Dry run: This issue passed the CQ dry run.

[estark, 2016-10-31 23:07:03]

Friendly ping

[Nathan Parker, 2016-10-31 23:54:38]

lgtm

hrm.  I think your solution is better than any I can propose.  I wish this code
could get less complicated rather than more, but I don't have a great
suggestion. After some consideration I think I like your idea of requiring tests
to create the whitelist, so you can validate that it always gets created in the
production code.

Navigation is... hard.

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:121: void
ClearURL(const GURL& url) { badurls.erase(url.spec()); }
nit: ClearBadURL.  Same below.

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:362: content::WebContents*
web_contents) {
nit: How about putting this code in a "WhitelistUrlSet* CreateWhiteList()"
function so you can reuse it below?

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:468: // pages queued up for different
resources on the same page, and the
I thought we didn't queue up several warnings per page anymore, per felt@...

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.h (right):

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.h:145: // The blocking page for
|web_contents| on the UI thread has
Excellent comments, thank you.

[estark, 2016-11-01 02:41:17]

Thanks!

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc (right):

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc:121: void
ClearURL(const GURL& url) { badurls.erase(url.spec()); }
On 2016/10/31 23:54:38, Nathan Parker wrote:
> nit: ClearBadURL.  Same below.

Done.

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:362: content::WebContents*
web_contents) {
On 2016/10/31 23:54:38, Nathan Parker wrote:
> nit: How about putting this code in a "WhitelistUrlSet* CreateWhiteList()"
> function so you can reuse it below?

Done.

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:468: // pages queued up for different
resources on the same page, and the
On 2016/10/31 23:54:38, Nathan Parker wrote:
> I thought we didn't queue up several warnings per page anymore, per felt@...

Hrm. I was basing this on
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...

And indeed SafeBrowsingBlockingPage::OnProceed does appear to show a second
interstitial if one has been queued up while the first one is showing:
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...

However... I don't see that anything ever gets added to the UnsafeResourcesMap.
I'll look into that and delete it in a separate CL if I can.

[estark, 2016-11-01 02:42:43]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-01 02:42:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2451623005/#ps140001
(title: "nparker comments")

[commit-bot: I haz the power, 2016-11-01 02:43:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 02:52:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 02:52:56]

Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[estark, 2016-11-01 02:55:54]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-01 02:55:54]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2451623005/#ps160001
(title: "typo fix")

[commit-bot: I haz the power, 2016-11-01 02:56:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2016-11-01 03:00:08]

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
File chrome/browser/safe_browsing/ui_manager.cc (right):

https://codereview.chromium.org/2451623005/diff/120001/chrome/browser/safe_br...
chrome/browser/safe_browsing/ui_manager.cc:468: // pages queued up for different
resources on the same page, and the
On 2016/11/01 02:41:17, estark wrote:
> On 2016/10/31 23:54:38, Nathan Parker wrote:
> > I thought we didn't queue up several warnings per page anymore, per felt@...
> 
> Hrm. I was basing this on
>
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...
> 
> And indeed SafeBrowsingBlockingPage::OnProceed does appear to show a second
> interstitial if one has been queued up while the first one is showing:
>
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...
> 
> However... I don't see that anything ever gets added to the
UnsafeResourcesMap.
> I'll look into that and delete it in a separate CL if I can.

I lied, things do get queued to the UnsafeResourcesMap here:
https://cs.chromium.org/chromium/src/chrome/browser/safe_browsing/safe_browsi...

[commit-bot: I haz the power, 2016-11-01 03:06:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-01 03:06:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2016-11-01 03:26:17]

The CQ bit was checked by estark@chromium.org

[estark, 2016-11-01 03:26:17]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2451623005/#ps180001
(title: "another typo fix")

[commit-bot: I haz the power, 2016-11-01 03:26:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-01 04:04:35]

Description was changed from

==========
Remove Dangerous indicator after going back from interstitial

While showing a Safe Browsing interstitial, the top-level hostname for
the WebContents gets added to a "pending" whitelist set. Previously, we
were never removing URLs from that set, meaning that if you went Back
from an interstitial to a URL on the same hostname, the Dangerous
indicator persisted.

This CL fixes this bug by removing pending whitelist URLs when an
interstitial is dismissed with the Don't Proceed option.

BUG=659709
TEST=Visit http://testsafebrowsing.appspot.com, click first link. Observe that
the icon in the omnibox is a red triangle. Click "Back to safety". Observe that
the icon in the omnibox is an (i), not a red triangle.
TEST=Added browser test
==========

to

==========
Remove Dangerous indicator after going back from interstitial

While showing a Safe Browsing interstitial, the top-level hostname for
the WebContents gets added to a "pending" whitelist set. Previously, we
were never removing URLs from that set, meaning that if you went Back
from an interstitial to a URL on the same hostname, the Dangerous
indicator persisted.

This CL fixes this bug by removing pending whitelist URLs when an
interstitial is dismissed with the Don't Proceed option.

BUG=659709
TEST=Visit http://testsafebrowsing.appspot.com, click first link. Observe that
the icon in the omnibox is a red triangle. Click "Back to safety". Observe that
the icon in the omnibox is an (i), not a red triangle.
TEST=Added browser test
==========

[commit-bot: I haz the power, 2016-11-01 04:04:36]

Committed patchset #9 (id:180001)

[commit-bot: I haz the power, 2016-11-01 04:06:38]

Description was changed from

==========
Remove Dangerous indicator after going back from interstitial

While showing a Safe Browsing interstitial, the top-level hostname for
the WebContents gets added to a "pending" whitelist set. Previously, we
were never removing URLs from that set, meaning that if you went Back
from an interstitial to a URL on the same hostname, the Dangerous
indicator persisted.

This CL fixes this bug by removing pending whitelist URLs when an
interstitial is dismissed with the Don't Proceed option.

BUG=659709
TEST=Visit http://testsafebrowsing.appspot.com, click first link. Observe that
the icon in the omnibox is a red triangle. Click "Back to safety". Observe that
the icon in the omnibox is an (i), not a red triangle.
TEST=Added browser test
==========

to

==========
Remove Dangerous indicator after going back from interstitial

While showing a Safe Browsing interstitial, the top-level hostname for
the WebContents gets added to a "pending" whitelist set. Previously, we
were never removing URLs from that set, meaning that if you went Back
from an interstitial to a URL on the same hostname, the Dangerous
indicator persisted.

This CL fixes this bug by removing pending whitelist URLs when an
interstitial is dismissed with the Don't Proceed option.

BUG=659709
TEST=Visit http://testsafebrowsing.appspot.com, click first link. Observe that
the icon in the omnibox is a red triangle. Click "Back to safety". Observe that
the icon in the omnibox is an (i), not a red triangle.
TEST=Added browser test

Committed: https://crrev.com/1ca09ca22c714b67a7d2be4070686065f47fd9d7
Cr-Commit-Position: refs/heads/master@{#428932}
==========

[commit-bot: I haz the power, 2016-11-01 04:06:40]

Patchset 9 (id:??) landed as
https://crrev.com/1ca09ca22c714b67a7d2be4070686065f47fd9d7
Cr-Commit-Position: refs/heads/master@{#428932}