1246220 - Remove SEC_NORMAL from netwerk/test

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

All of the tests listed underneath are using SEC_NORAML as a security flag when creating a new channel. We should use NetUtil.newChannel({uri: [url], loadUsingSystemPrincipal: true}); instead.

/netwerk/test/unit/test_aboutblank.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_referrer.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_bug337744.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_bug477578.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_about_protocol.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_authpromptwrapper.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_header_Accept-Language.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_http_headers.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_private_cookie_changed.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_bug826063.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_dns_proxy_bypass.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_websocket_offline.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_pac_generator.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_auth_dialog_permission.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_header_Accept-Language_case.js (View Hg log or Hg annotations)
/netwerk/test/unit/test_offlinecache_custom-directory.js (View Hg log or Hg annotations)

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Within File unit/test_NetUtil.js we should just replace SEC_NORMAL with SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL but leave everything else untouched.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: update_test_bug337744.patch

Hey Pat, we updated test_bug337744.js to use NewChannel2() within FF37 [1]. Unfortunately the test was not working correctly anymore after that change, because "Services" was not defined. We were missing a |Cu.import("resource://gre/modules/Services.jsm");| so that actually the line |Services.scriptSecurityManager.getSystemPrincipal()| was throwing the exception making the test succeed. In other words, we didn't have any actual test coverage for the expected behavior introduced within Bug 337744 anymore.

Within FF42, Bobby landed Bug 1161831 which removed ::NewChannel2() from nsResourceProtocolHandler [2], which made the test basically obsolete if I am not completely mistaken.

Within FF42 we landed the new securityManager and started to convert callsites to use asyncOpen2() [3], so I think it's just best if we rewrite the test to make sure that open2() and also asyncOpen2() on such 'resource' URIs fail. What do you think?

Potentially we should also update the filename because we are not testing Bug 337744 but rather the new securityManager.

[1] http://hg.mozilla.org/mozilla-central/rev/b60d7b975e9d
[2] https://hg.mozilla.org/mozilla-central/rev/7b078b5605a1
[3] http://hg.mozilla.org/mozilla-central/rev/1d919946baac

Comment 4

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: bug1246220.patch

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8719895 [details] [diff] [review]
bug1246220.patch

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

Hi Matt, looks good. Two nits: 1) please try to use a space after uri: and loadUsingSystemPrincipal and 2) please use the same syntax throughout the conversions, preferably:

var [channel] = NetUtil.newChannel({
  uri: [uri]
  loadUsingSystemPrincipal
})[.QueryInterface(bla)];

::: netwerk/test/unit/test_aboutblank.js
@@ +2,5 @@
>  
>  function run_test() {
> +  var base = NetUtil.newURI("http://www.example.com", null, null);
> +  var about1 = NetUtil.newURI("about:blank", null, null);
> +  var about2 = NetUtil.newURI("about:blank", null, base); 

nit: trailing space.

::: netwerk/test/unit/test_auth_dialog_permission.js
@@ +10,5 @@
> +/*
> + 0:00.82 TEST_STATUS: Thread-1 AuthPrompt.prototype.promptUsernameAndPassword FAIL 
> + [AuthPrompt.prototype.promptUsernameAndPassword : 70] Not expected the authentication prompt. - false == true
> +
> +*/

Please remove that file from your changeset. Sorry, I should have told you, but that test needs special attention so I created Bug 1240193. The test conversion is r+ed, but is blocked on converting callsites of TYPE_DOCUMENT.

::: netwerk/test/unit/test_authpromptwrapper.js
@@ +102,5 @@
>  
>    // Also have to make up a channel
> +  var uri = NetUtil.newURI("http://" + host, "", null)
> +  var chan = NetUtil.newChannel({
> +    uri:uri,

nit: missing space after uri:

@@ +202,5 @@
>  
>      // 5: FTP
> +    var uri2 = NetUtil.newURI("ftp://" + host, "", null);
> +    var ftpchan = NetUtil.newChannel({
> +      uri:uri2,

same here

::: netwerk/test/unit/test_bug337744.js
@@ +1,2 @@
> +// TODO: seems to be a bug
> +

please remove that file from your changeset. I created a separate patch for that testcase already.

::: netwerk/test/unit/test_header_Accept-Language.js
@@ +82,5 @@
>  function setupChannel(path) {
> +
> +  let chan = NetUtil.newChannel ({
> +    uri:"http://localhost:4444" + path,
> +    loadUsingSystemPrincipal:true

nit: space after uri: and loadUsingSystemPricnipal

::: netwerk/test/unit/test_header_Accept-Language_case.js
@@ +39,5 @@
>  function setupChannel(path) {
> +  let uri = NetUtil.newURI("http://localhost:4444" + path, "", null);
> +  let chan = NetUtil.newChannel({
> +    uri:uri,
> +    loadUsingSystemPrincipal:true

same nit here, missing space after uri:

::: netwerk/test/unit/test_offlinecache_custom-directory.js
@@ +16,4 @@
>  
>  var httpServer = null;
>  var cacheUpdateObserver = null;
>  var systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();

You can remove that line because you are not using the systemPrincipal anymore, right? Please make sure that line is removed from other tests as well in case the systemPrincipal is defined outside NewChannel().

Comment 6

[Matt Wobensmith [:mwobensmith][:matt:]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #5)

> ::: netwerk/test/unit/test_offlinecache_custom-directory.js
> @@ +16,4 @@
> >  
> >  var httpServer = null;
> >  var cacheUpdateObserver = null;
> >  var systemPrincipal = Services.scriptSecurityManager.getSystemPrincipal();
> 
> You can remove that line because you are not using the systemPrincipal
> anymore, right? Please make sure that line is removed from other tests as
> well in case the systemPrincipal is defined outside NewChannel().


Can't remove that, because this variable is also used in line 125.

Comment 7

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: bug1246220.patch

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8720030 [details] [diff] [review]
bug1246220.patch

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

Let's have Pat do a superreview just to make sure everything is fine (which I suppose it is, because it's just text replacement to use the new API and not changing any actual logic).

::: netwerk/test/unit/test_about_protocol.js
@@ +40,5 @@
>    registrar.registerFactory(classID, "", "@mozilla.org/network/protocol/about;1?what=unsafe", factory);
>  
> +  let aboutUnsafeChan = NetUtil.newChannel({
> +    uri: "about:unsafe",
> +    loadUsingSystemPrincipal: true 

Nit: trailing whitespace

::: netwerk/test/unit/test_aboutblank.js
@@ +7,4 @@
>  
> +  var chan1 = NetUtil.newChannel({
> +    uri: about1,
> +    loadUsingSystemPrincipal: true 

trailing whitespace

@@ +12,4 @@
>  
> +  var chan2 = NetUtil.newChannel({
> +    uri: about2,
> +    loadUsingSystemPrincipal: true 

trailing whitespace

::: netwerk/test/unit/test_authpromptwrapper.js
@@ +103,5 @@
>    // Also have to make up a channel
> +  var uri = NetUtil.newURI("http://" + host, "", null)
> +  var chan = NetUtil.newChannel({
> +    uri: uri,
> +    loadUsingSystemPrincipal: true 

trailing whitespace - seems like a copy/paste error, please update everywhere.

Comment 9

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: Fixed trailing whitespace issues.

Carrying over r+ from before.

Comment 10

[Matt Wobensmith [:mwobensmith][:matt:]]

Sorry, Bugzilla mistake. Should have marked this attachment so that it made the previous patch obsolete.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4aa0fb77ae72
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ecae2e500e4

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4aa0fb77ae72
https://hg.mozilla.org/mozilla-central/rev/9ecae2e500e4