Ad filtering / abusive enforcement should be triggered on a site if any of the redirects match safe browsing |
|||
Issue descriptionThis 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
,
Mar 27 2018
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.
,
Mar 27 2018
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?
,
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
,
Jun 1 2018
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
,
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
,
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
,
Jun 7 2018
,
Jun 7 2018
\o/! Just to clarify, we have the behavior logic in place under a flag, which we will roll out sometime in the near future.
,
Jun 11 2018
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 |
|||
Comment 1 by ericrobinson@chromium.org
, Mar 27 2018