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

Cosmetic filtering issue with Shadow DOM/tree/root #803

Closed
8 tasks
gwarser opened this issue Nov 30, 2019 · 33 comments
Closed
8 tasks

Cosmetic filtering issue with Shadow DOM/tree/root #803

gwarser opened this issue Nov 30, 2019 · 33 comments
Labels
enhancement New feature or request fixed issue has been addressed

Comments

@gwarser
Copy link

gwarser commented Nov 30, 2019

Details

Prerequisites

  • I verified that this is not a filter issue
  • This is not a support issue or a question
  • I performed a cursory search of the issue tracker to avoid opening a duplicate issue
    • Your issue may already be reported.
  • I tried to reproduce the issue when...
    • uBlock Origin is the only extension
    • uBlock Origin with default lists/settings
    • using a new, unmodified browser profile
  • I am running the latest version of uBlock Origin
  • I checked the documentation to understand that the issue I report is not a normal behavior

Description

  1. Generic cosmetic filters don't hide nodes in Shadow DOM. Probably not injected to the page because nodes "in shadow" are not detected.
  2. Specific filters works, but are not logged.
    • This does not work in ABP/AdBlock.

A specific URL where the issue occurs

https://www.virustotal.com/gui/home/upload

Steps to Reproduce

"Generic" case:

  1. Add ###euConsent to My filters
  2. Open linked VirusTotal page

"Specific" case:

  1. Add virustotal.com###euConsent to My filters
  2. Open linked VirusTotal page

Expected behavior:

Cookie notice on the bottom should be hidden with generic filter. Filter should be logged.

Actual behavior:

Notice is hidden only by specific filter. No DOM filters are logged.

Your environment

  • uBlock Origin version: 1.23 on Chrome, 1.24.3b1 on Nightly, 1.24 on 70.0.1 Fx
  • Browser Name and version:
  • Operating System and version: Manjaro KDE

@krystian3w, it's side effect of uBO performance/memory optimizations. ABP injects all cosmetic filters unconditionally (AFAIK). uBO injects only specific filters. For generic filters, DOM is scanned and only matching ones are injected. Here Shadow DOM is interfering.

@gorhill
Copy link
Member

gorhill commented Nov 30, 2019

Well just to be clear, uBO will never be converted to inject minimum of 20K+ CSS rules in every page and every frame of every page. So if a specific filter is the only solution then the resolution is "by design".

@krystian3w

This comment has been minimized.

@uBlock-user
Copy link
Contributor

uBlock-user commented Nov 30, 2019

Add ###euConsent to My filters

That CSS selector will only be present in the DOM if you connect via EU IP, do you have one that is present for all ?

Nevermind it's present in the DOM on my end too.

mapx- added a commit to uBlockOrigin/uAssets that referenced this issue Nov 30, 2019
@gorhill
Copy link
Member

gorhill commented Nov 30, 2019

I am not in a position to check myself... Is this Shadow DOM v1 or v2?

@gwarser
Copy link
Author

gwarser commented Nov 30, 2019

Personally for me this issue by itself is (partial) solution as the documentation (keywords in title).

This page (VirusTotal) is build on Shadow DOM so for testing you can choose any element you want. Picker sees only vt-virustotal-app.

@uBlock-user
Copy link
Contributor

Yes picker cannot dig on that page either beyond vt-virustotal-app

@uBlock-user
Copy link
Contributor

###share is shown in the logger if added.

@gwarser
Copy link
Author

gwarser commented Nov 30, 2019

I don't see #share even in DevTools.

@gwarser
Copy link
Author

gwarser commented Nov 30, 2019

Is this Shadow DOM v1 or v2?

Do you mean "v0 or v1"? I need to learn about it first.

@uBlock-user
Copy link
Contributor

uBlock-user commented Nov 30, 2019

I don't see #share even in DevTools.

image

document.querySelector('#euConsent') returns null, probably because it's inside the shadowDOM.

@gorhill
Copy link
Member

gorhill commented Nov 30, 2019

