Some Scriptlet Injections Filters [from uBlock] can/will crash Brave and not work correctly, because they are not parsed correctly in Brave's Adblocker
Opened this issue · 6 comments
Hi @antonok-edm
I found out this issue, because my browser started crashing.
It has to do with this replace-note-text / rpnt filter which was being ignored in Brave, but it was added to Brave-Unbreak , and since I sideload the Trusted Scriptlets, then the problem started happening, which is good, since it can be fixed before it happens to everyone.
If not, it will cause issues or any future scriptlet injection filters with a syntax like that, when Trusted Scriptlets get finally allowed in scriptlets.js, so I think it is better to try to fix it and figure it out to avoid any future issues.
As you can see by the rule added in brave-unbreak
##+js(rpnt, script, /(\(function serverContract\(\)|writeEmbed|var _F_cssRowKey|importantYT)/, 'const pruner=text=>{let json=JSON.parse(text);for(k of["playerAds","adPlacements","adSlots"]){json[k]=[];}return JSON.stringify(json);};const urlFromArg=arg=>{if(typeof arg==="string"){return arg;}if(arg instanceof Request){return arg.url;}return String(arg);};const realFetch=window.fetch;window.fetch=new Proxy(window.fetch,{apply:function(target,thisArg,args){if(!(urlFromArg(args[0]).includes("player?key="))){return Reflect.apply(target,thisArg,args);}return realFetch(...args).then(realResponse=>realResponse.text().then(text=>new Response(pruner(text),{status:realResponse.status,statusText:realResponse.statusText,headers:realResponse.headers,})));}});window.XMLHttpRequest.prototype.open=new Proxy(window.XMLHttpRequest.prototype.open,{apply:async(target,thisArg,args)=>{if(!(urlFromArg(args[1]).includes("player?key="))){return Reflect.apply(target,thisArg,args);}thisArg.addEventListener("readystatechange",function(){if(thisArg.readyState!==4){return;}const type=thisArg.responseType;if(type!==""&&type!=="text"){return;}const textin=thisArg.responseText;const textout=pruner(textin);if(textout===textin){return;}Object.defineProperty(thisArg,"response",{value:textout});Object.defineProperty(thisArg,"responseText",{value:textout});});return Reflect.apply(target,thisArg,args);}}); $1')
uBlock implemented implemented the way to add single quote symbols between values, and therefore, the Replacement value is written as 'const pruner=text=>{let json=JSON.parse(text).....' which means, they avoid having to escape commas.
The problem is Brave sees every non-escaped comma as a new argument and then it crashes the browser, because many arguments will crash the browser, in fact, you can force it with any Scriptlet, it doesn't even have to be valid one, for example: brave.com##+js(acs, this, probably, is, going, to, break, brave, and, crash, it, instead, of, ignoring, it).
So that's the issue, how uBlock might write rules like 'this, one' or this\, one and one will work fine, and the other might crash Brave and will not work correctly.
That means, Brave has to support/implement/workaround to avoid this issue, and be compatible with both ways, because even if all commas are escaped to avoid the crash, the filter still will not work correctly because it will add the single quote symbols to the value, and for example, Youtube doesn't not load correctly, if the single quotes are present
But for example, taking these youtube filters and removing them as exceptions and loading them in Brave, this is the result:
this one will crash Brave:
❌ youtube.com##+js(rpnt, script, /(\(function serverContract\(\)|writeEmbed|var _F_cssRowKey)/, 'const pruner=text=>{let json=JSON.parse(text);for(k of["playerAds","adPlacements","adSlots"]){json[k]=[];}return JSON.stringify(json);};const urlFromArg=arg=>{if(typeof arg==="string"){return arg;}if(arg instanceof Request){return arg.url;}return String(arg);};const realFetch=window.fetch;window.fetch=new Proxy(window.fetch,{apply:function(target,thisArg,args){if(!(urlFromArg(args[0]).includes("player?key="))){return Reflect.apply(target,thisArg,args);}return realFetch(...args).then(realResponse=>realResponse.text().then(text=>new Response(pruner(text),{status:realResponse.status,statusText:realResponse.statusText,headers:realResponse.headers,})));}});window.XMLHttpRequest.prototype.open=new Proxy(window.XMLHttpRequest.prototype.open,{apply:async(target,thisArg,args)=>{if(!(urlFromArg(args[1]).includes("player?key="))){return Reflect.apply(target,thisArg,args);}thisArg.addEventListener("readystatechange",function(){if(thisArg.readyState!==4){return;}const type=thisArg.responseType;if(type!==""&&type!=="text"){return;}const textin=thisArg.responseText;const textout=pruner(textin);if(textout===textin){return;}Object.defineProperty(thisArg,"response",{value:textout});Object.defineProperty(thisArg,"responseText",{value:textout});});return Reflect.apply(target,thisArg,args);}}); $1')
This one filter wouldn't crash Brave:
✅ youtube.com##+js(rpnt, script, /(\(function serverContract\(\)|writeEmbed|var _F_cssRowKey)/, const pruner=text=>{let json=JSON.parse(text);for(k of["playerAds"\,"adPlacements"\,"adSlots"]){json[k]=[];}return JSON.stringify(json);};const urlFromArg=arg=>{if(typeof arg==="string"){return arg;}if(arg instanceof Request){return arg.url;}return String(arg);};const realFetch=window.fetch;window.fetch=new Proxy(window.fetch\,{apply:function(target\,thisArg\,args){if(!(urlFromArg(args[0]).includes("player?key="))){return Reflect.apply(target\,thisArg\,args);}return realFetch(...args).then(realResponse=>realResponse.text().then(text=>new Response(pruner(text)\,{status:realResponse.status\,statusText:realResponse.statusText\,headers:realResponse.headers\,})));}});window.XMLHttpRequest.prototype.open=new Proxy(window.XMLHttpRequest.prototype.open\,{apply:async(target\,thisArg\,args)=>{if(!(urlFromArg(args[1]).includes("player?key="))){return Reflect.apply(target\,thisArg\,args);}thisArg.addEventListener("readystatechange"\,function(){if(thisArg.readyState!==4){return;}const type=thisArg.responseType;if(type!==""&&type!=="text"){return;}const textin=thisArg.responseText;const textout=pruner(textin);if(textout===textin){return;}Object.defineProperty(thisArg\,"response"\,{value:textout});Object.defineProperty(thisArg\,"responseText"\,{value:textout});});return Reflect.apply(target\,thisArg\,args);}}); $1)
Then This one (from Quick Fixes) would crash:
❌ youtube.com##+js(rpnt, script, (function serverContract(), 'const pruner=text=>{const json=JSON.parse(text);for(k of["playerAds","adPlacements","adSlots"]){json[k]=[];}return JSON.stringify(json);};const urlFromArg=arg=>{if(typeof arg==="string"){return arg;}if(arg instanceof Request){return arg.url;}return String(arg);};const realFetch=window.fetch;window.fetch=new Proxy(window.fetch,{apply:function(target,thisArg,args){if(!(urlFromArg(args[0]).includes("player?key="))){return Reflect.apply(target,thisArg,args);}return realFetch(...args).then(realResponse=>realResponse.text().then(text=>new Response(pruner(text),{status:realResponse.status,statusText:realResponse.statusText,headers:realResponse.headers,})));}}); (function serverContract()')
And this one wouldn't crash Brave:
✅ youtube.com##+js(rpnt, script, (function serverContract(), const pruner=text=>{const json=JSON.parse(text);for(k of["playerAds"\,"adPlacements"\,"adSlots"]){json[k]=[];}return JSON.stringify(json);};const urlFromArg=arg=>{if(typeof arg==="string"){return arg;}if(arg instanceof Request){return arg.url;}return String(arg);};const realFetch=window.fetch;window.fetch=new Proxy(window.fetch\,{apply:function(target\,thisArg\,args){if(!(urlFromArg(args[0]).includes("player?key="))){return Reflect.apply(target\,thisArg\,args);}return realFetch(...args).then(realResponse=>realResponse.text().then(text=>new Response(pruner(text)\,{status:realResponse.status\,statusText:realResponse.statusText\,headers:realResponse.headers\,})));}}); (function serverContract())
And as you can see, the Second and Fourth filters work as expected and don't crash Brave because, they are removing the single quote symbol and escaping all commas.
But yeah, if this is not fixed, any future filter that has this single quote value syntax will probably crash brave by having too commas and don't work at all.
And if this Youtube scriptlet filter stays, it will crash the browser not just by going to Youtube, but also others websites that use Youtube and load this scriptlet.
cc @pes10k, seems like something to check for in future scriplet audits
@diracdeltas i only audit the implementation of the scriptlets, not the filter list rules that invoke the scriptlets (where this issue would come up IIUC)
@antonok-edm this seems like something that’d need to be handled in Adblock-rs’s parsing code, no?
and thank you for catching this @Emi-TheDhamphirInLoveUnderTheFrozenStar !
@Emi-TheDhamphirInLoveUnderTheFrozenStar thanks again for finding this! Could have resulted in pretty ugly crashes in Release if left unaddressed.
I pushed a fix for the crashing in 0fae87e. That won't make any rules using the single-quote syntax work yet, but at least they won't be causing crashes. I'll followup with single-quote argument support later (#310).
@antonok-edm yeah I noticed this crash long time ago while doing my rules, but since rules will never need many arguments I never worried to mention it.
Thanks for addressing that, now I won't crash my browsers as much if I forget to escape the commas! and I will be able to update Brave unbreak soon instead of using every default list as custom and block Unbreak from updating so youtube won't crash.
But saying that, I hope trusted scriptlets will be available in Brave soon. I know security reasons is the reason but they are useful for a lot of things, like when people ask "how can I turn safe search in Brave Search", something simple, but people can learn to use the adblocker to do these kind of stuff, instead of expecting Brave to implement 3000 toggles for stuff only 2 users will use.
Last time I will say is, since single quote argument from uBlock is pretty much only used for this YT special rule for the type of rule it is, the best thing today is to either edit Brave Unbreak https://github.com/brave/adblock-lists/blob/071342715374222d0b9efec8037f5c04336c2a53/brave-unbreak.txt#L75 and either remove it completely or edit it to remove the single quote and escape every comma and make it work in Brave.
Even if it won't crash Brave anymore, it will still break Youtube as shown in my post, and let's say, tomorrow trusted scriptlets get implemented and then the single quote argument is not, well, you will get people complaining about Youtube not working and 'wait some hours to get it fixed' will not be a good excuse.
The rule can't depend on single quote arguments, when the feature is yet to be implemented, and the fix to make it compatible in Brave takes 3 seconds.
Since I don't know the timeline for trusted scriptlets in Brave or when Brave will get single quote arguments, well, there shouldn't be a risk to break everyone's Youtube if anything, better not to have something causing issues that are not needed yet, than having something that is problematic today.
Thanks for your quick fix and have a good day!
Trusted scriptlets are coming very soon (see brave/brave-core#19946), and I'll be working on single quote argument support in the meantime as well.
@antonok-edm do you think we need a proper parser (in the formal grammar / YACC sense) for handling scriptlets. You have a better sense than I do for how complicated things are getting, and if we need something that comprehensive