network.security.ports.banned.override read from all.js but not from profile (prefs.js or user.js)

VERIFIED FIXED in mozilla0.9.6

Status

()

--
major
VERIFIED FIXED
18 years ago
17 years ago

People

(Reporter: piskozub, Assigned: dougt)

Tracking

(Blocks: 1 bug, {relnote})

Trunk
mozilla0.9.6
x86
Windows ME
relnote
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

v2
8.67 KB, patch
dougt
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

18 years ago
The new preference to override default port blocking
network.security.ports.banned.override (bug 85601) works when inserted into the
default all.js file - for example pref("network.security.ports.banned.override",
"1080");. This setting lets me seeing any http://foo:1080 URL

However setting this pref in the profile (either prefs.js or user.js) as
user_pref("network.security.ports.banned.override", "1080"); does not have any
effect. That is a warning about banned port pops up.

The code to read this pref is in /mozilla/netwerk/base/src/nsIOService.cpp. I
cannot guess why it works only with all.js. Do prefs from the profile files need
registering?
(Reporter)

Comment 1

18 years ago
Setting severity to major as prefs in all.js are subject to overwriting with
every  Mozilla installation and adding mozilla0.9.3 as it seems very easy to fix
(if someone understands the prefs code).
Severity: normal → major
Keywords: mozilla0.9.3
->security?
Assignee: sgehani → mstoltz
Component: Preferences → Security: General
QA Contact: sairuh → ckritzer
(Reporter)

Comment 3

18 years ago
Why security? What is the reason of a setting to override the default port
blocking if an user cannot use them to see a page that has a "banned" port number? 

Actually there should be an UI setting for this but it is the focus of bug
85601. I believe therefore that this bug is blocking 85601 - marking
accordingly. If someone disagrees, plese clear the block.
Blocks: 85601
Over to dougt.
Assignee: mstoltz → dougt
(Assignee)

Comment 5

18 years ago
this is kind of pref problem... or you are not using the correct syntax in your
user.js file.

Try using "user_pref" instead of "pref"
(Reporter)

Comment 6

