1589754 - navigator.permissions.query({ name: "notifications" }) always returns { state: "denied" }

Status: RESOLVED FIXED

(Core :: Permission Manager, defect, P1)

Description

[Rob Wu [:robwu]]

STR:

1. Visit https://example.com/
2. Open the devtools and run the following in the console:

onclick = async () => {
 console.log(await Notification.requestPermission());
 console.log(await navigator.permissions.query({name:"notifications"}));
};

2. Click anywhere in the page and approve the permission request.

Expected:

• Step 2 shows: "granted" and { state: "granted" }

Actual:

• Step 2 shows: "granted" and { state: "denied" }

When I flip the permissions.delegation.enable preference from true to false, the expected result happens. Note that I'm able to show a notification with new Notification("titl") , so the response from Notification.requestPermission is accurate, and the result from navigator.permissions.query is really bad.

mozregression points to https://hg.mozilla.org/integration/autoland/rev/fa175de96c828f8d76fd318e127f7ac2b5fe43d5

Comment 1

[Rob Wu [:robwu]]

Upon inspection of the source code, I think that I understand what's happening.

https://searchfox.org/mozilla-central/rev/696cf3b52a9aa04aa5013008a8b448904a298356/extensions/permissions/PermissionDelegateHandler.cpp#91-102

  nsIPrincipal* topPrincipal = innerWindow->GetTopLevelPrincipal();

  // Permission is delegated in same origin
  if (principal->Subsumes(topPrincipal)) {
    return permMgr->TestPermissionFromPrincipal(topPrincipal, aType,
                                                aPermission);
  }

  if (info->mPolicy == DelegatePolicy::ePersistDeniedCrossOrigin) {
    *aPermission = nsIPermissionManager::DENY_ACTION;
    return NS_OK;
  }

topPrincipal is nullptr, because nsGlobalWindowInner::GetTopLevelPrincipal returns nullptr when cookie behavior is set to BEHAVIOR_REJECT_TRACKER. This logic was introduced by bug 1480131 (unconditionally) and bug 1577298 (limit to when tracking cookies are blocked), and is also documented in nsGlobalWindowInner.h.
Confusingly, there are other APIs where topLevelPrincipal is not unset. See e.g. logic in LoadContext. There is even an identically named function GetTopLevelPrincipal in nsContentPermissionHelper.cpp that does the expected thing (i.e. returning the principal instead of nullptr for the top-level document).

I'm going to fix this regression by adding an extra check in PermissionDelegateHandler.cpp

Comment 2

[(no longer active)]

nsGlobalWindowInner::GetTopLevelPrincipal() is a function with special semantics that the anti-tracking backend relies on. The usage of this function everywhere else without reviews from the privacy module peers are probably bugs.

(We should have probably called this GetTopLevelAntiTrackingPrincipal to avoid accidental usage elsewhere...)

If this code needs to access the principal of the top-level document, it needs to do the work of obtaining that properly.

Comment 3

[Rob Wu [:robwu]]

This regression seems to be caused by a misunderstanding of what nsGlobalWindowInner::GetTopLevelPrincipal does.
Some classes "GetTopLevelPrincipal" implementations return the actual "top level principal", whereas nsGlobalWindowInner::GetTopLevelPrincipal returns nullptr in case of the top-level document.

Ehsan - To avoid bugs like this, would it be viable to either change the behavior of the methods, or to rename them?
Is nsGlobalWindowInner::GetTopLevelPrincipal the only thing that would have to be renamed, or are there others?

Comment 4

[(no longer active)]

(In reply to Rob Wu [:robwu] from comment #3)

This regression seems to be caused by a misunderstanding of what nsGlobalWindowInner::GetTopLevelPrincipal does.
Some classes "GetTopLevelPrincipal" implementations return the actual "top level principal", whereas nsGlobalWindowInner::GetTopLevelPrincipal returns nullptr in case of the top-level document.

Indeed.

Ehsan - To avoid bugs like this, would it be viable to either change the behavior of the methods, or to rename them?

Changing the behaviour wouldn't, but changing the name certainly would. There is a paired method nsGlobalWindowInner::GetTopLevelStorageAreaPrincipal(), so it would be nice to think of a name that goes well with that (which is why I suggested GetTopLevelAntiTrackingPrincipal() above) but I'd be open to other suggestions.

Is nsGlobalWindowInner::GetTopLevelPrincipal the only thing that would have to be renamed, or are there others?

Great question! nsILoadInfo.topLevelPrincipal/topLevelStorageAreaPrincipal are the corresponding load info attributes that return the same data essentially (the top-level anti-tracking principal for the window from which we started the load, and so on...)

Comment 5

[Thomas Nguyen (:tnguyen)]

This regression seems to be caused by a misunderstanding of what nsGlobalWindowInner::GetTopLevelPrincipal does.
Some classes "GetTopLevelPrincipal" implementations return the actual "top level principal", whereas nsGlobalWindowInner::GetTopLevelPrincipal returns nullptr in case of the top-level document.

Ehsan - To avoid bugs like this, would it be viable to either change the behavior of the methods, or to rename them?
Is nsGlobalWindowInner::GetTopLevelPrincipal the only thing that would have to be renamed, or are there others?

Oh, that's true, thanks for reporting and taking this. Is it ok if you also fix the behavior of this GetTopLevelPrincipal getting not from top BrowsingContext but LoadInfo instead of (getting from top browsing context seems does not work in Fission)?
The topLevelPrincipal in LoadInfo should be correct because it is passed down the inheritance tree whenever we create a new loadinfo object.
I am not so sure if we should remove or change that behavior of Loadinfo to/not to pass topLevelPrincipal due to Fission's new model, but at the moment we will get a correct result of getTopLevelPrincipal from loadInfo in Fission.

Comment 6

[(no longer active)]

(In reply to Thomas Nguyen (:tnguyen) from comment #5)

This regression seems to be caused by a misunderstanding of what nsGlobalWindowInner::GetTopLevelPrincipal does.
Some classes "GetTopLevelPrincipal" implementations return the actual "top level principal", whereas nsGlobalWindowInner::GetTopLevelPrincipal returns nullptr in case of the top-level document.

Ehsan - To avoid bugs like this, would it be viable to either change the behavior of the methods, or to rename them?
Is nsGlobalWindowInner::GetTopLevelPrincipal the only thing that would have to be renamed, or are there others?

Oh, that's true, thanks for reporting and taking this. Is it ok if you also fix the behavior of this GetTopLevelPrincipal getting not from top BrowsingContext but LoadInfo instead of (getting from top browsing context seems does not work in Fission)?

We cannot use the load info as the source of information here, because the load info's topLevelPrincipal field itself is initialized using the return value of nsGlobalWindowInner::GetTopLevelPrincipal, so your suggestion sounds like a circular dependency...

As you mentioned, the BrowsingContext object doesn't carry the principal in out of process mode.

The topLevelPrincipal in LoadInfo should be correct because it is passed down the inheritance tree whenever we create a new loadinfo object.

Is it? That's kind of news to me to be honest. As far as I know it is currently only propagated through redirects.

I am not so sure if we should remove or change that behavior of Loadinfo to/not to pass topLevelPrincipal due to Fission's new model, but at the moment we will get a correct result of getTopLevelPrincipal from loadInfo in Fission.

I think we should make loadinfo's inherit topLevelPrincipal across the browsing context tree in Fission, and in non-top-level documents read the topLevelPrincipal from our loadinfo. So when a load starts in a top-level context, we compute the top-level principal, make the load info object hold on to it (but return null when external consumers ask for it, because topLevelPrincipal is null at the top-level) and propagate it down to child documents, where it will be non-null. nsGlobalWindowInner's function will have similar semantics.

topLevelStorageAreaPrincipal should be handled very similarly to topLevelPrincipal, except that it will only be non-null for single-level nested documents.

Comment 7

[Thomas Nguyen (:tnguyen)]

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

We cannot use the load info as the source of information here, because the load info's topLevelPrincipal field itself is initialized using the return value of nsGlobalWindowInner::GetTopLevelPrincipal, so your suggestion sounds like a circular dependency...
I think we should make loadinfo's inherit topLevelPrincipal across the browsing context tree in Fission, and in non-top-level documents read the topLevelPrincipal from our loadinfo. So when a load starts in a top-level context, we compute the top-level principal, make the load info object hold on to it (but return null when external consumers ask for it, because topLevelPrincipal is null at the top-level) and propagate it down to child documents, where it will be non-null. nsGlobalWindowInner's function will have similar semantics.

Thanks for clarifying, and agree with your idea. I just tried a simple test and I could see
document->GetChannel()->GetLoadInfo->GetTopLevelPrincipal() works even in cross-process document. I did not try to trace the code path, but it looks like we generated load info's topLevelPrincipal nsGlobalWindowInner::GetTopLevelPrincipal in the correct process then passed it around.

Comment 8

[Rob Wu [:robwu]]

I looked at several methods that are called "GetTopLevelPrincipal", and it is not obvious that some are null and others non-null.

LoadInfo's "topLevelPrincipal" was initially meant to be used for anti-tracking only, but later it was expanded to, well, be the "top level principal", in https://searchfox.org/mozilla-central/diff/a438b12ebdd1c4e6541c3dc7c8dd1e23ad989ca6/netwerk/base/LoadInfo.cpp#195

I'll therefore just limit this bug to the fix itself, and a rename of the nsGlobalWindowInner::GetTopLevelPrincipal method (which may or may not be merged), and let you take care of fixing the topLevelPrincipal stuff over in bug 1587743 .

Comment 9

[Rob Wu [:robwu]]

Attachment: Bug 1589754 - Fix permissions.query in top-level document

Comment 10

[Rob Wu [:robwu]]

Attachment: Bug 1589754 - Rename nsGlobalWindowInner::GetTopLevelPrincipal

Comment 11

[(no longer active)]

(In reply to Rob Wu [:robwu] from comment #8)

LoadInfo's "topLevelPrincipal" was initially meant to be used for anti-tracking only, but later it was expanded to, well, be the "top level principal", in https://searchfox.org/mozilla-central/diff/a438b12ebdd1c4e6541c3dc7c8dd1e23ad989ca6/netwerk/base/LoadInfo.cpp#195

Sorry that last part is inaccurate. The load info's "top level principal" was also added for the usage of the anti-tracking module (in order to propagate this principal across process boundaries.)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/49e887cef91d
Fix permissions.query in top-level document r=tnguyen
https://hg.mozilla.org/integration/autoland/rev/120203cf34b0
Rename nsGlobalWindowInner::GetTopLevelPrincipal r=Ehsan

Comment 13

[Rob Wu [:robwu]]

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

(In reply to Rob Wu [:robwu] from comment #8)

LoadInfo's "topLevelPrincipal" was initially meant to be used for anti-tracking only, but later it was expanded to, well, be the "top level principal", in https://searchfox.org/mozilla-central/diff/a438b12ebdd1c4e6541c3dc7c8dd1e23ad989ca6/netwerk/base/LoadInfo.cpp#195

Sorry that last part is inaccurate. The load info's "top level principal" was also added for the usage of the anti-tracking module (in order to propagate this principal across process boundaries.)

My confusion comes from the following:

• Initially, "topLevelPrincipal" was only set for loads associated with the top level window.
• In the change that I linked in my previous comment, this was extended by assigning "topLevelPrincipal" for non-document loads in top-level windows.
• The topLevelPrincipal declaration in nsILoadInfo.idl is documented to "Return the top-level principal, which is the principal of the top-level window". Based on that documentation alone, I wouldn't expect it to be void in the top-level window. Combined with the previous change, I assumed that the implementation started to get closer to the documented behavior.

Comment 14

[Narcis Beleuzu [:NarcisB]]

Backed out for mochitest failures on test_notification_permissions.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/f0838766c33a5fc6f854892adb6f096d6839a19b
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=272755768&repo=autoland&lineNumber=3010

Comment 15

[Rob Wu [:robwu]]

I am not able to reproduce the failure locally, but I suspect that the SpecialPowers.pushPermissions helper is at fault here, and will file a bug for that.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab5b38edece9
Fix permissions.query in top-level document r=tnguyen
https://hg.mozilla.org/integration/autoland/rev/32af5afdcfe3
Rename nsGlobalWindowInner::GetTopLevelPrincipal r=Ehsan

Comment 17

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/ab5b38edece9
https://hg.mozilla.org/mozilla-central/rev/32af5afdcfe3

Comment 18

[Rob Wu [:robwu]]

Comment on attachment 9103259 [details]
Bug 1589754 - Fix permissions.query in top-level document

Beta/Release Uplift Approval Request

• User impact if declined: The navigator.permissions.query API returns "denied" when the permissions.delegation.enable preference is set to true, which may result in broken functionality on websites. The regression was introduced in Firefox 71, and is currently not visible on Beta because the preference is only on by default on Nightly.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: bug 1591102, bug 1591924
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small change consisting of an updated conditional, verified by unit tests.
• String changes made/needed: none

Comment 19

[(no longer active)]

(In reply to Rob Wu [:robwu] from comment #13)

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

(In reply to Rob Wu [:robwu] from comment #8)

LoadInfo's "topLevelPrincipal" was initially meant to be used for anti-tracking only, but later it was expanded to, well, be the "top level principal", in https://searchfox.org/mozilla-central/diff/a438b12ebdd1c4e6541c3dc7c8dd1e23ad989ca6/netwerk/base/LoadInfo.cpp#195

Sorry that last part is inaccurate. The load info's "top level principal" was also added for the usage of the anti-tracking module (in order to propagate this principal across process boundaries.)

My confusion comes from the following:

• Initially, "topLevelPrincipal" was only set for loads associated with the top level window.
• In the change that I linked in my previous comment, this was extended by assigning "topLevelPrincipal" for non-document loads in top-level windows.
• The topLevelPrincipal declaration in nsILoadInfo.idl is documented to "Return the top-level principal, which is the principal of the top-level window". Based on that documentation alone, I wouldn't expect it to be void in the top-level window. Combined with the previous change, I assumed that the implementation started to get closer to the documented behavior.

Hmm, you're right, looks like the principal on load info has changed its meaning a bit to the point of confusing me also. :-)

Comment 20

[Pascal Chevrel:pascalc]

Rob, if bug 1591924 is needed for this uplift, could you also request an uplift there? thanks

Also, you put in your request "The regression was introduced in Firefox 71, and is currently not visible on Beta because the preference is only on by default on Nightly." If this is a bug in nightly only, why are you requesting an uplift to beta? Thanks

Comment 21

[Rob Wu [:robwu]]

(In reply to Pascal Chevrel:pascalc from comment #20)

If this is a bug in nightly only, why are you requesting an uplift to beta? Thanks

If the preference is flipped, the bug does become visible. Since the fix itself is quite straightforward and this is a recent regression that hasn't reached release yet, I figured that it would be a good candidate for uplifting.

Comment 22

[Pascal Chevrel:pascalc]

Comment on attachment 9103259 [details]
Bug 1589754 - Fix permissions.query in top-level document

Low risk as this is disabled by default and patch is covered by tests, uplift approved for 71 beta 6, thanks.

Comment 23

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/026a184115e2
https://hg.mozilla.org/releases/mozilla-beta/rev/5ada339b86ca

Comment 24

[Arthur Iakab [arthur_iakab]]

Backed out 2 changesets (bug 1589754) for causing mochitest failures on test_notification_permissions.html. a=backout

Backout revision https://hg.mozilla.org/releases/mozilla-beta/rev/ff4b3a93ec13704b7003df465cc6c3a4884dd1f0

Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta&resultStatus=testfailed%2Cbusted%2Cexception&revision=85d1ea939621193269f1fe6d3a65ca817d5b25a3&searchStr=linux%2Cx64%2Cquantumrender%2Cdebug%2Cmochitests%2Ctest-linux64-qr%2Fdebug-mochitest-e10s-6%2Cm%286%29

Failure log https://treeherder.mozilla.org/logviewer.html#?job_id=273880979&repo=mozilla-beta

Rob can you please take a look?

Comment 25

[Rob Wu [:robwu]]

I forgot to also ask for uplift of bug 1591102 (mentioned as dependency at comment 15).

Could you retry, but merge the first patch of bug 1591102 first? I'll file an uplift request in that bug as well.

Uplift order should be:

• bug 1591102 (fixes test helper)
• bug 1589754 (this bug)
• bug 1591924 (fixup test manifest to get test to pass on beta)

Comment 26

[Intermittent Failures Robot]

1 failures in 4314 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• autoland: 1

Platform breakdown:

• windows2012-64-shippable: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1589754&startday=2019-10-28&endday=2019-11-03&tree=all

Comment 27

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8902efec3955
https://hg.mozilla.org/releases/mozilla-beta/rev/bd900d6ea795