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

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Security
P2
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 months ago
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
(Assignee)

Updated

3 months ago
Blocks: 1331686
(Assignee)

Updated

3 months ago
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
(Assignee)

Comment 1

2 months ago
Created attachment 8852886 [details] [diff] [review]
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
Attachment #8852886 - Flags: feedback?(gijskruitbosch+bugs)
(Assignee)

Updated

2 months ago
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
(Assignee)

Comment 2

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

Comment 3

2 months ago
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?
Attachment #8852886 - Flags: feedback?(gijskruitbosch+bugs) → feedback+

Updated

2 months ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 4

2 months ago
(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.

Updated

2 months ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 5

2 months ago
Created attachment 8853883 [details] [diff] [review]
bug_1344706_do_not_reuse_originprincipal.patch
Attachment #8853883 - Flags: review?(gijskruitbosch+bugs)

Updated

2 months ago
Attachment #8853883 - Flags: review?(gijskruitbosch+bugs) → review+
(Assignee)

Updated

2 months ago
Attachment #8852886 - Attachment is obsolete: true

Comment 6

2 months ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb11fbb53cf
Do not reuse originPrincipal as triggeringPrincipal within utilityOverlay.js. r=gijs
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)
Flags: needinfo?(ckerschb)

Comment 8

2 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/48747e70be14
Backed out changeset fdb11fbb53cf for eslint failure

Comment 9

2 months ago
https://pastebin.mozilla.org/8984047

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

Comment 10

2 months ago
(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.
Flags: needinfo?(ckerschb)

Comment 11

2 months ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b5194257a9
Do not reuse originPrincipal as triggeringPrincipal within utilityOverlay.js. r=gijs
https://hg.mozilla.org/mozilla-central/rev/76b5194257a9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.