uBlockOrigin / uMatrix-issues

This is the community-maintained issue tracker for uMatrix
https://github.com/gorhill/uMatrix
123 stars 17 forks source link

Subdomains of blocklisted domains are incorrectly displayed as explicitly blocked #284

Open xofe opened 4 years ago

xofe commented 4 years ago

Prerequisites

Description

Subdomains of blocklisted domains are incorrectly displayed as explicitly blocked, rather than inherited blocked. This does not occur in v1.4.0. git bisect blames https://github.com/gorhill/uMatrix/commit/9b292304d33a44465922200efa5f8b378d0f9604.

A specific URL where the issue occurs

http://xofe.mm.st/umatrix-subdomain-test/

Site source:

<img src="//test-domain.invalid/">
<img src="//domain-not-in-list.test-domain.invalid/">
<img src="//domain-in-list.test-domain.invalid/">

Using hosts list: http://xofe.mm.st/umatrix-subdomain-test/blocklist.txt

List source:

0.0.0.0 test-domain.invalid
0.0.0.0 domain-in-list.test-domain.invalid

Steps to Reproduce

  1. Open clean browser profile and install uMatrix latest.
  2. Add hosts list http://xofe.mm.st/umatrix-subdomain-test/blocklist.txt.
  3. Navigate to http://xofe.mm.st/umatrix-subdomain-test/.
  4. Open uMatrix popup and notice domain-not-in-list.test-domain.invalid is dark red (explicit block) rather than pale red (inherited block)

Ruleset

Default ruleset

Supporting evidence

Version 1.4.0: 1 4 0

Version 1.4.1b6: 1 4 1

Your environment

gorhill commented 4 years ago

Pretty sure it's a side effect of now using HNTrie. Off the top of my head, this should be fixable by ensuring the hostname wholly matches here, i.e. if ubiquitousBlacklistRef.matches() returns > 0, it's an inherited block.

gorhill commented 4 years ago

@xofe You have contributed pull requests in the past, would you be willing to fix the issue here and submit a pull request?

xofe commented 4 years ago

Sure, I can do that.

Checking ubiquitousBlacklistRef.matches() is 0 fixes the specfic case I wrote above, but would mean domain-in-list.test-domain.invalid is not explicitly blocked after the change (despite appearing in blocklist), as HNTrie currently includes only test-domain.invalid, not any subdomains.

To fix this completely, I think it would require changes in HNTrie too. Namely, reverting part of https://github.com/gorhill/uBlock/commit/adabb56d for uMatrix so that blocklisted subdomains of already blocked domains are still added to the trie, and removing the early exit when a domain match is found here: https://github.com/gorhill/uMatrix/blob/0bcb766/src/js/hntrie.js#L222-L224

Does this sound right to you?

gorhill commented 4 years ago

blocklisted subdomains of already blocked domains are still added to the trie

I think this should be part of another issue. The fix to this is also not so trivial because this would require to also revisit the WASM code.

Also, I am also not sure this needs fixing, this can be argued that there is little to be gained to include all subdomains given that uMatrix is naturally meant to be used in a block-all/allow-exceptionally manner.

xofe commented 4 years ago

Fair points, I'll leave that part alone then.

I've made a pull request for this (and a few other minor fixes) at https://github.com/gorhill/uMatrix/pull/1015, here's the popup after the change: