Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#4191 closed defect (fixed)

mail.ru shadowRoot stylesheet circumvention

Reported by: Lain_13 Assignee: kzar
Priority: P2 Milestone: Adblock-Plus-1.12.2-for-Chrome-Opera-Safari
Module: Platform Keywords:
Cc: sebastian, kzar, greiner, mapx, Ross, scheer Blocked By:
Blocking: Platform: Chrome
Ready: yes Confidential: no
Tester: Scheer Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29348917/
https://codereview.adblockplus.org/29349749/
https://codereview.adblockplus.org/29349787/

Description (last modified by kzar)

Environment

Google Chrome 51.0.2704.103 with Adblock Plus for Chrome 1.12.1.

How to reproduce

  1. Disable all filter subscriptions and filters. Add the subscription "RuAdList+EasyList" and the filter e.mail.ru##.b-nav
  2. Sign up for a mail.ru account: https://e.mail.ru/signup?from=login
  3. Navigate to your inbox: https://e.mail.ru/messages/inbox/

Observed behaviour

The menu on the left is hidden, but after a few seconds it becomes visible again.

Expected behaviour

The menu on the left should stay hidden.

Notes

  • Running document.body.parentNode.shadowRoot.styleSheets[0].rules.length returns ~139 before the menu reappears but 0 afterwards.

Hints for testers

  • Ensure element hiding still works.
  • Follow the "How to reproduce" steps and ensure the problem no longer happens.
  • Make sure YouTube advert blocking for older versions of Safari still works. (That a regression like #4141 didn't happen again.)
  • Make sure WebSocket blocking still works #1727.

Change History (35)

comment:1 Changed 3 years ago by mapx

  • Cc sebastian kzar greiner mapx added

comment:2 Changed 3 years ago by mapx

example pages ?

comment:3 Changed 3 years ago by Lain_13

Oh, accidentally pasted wrong text instead of URL and haven't noticed that.
For example, here: https://news.mail.ru/politics/26214113/
Look for objects with class "rb-slot".

Last edited 3 years ago by Lain_13 (previous) (diff)

comment:4 Changed 3 years ago by mapx

I guess it's about the old issue (or ABP approach with hiding filters) with !important styles

comment:5 Changed 3 years ago by mapx

"it's not important" userscript does not work either on this kind of !important stuff

comment:6 Changed 3 years ago by mapx

  • Component changed from Unknown to Platform
  • Platform changed from Unknown / Cross platform to Chrome

comment:7 Changed 3 years ago by Lain_13

There are no style attribute in the first place. At least on the '.rb-slot' blocks. Also, I can see in the page inspector how at the moment when scripts loads whole block of ABPs styles disappears from the list of applied styles and "STYLE" element in the shadow root blinks like something changed in it.

Last edited 3 years ago by Lain_13 (previous) (diff)

comment:8 Changed 3 years ago by Lain_13

Looks like since recently they also dynamically add STYLE trick to everything that got hidden. Not sure if they still attempting to change stylesheets since "It's not important" now enough to counter this issue. However, when I tried to change blocks position with Stylish styles were removed. So, it doesn't seems like they gave up on the idea. Just replaced it with a more reliable solution. BTW, instead of disabling they are removing styles from the style.sheet list.

comment:9 Changed 3 years ago by Lain_13

Today I encountered this behavior in an unexpected place and this time it was done without using a script loaded through websocket.
Here: https://e.mail.ru/messages/inbox
One user requested to hide tooltips and I suggested to use mail.ru##.b-tooltip
The filter worked for me... but only for a ~5 seconds if you refresh a page. Then all ABP styles attached to the element disappears.
It's the same if you try to hide anything on that page. For example, that menu on the left:
mail.ru##.b-navitem - the menu will disappear, but it will reappear in ~5 seconds and all ABP styles will be deleted.

Try to paste this into console: document.body.parentNode.shadowRoot.styleSheets[0].rules.length as start it shows number of rules created by ABP, but later on it changes to zero.

Last edited 3 years ago by Lain_13 (previous) (diff)

comment:10 Changed 3 years ago by kzar

Attempting to reproduce this in order to triage the issue but so far haven't managed.

I disabled all filters + subscriptions and then added the following filters:

mail.ru##.js-article
mail.ru###portal-menu

I then browsed to https://news.mail.ru/politics/26214113/ and the article's title and the page menu was hidden like I expected. I waited for longer than 5 seconds and the hidden elements did not reappear. I refreshed the page a few more times and still did not witness the elements reappearing.

Could you guys help provide me with some concrete steps I can follow to reproduce the problem? (Preferably without requiring a mail.ru account.)

comment:11 Changed 3 years ago by Lain_13

As I told they returned to WebSocket trick there.
Also, as I told I've encountered this script in a different place:
https://e.mail.ru/messages/inbox (you need to have an account there)
Try to use filter: e.mail.ru##.b-nav
The page will load without a menu on the left, but it will became visible again in a few seconds.
Not sure why they removed it from all of their pages, but emails, but the code is still there and works.

Also, as I told above, to see that styles were removed you have to perform this code:
document.body.parentNode.shadowRoot.styleSheets[0].rules.length
Right after page load and then after menu being restored. In second case it will return 0.
Actually, in my case I don't even need to add custom filters. They remove all the filters added by ABP anyway.

Last edited 3 years ago by Lain_13 (previous) (diff)

comment:12 Changed 3 years ago by Lain_13

Try to use this one: abp_test_email / ABP!123

Last edited 3 years ago by Lain_13 (previous) (diff)

comment:13 Changed 3 years ago by kzar

  • Description modified (diff)
  • Summary changed from mail.ru, large news, games and all the other stuff portal in Runet, manipulates ABP's stylesheet to mail.ru manipulates ABP's stylesheet

Thanks, I've updated the issue description to include those steps.

Unfortunately I still can't reproduce the problem however, when using both our latest code and Adblock Plus for Chrome 1.12.1 the menu stays hidden. (I'm running on Chrome Version 51.0.2704.106 (64-bit) which is pretty close to what you're using.)

comment:14 Changed 3 years ago by Lain_13

I've decided to make a video: https://youtu.be/9iyVFB7hCaY

Last edited 3 years ago by Lain_13 (previous) (diff)

comment:15 Changed 3 years ago by mapx

in the video I can see easylist, ruadlist enabled.

comment:16 Changed 3 years ago by Lain_13

Oh! Have you tried to reproduce it only with that example filter? Sorry, I had to mention that I've used extra lists. I just used that filter as some visible example. That "feature" isn't necessary triggered by that filter... Most likely they check all stylesheets for some known filters. As I told above I can see how number of ABP rules drops to zero even without that example filter.

comment:17 Changed 3 years ago by kzar

  • Description modified (diff)
  • Priority changed from Unknown to P2
  • Ready set
  • Summary changed from mail.ru manipulates ABP's stylesheet to mail.ru shadowRoot stylesheet circumvention

OK finally managed to reproduce this, thanks for your patience. (Interestingly this does not seem to be a problem for Safari, couldn't reproduce on Safari 9.1.2.)

comment:18 Changed 3 years ago by kzar

The website is removing our style element, but only when we've used the Shadow DOM. That explains why I couldn't reproduce the problem on Safari 9.1.2.

We actually already handle this situation and put the style element back again. The weird part is that when this happens the style element's sheet is replaced with a different empty one:

var shadowRoot = document.body.parentNode.shadowRoot;
var style = shadowRoot.childNodes[1];
var sheet = style.sheet;

shadowRoot.removeChild(style);

window.setTimeout(function()
{
  console.log(sheet == style.sheet, sheet.rules.length, style.sheet.rules.length);
}, 0);

=> false 87 0

I've just tested as far back as Chrome 30 and this always seems to happen, so I'm not sure how the stylesheet reinjection logic was ever effective. I guess I'm still missing something... will continue to investigate.

comment:19 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

Well I have a fix working, I simply put all the rules back again if they're gone after the stylesheet is reinjected! (Perhaps there's a smarter way to go, so looking forward to hear what Sebastian thinks.)

Either way this was a good catch, thanks for the filing the issue!

comment:20 Changed 3 years ago by Lain_13

Is there no way to prevent removal of filters in the first place?

comment:21 Changed 3 years ago by Lain_13

[removed: accidental doublepost]

Last edited 3 years ago by Lain_13 (previous) (diff)

comment:22 Changed 3 years ago by kzar

  • Owner set to kzar

comment:23 Changed 3 years ago by Lain_13

If I understand right this commit: https://hg.adblockplus.org/adblockpluschrome/rev/20c6e8db2f5c
had to fix this issue as well. At least code supposed to prevent meddling with ABP stylesheets were updated there. However, I've tested this with "Adblock Plus development build 1.12.1.1631" and the issue is still there.

comment:24 Changed 3 years ago by mapx

I don't see the changes here: https://codereview.adblockplus.org/29348917/diff/29348918/include.preload.js

in the commit above.

comment:25 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 4191 - Protect our shadowRoot

comment:26 Changed 3 years ago by kzar

  • Cc Ross scheer added
  • Description modified (diff)
  • Milestone set to Adblock-Plus-for-Chrome-Opera-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:27 Changed 3 years ago by Lain_13

Tested on 1632. Works for me, thanks.

comment:28 Changed 3 years ago by kzar

Ace good to hear, thanks for letting us know :)

comment:29 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)

comment:30 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 4191 - Check shadowRoot getter exists before wrapping

comment:31 Changed 3 years ago by mapx

typo:

https://hg.adblockplus.org/adblockpluschrome/file/6aef9aa013a8/include.preload.js#l475

conifgurable: true, enumerable: true, get: function()

comment:32 Changed 3 years ago by kzar

Dang, thanks well spotted. Will open a review and fix that shortly.

comment:33 Changed 3 years ago by kzar

  • Review URL(s) modified (diff)

comment:34 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 4191 - Fix typo in shadowRoot descriptor

comment:35 Changed 3 years ago by scheer

  • Tester changed from Unknown to Scheer
  • Verified working set
  • Ads do not load on youtube.com or youtube.ru as per the previous regression with #4141
  • The filter now correctly blocks the left sidebar permanently in mail.ru.
  • Element hiding works correctly on www.msn.com and www.yahoo.com.
  • Websocket blocking still functions on opensubtitles.org. Sites with WebSockets load correctly, such as http://www.twitch.tv and http://www.pwnwin.com/dashboard.

ABP 1.12.1.1644
Chrome 30
Chrome 51
Opera 38
Safari 9
Safari 6

Note: See TracTickets for help on using tickets.