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: give mechanism to silence semicolon warning #49399

Open
chrisguiney opened this issue Nov 5, 2021 · 6 comments
Open

net/http: give mechanism to silence semicolon warning #49399

chrisguiney opened this issue Nov 5, 2021 · 6 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@chrisguiney
Copy link
Contributor

chrisguiney commented Nov 5, 2021

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

$ go version
go version go1.17.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

Linux/x86_64

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/chrisg/.cache/go-build"
GOENV="/home/chrisg/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/chrisg/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/chrisg"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/chrisg/sdk/go1.17.1"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/chrisg/sdk/go1.17.1/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.1"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
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-build3517216220=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Issued a request with non-encoded semicolons in the query string

https://play.golang.org/p/GsAqh5-wdQl

What did you expect to see?

No log output - we could receive a request like this from the internet easily. We have no mechanism to both silence this warning and accept the new behavior. This can cause log flooding for us. Any server that receives requests from the internet may get similar requests.

The only way that we can upgrade to go 1.17 is by using http.AllowQuerySemicolons and accepting the behavior from previous versions.

I understand the motivation to ensure that people are aware that there was a behavior change, but once we are aware, we're in a situation where there's really nothing we can do to silence these warnings. Any service that winds up with a nontrivial volume of requests that happen to have these query strings will get flooded with log output...I don't think that's any safer than the original issue it was trying to solve.

What did you see instead?

Un-silencable log output

@chrisguiney
Copy link
Contributor Author

chrisguiney commented Nov 5, 2021

After scratching my head a bit, the literal only way I've found to silence these messages is to intercept the log output from the http server: https://play.golang.org/p/v66AEQjrG4H

I don't think this is a reasonable approach to the issue though.

@seankhliao
Copy link
Member

seankhliao commented Nov 5, 2021

cc @FiloSottile

@cagedmantis cagedmantis added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 8, 2021
@cagedmantis cagedmantis added this to the Backlog milestone Nov 8, 2021
@cagedmantis
Copy link
Contributor

cagedmantis commented Nov 8, 2021

cc @neild also

@chrisguiney
Copy link
Contributor Author

chrisguiney commented Dec 8, 2021

@FiloSottile @neild

Just giving this a bump -- is there any plan to remove the log message in the future?

I'm happy to do any leg work, but it's not clear what an acceptable path forward is

@yktoo
Copy link

yktoo commented Dec 13, 2021

I second this, the warning must be dismissable.

@chrisguiney
Copy link
Contributor Author

chrisguiney commented Dec 13, 2021

I have found a workaround that works imho slightly better than filtering the log message out of the log

type semicolonKey struct{}

func storeRawQuery(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		if strings.Contains(r.URL.RawQuery, ";") {
			r2 := r.WithContext(context.WithValue(r.Context(), semicolonKey{}, r.URL.RawQuery))
			h.ServeHTTP(w, r2)
			return
		}
		h.ServeHTTP(w, r)
	})
}

func restoreRawQuery(h http.Handler) http.Handler {
	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		q, ok := r.Context().Value(semicolonKey{}).(string)
		if !ok {
			h.ServeHTTP(w, r)
			return
		}

		r2 := new(http.Request)
		*r2 = *r
		r2.URL = new(url.URL)
		*r2.URL = *r.URL
		r2.URL.RawQuery = q
		h.ServeHTTP(w, r2)
	})
}

func allowQuerySemicolons(h http.Handler) http.Handler {
         h = restoreRawQuery(h)
         h = http.AllowQuerySemicolons(h)
         h = storeRawQuery(h)
	return h
}

see here: https://go.dev/play/p/BcGLSVItJqU

Note this completely circumvents http.AllowQuerySemicolons -- except that it will disable the log message. Use with caution.

In my case, it's currently desirable because replacing ; with & is incredibly undesirable for me, but so is that log message. To give a sense of scope, I have to handle several hundred thousand requests per day containing semicolons...that log message is quite intrusive.

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

4 participants