18 years ago
Dough, please read the bug :-( 

One uses user_pref(); in prefs.js or user.js and pref(); in
/defaults/pref/all.js. Please notice the correct use of both setting in my
original comment. I used both exactly to avoid suggestions like that...

In meantme I asked off-page Darin Fisher who saw similar behavior in another
bug. He suggests that some necko components do not register themselves as
profile/pref change observers and therefore have no way of detecting differences
between prefs.js and all.js if the component were loaded before a profile is
selected.
(Assignee)

Comment 7

18 years ago
Samir, why does this work only in all.js?  What am I doing wrong?
dougt - based on the comments from darin, I suspect you need to register
yourself as a pref observer, with a callback. See the code in nsHttpHandler::Init()
(Assignee)

Comment 9

18 years ago
ugh.  you are right... the profilemanager reads these in AFTER the ioserver has
started.... crap.

This probably wont be fixed until after 0.9.3 unless someone submits a patch.

Target Milestone: --- → Future
relnote added to commercial release notes, and I'll add one to the 0.9.4 relnote
bug when one is created.
Keywords: relnote
(Assignee)

Updated

17 years ago
Target Milestone: Future → mozilla1.0
(Assignee)

Comment 11

17 years ago
people are annoyed that they can't do this.  moving to 9.6
Target Milestone: mozilla1.0 → mozilla0.9.6

Comment 13

17 years ago
Patch registers nsIOService with prefs.  Following Darin's lead in bug  102332,
it also listens for xpcom-shutdown so that the prefs observers can be removed --
otherwise everything leaks.
Keywords: patch, review

Comment 14

17 years ago
Comment on attachment 52120 [details] [diff] [review]
patch

>Index: nsIOService.cpp

>@@ -205,50 +208,23 @@

>+    // Further modifications to the port list come from prefs
>+    nsCOMPtr<nsIPrefBranch> prefBranch;
>+    GetPrefBranch(getter_AddRefs(prefBranch));
>+    if (prefBranch) {
>+        nsCOMPtr<nsIPrefBranchInternal> pbi = do_QueryInterface(prefBranch);
>+        if (pbi)
>+            pbi->AddObserver(NETWORK_PORT_PREFS, this);
>+        PrefsChanged(prefBranch);
>         }
> 

looks like you have an indentation problem here.




>+void
>+nsIOService::PrefsChanged(nsIPrefBranch *prefs, const char *pref)
>+{
>+    if (!prefs) return;
>+
>+    nsresult rv = NS_OK;
>+    nsXPIDLCString portList;
>+
>+    // Look for extra ports to block
>+    if ((pref == nsnull) || PL_strcmp(pref, "network.security.ports.banned") == 0) {
>+        rv = prefs->GetCharPref("network.security.ports.banned", 
>+                                getter_Copies(portList));
>+        if (portList) {
>+            char* tokp;
>+            char* currentPos = (char *)portList.get();  
>+            while ( (tokp = nsCRT::strtok(currentPos, ",", &currentPos)) != nsnull )
>+            {
>+                nsCAutoString tmp(tokp);
>+                tmp.StripWhitespace();
>+
>+                PRInt32 aErrorCode;
>+                PRInt32 value = tmp.ToInteger(&aErrorCode);
>+                mRestrictedPortList.AppendElement((void*)value);
>+            }
>+
>+        }
>+    }
>+    // ...as well as previous blocks to remove.
>+    if ((pref == nsnull) || PL_strcmp(pref, "network.security.ports.banned.override") == 0) {
>+        rv = prefs->GetCharPref("network.security.ports.banned.override", 
>+                                getter_Copies(portList));
>+        if (portList) {
>+            char* tokp;
>+            char* currentPos = (char *)portList.get();  
>+            while ( (tokp = nsCRT::strtok(currentPos, ",", &currentPos)) != nsnull )
>+            {
>+                nsCAutoString tmp(tokp);
>+                tmp.StripWhitespace();
>+
>+                PRInt32 aErrorCode;
>+                PRInt32 value = tmp.ToInteger(&aErrorCode);
>+                mRestrictedPortList.RemoveElement((void*)value);
>+            }
>+
>+        }
>+    }
>+}

this routine needs to be cleaned up...  do not duplicate code like this.  either use
a loop or a helper function.




>+// nsIObserver interface
>+NS_IMETHODIMP
>+nsIOService::Observe(nsISupports *subject,
>+                     const PRUnichar *topic,
>+                     const PRUnichar *data)
>+{
>+    if (!nsCRT::strcmp(topic, NS_LITERAL_STRING("nsPref:changed").get())) {
>+        nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(subject);
>+        if (prefBranch)
>+            PrefsChanged(prefBranch, NS_ConvertUCS2toUTF8(data).get());
>+    }
>+    else if (!nsCRT::strcmp(topic, NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get())) {
>+        // Clean up the observers
>+        nsCOMPtr<nsIPrefBranch> prefBranch;
>+        GetPrefBranch(getter_AddRefs(prefBranch));
>+        if (prefBranch) {
>+            nsCOMPtr<nsIPrefBranchInternal> pbi = do_QueryInterface(prefBranch);
>+            if (pbi)
>+                pbi->RemoveObserver(NETWORK_PORT_PREFS, this);
>+        }
>+
>+        nsCOMPtr<nsIObserverService> observerService = 
>+            do_GetService(NS_OBSERVERSERVICE_CONTRACTID);
>+        if (observerService)
>+            observerService->RemoveObserver(this, NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get());

I recall there being a bug that you can't remove a nsIObserver from the observer service
during its Observe method.  HTTP implements nsISupportsWeakReference, and the observer
service is implemented such that it'll actually hold only a weak reference to the observer
if it supports weak referencing.  This is why HTTP does not remove itself from the observer
service at xpcom-shutdown time.



>+    }
>+    return NS_OK;
>+}
>Index: nsIOService.h
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/base/src/nsIOService.h,v
>retrieving revision 1.35
>diff -u -r1.35 nsIOService.h
>--- nsIOService.h	2001/09/28 20:08:59	1.35
>+++ nsIOService.h	2001/10/04 22:47:15
>@@ -51,12 +51,16 @@
> #include "nsIEventQueueService.h"
> #include "nsIURLParser.h"
> #include "nsSupportsArray.h"
>+#include "nsIObserver.h"
> 
> #define NS_N(x) (sizeof(x)/sizeof(*x))
> 
> static const char *gScheme[] = {"chrome", "resource", "file", "http"};
> 
>+class nsIPrefBranch;
>+
> class nsIOService : public nsIIOService
>+                  , public nsIObserver
> {
> public:
>     NS_DECL_ISUPPORTS
>@@ -64,6 +68,9 @@
>     // nsIIOService methods:
>     NS_DECL_NSIIOSERVICE
> 
>+    // nsIObserver methods:
>+    NS_DECL_NSIOBSERVER
>+
>     // nsIOService methods:
>     nsIOService();
>     virtual ~nsIOService();
>@@ -88,6 +95,10 @@
> 
>     NS_METHOD CacheURLParser(const char *scheme,
>                              nsIURLParser* hdlr);
>+
>+    // Prefs
>+    void PrefsChanged(nsIPrefBranch *prefs, const char *pref = nsnull);
>+    void GetPrefBranch(nsIPrefBranch **);
> 
> protected:
>     PRBool      mOffline;

Comment 15

17 years ago
> looks like you have an indentation problem here.

oops.

> this routine needs to be cleaned up... 

This is the original code, so I wasn't going to muck with it much.  However,
this would be a good opportunity to tighten it up.

> I recall there being a bug that you can't remove a nsIObserver from the 
> observer service during its Observe method.

It looks like bug 94349 and bug 68200 mention this.  I'll add weak reference
support.







Updated

17 years ago
Attachment #52120 - Attachment is obsolete: true

Comment 17

17 years ago
Comment on attachment 52211 [details] [diff] [review]
v2

sr=darin (good job cleaning this up!)
Attachment #52211 - Flags: superreview+
(Assignee)

Updated

17 years ago
Attachment #52211 - Flags: review+

Comment 18

17 years ago
Thanks for the quick reviews.

When the trunk unfreezes, I'll need someone to check this in for me.
(Assignee)

Comment 19

17 years ago
Checking in nsIOService.cpp;
/cvsroot/mozilla/netwerk/base/src/nsIOService.cpp,v  <--  nsIOService.cpp
new revision: 1.115; previous revision: 1.114
done
Checking in nsIOService.h;
/cvsroot/mozilla/netwerk/base/src/nsIOService.h,v  <--  nsIOService.h
new revision: 1.36; previous revision: 1.35
done

Thanks alot for fixing this problem!
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 20

17 years ago
Marking verified as per above developer comments.
Status: RESOLVED → VERIFIED
QA Contact: ckritzer → bsharma
You need to log in before you can comment on or make changes to this bug.