1344706 - Do not reuse originPrincipal as triggeringPrincipal within browser/base/content/utilityOverlay.js

Status: RESOLVED FIXED

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

Description

[Christoph Kerschbaumer [:ckerschb]]

As agreed with Gijs over IRC (see also [1]) we should not reuse originPrincipal as the triggeringPrincipal within utilitOverlay.js. We introduced that behavior within [2]. I don't think it's critical at the moment, but we should fix that, probably by passing an explicit triggeringPrincipal argument.


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1343279#c4
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1331686

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1344706_do_not_reuse_originprincipal.patch

Gijs, I mostly based this patch on the patch that introduced OriginPrincipal [1]. Initial testing seems promising to me. Could you take a first look if you agree? In addition to running automated tests its crucial that we not regress view-image on canvas [2] which still works with this patch applied.

Here is a try run:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=268433065945f83ed94da3b4da2b3b4ac5d11e6d

Anything else I am missing?

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8793491&action=diff
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=1343279#c0

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

In fact we should fix this bug before moving on to fixing Bug 1333030, otherwise it gets too complicated.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8852886 [details] [diff] [review]
bug_1344706_do_not_reuse_originprincipal.patch

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

This looks OK at a glance for what this bug is about... though it's raised some other questions with me. I'll set needinfo and hopefully I'll have time to email you and a few other folks later today, otherwise tomorrow.

::: browser/base/content/utilityOverlay.js
@@ +260,5 @@
>    // opening a link in a new private window, or in a new container tab.
>    // 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 (aTriggeringPrincipal && aTriggeringPrincipal.isCodebasePrincipal) {

Do we not need to keep doing this for aPrincipal?

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #3)
> ::: browser/base/content/utilityOverlay.js
> @@ +260,5 @@
> >    // opening a link in a new private window, or in a new container tab.
> >    // 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 (aTriggeringPrincipal && aTriggeringPrincipal.isCodebasePrincipal) {
> 
> Do we not need to keep doing this for aPrincipal?

I think you are right and we need to keep doing this for aPrincipal as well.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1344706_do_not_reuse_originprincipal.patch

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb11fbb53cf
Do not reuse originPrincipal as triggeringPrincipal within utilityOverlay.js. r=gijs

Comment 7

[Carsten Book [:Tomcat]]

backed out for eslint failure like  TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/base/content/utilityOverlay.js:197:1 | Function 'openLinkIn' has a complexity of 44. (complexity)

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48747e70be14
Backed out changeset fdb11fbb53cf for eslint failure

Comment 9

[:Gijs (he/him)]

https://pastebin.mozilla.org/8984047

Should be enough to shut up the warning, and simplify the principal-changing code a tiny bit.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs from comment #9)
> https://pastebin.mozilla.org/8984047
> 
> Should be enough to shut up the warning, and simplify the principal-changing
> code a tiny bit.

Thanks - that shuts up the warning.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b5194257a9
Do not reuse originPrincipal as triggeringPrincipal within utilityOverlay.js. r=gijs

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/76b5194257a9