Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixed issue with including http:// in proxy URL #11229

Closed
wants to merge 2 commits into
from

Conversation

Projects
None yet
4 participants

Fixes #11117

Owner

ariya commented Apr 13, 2013

I think this may break the default port of 1080.

It does change it. It's defaulting to the default port for the specified scheme (e.g. 80 for http). Is the choice of 1080 for a specific reason? 3128 is the default port for squid. 6081 is the default for Varnish. Port 80 might not be a bad choice as it is the default for an http URL with no port specified and people would expect that

Collaborator

JamesMGreene commented Apr 13, 2013

Port 1080 seems like an odd default here as that port is usually for SOCKS proxies rather than HTTP proxies (which frequently default to 8080). I do know that Java will default an HTTP proxy without a specified port to 80, though.

Fixed issue in including http:// in proxy URL
#11117

Fixed issue in previous commit where not including the scheme
in proxy URL was not working.  Also changed the default port
back to 1080

I've taken another go at it. There was another big in my first commit. It wasn't working with host:port (ie no http://)

Owner

ariya commented Apr 14, 2013

1080 might be a bad choice for the default port but that's the current behavior and breaking the compatibility isn't worth it.

Makes sense. Especially as you can use socks. The second commit I've done has put the default back to that port

kanzure added a commit to kanzure/phantomjs that referenced this pull request May 27, 2013

fix quotes in proxyHost before connecting to proxy
HTTP-based proxy connections are now working again. The proxyPort parser
was working correctly, but QNetworkProxy was receiving bad data.

Might be related to #11052 or #11229.
Contributor

kanzure commented May 27, 2013

The patch provided by @simondean doesn't seem to work on Linux (I am not sure about other platforms). In particular, specifying --proxy=192.168.1.183:8080 on the command line will cause phantomjs (with his patch applied) to not send requests through the proxy. I am not yet sure why this is.

However, I should point out that without the patch from @simondean, it doesn't work anyway, so the root cause might not be something broken by his patch.

My changes in pull request #11353 combined with the patch from @simondean do not fix this problem. But #11353 applied alone does make phantomjs connect and send requests through a proxy.

Sorry if this sounds confusing. Let me know if I can help clarify.

Owner

ariya commented Jun 1, 2013

Superceded by #11360.

@ariya ariya closed this Jun 1, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment