89500 - SOCKS: only http|https re-directted in (post Mozilla 0.9.2)

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Bradley Baetz (:bbaetz)]

See bug 89206. Fixing this involves a bit of code reorganisation, and won't
happen for a while.

If someone can comment on the proposed release note, I'll go and get it added.

DRAFT RELEASE NOTE:

Netscape/Mozilla only supports the use of SOCKS proxies for http and https
connections. Other protocols (such as ftp) will not attempt to use a socks
proxy.

The mox relnotes can reference this bug.

Comment 1

[Bradley Baetz (:bbaetz)]

I need to discuss API changes with darin.

Comment 2

[Bradley Baetz (:bbaetz)]

Oh, and this will allow mail/news to work through socks as well, which according
to a post on npm.general ns4 could do.

Comment 3

[benc]

Check with someone in mailnews about bug 44995.

Comment 4

[Bradley Baetz (:bbaetz)]

I should hopefully have an infrastructure patch for this early next week

Comment 5

[Bradley Baetz (:bbaetz)]

OK, I'm going to attach a patch. This allows datetime to work through a socks
proxy. Its not finished - next is ftp, then I'll try to remove all the
now-unneeded socks cruft from http. Then I'll work on mailnews.

ssl support will need a change to the helper function signature to pass the type in.
We need a better way of handling proxable types, to remove the checks from
nsProtocolProxyService. I'm thinking of two extra flags on the protocol handler
(via the recently added URIType attribute); URI_CAN_PROXY and
URI_CAN_PROXY_HTTP. Because of PAC, I need to separate this out (some pac files
will just return "PROXY foo:80", and that doesn't mean that smtp should be done
through the proxy, and PAC will support socks soon - I may do this as part of my
changes).

If a protocol doesn't have the URI_CAN_PROXY flag then CreateTransportInfo will
return null before talking to the protocolproxy service. If it doesn't have
URI_CAN_PROXY_HTTP, then if we end up getting a proxy of type "http", we'll
return null. (This also means that existing protocol handlers only have to be
modified if they want to support proxying. All protocols supported by http
proxying are in the mozilla tree, so thats not an issue for third parties)

comments?

ccing valeski for the proposed minor api change, and any other comments.

Comment 6

[Bradley Baetz (:bbaetz)]

Attachment: v0.8, proof of concept

Comment 7

[Bradley Baetz (:bbaetz)]

AAfter sepaking with darin, I plan to move the various helper functions onto
more appropriate interfaces, and possibly get rid of the transport creation
helper, and just change the signature of nsSocketTransport::CreateTransport and
friends.

And I got the check for a null transportInfo wrong.

Comment 8

[Bradley Baetz (:bbaetz)]

Attachment: v0.9 patch

Comment 9

[Bradley Baetz (:bbaetz)]

I had to redo the API a bit to allow https via a proxy to work (PSM needs the
correct final hostname to do cert verification). I think that its cleaner now,
as well.

All non mailnews protcols have been modified - looking for testing, r/sr.

Comment 10

[Bradley Baetz (:bbaetz)]

Attachment: patch, v1.0

Comment 11

[Darin Fisher]

some nits:

1) there exist references to nsISupportsTransparentProxy in nsIProtocolHandler.idl

2) the comments for URI_PROXY and URI_PROXY_HTTP need to be fixed.  the meaning
of these flags is not clear from the comments?

3) does the for loop near line 270 of nsSocketTransport.cpp need to be wrapped?
4) out of curiosity: are there http proxies out there that implement the
datetime protocol?

5) off by 1 indentation problem near callsite of NewProxiedChannel in
nsHttpHandler.cpp

6) fix instances of NewProxyChannel in nsHttpHandler::NewProxiedChannel

7) add comment block for nsHttpHandler::nsIProxiedProtocolHandler
fix these, test it out a bunch, and r/sr=darin

Comment 12

[Darin Fisher]

dougt should review this too.

Comment 13

[Robert Ginda]

r=rginda for the chatzilla changes.

Comment 14

[Bradley Baetz (:bbaetz)]

1) Fixed
2) Fixed
3) If I don't then its 86 chars, but given the rest of the file I've just
unwrapped it.
4) Doubt it. And even if there are, we don't have the pref for manual proxies,
and not all proxies support that. The flag being there was a typo, and I didn't
pick it up because it made no difference with manual proxies. Fixed.
5) I really dislike msvc's editor... fixed.
6) Fixed
7) I don't understand this. nsIHttpProtocolHandler now inherits of
nsIProxiedProtocolHandler, rather than nsIProtocolHandler. What do you mean by this?

Comment 15

