Opened 5 years ago

Closed 9 months ago

Last modified 8 months ago

#242 closed defect (fixed)

Element hiding filters can be overridden by the element's style attribute

Reported by: mapx Assignee: mjethani
Priority: P3 Milestone: Adblock-Plus-3.0.3-for-Chrome-Opera-Firefox
Module: Platform Keywords: externaldependency, circumvention
Cc: saroyanm, sebastian, fanboy, arthur, scuturic, Lain_13, mjethani, hfiguiere, hayato, kochi, SMed79, BrentM, weissmar Blocked By:
Blocking: #6507 Platform: Chrome
Ready: no Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29564767/

Description (last modified by sebastian)

How to reproduce

  • Search for something on Bing (also see #4425)
  • Visit any article on The Telegraph (also see #4171)
  • Search for something on torrentz.eu (no longer reproducible, website seems to be broken)
  • Visit any web page that uses style="dislay: block !important" (or alike) to avoid element hiding

Observed behaviour

If a web page sets the display CSS property of an element using an !important rule applied via the element's style attribute, it takes precedence over element hiding filters applied by Adblock Plus, potentially preventing ads from being hidden.

That is because element hiding in Adblock Plus for Chrome (as well as on Safari and Microsoft Edge, but unlike on Firefox) is implemented using an author stylesheet, i.e. a regular <style> element which is injected into the (shadow) DOM.

Expected behaviour

Element hiding filters should take precedence over any styles applied by the web page, including those applied using the style attribute. Adblock Plus for Firefox currently achieves that by utilizing user stlesheets. This mechanism, however, is currently not supported by Chrome, see Chrome bug 632009.

Change History (59)

comment:1 Changed 5 years ago by mapx

another example:
go to http://www.drakulastream.eu/tennis-live-streaming-video.html
choose a match (flash, not unibet where you must have an account), a window opens with the top bar:
"Update your OnlineHD for Chrome in order to watch this video online"

in firefox the bar is hidden, but not in chrome

comment:2 Changed 5 years ago by mapx

  • Description modified (diff)

comment:3 Changed 5 years ago by mapx

  • Description modified (diff)

comment:4 Changed 5 years ago by trev

This isn't something we can fix as long as we are using regular <style> tags for element hiding. We should check whether chrome.tabs.insertCSS does things better by now and inserts a real user stylesheet. But even if it does we'll still have to file a Chromium issue - InjectDetails should allow specifying a frame ID rather than "all frames" only.

Last edited 5 years ago by trev (previous) (diff)

comment:5 Changed 5 years ago by trev

  • Cc trev added

comment:6 Changed 5 years ago by philll

  • Keywords externaldependency added
  • Priority changed from P2 to P3
  • Ready set

comment:7 Changed 5 years ago by trev

Just checked, chrome.tabs.insertCSS will inject a proper user stylesheet. However, !important keyword is ignored for user stylesheets (https://code.google.com/p/chromium/issues/detail?id=120428). The issue has been incorrectly resolved, it seems that we need to file a new one.

I couldn't find an existing issue on frameId support in chrome.tabs.insertCSS, we need to file that one as well.

comment:8 Changed 5 years ago by mapx

the chromium devs are not very open to such approach:
https://code.google.com/p/chromium/issues/detail?id=347016

comment:9 Changed 5 years ago by trev

That issue is irrelevant here - it's explicitly about user stylesheet files that users put into their profiles manually, not about extensions which stay supported.

comment:10 Changed 5 years ago by mapx

  • Description modified (diff)

comment:11 Changed 5 years ago by Spam404

I was directed here by "mapx" on the Adblock Plus forum. Here is what I posted on the forum -

"I was recommended to this forum to report this issue as a bug on the following thread - http://forums.lanik.us/viewtopic.php?f=62&t=16614

It may be best to read the thread first to get an idea of what I was trying to get filtered and how the bug was initially discovered.

In a nutshell I am trying to filter the following element on this page - http://astucespourlesjeuxmobiles.blogspot.co.uk/

[img]http://i.gyazo.com/2bb37909542eab5f4c0f1b04e0327535.png[/img]

I am trying to do this by using a social filter list by EasyList.

The problem is that the element is being hidden on Firefox but not Google Chrome."

comment:12 Changed 5 years ago by arthur

@Spam404
According to the screenshot in the EasyList forum you are using AdBlock which is a different extension.

However, I can reproduce your issue in Adblock Plus 1.7.4 as well. It seems to be fixed in the development build: https://adblockplus.org/en/development-builds

@mapx
This doesn't seem to be the same issue as the one reported by you. I don't see any "display: block !important;" in the source and it is still happening in the dev build.

comment:13 Changed 5 years ago by mapx

@arthur
here: http://forums.lanik.us/viewtopic.php?p=56583&sid=74642d8293634c3571fe813f20974add#p56583
I can see a lot of (9) !important keywords and as trev said: "!important keyword is ignored for user stylesheets"

Another thing:
first you said: "It seems to be fixed in the development build"
then: "it is still happening in the dev build"

It's true, the dev build seems to work well with the filter:

astucespourlesjeuxmobiles.blogspot.co.uk##.share-controls.delay

comment:14 Changed 5 years ago by arthur

@mapx
You're right. There are some "display: block !important;" in the source. I only checked this element:

<div class="share-controls delay" data-defer="true" data-delay="1000">

When I said "it is still happening in the dev build", I was referring to the original issue reported by you on 1 April.

comment:15 Changed 4 years ago by saroyanm

  • Cc manvel@… added
  • Platform set to Unknown

comment:16 Changed 4 years ago by philll

  • Platform changed from Unknown to Firefox

comment:17 Changed 4 years ago by philll

  • Platform changed from Firefox to Chrome
  • Summary changed from chrome: hiding filters do not work to chrome: hiding filters do not work at http://torrentz.eu/search?f=cow

comment:18 Changed 4 years ago by sebastian

Also see #1253

comment:19 Changed 4 years ago by sebastian

  • Cc saroyanm sebastian added; manvel@… removed

I filed a Chrome issue: https://crbug.com/426337

But even if they don't add a frame option that allows us to inject user stylesheets into a specific frame, we could still use the current version of chrome.tabs.injectCSS() to inject CSS into the top level frame, while keep using content scripts to inject <style> elements into subframes.

Last edited 4 years ago by sebastian (previous) (diff)

comment:20 Changed 4 years ago by sebastian

I filed another Chrome bug, to let user stylesheets override author stylesheets with !important directive: https://crbug.com/427527

comment:21 Changed 4 years ago by sebastian

  • Resolution set to invalid
  • Status changed from new to closed

Apparently Blink doesn't support user stylesheets. Stylesheets injected with chrome.tabs.injectCSS() are merely author stylesheets, and therefore aren't supposed to override other author stylesheets with !important priority. So the solution suggested above isn't an option.

Last edited 4 years ago by sebastian (previous) (diff)

comment:22 Changed 2 years ago by fhd

  • Tester set to Unknown

Dave filed https://bugs.chromium.org/p/chromium/issues/detail?id=632009# earlier this year, that should fix this, how I see it. But so far the Chromium people have ignored it. If lots of people star it, that might help a little.

comment:24 Changed 2 years ago by mapx

  • Cc fanboy added

comment:25 Changed 2 years ago by arthur

  • Cc arthur added

comment:26 Changed 2 years ago by scuturic

  • Cc scuturic added

comment:27 Changed 2 years ago by sebastian

  • Description modified (diff)
  • Ready unset
  • Resolution incomplete deleted
  • Sensitive set
  • Status changed from closed to reopened
  • Summary changed from chrome: hiding filters do not work at http://torrentz.eu/search?f=cow to Element hiding filters can be overridden by the element's style attribute

Since this circumvention method is becoming popular again, I reopened this issue and updated the description. Also I made the issue confidential, surprisingly this wasn't done back then, despite this providing an exploitable circumvention technique.

Currently, our plan is to see what's going to happen with Chrome bug 632009, in which we requested chrome.tabs.insertCSS() to use user stylesheets, so that we can migrate to that API, instead of injecting an author stylesheet, which will be even less effective once shadow DOM v0 gets removed from Chrome. Then again, due to the upcoming deprecation of shadow DOM v0, the Chromium team seem to consider prioritizing that issue now (also see #4713).

Alternatively, we could (theoretically) emulate element hiding filters, partially reimplementing CSS ourselves, so that we can operate on the nodes to be hidden rather than using a stylesheet. That is essentially what uBlock is doing. But this approach is a bit controversial, as the implementation tends to be quite complex and might potentially have a negative performance effect in particular on large pages and pages that change a lot dynamically. On the other hand it could also help reducing the memory usage, like it does in uBlock. But then again, we'd still mess with the DOM, in a merely slightly harder to circumvent way. Not to mention, that as more as we mess with the actual page content (e.g. through the DOM) as higher the legal risk we are facing. So this might only be a last resort, if at all.

comment:28 Changed 22 months ago by mapx

  • Cc Lain_13 added

comment:29 Changed 22 months ago by Lain_13

Another EH circumvention #4945.

comment:30 Changed 22 months ago by kzar

  • Component changed from Unknown to Platform

comment:31 Changed 19 months ago by fanboy

Possible to get some movement on this? Atleast an interim solution until Chrome has a proper fix.

comment:32 Changed 17 months ago by mjethani

Can you add me to the CC list for this one?

comment:33 Changed 17 months ago by mapx

  • Cc mjethani added

comment:34 Changed 17 months ago by Lain_13

Since ​implementation of Chrome bug 632009 is still in theoretical phase and this bug is open for more than 3 years already then I'd suggest to implement it as special case of custom styles (#756). Script which will apply inline style must check presence of display property in the style attribute and remove it if it going to apply the same property. The really tricky part is to make sure it _stays_ since some sites use MutationObserver to make sure display:block!important wasn't removed. So, it's important to wrap getAttribute/setAttribute in order to modify style before applying it to specific elements and return original string if site script want to check it. It might be tricky to keep it consistent with changes which came from style. ... properties. Also, it might be useful to wrap style property of the object into a Proxy or modify getter/setter for display property to return block for style.display like it's still there.

comment:35 Changed 17 months ago by mapx

  • Cc hfiguiere added

comment:36 Changed 15 months ago by sebastian

  • Cc hayato added

comment:37 Changed 15 months ago by sebastian

  • Cc kochi added

comment:38 Changed 14 months ago by mjethani

  • Review URL(s) modified (diff)

comment:39 Changed 13 months ago by mapx

It seems (perhaps not for all the pages) the !important keyword can be defeated converting the normal hiding filters in procedural filters (that's why Arthur added such filters in easylist)

for example a normal filter: jalopnik.com##.logo (hiding the logo / title on jalopnik.com)
does not work in chrome, but his procedural brother is working: jalopnik.com#?#.logo

perhaps the reason is:
"#?# is telling ABP to use querySelectorAll internally rather than just a CSS rule"

Last edited 13 months ago by mapx (previous) (diff)

comment:40 Changed 13 months ago by mapx

  • Cc SMed79 added

comment:41 Changed 13 months ago by Lain_13

perhaps the reason is:
"#?# is telling ABP to use querySelectorAll internally rather than just a CSS rule"

No exactly. Procedural hide modified style property of element to "display: none !important" which have highest priority in Google Chrome. It also monitors changes in the style property and returns it back to that value if site attempted to change it. Basically it utilizes the same trick, just to hide rather to show.

Kind of fighting fire with fire. Nice find, though!

Last edited 13 months ago by Lain_13 (previous) (diff)

comment:42 Changed 12 months ago by fhd

  • Cc trev removed

comment:43 Changed 10 months ago by abpbot

A commit referencing this issue has landed:
Issue 242 - Use user style sheets on Chromium

comment:44 Changed 10 months ago by mjethani

This should be fixed now on Chrome Canary 66.0.3335.0 for regular element hiding filters. It still does nothing for emulation filters, but we'll get there.

comment:45 Changed 10 months ago by abpbot

A commit referencing this issue has landed:
Issue 242 - Do not disable user style sheets on unrelated errors

comment:46 Changed 9 months ago by mjethani

  • Keywords circumvention added

comment:47 Changed 9 months ago by sebastian

  • Milestone set to Adblock-Plus-3.0.3-for-Chrome-Opera-Firefox

comment:48 Changed 9 months ago by mjethani

The cssOrigin option to tabs.insertCSS is now part of Google Chrome. It's available in both the Canary and Dev channel (version 66) builds, but not yet in the Beta channel (version 65) build. This means if all goes well, if there are no critical issues found, then it'll go into beta next week (March 15) and stable 40 days from today.

On the Adblock Plus side, we are detecting the availability of this feature and taking advantage of it if available. This already works with the Adblock Plus development build running on Google Chrome Canary and Google Chrome Dev channel.

I am closing this issue now. If there are any bugs or other issues found related specifically to this topic, they should be reported separately.

If the changes to Chromium are rolled back for any reason (still possible!), I'll reopen this issue and work on it again.

Last edited 9 months ago by mjethani (previous) (diff)

comment:49 Changed 9 months ago by mjethani

  • Owner set to mjethani

comment:50 Changed 9 months ago by mjethani

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:51 Changed 9 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done and working. Chrome now hides elements with inline styles that try to prevent hiding as Firefox does. Tested in Chrome Canary 67 as the current Chrome Beta still on 65 at the moment.

ABP 3.0.2.1983
Firefox 59 / Windows 7
Chrome Canary 67 / Windows 7

comment:52 Changed 9 months ago by mjethani

Chrome 66 Beta is out finally.

We have also switched over to user style sheets for element hiding emulation on platforms that have tabs.removeCSS (currently only Firefox).

comment:53 Changed 9 months ago by mjethani

  • Blocking 6507 added

comment:54 Changed 8 months ago by sebastian

  • Cc BrentM weissmar added

comment:55 Changed 8 months ago by sebastian

  • Sensitive unset

comment:56 Changed 8 months ago by kochi

Hi,

I'm contacting in the context for deprecating Shadow DOM V0 for real for Chrome.

As you said earlier that this is the blocking issue of migrating ABP
out of Shadow DOM V0, and now the user stylesheet feature is implemented and
is shipping soon in stable (M66), and I thoght it means ABP will not use
Shadow DOM V0 at all, if the feature is detected.?

I tested with my local build (M67), and it seems Adblock plus is still using
the V0 Element.createShadowRoot() API.

We would like to hear your plan about removing the usage of V0 shadow in ABP.

Eventually we would like to remove the feature in April 2019 (one year from now),
and would like to start showing deprecation message soon. Does the timeline
work for you?

comment:57 Changed 8 months ago by mapx

It seems the devs already removed ShadowRoot
see
https://issues.adblockplus.org/ticket/6441

comment:58 Changed 8 months ago by sebastian

Yes, Adblock Plus (as of version 3.0.3) should no longer use Shadow DOM (neither v0 nor v1) on Chrome 66 and above. Are sou sure that you tested with Adblock Plus 3.0.3 (or alternatively with the current development build)? Also did you tweak your user agent string by any chance (since we cannot detect the availability of user stylesheets from the content script, we have rely on the user agent string in order to avoid injecting a shadow root on Chrome 66 and above)? Otherwise can you elaborate how exactly you concluded that createShadowRoot() is still being used?

comment:59 Changed 8 months ago by kochi

Thanks for your quick replies!

It seems somehow my local installation of ABP got stuck at older version,
and removing & reinstalling updated it to 3.0.3.

Now I don't see the deprecation message. The message is shown on
Element.createShadowRoot() call (on my local build only now, but
once the change lands (in M67+) it will appear in end users' Chrome.

And thanks again for your effort to make the user stylesheets extension API!

Note: See TracTickets for help on using tickets.