about:config should use redirector, doesn't work, js error

VERIFIED FIXED in mozilla0.9.9

Status

()

Core
Networking
VERIFIED FIXED
16 years ago
15 years ago

People

(Reporter: Alexey Chernyak, Assigned: Biesinger)

Tracking

Trunk
mozilla0.9.9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

16 years ago
about:config should use redirector introduced in bug 68086

trivial modifications should be made to following files:
/mozilla/netwerk/build/nsNetModule.cpp
/mozilla/netwerk/protocol/about/src/nsAboutRedirector.cpp

and these 2 files should be removed:
/mozilla/netwerk/protocol/about/src/nsAboutConfig.cpp
/mozilla/netwerk/protocol/about/src/nsAboutConfig.h
taking
Assignee: gagan → cbiesinger
Target Milestone: --- → mozilla0.9.9
Created attachment 70921 [details] [diff] [review]
makefile changes

sigh, ok, I forgot to change the makefiles... please review both patches
dougt, can you review?
darin, can you super-review?
Status: NEW → ASSIGNED

Comment 5

16 years ago
Comment on attachment 70920 [details] [diff] [review]
patch

r=gagan
Attachment #70920 - Flags: review+

Comment 6

16 years ago
Comment on attachment 70920 [details] [diff] [review]
patch

r=gagan

Comment 7

16 years ago
Comment on attachment 70921 [details] [diff] [review]
makefile changes

r=gagan 

This is ok but please also make the relevant changes in the mac projects as
needed.
Attachment #70921 - Flags: review+

Comment 8

16 years ago
Comment on attachment 70921 [details] [diff] [review]
makefile changes

mac project changes?  with that, sr=darin
Attachment #70921 - Flags: superreview+

Comment 9

16 years ago
Comment on attachment 70920 [details] [diff] [review]
patch

sr=darin
Attachment #70920 - Flags: superreview+

Comment 10

16 years ago
a=asa (on behalf of drivers) for checkin to 0.9.9
Keywords: mozilla0.9.9+
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 12

16 years ago
about:config is broken

Error: uncaught exception: Permission denied to get property UnnamedClass.classes

Not enough security privileges in the redirector?
Can it be easily fixed or will it has to be backed out?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I won't have time to look at this until this weekend; if you want about:config
to work earlier, please back it out.
*** Bug 128292 has been marked as a duplicate of this bug. ***
*** Bug 128476 has been marked as a duplicate of this bug. ***
changing summary in hope to avoid more dups
Summary: about:config should use redirector → about:config should use redirector, doesn't work, js error
Created attachment 72219 [details] [diff] [review]
make about:config work again
Attachment #70920 - Attachment is obsolete: true
Attachment #70921 - Attachment is obsolete: true
ok, gagan/dougt, can you review?
darin, can you super-review?

(I will convert the tab to spaces before checking in)
(Reporter)

Comment 19

16 years ago
Comment on attachment 72219 [details] [diff] [review]
make about:config work again

+static const int kConfigNum = 3;

is this needed?

nits:

+    { "plugins", "chrome://global/content/plugins.html",  DROP},

DROP has 2 spaces in front and no space after it.

+	     //   these privileges

align this comment with others :o)
>+static const int kConfigNum = 3;
>is this needed?

no, it isn't... I'll remove it.

>+    { "plugins", "chrome://global/content/plugins.html",  DROP},
>DROP has 2 spaces in front and no space after it.

Oh, I thought I had already corrected this... Will really fix it before checking in.

>+
     //   these privileges
>align this comment with others :o)

This is the tab I talked about :)

Status: REOPENED → ASSIGNED

Comment 21

16 years ago
MY favorite resolution is to wontfix this and back out the change.
<about:config> could live just fine in its own class, given that it has
different needs than <about:credits>.

About the patch (This is no r=):
s/kRedirMap[i][2]/kRedirMap[i][2] != DONTDROP/
Comment on attachment 72219 [details] [diff] [review]
make about:config work again

