865204 - Change remaining CSP tests that use enablePrivilege to SpecialPowers

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Martijn Wargers (dead)]

Attachment: patch

This patch changes the following mochitests to use SpecialPowers:
content/base/test/test_CSP.html
content/base/test/test_CSP_frameancestors.html
content/base/test/test_CSP_inlinescript.html
content/base/test/test_csp_redirects.html

I tested it locally.

Comment 1

[Ian Melven :imelven]

See also Bug 841402 - Remove enablePrivilege from tests added in bug 746978 - where I need to SpecialPowers-ize the new CSP tests I added that are basically copies of the existing ones but send the spec compliant header. 

Thanks for working on this Martijn, this is awesome ! I will take a look ASAP. As an aside, do you know if this will make these tests work on B2G (ie does SpecialPowers do the right thing for adding an observer in a process-separated world?)

Comment 2

[Martijn Wargers (dead)]

(In reply to Ian Melven :imelven from comment #1)
> ASAP. As an aside, do you know if this will make these tests work on B2G (ie
> does SpecialPowers do the right thing for adding an observer in a
> process-separated world?)

No, it won't. That's bug 839490. I want to fix that bug too (I hope I'm able to), but wanted first to convert these tests to use SpecialPowers.

Comment 3

[Sid Stamm [:geekboy or :sstamm]]

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

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

Thanks for taking this on Martijn.  I know you didn't ask for my feedback, but this looks good.  Also thanks for plugging in the EventUtils to simplify the click simulations.

If you take another pass and edit the patch, would you mind removing some of the trailing whitespace I put in these files?  (Shame on me.)  Not crucial, but if you edit the files again just scrub them please.

Comment 4

[Ian Melven :imelven]

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

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

Overall, looks good. There's still a handful of EnablePrivilege calls in test_CSP_inlinescript.html. I removed them and ran the tests locally and everything passed :)

Also, I noticed I accidentally left a dump() in test_CSP_inlinescript.html a while back if you wanted to remove that as well ;)

I checked the CSP inline style test I'm adding in bug 763879, it doesn't use EnablePrivilege.

I also noticed one other EnablePrivilege in the CSP frame ancestor tests, see http://mxr.mozilla.org/mozilla-central/source/content/base/test/file_CSP_frameancestors.sjs#24
there's a response.write('netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");');

there's a copy of this one in content/base/test/file_CSP_frameancestors_spec_compliant.sjs as well, if you remove that too, with the rest of your patch 
bug 841402 is also addressed, apologies for creating more of a mess there.

::: content/base/test/test_CSP_inlinescript.html
@@ +94,5 @@
>  window.examiner = new examiner();
>  SimpleTest.waitForExplicitFinish();
>  
>  function clickit() {
>    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

still an enablePrivilege call here

@@ +102,4 @@
>  }
>  
>  function clickit2() {
>    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

and here

@@ +109,4 @@
>  }
>  
>  function clickit3() {
>    netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');

and here

Comment 5

[Martijn Wargers (dead)]

Attachment: Updated patch

Sorry, forgot about those enableprivilege calls in test_CSP_inlinescript.html.

Removal of enableprivilege in file_CSP_frameancestors.sjs and file_CSP_frameancestors_spec_compliant.sjs gives me this js error while trying to run test_CSP_frameancestors.html:
 0:08.24 JavaScript error: http://example.com/tests/content/base/test/file_CSP_frameancestors_spec_compliant.sjs?double=1&scriptedreport=abb_allow_spec_compliant, line 1: Permission denied to access property 'frameLoaded'

I tried to use the SpecialPowers.wrap() function, but I can't seem to use that for this purpose, so I'm leaving those enablePrivilege calls in for now (unless someone knows a way).

Sid, I don't know where that trailing whitespace is in the files, I guess XCode is not really handy for finding that trailing whitespace.

Comment 6

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Martijn, re: whitespace, http://stackoverflow.com/questions/1390329/trim-trailing-spaces-in-xcode -- scroll down to the "as of Xcode 4.4" and it'll show you the option.

Comment 7

[Ian Melven :imelven]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #5)
> Created attachment 741516 [details] [diff] [review]
> Updated patch
> 
> Sorry, forgot about those enableprivilege calls in
> test_CSP_inlinescript.html.

No worries :)

> Removal of enableprivilege in file_CSP_frameancestors.sjs and
> file_CSP_frameancestors_spec_compliant.sjs gives me this js error while
> trying to run test_CSP_frameancestors.html:
>  0:08.24 JavaScript error:
> http://example.com/tests/content/base/test/
> file_CSP_frameancestors_spec_compliant.
> sjs?double=1&scriptedreport=abb_allow_spec_compliant, line 1: Permission
> denied to access property 'frameLoaded'
> 
> I tried to use the SpecialPowers.wrap() function, but I can't seem to use
> that for this purpose, so I'm leaving those enablePrivilege calls in for now
> (unless someone knows a way).

Yeah, I tried to fix that up as well and ran into the same problem actually :( We could leave those in for now and make bug 841402 about cleaning up that last vestige, I suppose.

I'm not a DOM peer so I suggest asking someone from https://wiki.mozilla.org/Modules/Core#Document_Object_Model for the actual r+ 

Thanks again for doing this cleanup :)

Comment 9

[Justin Lebar (not reading bugmail)]

Can we try someone who's not on the b2g critical path?  bholley knows about security things; maybe he can bless this patch.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 741516 [details] [diff] [review]
Updated patch

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

From the bottom of my heart (and the bowels of Gecko), thank you for doing this. :-)

