105335 - PAC: SOCKS directive ignored

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[benc]

bbaetz says this has been broken for some time, but I haven't bothered to write
a bug until now:

STEPS:
On just about any post 0.9.0 version of mozilla, PAC directives "DIRECT" and
"PROXY" worked, but "SOCKS" never has.

Comment 1

[Bradley Baetz (:bbaetz)]

It not broken, it was just never checked in, because PAC was done before socks4
support was.

Its about 5 lines of code arround line 88 of nsProxyAutoConfig.js - basic, do
you want this?

Don't forget bug 78176.

Comment 2

[neeti]

PAC

Comment 3

[basic]

Attachment: patch (not tested)

Comment 4

[basic]

Attachment: patch (include fix for bug 91630)

Comment 5

[basic]

Attachment: patch (updated version of attachment 54125)

Comment 6

[Bradley Baetz (:bbaetz)]

Comment on attachment 54129 [details] [diff] [review]
patch (updated version of attachment 54125 [details] [diff] [review])


>-            // TODO warn about SOCKS
>+            // we ignore everything else past the first proxy.
>+            // we could theoretically check isResolvable now and continue
>+            // parsing (see bug 84798). but for now...
>+            var hostport = /^(.....) ([^:]+)(:\d+)?/(proxy);

I'd prefer something which just grabbed the first word, rather than expecting 5 characters.

You should do some sort of error handling if you get an unparsable string, right? Or do we just use DIRECT?

Comment 7

[basic]

currently we just do DIRECT (by not passing a type back). What sort of error
handling do you expect? A javascript error/warning/message?

Comment 8

[basic]

Attachment: patch

Comment 9

[basic]

Attachment: patch include fix for bug 91630 (take 3)

silly me, forgot to '+' the '\w'

Comment 10

[basic]

Attachment: patch (take 4) [includes fix for bug 91630]

improved the regexp a bit, made proxy type matching case insesitive and made
sure that we really ignore everything else past the first proxy.

Comment 11

[basic]

Attachment: patch (take 5) [includes fix for bug 91630]

tingley: mind taking another look?

Comment 12

[basic]

I think this is ready for review

Note that if this is checked in bug 112969 will probably need a new testcase or
something.

Comment 13

[Bradley Baetz (:bbaetz)]

Comment on attachment 61242 [details] [diff] [review]
patch (take 5) [includes fix for bug 91630]


>+                   type.value = "http";
>+                }
>+                // besides http proxy all other proxy types need a port number
>+                else if(typehostport[3] != null) {
>+                    port.value = typehostport[3].substr(1);

Are we not defaulting to 1080 for socks?

r=bbaetz in any event - do what ns4 did.

Comment 14

[basic]

Attachment: patch (take 6) [includes fix for bug 91630]

okay I tested PAC with nn4 and it turns out that SOCKS proxy defaults to port
1080
so here is take 6 (hopefully the last one)

Comment 15

[Darin Fisher]

the latest attachment is not a patch :P

Comment 16

[basic]

Attachment: patch (take 6) [includes fix for bug 91630] (the real thing)

Ooooopsssss sorry about that. Here is the patch

Comment 17

[Darin Fisher]

Comment on attachment 61492 [details] [diff] [review]
patch (take 6) [includes fix for bug 91630] (the real thing)

sr=darin

Comment 18

[basic]

assigning to tingley to get patch in after trunk opens

Comment 19

[Chase Tingley]

Fix checked in.

Comment 20

[benc]

+verifyme

I've added a sample PAC file. You need to download this file and modify it for
your test environment, b/c the SOCKS server mentioned does not exist anymore.

Comment 21

[benc]

VERIFIED: linux 2002-04-09-08 (pre branch)

Actually, you only need to map the hostname in your /etc/hosts to make the file
work if you want to try it yourself.

After realizing that Linux uses the TCP option for timestamps, was able to use
snoop on my server to verify the version number is 04.

# snoop -i pac -p 4 -x 66

  4   0.00000 h-10-169-111-116.nscp.aoltw.net -> carbuncle    SOCKS C port=1233 

           0: 0401 0051 3fc9 391d 4d4f 5a00              ...Q?.9.MOZ.

I'll verify the other platforms some other time.

Comment 22

[benc]

uh, this didn't work in the 0.9.4 branch right?