-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Revert redirect-url feature #20892
Comments
@ShivanKaul mind helping us with a test plan here? How does the following look, as a start? Steps:
Confirm we don't replace requests (using internal redirects?), and those resources return a cc @brave/legacy_qa |
@stephendonner I think the QA plan would be reverse of #18542 (comment). I can send the DAT file again. What you mentioned in ^ sounds perfect, but given the deployed filter list it would be sufficient to check only the rules on https://raw.githubusercontent.com/brave/adblock-lists/master/brave-lists/brave-sugarcoat.txt i.e. check https://dell.com for Added this test plan to the PR. |
@ShivanKaul please do send me the Question: So we're not completely removing this, correct? (Given you pointed to rules for |
We are removing the feature where the sugarcoat replacement resource would be loaded remotely. Local replacements would still work i.e. for dell.com and live.house.gov, you shouldn't see it loaded via pcdn.brave.com, but just be a 200 loaded locally. This is a screenshot of what it should look like: For all the other resources in #18542 (comment), no replacement at all should be happening, local or via PCDN (but that's just testing our brave component updater at this point). |
Verified
|
Brave | 1.36.107 Chromium: 99.0.4844.45 (Official Build) (x86_64) |
---|---|
Revision | edbc0b8343c7b10fddb0e1b4efb280b0f6e38cab-refs/branch-heads/4844@{#788} |
OS | macOS Version 11.6.3 (Build 20G415) |
Case 1: no adblock-redirect
in brave://flags
Steps:
- installed
1.36.107
- launched Brave
- loaded
brave://flags
- typed
adblock-redirect
Confirmed no match for adblock-redirect
Case 2: regression-testing of removal
Steps:
- installed
1.36.107
- launched Brave
- shut down
- copied
resources.json
andrs-ABPFilterParserData.dat
to/Users/stephendonner/Library/Application Support/BraveSoftware/Brave-Browser/cffkpbalmllkdoenhmdmpbkajipdjfam/1.0.1228
- relaunched Brave
- clicked on
hamburger
menu, choseMore tools
->Developer Tools
- clicked on the
Network
pane - loaded
reuters.com
- loaded
amazon.jobs
- loaded
fedex.com
- loaded
sky.it
Confirmed no async-sugarcoat-{hex}.js
redirects for various ad/tracking resources.
reuters.com |
amazon.jobs |
fedex.com |
sky.it |
---|---|---|---|
Case 3: sugarcoat inline script replacement
Steps:
- installed
1.36.107
- launched Brave
- shut down
- copied
resources.json
andrs-ABPFilterParserData.dat
to/Users/stephendonner/Library/Application Support/BraveSoftware/Brave-Browser/cffkpbalmllkdoenhmdmpbkajipdjfam/1.0.1228
- relaunched Brave
- clicked on
hamburger
menu, choseMore tools
->Developer Tools
- clicked on the
Network
pane - loaded
https://shivankaul.com/adblock-redirect-test.html
- clicked on
hn.js
- clicked on the
Preview
pane
Confirmed inline script replacement
1.36.107 | 1.35.104 |
---|---|
Verified
Case 1: no
|
reuters.com |
amazon.jobs |
fedex.com |
sky.it |
---|---|---|---|
Case 3: sugarcoat inline script replacement
Steps:
- installed
1.36.107
- launched Brave
- shut down
- copied
resources.json
andrs-ABPFilterParserData.dat
to~/.config/BraveSoftware/Brave-Browser/cffkpbalmllkdoenhmdmpbkajipdjfam/1.0.1229
- relaunched Brave
- clicked on
hamburger
menu, choseMore tools
->Developer Tools
- clicked on the
Network
pane - loaded
https://shivankaul.com/adblock-redirect-test.html
- clicked on
hn.js
- clicked on the
Preview
pane
Confirmed inline script replacement
1.36.107 | 1.35.103 |
---|---|
Description
Revert $redirect-url feature added by #18542.
The intention was to allow resource replacements over the network if there's an adblock filter match. In other words, the redirect-filter option in a filter rule instructs the browser to load a replacement resource for the matching resource via a URL instead of locally - this was to be mainly used by Sugarcoated resources.
However, we've decided to not go ahead with loading adblock-related resources over the network and instead rely on component-bundled resources. Given that we don't plan on using redirect-url feature in the near-term, we should remove it to reduce security and bug surface. More details (including QA) in #18542.
@stephendonner
The text was updated successfully, but these errors were encountered: