If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Change remaining CSP tests that use enablePrivilege to SpecialPowers

RESOLVED FIXED in mozilla23

Status

Testing
Mochitest
RESOLVED FIXED
5 years ago
5 years ago

People

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

Tracking

unspecified
mozilla23
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 741278 [details] [diff] [review]
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.
Attachment #741278 - Flags: review?(imelven)

Comment 1

5 years ago
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?)
(Assignee)

Comment 2

5 years ago
(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 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.
Attachment #741278 - Flags: feedback+

Updated

5 years ago
Blocks: 841402

Comment 4

5 years ago
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
Attachment #741278 - Flags: review?(imelven) → feedback+
(Assignee)

Comment 5

5 years ago
Created attachment 741516 [details] [diff] [review]
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.
Attachment #741278 - Attachment is obsolete: true
Attachment #741516 - Flags: review?(imelven)
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

5 years ago
(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 :)

Updated

5 years ago
Assignee: nobody → martijn.martijn
Status: NEW → ASSIGNED

Updated

5 years ago
Attachment #741516 - Flags: review?(imelven) → review+

Updated

5 years ago
Attachment #741516 - Flags: review+ → feedback+
(Assignee)

Updated

5 years ago
Attachment #741516 - Flags: review?(justin.lebar+bug)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 839480
Can we try someone who's not on the b2g critical path?  bholley knows about security things; maybe he can bless this patch.
Attachment #741516 - Flags: review?(justin.lebar+bug) → review?(bobbyholley+bmo)
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.
Attachment #741516 - Flags: review?(bobbyholley+bmo) → review+

Comment 11

5 years ago
(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).
(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.
(Assignee)

Comment 13

5 years ago
(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
(Assignee)

Comment 14

5 years ago
Created attachment 744333 [details] [diff] [review]
patch for checkin
Attachment #741516 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/915a35a8f516
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/915a35a8f516
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.