Skip to content

Commit

Permalink
[subresource_filter] Display a console warning when subresource filte…
Browse files Browse the repository at this point in the history
…ring computes

activation state as ENABLED.
BUG=724549

Review-Url: https://codereview.chromium.org/2898783002
Cr-Commit-Position: refs/heads/master@{#474869}
  • Loading branch information
shivanigithub authored and Commit bot committed May 26, 2017
1 parent 1fb72fe commit bdacc13
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
#include "components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h"
#include "components/subresource_filter/content/browser/content_ruleset_service.h"
#include "components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h"
#include "components/subresource_filter/core/browser/subresource_filter_constants.h"
#include "components/subresource_filter/core/browser/subresource_filter_features.h"
#include "components/subresource_filter/core/browser/subresource_filter_features_test_support.h"
#include "components/subresource_filter/core/common/activation_decision.h"
Expand Down Expand Up @@ -464,6 +465,9 @@ class SubresourceFilterWebSocketBrowserTest
// Tests -----------------------------------------------------------------------

IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, MainFrameActivation) {
content::ConsoleObserverDelegate console_observer(web_contents(),
kActivationConsoleMessage);
web_contents()->SetDelegate(&console_observer);
GURL url(GetTestUrl("subresource_filter/frame_with_included_script.html"));
ConfigureAsPhishingURL(url);
ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix(
Expand All @@ -476,6 +480,8 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, MainFrameActivation) {
ui_test_utils::NavigateToURL(browser(), url);
EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));

EXPECT_EQ(console_observer.message(), kActivationConsoleMessage);

// The main frame document should never be filtered.
SetRulesetToDisallowURLsWithPathSuffix("frame_with_included_script.html");
ui_test_utils::NavigateToURL(browser(), url);
Expand All @@ -485,6 +491,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, MainFrameActivation) {
#if defined(GOOGLE_CHROME_BUILD)
IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
MainFrameActivation_SubresourceFilterList) {
content::ConsoleObserverDelegate console_observer(web_contents(),
kActivationConsoleMessage);
web_contents()->SetDelegate(&console_observer);
GURL url(GetTestUrl("subresource_filter/frame_with_included_script.html"));
ConfigureAsSubresourceFilterOnlyURL(url);
ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix(
Expand All @@ -503,6 +512,8 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
ui_test_utils::NavigateToURL(browser(), url);
EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));

EXPECT_EQ(console_observer.message(), kActivationConsoleMessage);

// The main frame document should never be filtered.
SetRulesetToDisallowURLsWithPathSuffix("frame_with_included_script.html");
ui_test_utils::NavigateToURL(browser(), url);
Expand Down Expand Up @@ -619,6 +630,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,

IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
HistoryNavigationActivation) {
content::ConsoleObserverDelegate console_observer(web_contents(),
kActivationConsoleMessage);
web_contents()->SetDelegate(&console_observer);
GURL url_with_activation(GetTestUrl(kTestFrameSetPath));
GURL url_without_activation(
embedded_test_server()->GetURL("a.com", kTestFrameSetPath));
Expand All @@ -636,10 +650,16 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
ASSERT_NO_FATAL_FAILURE(ExpectParsedScriptElementLoadedStatusInFrames(
kSubframeNames, kExpectScriptInFrameToLoadWithoutActivation));

// No message should be displayed for navigating to URL without activation.
EXPECT_TRUE(console_observer.message().empty());

ui_test_utils::NavigateToURL(browser(), url_with_activation);
ASSERT_NO_FATAL_FAILURE(ExpectParsedScriptElementLoadedStatusInFrames(
kSubframeNames, kExpectScriptInFrameToLoadWithActivation));

// Console message should now be displayed.
EXPECT_EQ(console_observer.message(), kActivationConsoleMessage);

ASSERT_TRUE(web_contents()->GetController().CanGoBack());
content::WindowedNotificationObserver back_navigation_stop_observer(
content::NOTIFICATION_LOAD_STOP,
Expand Down Expand Up @@ -697,6 +717,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
// dynamically inserting a subframe afterwards, and still expecting activation.
IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
PageLevelActivationOutlivesSameDocumentNavigation) {
content::ConsoleObserverDelegate console_observer(web_contents(),
kActivationConsoleMessage);
web_contents()->SetDelegate(&console_observer);
GURL url(GetTestUrl(kTestFrameSetPath));
ConfigureAsPhishingURL(url);
ASSERT_NO_FATAL_FAILURE(
Expand All @@ -713,6 +736,8 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
content::RenderFrameHost* dynamic_frame = FindFrameByName("dynamic");
ASSERT_TRUE(dynamic_frame);
EXPECT_FALSE(WasParsedScriptElementLoaded(dynamic_frame));

EXPECT_EQ(console_observer.message(), kActivationConsoleMessage);
}

// If a navigation starts but aborts before commit, page level activation should
Expand Down Expand Up @@ -869,12 +894,19 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest, PageLoadMetrics) {
ui_test_utils::NavigateToURL(browser(), url);
EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));

content::ConsoleObserverDelegate console_observer(web_contents(),
kActivationConsoleMessage);
web_contents()->SetDelegate(&console_observer);

// Force a navigation to another page, which flushes page load metrics for the
// previous page load.
ui_test_utils::NavigateToURL(browser(), GURL(url::kAboutBlankURL));

