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.

Sign up for GitHub

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: backward compatibility issues with change 2 #19821

Closed
weppos opened this issue Apr 2, 2017 · 3 comments
Closed

x/net/idna: backward compatibility issues with change 2 #19821

weppos opened this issue Apr 2, 2017 · 3 comments
Labels
FrozenDueToAge
Milestone
Unreleased

Comments

@weppos
Copy link

weppos commented Apr 2, 2017

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

go version go1.7.3 darwin/amd64

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/weppos/go"
GORACE=""
GOROOT="/usr/local/Cellar/go/1.7.3/libexec"
GOTOOLDIR="/usr/local/Cellar/go/1.7.3/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/rk/llwxtd9s6jn91p2ky13tr30c0000gn/T/go-build024013537=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

This issue is a follow up to #18567. The changes just committed in golang/net@78ebe5c introduces again a compatibility error. I first discovered it thanks to the test failures in this PR: weppos/publicsuffix-go#64

The issue is that a leading dot is stripped in the input. Here's a code to reproduce it:

package main

import (
	"fmt"
	"golang.org/x/net/idna"
)

func main() {
	var input, output string
	var err error
	input = ".example.com"
	output, err = idna.ToASCII(input)
	fmt.Println("in:\t", input)
	fmt.Println("out:\t", output)
	fmt.Println("err:\t", err)
	input = "..example.com"
	output, err = idna.ToASCII(input)
	fmt.Println("in:\t", input)
	fmt.Println("out:\t", output)
	fmt.Println("err:\t", err)
}

WIth previous version:

➜  gco f2499483f923065a842d38eb4c7f1927e6fc6e6d
➜  gotest go run idna.go
in:	 .example.com
out:	 .example.com
err:	 <nil>
in:	 ..example.com
out:	 ..example.com
err:	 <nil>

With latest version:

➜  gco HEAD
➜  gotest go run idna.go
in:	 .example.com
out:	 example.com
err:	 <nil>
in:	 ..example.com
out:	 example.com
err:	 <nil>

In order to restore the previous functionality, I had to apply a terrible workaround: weppos/publicsuffix-go#66
I'd appreciate suggestions on how to handle this case in a better way, if this bugfix is not going to land in net/idna.

What did you expect to see?

I'd expect the following string conversion:

idna.ToASCII(".example.com") == ".example.com"
idna.ToASCII("..example.com") == "..example.com"

What did you see instead?

idna.ToASCII(".example.com") == "example.com"
idna.ToASCII("..example.com") == "example.com"

/cc @mpvl @nigeltao
@bradfitz bradfitz added this to the Unreleased milestone Apr 2, 2017
@weppos weppos mentioned this issue Apr 3, 2017
Merged
weppos added a commit to weppos/publicsuffix-go that referenced this issue Apr 3, 2017
@weppos
Workaround for recent changes in idna package (#66)
2fa34d4 
See golang/go#19821 and failures in #64
@mpvl
Copy link
Contributor

mpvl commented May 30, 2017

This behavior was added to pass the test set for IDNA 2008 provided by the Unicode consortium. The same behavior is displayed here: http://www.unicode.org/cldr/utility/idna.jsp?a=..example.com.
It is also consistent with the behavior of Chrome.

However, one could argue that dots should not be removed and/or that having leading dots is an error. Safari has this behavior.

I suggest making an option for this which is enabled in the Lookup profile, but disabled everywhere else. This would make the Punycode profile (used by idna.ToASCII) compatibly with the old behavior.

@gopherbot
Copy link

CL https://golang.org/cl/44380 mentions this issue.

@gopherbot
Copy link

CL https://golang.org/cl/44381 mentions this issue.

gopherbot pushed a commit to golang/text that referenced this issue Jun 27, 2017
@mpvl
internal/export/idna: don't remove leading dots
5a2c30c 
Dots are still removed for profiles for which this makes sense.
Update golang/go#19821

Change-Id: I3de20bc5ddd943557831d1de998554105d7c07e7
Reviewed-on: https://go-review.googlesource.com/44380
Run-TryBot: Marcel van Lohuizen <mpvl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Nigel Tao <nigeltao@golang.org>
@golang golang locked and limited conversation to collaborators Jun 27, 2018
@rsc rsc unassigned mpvl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@bradfitz @weppos @mpvl @gopherbot