Skip to content
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

proposal: net/http: return error for proxy CONNECT errors #44118

Closed
logandavies181 opened this issue Feb 5, 2021 · 5 comments
Closed

proposal: net/http: return error for proxy CONNECT errors #44118

logandavies181 opened this issue Feb 5, 2021 · 5 comments
Labels
Milestone

Comments

@logandavies181
Copy link

@logandavies181 logandavies181 commented Feb 5, 2021

What version of Go are you using (go version)?

$ go version
go version go1.15.8 linux/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

(actually using golang:latest eba5d0436b0b from dockerhub to compile)

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build988829932=/tmp/go-build -gno-record-gcc-switches"

What did you do?

main.go

package main

import (
        "fmt"
        "net/http"
)

func main() {
        url := "https://google.com"
        req, err := http.NewRequest("GET", url, nil)
        if err != nil {
                fmt.Println(err)
                return
        }

        resp, err := http.DefaultTransport.RoundTrip(req)
        if err != nil {
                fmt.Println(err)
        }
        if resp == nil {
                fmt.Println("Response is nil")
        }
}
# Proxy server returns 407 to unauthenticated requests
export https_proxy=http://proxy.example.com:3128
go run main.go

What did you expect to see?

I would like to make a contribution to update the error returned on non 200 response on CONNECT request from https proxy to be a new error type such that I can interrogate the response from the proxy, instead of having to handle the error based on the text of the error message.
So I'm able to do something like:

 switch e := err.(type) {
 case ProxyConnectError:
         handleProxyError(e.Response)
 default:
 return err
 }

Currently a basic error type is returned:

return nil, errors.New(f[1])

I'm happy to raise a PR if this is something that would be accepted

What did you see instead?

Proxy Authentication Required
Response is nil

@seankhliao seankhliao changed the title http.DefaultTransport doesn't expose CONNECT request to caller on https proxy error proposal: net/http: return error for proxy CONNECT errors Feb 5, 2021
@gopherbot gopherbot added this to the Proposal milestone Feb 5, 2021
@gopherbot gopherbot added the Proposal label Feb 5, 2021
@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Feb 5, 2021

I don't think RoundTrip should be returning an error for successfully getting a valid http response

@logandavies181
Copy link
Author

@logandavies181 logandavies181 commented Feb 5, 2021

Hi Sean, just to confirm, are you saying you think RoundTrip should return the non 200 reponse instead of the error it's currently returning?

@logandavies181
Copy link
Author

@logandavies181 logandavies181 commented Feb 5, 2021

I was going to make an alternative suggestion in this issue that RoundTrip return the error and the response;
seeing as it would be returning an error for a request you didn't explicitly make (i.e the CONNECT request)

@elagergren-spideroak
Copy link

@elagergren-spideroak elagergren-spideroak commented Feb 5, 2021

Dupe of #38143

@seankhliao
Copy link
Contributor

@seankhliao seankhliao commented Feb 5, 2021

let's keep it there

@seankhliao seankhliao closed this Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants