Intercept WebSocket connections using the webRequest API

[sebastian]

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.

[kzar]

... 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.

[sebastian]

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.

[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.

[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.

[sebastian]

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

[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.

[kzar]

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?

[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.

[kzar]

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.

[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.

[abpbot]

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

[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.

[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.

[sebastian]

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

[Ross]

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