Closed Bug 841402 Opened 7 years ago Closed 7 years ago

Remove last piece of EnablePrivilege usage from CSP tests

Categories

(Core :: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bholley, Assigned: martijn.martijn)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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. :-)
Blocks: 841405
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.
(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!
We should probably just clean up all the CSP tests and have them use SpecialPowers everywhere while we're at it.
Depends on: 865204
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
Summary: Remove last EnablePrivilege from CSP tests → Remove last piece of EnablePrivilege usage from CSP tests
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
Attached patch patch (obsolete) — Splinter Review
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 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)
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+
With check for .call as suggested in comment 8.
Attachment #744908 - Attachment is obsolete: true
Assignee: imelven → martijn.martijn
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b3e95646e451
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.