Opened 8 months ago

Closed 6 months ago

Last modified 5 months ago

#5042 closed change (fixed)

Handle requests that aren't associated with a browser tab

Reported by: sebastian Assignee: jsonesen
Priority: P3 Milestone: Adblock-Plus-1.13.3-for-Chrome-Opera
Module: Platform Keywords: goodfirstbug
Cc: 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/29418679/

Description (last modified by jsonesen)

Background

Adblock Plus is ignoring any request that isn't associated with any tab, as indicated with a tabId of -1. We did that in order to simplify the code path, by eliminating the scenario in which no context information are available, assuming that those requests aren't relevant anyway.

But now, there are two cases at least in which websites can cause requests that are not associated with the corresponding tab, potentially circumventing Adblock Plus:

Until these Chrome bugs are fixed, there is no way to block these requests based on the context, with filters like *$domain=example.com,websocket,ping. However, it would be possible to match those requests without context information, so that generic filters like ||example.com$websocket,ping work at least.

What to change

Remove the logic (in chrome/ext/background.js) that causes requests with tabId == -1 to be ignored, and adapt the logic down the code path in order to make the page/frame information optional.

Also consider the devtools panel (see lib/devtools.js), which logs requests to the active panel for the tab associated with the request. When the tabId is -1, however, it should log the request to all active panels.

Hints for testers

You can navigate to csp.kzar.co.uk and looking at the adblockplus devtools panel verifying that the shared worker connections are appearing in the list of requests. Additionally, adding the temporary websocket filter there should block the shared worker connections.

Change History (12)

comment:1 follow-up: Changed 8 months ago by kzar

  • Cc trev added

(Adding Wladimir since perhaps he has an opinion too.)

Interesting idea, definitely worth considering. I'm not sure it's a goodfirstbug, since while ostensibly trivial to implement, it's probably pretty hard to get the details correct. Testing might be a little tricky too.

What about a whitelisted website that indirectly creates a request through one of these methods? That could be incorrectly blocked right?

...adapt the logic down the code path in order to make the page/frame information optional.

I didn't look to carefully into it but I suspect that this part is not too trivial. I guess lib/popupblocker.js can simply ignore requests with a tabId of -1 but what about lib/requestBlocker.js?

Since the tabId is expected to be a unique identifier couldn't this cause the code to get confused if there are two of these requests with a tabId of -1? Perhaps we need to need to identify those in a different way somehow?

comment:2 in reply to: ↑ 1 Changed 8 months ago by sebastian

Replying to kzar:

I'm not sure it's a goodfirstbug, since while ostensibly trivial to implement, it's probably pretty hard to get the details correct. Testing might be a little tricky too.

I don't see why this should keep an issue from being marked as goodfirstbug. The only important point is that the change itself is trivial enough, that somebody new to the codebase can realistically get around it. However, no changes are done unsupervised, as there is code review and QA. So I don't share your concerns. I'm more concerned to have nothing for new developers and contributors to work on, in order to get started. Moreover, removing dead code and text changes alone (where you seem to see the bar), aren't sufficient to improve your understanding of the codebase or to keep you interested.

What about a whitelisted website that indirectly creates a request through one of these methods? That could be incorrectly blocked right?

Yes, requests that aren't associated with any document, aren't subject to document-based whitelisting. This doesn't seem any less accurate, however, as completely ignoring those requests, technically one could even argue that this is the expected behavior. Also considering the cases outlined above in which such requests occur, it doesn't seem to cause to much of a problem. If necessary those requests can still be whitelisted individually.

I didn't look to carefully into it but I suspect that this part is not too trivial. I guess lib/popupblocker.js can simply ignore requests with a tabId of -1

It seems that lib/popupBlocker.js is completely ignoring the possibility of sourceTabId being -1, which might not be possible after all. Anyway, the code there isn't affected by the changes suggested in this issue, as it doesn't use ext.onBeforeRequest.

but what about lib/requestBlocker.js?

Yes, the ext.onBeforeRequest handler in lib/requestBlocker.js must be updated as you adapt the logic down the code path. (The purpose of the "What to change" section is not to point to every single line of code that needs to be changed, otherwise you could just write a patch instead of writing the issue.)

Since the tabId is expected to be a unique identifier couldn't this cause the code to get confused if there are two of these requests with a tabId of -1?

It would cause problems if we'd create Page and Frame objects, and treat -1 as a regular tabId, but that is not the idea. Instead, the page and frame passed to the ext.onBeforeRequest listener should be null, which then just calls defaultMatcher.matchesAny() with an empty documentHost and no sitekey. No other code path needs to be aware of this scenario (except for the devtools panel).

comment:3 Changed 8 months ago by greiner

  • Cc greiner added

When the tabId is -1, however, it should log the request to all active panels.

Wouldn't that cause quite a bit of confusion? At least we could visually indicate that those requests didn't originate from the inspected tab.

comment:4 follow-up: Changed 8 months ago by sebastian

They will be visually (and technically) distinguished by having an empty Document domain. But yeah, if we could also render them differently in the devtools panel, perhaps with an icon showing a tooltip explaining that these requests aren't necessarily related to the current tab, that would be awesome. Do you want to do this? It would have to be done in adblockplusui, independently of the change here, anyway. :)

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

comment:5 in reply to: ↑ 4 Changed 8 months ago by greiner

Replying to sebastian:

They will be visually (and technically) distinguished by having an empty Document domain. But yeah, if we could also render them differently in the devtools panel, perhaps with an icon showing a tooltip explaining that these requests aren't necessarily related to the current tab, that would be awesome. Do you want to do this? It would have to be done in adblockplusui, independently of the change here, anyway. :)

I created #5093 so that it can be worked on independently. We need some feedback from the Product team on that though.
But given that those entries at least won't have a domain, it doesn't seem necessary to block it on #5093 so no objections from me against going ahead with this in parallel.

comment:6 Changed 7 months ago by jsonesen

  • Owner set to jsonesen

comment:7 Changed 7 months ago by jsonesen

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

comment:9 Changed 6 months ago by jsonesen

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:10 Changed 6 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-next

comment:11 Changed 5 months ago by jsonesen

  • Description modified (diff)

comment:12 Changed 5 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Working as described.

ABP 1.13.2.1785
Chrome 49 / 58 / Windows 7
Opera 36 / 45 / Windows 7

Note: See TracTickets for help on using tickets.