1669731 - Record a flag complementary to STATE_UNBLOCKED_UNSAFE_CONTENT to indicate that a channel was blocked by an actor external to URL Classifier

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, task, P1)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

Currently, when unblock() is called on a nsIUrlClassifierBlockedChannel, STATE_UNBLOCKED_UNSAFE_CONTENT is set as a content blocking flag. However, the way that unblock() is currently being used, the channel might still be "blocked" by another mechanism e.g. shimming.

What this means, is that actors calling unblock have no way to communicate to other actors (i.e. the protections panel UI), whether another form of blocking was done.

What we can do is allow the caller of unblock() to pass a boolean that indicates whether the resource will actually be blocked or not. And while we are doing it, maybe it would be good to rename "unblock" to something like "setBlockingOverride" or "captureBlocking" or "overrideBlocking" or something else along those lines.

Depending on the value of this new boolean, we would either set STATE_UNBLOCKED_UNSAFE_CONTENT or a new complementary flag like STATE_OTHER_BLOCKED_UNSAFE_CONTENT (or some other name - maybe we can rename STATE_UNBLOCKED_UNSAFE_CONTENT too if it helps to establish a pair of consistent names).

So finally:

1. unblock() accepts true or false
2. If true, set STATE_UNBLOCKED_UNSAFE_CONTENT, else set STATE_OTHER_BLOCKED_UNSAFE_CONTENT
3. Rename unblock to something nicer
4. Rename the state flags to something nicer

Dimi, Tom, what do you think? I think this is consistent with what we discussed last week. :)

Comment 1

[Dimi Lee [:dimi]]

(In reply to Nihanth Subramanya [:nhnt11] from comment #0)

Dimi, Tom, what do you think? I think this is consistent with what we discussed last week. :)

Yes, I think the idea is exactly what we discussed last week.

Comment 2

[Dimi Lee [:dimi]]

Attachment: Bug 1669731 - P1. Rename STATE_UNBLOCKED_UNSAFE_CONTENT to STATE_UNBLOCKED_TRACKING_CONTENT

unsafe content is for Safe Browsing(phishing, malware, etc), we should use tracking content instead.

Comment 3

[Dimi Lee [:dimi]]

Attachment: Bug 1669731 - P2. Rename REPLACED_UNSAFE_CONTENT to REPLACED_TRACKING_CONTENT

unsafe content is for phishing sites, we should use tracking content instead.

Depends on D93269

Comment 4

[Dimi Lee [:dimi]]

Attachment: Bug 1669731 - P3. Add Allow API to ChannelClassifierService.

UI needs to distinguish the cases when a channel is shimmed and is
unshimmed.
When a channel is unshimmed, we unblock the channel and simply treat the channel as a
non-tracking channel.
When a channel is shimmed, although the channel is unblocked by
URLCLassifier, we still want to show it in the UI. For this case,
URLClassifier will notify a content blocking event when a channel is
unblocked.

This patch adds a new allow API, so the caller can use unblock() or
allow() depending upon the case it requires.

Depends on D93270

Comment 5

[Dimi Lee [:dimi]]

Attachment: Bug 1669731 - P4. Add a testcase

Depends on D93271

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/98ea2a2e6e4e
P1. Rename STATE_UNBLOCKED_UNSAFE_CONTENT to STATE_UNBLOCKED_TRACKING_CONTENT r=timhuang
https://hg.mozilla.org/integration/autoland/rev/a66138ae984b
P2. Rename REPLACED_UNSAFE_CONTENT to REPLACED_TRACKING_CONTENT r=geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/9754af354ad7
P3. Add Allow API to ChannelClassifierService. r=timhuang,nhnt11
https://hg.mozilla.org/integration/autoland/rev/acccd84e203f
P4. Add a testcase r=timhuang

Comment 7

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/98ea2a2e6e4e
https://hg.mozilla.org/mozilla-central/rev/a66138ae984b
https://hg.mozilla.org/mozilla-central/rev/9754af354ad7
https://hg.mozilla.org/mozilla-central/rev/acccd84e203f