Closed pilgrim-brave closed 3 years ago
Hi @pilgrim-brave - would you be able to help QA by devising a mini test-plan to put in https://github.com/brave/brave-core/pull/7952, for us to key off?
I see the cases in https://github.com/brave/brave-core/blob/34f21675d82ed24168d057b69735ffb5f47cab07/browser/brave_shields/domain_block_page_browsertest.cc; if those would work for us, to test manually, can you help take a few examples of them and distill them into step-by-step tests?
And can you confirm they are in Adblock Plus filter format via brave://adblock
(https://adblockplus.org/filter-cheatsheet)?
Thanks! 🙏
(ccing: @brave/legacy_qa and setting QA/Blocked
, just until we're able to sync up on a good test-plan for this 🤜 🤛 )
Verified PASSED
with the following simple steps from https://github.com/brave/brave-core/pull/7952#pullrequestreview-594597262 and https://github.com/brave/brave-core/pull/7952#issuecomment-790643804, with build
Brave | 1.23.41 Chromium: 89.0.4389.90 (Official Build) nightly (x86_64) |
---|---|
Revision | 62eb262cdaae9ef819aadd778193781455ec7a49-refs/branch-heads/4389@{#1534} |
OS | macOS Version 11.2.3 (Build 20D91) |
1-1ads.com
and others, below, from the list https://pgl.yoyo.org/adservers/serverlist.php?hostformat=adblockplus&showintro=1&mimetype=plaintext
Suspicious site ahead
warnings for each1-1ads.com | actionsplash.com | adapt.tv | pub.chez.com | zzhc.vnet.cn |
---|---|---|---|---|
Enable domain blocking
to Disabled
1-1ads.com
and others from the list https://pgl.yoyo.org/adservers/serverlist.php?hostformat=adblockplus&showintro=1&mimetype=plaintext
Suspicious site ahead
warning, as abovebrave://flags
and toggled Enable domain blocking
from Default
to Disabled
1-1ads.com
, adapt.tv
, zzhc.vnet.cn
and othersConfirmed I got no interstitial pages, and was served the site (or not) as it exists/doesn't.
1-1ads.com | actionsplash.com | adapt.tv | pub.chez.com | zzhc.vnet.cn |
---|---|---|---|---|
Verification passed on
Brave | 1.23.56 Chromium: 89.0.4389.105 (Official Build) dev (64-bit)
-- | --
Revision | 14f44e21a9d539cd49c72468a29bfca4fa43f710-refs/branch-heads/4389_90@{#7}
OS | Windows 10 OS Version 2004 (Build 19041.867)
Verified PASSED
with the following simple steps from https://github.com/brave/brave-core/pull/7952#pullrequestreview-594597262 and https://github.com/brave/brave-core/pull/7952#issuecomment-790643804
Enable domain blocking
= Default
1-1ads.com
and others, below, from the list https://pgl.yoyo.org/adservers/serverlist.php?hostformat=adblockplus&showintro=1&mimetype=plaintext
Suspicious site ahead
warnings for each1-1ads.com | actionsplash.com | adapt.tv | pub.chez.com | zzhc.vnet.cn |
---|---|---|---|---|
Enable domain blocking
= Disabled
1-1ads.com
and others from the list https://pgl.yoyo.org/adservers/serverlist.php?hostformat=adblockplus&showintro=1&mimetype=plaintext
Suspicious site ahead
warning, as abovebrave://flags
and toggled Enable domain blocking
from Default
to Disabled
1-1ads.com
, adapt.tv
, zzhc.vnet.cn
and othersConfirmed I got no interstitial pages, and was served the site (or not) as it exists/doesn't.
1-1ads.com | actionsplash.com | adapt.tv | pub.chez.com | zzhc.vnet.cn |
---|---|---|---|---|
Verified passed with
Brave 1.23.63 Chromium: 89.0.4389.114 (Official Build) beta (64-bit)
Revision 1ea76e193b4fadb723bfea2a19a66c93a1bc0ca6-refs/branch-heads/4389@{#1616}
OS Linux
Used https://github.com/brave/brave-core/pull/7952#pullrequestreview-594597262 and https://github.com/brave/brave-core/pull/7952#issuecomment-790643804 as guide as above.
1-1ads.com
and others, below, from the list https://pgl.yoyo.org/adservers/serverlist.php?hostformat=adblockplus&showintro=1&mimetype=plaintext
Suspicious site ahead
warnings for each1-1ads.com | actionsplash.com | adapt.tv | pub.chez.com | zzhc.vnet.cn |
---|---|---|---|---|
Enable domain blocking
to Disabled
1-1ads.com
and others from the list https://pgl.yoyo.org/adservers/serverlist.php?hostformat=adblockplus&showintro=1&mimetype=plaintext
Suspicious site ahead
warning, as abovebrave://flags
and toggled Enable domain blocking
from Default
to Disabled
1-1ads.com
, adapt.tv
, zzhc.vnet.cn
and othersConfirmed I got no interstitial pages, and was served the site (or not) as it exists/doesn't.
1-1ads.com | actionsplash.com | adapt.tv | pub.chez.com | zzhc.vnet.cn |
---|---|---|---|---|
Howdy @karenkliu , @antonok-edm noticed that there are different designs used in a previous version of this issue (https://github.com/brave/brave-browser/issues/8559). Just wanted to check if the designs in this version all look 👍 (and the designs in #8559 are out of date) or if changes are needed here
@pes10k It's the reverse; the designs in this version are out of date. It should look like this:
Desktop:
Mobile:
Missing front-end engineering support on this. We still haven't done the umbrella issue for interstitial pages: https://github.com/brave/brave-ios/issues/483 That's why the designs in this version are out of date too.
I see, thanks @karenkliu. Would it be okay to go forward with the current, implemented design, and then update the UI for this feature in the future, we the "update all the interstitials" issue is tackled?
@pes10k NO! 😠 . Just kidding 😆 Yeah, this has to be the approach for all the design system-related debt that needs to be tackled one piece at a time. Implemented design seems fine for now.
@pes10k noticed the umbrella issue you referenced is for iOS (https://github.com/brave/brave-ios/issues/483) and there is https://github.com/brave/brave-browser/issues/7464 for Android, but is there one for desktop?
@LaurenWags I don't think there is currently a separate issue for desktop. @karenkliu, do you know if there is there a similar plan to revamp the desktop interstitials too?
@pes10k @LaurenWags The interstitials should be the same across all platforms. I believe they're done in plain HTML/CSS so Android and desktop can share https://github.com/brave/brave-browser/issues/7464 ?
cool, thanks @karenkliu - when I looked at https://github.com/brave/brave-browser/issues/7464 it didn't have the OS/Desktop
label and I didn't want to assume everything would be the same.
@LaurenWags I think it was just missed - thanks for checking - I added the label just now!
Verification passed on OnePlus 6T with Android 10 running 1.23.63 x64 build
Enable domain blocking
set to DefaultEnable domain blocking
set to DisabledVerification passed on Samsung Tab A with Android 10 running 1.23.63 x64 build
Enable domain blocking
set to DefaultEnable domain blocking
set to Disabledre-labeling as release-notes/exclude
as this feature has been turned off in Release Channel with https://github.com/brave/brave-browser/issues/15149
@LaurenWags do you know when are we wanting this to "ship"? (ex: would we potentially include release notes for this in the future?)
I know we can flip on using variations - but there may be some other work we'd like to include before it's live across the board (ex: https://github.com/brave/brave-browser/issues/15189). Wasn't sure if there was a date attached
@bsclifton apologies on the delay replying here, but the current plan is:
This is also tracked on this board: https://github.com/brave/brave-browser/projects/41#card-58260630
Hope thats all clarifying, at least a bit, happy to explain / spec / say more too if it'd be helpful :)
Has this been introduced in iOS as well? 🤔
It works for me on Linux desktop but it doesn't work on iOS.
@marekciupak yes, this is currently only supported on Desktop and Android, though support on iOS is planned for this year
@pes10k thank you for a quick answer! I really appreciate that. Do you know if there any place where I can follow the progress of introducing it to iOS? Any issue or ticket?
Other third-party blocking tools allow filter list authors to block the top-level, first-party request. This is useful when a page is overall harmful, but doesn’t fit SafeBrowsing’s threat model. It’s also useful as a defense-in-depth against phishing, bounce tracking, etc.
Brave currently does not have this capability. We don’t currently have a flexible way of saying “this page shouldn’t be loaded / given first-party storage”. The current way of doing this is SafeBrowsing (which we don’t control / fork) or rules that still load the page, but block all sub resources (i.e.
https://*$domain=evil.org
). Neither of these provide the security and privacy benefits of blocking the initial page load (e.g. inline scripts, bounce tracking, etc).An implementation should