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

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Alexey Chernyak]

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

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

taking

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: makefile changes

sigh, ok, I forgot to change the makefiles... please review both patches

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

dougt, can you review?
darin, can you super-review?

Comment 5

[Gagan]

Comment on attachment 70920 [details] [diff] [review]
patch

r=gagan

Comment 6

[Gagan]

Comment on attachment 70920 [details] [diff] [review]
patch

r=gagan

Comment 7

[Gagan]

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.

Comment 8

[Darin Fisher]

Comment on attachment 70921 [details] [diff] [review]
makefile changes

mac project changes?  with that, sr=darin

Comment 9

[Darin Fisher]

Comment on attachment 70920 [details] [diff] [review]
patch

sr=darin

Comment 10

[Asa Dotzler [:asa]]

a=asa (on behalf of drivers) for checkin to 0.9.9

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

checked in

Comment 12

[Alexey Chernyak]

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?

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

I won't have time to look at this until this weekend; if you want about:config
to work earlier, please back it out.

Comment 14

[Matthias Versen [:Matti]]

*** Bug 128292 has been marked as a duplicate of this bug. ***

Comment 15

[Matthias Versen [:Matti]]

*** Bug 128476 has been marked as a duplicate of this bug. ***

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

changing summary in hope to avoid more dups

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: make about:config work again

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, gagan/dougt, can you review?
darin, can you super-review?

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

Comment 19

[Alexey Chernyak]

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)

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

>+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 :)

Comment 21

[Ben Bucksch (:BenB)]

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 22

[Bradley Baetz (:bbaetz)]

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.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: 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

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

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 25

[Bradley Baetz (:bbaetz)]

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.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: hopefully final patch

stupid os/2...

Comment 27

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 72295 [details] [diff] [review]
hopefully final patch

bbaetz gave me r=bbaetz on irc provided mstoltz says the changes are OK

Comment 28

[Boris Zbarsky [:bzbarsky]]

*** Bug 129114 has been marked as a duplicate of this bug. ***

Comment 29

[Mitchell Stoltz (not reading bugmail)]

Attachment: 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 30

[Christian :Biesinger (don't email me, ping me on IRC)]

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

ok, r=[e-mail] on your changes

Comment 31

[Darin Fisher]

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

sr=darin

Comment 32

[Asa Dotzler [:asa]]

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

Comment 33

[Christian :Biesinger (don't email me, ping me on IRC)]

checked into trunk

leaving open for branch checkin (asa gave me a= via mail/irc)

Comment 34

[Christian :Biesinger (don't email me, ping me on IRC)]

checked into branch

Comment 35

[benc]

If this was branch checked, which branch and should it have a branch keyword?

Comment 36

[Alexey Chernyak]

CLOSED

Comment 37

[Matthias Versen [:Matti]]

Please don't use closed !

Comment 38

[Matthias Versen [:Matti]]

.

Comment 39

[Matthias Versen [:Matti]]

v