[Darin Fisher]

the comment block

//-----------------------------------------------------------------------------
// nsHttpHandler::nsIProxiedProtocolHandler
//-----------------------------------------------------------------------------

is just meant to indicate that this section contains nsHttpHandler's
implementation of the methods declared on nsIProxiedProtocolHandler.  please
add this comment above the NewProxiedChannel method definition.  thanks!

also, can you attach a new patch when you get around to it?

Comment 16

[Bradley Baetz (:bbaetz)]

Attachment: patch v1.1, fully tested

Comment 17

[Hong Kwon]

I think that bbaetz is gone, reassigning to smeredith.

Comment 18

[Bradley Baetz (:bbaetz)]

I'm here til tuesday, and have r/sr=darin, and r/sr=dougt after we discuss the
overloading of URIFlags with darin (and changing URI_PROXY to URI_ALLOW_PROXY,
which I've done in my local tree)

Taking back for now.

Comment 19

[Bradley Baetz (:bbaetz)]

OK, patch with minor changes (URIType->ProtocolFlags), and a few style nits.
This one is larger, because I changed the method name on all the protocol
handlers. I'll take care of the commercial one-liner fix.

I'll mail drivers for approval.

Comment 20

[Bradley Baetz (:bbaetz)]

Attachment: v1.2

Comment 21

[Bradley Baetz (:bbaetz)]

ok, so drivers rejected this. Moving to 0.9.5, and nominating nsBranch.

This means that it won't go in before Tuesday (monday is a public holiday)
Either darin will check this in for me, or I'll have net access by then and will
do it myself (darin mailed me the mac mcp files). Either way I'll sort it out
next week.

Comment 22

[grega]

Marking nsbranch+ and PDT+.  Who is going to check this in?  Darin?  Steve?  Thanks!

Comment 23

[Darin Fisher]

i can do it if bradley is unable to.  bradley?

Comment 24

[Bradley Baetz (:bbaetz)]

Darin: Yes, please check this in for me - I still don't have net access at home.
Remember the commercial one-liner.

Comment 25

[Darin Fisher]

-> me for checkin

Comment 26

[Darin Fisher]

fixed-on-trunk

Comment 27

[Darin Fisher]

-> 0.9.4 branch

Comment 28

[Darin Fisher]

untargeted until mozilla.org ships 0.9.4

Comment 29

[Darin Fisher]

back to 0.9.4, awaiting PDT approval for checkin on the 0.9.4 branch

Comment 30

[Asa Dotzler [:asa]]

0.9.4 is out the door

Comment 31

[Darin Fisher]

-> 0.9.4 for checkin on the nsbranch

Comment 32

[Jaime Rodriguez, Jr.]

Get r/sr = in patch status, and you can check it in.

Comment 33

[Darin Fisher]

Comment on attachment 47287 [details] [diff] [review]
v1.2

r=dougt
sr=darin

Comment 34

[Darin Fisher]

hoping to land this on the branch tomorrow.

Comment 35

[Ken]

Has this landed on the branch?

Comment 36

[Darin Fisher]

nope.. i got delayed.. i'm now planning to land this today.

Comment 37

[Darin Fisher]

fixed-on-branch

Comment 38

[benc]

->qa to trix.

I'll dogfood, but I'm not the lead tester here.

Comment 39

[Hong Kwon]

reassigning qa to lrg

Comment 40

[benc]

RELNOTE Netscape 6.2:

Mozilla|Netscape supports the use of SOCKS 4 and 5 proxies for http, https and
ftp. Other protocols will ignore the SOCKS preference.

Comment 41

[Bradley Baetz (:bbaetz)]

benc: No - the bug was that only http|https was proxied. This has been fixed
with this patch (except for mailnews) Resetting summary

relnote: netscape6.x: Non-mailnews protocols will use socks proxies. However,
news and mail protocls will ignore any socks proxy settings you have made.

(I assume theres a separate relnote on password-authenticated socks proxies not
working)

Comment 42

[benc]

FTP works right? that's all I really want to know.

Comment 43

[lrg]

Hmm, last message didn't seem to get through.

Verfied fixed on MacOS Linux and Windows, no major problems.  Handled ftp http 
and https well.  When the proxy address was invalid, Mail/News, IMAP, and AIM 
requests went through fine across all platforms.

Small concern dealing with IMAP, the images refered in these files are often 
hosted externally to the imap document; In the case that the server where these 
images are stored is invalid, the page loading process is extremely slow.  In 
other cases of invalid (by proxy configuration) urls, it seems that this failure 
is quickly noticed, and Netscape doesn't try to load the blocked page.