New issue
Advanced search Search tips

Issue 738192 link

Starred by 2 users

Issue metadata

Status: Verified
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Task

Blocked on:
issue 733815

Blocking:
issue 680790



Sign in to add a comment

More Safe Browsing tests

Project Member Reported by ntfschr@chromium.org, Jun 29 2017

Issue description

Once  issue 733815  is resolved, we should add more tests that require javascript:

 - click on links within the interstitial, like "learn more"
 - verify Loud vs. Quiet interstitials are created when we expect
 - "back to safety" button should be hidden if we have no page to navigate back to
 - "back to safety" should navigate backward for malicious subresource (dontProceed() isn't sufficient for this)

I'll update this bug if I think of more tests to add.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b

commit daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b
Author: Nate Fischer <ntfschr@chromium.org>
Date: Wed Aug 09 21:37:31 2017

AW: open extended reporting links in the same WebView

Previously, reporting-related links were always opening in a new
foreground tab, instead of following the preference from
should_open_links_in_new_tab.

This moves the OpenURL() helper from SafeBrowsingLoudErrorUI to
ControllerClient so that all links respect should_open_links_in_new_tab.

As before, chrome will open links in new foreground tabs. WebView will
now open all links in the current "tab" (WebView), which is the desired
behavior.

This includes full integration test coverage for WebView:

 * Opens each link on phishing/malware interstitials
 * Refactors tests to reduce duplicated code
 * Indicates with a comment which links are phishing-only or
   malware-only

Bug: 751258
Bug:  738192 
Test: run_webview_instrumentation_test_apk -f SafeBrowsingTest#testSafeBrowsingClick*
Change-Id: I34fb4d2e24bdd76f3579693601209d44381e2d0b
Reviewed-on: https://chromium-review.googlesource.com/606948
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#493149}
[modify] https://crrev.com/daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b/android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java
[modify] https://crrev.com/daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b/chrome/browser/ssl/captive_portal_blocking_page.cc
[modify] https://crrev.com/daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b/components/security_interstitials/core/bad_clock_ui.cc
[modify] https://crrev.com/daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b/components/security_interstitials/core/controller_client.cc
[modify] https://crrev.com/daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b/components/security_interstitials/core/controller_client.h
[modify] https://crrev.com/daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b/components/security_interstitials/core/safe_browsing_loud_error_ui.cc
[modify] https://crrev.com/daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b/components/security_interstitials/core/safe_browsing_loud_error_ui.h
[modify] https://crrev.com/daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b/components/security_interstitials/core/ssl_error_ui.cc

The CL above covers the "click on links within the interstitial, like 'learn more'" bullet. The remaining bullets still stand to be finished.
Project Member

Comment 3 by bugdroid1@chromium.org, Aug 10 2017

Labels: merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25a12bde33efa5cfc05e26e0c030253bdffaae4f

commit 25a12bde33efa5cfc05e26e0c030253bdffaae4f
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Aug 10 23:18:31 2017

AW: open extended reporting links in the same WebView

Previously, reporting-related links were always opening in a new
foreground tab, instead of following the preference from
should_open_links_in_new_tab.

This moves the OpenURL() helper from SafeBrowsingLoudErrorUI to
ControllerClient so that all links respect should_open_links_in_new_tab.

As before, chrome will open links in new foreground tabs. WebView will
now open all links in the current "tab" (WebView), which is the desired
behavior.

This includes full integration test coverage for WebView:

 * Opens each link on phishing/malware interstitials
 * Refactors tests to reduce duplicated code
 * Indicates with a comment which links are phishing-only or
   malware-only

TBR=ntfschr@chromium.org

(cherry picked from commit daf7d437bb2fd277c9a7bbbfd6ea6e1e5545fe6b)

Bug: 751258
Bug:  738192 
Test: run_webview_instrumentation_test_apk -f SafeBrowsingTest#testSafeBrowsingClick*
Change-Id: I34fb4d2e24bdd76f3579693601209d44381e2d0b
Reviewed-on: https://chromium-review.googlesource.com/606948
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Bo <boliu@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#493149}
Reviewed-on: https://chromium-review.googlesource.com/611285
Reviewed-by: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#470}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/25a12bde33efa5cfc05e26e0c030253bdffaae4f/android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java
[modify] https://crrev.com/25a12bde33efa5cfc05e26e0c030253bdffaae4f/chrome/browser/ssl/captive_portal_blocking_page.cc
[modify] https://crrev.com/25a12bde33efa5cfc05e26e0c030253bdffaae4f/components/security_interstitials/core/bad_clock_ui.cc
[modify] https://crrev.com/25a12bde33efa5cfc05e26e0c030253bdffaae4f/components/security_interstitials/core/controller_client.cc
[modify] https://crrev.com/25a12bde33efa5cfc05e26e0c030253bdffaae4f/components/security_interstitials/core/controller_client.h
[modify] https://crrev.com/25a12bde33efa5cfc05e26e0c030253bdffaae4f/components/security_interstitials/core/safe_browsing_loud_error_ui.cc
[modify] https://crrev.com/25a12bde33efa5cfc05e26e0c030253bdffaae4f/components/security_interstitials/core/safe_browsing_loud_error_ui.h
[modify] https://crrev.com/25a12bde33efa5cfc05e26e0c030253bdffaae4f/components/security_interstitials/core/ssl_error_ui.cc

Verified in Muskie/OPM1.170816.001 and S8/NRD90M with Monochrome Beta 61.0.3163.51 , back to safety and links 
sbashyam@, this bug itself doesn't require manual verification (it's just a tracker for adding more automated tests). Thanks for checking though!
Verified on latest M61- 61.0.3163.74 as per the steps mentioned in comment#0. And issue doesn't repro on latest M62: 62.0.3201.4

