Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(206)

Issue 29347034: Issue 1727 - Prevent circumvention via WebSocket (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 2 months ago by kzar
Modified:
1 year, 1 month ago
Visibility:
Public.

Description

Issue 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+121 lines, -44 lines) Patch
M include.preload.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 3 chunks +106 lines, -31 lines 0 comments Download
M lib/requestBlocker.js View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +11 lines, -1 line 0 comments Download
M safari/include.youtube.js View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 2 chunks +4 lines, -12 lines 0 comments Download

Messages

Total messages: 65
kzar
Patch Set 1
1 year, 2 months ago (2016-06-26 11:56:39 UTC) #1
lainverse
On 2016/06/26 11:56:39, kzar wrote: > Patch Set 1 kzar, wouldn't it be better to ...
1 year, 2 months ago (2016-06-26 15:58:03 UTC) #2
kzar
Using Proxy could have been nice, more conveniently wrapping WebSocket with potentially less code. However ...
1 year, 2 months ago (2016-06-26 20:22:25 UTC) #3
lainverse
It's indeed true that less code paths provide less room for bugs and easier to ...
1 year, 2 months ago (2016-06-26 22:22:30 UTC) #4
kzar
Well what advantage does using Proxy have for the users? As far as I see ...
1 year, 2 months ago (2016-06-27 10:28:34 UTC) #5
lainverse
That's the point: majority of users will receive simpler solution based on native browser capabilities ...
1 year, 2 months ago (2016-06-27 18:26:04 UTC) #6
lainverse
Also, if I understand your code correctly you create new WebSocket object right at the ...
1 year, 2 months ago (2016-06-27 20:02:08 UTC) #7
kzar
On 2016/06/27 18:26:04, lainverse wrote: > That's the point: majority of users will receive simpler ...
1 year, 2 months ago (2016-06-28 08:53:58 UTC) #8
Sebastian Noack
https://codereview.adblockplus.org/29347034/diff/29347035/background.js File background.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/background.js#newcode44 background.js:44: port.on("websocket-request", function(msg, sender) Please no new code in background.js. ...
1 year, 2 months ago (2016-06-28 16:17:51 UTC) #9
kzar
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#newcode462 include.preload.js:462: websocket.close(); On 2016/06/28 16:17:51, Sebastian Noack wrote: > As ...
1 year, 2 months ago (2016-06-28 16:32:40 UTC) #10
Sebastian Noack
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#newcode462 include.preload.js:462: websocket.close(); On 2016/06/28 16:32:40, kzar wrote: > On 2016/06/28 ...
1 year, 2 months ago (2016-06-28 17:04:58 UTC) #11
kzar
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#newcode44 background.js:44: port.on("websocket-request", function(msg, sender) On ...
1 year, 2 months ago (2016-06-29 13:40:31 UTC) #12
lainverse
https://codereview.adblockplus.org/29347034/diff/29347035/background.js File background.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/background.js#newcode47 background.js:47: new URL(msg.url), On 2016/06/28 16:17:51, Sebastian Noack wrote: > ...
1 year, 2 months ago (2016-06-29 13:54:59 UTC) #13
Sebastian Noack
https://codereview.adblockplus.org/29347034/diff/29347035/background.js File background.js (right): https://codereview.adblockplus.org/29347034/diff/29347035/background.js#newcode47 background.js:47: new URL(msg.url), On 2016/06/29 13:54:58, lainverse wrote: > On ...
1 year, 2 months ago (2016-07-01 17:03:40 UTC) #14
kzar
Patch Set 3 : Addressed feedback, use WeakMap (Will try using Proxy next, to see ...
1 year, 2 months ago (2016-07-06 16:39:21 UTC) #15
kzar
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#newcode491 include.preload.js:491: delete storage.queue; // FIXME onerror!? On 2016/07/06 16:39:20, kzar ...
1 year, 2 months ago (2016-07-07 07:51:32 UTC) #16
lainverse
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#newcode491 > ...
1 year, 2 months ago (2016-07-07 12:01:08 UTC) #17
kzar
Patch Set 4 : Dispatch real close and error events for blocked WebSockets I just ...
1 year, 2 months ago (2016-07-07 13:49:58 UTC) #18
lainverse
> Yes it's a problem, the isTrusted property is designed to stop us doing exactly ...
1 year, 2 months ago (2016-07-08 09:16:36 UTC) #19
kzar
Well for some reason the error event has isTrusted, but not the close event! (Both ...
1 year, 2 months ago (2016-07-08 10:48:18 UTC) #20
kzar
(To make things even more confusing on Safari WebSocket.prototype has the addEventListener and removeEventListener methods, ...
1 year, 2 months ago (2016-07-08 11:12:27 UTC) #21
kzar
On 2016/07/08 11:12:27, kzar wrote: > (To make things even more confusing on Safari WebSocket.prototype ...
1 year, 2 months ago (2016-07-11 07:43:44 UTC) #22
kzar
(OK that approach seems to work really nicely, I've got a rough version together. Will ...
1 year, 2 months ago (2016-07-11 14:50:33 UTC) #23
kzar
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#newcode458 include.preload.js:458: function ...
1 year, 2 months ago (2016-07-12 11:34:33 UTC) #24
lainverse
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#newcode534 include.preload.js:534: WebSocket = function(url, protocols) On 2016/07/12 11:34:33, kzar wrote: ...
1 year, 2 months ago (2016-07-12 12:22:21 UTC) #25
lainverse
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#newcode534 > ...
1 year, 2 months ago (2016-07-12 13:39:54 UTC) #26
lainverse
Update: Apparently uBlock Origin already encountered a problem with wrapping WebSocket since some sites already ...
1 year, 2 months ago (2016-07-12 14:08:01 UTC) #27
kzar
> BTW, redefining toString function of a WebSocket isn't enough... Well I didn't suggest to ...
1 year, 2 months ago (2016-07-12 15:04:30 UTC) #28
kzar
Looks like there's yet another way to circumvent this wrapper that I didn't consider. Looking ...
1 year, 2 months ago (2016-07-14 12:26:18 UTC) #29
tartpvule
On 2016/07/14 12:26:18, kzar wrote: > Looks like there's yet another way to circumvent this ...
1 year, 2 months ago (2016-07-14 13:20:19 UTC) #30
kzar
On 2016/07/14 13:20:19, tartpvule wrote: > Looking at the code, I have these observations... Just ...
1 year, 2 months ago (2016-07-14 17:36:07 UTC) #31
kzar
Patch Set 6 : Much simpler approach Here's a much simpler approach I thought of ...
1 year, 1 month ago (2016-07-27 11:21:58 UTC) #32
kzar
Patch Set 7 : Try our best to prevent circumvention
1 year, 1 month ago (2016-07-27 12:05:21 UTC) #33
kzar
Patch Set 8 : Stop detection via toString > As for the WebSocket.toString problem I ...
1 year, 1 month ago (2016-07-27 17:27:16 UTC) #34
lainverse
On 2016/07/27 17:27:16, kzar wrote: > Patch Set 8 : Stop detection via toString > ...
1 year, 1 month ago (2016-07-27 17:39:34 UTC) #35
kzar
On 2016/07/27 17:39:34, lainverse wrote: > ...does it help against Object.prototype.toString.call? > Also, does it ...
1 year, 1 month ago (2016-07-27 17:47:11 UTC) #36
tartpvule
kzar, The latest wrapper ("Patch Set 8 : Stop detection via toString") can be circumvented ...
1 year, 1 month ago (2016-07-27 19:04:50 UTC) #37
kzar
Patch Set 9 : Fix call.bind circumvention >> The latest wrapper ("Patch Set 8 : ...
1 year, 1 month ago (2016-07-27 20:20:17 UTC) #38
kzar
Patch Set 10 : Fix WebSocket.toString for Safari (A bonus of the simpler approach is ...
1 year, 1 month ago (2016-07-28 08:05:52 UTC) #39
tartpvule
kzar, Taking a look at the latest patch, I can no longer find any "redefine ...
1 year, 1 month ago (2016-07-31 05:21:59 UTC) #40
kzar
Patch Set 11 : Ensure the url is a String! Haha, I did not think ...
1 year, 1 month ago (2016-08-02 19:52:08 UTC) #41
kzar
Patch Set 11 : Ensure the url is a String! Haha, I did not think ...
1 year, 1 month ago (2016-08-02 19:52:08 UTC) #42
Sebastian Noack
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#newcode465 include.preload.js:465: return websocket[key] = value; On 2016/07/06 16:39:20, kzar wrote: ...
1 year, 1 month ago (2016-08-08 16:42:57 UTC) #43
kzar
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#newcode465 include.preload.js:465: return websocket[key] = ...
1 year, 1 month ago (2016-08-08 18:19:08 UTC) #44
Sebastian Noack
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#newcode353 include.preload.js:353: function injectJS(f) I just noticed that this function is ...
1 year, 1 month ago (2016-08-08 22:09:16 UTC) #45
kzar
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#newcode353 include.preload.js:353: function injectJS(f) ...
1 year, 1 month ago (2016-08-09 12:08:06 UTC) #46
Sebastian Noack
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#newcode456 include.preload.js:456: if (this === WebSocket) On 2016/08/09 12:08:04, kzar wrote: ...
1 year, 1 month ago (2016-08-09 14:53:31 UTC) #47
Sebastian Noack
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#newcode359 include.preload.js:359: // include.youtube.js passes this function is passed a RegExp ...
1 year, 1 month ago (2016-08-09 15:09:38 UTC) #48
Sebastian Noack
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#newcode391 include.preload.js:391: }; Bind those functions as well? Otherwise we can ...
1 year, 1 month ago (2016-08-09 15:41:16 UTC) #49
kzar
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#newcode456 include.preload.js:456: if (this ...
1 year, 1 month ago (2016-08-09 16:11:41 UTC) #50
kzar
Patch Set 15 : Throw correct exceptions if constructor is used improperly
1 year, 1 month ago (2016-08-09 16:55:05 UTC) #51
Sebastian Noack
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#newcode384 include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); I don't think that this ...
1 year, 1 month ago (2016-08-09 18:05:14 UTC) #52
kzar
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): ...
1 year, 1 month ago (2016-08-09 18:50:15 UTC) #53
Sebastian Noack
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#newcode384 include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); On 2016/08/09 18:50:14, kzar wrote: ...
1 year, 1 month ago (2016-08-10 08:01:47 UTC) #54
kzar
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#newcode384 include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); On 2016/08/10 08:01:46, Sebastian Noack ...
1 year, 1 month ago (2016-08-10 08:07:34 UTC) #55
Sebastian Noack
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#newcode384 include.preload.js:384: var boundCall = Function.prototype.call.bind(Function.prototype.call); On 2016/08/10 08:07:33, kzar wrote: ...
1 year, 1 month ago (2016-08-10 08:44:19 UTC) #56
kzar
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#newcode384 include.preload.js:384: ...
1 year, 1 month ago (2016-08-10 10:19:45 UTC) #57
Sebastian Noack
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#newcode361 include.preload.js:361: if (!(arg instanceof RegExp)) Perhaps I got a better ...
1 year, 1 month ago (2016-08-10 12:22:46 UTC) #58
kzar
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#newcode361 include.preload.js:361: if ...
1 year, 1 month ago (2016-08-10 12:58:56 UTC) #59
Sebastian Noack
LGTM
1 year, 1 month ago (2016-08-10 13:05:13 UTC) #60
kzar
Patch Set 19 : Fix WebSocket properties for Safari
1 year, 1 month ago (2016-08-10 15:43:44 UTC) #61
Sebastian Noack
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#newcode458 include.preload.js:458: CONNECTING: {value: 0, enumerable: true}, Still not a reason ...
1 year, 1 month ago (2016-08-10 16:15:40 UTC) #62
kzar
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#newcode458 include.preload.js:458: CONNECTING: {value: 0, ...
1 year, 1 month ago (2016-08-10 16:20:08 UTC) #63
kzar
Patch Set 21 : Don't hardcode connection state values
1 year, 1 month ago (2016-08-10 16:26:21 UTC) #64
Sebastian Noack
1 year, 1 month ago (2016-08-10 16:37:10 UTC) #65
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5