1364016 - Have _openURIInNewTab() provide the correct triggeringPrincipal

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openurinewtab_triggeringprincipal.patch

@baku, ultimately we would like that frontend code passes the correct triggeringprincipal all the way to docshell (See Bug 1333030). But that's quite an undertaking hence we split it up into small junks. I was wondering if you could review the dom/ changes?

@gijs: could you take a look at the browser changes? My thinking was that if aOpener is null, then we don't have a referrer and hence fall back to system within docshell code anyway. What do you think? Acceptable?

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8072bd8aece4d3df9a2eda6bed3fd432fb150c5a

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8867098 [details] [diff] [review]
bug_1364016_openurinewtab_triggeringprincipal.patch

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

::: browser/base/content/browser.js
@@ +5239,5 @@
>        referrerPolicy = aOpener.document.referrerPolicy;
>        triggeringPrincipal = aOpener.document.nodePrincipal;
>      }
> +    else {
> +      triggeringPrincipal = Services.scriptSecurityManager.getSystemPrincipal();

We implement noopener and noreferer as of 52, so what does this do in those cases? I would expect aOpener to be null...
If this is a 'cheap' way to get a webpage to execute a link with system principal, r-. Clearing review for now. :-)

(Note that this code only runs in the non-e10s case, according to the comment at the top of the file (is the comment wrong?), so I'm not sure how it relates to the other changes in this patch that baku is reviewing).

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #4)
> We implement noopener and noreferer as of 52, so what does this do in those
> cases? I would expect aOpener to be null...

I don't know at this point. Please keep in mind that if we don't pass a referrer then we fall back to using the systemPrincipal within nsDocshell anyway [1]. So I think in those cases we are not off any worse than we are right now. Would you rather extend OpenURI() with a triggeringPrincipal argument?

> (Note that this code only runs in the non-e10s case, according to the
> comment at the top of the file (is the comment wrong?), so I'm not sure how
> it relates to the other changes in this patch that baku is reviewing).

What is 'this' code? I didn't find that comment. In browser.js? The code that baku reviews definitely gets triggered (see TRY run from comment 1).

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
Oh, I think I forgot to set the ni? for you - please see comment 5.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openuri.patch

Here is a patch where I extended openURI with a triggeringPrincipal argument. It seems that in case of ContentParent 'aOpener' might be null in which case it would be nice to pass the triggeringPrincipal explicitly.

In all other cases were we pass 'null' as aOpener we fall back to using systemPrincipal in either cases whether we extend OpenURI() by a triggeringPrincipal argument or not.

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)
> (In reply to :Gijs from comment #4)
> > We implement noopener and noreferer as of 52, so what does this do in those
> > cases? I would expect aOpener to be null...
> 
> I don't know at this point. Please keep in mind that if we don't pass a
> referrer then we fall back to using the systemPrincipal within nsDocshell
> anyway [1]. So I think in those cases we are not off any worse than we are
> right now. Would you rather extend OpenURI() with a triggeringPrincipal
> argument?

Where is [1], and which OpenURI() method?

Do we remove any principal inherit flags that have been passed if we take that fallback path?

Ideally, I would like the principals to be well-defined here, even in the no-opener/no-referer cases. If that requires adding more arguments, I guess I would prefer that, even if it's sad to be passing around state like that *everywhere* which is what it is starting to feel like...

> > (Note that this code only runs in the non-e10s case, according to the
> > comment at the top of the file (is the comment wrong?), so I'm not sure how
> > it relates to the other changes in this patch that baku is reviewing).
> 
> What is 'this' code? I didn't find that comment. In browser.js? The code
> that baku reviews definitely gets triggered (see TRY run from comment 1).

Higher up in browser.js, ie https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#5196-5198 .

(FWIW, this would be easier to point out in reviews if you used mozreview... is there a reason you don't? :-) )

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #8)

> Where is [1], and which OpenURI() method?

Sorry, [1] is here and this OpenURI [2]

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1521
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8867823&action=diff

> Do we remove any principal inherit flags that have been passed if we take
> that fallback path?

Yes, we update the principal inherit flag here:
https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp#1461-1519

> Ideally, I would like the principals to be well-defined here, even in the
> no-opener/no-referer cases. If that requires adding more arguments, I guess
> I would prefer that, even if it's sad to be passing around state like that
> *everywhere* which is what it is starting to feel like...

Yeah, indeed, but ultimately I would like to end up in that state as well. Initially I thought we just have to update certain parts of the code, but it feels it really fans out. Looking at the dependency tree of Bug 1333030 highlights that as well.

> (FWIW, this would be easier to point out in reviews if you used mozreview...
> is there a reason you don't? :-) )

Personal preference. Probably I should migrate to using mozreview at some point.

Comment 10

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> (In reply to :Gijs from comment #8)
> 
> > Where is [1], and which OpenURI() method?
> 
> Sorry, [1] is here and this OpenURI [2]
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#1521
> [2] https://bugzilla.mozilla.org/attachment.cgi?id=8867823&action=diff
> 
> > Do we remove any principal inherit flags that have been passed if we take
> > that fallback path?
> 
> Yes, we update the principal inherit flag here:
> https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.
> cpp#1461-1519

This is pretty confusing - I think what you mean is, we never use the triggering principal as the principal to inherit. Is that right? If not, it's not happening at the code you're highlighting, and we don't reset the inheritPrincipal bool to false, even if we do end up using a system principal triggeringPrincipal...

> > Ideally, I would like the principals to be well-defined here, even in the
> > no-opener/no-referer cases. If that requires adding more arguments, I guess
> > I would prefer that, even if it's sad to be passing around state like that
> > *everywhere* which is what it is starting to feel like...
> 
> Yeah, indeed, but ultimately I would like to end up in that state as well.
> Initially I thought we just have to update certain parts of the code, but it
> feels it really fans out. Looking at the dependency tree of Bug 1333030
> highlights that as well.

:-(

If we have to, we have to, I guess.

Comment 11

[Andrea Marchesini [:baku]]

Comment on attachment 8867098 [details] [diff] [review]
bug_1364016_openurinewtab_triggeringprincipal.patch

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

::: dom/base/nsOpenURIInFrameParams.cpp
@@ +57,5 @@
> +
> +NS_IMETHODIMP
> +nsOpenURIInFrameParams::SetTriggeringPrincipal(nsIPrincipal* aTriggeringPrincipal)
> +{
> +  MOZ_ASSERT(aTriggeringPrincipal, "must be a valid triggeringPrincipal");

Don't assert. Return an error.

::: dom/ipc/ContentChild.cpp
@@ +677,1 @@
>  {

MOZ_ASSERT(aTriggeringPrincipal) ?

::: dom/ipc/ContentParent.cpp
@@ +4467,2 @@
>  
>  {

MOZ_ASSERT(aTriggeringPrincipal);

@@ +4636,5 @@
>    TabParent::AutoUseNewTab aunt(newTab, aURLToLoad);
>    const uint64_t nextTabParentId = ++sNextTabParentId;
>    sNextTabParents.Put(nextTabParentId, newTab);
>  
> +  nsCOMPtr<nsIPrincipal> triggeringPrincipal;

you are not using this, right?

@@ +4687,5 @@
>  {
>    nsCOMPtr<nsITabParent> newRemoteTab;
>    bool windowIsNew;
>    nsCOMPtr<nsIURI> uriToLoad = DeserializeURI(aURIToLoad);
> +  nsCOMPtr<nsIPrincipal> triggeringPrincipal;

You are not using this.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Thanks Baku, I incorporated your suggestions. But back to the browser/ code. In case openURI() is only called within e10s, then we  might get away with just falling back to using a NullPrincipal in case there is no aOpener. Let's see how that works out:

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7af3a1ebb563507b4588e95c41a345814831ab31

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Gijs, I incorporated all of the suggestions from baku. The remaining question is the following:
Looking at the TRY run from comment 12, we might get away with falling back to using a NullPrincipal instead of SystemPrincipal within OpenURI() [1]. Alternatively we could update OpenURI() to also pass an explicit triggeringPricipal as I do within the second patch attached to this bug (bug_1364016_openuri.patch).

Given the comment on top of OpenURI() that this function is only called in non-e10s mode I think we should simply fall back to using a NullPrincipal. What do you think?

[1] https://hg.mozilla.org/try/rev/fb23fdb49e7377c68bf233eede31504d929c8e28#l1.13

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
I think I set the ni? prematurely. There is an issue on TRY on e10s which I need to resolve first.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #14)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> I think I set the ni? prematurely. There is an issue on TRY on e10s which I
> need to resolve first.

Gijs, the failing test (as described in comment 14) is in fact orthogonal to the question I was about to ask you in comment 13. So, I guess I can put the ni? back while fixing the issue from comment 14.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_followup.patch

Hey Baku, a little follow up from the initial patch which I will merge into the other patch once r+ed. In case there is no 'opener' then we are not returning a principal from GetWindowParamsFromParent. I only realized this when running test_openWindow.htm. I think in that case we want to use a NullPrincipal as the triggeringPrincipal.

Comment 17

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #13)
> Gijs, I incorporated all of the suggestions from baku. The remaining
> question is the following:
> Looking at the TRY run from comment 12, we might get away with falling back
> to using a NullPrincipal instead of SystemPrincipal within OpenURI() [1].
> Alternatively we could update OpenURI() to also pass an explicit
> triggeringPricipal as I do within the second patch attached to this bug
> (bug_1364016_openuri.patch).
> 
> Given the comment on top of OpenURI() that this function is only called in
> non-e10s mode I think we should simply fall back to using a NullPrincipal.
> What do you think?
> 
> [1]
> https://hg.mozilla.org/try/rev/fb23fdb49e7377c68bf233eede31504d929c8e28#l1.13

I would be surprised if using null principal didn't break something obscure, but if it does it will only do so for 'privileged' URLs like file: or chrome: or about:, and so tbh, I would prefer to go with null principal and update any broken consumers if/when we find them. We can always end up passing the args later if that turns out to be necessary.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #17)
> I would be surprised if using null principal didn't break something obscure,
> but if it does it will only do so for 'privileged' URLs like file: or
> chrome: or about:, and so tbh, I would prefer to go with null principal and
> update any broken consumers if/when we find them. We can always end up
> passing the args later if that turns out to be necessary.

Unfortunately it's not that easy as I thought. As test browser/base/content/test/general/browser_bug537474.js reveals, we are running that code in e10s as well. At the moment I am preparing the a patch to extend OpenURI() by an explicit triggeringPrincipal. And you are right, that test disallows loading about: :-)

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openuri.patch

Gijs, here we are. Let me summarize what's going on for you:

1) After writing that patch I am pretty convinced that we *should* extend OpenURI() by an explicit triggeringPrincipal.
2) In the cases where we currently use the principal from aOpener.document.nodePrincipal, we would also use the same principal in the new world.
3) Using this patch it seems we can provide a more accurate principal, especially for workers, we can have baku review those changes.

Here are the two remaining questions I have:
1) Within nsWebHandlerApp.js, the channel is created using 'loadUsingSystemPrincipal' hence I suppose it's fine to also use the systemPrincipal to pass to OpenURI.
2) I am not sure about using SystemPrincipal within nsBrowserContentHandler.js, but I don't know how we could query a different one. Any ideas?

Finally, I removed the browser.js changes from the inital patch (the one that baku already reviewed). In other words, all the changes within browser/ are now within this patch. Thanks!

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8873384 [details] [diff] [review]
bug_1364016_openuri.patch

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

::: devtools/client/responsive.html/browser/tunnel.js
@@ +240,5 @@
>        // outside the main focus of RDM.
>        let { detail } = event;
>        event.preventDefault();
>        let uri = Services.io.newURI(detail.url);
> +      let sourceNode = event.dataTransfer;

let sourceNode = event.dataTransfer.mozSourceNode; :-)

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8873379 [details] [diff] [review]
bug_1364016_followup.patch

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

::: dom/ipc/ContentChild.cpp
@@ +690,5 @@
>    *aFullZoom = 1.0f;
>    auto* opener = nsPIDOMWindowOuter::From(aParent);
>    if (!opener) {
> +    nsCOMPtr<nsIPrincipal> nullPrincipal = NullPrincipal::Create();
> +    NS_ADDREF(*aTriggeringPrincipal = nullPrincipal);

We have a similar return NS_OK if openerDocShell is nullptr.
Update that as well.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openurinewtab_triggeringprincipal.patch

(In reply to Andrea Marchesini [:baku] from comment #21)
> > +    NS_ADDREF(*aTriggeringPrincipal = nullPrincipal);
> 
> We have a similar return NS_OK if openerDocShell is nullptr.
> Update that as well.

I guess there is no need, because in that case we would have already set the correct triggeringPricnipal using
> +  NS_ADDREF(*aTriggeringPrincipal = doc->NodePrincipal());

Carrying over r+ from baku.

Comment 23

[:Gijs (out for now; he/him)]

Comment on attachment 8873384 [details] [diff] [review]
bug_1364016_openuri.patch

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

::: browser/components/nsBrowserContentHandler.js
@@ +667,5 @@
>                          .getInterface(nsIDOMWindow);
>    var bwin = rootWin.QueryInterface(nsIDOMChromeWindow).browserDOMWindow;
>    bwin.openURI(uri, null, location,
> +               nsIBrowserDOMWindow.OPEN_EXTERNAL,
> +               Services.scriptSecurityManager.getSystemPrincipal());

hrmpf.

So, there are 4 callsites for this that I can find:

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/browser/components/nsBrowserContentHandler.js#739-749

this is clearly external, system principal is fine

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/browser/components/nsBrowserContentHandler.js#343-351

ditto.

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/browser/components/nsBrowserContentHandler.js#390-396

ditto.

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/browser/components/nsBrowserContentHandler.js#596-613

wuh? What is this... Well...

https://dxr.mozilla.org/mozilla-central/search?q=handlecontent(+-canhandlecontent+-path%3Ansicontent+-file%3Ahttptransaction&redirect=false

lists 2 implementations, 1 consumer. That one consumer has:

https://dxr.mozilla.org/mozilla-central/rev/bdb2387396b4a74dfefb7c983733eed3625e906a/uriloader/base/nsURILoader.cpp#502-504

        // XXXbz returning an error code to represent handling the
        // content is just bizarre!
        if (rv != NS_ERROR_WONT_HANDLE_CONTENT) {

but nsBrowserContentHandler is using this to signal that we indeed won't handle the content, so now I'm thoroughly confused. Ditto for amContentHandler.js. So maybe I'm just confused by the negations here, but the comment doesn't look correct to me.

In any case, this has an associated request and everything. So I think there's a 'real' triggering principal to be had here. So I think this will need to take a principal argument, pass system from the 3 'easy' commandline handler cases, and do something more elaborate for the remaining case, with 302 to bz for what this code is doing / supposed to do, because I'm struggling to make sense of it.

::: uriloader/exthandler/nsWebHandlerApp.js
@@ +143,5 @@
>      browserDOMWin.openURI(uriToSend,
>                            null, // no window.opener
>                            Ci.nsIBrowserDOMWindow.OPEN_DEFAULTWINDOW,
> +                          Ci.nsIBrowserDOMWindow.OPEN_NEW,
> +                          Services.scriptSecurityManager.getSystemPrincipal());

If the windowcontext is non-null (and qi's to a domwindow with a document), doesn't that provide a principal here? That said, I don't know if that will be the right thing to do... it looks like it gets called from here:

https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/handling/content/dialog.js#222

and so it's in response to the user picking an application explicitly in a dialog, and if that is a web app, and if it lives somewhere that for some reason isn't linkable from wherever the page that originally provided the uri/file lives, uhhh... yeah, I don't know. :-\

exthandler is pretty unloved at this point. I don't know who would know better.

On balance, I think because system principal is the assumption we make already, I guess this is fine. But I wish we were more confident about this type of stuff.

More generally, I wish this code used a better-understood codepath, but that's a battle for another day.

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openurinewtab_triggeringprincipal.patch

Rebasing this patch after Bug 1368046 has landed. Carrying over r+ from baku.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openuri.patch

(In reply to :Gijs from comment #23)
> pass system from the 3 'easy' commandline
> handler cases, and do something more elaborate for the remaining case

I actually agree. I updated the callsites of handURIToExistingBrowser to pass a systemPrincipal in the three easy cases. In the more complicated case we actually have a valid request. Without overthinking this, it's best if we use the triggeringPrincipal of that request's loadInfo. 

> ::: uriloader/exthandler/nsWebHandlerApp.js
> If the windowcontext is non-null (and qi's to a domwindow with a document),
> doesn't that provide a principal here? That said, I don't know if that will
> be the right thing to do... it looks like it gets called from here:

If aWindowContext is non-null, then we are taking a different codepath [1] and return early from that function.
Given that we also use 'loadUsingSystemPrincipal' in the case where aWindowContext != null, I am confident we can use the systemPrincipal when aWindowContext == null.

[1] https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsWebHandlerApp.js#81-114

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d2e63ebc6688a8f62e64a0579313fb6d4b17c41

Comment 27

[:Gijs (out for now; he/him)]

Comment on attachment 8879508 [details] [diff] [review]
bug_1364016_openuri.patch

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

r=me but please get someone else to sanity-check the serviceworkerclients code (was that meant to be in the other patch?)

::: dom/workers/ServiceWorkerClients.cpp
@@ +757,5 @@
>      nsCOMPtr<mozIDOMWindowProxy> win;
>      rv = bwin->OpenURI(uri, nullptr,
>                         nsIBrowserDOMWindow::OPEN_DEFAULTWINDOW,
>                         nsIBrowserDOMWindow::OPEN_NEW,
> +                       triggeringPrincipal,

I'm not a good reviewer for this code.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openuri.patch

It seems that uriloader/exthandler/nsWebHandlerApp.js needs to include Services.js - just added that inclusion, nothing else changed, carrying over r+ from gijs!

@baku: can you please review the chnage within dom/workers/ServiceWorkerClients.cpp? Everything else got covered by gijs - thanks!

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8879565 [details] [diff] [review]
bug_1364016_openuri.patch

Baku, please see previous comment - thanks!

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openuri_followup.patch

@gijs: When pushing to TRY I realized that I didn't update the callsite of OpenURI within nsNativeAppSupportWin which resulted in a compile error. There we are loading about:blank, hence I suppose we can use systemprincipal as the triggeringPrincipal.

@snorp: I saw that you reviewed [1]. It seems that jchen is still on PTO. Anyway, within this patch we are extending LoadURI with a triggeringPrincipal which is the security context that initiated the load. I am wondering what is triggering the load of 'uri' [1]. I browsed through the callsites but I can't really figure out what uri we are loading. If that uri comes from our own code, in other words it can't be influenced by the web, then using systemPrincipal would be fine. If that uri however comes from the web or something else we can't influence, then I suppose we have to do quite so many updates within some java code that get that right. What do you think?

[1] https://hg.mozilla.org/mozilla-central/rev/00a117d3c5fc#l4.75

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> Anyway, within this patch we are extending LoadURI with a

s/LoadURI/OpenURI

Comment 32

[:Gijs (out for now; he/him)]

Comment on attachment 8879647 [details] [diff] [review]
bug_1364016_openuri_followup.patch

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

r=me for the nsNativeAppSupportWin change.

For the android change, don't we also need to update the android nsIBrowserDOMWindow implementation in https://dxr.mozilla.org/mozilla-central/rev/464b2a3c25aa1065760d9ecbb0870bca4a66c62e/mobile/android/chrome/content/browser.js#3444 ?

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #32)
> For the android change, don't we also need to update the android
> nsIBrowserDOMWindow implementation in
> https://dxr.mozilla.org/mozilla-central/rev/
> 464b2a3c25aa1065760d9ecbb0870bca4a66c62e/mobile/android/chrome/content/
> browser.js#3444 ?

Probably you are right, let's wait for snorp's feedback first.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openuri.patch

I qfolded the nsNativeAppSupportWin.cpp change into this changeset so we can have a plain android patch.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openuri_android.patch

Snorp, please have a look at comment 30 first. Further, I am following up on Gijs suggestions from comment 32. Probably this needs a little more work. If we extend browser_openURI by an argument aTriggeringPrincipal, do we then also have to update _getBrowser() to pass that argument along? I don't know much how android is setup, hence I am wondering if we need to pass the triggeringPrincipal to BrowserApp.addTab() and potentially also to _getBrowser(). But I am not sure about all that. What's your take on this?

Comment 36

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8879938 [details] [diff] [review]
bug_1364016_openuri_android.patch

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

re: comment #30, that load is coming from the application, so the system principal seems fine.

::: mobile/android/chrome/content/browser.js
@@ +3435,5 @@
>  
>      // OPEN_CURRENTWINDOW and illegal values
>      let browser = BrowserApp.selectedBrowser;
>      if (aURI && browser) {
> +      // do we need to pass the triggeringPrincipal here as well?

I believe you do

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1364016_openuri_android.patch

Thanks for you feedback :snorp. For |function browser_openURIInFrame| we eventually need a follow up bug so that we can pass a valid triggeringPrincipal at some point. But I leave that up for a different bug.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=824fba6686d52e5f2cdeed5c9be500f6e9fec586

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/68b0765559ef
Have _openURIInNewTab() provide the correct triggeringPrincipal. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/46443484b9e6
Explicitly pass a triggeringPrincipal to openURI. r=gijs,baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ba929794237
Explicitly pass a triggeringPrincipal to openURI on android. r=snorp

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68b0765559ef
https://hg.mozilla.org/mozilla-central/rev/46443484b9e6
https://hg.mozilla.org/mozilla-central/rev/1ba929794237