105340 - Proxy: manual config uses trailing spaces

Status: VERIFIED FIXED

(Core :: Networking, defect, P3)

Description

[benc]

Mozilla 0.9.5 + Netscape 6.2 (0.9.4, 2001101703) on MacOS X

STEPS:
1- Configure manual proxy preference, but add a trailing space to the hostname
"proxy.server.com "

2- Attempt to connect

OBSERVED:
you get an error about being unable to connect to the host.

EXPECTED:
" " is not valid in an FQDN, so prefs should discard the value.

Comment 1

[[e-mail] (gone - use [e-mail] instead)]

Hi Darin, this is the bug I was asking you about on IRC today.

Comment 2

[Darin Fisher]

-> me

Comment 3

[benc]

Darin: I've been thinking that for it would be nice to have some functions that
validate networking parameters (validatedFQDN, validateport). It seems like they
would be good ways of trapping bad preferences and controlling input values.
This would make the user experience better, along with stopping potential
security exploits.

If you were to fix bug 140379, in that manner, you could fix this bug at the
same time.

Comment 4

[Darin Fisher]

benc: good idea.

Comment 5

[Darin Fisher]

Attachment: v1 patch

fixes bug + some general cleanup.

Comment 6

[Darin Fisher]

Attachment: v2 patch

more cleanup... couldn't help myself :-)

ExamineForProxy had a number of inefficiencies, and since it is called for each
and every URL created, it is probably worthwhile to optimize.

Comment 7

[Doug Turner (:dougt)]

Comment on attachment 97494 [details] [diff] [review]
v2 patch

nit: keep the brace style the same throughout nsProtocolProxyService.h

add a comment to address why we are now stripping ws off of what is returned
from CopyCharPref.

Kill PL_strcmp.  Use strcmp.

fix that and r=dougt

Comment 8

[Darin Fisher]

will do, thx for the review dougt!

Comment 9

[Darin Fisher]

Attachment: v2.1 patch

Comment 10

[Darin Fisher]

Comment on attachment 98085 [details] [diff] [review]
v2.1 patch

forwarding r=dougt

Comment 11

[Darin Fisher]

not critical for 1.2 alpha.

Comment 12

[rpotts (gone)]

one note:

+    if (!pref || !strcmp(pref, "network.proxy.socks_version")) {
+        PRInt32 version;
+        LoadIntPref("network.proxy.socks_version", version);
+        // make sure this preference value remains sane
+        if (version == 4)
+            mSOCKSProxyVersion = 4;
+        else
+            mSOCKSProxyVersion = 5;

it appears that mSOCKSProxyVersion will now default to '5' if LoadIntPref()
fails...  where as before, it was '-1'.  Do you think this is an issue? OR a
feature ;-)

-- rick

Comment 13

[rpotts (gone)]

Comment on attachment 98085 [details] [diff] [review]
v2.1 patch

sr=[e-mail]

Comment 14

[benc]

It should always default to 4, for profile migration reasons. Is this a new
problem, or was it always like that?

Comment 15

[Darin Fisher]

rick, benc: i'll make it default to 4 then :)

Comment 16

[Darin Fisher]

fixed-on-trunk

Comment 17

[Jeff Thieleke]

The patch to nsProtocolProxyService.{cpp,h} has broken proxy support - the proxy
settings are completely ignored as of this change. 


Example:

user_pref("network.http.proxy.version", "1.0");
user_pref("network.proxy.http", "127.0.0.2");
user_pref("network.proxy.http_port", 8000);
user_pref("network.proxy.ssl", "127.0.0.2");
user_pref("network.proxy.ssl_port", 8000);
user_pref("network.proxy.type", 1);

Expected: Error dialog indicating that the HTTP connection failed.
Results:  Proxy settings are ignored, and the HTTP connection succeeds.  
(Note that valid proxy settings are also ignored, not just the invalid IP
address used in the above demonstration.)


Reverting to nsProtocolProxyService.cpp v1.38 and nsProtocolProxyService.h v1.13
fixes this problem.

Comment 18

[Mel Reyes]

I'm seeing this also with 092304 build under WinXP, luckily Auto Proxy still works.

Comment 19

[Darin Fisher]

investigating...

Comment 20

[Darin Fisher]

whoops!  fix checked in... a stray early return was fouling up everything :(

Comment 21

[benc]

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

Comment 22

[rbs]

+nsProtocolProxyService::LoadStringPref(const char *aPref, nsCString &aResult)
+nsProtocolProxyService::LoadIntPref(const char *aPref, PRInt32 &aResult)

Could be more reader-friendly to use GetStringPref() / GetIntPref().

Seeing that Load...() in necko, hmmm, it wasn't immediately intuitive that it
wasn't 'loading' something.

Comment 23

[Darin Fisher]

good suggestion.

Comment 24

[John Unruh]

Verified fixed.