Remove SEC_NORMAL from netwerk/test

RESOLVED FIXED in Firefox 47

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ckerschb, Assigned: mwobensmith)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Reporter)

Comment 1

3 years ago
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)
Blocks: 1193558
(Reporter)

Updated

3 years ago
Summary: Remove SEC_NORMAL from network/test → Remove SEC_NORMAL from netwerk/test
(Reporter)

Updated

3 years ago
Assignee: nobody → mwobensmith
Status: NEW → ASSIGNED
(Reporter)

Comment 2

3 years ago
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.
(Reporter)

Comment 3

3 years ago
Created attachment 8719099 [details] [diff] [review]
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
Attachment #8719099 - Flags: review?(mcmanus)
(Reporter)

Comment 5

3 years ago
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().
Attachment #8719895 - Flags: review?(mozilla)
(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.
Created attachment 8720030 [details] [diff] [review]
bug1246220.patch
Attachment #8719895 - Attachment is obsolete: true
Attachment #8720030 - Flags: review?(mozilla)
(Reporter)

Comment 8

3 years ago
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.
Attachment #8720030 - Flags: superreview?(mcmanus)
Attachment #8720030 - Flags: review?(mozilla)
Attachment #8720030 - Flags: review+
Created attachment 8720047 [details] [diff] [review]
Fixed trailing whitespace issues.

Carrying over r+ from before.
Attachment #8720047 - Flags: review+
Sorry, Bugzilla mistake. Should have marked this attachment so that it made the previous patch obsolete.
Attachment #8720030 - Flags: superreview?(mcmanus) → superreview+
Attachment #8719099 - Flags: review?(mcmanus) → review+

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4aa0fb77ae72
https://hg.mozilla.org/mozilla-central/rev/9ecae2e500e4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.