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

VERIFIED FIXED in mozilla0.9.4

Status

()

Core
Networking
P1
normal
VERIFIED FIXED
17 years ago
4 years ago

People

(Reporter: bbaetz, Assigned: Darin Fisher)

Tracking

Trunk
mozilla0.9.4
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: r/sr=dougt,darin [PDT+] fixed-on-trunk,fixed-on-branch,testing-in-progress)

Attachments

(5 attachments)

(Reporter)

Description

17 years ago
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.

Updated

17 years ago
Summary: socks only works with http/https → SOCKS: only http|https re-directted in (post Mozilla 0.9.2)
(Reporter)

Comment 1

17 years ago
I need to discuss API changes with darin.
Priority: -- → P4
(Reporter)

Comment 2

17 years ago
Oh, and this will allow mail/news to work through socks as well, which according
to a post on npm.general ns4 could do.
Keywords: 4xp

Comment 3

17 years ago
Check with someone in mailnews about bug 44995.
(Reporter)

Comment 4

17 years ago
I should hopefully have an infrastructure patch for this early next week
Status: NEW → ASSIGNED
Keywords: nsenterprise
Priority: P4 → P2
Target Milestone: --- → mozilla0.9.4
(Reporter)

Comment 5

17 years ago
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.
(Reporter)

Comment 6

17 years ago
Created attachment 45879 [details] [diff] [review]
v0.8, proof of concept
(Reporter)

Updated

17 years ago
Blocks: 71565
(Reporter)

Updated

17 years ago
Blocks: 44995
(Reporter)

Comment 7

17 years ago
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.
(Reporter)

Comment 8

17 years ago
Created attachment 46170 [details] [diff] [review]
v0.9 patch
(Reporter)

Comment 9

17 years ago
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.
(Reporter)

Comment 10

17 years ago
Created attachment 46810 [details] [diff] [review]
patch, v1.0
(Assignee)

Comment 11

17 years ago
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
(Assignee)

Comment 12

17 years ago
dougt should review this too.

Comment 13

17 years ago
r=rginda for the chatzilla changes.
(Reporter)

Comment 14

17 years ago
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?
(Assignee)

Comment 15

17 years ago
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?
(Reporter)

Updated

17 years ago
Blocks: 96624
(Reporter)

Comment 16

17 years ago
Created attachment 46983 [details] [diff] [review]
patch v1.1, fully tested

Comment 17

17 years ago
I think that bbaetz is gone, reassigning to smeredith.
Assignee: bbaetz → smeredith
Status: ASSIGNED → NEW

Updated

17 years ago
Keywords: nsenterprise → nsenterprise+
(Reporter)

Comment 18

17 years ago
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.
Assignee: smeredith → bbaetz
(Reporter)

Comment 19

17 years ago
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.
Status: NEW → ASSIGNED
Whiteboard: r/sr=dougt,darin
(Reporter)

Comment 20

17 years ago
Created attachment 47287 [details] [diff] [review]
v1.2
(Reporter)

Comment 21

17 years ago
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.
Keywords: nsbranch
Target Milestone: mozilla0.9.4 → mozilla0.9.5

Comment 22

16 years ago
Marking nsbranch+ and PDT+.  Who is going to check this in?  Darin?  Steve?  Thanks!
Keywords: nsbranch → nsbranch+
Whiteboard: r/sr=dougt,darin → r/sr=dougt,darin [PTD+]

Updated

16 years ago
Whiteboard: r/sr=dougt,darin [PTD+] → r/sr=dougt,darin [PDT+]
(Assignee)

Comment 23

16 years ago
i can do it if bradley is unable to.  bradley?
(Reporter)

Comment 24

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

Comment 25

16 years ago
-> me for checkin
Assignee: bbaetz → darin
Status: ASSIGNED → NEW
(Assignee)

Comment 26

16 years ago
fixed-on-trunk
Status: NEW → ASSIGNED
Whiteboard: r/sr=dougt,darin [PDT+] → r/sr=dougt,darin [PDT+] fixed-on-trunk
(Assignee)

Comment 27

16 years ago
-> 0.9.4 branch
Target Milestone: mozilla0.9.5 → mozilla0.9.4
(Assignee)

Comment 28

16 years ago
untargeted until mozilla.org ships 0.9.4
Target Milestone: mozilla0.9.4 → ---
(Assignee)

Comment 29

16 years ago
back to 0.9.4, awaiting PDT approval for checkin on the 0.9.4 branch
Priority: P2 → P1
Target Milestone: --- → mozilla0.9.4

Comment 30

16 years ago
0.9.4 is out the door
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Comment 31

16 years ago
-> 0.9.4 for checkin on the nsbranch
Target Milestone: mozilla0.9.5 → mozilla0.9.4

Comment 32

16 years ago
Get r/sr = in patch status, and you can check it in.
(Assignee)

Comment 33

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

r=dougt
sr=darin
Attachment #47287 - Flags: superreview+
Attachment #47287 - Flags: review+
(Assignee)

Comment 34

16 years ago
hoping to land this on the branch tomorrow.

Comment 35

16 years ago
Has this landed on the branch?
(Assignee)

Comment 36

16 years ago
nope.. i got delayed.. i'm now planning to land this today.
(Assignee)

Comment 37

16 years ago
fixed-on-branch
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Whiteboard: r/sr=dougt,darin [PDT+] fixed-on-trunk → r/sr=dougt,darin [PDT+] fixed-on-trunk,fixed-on-branch

Comment 38

16 years ago
->qa to trix.

I'll dogfood, but I'm not the lead tester here.
QA Contact: benc → trix

Comment 39

16 years ago
reassigning qa to lrg
QA Contact: trix → lrg

Comment 40

16 years ago
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.
Summary: SOCKS: only http|https re-directted in (post Mozilla 0.9.2) → SOCKS: only http|https|ftp re-directted in (post Mozilla 0.9.2)
(Reporter)

Comment 41

16 years ago
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)
Summary: SOCKS: only http|https|ftp re-directted in (post Mozilla 0.9.2) → SOCKS: only http|https re-directted in (post Mozilla 0.9.2)

Comment 42

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

Updated

16 years ago
Whiteboard: r/sr=dougt,darin [PDT+] fixed-on-trunk,fixed-on-branch → r/sr=dougt,darin [PDT+] fixed-on-trunk,fixed-on-branch,testing-in-progress

Comment 43

16 years ago
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.