Skip to content
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

For blocked URLs, we should do 200 status code instead of redirects #2554

Closed
bbondy opened this issue Dec 13, 2018 · 7 comments · Fixed by brave/brave-core#1377
Closed

For blocked URLs, we should do 200 status code instead of redirects #2554

bbondy opened this issue Dec 13, 2018 · 7 comments · Fixed by brave/brave-core#1377

Comments

@bbondy
Copy link
Member

bbondy commented Dec 13, 2018

Description

We currently redirect to empty data URLs (or empty image data URLs for images).
We should instead be responding with 200 status code because data URLs can cause CSP violations.

I believe this will help with general webcompat no a large scale.

See also:
https://chromium.googlesource.com/chromium/src/+/master/net/docs/life-of-a-url-request.md

Test plan

See #2554 (comment)

@bbondy bbondy added the webcompat/shields Shields is breaking a website. label Dec 13, 2018
@NejcZdovc NejcZdovc added this to the 1.x Backlog milestone Jan 2, 2019
@mrzealot
Copy link

Just mentioning this here as well: like #2931 shows, there can be problems with not blocking, too, if the script thinks that it actually managed to load something that we blocked. Possibly these are rarer than the problems solved by not blocking, I don't have numbers, just an FYI.

@iefremov
Copy link
Contributor

@mrzealot Yeah, that makes sense. We are not going to quickly push this change to stable, I think we would rather treat this as an experiment.

@srirambv
Copy link
Contributor

Moving issue to 0.62.x based on brave/brave-core#1377 set milestone. @iefremov please add QA/Yes or QA/No labels accordingly

@srirambv srirambv modified the milestones: 1.x Backlog, 0.62.x - Nightly Jan 29, 2019
@iefremov
Copy link
Contributor

@srirambv thanks for pinging me, added QA/Yes

@btlechowski
Copy link

@iefremov Could you include a test plan in brave/brave-core#1377?

I have no idea how to test this one.

@iefremov
Copy link
Contributor

@btlechowski

  1. Open Devtools
  2. Open any website with lots of ads, e.g. https://www.foxnews.com/
    3a. Before this change, in a DevTools console one can see a lot of entries like Access to XMLHttpRequest at 'data:text/plain,'
    3b. After this change, these entries should vanish.
  3. There are also no entries like 'data:text/plain,' on a Network tab
  4. Ads are blocked as expected

@btlechowski
Copy link

btlechowski commented Mar 1, 2019

Thanks @iefremov !

Verification passed on

Brave 0.62.10 Chromium: 73.0.3683.39 (Official Build) dev (64-bit)
Revision cc53b0e12fcaf42e4bab8d6c23bd4fb7aae99f6c-refs/branch-heads/3683@{#413}
OS Windows 7 Service Pack 1 Build 7601.24312

Used test plan from #2554 (comment)
Checked several sites.

image

Verification passed on

Brave 0.62.26 Chromium: 73.0.3683.75 (Official Build) beta (64-bit)
Revision 909ee014fcea6828f9a610e6716145bc0b3ebf4a-refs/branch-heads/3683@{#803}
OS Linux

Verified passed with

Brave 0.62.31 Chromium: 73.0.3683.75 (Official Build) beta(64-bit)
Revision 909ee014fcea6828f9a610e6716145bc0b3ebf4a-refs/branch-heads/3683@{#803}
OS Mac OS X

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment