search

uBlockOrigin / uBlock-issues

This is the community-maintained issue tracker for uBlock Origin
https://github.com/gorhill/uBlock
945 stars 81 forks source link

bug in chrome with abort-current-inline-script #1274

Closed Procyon-b closed 4 years ago

Procyon-b commented 4 years ago

Prerequisites

Description

A while ago I've noticed a problem with a specific type of cosmetic/js filter. I didn't mind since the browser I was using on that computer is an older version of chrome (70). But today I took the time to test on several different browsers and computers.

The problem is with https://github.com/gorhill/uBlock/wiki/Resources-Library#acisjs- When specifying a third parameter (a string or a RE), this parameter must be matched for the filter to be applied. It works as documented with FF & WF, but not in any version of chrome. In chrome it always matches successfully whatever the string.

A specific URL where the issue occurs

http://users.skynet.be/x-or/

Steps to Reproduce

  1. Try to select some text on the page. Impossible due to this code document.onselectstart =
  2. Add this filter: users.skynet.be##+js(acis, document.onselectstart, /ftrue/) Reload the page and select text. It works
  3. Modify the filter by adding garbage to the string: users.skynet.be##+js(acis, document.onselectstart, /ssfsdfsftrue/) Reload and try selecting text. It shouldn't work (OK in FF & WF), but in chrome you can select text.

Expected behavior:

A non-matching string should ignore the filter.

Actual behavior:

In chrome it always matches whatever the string.

Your environment

Yuki2718 commented 4 years ago

Completely irrelevant word such as xyz also works as the third parameter.

gorhill commented 4 years ago

The scriptlet works fine.

It's a browser quirk: when you assign a getter to onselectstart, it appears Chromium bypasses the getter when it wants to internally fire the onselectstart event (and probably any on... event), and since no valid function is assign to onselectstart, the event is left unhandled.

Procyon-b commented 4 years ago

OK, I get it now. This is problem I encountered when programming a sandbox for an extension.

Chrome doesn't like this way of setting a property. It doesn't work. You have to make a copy of the original 'getter' and 'setter' and call them to get/set a value.

Here is a proof of concept (but with the OldGetter OldSetter properties public). Tested in chrome and FF.

also available here for a short time: http://users.skynet.be/alternity/ubo-res-b.js test with users.skynet.be##+js(myacis, document.onselectstart, /ftrue/)' and 'users.skynet.be##+js(myacis, document.onselectstart, /ssdftrue/)

/// my-abort-current-inline-script.js
/// alias myacis.js
(function() {
    const target = '{{1}}';
    if ( target === '' || target === '{{1}}' ) { return; }
    const needle = '{{2}}';
    let reText = '.?';
    if ( needle !== '' && needle !== '{{2}}' ) {
        reText = /^\/.+\/$/.test(needle)
            ? needle.slice(1,-1)
            : needle.replace(/[.*+?^${}()|[\]\\]/g, '\\$&');
    }
    const thisScript = document.currentScript;
    const re = new RegExp(reText);
    const chain = target.split('.');
    let owner = window;
    let prop;
    for (;;) {
        prop = chain.shift();
        if ( chain.length === 0 ) { break; }
        owner = owner[prop];
        if ( owner instanceof Object === false ) { return; }
    }
    let value;
    let desc = Object.getOwnPropertyDescriptor(owner, prop);
    if (
        desc instanceof Object === false ||
        desc.get instanceof Function === false
    ) {
        value = owner[prop];
        desc = undefined;
    }
    const magic = String.fromCharCode(Date.now() % 26 + 97) +
                  Math.floor(Math.random() * 982451653 + 982451653).toString(36);
    const validate = function() {
        const e = document.currentScript;
        if (
            e instanceof HTMLScriptElement &&
            e.src === '' &&
            e !== thisScript &&
            re.test(e.textContent)
        ) {
            throw new ReferenceError(magic);
        }
    };
    owner.OldSetter=owner.__lookupSetter__(prop);
    owner.OldGetter=owner.__lookupGetter__(prop);
    Object.defineProperty(owner, prop, {
        get: function() {
            validate();
            return this.OldGetter();
        },
        set: function(a) {
            validate();
            this.OldSetter(a);
        }
    });
    const oe = window.onerror;
    window.onerror = function(msg) {
        if ( typeof msg === 'string' && msg.indexOf(magic) !== -1 ) {
            return true;
        }
        if ( oe instanceof Function ) {
            return oe.apply(this, arguments);
        }
    }.bind();
})();
Procyon-b commented 4 years ago

Obviously, this version of the code is specific to this test. Multiple calls to 'acis' on 'document' will overwrite backed-up getter and setter.

gorhill commented 4 years ago

__lookupSetter__ is:

Deprecated This feature is no longer recommended.

At this point, I don't see a real world issue, the provided case is to prevent the page from disabling onselectstart and oncontextmenu, which can be accomplished with users.skynet.be##+js(set, document.onselectstart, null) etc.

Procyon-b commented 4 years ago

It is deprecated for many years, but I haven't found a way to achieve the same effect (setting a value from a inside a new setter).

This is only a test case. What if filtering is needed on the string (in another case)? Scripts on a page makes multiple attempts to read a value, but we only need it to fail at one place. (or maybe it's multiple versions of the page) This case must exist, or else there is no need of a third parameter in acis.

And is acis the only resource that uses this string matching?

Edit I'll try to find another method.

gorhill commented 4 years ago

or else there is no need of a third parameter in acis.

This is your argument? Because of cases involving one set of properties we should sacrifice filtering on text segment for all other cases where it's currently working fine?

Procyon-b commented 4 years ago

I don't say that we should sacrifice anything. Your argument about __lookupSetter__ is completely valid, And I wouldn't want acis to be changed considering that.

Yuki2718 commented 4 years ago

The third parameter is working fine for other properties such as document.write. (Test case I used [NSFW]: russianbeauties.jp The third parameter is vital here.)

Procyon-b commented 4 years ago

@Yuki2718 indeed it works (did my own test too). When testing, I thought the problem was more wide than it is actually. chrome is soooooo weird.

garry-ut99 commented 1 year ago

At this point, I don't see a real world issue

Here it is, another case - an user has reported a Disqus comment section breakage, due to an acis filter which shouldn't even run because none of inline scripts at notalwaysright.com contain "admiral" text anymore: https://github.com/uBlockOrigin/uAssets/issues/17039#issuecomment-1454900669.

And even if we travel back in time where "admiral" was still present, most likely the filter was still aborting too many inline scripts, because the page contains several different inline scripts with document.createElement property.

Anyway, on the other side, it doesn't seem to be aborting all inline scripts which contains document.createElement property, because if add:

There is 118 ##+js(acis/acs, document.createElement, [value]) filters in uBlock filters, most likely some of the websites have breakages.