Issue 29721716: Issue 6473 - Remove WebSocket wrapper (Closed)
|
Created:
March 13, 2018, 4:33 p.m. by Manish Jethani Modified:
March 13, 2018, 11:16 p.m. Base URL:
https://hg.adblockplus.org/adblockpluschrome/ Visibility:
Public. |
DescriptionIssue 6473 - Remove WebSocket wrapper
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address comments to Patch Set 1 #
MessagesTotal messages: 12
Patch Set 1
https://codereview.adblockplus.org/29721716/diff/29721717/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29721716/diff/29721717/inject.preload.js#n... inject.preload.js:141: * Shared request checking code, used by the RTCPeerConnection wrapper. Well, it's no longer shared. We probably should just move the RTCPeerConnection wrapper comment up here. https://codereview.adblockplus.org/29721716/diff/29721717/lib/requestBlocker.js File lib/requestBlocker.js (left): https://codereview.adblockplus.org/29721716/diff/29721717/lib/requestBlocker.... lib/requestBlocker.js:208: port.on("request.blockedByWrapper", (msg, sender) => You might want to rename this message to "request.blockedByRTCWrapper". https://codereview.adblockplus.org/29721716/diff/29721717/lib/requestBlocker.... lib/requestBlocker.js:223: msg.requestType, This can be hard-coded to "webrtc" now. https://codereview.adblockplus.org/29721716/diff/29721717/lib/requestBlocker.... lib/requestBlocker.js:227: }); Please also remove the hard-coded "WEBSOCKET" value from filterTypes.
Patch Set 2: Address comments to Patch Set 1 https://codereview.adblockplus.org/29721716/diff/29721717/inject.preload.js File inject.preload.js (right): https://codereview.adblockplus.org/29721716/diff/29721717/inject.preload.js#n... inject.preload.js:141: * Shared request checking code, used by the RTCPeerConnection wrapper. On 2018/03/13 17:43:01, Sebastian Noack wrote: > Well, it's no longer shared. We probably should just move the RTCPeerConnection > wrapper comment up here. Done. https://codereview.adblockplus.org/29721716/diff/29721717/lib/requestBlocker.js File lib/requestBlocker.js (left): https://codereview.adblockplus.org/29721716/diff/29721717/lib/requestBlocker.... lib/requestBlocker.js:208: port.on("request.blockedByWrapper", (msg, sender) => On 2018/03/13 17:43:01, Sebastian Noack wrote: > You might want to rename this message to "request.blockedByRTCWrapper". Done. https://codereview.adblockplus.org/29721716/diff/29721717/lib/requestBlocker.... lib/requestBlocker.js:223: msg.requestType, On 2018/03/13 17:43:01, Sebastian Noack wrote: > This can be hard-coded to "webrtc" now. Done. https://codereview.adblockplus.org/29721716/diff/29721717/lib/requestBlocker.... lib/requestBlocker.js:227: }); On 2018/03/13 17:43:01, Sebastian Noack wrote: > Please also remove the hard-coded "WEBSOCKET" value from filterTypes. Done.
LGTM
Argh, wait. There is also some legacy code, which is only used in combination with the WebSocket wrapper, in lib/csp.js, that needs to be removed.
On 2018/03/13 18:33:35, Sebastian Noack wrote: > Argh, wait. There is also some legacy code, which is only used in combination > with the WebSocket wrapper, in lib/csp.js, that needs to be removed. How about just waiting for #5241 to land then, since I more or less rewrite lib/csp.js there.
On 2018/03/13 18:33:35, Sebastian Noack wrote: > Argh, wait. There is also some legacy code, which is only used in combination > with the WebSocket wrapper, in lib/csp.js, that needs to be removed. I noticed that earlier, but that is not related to API wrapper injection? It looks like an independent attempt to make up for the lack of WebSocket blocking support, which is fine. My goal here is to remove the API wrapper only, the rest of it can stay if it works and doesn't hurt anyone. What do you think?
On 2018/03/13 20:55:16, kzar wrote: > On 2018/03/13 18:33:35, Sebastian Noack wrote: > > Argh, wait. There is also some legacy code, which is only used in combination > > with the WebSocket wrapper, in lib/csp.js, that needs to be removed. > > How about just waiting for #5241 to land then, since I more or less rewrite > lib/csp.js there. In my opinion, let's do the lib/csp.js part as part of the rewrite (#5241) then. This change should only be about the API wrapper strictly.
On 2018/03/13 22:12:11, Manish Jethani wrote: > On 2018/03/13 18:33:35, Sebastian Noack wrote: > > Argh, wait. There is also some legacy code, which is only used in combination > > with the WebSocket wrapper, in lib/csp.js, that needs to be removed. > > I noticed that earlier, but that is not related to API wrapper injection? It > looks like an independent attempt to make up for the lack of WebSocket blocking > support, which is fine. My goal here is to remove the API wrapper only, the rest > of it can stay if it works and doesn't hurt anyone. > > What do you think? Well, if we only remove the WebSocket wrapper, but not the CSP workaround which is triggered by $websocket filters, the resulting semantics on Chrome <=57 would be rather confusing, i.e. no WebSocket connection would be blocked directly, but we'd still apply a workaround, injecting CSP headers, with the purpose of countering circumvention of the WebSocket wrapper. It seems with Dave's upcoming changes, adding a seperate $csp filter option, we don't inject CSP headers on Chrome <=57 when using the $websocket option, anymore, anyway. So we can either wait for his changes landing first, or remove lib/csp.js with this patch in order to land this patch first.
On 2018/03/13 22:51:05, Sebastian Noack wrote: > On 2018/03/13 22:12:11, Manish Jethani wrote: > > On 2018/03/13 18:33:35, Sebastian Noack wrote: > > > Argh, wait. There is also some legacy code, which is only used in > combination > > > with the WebSocket wrapper, in lib/csp.js, that needs to be removed. > > > > I noticed that earlier, but that is not related to API wrapper injection? It > > looks like an independent attempt to make up for the lack of WebSocket > blocking > > support, which is fine. My goal here is to remove the API wrapper only, the > rest > > of it can stay if it works and doesn't hurt anyone. > > > > What do you think? > > Well, if we only remove the WebSocket wrapper, but not the CSP workaround which > is triggered by $websocket filters, the resulting semantics on Chrome <=57 would > be rather confusing, i.e. no WebSocket connection would be blocked directly, but > we'd still apply a workaround, injecting CSP headers, with the purpose of > countering circumvention of the WebSocket wrapper. Correct me if I'm wrong, but setting the content security policy to "connect-src http:" in a document's response headers effectively blocks all WebSocket connections, which is what is desired here. How is this related to the API wrapper?
On 2018/03/13 23:03:34, Manish Jethani wrote: > Correct me if I'm wrong, but setting the content security policy to "connect-src > http:" in a document's response headers effectively blocks all WebSocket > connections, which is what is desired here. How is this related to the API > wrapper? True, filters like $webrtc,domain=example.com would still work with this workaround, regardless of the WebSocket wrapper. But leaving this code around just for Chrome <=57 still wouldn't be worth it. Anyway, I guess we can tackle that separately (with Dave's upcoming changes). LGTM. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
