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/mail: ParseAddress() accepts valid RFC 5322 addresses with domain part starting with a dash which are invalid RFC 1035 addresses, should we tighten the permissive validation? #39488

Open
bmassemin opened this issue Jun 9, 2020 · 5 comments
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@bmassemin
Copy link

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

$ go version
go version go1.13 darwin/amd64
go go1.14.3 playground

Does this issue reproduce with the latest release?

Yes: https://play.golang.org/p/Z3a5zj6Qch6

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/bmassemin/Library/Caches/go-build"
GOENV="/Users/bmassemin/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/bmassemin"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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/tn/b45tkwwd0dg36r6yk1bzvzv40000gn/T/go-build066110340=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/Z3a5zj6Qch6

package main

import (
	"net/mail"
	"testing"

	"github.com/stretchr/testify/assert"
)

func Test(t *testing.T) {
	_, err := mail.ParseAddress("test@--domain.com")
	assert.Error(t, err)
	_, err = mail.ParseAddress("test@domain--.com")
	assert.Error(t, err)
}

What did you expect to see?

I'm expecting the test to pass, since test@--domain.com is not a valid address.
A domain can't starts or ends with a dash according to this RFC

What did you see instead?

The email address is properly parsed with the invalid domain.

@odeke-em odeke-em changed the title net/mail: mail.ParseAddress() accepts address with domain part starting with a dash net/mail: ParseAddress() accepts address with domain part starting with a dash Jun 9, 2020
@odeke-em
Copy link
Member

odeke-em commented Jun 9, 2020

Thank you for reporting this @bmassemin with detail, and welcome to the Go project!

Those addresses are valid according to the specification that net/mail follows aka RFC 5322, which is loose of a specification that what you are referencing in RFC 1035 and correct according to RFC 5322's syntax in which it is accepted as a dot-atom per atom
Screen Shot 2020-06-09 at 3 44 28 PM

Screen Shot 2020-06-09 at 3 41 36 PM

However, in that RFC 5322 that net/mail uses, they say that enforcement of stricter address validations are left to the discretion of the implementers per https://tools.ietf.org/html/rfc5322#section-3.4.1
Screen Shot 2020-06-09 at 3 43 49 PM

This bug will then perhaps a discussion of whether we should tighten address validation to dip into the requests of RFC 1035 et al, or whether we should leave it alone. I shall retitle this bug.

Kindly /cc-ing @iwdgo @dsymonds @ianlancetaylor

@odeke-em odeke-em changed the title net/mail: ParseAddress() accepts address with domain part starting with a dash net/mail: ParseAddress() accepts valid RFC 5322 addresses with domain part starting with a dash which are invalid RFC 1035 addresses, should we tighten the permissive validation? Jun 9, 2020
@odeke-em odeke-em added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jun 9, 2020
@dsymonds
Copy link
Contributor

dsymonds commented Jun 9, 2020

Yes, net/mail deliberately sticks closely to the spec, and, if memory serves, actually accepts slightly more. People have very weird old mail boxes that they like to be able to parse, so being a bit more liberal in the parsing was a deliberate choice.

I'd say leave net/mail as it is.

@beoran
Copy link

beoran commented Jun 10, 2020

I agree this can stay as is, however I would still add a note in the comments of ParseAddress to explain which RFC is implemented.

@dsymonds
Copy link
Contributor

I agree this can stay as is, however I would still add a note in the comments of ParseAddress to explain which RFC is implemented.

It already does that in the package doc comment.

@gopherbot
Copy link

Change https://golang.org/cl/238118 mentions this issue: net/mail: declare that domain parsing is less strict than expected

@seankhliao seankhliao added this to the Unplanned milestone Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants