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/url: invalid percent encodings rejected by go1.19 are now accepted #56884

Closed
liggitt opened this issue Nov 21, 2022 · 5 comments
Closed

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

liggitt opened this issue Nov 21, 2022 · 5 comments
Assignees
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone

Comments

@liggitt
Copy link
Contributor

liggitt commented Nov 21, 2022

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):

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
Copy link
Member

cc @neild @golang/security

@seankhliao seankhliao added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 21, 2022
@seankhliao seankhliao added this to the Go1.20 milestone Nov 21, 2022
@neild
Copy link
Contributor

neild commented Nov 21, 2022

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

@heschi
Copy link
Contributor

heschi commented Nov 21, 2022

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

@liggitt
Copy link
Contributor Author

liggitt commented Nov 22, 2022

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.

@neild neild self-assigned this Nov 22, 2022
@gopherbot
Copy link

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

@golang golang locked and limited conversation to collaborators Nov 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Development

No branches or pull requests

5 participants