Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable blocking of service worker requests in Chrome #82

Closed
wants to merge 1 commit into from
Closed

Enable blocking of service worker requests in Chrome #82

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions modules/webrequest-pipeline/sources/webrequest-context.es
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

import { parse } from '../core/url';
import logger from './logger';

import { isChromium } from '../core/platform';

/**
* Transform an array of headers (i.e.: `{ name, value }`) into a `Map`.
Expand Down Expand Up @@ -65,8 +65,8 @@ export default class WebRequestContext {

// Cliqz-specific extensions to webRequest details
context.page = page;
context.tabUrl = context.tabUrl || (page && page.getTabUrl());
context.frameUrl = context.frameUrl || (page && page.getFrameUrl(context));
context.tabUrl = context.tabUrl || (page && page.getTabUrl()) || context.url;
context.frameUrl = context.frameUrl || (page && page.getFrameUrl(context)) || context.url;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this needs a change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if it is sound to fill it with the url. But the reason that I added it was that there are multiple places where the adblocker assumes that it is present, for instance here:

common/modules/adblocker/sources/adblocker.es

Line 235 in 319cc4f

if (context.urlParts === null || context.frameUrlParts === null) {

And later it would throw in the whitelist check:

common/modules/adblocker/sources/adblocker.es

Line 255 in 319cc4f

if (this.whitelistChecks[i](context) === true) {

Copy link
Member Author

@philipp-classen philipp-classen Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this here is the main problem still. Filling it in like this is risky. Potentially we could apply it only if it is a request from a service worker; but even then, it is not clear which URL to fill in.

The other question is if it is sound to even attempt to fill it. But IMHO you can argue that it is:

  • It matches the behavior on Firefox
  • The initiator should be very close to what so far was exclusively the tab

But conceptually cleaner might be to put the service worker information in a separate field. (But with the consequence that all places where it is used will need to be revisited. Through the whole stack down to the adblocker library.)

context.isPrivate = page ? page.isPrivate : null;
context.isMainFrame = context.type === 'main_frame';
context.isRedirect = page && context.isMainFrame && page.isRedirect;
Expand Down Expand Up @@ -128,6 +128,19 @@ export default class WebRequestContext {
}

isBackgroundRequest() {
return this.tabId === -1;
if (this.tabId !== -1) {
return false;
}
// On Firefox, requests from Service Workers have tabId !== -1.
// On Chromium, tabId === -1, so check additionally for the initiator.
// Requests from the service worker use a different protocol
// (e.g. https). If there is a more direct way to detect requests
// from the extension, this could be simplified.
if (isChromium && !this.initiator.startsWith('chrome-extensions://')) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about Opera and Edge? maybe we should check for http/https instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

common/platforms/webextension/platform.es

Line 19 in 319cc4f

// Note: Chrome-forks such as Edge or Opera will also be reported

isChromium covers Chromium based browsers including Opera and Edge.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

http/https and ws/wss. But I tried to approach it from the other direction and identify requests from the extension. I think a better name for "isBackground" would be something in the line of "is it from the extension?".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've checked Opera and Edge and both use same chrome-extensions protocol

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the change here (not the URL filling) would be safe. On isolation, it will not make a difference though because the request will still be skipped because of the missing fields.

return false;
}

// internal requests from the extension
return true;
}
}