Remove last piece of EnablePrivilege usage from CSP tests

RESOLVED FIXED in mozilla24

Status

()

Core
Security
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bholley, Assigned: Martijn Wargers (dead))

Tracking

(Blocks: 2 bugs)

Trunk
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
I was just grepping through the commit log, and it appears that bug 746978 added nontrivial usage of enablePrivilege. That API is dangerous and deprecated, and per jst's decree new code may not add usage of enablePrivilege. So it looks like Ian's on the hook for fixing the tests. ;-)

The replacement API is SpecialPowers. Let me know if you need any help with it. :-)
(Reporter)

Updated

5 years ago
Blocks: 841405

Comment 1

5 years ago
Thanks for catching this, Bobby - I'll clean it up ASAP, I already modified a bunch of the other CSP tests to use SpecialPowers so this should be straight forward, I hope.
(Reporter)

Comment 2

5 years ago
(In reply to Ian Melven :imelven from comment #1)
> Thanks for catching this, Bobby - I'll clean it up ASAP, I already modified
> a bunch of the other CSP tests to use SpecialPowers so this should be
> straight forward, I hope.

Awesome, thanks Ian!

Comment 3

5 years ago
We should probably just clean up all the CSP tests and have them use SpecialPowers everywhere while we're at it.

Updated

5 years ago
Depends on: 865204

Comment 4

5 years ago
Martijn cleaned up almost all of this in his patch for bug 865204 (hooray!)

there is one remaining EnablePrivilege that he (and I previously) had trouble resolving using SpecialPowers.wrap. 

In file_CSP_frameancestors.sjs and file_CSP_frameancestors_spec_compliant.sjs we do:

 response.write('netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");');
 if (query['double'])
   response.write('window.parent.parent.parent.frameLoaded("' + query['scriptedreport'] +  
                  '",' + 'window.location.toString());');
 else
   response.write('window.parent.parent.frameLoaded("' + query['scriptedreport'] + '", ' +
                  'window.location.toString());');

it looks from the tests like we're using enablePrivilege to get around same origin. 

Martijn tried to use wrap() here, see https://bugzilla.mozilla.org/show_bug.cgi?id=865204#c13 :

"I also tried to use SpecialPowers.wrap(window).parent.parent.parent.frameLoaded in file_CSP_frameancestors_spec_compliant.sjs . With that, I got this error:
Error: TypeError: SpecialPowers.wrap(...).parent.parent.parent.frameLoaded is not a function
Source File: http://example.com/tests/content/base/test/file_CSP_frameancestors_spec_compliant.sjs?double=1&scriptedreport=abb_allow_spec_compliant
Line: 1"

We could work around the need for privilege here by reworking the test to use postMessage, I think. I'll take a stab at that when I can.
Component: DOM → Security
Summary: Remove enablePrivilege from tests added in bug 746978 → Remove last EnablePrivilege from CSP tests

Updated

5 years ago
Summary: Remove last EnablePrivilege from CSP tests → Remove last piece of EnablePrivilege usage from CSP tests

Comment 5

5 years ago
needsinfo'ing myself so I get nagged about this and don't forget about it.
Flags: needinfo?(imelven)
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Assignee)

Comment 6

5 years ago
Created attachment 744908 [details] [diff] [review]
patch

I want ahead and used the postMessage thing on those .sjs files. I hope that's ok, Ian. This seems to work fine.
Attachment #744908 - Flags: feedback?(imelven)

Comment 7

5 years ago
Comment on attachment 744908 [details] [diff] [review]
patch

Better than ok, that's awesome ! Maybe bholley can r+ this for us ;)
Attachment #744908 - Flags: review?(bobbyholley+bmo)
Attachment #744908 - Flags: feedback?(imelven)
Attachment #744908 - Flags: feedback+
Flags: needinfo?(imelven)
(Reporter)

Comment 8

5 years ago
Comment on attachment 744908 [details] [diff] [review]
patch

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

r=bholley with comment addressed.

::: content/base/test/test_CSP_frameancestors.html
@@ +115,5 @@
>  
> +window.addEventListener("message", receiveMessage, false);
> +
> +function receiveMessage(event) {
> +  if (event.data.call == 'frameLoaded')

This will break if someone postmessages you something tht doesn't have .call. Please check for that.
Attachment #744908 - Flags: review?(bobbyholley+bmo) → review+
(Assignee)

Comment 9

5 years ago
Created attachment 750353 [details] [diff] [review]
patch for checkin

With check for .call as suggested in comment 8.
Attachment #744908 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Assignee: imelven → martijn.martijn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b3e95646e451
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.