stylus icon indicating copy to clipboard operation
stylus copied to clipboard

Use browser.tabs.insertCSS everywhere

Open yurikhan opened this issue 6 years ago • 41 comments

  • 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?

yurikhan avatar Nov 19 '17 13:11 yurikhan

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.

tophf avatar Nov 19 '17 14:11 tophf

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

yurikhan avatar Nov 19 '17 15:11 yurikhan

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 avatar Nov 19 '17 15:11 tophf

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.

tophf avatar Nov 19 '17 15:11 tophf

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

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

yurikhan avatar Nov 20 '17 20:11 yurikhan

@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?

mechalynx avatar Nov 20 '17 21:11 mechalynx

~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 avatar Nov 21 '17 05:11 tophf

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.

tophf avatar Nov 25 '17 01:11 tophf

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.

silverwind avatar Jan 05 '18 12:01 silverwind

@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.

tophf avatar Jan 05 '18 12:01 tophf

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.

silverwind avatar Jan 05 '18 12:01 silverwind

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

It's the only way I'll accept.

tophf avatar Jan 05 '18 12:01 tophf

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?

silverwind avatar Jan 05 '18 12:01 silverwind

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.

tophf avatar Jan 05 '18 12:01 tophf

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?

silverwind avatar Jan 05 '18 12:01 silverwind

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 avatar Jan 05 '18 12:01 tophf

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.

tophf avatar Jan 05 '18 12:01 tophf

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.

silverwind avatar Jan 05 '18 13:01 silverwind

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.

tophf avatar Jan 05 '18 13:01 tophf

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.

silverwind avatar Jan 05 '18 13:01 silverwind

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.

tophf avatar Jan 05 '18 13:01 tophf

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?

silverwind avatar Jan 05 '18 14:01 silverwind

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.

tophf avatar Jan 05 '18 14:01 tophf

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 :)

silverwind avatar Jan 05 '18 14:01 silverwind

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.

tophf avatar Jan 05 '18 15:01 tophf

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.

silverwind avatar Jan 05 '18 15:01 silverwind

Now I just need to convince Mozilla :)

I think this discussion needs to be brought up on https://discourse.wicg.io/ to get feedback.

Mottie avatar Jan 05 '18 16:01 Mottie

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

In practice, Firefox behaves sanely. My injected styles consistenly win over page styles.

yurikhan avatar Jan 05 '18 16:01 yurikhan

In practice, Firefox behaves sanely. My injected styles consistenly win over page styles.

Uhm, this is too vague without describing whether the specificity was identical, whether !important was used, and where. We need guarantees. My point is that without explicitly stated behavior in the WebExtensions API documentation we can't just blindly trust current behavior of tabs.insertCSS as it may change unpredictably due to some browser developer's whim.

tophf avatar Jan 05 '18 16:01 tophf

Oh fine. I filed a documentation request for that.

yurikhan avatar Jan 05 '18 17:01 yurikhan