1359633 - Port Bug 1359092 - Extend loadURI within nsIWebNavigation.idl by a triggeringPrincipal argument to mailnews

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/mozilla-central/rev/fb4dc8b2f6af

Comment 1

[Jorg K (CEST = GMT+2)]

We seem to have three C++ callers of this:
	mailnews/base/src/nsMessenger.cpp
450	rv = webNav->LoadURI(NS_ConvertASCIItoUTF16(aURL).get(),   // URI string
	mailnews/base/src/nsMsgPrintEngine.cpp
499	rv = webNav->LoadURI(uri.get(),                        // URI string
	mailnews/base/src/nsMsgWindow.cpp
529	return webNav->LoadURI(NS_ConvertASCIItoUTF16(dataSpec).get(),

Patch coming.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1359633-loaduri.patch

To late to try to compile it now.

Using the system principal here seems like the way to go, right?

Comment 3

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/b217dfe75e798e7da5e80457ff1b18ed74e7ab31

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8861682 [details] [diff] [review]
1359633-loaduri.patch

I don't know.  It depends on where the URIs being loaded come from.

Comment 5

[Jorg K (CEST = GMT+2)]

I went be the description here:
https://hg.mozilla.org/mozilla-central/rev/fb4dc8b2f6af#l4.12

+   * @param aTriggeringPrincipal
+   *        The principal that initiated the load of aURI. If omitted docShell
+   *        tries to create a codeBasePrincipal from aReferrer if not null. If
+   *        aReferrer is also null docShell peforms a load using the
+   *        SystemPrincipal as the triggeringPrincipal.

Since there is no referrer, I use the SystemPrincipal.

As for the three calls:
nsMsgWindow::DisplayHTMLInMessagePane()
https://dxr.mozilla.org/comm-central/rev/5e4e889f13eb1fd5091f60921a1566c661f2c630/mailnews/base/src/nsMsgWindow.cpp#502
That seems to display some HTML passed in and converted to "data:text/html;base64,".
So maybe a null principal would have done the job here.

nsMsgPrintEngine::FireThatLoadOperation()
https://dxr.mozilla.org/comm-central/rev/5e4e889f13eb1fd5091f60921a1566c661f2c630/mailnews/base/src/nsMsgPrintEngine.cpp#499
That appears to be loading an arbitrary URI.

nsMessenger::OpenURL()
https://dxr.mozilla.org/comm-central/rev/5e4e889f13eb1fd5091f60921a1566c661f2c630/mailnews/base/src/nsMessenger.cpp#450
Also seems to be loading an arbitrary URL especially looking at the call sites of that function in nsMessenger::OpenAttachment() and nsMessenger::SaveAttachment().

Not easy to decide under bustage stress, but I can correct this in a follow-up.

Magnus, do you have an opinion here?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I went be the description here:

Right, so SystemPrincipal() is probably the behavior-preserving thing here.  Whether it's the _right_ thing, I can't tell you.

I'm fine with a followup to figure out what the right thing is, obviously.

Comment 7

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #5)

> As for the three calls:
> nsMsgWindow::DisplayHTMLInMessagePane()
> https://dxr.mozilla.org/comm-central/rev/
> 5e4e889f13eb1fd5091f60921a1566c661f2c630/mailnews/base/src/nsMsgWindow.
> cpp#502
> That seems to display some HTML passed in and converted to
> "data:text/html;base64,".
> So maybe a null principal would have done the job here

Agreed, this should not need system principal.

> nsMsgPrintEngine::FireThatLoadOperation()
> https://dxr.mozilla.org/comm-central/rev/
> 5e4e889f13eb1fd5091f60921a1566c661f2c630/mailnews/base/src/nsMsgPrintEngine.
> cpp#499
> That appears to be loading an arbitrary URI.
> 
> nsMessenger::OpenURL()
> https://dxr.mozilla.org/comm-central/rev/
> 5e4e889f13eb1fd5091f60921a1566c661f2c630/mailnews/base/src/nsMessenger.
> cpp#450
> Also seems to be loading an arbitrary URL especially looking at the call
> sites of that function in nsMessenger::OpenAttachment() and
> nsMessenger::SaveAttachment().

Yes, looks correct with system principal for these.

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8861682 [details] [diff] [review]
1359633-loaduri.patch

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

::: mailnews/base/src/nsMsgWindow.cpp
@@ +529,5 @@
>  
>    return webNav->LoadURI(NS_ConvertASCIItoUTF16(dataSpec).get(),
>                           nsIWebNavigation::LOAD_FLAGS_NONE,
> +                         nullptr, nullptr, nullptr,
> +                         nsContentUtils::GetSystemPrincipal());

Please make this null principal instead.
Not that it really matter much. The content we put here is at least currently completely under our own control.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1359633-follow-up-null.patch

Follow-up patch using a null principal in one case.

Comment 10

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/b217dfe75e798e7da5e80457ff1b18ed74e7ab31 (comment #3)
https://hg.mozilla.org/comm-central/rev/91accf84262c025310e1c1be51c44e6e132c2c26 (follow-up)