histogram_tester.ExpectTotalCount(internal::kHistogramSubresourceFilterCount,
1);

// No message should be displayed for about blank URL.
EXPECT_TRUE(console_observer.message().empty());
}

IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
Expand Down Expand Up @@ -989,6 +1021,10 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
ui_test_utils::NavigateToURL(browser(), url);
EXPECT_FALSE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));

content::ConsoleObserverDelegate console_observer(web_contents(),
kActivationConsoleMessage);
web_contents()->SetDelegate(&console_observer);

// Simulate an explicity whitelisting via content settings.
HostContentSettingsMap* settings_map =
HostContentSettingsMapFactory::GetForProfile(browser()->profile());
Expand All @@ -998,6 +1034,9 @@ IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,

ui_test_utils::NavigateToURL(browser(), url);
EXPECT_TRUE(WasParsedScriptElementLoaded(web_contents()->GetMainFrame()));

// No message for whitelisted url.
EXPECT_TRUE(console_observer.message().empty());
}

IN_PROC_BROWSER_TEST_F(SubresourceFilterBrowserTest,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,15 @@ void ContentSubresourceFilterDriverFactory::NotifyPageActivationComputed(
ActivationState state = ActivationState(activation_options_.activation_level);
state.measure_performance = ShouldMeasurePerformanceForPageLoad(
activation_options_.performance_measurement_rate);
// TODO(csharrison): Set state.enable_logging based on metadata returns from
// the safe browsing filter, when it is available. Add tests for this
// behavior.

// TODO(csharrison): Also use metadata returned from the safe browsing filter,
// when it is available to set enable_logging. Add tests for this behavior.
state.enable_logging =
activation_options_.activation_level == ActivationLevel::ENABLED &&
!activation_options_.should_suppress_notifications &&
base::FeatureList::IsEnabled(
kSafeBrowsingSubresourceFilterExperimentalUI);

SubresourceFilterObserverManager::FromWebContents(web_contents())
->NotifyPageActivationComputed(navigation_handle, activation_decision_,
state);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
#include "components/subresource_filter/content/browser/subframe_navigation_filtering_throttle.h"
#include "components/subresource_filter/content/browser/subresource_filter_observer_manager.h"
#include "components/subresource_filter/content/common/subresource_filter_messages.h"
#include "components/subresource_filter/core/browser/subresource_filter_constants.h"
#include "content/public/browser/navigation_handle.h"
#include "content/public/browser/navigation_throttle.h"
#include "content/public/browser/render_frame_host.h"
#include "content/public/browser/web_contents.h"
#include "content/public/common/console_message_level.h"
#include "net/base/net_errors.h"

namespace subresource_filter {
Expand Down Expand Up @@ -102,19 +104,25 @@ void ContentSubresourceFilterThrottleManager::DidFinishNavigation(
ongoing_activation_throttles_.erase(throttle);
}

content::RenderFrameHost* frame_host =
navigation_handle->GetRenderFrameHost();
if (navigation_handle->IsInMainFrame()) {
current_committed_load_has_notified_disallowed_load_ = false;
statistics_.reset();
if (filter) {
statistics_ =
base::MakeUnique<PageLoadStatistics>(filter->activation_state());
if (filter->activation_state().enable_logging) {
DCHECK(filter->activation_state().activation_level !=
ActivationLevel::DISABLED);
frame_host->AddMessageToConsole(content::CONSOLE_MESSAGE_LEVEL_WARNING,
kActivationConsoleMessage);
}
}
}

// Make sure |activated_frame_hosts_| is updated or cleaned up depending on
// this navigation's activation state.
content::RenderFrameHost* frame_host =
navigation_handle->GetRenderFrameHost();
if (filter) {
filter->set_first_disallowed_load_callback(base::Bind(
&ContentSubresourceFilterThrottleManager::MaybeCallFirstDisallowedLoad,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,8 @@ const base::FilePath::CharType kUnindexedRulesetLicenseFileName[] =
const base::FilePath::CharType kUnindexedRulesetDataFileName[] =
FILE_PATH_LITERAL("Filtering Rules");

// TODO(shivanisha): Update the string when finalized.
const std::string kActivationConsoleMessage =
"Subresource filter is activated on this site";

} // namespace subresource_filter
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ extern const base::FilePath::CharType kUnindexedRulesetLicenseFileName[];
// The name of the file that stores the unindexed filtering rules.
extern const base::FilePath::CharType kUnindexedRulesetDataFileName[];

// Console message to be displayed on activation.
extern const std::string kActivationConsoleMessage;

} // namespace subresource_filter

#endif // COMPONENTS_SUBRESOURCE_FILTER_CORE_BROWSER_SUBRESOURCE_FILTER_CONSTANTS_H_
3 changes: 2 additions & 1 deletion components/subresource_filter/core/common/activation_state.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ struct ActivationState {
(filtering_disabled_for_document ||
generic_blocking_rules_disabled ==
rhs.generic_blocking_rules_disabled) &&
measure_performance == rhs.measure_performance;
measure_performance == rhs.measure_performance &&
enable_logging == rhs.enable_logging;
}

bool operator!=(const ActivationState& rhs) const { return !operator==(rhs); }
Expand Down

0 comments on commit bdacc13

Please sign in to comment.