Thanks!

Tested device:Pixel XL / OPM1.170831.003
Labels: WebView-SafeBrowsing

Comment 9 by est...@chromium.org, Nov 10 2017

Labels: Hotlist-EnamelAndFriendsFixIt
Cc: changwan@chromium.org
Another idea to test: the visibility of the Safe Browsing Extended Reporting checkbox (and the description text for reporting). This should be visible by default, but apps should be able to hide it using onSafeBrowsingHit().
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7728e879c423acac345a5c83efa36bdc8f476eea

commit 7728e879c423acac345a5c83efa36bdc8f476eea
Author: Nate Fischer <ntfschr@chromium.org>
Date: Thu Feb 01 19:15:23 2018

AW: test Safe Browsing checkbox value

No change to production logic, only changes tests.

This adds two new tests to check the Safe Browsing Extended Reporting
checkbox:

 * The checkbox is visible by default
 * The checkbox can be hidden using onSafeBrowsingHit()

This makes no assertion about the default *value* of the checkbox (since
this depends on the user's last reporting preference within the browsing
session). We're only checking whether it's hidden.

This also does some refactoring:

 * Adds evaluateJavaScriptOnInterstitialOnUiThreadSync() method and uses
   it where applicable
 * Renames waitForInterstitialToLoad to waitForInterstitialDomToLoad (to
   clarify what it actually does)

Bug:  738192 
Test: run_webview_instrumentation_test_apk -f SafeBrowsingTest#testSafeBrowsing*ReportingCheckbox*
Change-Id: Ib1d6cc74c037f60d3c026c87264bf9f768884544
Reviewed-on: https://chromium-review.googlesource.com/896790
Reviewed-by: Bo <boliu@chromium.org>
Commit-Queue: Nate Fischer <ntfschr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#533767}
[modify] https://crrev.com/7728e879c423acac345a5c83efa36bdc8f476eea/android_webview/javatests/src/org/chromium/android_webview/test/SafeBrowsingTest.java

Labels: -Hotlist-EnamelAndFriendsFixIt
Labels: -Type-Bug Type-Task
Status: Fixed (was: Assigned)
I don't think we have an immediate need for more tests, so I'll mark this as fixed (no manual verification necessary).
Status: Verified (was: Fixed)
Bulk edit: marking stale 'fixed' bugs as 'verified' since they don't need verification at this point.

Sign in to add a comment