New issue
Advanced search Search tips

Issue 752575 link

Starred by 1 user

Issue metadata

Status: WontFix
Owner:
Closed: Mar 2018
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

subresource_filter: add safeguards to prevent bad configurations

Project Member Reported by csharrison@chromium.org, Aug 4 2017

Issue description

We should try our best not to move forward with configurations that are bad or lead to wrong activation.

We could remove offending configurations at runtime this occurs.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 17 2017

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

commit 826deb2b00f0b13fc8d2c9e8e53d5df0cdf12ec6
Author: Charles Harrison <csharrison@chromium.org>
Date: Thu Aug 17 17:55:06 2017

[subresource_filter] Refactor: persist matching configuration

This CL starts persisting the matching Configuration in whole in
the driver factory rather than the activation options. This will be
necessary for future changes which want to know which configuration's
ActivationConditions triggered blocking.

Bug:  752575 
Change-Id: I7d4f71ef25c5957c84e2e425700ca9dc91ae0426
Reviewed-on: https://chromium-review.googlesource.com/602374
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495214}
[modify] https://crrev.com/826deb2b00f0b13fc8d2c9e8e53d5df0cdf12ec6/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
[modify] https://crrev.com/826deb2b00f0b13fc8d2c9e8e53d5df0cdf12ec6/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
[modify] https://crrev.com/826deb2b00f0b13fc8d2c9e8e53d5df0cdf12ec6/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
[modify] https://crrev.com/826deb2b00f0b13fc8d2c9e8e53d5df0cdf12ec6/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.h

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 23 2017

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

commit 2bf5d7200067f017046f6e3c0bd327d2bcb39df5
Author: Charles Harrison <csharrison@chromium.org>
Date: Wed Aug 23 15:04:15 2017

[subresource_filter] Refactor: Add ShowUI method to the Chrome client

This CL should change no behavior, except for additionally logging
a histogram when blocking ads via devtools forced activation.

Bug:  752575 
Change-Id: I52990931c6dd3a6132beb9657d139501fbcad93e
Reviewed-on: https://chromium-review.googlesource.com/623887
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#496687}
[modify] https://crrev.com/2bf5d7200067f017046f6e3c0bd327d2bcb39df5/chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
[modify] https://crrev.com/2bf5d7200067f017046f6e3c0bd327d2bcb39df5/chrome/browser/subresource_filter/chrome_subresource_filter_client.h
[modify] https://crrev.com/2bf5d7200067f017046f6e3c0bd327d2bcb39df5/chrome/browser/subresource_filter/subresource_filter_unittest.cc
[modify] https://crrev.com/2bf5d7200067f017046f6e3c0bd327d2bcb39df5/tools/metrics/histograms/enums.xml

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 29 2017

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

commit 8964f0453ba2d5f69e49800a416ac7f3833201cb
Author: Charles Harrison <csharrison@chromium.org>
Date: Tue Aug 29 18:04:54 2017

[subresource_filter] Simple client refactor

This patch splits ToggleNotificationVisibility into two methods, one
for showing the UI and one for notifying the client of a new navigation.

This is a precursor for a CL which sends along the Configuration that
triggered the UI showing to the Client.

Bug:  752575 
Change-Id: Ifec5e15ad9e7e016f18f70dd7fd65dc16f30c278
Reviewed-on: https://chromium-review.googlesource.com/638950
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498165}
[modify] https://crrev.com/8964f0453ba2d5f69e49800a416ac7f3833201cb/chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
[modify] https://crrev.com/8964f0453ba2d5f69e49800a416ac7f3833201cb/chrome/browser/subresource_filter/chrome_subresource_filter_client.h
[modify] https://crrev.com/8964f0453ba2d5f69e49800a416ac7f3833201cb/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
[modify] https://crrev.com/8964f0453ba2d5f69e49800a416ac7f3833201cb/components/subresource_filter/content/browser/subresource_filter_client.h
[modify] https://crrev.com/8964f0453ba2d5f69e49800a416ac7f3833201cb/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle_unittest.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 31 2017

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

commit ddc925215f41ae9fb301416ce5d9d01785ddedf1
Author: Charles Harrison <csharrison@chromium.org>
Date: Thu Aug 31 20:42:18 2017

[subresource_filter] Make forced activation part of Configuration

The core reason for why this change makes sense is that it means
that the subresource filter throttle no longer has to mutate the
configuration anymore. This will enable downstream consumers to
check to assume any configuration that caused activation is either:
1. A configuration in our global configuration list
2. Forced activation

The only behavior change in this CL is to make devtools activation more
consistent. Rather than using the matched config (or an empty config),
and setting ENABLED on it, we always use a brand new config which simply
sets activation level ENABLED.

Bug:  752575 
Change-Id: I21f55f03570973adfd349d04be6819a5c276f30e
Reviewed-on: https://chromium-review.googlesource.com/609181
Commit-Queue: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Shivani Sharma <shivanisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498987}
[modify] https://crrev.com/ddc925215f41ae9fb301416ce5d9d01785ddedf1/chrome/browser/subresource_filter/chrome_subresource_filter_client.cc
[modify] https://crrev.com/ddc925215f41ae9fb301416ce5d9d01785ddedf1/chrome/browser/subresource_filter/subresource_filter_unittest.cc
[modify] https://crrev.com/ddc925215f41ae9fb301416ce5d9d01785ddedf1/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc
[modify] https://crrev.com/ddc925215f41ae9fb301416ce5d9d01785ddedf1/components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h
[modify] https://crrev.com/ddc925215f41ae9fb301416ce5d9d01785ddedf1/components/subresource_filter/content/browser/subresource_filter_safe_browsing_activation_throttle.cc
[modify] https://crrev.com/ddc925215f41ae9fb301416ce5d9d01785ddedf1/components/subresource_filter/core/browser/subresource_filter_features.cc
[modify] https://crrev.com/ddc925215f41ae9fb301416ce5d9d01785ddedf1/components/subresource_filter/core/browser/subresource_filter_features.h
[modify] https://crrev.com/ddc925215f41ae9fb301416ce5d9d01785ddedf1/components/subresource_filter/core/browser/subresource_filter_features_unittest.cc

Status: WontFix (was: Assigned)
Rather than doing this I think it just makes sense to encode everything we are planning on shipping to a normal feature flag. We did this e.g. for Ad tagging.

Sign in to add a comment