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

Use browser.tabs.insertCSS everywhere #248

Open
yurikhan opened this issue Nov 19, 2017 · 52 comments
Open

Use browser.tabs.insertCSS everywhere #248

yurikhan opened this issue Nov 19, 2017 · 52 comments

Comments

@yurikhan
Copy link

  • Browser: Firefox but probably not important
  • Operating System: GNU/Linux but not important

Currently, Stylus applies styles to most pages by sending them in a message to a content script which creates a style element and inserts it into the page DOM.

(An exception to that is XML documents, for which the message gets reflected back to the background script and styles are applied via the browser.tabs.insertCSS API.)

As a result of injection being performed from a content script, the page’s own scripts see the style elements and can tamper with them. As a realistic example, a slowly loading page can unintentionally overwrite the injected styles.

Taken from the security point of view, this is an information disclosure vulnerability (page script has access to the userstyle source, can leak it to the site owner to fingerprint the user) and can be a tampering vulnerability (page script can deny the user’s right to customize site appearance).

I have made a proof-of-concept page.

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<title>Test style injection detection</title>
<link rel="stylesheet" href="data:text/css,body%7Bbackground:%23faf0d7%7D" />
<script>
setInterval(function () {
    var styles = document.querySelectorAll('style');
    document.getElementById('n-styles').textContent = '' + styles.length;
    styles.forEach(function (style) {
        style.remove();
    })
}, 1000);
</script>
</head>
<body>
<p>I see <span id="n-styles">no</span> styles in this page.</p>
</body>
</html>

To reproduce:

  1. Create a global style or a style that applies specifically to that URI. E.g.:

    @-moz-document url-prefix("http://centaur.ath.cx/test-styles.html") {
    body { background: #e4ffc7 }
    }
    
  2. Navigate to that page. Observe that the page background is a pleasant light green, as specified by the user style.

  3. Wait a second. Observe that the page script notices the user style and removes it, restoring its ugly brown background.

I have loaded the apply-css example extension that demonstrates the insertCSS API, and the style it injects is not detected by the page script.

Is there any reason why insertCSS should not be used for all pages which it is able to handle?

@tophf
Copy link
Member

tophf commented Nov 19, 2017

tabs.insertCSS approach has some problems which I've described several times over the last year here and there.

  • it's effectively Firefox-only because in the original Chrome API there's no chrome.tabs.removeCSS which means:
    • we need to have a separate manifest.json for Chrome and Firefox
    • rework the entire communication code interspersed everywhere to support both approaches
  • as you can see in style-via-api.js we need to keep a copy of each injected style to be able to remove them when updating/toggling - it means on extension update all tabs will get the styles "orphaned" and Stylus, being a WebExtension, won't be able to control them and hence will inject a copy of the styles, meaning toggling won't work and style updates will be incorrect until the old tabs are reloaded.

Right now I'm not interested in the least. In the future, yeah quite possible. Maybe someone else will do it as I'm just another user and I don't visit weird sites that remove our styles or overwrite their pages entirely.

Observe that the page script notices the user style and removes it, restoring its ugly brown background

We can protect the style elements, it's neither hard nor CPU-intensive.

@yurikhan
Copy link
Author

Makes sense, thanks. I will watch for any cases of style disappearance and try to understand how it happens.

@tophf
Copy link
Member

tophf commented Nov 19, 2017

I remember when we dealt with mmo-champion.com page, which was rewritten by uBlock Extra extension via document.write, I experimented with protecting and used for that a non-recursive (hence superfast) MutationObserver on the documentElement. It turned out unnecessary so the change wasn't committed. If you find real sites (not PoC) that delete our style elements, I'll reintroduce the feature. Probably with some kind of time limit in order not to cause deadlocks with a super aggressive site script.

@tophf
Copy link
Member

tophf commented Nov 19, 2017

Another approach is to insert style code inside a ShadowDOM root on documentElement - it requires dynamic rewriting of all CSS selectors in order to add :host or something like that, also doable since we have CSS parser.

@yurikhan
Copy link
Author

It looks like there is a Chrome feature request to add removeCSS:

https://bugs.chromium.org/p/chromium/issues/detail?id=608854

@mechalynx
Copy link

@yurikhan actually, it looks like the patch for it already exists and has more or less passed review as well. It's also compatible with the Firefox implementation. Seems like this could land in Chrome any day now. Perhaps the solution is for someone representative (as in, a project contributor+) to draw attention to that bug?

@tophf
Copy link
Member

tophf commented Nov 21, 2017

There'll be another problem in Chrome 64 and on - it switched to injecting user level stylesheets instead of author level - to be able to override page !important rules - and it means that it breaks lots of injected CSS that don't use !important since user level stylesheets have lower priority as per the specification, and Chrome doesn't offer cssOrigin in tabs.insertCSS

Update: crrev.com/c/765642 adds CSSOrigin to the internals so we can hope it'll be exposed in the extensions API. Not that it solves the bigger problem with the lack of tabs.removeCSS in Chrome...

@tophf
Copy link
Member

tophf commented Nov 25, 2017

Test version that uses tabs.insertCSS everywhere in FF [edit: updated the link] fixes the issue.
Needs testing with style updates, editing, basically everything. See temporary installation article.

@silverwind
Copy link
Contributor

silverwind commented Jan 5, 2018

Gave that test version a quick try in Firefox Nightly 59.0a1 (2018-01-05):

I noticed this warning when loading the temporary addon, but the extension still worked:

Reading manifest: Error processing permissions.4: Value "declarativeContent" must either: must either [must either [be one of ["clipboardRead", "clipboardWrite", "geolocation", "idle", "notifications"], be one of ["bookmarks"], be one of ["find"], be one of ["history"], be one of ["activeTab", "tabs"], be one of ["browserSettings"], be one of ["cookies"], be one of ["topSites"], be one of ["webNavigation"], or be one of ["webRequest", "webRequestBlocking"]], be one of ["alarms", "mozillaAddons", "storage", "unlimitedStorage"], be one of ["browsingData"], be one of ["devtools"], be one of ["identity"], be one of ["menus", "contextMenus"], be one of ["pkcs11"], be one of ["geckoProfiler"], be one of ["sessions"], be one of ["contextualIdentities"], be one of ["downloads", "downloads.open"], be one of ["management"], be one of ["privacy"], be one of ["proxy"], be one of ["nativeMessaging"], be one of ["theme"], or match the pattern /^experiments(\.\w+)+$/], or must either [be one of ["<all_urls>"], match the pattern /^(https?|wss?|file|ftp|\*):\/\/(\*|\*\.[^*/]+|[^*/]+)\/.*$/, or match the pattern /^file:\/\/\/.*$/]

Updating a style seems to be partially broken in this version. When I edit a style, the page style is not updated immeditately. When I reload the page, it is unstyled on first reload but when reloading a second time, the style gets applied again.

I strongly recommend using the cssOrigin: 'user' option because that allows userstyles to outweight !important page styles, which is pretty crucial for styles to work on many sites.

@tophf
Copy link
Member

tophf commented Jan 5, 2018

@silverwind, that version is buggy and currently not developed, mainly because I don't use FF and the implementation turned out to be more complicated - Stylus has to enumerate sub-frames on each page, and IIRC there are certain bugs in various versions of FF that aren't easy to circumvent.

Error processing permissions.4: Value "declarativeContent"

You can disregard it.

I strongly recommend using the cssOrigin: 'user' option

Bad idea. Making it the default will break all styles that don't use !important because user origin precedes author styles in the cascade. There are lots of styles that don't use !important for the obvious reason (I'll cite it just in case: !important is designed to be used as an exception, not as a rule). A better solution is to expose a per-style option (or a global one) to set the origin.

@silverwind
Copy link
Contributor

Bad idea. Making it the default will break all styles that don't use !important because user origin precedes author styles in the cascade

I'm aware of that, but that's how the old Stylish for Firefox actually worked and their docs recommend putting !important everywhere, and IIRC correctly, most styles on userstyles.org use !important everywhere. But I agree, a style option might be the best way to go about it.

@tophf
Copy link
Member

tophf commented Jan 5, 2018

a style option might be the best way to go about it.

It's the only way I'll accept.

@silverwind
Copy link
Contributor

Okay. BTW, can you elborate

we need to have a separate manifest.json for Chrome and Firefox

Is this because Chrome would error out when loading the extension because it requests unknown permissions?

@tophf
Copy link
Member

tophf commented Jan 5, 2018

we need to have a separate manifest.json for Chrome and Firefox

It's an old remark, no longer relevant as I found a way to use declarativeContent in Chrome.

@silverwind
Copy link
Contributor

Let's say that I'd like to add such a option and work on using insertCSS in Firefox. Should I take current master as a starting point for that, or should I wait for whatever rewrite you're planning?

@tophf
Copy link
Member

tophf commented Jan 5, 2018

You can use both branches, but be prepared to have a hard time as there are lots of edge cases. I don't know when I'll be interested in working on that branch. I don't plan to rewrite it per se, it's just the way it goes: I already rewrote it several times because the implementation was either incorrect or I got blocked by some bug in FF. Just found I have another attempt locally so I've pushed it into insertcss2 branch.

@tophf
Copy link
Member

tophf commented Jan 5, 2018

that's how the old Stylish for Firefox actually worked and their docs recommend putting !important everywhere

Bad outdated advice based on the web in those days with lots of inline styles etc.

Anyway, Stylus is based on Stylish-for-Chrome, with its more than a million of users, which always used author origin because it inserted style elements in the page. We can't just change the way the styles are inserted based on the behavior of a technically unrelated Firefox addon, even though it was the original one. On the other hand, I understand that migrating Firefox users would expect the classic Stylish behavior. However, new users of Stylus in Firefox wouldn't necessarily expect it.

most styles on userstyles.org use !important everywhere.

I don't see that.

@silverwind
Copy link
Contributor

silverwind commented Jan 5, 2018

Actually according to the page I linked, even Stylish for Firefox was inserting author sheets, so my impression that it was doing user sheets was wrong. Anyways, I see it pretty important to allow using user and possible (if browsers ever implement it) user-agent sheets to allow style authors to deal with !important page styles. This is especially needed for global styles.

@tophf
Copy link
Member

tophf commented Jan 5, 2018

Currently Stylus adds style elements at the end of DOM so a userstyle's !important already overrides page's !important with the same specificity. Which is why I'm reluctant to work on this branch: everything is working fine already. The only realistic benefit of tabs.insertCSS is overcoming longstanding FF bugs that prevent Stylus from styling certain dynamic iframes (FF's webNavigation API doesn't "see" them). But we could reintroduce iframe observers, which I've removed in favor of match_about_blank in manifest.json that works correctly in Chrome. Another "benefit" is hiding the styles from sites, but that's a really contrived use case.

As for USER_AGENT, there's simply no method to implement it other than process the entire DOM manually. Obviously, it'll be slow.

@silverwind
Copy link
Contributor

silverwind commented Jan 5, 2018

Let me give you an example why I need user sheets. I have a global style that does

body {
  background-color: #111 !important;
}

Now take a look at the CSS of http://www.imdb.com/: they have

body#styleguide-v2 {
    background-color: #e3e2dd !important;
}

In the old Stylish, I just used a user-agent level sheet to overcome this specificity issue. In Stylus, I'd have to at least match the specificity of the id selector for every single page that's using !important on their body style. A method that obviously won't scale.

@tophf
Copy link
Member

tophf commented Jan 5, 2018

Well, USER_AGENT is out of the question.

As for the imdb example, you can simply use a specificity hack like body:not(#foo). IMHO this is on the same level of dirtiness as using !important in the first place.

@silverwind
Copy link
Contributor

silverwind commented Jan 5, 2018

you can simply use a specificity hack like body:not(#foo)

Interesting. I haven't thought about exploiting :not like that, I'll try that.

USER_AGENT is out of the question

Why thought? If we implement a select box for CSS origin and a browser supports inserting user agent sheets, why not provide the option?

@tophf
Copy link
Member

tophf commented Jan 5, 2018

tabs.insertCSS doesn't support user-agent origin. How do you plan to override a random element's inline style with !important inside? Obviously you need to find that element in DOM first, and replace its inline style. For that you'll have to use MutationObserver to be able to avoid FOUC, but observing makes the pages noticeably slower, especially if the user has other extensions monitoring the page like e.g. uBlock/AdBlock.

@silverwind
Copy link
Contributor

How do you plan to override a random element's inline style with !important inside

I meant we should only support it if the browser supports inserting user-agent sheets of course, not hacks like you mentioned. Now I just need to convince Mozilla :)

@tophf
Copy link
Member

tophf commented Jan 5, 2018

Another concern is that tabs.insertCSS with author origin doesn't have a way to specify the cascade order. Currently, we insert styles at the end of DOM so we fully control the cascade within 'author' origin and same-specificity selectors always win. I'm not sure it'll be the case with tabs.insertCSS.

@silverwind
Copy link
Contributor

tabs.insertCSS with author origin doesn't have a way to specify the cascade order

Yes, that sounds like a possible source of issues because insertCSS does not mention the location of the inserted style. I'd say anything else than "last author sheet" should be considered a bug. Maybe they should provide an option like last, first to allow more granular control.

@uBlock-user
Copy link

@mjethani It appears the patch is stuck in a merge conflict. is that the holdup ?

@mjethani
Copy link

@uBlock-user the patch has been stuck in review forever. I have asked multiple times but received no response. I have no idea what the problem is, but I am disappointed by the lack of transparency and openness.

If you would like to see this resolved, please star Chromium issue #608854 and leave a comment explaining your use case.

@uBlock-user
Copy link

@mjethani That's really disappointing to see that happen to you when you put so much effort and time into this. I have starred the issue.

@joshsleeper
Copy link

it looks like the docs have finally been clarified for Firefox at least.

bugzilla issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1428398
updated MDN docs: https://wiki.developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/tabs/insertCSS#Parameters

@narcolepticinsomniac
Copy link
Member

IIRC, Chromium is the problem. tabs.removeCSS is not supported. It's been years now.

@joshsleeper
Copy link

Right you are, sorry. I should've clarified that I was speaking specifically to this documentation request from earlier in this thread that was filed to assuage concerns about the cascade order.

@palashkaria
Copy link

palashkaria commented Jun 7, 2020

@mjethani They mention there that it's stuck in a merge conflict c45. Any updates would be useful! Thanks a lot, I just want to use this in something I'm building for myself/in an extension :)

@tophf
Copy link
Member

tophf commented Oct 1, 2020

https://crrev.com/c/2250506 finally added removeCSS based on @mjethani's code so it'll be available in Chrome 87, right now in Chromium trunk builds, and Chrome Canary in a day or two.

@krystian3w
Copy link

krystian3w commented Nov 4, 2020

No generate incompatible few CSS-es (?), inline <style> can forget use !important; in CSS / .user.css projects.

Perhaps it is better to introduce detection of:

*[style*="important"] { 
   ... : ...;
}

and then only try use tabs.insertCSS/scripting.insertCSS()?

@BoffinBrain
Copy link

I was just about to report this as a security and privacy vulnerability...

But it turns out everyone's known about this since 2017?

Surely this should be top priority.

@tophf
Copy link
Member

tophf commented Dec 27, 2024

There are many problems with this API:

  • It's asynchronous so it'll cause FOUC randomly.
  • Chrome puts the styles below document.adoptedStyleSheets in the cascade and I don't expect them to fix it anytime soon.
  • No alternative API in Chrome yet. A faster API to register CSS code exists in Firefox.
  • Neither of the alternatives matches URLs the way we do, e.g. it can't differentiate #hash part and can't handle regexps at all. AFAICT it'll never be implemented. These APIs are essentially unusable for many styles, so we would have to load them via the asynchronous API, thus introducing an inconsistency between different styles as well as random FOUCs.

I thought about hiding the style elements inside Shadow DOM, but that would limit their scope to that shadow root. There's really no good solution until browsers implement native support for userstyles the full way or at least give us something like chrome.dom.insertCSS() in the content script so we can call it synchronously at page start.

As for privacy, I've been using the web for more than 20 years and I don't share the concern in general. Maybe I still think of the internet as it were in the old days? That said, I'm blocking all 3rd-party resources on all sites by default for adblocking and improving page load speed, only unblocking manually their CDN.

@BoffinBrain
Copy link

While I'm trying my best to block third-party tracking, ultimately the presence of UserStyles in the DOM is not just a vector for fingerprinting, but it is definitely enough to uniquely identify a user (even if multiple users have the same style installed, their style IDs will be different - this is something that could be addressed in the meantime by changing the add-on code).

Even if the alternative methods of inserting CSS are imperfect, perhaps we should grant users the option to use them at their own discretion and get feedback about their efficacy.

Do we have any contacts we can leverage at Mozilla to get these problems addressed in Firefox? I'll consider Chrome users to be a lost cause as far as privacy is concerned.

@tophf
Copy link
Member

tophf commented Jan 3, 2025

I'll implement an alternative solution when the minimally necessary scripting.registerContentScripts for CSS strings is implemented in Chrome (w3c/webextensions#615). It'll be incorrect for some sites due to the cascade problem, but I'll report that after the API itself is implemented, which may take years judging by the way Chromium team prioritizes things (they spent months on implementing the UI to disable ManifestV2 while doing nothing to fix the actual bugs in ManifestV3, which still has them in droves).

@BoffinBrain
Copy link

Chrome is a lost cause. Can we get some optional features added to Firefox?

@tophf
Copy link
Member

tophf commented Jan 3, 2025

Personally I'm not that interested because I haven't been using Firefox for 10+ years, but I'll reconsider if Chromium team continues its slide into delusion with its ManifestV3 direction. I switched from Firefox to Chrome when Mozilla decided to change its architecture for addons and broke most of extensions I used back then. Ironically Google does almost the same now.

@tophf
Copy link
Member

tophf commented Jan 4, 2025

FWIW an opt-in solution is to enable [x] Circumvent CSP 'style-src' via adoptedSty­leSheets in the options.

tophf added a commit that referenced this issue Jan 4, 2025
+ simplify `code` back to a string
@BoffinBrain
Copy link

FWIW an opt-in solution is to enable [x] Circumvent CSP 'style-src' via adoptedSty­leSheets in the options.

I gave this a try and everything seems to be working just fine, with the style tags not appearing in the DOM. Looks good. What's going on behind the scenes and what are the caveats?

@tophf
Copy link
Member

tophf commented Jan 5, 2025

Well, it's a new awkward API that doesn't have proper notifications for changes so if the page or another extension modifies the applied styles we can't tell it without overriding the code in the page, which is pretty invasive, doesn't work if another extension intercepted it earlier and has thrown an exception, also it adds overhead. The designer of the API explicitly doesn't care about extensions in general and observation use case in particular, but that's probably because I couldn't resist mocking him for the ridiculous idea of hijacking global prototypes just to track changes.

Note that the currently published Stylus still exposes the installed id inside the data that can be seen by a web page script. I've disabled it by default in 5313867 which is not yet published and can be tested as a temporary extension.

@BoffinBrain
Copy link

BoffinBrain commented Jan 9, 2025

so if the page or another extension modifies the applied styles we can't tell

I don't see this being an issue, since the page shouldn't be able to see or modify UserStyles anyway. I can't imagine needing another extension to manage UserStyles in addition to Stylus, but YMMV on that one.

One more quick question: is there a way to get Stylus to load images outside of the current domain with this mode enabled? Even data: URLs embedded in the style will not work. If that can't be done right now, perhaps we can extend the feature to punch more holes in the CSP based on URLs seen in the CSS, or some kind of metadata variable for allowed domains?

@tophf
Copy link
Member

tophf commented Jan 9, 2025

  1. This is about adoptedStyleSheets API which is visible to the page and other extensions by design
  2. There is an option to patch CSP in Stylus, but it allows all external URLs for images, not the ones actually used in the style. Enabling it in ManifestV3 requires additional setup as explained in the info beside the option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests