net/url: invalid percent encodings rejected by go1.19 are now accepted

[liggitt]

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

$ go version

go version devel go1.20-831c6509cc Mon Nov 21 15:23:39 2022 +0000 darwin/arm64

Does this issue reproduce with the latest release?

No, this behavior is new in master

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

go env Output

$ go env
GO111MODULE=""

GOARCH="arm64"

GOBIN=""

GOCACHE="/Users/liggitt/Library/Caches/go-build"

GOENV="/Users/liggitt/Library/Application Support/go/env"

GOEXE=""

GOEXPERIMENT=""

GOFLAGS=""

GOHOSTARCH="arm64"

GOHOSTOS="darwin"

GOINSECURE=""

GOMODCACHE="/Users/liggitt/go/pkg/mod"

GONOPROXY=""

GONOSUMDB=""

GOOS="darwin"

GOPATH="/Users/liggitt/go"

GOPRIVATE=""

GOPROXY="https://proxy.golang.org,direct"

GOROOT="/Users/liggitt/git/go"

GOSUMDB="sum.golang.org"

GOTMPDIR=""

GOTOOLDIR="/Users/liggitt/git/go/pkg/tool/darwin_arm64"

GOVCS=""

GOVERSION="devel go1.20-831c6509cc Mon Nov 21 15:23:39 2022 +0000"

GCCGO="gccgo"

AR="ar"

CC="clang"

CXX="clang++"

CGO_ENABLED="1"

GOMOD="/dev/null"

GOWORK=""

CGO_CFLAGS="-O2 -g"

CGO_CPPFLAGS=""

CGO_CXXFLAGS="-O2 -g"

CGO_FFLAGS="-O2 -g"

CGO_LDFLAGS="-O2 -g"

PKG_CONFIG="pkg-config"

GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/37/ns7gt60104scfm9fvg02p1jh00kjgb/T/go-build1773376802=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Use url.Parse to check for invalid %-encoding sequences.

package main

import (
        "fmt"
        "net/url"
)

func main() {
        fmt.Println(url.Parse("invalid%"))
}

What did you expect to see?

An error, matching existing go releases.

What did you see instead?

No error.

Details

This change is due to #56732 / https://go-review.googlesource.com/c/go/+/450375, which intentionally relaxes validation requirements for percent-decoding of URLs.

This means that systems using url.Parse() to ensure URLs are well-formed now accept malformed %-encoding sequences. This is already showing up as unit test failures in other projects testing against the golang dev branch, e.g. kubernetes/kubernetes#113948, and required relaxing existing go unit tests in https://go-review.googlesource.com/c/go/+/450375 to accept previously rejected data.

Hoisting my question from #56732 (comment):

• Doesn't loosening of URI validation risk propagating invalid %-encodings to other systems that will choke on the invalid URIs (like, for example, something built with go1.19)? This seems opposite to the rationale used to justify tightening of ParseIP logic in go1.17 (net: reject leading zeros in IP address parsers [freeze exception] #30999).

If the implications of relaxing this are considered, and go1.20 decides to proceed relaxing this parsing to accept URIs as valid which go1.19 rejected as invalid, my follow-up questions are about how to roll out this change in a controlled way:

• For go1.20 users that want to keep validating things strictly to disallow invalid %-encodings, how should they change their code?
• For systems currently building with go1.19 that will need to update to build with go1.20 and want to preserve existing runtime behavior with minimally invasive code changes, how can they accomplish that?

cc @dgryski @ianlancetaylor as authors of the prompting issue and CL

cc @aojea @dims for implications on cross-version Kubernetes compatibility

cc @rsc for intersection with runtime compatibility behavior discussed in #55090

[seankhliao]

cc @neild @golang/security

[neild]

This change doesn't seem urgent. Should we roll it back and take our time figuring out the correct behavior in 1.21?

[heschi]

It also broke a wide variety of Google's tests.

[liggitt]

This change doesn't seem urgent. Should we roll it back and take our time figuring out the correct behavior in 1.21?

That would be my inclination, since it's not clear the cross-version and downstream implications of relaxing validation here were considered, and the standard change this was intended to improve interoperability with is not a recent one.

[gopherbot]

Change https://go.dev/cl/452795 mentions this issue: Revert "net/url, net/http/httputil: accept invalid percent encodings"