Issue 29347034: Issue 1727 - Prevent circumvention via WebSocket (Closed)
DescriptionIssue 1727 - Prevent circumvention via WebSocket
Patch Set 1 #
Total comments: 14
Patch Set 2 : Addressed feedback #
Total comments: 20
Patch Set 3 : Addressed feedback, use WeakMap #
Total comments: 4
Patch Set 4 : Dispatch real close and error events for blocked WebSockets #Patch Set 5 : Use Proxy, intercept events #
Total comments: 5
Patch Set 6 : Much simpler approach #Patch Set 7 : Try our best to prevent circumvention #Patch Set 8 : Stop detection via toString #Patch Set 9 : Fix call.bind circumvention #Patch Set 10 : Fix WebSocket.toString for Safari #
Total comments: 8
Patch Set 11 : Ensure the url is a String! #
Total comments: 6
Patch Set 12 : Addressed feedback #
Total comments: 20
Patch Set 13 : Addressed further feedback #
Total comments: 10
Patch Set 14 : Addressed further feedback #Patch Set 15 : Throw correct exceptions if constructor is used improperly #
Total comments: 6
Patch Set 16 : Don't bother using boundCall for CSSStyleSheet.prototype functions #Patch Set 17 : Go with simpler toString fix #
Total comments: 4
Patch Set 18 : Remove boundCall, simplify runInPage #Patch Set 19 : Fix WebSocket properties for Safari #
Total comments: 2
Patch Set 20 : Added newlines #Patch Set 21 : Don't hardcode connection state values #
MessagesTotal messages: 65
Patch Set 1
On 2016/06/26 11:56:39, kzar wrote: > Patch Set 1 kzar, wouldn't it be better to use Proxy when it's available? You can simply check if(!window.Proxy) or same as in your code if(typeof Proxy == "undefined"). It's even possible to do so at some earlier stage and only once, before including code to the page, to avoid including any extra code. Later on it will be possible to drop old code without Proxy if minimum supported browser version will be raised for some other reason.
Using Proxy could have been nice, more conveniently wrapping WebSocket with potentially less code. However since we can't always use it I don't see the point in ever using it. Having two code paths here would mean more code, complexity and potential for bugs. With one code path at least it will be solidly tested, if there are bugs we will hopefully catch them early on.
It's indeed true that less code paths provide less room for bugs and easier to support, but ABP already uses polyfills for promise and fetch instead of always relying on internal implementations of both. As I understand it's not possible to make a full polyfill for a Proxy, but isn't it better to provide a better solution for majority of users (am I wrong here?)?
Well what advantage does using Proxy have for the users? As far as I see it the only advantage to using Proxy here would be simpler code, and so adding extra code in order to use it seems counter productive. Maybe I'm missing something?
That's the point: majority of users will receive simpler solution based on native browser capabilities with less potential issues on sites.
Also, if I understand your code correctly you create new WebSocket object right at the start of a wrapper constructor. As I understand this allows to use WS connections at least for tracking. Even though it won't be possible to use connection the connection still will be established and data could be passed to server as part of a URL.
On 2016/06/27 18:26:04, lainverse wrote: > That's the point: majority of users will receive simpler solution based on > native browser capabilities with less potential issues on sites. I guess we just disagree on this one, as far as I see it the more code paths and code we have the less simple the solution is and the more potential issues there are. On 2016/06/27 20:02:08, lainverse wrote: > Also, if I understand your code correctly you create new WebSocket object right > at the start of a wrapper constructor. As I understand this allows to use WS > connections at least for tracking. Even though it won't be possible to use > connection the connection still will be established and data could be passed to > server as part of a URL. Good point, I had not considered that. I just fired up WireShark to verify what happens. On my system with Chrome 50 the WebSocket is closed before even the initial Upgrade HTTP request is sent. But of course it's a race condition and therefore conceivable that we could occasionally leak data this way. Thing is avoiding that (pretty minor) risk would be far from trivial. Thanks for keeping me on my toes, I appreciate the feedback and ideas.
https://codereview.adblockplus.org/29347034/diff/29347035/background.js File background.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/background.js#newco... background.js:44: port.on("websocket-request", function(msg, sender) Please no new code in background.js. This file is matter to be removed. And this seems to rather belong in requestBlocker.js anyway. https://codereview.adblockplus.org/29347034/diff/29347035/background.js#newco... background.js:47: new URL(msg.url), Is it possible to create a WebSocket with a relative URL? Then we'd have to make sure to pass the corresponding base URL to the URl() constructor here. https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js#... include.preload.js:450: for (key of ["close", "send", "addEventListener", "removeEventListener"]) Is this script processed with jsHydra? It seems not. Otherwise, this won't work on old browser versions. https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js#... include.preload.js:462: websocket.close(); As Lain pointed out, this approach allows WebSockets to be opened, sending and potentially even retrieving data until we get an asynchronous response from the background page. So we should rather defer opening the WebSocket until we got a response from the background page. https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js#... include.preload.js:480: script.textContent = "(" + wrapper.toString() + ")(\"" + eventName + "\");"; Ideally, we inject only one script. Note that we already inject a script to prevent the DOM API from messing with our stylesheet. So mind merging those scripts?
https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js#... include.preload.js:462: websocket.close(); On 2016/06/28 16:17:51, Sebastian Noack wrote: > As Lain pointed out, this approach allows WebSockets to be opened, sending and > potentially even retrieving data until we get an asynchronous response from the > background page. So we should rather defer opening the WebSocket until we got a > response from the background page. > I actually tested this with WireShark and I found no requests made it out before the WebSocket was closed. (Not even the initial HTTP request that requested the upgrade to the WebSocket protocol!) I concede that the initial HTTP request might sometimes be sent, but rewriting the wrapper to defer all method calls, adding/removing of event listeners and property assignments assignments until after the block/allow result has been asynchronously returned is no simple matter, is it really worth it?!
https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js#... include.preload.js:462: websocket.close(); On 2016/06/28 16:32:40, kzar wrote: > On 2016/06/28 16:17:51, Sebastian Noack wrote: > > As Lain pointed out, this approach allows WebSockets to be opened, sending and > > potentially even retrieving data until we get an asynchronous response from > the > > background page. So we should rather defer opening the WebSocket until we got > a > > response from the background page. > > > > I actually tested this with WireShark and I found no requests made it out before > the WebSocket was closed. (Not even the initial HTTP request that requested the > upgrade to the WebSocket protocol!) > > I concede that the initial HTTP request might sometimes be sent, but rewriting > the wrapper to defer all method calls, adding/removing of event listeners and > property assignments assignments until after the block/allow result has been > asynchronously returned is no simple matter, is it really worth it?! I have two concerns here: 1. It might be a potential timing issue, i.e. depend on how fast the response arrives. 2. It might be an implementation detail that is matter to change or differ accross platforms.
Patch Set 2: Addressed feedback https://codereview.adblockplus.org/29347034/diff/29347035/background.js File background.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/background.js#newco... background.js:44: port.on("websocket-request", function(msg, sender) On 2016/06/28 16:17:51, Sebastian Noack wrote: > Please no new code in background.js. This file is matter to be removed. And this > seems to rather belong in requestBlocker.js anyway. Done. https://codereview.adblockplus.org/29347034/diff/29347035/background.js#newco... background.js:47: new URL(msg.url), On 2016/06/28 16:17:51, Sebastian Noack wrote: > Is it possible to create a WebSocket with a relative URL? Then we'd have to make > sure to pass the corresponding base URL to the URl() constructor here. No, I don't believe it's possible. (I see an exception when attempting to do it.) https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js#... include.preload.js:450: for (key of ["close", "send", "addEventListener", "removeEventListener"]) On 2016/06/28 16:17:51, Sebastian Noack wrote: > Is this script processed with jsHydra? It seems not. Otherwise, this won't work > on old browser versions. Acknowledged. https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js#... include.preload.js:462: websocket.close(); On 2016/06/28 17:04:58, Sebastian Noack wrote: > On 2016/06/28 16:32:40, kzar wrote: > > On 2016/06/28 16:17:51, Sebastian Noack wrote: > > > As Lain pointed out, this approach allows WebSockets to be opened, sending > and > > > potentially even retrieving data until we get an asynchronous response from > > the > > > background page. So we should rather defer opening the WebSocket until we > got > > a > > > response from the background page. > > > > > > > I actually tested this with WireShark and I found no requests made it out > before > > the WebSocket was closed. (Not even the initial HTTP request that requested > the > > upgrade to the WebSocket protocol!) > > > > I concede that the initial HTTP request might sometimes be sent, but rewriting > > the wrapper to defer all method calls, adding/removing of event listeners and > > property assignments assignments until after the block/allow result has been > > asynchronously returned is no simple matter, is it really worth it?! > > I have two concerns here: > > 1. It might be a potential timing issue, i.e. depend on how fast the response > arrives. > 2. It might be an implementation detail that is matter to change or differ > accross platforms. OK, done. https://codereview.adblockplus.org/29347034/diff/29347035/include.preload.js#... include.preload.js:480: script.textContent = "(" + wrapper.toString() + ")(\"" + eventName + "\");"; On 2016/06/28 16:17:51, Sebastian Noack wrote: > Ideally, we inject only one script. Note that we already inject a script to > prevent the DOM API from messing with our stylesheet. So mind merging those > scripts? Done.
https://codereview.adblockplus.org/29347034/diff/29347035/background.js File background.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/background.js#newco... background.js:47: new URL(msg.url), On 2016/06/28 16:17:51, Sebastian Noack wrote: > Is it possible to create a WebSocket with a relative URL? Then we'd have to make > sure to pass the corresponding base URL to the URl() constructor here. As I understood you must pass FQDN URL. At least for now. Neither Chrome, nor Firefox supports relative URLs and there are a lot of questions on the web regarding this with the same answer to generate a full URL.
https://codereview.adblockplus.org/29347034/diff/29347035/background.js File background.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/background.js#newco... background.js:47: new URL(msg.url), On 2016/06/29 13:54:58, lainverse wrote: > On 2016/06/28 16:17:51, Sebastian Noack wrote: > > Is it possible to create a WebSocket with a relative URL? Then we'd have to > make > > sure to pass the corresponding base URL to the URl() constructor here. > > As I understood you must pass FQDN URL. At least for now. Neither Chrome, nor > Firefox supports relative URLs and there are a lot of questions on the web > regarding this with the same answer to generate a full URL. Ah right. Of course it needs to be an absolute URL because it's a different protocol anyway. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:403: var eventName = "abpws-" + Math.random().toString().substr(2); Instead of duplicating this logic, perhaps just assign the random string to a global variable? It doesn't matter if it's the same. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:431: var websocket; Since you access this variable before it might get set, please initialize with null. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:432: var websocketProperties = { It seems you don't need to hard-code that properties but can get the list by Object.getOwnPropertyNames(WebSocket.prototype), though obviously not the default values without actually creating a WebSocket. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:455: enumerable: true, Instead hard-coding metadata like "enumerable" you could get them from the original descriptor to ensure consistent behavior: Object.getOwnPropertyDescriptor(WebSocket.prototype, key) https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:465: return websocket[key] = value; Why do you return in a setter? https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:492: Object.defineProperties(this, properties); Note that those properties (in Chrome at least) are not on the WebSocket object but on it's prototype. So please consider doing the same, to avoid side effects. At the very least websites could easily detect that hack otherwise. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:534: WebSocket.prototype = Object.create(window.EventTarget.prototype, readyStates); Can't you simply reuse the original parent prototype? WebSocket.prototype = Object.getPrototypeOf(originalWebSocket.prototype) https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:538: script.textContent += "(" + wrapper.toString() + ")(\"" + eventName + "\");"; This is inconsistent with how we generate the code to prevent messing with the stylesheet where we use plain string concatenation, while we convert a function to a string here. I don't mind either way, but would you mind to keep it consistent? https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:601: if (script) Sorry, I didn't notice that this a different code path. So I guess it actually makes more sense to inject the scripts separately. Besides, the worse code locality this change prevents the stylesheet to be protected if it's updated later.
Patch Set 3 : Addressed feedback, use WeakMap (Will try using Proxy next, to see how much that helps us.) https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:403: var eventName = "abpws-" + Math.random().toString().substr(2); On 2016/07/01 17:03:39, Sebastian Noack wrote: > Instead of duplicating this logic, perhaps just assign the random string to a > global variable? It doesn't matter if it's the same. Done. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:431: var websocket; On 2016/07/01 17:03:39, Sebastian Noack wrote: > Since you access this variable before it might get set, please initialize with > null. Done. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:432: var websocketProperties = { On 2016/07/01 17:03:39, Sebastian Noack wrote: > It seems you don't need to hard-code that properties but can get the list by > Object.getOwnPropertyNames(WebSocket.prototype), though obviously not the > default values without actually creating a WebSocket. Done. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:455: enumerable: true, On 2016/07/01 17:03:40, Sebastian Noack wrote: > Instead hard-coding metadata like "enumerable" you could get them from the > original descriptor to ensure consistent behavior: > > Object.getOwnPropertyDescriptor(WebSocket.prototype, key) Done. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:465: return websocket[key] = value; On 2016/07/01 17:03:40, Sebastian Noack wrote: > Why do you return in a setter? Because the return value for an assignment is usually the value assigned. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:492: Object.defineProperties(this, properties); On 2016/07/01 17:03:40, Sebastian Noack wrote: > Note that those properties (in Chrome at least) are not on the WebSocket object > but on it's prototype. So please consider doing the same, to avoid side effects. > At the very least websites could easily detect that hack otherwise. Done. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:534: WebSocket.prototype = Object.create(window.EventTarget.prototype, readyStates); On 2016/07/01 17:03:40, Sebastian Noack wrote: > Can't you simply reuse the original parent prototype? > > WebSocket.prototype = Object.getPrototypeOf(originalWebSocket.prototype) Done. (Good point as it is different in Safari.) https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:538: script.textContent += "(" + wrapper.toString() + ")(\"" + eventName + "\");"; On 2016/07/01 17:03:40, Sebastian Noack wrote: > This is inconsistent with how we generate the code to prevent messing with the > stylesheet where we use plain string concatenation, while we convert a function > to a string here. I don't mind either way, but would you mind to keep it > consistent? Done, however wasn't sure how to test the code above still works. https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:601: if (script) On 2016/07/01 17:03:39, Sebastian Noack wrote: > Sorry, I didn't notice that this a different code path. So I guess it actually > makes more sense to inject the scripts separately. Besides, the worse code > locality this change prevents the stylesheet to be protected if it's updated > later. Done. https://codereview.adblockplus.org/29347034/diff/29347312/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347312/include.preload.js#... include.preload.js:491: delete storage.queue; // FIXME onerror!? So far I don't fire the error / close event when a WebSocket connection is blocked. I could do that here but I'm not sure how to reproduce the event properly, for example isTrusted! Closest I can think to do is to create a new WebSocket with a bogus URL, providing their onerror listener. Of course they could then check the URL... https://codereview.adblockplus.org/29347034/diff/29347312/include.preload.js#... include.preload.js:508: if (key == "prototype") (For Safari) https://codereview.adblockplus.org/29347034/diff/29347312/include.preload.js#... include.preload.js:518: if (!storage) So that the "Uncaught TypeError: Illegal invocation(…)" exception is thrown when doing something like `WebSocket.prototype.send(...)`. (Similar below too.)
https://codereview.adblockplus.org/29347034/diff/29347312/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347312/include.preload.js#... include.preload.js:491: delete storage.queue; // FIXME onerror!? On 2016/07/06 16:39:20, kzar wrote: > So far I don't fire the error / close event when a WebSocket connection is > blocked. > > I could do that here but I'm not sure how to reproduce the event properly, for > example isTrusted! Closest I can think to do is to create a new WebSocket with a > bogus URL, providing their onerror listener. Of course they could then check the > URL... Note: My earlier implementation which always created a WebSocket but then called .close() if blocked didn't have this limitation. A real close event was fired. If you really insist that we don't just do that again we could still do something similar. We could create a WebSocket when the block/allow result comes back, but then immediately close it if blocked. That would be even less likely to leak connections than the earlier implementation. (And I never witnessed the earlier implementation leaking connections anyway.)
On 2016/07/07 07:51:32, kzar wrote: > https://codereview.adblockplus.org/29347034/diff/29347312/include.preload.js > File include.preload.js (right): > > https://codereview.adblockplus.org/29347034/diff/29347312/include.preload.js#... > include.preload.js:491: delete storage.queue; // FIXME onerror!? > On 2016/07/06 16:39:20, kzar wrote: > If you really insist that we don't just do that again we could still do > something similar. We could create a WebSocket when the block/allow result comes > back, but then immediately close it if blocked. That would be even less likely > to leak connections than the earlier implementation. (And I never witnessed the > earlier implementation leaking connections anyway.) Why not just simulate it if connection was blocked and .close() called? The only issue is to make sure it looks the same as the real one. Is it a problem? BTW, check .send() too. In my tests it also closed the connection and some sites actually rely on this behavior like these two: http://freerutor.com/ http://rustorka.com/forum/index.php If readyState not closed properly at least one of them will continuously check was it closed yet.
Patch Set 4 : Dispatch real close and error events for blocked WebSockets I just realised this breaks addEventListener and removeEventListener. Think we have to use Proxy after all, otherwise we'd need to either put those methods on the WebSocket wrapper in the wrong place or mess with EventTarget itself. Working on it now. > Why not just simulate it if connection was blocked and .close() called? The only > issue is to make sure it looks the same as the real one. Is it a problem? Yes it's a problem, the isTrusted property is designed to stop us doing exactly that. https://developer.mozilla.org/en-US/docs/Web/API/Event/isTrusted
> Yes it's a problem, the isTrusted property is designed to stop us doing exactly > that. https://developer.mozilla.org/en-US/docs/Web/API/Event/isTrusted WebSocket uses CloseEvent object and I haven't found isTrusted there even thought it's derived from Event. Is it accessible? On the other hand new CloseEvent('close',{wasClean:true,code:1000}) returns result rather similar to the real one. Not sure about the details. BTW, the reason why I think it's a bad idea to create real WebSocket object is this: > var s = new WebSocket('wss://wsp.marketgid.com/ws'); > s.onclose = function(e) {window.someobj = e.target;}; > s.close(); > new window.someobj.__proto__.constructor('wss://wsp.marketgid.com/ws'); WebSocket {url: "wss://wsp.marketgid.com/ws", readyState: 0, bufferedAmount: 0, onopen: null, onerror: null…}
Well for some reason the error event has isTrusted, but not the close event! (Both are fired if the WebSocket is closed immediately, before it has connected.) You're right about event.target.__proto__.constructor, didn't think of that :/ So I guess dispatching a fake close event might be the best we can do, they are a lot simpler than the error events too so it wouldn't be hard. I guess the other option might be to wrap error events in a Proxy before they're dispatched. Ugh... will continue to work on it.
(To make things even more confusing on Safari WebSocket.prototype has the addEventListener and removeEventListener methods, but on Chrome WebSocket.prototype.__proto__ (EventTarget) has them.)
On 2016/07/08 11:12:27, kzar wrote: > (To make things even more confusing on Safari WebSocket.prototype has the > addEventListener and removeEventListener methods, but on Chrome > WebSocket.prototype.__proto__ (EventTarget) has them.) OK so I've been thinking about this a lot for a the last few days: - We need to intercept all events before they're dispatched, even ones for WebSockets that are allowed. Otherwise event.target.__proto__.constructor can be used for circumvention like you pointed out. (I don't think the other ad blockers take this into account and could be easily circumvented.) - We need to keep the structure of WebSocket, WebSocket.prototype and WebSocket.prototype.__proto__ as close to reality as possible to avoid breaking websites. For Chrome addEventListener and removeEventListener actually belong to WebSocket.prototype.__proto__ but on Safari they belong to WebSocket.prototype. - Since we must intercept calls to addEventListener and removeEventListener we have to put them on WebSocket.prototype for all browsers, wrap WebSocket.prototype.__proto__ for Chrome or use a Proxy. I think wrapping WebSocket.prototype in a Proxy makes the most sense here. - Since we're going to use a Proxy to mess with addEventListener and removeEventListener anyway we might as well use it throughout. As a bonus perhaps we can wrap the events we're intercepting in a Proxy that fakes isTrusted and target too? - Since we have to mess with all events anyway there's no point creating a WebSocket connection if blocked, it makes more sense to dispatch a fake error event in the same way the other fake events are dispatched. So I finally have a plan as to how this should all work, unfortunately it's rather complicated and it will take some time to get right. So far I have most parts roughly working except event interception. I'm continuing to work on it this week, will keep you guys posted. Sorry for the delay.
(OK that approach seems to work really nicely, I've got a rough version together. Will tidy it up tomorrow and update the review.)
Patch Set 5 : Use Proxy, intercept events https://codereview.adblockplus.org/29347034/diff/29347521/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347521/include.preload.js#... include.preload.js:458: function addRemoveEventListener(storage, key, type, listener) Should we care about the useCapture parameter for addEventListener? So far I just ignore it. https://codereview.adblockplus.org/29347034/diff/29347521/include.preload.js#... include.preload.js:483: function processQueue(storage) Queuing up assignments and method calls seems rather pointless now that we manage events ourselves. In my testing I couldn't find any website which use this, it's usual to wait for a connection to be opened before attempting to send for example. I'd like to remove storage.queue and all related logic, any opinions? https://codereview.adblockplus.org/29347034/diff/29347521/include.preload.js#... include.preload.js:534: WebSocket = function(url, protocols) So far I don't intercept WebSocket.toString() so it could be used to detect us. I'm inclined to wrap WebSocket in a Proxy to fix that. On the other hand this code is already too complex for my taste. Opinions? https://codereview.adblockplus.org/29347034/diff/29347521/include.preload.js#... include.preload.js:566: for (var i = 0; i < eventNames.length; i += 1) If a website creates a WebSocket (which manages to connect), doesn't add a close event listener and then deletes all references to it the WebSocket should be garbage collected. As we always add a close event listener here the (actual, not wrapper of) WebSocket wouldn't be garbage collected until the connection was closed. Personally I don't think this is a big deal, and certainly not worth the extra logic to fix. Any opinions?
https://codereview.adblockplus.org/29347034/diff/29347521/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347521/include.preload.js#... include.preload.js:534: WebSocket = function(url, protocols) On 2016/07/12 11:34:33, kzar wrote: > So far I don't intercept WebSocket.toString() so it could be used to detect us. > I'm inclined to wrap WebSocket in a Proxy to fix that. On the other hand this > code is already too complex for my taste. Opinions? If we going all the way to avoid detection then we have to. However, make sure to handle chained toString calls and calls on functions, like ws.toString.toString(), ws.close.toString(). Also, there is toLocaleString function with the same output (not sure if some specificl locale parameters provided, but should be the same).
On 2016/07/12 12:22:21, lainverse wrote: > https://codereview.adblockplus.org/29347034/diff/29347521/include.preload.js > File include.preload.js (right): > > https://codereview.adblockplus.org/29347034/diff/29347521/include.preload.js#... > include.preload.js:534: WebSocket = function(url, protocols) > On 2016/07/12 11:34:33, kzar wrote: > > So far I don't intercept WebSocket.toString() so it could be used to detect > us. > > I'm inclined to wrap WebSocket in a Proxy to fix that. On the other hand this > > code is already too complex for my taste. Opinions? > > If we going all the way to avoid detection then we have to. However, make sure > to handle chained toString calls and calls on functions, like > ws.toString.toString(), ws.close.toString(). Also, there is toLocaleString > function with the same output (not sure if some specificl locale parameters > provided, but should be the same). BTW, redefining toString function of a WebSocket isn't enough, since you can do something like this: Object.toString.call(WebSocket) And I'd rather not touch prototype of the Object class and redefine toString for everything just to conceal presence of the WebSocket wrapper.
Update: Apparently uBlock Origin already encountered a problem with wrapping WebSocket since some sites already check for WS being "native"... and not even for advertisement purpose! This one, for example: https://new.tradingview.com/chart/ Look for /native/.test(window[wsName].prototype.send) in the code. Source: https://github.com/gorhill/uBlock/issues/1605 Raymond circumvented that by adding a string 'native'; in the code.
> BTW, redefining toString function of a WebSocket isn't enough... Well I didn't suggest to redefine it, rather intercept it using a Proxy. However you're right `Object.toString.call(WebSocket)` would still be a problem. I will try to figure out a way around that, I _really_ don't want to mess with Object.toString however! > Apparently uBlock Origin already encountered a problem with wrapping WebSocket > since some sites already check for WS being "native"... and not even for > advertisement purpose! This one, for example: https://new.tradingview.com/chart/ > Look for /native/.test(window[wsName].prototype.send) in the code. > Source: https://github.com/gorhill/uBlock/issues/1605 > Raymond circumvented that by adding a string 'native'; in the code. I can't reproduce that problem when running my code as of Patch Set 5, since WebSocket.prototype.send returns the real send function. (Come to think of it I wonder if you can somehow get back to the real WebSocket.prototype from that?!) A check like this would still be a problem however: var websocket = new WebSocket("ws://echo.websocket.org"); /native/.test(websocket.send); While avoiding detection where possible is preferable, avoiding it everywhere is just not realistic. Priorities are (in order) to not break websites, to avoid circumvention and where possible to avoid detection.
Looks like there's yet another way to circumvent this wrapper that I didn't consider. Looking into it now https://github.com/gorhill/uBO-WebSocket/issues/5
On 2016/07/14 12:26:18, kzar wrote: > Looks like there's yet another way to circumvent this wrapper that I didn't > consider. Looking into it now https://github.com/gorhill/uBO-WebSocket/issues/5 Looking at the code, I have these observations, haven't tested, though. 1. (Line 445) Might it be possible to redefine document.addEventListener to capture the reference to listener then proceed to call it directly with a crafted "Allow" response? Could document.dispatchEvent or the CustomEvent constructor be modified to send another URI than the "real" one? 2. (Line 554) instanceStorage is a WeakMap. Is it possible to trap the values passed to .set(a, b)? If so, storage is passed by reference, which allows to redefine "websocket" member into a setter trap to trap an instance of RealWebSocket and get storage.websocket.constructor. 3. (Line 567) Could EventTarget.prototype.addEventListener be replaced to get access to "storage.websocket" via "this" which RealWebSocket would be accessible via this.constructor? 4. (Line 585) Could WeakMap.prototype.get be trapped to get "storage"? 5. (Line 620) Once Object.getOwnPropertyDescriptor is trapped to get "target", might it be possible to get .constructor from it? 6. (Line 494 and Line 625) Function.prototype.apply? I think this is a losing battle :( Caching all the references to these functions as variables in the "wrapper" function closure is tedious at best.
On 2016/07/14 13:20:19, tartpvule wrote: > Looking at the code, I have these observations... Just wanted to say thanks for taking the time, continuing to work on the code and will responded properly when I get a chance - Dave
Patch Set 6 : Much simpler approach Here's a much simpler approach I thought of after looking at how Gorhill avoided circumvention by simply overriding the constructor for WebSocket instances[1]. I take it one step further and just change it for WebSocket.prototype, which IMHO makes more sense. It's much cleaner that the previous solution and seems to work fine. The problem of *possibly* sometimes leaking the initial request exists here but IMHO this is a trade off we just need to make at this point. As tartpvule points out we must be really careful to stop the website overriding global functions and Objects that we're using in the injected code. I think keeping our solution as simple as possible is the only way we stand any chance to get this right. I started trying to attempt it for the previous (complex) code and it was a complete nightmare. I am now going to look at keeping track of all the globals and will hopefully post another patchset for that shortly. As for the WebSocket.toString problem I can't see an easy way around it without wrapping Object in a Proxy! Since not breaking websites is more important than avoiding detection I vote that we ignore this for now. [1] - https://github.com/gorhill/uBO-WebSocket/blob/master/contentscript.js#L127
Patch Set 7 : Try our best to prevent circumvention
Patch Set 8 : Stop detection via toString > As for the WebSocket.toString problem I can't see an easy way around it... It turns out to be pretty easy once you know how, so I went ahead and did fixed that too. Pretty happy with the WebSockets wrapper now, I can't see how anyone can circumvent or even detect it. (Of course it's possible I missed something however...)
On 2016/07/27 17:27:16, kzar wrote: > Patch Set 8 : Stop detection via toString > > > As for the WebSocket.toString problem I can't see an easy way around it... > > It turns out to be pretty easy once you know how, so I went ahead and did fixed > that too. Well, it's almost the same as modifying Object.prototype. BTW, does it help against Object.prototype.toString.call? Also, does it work with toLocaleString too?
On 2016/07/27 17:39:34, lainverse wrote:
> ...does it help against Object.prototype.toString.call?
> Also, does it work with toLocaleString too?
Yep, Object.prototype.toString(WebSocket)
Object.prototype.toLocaleString(WebSocket), WebSocket.toString() and
WebSocket.toLocaleString() all return what they should. ("[object Object]" for
the first two and "function WebSocket() { [native code] }" for the second two in
Chrome at least.)
kzar,
The latest wrapper ("Patch Set 8 : Stop detection via toString") can be
circumvented by redefining Function.prototype.bind
window.SnatchedWebSocket = (function() {
var _prize = null;
var _bind = Function.prototype.bind;
Function.prototype.bind = function() {
return function(x) {
_prize = x;
};
};
WebSocket.toString();
Function.prototype.bind = _bind;
return _prize;
})();
The "checkRequest" function is also similarly vulnerable to a redefined
Function.prototype.bind returning a booby trap in response to calls with the
argument being document.addEventListener and document.removeEventListener to
grab a reference to the "listener" function and call it with "{ detail: false }"
This occurs because the pattern <function>.<something>(...) searches the
untrusted Function.prototype for <something>
One way I can think of to solve this problem is
var bcall = Function.prototype.call.bind(Function.prototype.call);
then replace all instances of
call.bind(f)(x, y, z, ...);
with
bcall(f, x, y, z, ...);
P.S. Maybe I'm being paranoid, but doesn't having "new RealWebSocket(...)" in
the constructor here mean that at least a connection attempt is made
unconditionally to the WebSocket server?
Patch Set 9 : Fix call.bind circumvention
>> The latest wrapper ("Patch Set 8 : Stop detection via toString") can be
circumvented by redefining Function.prototype.bind...
Wow, nice catch, I'm impressed. Fixed as you suggested.
>> P.S. Maybe I'm being paranoid, but doesn't having "new RealWebSocket(...)" in
the constructor here mean that at least a connection attempt is made
unconditionally to the WebSocket server?
Please see my notes published along with Patch Set 6. (During testing with
WireShark I found that this does not happen in practice, the WebSocket is closed
before any connection is actually made. That said you're right that it *could*
happen depending on how the browser is implemented. It's a trade-off I chose
because the alternative implementation using Proxy (Patch Set 5) was getting
extremely complex and therefore really hard to protect against circumvention. Or
as you put it, it was becoming "a loosing battle"!)
Patch Set 10 : Fix WebSocket.toString for Safari (A bonus of the simpler approach is that it works great for Safari 9 too. I just had to make this tweak to get WebSocket.toString() to behave.)
kzar, Taking a look at the latest patch, I can no longer find any "redefine global variable"-type circumvention vector. However, I just came up with an even stranger way to circumvent it. Tested on https://www.websocket.org/echo.html on Chromium 53.0.2774.3 window._WebSocket = WebSocket; window.WebSocket = function(url, protocols) { var proxy = new Proxy(new String(url), { get: function(target, property, receiver) { if (property === 'toString' || property === 'valueOf') { return function() { return url; }; } return Reflect.get(target, property, receiver); } }); return new _WebSocket(proxy, protocols); }; This causes the port handler for "websocket-request" in the background script to throw on the "new URL(msg.url)" statement, resulting in the url not being dispatched to the onBeforeRequest handler at all and abort the execution of the function. This, in turn, means that the "block this request" message will not be sent to the event listener in the wrapper. End result: closeWebSocket is never called on the created WebSocket instance. The magical thing here is that this proxy is accepted by the real WebSocket constructor just fine. One way to fix this is to do "url = url.toString();" before anything else in the wrapped WebSocket constructor (or cache a reference to String.prototype.toString and boundcall() it.
Patch Set 11 : Ensure the url is a String! Haha, I did not think of that! I realise we need to first call url.toString() in case the given url is a URL instead of a String (String.prototype.toString will not work on a URL, nor URL.prototype.toString on a String). Then once we apparently have a string I use String.prototype.toString to ensure it _really_ is a String, and not some crazy Proxy!
Patch Set 11 : Ensure the url is a String! Haha, I did not think of that! I realise we need to first call url.toString() in case the given url is a URL instead of a String (String.prototype.toString will not work on a URL, nor URL.prototype.toString on a String). Then once we apparently have a string I use String.prototype.toString to ensure it _really_ is a String, and not some crazy Proxy!
https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:465: return websocket[key] = value; On 2016/07/06 16:39:20, kzar wrote: > On 2016/07/01 17:03:40, Sebastian Noack wrote: > > Why do you return in a setter? > > Because the return value for an assignment is usually the value assigned. An assignment operation returns it's right-hand value regardless of what the setter returns. https://codereview.adblockplus.org/29347034/diff/29348815/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29348815/include.preload.js#... include.preload.js:378: for (i = 0; i < disableables.length; i += 1) Nit: This is JS. How about i++? https://codereview.adblockplus.org/29347034/diff/29348815/include.preload.js#... include.preload.js:387: (function(method) Instead this wrapper function, why not simply using methods.forEach() in the first place? https://codereview.adblockplus.org/29347034/diff/29348815/include.preload.js#... include.preload.js:482: Object.defineProperties(WebSocket, { I wonder whether we should dynamically generate those properties rather than hard-coding them and their values: var properties = Object.getOwnPropertyNames(RealWebSocket); for (var i = 0; i < properties.length; i++) { var name = properties[i]; var desc = Object.getOwnPropertyDescriptor(RealWebSocket, name); Object.defineProperty(WebSocket, name, desc); } Note that this copies the prototype property as well, which BTW is read-only on the original WebSocket function but not on your fake WebSocket at the moment. https://codereview.adblockplus.org/29347034/diff/29348815/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29347034/diff/29348815/lib/requestBlocker.... lib/requestBlocker.js:160: var results = ext.webRequest.onBeforeRequest._dispatch( Nit: We prefer block scoping (i.e. "let") in modules processed by jsHydra. Personally, I would not create a variable here in the first place. https://codereview.adblockplus.org/29347034/diff/29349009/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349009/include.preload.js#... include.preload.js:433: var closeWebSocket = RealWebSocket.prototype.close; Since the real WebSocket isn't reachable anymore from outside this closure, once this code runs (and since it is a document_start script it does before any web page script), there is no way that it's prototype or any of it's methods gets overridden. Same for RealWebSocket.toString below. Or do I miss something? https://codereview.adblockplus.org/29347034/diff/29349009/include.preload.js#... include.preload.js:436: var removeEventListener = document.removeEventListener; Any reason why you don't simply bind those functions here: var addEventListener = document.addEventListener.bind(document); var removeEventListener = document.removeEventListener.bind(document); var dispatchEvent = document.dispatchEvent.bind(document); That way you don't need to cache the document. Moreover, you don't need that confusing boundCall() wrapper function. https://codereview.adblockplus.org/29347034/diff/29349009/include.preload.js#... include.preload.js:475: url = boundCall(stringToString, url.toString()); This generates a different error when url is null or undefiend, compared to the original WebSocket construstor. Why not simply passing the value through to the real WebSocket? And convert the url to string afterwards if no exception got raised when dispatching the event.
Patch Set 12 : Addressed feedback https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29347148/include.preload.js#... include.preload.js:465: return websocket[key] = value; On 2016/08/08 16:42:53, Sebastian Noack wrote: > On 2016/07/06 16:39:20, kzar wrote: > > On 2016/07/01 17:03:40, Sebastian Noack wrote: > > > Why do you return in a setter? > > > > Because the return value for an assignment is usually the value assigned. > > An assignment operation returns it's right-hand value regardless of what the > setter returns. Ah, I didn't know that! https://codereview.adblockplus.org/29347034/diff/29348815/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29348815/include.preload.js#... include.preload.js:378: for (i = 0; i < disableables.length; i += 1) On 2016/08/08 16:42:54, Sebastian Noack wrote: > Nit: This is JS. How about i++? Done. https://codereview.adblockplus.org/29347034/diff/29348815/include.preload.js#... include.preload.js:387: (function(method) On 2016/08/08 16:42:54, Sebastian Noack wrote: > Instead this wrapper function, why not simply using methods.forEach() in the > first place? Done. https://codereview.adblockplus.org/29347034/diff/29348815/include.preload.js#... include.preload.js:482: Object.defineProperties(WebSocket, { On 2016/08/08 16:42:55, Sebastian Noack wrote: > I wonder whether we should dynamically generate those properties rather than > hard-coding them and their values: > > var properties = Object.getOwnPropertyNames(RealWebSocket); > for (var i = 0; i < properties.length; i++) > { > var name = properties[i]; > var desc = Object.getOwnPropertyDescriptor(RealWebSocket, name); > Object.defineProperty(WebSocket, name, desc); > } > > Note that this copies the prototype property as well, which BTW is read-only on > the original WebSocket function but not on your fake WebSocket at the moment. Done. https://codereview.adblockplus.org/29347034/diff/29348815/lib/requestBlocker.js File lib/requestBlocker.js (right): https://codereview.adblockplus.org/29347034/diff/29348815/lib/requestBlocker.... lib/requestBlocker.js:160: var results = ext.webRequest.onBeforeRequest._dispatch( On 2016/08/08 16:42:55, Sebastian Noack wrote: > Nit: We prefer block scoping (i.e. "let") in modules processed by jsHydra. > Personally, I would not create a variable here in the first place. Done. https://codereview.adblockplus.org/29347034/diff/29349009/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349009/include.preload.js#... include.preload.js:433: var closeWebSocket = RealWebSocket.prototype.close; On 2016/08/08 16:42:55, Sebastian Noack wrote: > Since the real WebSocket isn't reachable anymore from outside this closure, once > this code runs (and since it is a document_start script it does before any web > page script), there is no way that it's prototype or any of it's methods gets > overridden. Same for RealWebSocket.toString below. Or do I miss something? Well our WebSocket wrapper gets assigned the real WebSocket's prototype as its prototype, so WebSocket.prototype.close could really be messed with by the website. As for RealWebSocket.toString, that's a way to obtain Function.prototype.toString on Chrome and Object.prototype.toString on Safari. Necessary since Safari considers WebSocket an Object but Chrome considers it a Function. https://codereview.adblockplus.org/29347034/diff/29349009/include.preload.js#... include.preload.js:436: var removeEventListener = document.removeEventListener; On 2016/08/08 16:42:56, Sebastian Noack wrote: > Any reason why you don't simply bind those functions here: > > var addEventListener = document.addEventListener.bind(document); > var removeEventListener = document.removeEventListener.bind(document); > var dispatchEvent = document.dispatchEvent.bind(document); > > That way you don't need to cache the document. Moreover, you don't need that > confusing boundCall() wrapper function. Done, but unfortunately I do still need boundCall elsewhere. https://codereview.adblockplus.org/29347034/diff/29349009/include.preload.js#... include.preload.js:475: url = boundCall(stringToString, url.toString()); On 2016/08/08 16:42:56, Sebastian Noack wrote: > This generates a different error when url is null or undefiend, compared to the > original WebSocket construstor. Why not simply passing the value through to the > real WebSocket? And convert the url to string afterwards if no exception got > raised when dispatching the event. Ah, well spotted. Fixed. As for _why_ I do this, see tartpvule's comment with a crazy circumvention approach https://codereview.adblockplus.org/29347034/#msg40 !
https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:353: function injectJS(f) I just noticed that this function is redundant with runInPage() in safari/include.youtube.js. Functions defined here can be reused there. FWIW, note how that implementation is also somewhat simpler by always expecting exactly one argument which is all we'd need. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:376: var i; It seems that you don' reuse the variable i anymore. So put the declaration in the loop header? https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:382: var methods = ["deleteRule", "removeRule"]; How about ["deleteRule", "removeRule"].forEach(), without creating a variable? https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:437: var WebSocketString = RealWebSocket.toString(); I suppose this variable should rather be lowercase? https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:456: if (this === WebSocket) As per Mozilla's coding practices, we prefer == over ===. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:458: if (this === wrappedToString) This special case is unneccessary if you simply assign Function.prototype.toString = wrappedToString.bind() below. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:468: return new RealWebSocket(); You still get a different error when null or undefined is given. Not sure whether it's worth to be considered. But then I wonder why you don't simply call |websocket = new RealWebSocket(url, protocols)| unconditionally. IMO, the string-to-string dance should happen afterwards anyway as its just for the case if a crazy object is given that can be converted to a string by the WebSocket constructor but isn't automatically converted through event messaging. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:472: url = boundCall(stringToString, url.toString()); 8Why) do you have to call both, stringToString() and .toString()?
Patch Set 13 : Addressed further feedback https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:353: function injectJS(f) On 2016/08/08 22:09:15, Sebastian Noack wrote: > I just noticed that this function is redundant with runInPage() in > safari/include.youtube.js. Functions defined here can be reused there. > > FWIW, note how that implementation is also somewhat simpler by always expecting > exactly one argument which is all we'd need. Done. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:376: var i; On 2016/08/08 22:09:15, Sebastian Noack wrote: > It seems that you don' reuse the variable i anymore. So put the declaration in > the loop header? Done. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:382: var methods = ["deleteRule", "removeRule"]; On 2016/08/08 22:09:15, Sebastian Noack wrote: > How about ["deleteRule", "removeRule"].forEach(), without creating a variable? Done. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:437: var WebSocketString = RealWebSocket.toString(); On 2016/08/08 22:09:15, Sebastian Noack wrote: > I suppose this variable should rather be lowercase? Done. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:456: if (this === WebSocket) On 2016/08/08 22:09:16, Sebastian Noack wrote: > As per Mozilla's coding practices, we prefer == over ===. Sure but here we want to check that `this` points to the same function as WebSocket. We don't want any type conversion. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:458: if (this === wrappedToString) On 2016/08/08 22:09:15, Sebastian Noack wrote: > This special case is unneccessary if you simply assign > Function.prototype.toString = wrappedToString.bind() below. (I tried it out but found I would get the exception "VM2877:39 Uncaught TypeError: Function.prototype.toString is not generic(…)" when doing things like `Function.prototype.toString.call(WebSocket)` or `Function.prototype.toString.call(Function.prototype.toString)` which worked before.) https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:468: return new RealWebSocket(); On 2016/08/08 22:09:15, Sebastian Noack wrote: > You still get a different error when null or undefined is given. Not sure > whether it's worth to be considered. But then I wonder why you don't simply call > |websocket = new RealWebSocket(url, protocols)| unconditionally. IMO, the > string-to-string dance should happen afterwards anyway as its just for the case > if a crazy object is given that can be converted to a string by the WebSocket > constructor but isn't automatically converted through event messaging. Hmm good point and we can even just use `websocket.url` since then the WebSocket constructor does the work of normalising the url for us! Done.
https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:456: if (this === WebSocket) On 2016/08/09 12:08:04, kzar wrote: > On 2016/08/08 22:09:16, Sebastian Noack wrote: > > As per Mozilla's coding practices, we prefer == over ===. > > Sure but here we want to check that `this` points to the same function as > WebSocket. We don't want any type conversion. I think, if none of the values has a primitive type, == wouldn't do any conversion, or do I miss something? https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:458: if (this === wrappedToString) On 2016/08/09 12:08:04, kzar wrote: > On 2016/08/08 22:09:15, Sebastian Noack wrote: > > This special case is unneccessary if you simply assign > > Function.prototype.toString = wrappedToString.bind() below. > > (I tried it out but found I would get the exception "VM2877:39 Uncaught > TypeError: Function.prototype.toString is not generic(…)" when doing things like > `Function.prototype.toString.call(WebSocket)` or > `Function.prototype.toString.call(Function.prototype.toString)` which worked > before.) Even better, why not just |WebSocket = function(...) {...}.bind()|? That would make the whole Function.prototype.toString hack redundant. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:468: return new RealWebSocket(); On 2016/08/09 12:08:05, kzar wrote: > On 2016/08/08 22:09:15, Sebastian Noack wrote: > > You still get a different error when null or undefined is given. Not sure > > whether it's worth to be considered. But then I wonder why you don't simply > call > > |websocket = new RealWebSocket(url, protocols)| unconditionally. IMO, the > > string-to-string dance should happen afterwards anyway as its just for the > case > > if a crazy object is given that can be converted to a string by the WebSocket > > constructor but isn't automatically converted through event messaging. > > Hmm good point and we can even just use `websocket.url` since then the WebSocket > constructor does the work of normalising the url for us! Done. Even better, nice! https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:361: if (!(arg instanceof RegExp)) It seems this wasn't necessary before. So why is it now? https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:423: function wrapper(eventName) I just noticed that the argument name is the same as the name of the variable in the outer closure. That might be confusing, potentially raising wrong assumptions when reading the code. While they are essentially the same, it needs to be explicitly passed in the generated code. https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:423: function wrapper(eventName) I just noticed a minor inconsistency, while you use an anonymous function above (as well ass in include.youtube.js), you pass a named function to run runInPage() here, which results into following code to be injected: (function wrapper(eventName) { ... })(...); instead of (function (eventName) { ... })(...); FWIW, I'd prefer the latter, as it keeps the generated/injected code simpler and shorter. Also I'm not sure if using a named function could potentially leak the function in any JavaScript version/implementation, doesn't seem to be the case in modern Chrome versions though. (Or perhaps, skip the variable all together, and pass an anonymouse function directly to runInPage().)
https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:359: // include.youtube.js passes this function is passed a RegExp which This reads weird. Typo? In include.youtube.js, this function is passed a RegExp ... https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:361: if (!(arg instanceof RegExp)) On 2016/08/09 14:53:30, Sebastian Noack wrote: > It seems this wasn't necessary before. So why is it now? Never mind, I got it, (misinterpreted the condition before). However, note that alternatively, we could move JSON.stringify() into the calling code, avoiding a special case here. Not sure, which is better though..
https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:391: }; Bind those functions as well? Otherwise we can still be detect.
Patch Set 14 : Addressed further feedback https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:456: if (this === WebSocket) On 2016/08/09 14:53:29, Sebastian Noack wrote: > On 2016/08/09 12:08:04, kzar wrote: > > On 2016/08/08 22:09:16, Sebastian Noack wrote: > > > As per Mozilla's coding practices, we prefer == over ===. > > > > Sure but here we want to check that `this` points to the same function as > > WebSocket. We don't want any type conversion. > > I think, if none of the values has a primitive type, == wouldn't do any > conversion, or do I miss something? Well to tell you the truth I'm not 100% sure if type conversion is ever a problem here, but since we really are checking for identity I think it makes more sense to write that explicitly. No chance for mistake that way and it also more accurately illustrates what we're doing. https://codereview.adblockplus.org/29347034/diff/29349219/include.preload.js#... include.preload.js:458: if (this === wrappedToString) On 2016/08/09 14:53:29, Sebastian Noack wrote: > On 2016/08/09 12:08:04, kzar wrote: > > On 2016/08/08 22:09:15, Sebastian Noack wrote: > > > This special case is unneccessary if you simply assign > > > Function.prototype.toString = wrappedToString.bind() below. > > > > (I tried it out but found I would get the exception "VM2877:39 Uncaught > > TypeError: Function.prototype.toString is not generic(…)" when doing things > like > > `Function.prototype.toString.call(WebSocket)` or > > `Function.prototype.toString.call(Function.prototype.toString)` which worked > > before.) > > Even better, why not just |WebSocket = function(...) {...}.bind()|? That would > make the whole Function.prototype.toString hack redundant. Well nice idea but then WebSocket.toString() gives the wrong values: Chrome: "function WebSocket() { [native code] }" -> "function () { [native code] }" Safari: "[object WebSocketConstructor]" -> "function WebSocket() { [native code] }" (Newlines omitted.) https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:359: // include.youtube.js passes this function is passed a RegExp which On 2016/08/09 15:09:37, Sebastian Noack wrote: > This reads weird. Typo? > > In include.youtube.js, this function is passed a RegExp ... Done. https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:391: }; On 2016/08/09 15:41:15, Sebastian Noack wrote: > Bind those functions as well? Otherwise we can still be detect. Done. https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:423: function wrapper(eventName) On 2016/08/09 14:53:30, Sebastian Noack wrote: > I just noticed a minor inconsistency, while you use an anonymous function above > (as well ass in include.youtube.js), you pass a named function to run > runInPage() here, which results into following code to be injected: > > (function wrapper(eventName) > { > ... > })(...); > > instead of > > > (function (eventName) > { > ... > })(...); > > FWIW, I'd prefer the latter, as it keeps the generated/injected code simpler and > shorter. Also I'm not sure if using a named function could potentially leak the > function in any JavaScript version/implementation, doesn't seem to be the case > in modern Chrome versions though. > > (Or perhaps, skip the variable all together, and pass an anonymouse function > directly to runInPage().) Done. https://codereview.adblockplus.org/29347034/diff/29349261/include.preload.js#... include.preload.js:423: function wrapper(eventName) On 2016/08/09 14:53:30, Sebastian Noack wrote: > I just noticed that the argument name is the same as the name of the variable in > the outer closure. That might be confusing, potentially raising wrong > assumptions when reading the code. While they are essentially the same, it needs > to be explicitly passed in the generated code. I'd rather leave this being called eventName, I think especially now that the function's not named it's pretty obvious what's going on. (If you insist I suppose I don't mind changing it, but in that case do you have a suggestion for a better name?)
Patch Set 15 : Throw correct exceptions if constructor is used improperly
https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js#... include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); I don't think that this is necessary here. We don't call "call" unless we allow the call. So all one could do by patching Function.prototype.call is further restricting the behavior here. Perhaps they could also detect us that way, but they could as well just check for CSSStyleSheet.prototype.deleteRule.toString(), that is what I actually meant. But as just discussed on IRC, that paranoia about detection is slowly becoming crazy. We still need to be able to maintain this code. And I fear eventually, there will always be some way to detect us.
Patch Set 16 : Don't bother using boundCall for CSSStyleSheet.prototype functions https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js#... include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); On 2016/08/09 18:05:14, Sebastian Noack wrote: > I don't think that this is necessary here. We don't call "call" unless we allow > the call. So all one could do by patching Function.prototype.call is further > restricting the behavior here. > > Perhaps they could also detect us that way, but they could as well just check > for CSSStyleSheet.prototype.deleteRule.toString(), that is what I actually > meant. But as just discussed on IRC, that paranoia about detection is slowly > becoming crazy. We still need to be able to maintain this code. And I fear > eventually, there will always be some way to detect us. OK, I removed that again. I agree that we should not worry about .toString here. Since this was already the case before the WebSocket changes we're not regressing anything. If someone does use `CSSStyleSheet.prototype.deleteRule.toString()` for detection at some point in the future we can always worry about it then.
https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js#... include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); On 2016/08/09 18:50:14, kzar wrote: > On 2016/08/09 18:05:14, Sebastian Noack wrote: > > I don't think that this is necessary here. We don't call "call" unless we > allow > > the call. So all one could do by patching Function.prototype.call is further > > restricting the behavior here. > > > > Perhaps they could also detect us that way, but they could as well just check > > for CSSStyleSheet.prototype.deleteRule.toString(), that is what I actually > > meant. But as just discussed on IRC, that paranoia about detection is slowly > > becoming crazy. We still need to be able to maintain this code. And I fear > > eventually, there will always be some way to detect us. > > OK, I removed that again. > > I agree that we should not worry about .toString here. Since this was already > the case before the WebSocket changes we're not regressing anything. If someone > does use `CSSStyleSheet.prototype.deleteRule.toString()` for detection at some > point in the future we can always worry about it then. Interesting, so why do you do a much more elaborate hack for WebSocket.toString() then?
https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js#... include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); On 2016/08/10 08:01:46, Sebastian Noack wrote: > On 2016/08/09 18:50:14, kzar wrote: > > On 2016/08/09 18:05:14, Sebastian Noack wrote: > > > I don't think that this is necessary here. We don't call "call" unless we > > allow > > > the call. So all one could do by patching Function.prototype.call is further > > > restricting the behavior here. > > > > > > Perhaps they could also detect us that way, but they could as well just > check > > > for CSSStyleSheet.prototype.deleteRule.toString(), that is what I actually > > > meant. But as just discussed on IRC, that paranoia about detection is slowly > > > becoming crazy. We still need to be able to maintain this code. And I fear > > > eventually, there will always be some way to detect us. > > > > OK, I removed that again. > > > > I agree that we should not worry about .toString here. Since this was already > > the case before the WebSocket changes we're not regressing anything. If > someone > > does use `CSSStyleSheet.prototype.deleteRule.toString()` for detection at some > > point in the future we can always worry about it then. > > Interesting, so why do you do a much more elaborate hack for > WebSocket.toString() then? How is it much more elaborate than what would be required here to fix the string values? (My logic is that CSSStyleSheet.prototype.removeRule already has the wrong string value, so in this code review we're not making anything worse.)
https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js#... include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); On 2016/08/10 08:07:33, kzar wrote: > On 2016/08/10 08:01:46, Sebastian Noack wrote: > > On 2016/08/09 18:50:14, kzar wrote: > > > On 2016/08/09 18:05:14, Sebastian Noack wrote: > > > > I don't think that this is necessary here. We don't call "call" unless we > > > allow > > > > the call. So all one could do by patching Function.prototype.call is > further > > > > restricting the behavior here. > > > > > > > > Perhaps they could also detect us that way, but they could as well just > > check > > > > for CSSStyleSheet.prototype.deleteRule.toString(), that is what I actually > > > > meant. But as just discussed on IRC, that paranoia about detection is > slowly > > > > becoming crazy. We still need to be able to maintain this code. And I fear > > > > eventually, there will always be some way to detect us. > > > > > > OK, I removed that again. > > > > > > I agree that we should not worry about .toString here. Since this was > already > > > the case before the WebSocket changes we're not regressing anything. If > > someone > > > does use `CSSStyleSheet.prototype.deleteRule.toString()` for detection at > some > > > point in the future we can always worry about it then. > > > > Interesting, so why do you do a much more elaborate hack for > > WebSocket.toString() then? > > How is it much more elaborate than what would be required here to fix the string > values? (My logic is that CSSStyleSheet.prototype.removeRule already has the > wrong string value, so in this code review we're not making anything worse.) You are right, we'd need to patch Function.prototype.toString() the same way to also make CSSStyleSheet.prototype.removeRule.toString() return it's original value. I mistakenly assumed that simply calling bind() on our wrapper would do. But still, what is the point of doing so for WebSocket, if people could just detect us instead by checking for removeRule.toString()? And as you said, if that becomes an issue, we can still address it later.
Patch Set 17 : Go with simpler toString fix https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349333/include.preload.js#... include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); On 2016/08/10 08:44:18, Sebastian Noack wrote: > On 2016/08/10 08:07:33, kzar wrote: > > On 2016/08/10 08:01:46, Sebastian Noack wrote: > > > On 2016/08/09 18:50:14, kzar wrote: > > > > On 2016/08/09 18:05:14, Sebastian Noack wrote: > > > > > I don't think that this is necessary here. We don't call "call" unless > we > > > > allow > > > > > the call. So all one could do by patching Function.prototype.call is > > further > > > > > restricting the behavior here. > > > > > > > > > > Perhaps they could also detect us that way, but they could as well just > > > check > > > > > for CSSStyleSheet.prototype.deleteRule.toString(), that is what I > actually > > > > > meant. But as just discussed on IRC, that paranoia about detection is > > slowly > > > > > becoming crazy. We still need to be able to maintain this code. And I > fear > > > > > eventually, there will always be some way to detect us. > > > > > > > > OK, I removed that again. > > > > > > > > I agree that we should not worry about .toString here. Since this was > > already > > > > the case before the WebSocket changes we're not regressing anything. If > > > someone > > > > does use `CSSStyleSheet.prototype.deleteRule.toString()` for detection at > > some > > > > point in the future we can always worry about it then. > > > > > > Interesting, so why do you do a much more elaborate hack for > > > WebSocket.toString() then? > > > > How is it much more elaborate than what would be required here to fix the > string > > values? (My logic is that CSSStyleSheet.prototype.removeRule already has the > > wrong string value, so in this code review we're not making anything worse.) > > You are right, we'd need to patch Function.prototype.toString() the same way to > also make CSSStyleSheet.prototype.removeRule.toString() return it's original > value. I mistakenly assumed that simply calling bind() on our wrapper would do. > > But still, what is the point of doing so for WebSocket, if people could just > detect us instead by checking for removeRule.toString()? And as you said, if > that becomes an issue, we can still address it later. Fair enough. How about we just go with your .bind() idea for WebSocket? It's simple and it mostly works.
https://codereview.adblockplus.org/29347034/diff/29349349/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349349/include.preload.js#... include.preload.js:361: if (!(arg instanceof RegExp)) Perhaps I got a better idea to avoid that special case: Just pass RegExp.source in safari/include.youtube.js and pass it to the RegExp constructor in the function. https://codereview.adblockplus.org/29347034/diff/29349349/include.preload.js#... include.preload.js:431: var boundCall = Function.prototype.call.bind(Function.prototype.call); Since WebSocket.prototype.close is the only method called later now, I suppose you could simplify the logic a bit: var close = Function.prototype.call.bind(RealWebSocket.prototype.close); ... close(websocket);
Patch Set 18 : Remove boundCall, simplify runInPage https://codereview.adblockplus.org/29347034/diff/29349349/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349349/include.preload.js#... include.preload.js:361: if (!(arg instanceof RegExp)) On 2016/08/10 12:22:45, Sebastian Noack wrote: > Perhaps I got a better idea to avoid that special case: Just pass RegExp.source > in safari/include.youtube.js and pass it to the RegExp constructor in the > function. Done. https://codereview.adblockplus.org/29347034/diff/29349349/include.preload.js#... include.preload.js:431: var boundCall = Function.prototype.call.bind(Function.prototype.call); On 2016/08/10 12:22:45, Sebastian Noack wrote: > Since WebSocket.prototype.close is the only method called later now, I suppose > you could simplify the logic a bit: > > var close = Function.prototype.call.bind(RealWebSocket.prototype.close); > ... > close(websocket); Done.
LGTM
Patch Set 19 : Fix WebSocket properties for Safari
https://codereview.adblockplus.org/29347034/diff/29349632/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349632/include.preload.js#... include.preload.js:458: CONNECTING: {value: 0, enumerable: true}, Still not a reason to hard-code the values: Object.defineProperties(WebSocket, { CONNECTING: {value: RealWebSocket.CONNECTING, ... Also, the blank lines above and below helped the readability.
Patch Set 20 : Added newlines https://codereview.adblockplus.org/29347034/diff/29349632/include.preload.js File include.preload.js (right): https://codereview.adblockplus.org/29347034/diff/29349632/include.preload.js#... include.preload.js:458: CONNECTING: {value: 0, enumerable: true}, On 2016/08/10 16:15:39, Sebastian Noack wrote: > Still not a reason to hard-code the values: > > Object.defineProperties(WebSocket, { > CONNECTING: {value: RealWebSocket.CONNECTING, ... > > Also, the blank lines above and below helped the readability. Added the blank lines back, sorry didn't mean to remove those. Do you insist about the values? I'd rather keep it like this since it looks nicer and they're hardly going to change. (If the values change the names could just as well change too.)
Patch Set 21 : Don't hardcode connection state values
LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