One solution I can think of is to make use of ABP's semantic, #?#, in which case uBO would put the cosmetic filter in the highly generic bin and thus would inject it unconditionally on each page/frame. Of course, not to be abused if we go down that route.

@gorhill
Copy link
Member

gorhill commented Nov 30, 2019

Do you mean "v0 or v1"?

Yes.

@gwarser
Copy link
Author

gwarser commented Nov 30, 2019

This g is inside svg image, Chrome does not show this.

make use of ABP's semantic, #?#

This is not a solution. Filter authors will be forced to adjust filters, does not matter if they make them specific or modify syntax in different way. Individual cases will need to be found and fixed. It will be rare/surprising now, when not many such cases exist, and eventually later when SD will become popular, all filters will need to be (will be) rewritten. There is no pattern: "#euConsent will in 80% be inside shadow dom"

@gorhill
Copy link
Member

gorhill commented Nov 30, 2019

[oops, finger twitched and closed by mistake]

I don't understand your argument. If indeed #euConsent become widespread and part of Shadow DOM (maybe by mean of JS library), how is #?##euConsent not useful? Yes, it needs to be revisited but it allows to no longer hunt for where it's used in order to keep track of specific sites using it.

@gorhill gorhill closed this as completed Nov 30, 2019
@gorhill gorhill reopened this Nov 30, 2019
@gwarser
Copy link
Author

gwarser commented Nov 30, 2019

If indeed #euConsent become widespread and part of Shadow DOM (maybe by mean of JS library),

