New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 823414 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Ad filtering / abusive enforcement should be triggered on a site if any of the redirects match safe browsing

Project Member Reported by csharrison@chromium.org, Mar 19 2018

Issue description

This should be a relatively straightforward change. See SubresourceFilterSafeBrowsingActivationThrottle::NotifyResult. Right now we just use the last URL in the redirect chain.

In general, what we probably want to do is:
 - Ensure that each SB check has completed (debatable if we don't want to regress performance)
 - Iterate through each SB check and find the Configuration for it
 - Pick the most high priority Configuration that we found
 
Owner: ericrobinson@chromium.org

Comment 2 Deleted

Excellent questions. Here are my initial thoughts. This did end up being a bit more complex than I originally thought :D

1. NotifySafeBrowsingCheckComplete should just send the client the result of all the checks, since that consumer might want to apply a different policy (e.g. loop through them, or pick the last one). This could be implemented as a separate CL. The consumers have no notion of priority as internal to subresource_filter.

2. Warning should be set by highest priority config.

3. All Notification (inc NotifyPageActivationComputed) should happen once.
I'm working on a fix for this but there are a few considerations here with forced activation, which seems to be handled somewhat oddly.  Currently:
1. Matched_lists are computed in all cases using the last known check_result, this list is ignored for forced activation.
2. NotifySafeBrowsingCheckComplete is called using the last known check_result (threat type and metadata) even though that's ignored by forced activation.
3. The "warning" variable is set based on the matched_list results, even though that's ignored by forced activation.
4. Regular activation uses the above to form the Configuration and reach an activation decision, forced special cases these.

The new code will loop through all of the check results and take the one with the highest priority (as specified in the Configuration).  That leads to a few questions regarding behavior:
1. Does forced activation have to do this looping at all?  If it doesn't, how does it account for warning and the threat type/metadata for the Notification.
2. Warning should probably be set based on the highest priority?
3. Notification should presumably happen once, using the information from the highest priority?
Project Member

Comment 5 by bugdroid1@chromium.org, May 14 2018

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

commit c93faebde5f9dabe17ccba3ebe2cbfa274df8254
Author: Eric Robinson <ericrobinson@chromium.org>
Date: Mon May 14 02:38:00 2018

Wait until all SafeBrowsing checks complete before calling NotifyResult.

This CL is the first step in having NotifyResult select the check with the
highest priority.  In order to do that (and not store intermediate data), all of
the checks must have finished prior to NotifyResult being called.

This *may* regress performance, and we should track the potential regression in
SubresourceFilter.PageLoad.SafeBrowsingDelay.

Bug:  823414 
Change-Id: I1339e3795e9178dd7dd789ac98fcd21a2b76d726
Reviewed-on: https://chromium-review.googlesource.com/984672
Commit-Queue: Eric Robinson <ericrobinson@chromium.org>
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Reviewed-by: Asanka Herath <asanka@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558178}
[modify] https://crrev.com/c93faebde5f9dabe17ccba3ebe2cbfa274df8254/chrome/browser/subresource_filter/subresource_filter_intercepting_browsertest.cc
[modify] https://crrev.com/c93faebde5f9dabe17ccba3ebe2cbfa274df8254/components/safe_browsing/db/v4_embedded_test_server_util.cc
[modify] https://crrev.com/c93faebde5f9dabe17ccba3ebe2cbfa274df8254/components/safe_browsing/db/v4_embedded_test_server_util.h
[modify] https://crrev.com/c93faebde5f9dabe17ccba3ebe2cbfa274df8254/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
[modify] https://crrev.com/c93faebde5f9dabe17ccba3ebe2cbfa274df8254/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h
[modify] https://crrev.com/c93faebde5f9dabe17ccba3ebe2cbfa274df8254/net/test/embedded_test_server/default_handlers.cc
[modify] https://crrev.com/c93faebde5f9dabe17ccba3ebe2cbfa274df8254/net/test/embedded_test_server/http_response.cc
[modify] https://crrev.com/c93faebde5f9dabe17ccba3ebe2cbfa274df8254/net/test/embedded_test_server/http_response.h

There are a couple other CLs I forgot to add bug tags to:

Split ComputeActivation into its two components:
https://chromium-review.googlesource.com/1059893

Added functionality to find highest priority safebrowsing result:
https://chromium-review.googlesource.com/1058087

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 1 2018

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

commit 3a9d09cf4b8521d7c47da359f42f0885501a646d
Author: Charlie Harrison <csharrison@chromium.org>
Date: Fri Jun 01 23:43:29 2018

Deprecate SubresourceFilter.PageLoad.SafeBrowsingDelay.NoRedirectSpeculation

This metric has shown that on Android, we definitely need our speculative
redirect checks.
95%ile:   22ms
99%ile:   84ms
99.5%ile: 175ms

Additionally, we are working on a new feature which depends on all the
redirect checks actually happening, so there's no going back to a model
where we only do a Safe Browsing check on the last result.

Bug:  823414 
Change-Id: If2911a53ac4e4b21d7623292c54728da95d972a1
Reviewed-on: https://chromium-review.googlesource.com/1081019
Reviewed-by: Gayane Petrosyan <gayane@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563864}
[modify] https://crrev.com/3a9d09cf4b8521d7c47da359f42f0885501a646d/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
[modify] https://crrev.com/3a9d09cf4b8521d7c47da359f42f0885501a646d/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc
[modify] https://crrev.com/3a9d09cf4b8521d7c47da359f42f0885501a646d/tools/metrics/histograms/histograms.xml

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 6 2018

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

commit d88ee85578da6ccb341df56645590bad8fa19952
Author: Eric Robinson <ericrobinson@chromium.org>
Date: Wed Jun 06 00:03:40 2018

Safe browsing notification reports all results.

Changed the safe browsing notifications to report all of the results,
not just the final one obtained.  This is done by passing the
check_results_ vector directly to the consumers.  The only non-test
consumer currently is the pop-up blocker, which now scans for any
matching condition and reports it rather than just looking at the
final result and determining if it was a match.

Bug:  823414 
Change-Id: I9c820e6d1ca2b640339cc31a17c42d3fb8486f2b
Reviewed-on: https://chromium-review.googlesource.com/1081048
Commit-Queue: Eric Robinson <ericrobinson@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564710}
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker.cc
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker.h
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/chrome/browser/ui/blocked_content/safe_browsing_triggered_popup_blocker_unittest.cc
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/components/subresource_filter/content/browser/subresource_filter_observer.h
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/components/subresource_filter/content/browser/subresource_filter_observer_manager.cc
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/components/subresource_filter/content/browser/subresource_filter_observer_manager.h
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/components/subresource_filter/content/browser/subresource_filter_observer_test_utils.cc
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/components/subresource_filter/content/browser/subresource_filter_observer_test_utils.h
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
[modify] https://crrev.com/d88ee85578da6ccb341df56645590bad8fa19952/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h

Status: Fixed (was: Untriaged)
\o/!
Just to clarify, we have the behavior logic in place under a flag, which we will roll out sometime in the near future.
Cc: jud@google.com
jud@:  Note that this behavior has been added and I've defaulted it to "enabled".  It's easy enough to change it to "disabled" if needed prior to the next release (Feature Freeze June 22nd, Branch July 19th).

I know there was some work on your end for this, so let me know how that's going/what you'd prefer me to do here.

Sign in to add a comment