841402 - Remove last piece of EnablePrivilege usage from CSP tests

Status: RESOLVED FIXED

(Core :: Security, defect)

Description

[Bobby Holley (:bholley)]

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. :-)

Comment 1

[Ian Melven :imelven]

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.

Comment 2

[Bobby Holley (:bholley)]

(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

[Ian Melven :imelven]

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

Comment 4

[Ian Melven :imelven]

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.

Comment 5

[Ian Melven :imelven]

needsinfo'ing myself so I get nagged about this and don't forget about it.

Comment 6

[Martijn Wargers (dead)]

Attachment: patch

I want ahead and used the postMessage thing on those .sjs files. I hope that's ok, Ian. This seems to work fine.

Comment 7

[Ian Melven :imelven]

Comment on attachment 744908 [details] [diff] [review]
patch

Better than ok, that's awesome ! Maybe bholley can r+ this for us ;)

Comment 8

[Bobby Holley (:bholley)]

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.

Comment 9

[Martijn Wargers (dead)]

Attachment: patch for checkin

With check for .call as suggested in comment 8.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3e95646e451

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b3e95646e451