Closed Bug 1359633 Opened 3 years ago Closed 3 years ago

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

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: jorgk, Assigned: jorgk)

Details

Attachments

(2 files)

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.
To late to try to compile it now.

Using the system principal here seems like the way to go, right?
Attachment #8861682 - Flags: feedback?(bzbarsky)
https://hg.mozilla.org/comm-central/rev/b217dfe75e798e7da5e80457ff1b18ed74e7ab31
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
Comment on attachment 8861682 [details] [diff] [review]
1359633-loaduri.patch

I don't know.  It depends on where the URIs being loaded come from.
Attachment #8861682 - Flags: feedback?(bzbarsky)
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?
Flags: needinfo?(mkmelin+mozilla)
Attachment #8861682 - Flags: review?(mkmelin+mozilla)
> 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.
(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.
Flags: needinfo?(mkmelin+mozilla)
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.
Attachment #8861682 - Flags: review?(mkmelin+mozilla) → review+
Assignee: nobody → jorgk
Follow-up patch using a null principal in one case.
Attachment #8862633 - Flags: review+
You need to log in before you can comment on or make changes to this bug.