1488053 - Remove options.allowInheritPrincipal = true from ext-tabs.js

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P5)

Description

[Rob Wu [:robwu]]

In ext-tabs.js, the allowInheritPrincipal and triggeringPrincipal logic has grown into something complicated.

Initially, ext-tabs.js unconditionally used options.disallowInheritPrincipal=true.
Then it was made conditional, and the motivation for doing so was:

> // Only set disallowInheritPrincipal on non-discardable urls as it
> // will override creating a lazy browser. 

https://searchfox.org/mozilla-central/diff/aad6d3674f408b409b27b8c294515800c28aad1f/browser/components/extensions/parent/ext-tabs.js#572-573

The comment seems to imply that the disallowInheritPrincipal flag should not be touched unless necessary.

In bug 1466801, disallowInheritPrincipal was flipped to allowInheritPrincipal:
https://searchfox.org/mozilla-central/diff/a91f8e81081b54280c841d480331e19f6ba562f4/browser/components/extensions/parent/ext-tabs.js#576

Now the logic is:

> // Only set allowInheritPrincipal on discardable urls as it
> // will override creating a lazy browser.  Setting triggeringPrincipal
> // will ensure other cases are handled, but setting it may prevent
> // creating about and data urls.
> let discardable = url && !url.startsWith("about:");
> if (!discardable) {
>   options.allowInheritPrincipal = false;
>   ...
> } else {
>   options.allowInheritPrincipal = true;
>   ...

https://searchfox.org/mozilla-central/rev/c8483bebfa09173b1db5590b3c19f42270d819c9/browser/components/extensions/parent/ext-tabs.js#571-588

The "allowInheritPrincipal = true;" line should be removed, and the surrounding code can be simplified now that "allowInheritPrincipal" defaults to false.
There won't be behavioral changes, because extensions cannot open data: or javascript:-URLs anyway via browser.tabs.create.

Comment 1

[Rob Wu [:robwu]]

Shane, why was it necessary to limit "disallowInheritPrincipal = true;" to about:-URLs only?
What were the consequences of unconditionally setting (dis)allowInheritPrincipal?

https://searchfox.org/mozilla-central/diff/aad6d3674f408b409b27b8c294515800c28aad1f/browser/components/extensions/parent/ext-tabs.js#572-583

Comment 2

[Jonathan Kingston [:jkt] he/him]

Thanks for filing this, I meant to make a follow up for this too. Also the triggeringPrincipal is calculated twice in this function because of merges. The code is correct but could be equally cleaned up in this bug.

Comment 3

[Jonathan Kingston [:jkt] he/him]

Making moco confidential after initial investigation into the functions based on Comment 2.

Comment 4

[Rob Wu [:robwu]]

There is more code in https://hg.mozilla.org/mozilla-central/rev/fb52c3f74edc684c1fa99b7f909af2a2a9c86a6f
such as https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3806
where the resulting logic has become redundant due to the change from bug 1466801 (where principals became non-inheritable by default).

PS. None of the issues reported here are sensitive, so re-opening the bug should be fine.

Comment 5

[Jonathan Kingston [:jkt] he/him]

I'm not sure if it is redundant mobile/android/components/extensions/ext-tabs.js#284 is still using that code however it should be flipped to be allowInheritPrincipal as all the other call sites are.

I think we are good for security after having a closer look.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=904af94208842016e6f41a4e328df523cda36399

Comment 6

[Jonathan Kingston [:jkt] he/him]

Attachment: Bug 1488053 - Simplify triggeringPrincipal and inheritingPrincipal logic for extensions. r?mixedpuppy

Comment 7

[Jonathan Kingston [:jkt] he/him]

Pretty sure we can't land this as is. The patch creates these test failures and I think this is because of the inheritPrincipal removal here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c1ae2ec2375498773e5495b177481ae78d99b5f

Comment 8

[Rob Wu [:robwu]]

(already answered in Phabricator/IRC)

Comment 9

[Shane Caraveo (:mixedpuppy)]

(In reply to Rob Wu [:robwu] from comment #1)
> Shane, why was it necessary to limit "disallowInheritPrincipal = true;" to
> about:-URLs only?
> What were the consequences of unconditionally setting
> (dis)allowInheritPrincipal?
> 
> https://searchfox.org/mozilla-central/diff/
> aad6d3674f408b409b27b8c294515800c28aad1f/browser/components/extensions/
> parent/ext-tabs.js#572-583

using the new name, allowInheritPrincipal...

If allowInheritPrincipal is false, then b.loadURI is called.  This causes the lazy browser (just created) to become a real browser, preventing the ability to create a discarded tab.

https://searchfox.org/mozilla-central/rev/c3fef66a5b211ea8038c1c132706d02db408093a/browser/base/content/tabbrowser.js#2493

Comment 10

[Rob Wu [:robwu]]

The (dis)allowInheritPrincipal was introduced for in bug 1310331 - https://searchfox.org/mozilla-central/diff/2c891cc35b174ee93e592e66a9c0e7e677ec1e49/browser/base/content/tabbrowser.xml#2246
Before that patch, there was only a check for about:blankness (blank frames did not result in an additional loadURI call).

This flag is not obviously related to discard/lazy browsers, so using this flag for its side effect seems wrong to me.
I think that the right fix is to modify tihis condition and add "&& !createLazyBrowser":
https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/browser/base/content/tabbrowser.js#2495

Comment 11

[Shane Caraveo (:mixedpuppy)]

What is the status on finishing this?

Comment 12

[Shane Caraveo (:mixedpuppy)]

thanks for the info

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:mixedpuppy, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 14

[Rob Wu [:robwu]]

Oriol attached a patch in bug 1808615 that removes the obsolete uses of allowInheritPrincipal in ext-tabs.

Comment 15

[Oriol Brufau [:Oriol]]

Done in bug 1808615.