::: content/base/test/test_CSP.html
@@ +74,5 @@
>      //_bad things better be stopped!
>  
>      if (topic === "http-on-modify-request") {
>        //these things were allowed by CSP
> +      var asciiSpec = SpecialPowers.getPrivilegedProps(SpecialPowers.do_QueryInterface(subject, "nsIHttpChannel"), "URI.asciiSpec");

This could probably be |SpecialPowers.wrap(subject).QueryInterface(SpecialPowers.Ci.nsIHttpChannel).URI.asciiSpec|;

This pattern appears elsewhere. Basically, for simple stuff, SpecialPowers.wrap(foo).get.something.privileged is the simplest way to go.

Comment 11

[Ian Melven :imelven]

(In reply to Bobby Holley (:bholley) from comment #10)
>
> This could probably be
> |SpecialPowers.wrap(subject).QueryInterface(SpecialPowers.Ci.nsIHttpChannel).
> URI.asciiSpec|;
> 
> This pattern appears elsewhere. Basically, for simple stuff,
> SpecialPowers.wrap(foo).get.something.privileged is the simplest way to go.

Thanks Bobby - any advice on how to use .wrap correctly for the bit we had trouble with in file_CSP_frameancestors.sjs :

 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. Could we use .wrap to help there ? Or would it be better to rewrite these to use postMessage (which is what I do in the iframe sandbox tests) ? (I would be happy to morph bug 841402 into switching to postMessage here and then take care of that).

Comment 12

[Bobby Holley (:bholley)]

(In reply to Ian Melven :imelven from comment #11)


> Thanks Bobby - any advice on how to use .wrap correctly for the bit we had
> trouble with in file_CSP_frameancestors.sjs :
> 
>  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. Could we use .wrap to help there ? Or would it be better to rewrite
> these to use postMessage (which is what I do in the iframe sandbox tests) ?
> (I would be happy to morph bug 841402 into switching to postMessage here and
> then take care of that).

SpecialPowers.wrap(foo) just gives you a js-proxy to foo that forwards any operations you do so that they happen in a system-principal-ed scope. It's basically just a one-size-fits-all replacement for stuff like SpecialPowers.do_QueryInterface and SpecialPowers.getPrivilegedProps.

Comment 13

[Martijn Wargers (dead)]

(In reply to Bobby Holley (:bholley) from comment #10)
> This could probably be
> |SpecialPowers.wrap(subject).QueryInterface(SpecialPowers.Ci.nsIHttpChannel).
> URI.asciiSpec|;

I tried to use that, it gave me this error:
 0:06.74 * Call to xpconnect wrapped JSObject produced this error:  *
 0:06.74 [Exception... "Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]"  nsresult: "0x80004002 (NS_NOINTERFACE)"  location: "JS frame :: chrome://specialpowers/content/specialpowersAPI.js :: doApply :: line 83"  data: no]

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

Comment 14

[Martijn Wargers (dead)]

Attachment: patch for checkin

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/915a35a8f516

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/915a35a8f516