>? .nsAboutRedirector.cpp.swp
>Index: nsAboutRedirector.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/protocol/about/src/nsAboutRedirector.cpp,v
>retrieving revision 1.6
>diff -u -r1.6 nsAboutRedirector.cpp
>--- nsAboutRedirector.cpp	2002/02/27 11:53:26	1.6
>+++ nsAboutRedirector.cpp	2002/03/02 13:51:03
>@@ -50,14 +50,20 @@
> 
> NS_IMPL_ISUPPORTS1(nsAboutRedirector, nsIAboutModule)
> 
>+static const char* DROP = (const char*)1;
>+static const char* DONTDROP = (const char*)0;
>+

Eww, use a struct.

>+static const int kConfigNum = 3;

Not needed, just loop for sizeof(foo)/sizeof(sturct whatever*)

This will return about:config to what it was before, but mstoltz should look at
this.
Attachment #72219 - Flags: needs-work+
Created attachment 72294 [details] [diff] [review]
new patch

doesn't use the sizeof method to calculate the length of the struct, because
the original code contains this:
static const int kRedirTotal = 4; // sizeof(kRedirMap)/sizeof(*kRedirMap)

so I thought there was a reason why it was commented out
but if you want me to use it, I'll do it
Attachment #72219 - Attachment is obsolete: true
mstoltz, could you take a look at this patch?

It makes it possible for pages using the about: redirector to get chrome
privileges, if they need to - this is needed for about:config
Comment on attachment 72294 [details] [diff] [review]
new patch

>? .nsAboutRedirector.cpp.swp
>Index: nsAboutRedirector.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/netwerk/protocol/about/src/nsAboutRedirector.cpp,v
>retrieving revision 1.6
>diff -u -r1.6 nsAboutRedirector.cpp
>--- nsAboutRedirector.cpp	2002/02/27 11:53:26	1.6
>+++ nsAboutRedirector.cpp	2002/03/03 12:17:36
>@@ -50,12 +50,18 @@
> 
> NS_IMPL_ISUPPORTS1(nsAboutRedirector, nsIAboutModule)
> 
>+struct RedirEntry {
>+    const char* id;
>+    const char* url;
>+    bool dropChromePrivs; // if true, the page will not have chrome privileges
>+};
>+

Need to use PRBool and PR_TRUE/PR_FALSE.
Attachment #72294 - Flags: needs-work+
Created attachment 72295 [details] [diff] [review]
hopefully final patch

stupid os/2...
Attachment #72294 - Attachment is obsolete: true
Comment on attachment 72295 [details] [diff] [review]
hopefully final patch

bbaetz gave me r=bbaetz on irc provided mstoltz says the changes are OK
Attachment #72295 - Flags: review+
*** Bug 129114 has been marked as a duplicate of this bug. ***
Created attachment 72716 [details] [diff] [review]
New patch - added a warning comment and a related security improvement

Your change looks fine, but I want to make sure about:config is safe before we
give it chrome privs again. The attached patch includes a change to config.js
that html-escapes the pref names as well as the pref values. This should
prevent future exploits. I've included your patch in mine, with the addition of
a warning comment to avoid turning on chrome privileges unnecessarily.
Christian, you've got my r= on your changes, could you review mine?
Comment on attachment 72716 [details] [diff] [review]
New patch - added a warning comment and a related security improvement

ok, r=cbiesinger@web.de on your changes
Attachment #72716 - Flags: review+

Comment 31

16 years ago
Comment on attachment 72716 [details] [diff] [review]
New patch - added a warning comment and a related security improvement

sr=darin
Attachment #72716 - Flags: superreview+

Comment 32

16 years ago
Comment on attachment 72716 [details] [diff] [review]
New patch - added a warning comment and a related security improvement

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #72716 - Flags: approval+
checked into trunk

leaving open for branch checkin (asa gave me a= via mail/irc)
checked into branch
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago16 years ago
Resolution: --- → FIXED

Comment 35

16 years ago
If this was branch checked, which branch and should it have a branch keyword?
(Reporter)

Comment 36

15 years ago
CLOSED
Status: RESOLVED → CLOSED
Please don't use closed !
Status: CLOSED → REOPENED
Resolution: FIXED → ---
.
Status: REOPENED → RESOLVED
Last Resolved: 16 years ago15 years ago
Resolution: --- → FIXED
v
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.