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

Jump to bottom

x/net/idna: separate CheckHyphens from ValidateLabel #41732

Closed
TimothyGu opened this issue Oct 1, 2020 · 8 comments
Closed

x/net/idna: separate CheckHyphens from ValidateLabel #41732

TimothyGu opened this issue Oct 1, 2020 · 8 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@TimothyGu
Copy link
Contributor

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

$ go version
go version go1.15.2 linux/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
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/timothy-gu/.cache/go-build"
GOENV="/home/timothy-gu/.config/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/timothy-gu/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/timothy-gu/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/lib/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/lib/go/pkg/tool/linux_amd64"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/timothy-gu/dev/go/url/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 -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build009079035=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Since UTS #46 version 10.0.0, ToASCII has had the CheckHyphens option to control checking for hyphens in the validation stage of IDNA conversion independently from other options. Indeed, the URL standard for web browsers simultaneously has these two requirements:

  1. error out on "https://al\u200Cp.com/"
  2. but allow "http://r2---sn-gvbxgn-tt1s.googlevideo.com/"

This behavior is seen in Node.js as well as WebKit-based browsers like Safari. Here's the output from Node.js:

> new URL("https://al\u200Cp.com/")
Uncaught TypeError [ERR_INVALID_URL]: Invalid URL: https://al‌p.com/
> new URL("http://r2---sn-gvbxgn-tt1s.googlevideo.com/")
URL { ... }

However, x/net/idna does not allow controlling these two cases independently. When the ValidateLabels option is specified, the package would treat both as invalid; and if it's not specified, the package would treat both as valid: https://play.golang.org/p/O-nRi96pQH3

What did you expect to see?

x/net/idna should allow disabling hyphen checking independently of other label validation routines.

What did you see instead?

x/net/idna couples hyphen checking with other label validation routines.
@gopherbot
Copy link

Change https://golang.org/cl/258837 mentions this issue: internal/export/idna: Allow specifying CheckHyphens and CheckJoiners

@andybons
Copy link
Member

andybons commented Oct 1, 2020

@nigeltao @mpvl @bradfitz

@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 1, 2020
@andybons andybons added this to the Unplanned milestone Oct 1, 2020
@janikrabe
Copy link

Thank you @TimothyGu. At Cloudflare some of our customers are seeing issues because of this that your change would allow us to fix. Let us know if we can be of any help.

@TimothyGu
Copy link
Contributor Author

I've got a CL open: https://golang.org/cl/258837. It just needs to be reviewed. Ping @mpvl again.

@TimothyGu
Copy link
Contributor Author

Okay, the x/text CL is in. We still need a x/net CL to sync the generated files with x/text.

@gopherbot
Copy link

Change https://golang.org/cl/273586 mentions this issue: idna: allow specifying CheckHyphens and CheckJoiners

@janik-cloudflare
Copy link

@mpvl and @nigeltao, any chance you could take a look at the x/net CL as well? 🙏 It would be great if we could get this merged into the February 2022 release.

gopherbot pushed a commit to golang/net that referenced this issue May 1, 2021
@TimothyGu @tklauser
idna: allow specifying CheckHyphens and CheckJoiners
aec3718 
This copies CL 258837 from x/text over to this module.

Fixes golang/go#41732.

Change-Id: I41fc11c9a13faacb42504827063b60f7648426d2
Reviewed-on: https://go-review.googlesource.com/c/net/+/273586
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
Trust: Nigel Tao <nigeltao@golang.org>
Trust: Cherry Zhang <cherryyz@google.com>
@janik-cloudflare
Copy link

Thank you all!

@TimothyGu TimothyGu mentioned this issue May 6, 2021
Open
@golang golang locked and limited conversation to collaborators May 3, 2022
xhit pushed a commit to xhit/text that referenced this issue Oct 10, 2022
@TimothyGu @xhit
internal/export/idna: Allow specifying CheckHyphens and CheckJoiners
94a3dd4 
This aligns with the options in the latest version of UTS 46, and in
particular allows implementing the WHATWG URL Standard.

Fixes golang/go#41732.

Change-Id: Iab577eff4303f3eea64512d07d968c891acf126f
Reviewed-on: https://go-review.googlesource.com/c/text/+/258837
Reviewed-by: Marcel van Lohuizen <mpvl@golang.org>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Nigel Tao <nigeltao@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@andybons @TimothyGu @gopherbot @janikrabe @janik-cloudflare