👍, here it will work. And I even see example - content of Twitter widgets (DandelionSprout looked for this DandelionSprout/adfilt#7 (comment))


https://hub.filterlists.com/t/circumvention-via-shadow-root/34

@uBlock-user uBlock-user added the enhancement New feature or request label Nov 30, 2019
@liamengland1
Copy link

liamengland1 commented Dec 3, 2019

Another site using a shadow root DOM: https://yandex.ru/pogoda/new-york. They are using it to mess with element-pickers (I think) and make their ads harder to block. If you don't see the ads refresh the page.

@gorhill
Copy link
Member

gorhill commented Feb 13, 2020

Issue can be reproduced using #mainContent selector, so no need to get a EU IP address to reproduce.

Weeks ago I have investigated a javascript-based solution and it does not work -- there is no way to querySelector across shadow DOM boundaries and adding code to work around this is anything but trivial.

So back then I locally added code to interpret #?##mainContent as "bypass surveyor and inject unconditionally the selector". But I keep having second thoughts about this and the changes have been lingering in my local dev build.

However, today I realize that *###mainContent does also fix the issue with no code change (i.e. this would work immediately in current stable release), so I am wondering whether this should simply be used rather than introducing the new syntax above. We could simply define * as hostname to mean "bypass surveyor and inject unconditionally" and revise the documentation and warn this should only be only when needed.

gorhill added a commit to gorhill/uBlock that referenced this issue Feb 14, 2020
A specific cosmetic filter of the following form...

    *##.selector

... will be unconditionally injected into all web pages,
whereas a cosmetic filter of the form...

    ##.selector

... would be injected only when uBO's DOM surveyor finds
at least one matching element in a web page.

The new specific-generic form will also be disabled when a
web page is subject to a `generichide` exception filter,
since the filter is essentially a generic one -- the only
difference from the usual generic form is that the filter
is injected unconditionally instead of through the DOM
surveyor.

Specific-generic cosmetic filters will NOT be discarded
when checking the "Ignore generic cosmetic filters"
option in the "Filter lists" pane -- since the purpose
of this option is primarily to disable the DOM surveyor.

Specific-generic cosmetic filters should be used
parcimoniously and only when using a normal specific
filter is really impractical.

Related issue:
- uBlockOrigin/uBlock-issues#803
@uBlock-user
Copy link
Contributor

uBlock-user commented Feb 16, 2020

Can new syntax like #?##.selector support specific hostnames ? Because filters like *##.selector can be added in any third-party filterlist for fixing some website that a user like me would never visit and wouldn't feel comfortable knowing the performance issue that brings at hand, so having such filters be able to target only specific hostnames would rather be preferable.

@gorhill
Copy link
Member

gorhill commented Feb 16, 2020

performance issue that brings at hand

Please no unfounded speculation of "performance issue", this is not helpful. If you want to make the case of "performance issue", this needs to be done with profiling results.

@uBlock-user
Copy link
Contributor

By performance issue, I meant the filter is applied on every webpage on every website, doesn't that account for some performance cost ?

@krystian3w
Copy link

And EasyList / EasyList Cookie (easylist/easylist#4232) will not use:

*##ELEMENT

for uBO only...

@gorhill
Copy link
Member

gorhill commented Feb 16, 2020

doesn't that account for some performance cost ?

uBO already injects over 750+ selectors unconditionally on every page with default configuration1 -- the "highly generic ones" -- out of a total of nearly 21,000 cosmetic filters.

The change here just allows to explicitly force such unconditional injection for some filters which must be injected everywhere but yet can't be injected conditionally through the DOM surveyor.

I expect filter list authors will use the new feature intelligently, the same way the use of regex-based network filters are discouraged, yet they are not forbidden and filter list authors know that they should be used only when there is no other solution. Same approach applies here -- though regex-based network filters is a very bad comparison performance-wise, unconditionally injecting extra CSS selectors should be considered a no-op).

Also I will likely consider #821 to be solved by the solution implemented here.


[1] 1,400+ with Fanboy Social enabled out of a total of 32,600+.

@uBlock-user
Copy link
Contributor

Still we would have some control if it was specific to a hostname instead of getting applied on every website regardless of whether the element is present in the DOM or not.

@gorhill
Copy link
Member

gorhill commented Feb 16, 2020

if it was specific to a hostname instead of getting applied on every website

Generic filters which apply everywhere are already supported and unconditional injection is already occurring. There is no good rational reason to worry about the change here. I will leave it at that, I can't prevent people from being unduly anxious about non-issues, I have no control over that.

@uBlock-user uBlock-user added the fixed issue has been addressed label Feb 16, 2020
@gwarser
Copy link
Author

gwarser commented Feb 16, 2020

@uBlock-user what you are talking about? Specific filters still work.

@gorhill how about pont 2:

Specific filters works, but are not logged.

?

@krystian3w
Copy link

krystian3w commented Feb 16, 2020

No visible in logger (on table)? No everyone use dom tree debugger "</>".

@gorhill
Copy link
Member

gorhill commented Feb 16, 2020

Point 2 is "wontfix" -- this would require to find all nodes with a shadow DOM (which requires traversing all nodes -- there is no native function to get all shadow DOM nodes), then search independently for a match in all these nodes, along with installing a mutation observer for each of those nodes to find out changes withing the shadow DOMs. I decline to make the code more complex than it already is.

@uBlock-user
Copy link
Contributor

uBlock-user commented Feb 16, 2020

what you are talking about? Specific filters still work.

@gwarser It was about *##.selector, it can't be made specific. You can disable that filter at specific sites at best.

@h1z1
Copy link

h1z1 commented Apr 7, 2020

FWIW you can add pornhub to the list. They have an annoying banner that cannot afaik be blocked because it uses a shadow root.

If you're not getting it the banner itself is random. It was for corvid. Probably spent more time then was healthy trying to block it but I was curious wtf they were doing.

@krystian3w
Copy link

krystian3w commented May 25, 2020

ghide can break:

@@*$ghide
*##.selector

this is good behavior?


in legacy try:

@@*$generichide
github.com,##.selector

@gorhill
Copy link
Member

gorhill commented May 26, 2020

this is good behavior?

Not sure. I lean toward yes. Is this an issue in a list?

@krystian3w
Copy link

IMO possible revert uBlockOrigin/uAssets@0d78532 due easylist/easylist@d3530c0

@mapx-
Copy link
Contributor

mapx- commented Feb 4, 2023

uBlockOrigin/uAssets@8714e19

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed issue has been addressed
Projects
None yet
Development

No branches or pull requests

8 participants
@gorhill @gwarser @mapx- @liamengland1 @h1z1 @uBlock-user @krystian3w and others