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

net/http: redirect with 308 rather than 301 #60769

Open
xilu0 opened this issue Jun 13, 2023 · 5 comments
Open

net/http: redirect with 308 rather than 301 #60769

xilu0 opened this issue Jun 13, 2023 · 5 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@xilu0
Copy link

xilu0 commented Jun 13, 2023

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

$ go version
go version go1.17 linux/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output
$ go env
GO111MODULE="on"
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/root/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/root/go"
GOPRIVATE=""
GOROOT="/root/sdk/go1.17"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/root/sdk/go1.17/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-I/opt/src//clidriver/include"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-L/opt/src//clidriver/lib"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build210934838=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I implemented an HTTP proxy server in Go using the net/http package. The server is supposed to handle and forward incoming requests without modifying them. The proxy server encountered a specific scenario where incoming HTTP requests have a double slash ("//") at the beginning of their path, for instance, //PAM-OAuth/oauth2/token.

What did you expect to see?

I expected the Go net/http library to forward the incoming requests as is, preserving their original form including the HTTP method, the path with the double slash and all request parameters.

What did you see instead?

What I observed was the HTTP request's method was inadvertently changed from POST to GET during the proxying process, and the request path was altered from //PAM-OAuth/oauth2/token to /PAM-OAuth/oauth2/token (removing the double slash). Moreover, the request parameters were lost during this process, leading to a failed request when it reached the final destination server.
I understand that the net/http package may be trying to sanitize the request path by removing the double slash. However, in the context of a proxy server, this automatic alteration of the request details leads to unintended consequences and erroneous behavior.
To resolve this issue, I propose adding an option to disable this automatic sanitization or clean-up in the net/http package, especially in proxy server scenarios. This would ensure that the incoming HTTP requests are faithfully forwarded as they are, without any alterations to the request method, path, or parameters.

Please consider this proposal as it would significantly improve the usability and robustness of the Go net/http package in handling complex real-world scenarios such as proxy server implementations.

I look forward to your feedback and am happy to provide any further details or clarifications as needed.

@seankhliao
Copy link
Member

I don't think the net/http server does that
https://go.dev/play/p/HAcL0HdLGd-

@seankhliao seankhliao added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 13, 2023
@seankhliao seankhliao changed the title affected/package: Proposal: net/http should provide an option to disable path cleaning proposal: net/http: should provide an option to disable path cleaning Jun 13, 2023
@gopherbot gopherbot added this to the Proposal milestone Jun 13, 2023
@bcmills
Copy link
Contributor

bcmills commented Jun 13, 2023

Per https://pkg.go.dev/net/http#Client.Do:

“If permitted, a 301, 302, or 303 redirect causes subsequent requests to use HTTP method GET (or HEAD if the original request was HEAD), with no body. A 307 or 308 redirect preserves the original HTTP method and body, provided that the Request.GetBody function is defined.”

That is also consistent with the behavior described in RFC 9110; see in particular the note at https://www.rfc-editor.org/rfc/rfc9110.html#section-15.4-3.1.


Where this comes into play on the server side is with path cleaning in (*ServeMux).Handler here. If the path is not clean, it is implicitly redirected using code 301, not the method-preserving equivalent code 308.

But that makes me wonder: should ServeMux.Handler just be changed to use code 308 instead of 301 unconditionally?

(attn @neild)

@bcmills bcmills removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 13, 2023
@neild
Copy link
Contributor

neild commented Jun 13, 2023

Changing ServeMux.Handler to use 308 rather than 301 seems reasonable to me.

@bcmills
Copy link
Contributor

bcmills commented Jun 14, 2023

I guess the caveat with status 308 is (per https://www.rfc-editor.org/rfc/rfc7538#section-4) that that status code has only been defined since June 2014, so some very old HTTP clients still might not understand it.

Still, though, I think with an appropriate GODEBUG setting to revert to 301, 308 seems like a better semantic fit, and 9 years seems like long enough for clients to have built out support...

@ianlancetaylor
Copy link
Contributor

Sounds like this is a reasonable net/http change, so taking it out of the proposal process. Please comment if you disagree.

@ianlancetaylor ianlancetaylor changed the title proposal: net/http: should provide an option to disable path cleaning net/http: redirect with 308 rather than 301 Jun 14, 2023
@ianlancetaylor ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed Proposal labels Jun 14, 2023
@ianlancetaylor ianlancetaylor modified the milestones: Proposal, Backlog Jun 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

6 participants