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

Shields should be robust against invalid CSS in stylesheet injections #40177

Closed
antonok-edm opened this issue Jul 31, 2024 · 7 comments · Fixed by brave/brave-core#24954 or brave/brave-core#25015
Assignees
Labels
bug feature/shields/adblock Blocking ads & trackers with Shields OS/Android Fixes related to Android browser functionality OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA/In-Progress Indicates that QA is currently in progress for that particular issue QA Pass - Android ARM QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-All-Platforms QA/Yes release-notes/include

Comments

@antonok-edm
Copy link
Collaborator

A recent change to uBO filter lists resulted in the following two cosmetic filters being shipped out to Brave users:

##html[lang] > body#body > head\" > div.cv-xwrapper > div.cvc > div.cv-inner
##html[lang] > body#body > head\" > div.cvh.BlockClicksActivityBusy

Users who received those rules faced some issues with cosmetic filtering, due to errors thrown by Brave's injected content script. The issue was ultimately resolved by a serverside patch to remove the problematic rules.

An error was shown in the console for affected users:

Uncaught DOMException: Failed to execute 'insertRule' on 'CSSStyleSheet': Failed to parse the rule '##html[lang] > body#body > head\" > div.cv-xwrapper > div.cvc > div.cv-inner{display:none !important;}'

That error ultimately traces to this insertRule call. It turns out that invalid CSS injected here can abort the rest of the injections.

We should add a try/catch block around the injection to improve robustness against invalid data.

@antonok-edm antonok-edm added bug feature/shields/adblock Blocking ads & trackers with Shields OS/Android Fixes related to Android browser functionality OS/Desktop labels Jul 31, 2024
@antonok-edm antonok-edm self-assigned this Jul 31, 2024
@antonok-edm
Copy link
Collaborator Author

see also #40151 (comment)

@LaurenWags
Copy link
Member

LaurenWags commented Aug 28, 2024

Verified with

Brave | 1.70.95 Chromium: 128.0.6613.114 (Official Build) beta (x86_64)
-- | --
Revision | 58a39a29fce4db0417a269766b0157fad397767b
OS | macOS Version 13.6.9 (Build 22G830)

Verified test plan from brave/brave-core#25015.
Confirmed no messages in the console about Uncaught DOMException: Failed to execute 'insertRule' on 'CSSStyleSheet'.

Example Example
Screenshot 2024-08-30 at 10 23 39 AM Screenshot 2024-08-30 at 10 25 11 AM

@LaurenWags
Copy link
Member

@brave/qa-team use the test plan under brave/brave-core#25015 for verifying this issue 👍🏻

@LaurenWags LaurenWags added QA Pass-macOS and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Aug 30, 2024
@GeetaSarvadnya GeetaSarvadnya added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Sep 4, 2024
@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Sep 4, 2024

Verification PASSED on

Brave | 1.70.94 Chromium: 128.0.6613.114 (Official Build) beta (64-bit)
-- | --
Revision | 7ee7ec28264896bd8a9aff39cdb9432da38be028
OS | Windows 10 Version 22H2 (Build 19045.4780)

Verified test plan from brave/brave-core#25015.
Confirmed no messages in the console about Uncaught DOMException: Failed to execute 'insertRule' on 'CSSStyleSheet'.

Example Example
image image

@GeetaSarvadnya GeetaSarvadnya added QA Pass-Win64 and removed QA/In-Progress Indicates that QA is currently in progress for that particular issue labels Sep 4, 2024
@hffvld
Copy link
Contributor

hffvld commented Sep 10, 2024

This issue also affects Android devices, but because we don't have access to Profile folder on our test devices, we're unable to verify it right now.

@GeetaSarvadnya GeetaSarvadnya added the QA/In-Progress Indicates that QA is currently in progress for that particular issue label Sep 13, 2024
@GeetaSarvadnya
Copy link

Verification PASSED on

Brave	1.70.110 Chromium: 128.0.6613.138 (Official Build) beta (64-bit) 
Revision	7dec99ca278e09ce2087e9f04914f0aa2507ea4c
OS	Linux

Verified test plan from brave/brave-core#25015.
Confirmed no messages in the console about Uncaught DOMException: Failed to execute 'insertRule' on 'CSSStyleSheet'.

Example Example
image image

@hffvld
Copy link
Contributor

hffvld commented Sep 17, 2024

Verified on rooted Pixel 6 using version(s):

Device/OS: Pixel 6 / oriole-user 14 AP2A.240905.003.F1 release-keys
Brave build: 1.70.114
Chromium: 129.0.6668.42 (Official Build) (64-bit)

STEPS:

  1. Follow the steps from Update adblock-rust to v0.8.12 brave-core#25015 (comment)
  2. Verify

ACTUAL RESULTS:

  • Verified that no messages in the console about Uncaught DOMException: Failed to execute 'insertRule' on 'CSSStyleSheet'.

1 2 3
1 2 3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature/shields/adblock Blocking ads & trackers with Shields OS/Android Fixes related to Android browser functionality OS/Desktop priority/P2 A bad problem. We might uplift this to the next planned release. QA/In-Progress Indicates that QA is currently in progress for that particular issue QA Pass - Android ARM QA Pass-Linux QA Pass-macOS QA Pass-Win64 QA/Test-All-Platforms QA/Yes release-notes/include
Projects
None yet
6 participants