This is a follow up from Bug 1329032: STR: 1. Open about:support 2. Click on "about:plugins" or other "about:" links Error shown in Browser Console: > Security Error: Content at about:support may not load or link to about:plugins. So far: a) Opening about:support and clicking on about:plugins works b) Opening about:support and CTRL+CLICK on about:plugins does not work -> Security Error: Content at about:support may not load or link to about:plugins. c) Opening about:support and right-click->open in new tab does not work -> Security Error: Content at about:support may not load or link to about:plugins. d) Opening about:support and right-click-> open in new window does not work -> Security Error: Content at about:support may not load or link to about:plugins. e) Opening about:support and drag/drop about:plugins into a new tab works I guess there is more things we have to fix, I'll look into that right away!

[Tracking Requested - why for this release]: Regression caused by Bug 1182569

Is this bug just for cases (b)-(d)?

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #2) > Is this bug just for cases (b)-(d)? Yes, that is correct. Within this bug we will handle the cases (b) - (d). Sorry for not being explicit.

This one is complicated and I would imagine that I still miss some corner cases and I am also not entirely sure whether I use the right principal in all the cases. It passes the attached tests though. Gijs, can you help me drive this patch?

Comment on attachment 8828463 [details] [diff] [review] bug_1331686_about_plugin_new_tab_tests.patch Review of attachment 8828463 [details] [diff] [review]: ----------------------------------------------------------------- r=me assuming this works with the CPOW usage removed. ::: browser/base/content/test/general/browser_e10s_about_page_triggeringprincipal.js @@ +11,5 @@ > }); > > +// Waits for a load event somewhere in the browser but ignore events coming > +// from <xul:browser>s without a tab assigned. That are most likely browsers > +// that preload the new tab page. This is relying on CPOWs, which I would prefer not to do... Can you use BrowserTestUtils.waitForNewTab ( https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#280 ) instead? Same issue below. @@ +86,5 @@ > + is(loadingPrincipal, null, > + "sanity check - load of TYPE_DOCUMENT must have a null loadingPrincipal"); > + }); > + > + gBrowser.removeCurrentTab(); Nit: yield BTU.removeTab(tab); Also, no empty line at the end of a block. Same nits in the next test.

After the fix for bug 1182569, I noticed a problem where after loading about:sync-log, clicking on the file:// link "Up to higher level directory" causes a hang. Christoph, shall I file a new bug or will that be covered by this one? If you have an updated patch, I could give it a try. The patch from comment 5 didn't apply cleanly for me. Note: On Mac, to test the fix for this, you'll need to set security.sandbox.content.level=1. Using level=2 prevents the content process from being able to read. And level=2 is limited to Nightly for now.

