1612378 - Make CheckContentBlockingAllowList fission compatible

Status: RESOLVED FIXED

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

Description

[Dimi Lee [:dimi]]

See here. It calls top-level document's GetContentBlockingAllowListPrincipal

Comment 1

[Dimi Lee [:dimi]]

We are going to move this to CookieSetting, details can be found in the design doc.

Comment 2

[Chris Peterson [:cpeterson]]

Tracking for Fission Nightly (M6)

Comment 3

[Tim Huang[:timhuang]]

In Bug 1615966, it will change the place where we compute the ContentBlockingAllowList from the content to the parent. This would also affect how do we set the IsOnContentBlockingAllowList in the CookieJarSettings. Here's the idea as steps.
1. Channel be created with the LoadInfo which has the CookieJarSettings, but it doesn't have the flag yet since the final uri is unknown here.
2. Channel compute the ContentBlockingAllowListPrincpal in parent.
3. Set the flag in the CookieJarSettings of the channel's loadInfo when the ContentBlockingAllowList been set to the Channel here.
4. The child will get the CookieJArSettings and the flag when the document starts to load.

Ehsan, what do you think? Is this a viable approach to set the IsOnContentBlockingAllowList in the CookieJarSettings?

Comment 4

[(no longer active)]

So once Matt's patches landed, I'd like us to do one thing first. That is, look into why we needed AntiTrackingCommon::RecomputeContentBlockingAllowListPrincipal() again. Matt was asking me about this the other day and I couldn't remember the reason. All I remembered was that I added that code to make some test failure pass, but I couldn't remember what exactly. Basically I'd like us to comment it all out and see what breaks. :-) In the ideal scenario we'd be able to remove all of that code and everything that calls it.

After that, the other thing to look at is what remaining cases we have for accessing the content blocking allow list principal in the content process and where those access happen off of. If all of those places have a channel around, then perhaps leaving this information on the load info is sufficient and we won't have to do anything more.

Basically I think bug 1615966 changes enough things that we should take a step back and re-evaluate whether it even makes sense to put this principal in the cookie jar settings at all? Maybe leaving it in load info would be all that's needed after we investigate the issue in the first paragraph... What do you think?

Comment 5

[Tim Huang[:timhuang]]

(In reply to :ehsan akhgari from comment #4)

Basically I think bug 1615966 changes enough things that we should take a step back and re-evaluate whether it even makes sense to put this principal in the cookie jar settings at all? Maybe leaving it in load info would be all that's needed after we investigate the issue in the first paragraph... What do you think?

We won't store this principal in the cookie jar settings. We will only store a boolean which indicates whether the top-level site is on the content blocking allow list. IIUC, we would know this when we compute the ContentBlockingAllowList in the parent and set it into the CookieJarSettings in the channel's loadInfo. So, the content can directly check this through this boolean in the CookieJarSettings.

I think we can remove the ContentBlockingAllowListPrincipal entirely from the content with this. But, we still need it in the parent since the ContentBlockingAllowList.jsm needs it. And I think we can compute this when OnLocationChange() event happens.

What do you think?

Comment 6

[Dimi Lee [:dimi]]

(In reply to :ehsan akhgari from comment #4)

So once Matt's patches landed, I'd like us to do one thing first. That is, look into why we needed AntiTrackingCommon::RecomputeContentBlockingAllowListPrincipal() again. Matt was asking me about this the other day and I couldn't remember the reason. All I remembered was that I added that code to make some test failure pass, but I couldn't remember what exactly. Basically I'd like us to comment it all out and see what breaks. :-) In the ideal scenario we'd be able to remove all of that code and everything that calls it.

Hi Ehsan,
I spent some time reading why we add RecomputeContentBlockingAllowListPrincipal and I have some questions.
Below is my understanding, please correct me if anything is wrong, thanks!

According to the comment, Recompute is added to address top-level navigation: about:blank -> example.com
If i understand correctly, the flow is:

1. We are navigating from about:blank to example.com
2. While loading channel example.com, we use top-level window to calculate ContentBlockingAllowListPrincipal. [link]
3. At this point, the top-level window is still about:blank. We detect “about:blank” by using IsNullPrincipal, if it is, we recompute the principal with “aURIBeingLoaded”, which is “example.com”.
4. The document’s contentBlockingAllowListPrincipal is also updated(One question is that why we need to update mContentBlockingAllowListPrincipal of about:blank’s document)[link]

But this makes me think, what if we are navigating from A.com -> B.com

In the step3 above, because the top-level window is not about:blank, we will use the current top-level window's contentBlockingAllowListprincipal as the loading channel's allowlist principal, which is A.com. If this is true, I guess this might cause some bugs?(Bug 1543001) and the patch also has the same issue.

I think the solution should be that if it is top-level load(check TYPE_DOCUMENT for example), use the channel's URI to compute contentBlockingAllowListprincipal?

Comment 7

[Dimi Lee [:dimi]]

I’ll propose we store the result of IsOnContentBlockingAllowList in WindowContext.

When we get mDocContentBlockingAllowListPrincipal in WindowGlobalParent and it is a top-level window, calculate the result of IsOnContentBlockingAllowList and store it in WindowContext's sync field.

If we have a window and want to test if it is in the allow list, find the top-level WindowContext and get the flag(maybe we should also assert if accessing the flag in non top-level WindowContext).
If we have a channel and want to test if it is in the allow list, use nsIHttpChannelInternal.contentBlockingAllowListPrincipal.

Comment 8

[(no longer active)]

(In reply to Dimi Lee [:dimi][:dlee] from comment #7)

I’ll propose we store the result of IsOnContentBlockingAllowList in WindowContext.

When we get mDocContentBlockingAllowListPrincipal in WindowGlobalParent and it is a top-level window, calculate the result of IsOnContentBlockingAllowList and store it in WindowContext's sync field.

That sounds good!

If we have a window and want to test if it is in the allow list, find the top-level WindowContext and get the flag(maybe we should also assert if accessing the flag in non top-level WindowContext).

Yes, the flag should only be accessed on top-level windows.

If we have a channel and want to test if it is in the allow list, use nsIHttpChannelInternal.contentBlockingAllowListPrincipal.

Good, though if we can avoid doing this check in the content process perhaps we can use the WindowContext field there also?

Comment 9

[(no longer active)]

(In reply to Dimi Lee [:dimi][:dlee] from comment #6)

Hi Ehsan,
I spent some time reading why we add RecomputeContentBlockingAllowListPrincipal and I have some questions.
Below is my understanding, please correct me if anything is wrong, thanks!

According to the comment, Recompute is added to address top-level navigation: about:blank -> example.com

Aha, yes! Thanks for digging that up.

If i understand correctly, the flow is:

1. We are navigating from about:blank to example.com
2. While loading channel example.com, we use top-level window to calculate ContentBlockingAllowListPrincipal. [link]
3. At this point, the top-level window is still about:blank. We detect “about:blank” by using IsNullPrincipal, if it is, we recompute the principal with “aURIBeingLoaded”, which is “example.com”.
4. The document’s contentBlockingAllowListPrincipal is also updated(One question is that why we need to update mContentBlockingAllowListPrincipal of about:blank’s document)[link]

That sounds like a mistake...

But this makes me think, what if we are navigating from A.com -> B.com

In the step3 above, because the top-level window is not about:blank, we will use the current top-level window's contentBlockingAllowListprincipal as the loading channel's allowlist principal, which is A.com. If this is true, I guess this might cause some bugs?(Bug 1543001) and the patch also has the same issue.

I think there's a bug here, but one thing that I didn't understand in your description was what's A.com in this example?

I think the solution should be that if it is top-level load(check TYPE_DOCUMENT for example), use the channel's URI to compute contentBlockingAllowListprincipal?

That makes sense, it's very similar to what the old code did.

Comment 10

[Dimi Lee [:dimi]]

(In reply to :ehsan akhgari from comment #9)

(In reply to Dimi Lee [:dimi][:dlee] from comment #6)

In the step3 above, because the top-level window is not about:blank, we will use the current top-level window's contentBlockingAllowListprincipal as the loading channel's allowlist principal, which is A.com. If this is true, I guess this might cause some bugs?(Bug 1543001) and the patch also has the same issue.

I think there's a bug here, but one thing that I didn't understand in your description was what's A.com in this example?

In the example, A.com is the top-level site before navigating.
Replace A.com to old-top.com & B.com to new-top.com and explain the flow again to see if this is clearer:

1. old-top.com is the top-level window.
2. Do top-level navigation: from old-top.com -> new-top.com
3. Start loading channel new-top.com , call DocumentChannelChild::AsyncOpen(new-top.com)
4. DocumentChannelChild::AsyncOpen call ThirdPartyUtil::GetTopWindowForChannel(new-top.com), but at this point, GetTopWindowForChannel(new-top.com) returns old-top.com.
5. Set old-top.com to channel new-top.com's contentBlockingAllowListprincipal

Comment 11

[Tim Huang[:timhuang]]

I can confirm the concern of Dimi.

The problem occurs when we do a top-level load from an existing site, say old-top.com, to a new site, say new-top.com. And the top-level load here means we type in the URL bar, "new-top.com", and press enter. In this case, we won't recompute the ContentBlockingAllowListPrincipal here. It seems we will still use the ContentBlockingAllowListPrinicpal from the top-level window, which is "old-top.com". And the recomputation doesn't happen because this is not a NullPrincipal. So, the channel here will use the wrong ContentBlockingAllowListPrincipal, which is "old-top.com", while loading the "new-top.com".

Given this, I think we should recompute the ContentBlockingAllowListPrincipal if it is the top-level load like Dimi suggested in comment 6.

Comment 12

[(no longer active)]

Yes, that definitely sounds like a pretty bad bug. Thanks a lot for catching it!

Comment 13

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 1: Fix the issue that CookieJarSettings is not synced to the WindowContext. r?dimi

We sync the CookieJarSettings to the WindowContext in
Document::StartDocumentLoad(). This is incorrect because the inner
window haven't been set to the document in
Document::StartDocumentLoad(). So, the CookieJarSettings doesn't be
synced to the WindowContext properly.

This patch fixes this issue by change the place where we do the sync. It
changes it to do the sync at the end of
nsGlobalWindowOuter::SetNewDocument() where the inner window and the
window context are both ready for the document.

Comment 14

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 1: Add a boolean IsOnContentBlockingAllowList in the CookieJarSettings. r?dimi

In this patch, we add a IsOnContentBlockingAllowList boolean in the
CookieJarSettings. This boolean would be used to indicate whether the
top-level site in in the content blocking allow list.

Depends on D66207

Comment 15

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 2: Update the IsOnContentBlockingAllowList boolean in the CookieJarSettings when doing a top-level load. r?dimi

In this patch, we update the IsOnContentBlockingAllowList boolean in the
CookieJarSettings when doing a top-level load. The boolean will be set
in DocumentLoadListener::Open(). And this boolean would be propagated
with the CookieJarSettings. So, we can check this boolean to see if the
top-level site is in the ContentBlockingAllowList.

Depends on D66208

Comment 16

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 3: Change the ContentBlockingAllowList::Check() to use IsOnContentBlockingAllowList in the CookieJarSettings. r?dimi

This patch changes two ContentBlockingAllowList::Check() functions,
including the window version and the channel version, to use the
IsOnContentBlockingAllowList boolean in the CookieJarSettings.

Depends on D66209

Comment 17

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 4: Remove the ContentBlockingAllowListCache. r?dimi

Due to the fact that we compute the IsOnContentBlockingAllowList in the
parent process and pass it as a flag to the content processes, we don't
need to do the cache anymore. Because the check would be just to read a
bool from the WindowContext. The cost should be low enough that we don't
need a cache anymore.

Depends on D66210

Comment 18

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 5: Make the browser.ContentBlockingAllowListPrincipal to directly get the ContentBlockingAllowListPrincipal from the WindowGlobalParent. r?dimi

Right now, we have a ContentBlockingAllowListPrincipal in the
WindowGlobalParent. So, the browse element can directly get this
principal from there. And we can stop sending the
ContentBlockingAllowListPrincipal from the content to parent when
OnLocationChange happens.

Depends on D66211

Comment 19

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 6: Make UrlClassifierCommon::IsAllowListed() to use the ContentBlockingAllowListPrincipal from the WindowGlobalParent. r?dimi

In this patch, we make the UrlClassifierCommon::IsAllowListed() to get
the ContentBlockingAllowListPrincipal from the WindowGlobalParent
instead of from the channel.

Depends on D66212

Comment 20

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 7: Remove the ContentBlockingAllowListPrincipal from the nsIHttpChannelInternal. r?dimi

We nolonger need to use the ContentBlockingAllowListPrincipal in the
channel because we move to check the IsOnContentBlockingAllowList in the
CookieJarSettings when we do a content blocking allow list check. Also,
we would potentially expose the cross-origin info through the
ContentBlockingAllowListPrincipal in the channel. Hence, we will remove
it from the channel.

Depends on D66213

Comment 21

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 8: Remove "trackingprotection" and "trackingprotection-pb" from the preload permission list. r?dimi

We can remove this two permissions from the preload list since we would
only access these two permissions in the parent process. So, it doesn't
need to be sent to content processes anymore.

Depends on D66214

Comment 22

[Phabricator Automation]

Comment on attachment 9132218 [details]
Bug 1612378 - Part 1: Fix the issue that CookieJarSettings is not synced to the WindowContext. r?dimi

Revision D66207 was moved to bug 1621282. Setting attachment 9132218 [details] to obsolete.

Comment 24

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 9: Make StorageAccess::InternalStorageAllowedCheck() to aslo checks the document's URI when doing a about URI check. r?dimi

In this patch, we make the window version of the
ContentBlockingAllowList::Check() to return true if the window uses the
system principal.

Some system privilege page would also check the
ContentBlockingAllowList, for example, "about:devtools-toolbox". In that case, we
should pass the check.

We need this because we cannot tell if the page has the system privilege
from the IsOnContentBlockingAllowList in the WindowContext. So, we have
to check if the window has the system principal before we check the
IsOnContentBlockingAllowList in the WindowContext.

Depends on D66215

Comment 25

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 10: Move the ContentBlockingAllowList check in the Document::RequestStorageAccess() to use the IsOnContentBlockingAllowList flag. r?dimi

After we move the IsOnContentBlockingAllowList to the CookieJarSettings.
We suppose to use this in the Document::RequestStorageAccess() since
this would be called in content processes.

Depends on D66481

Comment 26

[Tim Huang[:timhuang]]

Attachment: Bug 1612378 - Part 11: Change isOnContentBlockingAllowList in storageAccessAPIHelpers.js to use allowListed flag instead of permission. r?baku

Because we remove the content blocking allow list permission from the
permission preload list. The permission in the third party window might
not be available when we check it. So, we move to check the allowListed
flag which is passed by the test framework instead of check the
permission.

Depends on D66737

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f1be7541d81c
Part 1: Add a boolean IsOnContentBlockingAllowList in the CookieJarSettings. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/a1f77b99a1b8
Part 2: Update the IsOnContentBlockingAllowList boolean in the CookieJarSettings when doing a top-level load. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/1b39850be0be
Part 3: Change the ContentBlockingAllowList::Check() to use IsOnContentBlockingAllowList in the CookieJarSettings. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/b05dfc8127b4
Part 4: Remove the ContentBlockingAllowListCache. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/7f47663d9e02
Part 5: Make the browser.ContentBlockingAllowListPrincipal to directly get the ContentBlockingAllowListPrincipal from the WindowGlobalParent. r=dimi,johannh,baku
https://hg.mozilla.org/integration/autoland/rev/b1858510a2ca
Part 6: Make UrlClassifierCommon::IsAllowListed() to use the ContentBlockingAllowListPrincipal from the WindowGlobalParent. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/8df44770d70f
Part 7: Remove the ContentBlockingAllowListPrincipal from the nsIHttpChannelInternal. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/d9ac725f5a57
Part 8: Remove "trackingprotection" and "trackingprotection-pb" from the preload permission list. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/50d282ea77f1
Part 9: Make StorageAccess::InternalStorageAllowedCheck() to aslo checks the document's URI when doing a about URI check. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/a147f4b085f2
Part 10: Move the ContentBlockingAllowList check in the Document::RequestStorageAccess() to use the IsOnContentBlockingAllowList flag. r=dimi,baku
https://hg.mozilla.org/integration/autoland/rev/120c7c6a1961
Part 11: Change isOnContentBlockingAllowList in storageAccessAPIHelpers.js to use allowListed flag instead of permission. r=baku

Comment 28

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/f1be7541d81c
https://hg.mozilla.org/mozilla-central/rev/a1f77b99a1b8
https://hg.mozilla.org/mozilla-central/rev/1b39850be0be
https://hg.mozilla.org/mozilla-central/rev/b05dfc8127b4
https://hg.mozilla.org/mozilla-central/rev/7f47663d9e02
https://hg.mozilla.org/mozilla-central/rev/b1858510a2ca
https://hg.mozilla.org/mozilla-central/rev/8df44770d70f
https://hg.mozilla.org/mozilla-central/rev/d9ac725f5a57
https://hg.mozilla.org/mozilla-central/rev/50d282ea77f1
https://hg.mozilla.org/mozilla-central/rev/a147f4b085f2
https://hg.mozilla.org/mozilla-central/rev/120c7c6a1961