Opened 7 months ago

Closed 6 months ago

Last modified 4 months ago

#5027 closed change (fixed)

Intercept WebSocket connections using the webRequest API

Reported by: sebastian Assignee: jsonesen
Priority: P3 Milestone: Adblock-Plus-1.13.3-for-Chrome-Opera
Module: Platform Keywords: goodfirstbug
Cc: kzar, jsonesen, wspee, mapx, Lain_13, fanboy, trev, greiner Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29394585/

Description (last modified by jsonesen)

Background

Before Chrome 58 (currently in beta) the webRequest API couldn't intercept WebSocket connections. So we implemented a workaround with #1727 and #4372. However, using the webRequest API in order to intercept WebSockets (when possible) is preferable, and we eventually want to get rid of that workaround.

What to change

Add patterns for the WebSocket protocol (i.e. ws://*/* and wss://*/*) to the urls filter when registering the chrome.webRequest.onBeforeRequest handler in the request blocking code (if possible just use <all_urls> here), as well to the permissions in metadata.chrome.

Ignore WebSocket connections reported by the WebSocket wrapper (#1727) and don't inject CSP headers (#4372), if the webRequest API supports WebSockets (see chrome.webRequest.ResourceType).

How to test

Make sure no WebSockets are opened (except the one created by the SharedWorker, see the discussion below) by the test page at http://csp.kzar.co.uk/, both, on Chrome >=58 when not injecting a CSP or using the WebSocket wrapper, and on Chrome <58 when still relying on that workaround. Note that most of the WebSocket connections are listed in the "Network" panel of the developer tools, but not all. For some you need to watch the console log messages to see if it was successful or not.

WebSocket connections should also be logged, and indicate whether they have been blocked, in the "Adblock Plus" devtools panel. Crosscheck with the method described above, in order to verify that the "Adblock Plus" devtools panel is working correctly. WebSockets created by SharedWorkers not showing up here, is expected. Furthermore, on Chrome <58, only connections intercepted by the WebSockets wrapper, not by CSP injection, are expected to show up.

Permissions are being added here for websocket connections, we need make sure the user does not get permissions warnings: https://developer.chrome.com/extensions/permission_warnings upon upgrading to the latest devbuild in the webstore. Install a previous devbuild and test that upgrading to the latest devbuild works on an old (version Chrome 49) and a new (>= version 58) version of Chrome.

Change History (29)

comment:1 Changed 7 months ago by jsonesen

  • Owner set to jsonesen

comment:2 Changed 7 months ago by sebastian

  • Keywords goodfirstbug added; goofirstbug removed

comment:3 Changed 7 months ago by kzar

  • Ready unset

... and don't inject CSP headers (#4372), if the webRequest API supports WebSockets

I'm not sure that's a good idea since some WebSocket connections might not be caught by the new API. I know even the Firefox APIs struggles to block some of the cases that the CSP can, for examples see my test page http://csp.kzar.co.uk

Perhaps we should test that they really are all blocked before we decide to disable CSP injection.

comment:4 Changed 7 months ago by sebastian

  • Ready set

I don't see any reason why the webRequest API should miss WebSocket connections created in frames loaded from a blob: or data: URL, which is the reason we inject those CSP headers. And if it does, this probably isn't limited to WebSocket connections. If necessary we can still enable that workaround again, but lets be optimistic and hope that we don't need to. Otherwise, we will figure out during testing. Feel free to add your test cases to the issue description.

comment:5 Changed 7 months ago by sebastian

  • Description modified (diff)

comment:6 Changed 7 months ago by kzar

  • Description modified (diff)

comment:7 in reply to: ↑ description ; follow-up: Changed 7 months ago by sebastian

Replying to kzar:

Before pushing this change...

I think this should rather go into a "Hints for testers" section.

Note most of the WebSocket connections are listed in the Network pane of the developer tools, but not all. For the other one you need to watch the console log messages to see if it was successful or not.

Actually, if we no longer inject CSP headers, but block WebSocket connections only using the webRequest API, everything that is blocked should be logged in the devtools panel.

Last edited 7 months ago by sebastian (previous) (diff)

comment:8 in reply to: ↑ 7 Changed 7 months ago by kzar

Replying to sebastian:

Replying to kzar:

Before pushing this change...

I think this should rather go into a "Hints for testers" section.

Feel free to move that.

Actually, if we no longer inject CSP headers, but block WebSocket connections only using the webRequest API, everything that is blocked should be logged in the devtools panel.

Try that page for yourself, open the test page, don't inject a CSP and watch the console log. Notice how that particular WebSocket connection does not show up in the network pane at all.

comment:9 Changed 7 months ago by sebastian

You are right. I was thinking about the Adblock Plus devtools panel.

Last edited 7 months ago by sebastian (previous) (diff)

comment:10 Changed 7 months ago by mapx

  • Cc mapx added

comment:11 Changed 7 months ago by sebastian

  • Description modified (diff)

comment:12 Changed 7 months ago by sebastian

  • Description modified (diff)

comment:13 Changed 7 months ago by jsonesen

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:14 Changed 7 months ago by sebastian

As pointed out in the review, and discussed on IRC, WebSockets in SharedWorkers would indeed no longer be blocked if we land this change (as specified in the current issue description), on Chrome >=58, bringing back the issue reported in #4866.

As discussed in ticket:4770#comment:9, that is because SharedWorkers are not associated with the context in which they were created. But I still think that this minor regression is preferable over keep performing the workaround, injecting CSP headers (#4372), here, for following reasons:

  • The problem here is by no means limited or specific to WebSockets (as pointed out in the other discussion). The problem is rather that SharedWorkers, being potentially shared between multiple documents, are not associated with any of them, causing problems blocking any request (not only WebSocket connections) from within the SharedWorker.
  • This workaround is causing false positives, as with filters like *$domain=example.com,websocket, not only WebSocket connections, but also all workers and non-HTTP(s) frames are blocked, regardless whether they are misused to circumvent WebSocket blocking or not.
  • Messing with the actual content/traffic (either on the HTTP or DOM level) should generally be avoided, except there is a strong case to make an exception, as these approaches tend to be error-prone and cause side effects, not to mention legal aspects. Since WebSocket connections can be blocked using the webRequest API now (except in the SharedWorker scenario, which again isn't directly related to WebSockets), I don't see a strong case here anymore.
  • Without relying on CSP, all requests that can potentially be blocked are logged in the "Adblock Plus" developer tools panel again.

So I suggest the following:

  1. Go ahead with this change, disabling CSP injection on Chrome >=58.
  2. File a Chrome bug, and figure out whether the Chromium team would consider setting the tabId/frameId for SharedWorkers to correspond to the document that caused it to be created, even though there might be multiple documents using it, which is the only way to properly solve the actual problem.
  3. If the above doesn't get addressed in Chrome, but the symptoms become important enough to justify a workaround like this, consider re-implementing CSP injection with a new filter option that doesn't imply blocking of WebSockets.
Last edited 7 months ago by sebastian (previous) (diff)

comment:15 Changed 7 months ago by mapx

  • Cc Lain_13 added

comment:16 Changed 7 months ago by mapx

  • Cc fanboy added

comment:17 follow-up: Changed 7 months ago by kzar

  • Cc trev added
  • Ready unset

But I still think that this minor regression is preferable over keep performing the workaround, injecting CSP headers

I see your point but I somewhat disagree.

  1. This is not a minor regression, we want to reduce the available methods of circumvention and not increase them. We're already behind uBlock in many ways there and this will make it worse.
  2. We cannot remove the CSP injection code (or WebSocket wrapper) until Chrome >= 58 is our minimum supported version anyway.

So as far as I see it rushing ahead here will just mean more adverts for our users in practice.

I rather suggest that you guys:

  1. Open that Chromium issue you described about SharedWorkers.
  2. Go ahead with the logic to disable the WebSocket wrapper for Chrome versions >= 58.
  3. Hold back from disabling CSP injection until the Chromium issue is resolved.
  4. If the Chromium issue takes too long to resolve and it is holding us back from removing the CSP code after >= 58 is the minimum Chrome version we can look into adding the new filter option you suggest.

As far as I see it, this is a choice between some vision of code purity and effective ad blocking. I'd rather choose the later, and besides lib/csp.js is a pretty much self-contained module which will be easy to remove at any time in the future. Why rush?

P.S. I don't think this issue is Ready, but you do. What happens in this situation? Does the module owner's opinion simply take priority over the peer's?

comment:18 in reply to: ↑ 17 ; follow-up: Changed 7 months ago by sebastian

Replying to kzar:

This is not a minor regression, we want to reduce the available methods of circumvention and not increase them. We're already behind uBlock in many ways there and this will make it worse.

If we use the webRequest API, instead of relying on CSP injection, we will generally block WebSocket connections more reliable. Implicit blocking of workers (and non-HTTP/S frames), as done by the CSP injection workaround, doesn't result in any more accurrate blocking, but just trades in some false negatives for some false positive. Even if we ignore the potential false positives here, this workaround is not very effective, as those websites circumventing us this way will just use XHR instead of WebSockets now, which still cannot be blocked when sent by SharedWorkers.

As far as I see it, this is a choice between some vision of code purity and effective ad blocking.

It is not, I didn't even make the point about code quality in the perspective of eventually getting rid of that workaround code all together, but pointed out quite some other problems. FWIW, I would never have LGMT'd the "fix" in #4866, for these reasons, but it landed as I was unavailable when it was reviewed. Otherwise, we wouldn't have this discussion now, in the first place.

P.S. I don't think this issue is Ready, but you do. What happens in this situation? Does the module owner's opinion simply take priority over the peer's?

We should try to find a consensus. But if we fail, the super reviewer, that is trev, breaks tie. I'm interested to hear his opinion here, anyway.

comment:19 in reply to: ↑ 18 Changed 7 months ago by kzar

  • Ready set

Replying to sebastian:

FWIW, I would never have LGMT'd the "fix" in #4866, for these reasons, but it landed as I was unavailable when it was reviewed. Otherwise, we wouldn't have this discussion now, in the first place.

I am sorry you disagree with a change I made in your absence, but I wonder why you didn't talk to me about that or mention it in the corresponding issue. I wonder if anything else I did whilst you were away was wrong in your opinion.

Let's just go with what you think regarding this issue.

comment:20 Changed 7 months ago by sebastian

  • Description modified (diff)

comment:21 Changed 7 months ago by jsonesen

  • Description modified (diff)

comment:22 Changed 7 months ago by kzar

...make sure the user does not get permissions warnings upon upgrading to Chrome >= 58

This isn't right. We need to test that upgrading Adblock Plus works, not that upgrading Chrome works.

The instructions should be something like to install the previous devbuild, then testing that upgrading to a devbuild containing these extra permissions works. That needs to be tried on both an old (version 49) and new (>= version 58) version of Chrome. Opera also needs to be tested I guess.

comment:23 Changed 7 months ago by jsonesen

  • Description modified (diff)

comment:24 Changed 7 months ago by abpbot

A commit referencing this issue has landed:
Issue 5027 - Use updated webRequest API for WebSocket blocking

comment:25 Changed 7 months ago by sebastian

Due to this change, the upload to the Chrome Web Store currently fails. While Chrome requires adding permissions for the ws:// and wss:// protocol schemes, the validation in the Chrome Web Store still rejects extensions that use them.

trev suggested to use <all_urls> which, however, also matches file:// URLs. But it seems we could use *://*/* instead which doesn't seem to imply file:// URLs.

comment:26 Changed 7 months ago by sebastian

Apparently *://*/* doesn't match ws[s]:// URLs either. So as discussed on IRC, we will wait now, hoping that the Chrome Web Store will fix their validation soon.

comment:27 Changed 7 months ago by greiner

  • Cc greiner added

comment:28 Changed 6 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

For reference, we are using the <all_urls> permission, see #5130, and devbuilds are back, now.

comment:29 Changed 4 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Working as described in the how to test notes. Websocket connections can be blocked in both 49 or 59. The connections appear in the devtools panel. Sharedworker websockets are not blocked. Upgrading to the new devbuild with added permissions works okay/without errors.

ABP 1.13.2.1785
Chrome 49 / 59 / Windows 7

Note: See TracTickets for help on using tickets.