”Open Link In New Private Window” prevents the add-on installation

VERIFIED FIXED in Firefox 53

Status

()

Toolkit
Add-ons Manager
VERIFIED FIXED
7 months ago
7 months ago

People

(Reporter: Vasilica Mihasca, QA [away for an extended period of time - please needinfo addonsqa@softvision.ro], Assigned: ckerschb)

Tracking

({regression})

53 Branch
mozilla55
regression
Points:
---

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53 verified, firefox54 verified, firefox55 verified)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Created attachment 8849071 [details]
2017-03-20_1430.png

[Affected versions]:
Firefox 55.0a1 (2017-03-20)
Firefox 54.0a2 (2017-03-20)
Firefox 53.0b4 (20170316212821)


[Affected platforms]:
Windows 10 64-bit
Ubuntu 16.04 32-bit

[Steps to reproduce]:
1.Launch Firefox with clean profile.
2.Navigate to https://addons.mozilla.org/en-US/firefox/addon/video-downloadhelper/?src=cb-dl-users
3.Right click on “+ Add to Firefox” green button.
4.Select ”Open Link In New Private Window” option.

[Expected Results]:
The add-on is successfully installed.

[Actual Results]:
- The add-on is not installed because “Nightly prevented this site from asking you to install software on your computer.” 
- See attached screenshot.

[Additional Notes]:
This issue is not reproducible while opening the link in a normal window.

[Regression Range]:
Last good revision: 80d47de5e1bf04fd59ba08df1e438ee4640e04d1
First bad revision: 3444d123020d204cbe94053c32987c4ee495c8f6
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=80d47de5e1bf04fd59ba08df1e438ee4640e04d1&tochange=3444d123020d204cbe94053c32987c4ee495c8f6

Looks like the following bug has the changes which introduced the regression:
https://bugzilla.mozilla.org/show_bug.cgi?id=1331686
Flags: needinfo?(ckerschb)
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 1

7 months ago
I only got a few minutes to debug the problem right now, but I guess it needs a little more than that, so I will have to come back to this bug a little later. What I can tell so far is: We changed the triggeringPrincipal within Bug 1331686 which apparently affects the addon installation here. At this point I am not certain where this security check fails, but what I can provide is a list of all the arguments within the loadInfo:

channelURI: https://addons.mozilla.org/firefox/downloads/latest/video-downloadhelper/addon-3006-latest.xpi?src=cb-dl-users
loadingPrincipal: nullptr
triggeringPrincipal: https://addons.mozilla.org/en-US/firefox/addon/video-downloadhelper/?src=cb-dl-users
principalToInherit: https://addons.mozilla.org/en-US/firefox/addon/video-downloadhelper/?src=cb-dl-users
contentPolicyType: 6
securityMode: SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL, 
initalSecurityChecksDone: no
enforceSecurity: no
(Assignee)

Comment 2

7 months ago
Also, there is no corresponding message in the browser console and also there is no useful message on the console when running a debug build. What prevents the addon from being installed? Is that application reputation?
(Assignee)

Updated

7 months ago
Flags: needinfo?(ckerschb)
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Is that application reputation?

That download doesn't appear to be tracked by the download manager.

