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

cmd/vet: false positive printf detection for URL-encoded `/` (`%2F`) in string literal #29854

Open
kaedys opened this Issue Jan 21, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@kaedys
Copy link

kaedys commented Jan 21, 2019

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

$ go version
go version go1.11 darwin/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
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/msl21dp/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/msl21dp/go"
GOPROXY=""
GORACE=""
GOROOT="/Users/msl21dp/.gvm/gos/go1.11"
GOTMPDIR=""
GOTOOLDIR="/Users/msl21dp/.gvm/gos/go1.11/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/msl21dp/gomod/homedepot.com/vulcancore/go.mod"
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/4y/g8py5x0x0rx2qxlpxls23gnw0000gn/T/go-build585874623=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Using the logging library github.com/spf13/jwalterweatherman, I have this line in our logs:

logging.INFO.Println("Logs can be viewed at https://console.cloud.google.com/logs/viewer?project=redacted&logName=projects%2Fredacted%2Flogs%redacted")

This is supposed to print a URL to console that the user can click to go to the logs. Since the URL includes a slash-separated string as a parameter, those slashes have to be URL-encoded, which for a forward slach is %2F.

What did you expect to see?

No vet error

What did you see instead?

vet fires an error:

Logger.Println call has possible formatting directive %2F

Normally this wouldn't be an issue, but as of Go 1.10, go test now runs go vet natively with a hardcoded list of checks, and the only option is to either convert this to a Printf and doubling the percent signs to escape them, or disable go vet during go test, neither of which is a fantastic solution.

Recommendations / possible resolutions:

  • Figure out how to filter out situations like this. In this situation, the possible formatting directive is A) uppercase, and B) in a Print call with no arguments to take the place of that directive (which would mean I would have had to have improperly use a formatting directive in a non-f call and neglected to include the value to replace it with in the formatted string).
  • Allow specific tests to be disabled in the go test automatic run of go vet, possibly via a flag like -vetflags="-printf=false". This would also allow any of the other vet-specific flags to be passed down to it.
@dominikh

This comment has been minimized.

Copy link
Member

dominikh commented Jan 21, 2019

The real question is why vet thinks that Println is a printf wrapper when it isn't. If it was in fact a printf wrapper, then the report would be correct, and you should use %%2F to escape the %.

@kaedys

This comment has been minimized.

Copy link
Author

kaedys commented Jan 21, 2019

I think it's more calling out that this print call may have been intended to be a Printf call, but was inadvertently coded as Println instead, causing the directive to be ignored and any arguments simply appended. And that's a reasonable and fairly useful check. Just kinda falls down when you're using Println to print out URL-encoded characters that mimic actual formatting directives.

@dominikh

This comment has been minimized.

Copy link
Member

dominikh commented Jan 21, 2019

You're right, of course.

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Jan 21, 2019

Perhaps we could teach vet to ignore certain common false positives like %2F for /, %3F for ?, %3D for =, and so on. As long as we limit these to capital letters, I don't think we should worry about adding false negatives.

@alandonovan @josharian thoughts?

@mvdan mvdan added the NeedsDecision label Jan 21, 2019

@alandonovan

This comment has been minimized.

Copy link
Contributor

alandonovan commented Jan 22, 2019

Yes, checking for %XX (uppercase hex not starting with zero) might be the simplest workaround for all the URL encoding cases you mention. There will still be occasional false positives, of course.

@dominikh

This comment has been minimized.

Copy link
Member

dominikh commented Jan 22, 2019

IMO it shouldn't flag calls that have a single argument, either. It is unlikely that someone uses Println instead of Printf by accident and forgets to provide arguments for the format verbs.

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