Comment on attachment 8828466 [details] [diff] [review] bug_1331686_about_plugin_new_tab.patch Review of attachment 8828466 [details] [diff] [review]: ----------------------------------------------------------------- This looks like it's heading in the right direction, but needs some more work. ::: browser/base/content/browser.js @@ +78,5 @@ > ["fxAccounts", "resource://gre/modules/FxAccounts.jsm"], > ["gDevTools", "resource://devtools/client/framework/gDevTools.jsm"], > ["gDevToolsBrowser", "resource://devtools/client/framework/gDevTools.jsm"], > + ["webrtcUI", "resource:///modules/webrtcUI.jsm"], > + ["Utils","resource://gre/modules/sessionstore/Utils.jsm"], Hm, I don't think we should put the Sessionstore Utils in the browser.xul global like this... @@ +850,5 @@ > uri = "about:blank"; > } > + let triggeringPrincipal = ("triggeringPrincipal" in params) > + ? params.triggeringPrincipal > + : null; Nit: let triggeringPrincipal = params.triggeringPrincipal || null; @@ +890,5 @@ > > let loadParams = { > uri, > + triggeringPrincipal: triggeringPrincipal > + ? Utils.serializePrincipal(triggeringPrincipal) This seems to just be a wrapper around the serialization helper. You can probably do this at the top of browser.js: XPCOMUtils.defineLazyServiceGetter(this, "gSerializationHelper", "@mozilla.org/network/serialization-helper;1", "nsISerializationHelper"); and then in here use gSerializationHelper.serializePrincipal instead. @@ +4964,5 @@ > > nsBrowserAccess.prototype = { > QueryInterface: XPCOMUtils.generateQI([Ci.nsIBrowserDOMWindow, Ci.nsISupports]), > > + _openURIInNewTab(aURI, aTriggeringPrincipal, aReferrer, aReferrerPolicy, aIsPrivate, Can you add this param at the end and default it to null? This is already going to break add-ons, I suspect, because they've been known to try to monkeypatch this code. Anyway, let's do what we can to limit the damage. @@ +5041,5 @@ > aWhere = gPrefService.getIntPref("browser.link.open_newwindow"); > } > > let referrer = aOpener ? makeURI(aOpener.location.href) : null; > + let triggeringPrincipal = aOpener.document.nodePrincipal; You need to nullcheck aOpener here, like the line above it. ::: browser/base/content/tabbrowser.xml @@ +4870,5 @@ > > let newTab = this.loadOneTab(data.href, { > referrerURI: (data.referrer ? makeURI(data.referrer) : null), > referrerPolicy: Ci.nsIHttpChannel.REFERRER_POLICY_UNSET, > + triggeringPrincipal: document.nodePrincipal, I don't know what this code does. It was added in bug 1315105, and AFAICT wasn't reviewed by a browser peer. :-\ In any case, it looks like it's about prerendering documents, which is behind a pref (which I hope means we're not shipping it yet - it doesn't look like it deals with containers correctly, for instance...?), and it's not relevant for the usecases here I would imagine. Plus you're explicitly passing system principal which is probably wrong. I'd imagine that really whatever platform code this is coming from needs to pass the correct principal, otherwise we might end up doing prerenders of things that the user can't actually get to directly by clicking links in the original page (because security). Anyway, please bump this to another followup bug. @@ +7600,5 @@ > flags: aFlags, > referrerURI: aReferrerURI, > charset: aCharset, > postData: aPostData, > + triggeringPrincipal: document.nodePrincipal, `document` here is browser.xul, so you're explicitly passing the system principal. This should likely grow an argument or pass nothing (for now). @@ +7631,5 @@ > flags: aFlags, > referrerURI: aReferrerURI, > charset: aCharset, > postData: aPostData, > + triggeringPrincipal: document.nodePrincipal, Same. ::: browser/base/content/utilityOverlay.js @@ +90,5 @@ > allowThirdPartyFixup: aAllowThirdPartyFixup, > postData: aPostData, > referrerURI: aReferrerURI, > referrerPolicy: Components.interfaces.nsIHttpChannel.REFERRER_POLICY_UNSET, > + triggeringPrincipal: document.nodePrincipal, This lives in the parent process, so again you're explicitly passing system principal. Pass nothing or grow an argument. Ditto lower down for the cases where you use document.nodePrincipal ::: toolkit/content/widgets/browser.xml @@ +145,5 @@ > aReferrerPolicy = params.referrerPolicy; > } > + if ('triggeringPrincipal' in params) { > + aTriggeringPrincipal = params.triggeringPrincipal; > + } Nit: Double quotes please.

(In reply to Haik Aftandilian [:haik] from comment #7) > After the fix for bug 1182569, I noticed a problem where after loading > about:sync-log, clicking on the file:// link "Up to higher level directory" > causes a hang. Christoph, shall I file a new bug or will that be covered by > this one? If you have an updated patch, I could give it a try. The patch > from comment 5 didn't apply cleanly for me. Single left-button mouse click with no modifiers? Definitely a separate bug.

(In reply to :Gijs from comment #9) > (In reply to Haik Aftandilian [:haik] from comment #7) > > After the fix for bug 1182569, I noticed a problem where after loading > > about:sync-log, clicking on the file:// link "Up to higher level directory" > > causes a hang. Christoph, shall I file a new bug or will that be covered by > > this one? If you have an updated patch, I could give it a try. The patch > > from comment 5 didn't apply cleanly for me. > > Single left-button mouse click with no modifiers? Definitely a separate bug. Yes, single left-button click. Will file a new bug. Thanks.

Thanks Gijs - thanks for the speedy review - BrowserTestUtils.waitForNewTab(...) is of course the way to go - updated the test accordingly. (In reply to Haik Aftandilian [:haik] from comment #7) > Note: On Mac, to test the fix for this, you'll need to set > security.sandbox.content.level=1. Using level=2 prevents the content process > from being able to read. And level=2 is limited to Nightly for now. Thanks for the info - updated.

This patch passes the attached tests. I also manually verified that the patch works for the three cases from the description: b) Opening about:support and CTRL+CLICK on about:plugins does not work c) Opening about:support and right-click->open in new tab does not work d) Opening about:support and right-click-> open in new window does not work

Comment on attachment 8828775 [details] [diff] [review] bug_1331686_about_plugin_new_tab.patch Review of attachment 8828775 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK to me besides the nit below. ::: browser/base/content/browser.js @@ +5040,5 @@ > aWhere = gPrefService.getIntPref("browser.link.open_newwindow"); > } > > let referrer = aOpener ? makeURI(aOpener.location.href) : null; > + let triggeringPrincipal = aOpener ? aOpener.document.nodePrincipal : null; Er, sorry, I should have read more carefully. Apparently you also need to nullcheck aOpener.document - see the code a few lines down from this. So maybe just set it to null, and in the extant if (aOpener && aOpener.document) statement, update to aOpener.document.nodePrincipal.

(In reply to :Gijs from comment #13) > Er, sorry, I should have read more carefully. Apparently you also need to > nullcheck aOpener.document - see the code a few lines down from this. So > maybe just set it to null, and in the extant if (aOpener && > aOpener.document) statement, update to aOpener.document.nodePrincipal. Should have seen that myself. Thanks for pointing that out - fixed! https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cee37a3aa0dfe0e716d04f4af8c28cfc451a8eb

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11) > Created attachment 8828774 [details] [diff] [review] > bug_1331686_about_plugin_new_tab_tests.patch > > Thanks Gijs - thanks for the speedy review - > BrowserTestUtils.waitForNewTab(...) is of course the way to go - updated the > test accordingly. > > (In reply to Haik Aftandilian [:haik] from comment #7) > > Note: On Mac, to test the fix for this, you'll need to set > > security.sandbox.content.level=1. Using level=2 prevents the content process > > from being able to read. And level=2 is limited to Nightly for now. > > Thanks for the info - updated. From the diff: > SpecialPowers.pushPrefEnv({"set": [["security.sandbox.content.level", 1]],}); Can you avoid setting the level to 1 in the test? That only applies to Mac and probably isn't what you need. Sorry for the confusion. I just meant that in order to manually reproduce the problem on Mac, you need to lower the level to 1. That won't be the case for too much longer and setting the level here affects all platforms, but isn't the right thing to do for Windows or Linux. The sandbox issue only affects Mac content processes trying to read from ~/Library and $PROFILE excluding $PROFILE/{extensions,weave,chrome} and will be addressed soon because those file:// URI's will be loaded in a file-content process which will have full read dir permissions.

Tracking 53+ for this regression.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/338508c5cc01 Pass correct triggeringPrincipal for tabs openen through ctrl-click and open link in new tab. r=gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/3444d123020d Test triggeringPrincipal for tabs openend through ctrl-click and open link in new tab. r=gijs

Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/mozilla-inbound/rev/8bb8def65a84 Follow-up: Remove trailing whitespace in browser_e10s_about_page_triggeringprincipal.js. r=eslint-fix

https://hg.mozilla.org/mozilla-central/rev/338508c5cc01 https://hg.mozilla.org/mozilla-central/rev/3444d123020d https://hg.mozilla.org/mozilla-central/rev/8bb8def65a84

(In reply to Haik Aftandilian [:haik] from comment #15) > From the diff: > > SpecialPowers.pushPrefEnv({"set": [["security.sandbox.content.level", 1]],}); > > Can you avoid setting the level to 1 in the test? That only applies to Mac > and probably isn't what you need. > > Sorry for the confusion. I just meant that in order to manually reproduce > the problem on Mac, you need to lower the level to 1. Haik, I am slightly confused. What do you suggest is the best way forward? Should we just remove the line > SpecialPowers.pushPrefEnv({"set": [["security.sandbox.content.level", 1]],}); from the test again? I thought that's what you meant within comment 7, but apparently there was some confusion - thanks!

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20) > (In reply to Haik Aftandilian [:haik] from comment #15) > > From the diff: > > > SpecialPowers.pushPrefEnv({"set": [["security.sandbox.content.level", 1]],}); > > > > Can you avoid setting the level to 1 in the test? That only applies to Mac > > and probably isn't what you need. > > > > Sorry for the confusion. I just meant that in order to manually reproduce > > the problem on Mac, you need to lower the level to 1. > > Haik, I am slightly confused. What do you suggest is the best way forward? > Should we just remove the line > > SpecialPowers.pushPrefEnv({"set": [["security.sandbox.content.level", 1]],}); > from the test again? I thought that's what you meant within comment 7, but > apparently there was some confusion - thanks! Yep, please remove it. We shouldn't need it. Sorry about the confusion. I meant that if you were going to try and debug/reproduce bug 1332589 on Mac, you would have to manually set that pref in about:config. For more context: on Mac on Nightly, the sandbox prevents reading of some local files in ~/Library and $PROFILE. This will be fixed in bug 1332522 and is dependent on the file-content process already being used in Nightly.

I have reproduce this bug with Nightly 53.0a1 (2017-01-17) (64- bit) in Windows 10. This bug's fix is verified with latest Release 53.0 (64-bit). Build ID : 20170413192749 User Agent : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0 [testday-20170428]