When setting MOZ_LOG="ApplicationReputation:5", you get some terminal output when a download goes through the application reputation stack. There's no output in this case.
Right-clicking on https://addons.mozilla.org/firefox/downloads/latest/video-downloadhelper/addon-3006-latest.xpi?src=cb-dl-users and choosing "Open Link in New Container Tab" also displays the same problem as with Private Browsing.
It's because this check:

      } else if (!aBrowser.contentPrincipal || !aInstallingPrincipal.subsumes(aBrowser.contentPrincipal)) {

in https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/mozapps/extensions/AddonManager.jsm#2142

fails - which makes sense, because private and non-private (and different container) principals will never subsume each other.
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #5)
> It's because this check:
> 
>       } else if (!aBrowser.contentPrincipal ||
> !aInstallingPrincipal.subsumes(aBrowser.contentPrincipal)) {
> 
> in
> https://dxr.mozilla.org/mozilla-central/rev/
> 1b9293be51637f841275541d8991314ca56561a5/toolkit/mozapps/extensions/
> AddonManager.jsm#2142
> 
> fails - which makes sense, because private and non-private (and different
> container) principals will never subsume each other.

Though actually, this is a little confusing - the content principal has the usercontextid and/or private browsing flag. The installing principal doesn't, but why?

https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/mozapps/extensions/addonManager.js#198-200

seems to be using principalToInherit for this. I'm a bit surprised that doesn't get updated for the right private browsing flags and/or userContextId. It should have done (earlier on)... otherwise we might have other (bigger?) problems...
(Assignee)

Comment 7

7 months ago
I am only guessing here and might be completely wrong. Anyway, if the toplevel load does not provide a principal, then we create one from the referrer [1] which then uses mOriginAttributes of the current docshell, which might be different from the one that are used from the private browsing content principal, same for containers.

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1531
So I think this is coming from https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/toolkit/mozapps/extensions/amContentHandler.js#49-52 and I've just done another build to check what's happening there (doesn't seem to show up in the browser debugger on vanilla nightly...)
(In reply to :Gijs from comment #8)
> So I think this is coming from
> https://dxr.mozilla.org/mozilla-central/rev/
> 1b9293be51637f841275541d8991314ca56561a5/toolkit/mozapps/extensions/
> amContentHandler.js#49-52 and I've just done another build to check what's
> happening there (doesn't seem to show up in the browser debugger on vanilla
> nightly...)

OK, so both of these are set to an attributes-less addons.mozilla.org.

Christoph, can we just teach the code that does the "open in new window / tab" stuff to convert any inheriting/triggering principals, if they are codebase or null principals, to have the right OA and/or private browsing flag? That seems like it'd be the most correct fix.
Flags: needinfo?(ckerschb)
(Assignee)

Comment 10

7 months ago
(In reply to :Gijs from comment #9)
> Christoph, can we just teach the code that does the "open in new window /
> tab" stuff to convert any inheriting/triggering principals, if they are
> codebase or null principals, to have the right OA and/or private browsing
> flag? That seems like it'd be the most correct fix.

I suppose we would need to create a new codebase/null principal handing in the correct origin attributes. We don't have any API that allows to overrule the OA of a principal. The only available API allows to overwrite OA of the loadingPrincipal within the loadInfo [1], but I suppose that doesn't help us here.

Where in the code does that need to happen? If that needs to happen in docshell than I am not sure if that is a great idea. If that can happen in e.g. browser.js, I suppose we could generate a new principal with the right OA.

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/LoadInfo.cpp#763
Flags: needinfo?(ckerschb)
I think we can just teach openLinkIn to reuse the OA/pb stuff - so in here: https://dxr.mozilla.org/mozilla-central/rev/1b9293be51637f841275541d8991314ca56561a5/browser/base/content/utilityOverlay.js#197
(Assignee)

Comment 12

7 months ago
Created attachment 8849539 [details] [diff] [review]
bug_1348801_teach_triggeringprincipal_about_oa.patch

Gijs, you know browser/ code way better than I do - maybe you can have a first look, because I assume this is the patch you were thinking about.

When opening in new Container (e.g. work), then I get the following arguments:
aURI: https://addons.mozilla.org/firefox/downloads/latest/video-downloadhelper/addon-3006-latest.xpi?src=cb-dl-users
aPrincipal: https://addons.mozilla.org/en-US/firefox/addon/video-downloadhelper/?src=cb-dl-users
aUserContextId: 2
aIsPrivate: undefined
which we then use to create a new Principal with the new OA of: ^userContextId=2
and hence addon installation works!


When opening in a new private window, we then get the following arguments:
aURI: https://addons.mozilla.org/firefox/downloads/latest/video-downloadhelper/addon-3006-latest.xpi?src=cb-dl-users
aPrincipal: https://addons.mozilla.org/en-US/firefox/addon/video-downloadhelper/?src=cb-dl-users
aUserContextId: 0
aIsPrivate: undefined
which then results in new OA of "" (empty string) because default arguments are not displayed when calling principal.originSuffix.
Hence that does not work yet.

Question: Is there potentially something phishy with the argument aIsPrivate or is the private browsing info hidden within some other argument?

Further, I don't think we can query any other information regarding OA, but just to make sure, here is a list of all OA values:
 MOZ_INIT_OUTSIDE_CTOR uint32_t mAppId;
 MOZ_INIT_OUTSIDE_CTOR nsString mFirstPartyDomain;
 MOZ_INIT_OUTSIDE_CTOR bool mInIsolatedMozBrowser;
 MOZ_INIT_OUTSIDE_CTOR uint32_t mPrivateBrowsingId;
 MOZ_INIT_OUTSIDE_CTOR uint32_t mUserContextId;
which are all defined here:
https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-pc-linux-gnu/dist/include/mozilla/dom/ChromeUtilsBinding.h#82
Assignee: nobody → ckerschb
Attachment #8849539 - Flags: feedback?(gijskruitbosch+bugs)
(Assignee)

Updated

7 months ago
Status: NEW → ASSIGNED
Comment on attachment 8849539 [details] [diff] [review]
bug_1348801_teach_triggeringprincipal_about_oa.patch

Review of attachment 8849539 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/utilityOverlay.js
@@ +230,5 @@
> +  // actually are the same object (See Bug: 1346759)
> +  if (aPrincipal && aPrincipal.isCodebasePrincipal) {
> +    var attrs = {
> +      userContextId: aUserContextId,
> +      privateBrowsingId: aIsPrivate ? 1 : 0

I actually see aIsPrivate being true for the first call to openLinkIn when using the 'open in new private window' menuitem. We then call openLinkIn again to open the link in the current tab of the newly opened private window. 

As a result, I expect you will want to insert this code a bit further down, ie right before:

https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/browser/base/content/utilityOverlay.js#258

and then use:

aIsPrivate || PrivateBrowsingUtils.isWindowPrivate(w)

to determine if the OA should be private.
Attachment #8849539 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
(Assignee)

Comment 14

7 months ago
Created attachment 8849553 [details] [diff] [review]
bug_1348801_teach_triggeringprincipal_about_oa.patch

Gijs, thanks for your help. Moving the code down within the 'window' case actually fixed the problem. But since this also needs to work for 'Open Link in New Container Tab' I had to also insert the code for that branch which is slightly sub-optimal, but I guess we can't do any better.

I manually verified (using the STRs from comment 0) that:
* Open Link in New Tab -> works
* Open Link in New Container Tab -> works
* Open Link in New Window -> works
* Open Link in New Private Window -> works
Attachment #8849539 - Attachment is obsolete: true
Attachment #8849553 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8849553 [details] [diff] [review]
bug_1348801_teach_triggeringprincipal_about_oa.patch

Review of attachment 8849553 [details] [diff] [review]:
-----------------------------------------------------------------

It can go before the `  if (!w || where == "window") {` check on line 258, right? Though we need to nullcheck 'w', sorry for missing that. I think your patch as-attached misses the where="current" case.
Attachment #8849553 - Flags: review?(gijskruitbosch+bugs)

Updated

7 months ago
Whiteboard: triaged

Updated

7 months ago
Whiteboard: triaged
(Assignee)

Comment 16

7 months ago
Created attachment 8849696 [details] [diff] [review]
bug_1348801_teach_triggeringprincipal_about_oa.patch

Thanks for your help with this Gijs - it works!
Attachment #8849553 - Attachment is obsolete: true
Attachment #8849696 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 17

7 months ago
Created attachment 8849697 [details] [diff] [review]
bug_1348801_test_oa_for_private_win.patch

And the automated test, the important part is the '.originSuffix' check which fails if the patch is not applied!
Attachment #8849697 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 18

7 months ago
And here is the TRY run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=005a7c211c1a4d312344642e40f491c161832cac
Comment on attachment 8849696 [details] [diff] [review]
bug_1348801_teach_triggeringprincipal_about_oa.patch

Review of attachment 8849696 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. Please doublecheck if this passes eslint.

::: browser/base/content/utilityOverlay.js
@@ +260,5 @@
> +  // Please note we do not have to do that for SystemPrincipals and we
> +  // can not do it for NullPrincipals since NullPrincipals are only
> +  // identical if they actually are the same object (See Bug: 1346759)
> +  if (aPrincipal && aPrincipal.isCodebasePrincipal) {
> +    var attrs = {

Nit: let.

@@ +262,5 @@
> +  // identical if they actually are the same object (See Bug: 1346759)
> +  if (aPrincipal && aPrincipal.isCodebasePrincipal) {
> +    var attrs = {
> +      userContextId: aUserContextId,
> +      privateBrowsingId: aIsPrivate || (w && PrivateBrowsingUtils.isWindowPrivate(w))

Nit: please add a trailing comma.
Attachment #8849696 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8849697 [details] [diff] [review]
bug_1348801_test_oa_for_private_win.patch

Review of attachment 8849697 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/test/general/browser.ini
@@ +267,5 @@
>  subsuite = clipboard
>  [browser_minimize.js]
>  [browser_modifiedclick_inherit_principal.js]
>  [browser_new_http_window_opened_from_file_tab.js]
> +[browser_oa_private_browsing_window.js]

Can you add this to the private browsing tests in https://dxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser , rather than browser/base/content/test/general/ ? You'll need to update the file's content reference to reference a valid alternative file.

::: browser/base/content/test/general/browser_oa_private_browsing_window.js
@@ +22,5 @@
> +
> +    yield ContentTask.spawn(privateWin.gBrowser.selectedBrowser, {}, function*() {
> +      let channel = content.document.docShell.currentDocumentChannel;
> +      is(channel.URI.spec,
> +         "http://example.com/browser/browser/base/content/test/general/dummy_page.html",

This should use DUMMY_PAGE. You'll need to pass it through as an arg, so pass:

{DUMMY_PAGE, TEST_PAGE}

as the second arg to ContentTask.spawn, then make the function signature:

function*({DUMMY_PAGE, TEST_PAGE}) {
too.

It's possible you'll need to shut up eslint (disable the shadowing rule for that line) when you do this.

@@ +29,5 @@
> +      let triggeringPrincipal = channel.loadInfo.triggeringPrincipal;
> +      ok(triggeringPrincipal.isCodebasePrincipal,
> +         "sanity check to ensure principal is a codebasePrincipal");
> +      is(triggeringPrincipal.URI.spec,
> +         "http://example.com/browser/browser/base/content/test/general/file_triggeringprincipal_oa.html",

This should use TEST_PAGE.

@@ +31,5 @@
> +         "sanity check to ensure principal is a codebasePrincipal");
> +      is(triggeringPrincipal.URI.spec,
> +         "http://example.com/browser/browser/base/content/test/general/file_triggeringprincipal_oa.html",
> +         "test page must be the triggering page");
> +      is(triggeringPrincipal.originSuffix, "^privateBrowsingId=1",

I expect it would be more futureproof to check originAttributes.privateBrowsingId directly.

::: browser/base/content/test/general/file_triggeringprincipal_oa.html
@@ +4,5 @@
> +  <meta charset="utf-8">
> +  <title>Test for Bug 1348801</title>
> +</head>
> +<body>
> +  <a href="http://example.com/browser/browser/base/content/test/general/dummy_page.html" id="checkPrincipalOA">checkPrincipalOA</a>

Please make this a relative reference
Attachment #8849697 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 21

7 months ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4568c99d705d
Teach TriggeringPrincipal about OA when opening link in private window. r=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/15eaaf95fb26
Test OA of principal when opening link in new private window. r=gijs
(Assignee)

Comment 22

7 months ago
Comment on attachment 8849696 [details] [diff] [review]
bug_1348801_teach_triggeringprincipal_about_oa.patch

Approval Request Comment
Within Bug 1331686 we started to explicitly pass a triggeringPrincipal for new window loads instead of using the fallback within docshell. Apparently that triggeringPrincipal needs to know about the right origin attributes, in particular about the privateBrowsingId.

We should definitely uplift that bug!

[Feature/Bug causing the regression]:
Bug 1331686 - About:support/about can not load about:plugins/serviceworkers when the latter is loaded via various "load in new tab/window" UI actions

[User impact if declined]:
Users relying on private browsing might experience unexpected, privacy violating experience.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
Not yet.

[Needs manual test from QE? If yes, steps to reproduce]: 
Not sure since we have good automated test coverage. STRs from comment 0 are precise and accurate if needed.

[List of other uplifts needed for the feature/fix]:
No

[Is the change risky?]:
No, since we only teach the triggeringPrincipal about the right Origin Attributes.

[Why is the change risky/not risky?]:
See above.

[String changes made/needed]:
No
Attachment #8849696 - Flags: approval-mozilla-beta?
Attachment #8849696 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

7 months ago
Comment on attachment 8849697 [details] [diff] [review]
bug_1348801_test_oa_for_private_win.patch

Approval Request Comment
See previous comment
Attachment #8849697 - Flags: approval-mozilla-beta?
Attachment #8849697 - Flags: approval-mozilla-aurora?

Comment 24

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4568c99d705d
https://hg.mozilla.org/mozilla-central/rev/15eaaf95fb26
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 25

7 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/e03e0c60462c
bump eslint to fix orange a=tomcat r=Standard8
(In reply to Pulsebot from comment #25)
> Pushed by cbook@mozilla.com:
> https://hg.mozilla.org/mozilla-central/rev/e03e0c60462c
> bump eslint to fix orange a=tomcat r=Standard8

note talked via irc with standard8 and this was done i favor to prevent the backout of this patches here
Comment on attachment 8849696 [details] [diff] [review]
bug_1348801_teach_triggeringprincipal_about_oa.patch

Fix a broken private browsing experience and also include tests. Aurora54+ & Beta53+.

Hi Vasilica, could you help verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(vasilica.mihasca)
Attachment #8849696 - Flags: approval-mozilla-beta?
Attachment #8849696 - Flags: approval-mozilla-beta+
Attachment #8849696 - Flags: approval-mozilla-aurora?
Attachment #8849696 - Flags: approval-mozilla-aurora+
Attachment #8849697 - Flags: approval-mozilla-beta?
Attachment #8849697 - Flags: approval-mozilla-beta+
Attachment #8849697 - Flags: approval-mozilla-aurora?
Attachment #8849697 - Flags: approval-mozilla-aurora+
Confirm that this issue is fixed on Firefox 55.0a1 (2017-03-23) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac Os X 10.12.1. The add-ons are successfully installed while selecting “Open Link in New Container Tab” and “Open Link in New Private Window”: https://www.screencast.com/t/zDBHPDhM , https://www.screencast.com/t/orbZ7f73OD8
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: needinfo?(vasilica.mihasca)

Comment 29

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e1fac4a318a
https://hg.mozilla.org/releases/mozilla-aurora/rev/7fec8b804687
status-firefox54: affected → fixed

Comment 30

7 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/33a1b844b718
https://hg.mozilla.org/releases/mozilla-beta/rev/2fca9bc9660f
status-firefox53: affected → fixed
Verified fixed on Firefox 54.0a2 (2017-03-24) and Firefox 53.0b6 (20170323090023) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac Os X 10.12.1. The add-ons are successfully installed while selecting “Open Link in New Private Window”. Setting the tracking flags accordingly.
status-firefox53: fixed → verified
status-firefox54: fixed → verified
status-firefox-esr52: --- → unaffected
Version: Trunk → 53 Branch
You need to log in before you can comment on or make changes to this bug.