Commit 3d401f53 authored by Carlos IL's avatar Carlos IL Committed by Commit Bot

Fix CanGoBack check when creating loud SB errors

Checking CanGoBack when constructing an interstitial from a throttle
(the way they are created with committed interstitials) resulted in
false negatives in some cases. This functionality is not used in Chrome
but is in Android Webview for safe browsing interstitials (they do not
show the Back To Safety button if there is nothing to go back to).
This CL introduces a separate method to check in that case.

Test: Covered by testSafeBrowsingDontProceedNavigatesBackForMainFrame

Bug: 1031620
Change-Id: I3638e173dcc226a18110153c6a33595a5cf45ab0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1956120
Commit-Queue: Carlos IL <carlosil@chromium.org>
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Auto-Submit: Carlos IL <carlosil@chromium.org>
Reviewed-by: default avatarDaniel Rubery <drubery@chromium.org>
Reviewed-by: default avatarMustafa Emre Acer <meacer@chromium.org>
Reviewed-by: default avatarEugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#722703}
parent 36a400bc
...@@ -69,7 +69,10 @@ BaseBlockingPage::BaseBlockingPage( ...@@ -69,7 +69,10 @@ BaseBlockingPage::BaseBlockingPage(
display_options, display_options,
ui_manager->app_locale(), ui_manager->app_locale(),
base::Time::NowFromSystemTime(), base::Time::NowFromSystemTime(),
controller())) {} controller(),
/* created_prior_to_navigation */
IsMainPageLoadBlocked(unsafe_resources) &&
base::FeatureList::IsEnabled(kCommittedSBInterstitials))) {}
BaseBlockingPage::~BaseBlockingPage() {} BaseBlockingPage::~BaseBlockingPage() {}
......
...@@ -111,4 +111,10 @@ void SecurityInterstitialControllerClient::LaunchDateAndTimeSettings() { ...@@ -111,4 +111,10 @@ void SecurityInterstitialControllerClient::LaunchDateAndTimeSettings() {
NOTREACHED(); NOTREACHED();
} }
bool SecurityInterstitialControllerClient::CanGoBackBeforeNavigation() {
// If checking before navigating to the interstitial, back to safety is
// possible if there is already at least one prior entry.
return web_contents_->GetController().GetEntryCount() > 0;
}
} // namespace security_interstitials } // namespace security_interstitials
...@@ -47,6 +47,7 @@ class SecurityInterstitialControllerClient ...@@ -47,6 +47,7 @@ class SecurityInterstitialControllerClient
const std::string& GetApplicationLocale() const override; const std::string& GetApplicationLocale() const override;
bool CanLaunchDateAndTimeSettings() override; bool CanLaunchDateAndTimeSettings() override;
void LaunchDateAndTimeSettings() override; void LaunchDateAndTimeSettings() override;
bool CanGoBackBeforeNavigation() override;
protected: protected:
// security_interstitials::ControllerClient overrides. // security_interstitials::ControllerClient overrides.
......
...@@ -79,6 +79,9 @@ class ControllerClient { ...@@ -79,6 +79,9 @@ class ControllerClient {
virtual void GoBack() = 0; virtual void GoBack() = 0;
// Whether it is possible to go 'Back to safety'. // Whether it is possible to go 'Back to safety'.
virtual bool CanGoBack() = 0; virtual bool CanGoBack() = 0;
// Alternate method to check if the user can go "back to safety", meant to
// be called before navigating to the interstitial.
virtual bool CanGoBackBeforeNavigation() = 0;
// If the offending entry has committed, go back or to a safe page without // If the offending entry has committed, go back or to a safe page without
// closing the error page. This error page will be closed when the new page // closing the error page. This error page will be closed when the new page
......
...@@ -43,14 +43,16 @@ SafeBrowsingLoudErrorUI::SafeBrowsingLoudErrorUI( ...@@ -43,14 +43,16 @@ SafeBrowsingLoudErrorUI::SafeBrowsingLoudErrorUI(
const SBErrorDisplayOptions& display_options, const SBErrorDisplayOptions& display_options,
const std::string& app_locale, const std::string& app_locale,
const base::Time& time_triggered, const base::Time& time_triggered,
ControllerClient* controller) ControllerClient* controller,
bool created_prior_to_navigation)
: BaseSafeBrowsingErrorUI(request_url, : BaseSafeBrowsingErrorUI(request_url,
main_frame_url, main_frame_url,
reason, reason,
display_options, display_options,
app_locale, app_locale,
time_triggered, time_triggered,
controller) { controller),
created_prior_to_navigation_(created_prior_to_navigation) {
controller->metrics_helper()->RecordUserDecision(MetricsHelper::SHOW); controller->metrics_helper()->RecordUserDecision(MetricsHelper::SHOW);
controller->metrics_helper()->RecordUserInteraction( controller->metrics_helper()->RecordUserInteraction(
MetricsHelper::TOTAL_VISITS); MetricsHelper::TOTAL_VISITS);
...@@ -82,9 +84,14 @@ void SafeBrowsingLoudErrorUI::PopulateStringsForHtml( ...@@ -82,9 +84,14 @@ void SafeBrowsingLoudErrorUI::PopulateStringsForHtml(
l10n_util::GetStringUTF16(IDS_SAFEBROWSING_OVERRIDABLE_SAFETY_BUTTON)); l10n_util::GetStringUTF16(IDS_SAFEBROWSING_OVERRIDABLE_SAFETY_BUTTON));
load_time_data->SetBoolean("overridable", !is_proceed_anyway_disabled()); load_time_data->SetBoolean("overridable", !is_proceed_anyway_disabled());
load_time_data->SetBoolean( if (always_show_back_to_safety()) {
"hide_primary_button", load_time_data->SetBoolean("hide_primary_button", false);
always_show_back_to_safety() ? false : !controller()->CanGoBack()); } else {
load_time_data->SetBoolean("hide_primary_button",
created_prior_to_navigation_
? !controller()->CanGoBackBeforeNavigation()
: !controller()->CanGoBack());
}
load_time_data->SetBoolean( load_time_data->SetBoolean(
"billing", "billing",
......
...@@ -20,6 +20,9 @@ namespace security_interstitials { ...@@ -20,6 +20,9 @@ namespace security_interstitials {
// This class displays UI for Safe Browsing errors that block page loads. This // This class displays UI for Safe Browsing errors that block page loads. This
// class is purely about visual display; it does not do any error-handling logic // class is purely about visual display; it does not do any error-handling logic
// to determine what type of error should be displayed when. // to determine what type of error should be displayed when.
// |created_prior_to_navigation| should be set to true if this UI was created
// prior to navigating to the error page (e.g. from WillFailResponse in a
// navigation throttle), false otherwise.
class SafeBrowsingLoudErrorUI class SafeBrowsingLoudErrorUI
: public security_interstitials::BaseSafeBrowsingErrorUI { : public security_interstitials::BaseSafeBrowsingErrorUI {
public: public:
...@@ -30,7 +33,8 @@ class SafeBrowsingLoudErrorUI ...@@ -30,7 +33,8 @@ class SafeBrowsingLoudErrorUI
const BaseSafeBrowsingErrorUI::SBErrorDisplayOptions& display_options, const BaseSafeBrowsingErrorUI::SBErrorDisplayOptions& display_options,
const std::string& app_locale, const std::string& app_locale,
const base::Time& time_triggered, const base::Time& time_triggered,
ControllerClient* controller); ControllerClient* controller,
bool created_prior_to_navigation);
~SafeBrowsingLoudErrorUI() override; ~SafeBrowsingLoudErrorUI() override;
...@@ -49,6 +53,8 @@ class SafeBrowsingLoudErrorUI ...@@ -49,6 +53,8 @@ class SafeBrowsingLoudErrorUI
void PopulatePhishingLoadTimeData(base::DictionaryValue* load_time_data); void PopulatePhishingLoadTimeData(base::DictionaryValue* load_time_data);
void PopulateBillingLoadTimeData(base::DictionaryValue* load_time_data); void PopulateBillingLoadTimeData(base::DictionaryValue* load_time_data);
const bool created_prior_to_navigation_;
DISALLOW_COPY_AND_ASSIGN(SafeBrowsingLoudErrorUI); DISALLOW_COPY_AND_ASSIGN(SafeBrowsingLoudErrorUI);
}; };
......
...@@ -38,6 +38,7 @@ class IOSChromeControllerClient ...@@ -38,6 +38,7 @@ class IOSChromeControllerClient
void LaunchDateAndTimeSettings() override; void LaunchDateAndTimeSettings() override;
void GoBack() override; void GoBack() override;
bool CanGoBack() override; bool CanGoBack() override;
bool CanGoBackBeforeNavigation() override;
void GoBackAfterNavigationCommitted() override; void GoBackAfterNavigationCommitted() override;
void Proceed() override; void Proceed() override;
void Reload() override; void Reload() override;
......
...@@ -48,6 +48,11 @@ bool IOSChromeControllerClient::CanGoBack() { ...@@ -48,6 +48,11 @@ bool IOSChromeControllerClient::CanGoBack() {
return web_state_->GetNavigationManager()->CanGoBack(); return web_state_->GetNavigationManager()->CanGoBack();
} }
bool IOSChromeControllerClient::CanGoBackBeforeNavigation() {
NOTREACHED();
return false;
}
void IOSChromeControllerClient::GoBackAfterNavigationCommitted() { void IOSChromeControllerClient::GoBackAfterNavigationCommitted() {
NOTREACHED(); NOTREACHED();
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment