New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http-proxy: only attempt FTP over HTTP proxy #1505

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@bagder
Member

bagder commented May 22, 2017

... all other protocol schemes are now defaulting to "tunnel trough"
mode if a HTTP proxy is specified. In reality there are no HTTP proxies
out there that allow those other schemes.

ping @jay and @mkauf

Assisted-by: Ray Satiro, Michael Kaufmann

@mention-bot

This comment has been minimized.

mention-bot commented May 22, 2017

@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @captain-caveman2k, @yangtse and @yirkha to be potential reviewers.

@bagder

This comment has been minimized.

Member

bagder commented May 22, 2017

all other non-HTTP schemes

http-proxy: only attempt FTP over HTTP proxy
... all other non-HTTP protocol schemes are now defaulting to "tunnel
trough" mode if a HTTP proxy is specified. In reality there are no HTTP
proxies out there that allow those other schemes.

Assisted-by: Ray Satiro, Michael Kaufmann

@bagder bagder force-pushed the bagder/non-http-no-http-proxy branch from 7906896 to 8e34b02 May 22, 2017

@bagder

This comment has been minimized.

Member

bagder commented May 23, 2017

Due to the slight change in behavior, I figured I'll wait with merging this until the next feature-window opens.

@mkauf

mkauf approved these changes May 24, 2017

Thanks, this really simplifies the proxy code :-)

@@ -855,6 +855,7 @@ struct Curl_handler {
#define PROTOPT_STREAM (1<<9) /* a protocol with individual logical streams */
#define PROTOPT_URLOPTIONS (1<<10) /* allow options part in the userinfo field
of the URL */
#define PROTOPT_HTTP_PROXY (1<<11) /* allow over a HTTP proxy */

This comment has been minimized.

@mkauf

mkauf May 24, 2017

Contributor

I think we should explain that this flag is not intended for HTTP itself, and that the only difference is whether tunneling will be enabled automatically.

Maybe "HTTP proxies may know this protocol and act as a gateway" ?

This comment has been minimized.

@bagder

bagder May 24, 2017

Member

Thanks, that's a good point. I'll add something in that style.

This comment has been minimized.

@jay

jay May 24, 2017

Member

why don't we just put it directly in the flag, PROTOPT_ALLOW_HTTP_PROXY or something

This comment has been minimized.

@jay

jay May 24, 2017

Member

PROTOPT_PROXY_AS_HTTP is another one

This comment has been minimized.

@bagder

bagder May 24, 2017

Member

I like the latter suggestion more. PROTOPT_PROXY_AS_HTTP it shall be, thanks!

@bagder bagder closed this in efc83d6 Jun 15, 2017

@bagder bagder deleted the bagder/non-http-no-http-proxy branch Jun 15, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 13, 2018

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