Skip to content

Commit

Permalink
Clean up mixed forms interstitial feature flag
Browse files Browse the repository at this point in the history
Mixed forms interstitials have launched a while ago and there are no
plans to roll them back, this cleans up the feature flag.

This also cleans up the mode parameter that controls whether warnings
are displayed for redirects through HTTP. While we previously discussed
launching warnings for all redirects (not just those that leak form
data), there are no concrete plans for doing so at the moment, and if
we decide to launch, we can add a feature flag that controls that
specifically, instead of the convoluted parameter handling that is
currently being done.

Change-Id: I416a1651ef4fe67288b668e0007b5f2a63d1c2a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3226845
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Colin Blundell <blundell@chromium.org>
Auto-Submit: Carlos IL <carlosil@chromium.org>
Commit-Queue: Colin Blundell <blundell@chromium.org>
Cr-Commit-Position: refs/heads/main@{#933404}
  • Loading branch information
carlosjoan91 authored and Chromium LUCI CQ committed Oct 20, 2021
1 parent bbf6ea0 commit 0370889
Show file tree
Hide file tree
Showing 12 changed files with 38 additions and 565 deletions.
6 changes: 0 additions & 6 deletions chrome/browser/about_flags.cc
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@
#include "components/safe_browsing/core/common/features.h"
#include "components/search/ntp_features.h"
#include "components/security_interstitials/content/stateful_ssl_host_state_delegate.h"
#include "components/security_interstitials/core/features.h"
#include "components/security_state/core/features.h"
#include "components/security_state/core/security_state.h"
#include "components/send_tab_to_self/features.h"
Expand Down Expand Up @@ -6747,11 +6746,6 @@ const FeatureEntry kFeatureEntries[] = {
flag_descriptions::kEditPasswordsInSettingsDescription, kOsAll,
FEATURE_VALUE_TYPE(password_manager::features::kEditPasswordsInSettings)},

{"mixed-forms-interstitial", flag_descriptions::kMixedFormsInterstitialName,
flag_descriptions::kMixedFormsInterstitialDescription, kOsAll,
FEATURE_VALUE_TYPE(
security_interstitials::kInsecureFormSubmissionInterstitial)},

#if BUILDFLAG(IS_CHROMEOS_ASH)
{"frame-throttle-fps", flag_descriptions::kFrameThrottleFpsName,
flag_descriptions::kFrameThrottleFpsDescription, kOsCrOS,
Expand Down
6 changes: 0 additions & 6 deletions chrome/browser/flag_descriptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1624,12 +1624,6 @@ const char kMetricsSettingsAndroidName[] = "Metrics Settings on Android";
const char kMetricsSettingsAndroidDescription[] =
"Enables the new design of metrics settings.";

const char kMixedFormsInterstitialName[] = "Mixed forms interstitial";
const char kMixedFormsInterstitialDescription[] =
"When enabled, a full-page interstitial warning is shown when a mixed "
"content form (a form on an HTTPS site that submits over HTTP) is "
"submitted.";

const char kMobileIdentityConsistencyFREName[] =
"Mobile identity consistency FRE";
const char kMobileIdentityConsistencyFREDescription[] =
Expand Down
3 changes: 0 additions & 3 deletions chrome/browser/flag_descriptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -933,9 +933,6 @@ extern const char kMediaSessionWebRTCDescription[];
extern const char kMetricsSettingsAndroidName[];
extern const char kMetricsSettingsAndroidDescription[];

extern const char kMixedFormsInterstitialName[];
extern const char kMixedFormsInterstitialDescription[];

extern const char kMobileIdentityConsistencyFREName[];
extern const char kMobileIdentityConsistencyFREDescription[];

Expand Down
19 changes: 7 additions & 12 deletions chrome/browser/ssl/security_state_tab_helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
#include "components/prefs/pref_service.h"
#include "components/safe_browsing/buildflags.h"
#include "components/safe_browsing/content/browser/ui_manager.h"
#include "components/security_interstitials/core/features.h"
#include "components/security_interstitials/core/pref_names.h"
#include "components/security_state/content/content_utils.h"
#include "content/public/browser/browser_context.h"
Expand Down Expand Up @@ -113,17 +112,13 @@ SecurityStateTabHelper::GetVisibleSecurityState() {
: security_state::SafetyTipInfo(
{security_state::SafetyTipStatus::kUnknown, GURL()});

// If both the on-form warning and the on-submit warning are enabled for mixed
// forms (and they are not disabled by policy) we don't degrade the lock icon
// for sites with mixed forms present.
if (base::FeatureList::IsEnabled(
security_interstitials::kInsecureFormSubmissionInterstitial)) {
Profile* profile =
Profile::FromBrowserContext(web_contents()->GetBrowserContext());
if (profile &&
profile->GetPrefs()->GetBoolean(prefs::kMixedFormsWarningsEnabled)) {
state->should_treat_displayed_mixed_forms_as_secure = true;
}
// If both the mixed form warnings are not disabled by policy we don't degrade
// the lock icon for sites with mixed forms present.
Profile* profile =
Profile::FromBrowserContext(web_contents()->GetBrowserContext());
if (profile &&
profile->GetPrefs()->GetBoolean(prefs::kMixedFormsWarningsEnabled)) {
state->should_treat_displayed_mixed_forms_as_secure = true;
}

auto* https_only_mode_tab_helper =
Expand Down
44 changes: 2 additions & 42 deletions chrome/browser/ssl/security_state_tab_helper_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@
#include "components/safe_browsing/core/common/proto/csd.pb.h"
#include "components/security_interstitials/content/security_interstitial_tab_helper.h"
#include "components/security_interstitials/content/ssl_blocking_page.h"
#include "components/security_interstitials/core/features.h"
#include "components/security_interstitials/core/pref_names.h"
#include "components/security_state/core/features.h"
#include "components/security_state/core/security_state.h"
Expand Down Expand Up @@ -1850,20 +1849,7 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest, SafetyTipFormHistogram) {
}
}

class SecurityStateTabHelperTestWithMixedFormsWarnings
: public SecurityStateTabHelperTest {
public:
SecurityStateTabHelperTestWithMixedFormsWarnings() {
std::vector<base::Feature> enabled_features = {
security_interstitials::kInsecureFormSubmissionInterstitial};
feature_list.InitWithFeatures(enabled_features, {});
}

private:
base::test::ScopedFeatureList feature_list;
};

IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTestWithMixedFormsWarnings,
IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest,
MixedFormsShowLockIfWarningsAreEnabled) {
SetUpMockCertVerifierForHttpsServer(0, net::OK);

Expand All @@ -1876,7 +1862,7 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTestWithMixedFormsWarnings,
false /* expect cert status error */);
}

IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTestWithMixedFormsWarnings,
IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTest,
MixedFormsDontShowLockIfWarningsAreDisabledByPolicy) {
SetUpMockCertVerifierForHttpsServer(0, net::OK);

Expand All @@ -1892,32 +1878,6 @@ IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTestWithMixedFormsWarnings,
false /* expect cert status error */);
}

class SecurityStateTabHelperTestWithMixedFormsWarningsDisabled
: public SecurityStateTabHelperTest {
public:
SecurityStateTabHelperTestWithMixedFormsWarningsDisabled() {
std::vector<base::Feature> disabled_features = {
security_interstitials::kInsecureFormSubmissionInterstitial};
feature_list.InitWithFeatures({}, disabled_features);
}

private:
base::test::ScopedFeatureList feature_list;
};

IN_PROC_BROWSER_TEST_F(SecurityStateTabHelperTestWithMixedFormsWarningsDisabled,
MixedFormsDontShowLockIfWarningsAreDisabled) {
SetUpMockCertVerifierForHttpsServer(0, net::OK);

ASSERT_TRUE(ui_test_utils::NavigateToURL(
browser(),
https_server_.GetURL("/ssl/page_displays_insecure_form.html")));
CheckSecurityInfoForSecure(
browser()->tab_strip_model()->GetActiveWebContents(),
security_state::NONE, false, false, false,
false /* expect cert status error */);
}

class SignedExchangeSecurityStateTest
: public CertVerifierBrowserTest,
public testing::WithParamInterface<
Expand Down
Loading

0 comments on commit 0370889

Please sign in to comment.