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

kaedys commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

You're right, of course.

mvdan commented 5 years ago

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?

alandonovan commented 5 years ago

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 commented 5 years ago

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.

golang / go